Skip to content

test: cross-examine the six transaction claims #1021 rests on - #1039

Draft
grypez wants to merge 5 commits into
sirtimid/vat-lifecycle-consistency-v2from
grypez/crank-claims-repro
Draft

test: cross-examine the six transaction claims #1021 rests on#1039
grypez wants to merge 5 commits into
sirtimid/vat-lifecycle-consistency-v2from
grypez/crank-claims-repro

Conversation

@grypez

@grypez grypez commented Aug 31, 2026

Copy link
Copy Markdown
Member

Explanation

Cross-examination of the six load-bearing claims in #1021, as executable tests.
Each is a repro, not a fix: five files, ten tests, one commit per file. Seven
fail and three pass, and the split is the point — the passing three guard a fix
this branch already made.

Read the base ref carefully. #1021 is the PR whose claims these examine, but
#1022 and #1023 share no commits with it (811ee0b23, #1021's core fix, is not
an ancestor of #1022; the merge-base is 180e6ac47 on main, before #1021
starts). They are parallel re-applications of overlapping work, not a stack, so
"against the tip" and "against #1021" are different questions with different
answers. This PR targets the tip, sirtimid/vat-lifecycle-consistency-v2, and
every outcome below was measured there.

# Claim under test File Result at this base
1 Releasing crank in endCrank is the crank's one commit point nodejs.savepoint-interleaving.test.ts 3 fail
2 Flush last, then audit — so no caller is answered before the fallible work KernelQueue.audit-ordering.test.ts 1 fail
3 A failed ROLLBACK TO discards the whole transaction, in both drivers nodejs.transaction-survival.test.ts 2 fail
4 Every kref in maybeFreeKrefs was put there by this crank crank.cross-crank-gc.test.ts 3 pass
5 Reverting caches is safe on the failed-rollback path too (covered by 3 and 4) see comment
6 commitIfNeeded leaves no transaction behind wasm.transaction-survival.test.ts 1 fail

Claim 4 is false against #1021 and true here: eaa71ac00 on this branch
snapshots maybeFreeKrefs per savepoint and restores it rather than clearing.
Those three tests pass and are offered as a regression guard on that fix — they
fail against #1021's head, where the same code calls ctx.maybeFreeKrefs.clear().

Claim 6 runs the other way. This branch's wasm commitIfNeeded clears _inTx
before the COMMIT but never aborts, so a failed COMMIT leaves an ownerless
transaction — the gap b90e7a5e5 closes on #1021 and which this branch, not
descending from it, does not have. Whichever lands second has to carry that fix
across.

Claims 1, 3, and the nodejs half of 6 are unchanged everywhere and fail against
main, #1021, and this tip alike.

A per-claim walkthrough — what the claim is, why it matters, how to evaluate it,
what came out — is in the review comments, one per claim.

No production code and no changelog entries: this PR only adds tests.

References

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've communicated my changes to consumers by updating changelogs for packages I've changed, highlighting breaking changes as necessary — n/a, tests only
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

grypez and others added 5 commits August 31, 2026 11:51
…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.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A failed `ROLLBACK TO` is taken to discard the whole transaction, which is what
makes truncating the savepoint list to zero match the database. That holds only
when the compensating abort succeeds, and both drivers catch and log one that
does not. This driver reads `db.inTransaction` from SQLite, so it cannot wedge a
flag -- and also cannot end an ownerless transaction.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Clearing `_inTx` before stepping the COMMIT stops a throwing COMMIT wedging the
flag true, and leaves nothing able to end the transaction it left open:
`rollbackIfNeeded` reads the false flag and returns, and `releaseSavepoint`
reaches `commitIfNeeded` with nothing wrapping it.

Co-Authored-By: Claude Opus 5 <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: Claude Opus 5 <noreply@anthropic.com>
`maybeFreeKrefs` is not per-crank: only `collectGarbage` empties it, so a
candidate added while no crank was open must survive an unrelated crank's
rollback. `RemoteManager.#handlePeerIncarnation` is such a producer, and the
objects it abandons are invisible to the reference count audit once lost.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@grypez

