fix(ocap-kernel): keep the kernel's account of a vat's life in one piece - #1023
fix(ocap-kernel): keep the kernel's account of a vat's life in one piece#1023sirtimid wants to merge 13 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Recording the vat's death only saves the deliveries that come after it. The one in flight when the worker died stays parked on an RPC client with no timeout, so its crank never completes — the same hang `onCriticalFailure` exists to prevent, one delivery earlier. The worker was left running too, since nothing else would stop it once the handle was off the books. Found by Cursor Bugbot on #1023. Also reverts this branch's additions to the extension control-panel e2e test. They asserted `ko6.refCount` directly, and which vat owns `ko6` is not stable: #983 launches subcluster vats in parallel, so the root krefs fall in completion order. The behaviour they checked is covered by the refcount audit, which runs on every kernel `kernel-test` builds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
… exists `onCriticalFailure` closed over the binding `VatHandle.make` was still to return, so a stream that broke during `#init` threw on the temporal dead zone — after the vat had been retired, and before anything rejected the pending `initVat` that nothing else will ever settle. The handle is now passed to the callback, and `runVat` refuses to put a handle on the books for a vat retired while it was being made. The teardown also awaited the worker kill before `terminate`, which is what rejects the vat's pending RPCs. A worker slow to die — or one that never does — kept the parked delivery parked, which is the hang this path exists to clear. The two now run alongside each other, `terminate` first. Found by Cursor Bugbot on #1023. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…hem (Consensys-Incorporated#1024) Fixes a pre-existing e2e flake on `main`, surfaced while rebasing the Consensys-Incorporated#1020–Consensys-Incorporated#1023 stack. Small and self-contained so the whole stack inherits it. ## The defect `control-panel.test.ts` › `should collect garbage` asserted that Carol's root object is `ko6` and Bob's is `ko5`, and that their promises are `kp4` and `kp3`: ```js '{"key":"ko6.owner","value":"v3"}', '{"key":"v3.c.ko6","value":"R o+0"}', ``` Since Consensys-Incorporated#983, subcluster vats launch **in parallel**. Each vat's root is exported when its own launch finishes, so which of `ko5`/`ko6` belongs to Bob and which to Carol changes between runs. When they come back the other way round, the test fails — and `database-inspector.test.ts` fails alongside it, because it reads the same kv dump. Observed directly: a failing run had `ko6.owner = v2` and `ko5.owner = v3`, the exact inverse of what is asserted. The vat ids themselves are stable — they are handed out in config order, so alice is always `v1` — so only the object and promise krefs need deriving. ## Approach Three small helpers read the dump and look up what the assertions used to hardcode: `rootKrefOf(dump, vatId)` by owner, `promiseKrefOf(dump, vatId)` by c-list entry, and `erefOf(dump, vatId, kref)`. The erefs are derived in **full** rather than matched by prefix. A c-list entry's reverse direction is keyed by eref and valued by kref, so a loose `,"value":"ko5"}` also matches the *owning* vat's own `v2.c.o+0` entry. That passed while both vats were alive and broke the negative assertions the moment one outlived the other — which is what the test checks after terminating v3. ## Testing `yarn lint` clean. Extension e2e run three times: the kref failure is gone, and the two clean runs finish in ~50s rather than ~2.7m because no retries are needed. **What this does not fix.** The extension e2e suite has separate instability that this change does not touch and does not claim to: `object-registry.test.ts` failures, and a UI timing flake where `Terminated vat "v1"` does not render because the panel is still showing query output. One of the three runs hit those. They are unrelated to kref assignment and were present before this change. ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, `README.md`, `CHANGELOG.md`) as appropriate — test-only change, no changelog entry <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Test-only change to e2e assertions and helpers; no production or runtime behavior is modified. > > **Overview** > Fixes flaky **`should collect garbage`** assertions in `control-panel.test.ts` that assumed fixed kernel refs (`ko5`/`ko6`, `kp3`/`kp4`) for Bob and Carol. Parallel subcluster launches mean those object and promise krefs can swap between runs while vat ids (`v2`/`v3`) stay stable. > > Adds helpers to parse the Database Inspector kv dump and **derive** root krefs (via `.owner`), promise krefs (via c-list), and v1’s **erefs** (full c-list lookup so reverse entries don’t false-match). The garbage-collection expectations are built from those values instead of literals. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit d8e81f7. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Recording the vat's death only saves the deliveries that come after it. The one in flight when the worker died stays parked on an RPC client with no timeout, so its crank never completes — the same hang `onCriticalFailure` exists to prevent, one delivery earlier. The worker was left running too, since nothing else would stop it once the handle was off the books. Found by Cursor Bugbot on #1023. Also reverts this branch's additions to the extension control-panel e2e test. They asserted `ko6.refCount` directly, and which vat owns `ko6` is not stable: order. The behaviour they checked is covered by the refcount audit, which runs on every kernel `kernel-test` builds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… exists `onCriticalFailure` closed over the binding `VatHandle.make` was still to return, so a stream that broke during `#init` threw on the temporal dead zone — after the vat had been retired, and before anything rejected the pending `initVat` that nothing else will ever settle. The handle is now passed to the callback, and `runVat` refuses to put a handle on the books for a vat retired while it was being made. The teardown also awaited the worker kill before `terminate`, which is what rejects the vat's pending RPCs. A worker slow to die — or one that never does — kept the parked delivery parked, which is the hang this path exists to clear. The two now run alongside each other, `terminate` first. Found by Cursor Bugbot on #1023. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
93efa62 to
c9b917b
Compare
|
Raised from reviewing #1020, verified by execution against this branch's tip ( The asymmetry
That asymmetry is what made the remote case visible, and this PR closes the remote half by unioning in The vat half is still open
The recognizable unit it still holds buys no protection. The orphan branch is Reproduced at the store level on this branch: Sequence: A control isolates the cause exactly: the identical sequence without Severity: transient, but fatal with auditing onThe next crank's cleanup sweeps the entry and the audit goes clean — no throw, no lasting dangle, no underflow: So the harm is confined to the window. With Reachability in a real run loop is a code reading rather than an executed result, so treat it as an argument: Suggested fixMake Setup caveat for anyone writing the regression test: |
Recording the vat's death only saves the deliveries that come after it. The one in flight when the worker died stays parked on an RPC client with no timeout, so its crank never completes — the same hang `onCriticalFailure` exists to prevent, one delivery earlier. The worker was left running too, since nothing else would stop it once the handle was off the books. Found by Cursor Bugbot on #1023. Also reverts this branch's additions to the extension control-panel e2e test. They asserted `ko6.refCount` directly, and which vat owns `ko6` is not stable: order. The behaviour they checked is covered by the refcount audit, which runs on every kernel `kernel-test` builds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… exists `onCriticalFailure` closed over the binding `VatHandle.make` was still to return, so a stream that broke during `#init` threw on the temporal dead zone — after the vat had been retired, and before anything rejected the pending `initVat` that nothing else will ever settle. The handle is now passed to the callback, and `runVat` refuses to put a handle on the books for a vat retired while it was being made. The teardown also awaited the worker kill before `terminate`, which is what rejects the vat's pending RPCs. A worker slow to die — or one that never does — kept the parked delivery parked, which is the hang this path exists to clear. The two now run alongside each other, `terminate` first. Found by Cursor Bugbot on #1023. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c9b917b to
b8b2b45
Compare
| fatalError = error; | ||
| this.#retireVat(vatId, error); | ||
| this.#startFailedVatTeardown(vatId, failedVat, error); | ||
| }, |
There was a problem hiding this comment.
Stream failure rolls back vat death
High Severity
onCriticalFailure records the vat dead with #retireVat during the current crank, then rejects the in-flight RPC. A throw from deliverNotify or deliverBringOutYourDead rolls that crank back, undoing the store writes while #vats already dropped the handle. The store again says the vat is live with no handle, which kills the run loop and lets initializeAllVats resurrect it.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit b8b2b45. Configure here.
There was a problem hiding this comment.
Reopened: this one is real and is not fixed. It got auto-resolved by the push that fixed the other three.
Confirmed against a real SQLite store rather than the map test DB, whose savepoint methods are no-ops and so report the opposite. #retireVats store writes land inside the delivery savepoint, and after rollbackCrank(delivery) the store reports isVatActive: true, isVatTerminated: false while #vats has already dropped the handle.
#processCrankResult already has the mechanism for this and states the invariant at the call site — crankResult.terminate runs after the rollback, inside crank, because "its writes must outlive the rollback above". Routing a mid-delivery stream death through that, instead of writing the store from the callback, is the fix I would argue for. It reshapes the boundary between a delivery and the retirement that discovered it, so it wants its own change rather than riding along here. Written up in the PR description.
…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>
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>
Only `deleteVat` removes `vatConfig.<vatId>`, and `cleanupTerminatedVat`
sweeps `${vatId}.`-prefixed keys, which never match it. The writes making up
a vat's death were interleaved with awaits across `VatManager.stopVat`,
`#endVat`'s `finally`, `VatHandle.terminate` and a lambda in `Kernel.ts`, so a
throw part-way left the vat marked terminated with its config alive — which
reads as *active* again as soon as cleanup drops the mark, killing the run
loop over the disagreement and resurrecting the vat on the next process start.
`VatManager.#retireVat` now makes all four writes with no await between them,
modelled on SwingSet's synchronous prelude in `kernel.js` `terminateVat`;
worker teardown follows and is best-effort. `#endVat` and `#abandonVat` go as
duplicates of it, and `VatHandle.terminate` is left with only its own channel
to close.
A vat whose stream fails is retired by the manager, via a new
`onCriticalFailure`, rather than tearing itself down: that left the handle in
the manager and the vat live in the store, so the next delivery went to a
worker that could not answer and, the vat RPC client having no timeout, the
crank never completed while the run loop still reported itself running.
`makeGCAndFinalize` drains the queues before sweeping, since a pending
continuation still holds its closure's objects, so a vat reports its dropped
imports on the `bringOutYourDead` that provoked them rather than a later one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tial launch terminated Two conflicts the rebase onto the GC-hardening stack surfaced, both real disagreements rather than textual ones. `revertStateBeneathRollback` cleared `maybeFreeKrefs` outright. The set is not per-crank — only `collectGarbage` empties it — so a candidate added while the run loop was idle, which `terminateVat` unpinning a root produces, was owed a collection and lost it to an unrelated crank's rollback. It now restores the savepoint's snapshot, which discards the abandoned crank's additions and keeps everything that predates it. The unit test had encoded the old behaviour and is updated to distinguish the two cases. `launchVat`'s cleanup relied on `stopVat` reaching `#retireVat` to record the death, but `stopVat` refuses a vat the kernel has no handle for and the store does not call active — which is what a partial launch looks like. The mark is asserted directly again, as it was before this branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Recording the vat's death only saves the deliveries that come after it. The one in flight when the worker died stays parked on an RPC client with no timeout, so its crank never completes — the same hang `onCriticalFailure` exists to prevent, one delivery earlier. The worker was left running too, since nothing else would stop it once the handle was off the books. Found by Cursor Bugbot on #1023. Also reverts this branch's additions to the extension control-panel e2e test. They asserted `ko6.refCount` directly, and which vat owns `ko6` is not stable: order. The behaviour they checked is covered by the refcount audit, which runs on every kernel `kernel-test` builds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… exists `onCriticalFailure` closed over the binding `VatHandle.make` was still to return, so a stream that broke during `#init` threw on the temporal dead zone — after the vat had been retired, and before anything rejected the pending `initVat` that nothing else will ever settle. The handle is now passed to the callback, and `runVat` refuses to put a handle on the books for a vat retired while it was being made. The teardown also awaited the worker kill before `terminate`, which is what rejects the vat's pending RPCs. A worker slow to die — or one that never does — kept the parked delivery parked, which is the hang this path exists to clear. The two now run alongside each other, `terminate` first. Found by Cursor Bugbot on #1023. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…dies `#retireVat` could not be called twice. `deleteVat` removes the vat's subcluster mapping, and `removeVatFromSubcluster` fails on a vat that has none, so the second call threw. `performVatRestart` calls it twice whenever a relaunch breaks the stream: `onCriticalFailure` retires the vat, `runVat` rethrows, and the catch retires it again. The throw escaped the catch that must not throw, so the crank rolled back the termination records and returned the failing restart to the run queue for the next process start to replay. The mock store hid it. `deleteVat` was a bare `vi.fn()` that shrugged at a repeat; it now refuses one the way the real store does, and `isVatTerminated` answers for what `markVatAsTerminated` was told. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5d9bd14 to
f624a34
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f624a34. Configure here.
`restartVat` enqueued an item every time while `#awaitRestart` superseded only the in-memory waiter, so two concurrent requests left two items and one waiter. The first crank consumed that waiter and handed its caller a live handle; the leftover item then ran anyway, stopping that worker and — if the relaunch failed — retiring the vat the caller had just been told about. A request arriving while one is still queued now takes over its item. The waiter is the signal: `performVatRestart` claims it the moment it starts, so a request landing after that still gets an item of its own. The existing supersede test could not catch this — it stubs `enqueueRestartVat` to a no-op and drives `performVatRestart` once, so the leftover item was never exercised. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`getImporters` enumerated vats through their `vatConfig` rows, which `deleteVat` has already dropped by the time a terminated vat's c-list is swept — cleanup reaches one vat per crank. In that window the vat is deregistered but still holds its imports, so `retireKernelObjects` queued no `retireImport` for it and deleted the object anyway, leaving a c-list entry naming nothing and the audit reporting it as dangling. Terminating an object's owner and its importer close together, owner marked first, lands the collection inside the window. Harmless with auditing off; with it on the audit kills the run loop. Raised by grypez on #1023, verified by execution. Their repro reproduces here byte for byte, and the fix yields their control case: one `retireImport` for the terminated importer and a clean audit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Confirmed and fixed in 9969a97. Your repro reproduces here byte for byte at the current tip: Took the fix you suggested — With it, the same sequence yields your control case: Deduplicated into a Regression test is in Two notes on the rest of your comment: You were right that this PR widens the window.
|
2 participants
Stacked on #1022, which is stacked on #1021; review those first. #1020 has merged. Replaces #1019, rebased onto the split stack.
Last of four. Closes #1015.
The defect
Only
kernelStore.deleteVat()removesvatConfig.<vatId>.cleanupTerminatedVatsweeps${vatId}.-prefixed keys, which never matchvatConfig.<vatId>. So the two writes that together mean "this vat is dead" —deleteVatandmarkVatAsTerminated— both had to land, and nothing made them.They were spread across four functions with awaits between them:
VatManager.stopVat,#endVat'sfinally,VatHandle.terminate, and a lambda inKernel.ts. A throw part-way left the vat marked terminated with its config alive. That reads as active again the momentcleanupTerminatedVatcallsforgetTerminatedVat, at which pointKernelRouter.#resolveEndpointsees a vat the store calls live and the kernel has no handle for, rethrows, and kills the run loop.initializeAllVatsthen resurrects the vat on the next process start.Two more, found alongside it:
VatHandle.#init's drain catch calledthis.terminate(true, …), which diddeleteVatbut nevermarkVatAsTerminatedand never removed the handle fromVatManager.#vats. The router went on resolving the handle successfully, the write went nowhere, and since the vat RPC client has no timeout the crank never completed — the run loop hung whilegetRunLoopStatus()still reportedrunning.bringOutYourDeadlate.makeGCAndFinalizecalledgc()without draining the queues first. A pending continuation still holds its closure's objects, so a sweep with work outstanding finds them reachable and the vat reports nothing on the BOYD that provoked it.Approach
VatManager.#retireVat— the store side of a vat's death, in one synchronous step. Reject the promises it was deciding, unpin its root,deleteVat,markVatAsTerminated, with no await between them, so the half-written state cannot arise. Modelled on SwingSet'sterminateVat(kernel.js:345-412) and its comment at:348about the "synchronous prelude". Killing the worker is deliberately not part of it: that can fail, and a store that says the vat is dead is worth more than a store still waiting to find out.#endVatand#abandonVatwere both partial copies of this and are gone.Kernel.ts's lambda andlaunchVatlose their trailingmarkVatAsTerminated.VatHandle.terminateis left with only its own channel to close, and rejects its pending RPCs ahead of ending the stream rather than after, so a stream that will not close does not strand callers on a worker that is already dead.stopVat(vatId, true, …)tolerates a missing handle. A restart needs a live handle to read its config from; an ending vat does not, and must not — the vat may be one the store still lists while the kernel has lost its handle, which is exactly whatSubclusterManager.terminateSubclustercan hand it.A fatal stream error is routed through the manager, via a new required
onCriticalFailureprop onVatHandle. Only the manager can put the vat's death on record and drop the handle.The run loop carries out a restart itself, as a queued
restartVatitem, so a vat is never out of the kernel's reach while cranks run.#1015 is closed here, both halves:
getImportersnow counts remotes and terminated vats cleanup has not reached, so retiring an object queues aretireImportfor each rather than deleting the object and leaving their c-list entries naming nothing. #1022 supplied the exemption site and thedanglingdiscriminant; this supplies the missing importers. The vat half was raised by @grypez in review — see below.Two conflicts the rebase surfaced, resolved on the merits
Both were real disagreements between this branch and the stack beneath it, not textual noise. They are in their own commit,
363c3e103.maybeFreeKrefswas being cleared, not restored. #1021'srevertStateBeneathRollbackempties the set on rollback; this branch restores the savepoint's snapshot. Restoring is correct, and #1021's own comment admits the caveat ("correct only while every rollback discards the whole delivery"): the set is not per-crank — onlycollectGarbageempties it — so a candidate added while the run loop was idle, whichterminateVatunpinning a root produces, was owed a collection and lost it to an unrelated crank's rollback. This branch introduces exactly that path. #1021's unit test had encoded theclear()behaviour and now distinguishes a pre-savepoint kref from one the abandoned crank added.A fix from #1022 was regressed. #1022 marks a vat terminated unconditionally after a failed launch; this branch relied on
stopVatreaching#retireVatto record it. ButstopVatrefuses a vat the kernel has no handle for and the store does not call active — which is what a partial launch looks like. The unconditional mark is asserted again.Three fixes from review
Bugbot flagged four things on this branch; three are fixed here and the fourth is argued below.
A vat's death could not be recorded twice, and one path recorded it twice.
deleteVatremoves the vat's subcluster mapping, andremoveVatFromSubclusterfails on a vat that has none, so the second call threw.performVatRestartreaches that whenever a relaunch breaks the stream:onCriticalFailureretires the vat,runVatrethrows, and the catch retires it again — so the throw escaped the one catch in this PR that must not throw, rolling back the termination records and returning the failing restart to the queue to be replayed forever.#retireVatis now a no-op the second time. Invisible to the suite until now, because the mock store'sdeleteVatwas a barevi.fn(); it refuses a repeat the way the real one does.Two concurrent restart requests queued two items and kept one waiter. The first crank consumed the waiter and handed its caller a live handle; the leftover item then stopped that worker and, on a failed relaunch, retired the vat the caller had just been told about. A request arriving while one is still queued now takes over its item. The existing supersede test could not catch it — it stubs
enqueueRestartVatto a no-op and drivesperformVatRestartonce.getImporterscould not see a terminated vat still holding its imports. @grypez's finding, verified by execution; their repro reproduces byte for byte and their suggested fix is the one taken. Details in the review thread.Both restart fixes are mutation-verified: revert the production hunk and the named test fails for the stated reason.
The fourth: a rolled-back crank can un-record a vat's death
Not fixed here, and I do not think it should be without a decision.
onCriticalFailuredrops the handle from#vats— plain RAM — and records the death in the store. Those store writes land inside thedeliverysavepoint, so a delivery that then throws rolls them back while the handle stays gone. Confirmed against a real SQLite store: afterrollbackCrank('delivery')the store reportsisVatActive: true, isVatTerminated: false, which is the store-says-live, kernel-has-no-handle disagreement this PR exists to prevent, and#resolveEndpointturns it into a dead run loop.#processCrankResultalready has the mechanism this wants, and says so at the call site:crankResult.terminateruns after the rollback, insidecrank, because "its writes must outlive the rollback above". Routing a mid-delivery stream death through that rather than writing the store from the callback is the fix I would argue for — but it reshapes the boundary between a delivery and the retirement that discovered it, and getting it wrong reintroduces the hang this PR removes. Worth its own change.Testing
yarn lintclean,yarn build31/31,changelog:validateclean.@metamask/ocap-kerneland@ocap/kernel-testfully green, withauditRefCountson for every kernelkernel-testbuilds.Note for reviewers of #1021:
garbage-collection.test.ts › an object shared by two importers › survives until both importers let gowas intermittently flaky on the branches beneath this one. ThemakeGCAndFinalizefix here is what addresses it.Checklist
README.md,CHANGELOG.md) as appropriateNote
High Risk
Changes core run-loop delivery, synchronous vat death recording, crank rollback GC state, and restart/termination ordering—bugs here can deadlock or kill the kernel, but coverage is extensive.
Overview
Keeps the kernel’s view of vat life, restarts, and deliveries consistent so half-written death records and mid-restart races no longer kill the run loop or resurrect vats on restart.
Vat retirement is centralized in
VatManager.#retireVat: reject decider promises, unpin the root,deleteVat, andmarkVatAsTerminatedin one synchronous step before any worker teardown. Fatal stream errors go throughonCriticalFailureinstead ofVatHandletearing down store state itself;terminateonly closes the channel and rejects in-flight RPCs.Restarts are run-queue work (
restartVat+enqueueRestartVat/performVatRestart), with one queued request per vat, superseded waiters, and failed relaunches terminating the vat without aborting the crank. Endpoint resolution is async viaprovideVatand#resolveEndpoint: deliveries to vats gone for good are dropped or splatted; absent-but-not-terminated vats still surface errors;notify/bringOutYourDeadskip safely when appropriate.GC / rollback: crank savepoints snapshot
maybeFreeKrefsand rollback restores that set (idle candidates are not wiped).getImportersincludes remotes and terminated vats awaiting cleanup.gcAndFinalizedrains the event loop before sweeping so dropped imports show up on the provokingbringOutYourDead.Reviewed by Cursor Bugbot for commit 9969a97. Bugbot is set up for automated code reviews on this repo. Configure here.