fix(ocap-kernel): keep the kernel's account of a vat's life in one piece

packages/kernel-test/src/crank-rollback.test.ts

@@ -238,6 +238,33 @@ describe('crank rollback against a real database', () => {
     kernelStore.endCrank();
   });
 
+  // The set is not per-crank: only `collectGarbage` empties it, and that runs at
+  // the end of a crank that had an item. So a candidate created while the run
+  // loop was idle — `terminateVat` unpinning a root is the real path — is still
+  // owed a collection, and an unrelated crank's rollback must not cancel it.
+  it('keeps GC candidates that predate the crank it rolled back', async () => {
+    const { kernelStore } = await makeStore();
+    const idle = kernelStore.initKernelPromise()[0];
+    kernelStore.decrementRefCount(idle, 'test');
+
+    kernelStore.startCrank();
+    kernelStore.createCrankSavepoint('start');
+    const abandoned = kernelStore.initKernelPromise()[0];
+    kernelStore.decrementRefCount(abandoned, 'test');
+    kernelStore.rollbackCrank('start');
+    kernelStore.endCrank();
+
+    kernelStore.startCrank();
+    kernelStore.createCrankSavepoint('start');
+    kernelStore.collectGarbage();
+    kernelStore.endCrank();
+
+    // Collected, because it was owed before the abandoned crank began.
+    expect(() => kernelStore.getKernelPromise(idle)).toThrow(
+      'unknown kernel promise',
+    );
+  });
+
   // `createCrankSavepoint` records the name only once the database has the
   // savepoint. Asking to roll back one that was never created must therefore say
   // so, rather than releasing someone else's savepoint.

packages/kernel-test/src/garbage-collection.test.ts