grypez commented Aug 31, 2026

Copy link
Copy Markdown
Member Author

Claim 1 — "releasing crank in endCrank is this crank's one commit point"

What the claim is

KernelQueue.#runLoop opens two savepoints per crank:

this.#kernelStore.createCrankSavepoint('crank');
this.#kernelStore.createCrankSavepoint('delivery');

with the reasoning that rolling back the outermost savepoint discards the
enclosing transaction, and an aborted crank still has writes to make
(#terminateVat, collectGarbage). So delivery absorbs the rollback and
crank survives it. endCrank then releases crank, and that release is the
crank's single commit point
: exactly one place where a crank's work becomes
durable, exactly one place where it can be lost wholesale.

Why it matters

The claim is the whole thesis of #1021. If it holds, a crank is atomic: either
the delivery, the termination bookkeeping, and the flush all land, or none of
them do, and a restart resumes from a coherent point. If it does not hold, then
"one transaction per crank" is a description of the intended shape rather than
an invariant, and the failure mode is the one this PR series exists to remove —
a crank half in the database.

How to evaluate it

The claim is about the database's savepoint stack, not ctx.savepoints. Two
questions:

  1. Is t0 always the outermost savepoint on the connection? releaseAllSavepoints
    hardcodes kdb.releaseSavepoint('t0'), and createCrankSavepoint names by
    ordinal (t${ctx.savepoints.length}), so t0 is outermost only if the crank
    opened the first savepoint on the connection.
  2. Can anything else open a savepoint the crank cannot see?

For (2), KernelStore exposes createSavepoint/releaseSavepoint/
rollbackSavepoint (store/index.ts:305-325) which call kdb directly and
never touch ctx.savepoints. Two production callers:

  • RemoteHandle.handleRemoteMessagereceive_${remoteId}_${seq}, opened at
    line 1008 and held across await this.#handleRedeemURLRequest(...)
  • RemoteManager.#handlePeerIncarnationpeerIncarnation_${peerId}, line 242

Neither calls waitForCrank or checks isInCrank. Kernel.#init installs the
remote message handler as a bare async callback with no crank coordination, and
the run loop spends its time in await deliver(queueItem). So the two stacks
interleave, and all three relative orderings are reachable.

Then ask what SQLite actually does in each ordering. That is a question about
savepoint semantics, so it should be answered by SQLite and not by a mock — note
that the driver's own tests stub db.exec entirely, so nothing in the existing
suite exercises real SAVEPOINT/RELEASE/ROLLBACK TO behaviour.

nodejs.savepoint-interleaving.test.ts runs the real driver against an in-memory
better-sqlite3, one test per ordering.

Outcome: claim is false. Three failures.

Ordering What happens
Remote savepoint outside the crank RELEASE t0 leaves _spStack = ['receive_r1_7'], so commitIfNeeded does not fire. No commit. The remote's later ROLLBACK TO then discards the entire "committed" crank. Silent.
Remote savepoint inside, delivery aborts ROLLBACK TO t1 cancels every savepoint started after t1, including the remote's. The remote handler gets No such savepoint: receive_r1_7. Loud and recoverable — the best of the three.
Remote savepoint inside, crank succeeds RELEASE t0 releases everything above it, committing the remote's half-finished message — it is parked on its await and has not reached setRemoteHighestReceivedSeq. The handler then fails, so the peer retries an effect that has already landed. Exactly-once is broken.

The first row is the serious one: it is the exact failure this PR series set out
to eliminate, reachable without any I/O error, and it is silent.

This is pre-existing, not a regression — the raw-savepoint bypass predates
#1021, and these tests fail against main, #1021, and this tip alike. But the
comment in #runLoop states an invariant the code does not have, and that
comment is what the next person will build on.

What would make the claim true

Any one of: route the two remote paths through createCrankSavepoint so they
join the ordinal numbering; make them await waitForCrank() and assert
!isInCrank() before taking a savepoint; or have releaseAllSavepoints release
ctx.savepoints[0] by identity and assert it is the bottom of kdb's stack, so
a foreign savepoint underneath fails loudly instead of quietly deferring the
commit. Failing all of those, the comment should say that crank's release is
the crank's intended commit point and name what can sit underneath it.

@grypez

grypez commented Aug 31, 2026

Copy link
Copy Markdown
Member Author

Claim 2 — flush after the fallible work, audit after the flush

What the claim is

Two ordering decisions in #processCrankResult, each with its own stated
reason:

this.#kernelStore.collectGarbage();
if (!crankResult?.abort) {
  // After the fallible work above, not before it. The flush settles the
  // promise `enqueueMessage` gave an external caller, so a later rollback
  // would discard the state that answer was computed from.
  this.#flushCrankBuffer();
}
// After the flush, because the audit reads the run queue as ground truth
// while a buffered item's references were already counted when it was
// enqueued: audited mid-flush, every buffered item reads as a leak.
this.#kernelStore.assertRefCountsIfAuditing();

So: flush last, because answering an external caller is irreversible and must
not precede anything that can still roll back. And audit after the flush,
because the audit's ground truth is the persisted store, which does not include
the in-RAM crank buffer.

Why it matters

The first ordering is a correctness property with no test coverage and no type to
protect it — it lives entirely in the sequence of statements. The second is what
makes auditRefCounts usable at all; get it wrong and the option is a false-alarm
generator that kills the run loop on healthy state.

How to evaluate it

Take the audit premise first. Read what auditRefCounts treats as ground truth:
computeExpectedRefCounts walks getPrefixedKeys('') and credits only
${endid}.c.${eref} c-list rows, queue.* entries, ${kpid}.state, resolution
slots, and pins (refcount-audit.ts:182-261). All KV. ctx.crankBuffer is a
plain array in RAM and appears nowhere. Meanwhile enqueueSend/enqueueNotify
increment refcounts before choosing buffered or immediate. So pre-flush, every
buffered item has paid for references that no visible holder accounts for →
"stored too high" → violation. The premise is correct.

Then the flush ordering. It is not enough that the flush is the last statement
that answers callers
; the question is whether anything after it can still undo
the crank, and whether anything before it already answered. Two probes:

  1. Does anything before the flush settle an external subscription? Yes, and this
    branch documents it: #terminateVat resolves the dying vat's promises via
    resolvePromises (VatManager.ts:334), whose immediate defaults to true,
    invoking their subscriptions before collectGarbage. The comment says so
    outright — "Not airtight". #processCrankResult also rejects the aborted
    send's result subscription directly, earlier still.
  2. Does anything after the flush roll the crank back? This is the untested one.
    assertRefCountsIfAuditing throws. On the success path #crankRollbackAttempted
    is false, so the run loop's catch runs rollbackCrank('delivery').

KernelQueue.audit-ordering.test.ts drives one crank with a buffered notify, a
kernel subscription on its kpid, and an audit that reports drift, then asserts
what happened in which order.

Outcome: premise true, ordering rationale defeated — one failure.

The audit premise is confirmed by reading; no test needed and none written.

The flush-last rationale does not survive. The test shows the subscription's
resolve is invoked by the flush, and then rollbackCrank('delivery') is called —
the caller has its answer and the state that answer was computed from is
discarded underneath it. That is verbatim the hazard the comment above the flush
says the ordering prevents.

The two orderings are in direct conflict:

  • put the audit before the flush and it reports every buffered item as a leak;
  • put it after and it becomes fallible work running after the answers have gone
    out.

This branch picked audit-last and thereby gave up the property the flush-last
comment claims. Nothing pins it: KernelQueue.test.ts mocks
assertRefCountsIfAuditing as a no-op, so no existing test ever takes the throw.

Worth noting this is new. On main the audit throwing led to
rollbackCrank('start'), which discarded the whole transaction — the answers were
wrong there too, but the shape was different. The two-savepoint change altered
what a post-flush throw does without the ordering being revisited.

What would make the claim true

The audit does not need to see a consistent store — it needs to not be fooled by
the buffer. Teach computeExpectedRefCounts to credit ctx.crankBuffer items
the same way it credits queue.run.* rows, and the audit can run before the
flush, where a throw still rolls back cleanly and no caller has been answered.
That resolves the conflict rather than picking a side. Short of that, the
flush-last comment should be narrowed to what it actually buys, the way the
#terminateVat gap already was.

@grypez

grypez commented Aug 31, 2026

Copy link
Copy Markdown
Member Author

Claim 3 — a failed ROLLBACK TO discards the whole transaction, in both drivers

What the claim is

rollbackCrank, on a rollback that throws:

} catch (error) {
  // A failed rollback discards the whole transaction, so every savepoint
  // is gone, not just this one. Truncating to `ordinal` would have
  // `endCrank` release a `t0` the database lacks and throw over whatever
  // really killed the kernel.
  ctx.savepoints.length = 0;

The claim: a failed ROLLBACK TO leaves no transaction and therefore no
savepoints, so emptying ctx.savepoints keeps the in-memory list in step with
the database — and this is true of nodejs.ts and wasm.ts alike.

Why it matters

ctx.savepoints is not bookkeeping. Its length is the ordinal the next
savepoint is named by, and releaseAllSavepoints releases t0 iff it is
non-empty. If the list says zero while the database still has savepoints and an
open transaction, then the next crank creates a second t0 inside the surviving
transaction, and the invariant "empty stack ⇒ nothing to commit" — which is
exactly what commitIfNeeded keys on — is false. Every later write on the
connection joins a transaction nobody owns.

How to evaluate it

The claim is about the drivers, so read what each does when ROLLBACK TO throws.
Both are the same shape:

db._spStack.length = 0;
try {
  rollbackIfNeeded();
} catch (abortError) {
  logger?.error('failed to discard transaction after rollback', abortError);
}
throw error;

The transaction is discarded by rollbackIfNeeded, i.e. by a ROLLBACK TRANSACTION that is itself caught and logged if it fails. So the claim's
"⇒" is conditional on that abort succeeding — and the code, by catching, states
that it might not. (#1021 added exactly this logging, so the possibility is
acknowledged, not hypothetical.)

That gives a precise question: what state are we in when both the rollback and
the abort fail, and does anything later depend on it? Follow the consequence
rather than stopping at the state:

  • nodejs: db.inTransaction is read from SQLite, so it stays true.
    _spStack is []. beginIfNeeded sees the live transaction and skips
    BEGIN, so the next savepoint is created inside it; when that savepoint is
    released the stack empties and commitIfNeeded fires.
  • wasm: rollbackIfNeeded sets _inTx = false before stepping the abort,
    so a throwing abort leaves the flag false with a live transaction. The next
    beginIfNeeded issues a BEGIN that SQLite rejects.

Is there a "next savepoint" after the run loop has died? Yes — assertRunLoopAlive's
own doc comment says teardown must not be refused, and reset, terminateAllVats,
a peer incarnation change and a remote message all take savepoints.

nodejs.transaction-survival.test.ts drives the double failure and then does what
teardown does.

Outcome: claim is conditionally false — two failures.

The invariant holds whenever the compensating abort succeeds, which is the
overwhelmingly common case. When it does not:

  • nodejs issues COMMIT TRANSACTION on the transaction that was supposed to
    have been discarded, at the next teardown savepoint's release. The abandoned
    crank is committed. Silent.
  • wasm fails loudly at the next BEGIN.

Two drivers, opposite desynchronisations, only one of them noisy. So the second
half of the claim — "true for both drivers" — is where it breaks: they do not
degrade the same way, and the crank layer above them is written as if they do.

Severity, honestly

This needs a double I/O failure to reach, and after it the kernel is dead either
way. I would not block on it. But two things follow that are worth writing down:
the comment should say "discards the transaction if the abort succeeds", and
the divergence between drivers under the same fault is the same asymmetry claim 6
turns on, from the other end.

Interaction with claim 5

See that comment: reverting the caches on this path is safe in isolation, but
combined with the state above the caches say "rolled back" while the database
still holds the abandoned writes and may yet commit them.

@grypez

grypez commented Aug 31, 2026

Copy link
Copy Markdown
Member Author

Claim 4 — every kref in maybeFreeKrefs is there because this crank decremented it

What the claim is

As stated on #1021, where revertStateBeneathRollback ends with:

// Clearing all of them is correct only while a rollback discards the whole
// delivery, which is all any caller asks for.
ctx.maybeFreeKrefs.clear();

The justification for clear() is that the set is effectively per-crank: its
entries are collection candidates only because of decrements this crank made,
and the rollback just undid those decrements, so the entries are meaningless and
dropping them is not a loss.

Why it matters

maybeFreeKrefs is the only record that a kref might be collectable. Nothing
recomputes it — collectGarbage consumes and empties it, and krefs enter only by
being added at the moment of a decrement, a clearReachableFlag, or a c-list
teardown. A kref dropped from the set is not rediscovered later. It becomes an
object with no owner, no c-list entry, a zero refcount and no path to deletion.

And that leak is invisible to the auditor: auditRefCounts compares stored
counts against credited holders, and an orphan with zero holders and a stored
count of 0,0 matches. So this is the rare failure with no downstream signal at
all.

How to evaluate it

The claim is a universally-quantified statement about set membership, so it
falls to a single counterexample: a kref in maybeFreeKrefs that got there
outside this crank. Enumerate the producers (git grep 'maybeFreeKrefs.add'):

  • refcount.ts:164,180decrementRefCount
  • reachable.ts:105clearReachableFlag
  • vat.ts:267cleanupTerminatedVat
  • vat.ts:396forgetEndpointImports

and then ask, for each, whether it can run with no crank open. forgetEndpointImports
answers immediately: its only caller is RemoteHandle.persistPeerRestart, called
from RemoteManager.#handlePeerIncarnation, which runs from a network callback,
takes its own peerIncarnation_${peerId} savepoint, and calls no
collectGarbage. So the krefs it adds are committed-and-orphaned, sitting in the
set, waiting for whichever crank next harvests them.

Then: if that crank rolls back, are they still there?

crank.cross-crank-gc.test.ts orphans a remote export exactly as a peer restart
does, and runs cranks around it — one that succeeds (control), one that rolls
back, and a rollback followed by five clean cranks.

Outcome: false against #1021, true here. Three passes.

Against #1021's head the control passes and the other two fail: the orphan
survives the rollback, and no number of later cranks ever collects it. Permanent,
silent leak.

Against this branch all three pass. eaa71ac00 replaced the clear with a
snapshot-and-restore:

ctx.savepoints.push({ name, maybeFreeKrefs: new Set(ctx.maybeFreeKrefs) });
...
ctx.maybeFreeKrefs.clear();
for (const kref of restored.maybeFreeKrefs) {
  ctx.maybeFreeKrefs.add(kref);
}

and its comment states the corrected rule directly — "the set is not per-crank:
only collectGarbage empties it, so a candidate added while the run loop was
idle — terminateVat unpinning a root is the real path — is still owed a
collection and must survive an unrelated crank's rollback."

That is the right fix and it is already made. These three tests are offered as a
regression guard on it: they pass here and fail against #1021's head.

One thing to carry across

#1021 still has the clear(). Since these branches do not descend from one
another, whichever merges second needs the snapshot version, not the clear —
this is the mirror image of the wasm commitIfNeeded situation in claim 6, where
the fix exists on #1021 and is missing here.

Also worth pinning if you want belt and braces: #1021's own
crank.test.ts test 'reverts the caches the database cannot reach even when the rollback fails' seeds kp1 into maybeFreeKrefs before creating the
savepoints and asserts the set ends up empty — i.e. it asserts the pre-crank kref
is discarded. Under the new semantics that assertion is wrong, and it should be
updated to expect kp1 to survive rather than deleted.

@grypez

grypez commented Aug 31, 2026

Copy link
Copy Markdown
Member Author

Claim 5 — reverting the caches is safe on the failed-rollback path too

What the claim is

rollbackCrank calls revertStateBeneathRollback on both exits, and the failed
one is deliberate:

} catch (error) {
  ctx.savepoints.length = 0;
  // Before the rethrow, and not only on the path below. 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 these caches are at least as stale.
  revertStateBeneathRollback(restored, error);
  throw error;
}

