Skip to content

Commit 997c10d

Browse files
grypezclaude
authored andcommitted
test(ocap-kernel): pin the release failure lost at the remote savepoint
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 <noreply@anthropic.com>
1 parent ad07e8a commit 997c10d

1 file changed

Lines changed: 43 additions & 0 deletions

File tree

packages/ocap-kernel/src/remotes/kernel/RemoteHandle.test.ts

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -220,6 +220,49 @@ describe('RemoteHandle', () => {
220220
});
221221
});
222222

223+
// FAILING REPRO — see the commit message for this test.
224+
//
225+
// `handleRemoteMessage` releases its savepoint inside the `try` and rolls
226+
// back in the `catch`. #1012 made a failed `RELEASE` discard the whole
227+
// savepoint stack, so that rollback now reports a savepoint that no longer
228+
// exists, and it throws out of the `catch` in place of the failure that
229+
// brought it there.
230+
it('reports the release failure rather than a missing savepoint', async () => {
231+
// The drivers' bookkeeping as #1012 leaves it, verified against both: a
232+
// failed RELEASE clears `_spStack`, and rolling back a name that is not on
233+
// it throws `No such savepoint`.
234+
const savepoints: string[] = [];
235+
const releaseFailure = new Error('database or disk is full');
236+
mockKernelStore = {
237+
...mockKernelStore,
238+
createSavepoint: (name: string) => {
239+
savepoints.push(name);
240+
},
241+
releaseSavepoint: () => {
242+
savepoints.length = 0;
243+
throw releaseFailure;
244+
},
245+
rollbackSavepoint: (name: string) => {
246+
if (!savepoints.includes(name)) {
247+
throw new Error(`No such savepoint: ${name}`);
248+
}
249+
},
250+
} as KernelStore;
251+
const remote = makeRemote();
252+
253+
const delivery = JSON.stringify({
254+
seq: 1,
255+
method: 'deliver',
256+
params: ['bringOutYourDead'],
257+
});
258+
259+
// The error an operator needs is the one the database gave, not the
260+
// bookkeeping artefact of trying to clean up after it.
261+
await expect(remote.handleRemoteMessage(delivery)).rejects.toBe(
262+
releaseFailure,
263+
);
264+
});
265+
223266
// A dead run loop will never deliver the message, and `handleRemoteMessage`
224267
// rolls back without advancing the received sequence number, so the peer
225268
// retries and gives up rather than being acknowledged by a black hole.

0 commit comments

Comments
 (0)