fix(ocap-kernel): finish retiring a vat that is not running

packages/ocap-kernel/CHANGELOG.md

@@ -61,6 +61,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- `terminateVat` now retires a vat that is persisted but not running, instead of throwing `VatNotFoundError` ([#1030](https://github.com/MetaMask/ocap-kernel/pull/1030))
+  - Reachable through `restartVat`: it stops the vat and then runs it again, and a relaunch that fails leaves the vat gone from the running map with its record, its own store and its root pin all still in place. Such a vat could not be terminated at all, so the only way to be rid of one was to discard the whole store, and `terminateSubcluster` — which walks persisted membership — rejected part-way through on reaching one, after deleting the system-subcluster mapping and before removing the subcluster record
+  - Retiring one does everything `stopVat` does apart from stopping a worker: it discards the vat's persisted record, rejects the promises the vat was deciding, and releases the pin `launchVat` took on its root. Marking it terminated is not enough on its own — the deferred cleanup that follows walks keys prefixed `${vatId}.`, which never matches the `vatConfig.${vatId}` that decides whether the next boot restores the vat, and it states that its caller has already rejected those promises
+  - A vat that is neither running nor persisted still throws
+  - `removeVatFromSubcluster` no longer reports a vat that belongs to no subcluster. It is reached from `deleteVat` while a vat is being discarded, which is the one moment a failure cannot be retried past
 - A `notify`, GC action, or `bringOutYourDead` addressed to an endpoint that is not running is skipped instead of killing the run loop ([#1029](https://github.com/MetaMask/ocap-kernel/pull/1029))
   - These deliveries looked up their endpoint unguarded, so an endpoint named by persisted state but absent from the running kernel threw `VatNotFoundError` from inside the crank, which killed the run loop permanently. Because the crank was rolled back the item was restored to the queue, so the next boot dequeued it and died too
   - Reached by a peer's routine remote GC. A kernel answers a peer's `bringOutYourDead` by scheduling a reap against the remote it came from, and the reap queue is persisted. `scheduleReap` does not wake a parked run loop, so on an otherwise idle kernel that reap sits in the queue until something else gives the loop work — and a kernel shut down in the meantime carries it into the next incarnation. That incarnation starts its run loop inside `Kernel.make`, before an embedder can call `initRemoteComms`, which is what restores remote handles; reaps are taken ahead of the run queue, so the loop's first act is to deliver one addressed to a remote that does not exist yet. The kernel is dead before `Kernel.make` returns, and stays dead on every boot after that

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', () => {

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

packages/ocap-kernel/src/vats/VatManager.test.ts

@@ -69,7 +69,10 @@ describe('VatManager', () => {
         })(),
       ),
       getVatSubcluster: vi.fn().mockReturnValue('s1'),
+      isVatActive: vi.fn().mockReturnValue(true),
       markVatAsTerminated: vi.fn(),
+      deleteVat: vi.fn(),
+      getPromisesByDecider: vi.fn().mockReturnValue([]),
       getRootObject: vi.fn().mockReturnValue('ko1'),
       pinObject: vi.fn(),
       unpinObject: vi.fn(),
@@ -80,6 +83,7 @@ describe('VatManager', () => {
 
     mockKernelQueue = {
       waitForCrank: vi.fn().mockResolvedValue(undefined),
+      resolvePromises: vi.fn(),
     } as unknown as Mocked<KernelQueue>;
 
     mockLogger = new Logger('test');
@@ -345,6 +349,95 @@ describe('VatManager', () => {
         expect.objectContaining({ message: 'Vat termination: Custom reason' }),
       );
     });
+
+    describe('a vat that is persisted but not running', () => {
+      // `restartVat` stops the vat and then runs it again. A relaunch that
+      // fails — the bundle has moved since the vat was launched, the worker
+      // will not spawn — leaves the vat gone from the running map with its
+      // record, its vat store and its root pin all still in place. Nothing
+      // else retires one, so `terminateVat` has to.
+      const leaveVatPersistedButNotRunning = async (): Promise<void> => {
+        await vatManager.runVat('v1', createMockVatConfig());
+        mockPlatformServices.launch.mockRejectedValueOnce(
+          new Error('ENOENT: no such file or directory'),
+        );
+        await expect(vatManager.restartVat('v1')).rejects.toThrow('ENOENT');
+        expect(vatManager.hasVat('v1')).toBe(false);
+      };
+
+      it('can be terminated after a failed restart', async () => {
+        await leaveVatPersistedButNotRunning();
+
+        // Went through `stopVat` → `getVat` and threw, so a vat in this state
+        // could not be retired at all, and `terminateSubcluster` — which walks
+        // persisted membership — rejected part way through on reaching one.
+        expect(await vatManager.terminateVat('v1')).toBeUndefined();
+        expect(mockKernelStore.markVatAsTerminated).toHaveBeenCalledWith('v1');
+      });
+
+      it('discards its persisted record', async () => {
+        await leaveVatPersistedButNotRunning();
+
+        await vatManager.terminateVat('v1');
+
+        // Marking it terminated is not enough. The cleanup that schedules walks
+        // keys prefixed `${vatId}.`, which never matches `vatConfig.${vatId}`;
+        // only `deleteVat` takes that, along with the vat's own store and its
+        // subcluster membership. Left behind, the record restores the vat at
+        // the next boot whose code is reachable.
+        expect(mockKernelStore.deleteVat).toHaveBeenCalledWith('v1');
+      });
+
+      it('rejects the promises it was deciding', async () => {
+        mockKernelStore.getPromisesByDecider.mockReturnValue(['kp1']);
+        await leaveVatPersistedButNotRunning();
+
+        await vatManager.terminateVat('v1');
+
+        // `cleanupTerminatedVat` deletes these promises' c-list entries and
+        // drops the decider's refcount on the stated understanding that its
+        // caller has already rejected them. Nothing else can: a promise left
+        // unresolved with a decider that no longer exists hangs its waiters for
+        // good.
+        expect(mockKernelQueue.resolvePromises).toHaveBeenCalledWith('v1', [
+          ['kp1', true, expect.anything()],
+        ]);
+      });
+
+      it('carries the termination reason into those rejections', async () => {
+        mockKernelStore.getPromisesByDecider.mockReturnValue(['kp1']);
+        await leaveVatPersistedButNotRunning();
+
+        await vatManager.terminateVat('v1', {
+          body: 'Custom reason',
+          slots: [],
+        });
+
+        const [, resolutions] = mockKernelQueue.resolvePromises.mock
+          .calls[0] as [string, [string, boolean, { body: string }][]];
+        expect(resolutions[0]?.[2]?.body).toContain('Custom reason');
+      });
+
+      it('releases the pin its root was launched with', async () => {
+        await leaveVatPersistedButNotRunning();
+
+        await vatManager.terminateVat('v1');
+
+        // `launchVat` pins the root for the vat's lifetime and `stopVat`
+        // releases it when terminating. A vat retired without going through
+        // `stopVat` would leave its root pinned, and so uncollectable, forever.
+        expect(mockKernelStore.unpinObject).toHaveBeenCalledWith('ko1');
+      });
+
+      it('throws for a vat that is neither running nor persisted', async () => {
+        mockKernelStore.isVatActive.mockReturnValue(false);
+
+        await expect(vatManager.terminateVat('v9')).rejects.toThrow(
+          VatNotFoundError,
+        );
+        expect(mockKernelStore.markVatAsTerminated).not.toHaveBeenCalled();
+      });
+    });
   });
 
   describe('restartVat', () => {

packages/ocap-kernel/src/vats/VatManager.ts

@@ -8,6 +8,7 @@ import { stringify } from '@metamask/kernel-utils';
 import { Logger, splitLoggerStream } from '@metamask/logger';
 
 import type { KernelQueue } from '../KernelQueue.ts';
+import { makeKernelError } from '../liveslots/kernel-marshal.ts';
 import type { KernelStore } from '../store/index.ts';
 import type {
   VatId,
@@ -202,15 +203,58 @@ export class VatManager {
     this.#vats.delete(vatId);
   }
 
+  /**
+   * Retire a persisted vat that is not running.
+   *
+   * A vat can be persisted without running: `restartVat` stops it and then runs
+   * it again, and a relaunch that fails leaves the vat gone from the running
+   * map with its record, its own store and its root pin all still in place.
+   * Such a vat never reaches `VatHandle.terminate`, which is where a running
+   * vat's records are discarded and the promises it was deciding are rejected,
+   * and nobody else does either: the deferred `cleanupTerminatedVat` states
+   * outright that its caller has already rejected those promises, and it walks
+   * keys prefixed `${vatId}.`, which never matches the `vatConfig.${vatId}`
+   * that decides whether the next boot restores this vat.
+   *
+   * Everything `stopVat` does apart from stopping a worker, therefore.
+   *
+   * @param vatId - The ID of the vat.
+   * @param reason - The reason for the termination, if any.
+   */
+  #retirePersistedVat(vatId: VatId, reason?: CapData<KRef>): void {
+    const terminationError = reason
+      ? new Error(`Vat termination: ${reason.body}`)
+      : new VatDeletedError(vatId);
+    this.releaseVatRootPin(vatId);
+    const failure = makeKernelError('VAT_TERMINATED', terminationError.message);
+    for (const kpid of this.#kernelStore.getPromisesByDecider(vatId)) {
+      this.#kernelQueue.resolvePromises(vatId, [[kpid, true, failure]]);
+    }
+    this.#kernelStore.deleteVat(vatId);
+  }
+
   /**
    * Terminate a vat with extreme prejudice.
    *
+   * Terminates a persisted vat that is not running as readily as one that is
+   * (see `#retirePersistedVat`) — otherwise the only way to be rid of one is to
+   * discard the whole store, and `SubclusterManager.terminateSubcluster`, which
+   * walks persisted membership, strands every subcluster containing one.
+   *
    * @param vatId - The ID of the vat.
    * @param reason - If the vat is being terminated, the reason for the termination.
    */
   async terminateVat(vatId: VatId, reason?: CapData<KRef>): Promise<void> {
     await this.#kernelQueue.waitForCrank();
-    await this.stopVat(vatId, true, reason);
+    if (this.hasVat(vatId)) {
+      await this.stopVat(vatId, true, reason);
+    } else if (this.#kernelStore.isVatActive(vatId)) {
+      this.#retirePersistedVat(vatId, reason);
+    } else {
+      // Not running *and* not persisted: this vat is simply unknown, and
+      // saying so beats silently retiring records that were never there.
+      throw new VatNotFoundError(vatId);
+    }
     // Mark for deletion (which will happen later, in vat-cleanup events)
     this.#kernelStore.markVatAsTerminated(vatId);
   }