Repository navigation
Comments and changelog entries in the #1021–#1023 stack that the code does not support #1076
Copy link
Copy link
Closed
Description
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.- The
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.- "Only a failed RELEASE discards the savepoint stack" — a failed
COMMITdoes it too, viareleaseSavepoint→commitIfNeeded→discardTransaction('commit'), and that path is new in this PR.assertNotAbandonedis a third way. - "the next
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 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. - "
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. - "the run loop is otherwise synchronous from
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.- The ownership guard's counterfactual overstates. "without this a vat could disown an object belonging to a different, live vat" —
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. - "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.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.- "This is the check standing in for the accounting invariant
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. - 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.
#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. - "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
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.) - "by the time a waiter is told the vat is gone, the store now says so", at three sites —
#trackFluxrecordsstarted.catch(() => undefined), so a teardown that fails before#retireVatcompletes still resolves the flux promise andprovideVatthrows 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" —deleteVatnever touches the c-list. - "Both halves are needed" justifies only the
isVatActivehalf ofisVatActive && !isVatTerminated. - A pre-existing comment in
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. - Two of three stated consequences never happen in the
#trackFluxwrapping comment: theFailthrows before anything is returned, so the result cannot "collapse toundefined" and the destructure never runs. - "Never rejects" on two teardown JSDoc blocks, against a
#startFailedVatTeardownthat catches"Unexpected failure tearing down vat". The catch is right; the prose is wrong. - A changelog entry links issue retireKernelObjects never notifies remote importers, leaving a dangling c-list entry #1015 instead of the PR, against
docs/contributing/updating-changelogs.md.
Volume
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.
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
No labels
Activity
sirtimid commentedon Sep 15, 2026
Closing: the comments and changelog entries this lists belong to the stack, which is being split into small PRs against main with prose rewritten per PR.