fix: keep a crank's store work inside one transaction - #1021
Open
sirtimid wants to merge 36 commits into
Open
Conversation
This was referenced Aug 13, 2026
Contributor
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
sirtimid
force-pushed
the
sirtimid/crank-rollback-integrity
branch
from
August 17, 2026 10:08
3311d5c to
0892784
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
2 tasks
sirtimid
force-pushed
the
sirtimid/crank-rollback-integrity
branch
from
August 20, 2026 13:59
0892784 to
65bd467
Compare
sirtimid
force-pushed
the
sirtimid/crank-rollback-integrity
branch
from
August 27, 2026 11:27
913524a to
05b273d
Compare
4 tasks
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>
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>
…lback A database rollback cannot reach two pieces of state, so `rollbackCrank` now reverts both itself. Every `provideCachedStoredValue` answers reads from a closure and only writes through to kv. Reverting the database therefore left the closure holding the abandoned crank's value, and the next `set` persisted it. `processGCActionSet` takes an action out of the set before delivering it, so an aborted delivery lost the action outright rather than retrying it. `reapQueue` was exposed the same way. `maybeFreeKrefs` lives in RAM, so nothing reverted it either. Its entries are collection candidates only because of the decrements the rollback undid, and a later `collectGarbage` threw outright on a promise the rollback had deleted, killing the run loop. No live bug either way: every `abort` `#deliverGCAction` returns is paired with a `terminate`, which is what made losing the action harmless. The comment there claimed the rollback restored the action, which is the thing a future reader would trust when adding an abort path that isn't paired with a termination; it now states the real causality. The cached values are declared once so that initialization and the refresher cannot disagree about which ones exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
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 <noreply@anthropic.com>
`rollbackIfNeeded` was corrected for this ordering already; `commitIfNeeded` still stepped the COMMIT first. `_inTx` is tracked in the driver rather than read from SQLite, so a throwing COMMIT wedged it true: `beginIfNeeded` became a permanent no-op, and the next savepoint was created bare — where its RELEASE commits (Agoric/agoric-sdk#8423) and no later rollback could undo the delivery. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…un loop Now that the delivery rollback spares the `crank` savepoint, `endCrank`'s release is a real RELEASE and COMMIT on the dying path, where it used to be a no-op. `#runLoop` called it from a bare `finally`, so a failing one silently replaced whatever killed the kernel — and only `error.message` crosses the wire, so the real failure reached neither `getStatus` nor the daemon log. Report it with the crank's failure as the `cause`, the shape the rollback path already uses. The in-flight error is boxed rather than left `undefined`, so a crank that threw `undefined` stays distinguishable from one that did not throw. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…vepoint `RemoteHandle.handleRemoteMessage` releases its savepoint inside the `try` and rolls back in the `catch`. Now that a failed RELEASE discards the whole savepoint stack, that rollback names a savepoint that is already gone and threw `No such savepoint` out of the `catch` in place of the database failure that brought it there — not even as `cause`. Log the rollback failure instead of throwing it. The rollback is still attempted, because a release that failed for a reason of its own may well have left the savepoint standing. `RemoteManager`'s `peerIncarnation_*` savepoint has the identical shape and had no coverage of it at all, so a fix applied here and forgotten there would have left its suite green. Fixed alike, and the savepoint-stack model both tests drive the drivers with is now shared. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…llback `rollbackCrank` gained two pieces of work that compose the wrong way round if the failure path simply rethrows: forgetting every savepoint, and reverting the caches a database rollback cannot reach. A failed rollback discards the whole transaction, so the database has moved back at least as far as a successful rollback would have taken it and those caches are at least as stale — the one case where skipping the revert leaves the consumed GC action lost and krefs queued for a collection that then kills the run loop. The second test pins the other direction: reverting must not become a way to lose the database error either. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
No production call site passed one, so every `logger?.` call in the SQLite drivers was dead code — including the aborts they report while discarding a transaction, which fire on exactly the path where the kernel is already dying and a diagnostic is worth most. The browser worker has a module-level `Logger` already, so both drivers are covered rather than only the nodejs one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`commitIfNeeded` clears `_inTx` before stepping the COMMIT, so by the time the failure reaches `safeMutate` the flag it recovers on is already false and `rollbackIfNeeded` no-ops. A COMMIT can fail with the transaction still open (classically `SQLITE_BUSY`), and nothing is then left to end it, so every later write on the connection joins it, reports success, and is lost at close. Both assertions count steps rather than naming the statement, because every prepared statement in this file shares one mock. Also replaces the stale `FAILING REPRO` marker on the neighbouring test, whose comment still described the ordering bug fixed in 86d16dc as present, with a pointer to the rationale in `commitIfNeeded`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
86d16dc cleared `_inTx` before stepping the COMMIT so a throwing one could not wedge the flag true. That fixed the wedge but left nothing to end the transaction: SQLite can fail a COMMIT with it still open, and `safeMutate`'s recovery calls `rollbackIfNeeded`, which reads the now-false flag and no-ops. `releaseSavepoint` reaches `commitIfNeeded` outside any try/catch at all. So the transaction stayed open with no owner, took every later write on the connection, reported success, and lost them at `close()` — the same hazard `rollbackSavepoint` and `releaseSavepoint` already discard the transaction to avoid, by a third door. `commitIfNeeded` now does the same. Reported by Cursor Bugbot. The nodejs driver reads `db.inTransaction` from SQLite rather than caching it, so it has no equivalent gap; that asymmetry is #1013. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The changelog entries for this PR explained mechanism where a consumer only needs effect; the comments repeated what the code says or said it twice across duplicated call sites. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Stale TDD markers ("FAILING REPRO") that now describe bugs the branch
fixed, and per-test preambles restating what the test title and the
source comment already say.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two invariants the crank layer relies on: a failed `ROLLBACK TO` discards the whole transaction, so truncating `ctx.savepoints` to zero still matches the database; and `commitIfNeeded` leaves no transaction behind. Both drivers catch and log an abort that fails while discarding a transaction, so the first holds only when that abort succeeds. This driver reads `db.inTransaction` from SQLite rather than caching it, which prevents a wedged flag but does not end a transaction nothing owns. The mock tracks the transaction the way SQLite does — a statement that throws changes nothing, one that succeeds opens or closes it — because a mock whose `inTransaction` never clears can only ever model a wedged connection, which is the state under test and so the one thing that must not be assumed. Co-Authored-By: Dimitris Marlagkoutsos <info@sirtimid.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two doors onto the same hazard, one per driver. `releaseSavepoint` in the nodejs driver reaches `commitIfNeeded` outside any try of its own, so a throwing COMMIT left the transaction open with nothing to end it — the gap b90e7a5 closed in the wasm driver. Reading `db.inTransaction` prevents a wedged flag but does not end a transaction nothing owns; that asymmetry was #1013. And when the compensating abort itself failed, both drivers emptied the savepoint list while SQLite still held the transaction. The next `createSavepoint` then saw `inTransaction`, skipped BEGIN, nested inside the abandoned transaction, and its RELEASE committed the crank the abort had been trying to throw away. Both now retry the abort and, if the transaction survives it, throw rather than return: reporting that the connection can no longer persist anything is the only honest answer left, where returning normally tells the caller its write landed. Checking the flag before `_inTx` in the wasm driver also makes that guard reachable — `abortTransaction` clears `_inTx` on its way to failing, so gating on it made the branch dead code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s callers The flush is last of the crank's own work so that no external caller is answered before the fallible work is done, and the reference count audit runs after the flush so that buffered items are not read as leaks. The audit is itself fallible, so the two orderings contradict each other. Co-Authored-By: Dimitris Marlagkoutsos <info@sirtimid.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… inside it `assertRefCountsIfAuditing` has to run after the crank buffer flush, because a buffered item's references were counted at enqueue time and so read as a leak mid-flush. But the flush is also what settles the promise `enqueueMessage` gave an external caller, and the audit is fallible: a violation threw into the run loop's catch, which rolled back the delivery the caller had already been answered from. Moved past `endCrank`, where a violation still kills the run loop — which is what an audit failure means — without pretending to undo a crank that has landed. Guarded on the crank having delivered something, so an idle crank about to sleep is not audited. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…point `KernelQueue.#runLoop` calls releasing its `crank` savepoint "this crank's one commit point". `releaseAllSavepoints` releases `t0`, which is the outermost savepoint only when the crank opened the first one, and two production paths open savepoints through `KernelStore.createSavepoint` -- invisible to the ordinal numbering, uncoordinated with the crank, one of them held across an await. Real SQLite through the real driver, one test per interleaving. The assertions record what SQLite actually does rather than what the kernel ought to: a savepoint that is not the outermost one does not commit on release, and `ROLLBACK TO` cancels every savepoint taken after its target. Those are the semantics the kernel-side serialization exists to respect, and they do not change with it. Co-Authored-By: Dimitris Marlagkoutsos <info@sirtimid.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rlapping Savepoints taken through `KernelStore.createSavepoint` bypass `ctx.savepoints` and so are invisible to `createCrankSavepoint`'s ordinal naming. One held open across a crank left `releaseAllSavepoints` releasing `t0` into it rather than to a commit, so the holder's later rollback discarded the whole crank. One opened inside a crank was cancelled by a delivery rollback it had nothing to do with, or committed by a release beneath it — in both cases after the peer had been told the message was durably received. Both directions are now refused. `createSavepoint` throws inside a crank, `startCrank` throws while a caller holds the store, and the two production callers take their turn through `beginOutOfCrank`/`endOutOfCrank`, which the run loop consults via `outOfCrankWorkPending` before each crank. The run loop re-checks that gate rather than awaiting it once: a caller registers synchronously, so one arriving in a microtask queued ahead of the loop's resumption would otherwise meet `startCrank`'s refusal and kill the kernel over ordinary concurrent remote traffic. `handleRemoteMessage` decodes an incoming `redeemURL` before opening its savepoint rather than awaiting inside it. `redeemLocalOcapURL` only parses and decrypts, so the message stays atomic, and the window is now synchronous — which is the rule the whole arrangement rests on, since awaiting while holding the store would park the run loop for the duration. Trade-off: an inbound remote message or a peer handshake arriving mid-crank now waits for that crank to end, so its latency is bounded by the slowest crank rather than independent of it. Routing inbound messages through the run queue, as SwingSet's comms vat does, would remove that coupling and is the better long-term shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e times 57bf771 replaced a fixed reap count with polling for `reapImporterUntil` and left `reapAndSettle` on three attempts. Three was enough on an idle machine and not under a loaded one: the whole suite in parallel failed `survives until both importers let go` about once in seven runs, on `main` as well as here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sirtimid
force-pushed
the
sirtimid/crank-rollback-integrity
branch
from
September 7, 2026 23:28
fc0ed30 to
9b4717f
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9b4717f. Configure here.
…d, not just the ones that begin or commit Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…es its timestamp Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sirtimid
added a commit
that referenced
this pull request
Sep 8, 2026
`RefCountViolation` gained its `kind` discriminant here, so the expectations #1021 wrote against the old shape needed it. The audit also moved out of the crank and runs after it commits, and `utils.ts` grew hooks that fail a test whose run loop died — so the case that kills the loop on purpose now claims that death as its result. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Sep 8, 2026
The repo moved to Consensys-Incorporated. Only the links this branch adds are rewritten; entries for PRs that really did live at MetaMask/ocap-kernel keep naming it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sirtimid
added a commit
that referenced
this pull request
Sep 11, 2026
`RefCountViolation` gained its `kind` discriminant here, so the expectations #1021 wrote against the old shape needed it. The audit also moved out of the crank and runs after it commits, and `utils.ts` grew hooks that fail a test whose run loop died — so the case that kills the loop on purpose now claims that death as its result. Co-Authored-By: Claude Opus 5 (1M context) <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.
#1020 has merged; this is rebased onto
mainand stands alone. Replaces #1012, #1018 and #1011, which are closed.Four PRs were editing the same
rollbackCranklines. This one owns all of the kernel's crank-rollback, savepoint and transaction-boundary semantics, so nothing else has to.Closes #1016 — a throw out of
delivernow rolls the delivery back instead of committing the partial crank.Why these had to merge
#1012 rewrote
rollbackCrank'sfinallyinto atry/catchthat truncates the savepoint stack and rethrows. #1010 changedctx.savepointsfromstring[]to{name, maybeFreeKrefs}[]and added a cache/GC-candidate restore further down the same function. Composed naively the rethrow fires before the restore, so a failed rollback leaves stale in-memory GC candidates and stale cached stored values behind. Neither PR could see that from inside itself.A second one surfaced while assembling this branch, and is the sharper argument: #1020's audit and #1012's flush reorder are silently incompatible. The audit reads the run queue as ground truth, but a buffered item's refcounts are incremented at
enqueueSend/enqueueNotifytime, so auditing before the flush reports every buffered item as a leak.assertRefCountsIfAuditingnow runs after the flush.What's fixed
Carried from #1012:
releaseSavepointis hardened the wayrollbackSavepointwas. ARELEASEthat threw 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.crankanddeliverysavepoints and the run loop rolls back onlydelivery. Rolling back the outermost one ended the transaction, so the work an aborted crank still owes — terminating the vat, collecting garbage — was autocommitting a statement at a time._inTxis cleared before the abort is attempted, because the abort can throw.Fixing the four defects #1018 pinned as failing repros — its tests are carried here unmodified, with Ryan's authorship, and the fixes land as later commits so the branch reads test-then-fix:
RemoteHandlereports the release failure, not a missing savepoint. It released inside itstryand rolled back in thecatch; now that a failedRELEASEdiscards the stack, that rollback threwNo such savepointin place of the real error.RemoteManagerhad the identical shape at itspeerIncarnation_*savepoint with zero coverage — test: failing repros for four defects found reviewing #1012 #1018 flagged it and it is fixed and covered here. The savepoint-stack model is shared (test/savepoint-stack.ts), so a fix applied to one and forgotten in the other can't leave a green suite.endCrankno longer buries the error that killed the run loop.#runLoopcalled it from a barefinally. The in-flight error is boxed rather than compared againstundefined, so a crank that threwundefinedstays distinguishable from one that didn't throw.makeSQLKernelDatabase, making fourlogger?.errorcalls dead code. Fixed for both the nodejs path andkernel-browser-runtime'skernel-worker.ts, so both drivers' calls are live.commitIfNeededclears_inTxbefore the COMMIT, the orderingrollbackIfNeededwas already corrected for. A throwing COMMIT wedged_inTxtrue andbeginIfNeededbecame a permanent no-op.And the composition bug above:
revertStateBeneathRollback()and called from both sites. A failedROLLBACK TOmakes the driver discard the whole transaction, so the database has moved back at least as far as a successful rollback would have taken it — the caches are at least as stale, and that is precisely where a lost GC action does the most damage. If the revert itself throws while a rollback error is in flight, the rollback error is preserved ascause.Note for the reviewer
refreshCachedValues()refreshesgcActions, and it now runs on both rollback paths. The GC-delivery hardening PR stacked after this one depends on that: a DB rollback restores thegcActionsrow but not the cached closure over it, so without this the audit builds its exemption set from a stale cache and kills the run loop. Verified against a real SQLite store.Testing
yarn lintclean,yarn build31/31, fourchangelog:validateruns clean. kernel-store, ocap-kernel, kernel-node-runtime, kernel-browser-runtime and kernel-test all green.Every fix mutation-verified — revert the production hunk and the named test fails for the stated reason.
@ocap/kernel-testis intermittently flaky, on this branch and on its base. Roughly two failures in twelve runs, alwaysgarbage-collection › survives until both importers let goorsupervisor › initializes vat with powers, each passing in isolation and in five consecutive clean runs after. Both are GC/timing-dependent and untouched by crank, savepoint or logger code. The first is what the vat-lifecycle PR further up this stack targets, viamakeGCAndFinalizedraining the queues beforegc(). Not introduced here, and not claimed green.Not done
kernel-worker.tslogger wiring;kernel-browser-runtimehas no test module for it and standing up the browser mock surface is disproportionate for a one-line change. The nodejs equivalent is pinned by test: failing repros for four defects found reviewing #1012 #1018's own repro.kernel-node-runtime/test/helpers/remote-comms.tsandkernel-test-local/src/lms-chat.tsstill callmakeSQLKernelDatabasewithout a logger — test harnesses, not production call sites.Follow-ups filed
createCrankSavepoint's name parameter can't express "crank outer, delivery inner", the invariant that fails silentlyChecklist
README.md,CHANGELOG.md) as appropriateNote
High Risk
Changes core run-loop commit/rollback, SQLite transaction recovery, and remote-message serialization—bugs here cause silent data loss or stuck kernels.
Overview
Hardens SQLite kernel-store transaction and savepoint recovery on both nodejs and wasm drivers: failed
RELEASE/COMMIT/ROLLBACKpaths discard or retry transactions, refuse further writes when a transaction cannot be aborted, and surface driver diagnostics via loggers wired from browser and node runtimes.Crank boundaries now use nested
crank+deliverysavepoints so only the delivery rolls back while vat termination, GC, and post-abort store work stay in one commit. The run loop flushes crank buffers only after that fallible work, invokes ref-count audits after commit (so audits do not roll back already-answered deliveries), chainsendCrankfailures ascause, and waits on newbeginOutOfCrank/endOutOfCrankgates so inbound remote messages and peer incarnation updates cannot interleave savepoints with cranks.rollbackCrankalso refreshes in-memory caches and clears GC candidates when the DB rollback fails.RemoteHandle decodes
redeemURLbefore opening its savepoint; savepoint rollback failures are logged instead of masking release errors.Kernel.stoprecords last-active time best-effort so teardown still runs if the store refuses writes.Reviewed by Cursor Bugbot for commit 99eeb9c. Bugbot is set up for automated code reviews on this repo. Configure here.