From 0077123efbae7a344dac1fbe5682112ec191d06c Mon Sep 17 00:00:00 2001 From: grypez <143971198+grypez@users.noreply.github.com> Date: Mon, 24 Aug 2026 12:39:57 -0400 Subject: [PATCH 1/2] test(ocap-kernel): reproduce a vat that cannot be terminated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `restartVat` stops a vat and then runs it again. A relaunch that fails — the bundle has moved since the vat was launched, the worker will not spawn — leaves the vat gone from the running map with its record, its vat store and its root pin all still in place. Nothing retires a vat in that state. `terminateVat` goes through `stopVat` → `getVat` and throws `VatNotFoundError`, so the only way to be rid of one is to discard the whole store, and `terminateSubcluster` — which walks persisted membership rather than the running map — rejects part way through on reaching one, after deleting the system-subcluster mapping and before removing the subcluster record. Retiring one has to do what `stopVat` does besides stopping a worker, and none of it is visible from `terminateVat` alone: `deleteVat` takes the vat's config, its own store, and its subcluster membership. `markVatAsTerminated` does not: the cleanup it schedules walks keys prefixed `${vatId}.`, which never matches `vatConfig.${vatId}`. A record left behind restores the vat at the next boot whose code is reachable. The promises the vat was deciding are rejected. `cleanupTerminatedVat` deletes their c-list entries and drops the decider's refcount on the stated understanding that its caller has already rejected them. The pin `launchVat` took on the vat's root is released, or the root stays pinned and uncollectable for the life of the store. `removeVatFromSubcluster` reports a vat that is in no subcluster, which `deleteVat` reaches while discarding a vat — the one moment a failure cannot be retried past. Co-Authored-By: Claude Opus 5 --- .../src/store/methods/subclusters.test.ts | 12 ++- .../ocap-kernel/src/vats/VatManager.test.ts | 93 +++++++++++++++++++ 2 files changed, 103 insertions(+), 2 deletions(-) diff --git a/packages/ocap-kernel/src/store/methods/subclusters.test.ts b/packages/ocap-kernel/src/store/methods/subclusters.test.ts index eda07e6bb2..47ef24e0a6 100644 --- a/packages/ocap-kernel/src/store/methods/subclusters.test.ts +++ b/packages/ocap-kernel/src/store/methods/subclusters.test.ts @@ -495,11 +495,19 @@ describe('getSubclusterMethods', () => { expect(map[vatId2]).toBe(scId); }); - it('should throw an error if the vat is not in any subcluster', () => { + it('leaves a vat that is in no subcluster alone', () => { const nonMappedVat = 'vNonMapped' as VatId; + + // Already in the state this asks for. Reporting it instead strands the + // teardown that called it — `deleteVat` reaches here while discarding a + // vat, which is the one moment a failure cannot be retried past. expect(() => subclusterMethods.removeVatFromSubcluster(nonMappedVat), - ).toThrow('Vat "vNonMapped" has no subcluster'); + ).not.toThrow(); + expect(subclusterMethods.getSubcluster(scId)?.vats).toStrictEqual({ + vat1: vatId1, + vat2: vatId2, + }); }); it('should handle removing the last vat from a subcluster', () => { diff --git a/packages/ocap-kernel/src/vats/VatManager.test.ts b/packages/ocap-kernel/src/vats/VatManager.test.ts index 21361f942c..9f23826c14 100644 --- a/packages/ocap-kernel/src/vats/VatManager.test.ts +++ b/packages/ocap-kernel/src/vats/VatManager.test.ts @@ -69,7 +69,10 @@ describe('VatManager', () => { })(), ), getVatSubcluster: vi.fn().mockReturnValue('s1'), + isVatActive: vi.fn().mockReturnValue(true), markVatAsTerminated: vi.fn(), + deleteVat: vi.fn(), + getPromisesByDecider: vi.fn().mockReturnValue([]), getRootObject: vi.fn().mockReturnValue('ko1'), pinObject: vi.fn(), unpinObject: vi.fn(), @@ -80,6 +83,7 @@ describe('VatManager', () => { mockKernelQueue = { waitForCrank: vi.fn().mockResolvedValue(undefined), + resolvePromises: vi.fn(), } as unknown as Mocked; mockLogger = new Logger('test'); @@ -345,6 +349,95 @@ describe('VatManager', () => { expect.objectContaining({ message: 'Vat termination: Custom reason' }), ); }); + + describe('a vat that is persisted but not running', () => { + // `restartVat` stops the vat and then runs it again. A relaunch that + // fails — the bundle has moved since the vat was launched, the worker + // will not spawn — leaves the vat gone from the running map with its + // record, its vat store and its root pin all still in place. Nothing + // else retires one, so `terminateVat` has to. + const leaveVatPersistedButNotRunning = async (): Promise => { + await vatManager.runVat('v1', createMockVatConfig()); + mockPlatformServices.launch.mockRejectedValueOnce( + new Error('ENOENT: no such file or directory'), + ); + await expect(vatManager.restartVat('v1')).rejects.toThrow('ENOENT'); + expect(vatManager.hasVat('v1')).toBe(false); + }; + + it('can be terminated after a failed restart', async () => { + await leaveVatPersistedButNotRunning(); + + // Went through `stopVat` → `getVat` and threw, so a vat in this state + // could not be retired at all, and `terminateSubcluster` — which walks + // persisted membership — rejected part way through on reaching one. + expect(await vatManager.terminateVat('v1')).toBeUndefined(); + expect(mockKernelStore.markVatAsTerminated).toHaveBeenCalledWith('v1'); + }); + + it('discards its persisted record', async () => { + await leaveVatPersistedButNotRunning(); + + await vatManager.terminateVat('v1'); + + // Marking it terminated is not enough. The cleanup that schedules walks + // keys prefixed `${vatId}.`, which never matches `vatConfig.${vatId}`; + // only `deleteVat` takes that, along with the vat's own store and its + // subcluster membership. Left behind, the record restores the vat at + // the next boot whose code is reachable. + expect(mockKernelStore.deleteVat).toHaveBeenCalledWith('v1'); + }); + + it('rejects the promises it was deciding', async () => { + mockKernelStore.getPromisesByDecider.mockReturnValue(['kp1']); + await leaveVatPersistedButNotRunning(); + + await vatManager.terminateVat('v1'); + + // `cleanupTerminatedVat` deletes these promises' c-list entries and + // drops the decider's refcount on the stated understanding that its + // caller has already rejected them. Nothing else can: a promise left + // unresolved with a decider that no longer exists hangs its waiters for + // good. + expect(mockKernelQueue.resolvePromises).toHaveBeenCalledWith('v1', [ + ['kp1', true, expect.anything()], + ]); + }); + + it('carries the termination reason into those rejections', async () => { + mockKernelStore.getPromisesByDecider.mockReturnValue(['kp1']); + await leaveVatPersistedButNotRunning(); + + await vatManager.terminateVat('v1', { + body: 'Custom reason', + slots: [], + }); + + const [, resolutions] = mockKernelQueue.resolvePromises.mock + .calls[0] as [string, [string, boolean, { body: string }][]]; + expect(resolutions[0]?.[2]?.body).toContain('Custom reason'); + }); + + it('releases the pin its root was launched with', async () => { + await leaveVatPersistedButNotRunning(); + + await vatManager.terminateVat('v1'); + + // `launchVat` pins the root for the vat's lifetime and `stopVat` + // releases it when terminating. A vat retired without going through + // `stopVat` would leave its root pinned, and so uncollectable, forever. + expect(mockKernelStore.unpinObject).toHaveBeenCalledWith('ko1'); + }); + + it('throws for a vat that is neither running nor persisted', async () => { + mockKernelStore.isVatActive.mockReturnValue(false); + + await expect(vatManager.terminateVat('v9')).rejects.toThrow( + VatNotFoundError, + ); + expect(mockKernelStore.markVatAsTerminated).not.toHaveBeenCalled(); + }); + }); }); describe('restartVat', () => { From 6819b5d3bfa39ae6e534408ebeaf4c6ec3576213 Mon Sep 17 00:00:00 2001 From: grypez <143971198+grypez@users.noreply.github.com> Date: Mon, 24 Aug 2026 12:43:25 -0400 Subject: [PATCH 2/2] fix(ocap-kernel): finish retiring a vat that is not running MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `terminateVat` went through `stopVat`, so a vat that is persisted but not running threw `VatNotFoundError` and could not be retired at all. It now does everything `stopVat` does apart from stopping a worker. The record goes. `markVatAsTerminated` alone does not retire a vat: the cleanup it schedules walks keys prefixed `${vatId}.`, which never matches `vatConfig.${vatId}`, and that record is what the next boot restores from. The decider promises are rejected, carrying the termination reason. `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. The root pin goes. `launchVat` pins a vat's root for its lifetime and `stopVat` releases it when terminating, so a vat retired without passing through `stopVat` left its root pinned, and so uncollectable, for the life of the store. `removeVatFromSubcluster` no longer reports a vat that is in no subcluster. It is reached from `deleteVat` while a vat is being discarded — the one moment a failure cannot be retried past — and a vat that is in no subcluster is already in the state it asks for. Co-Authored-By: Claude Opus 5 --- packages/ocap-kernel/CHANGELOG.md | 5 ++ .../src/store/methods/subclusters.ts | 10 +++- packages/ocap-kernel/src/vats/VatManager.ts | 46 ++++++++++++++++++- 3 files changed, 58 insertions(+), 3 deletions(-) diff --git a/packages/ocap-kernel/CHANGELOG.md b/packages/ocap-kernel/CHANGELOG.md index 726948e974..302a2a6780 100644 --- a/packages/ocap-kernel/CHANGELOG.md +++ b/packages/ocap-kernel/CHANGELOG.md @@ -61,6 +61,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `terminateVat` now retires a vat that is persisted but not running, instead of throwing `VatNotFoundError` ([#1030](https://github.com/MetaMask/ocap-kernel/pull/1030)) + - Reachable through `restartVat`: it stops the vat and then runs it again, and a relaunch that fails leaves the vat gone from the running map with its record, its own store and its root pin all still in place. Such a vat could not be terminated at all, so the only way to be rid of one was to discard the whole store, and `terminateSubcluster` — which walks persisted membership — rejected part-way through on reaching one, after deleting the system-subcluster mapping and before removing the subcluster record + - Retiring one does everything `stopVat` does apart from stopping a worker: it discards the vat's persisted record, rejects the promises the vat was deciding, and releases the pin `launchVat` took on its root. Marking it terminated is not enough on its own — the deferred cleanup that follows walks keys prefixed `${vatId}.`, which never matches the `vatConfig.${vatId}` that decides whether the next boot restores the vat, and it states that its caller has already rejected those promises + - A vat that is neither running nor persisted still throws + - `removeVatFromSubcluster` no longer reports a vat that belongs to no subcluster. It is reached from `deleteVat` while a vat is being discarded, which is the one moment a failure cannot be retried past - A `notify`, GC action, or `bringOutYourDead` addressed to an endpoint that is not running is skipped instead of killing the run loop ([#1029](https://github.com/MetaMask/ocap-kernel/pull/1029)) - These deliveries looked up their endpoint unguarded, so an endpoint named by persisted state but absent from the running kernel threw `VatNotFoundError` from inside the crank, which killed the run loop permanently. Because the crank was rolled back the item was restored to the queue, so the next boot dequeued it and died too - Reached by a peer's routine remote GC. A kernel answers a peer's `bringOutYourDead` by scheduling a reap against the remote it came from, and the reap queue is persisted. `scheduleReap` does not wake a parked run loop, so on an otherwise idle kernel that reap sits in the queue until something else gives the loop work — and a kernel shut down in the meantime carries it into the next incarnation. That incarnation starts its run loop inside `Kernel.make`, before an embedder can call `initRemoteComms`, which is what restores remote handles; reaps are taken ahead of the run queue, so the loop's first act is to deliver one addressed to a remote that does not exist yet. The kernel is dead before `Kernel.make` returns, and stays dead on every boot after that diff --git a/packages/ocap-kernel/src/store/methods/subclusters.ts b/packages/ocap-kernel/src/store/methods/subclusters.ts index 7d77d936a6..d5f4472cde 100644 --- a/packages/ocap-kernel/src/store/methods/subclusters.ts +++ b/packages/ocap-kernel/src/store/methods/subclusters.ts @@ -239,11 +239,17 @@ export function getSubclusterMethods(ctx: StoreContext) { /** * Removes a vat from its subcluster. * + * A vat that belongs to no subcluster is already in the state this asks for, + * so it is left alone rather than reported: `deleteVat` reaches here while + * discarding a vat, which is the one moment a failure cannot be retried past. + * * @param vatId - The ID of the vat to remove. */ function removeVatFromSubcluster(vatId: VatId): void { - const subclusterId = getVatSubcluster(vatId); - deleteSubclusterVat(subclusterId, vatId); + const subclusterId = getVatToSubclusterMap()[vatId]; + if (subclusterId) { + deleteSubclusterVat(subclusterId, vatId); + } } // System subcluster mapping methods diff --git a/packages/ocap-kernel/src/vats/VatManager.ts b/packages/ocap-kernel/src/vats/VatManager.ts index f080dbf177..ff5e56cee0 100644 --- a/packages/ocap-kernel/src/vats/VatManager.ts +++ b/packages/ocap-kernel/src/vats/VatManager.ts @@ -8,6 +8,7 @@ import { stringify } from '@metamask/kernel-utils'; import { Logger, splitLoggerStream } from '@metamask/logger'; import type { KernelQueue } from '../KernelQueue.ts'; +import { makeKernelError } from '../liveslots/kernel-marshal.ts'; import type { KernelStore } from '../store/index.ts'; import type { VatId, @@ -202,15 +203,58 @@ export class VatManager { this.#vats.delete(vatId); } + /** + * Retire a persisted vat that is not running. + * + * A vat can be persisted without running: `restartVat` stops it and then runs + * it again, and a relaunch that fails leaves the vat gone from the running + * map with its record, its own store and its root pin all still in place. + * Such a vat never reaches `VatHandle.terminate`, which is where a running + * vat's records are discarded and the promises it was deciding are rejected, + * and nobody else does either: the deferred `cleanupTerminatedVat` states + * outright that its caller has already rejected those promises, and it walks + * keys prefixed `${vatId}.`, which never matches the `vatConfig.${vatId}` + * that decides whether the next boot restores this vat. + * + * Everything `stopVat` does apart from stopping a worker, therefore. + * + * @param vatId - The ID of the vat. + * @param reason - The reason for the termination, if any. + */ + #retirePersistedVat(vatId: VatId, reason?: CapData): void { + const terminationError = reason + ? new Error(`Vat termination: ${reason.body}`) + : new VatDeletedError(vatId); + this.releaseVatRootPin(vatId); + const failure = makeKernelError('VAT_TERMINATED', terminationError.message); + for (const kpid of this.#kernelStore.getPromisesByDecider(vatId)) { + this.#kernelQueue.resolvePromises(vatId, [[kpid, true, failure]]); + } + this.#kernelStore.deleteVat(vatId); + } + /** * Terminate a vat with extreme prejudice. * + * Terminates a persisted vat that is not running as readily as one that is + * (see `#retirePersistedVat`) — otherwise the only way to be rid of one is to + * discard the whole store, and `SubclusterManager.terminateSubcluster`, which + * walks persisted membership, strands every subcluster containing one. + * * @param vatId - The ID of the vat. * @param reason - If the vat is being terminated, the reason for the termination. */ async terminateVat(vatId: VatId, reason?: CapData): Promise { await this.#kernelQueue.waitForCrank(); - await this.stopVat(vatId, true, reason); + if (this.hasVat(vatId)) { + await this.stopVat(vatId, true, reason); + } else if (this.#kernelStore.isVatActive(vatId)) { + this.#retirePersistedVat(vatId, reason); + } else { + // 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); + } // Mark for deletion (which will happen later, in vat-cleanup events) this.#kernelStore.markVatAsTerminated(vatId); }