The argument is monotonic: a failed rollback discards the whole transaction,
which is further back than the savepoint asked for, so caches built over the
abandoned crank are at least as stale as on the success path, and refreshing them
can only help.

Why it matters

This is the one place in the series that deliberately does work on the way out of
a fatal error. If it were wrong it would be wrong precisely when the operator has
least information — a database failure, a dying run loop, and now caches mutated
on the way past. The alternative (rethrow immediately) is the obvious-looking
choice, and the comment exists to say why it is worse: it would leave the dying
crank holding the GC action it consumed and the krefs it was about to collect.

How to evaluate it

The claim is a comparison, so enumerate the reachable database states after a
failed ROLLBACK TO and ask, for each, whether refreshing is worse than not:

  1. Rollback failed, abort succeeded (the normal case). The transaction is
    gone; the database is back past t0. Every cache re-read comes from committed
    state. Refreshing is not merely safe, it is mandatory — provideCachedStoredValue
    answers from a closure and writes through, so an unrefreshed closure would
    persist the abandoned value on its next set.
  2. Rollback failed, abort also failed (claim 3's hole). The database did
    not move back; the abandoned writes are still visible on this connection.
    refreshCachedValues re-reads them — the same values the caches already held.
    No change, so no harm.
  3. Partially rolled back. Not a state SQLite can produce; ROLLBACK TO is
    atomic.

So there is no state in which the refresh makes the caches worse. The restored
snapshot in state 2 is the interesting sub-case, but it restores
maybeFreeKrefs to a point in the past, which is conservative in the right
direction — see claim 4.

Outcome: claim holds. No test written; nothing to pin.

I could not construct a counterexample, and I do not think one exists at the
level the claim is pitched. The reasoning is sound and the comment states it
accurately.

The caveat worth recording

Claim 5 is safe about the caches. It is not a statement about the system, and
in state 2 above the combination is genuinely bad:

  • the caches have been reverted, so in-memory state says "this crank did not
    happen";
  • the database still holds the crank's writes in a live transaction, and per
    claim 3 the next teardown savepoint's release will commit them on the nodejs
    driver.

Caches say rolled back, disk says committed. Neither claim 3 nor claim 5 is
individually false, and the conjunction is a split brain. That is an argument for
fixing claim 3's hole (abort failure should be terminal for the connection, not
logged and stepped over), not for changing anything here.

