Skip to content

Commit 64a33e1

Browse files
grypezclaude
authored andcommitted
test(kernel-store): pin the failed COMMIT that wedges _inTx
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>
1 parent e5b2e27 commit 64a33e1

1 file changed

Lines changed: 38 additions & 0 deletions

File tree

packages/kernel-store/src/sqlite/wasm.test.ts

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -599,6 +599,44 @@ describe('makeSQLKernelDatabase', () => {
599599
expect(mockDb.exec).toHaveBeenCalledWith('SAVEPOINT next');
600600
});
601601

602+
// FAILING REPRO — see the commit message for this test.
603+
//
604+
// `commitIfNeeded` still steps the COMMIT before clearing `_inTx`, the exact
605+
// ordering `rollbackIfNeeded` was corrected to avoid. A COMMIT that throws
606+
// therefore leaves `_inTx` true against a database that may hold no
607+
// transaction, `beginIfNeeded` is a no-op forever after, and the next
608+
// savepoint is created outside a transaction — which commits when released
609+
// (Agoric/agoric-sdk#8423). This is the crank's commit point, so the writes
610+
// that leak are a whole crank's.
611+
//
612+
// The comment above `stops believing it is in a transaction when the abort
613+
// fails too` calls a failed abort "the one case that can leave `_inTx`
614+
// disagreeing with the database". This is the second case.
615+
it('stops believing it is in a transaction when the commit fails', async () => {
616+
const db = await makeSQLKernelDatabase({});
617+
mockDb._inTx = true;
618+
mockDb._spStack = ['point1'];
619+
// The RELEASE goes through `exec` and succeeds; COMMIT is the first
620+
// prepared statement this path steps, and it is what fails.
621+
mockStatement.step.mockImplementationOnce(() => {
622+
throw new Error('disk I/O error');
623+
});
624+
625+
expect(() => db.releaseSavepoint('point1')).toThrowError(
626+
'disk I/O error',
627+
);
628+
629+
expect(mockDb._inTx).toBe(false);
630+
631+
// And so the next savepoint gets a transaction of its own rather than
632+
// being created bare.
633+
mockDb.exec.mockClear();
634+
mockStatement.step.mockClear();
635+
db.createSavepoint('next');
636+
expect(mockStatement.step).toHaveBeenCalledOnce();
637+
expect(mockDb.exec).toHaveBeenCalledWith('SAVEPOINT next');
638+
});
639+
602640
it('supports nested savepoints', async () => {
603641
const db = await makeSQLKernelDatabase({});
604642
db.createSavepoint('outer');

0 commit comments

Comments
 (0)