Skip to content

fix(kernel-store): ask SQLite whether a transaction is open - #1089

Merged
sirtimid merged 7 commits into
mainfrom
sirtimid/wasm-live-in-transaction
Oct 8, 2026
Merged

sirtimid merged 7 commits into
mainfrom
sirtimid/wasm-live-in-transaction

Conversation

@sirtimid

@sirtimid sirtimid commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

The wasm driver tracked open transactions with its own _inTx flag. SQLite can roll a transaction back on its own after SQLITE_FULL, SQLITE_IOERR or SQLITE_NOMEM, and the flag stayed true. The next createSavepoint skipped its BEGIN, so its SAVEPOINT opened a transaction that RELEASE committed, and the driver's own COMMIT then threw cannot commit - no transaction is active. The flag never cleared, so later releases threw the same.

inTransaction is now a getter over sqlite3_get_autocommit. Init fails if the wasm build does not bind that function. A closed database reports no transaction open.

  • beginIfNeeded clears the savepoint stack when it opens a transaction, in both drivers. The Node driver has had this bug since the savepoint stack was added (feat(kernel): Handle syscall failures #520); the wasm driver would have gained it with the live getter.
  • The wasm kv and vatstore statements reset in finally, so one SQLITE_FULL no longer makes every later call to that operation throw SQLITE_MISUSE.

Testing

  • wasm.transactions.test.ts and the new nodejs.transactions.test.ts run the drivers against real SQLite and fail a write with a real SQLITE_FULL (PRAGMA max_page_count). 'takes the next crank in a transaction of its own' and 'keeps taking kv/vatstore writes after one fails' fail on main. 'commits a write made after SQLite ends the transaction itself' fails without the wasm stack clear, and the Node test, whose write must survive closing and reopening the database, fails without the Node one.
  • The mocked wasm.test.ts and nodejs.test.ts flip transaction state when BEGIN/COMMIT/ABORT run, as SQLite does, which fixed three assertions that could not fail. An it.each in wasm.test.ts pins the reset at each of the seven bound statements.
  • Each fix was mutation-checked against the tests named here.

Resolves #1074

🤖 Generated with Claude Code


Note

Medium Risk
Changes transaction and savepoint handling in the kernel persistence layer; bugs here could lose or fail to commit vat state, though the PR corrects known failure modes after SQLite auto-rollback.

Overview
Fixes kernel-store SQLite drivers when SQLite implicitly rolls back a transaction (e.g. SQLITE_FULL / I/O errors) while the driver still thought a transaction and savepoints were active.

The wasm driver stops caching transaction state in _inTx and exposes inTransaction from sqlite3_get_autocommit (init fails if that C API is missing). beginIfNeeded in both Node and wasm clears the savepoint stack when opening a new transaction so stale names cannot block commits or cause cannot commit - no transaction is active on later savepoint releases. Wasm KV/vatstore paths reset prepared statements in finally so one failed step does not poison later calls with SQLITE_MISUSE.

Tests add real-SQLite transaction suites (wasm.transactions.test.ts, nodejs.transactions.test.ts) and tighten mocks so BEGIN/COMMIT/ABORT flip transaction state like SQLite. CHANGELOG also records the separate better-sqlite3 ^13 bump (Node 22+).

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

sirtimid and others added 3 commits September 15, 2026 19:12
The wasm driver tracked the transaction itself in `_inTx`. SQLite ends a
transaction on its own after SQLITE_FULL, SQLITE_IOERR or SQLITE_BUSY, and the
flag then latched: the next savepoint skipped its `BEGIN`, and releasing it
threw "cannot commit - no transaction is active". `inTransaction` now reads
`sqlite3_get_autocommit`, which the wasm build binds but does not type — hence
the cast, and the check at init that it is there.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Without the `pointer` guard, `sqlite3_get_autocommit(undefined)` returns 0 and a
closed connection reports a transaction open.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…egins

Review follow-up. Reading `inTransaction` from SQLite drops the invariant the
cached flag carried, that no transaction implies an empty savepoint stack. When
SQLite ends a transaction itself, the stale stack makes `commitIfNeeded` defer
to a savepoint owner that no longer exists, and the next write sits in a
transaction nobody closes. The Node driver has read better-sqlite3's live
`inTransaction` all along and had the same hole; both are fixed, and both mocks
now move the flag the way SQLite does.

The capi check moves above the open so a build without `sqlite3_get_autocommit`
does not leak the handle, and `inTransaction` is no longer `configurable`.

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 17:35
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
# Conflicts:
#	packages/kernel-store/CHANGELOG.md
@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 73.65%
⬆️ +0.05%
10220 / 13875
🔵 Statements 73.41%
⬆️ +0.05%
10358 / 14109
🔵 Functions 73.55%
⬆️ +0.01%
2386 / 3244
🔵 Branches 68.47%
⬆️ +0.03%
4256 / 6215
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
packages/kernel-store/src/sqlite/nodejs.ts 99.01%
⬆️ +0.01%
89.28%
⬇️ -7.14%
100%
🟰 ±0%
99.01%
⬆️ +0.01%
79
packages/kernel-store/src/sqlite/wasm.ts 100%
⬆️ +2.02%
95.23%
⬆️ +5.76%
100%
🟰 ±0%
100%
⬆️ +2.03%
Generated in workflow #5227 for commit 006ae98 by the Vitest Coverage Report Action

sirtimid and others added 2 commits October 8, 2026 13:59
A wasm write that failed with SQLITE_FULL left its prepared statement
unreset, and SQLite refuses to bind one in that state, so every later
call to the same operation threw SQLITE_MISUSE. The kv and vatstore
statements now reset in `finally`, as `getKVData` and `executeQuery`
already did.

The real-SQLite tests now end the transaction with a real SQLITE_FULL
instead of a hand-issued `ROLLBACK TRANSACTION`, which never failed a
statement and so could not see this. A new Node test runs the savepoint
stack clear against real better-sqlite3, and the changelog says what
`main` actually did.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sirtimid added a commit that referenced this pull request Oct 8, 2026
…int's transaction

After SQLITE_FULL or an I/O error, SQLite can roll back the transaction
a crank's savepoints were in while the crank carries on. Every write it
made from then on landed outside any transaction and committed alone:
the dequeue was undone but the vatstore and promise resolutions stuck,
so a restart redelivered the message to a vat that had already handled
it.

Every driver write now goes through `guardWrite`, which refuses while
the savepoint stack names a transaction SQLite no longer has.
`rollbackSavepoint` and `releaseSavepoint` tell the savepoint's owner
that SQLite already ended the transaction and clear the stack, which
ends the refusal. Both errors carry the write that failed as `cause`,
since the code that caught it may have swallowed it. The stack clear
#1089 added to `beginIfNeeded` is unreachable behind the new check and
goes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sirtimid
sirtimid added this pull request to the merge queue Oct 8, 2026
Merged via the queue into main with commit 8dc64f0 Oct 8, 2026
28 checks passed
@sirtimid
sirtimid deleted the sirtimid/wasm-live-in-transaction branch October 8, 2026 20:04
sirtimid added a commit that referenced this pull request Oct 8, 2026
sirtimid added a commit that referenced this pull request Oct 9, 2026
Conflicts: the changelog keeps #1092's wasm COMMIT entry and drops
#1089's Node entry, which this branch replaces; the corrected
`rollbackSavepoint` comment sits above `discardTransaction('rollback')`;
the fake driver is this branch's, with `abort` destructured where #1092
restored its assertion.

Co-Authored-By: Claude Opus 5.5 <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.

The wasm driver's sqlite3_get_autocommit binding is asserted by cast, not checked

2 participants