Skip to content

Comments and changelog entries in the #1021–#1023 stack that the code does not support #1076

Description

@sirtimid

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

  1. 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 by withStoreOutOfCrank and that the production callers "take their turn through the new beginOutOfCrank/endOutOfCrank". All four should say withStoreOutOfCrank.
  2. 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.
  3. RemoteManager.#handlePeerIncarnation does not exist (two references in crank.cross-crank-gc.test.ts); the method is #handleIncarnationChange.
  4. "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.
  5. "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.
  6. 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.
  7. "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.
  8. "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.
  9. "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.

#1022

  1. 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.
  2. 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.
  3. "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.
  4. 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.
  5. "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.
  6. "stopVat only tears down the worker" — it also releases the root pin, a store write, and this call site passes terminating = true.
  7. 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.
  8. 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

  1. "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.
  2. "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.)
  3. "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.
  4. 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.
  5. "Both halves are needed" justifies only the isVatActive half of isVatActive && !isVatTerminated.
  6. 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.
  7. 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.
  8. "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.
  9. 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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions