Skip to content

fix(kernel-store): refuse writes into a transaction nothing can end - #1094

Open
sirtimid wants to merge 3 commits into
sirtimid/sqlite-transaction-helpersfrom
sirtimid/sqlite-abandoned-transaction
Open

sirtimid wants to merge 3 commits into
sirtimid/sqlite-transaction-helpersfrom
sirtimid/sqlite-abandoned-transaction

Conversation

@sirtimid

@sirtimid sirtimid commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Stacked on #1092, which is stacked on #1089. Review those first; this branch's base is sirtimid/sqlite-transaction-helpers.

A savepoint operation that fails can leave a SQLite transaction open with nothing on the savepoint stack to reach it through. rollbackSavepoint has guarded against that since #1005 — but only for its own ROLLBACK TO. Three other doors lead to the same place:

  • a failed RELEASE SAVEPOINT, where releaseSavepoint had no catch at all;
  • a failed COMMIT, which SQLite may leave the transaction open after, reached from releaseSavepoint outside any try of its own;
  • a failed SAVEPOINT on a transaction createSavepoint had just opened — nothing was pushed, so no savepoint path would ever reach that transaction again.

Each of those now discards the transaction. Behind all three, the state nobody had a story for: the abort doing the discarding can itself fail. The transaction is then still open, still holding the crank that was being thrown away, and a savepoint taken afterwards would be created inside it and released into it — committing exactly what was discarded. There is nothing honest to do but refuse, so the driver records txAbandoned and every write asks first. It retries the abort on each attempt, so a connection SQLite recovers starts writing again rather than being refused forever.

Because #1092 unified the two drivers, all of that is one copy.

Changes

  • transactions.ts: discardTransaction(after), called from the failed release, commit and savepoint-creation paths as well as the existing rollback one; txAbandoned and assertNotAbandoned(). The refusal carries the abort's error as its cause, since the logger that would otherwise report it is the embedder's to pass.
  • createSavepoint validates the name before opening a transaction, so a name it rejects leaves none behind. It also only discards a transaction it opened itself — one it merely found open belongs to the crank that opened it, which still has its own savepoint to roll back through.
  • Every write door asks: kv set and delete through an injected assertWritable, plus clear, deleteVatStore, and the Node driver's vatstore update. The wasm driver's vatstore update has no second guard because safeMutatebeginIfNeeded already asks — a test pins that.
  • Kernel.stop records its last-active timestamp best-effort. A refused timestamp is not worth failing to release remote comms, the vat workers and the database handle over.
  • releaseAllSavepoints forgets the crank's savepoints when the release throws, the way rollbackCrank already does — see Testing.

Testing

One shared suite in transactions.test.ts, against a fake database that models SQLite's transaction state rather than a boolean: a statement that throws changes nothing, one that succeeds opens or closes the transaction, and two levers let a test say "SQLite ended it itself" and "the abort returned but the transaction is still there". A mock that cannot model recovery can only ever model a wedged connection, which is the state under test. Each driver keeps an it.each over its own write doors.

This re-lands the work whose tests were nodejs.transaction-survival.test.ts and wasm.transaction-survival.test.ts on sirtimid/crank-rollback-integrity, which in turn grew out of @grypez's #1039. Claim 3 ("a failed ROLLBACK TO discards the whole transaction, in both drivers") and claim 6 ("commitIfNeeded leaves no transaction behind") both land here, as discards the transaction when the rollback itself fails and discards the transaction when the commit fails plus the a transaction an abort could not end block that qualifies both — they hold only while the abort succeeds, which is what the rest of this PR is about. Claim 4 lands with the crank-cache PR, claims 1 and 2 with the two-savepoint and audit-ordering PRs.

Every behaviour above was mutation-checked by reverting it alone and confirming the named test fails, including each discardTransaction label, the txAbandoned set and clear on both paths, each driver's guards, and Kernel.stop's catch.

Review turned up one regression this PR would otherwise have introduced. releaseAllSavepoints did not clear ctx.savepoints when the release threw — harmless while a failed release left the store's own stack intact, and not harmless now that it discards it. The two stacks would have disagreed from then on: every later crank released a t0 the store no longer had, throwing before it reached SQLite, so nothing ever ended the transaction and Kernel.reset never got past kdb.clear() to restore the keySeed, peerId and ocapURLKey it had preserved. It now mirrors rollbackCrank's finally, with a test.

kernel-store and ocap-kernel suites and builds are green locally; @ocap/kernel-test's crank-rollback, vatstore and garbage-collection are green against a rebuilt dist.

Two things deliberately left out. executeQuery does not ask, because it takes arbitrary SQL including reads and a wedged connection can still read; it is dev-mode-only. And RemoteHandle.handleRemoteMessage / RemoteManager's incarnation change release a savepoint inside a try whose catch rolls it back, so a failed release now surfaces No such savepoint over the real error — a diagnosis regression, not a data one, in two blocks that the run-queue work removes outright.

🤖 Generated with Claude Code


Note

High Risk
Changes core SQLite transaction and savepoint lifecycle for all kernel persistence; mistakes here can still cause silent data loss or block writes until restart.

Overview
Fixes SQLite kernel-store transaction handling so failed RELEASE, COMMIT, or SAVEPOINT operations no longer leave an open transaction that later writes join silently (success reported, data lost on close). Those paths now discard the transaction via shared discardTransaction, matching the existing rollback behavior.

When the abort that performs that discard also fails, the driver sets txAbandoned and assertNotAbandoned blocks all persistence entry points (KV set/delete, clear, vatstore updates/deletes) in both Node and WASM drivers, retrying abort on each attempt so a recovered connection can write again. Invalid savepoint names are validated before BEGIN, so a rejected name cannot orphan a transaction.

On the kernel side, releaseAllSavepoints clears in-memory crank savepoints in a finally even when the DB release throws, staying aligned with the store’s discarded stack (avoiding stuck transactions and broken Kernel.reset). Kernel.stop records last-active time best-effort so shutdown still tears down remote comms, workers, and the DB when the store refuses writes.

Reviewed by Cursor Bugbot for commit 1e636d7. Bugbot is set up for automated code reviews on this repo. Configure here.

sirtimid and others added 2 commits September 15, 2026 20:06
A `RELEASE`, `COMMIT` or `SAVEPOINT` that fails can leave the transaction open
with nothing on the savepoint stack to reach it through, so every later write
joins it, reports success, and vanishes on close. Each of those paths now
discards the transaction instead.

When even that abort fails there is nothing honest left to do but refuse: a
savepoint taken afterwards would be released into the crank the abort was
throwing away. Every write door asks first, the abort is retried on each
attempt, and a connection SQLite recovers starts writing again.

`Kernel.stop` no longer lets a refused timestamp stop it from releasing remote
comms, the vat workers and the database handle.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review follow-up. A failed release now discards the transaction and every
savepoint in it, so `releaseAllSavepoints` has to forget them the way
`rollbackCrank` already does. Left listed, every later crank released a `t0`
the store no longer had — it threw before reaching SQLite, so nothing ever
ended the transaction and `Kernel.reset` never restored the keys it preserved.

The refusal now carries the abort's error as its cause: the logger that would
otherwise report it is the embedder's to pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sirtimid
sirtimid requested a review from a team as a code owner September 15, 2026 18:30
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant