fix(ocap-kernel): make c-list import accounting symmetric - #1010
Open
sirtimid wants to merge 12 commits into
Open
fix(ocap-kernel): make c-list import accounting symmetric#1010sirtimid wants to merge 12 commits into
sirtimid wants to merge 12 commits into
Conversation
sirtimid
added a commit
that referenced
this pull request
Aug 5, 2026
- Drop the refCountScheme migration: no production stores exist with the old counting scheme, so the recompute-on-open path is dead code - Update changelog PR links from #1006 (issue) to #1010 (this PR) - Fix changelog formatting: add blank lines before sub-bullets of the @@name and 'Fix the stale cle./clk.' entries to satisfy auto-changelog --prettier validation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
sirtimid
marked this pull request as ready for review
August 5, 2026 16:40
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
sirtimid
marked this pull request as draft
August 5, 2026 17:16
sirtimid
marked this pull request as ready for review
August 5, 2026 17:25
This was referenced Aug 6, 2026
grypez
reviewed
Aug 6, 2026
grypez
left a comment
grypez
left a comment
Member
There was a problem hiding this comment.
Reviewed the accounting change and the two judgment calls. The core fix reads as correct to me, and the checker-first ordering clearly earned its keep. Both judgment calls are sound; my notes below are on the reasoning around them, not the decisions.
One inline comment on a stale invariant claim, plus the notes here. Everything else I found is pre-existing rather than introduced by this PR, and I've written those up as separate issues rather than pile them onto this diff — links at the end.
The disabled gc.ts assert
I traced this and agree. At gc.ts:210-216, when the last holder drops and retires in one crank, clearReachableFlag takes reachable to 0 and forgetKref takes recognizable to 0 before collectGarbage runs, while the owner's own flag is untouched until the first delivery — so both actions get queued with vatConsidersReachable === true and recognizable === 0, exactly the assert's negation. Leaving it off and replacing the stale TODO with the reason is the right call.
The thing worth drawing out: "The audit is what validates the accounting now" makes the audit load-bearing for correctness, while the Kernel.make JSDoc scopes it as "intended for tests and debugging." Those pull in different directions, and the coverage suggests the first framing is currently ahead of the artifact:
File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
refcount-audit.ts | 95.57 | 89.65 | 100 | 95.57 | 156,207-210
Line 156 is credit(message.result, …) — no test queues a message with a non-null result. Lines 207-210 are the entire promise-queue branch; nothing in the audit tests calls enqueuePromiseMessage. And drift is asserted in both directions for only one of the eight credit sources (c-list import, refcount-audit.test.ts:124-162); the other seven are exercised only in the "audit is clean" direction, so six of the rules could be off by a constant and the suite would stay green. Since this is the artifact inheriting the assert's job, per-credit-source drift coverage seems worth having before it carries that weight.
Settled promises' c-list entries
The UI constraint is a real product call and I'm not arguing with it; the TODO states the cost accurately. But one inference in the description doesn't hold:
and the audit is green without it
The audit is green here by construction, not as evidence. The retained c-list entry is itself a credited holder (refcount-audit.ts:179-186), so the stored count and the recomputed count agree — and they would agree at any value, as long as an entry exists to justify it. The auditor's ground truth is the holder set, so it can detect a count that disagrees with a holder but structurally cannot detect a leaked holder. Worth knowing precisely because this is the one leak the PR knowingly retains.
Same reason the CHANGELOG line reads broader than the behaviour: "counts too high with no holder (a leak)" catches an orphaned count, not an orphaned reference. Might be worth a sentence in the audit's doc comment saying which of the two it finds.
Follow-ups filed separately
Three things I believe are pre-existing and out of scope here, written up with reproductions so they can be judged independently:
- #1015 —
retireKernelObjectsnever notifies remote importers, leaving a dangling c-list entry. Latent today; the topology is not covered bykernel-test, so it does not contradict the clean-audit claim in the description. - #1016 — a throw inside a crank commits the partial crank rather than rolling it back. Identical
try/finallyshape onmain; this PR only adds one new throw source. - #1017 — GC deliveries to remotes carry rrefs in the sender's frame, so the receiver mints a phantom object and the action has no effect. Also pre-existing; this PR's new
isVatIdcatch actually improves the surrounding failure handling.
| // A vat is local and reliable, so a refusal means it is broken. Undo the | ||
| // teardown rather than commit it: leaving the two disagreeing would have | ||
| // the vat mint fresh krefs for objects the kernel thinks it let go of. | ||
| // Aborting restores the entries and the action; terminating the vat is |
Member
There was a problem hiding this comment.
rollbackCrank doesn't restore the consumed GC action, so this comment's second clause is inverted.
Aborting restores the entries and the action; terminating the vat is what stops that restored action from being retried forever.
The entries, yes — the DB rollback covers those. The action, no. gcActions is a provideCachedStoredValue (store/index.ts:147), which keeps the value in a closure and writes through to kv (base.ts:98-117). rollbackCrank (crank.ts:44-62) rolls the database back and then refreshes only the run queue. The gcActions closure still holds the post-processGCActionSet value, so the reduced set wins and the next set persists the loss.
Reproduction, against a real store:
AssertionError: expected [] to strictly equal [ 'v1 dropExport ko1' ]
reapQueue is cached the same way and behaves the same way; that exposure is pre-existing.
So the causality is the other way round from what the comment says: terminating the vat isn't what stops the restored action being retried — it's what makes losing the action harmless, because the action was going to a vat that no longer exists. Since every abort this function returns is paired with terminate, there's no live bug. I'm flagging it because the comment is the thing a future reader will trust when they add an abort path that isn't paired with a termination.
Fix is one line: re-provide both cached values in rollbackCrank, as reset() already does at store/index.ts:217-218. I have the failing test written and can hand it over.
Adjacent, same function: rollbackCrank doesn't clear ctx.maybeFreeKrefs either, which store/index.ts:140-144 states as an invariant. The GC rollback paths happen to survive it because collectGarbage re-reads counts, but gc.ts:161 getKernelPromise throws for a promise a rollback deleted.
Creating an import c-list entry changed no refcount while tearing one
down decremented both, and `initKernelObject` compensated by minting
every object at (1, 1). That constant is correct for exactly one
importer, which is why nothing caught it: with two importers a live
capability gets dropped and retired out from under a holder, and the
same unit is claimed by both an importer's drop and the owner's
termination, so cleanup underflows and leaves a vat half-cleaned.
Restore the increment and rebase the baseline to (0, 0), matching
SwingSet, so `collectGarbage` — already a faithful port — receives the
inputs it was written for.
Build the invariant checker first, since every existing compensation
becomes a double-count the moment the increment lands. It recomputes
each kref's counts from ground truth (c-list entries and their reachable
flags, run-queue and promise-queue messages, promise resolution values,
pins) and reports drift in both directions: too low collects a live
capability, too high leaks it. Enabled via `Kernel.make`'s
`auditRefCounts` and run after every crank; on in kernel-test.
The audit found four more unbalanced paths that the phantom baseline had
been absorbing, each fixed here: a delivered message charged its target
against the routed kref rather than the run-queue item's own, so a
message routed through a resolved promise decremented an object nobody
charged and leaked the promise; a notification leaked its reference on
both early-return paths and decremented promises retired alongside it
that nobody had taken; a message queued on an unresolved promise
duplicated every reference it carried on re-enqueue; and `resolve|kpid`
incremented with no matching release.
Two things the baseline was silently standing in for, now explicit: vat
roots are pinned for the lifetime of their vat (a root is addressable
whether or not anyone imports it), and GC action delivery moves the
kernel's own c-list so a dropped export's flag clears and retired
entries don't outlive their objects.
Also fixes the stale `cle.`/`clk.` key prefixes in
`getPromisesByDecider` and `deleteEndpoint`, which stopped matching the
`${endpointId}.c.` layout. `getPromisesByDecider` matched nothing, so
promises a terminating vat was deciding were never rejected — load
bearing here, because releasing a promise's unsettled reference is what
makes the cleanup path's accounting add up.
Refcounts are persisted, so counts written under the old scheme are
recomputed from ground truth on first open, keyed off a new
`refCountScheme` entry.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Drop the refCountScheme migration: no production stores exist with the old counting scheme, so the recompute-on-open path is dead code - Update changelog PR links from #1006 (issue) to #1010 (this PR) - Fix changelog formatting: add blank lines before sub-bullets of the @@name and 'Fix the stale cle./clk.' entries to satisfy auto-changelog --prettier validation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Follow-up to the c-list accounting fix, addressing defects found in review. An owner that stops naming its own export left the object behind. Both the delivered `retireExport` and the `retireExports`/`abandonExports` syscalls tore down the owner's c-list entry but left `owner` and `refCount` in place, with no path that could ever reclaim them: `cleanupTerminatedVat` finds krefs by walking the owner's c-list, and the collector only revisits krefs in `maybeFreeKrefs`. The records leaked, and the next collection to visit such a kref read the owner's deleted entry through `getRequired` and took the run loop down with it. New `orphanKernelObject` drops the owner mapping and hands the object to the collector, which already knows how to retire an orphan. `collectGarbage` also treats an owner with no c-list entry as orphaned rather than trusting the mapping. Reporting a dead run loop belongs to #1005, which landed on main first. It is what makes the audit usable at all: `assertRefCountsIfAuditing` throws from inside a crank, so with the failure logged and swallowed a violation's sole symptom was a test hanging to its timeout with no mention of reference counts. The `kernel-test` case here asserts that shape — the caller is told the run loop died, and the audit error rides along as the `cause`. Also: GC action delivery survives a vanished endpoint or a failed delivery instead of stopping the loop; `launchVat` tears down a worker whose kernel-side registration failed rather than stranding it; `RefCountViolation` discriminates on `kind` instead of sentinel-matching `stored`; and the store context's auditing flag no longer shares a name with `auditRefCounts()`. Tests cover the crash path, the orphan-and-collect sequence, retiring stragglers, GC-action robustness, and that a violation reaches a caller. The `item.target` charge and both `deliver|notify` early returns now have assertions that fail if the fix is reverted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review of the previous commit found that four of the five error handlers it added turned a crash into a state the kernel can no longer detect. Corrects that, and closes a hole the orphaning opened. `orphanKernelObject` took an object's owner mapping on trust. Nothing upstream of `performExportCleanup` checks that the vref it was handed is even an export — `translateSyscallVtoK` maps both directions alike — so a vat could pass an import to `abandonExports`, which needs no precondition at all, and erase a different live vat's claim to an object it was still exporting. Sends to that object then went splat with OBJECT_DELETED, terminating the victim tripped `cleanupTerminatedVat`'s ownership assertion and took the run loop with it, and the audit could not see any of it, because an export entry carries no count. Disowning is now the owner's own doing: the expected owner is a required argument and must match, and the syscall path rejects a mismatch outright. The vanished-endpoint catch returned before the teardown, but `processGCActionSet` had already consumed the action, so neither the kernel nor the durable set remembered the object — a permanent leak, also invisible to the audit. The kernel's side is now released whether or not anyone is left to tell, and krefs whose entries a cleanup already removed are skipped rather than assumed present. The delivery-failure catch committed the teardown after the endpoint had failed to hear about it, so the endpoint would go on to mint a fresh kref for an object the kernel believed it had let go of — the same object with two identities. It now aborts, which restores both the entries and the action, and terminates the vat that could not accept the delivery. `launchVat`'s cleanup path stopped the worker without marking the vat terminated, so nothing ever reclaimed the records a partial launch had written. The audit counted an importer's c-list entry as a holder during the window between `retireKernelObjects` deleting an object and delivering the matching `retireImport`, so the collector's own output failed the end-of-crank check. The missing assertion in the test covering that sequence is now present. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…very Aborting a failed GC delivery restores the action to the durable set, and `processGCActionSet` is consulted ahead of all other run-queue work. For a vat that is fine, because terminating it is what stops the restored action from coming back. A remote cannot be terminated, so the same item would be selected every crank and nothing else would ever run. A remote is a separate kernel across a link that can drop messages anyway, and it reconciles on the next incarnation change, so its failures no longer abort. Also stop `orphanKernelObject` throwing on an object that is already orphaned. Disowning something nobody owns is a no-op, not an error: only a mismatch with a different, live owner is, which is the case the check exists for. Same for the syscall path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lback A database rollback cannot reach two pieces of state, so `rollbackCrank` now reverts both itself. Every `provideCachedStoredValue` answers reads from a closure and only writes through to kv. Reverting the database therefore left the closure holding the abandoned crank's value, and the next `set` persisted it. `processGCActionSet` takes an action out of the set before delivering it, so an aborted delivery lost the action outright rather than retrying it. `reapQueue` was exposed the same way. `maybeFreeKrefs` lives in RAM, so nothing reverted it either. Its entries are collection candidates only because of the decrements the rollback undid, and a later `collectGarbage` threw outright on a promise the rollback had deleted, killing the run loop. No live bug either way: every `abort` `#deliverGCAction` returns is paired with a `terminate`, which is what made losing the action harmless. The comment there claimed the rollback restored the action, which is the thing a future reader would trust when adding an abort path that isn't paired with a termination; it now states the real causality. The cached values are declared once so that initialization and the refresher cannot disagree about which ones exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The clean-audit cases prove each rule agrees with whatever the store did, which stays true if a rule and the code it mirrors are wrong by the same constant. Six of eight rules could have drifted and the suite would have stayed green. Each of the ten credit sources now pins its count and holder labels to literals and asserts drift in both directions: too low collects a live capability, too high leaks it. That closes the two coverage gaps as a side effect — a run-queue send's result promise, and a message parked on an unresolved promise, neither of which any test reached. Also states what the audit can and cannot find, which matters because its ground truth *is* the holder set: a count that disagrees with its holders is caught either way, but a holder that should have been torn down and wasn't justifies its own count at any value, so a leaked reference is invisible to it by construction. That is exactly the case the retained settled-promise c-list entry leaves behind, so the CHANGELOG no longer claims the audit would catch it. The `auditRefCounts` JSDoc no longer scopes the option as "intended for tests and debugging": it stands in for the invariant `collectGarbage` cannot assert, and is off by default only because it walks the whole store. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s with Releasing the kernel's side of a garbage-collection action when the endpoint has vanished is right for an endpoint that is gone, and wrong for one that is merely out of reach. `restartVat` keeps the vat's c-list and takes the vat out of the kernel's vat table for as long as launching a worker and negotiating with it takes, so a GC action selected in that window found the vat absent, released entries the returning incarnation still holds, and committed — leaving the vat free to mint fresh krefs for objects the kernel thinks it let go of. That is the same divergence the failed-delivery path below rolls back to avoid. The endpoint is now resolved before anything is torn down, so the outcome is decided rather than discovered halfway through, and the release commits only where the endpoint is genuinely gone: a vat the store has marked terminated, whose cleanup tears the whole c-list down regardless, or a remote, which reconciles on its next incarnation. A vat that is absent yet not terminated fails the crank instead, which is what this path did before the release was added to it. This does not make a vat restart safe, and is not trying to: it stops the GC path from turning that window into silent corruption. The window itself needs the vat to stop being unreachable while it restarts — `restartVat` is an RPC handler mutating kernel state alongside a running run loop, which a send already resolves as a splat and a `notify` already dies on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sirtimid
force-pushed
the
sirtimid/clist-import-refcount
branch
from
August 10, 2026 11:41
6618411 to
988411e
Compare
…t as gone `restartVat` keeps the vat's c-list and takes the vat out of the kernel's vat table for as long as launching a worker and negotiating with it takes. Absence from that table was the only signal available, so a crank landing in the window resolved a live vat as a dead one: a message went splat, a `notify` or `bringOutYourDead` took the run loop down, and a garbage-collection action released the kernel's side of entries the returning incarnation still holds. The vat's flux is now recorded rather than guarded against. `provideVat` waits on that record, so a crank arriving mid-restart delivers to the new incarnation, and the kernel's endpoint lookup is asynchronous to let it wait. The crank waits for the vat, rather than the restart waiting for the run loop — which is the same direction SwingSet takes it, where a delivery to an evicted vat awaits `ensureVatOnline` and eviction is routine. Inverted the other way, as a lock the restart holds while the loop stands still, whatever holds it must never await anything the loop has to deliver, and `runVat` is exactly that kind of await. The wait for the crank in flight stays ahead of the record, which is load-bearing: record first and wait after, and a crank that is already running reaches its endpoint lookup, finds the record, and waits for a restart that is waiting for that crank to end. What the ordering leaves open is a crank the run loop starts in the turn between the wait resolving and the record appearing — it takes the outgoing handle and can still be mid-delivery when the worker goes down. Closing that needs the restart to happen inside a crank, the way `processUpgradeVat` does upstream, where the vat is idle by construction and nothing mutates kernel state from outside the run loop. A relaunch that fails now marks the vat terminated. It previously left a vat with no worker that the store still counted among the living, which nothing revisits: `cleanupTerminatedVat` only walks vats that are marked. The GC action guard for a vat that is absent but not terminated stays, now as an assertion rather than a live path, with its reasoning corrected: aborting the crank does preserve the action, since `rollbackCrank` restores the cached GC set, but nothing about the vat changes between cranks, so the action would be re-selected and re-aborted forever with no delivery to wait on. Also shortens this PR's CHANGELOG entries, which had grown to carry rationale that belongs in these messages. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…for endpoints that are gone Restarting a vat alongside a running run loop cannot be made safe by ordering alone. The previous approach recorded the vat as mid-flux so a delivery would wait for the new incarnation, and the record had to be installed *after* waiting out the crank in flight — install it before, and a crank that is already running reaches its endpoint lookup, finds the record, and waits for a restart that is waiting for that crank to end. That ordering left a turn of its own: a crank the run loop starts between the wait resolving and the record appearing takes the outgoing handle, and can be mid-delivery when the worker goes down. So the restart is now the run loop's own work, as a queued `restartVat` item, the way SwingSet queues `upgrade-vat` for `processUpgradeVat`. In a crank of its own there is no window to close: the run loop is the only thing that delivers, and it is here instead, so the vat is idle by construction. `Kernel.restartVat` settles when the crank has done it, and refuses outright if the run loop is dead, since nothing would ever carry the request out. Termination keeps the flux record, because it cannot be queued: `reset` and `clearStorage` tear vats down on kernels whose run loop has died. Both of its steps now live inside `#trackFlux`, in the order that does not deadlock, so a caller does not sequence them and cannot get them wrong — with a test that hangs if the order is reversed. Two more, found in review of the previous round: `#deliverNotify` and `#deliverBringOutYourDead` awaited the endpoint with no handling for one that has vanished, so a crank landing during a termination took the rejection into the run loop and killed it. This predates the wait — the lookup used to throw synchronously in the same case — but the wait is what makes it routine. All three of notify, reap, and GC-action delivery now go through `#resolveEndpoint`, which drops the work for an endpoint that is gone for good (a terminated vat, or a remote) and propagates anything else. The notify resolves its endpoint before translating, which would otherwise mint c-list entries for an endpoint with no way to hear about them. A relaunch that failed marked the vat terminated but left its root pinned: `stopVat` releases that pin only when it is the one ending the vat, and it had been told the vat was coming back, while vat cleanup does not touch pins at all. The pin, and the root's refcount, were held for the life of the kernel. Both paths now release it through one helper. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| settle?.reject(error); | ||
| throw error; | ||
| } | ||
| settle?.resolve(); |
There was a problem hiding this comment.
Failed restart mark is rolled back
High Severity
performVatRestart marks the vat terminated and unpins the root on relaunch failure, then throws so that mark will stick. Moving the restart into a crank defeats that: the run loop’s catch path always calls rollbackCrank, which undoes both writes. The worker is already gone in RAM, so the store still lists a live vat with no worker and a held root pin. A permanently unloadable vat can then make initializeAllVats fail on the next process start.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit f5fbb4e. Configure here.
| // Not queued for the run loop the way `restartVat` is: teardown has to work | ||
| // on a kernel whose run loop has died, which `reset` and `clearStorage` | ||
| // depend on. So this one closes its window with a flux record instead. | ||
| await this.#trackFlux(vatId, async () => this.#endVat(vatId, reason)); |
There was a problem hiding this comment.
Pending restart survives termination
High Severity
terminateVat does not clear #restartWaiters or cancel a queued restartVat item. After the vat is torn down, the later restart crank calls getVat, throws, rejects the stranded waiter, and kills the run loop. Both operations are exposed as RPCs, so restart-then-terminate is a realistic path.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f5fbb4e. Configure here.
The send path caught every endpoint lookup failure and treated it as a splat, which its own TODO called out: an error that is not "this endpoint is gone" silently discarded a deliverable message and rejected its result with ENDPOINT_UNREACHABLE. It is now the last of the four delivery paths to go through `resolveEndpoint`, so a splat happens where the endpoint will not be back — a terminated vat, or a remote — and anything else propagates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nd rollback Four ways to kill or wedge the kernel, found reviewing this branch. `rollbackCrank` emptied `maybeFreeKrefs` rather than restoring it. The set is not per-crank — only `collectGarbage` empties it, at the end of a crank that had an item — so a candidate created while the run loop was idle, as `terminateVat` unpinning a root creates one, was owed a collection that any later crank's rollback silently cancelled. Savepoints now carry the set as it stood when they were taken. The audit cannot see this one: the counts stay self-consistent at 0. A restart that could not relaunch its vat threw, and the run loop's catch rolls back on any throw — undoing the termination records `performVatRestart` had just written and returning the request to the run queue. Every subsequent process start dequeued it and failed the same way. It now terminates the vat and reports through the waiter, so the crank commits and the request is spent. The comment claiming the throw preserved those records had the causality backwards. Terminating a vat left a queued restart for it to be carried out against a vat that no longer existed; `#restartVatWorker` is the one item type that does not go through `#resolveEndpoint`, so the resulting `VatNotFoundError` propagated. Restart-then-terminate is reachable from RPC. The waiter is now rejected when the vat is terminated and the request dropped when the crank reaches it. `cleanupTerminatedVat` ends by *unmarking* the vat it finished, so work outliving it — a `bringOutYourDead` scheduled before it died, which nothing purges from the reap queue — arrived at an endpoint that was neither present nor terminated, which `#resolveEndpoint` reserves its throw for. It now asks whether the store has a live record of the vat at all. Also fixed, from the same review: - `getImporters` counted only vats, so retiring an object deleted it without telling a remote importer, leaving a c-list entry naming nothing — which the audit reports as dangling, taking the run loop with it. Adds `getRemoteIds`. - `#deliverGCAction` computed the live kref set before awaiting the endpoint and used it after. A remote re-handshaking in that window clears its c-list without waiting for the crank, and `krefsToErefs` throws rather than returning short. - `#endVat` marks the vat terminated in a `finally`. A teardown that threw left it unmarked, which is the state above, and falsified `#trackFlux`'s stated invariant that waiters can read "gone" as terminated. - Comments that no longer described the code: `provideVat` waiting on restarts (only teardown is recorded), `stopVat` tearing down "only the worker" (it releases the root pin, as of this branch), `clearStorage` terminating vats, the audit standing in for the disabled `retireExport` assert, and a stale `(1, 1)` baseline rationale. `#vatsInFlux` narrows to `Promise<void>`, which removes a branch of `provideVat` that could not be reached. Tests: each fix has a regression test that fails against the code without it. Closes the two coverage gaps the review named — the splat path charging the run queue item's own target when routing went through a promise, and `ko6.refCount` in the control-panel e2e, restored as three per-checkpoint values rather than dropped as nondeterministic. Full unit suite, kernel-test with auditing on every crank, and `test:e2e:ci` at 17/17. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fcfa5f2. Configure here.
| // merely lost track of. | ||
| this.#vats.delete(vatId); | ||
| this.#kernelStore.markVatAsTerminated(vatId); | ||
| } |
There was a problem hiding this comment.
Teardown leaves active vatConfig
High Severity
#endVat now marks a vat terminated in a finally when stopVat throws, but deleteVat (which clears vatConfig / isVatActive) only runs after vatStream.end inside VatHandle.terminate. If the stream errors after the platform kill — the failure mode the new comment calls out — cleanup later unmarks the vat while vatConfig remains. #resolveEndpoint's new isVatActive check then treats later work for that vat as a live disagreement and kills the run loop.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit fcfa5f2. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1006.
The defect
Creating an import c-list entry changed no refcount; tearing one down decremented both
reachableandrecognizable.initKernelObjectcompensated by minting every object at(1, 1), which is exactly right for one importer — the only topology our tests exercised. There is nosetReachableFlagin the repo; it was never ported.That single unit was also claimed by two parties: importer-side (
object.ts: born at 1 "on the assumption that the new object corresponds to an object that has just been imported") and owner-side (vat.ts: "the baseline decrement below corresponds to the implicit referenceexportFromEndpointinstalled…"). Both an importer's drop and the owner's termination were entitled to spend it.All four symptoms in the issue reproduced against the real store before the fix, and are covered by regression tests now.
Approach
Followed the issue's proposed path, in order.
Step 1 — the invariant checker, first.
store/methods/refcount-audit.tsrecomputes each kref's counts from ground truth — c-list entries and their reachable flags, run-queue and promise-queue messages, promise resolution values, pins — and reports drift in both directions: too low collects a live capability, too high leaks it (the issue's symptom 4 would pass an underflow-only check). The credits mirrorincrementRefCountcase for case.Enabled per kernel via
Kernel.make({ auditRefCounts: true }), run after every crank, and on for every kernelkernel-testbuilds — so a violation fails the build.Step 2 — restore the increment, rebase the baseline.
initKernelObject→(0, 0);addCListEntrytakes the entry's reference, mirroringdeleteCListEntry; newsetReachableFlag; owner-side baseline decrements deleted.collectGarbageis already a faithful port ofprocessRefcounts, so this hands it the inputs it was written for.Step 3 — remove the compensations. This is where the checker earned its keep. It found four more unbalanced paths the phantom baseline had been absorbing:
#deliverSendcharged the target against the routed kref, not the run-queue item's own. For a message routed through a resolved promise those differ, so it decremented an object nobody charged and leaked the promise.#deliverNotifyreleased its reference only on the success path, leaking it on both early returns, and decremented promises retired alongside it that nobody had taken.resolve|kpidincremented with no matching release. (I had assumedresolve|decidercancelled it; that releases the distinct unsettled-promise reference.)Two things the baseline was silently standing in for, now explicit:
pinVatRootalready existed and was never called internally.dropExportsclears the owner's flag,retireExports/retireImportstear the entry down.krefsToExistingErefs→krefsToErefs, which throws rather than silently dropping an unmapped kref.Two judgment calls worth review
The
gc.ts:169assert is not re-enabled. The issue asks for it; I believe it would fire legitimately. When the last holder drops and retires before GC runs,dropExportandretireExportare queued in the same pass and the owner's flag is still set until the first is delivered —drops an object once the last of several importers lets godemonstrates exactly this. Upstream SwingSet also leaves it disabled with the same TODO. I replaced the dead line and stale TODO with the reason. The audit is what validates the accounting now.Settled promises' c-list entries are still not torn down on notify. SwingSet does this (
translateNotify), and I had it working, but it breaks the debug UI:kernel-uidiscovers exported ocap URLs by scraping settled promise values found through c-list entries, andissueOcapURLis stateless — nothing persists issued URLs, so it has no other source. The refcount corrections in that function are all kept; only the record-freeing cleanup is deferred, with a TODO. This is pre-existing behaviour, not a regression. Giving the UI a real source is separate work.An earlier version of this description said "the audit is green without it," which doesn't hold as evidence and I've withdrawn it — grypez is right that it's green here by construction. The retained c-list entry is itself a credited holder, so the stored count and the recomputed count agree, and they'd agree at any value as long as an entry exists to justify it. The audit's ground truth is the holder set, so it catches a count that disagrees with its holders in either direction but structurally cannot catch a leaked holder — which is exactly what this deferral leaves behind. The audit's doc comment and the CHANGELOG now say so.
Added in review
rollbackCranknow reverts the two pieces of state a database rollback cannot reach. grypez traced this from the abort-path comment in#deliverGCAction, which claimed the rollback restored the consumed GC action; it didn't.provideCachedStoredValueanswers reads from a closure and only writes through to kv, so reverting the database left it holding the abandoned crank's value and the nextsetpersisted that.processGCActionSettakes an action out of the set before delivering it, so an aborted delivery lost the action outright rather than retrying it. Repro, verbatim from the review:expected [] to strictly equal [ 'v1 dropExport ko1' ].reapQueuewas exposed the same way.maybeFreeKrefsis cleared. It lives in RAM, so nothing reverted it; its entries are collection candidates only because of the decrements the rollback just undid, and a latercollectGarbagethrew outright on a promise the rollback had deleted, killing the run loop. This is correct only because every rollback is to the crank's own start —KernelQueue.tsis the solecreateCrankSavepointcaller.No live bug either way, since every
abort#deliverGCActionreturns is paired with aterminate. Fixed because the comment was the thing a future reader would trust when adding an abort path that isn't. The comment now states the real causality.Per-credit-source drift coverage for the audit, which grypez asked for before the audit inherits the disabled assert's job. Drift is now asserted in both directions for all 10 credit sources rather than 1 of 8, and both coverage gaps named in the review are closed — line 156 by a run-queue send's result promise, 207-210 by a message parked on an unresolved promise.
refcount-audit.tsis at 100% stmts/lines, 93.1% branch; the remaining uncovered branches are defaults and a dangling-with-no-holders case.The
auditRefCountsJSDoc no longer scopes the option as "intended for tests and debugging," which pulled against making the audit load-bearing. It's off by default because it walks the whole store, not because it's optional.Verification
auditRefCountsclean across all ofkernel-test, which now runs it after every crankyarn test:e2e:ci: 17/17 inextensioncleanupTerminatedVat(previously covered only by a name-export assertion), and a ≥3-endpoint topology all have regression tests — plus an end-to-end two-importer test inkernel-testproving the shared object survives the first importer letting gorollbackCrankregression tests fails against a real store without the fix, with the symptom it names: the restored GC action, the restoredreapQueue, andcollectGarbagethrowingunknown kernel promise kp1Test expectation changes, and why
object.test.ts,store/index.test.ts:(1,1)→(0,0)at birth, as the issue predictedclist.test.ts: an import entry is born un-flaggedpromise.test.tsgetPromisesByDecider: rewritten against the real key layout — it had mockedgetPrefixedKeysto return the stalecle.keys, which is what hid the prefix bugpersistence.test.ts: a hand-writtenrefCountfixture encoded the old accountingcontrol-panel.test.ts(e2e): dropped theko6.refCountassertion. Root pinning ties that value to vat liveness, so it now flips between1,1and2,2depending on whether carol's termination has been processed when the dump is taken. The semantics are covered deterministically inclist-accounting.test.tsinstead.Note on #994
#994 says
translateRefKtoE(remoteId, kref, true)"allocates a c-list entry and increments the refcount". Before this PR no increment occurred, so its pinned-refcount consequence was unfounded; after this PR the increment does happen. Its other two consequences were always unaffected.🤖 Generated with Claude Code
Note
High Risk
Breaking refcount semantics and extensive changes to GC, delivery, crank rollback, and vat lifecycle in the kernel run loop; incorrect accounting could collect live capabilities or wedge the run loop.
Overview
Makes c-list import accounting symmetric: new objects start at
(0, 0),addCListEntrytakes a reference (withsetReachableFlagfor imports), and the old owner-side baseline decrements are removed. Adds per-crank reference-count auditing (Kernel.make({ auditRefCounts }), enabled inkernel-test) and store helpers includingorphanKernelObjectandrecomputeRefCounts.Pins vat roots for the vat’s lifetime and adjusts GC so delivering
dropExports/retireExports/retireImportsupdates the kernel’s c-list;krefsToExistingErefsbecomeskrefsToErefs(throws on missing mappings). The router/run loop gainsrestartVatas queued work, asyncprovideVat, safer handling when endpoints are gone, and refcount fixes for promise requeue, notify, and send targets.rollbackCranknow refreshes cached KV values and restoresmaybeFreeKrefs/ GC action state that DB rollback could not revert. FixesgetPromisesByDeciderc-list key prefixes (cle.→${vatId}.c.). Tests and e2e expectations are updated for the new counts and multi-importer GC behavior.Reviewed by Cursor Bugbot for commit fcfa5f2. Bugbot is set up for automated code reviews on this repo. Configure here.