From db8768aefe9da45aeda590ab1df7a7af8c6e1982 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Tue, 15 Sep 2026 19:59:41 +0200 Subject: [PATCH 1/5] fix(ocap-kernel): record a vat's death in one synchronous step MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A vat's death is four writes: the promises it was deciding rejected, its root unpinned, its config and store dropped, the terminated mark set. They were spread across `stopVat` and `VatHandle.terminate` with awaits between them, so a failure part-way left a state nothing recovers from. The sharpest is marked terminated while `vatConfig` survives — only `deleteVat` removes that row, while the cleanup the mark schedules sweeps `${vatId}.` keys, which never match `vatConfig.${vatId}` — and such a vat reads as active again the moment cleanup drops the mark. `#retireVat` does all four with no await between them, before the worker is touched, and the mark happens even if an earlier write throws. 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 one still waiting to find out. It runs in a `finally`, so a record that fails cannot leave a worker running either. `stopVat` no longer insists on a live handle when terminating, which is what lets a vat left behind by a failed relaunch be retired at all, and `launchVat` tears its worker down when the kernel-side registration fails. `VatHandle.terminate` is now only the handle's own business: reject the callers waiting on commands the worker will never answer, then close the channel. Supersedes #1030 by @grypez. Co-Authored-By: Claude Opus 5 (1M context) --- packages/ocap-kernel/CHANGELOG.md | 3 + packages/ocap-kernel/src/Kernel.ts | 6 +- .../src/store/methods/subclusters.test.ts | 12 +- .../src/store/methods/subclusters.ts | 10 +- packages/ocap-kernel/src/vats/VatHandle.ts | 41 ++-- .../ocap-kernel/src/vats/VatManager.test.ts | 224 ++++++++++++++++++ packages/ocap-kernel/src/vats/VatManager.ts | 138 +++++++++-- 7 files changed, 377 insertions(+), 57 deletions(-) diff --git a/packages/ocap-kernel/CHANGELOG.md b/packages/ocap-kernel/CHANGELOG.md index cd4f20ae1d..f4260d785d 100644 --- a/packages/ocap-kernel/CHANGELOG.md +++ b/packages/ocap-kernel/CHANGELOG.md @@ -71,6 +71,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - A service or IO channel named `__proto__` in a cluster config reaches the bootstrap vat as an ordinary key of `services` instead of replacing that object's prototype ([#1121](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1121)) - A delivery to a remote peer is written down inside the crank making it and sent once that crank commits, instead of going out mid-crank. A crank that then failed took the kernel's record of the message back while the peer already had it, leaving the kernel's next send sequence behind what the peer had seen. The send sequence counters move with that record rather than in memory beside it, so a crank that rolls back takes them too, and outgoing messages reach the peer in sequence order however long a crank takes to commit — and across a restart, which sends the queue it inherits before anything numbered after it. Giving up on a peer covers a delivery whose crank has yet to commit, so it is not sent to a peer whose promise for it has just been rejected. A peer's messages go to the transport one at a time, since a send that has to dial and shake hands first can otherwise be overtaken by one issued after it, and one whose turn comes after a peer restart or a give-up has retired the queue is dropped rather than sent to a peer it is no longer owed to ([#1101](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1101)) - With the reference count audit crediting the crank buffer, a delivery that queued a message for later no longer reads as a leak, and the audit runs before the flush hands those messages on — so a violation stops the kernel while the crank can still be undone, rather than after the answers have gone out ([#1095](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1095)) +- A vat's death is recorded in one synchronous step, so a failure part-way through terminating it can no longer leave the store half-told. Marked terminated while its `vatConfig` row survived, a vat read as active again the moment the deferred cleanup dropped the mark ([#1093](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1093)) +- `terminateVat` retires a vat that is persisted but not running instead of throwing `VatNotFoundError`. A failed relaunch leaves one in that state, and it could then only be removed by discarding the whole store; `terminateSubcluster`, which walks persisted membership, gave up part-way through on reaching one ([#1093](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1093)) +- A vat whose worker launched but whose kernel-side registration failed is torn down and marked for cleanup, rather than left running with no record ([#1093](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1093)) - A kernel object whose owner retires or abandons it is now orphaned, so its owner and reference-count records no longer outlive the c-list entry they were reachable through; before, the next collection to visit such a kref read an entry that was gone and killed the run loop ([#1091](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1091)) - A crank that aborts keeps the writes that follow the abort — the vat's death, the collection after it — instead of autocommitting them one statement at a time outside any transaction: the run loop takes a savepoint for the crank and one for the delivery, and only the delivery is rolled back ([#1090](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1090)) - A vat that exits gracefully stays dead when the rest of its crank then throws; before, the run loop's catch rolled the death record back and would have relaunched a vat whose worker was gone and whose callers had been answered ([#1090](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1090)) diff --git a/packages/ocap-kernel/src/Kernel.ts b/packages/ocap-kernel/src/Kernel.ts index c041d060be..60e0b47c33 100644 --- a/packages/ocap-kernel/src/Kernel.ts +++ b/packages/ocap-kernel/src/Kernel.ts @@ -151,10 +151,8 @@ export class Kernel { // which would deadlock — this callback is invoked from within a crank. this.#kernelQueue = new KernelQueue( this.#kernelStore, - async (vatId, reason) => { - await this.#vatManager.stopVat(vatId, true, reason); - this.#kernelStore.markVatAsTerminated(vatId); - }, + async (vatId, reason) => + await this.#vatManager.stopVat(vatId, true, reason), ); this.#vatManager = new VatManager({ 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/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/VatHandle.ts b/packages/ocap-kernel/src/vats/VatHandle.ts index a1c18a72c2..44feaf550e 100644 --- a/packages/ocap-kernel/src/vats/VatHandle.ts +++ b/packages/ocap-kernel/src/vats/VatHandle.ts @@ -16,10 +16,7 @@ import { isJsonRpcNotification, isJsonRpcResponse } from '@metamask/utils'; import type { JsonRpcNotification, JsonRpcResponse } from '@metamask/utils'; import type { KernelQueue } from '../KernelQueue.ts'; -import { - makeKernelError, - makeFatalKernelError, -} from '../liveslots/kernel-marshal.ts'; +import { makeFatalKernelError } from '../liveslots/kernel-marshal.ts'; import { vatMethodSpecs, vatSyscallHandlers } from '../rpc/index.ts'; import type { PingVatResult, VatMethod } from '../rpc/index.ts'; import type { KernelStore } from '../store/index.ts'; @@ -68,18 +65,12 @@ export class VatHandle implements EndpointHandle { /** Optional list of allowed global names for vat endowments */ readonly #allowedGlobalNames: AllowedGlobalName[] | undefined; - /** Storage holding the kernel's persistent state */ - readonly #kernelStore: KernelStore; - /** Storage holding this vat's persistent state */ readonly #vatStore: VatStore; /** The vat's syscall */ readonly #vatSyscall: VatSyscall; - /** The kernel's queue */ - readonly #kernelQueue: KernelQueue; - readonly #rpcClient: RpcClient; readonly #rpcService: RpcService; @@ -111,9 +102,7 @@ export class VatHandle implements EndpointHandle { this.#logger = logger; this.#allowedGlobalNames = allowedGlobalNames; this.#vatStream = vatStream; - this.#kernelStore = kernelStore; this.#vatStore = kernelStore.makeVatStore(vatId); - this.#kernelQueue = kernelQueue; this.#vatSyscall = new VatSyscall({ vatId, kernelQueue, @@ -306,27 +295,25 @@ export class VatHandle implements EndpointHandle { } /** - * Terminates the vat. + * Closes this handle's channel to the vat worker. * - * @param terminating - If true, the vat is being killed permanently, so clean - * up its state and reject any promises that would be left dangling. + * Only the handle's own business: the store side of a vat's death belongs to + * `VatManager`, which writes it in one synchronous step. Split that way + * because the two have opposite failure requirements — ending a stream can + * fail and it does not matter, since the worker is already being killed, + * while a store left half-told about a vat is a state nothing recovers from. + * + * @param terminating - If true, the vat is being killed permanently, so + * callers waiting on a command it will never answer are told now. * @param error - The error to terminate the vat with. */ async terminate(terminating: boolean, error?: Error): Promise { - await this.#vatStream.end(error); - const terminationError = error ?? new VatDeletedError(this.vatId); if (terminating) { - // Reject promises exported to other vats for which this vat is the decider - const failure = makeKernelError( - 'VAT_TERMINATED', - terminationError.message, - ); - for (const kpid of this.#kernelStore.getPromisesByDecider(this.vatId)) { - this.#kernelQueue.resolvePromises(this.vatId, [[kpid, true, failure]]); - } - this.#rpcClient.rejectAll(terminationError); - this.#kernelStore.deleteVat(this.vatId); + // Ahead of the stream, so a stream that refuses to close does not leave + // these callers waiting on a worker that is already dead. + this.#rpcClient.rejectAll(error ?? new VatDeletedError(this.vatId)); } + await this.#vatStream.end(error); } /** diff --git a/packages/ocap-kernel/src/vats/VatManager.test.ts b/packages/ocap-kernel/src/vats/VatManager.test.ts index 21361f942c..9e5f0bd1c5 100644 --- a/packages/ocap-kernel/src/vats/VatManager.test.ts +++ b/packages/ocap-kernel/src/vats/VatManager.test.ts @@ -69,7 +69,11 @@ describe('VatManager', () => { })(), ), getVatSubcluster: vi.fn().mockReturnValue('s1'), + isVatActive: vi.fn().mockReturnValue(true), + isVatTerminated: vi.fn().mockReturnValue(false), markVatAsTerminated: vi.fn(), + deleteVat: vi.fn(), + getPromisesByDecider: vi.fn().mockReturnValue([]), getRootObject: vi.fn().mockReturnValue('ko1'), pinObject: vi.fn(), unpinObject: vi.fn(), @@ -80,6 +84,7 @@ describe('VatManager', () => { mockKernelQueue = { waitForCrank: vi.fn().mockResolvedValue(undefined), + resolvePromises: vi.fn(), } as unknown as Mocked; mockLogger = new Logger('test'); @@ -207,6 +212,55 @@ describe('VatManager', () => { expect((error as Error).cause).toBe(cause); }); + + it('tears the worker down when kernel-side registration fails', async () => { + const config = createMockVatConfig(); + const cause = new Error('initEndpoint threw'); + mockKernelStore.initEndpoint.mockImplementationOnce(() => { + throw cause; + }); + + const error = await vatManager + .launchVat(config, 'bob', 's1') + .catch((reason: unknown) => reason); + + expect((error as Error).message).toBe('Failed to launch vat v1 (bob)'); + expect((error as Error).cause).toBe(cause); + expect(mockPlatformServices.terminate).toHaveBeenCalledWith( + 'v1', + expect.any(Error), + ); + expect(vatHandles[0]?.terminate).toHaveBeenCalled(); + expect(mockKernelStore.markVatAsTerminated).toHaveBeenCalledWith('v1'); + expect(vatManager.hasVat('v1')).toBe(false); + }); + + it('still marks the vat terminated when the cleanup itself fails', async () => { + const config = createMockVatConfig(); + const cause = new Error('setVatConfig threw'); + mockKernelStore.setVatConfig.mockImplementationOnce(() => { + throw cause; + }); + // The handle teardown is the last step of `stopVat`, and the one whose + // failure escapes it. + vatHandles.push = ((handle: Mocked) => { + handle.terminate.mockRejectedValueOnce( + new Error('stream will not end'), + ); + return Array.prototype.push.call(vatHandles, handle); + }) as typeof vatHandles.push; + + const error = await vatManager + .launchVat(config, 'bob', 's1') + .catch((reason: unknown) => reason); + + expect((error as Error).message).toBe( + 'Failed to launch vat v1 (bob) (cleanup also failed)', + ); + // The launch failure, not the cleanup failure, is what the caller needs. + expect((error as Error).cause).toBe(cause); + expect(mockKernelStore.markVatAsTerminated).toHaveBeenCalledWith('v1'); + }); }); describe('runVat', () => { @@ -262,6 +316,100 @@ describe('VatManager', () => { expect(mockKernelStore.unpinObject).toHaveBeenCalledWith('ko1'); }); + it('records the whole death before touching the worker', async () => { + await vatManager.runVat('v1', createMockVatConfig()); + mockKernelStore.getPromisesByDecider.mockReturnValueOnce(['kp1']); + const order: string[] = []; + for (const [name, mock] of [ + ['resolvePromises', mockKernelQueue.resolvePromises], + ['unpinObject', mockKernelStore.unpinObject], + ['deleteVat', mockKernelStore.deleteVat], + ['markVatAsTerminated', mockKernelStore.markVatAsTerminated], + ] as const) { + mock.mockImplementation((() => { + order.push(name); + }) as never); + } + mockPlatformServices.terminate.mockImplementation(async () => { + order.push('terminateWorker'); + }); + + await vatManager.stopVat('v1', true); + + expect(order).toStrictEqual([ + 'resolvePromises', + 'unpinObject', + 'deleteVat', + 'markVatAsTerminated', + 'terminateWorker', + ]); + }); + + it('marks the vat terminated even when an earlier write throws', async () => { + await vatManager.runVat('v1', createMockVatConfig()); + mockKernelStore.deleteVat.mockImplementationOnce(() => { + throw new Error('deleteVat failed'); + }); + + await expect(vatManager.stopVat('v1', true)).rejects.toThrow( + 'deleteVat failed', + ); + + // Otherwise the store goes on calling the vat active while the kernel has + // no handle for it. + expect(mockKernelStore.markVatAsTerminated).toHaveBeenCalledWith('v1'); + expect(mockPlatformServices.terminate).toHaveBeenCalled(); + expect(vatManager.hasVat('v1')).toBe(false); + }); + + it('records nothing a second time', async () => { + await vatManager.runVat('v1', createMockVatConfig()); + mockKernelStore.isVatTerminated.mockReturnValue(true); + + await vatManager.stopVat('v1', true); + + // `deleteVat` fails on a vat whose subcluster mapping the first call + // removed, and a second unpin would spend a pin this vat no longer holds. + expect(mockKernelStore.deleteVat).not.toHaveBeenCalled(); + expect(mockKernelStore.unpinObject).not.toHaveBeenCalled(); + }); + + it('keeps the records across a restart', async () => { + await vatManager.runVat('v1', createMockVatConfig()); + + await vatManager.stopVat('v1', false); + + expect(mockKernelStore.deleteVat).not.toHaveBeenCalled(); + expect(mockKernelStore.markVatAsTerminated).not.toHaveBeenCalled(); + }); + + it.each([ + { + step: 'unpinning the root', + arrange: () => { + mockKernelStore.unpinObject.mockImplementationOnce(() => { + throw new Error('unpin failed'); + }); + }, + }, + { + step: 'terminating the handle', + arrange: () => { + vatHandles[0]?.terminate.mockRejectedValueOnce( + new Error('terminate failed'), + ); + }, + }, + ])('forgets the vat when $step throws', async ({ arrange }) => { + await vatManager.runVat('v1', createMockVatConfig()); + arrange(); + + await expect(vatManager.stopVat('v1', true)).rejects.toThrow('failed'); + + expect(vatManager.hasVat('v1')).toBe(false); + expect(vatManager.getVatIds()).toStrictEqual([]); + }); + it('stops a vat for termination with reason', async () => { const config = createMockVatConfig(); await vatManager.runVat('v1', config); @@ -345,6 +493,82 @@ describe('VatManager', () => { expect.objectContaining({ message: 'Vat termination: Custom reason' }), ); }); + + describe('a vat that is persisted but not running', () => { + /** + * Leave the vat in the state a failed relaunch does: gone from the + * running map, with its record, its own store and its root pin all still + * in place. + */ + async function givenAFailedRestart(): 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', async () => { + await givenAFailedRestart(); + + await vatManager.terminateVat('v1'); + + expect(mockKernelStore.markVatAsTerminated).toHaveBeenCalledWith('v1'); + }); + + it('discards its persisted record', async () => { + await givenAFailedRestart(); + + await vatManager.terminateVat('v1'); + + // The cleanup the mark 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. + expect(mockKernelStore.deleteVat).toHaveBeenCalledWith('v1'); + }); + + it('rejects the promises it was deciding', async () => { + mockKernelStore.getPromisesByDecider.mockReturnValue(['kp1']); + await givenAFailedRestart(); + + await vatManager.terminateVat('v1', { + body: 'Custom reason', + slots: [], + }); + + // `cleanupTerminatedVat` deletes these promises' c-list entries and + // drops the decider's refcount on the understanding that its caller + // rejected them first. A promise left unresolved with a decider that no + // longer exists hangs its waiters for good. + expect(mockKernelQueue.resolvePromises).toHaveBeenCalledWith('v1', [ + [ + 'kp1', + true, + expect.objectContaining({ + body: expect.stringContaining('Custom reason'), + }), + ], + ]); + }); + + it('releases the pin its root was launched with', async () => { + await givenAFailedRestart(); + + await vatManager.terminateVat('v1'); + + 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', () => { diff --git a/packages/ocap-kernel/src/vats/VatManager.ts b/packages/ocap-kernel/src/vats/VatManager.ts index f080dbf177..a027b5203c 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, @@ -123,18 +124,44 @@ export class VatManager { cause: error, }); } - this.#kernelStore.initEndpoint(vatId); - const rootRef = this.#kernelStore.exportFromEndpoint( - vatId, - ROOT_OBJECT_VREF, - ); - // A root is addressable for as long as its vat lives, whether or not - // anyone currently imports it: the kernel's own API hands out root krefs - // and `getRootObject` resolves them through this c-list entry. Without a - // pin, GC would retire the entry the moment the last importer let go. - this.#kernelStore.pinObject(rootRef); - this.#kernelStore.setVatConfig(vatId, vatConfig); - return rootRef; + try { + this.#kernelStore.initEndpoint(vatId); + const rootRef = this.#kernelStore.exportFromEndpoint( + vatId, + ROOT_OBJECT_VREF, + ); + // A root is addressable for as long as its vat lives, whether or not + // anyone currently imports it: the kernel's own API hands out root krefs + // and `getRootObject` resolves them through this c-list entry. Without a + // pin, GC would retire the entry the moment the last importer let go. + this.#kernelStore.pinObject(rootRef); + this.#kernelStore.setVatConfig(vatId, vatConfig); + return rootRef; + } catch (error) { + // The worker is already running, so leaving it would strand a vat the + // kernel has no record of. + let stopFailure: unknown; + try { + await this.stopVat(vatId, true); + } catch (caught) { + stopFailure = caught; + this.#logger.error( + `Failed to stop vat ${vatId} after incomplete launch; its worker may still be running:`, + caught, + ); + } + // `stopVat` records the death itself, but it can refuse before it gets + // that far, and a partial launch is exactly the shape that reaches: no + // `vatConfig` row, so the store does not call the vat active. The mark is + // what makes the terminated-vat cleanup reclaim the endpoint counters and + // the root's c-list pair, so it is asserted rather than assumed. Marking + // an already-marked vat is a no-op. + this.#kernelStore.markVatAsTerminated(vatId); + throw new Error( + `Failed to launch vat ${vatId} (${vatName})${stopFailure ? ' (cleanup also failed)' : ''}`, + { cause: error }, + ); + } } /** @@ -184,22 +211,91 @@ export class VatManager { terminating: boolean, reason?: CapData, ): Promise { - const vat = this.getVat(vatId); + // A restart needs a live handle to read its config from and to come back + // into; an ending vat does not, and must not. A failed relaunch leaves a + // vat the store still lists and the kernel has no handle for, and retiring + // it is exactly what puts that right. + const vat = terminating ? this.#vats.get(vatId) : this.getVat(vatId); + if (terminating && !vat && !this.#kernelStore.isVatActive(vatId)) { + throw new VatNotFoundError(vatId); + } let terminationError: Error | undefined; if (reason) { terminationError = new Error(`Vat termination: ${reason.body}`); } else if (terminating) { terminationError = new VatDeletedError(vatId); } - if (terminating) { - // A restart keeps the pin: the same root comes back. - this.releaseVatRootPin(vatId); + try { + if (terminating) { + this.#retireVat(vatId, terminationError as Error); + } else { + // A restart keeps the pin and the records: the same vat, and the same + // root, are coming back. Only the handle goes. + this.#vats.delete(vatId); + } + } finally { + // Best-effort, and deliberately after the records: the worker is being + // killed either way, and a teardown that fails must not leave the + // kernel's account of the vat half-written — nor may a record that fails + // leave the worker running. + await this.#platformServices + .terminate(vatId, terminationError) + .catch(this.#logger.error); + await vat?.terminate(terminating, terminationError); } - await this.#platformServices - .terminate(vatId, terminationError) - .catch(this.#logger.error); - await vat.terminate(terminating, terminationError); + } + + /** + * Record a vat's death: everything the kernel has to remember about it, in + * one synchronous step. + * + * Synchronous is the point. A death is four writes — the promises it was + * deciding rejected, its root unpinned, its config and store dropped, the + * terminated mark set — and none means much without the others. Interleaved + * with awaits, a failure part-way leaves states nothing recovers from; the + * sharpest is marked-terminated while `vatConfig` survives, since only + * `deleteVat` removes that row and `cleanupTerminatedVat` sweeps `${vatId}.` + * keys, which never match `vatConfig.${vatId}`. Such a vat reads as active + * again the moment cleanup drops the mark. + * + * 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 one still waiting to + * find out. + * + * Calling it twice records nothing the second time. Two of the writes cannot + * be repeated: `deleteVat` fails on a vat whose subcluster mapping the first + * call removed, and a second `releaseVatRootPin` would spend a pin this vat + * no longer holds. + * + * @param vatId - The vat being retired. + * @param error - Why, for the rejections its subscribers are owed. + */ + #retireVat(vatId: VatId, error: Error): void { + // Ahead of the guard, and safe there because nothing below reads it: a vat + // the store already calls dead must not keep a handle the router would go + // on resolving. this.#vats.delete(vatId); + if (this.#kernelStore.isVatTerminated(vatId)) { + return; + } + try { + const failure = makeKernelError('VAT_TERMINATED', error.message); + // First, while the c-list this reads through is still there: subscribers + // are told rather than left waiting on a decider that no longer exists. + for (const kpid of this.#kernelStore.getPromisesByDecider(vatId)) { + this.#kernelQueue.resolvePromises(vatId, [[kpid, true, failure]]); + } + // The root is found through the c-list, so this stays ahead of the + // deletes. + this.releaseVatRootPin(vatId); + this.#kernelStore.deleteVat(vatId); + } finally { + // Last, and unconditional: the mark is what makes the vat eligible for + // `nextTerminatedVatCleanup`, which reclaims the c-list everything above + // needed. Without it, a write that failed leaves the store calling the + // vat active while the kernel has no handle for it. + this.#kernelStore.markVatAsTerminated(vatId); + } } /** @@ -211,8 +307,6 @@ export class VatManager { async terminateVat(vatId: VatId, reason?: CapData): Promise { await this.#kernelQueue.waitForCrank(); await this.stopVat(vatId, true, reason); - // Mark for deletion (which will happen later, in vat-cleanup events) - this.#kernelStore.markVatAsTerminated(vatId); } /** From 10ea11894a31431e0700a75b53cf005ced5199eb Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Tue, 15 Sep 2026 20:25:36 +0200 Subject: [PATCH 2/5] fix(ocap-kernel): do not mark a vat terminated after a failed retirement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The mark makes the vat eligible for `nextTerminatedVatCleanup`, which deletes its decider promises' c-list entries on the stated understanding that its caller has already rejected them. Marking in a `finally` therefore turned a failed rejection loop into subscribers that hang for good, and the retry that would have finished the job hit the already-terminated guard and did nothing. The mark is now simply the last step, so a failure leaves a vat that has not been retired, with its c-list intact and the step available to be tried again. `deleteVat` sits immediately before it: the two must not be separated. `stopVat` no longer lets a channel that will not close stand in for a store left half-written, and `launchVat`'s first catch stops the worker that `platformServices.launch` has already spawned — `runVat` records no handle to reach it by, so nothing else could. Co-Authored-By: Claude Opus 5 (1M context) --- .../kernel-test/src/vat-lifecycle.test.ts | 35 +++++++ packages/ocap-kernel/CHANGELOG.md | 2 +- .../ocap-kernel/src/vats/VatHandle.test.ts | 50 ++++++++++ .../ocap-kernel/src/vats/VatManager.test.ts | 72 +++++++++++--- packages/ocap-kernel/src/vats/VatManager.ts | 98 +++++++++++-------- 5 files changed, 204 insertions(+), 53 deletions(-) diff --git a/packages/kernel-test/src/vat-lifecycle.test.ts b/packages/kernel-test/src/vat-lifecycle.test.ts index 209da17e57..99ddfa6cae 100644 --- a/packages/kernel-test/src/vat-lifecycle.test.ts +++ b/packages/kernel-test/src/vat-lifecycle.test.ts @@ -144,4 +144,39 @@ describe('Vat Lifecycle', { timeout: 30_000 }, () => { // The target root object should be cleaned up after GC expect(kernelStore.getRootObject(deadVatId)).toBeUndefined(); }); + + it('leaves no record of a terminated vat for the next boot to restore', async () => { + const kernelDatabase = await makeSQLKernelDatabase({ + dbFilename: ':memory:', + }); + const kernel = await makeKernel( + kernelDatabase, + true, + logger.logger.subLogger({ tags: ['test'] }), + ); + const kernelStore = makeKernelStore(kernelDatabase); + + await runTestVats(kernel, { + bootstrap: 'main', + vats: { + main: { + bundleSpec: getBundleSpec('logger-vat'), + parameters: { name: 'DoomedVat' }, + }, + }, + }); + await waitUntilQuiescent(); + const vatId = kernel.getVats()[0]?.id as string; + + await kernel.terminateVat(vatId); + await waitUntilQuiescent(); + // The mark is what schedules this, and it is dropped once the sweep is + // done — so the `vatConfig` row it never touches is the thing that decides + // whether the vat is active after it. + kernel.collectGarbage(); + await waitUntilQuiescent(); + + expect(kernelStore.isVatActive(vatId)).toBe(false); + expect([...kernelStore.getAllVatRecords()]).toStrictEqual([]); + }); }); diff --git a/packages/ocap-kernel/CHANGELOG.md b/packages/ocap-kernel/CHANGELOG.md index f4260d785d..5e256989df 100644 --- a/packages/ocap-kernel/CHANGELOG.md +++ b/packages/ocap-kernel/CHANGELOG.md @@ -73,7 +73,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - With the reference count audit crediting the crank buffer, a delivery that queued a message for later no longer reads as a leak, and the audit runs before the flush hands those messages on — so a violation stops the kernel while the crank can still be undone, rather than after the answers have gone out ([#1095](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1095)) - A vat's death is recorded in one synchronous step, so a failure part-way through terminating it can no longer leave the store half-told. Marked terminated while its `vatConfig` row survived, a vat read as active again the moment the deferred cleanup dropped the mark ([#1093](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1093)) - `terminateVat` retires a vat that is persisted but not running instead of throwing `VatNotFoundError`. A failed relaunch leaves one in that state, and it could then only be removed by discarding the whole store; `terminateSubcluster`, which walks persisted membership, gave up part-way through on reaching one ([#1093](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1093)) -- A vat whose worker launched but whose kernel-side registration failed is torn down and marked for cleanup, rather than left running with no record ([#1093](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1093)) +- A vat whose worker launched but whose kernel-side setup then failed has that worker stopped, rather than left running with no record — whether the failure came from the handshake with the worker or from registering the vat afterwards ([#1093](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1093)) - A kernel object whose owner retires or abandons it is now orphaned, so its owner and reference-count records no longer outlive the c-list entry they were reachable through; before, the next collection to visit such a kref read an entry that was gone and killed the run loop ([#1091](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1091)) - A crank that aborts keeps the writes that follow the abort — the vat's death, the collection after it — instead of autocommitting them one statement at a time outside any transaction: the run loop takes a savepoint for the crank and one for the delivery, and only the delivery is rolled back ([#1090](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1090)) - A vat that exits gracefully stays dead when the rest of its crank then throws; before, the run loop's catch rolled the death record back and would have relaunched a vat whose worker was gone and whose callers had been answered ([#1090](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1090)) diff --git a/packages/ocap-kernel/src/vats/VatHandle.test.ts b/packages/ocap-kernel/src/vats/VatHandle.test.ts index c3975cc0d2..ec7519fb4a 100644 --- a/packages/ocap-kernel/src/vats/VatHandle.test.ts +++ b/packages/ocap-kernel/src/vats/VatHandle.test.ts @@ -295,5 +295,55 @@ describe('VatHandle', () => { value: undefined, }); }); + + it('rejects pending commands before closing the channel', async () => { + const inner = await TestDuplexStream.make( + () => undefined, + { validateInput: isJsonRpcMessage }, + ); + let releaseEnd = (): void => undefined; + const stalling = new Promise((resolve) => { + releaseEnd = resolve; + }); + const vat = await VatHandle.make({ + kernelQueue: null as unknown as KernelQueue, + kernelStore: mockKernelStore, + vatId: 'v0', + vatConfig: { sourceSpec: 'not-really-there.js' }, + // `end` never settles until released, so a rejection that arrives + // before then is one that did not wait for the channel to close. + vatStream: { + drain: inner.drain.bind(inner), + write: inner.write.bind(inner), + end: async () => stalling, + } as unknown as typeof inner, + }); + sendVatCommandMock.mockRestore(); + const messagePromise = vat.sendVatCommand({ + method: 'ping' as const, + params: [], + }); + + const terminating = vat.terminate(true); + + await expect(messagePromise).rejects.toThrow('Vat was deleted.'); + releaseEnd(); + await terminating; + }); + + it('leaves pending commands alone across a restart', async () => { + const { vat } = await makeVat(); + const settled = vi.fn(); + // eslint-disable-next-line promise/catch-or-return + vat + .sendVatCommand({ method: 'ping' as const, params: [] }) + .then(settled, settled); + + await vat.terminate(false); + await delay(10); + + // The same vat is coming back, and its answer with it. + expect(settled).not.toHaveBeenCalled(); + }); }); }); diff --git a/packages/ocap-kernel/src/vats/VatManager.test.ts b/packages/ocap-kernel/src/vats/VatManager.test.ts index 9e5f0bd1c5..adae6d3976 100644 --- a/packages/ocap-kernel/src/vats/VatManager.test.ts +++ b/packages/ocap-kernel/src/vats/VatManager.test.ts @@ -185,6 +185,18 @@ describe('VatManager', () => { expect(kref).toBe('ko1'); }); + it('stops the worker when the handle never comes back', async () => { + makeVatHandleMock.mockRejectedValueOnce(new Error('handshake timed out')); + + await expect( + vatManager.launchVat(createMockVatConfig(), 'bob', 's1'), + ).rejects.toThrow('Failed to launch vat v1 (bob)'); + + // The worker is spawned before anything that can fail here, and no handle + // was recorded, so this is the only chance to stop it. + expect(mockPlatformServices.terminate).toHaveBeenCalledWith('v1'); + }); + it('attributes a launch failure to the vat by id and config name', async () => { const config = createMockVatConfig(); const cause = new Error( @@ -241,14 +253,11 @@ describe('VatManager', () => { mockKernelStore.setVatConfig.mockImplementationOnce(() => { throw cause; }); - // The handle teardown is the last step of `stopVat`, and the one whose - // failure escapes it. - vatHandles.push = ((handle: Mocked) => { - handle.terminate.mockRejectedValueOnce( - new Error('stream will not end'), - ); - return Array.prototype.push.call(vatHandles, handle); - }) as typeof vatHandles.push; + // `stopVat` stops short of the mark when a write before it throws, which + // is what makes the assertion below about this catch rather than it. + mockKernelStore.deleteVat.mockImplementationOnce(() => { + throw new Error('deleteVat failed'); + }); const error = await vatManager .launchVat(config, 'bob', 's1') @@ -345,7 +354,31 @@ describe('VatManager', () => { ]); }); - it('marks the vat terminated even when an earlier write throws', async () => { + it('records the death with no yield point in it', async () => { + await vatManager.runVat('v1', createMockVatConfig()); + mockKernelStore.getPromisesByDecider.mockReturnValueOnce(['kp1']); + + const stopping = vatManager.stopVat('v1', true); + // Read before awaiting: everything the store has to be told is written in + // `stopVat`'s synchronous prefix, so a crank cannot land part-way through. + const writesBeforeTheFirstAwait = { + resolvePromises: mockKernelQueue.resolvePromises.mock.calls.length, + unpinObject: mockKernelStore.unpinObject.mock.calls.length, + deleteVat: mockKernelStore.deleteVat.mock.calls.length, + markVatAsTerminated: + mockKernelStore.markVatAsTerminated.mock.calls.length, + }; + await stopping; + + expect(writesBeforeTheFirstAwait).toStrictEqual({ + resolvePromises: 1, + unpinObject: 1, + deleteVat: 1, + markVatAsTerminated: 1, + }); + }); + + it('leaves the vat unmarked when an earlier write throws', async () => { await vatManager.runVat('v1', createMockVatConfig()); mockKernelStore.deleteVat.mockImplementationOnce(() => { throw new Error('deleteVat failed'); @@ -355,13 +388,28 @@ describe('VatManager', () => { 'deleteVat failed', ); - // Otherwise the store goes on calling the vat active while the kernel has - // no handle for it. - expect(mockKernelStore.markVatAsTerminated).toHaveBeenCalledWith('v1'); + // Marking it would make the deferred cleanup delete the decider + // promises' c-list entries believing they were rejected. Unmarked, the + // vat is simply not retired yet and the step can be tried again. + expect(mockKernelStore.markVatAsTerminated).not.toHaveBeenCalled(); expect(mockPlatformServices.terminate).toHaveBeenCalled(); expect(vatManager.hasVat('v1')).toBe(false); }); + it('reports the store failure rather than the channel failure', async () => { + await vatManager.runVat('v1', createMockVatConfig()); + mockKernelStore.deleteVat.mockImplementationOnce(() => { + throw new Error('deleteVat failed'); + }); + vatHandles[0]?.terminate.mockRejectedValueOnce( + new Error('stream will not end'), + ); + + await expect(vatManager.stopVat('v1', true)).rejects.toThrow( + 'deleteVat failed', + ); + }); + it('records nothing a second time', async () => { await vatManager.runVat('v1', createMockVatConfig()); mockKernelStore.isVatTerminated.mockReturnValue(true); diff --git a/packages/ocap-kernel/src/vats/VatManager.ts b/packages/ocap-kernel/src/vats/VatManager.ts index a027b5203c..0052d5107d 100644 --- a/packages/ocap-kernel/src/vats/VatManager.ts +++ b/packages/ocap-kernel/src/vats/VatManager.ts @@ -119,6 +119,18 @@ export class VatManager { try { await this.runVat(vatId, vatConfig); } catch (error) { + // `platformServices.launch` has already spawned the worker by the time + // anything below it can fail, and `runVat` records no handle to reach it + // by, so this is the only chance to stop it. `stopVat` is no use here: + // there is neither a handle nor a `vatConfig` row for it to work from. + await this.#platformServices + .terminate(vatId) + .catch((stopError: unknown) => + this.#logger.error( + `Failed to stop the worker for vat ${vatId} after an incomplete launch:`, + stopError, + ), + ); // Attribute the failure to the specific vat by kernel id and name. throw new Error(`Failed to launch vat ${vatId} (${vatName})`, { cause: error, @@ -150,12 +162,11 @@ export class VatManager { caught, ); } - // `stopVat` records the death itself, but it can refuse before it gets - // that far, and a partial launch is exactly the shape that reaches: no - // `vatConfig` row, so the store does not call the vat active. The mark is - // what makes the terminated-vat cleanup reclaim the endpoint counters and - // the root's c-list pair, so it is asserted rather than assumed. Marking - // an already-marked vat is a no-op. + // `stopVat` marks the vat itself, but only if every write before the + // mark succeeded. The mark is what makes the terminated-vat cleanup + // reclaim the endpoint counters and the root's c-list pair, and a vat + // this young has no decider promises for that cleanup to get wrong, so + // here it is asserted rather than assumed. this.#kernelStore.markVatAsTerminated(vatId); throw new Error( `Failed to launch vat ${vatId} (${vatName})${stopFailure ? ' (cleanup also failed)' : ''}`, @@ -225,6 +236,7 @@ export class VatManager { } else if (terminating) { terminationError = new VatDeletedError(vatId); } + let recordFailure: Error | undefined; try { if (terminating) { this.#retireVat(vatId, terminationError as Error); @@ -233,15 +245,26 @@ export class VatManager { // root, are coming back. Only the handle goes. this.#vats.delete(vatId); } - } finally { - // Best-effort, and deliberately after the records: the worker is being - // killed either way, and a teardown that fails must not leave the - // kernel's account of the vat half-written — nor may a record that fails - // leave the worker running. - await this.#platformServices - .terminate(vatId, terminationError) - .catch(this.#logger.error); + } catch (error) { + // Held rather than thrown: the worker is being killed either way, and a + // record that failed must not leave one running. + recordFailure = error as Error; + } + await this.#platformServices + .terminate(vatId, terminationError) + .catch(this.#logger.error); + try { await vat?.terminate(terminating, terminationError); + } catch (error) { + // A channel that will not close is survivable and a store left + // half-written is not, so the latter is what the caller hears about. + if (recordFailure === undefined) { + throw error; + } + this.#logger.error(`Channel to vat ${vatId} would not close:`, error); + } + if (recordFailure !== undefined) { + throw recordFailure; } } @@ -252,20 +275,23 @@ export class VatManager { * Synchronous is the point. A death is four writes — the promises it was * deciding rejected, its root unpinned, its config and store dropped, the * terminated mark set — and none means much without the others. Interleaved - * with awaits, a failure part-way leaves states nothing recovers from; the - * sharpest is marked-terminated while `vatConfig` survives, since only - * `deleteVat` removes that row and `cleanupTerminatedVat` sweeps `${vatId}.` - * keys, which never match `vatConfig.${vatId}`. Such a vat reads as active - * again the moment cleanup drops the mark. + * with awaits, as they used to be, a crank lands between them and reads a vat + * that is half dead. * * 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 one still waiting to * find out. * - * Calling it twice records nothing the second time. Two of the writes cannot - * be repeated: `deleteVat` fails on a vat whose subcluster mapping the first - * call removed, and a second `releaseVatRootPin` would spend a pin this vat - * no longer holds. + * The mark goes last, and is not attempted if anything above it throws. It is + * what makes the vat eligible for `nextTerminatedVatCleanup`, and that + * cleanup deletes the decider promises' c-list entries on the stated + * understanding that its caller has already rejected them. A vat marked after + * those rejections failed is one whose subscribers hang for good, so a + * failure part-way leaves a vat that has simply not been retired — its c-list + * intact, and the whole step available to be tried again. + * + * Calling it twice records nothing the second time: a second + * `releaseVatRootPin` would spend a pin this vat no longer holds. * * @param vatId - The vat being retired. * @param error - Why, for the rejections its subscribers are owed. @@ -278,24 +304,16 @@ export class VatManager { if (this.#kernelStore.isVatTerminated(vatId)) { return; } - try { - const failure = makeKernelError('VAT_TERMINATED', error.message); - // First, while the c-list this reads through is still there: subscribers - // are told rather than left waiting on a decider that no longer exists. - for (const kpid of this.#kernelStore.getPromisesByDecider(vatId)) { - this.#kernelQueue.resolvePromises(vatId, [[kpid, true, failure]]); - } - // The root is found through the c-list, so this stays ahead of the - // deletes. - this.releaseVatRootPin(vatId); - this.#kernelStore.deleteVat(vatId); - } finally { - // Last, and unconditional: the mark is what makes the vat eligible for - // `nextTerminatedVatCleanup`, which reclaims the c-list everything above - // needed. Without it, a write that failed leaves the store calling the - // vat active while the kernel has no handle for it. - this.#kernelStore.markVatAsTerminated(vatId); + const failure = makeKernelError('VAT_TERMINATED', error.message); + for (const kpid of this.#kernelStore.getPromisesByDecider(vatId)) { + this.#kernelQueue.resolvePromises(vatId, [[kpid, true, failure]]); } + this.releaseVatRootPin(vatId); + // Together, always. `cleanupTerminatedVat` sweeps `${vatId}.` keys, which + // never match `vatConfig.${vatId}`, so a vat left with that row reads as + // active again the moment the cleanup drops the mark. + this.#kernelStore.deleteVat(vatId); + this.#kernelStore.markVatAsTerminated(vatId); } /** From 26e9803fd49d8fbd5e03e7d4c86ae8c225db5311 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Fri, 25 Sep 2026 20:04:13 +0200 Subject: [PATCH 3/5] fix(ocap-kernel): retire a vat whose channel to its worker fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moving the store side of a death out of `VatHandle.terminate` left the one caller that had nothing else to fall back on: the catch on the handle's own drain, which is how the kernel hears that a channel has failed. Left as it was, such a vat kept its handle, its `vatConfig` row and the promises it was deciding, so the kernel went on delivering over a channel that was gone and the next boot brought it back. The handle now takes an `onStreamFailure` and does the part that is its own — rejecting the callers waiting on commands the worker will never answer, since a crank blocked on one of them is what the retirement would wait behind — and `VatManager` retires the vat exactly as a requested termination would. Between cranks, like `terminateVat`: `#retireVat` writes synchronously, so run inside an open delivery savepoint the whole death is undone by any crank that goes on to abort, while the handle it dropped stays dropped. Reports are matched against the handle that made them, since a restart puts a new handle under the same vat id, and a vat the kernel is itself stopping reports the same way. The terminated mark moves ahead of `deleteVat`, which drops the `vatConfig` row `stopVat` reads to decide there is still a vat here to retire: a mark not reached by then could never be set, and the c-list it gates never swept. Every way a retirement can now fail leaves either the mark set, or the row that lets the whole step be tried again — so the idempotence guard covers only the writes that may happen once, and `deleteVat` runs again to carry an interrupted retirement to the end. `cleanupTerminatedVat` discards the `vatConfig` row too, so a vat marked but not discarded cannot read as active again once the sweep drops its mark. `runVat` stops a worker left over from a failed handshake, which `launchVat` alone used to do and only for its own launches, and which asked both runtimes to stop a worker they had never registered. Its unconditional `markVatAsTerminated` goes: `stopVat` sets the mark early enough that asserting it afterwards could only add it where `stopVat` stopped short of `deleteVat`. Co-Authored-By: Claude Opus 5 (1M context) --- .../kernel-test/src/vat-lifecycle.test.ts | 43 +++ packages/ocap-kernel/CHANGELOG.md | 3 + packages/ocap-kernel/src/store/index.test.ts | 16 ++ packages/ocap-kernel/src/store/methods/vat.ts | 6 + .../ocap-kernel/src/vats/VatHandle.test.ts | 33 +++ packages/ocap-kernel/src/vats/VatHandle.ts | 36 ++- .../ocap-kernel/src/vats/VatManager.test.ts | 193 ++++++++++++-- packages/ocap-kernel/src/vats/VatManager.ts | 246 ++++++++++++++---- 8 files changed, 498 insertions(+), 78 deletions(-) diff --git a/packages/kernel-test/src/vat-lifecycle.test.ts b/packages/kernel-test/src/vat-lifecycle.test.ts index 99ddfa6cae..e943ea76d1 100644 --- a/packages/kernel-test/src/vat-lifecycle.test.ts +++ b/packages/kernel-test/src/vat-lifecycle.test.ts @@ -1,6 +1,8 @@ +import { NodejsPlatformServices } from '@metamask/kernel-node-runtime'; import { makeSQLKernelDatabase } from '@metamask/kernel-store/sqlite/nodejs'; import { waitUntilQuiescent } from '@metamask/kernel-utils'; import { makeKernelStore } from '@metamask/ocap-kernel'; +import type { VatId } from '@metamask/ocap-kernel'; import { describe, expect, it, beforeEach } from 'vitest'; import { @@ -179,4 +181,45 @@ describe('Vat Lifecycle', { timeout: 30_000 }, () => { expect(kernelStore.isVatActive(vatId)).toBe(false); expect([...kernelStore.getAllVatRecords()]).toStrictEqual([]); }); + + it('retires a vat whose channel to its worker fails', async () => { + const kernelDatabase = await makeSQLKernelDatabase({ + dbFilename: ':memory:', + }); + const platformServices = new NodejsPlatformServices({ + logger: logger.logger.subLogger({ tags: ['vat-worker-manager'] }), + }); + const kernel = await makeKernel( + kernelDatabase, + true, + logger.logger.subLogger({ tags: ['test'] }), + undefined, + platformServices, + ); + const kernelStore = makeKernelStore(kernelDatabase); + + await runTestVats(kernel, { + bootstrap: 'main', + vats: { + main: { + bundleSpec: getBundleSpec('logger-vat'), + parameters: { name: 'DoomedVat' }, + }, + }, + }); + await waitUntilQuiescent(); + const vatId = kernel.getVats()[0]?.id as VatId; + + // Nothing listens for a worker that dies outright, so what the kernel can + // actually see is a worker still sending: this is that worker's own + // message event, carrying something the reader will not take. + platformServices.workers.get(vatId)?.worker.emit('message', NaN); + await waitUntilQuiescent(); + kernel.collectGarbage(); + await waitUntilQuiescent(); + + expect(kernel.getVats()).toStrictEqual([]); + expect(kernelStore.isVatActive(vatId)).toBe(false); + expect([...kernelStore.getAllVatRecords()]).toStrictEqual([]); + }); }); diff --git a/packages/ocap-kernel/CHANGELOG.md b/packages/ocap-kernel/CHANGELOG.md index 5e256989df..dda9e60679 100644 --- a/packages/ocap-kernel/CHANGELOG.md +++ b/packages/ocap-kernel/CHANGELOG.md @@ -74,6 +74,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - A vat's death is recorded in one synchronous step, so a failure part-way through terminating it can no longer leave the store half-told. Marked terminated while its `vatConfig` row survived, a vat read as active again the moment the deferred cleanup dropped the mark ([#1093](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1093)) - `terminateVat` retires a vat that is persisted but not running instead of throwing `VatNotFoundError`. A failed relaunch leaves one in that state, and it could then only be removed by discarding the whole store; `terminateSubcluster`, which walks persisted membership, gave up part-way through on reaching one ([#1093](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1093)) - A vat whose worker launched but whose kernel-side setup then failed has that worker stopped, rather than left running with no record — whether the failure came from the handshake with the worker or from registering the vat afterwards ([#1093](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1093)) +- A vat whose channel to its worker fails is retired in full, like one the kernel was asked to terminate. It used to keep its handle, so the kernel went on delivering over a channel that was gone, along with its root's pin and its whole c-list ([#1093](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1093)) +- `cleanupTerminatedVat` discards a terminated vat's `vatConfig` row along with the rest of it. A vat whose retirement was marked but did not finish came back to life as the cleanup dropped the mark, and the next boot relaunched it with everything else about it already swept away ([#1093](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1093)) +- A vat whose worker survives a failed handshake has that worker stopped by `runVat`, so `initializeAllVats` and `restartVat` no longer strand one where only `launchVat` cleaned up ([#1093](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1093)) - A kernel object whose owner retires or abandons it is now orphaned, so its owner and reference-count records no longer outlive the c-list entry they were reachable through; before, the next collection to visit such a kref read an entry that was gone and killed the run loop ([#1091](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1091)) - A crank that aborts keeps the writes that follow the abort — the vat's death, the collection after it — instead of autocommitting them one statement at a time outside any transaction: the run loop takes a savepoint for the crank and one for the delivery, and only the delivery is rolled back ([#1090](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1090)) - A vat that exits gracefully stays dead when the rest of its crank then throws; before, the run loop's catch rolled the death record back and would have relaunched a vat whose worker was gone and whose callers had been answered ([#1090](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1090)) diff --git a/packages/ocap-kernel/src/store/index.test.ts b/packages/ocap-kernel/src/store/index.test.ts index 632d599d73..3b48929e51 100644 --- a/packages/ocap-kernel/src/store/index.test.ts +++ b/packages/ocap-kernel/src/store/index.test.ts @@ -616,6 +616,22 @@ describe('kernel store', () => { }); }); + describe('cleanupTerminatedVat', () => { + it('discards the config of a vat whose retirement did not', () => { + const ks = makeKernelStore(mockKernelDatabase); + ks.setVatConfig('v1', { sourceSpec: 'vat.js' }); + ks.markVatAsTerminated('v1'); + + ks.cleanupTerminatedVat('v1'); + + // The sweep drops the mark on its way out, and `isVatActive` reads only + // this row — left behind, the vat reads as running again and the next + // boot relaunches it with everything else about it swept away. + expect(ks.isVatActive('v1')).toBe(false); + expect(ks.isVatTerminated('v1')).toBe(false); + }); + }); + describe('incarnation ID', () => { it('generates a new incarnation ID on first call', () => { const ks = makeKernelStore(mockKernelDatabase); diff --git a/packages/ocap-kernel/src/store/methods/vat.ts b/packages/ocap-kernel/src/store/methods/vat.ts index e603ce5248..9a1dc10318 100644 --- a/packages/ocap-kernel/src/store/methods/vat.ts +++ b/packages/ocap-kernel/src/store/methods/vat.ts @@ -312,6 +312,12 @@ export function getVatMethods(ctx: StoreContext) { // Clean up any remaining c-list entries and vat-specific counters deleteEndpoint(vatID); + // The sweep above matches `${vatID}.` keys, which `vatConfig.${vatID}` is + // not, and `isVatActive` reads nothing else. A vat retired by a caller that + // marked it and then failed to discard it would come back to life here as + // the mark is dropped, and be relaunched — gutted — on the next boot. + deleteVatConfig(vatID); + // Remove the vat from the terminated vats list forgetTerminatedVat(vatID); diff --git a/packages/ocap-kernel/src/vats/VatHandle.test.ts b/packages/ocap-kernel/src/vats/VatHandle.test.ts index ec7519fb4a..6e0b3f8285 100644 --- a/packages/ocap-kernel/src/vats/VatHandle.test.ts +++ b/packages/ocap-kernel/src/vats/VatHandle.test.ts @@ -33,9 +33,11 @@ let mockKernelStore: KernelStore; const makeVat = async ({ logger, dispatch, + onStreamFailure = () => undefined, }: { logger?: Logger; dispatch?: (input: unknown) => void | Promise; + onStreamFailure?: (error: Error) => void; } = {}): Promise<{ vat: VatHandle; stream: TestDuplexStream; @@ -54,6 +56,7 @@ const makeVat = async ({ vatConfig: { sourceSpec: 'not-really-there.js' }, vatStream, logger, + onStreamFailure, }), stream: vatStream, }; @@ -121,6 +124,35 @@ describe('VatHandle', () => { }), ); }); + + it('reports a dead channel to its owner', async () => { + const onStreamFailure = vi.fn(); + const { stream } = await makeVat({ onStreamFailure }); + + await stream.receiveInput(NaN); + await delay(10); + + expect(onStreamFailure).toHaveBeenCalledWith( + expect.objectContaining({ + message: 'Unexpected stream read error.', + }), + ); + }); + + it('rejects pending commands when the channel dies', async () => { + const { vat, stream } = await makeVat(); + sendVatCommandMock.mockRestore(); + const messagePromise = vat.sendVatCommand({ + method: 'ping' as const, + params: [], + }); + + await stream.receiveInput(NaN); + + await expect(messagePromise).rejects.toThrow( + 'Unexpected stream read error.', + ); + }); }); describe('ping', () => { @@ -317,6 +349,7 @@ describe('VatHandle', () => { write: inner.write.bind(inner), end: async () => stalling, } as unknown as typeof inner, + onStreamFailure: () => undefined, }); sendVatCommandMock.mockRestore(); const messagePromise = vat.sendVatCommand({ diff --git a/packages/ocap-kernel/src/vats/VatHandle.ts b/packages/ocap-kernel/src/vats/VatHandle.ts index 44feaf550e..8cf946361a 100644 --- a/packages/ocap-kernel/src/vats/VatHandle.ts +++ b/packages/ocap-kernel/src/vats/VatHandle.ts @@ -44,6 +44,11 @@ type VatConstructorProps = { kernelQueue: KernelQueue; logger?: Logger | undefined; allowedGlobalNames?: AllowedGlobalName[] | undefined; + /** + * Called when the channel to the worker fails, so that whoever keeps this + * vat's records can retire it. + */ + onStreamFailure: (error: Error) => void; }; /** @@ -71,6 +76,9 @@ export class VatHandle implements EndpointHandle { /** The vat's syscall */ readonly #vatSyscall: VatSyscall; + /** Told when the channel to the worker fails */ + readonly #onStreamFailure: (error: Error) => void; + readonly #rpcClient: RpcClient; readonly #rpcService: RpcService; @@ -86,6 +94,7 @@ export class VatHandle implements EndpointHandle { * @param params.kernelQueue - The kernel's queue. * @param params.logger - Optional logger for error and diagnostic output. * @param params.allowedGlobalNames - Optional list of allowed global names for vat endowments. + * @param params.onStreamFailure - Called when the channel to the worker fails. */ // eslint-disable-next-line no-restricted-syntax private constructor({ @@ -96,12 +105,14 @@ export class VatHandle implements EndpointHandle { kernelQueue, logger, allowedGlobalNames, + onStreamFailure, }: VatConstructorProps) { this.vatId = vatId; this.config = vatConfig; this.#logger = logger; this.#allowedGlobalNames = allowedGlobalNames; this.#vatStream = vatStream; + this.#onStreamFailure = onStreamFailure; this.#vatStore = kernelStore.makeVatStore(vatId); this.#vatSyscall = new VatSyscall({ vatId, @@ -154,12 +165,27 @@ export class VatHandle implements EndpointHandle { */ async #init(): Promise { Promise.all([this.#vatStream.drain(this.#handleMessage.bind(this))]).catch( - async (error) => { + (error) => { this.#logger?.error(`Unexpected read error`, error); - await this.terminate( - true, - new StreamReadError({ vatId: this.vatId }, error), - ); + const streamError = new StreamReadError({ vatId: this.vatId }, error); + try { + // Not left to the retirement's own `terminate`, which reaches it only + // after the worker has been killed: a crank blocked on a delivery + // this channel will never carry waits behind that. + this.#rpcClient.rejectAll(streamError); + } finally { + try { + this.#onStreamFailure(streamError); + } catch (reportError) { + // Nothing awaits this handler, so a throw would be an unhandled + // rejection and the only account of the vat's death would be the + // read error logged above. + this.#logger?.error( + `Failed to report the dead channel of vat ${this.vatId}`, + reportError, + ); + } + } }, ); diff --git a/packages/ocap-kernel/src/vats/VatManager.test.ts b/packages/ocap-kernel/src/vats/VatManager.test.ts index adae6d3976..eff03e8878 100644 --- a/packages/ocap-kernel/src/vats/VatManager.test.ts +++ b/packages/ocap-kernel/src/vats/VatManager.test.ts @@ -6,6 +6,7 @@ import { import type { JsonRpcMessage } from '@metamask/kernel-utils'; import { Logger } from '@metamask/logger'; import type { DuplexStream } from '@metamask/streams'; +import { delay } from '@ocap/repo-tools/test-utils'; import type { Mocked, MockInstance } from 'vitest'; import { describe, it, expect, vi, beforeEach } from 'vitest'; @@ -197,6 +198,21 @@ describe('VatManager', () => { expect(mockPlatformServices.terminate).toHaveBeenCalledWith('v1'); }); + it('stops no worker when there was never one to stop', async () => { + mockPlatformServices.launch.mockRejectedValueOnce( + new Error('worker file not found'), + ); + + await expect( + vatManager.launchVat(createMockVatConfig(), 'bob', 's1'), + ).rejects.toThrow('Failed to launch vat v1 (bob)'); + + // Both runtimes throw for a vat they have no worker for, so asking would + // report a cleanup failure over the launch failure an operator is reading + // the log to find. + expect(mockPlatformServices.terminate).not.toHaveBeenCalled(); + }); + it('attributes a launch failure to the vat by id and config name', async () => { const config = createMockVatConfig(); const cause = new Error( @@ -247,14 +263,12 @@ describe('VatManager', () => { expect(vatManager.hasVat('v1')).toBe(false); }); - it('still marks the vat terminated when the cleanup itself fails', async () => { + it('reports the launch failure when the cleanup itself fails', async () => { const config = createMockVatConfig(); const cause = new Error('setVatConfig threw'); mockKernelStore.setVatConfig.mockImplementationOnce(() => { throw cause; }); - // `stopVat` stops short of the mark when a write before it throws, which - // is what makes the assertion below about this catch rather than it. mockKernelStore.deleteVat.mockImplementationOnce(() => { throw new Error('deleteVat failed'); }); @@ -268,7 +282,26 @@ describe('VatManager', () => { ); // The launch failure, not the cleanup failure, is what the caller needs. expect((error as Error).cause).toBe(cause); - expect(mockKernelStore.markVatAsTerminated).toHaveBeenCalledWith('v1'); + }); + + it('leaves the vat unmarked when the cleanup stops short of the mark', async () => { + const config = createMockVatConfig(); + mockKernelStore.setVatConfig.mockImplementationOnce(() => { + throw new Error('setVatConfig threw'); + }); + mockKernelStore.unpinObject.mockImplementationOnce(() => { + throw new Error('unpin failed'); + }); + + await expect(vatManager.launchVat(config, 'bob', 's1')).rejects.toThrow( + 'Failed to launch vat v1 (bob) (cleanup also failed)', + ); + + // Marking here would schedule the cleanup for a vat whose `deleteVat` + // never ran, and the mark is dropped once that cleanup has swept: the + // vat store and the subcluster row would be left with no way to reach + // them again. + expect(mockKernelStore.markVatAsTerminated).not.toHaveBeenCalled(); }); }); @@ -290,6 +323,113 @@ describe('VatManager', () => { VatAlreadyExistsError, ); }); + + describe('a vat whose channel fails', () => { + const reportStreamFailure = (error: Error, launch = 0): void => { + const { onStreamFailure } = makeVatHandleMock.mock.calls[ + launch + ]?.[0] as { + onStreamFailure: (error: Error) => void; + }; + onStreamFailure(error); + }; + + it('is retired', async () => { + await vatManager.runVat('v1', createMockVatConfig()); + mockKernelStore.getPromisesByDecider.mockReturnValueOnce(['kp1']); + const cause = new Error('channel failed'); + + reportStreamFailure(cause); + await delay(10); + + expect(mockKernelQueue.resolvePromises).toHaveBeenCalledWith('v1', [ + [ + 'kp1', + true, + expect.objectContaining({ + body: expect.stringContaining('channel failed'), + }), + ], + ]); + expect(mockKernelStore.deleteVat).toHaveBeenCalledWith('v1'); + expect(mockKernelStore.markVatAsTerminated).toHaveBeenCalledWith('v1'); + expect(mockPlatformServices.terminate).toHaveBeenCalledWith( + 'v1', + cause, + ); + expect(vatHandles[0]?.terminate).toHaveBeenCalledWith(true, cause); + expect(vatManager.hasVat('v1')).toBe(false); + }); + + it('waits for the open crank before writing the death down', async () => { + await vatManager.runVat('v1', createMockVatConfig()); + let releaseCrank = (): void => undefined; + mockKernelQueue.waitForCrank.mockReturnValueOnce( + new Promise((resolve) => { + releaseCrank = resolve; + }), + ); + + reportStreamFailure(new Error('channel failed')); + await delay(10); + // Written inside an open delivery savepoint, the whole death is undone + // by a crank that goes on to abort, while the dropped handle is not. + const writesDuringTheCrank = + mockKernelStore.deleteVat.mock.calls.length; + releaseCrank(); + await delay(10); + + expect(writesDuringTheCrank).toBe(0); + expect(mockKernelStore.deleteVat).toHaveBeenCalledWith('v1'); + }); + + it('is left alone when the kernel has already stopped it', async () => { + await vatManager.runVat('v1', createMockVatConfig()); + await vatManager.stopVat('v1', true); + const storeWrites = (): object => ({ + deleteVat: mockKernelStore.deleteVat.mock.calls.length, + markVatAsTerminated: + mockKernelStore.markVatAsTerminated.mock.calls.length, + unpinObject: mockKernelStore.unpinObject.mock.calls.length, + }); + const before = storeWrites(); + + reportStreamFailure(new Error('channel failed')); + await delay(10); + + expect(storeWrites()).toStrictEqual(before); + }); + + it('is left alone when a restart has replaced its handle', async () => { + await vatManager.runVat('v1', createMockVatConfig()); + await vatManager.restartVat('v1'); + const deletesBefore = mockKernelStore.deleteVat.mock.calls.length; + + // The handle the first launch made, reporting after the handle that + // replaced it is the one the manager keeps. + reportStreamFailure(new Error('channel failed')); + await delay(10); + + expect(mockKernelStore.deleteVat).toHaveBeenCalledTimes(deletesBefore); + expect(vatManager.hasVat('v1')).toBe(true); + }); + + it('logs a retirement that fails', async () => { + await vatManager.runVat('v1', createMockVatConfig()); + const logError = vi.spyOn(mockLogger, 'error').mockReturnValue(); + mockKernelStore.deleteVat.mockImplementationOnce(() => { + throw new Error('deleteVat failed'); + }); + + reportStreamFailure(new Error('channel failed')); + await delay(10); + + expect(logError).toHaveBeenCalledWith( + expect.stringContaining('terminate it to try again'), + expect.objectContaining({ message: 'deleteVat failed' }), + ); + }); + }); }); describe('stopVat', () => { @@ -348,8 +488,8 @@ describe('VatManager', () => { expect(order).toStrictEqual([ 'resolvePromises', 'unpinObject', - 'deleteVat', 'markVatAsTerminated', + 'deleteVat', 'terminateWorker', ]); }); @@ -380,12 +520,12 @@ describe('VatManager', () => { it('leaves the vat unmarked when an earlier write throws', async () => { await vatManager.runVat('v1', createMockVatConfig()); - mockKernelStore.deleteVat.mockImplementationOnce(() => { - throw new Error('deleteVat failed'); + mockKernelStore.unpinObject.mockImplementationOnce(() => { + throw new Error('unpin failed'); }); await expect(vatManager.stopVat('v1', true)).rejects.toThrow( - 'deleteVat failed', + 'unpin failed', ); // Marking it would make the deferred cleanup delete the decider @@ -396,30 +536,49 @@ describe('VatManager', () => { expect(vatManager.hasVat('v1')).toBe(false); }); - it('reports the store failure rather than the channel failure', async () => { + it('marks the vat before the write that makes it unfindable', async () => { await vatManager.runVat('v1', createMockVatConfig()); mockKernelStore.deleteVat.mockImplementationOnce(() => { throw new Error('deleteVat failed'); }); - vatHandles[0]?.terminate.mockRejectedValueOnce( - new Error('stream will not end'), - ); await expect(vatManager.stopVat('v1', true)).rejects.toThrow( 'deleteVat failed', ); + + expect(mockKernelStore.markVatAsTerminated).toHaveBeenCalledWith('v1'); }); - it('records nothing a second time', async () => { + it('finishes a retirement its first attempt stopped part-way', async () => { await vatManager.runVat('v1', createMockVatConfig()); + mockKernelStore.getPromisesByDecider.mockReturnValue(['kp1']); + mockKernelStore.deleteVat.mockImplementationOnce(() => { + throw new Error('deleteVat failed'); + }); + await expect(vatManager.stopVat('v1', true)).rejects.toThrow( + 'deleteVat failed', + ); mockKernelStore.isVatTerminated.mockReturnValue(true); await vatManager.stopVat('v1', true); - // `deleteVat` fails on a vat whose subcluster mapping the first call - // removed, and a second unpin would spend a pin this vat no longer holds. - expect(mockKernelStore.deleteVat).not.toHaveBeenCalled(); - expect(mockKernelStore.unpinObject).not.toHaveBeenCalled(); + expect(mockKernelStore.deleteVat).toHaveBeenCalledTimes(2); + expect(mockKernelStore.unpinObject).toHaveBeenCalledTimes(1); + expect(mockKernelQueue.resolvePromises).toHaveBeenCalledTimes(1); + }); + + it('reports the store failure rather than the channel failure', async () => { + await vatManager.runVat('v1', createMockVatConfig()); + mockKernelStore.deleteVat.mockImplementationOnce(() => { + throw new Error('deleteVat failed'); + }); + vatHandles[0]?.terminate.mockRejectedValueOnce( + new Error('stream will not end'), + ); + + await expect(vatManager.stopVat('v1', true)).rejects.toThrow( + 'deleteVat failed', + ); }); it('keeps the records across a restart', async () => { diff --git a/packages/ocap-kernel/src/vats/VatManager.ts b/packages/ocap-kernel/src/vats/VatManager.ts index 0052d5107d..42f115f0c4 100644 --- a/packages/ocap-kernel/src/vats/VatManager.ts +++ b/packages/ocap-kernel/src/vats/VatManager.ts @@ -22,6 +22,20 @@ import type { AllowedGlobalName } from './endowments.ts'; import { VatHandle } from './VatHandle.ts'; import type { PingVatResult } from '../rpc/index.ts'; +/** + * A vat being stopped, and the error its death is to be reported as. An ending + * vat always has one, since `#retireVat` rejects the promises it was deciding + * with it; a restarting vat must not, since `terminate` reads its absence as + * the difference. + */ +type StopVatOptions = { vatId: VatId } & ( + | { terminating: true; terminationError: Error } + | { terminating: false; terminationError?: never } +); + +/** The handle `runVat` made, once `VatHandle.make` has returned one. */ +type RunningVat = { handle?: VatHandle }; + type VatManagerOptions = { platformServices: PlatformServices; kernelStore: KernelStore; @@ -119,19 +133,8 @@ export class VatManager { try { await this.runVat(vatId, vatConfig); } catch (error) { - // `platformServices.launch` has already spawned the worker by the time - // anything below it can fail, and `runVat` records no handle to reach it - // by, so this is the only chance to stop it. `stopVat` is no use here: - // there is neither a handle nor a `vatConfig` row for it to work from. - await this.#platformServices - .terminate(vatId) - .catch((stopError: unknown) => - this.#logger.error( - `Failed to stop the worker for vat ${vatId} after an incomplete launch:`, - stopError, - ), - ); - // Attribute the failure to the specific vat by kernel id and name. + // Attribute the failure to the specific vat by kernel id and name. Any + // worker `runVat` started has already been stopped. throw new Error(`Failed to launch vat ${vatId} (${vatName})`, { cause: error, }); @@ -151,7 +154,10 @@ export class VatManager { return rootRef; } catch (error) { // The worker is already running, so leaving it would strand a vat the - // kernel has no record of. + // kernel has no record of. `stopVat` sets the terminated mark too, and + // early enough that asserting it here could only add it where `stopVat` + // stopped short of `deleteVat` — scheduling a cleanup for a vat whose + // own store and subcluster row it will not reach. let stopFailure: unknown; try { await this.stopVat(vatId, true); @@ -162,12 +168,6 @@ export class VatManager { caught, ); } - // `stopVat` marks the vat itself, but only if every write before the - // mark succeeded. The mark is what makes the terminated-vat cleanup - // reclaim the endpoint counters and the root's c-list pair, and a vat - // this young has no decider promises for that cleanup to get wrong, so - // here it is asserted rather than assumed. - this.#kernelStore.markVatAsTerminated(vatId); throw new Error( `Failed to launch vat ${vatId} (${vatName})${stopFailure ? ' (cleanup also failed)' : ''}`, { cause: error }, @@ -178,6 +178,11 @@ export class VatManager { /** * Start a new or resurrected vat running. * + * `launch` spawns the worker before the handshake that can fail, and a + * handshake that fails records no handle to reach it by, so a worker left + * over from one is stopped here — the one place that knows whether `launch` + * got far enough to leave one. + * * @param vatId - The ID of the vat to start. * @param vatConfig - Its configuration. */ @@ -192,18 +197,116 @@ export class VatManager { loggerStream as unknown as Parameters[0], (error) => this.#logger.error(`Vat ${vatId} error: ${stringify(error)}`), ); - const vat = await VatHandle.make({ - vatId, - vatConfig, - vatStream, - kernelStore: this.#kernelStore, - kernelQueue: this.#kernelQueue, - logger: vatLogger, - allowedGlobalNames: this.#allowedGlobalNames, - }); + // Holds the handle so that a report can be matched against the one that + // made it: a restart puts a new handle under the same vat id. Empty while + // `make` runs, in which window the catch below answers for the channel. + const running: RunningVat = {}; + let vat: VatHandle; + try { + vat = await VatHandle.make({ + vatId, + vatConfig, + vatStream, + kernelStore: this.#kernelStore, + kernelQueue: this.#kernelQueue, + logger: vatLogger, + allowedGlobalNames: this.#allowedGlobalNames, + onStreamFailure: (error) => + this.#reportLostVat({ vatId, running, error }), + }); + } catch (error) { + await this.#platformServices + .terminate(vatId) + .catch((stopError: unknown) => + this.#logger.error( + `Failed to stop the worker for vat ${vatId} after a failed handshake:`, + stopError, + ), + ); + throw error; + } + running.handle = vat; this.#vats.set(vatId, vat); } + /** + * Take a handle's word that its channel has failed. + * + * @param options - Named options. + * @param options.vatId - The vat whose channel failed. + * @param options.running - The handle that reported, once there is one. + * @param options.error - What the channel failed with. + */ + #reportLostVat({ + vatId, + running, + error, + }: { + vatId: VatId; + running: RunningVat; + error: Error; + }): void { + const { handle } = running; + if (!handle) { + return; + } + this.#retireLostVat({ vatId, handle, error }).catch((failure) => + this.#logger.error( + `Vat ${vatId} lost its worker and could not be retired, so the kernel still holds it as running; terminate it to try again:`, + failure, + ), + ); + } + + /** + * Retire a vat whose channel to its worker has failed. + * + * Nobody asked for this death, so nobody is waiting to be told of it: the + * store learns of it here or not at all. Untold, the vat keeps its handle, + * its `vatConfig` row and the promises it was deciding, so the kernel goes on + * routing to a worker it cannot reach and the next boot brings it back. + * + * Between cranks, like `terminateVat`, and for a sharper reason. `#retireVat` + * writes synchronously, so run inside an open delivery savepoint the whole + * death is undone by any crank that goes on to abort — the ordinary vat-error + * path, not a failure — while the handle this dropped from the running map + * stays dropped, since nothing rolls memory back. That leaves a vat the store + * calls alive and the kernel cannot reach, and no error anywhere. Waiting is + * safe because the handle has already answered the callers this worker never + * will, so a crank blocked on one of them is free to finish. + * + * A vat the kernel is itself stopping reports the same way, since closing a + * channel with an error breaks the read its handle is draining. The identity + * check covers that along with everything the wait may have let happen: by + * then the vat may have been terminated, or restarted under a new handle. + * + * There is no one to retry this: a store failure here ends in a log line, + * leaving the vat unmarked, undeleted and handle-less until someone + * terminates it by hand. Said plainly in that log, for want of a better + * answer than the caller this path does not have. + * + * @param options - Named options. + * @param options.vatId - The vat whose channel failed. + * @param options.handle - The handle that reported it. + * @param options.error - What the channel failed with, for the rejections its + * subscribers are owed. + */ + async #retireLostVat({ + vatId, + handle, + error, + }: { + vatId: VatId; + handle: VatHandle; + error: Error; + }): Promise { + await this.#kernelQueue.waitForCrank(); + if (this.#vats.get(vatId) !== handle) { + return; + } + await this.#stopVat({ vatId, terminating: true, terminationError: error }); + } + /** * Stop a vat from running. * @@ -222,6 +325,36 @@ export class VatManager { terminating: boolean, reason?: CapData, ): Promise { + await this.#stopVat( + terminating + ? { + vatId, + terminating: true, + terminationError: reason + ? new Error(`Vat termination: ${reason.body}`) + : new VatDeletedError(vatId), + } + : { vatId, terminating: false }, + ); + } + + /** + * Stop a vat, given the error its death is to be reported as. + * + * Split from `stopVat` for the caller that has an error rather than a + * serialized reason to give: a channel that died on its own. + * + * @param options - The vat to stop, and how. + * @param options.vatId - The ID of the vat. + * @param options.terminating - If true, the vat is being killed, if false, + * it's being restarted. + * @param options.terminationError - Why it is ending, if it is. + */ + async #stopVat({ + vatId, + terminating, + terminationError, + }: StopVatOptions): Promise { // A restart needs a live handle to read its config from and to come back // into; an ending vat does not, and must not. A failed relaunch leaves a // vat the store still lists and the kernel has no handle for, and retiring @@ -230,16 +363,10 @@ export class VatManager { if (terminating && !vat && !this.#kernelStore.isVatActive(vatId)) { throw new VatNotFoundError(vatId); } - let terminationError: Error | undefined; - if (reason) { - terminationError = new Error(`Vat termination: ${reason.body}`); - } else if (terminating) { - terminationError = new VatDeletedError(vatId); - } let recordFailure: Error | undefined; try { if (terminating) { - this.#retireVat(vatId, terminationError as Error); + this.#retireVat(vatId, terminationError); } else { // A restart keeps the pin and the records: the same vat, and the same // root, are coming back. Only the handle goes. @@ -282,16 +409,27 @@ export class VatManager { * store that says the vat is dead is worth more than one still waiting to * find out. * - * The mark goes last, and is not attempted if anything above it throws. It is - * what makes the vat eligible for `nextTerminatedVatCleanup`, and that - * cleanup deletes the decider promises' c-list entries on the stated - * understanding that its caller has already rejected them. A vat marked after - * those rejections failed is one whose subscribers hang for good, so a - * failure part-way leaves a vat that has simply not been retired — its c-list - * intact, and the whole step available to be tried again. + * The order is what a partial failure leaves behind. The mark comes after the + * rejections because it is what makes the vat eligible for + * `nextTerminatedVatCleanup`, and that cleanup deletes the decider promises' + * c-list entries on the stated understanding that its caller has already + * rejected them: a vat marked after those rejections failed is one whose + * subscribers hang for good. It comes before `deleteVat` because `deleteVat` + * drops the `vatConfig` row `stopVat` reads to decide there is still a vat + * here to retire — a mark not reached by then could never be set, and the + * c-list it gates never swept. So every way this can fail leaves either the + * mark set, or the row that lets the whole step be tried again. * - * Calling it twice records nothing the second time: a second - * `releaseVatRootPin` would spend a pin this vat no longer holds. + * What that ordering costs: a `deleteVat` that throws leaves a marked vat + * whose `vatConfig` row survives, and `cleanupTerminatedVat` sweeps + * `${vatId}.` keys, which never match `vatConfig.${vatId}` — so once the + * cleanup drops the mark such a vat reads as active again. Retrying is what + * closes that, and the failure propagates so that someone can. + * + * Which is why only the writes that may happen once are guarded: a second + * `releaseVatRootPin` would spend an embedder's `pinVatRoot` pin instead of + * the launch pin this vat no longer holds. `deleteVat` is idempotent, and + * runs again to carry an interrupted retirement to the end. * * @param vatId - The vat being retired. * @param error - Why, for the rejections its subscribers are owed. @@ -301,19 +439,15 @@ export class VatManager { // the store already calls dead must not keep a handle the router would go // on resolving. this.#vats.delete(vatId); - if (this.#kernelStore.isVatTerminated(vatId)) { - return; - } - const failure = makeKernelError('VAT_TERMINATED', error.message); - for (const kpid of this.#kernelStore.getPromisesByDecider(vatId)) { - this.#kernelQueue.resolvePromises(vatId, [[kpid, true, failure]]); + if (!this.#kernelStore.isVatTerminated(vatId)) { + const failure = makeKernelError('VAT_TERMINATED', error.message); + for (const kpid of this.#kernelStore.getPromisesByDecider(vatId)) { + this.#kernelQueue.resolvePromises(vatId, [[kpid, true, failure]]); + } + this.releaseVatRootPin(vatId); + this.#kernelStore.markVatAsTerminated(vatId); } - this.releaseVatRootPin(vatId); - // Together, always. `cleanupTerminatedVat` sweeps `${vatId}.` keys, which - // never match `vatConfig.${vatId}`, so a vat left with that row reads as - // active again the moment the cleanup drops the mark. this.#kernelStore.deleteVat(vatId); - this.#kernelStore.markVatAsTerminated(vatId); } /** From 2125267d55351356061260c82065d2deb6a113fd Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Thu, 1 Oct 2026 10:29:01 +0200 Subject: [PATCH 4/5] docs(ocap-kernel): cut the retirement comments down to what the code cannot say Co-Authored-By: Claude Opus 5.5 (1M context) --- .../kernel-test/src/vat-lifecycle.test.ts | 7 +- packages/ocap-kernel/CHANGELOG.md | 11 +- packages/ocap-kernel/src/store/index.test.ts | 3 - .../src/store/methods/subclusters.test.ts | 3 - .../src/store/methods/subclusters.ts | 5 +- packages/ocap-kernel/src/store/methods/vat.ts | 7 +- .../ocap-kernel/src/vats/VatHandle.test.ts | 1 - packages/ocap-kernel/src/vats/VatHandle.ts | 28 +--- .../ocap-kernel/src/vats/VatManager.test.ts | 33 +---- packages/ocap-kernel/src/vats/VatManager.ts | 126 ++++-------------- 10 files changed, 49 insertions(+), 175 deletions(-) diff --git a/packages/kernel-test/src/vat-lifecycle.test.ts b/packages/kernel-test/src/vat-lifecycle.test.ts index e943ea76d1..fd45f4ea9e 100644 --- a/packages/kernel-test/src/vat-lifecycle.test.ts +++ b/packages/kernel-test/src/vat-lifecycle.test.ts @@ -172,9 +172,6 @@ describe('Vat Lifecycle', { timeout: 30_000 }, () => { await kernel.terminateVat(vatId); await waitUntilQuiescent(); - // The mark is what schedules this, and it is dropped once the sweep is - // done — so the `vatConfig` row it never touches is the thing that decides - // whether the vat is active after it. kernel.collectGarbage(); await waitUntilQuiescent(); @@ -210,9 +207,7 @@ describe('Vat Lifecycle', { timeout: 30_000 }, () => { await waitUntilQuiescent(); const vatId = kernel.getVats()[0]?.id as VatId; - // Nothing listens for a worker that dies outright, so what the kernel can - // actually see is a worker still sending: this is that worker's own - // message event, carrying something the reader will not take. + // The kernel cannot see a killed worker, only a bad frame from a live one. platformServices.workers.get(vatId)?.worker.emit('message', NaN); await waitUntilQuiescent(); kernel.collectGarbage(); diff --git a/packages/ocap-kernel/CHANGELOG.md b/packages/ocap-kernel/CHANGELOG.md index dda9e60679..c1f2f15c3a 100644 --- a/packages/ocap-kernel/CHANGELOG.md +++ b/packages/ocap-kernel/CHANGELOG.md @@ -71,12 +71,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - A service or IO channel named `__proto__` in a cluster config reaches the bootstrap vat as an ordinary key of `services` instead of replacing that object's prototype ([#1121](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1121)) - A delivery to a remote peer is written down inside the crank making it and sent once that crank commits, instead of going out mid-crank. A crank that then failed took the kernel's record of the message back while the peer already had it, leaving the kernel's next send sequence behind what the peer had seen. The send sequence counters move with that record rather than in memory beside it, so a crank that rolls back takes them too, and outgoing messages reach the peer in sequence order however long a crank takes to commit — and across a restart, which sends the queue it inherits before anything numbered after it. Giving up on a peer covers a delivery whose crank has yet to commit, so it is not sent to a peer whose promise for it has just been rejected. A peer's messages go to the transport one at a time, since a send that has to dial and shake hands first can otherwise be overtaken by one issued after it, and one whose turn comes after a peer restart or a give-up has retired the queue is dropped rather than sent to a peer it is no longer owed to ([#1101](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1101)) - With the reference count audit crediting the crank buffer, a delivery that queued a message for later no longer reads as a leak, and the audit runs before the flush hands those messages on — so a violation stops the kernel while the crank can still be undone, rather than after the answers have gone out ([#1095](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1095)) -- A vat's death is recorded in one synchronous step, so a failure part-way through terminating it can no longer leave the store half-told. Marked terminated while its `vatConfig` row survived, a vat read as active again the moment the deferred cleanup dropped the mark ([#1093](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1093)) -- `terminateVat` retires a vat that is persisted but not running instead of throwing `VatNotFoundError`. A failed relaunch leaves one in that state, and it could then only be removed by discarding the whole store; `terminateSubcluster`, which walks persisted membership, gave up part-way through on reaching one ([#1093](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1093)) -- A vat whose worker launched but whose kernel-side setup then failed has that worker stopped, rather than left running with no record — whether the failure came from the handshake with the worker or from registering the vat afterwards ([#1093](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1093)) -- A vat whose channel to its worker fails is retired in full, like one the kernel was asked to terminate. It used to keep its handle, so the kernel went on delivering over a channel that was gone, along with its root's pin and its whole c-list ([#1093](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1093)) -- `cleanupTerminatedVat` discards a terminated vat's `vatConfig` row along with the rest of it. A vat whose retirement was marked but did not finish came back to life as the cleanup dropped the mark, and the next boot relaunched it with everything else about it already swept away ([#1093](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1093)) -- A vat whose worker survives a failed handshake has that worker stopped by `runVat`, so `initializeAllVats` and `restartVat` no longer strand one where only `launchVat` cleaned up ([#1093](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1093)) +- A vat's death is recorded in one synchronous step, so a crank can no longer observe a half-terminated vat ([#1093](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1093)) +- `terminateVat` retires a vat that is persisted but not running, such as one whose relaunch failed, instead of throwing `VatNotFoundError` ([#1093](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1093)) +- A worker whose vat fails to finish launching is stopped rather than left running with no record ([#1093](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1093)) +- A vat whose channel to its worker fails is retired, rather than kept as running over a dead channel ([#1093](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1093)) +- `cleanupTerminatedVat` discards the vat's `vatConfig` row, so a partly retired vat is not relaunched on the next boot ([#1093](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1093)) - A kernel object whose owner retires or abandons it is now orphaned, so its owner and reference-count records no longer outlive the c-list entry they were reachable through; before, the next collection to visit such a kref read an entry that was gone and killed the run loop ([#1091](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1091)) - A crank that aborts keeps the writes that follow the abort — the vat's death, the collection after it — instead of autocommitting them one statement at a time outside any transaction: the run loop takes a savepoint for the crank and one for the delivery, and only the delivery is rolled back ([#1090](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1090)) - A vat that exits gracefully stays dead when the rest of its crank then throws; before, the run loop's catch rolled the death record back and would have relaunched a vat whose worker was gone and whose callers had been answered ([#1090](https://github.com/Consensys-Incorporated/ocap-kernel/pull/1090)) diff --git a/packages/ocap-kernel/src/store/index.test.ts b/packages/ocap-kernel/src/store/index.test.ts index 3b48929e51..67371910a3 100644 --- a/packages/ocap-kernel/src/store/index.test.ts +++ b/packages/ocap-kernel/src/store/index.test.ts @@ -624,9 +624,6 @@ describe('kernel store', () => { ks.cleanupTerminatedVat('v1'); - // The sweep drops the mark on its way out, and `isVatActive` reads only - // this row — left behind, the vat reads as running again and the next - // boot relaunches it with everything else about it swept away. expect(ks.isVatActive('v1')).toBe(false); expect(ks.isVatTerminated('v1')).toBe(false); }); diff --git a/packages/ocap-kernel/src/store/methods/subclusters.test.ts b/packages/ocap-kernel/src/store/methods/subclusters.test.ts index 47ef24e0a6..4e2a75ad21 100644 --- a/packages/ocap-kernel/src/store/methods/subclusters.test.ts +++ b/packages/ocap-kernel/src/store/methods/subclusters.test.ts @@ -498,9 +498,6 @@ describe('getSubclusterMethods', () => { 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), ).not.toThrow(); diff --git a/packages/ocap-kernel/src/store/methods/subclusters.ts b/packages/ocap-kernel/src/store/methods/subclusters.ts index d5f4472cde..7c8edb12f2 100644 --- a/packages/ocap-kernel/src/store/methods/subclusters.ts +++ b/packages/ocap-kernel/src/store/methods/subclusters.ts @@ -239,9 +239,8 @@ 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. + * A no-op for a vat in no subcluster, since `deleteVat` calls this for every + * vat it discards. * * @param vatId - The ID of the vat to remove. */ diff --git a/packages/ocap-kernel/src/store/methods/vat.ts b/packages/ocap-kernel/src/store/methods/vat.ts index 9a1dc10318..43900545f2 100644 --- a/packages/ocap-kernel/src/store/methods/vat.ts +++ b/packages/ocap-kernel/src/store/methods/vat.ts @@ -312,10 +312,9 @@ export function getVatMethods(ctx: StoreContext) { // Clean up any remaining c-list entries and vat-specific counters deleteEndpoint(vatID); - // The sweep above matches `${vatID}.` keys, which `vatConfig.${vatID}` is - // not, and `isVatActive` reads nothing else. A vat retired by a caller that - // marked it and then failed to discard it would come back to life here as - // the mark is dropped, and be relaunched — gutted — on the next boot. + // `vatConfig.${vatID}` is outside the `${vatID}.` sweep. Left behind, the + // vat reads as active once the mark is dropped, and the next boot + // relaunches it. deleteVatConfig(vatID); // Remove the vat from the terminated vats list diff --git a/packages/ocap-kernel/src/vats/VatHandle.test.ts b/packages/ocap-kernel/src/vats/VatHandle.test.ts index 6e0b3f8285..7c87f058d5 100644 --- a/packages/ocap-kernel/src/vats/VatHandle.test.ts +++ b/packages/ocap-kernel/src/vats/VatHandle.test.ts @@ -375,7 +375,6 @@ describe('VatHandle', () => { await vat.terminate(false); await delay(10); - // The same vat is coming back, and its answer with it. expect(settled).not.toHaveBeenCalled(); }); }); diff --git a/packages/ocap-kernel/src/vats/VatHandle.ts b/packages/ocap-kernel/src/vats/VatHandle.ts index 8cf946361a..b9a6a16d36 100644 --- a/packages/ocap-kernel/src/vats/VatHandle.ts +++ b/packages/ocap-kernel/src/vats/VatHandle.ts @@ -44,10 +44,7 @@ type VatConstructorProps = { kernelQueue: KernelQueue; logger?: Logger | undefined; allowedGlobalNames?: AllowedGlobalName[] | undefined; - /** - * Called when the channel to the worker fails, so that whoever keeps this - * vat's records can retire it. - */ + /** Called when the channel to the worker fails. */ onStreamFailure: (error: Error) => void; }; @@ -169,17 +166,13 @@ export class VatHandle implements EndpointHandle { this.#logger?.error(`Unexpected read error`, error); const streamError = new StreamReadError({ vatId: this.vatId }, error); try { - // Not left to the retirement's own `terminate`, which reaches it only - // after the worker has been killed: a crank blocked on a delivery - // this channel will never carry waits behind that. + // Now, not in the retirement's `terminate`: that waits for the + // current crank, which may be blocked on a pending command. this.#rpcClient.rejectAll(streamError); } finally { try { this.#onStreamFailure(streamError); } catch (reportError) { - // Nothing awaits this handler, so a throw would be an unhandled - // rejection and the only account of the vat's death would be the - // read error logged above. this.#logger?.error( `Failed to report the dead channel of vat ${this.vatId}`, reportError, @@ -321,22 +314,15 @@ export class VatHandle implements EndpointHandle { } /** - * Closes this handle's channel to the vat worker. - * - * Only the handle's own business: the store side of a vat's death belongs to - * `VatManager`, which writes it in one synchronous step. Split that way - * because the two have opposite failure requirements — ending a stream can - * fail and it does not matter, since the worker is already being killed, - * while a store left half-told about a vat is a state nothing recovers from. + * Closes this handle's channel to the vat worker. The store side of a vat's + * death is `VatManager`'s. * - * @param terminating - If true, the vat is being killed permanently, so - * callers waiting on a command it will never answer are told now. + * @param terminating - If true, the vat is being killed permanently. * @param error - The error to terminate the vat with. */ async terminate(terminating: boolean, error?: Error): Promise { if (terminating) { - // Ahead of the stream, so a stream that refuses to close does not leave - // these callers waiting on a worker that is already dead. + // Before `end`, which may never settle. this.#rpcClient.rejectAll(error ?? new VatDeletedError(this.vatId)); } await this.#vatStream.end(error); diff --git a/packages/ocap-kernel/src/vats/VatManager.test.ts b/packages/ocap-kernel/src/vats/VatManager.test.ts index eff03e8878..0c140d81d2 100644 --- a/packages/ocap-kernel/src/vats/VatManager.test.ts +++ b/packages/ocap-kernel/src/vats/VatManager.test.ts @@ -193,8 +193,6 @@ describe('VatManager', () => { vatManager.launchVat(createMockVatConfig(), 'bob', 's1'), ).rejects.toThrow('Failed to launch vat v1 (bob)'); - // The worker is spawned before anything that can fail here, and no handle - // was recorded, so this is the only chance to stop it. expect(mockPlatformServices.terminate).toHaveBeenCalledWith('v1'); }); @@ -207,9 +205,7 @@ describe('VatManager', () => { vatManager.launchVat(createMockVatConfig(), 'bob', 's1'), ).rejects.toThrow('Failed to launch vat v1 (bob)'); - // Both runtimes throw for a vat they have no worker for, so asking would - // report a cleanup failure over the launch failure an operator is reading - // the log to find. + // Both runtimes throw for an unknown worker, burying the launch failure. expect(mockPlatformServices.terminate).not.toHaveBeenCalled(); }); @@ -280,7 +276,6 @@ describe('VatManager', () => { expect((error as Error).message).toBe( 'Failed to launch vat v1 (bob) (cleanup also failed)', ); - // The launch failure, not the cleanup failure, is what the caller needs. expect((error as Error).cause).toBe(cause); }); @@ -297,10 +292,6 @@ describe('VatManager', () => { 'Failed to launch vat v1 (bob) (cleanup also failed)', ); - // Marking here would schedule the cleanup for a vat whose `deleteVat` - // never ran, and the mark is dropped once that cleanup has swept: the - // vat store and the subcluster row would be left with no way to reach - // them again. expect(mockKernelStore.markVatAsTerminated).not.toHaveBeenCalled(); }); }); @@ -372,8 +363,6 @@ describe('VatManager', () => { reportStreamFailure(new Error('channel failed')); await delay(10); - // Written inside an open delivery savepoint, the whole death is undone - // by a crank that goes on to abort, while the dropped handle is not. const writesDuringTheCrank = mockKernelStore.deleteVat.mock.calls.length; releaseCrank(); @@ -405,8 +394,7 @@ describe('VatManager', () => { await vatManager.restartVat('v1'); const deletesBefore = mockKernelStore.deleteVat.mock.calls.length; - // The handle the first launch made, reporting after the handle that - // replaced it is the one the manager keeps. + // The first launch's handle, reporting after the restart replaced it. reportStreamFailure(new Error('channel failed')); await delay(10); @@ -499,8 +487,7 @@ describe('VatManager', () => { mockKernelStore.getPromisesByDecider.mockReturnValueOnce(['kp1']); const stopping = vatManager.stopVat('v1', true); - // Read before awaiting: everything the store has to be told is written in - // `stopVat`'s synchronous prefix, so a crank cannot land part-way through. + // Read before awaiting, to pin that no write follows a yield point. const writesBeforeTheFirstAwait = { resolvePromises: mockKernelQueue.resolvePromises.mock.calls.length, unpinObject: mockKernelStore.unpinObject.mock.calls.length, @@ -528,9 +515,6 @@ describe('VatManager', () => { 'unpin failed', ); - // Marking it would make the deferred cleanup delete the decider - // promises' c-list entries believing they were rejected. Unmarked, the - // vat is simply not retired yet and the step can be tried again. expect(mockKernelStore.markVatAsTerminated).not.toHaveBeenCalled(); expect(mockPlatformServices.terminate).toHaveBeenCalled(); expect(vatManager.hasVat('v1')).toBe(false); @@ -703,9 +687,7 @@ describe('VatManager', () => { describe('a vat that is persisted but not running', () => { /** - * Leave the vat in the state a failed relaunch does: gone from the - * running map, with its record, its own store and its root pin all still - * in place. + * Leave the vat persisted but not running, as a failed relaunch does. */ async function givenAFailedRestart(): Promise { await vatManager.runVat('v1', createMockVatConfig()); @@ -729,9 +711,6 @@ describe('VatManager', () => { await vatManager.terminateVat('v1'); - // The cleanup the mark 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. expect(mockKernelStore.deleteVat).toHaveBeenCalledWith('v1'); }); @@ -744,10 +723,6 @@ describe('VatManager', () => { slots: [], }); - // `cleanupTerminatedVat` deletes these promises' c-list entries and - // drops the decider's refcount on the understanding that its caller - // rejected them first. A promise left unresolved with a decider that no - // longer exists hangs its waiters for good. expect(mockKernelQueue.resolvePromises).toHaveBeenCalledWith('v1', [ [ 'kp1', diff --git a/packages/ocap-kernel/src/vats/VatManager.ts b/packages/ocap-kernel/src/vats/VatManager.ts index 42f115f0c4..349d2446a5 100644 --- a/packages/ocap-kernel/src/vats/VatManager.ts +++ b/packages/ocap-kernel/src/vats/VatManager.ts @@ -22,18 +22,12 @@ import type { AllowedGlobalName } from './endowments.ts'; import { VatHandle } from './VatHandle.ts'; import type { PingVatResult } from '../rpc/index.ts'; -/** - * A vat being stopped, and the error its death is to be reported as. An ending - * vat always has one, since `#retireVat` rejects the promises it was deciding - * with it; a restarting vat must not, since `terminate` reads its absence as - * the difference. - */ type StopVatOptions = { vatId: VatId } & ( | { terminating: true; terminationError: Error } | { terminating: false; terminationError?: never } ); -/** The handle `runVat` made, once `VatHandle.make` has returned one. */ +/** Set once `VatHandle.make` returns. */ type RunningVat = { handle?: VatHandle }; type VatManagerOptions = { @@ -133,8 +127,7 @@ export class VatManager { try { await this.runVat(vatId, vatConfig); } catch (error) { - // Attribute the failure to the specific vat by kernel id and name. Any - // worker `runVat` started has already been stopped. + // Attribute the failure to the specific vat by kernel id and name. throw new Error(`Failed to launch vat ${vatId} (${vatName})`, { cause: error, }); @@ -153,11 +146,8 @@ export class VatManager { this.#kernelStore.setVatConfig(vatId, vatConfig); return rootRef; } catch (error) { - // The worker is already running, so leaving it would strand a vat the - // kernel has no record of. `stopVat` sets the terminated mark too, and - // early enough that asserting it here could only add it where `stopVat` - // stopped short of `deleteVat` — scheduling a cleanup for a vat whose - // own store and subcluster row it will not reach. + // No mark here: `stopVat` marks before `deleteVat`, so marking after it + // failed could schedule a cleanup for a vat `deleteVat` never reached. let stopFailure: unknown; try { await this.stopVat(vatId, true); @@ -178,10 +168,8 @@ export class VatManager { /** * Start a new or resurrected vat running. * - * `launch` spawns the worker before the handshake that can fail, and a - * handshake that fails records no handle to reach it by, so a worker left - * over from one is stopped here — the one place that knows whether `launch` - * got far enough to leave one. + * Stops the worker if the handshake fails, since no handle is left to stop + * it by. * * @param vatId - The ID of the vat to start. * @param vatConfig - Its configuration. @@ -197,9 +185,8 @@ export class VatManager { loggerStream as unknown as Parameters[0], (error) => this.#logger.error(`Vat ${vatId} error: ${stringify(error)}`), ); - // Holds the handle so that a report can be matched against the one that - // made it: a restart puts a new handle under the same vat id. Empty while - // `make` runs, in which window the catch below answers for the channel. + // Matches a report to the handle that made it, since a restart reuses the + // vat id. Until `make` returns, the catch below covers a failed channel. const running: RunningVat = {}; let vat: VatHandle; try { @@ -230,7 +217,7 @@ export class VatManager { } /** - * Take a handle's word that its channel has failed. + * Retire a vat whose handle reports a failed channel. * * @param options - Named options. * @param options.vatId - The vat whose channel failed. @@ -261,35 +248,14 @@ export class VatManager { /** * Retire a vat whose channel to its worker has failed. * - * Nobody asked for this death, so nobody is waiting to be told of it: the - * store learns of it here or not at all. Untold, the vat keeps its handle, - * its `vatConfig` row and the promises it was deciding, so the kernel goes on - * routing to a worker it cannot reach and the next boot brings it back. - * - * Between cranks, like `terminateVat`, and for a sharper reason. `#retireVat` - * writes synchronously, so run inside an open delivery savepoint the whole - * death is undone by any crank that goes on to abort — the ordinary vat-error - * path, not a failure — while the handle this dropped from the running map - * stays dropped, since nothing rolls memory back. That leaves a vat the store - * calls alive and the kernel cannot reach, and no error anywhere. Waiting is - * safe because the handle has already answered the callers this worker never - * will, so a crank blocked on one of them is free to finish. - * - * A vat the kernel is itself stopping reports the same way, since closing a - * channel with an error breaks the read its handle is draining. The identity - * check covers that along with everything the wait may have let happen: by - * then the vat may have been terminated, or restarted under a new handle. - * - * There is no one to retry this: a store failure here ends in a log line, - * leaving the vat unmarked, undeleted and handle-less until someone - * terminates it by hand. Said plainly in that log, for want of a better - * answer than the caller this path does not have. + * Waits for the current crank, since an aborting crank would roll back the + * retirement but not the handle's removal from `#vats`. Skips a handle that + * is no longer the vat's: the kernel may have stopped or restarted it. * * @param options - Named options. * @param options.vatId - The vat whose channel failed. * @param options.handle - The handle that reported it. - * @param options.error - What the channel failed with, for the rejections its - * subscribers are owed. + * @param options.error - What the channel failed with. */ async #retireLostVat({ vatId, @@ -339,26 +305,21 @@ export class VatManager { } /** - * Stop a vat, given the error its death is to be reported as. - * - * Split from `stopVat` for the caller that has an error rather than a - * serialized reason to give: a channel that died on its own. + * Stop a vat, given an error rather than a serialized reason. * * @param options - The vat to stop, and how. * @param options.vatId - The ID of the vat. * @param options.terminating - If true, the vat is being killed, if false, * it's being restarted. - * @param options.terminationError - Why it is ending, if it is. + * @param options.terminationError - Why it is ending, if terminating. */ async #stopVat({ vatId, terminating, terminationError, }: StopVatOptions): Promise { - // A restart needs a live handle to read its config from and to come back - // into; an ending vat does not, and must not. A failed relaunch leaves a - // vat the store still lists and the kernel has no handle for, and retiring - // it is exactly what puts that right. + // A terminating vat may have no handle: a failed relaunch leaves one + // persisted but not running. const vat = terminating ? this.#vats.get(vatId) : this.getVat(vatId); if (terminating && !vat && !this.#kernelStore.isVatActive(vatId)) { throw new VatNotFoundError(vatId); @@ -368,13 +329,11 @@ export class VatManager { if (terminating) { this.#retireVat(vatId, terminationError); } else { - // A restart keeps the pin and the records: the same vat, and the same - // root, are coming back. Only the handle goes. + // A restart keeps the pin and the records. this.#vats.delete(vatId); } } catch (error) { - // Held rather than thrown: the worker is being killed either way, and a - // record that failed must not leave one running. + // Rethrown below, once the worker is stopped. recordFailure = error as Error; } await this.#platformServices @@ -383,8 +342,7 @@ export class VatManager { try { await vat?.terminate(terminating, terminationError); } catch (error) { - // A channel that will not close is survivable and a store left - // half-written is not, so the latter is what the caller hears about. + // The store failure takes precedence. if (recordFailure === undefined) { throw error; } @@ -396,48 +354,18 @@ export class VatManager { } /** - * Record a vat's death: everything the kernel has to remember about it, in - * one synchronous step. - * - * Synchronous is the point. A death is four writes — the promises it was - * deciding rejected, its root unpinned, its config and store dropped, the - * terminated mark set — and none means much without the others. Interleaved - * with awaits, as they used to be, a crank lands between them and reads a vat - * that is half dead. - * - * 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 one still waiting to - * find out. - * - * The order is what a partial failure leaves behind. The mark comes after the - * rejections because it is what makes the vat eligible for - * `nextTerminatedVatCleanup`, and that cleanup deletes the decider promises' - * c-list entries on the stated understanding that its caller has already - * rejected them: a vat marked after those rejections failed is one whose - * subscribers hang for good. It comes before `deleteVat` because `deleteVat` - * drops the `vatConfig` row `stopVat` reads to decide there is still a vat - * here to retire — a mark not reached by then could never be set, and the - * c-list it gates never swept. So every way this can fail leaves either the - * mark set, or the row that lets the whole step be tried again. - * - * What that ordering costs: a `deleteVat` that throws leaves a marked vat - * whose `vatConfig` row survives, and `cleanupTerminatedVat` sweeps - * `${vatId}.` keys, which never match `vatConfig.${vatId}` — so once the - * cleanup drops the mark such a vat reads as active again. Retrying is what - * closes that, and the failure propagates so that someone can. + * Record a vat's death in one synchronous step, so no crank sees it half + * done. * - * Which is why only the writes that may happen once are guarded: a second - * `releaseVatRootPin` would spend an embedder's `pinVatRoot` pin instead of - * the launch pin this vat no longer holds. `deleteVat` is idempotent, and - * runs again to carry an interrupted retirement to the end. + * The mark follows the rejections, which the cleanup it schedules assumes + * happened, and precedes `deleteVat`, which drops the `vatConfig` row a retry + * needs to find the vat. The guard keeps a retry from unpinning twice and + * spending an embedder's pin. * * @param vatId - The vat being retired. - * @param error - Why, for the rejections its subscribers are owed. + * @param error - Why it is being retired. */ #retireVat(vatId: VatId, error: Error): void { - // Ahead of the guard, and safe there because nothing below reads it: a vat - // the store already calls dead must not keep a handle the router would go - // on resolving. this.#vats.delete(vatId); if (!this.#kernelStore.isVatTerminated(vatId)) { const failure = makeKernelError('VAT_TERMINATED', error.message); From 4dd8e8a48970c4f85c4f07c1248309c66709d300 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Thu, 1 Oct 2026 13:46:51 +0200 Subject: [PATCH 5/5] fix(ocap-kernel): drop a vat's config last so a failed deleteVat can be retried `#stopVat` finds a handle-less vat by its `vatConfig` row, and `deleteVat` dropped that row first. A failure on the vat store or subcluster write after it left those behind for good: a retry threw `VatNotFoundError`. Co-Authored-By: Claude Opus 5.5 (1M context) --- packages/ocap-kernel/src/store/index.test.ts | 20 +++++++++++++++++++- packages/ocap-kernel/src/store/index.ts | 3 ++- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/packages/ocap-kernel/src/store/index.test.ts b/packages/ocap-kernel/src/store/index.test.ts index 67371910a3..058b3505c3 100644 --- a/packages/ocap-kernel/src/store/index.test.ts +++ b/packages/ocap-kernel/src/store/index.test.ts @@ -1,5 +1,5 @@ import type { KernelDatabase } from '@metamask/kernel-store'; -import { describe, it, expect, beforeEach } from 'vitest'; +import { describe, it, expect, beforeEach, vi } from 'vitest'; import { makeKernelStore } from './index.ts'; import { makeMapKernelDatabase } from '../../test/storage.ts'; @@ -616,6 +616,24 @@ describe('kernel store', () => { }); }); + describe('deleteVat', () => { + it('keeps the config until the rest of the vat is gone', () => { + const ks = makeKernelStore(mockKernelDatabase); + ks.setVatConfig('v1', { sourceSpec: 'vat.js' }); + vi.spyOn(mockKernelDatabase, 'deleteVatStore').mockImplementationOnce( + () => { + throw new Error('deleteVatStore failed'); + }, + ); + + expect(() => ks.deleteVat('v1')).toThrow('deleteVatStore failed'); + expect(ks.isVatActive('v1')).toBe(true); + + ks.deleteVat('v1'); + expect(ks.isVatActive('v1')).toBe(false); + }); + }); + describe('cleanupTerminatedVat', () => { it('discards the config of a vat whose retirement did not', () => { const ks = makeKernelStore(mockKernelDatabase); diff --git a/packages/ocap-kernel/src/store/index.ts b/packages/ocap-kernel/src/store/index.ts index 35f2a0196b..50ec8e45dd 100644 --- a/packages/ocap-kernel/src/store/index.ts +++ b/packages/ocap-kernel/src/store/index.ts @@ -251,9 +251,10 @@ export function makeKernelStore(kdb: KernelDatabase, logger?: Logger) { * @param vatId - The vat whose state is to be deleted. */ function deleteVat(vatId: VatId): void { - vat.deleteVatConfig(vatId); kdb.deleteVatStore(vatId); subclusters.removeVatFromSubcluster(vatId); + // Last: a retry finds the vat by this row. + vat.deleteVatConfig(vatId); } /**