Skip to content

refactor(kernel-store): one copy of the savepoint and transaction methods - #1092

Open
sirtimid wants to merge 15 commits into
mainfrom
sirtimid/sqlite-transaction-helpers
Open

sirtimid wants to merge 15 commits into
mainfrom
sirtimid/sqlite-transaction-helpers

Conversation

@sirtimid

@sirtimid sirtimid commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

The Node and wasm drivers carried the same six transaction and savepoint methods, beginIfNeeded, commitIfNeeded, rollbackIfNeeded, createSavepoint, rollbackSavepoint and releaseSavepoint, differing only in how a prepared statement runs.

They move into the internal sqlite/transactions.ts, built by makeTransactionMethods({ db, begin, commit, abort }), and each driver injects its three statement runners.

Testing

  • The savepoint tests duplicated across both driver suites are now one suite, transactions.test.ts, against a fake database. beginIfNeeded, commitIfNeeded and rollbackIfNeeded get direct tests there, and a repeated name resolves to its newest savepoint.
  • Each driver keeps one test that it runs its own prepared statements.
  • A real-SQLite test fails a COMMIT on a deferred foreign key, rolls back, and drops a table; without the finally the drop hits SQLITE_LOCKED.
  • vatStore.updateKVData's begin and commit assertions check the BEGIN and COMMIT statements; they were checking the kv statement.

Closes #1013

🤖 Generated with Claude Code


Note

Medium Risk
Changes shared persistence transaction/savepoint behavior for both drivers; the wasm COMMIT-reset fix alters lock lifecycle after errors, though behavior is heavily tested.

Overview
Unifies SQLite transaction and savepoint handling across the Node (better-sqlite3) and wasm drivers by moving beginIfNeeded, commitIfNeeded, rollbackIfNeeded, and the three savepoint APIs into shared makeTransactionMethods in sqlite/transactions.ts. Each driver only supplies how it runs its prepared BEGIN/COMMIT/ROLLBACK statements.

The wasm driver now runs those statements (and clear) through a runStatement helper that always resets after step(), fixing a case where a failed COMMIT left the statement busy and held a read lock so DROP TABLE / VACUUM failed until the next commit (#1092).

Tests: duplicated savepoint suites in nodejs.test.ts and wasm.test.ts are replaced by transactions.test.ts against a fake DB; each driver keeps one test that its own prepared statements are invoked. Integration coverage adds the post-failed-COMMIT DROP TABLE scenario in wasm.transactions.test.ts. Changelog wording for failed rollbackSavepoint is clarified.

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

sirtimid and others added 6 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>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…hods

`createSavepoint`, `rollbackSavepoint` and `releaseSavepoint` were byte-for-byte
identical in both drivers, and after the last change so were the three
transaction helpers under them. The drivers now differ only in how they run a
prepared statement, which is what they inject.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review follow-up. Both driver suites had `releaseSavepoint removes all points
after target` and the extracted suite did not, so `splice(idx, 1)` there passed
everything — which would leave the crank's outermost release with a non-empty
stack and its transaction unowned.

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:55
@sirtimid sirtimid added the no-changelog Indicates that no changelog updates are required, and that related CI checks should be skipped. label Sep 15, 2026
@sirtimid
sirtimid added this pull request to stack #1109 September 15, 2026 22:42
sirtimid and others added 5 commits October 7, 2026 22:17
# Conflicts:
#	packages/kernel-store/CHANGELOG.md
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>
…n' into sirtimid/sqlite-transaction-helpers

# Conflicts:
#	packages/kernel-store/src/sqlite/nodejs.test.ts
#	packages/kernel-store/src/sqlite/wasm.test.ts
#	packages/kernel-store/src/sqlite/wasm.ts
@github-actions

github-actions Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 73.58%
⬇️ -0.02%
10184 / 13840
🔵 Statements 73.34%
⬇️ -0.02%
10322 / 14074
🔵 Functions 73.57%
⬆️ +0.03%
2389 / 3247
🔵 Branches 68.42%
⬇️ -0.02%
4240 / 6197
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
packages/kernel-store/src/sqlite/nodejs.ts 98.52%
⬇️ -0.48%
91.66%
⬇️ -4.76%
100%
🟰 ±0%
98.52%
⬇️ -0.48%
76
packages/kernel-store/src/sqlite/transactions.ts 100% 100% 100% 100%
packages/kernel-store/src/sqlite/wasm.ts 100%
⬆️ +2.02%
96.15%
⬆️ +6.68%
100%
🟰 ±0%
100%
⬆️ +2.03%
Generated in workflow #5236 for commit be53378 by the Vitest Coverage Report Action

Base automatically changed from sirtimid/wasm-live-in-transaction to main October 8, 2026 20:04
sirtimid and others added 4 commits October 8, 2026 22:07
A COMMIT that fails and leaves the transaction open (SQLITE_BUSY, a
deferred foreign key) stays busy until reset, and the connection keeps a
read lock past the ROLLBACK that ends the transaction: DROP TABLE fails
with SQLITE_LOCKED until the next COMMIT. `runStatement` now resets in a
`finally`, as the kv and vatstore statements already do, and `kvClear`
goes through it. The local `PreparedStatement` type gives way to the one
sqlite-wasm declares.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…w found untested

- The `TransactionalDatabase` docstring now says why `_spStack` dropping a
  rolled-back name is safe: no name is opened twice on the stack.
- `rollbackSavepoint`'s catch no longer claims a failed rollback discards
  only what it was abandoning; it discards the savepoints below too.
- New tests: a repeated name resolves to its newest savepoint;
  `beginIfNeeded` reports that it began; the abort is attempted when the
  rollback fails. `vatStore.updateKVData`'s begin and commit assertions
  now check the BEGIN and COMMIT statements rather than the kv one.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…1005 entry

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sirtimid sirtimid removed the no-changelog Indicates that no changelog updates are required, and that related CI checks should be skipped. label Oct 9, 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>

This branch has not been deployed

No deployments
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.

Extract the duplicated sqlite savepoint methods into one module

1 participant