Uh oh!
There was an error while loading. Please reload this page.
fix(ocap-kernel): boot past a vat whose code can no longer be loaded - #1025
Draft
grypez wants to merge 6 commits into
Draft
fix(ocap-kernel): boot past a vat whose code can no longer be loaded#1025grypez wants to merge 6 commits into
grypez wants to merge 6 commits into
Conversation
A persisted vat whose bundle is gone by the next incarnation (rebuilt to a new path, pruned, or an absolute path that did not survive relocation) makes `fetchBlob` reject with ENOENT. `initializeAllVats` restores every vat in one `Promise.all`, so that single rejection propagates out of `Kernel.#init` and `Kernel.make` rejects — one orphaned bundle reference makes the whole kernel unbootable, and under the daemon that surfaces only as a startup timeout. The failing tests state the defect, not a remedy: the unrestorable vat sits in a different subcluster from the healthy one, so no lifecycle policy under discussion in #979 (including bootstrap-vat death cascading to its own subcluster) makes the healthy vat's loss correct. The unrestorable vat's own fate is left unasserted for the same reason. Ref #964, #979 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`initializeAllVats` restored every persisted vat in one `Promise.all`, so a vat whose code could no longer be loaded rejected out of `Kernel.#init` and made `Kernel.make` reject — one orphaned bundle reference cost the whole kernel, every healthy subcluster included, and under the daemon it surfaced only as a startup timeout. Each vat is now restored in isolation. The one that fails is skipped, and the rest of the kernel boots. Its persisted record is kept rather than pruned, so a vat whose code becomes reachable again is restored by a later boot; discarding persisted state is not a call the restore path gets to make. An error names the vat, its subcluster, and its code source, because trading an unbootable kernel for a kernel quietly missing a vat is no trade at all. The leftover worker is reaped. A bundle is fetched inside the vat's own worker, so by the time the load fails `launch` has long since resolved and the failure arrives from a worker that is already running; one left behind is the wedged process this failure mode is known by. What an unrestorable vat should mean for its *subcluster* is left alone: that is a lifecycle-policy question (#979), and answering it here is what closed#977. This change only declines to lose the healthy vats. The Kernel-level tests mock the worker away, so the mocked failure lands at `launch`; the added `kernel-test` case drives the real path — a real bundle deleted between two real incarnations, failing inside a real worker — and fails without this fix. Its sibling test's logger capture moves from spying on the injected logger's methods to a transport, since `subLogger` returns a fresh `Logger` sharing only transports, so a sub-logger's output never passed through the methods being spied on. Fixes#964 Ref #979 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Skipping an unrestorable vat leaves the kernel in a state it did not model before: a vat named by persisted state that is neither running nor terminated. Its c-lists and reachable flags are still in the store, so the kernel goes on addressing it — and only one of the four delivery paths was written to survive that. `#deliverSend` catches a vanished endpoint and rejects the caller with ENDPOINT_UNREACHABLE. `#deliverNotify`, `#deliverGCAction` and `#deliverBringOutYourDead` looked their endpoint up bare, so `VatNotFoundError` escaped the crank and killed the run loop for good. Because the crank is rolled back, the GC action was restored and re-dequeued on the next boot, killing that one too. So the previous commit traded a kernel that would not boot for one that boots, reports itself healthy, and then dies for good at an arbitrary later crank — reachable as soon as a refcount for one of the skipped vat's exports hits zero, or a promise it subscribed to resolves, or a reap it was queued for comes round. That is worse, not better. None of those three has a caller to reject: nobody is waiting on a dropExport. An endpoint that is not there is something to skip, not a kernel fault. A skipped notify still releases the reference `enqueueNotify` took, so declining to deliver does not strand a kpid forever. `terminateVat` now retires a vat that is persisted but not running instead of throwing. Without that the skipped vat could not be terminated at all, and `terminateSubcluster` — which walks persisted membership — rejected part way through, after deleting the system-subcluster mapping and before removing the subcluster record, so the obvious remedy stranded the subcluster half torn down. A vat that is neither running nor persisted still throws. Also from review: cover several unrestorable vats at once, which is the case that cannot be satisfied by catching around the whole batch; pin that the reap takes only the failed vat's worker; prove end to end that a vat whose bundle returns is restored with the durable state it left off with, and that the stranded subcluster can be torn down. The temp bundle is now removed in `afterEach` — cwd is the monorepo root, where a leaked `*.bundle` is gitignored and survives `yarn clean`. Ref #964, #979 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ 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 f3288f5. Configure here.
| // Not running *and* not persisted: this vat is simply unknown, and | ||
| // saying so beats silently retiring records that were never there. | ||
| throw new VatNotFoundError(vatId); | ||
| } |
There was a problem hiding this comment.
Skipped vat terminate leaves zombies
High Severity
terminateVat for a persisted-but-not-running vat only calls markVatAsTerminated and skips the work VatHandle.terminate normally does: rejecting promises this vat decides and calling deleteVat. Without those steps, waiters hang forever, and because vatConfig remains, getAllVatRecords can restore the vat on the next boot as if it were never terminated.
Reviewed by Cursor Bugbot for commit f3288f5. Configure here.
Contributor
Coverage Report
File Coverage
| ||||||||||||||||||||||||||||||||||||||||||||
grypez
marked this pull request as draft
August 18, 2026 13:59
Skipping a vat at boot left three paths written for a world where every
persisted vat is either running or gone, and review found each of them
still assuming it.
`terminateVat` retires a persisted-but-not-running vat by marking it and
nothing else. `markVatAsTerminated` schedules a cleanup that walks keys
prefixed `${vatId}.`, which never matches `vatConfig.${vatId}`; only
`deleteVat` removes that, and only `VatHandle.terminate` calls it. So the
record survives, and the integration test shows what that costs: give the
bundle back after tearing the subcluster down and the vat the operator was
rid of boots again — running, resuming from its durable state, belonging
to a subcluster that no longer exists. `getVats` asks every vat for its
subcluster, so `getStatus` throws for every caller from then on.
`VatHandle.terminate` is also the only thing that rejects the promises a
vat was the decider of, and `cleanupTerminatedVat` says outright that its
caller is expected to have done so already. It deletes their c-list
entries and drops the decider's refcount regardless, leaving them
unresolved with a decider that cannot ever resolve them.
`#deliverNotify` translates the resolution and its slots before it looks
the endpoint up, and both translations import if needed — minting c-list
entries with the reachable flag set. That was harmless while the lookup
threw and rolled the crank back; now that it skips, the crank commits
them into an endpoint nobody will ever tell.
Two smaller ones: the lookup's catch is broad enough to swallow an
invalid endpoint id, which is corrupt state rather than an absent vat,
and it reports the skip on the per-delivery trace channel.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>`terminateVat` marked a persisted-but-not-running vat terminated and
stopped there, leaving the two things `VatHandle.terminate` does for a
running one. It now does both.
The record goes. The cleanup that `markVatAsTerminated` schedules walks
keys prefixed `${vatId}.`, which never matches `vatConfig.${vatId}`, and
that record is what the next boot restores from — so a terminated vat came
back as soon as its code was reachable again, running, resuming from
durable state, in a subcluster that had been deleted underneath it. Every
`getStatus` failed from then on, since it asks each vat for its
subcluster. While the code stayed missing the vat was simply undeletable,
which is the state this branch existed to prevent.
The decider promises are rejected. `cleanupTerminatedVat` deletes their
c-list entries and drops the decider's refcount on the stated
understanding that its caller rejected them first; nothing did, so they
were left unresolved with a decider that no longer existed and no way to
ever settle. Callers waiting on the vat waited for good. The reason
argument now reaches them, as it does for a running vat.
`removeVatFromSubcluster` no longer reports a vat that is in no
subcluster. It runs while a vat is being discarded — reached here through
`deleteVat` — and a vat with no subcluster is already in the state it
asks for, so failing there only strands the teardown part-way through.
`#deliverNotify` looks its endpoint up before translating the resolution
rather than after. Both translations import if needed, minting c-list
entries with the reachable flag set and taking a reference on every slot;
doing that for an endpoint that will never be told writes rows only that
endpoint could release. While an absent endpoint threw, the rollback undid
them. Now that it is skipped the crank commits, so each skipped notify
leaked into the store for good.
The endpoint lookup no longer swallows an id that names neither a vat nor
a remote. A missing vat and a missing remote are ordinary; that is corrupt
state or a kernel bug, and GC actions pass `insistEndpointId` before they
are ever queued. The skip is reported at warn rather than on the
per-delivery trace channel: a delivery dropped on the floor is not routine
traffic, and it is the only trace of a vat that has quietly stopped doing
anything.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Explanation
A vat outlives the code it was launched from. The kernel stores a vat's durable state but not its code, re-fetching from the recorded
bundleSpecon every incarnation — so a bundle that is rebuilt to a new path, pruned, or recorded as an absolute path that did not survive relocation leaves a persisted vat that cannot be restored.VatManager.initializeAllVatsrestored every vat inside onePromise.all, so one such vat rejected out ofKernel.#init,Kernel.makerejected, and the whole kernel — every healthy subcluster included — was unbootable. Under the daemon that surfaced only as "Daemon did not start within 30s".Each vat is now restored in isolation. The one that fails is skipped and the rest of the kernel boots. Its persisted record is kept rather than pruned, so a vat whose code becomes reachable again is restored by a later boot, resuming from the durable state it left off with. An error names the vat, its subcluster, and its code source — trading an unbootable kernel for one that is quietly missing a vat would be no trade at all.
The leftover worker is reaped. A bundle is fetched inside the vat's own worker (
VatSupervisor.#initVat→fetchBlob), so by the time the load failslaunchhas long since resolved and the worker is live; one left behind is the wedged process holding the sqlite lock that this failure mode is known by.The state this introduces, and the two defects it exposed
Skipping a vat creates a state the kernel did not model: a vat named by persisted state that is neither running nor terminated. Its c-lists and reachable flags stay in the store, so the kernel goes on addressing it. Two paths were not written to survive that, and both are fixed here — the first is why this PR is larger than its issue.
Deliveries to an absent endpoint killed the run loop.
#deliverSendalready tolerated a vanished endpoint (rejecting the caller withENDPOINT_UNREACHABLE, from #1007), but#deliverNotify,#deliverGCActionand#deliverBringOutYourDeadlooked their endpoint up bare, soVatNotFoundErrorescaped the crank and killed the run loop permanently. Since the crank is rolled back, the GC action was restored and re-dequeued on the next boot, killing that one too. Reachable as soon as a refcount for one of the skipped vat's exports hits zero, a promise it had subscribed to resolves, or a reap queued for it comes round.Left unfixed, this PR would have traded a kernel that refuses to boot for one that boots, reports
runLoop: running, and then dies for good at an arbitrary later crank — worse than the bug it fixes. None of those three deliveries has a caller to reject (nobody is waiting on adropExport), so an absent endpoint is now something to skip. A skippednotifystill releases the referenceenqueueNotifytook.A skipped vat could not be terminated.
terminateVatwent throughgetVatand threw, soterminateSubcluster— which walks persisted membership — rejected part way through, after deleting the system-subcluster mapping and before removing the subcluster record. The obvious remedy stranded the subcluster half torn down, with no way to be rid of the vat short of discarding the store.terminateVatnow retires a persisted-but-not-running vat; one that is neither running nor persisted still throws.Scope
Deliberately not decided here: whether an unrestorable vat should take its whole subcluster down with it. That is the coterminous-lifecycle question in #979, and the tests are written so no lifecycle policy is presumed — the unrestorable vat sits in a different subcluster from the healthy one, so no policy under discussion makes the healthy vat's loss correct.
The observability half of #964 (rendering the real fatal to
daemon.lograther than[object Object]on a discarded stderr) landed separately in #966.fetchBlobneeds no change: Node's errno already carries the path, and the[object Object]came from the error crossing the worker's JSON-RPC boundary.Testing
Three layers, and the first commit is the failing repro on its own.
Kernel.test.ts— boot completes with the healthy vat up and the unrestorable one absent, and one error entry names both the vat and its bundle.VatManager.test.ts— both failure timings (before the worker comes up, and after it is live, which is the production one); the reap takes only the failed vat's worker; boot survives a reap that cannot happen; the record is not pruned; all threeVatConfigsource shapes and the no-subcluster fallback are named; and several unrestorable vats at once, which is the case that cannot be satisfied by catching around the whole batch.KernelRouter.test.ts— a notify, a GC action, and abringOutYourDeadaddressed to an absent endpoint are skipped rather than thrown out of the crank, while endpoints that are running still receive theirs. All four fail without the guard.kernel-test/persistence.test.ts— the real path end to end: real bundles, real workers, a real file deleted between two real incarnations. Verified to fail without the fix with the genuinefetchBlobENOENT stack from inside the worker. It also tears down the stranded subcluster, and a second case proves a vat whose bundle returns comes back with the state it left off with.@metamask/ocap-kernel,@ocap/kernel-testand@metamask/kernel-node-runtimesuites,lint:fix,buildandchangelog:validateall pass.Checklist
🤖 Generated with Claude Code
Note
High Risk
Changes kernel startup, run-loop delivery, and vat termination in core ocap-kernel paths; mistakes could leave silent missing vats, wedged workers, or incorrect GC/notify behavior.
Overview
Kernel boot no longer fails when one persisted vat’s bundle is missing.
VatManagerrestores each vat in isolation via#restoreVat: the failing vat is skipped, its stray worker is terminated, an error logs vat id, subcluster, and code source (bundleSpec/sourceSpec/bundleName), and persisted state is kept so a later boot can restore the vat if the bundle returns.Follow-on fixes for “persisted but not running” vats.
KernelRouterskipsnotify, GC actions, andbringOutYourDeadwhen the target endpoint is absent (instead of throwing and killing the run loop); skipped notifies still drop their enqueue ref.terminateVatcan mark persisted, non-running vats terminated soterminateSubclustercan finish cleanup.Integration coverage in
kernel-test/persistence.test.ts(bundle deleted between incarnations, subcluster teardown, bundle restored on a third boot).Reviewed by Cursor Bugbot for commit 7b40ea9. Bugbot is set up for automated code reviews on this repo. Configure here.