From 9c1fa121595dfa0c381999c07758de7b1ed22c31 Mon Sep 17 00:00:00 2001 From: grypez <143971198+grypez@users.noreply.github.com> Date: Mon, 10 Aug 2026 11:51:25 -0400 Subject: [PATCH 1/5] test(kernel-store): pin the failed COMMIT that wedges `_inTx` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Failing repro, not a fix. ## The issue `rollbackIfNeeded` was corrected in #1012 to clear `_inTx` *before* stepping the abort, because the abort can throw and `_inTx` is tracked in the driver rather than read from SQLite. `commitIfNeeded` has the identical shape and was left alone: function commitIfNeeded(): void { if (db._inTx && db._spStack.length === 0) { sqlCommitTransaction.step(); // can throw sqlCommitTransaction.reset(); db._inTx = false; // ...so this never runs } } A COMMIT that throws leaves `_inTx` true against a database that may hold no transaction. `beginIfNeeded` is then a no-op forever after, so the next `createSavepoint` issues its SAVEPOINT outside a transaction — and a savepoint taken outside a transaction commits when it is released (Agoric/agoric-sdk#8423). That is the hazard the whole `beginIfNeeded` dance exists to prevent, and `commitIfNeeded` is reached from `releaseSavepoint`, which is the crank's commit point. The writes that leak are a whole crank's. The nodejs driver is unaffected, for the same reason it was unaffected by the abort case: it reads `db.inTransaction` live from SQLite. Worth noting that the comment introduced above `stops believing it is in a transaction when the abort fails too` asserts that a failed abort is "the one case that can leave `_inTx` disagreeing with the database". This is the second case, so that comment needs correcting along with the code. ## What we hope to see instead `releaseSavepoint` still throws the COMMIT failure, but `_inTx` is false afterwards, so the next `createSavepoint` opens a transaction of its own instead of creating a bare savepoint. Same two-line reorder as `rollbackIfNeeded`, and the "one case" comment updated. ## Current failure AssertionError: expected true to be false packages/kernel-store/src/sqlite/wasm.test.ts > stops believing it is in a transaction when the commit fails Co-Authored-By: Claude Opus 5 --- packages/kernel-store/src/sqlite/wasm.test.ts | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/packages/kernel-store/src/sqlite/wasm.test.ts b/packages/kernel-store/src/sqlite/wasm.test.ts index 8e82a00bc..fa4e2c8a8 100644 --- a/packages/kernel-store/src/sqlite/wasm.test.ts +++ b/packages/kernel-store/src/sqlite/wasm.test.ts @@ -599,6 +599,44 @@ describe('makeSQLKernelDatabase', () => { expect(mockDb.exec).toHaveBeenCalledWith('SAVEPOINT next'); }); + // FAILING REPRO — see the commit message for this test. + // + // `commitIfNeeded` still steps the COMMIT before clearing `_inTx`, the exact + // ordering `rollbackIfNeeded` was corrected to avoid. A COMMIT that throws + // therefore leaves `_inTx` true against a database that may hold no + // transaction, `beginIfNeeded` is a no-op forever after, and the next + // savepoint is created outside a transaction — which commits when released + // (Agoric/agoric-sdk#8423). This is the crank's commit point, so the writes + // that leak are a whole crank's. + // + // The comment above `stops believing it is in a transaction when the abort + // fails too` calls a failed abort "the one case that can leave `_inTx` + // disagreeing with the database". This is the second case. + it('stops believing it is in a transaction when the commit fails', async () => { + const db = await makeSQLKernelDatabase({}); + mockDb._inTx = true; + mockDb._spStack = ['point1']; + // The RELEASE goes through `exec` and succeeds; COMMIT is the first + // prepared statement this path steps, and it is what fails. + mockStatement.step.mockImplementationOnce(() => { + throw new Error('disk I/O error'); + }); + + expect(() => db.releaseSavepoint('point1')).toThrowError( + 'disk I/O error', + ); + + expect(mockDb._inTx).toBe(false); + + // And so the next savepoint gets a transaction of its own rather than + // being created bare. + mockDb.exec.mockClear(); + mockStatement.step.mockClear(); + db.createSavepoint('next'); + expect(mockStatement.step).toHaveBeenCalledOnce(); + expect(mockDb.exec).toHaveBeenCalledWith('SAVEPOINT next'); + }); + it('supports nested savepoints', async () => { const db = await makeSQLKernelDatabase({}); db.createSavepoint('outer'); From e8e70d9337b2ce1ed9dd9434e896547a8c9990bf Mon Sep 17 00:00:00 2001 From: grypez <143971198+grypez@users.noreply.github.com> Date: Mon, 10 Aug 2026 11:52:51 -0400 Subject: [PATCH 2/5] test(ocap-kernel): pin the endCrank failure that buries the real error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Failing repro, not a fix. ## The issue #1012 fixes one error-masking path at the start of a dying crank and opens another at its end. Before the two-savepoint scheme, `rollbackCrank('start')` emptied `ctx.savepoints`, so `endCrank` -> `releaseAllSavepoints` was a guaranteed no-op on the dying path: nothing to release, nothing that could throw. Now `rollbackCrank('delivery')` truncates to the ordinal and leaves `['crank']` behind (crank.ts:56, deliberately — that is what keeps the transaction open for the work an aborted crank still owes). So `endCrank` issues a real `RELEASE t0`, which commits, which can fail. `#runLoop` calls it from a bare `finally`: } finally { this.#kernelStore.endCrank(); ... } A throw there replaces the pending exception. The disk error that actually killed the kernel is discarded — not demoted to `cause`, discarded — and `run()` rejects with the release failure instead. `#failRunLoop` records that, so `getRunLoopStatus().detail` loses the root cause too, and `onRunLoopFailure` — what the daemon logs as fatal — gets the wrong error. A/B against origin/main with the same repro: main reports `crank exploded`, this branch reports `database is gone` with `cause: undefined`. This is the same class of bug as the `No such savepoint: t0` masking that 82b88ce62 fixes, and the same class the `reports both failures when the rollback also fails` test above already guards on the other path. ## What we hope to see instead Whatever names the release failure, the error that killed the crank stays reachable. The rollback path already has the shape to copy: throw new Error( `Run loop died and its crank could not be rolled back: ${...}`, { cause: error }, ); The assertion is deliberately fix-agnostic — it walks the `cause` chain — so either wrapping `endCrank`'s failure with the original as `cause`, or reporting it and rethrowing the original, will satisfy it. ## Current failure AssertionError: expected [ Error: database is gone ] to include Error: crank exploded packages/ocap-kernel/src/KernelQueue.test.ts > reports both failures when endCrank also fails Co-Authored-By: Claude Opus 5 --- packages/ocap-kernel/src/KernelQueue.test.ts | 55 ++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/packages/ocap-kernel/src/KernelQueue.test.ts b/packages/ocap-kernel/src/KernelQueue.test.ts index 1af58d81f..90c74f202 100644 --- a/packages/ocap-kernel/src/KernelQueue.test.ts +++ b/packages/ocap-kernel/src/KernelQueue.test.ts @@ -25,6 +25,24 @@ vi.mock('@endo/promise-kit', () => ({ */ const STOP_RUN_LOOP = 'test: stop run loop'; +/** + * Collect an error and every error reachable through its `cause` chain, so that + * a test can assert a root cause survived without pinning how its reporter + * chose to wrap it. + * + * @param error - The error to walk. + * @returns The chain, outermost first. + */ +const causeChain = (error: unknown): unknown[] => { + const chain: unknown[] = []; + let current = error; + while (current instanceof Error) { + chain.push(current); + current = current.cause; + } + return chain; +}; + describe('KernelQueue', () => { let kernelStore: KernelStore; let kernelQueue: KernelQueue; @@ -539,6 +557,43 @@ describe('KernelQueue', () => { }); }); + // FAILING REPRO — see the commit message for this test. + // + // The companion of the case above, at the other end of the crank. Since the + // delivery rollback now spares `crank`, `endCrank`'s release is a real + // RELEASE + COMMIT on the dying path where it used to be a no-op, and + // `#runLoop` calls it from a bare `finally` — so when it throws it replaces + // the error that killed the kernel instead of being reported alongside it. + it('reports both failures when endCrank also fails', async () => { + ( + kernelStore.runQueueLength as unknown as MockInstance + ).mockReturnValueOnce(1); + (kernelStore.dequeueRun as unknown as MockInstance).mockReturnValueOnce({ + type: 'send', + target: 'ko123', + message: {} as KernelMessage, + }); + (kernelStore.endCrank as unknown as MockInstance).mockImplementation( + () => { + throw new Error('database is gone'); + }, + ); + const crankError = new Error('crank exploded'); + const deliver = vi.fn().mockRejectedValue(crankError); + + const failure = await kernelQueue + .run(deliver) + .catch((error: unknown) => error); + + // However the release failure is named, the error that actually killed the + // kernel has to stay reachable — as the rollback path already manages. + expect(causeChain(failure)).toContain(crankError); + expect(kernelQueue.getRunLoopStatus()).toMatchObject({ + state: 'failed', + detail: expect.stringContaining('crank exploded'), + }); + }); + // `rollbackCrank` discards the savepoint even when its database call throws, // so a second attempt could only report a missing savepoint. Without the // `finally` that records the attempt, the abort path leaves the flag unset, From a8fc301da7ce4599b7c6df69a269d60c8f98a611 Mon Sep 17 00:00:00 2001 From: grypez <143971198+grypez@users.noreply.github.com> Date: Mon, 10 Aug 2026 11:53:48 -0400 Subject: [PATCH 3/5] test(kernel-node-runtime): pin the kernel store's missing logger MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Failing repro, not a fix. ## The issue #1012 replaces four silently-swallowed aborts with `logger?.error(...)` in the SQLite drivers, and its description says: "Four swallowed aborts were silent. Now logged." They are not. No production call site passes a `logger` to `makeSQLKernelDatabase`, so every one of those calls is dead code: packages/kernel-node-runtime/src/kernel/make-kernel.ts:63 packages/kernel-browser-runtime/src/kernel-worker/kernel-worker.ts:47 packages/kernel-test-local/src/lms-chat.ts:30 packages/kernel-node-runtime/test/helpers/remote-comms.ts:172 `make-kernel.ts` is the clearest case: it builds a `rootLogger` and hands sub-loggers to `NodejsPlatformServices` and to `Kernel.make`, then constructs the store with `{ dbFilename }` alone. The store is the one collaborator that gets no logger. Nor does any test pass one, which is why the gap survived review. This matters more than a missing log line. On the nodejs driver a failed abort leaves `db.inTransaction` true with nothing that will ever commit or abort it, so later writes on that connection join a transaction that vanishes on close. The driver's own comment concedes "Nothing here can repair that" — the log is the entire remedy, and it does not reach anyone. `logger?.error` is the right convention for this package; the injection is what is missing. ## What we hope to see instead `makeKernel` passes a tagged sub-logger to `makeSQLKernelDatabase`, as it already does for its other collaborators — something like `rootLogger.subLogger({ tags: ['store'] })`. The other three call sites want the same treatment, and are worth covering once this one is fixed. ## Current failure AssertionError: expected "vi.fn()" to be called with arguments: [ ObjectContaining{…} ] - "logger": Any, packages/kernel-node-runtime/src/kernel/make-kernel.test.ts > gives the kernel store a logger Co-Authored-By: Claude Opus 5 --- .../src/kernel/make-kernel.test.ts | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/packages/kernel-node-runtime/src/kernel/make-kernel.test.ts b/packages/kernel-node-runtime/src/kernel/make-kernel.test.ts index 57b0293d6..98490dd65 100644 --- a/packages/kernel-node-runtime/src/kernel/make-kernel.test.ts +++ b/packages/kernel-node-runtime/src/kernel/make-kernel.test.ts @@ -1,3 +1,5 @@ +import { makeSQLKernelDatabase } from '@metamask/kernel-store/sqlite/nodejs'; +import { Logger } from '@metamask/logger'; import { Kernel } from '@metamask/ocap-kernel'; import { describe, expect, it, vi } from 'vitest'; @@ -8,7 +10,9 @@ vi.mock('@metamask/kernel-store/sqlite/nodejs', async () => { '../../../ocap-kernel/test/storage.ts' ); return { - makeSQLKernelDatabase: makeMapKernelDatabase, + // Wrapped so that a test can see what the database was constructed with, + // while still getting a real store back. + makeSQLKernelDatabase: vi.fn(makeMapKernelDatabase), }; }); @@ -18,4 +22,18 @@ describe('makeKernel', () => { expect(kernel).toBeInstanceOf(Kernel); }); + + // FAILING REPRO — see the commit message for this test. + // + // The kernel store is the only collaborator `makeKernel` builds without + // handing it a logger, so every `logger?.` call inside the SQLite driver is + // dead code in production — including the four abort failures #1012 added + // logging for. + it('gives the kernel store a logger', async () => { + await makeKernel({}); + + expect(makeSQLKernelDatabase).toHaveBeenCalledWith( + expect.objectContaining({ logger: expect.any(Logger) }), + ); + }); }); From f0e63f31cbcab26c582137e7dc45caf9b1c0ee19 Mon Sep 17 00:00:00 2001 From: grypez <143971198+grypez@users.noreply.github.com> Date: Mon, 10 Aug 2026 11:56:04 -0400 Subject: [PATCH 4/5] test(ocap-kernel): pin the release failure lost at the remote savepoint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Failing repro, not a fix. ## The issue #1012 hardens `releaseSavepoint` so that a failed `RELEASE` discards the enclosing transaction, clearing the driver's `_spStack` on the way. Two callers it does not touch depend on the old behaviour, and both are now worse off than before the change. `RemoteHandle.handleRemoteMessage` releases inside the `try` and rolls back in the `catch`: this.#kernelStore.setRemoteHighestReceivedSeq(this.remoteId, seq); this.#kernelStore.releaseSavepoint(savepointName); // fails } catch (error) { this.#kernelStore.rollbackSavepoint(savepointName); // "No such savepoint" throw error; // never reached } Since the release already cleared the stack, the rollback throws `No such savepoint: receive_r0_1`, which escapes the `catch` and replaces the real failure. Not demoted to `cause` — replaced. `RemoteManager` has the same shape at its `peerIncarnation_*` savepoint. A/B verified against origin/main with a real driver: main's rollback succeeds and `database or disk is full` propagates; on this branch the caller gets the missing-savepoint error instead. So the PR description's "the release failure still propagates" holds for the crank path it fixed and not for these two. `crank.ts:57-63` shows the author recognised exactly this hazard — a stale savepoint list producing `No such savepoint` over the real error — and fixed it for the crank only. The remote paths were missed because nothing exercised them. Note the secondary effect these tests don't reach: `ctx.savepoints` still lists the crank's own savepoints after this, so the next `endCrank` throws `No such savepoint: t0` over whatever is left of the failure. ## What we hope to see instead The failure the database reported is what reaches the caller. Any of these does it, and the assertion doesn't care which: - move the release out of the `try`, so a release failure isn't followed by a rollback attempt at all - have the `catch` tolerate a rollback that reports a savepoint already discarded, rethrowing the original either way - make the driver's discard leave the name rollback-able as a no-op The mock models the drivers' bookkeeping rather than the expected outcome, so it is `RemoteHandle`'s error handling under test, not the mock's. ## Current failure AssertionError: expected Error: No such savepoint: receive_r0_1 to be Error: database or disk is full packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts > reports the release failure rather than a missing savepoint Co-Authored-By: Claude Opus 5 --- .../src/remotes/kernel/RemoteHandle.test.ts | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts b/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts index c14539bc6..031c6466d 100644 --- a/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts +++ b/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts @@ -220,6 +220,49 @@ describe('RemoteHandle', () => { }); }); + // FAILING REPRO — see the commit message for this test. + // + // `handleRemoteMessage` releases its savepoint inside the `try` and rolls + // back in the `catch`. #1012 made a failed `RELEASE` discard the whole + // savepoint stack, so that rollback now reports a savepoint that no longer + // exists, and it throws out of the `catch` in place of the failure that + // brought it there. + it('reports the release failure rather than a missing savepoint', async () => { + // The drivers' bookkeeping as #1012 leaves it, verified against both: a + // failed RELEASE clears `_spStack`, and rolling back a name that is not on + // it throws `No such savepoint`. + const savepoints: string[] = []; + const releaseFailure = new Error('database or disk is full'); + mockKernelStore = { + ...mockKernelStore, + createSavepoint: (name: string) => { + savepoints.push(name); + }, + releaseSavepoint: () => { + savepoints.length = 0; + throw releaseFailure; + }, + rollbackSavepoint: (name: string) => { + if (!savepoints.includes(name)) { + throw new Error(`No such savepoint: ${name}`); + } + }, + } as KernelStore; + const remote = makeRemote(); + + const delivery = JSON.stringify({ + seq: 1, + method: 'deliver', + params: ['bringOutYourDead'], + }); + + // The error an operator needs is the one the database gave, not the + // bookkeeping artefact of trying to clean up after it. + await expect(remote.handleRemoteMessage(delivery)).rejects.toBe( + releaseFailure, + ); + }); + // A dead run loop will never deliver the message, and `handleRemoteMessage` // rolls back without advancing the received sequence number, so the peer // retries and gives up rather than being acknowledged by a black hole. From f3af96f2c25d2818432017e3a74289221a9f320c Mon Sep 17 00:00:00 2001 From: grypez <143971198+grypez@users.noreply.github.com> Date: Mon, 10 Aug 2026 12:09:39 -0400 Subject: [PATCH 5/5] test: tighten the four repros after review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No change to what any of them proves; all four still fail for the reasons their own commits describe. - `RemoteHandle`: assert the rollback is still *attempted*. Without this, deleting the rollback from the `catch` outright would turn the test green, which is not the fix — a `RELEASE` that failed for a reason of its own may well have left the savepoint standing. - `RemoteHandle`: drop an unnecessary `as KernelStore` cast, and say why the store is replaced wholesale rather than having its methods assigned over (`makeKernelStore` hardens what it returns). - `make-kernel`: note that `kernel-worker.ts` omits the logger too, so the wasm driver's pair of `logger?.error` calls stays dead even once this test passes. Use `vi.mocked`, as the sibling `make-kernel-options.test.ts` does. - `causeChain` returns `Error[]`; every element is already narrowed by the loop guard. - Drop "see the commit message for this test" from the four comment blocks: each stands alone, and the reference would not survive a squash-merge. Restate the claim the wasm comment made by citing a neighbouring test's title, which would have broken silently on rename. Co-Authored-By: Claude Opus 5 --- .../src/kernel/make-kernel.test.ts | 7 +++--- packages/kernel-store/src/sqlite/wasm.test.ts | 8 +++---- packages/ocap-kernel/src/KernelQueue.test.ts | 6 ++--- .../src/remotes/kernel/RemoteHandle.test.ts | 23 ++++++++++++------- 4 files changed, 26 insertions(+), 18 deletions(-) diff --git a/packages/kernel-node-runtime/src/kernel/make-kernel.test.ts b/packages/kernel-node-runtime/src/kernel/make-kernel.test.ts index 98490dd65..672313a02 100644 --- a/packages/kernel-node-runtime/src/kernel/make-kernel.test.ts +++ b/packages/kernel-node-runtime/src/kernel/make-kernel.test.ts @@ -23,16 +23,17 @@ describe('makeKernel', () => { expect(kernel).toBeInstanceOf(Kernel); }); - // FAILING REPRO — see the commit message for this test. + // FAILING REPRO. // // The kernel store is the only collaborator `makeKernel` builds without // handing it a logger, so every `logger?.` call inside the SQLite driver is // dead code in production — including the four abort failures #1012 added - // logging for. + // logging for. `kernel-worker.ts` omits it too, which keeps the wasm driver's + // pair dead even once this passes. it('gives the kernel store a logger', async () => { await makeKernel({}); - expect(makeSQLKernelDatabase).toHaveBeenCalledWith( + expect(vi.mocked(makeSQLKernelDatabase)).toHaveBeenCalledWith( expect.objectContaining({ logger: expect.any(Logger) }), ); }); diff --git a/packages/kernel-store/src/sqlite/wasm.test.ts b/packages/kernel-store/src/sqlite/wasm.test.ts index fa4e2c8a8..1cf496dc9 100644 --- a/packages/kernel-store/src/sqlite/wasm.test.ts +++ b/packages/kernel-store/src/sqlite/wasm.test.ts @@ -599,7 +599,7 @@ describe('makeSQLKernelDatabase', () => { expect(mockDb.exec).toHaveBeenCalledWith('SAVEPOINT next'); }); - // FAILING REPRO — see the commit message for this test. + // FAILING REPRO. // // `commitIfNeeded` still steps the COMMIT before clearing `_inTx`, the exact // ordering `rollbackIfNeeded` was corrected to avoid. A COMMIT that throws @@ -609,9 +609,9 @@ describe('makeSQLKernelDatabase', () => { // (Agoric/agoric-sdk#8423). This is the crank's commit point, so the writes // that leak are a whole crank's. // - // The comment above `stops believing it is in a transaction when the abort - // fails too` calls a failed abort "the one case that can leave `_inTx` - // disagreeing with the database". This is the second case. + // A failed abort is therefore not, as the abort case above claims, the one + // case that can leave `_inTx` disagreeing with the database. This is the + // second. it('stops believing it is in a transaction when the commit fails', async () => { const db = await makeSQLKernelDatabase({}); mockDb._inTx = true; diff --git a/packages/ocap-kernel/src/KernelQueue.test.ts b/packages/ocap-kernel/src/KernelQueue.test.ts index 90c74f202..9df06624c 100644 --- a/packages/ocap-kernel/src/KernelQueue.test.ts +++ b/packages/ocap-kernel/src/KernelQueue.test.ts @@ -33,8 +33,8 @@ const STOP_RUN_LOOP = 'test: stop run loop'; * @param error - The error to walk. * @returns The chain, outermost first. */ -const causeChain = (error: unknown): unknown[] => { - const chain: unknown[] = []; +const causeChain = (error: unknown): Error[] => { + const chain: Error[] = []; let current = error; while (current instanceof Error) { chain.push(current); @@ -557,7 +557,7 @@ describe('KernelQueue', () => { }); }); - // FAILING REPRO — see the commit message for this test. + // FAILING REPRO. // // The companion of the case above, at the other end of the crank. Since the // delivery rollback now spares `crank`, `endCrank`'s release is a real diff --git a/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts b/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts index 031c6466d..9dc0c9661 100644 --- a/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts +++ b/packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts @@ -220,7 +220,7 @@ describe('RemoteHandle', () => { }); }); - // FAILING REPRO — see the commit message for this test. + // FAILING REPRO. // // `handleRemoteMessage` releases its savepoint inside the `try` and rolls // back in the `catch`. #1012 made a failed `RELEASE` discard the whole @@ -230,9 +230,16 @@ describe('RemoteHandle', () => { it('reports the release failure rather than a missing savepoint', async () => { // The drivers' bookkeeping as #1012 leaves it, verified against both: a // failed RELEASE clears `_spStack`, and rolling back a name that is not on - // it throws `No such savepoint`. + // it throws `No such savepoint`. Replacing the store wholesale rather than + // assigning over its methods because `makeKernelStore` hardens what it + // returns. const savepoints: string[] = []; const releaseFailure = new Error('database or disk is full'); + const rollbackSavepoint = vi.fn((name: string) => { + if (!savepoints.includes(name)) { + throw new Error(`No such savepoint: ${name}`); + } + }); mockKernelStore = { ...mockKernelStore, createSavepoint: (name: string) => { @@ -242,12 +249,8 @@ describe('RemoteHandle', () => { savepoints.length = 0; throw releaseFailure; }, - rollbackSavepoint: (name: string) => { - if (!savepoints.includes(name)) { - throw new Error(`No such savepoint: ${name}`); - } - }, - } as KernelStore; + rollbackSavepoint, + }; const remote = makeRemote(); const delivery = JSON.stringify({ @@ -261,6 +264,10 @@ describe('RemoteHandle', () => { await expect(remote.handleRemoteMessage(delivery)).rejects.toBe( releaseFailure, ); + // Still attempted, so that abandoning the rollback is not a way to pass + // this test: a release that failed for a reason of its own may well have + // left the savepoint standing. + expect(rollbackSavepoint).toHaveBeenCalledWith('receive_r0_1'); }); // A dead run loop will never deliver the message, and `handleRemoteMessage`