fix: keep a crank's store work inside one transaction - #1012
Closed
sirtimid wants to merge 11 commits into
Closed
Conversation
Eight tests, all currently failing, for three defects that landed with #1005. They change no production code: each one states the invariant the fix has to restore, so the diff that repairs them is the specification being met rather than a claim about it. `releaseSavepoint` was never hardened the way `rollbackSavepoint` was in that PR. A RELEASE that throws leaves the savepoint on the stack and the transaction open with nothing that will ever commit or abort it, so every later write on the connection joins it, reports success, and vanishes on close — verbatim the failure mode #1005 documents for the other door. The driver tests sit beside their rollback counterparts so the asymmetry is visible in place. `endCrank` gets the companion case: it now settles its waiters in a `finally`, which is right, but it also leaves the savepoint listed, so the next crank numbers its savepoint `t1` against a database that still has `t0`. `#processCrankResult` does fallible work after the crank's transactional boundary has already been crossed. On the success path `#flushCrankBuffer` settles the promise `enqueueMessage` handed an external caller, and only then can `#terminateVat` throw and have the new catch roll the crank back — so the caller keeps an answer computed from state the store discarded, and a restart delivers the message again. On the abort path the rollback ends the transaction, so `#terminateVat` and `collectGarbage` autocommit piecemeal and the second rollback the flag correctly suppresses would have had nothing left to undo either way. The invariant is stated as "the rollback is the last thing the crank asks of the store", which leaves the choice of remedy open. The wasm driver tracks `_inTx` itself rather than reading it from SQLite, so a failed abort inside the new catch is the one case that can leave it disagreeing with the database. Left true, `beginIfNeeded` is a no-op from then on and the next `createSavepoint` runs in autocommit mode, where the matching RELEASE commits (Agoric/agoric-sdk#8423, already cited two lines above the code) and no rollback can undo the delivery. The second test runs that next `createSavepoint` and asserts the BEGIN, so the corruption path is observable instead of argued. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three transaction-integrity defects, all in the same family: a store call fails, and the layer above goes on as though its bookkeeping still matched the database. - `releaseSavepoint` (both SQLite drivers) discards the enclosing transaction when `RELEASE` fails, as `rollbackSavepoint` already does when `ROLLBACK TO` fails. Left as it was, the savepoint stayed on the stack and the transaction open with nothing to ever commit or abort it, so every later write on the connection joined it, reported success, and vanished on `close()`. - `releaseAllSavepoints` forgets its savepoints even if the release throws, as `rollbackCrank` already does. A savepoint left listed had the next crank number its savepoint `t1` while the database still had `t0`, from which point every release and rollback aimed one crank past the one it meant to end. - The wasm driver stops believing it is in a transaction when an abort fails. `_inTx` is tracked in the driver rather than read from SQLite, and an abort usually fails because SQLite already rolled back on its own. Left true, `beginIfNeeded` was a no-op from then on and the next `createSavepoint` ran in autocommit mode, where its `RELEASE` commits (Agoric/agoric-sdk#8423) and no later rollback could undo the delivery. And the crank boundary itself, in two parts: - A crank now takes two savepoints. Rolling back to the outermost one discards the enclosing transaction, so the work an aborted crank still owes — terminating the vat whose delivery failed, collecting garbage — was autocommitting statement by statement, beyond the reach of any later rollback. That work has to follow the rollback, since the worker is gone and the store must not go on believing the vat is alive, so it is the rollback that spares the transaction. Releasing the outer savepoint in `endCrank` is now a crank's one commit point. - `#flushCrankBuffer` runs last, after everything that can still fail. It settles the promise `enqueueMessage` handed an external caller, reading the result out of the store; rolling the crank back after that left the caller holding an answer computed from state the store had discarded, and a restart would deliver the message again. Tests for the first three defects are Ryan's, from #1011. The two crank tests there specify the remedy as "the rollback is the last thing the crank asks of the store", which reordering the fallible work before it would satisfy — but that rollback would then undo the vat termination. They are restated here as the invariant the fix does hold. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
`should trigger GC syscalls through bringOutYourDead` scheduled one reap and then ran three cranks. `scheduleReap` dedupes, so that bought one `bringOutYourDead`, not three — and an import is only reported as dropped once the engine has collected the vat's presence and run its finalizer, which the forced GC pass inside `bringOutYourDead` cannot guarantee on the first attempt. When it hadn't, no further reap was ever scheduled and the refcount stayed where it was: `expected 2 to be 1`, as on main in 31081630878. Each attempt now schedules its own reap and stops as soon as the kernel's bookkeeping catches up, so the common case is one crank rather than three. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
A failed `ROLLBACK TO` discards the whole transaction, taking every savepoint with it — not just the one rolled back to. `rollbackCrank` truncated `ctx.savepoints` to the rolled-back ordinal regardless, which was correct while a crank took one savepoint at ordinal 0 and cleared the list, but leaves `['crank']` listed now that the delivery sits at ordinal 1. `endCrank` then releases a `t0` the database no longer has, and throws "No such savepoint: t0" from the run loop's `finally` — replacing the failure that actually killed the kernel, with no `cause`. That is the masking this branch's own error-preservation exists to prevent. Clear the list on the throwing path, truncate to the ordinal only on success. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tion Both drivers recover from a failed savepoint operation by discarding the enclosing transaction, and swallow any error from that abort so the savepoint failure stays the one reported. That part is right, but it left the abandoned transaction entirely silent: on the nodejs driver, where `inTransaction` is read from SQLite, the next crank's `beginIfNeeded` sees the transaction still open, skips its `BEGIN`, and commits the dead crank's writes alongside the new crank's. Nothing here can repair that, so at least record it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Moving `#invokeKernelSubscription` out of the enqueue loop and after it was the one production change on this branch with no test: reverting `#flushCrankBuffer` to its interleaved form left all 2412 ocap-kernel tests passing. Same hazard as the crank-level ordering a few tests up, one level down — `#enqueueRun` is store work and can fail part-way, so answering the first caller while the second enqueue is still ahead hands out a result the crank's rollback then discards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five comments on this branch asserted more than the code holds: - `wasm.ts` claimed a stale `_inTx` meant "no later rollback can undo the delivery". False: a savepoint created in autocommit mode does open a transaction, and an inner savepoint still rolls back. The real cost is that writes outside a savepoint autocommit one statement at a time, and the outermost `RELEASE` commits. The "an abort typically fails because SQLite already rolled back" premise was unsupported and isn't the reason for the reorder — the reason is simply that the abort can throw. - `#processCrankResult` said "the worker is already gone" ahead of the call that kills the worker. - The flush was described as running "once nothing fallible remains". It doesn't: `#terminateVat` resolves the dying vat's promises through `resolvePromises`, which defaults to `immediate` and invokes their kernel subscriptions before `collectGarbage`. Reachable without an abort, via a clean `exitVat`. Recorded rather than fixed — closing it changes termination semantics, not crank ordering. - "Only `delivery` is ever rolled back" is true of the run loop but not of the tests. Scoped, and the ordinal coupling it depends on is now stated: `endCrank` releases `t0` by position, so `crank` must stay first. - `reapImporterUntil` credited `scheduleReap` deduping for the old one-BOYD behaviour; it was `nextReapAction` shifting the single entry off, leaving the later cranks nothing to do. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 6, 2026
Comment the non-obvious why, in the shortest form that carries it. The two-savepoint rationale was re-argued in full in four places; the tests now point at `#runLoop` and `#processCrankResult` instead of restating them, and the hazard block duplicated across both driver test files is a line. No reasoning removed, only the retelling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This was referenced Aug 12, 2026
Member
Author
|
Superseded by #1021, which carries all of this branch's work plus the fixes for #1018's four repros. Why it merged rather than landing standalone: three PRs were editing the same A second incompatibility surfaced while assembling it, and is the sharper one: #1020's refcount audit and this branch's flush reorder are silently incompatible. The audit reads the run queue as ground truth, but a buffered item's refcounts are incremented at enqueue time, so auditing before the flush reports every buffered item as a leak. Had these landed separately that would have been a live bug on All of this branch's commits are in #1021 with authorship intact. Follow-ups #1013 and #1014 still stand. |
sirtimid
pushed a commit
that referenced
this pull request
Aug 17, 2026
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 <noreply@anthropic.com>
sirtimid
pushed a commit
that referenced
this pull request
Aug 17, 2026
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 82b88ce 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 <noreply@anthropic.com>
sirtimid
pushed a commit
that referenced
this pull request
Aug 17, 2026
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<Logger>, packages/kernel-node-runtime/src/kernel/make-kernel.test.ts > gives the kernel store a logger Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
sirtimid
pushed a commit
that referenced
this pull request
Aug 17, 2026
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation
Fixes the three defects #1011 pinned as failing tests, and carries those tests along. All eight are green here.
1.
releaseSavepointis hardened the wayrollbackSavepointwasA
RELEASEthat throws left the savepoint on the stack and the transaction open with nothing that would ever commit or abort it, so every later write on the connection joined it, reported success, and vanished onclose(). Both drivers now discard the transaction; the release failure still propagates.releaseAllSavepointsgets the companion case, so the next crank can't number its savepointt1against a database that has not0.2. A crank's store work stays inside one transaction
A crank takes two savepoints,
crankanddelivery, and the run loop rolls back onlydelivery. Rolling back the outermost one ends the transaction, so the work an aborted crank still owes — terminating the vat, collecting garbage — was autocommitting a statement at a time.Buffered outputs are flushed after that work rather than before it, so a later failure can no longer roll the crank back underneath an answer already given to a
queueMessagecaller.3. The wasm driver can no longer believe it is in a transaction it isn't
_inTxis cleared before the abort is attempted, because the abort can throw. Left true,beginIfNeededbecame a permanent no-op; cleared, a still-open transaction surfaces as a failedBEGIN. The nodejs driver readsdb.inTransactionand was never affected.Review findings, fixed here
Six review agents went over the branch; four findings were worth acting on.
The two-savepoint scheme reopened the error-masking bug this branch fixes elsewhere.
rollbackCranktruncatedctx.savepointsto the rolled-back ordinal even when the rollback threw — harmless at ordinal 0, but the delivery now sits at ordinal 1, so a failed rollback left['crank']listed against a database that had discarded every savepoint.endCrankthen threwNo such savepoint: t0from the run loop'sfinally, replacing the disk error that actually killed the kernel. Reproduced against the realgetCrankMethodsbefore fixing.The
#flushCrankBufferreorder had no test — reverting it left all 2412 ocap-kernel tests passing. Now pinned.Four swallowed aborts were silent. Now logged. On nodejs this matters:
beginIfNeededreadsinTransactionfrom SQLite, so the next crank would skip itsBEGINand commit the dead crank's writes alongside its own.Five comments claimed more than the code holds, detailed in the
docs:commit. The one worth calling out: the flush is not "last, once nothing fallible remains" —#terminateVatresolves the dying vat's promises throughresolvePromises, which defaults toimmediateand invokes their subscriptions beforecollectGarbage, reachable via a cleanexitVat. Narrower than before this branch but not closed, so it's documented at the site. Closing it changes termination semantics, which is wider than this PR.Follow-ups filed
createCrankSavepoint's name parameter can't express "crank outer, delivery inner", the invariant that fails silentlyTesting
yarn lintclean,yarn build30/30. kernel-store and ocap-kernel (2412) all pass; kernel-test 102 pass, 3 todo.Both fixes with new tests were mutation-verified: revert the production hunk and the test fails for the stated reason.
Checklist
🤖 Generated with Claude Code
Note
High Risk
Changes core crank transaction boundaries, savepoint rollback/release failure paths, and when external callers see message results—bugs could corrupt kernel store state or mis-settle promises.
Overview
Keeps each crank’s SQLite work in a single transaction by using nested savepoints
crankanddelivery: aborted deliveries roll back onlydelivery, so post-rollback work (vat termination, GC) stays in the same transaction instead of autocommitting statement-by-statement. The run loop moves buffered vat output flush to after that fallible work and only invokesqueueMessagesubscriptions once everyenqueueRunsucceeds, so callers are not answered from state that a later rollback would discard.kernel-store mirrors
rollbackSavepointfor failedRELEASE(discard the transaction, still throw the release error), logs failed aborts during recovery, and on the wasm driver clears_inTxbefore attempting abort so a failed abort cannot leave the driver thinking it is still in a transaction.Crank bookkeeping clears all in-memory savepoint names when a rollback or
endCrankrelease fails (avoiding spuriousNo such savepoint: t0over the real DB error), androllbackCrankclears the full savepoint list when the DB rollback throws under the two-savepoint scheme.Reviewed by Cursor Bugbot for commit 391f9f8. Bugbot is set up for automated code reviews on this repo. Configure here.