One smaller note: because a failed delivery rollback discards the whole
transaction, writes the two-savepoint design deliberately places in crank are
lost on that path too — the crank savepoint buys nothing when delivery's
rollback fails. In #processCrankResult the ordering saves us: rollbackCrank
precedes #terminateVat, so a failed rollback throws before the worker is
killed, and the store is not left believing a dead vat is alive. Worth a comment
at the #terminateVat call, since the current one explains why its writes must
outlive the rollback without noting that on the failed path they do not exist yet.

@grypez

grypez commented Aug 31, 2026

Copy link
Copy Markdown
Member Author

Claim 6 — commitIfNeeded clears _inTx before the COMMIT; wasm clears before the abort

What the claim is

Two related driver changes. In wasm.ts, _inTx is cleared before the
statement it guards, in both commitIfNeeded and rollbackIfNeeded, with the
reason given at rollbackIfNeeded:

// Cleared before the abort, which can throw: left true, `beginIfNeeded` is
// a no-op forever after and writes autocommit one statement at a time (see
// `createSavepoint`).
db._inTx = false;

The claim: clearing first is strictly better, because a throwing statement cannot
then wedge the flag true and turn beginIfNeeded into a permanent no-op —
after which savepoints are created bare, their RELEASE autocommits
(Agoric/agoric-sdk#8423), and no rollback can undo a delivery.

Why it matters

_inTx is the wasm driver's entire model of whether a transaction exists. Both
commitIfNeeded and rollbackIfNeeded gate on it, so if it disagrees with
SQLite, one of two things happens: the driver thinks there is a transaction when
there is none (every commit/abort throws), or it thinks there is none when there
is (nothing will ever end it, and every write joins it and reports success).
nodejs.ts has no such flag, reading db.inTransaction from SQLite, and #1021's
b90e7a5e5 cites that as the reason the two drivers differ — filed as #1013.

How to evaluate it

The falsification to look for is the other direction: a throw between the clear
and the statement, leaving _inTx wrong the opposite way.

Clearing first closes the wedge and opens the reverse hole, and that hole is
reachable without any exotic assumption: SQLite can fail a COMMIT with the
transaction still open.
Trace wasm.releaseSavepoint on this branch:

  1. db.exec('RELEASE SAVEPOINT t0') succeeds
  2. _spStack.splice(idx) empties the stack
  3. commitIfNeeded()_inTx = false, then COMMIT TRANSACTION throws

Now _inTx is false with a live transaction. rollbackIfNeeded reads the false
flag and returns. commitIfNeeded will not run again until some savepoint release
empties the stack. And note step 3 is reached from outside the try/catch that
wraps the RELEASE, so nothing catches it there either. The transaction has no
owner — the exact hazard rollbackSavepoint and releaseSavepoint already
discard the transaction to avoid, reached by a third door.

So the evaluation is: for each driver, does commitIfNeeded leave a transaction
behind when the COMMIT fails? wasm.transaction-survival.test.ts and the second
case of nodejs.transaction-survival.test.ts assert that an abort follows.

Outcome: both drivers fail. Two failures, and the fix already exists on #1021.

That second one contradicts b90e7a5e5's stated reason for treating this as
wasm-only:

The nodejs driver reads db.inTransaction from SQLite rather than caching it,
so it has no equivalent gap; that asymmetry is #1013.

Reading the flag from SQLite prevents the wedge. It does not prevent the
ownerless transaction, which is the half that loses data — and on nodejs it is
worse, because inTransaction staying honest is exactly what lets the next
teardown savepoint join the doomed transaction and COMMIT it (see claim 3).

So: the claim is true of wasm as written on #1021, false of wasm as it stands
here, and the parenthetical about nodejs is false everywhere.

What to do

  1. Carry b90e7a5e5 onto this branch, or land fix: keep a crank's store work inside one transaction #1021 first and rebase. Whichever
    merges second needs it.
  2. Give nodejs.commitIfNeeded the same treatment wasm's got, and drop the
    "no equivalent gap" note from the changelog and Extract the duplicated sqlite savepoint methods into one module #1013.
  3. Consider whether the flag is worth keeping at all. nodejs gets by without
    one, and a driver that asks SQLite cannot disagree with it.
    sqlite3_get_autocommit is exported by the wasm build
    (@sqlite.org/sqlite-wasm/sqlite-wasm/jswasm/sqlite3.mjs) though it is absent
    from the package's index.d.ts, so this would need a typing shim rather than
    just a call — worth weighing against carrying _inTx correctly forever.

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