@@ -257,26 +257,39 @@ describe('Garbage Collection', () => {
      * Give an importer a chance to notice a dropped object and tell the kernel,
      * then keep cranking until the resulting GC actions have all been consumed.
      *
+     * Waits for `done` as well as for an empty action set, because an empty set
+     * is also what "the vat has not told us anything yet" looks like. A vat
+     * reports a dropped import only once the engine has actually collected it,
+     * and `gcAndFinalize` can only provoke that, not guarantee it on the first
+     * try — so a round that reports nothing has to be retried rather than read
+     * as the end of the story. Reaped afresh each round for the same reason:
+     * the report rides on a `bringOutYourDead`.
+     *
      * @param vatId - The vat to reap.
      * @param rootKRef - That vat's root, to poke with cranks afterwards.
+     * @param done - The outcome being waited for.
      */
-    async function reapAndSettle(vatId: VatId, rootKRef: KRef): Promise<void> {
-      kernel.reapVats((id) => id === vatId);
-      // BOYD has to reach the vat, the vat has to answer, and the kernel has to
-      // act on the answer — but a round can queue more work, so loop until the
-      // queue is actually empty rather than guessing at a crank count.
+    async function reapAndSettle(
+      vatId: VatId,
+      rootKRef: KRef,
+      done: () => boolean,
+    ): Promise<void> {
       const maxRounds = 10;
       for (let round = 0; round < maxRounds; round++) {
+        kernel.reapVats((id) => id === vatId);
+        // BOYD has to reach the vat, the vat has to answer, and the kernel has
+        // to act on the answer — but a round can queue more work, so loop until
+        // the queue is actually empty rather than guessing at a crank count.
         await kernel.queueMessage(rootKRef, 'noop', []);
         await waitUntilQuiescent(500);
-        if ([...kernelStore.getGCActions()].length === 0) {
+        if ([...kernelStore.getGCActions()].length === 0 && done()) {
           return;
         }
       }
       throw Error(
-        `GC actions still pending after ${maxRounds} rounds: ${[
-          ...kernelStore.getGCActions(),
-        ].join(', ')}`,
+        `GC did not settle after ${maxRounds} rounds; actions pending: ${
+          [...kernelStore.getGCActions()].join(', ') || '(none)'
+        }`,
       );
     }
 
@@ -311,7 +324,11 @@ describe('Garbage Collection', () => {
       await kernel.queueMessage(importerKRef, 'makeWeak', [objectId]);
       await kernel.queueMessage(importerKRef, 'forgetImport', []);
       await waitUntilQuiescent();
-      await reapAndSettle(importerVatId, importerKRef);
+      await reapAndSettle(
+        importerVatId,
+        importerKRef,
+        () => !kernelStore.getImporters(sharedKRef).includes(importerVatId),
+      );
 
       // The exporter must not have been told to drop it: the second importer
       // legitimately still holds it
@@ -344,7 +361,11 @@ describe('Garbage Collection', () => {
       await kernel.queueMessage(secondImporterKRef, 'makeWeak', [objectId]);
       await kernel.queueMessage(secondImporterKRef, 'forgetImport', []);
       await waitUntilQuiescent();
-      await reapAndSettle(secondImporterVatId, secondImporterKRef);
+      await reapAndSettle(
+        secondImporterVatId,
+        secondImporterKRef,
+        () => kernelStore.getImporters(sharedKRef).length === 0,
+      );
 
       expect(kernelStore.getImporters(sharedKRef)).toStrictEqual([]);
       // Only the createObject result's stored value still names it

packages/ocap-kernel/CHANGELOG.md

@@ -77,6 +77,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Forget a crank's savepoints when the store call that ends them fails, in both `rollbackCrank` and `endCrank` ([#1021](https://github.com/MetaMask/ocap-kernel/pull/1021))
   - A failed rollback or release discards the whole transaction, savepoints included. Keeping them listed had `endCrank` throw `No such savepoint: t0` from the run loop's `finally`, over whatever really killed the kernel
 - `rollbackCrank` now also reverts the two pieces of state a database rollback cannot reach, whether or not the rollback itself succeeded: every cached stored value is re-read, and the crank's accumulated garbage-collection candidates are discarded ([#1021](https://github.com/MetaMask/ocap-kernel/pull/1021))
+  - The candidate set is restored to its state at the savepoint rather than emptied, since candidates accrued while the run loop was idle are owed a collection an unrelated crank's rollback must not cancel ([#1023](https://github.com/MetaMask/ocap-kernel/pull/1023))
   - A cached stored value answers reads from a closure and only writes through to the database, so reverting the database left it holding the abandoned crank's value and the next write persisted that. `processGCActionSet` takes an action out of the set before delivering it, so an aborted delivery lost the action outright instead of retrying it
   - `maybeFreeKrefs` lives in RAM, so nothing reverted it either. Its entries are collection candidates only because of decrements the rollback undid, and a later `collectGarbage` threw outright on a promise the rollback had deleted, killing the run loop
   - A failed rollback discards the whole transaction, moving the database back at least as far as a successful rollback would have — so reverting only on success left exactly the state that is least able to tolerate it
@@ -95,6 +96,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   - Renames `krefsToExistingErefs` to `krefsToErefs`, which now throws on an unmapped kref instead of silently dropping it
 - Pin vat root objects for the lifetime of their vat, and release the pin on termination ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020))
   - A root is addressable while its vat lives whether or not anyone imports it; the old `(1, 1)` birth baseline was standing in for this
+  - The pin is released when a relaunch fails too, which vat cleanup does not do ([#1023](https://github.com/MetaMask/ocap-kernel/pull/1023))
 - Garbage-collection action delivery now moves the kernel's own c-list: `dropExports` clears the owner's reachable flag and `retireExports`/`retireImports` tear the entry down ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020))
   - Previously the owner's flag never cleared, so the same action could be re-derived, and retired entries outlived the objects they named
 - Orphan a kernel object when its owner stops naming it (a delivered `retireExport`, or a `retireExports`/`abandonExports` syscall), so its `owner` and `refCount` records no longer outlive the c-list entry they were reachable through ([#1022](https://github.com/MetaMask/ocap-kernel/pull/1022))
@@ -105,6 +107,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   - It releases only where the endpoint is genuinely gone: a terminated vat, whose cleanup tears the whole c-list down anyway, or a remote, which reconciles on its next incarnation. A vat that is absent yet not terminated is one `restartVat` has taken out of the kernel's reach while keeping its c-list, so the crank fails there instead of committing a release the returning incarnation would disagree with
 - A failed garbage-collection delivery to a remote is logged and survived rather than escaping the crank and stopping the run loop ([#1022](https://github.com/MetaMask/ocap-kernel/pull/1022))
 - Tear down and mark for cleanup a vat whose worker launched but whose kernel-side registration then failed ([#1022](https://github.com/MetaMask/ocap-kernel/pull/1022))
+- A vat's death is recorded in one synchronous step, so a worker that refuses to go cannot leave the record half-written ([#1023](https://github.com/MetaMask/ocap-kernel/pull/1023))
+  - Only `deleteVat` removes a vat's config, and terminated-vat cleanup does not call it, so the previous interleaving could leave a vat marked terminated whose config survived — which reads as _active_ again as soon as cleanup drops the mark, killing the run loop over the disagreement and resurrecting the vat on the next process start
+- The run loop carries out a vat restart itself, as a queued request, so a vat is never out of the kernel's reach while cranks run; a crank that landed in that window read a live vat as a dead one ([#1023](https://github.com/MetaMask/ocap-kernel/pull/1023))
+  - Adds the `restartVat` run-queue item and `KernelQueue.enqueueRestartVat()`. `Kernel.restartVat` settles when the crank has done it, and rejects outright if the run loop is dead
+- A delivery whose endpoint has vanished is dropped only where the endpoint is gone for good — a terminated vat or a remote. `notify` and `bringOutYourDead` no longer take the run loop down when it has, and a `send` no longer reports a live endpoint as unreachable and discards a deliverable message ([#1023](https://github.com/MetaMask/ocap-kernel/pull/1023))
+  - Adds `VatManager.provideVat()`, which waits out a vat being torn down before answering, and makes the kernel's endpoint lookup asynchronous
+- A restart that cannot relaunch its vat now terminates it and reports the failure to the caller, instead of killing the run loop — which rolled the crank back, undoing the termination records and returning the request to the queue, so every subsequent process start replayed the same failing restart ([#1023](https://github.com/MetaMask/ocap-kernel/pull/1023))
+- Terminating a vat with a restart still queued for it no longer kills the run loop when the crank reaches that request ([#1023](https://github.com/MetaMask/ocap-kernel/pull/1023))
+- Work outliving a vat that has already been cleaned up — a `bringOutYourDead` scheduled before it died, say — is dropped rather than taken as a live vat the kernel has lost track of, which killed the run loop. Cleanup unmarks the vat it finishes, so "terminated" alone could not identify one ([#1023](https://github.com/MetaMask/ocap-kernel/pull/1023))
+- `getImporters` now counts remotes, so retiring an object queues a `retireImport` for a remote importer rather than deleting the object and leaving the remote's c-list entry naming nothing ([#1015](https://github.com/MetaMask/ocap-kernel/issues/1015))
+- A vat reports its dropped imports on the `bringOutYourDead` that provoked the collection, rather than on some later one. The queues are now drained before the sweep, since a pending continuation still holds its closure's objects and a sweep run with work outstanding finds them reachable ([#1023](https://github.com/MetaMask/ocap-kernel/pull/1023))
 - Charge a delivered message's target reference against the run-queue item's own target rather than the routed target, so a message routed through a resolved promise no longer decrements an object nobody charged while leaking the promise ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020))
 - Release a queued notification's reference before the paths that decide there is nothing to deliver, and stop decrementing references on promises retired alongside it that nobody had taken ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020))
 - Transfer, rather than duplicate, the references a message carries when it is queued on an unresolved promise and later re-enqueued on resolution ([#1020](https://github.com/MetaMask/ocap-kernel/pull/1020))

packages/ocap-kernel/src/Kernel.test.ts

@@ -31,13 +31,23 @@ const mocks = vi.hoisted(() => {
 
     #rejectRunLoop: ((error: Error) => void) | undefined;
 
+    #deliver: ((item: unknown) => Promise<unknown>) | undefined;
+
     // Like the real run loop, this settles only if the kernel dies.
-    run = vi.fn(
-      async () =>
-        new Promise<never>((_resolve, reject) => {
-          this.#rejectRunLoop = reject;
-        }),
-    );
+    run = vi.fn(async (deliver: (item: unknown) => Promise<unknown>) => {
+      this.#deliver = deliver;
+      return new Promise<never>((_resolve, reject) => {
+        this.#rejectRunLoop = reject;
+      });
+    });
+
+    // A restart is the run loop's work, so stand in for it reaching the request
+    // on its next crank. The failure is absorbed here rather than dropped: the
+    // real run loop would die of it, and the caller hears about it from the
+    // waiter `restartVat` registered, not from this call.
+    enqueueRestartVat = vi.fn((vatId: string) => {
+      this.#deliver?.({ type: 'restartVat', vatId }).catch(() => undefined);
+    });
 
     /**
      * Fail the run loop, in the order the real `KernelQueue.run` does: the

packages/ocap-kernel/src/Kernel.ts

@@ -111,12 +111,12 @@ export class Kernel {
    * @param options.onRunLoopFailure - Optional handler called if the run loop dies.
    * @param options.auditRefCounts - If true, verify every kref's reference
    * counts against the references the kernel actually holds at the end of each
-   * crank, and throw on any mismatch. This is the check standing in for the
-   * accounting invariant `collectGarbage` still cannot assert (see the comment
-   * on its `retireExport` branch), so it is not optional
-   * instrumentation: it is off by default only because it walks the whole store
-   * every crank. Any kernel whose accounting is under test wants it on, and
-   * every kernel `kernel-test` builds enables it.
+   * crank, and throw on any mismatch. Not optional instrumentation: it is what
+   * establishes that the accounting is right, and is off by default only because
+   * it walks the whole store every crank. Any kernel whose accounting is under
+   * test wants it on, and every kernel `kernel-test` builds enables it. Note
+   * that it checks counts against their holders, which is a different invariant
+   * from the one `collectGarbage`'s `retireExport` branch still cannot assert.
    */
   // eslint-disable-next-line no-restricted-syntax
   private constructor(
@@ -155,10 +155,13 @@ 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);
-      },
+      // `stopVat` rather than `terminateVat`: this runs inside the crank that
+      // decided the vat has to go, and `terminateVat` would wait for that same
+      // crank to end. It needs no such wait — the run loop is right here — and
+      // `stopVat` puts the whole death on record before its first await, so a
+      // worker that refuses to die cannot leave the store half-told.
+      async (vatId, reason) =>
+        await this.#vatManager.stopVat(vatId, true, reason),
     );
 
     this.#vatManager = new VatManager({
@@ -230,6 +233,7 @@ export class Kernel {
       this.#kernelServiceManager.invokeKernelService.bind(
         this.#kernelServiceManager,
       ),
+      this.#vatManager.performVatRestart.bind(this.#vatManager),
       this.#logger,
     );
 
@@ -651,13 +655,19 @@ export class Kernel {
   /**
    * Gets an endpoint by its ID.
    *
+   * Asynchronous because a vat may be mid-teardown: `provideVat` waits that out
+   * rather than answering from a vat table the store has not caught up with, so
+   * by the time a caller is told the vat is gone the store says so too — which
+   * is what lets `#resolveEndpoint` tell a terminated vat from a missing one. A
+   * restart needs no such window, being carried out by the run loop itself.
+   *
    * @param endpointId - The ID of the endpoint to retrieve.
-   * @returns The endpoint handle for the given ID.
+   * @returns A promise for the endpoint handle for the given ID.
    * @throws If the endpoint ID is invalid (neither a vat ID nor a remote ID).
    */
-  #getEndpoint(endpointId: EndpointId): EndpointHandle {
+  async #getEndpoint(endpointId: EndpointId): Promise<EndpointHandle> {
     if (isVatId(endpointId)) {
-      return this.#vatManager.getVat(endpointId);
+      return await this.#vatManager.provideVat(endpointId);
     }
     if (isRemoteId(endpointId)) {
       return this.#remoteManager.getRemote(endpointId);

packages/ocap-kernel/src/KernelQueue.test.ts

@@ -1224,6 +1224,27 @@ describe('KernelQueue', () => {
     });
   });
 
+  describe('enqueueRestartVat', () => {
+    it('enqueues the request for the run loop to carry out', () => {
+      kernelQueue.enqueueRestartVat('v1');
+
+      expect(kernelStore.enqueueRun).toHaveBeenCalledWith({
+        type: 'restartVat',
+        vatId: 'v1',
+      });
+    });
+
+    it('refuses once the run loop has died', async () => {
+      await killRunLoop(new Error('boom'));
+
+      // The restart is the loop's work, so a dead loop will never do it and the
+      // caller would wait forever.
+      expect(() => kernelQueue.enqueueRestartVat('v1')).toThrow(
+        'Kernel run loop died; cannot restart a vat',
+      );
+    });
+  });
+
   describe('waitForCrank', () => {
     it('handles when waitForCrank returns a delayed promise', async () => {
       let resolvePromise: ((value: void) => void) | undefined;

packages/ocap-kernel/src/KernelQueue.ts

@@ -501,6 +501,23 @@ export class KernelQueue {
     }
   }
 
+  /**
+   * Enqueue a request to replace a vat's worker.
+   *
+   * The work itself belongs to the run loop, which is the point: a restart done
+   * where it is asked for takes the vat out of the kernel's reach while cranks
+   * continue, and a crank that lands in that window reads a live vat as a dead
+   * one. Queued, the restart happens in a crank of its own.
+   *
+   * @param vatId - The vat whose worker is to be replaced.
+   */
+  enqueueRestartVat(vatId: VatId): void {
+    // The restart is the run loop's work now, so a dead loop will never do it,
+    // and a caller awaiting it would wait forever.
+    this.assertRunLoopAlive('restart a vat');
+    this.#enqueueRun({ type: 'restartVat', vatId });
+  }
+
   /**
    * Enqueue a notification of promise resolution to an endpoint.
    *