You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Collected from a comment-accuracy pass over #1021, #1022 and #1023. Filed rather than fixed in place, to keep those PRs reviewable. Each is a claim a reader would act on that the code contradicts.
beginOutOfCrank is named as the caller-facing API in four places, but is private to crank.ts. Worst of them is a runtime message: createSavepoint ${q(name)} inside a crank; use beginOutOfCrank — an operator who hits it is told to call a function that is not on the store. Also store/index.ts's JSDoc, nodejs.savepoint-interleaving.test.ts, and CHANGELOG.md, which says both that the pair is replaced bywithStoreOutOfCrank and that the production callers "take their turn through the new beginOutOfCrank/endOutOfCrank". All four should say withStoreOutOfCrank.
The BREAKING marker on the withStoreOutOfCrank changelog entry is wrong.beginOutOfCrank/endOutOfCrank do not exist on main — they were introduced and removed inside fix: keep a crank's store work inside one transaction #1021, so no consumer can break.
RemoteManager.#handlePeerIncarnation does not exist (two references in crank.cross-crank-gc.test.ts); the method is #handleIncarnationChange.
"Only a failed RELEASE discards the savepoint stack" — a failed COMMIT does it too, via releaseSavepoint → commitIfNeeded → discardTransaction('commit'), and that path is new in this PR. assertNotAbandoned is a third way.
"the next set would write it straight back" — provideCachedStoredValue.set writes the new value. The actual hazard is the read side: get() returns the abandoned value, so a read-modify-write built on it persists something derived from state the rollback undid.
kernel-store/CHANGELOG.md describes a latch main cannot produce. On main the wasm driver has no refusal mechanism at all; txAbandoned is introduced by this same PR and the caching fix repairs it. As written, a consumer is told a shipped bug was fixed.
"SQLite ends the transaction after SQLITE_FULL/SQLITE_IOERR/SQLITE_BUSY" — SQLite documents this as may, which is precisely why the driver has to ask rather than assume.
"all refuses a statement that returns no rows" — better-sqlite3's .all() refuses a statement that returns no data (a non-readonly statement). A SELECT matching zero rows is fine.
"the run loop is otherwise synchronous from endCrank to the next startCrank" omits the await wakeUpPromise in the run loop's finally. KernelQueue.ts states it correctly; the crank.ts copy does not.
forgetEndpointImports is described backwards, in both a comment and a changelog bullet: "keeps only entries whose direction is export". It deletes the export-direction entries and keeps the imports. The conclusion drawn (a retireImports is not reconciled) is right; the reason is inverted.
The ownership guard's counterfactual overstates. "without this a vat could disown an object belonging to a different, live vat" — forgetKref touches only the calling endpoint's c-list, and orphanKernelObject already Fails on a mismatched owner. Without the guard the outcome is that the caller's own import entry is torn down and the crank aborts with a misleading error.
"the audit could not see the damage, because an export entry carries no count" — in the covered scenario the entry destroyed is an import entry, which does carry a count. The audit stays quiet because the teardown decrements consistently.
refcount-audit.ts contradicts the dangling kind this same PR adds. "a c-list entry that outlives what it names is the case that matters" is exactly the new dangling violation. The genuinely invisible case is the opposite one.
"This is the check standing in for the accounting invariant collectGarbage still cannot assert" — the audit credits a c-list entry only when direction === 'import', so the owner's reachable flag on its own export entry is invisible to it by construction.
"stopVat only tears down the worker" — it also releases the root pin, a store write, and this call site passes terminating = true.
The sort comment names the wrong throw site (the throw came from processGCActionSet, the caller) and says "two actions" where it takes two surviving krefs.
A changelog bullet claims a sequence that was not reachable before this PR ("the store went on to wipe the vat's state") — the wipe needs markVatAsTerminated, which every pre-existing call site skips on a throw.
"stopVat … can refuse before it gets that far" describes an unreachable path: the catch is entered only after runVat resolved, and runVat ends by setting the handle. The mark is load-bearing, for a sharper reason — #retireVat can throw part-way at deleteVat for a vat launched into no subcluster.
"The vat is only ever out of reach inside the crank that carries the request out" is false, and is the thing Bugbot found: between stopVat and runVat, every non-crank caller sees the vat as missing. (Fixed in the pushed commit; the comment and the matching changelog line "so a vat is never out of the kernel's reach while cranks run" still overclaim — what holds is that no crank can observe it.)
"by the time a waiter is told the vat is gone, the store now says so", at three sites — #trackFlux records started.catch(() => undefined), so a teardown that fails before #retireVat completes still resolves the flux promise and provideVat throws with the store still calling the vat active.
Three ordering comments inside #retireVat, which is wholly synchronous and cannot interleave. "First, while the c-list this reads through is still there" — deleteVat never touches the c-list.
"Both halves are needed" justifies only the isVatActive half of isVatActive && !isVatTerminated.
A pre-existing comment in Kernel.ts made false by this PR: "Bypass VatManager.terminateVat() here because it calls waitForCrank(), which would deadlock". It no longer calls waitForCrank. The PR added a correct replacement immediately below; the stale two lines should go.
Two of three stated consequences never happen in the #trackFlux wrapping comment: the Fail throws before anything is returned, so the result cannot "collapse to undefined" and the destructure never runs.
"Never rejects" on two teardown JSDoc blocks, against a #startFailedVatTeardown that catches "Unexpected failure tearing down vat". The catch is right; the prose is wrong.
Separately from accuracy: 719 of 2335 added lines in #1023 are comment or JSDoc (31%), and in VatManager.ts it is 240 of 422 (57%). Two arguments are each stated eight times across the diff — "the restart is queued so the run loop does it" and "store-says-dead / kernel-says-live kills the run loop". Per .claude/skills/prose-pass/SKILL.md's "say it once", each should survive at exactly one site.
Collected from a comment-accuracy pass over #1021, #1022 and #1023. Filed rather than fixed in place, to keep those PRs reviewable. Each is a claim a reader would act on that the code contradicts.
#1021
beginOutOfCrankis named as the caller-facing API in four places, but is private tocrank.ts. Worst of them is a runtime message:createSavepoint ${q(name)} inside a crank; use beginOutOfCrank— an operator who hits it is told to call a function that is not on the store. Alsostore/index.ts's JSDoc,nodejs.savepoint-interleaving.test.ts, andCHANGELOG.md, which says both that the pair is replaced bywithStoreOutOfCrankand that the production callers "take their turn through the newbeginOutOfCrank/endOutOfCrank". All four should saywithStoreOutOfCrank.BREAKINGmarker on thewithStoreOutOfCrankchangelog entry is wrong.beginOutOfCrank/endOutOfCrankdo not exist onmain— they were introduced and removed inside fix: keep a crank's store work inside one transaction #1021, so no consumer can break.RemoteManager.#handlePeerIncarnationdoes not exist (two references incrank.cross-crank-gc.test.ts); the method is#handleIncarnationChange.COMMITdoes it too, viareleaseSavepoint→commitIfNeeded→discardTransaction('commit'), and that path is new in this PR.assertNotAbandonedis a third way.setwould write it straight back" —provideCachedStoredValue.setwrites the new value. The actual hazard is the read side:get()returns the abandoned value, so a read-modify-write built on it persists something derived from state the rollback undid.kernel-store/CHANGELOG.mddescribes a latchmaincannot produce. Onmainthe wasm driver has no refusal mechanism at all;txAbandonedis introduced by this same PR and the caching fix repairs it. As written, a consumer is told a shipped bug was fixed.SQLITE_FULL/SQLITE_IOERR/SQLITE_BUSY" — SQLite documents this as may, which is precisely why the driver has to ask rather than assume.allrefuses a statement that returns no rows" — better-sqlite3's.all()refuses a statement that returns no data (a non-readonly statement). ASELECTmatching zero rows is fine.endCrankto the nextstartCrank" omits theawait wakeUpPromisein the run loop'sfinally.KernelQueue.tsstates it correctly; thecrank.tscopy does not.#1022
forgetEndpointImportsis described backwards, in both a comment and a changelog bullet: "keeps only entries whose direction isexport". It deletes the export-direction entries and keeps the imports. The conclusion drawn (aretireImportsis not reconciled) is right; the reason is inverted.forgetKreftouches only the calling endpoint's c-list, andorphanKernelObjectalreadyFails on a mismatched owner. Without the guard the outcome is that the caller's own import entry is torn down and the crank aborts with a misleading error.refcount-audit.tscontradicts thedanglingkind this same PR adds. "a c-list entry that outlives what it names is the case that matters" is exactly the newdanglingviolation. The genuinely invisible case is the opposite one.collectGarbagestill cannot assert" — the audit credits a c-list entry only whendirection === 'import', so the owner's reachable flag on its own export entry is invisible to it by construction.stopVatonly tears down the worker" — it also releases the root pin, a store write, and this call site passesterminating = true.processGCActionSet, the caller) and says "two actions" where it takes two surviving krefs.markVatAsTerminated, which every pre-existing call site skips on a throw.#1023
stopVat… can refuse before it gets that far" describes an unreachable path: the catch is entered only afterrunVatresolved, andrunVatends by setting the handle. The mark is load-bearing, for a sharper reason —#retireVatcan throw part-way atdeleteVatfor a vat launched into no subcluster.stopVatandrunVat, every non-crank caller sees the vat as missing. (Fixed in the pushed commit; the comment and the matching changelog line "so a vat is never out of the kernel's reach while cranks run" still overclaim — what holds is that no crank can observe it.)#trackFluxrecordsstarted.catch(() => undefined), so a teardown that fails before#retireVatcompletes still resolves the flux promise andprovideVatthrows with the store still calling the vat active.#retireVat, which is wholly synchronous and cannot interleave. "First, while the c-list this reads through is still there" —deleteVatnever touches the c-list.isVatActivehalf ofisVatActive && !isVatTerminated.Kernel.tsmade false by this PR: "Bypass VatManager.terminateVat() here because it calls waitForCrank(), which would deadlock". It no longer callswaitForCrank. The PR added a correct replacement immediately below; the stale two lines should go.#trackFluxwrapping comment: theFailthrows before anything is returned, so the result cannot "collapse toundefined" and the destructure never runs.#startFailedVatTeardownthat catches"Unexpected failure tearing down vat". The catch is right; the prose is wrong.docs/contributing/updating-changelogs.md.Volume
Separately from accuracy: 719 of 2335 added lines in #1023 are comment or JSDoc (31%), and in
VatManager.tsit is 240 of 422 (57%). Two arguments are each stated eight times across the diff — "the restart is queued so the run loop does it" and "store-says-dead / kernel-says-live kills the run loop". Per.claude/skills/prose-pass/SKILL.md's "say it once", each should survive at exactly one site.