fix(swingset): change processUpgradeVat to delete clist entries

The kernel is responsible for "disconnecting" (rejecting) promises
decided by a vat being upgraded, because Promise settlement
functions (resolve/reject) are ephemeral, so they are lost during the
upgrade, so the new incarnation will have no way to ever settle them.

When the vat resolves a promise with syscall.resolve(), that vat's
c-list entry is removed by the syscall handler. But when the kernel
did the rejection as part of upgrade, the c-list entry was left in
place, causing the promise to be pinned forever.

This changes the kernel to remove the c-list entry as well, except in
the case that the vat is also subscribed to its own promise (which
happens when a PromiseWatcher is used on a locally-decided promise:
unusual but we needed to tolerate that case). Self-subscribed promises
are left in the c-list, because the subsequent dispatch.notify() will
finish the job and remove the entry.

This adds unit tests for the new vat-upgrade -time behavior (including
the self-subscription case). It does not yet add remediation code,
which will appear in a later commit. Therefore it merely:

refs #9039

rather than fixing it entirely.

packages/SwingSet/src/kernel/kernel.js

@@ -1000,9 +1000,30 @@ export default function buildKernel(
       return results;
     }
 
-    // reject all promises for which the vat was decider
-    for (const [kpid, _p] of kernelKeeper.enumeratePromisesByDecider(vatID)) {
-      resolveToError(kpid, disconnectionCapData, vatID);
+    // We are homesick for a future in which most promises are
+    // durable, and vats do not need to subscribe to their own
+    // promises to make promise-watchers work. In that world, vats
+    // somehow mark the non-durable promises, which we must
+    // reject/disconnect on their behalf during upgrade.
+    //
+    // To handle the present reality, without durable promises, we
+    // pretend that all promises are so marked.
+
+    // reject all ephemeral promises for which the vat was decider
+    for (const [kpid, p] of kernelKeeper.enumeratePromisesByDecider(vatID)) {
+      const isEphemeral = true; // future vats will mark these explicitly
+      const selfSubscribed =
+        p.state === 'unresolved' && p.subscribers.includes(vatID);
+      if (isEphemeral) {
+        resolveToError(kpid, disconnectionCapData, vatID);
+        if (!selfSubscribed) {
+          // If the vat was subscribed to its own promise, the
+          // resolveToError will enqueue a dispatch.notify, whose delivery
+          // will delete the c-list entry. If it is *not* subscribed, we
+          // should delete the c-list entry now, because nobody else will.
+          vatKeeper.deleteCListEntriesForKernelSlots([kpid]);
+        }
+      }
     }
 
     // simulate an abandonExports syscall from the vat,

packages/SwingSet/test/upgrade/bootstrap-scripted-upgrade.js

@@ -78,8 +78,19 @@ export const buildRootObject = () => {
       p1.catch(() => 'hush');
       const p2 = E(ulrikRoot).getEternalPromise();
       p2.catch(() => 'hush');
-
-      return { version, data, p1, p2, retain, ...parameters };
+      const p3 = E(ulrikRoot).getWatchedDecidedPromise();
+
+      // Create our own promises that capture the rejection data,
+      // because the originals will be GCed when the rejection
+      // notifications finish delivery. Promise.all() returns a new
+      // promise, which either fulfills to an array of fulfillment
+      // values, or rejects with the first rejection reason, so for
+      // p1/p2 (which are supposed to reject), it effectively wraps
+      // the promise in a new one that behaves the same way.
+      const p1w = Promise.all([p1]);
+      const p2w = Promise.all([p2]);
+
+      return { version, data, p1, p2, p3, p1w, p2w, retain, ...parameters };
     },
 
     upgradeV2: async () => {
@@ -145,8 +156,17 @@ export const buildRootObject = () => {
       resolve(`message for your predecessor, don't freak out`);
 
       const newDur = await E(ulrikRoot).getNewDurandal();
-
-      return { version, data, remoerr, newDur, upgradeResult, ...parameters };
+      const watcherResult = await E(ulrikRoot).getWatcherResult();
+
+      return {
+        version,
+        data,
+        remoerr,
+        newDur,
+        upgradeResult,
+        watcherResult,
+        ...parameters,
+      };
     },
 
     buildV1WithLostKind: async () => {

packages/SwingSet/test/upgrade/upgrade.test.js

@@ -363,6 +363,11 @@ const testUpgrade = async (t, defaultManagerType, options = {}) => {
   // grab the promises that should be rejected
   const v1p1Kref = verifyPresence(v1result.p1);
   const v1p2Kref = verifyPresence(v1result.p2);
+  const v1p1wKref = verifyPresence(v1result.p1w);
+  const v1p2wKref = verifyPresence(v1result.p2w);
+  const v1p3Kref = verifyPresence(v1result.p3);
+  c.kpRegisterInterest(v1p1wKref);
+  c.kpRegisterInterest(v1p2wKref);
   // grab krefs for the exported durable/virtual objects to check their abandonment
   const retainedKrefs = objectMap(v1result.retain, obj => verifyPresence(obj));
   const retainedNames = 'dur1 vir2 vir5 vir7 vc1 vc3 dc4 rem1 rem2 rem3'.split(
@@ -461,16 +466,34 @@ const testUpgrade = async (t, defaultManagerType, options = {}) => {
   const newDurKref = verifyPresence(v2result.newDur);
   t.not(newDurKref, dur1Kref);
 
-  // the old version's non-durable promises should be rejected
-  t.is(c.kpStatus(v1p1Kref), 'rejected');
+  // The old version's non-durable promises should be rejected. The
+  // original kpids will be GCed. Bug #9039 failed to GC them.
+  t.is(kvStore.get(`${v1p1Kref}.refCount`), undefined);
+  t.is(kvStore.get(`${v1p2Kref}.refCount`), undefined);
+  t.is(kvStore.get(`${vatID}.c.${v1p1Kref}`), undefined);
+  t.is(kvStore.get(`${vatID}.c.${v1p2Kref}`), undefined);
+  t.is(kvStore.get(`${v1p1Kref}.state`), undefined);
+  t.is(kvStore.get(`${v1p2Kref}.state`), undefined);
+  // but vat-bootstrap wraps them (with Promise.all() so we can
+  // examine their rejection data.
+  t.is(c.kpStatus(v1p1wKref), 'rejected');
   const vatUpgradedError = {
     name: 'vatUpgraded',
     upgradeMessage: 'test upgrade',
     incarnationNumber: 0,
   };
-  t.deepEqual(kunser(c.kpResolution(v1p1Kref)), vatUpgradedError);
-  t.is(c.kpStatus(v1p2Kref), 'rejected');
-  t.deepEqual(kunser(c.kpResolution(v1p2Kref)), vatUpgradedError);
+  t.deepEqual(kunser(c.kpResolution(v1p1wKref)), vatUpgradedError);
+  t.is(c.kpStatus(v1p2wKref), 'rejected');
+  t.deepEqual(kunser(c.kpResolution(v1p2wKref)), vatUpgradedError);
+
+  // The local-promise watcher should have fired. If vat-upgrade is
+  // too aggressive about removing c-list entries (i.e. it doesn't
+  // check for self-subscription first), the kernel-provoked
+  // dispatch.notify won't be delivered, and the new incarnation won't
+  // fire the durable watcher.
+  t.deepEqual(v2result.watcherResult, ['reject', vatUpgradedError]);
+  // and the promise it was watching should be GCed
+  t.is(kvStore.get(`${v1p3Kref}.refCount`), undefined);
 
   // verify export abandonment/garbage collection/etc.
   // This used to be MUCH more extensive, but GC was cut to the bone

packages/SwingSet/test/upgrade/vat-ulrik-1.js

@@ -8,6 +8,7 @@ import {
   defineKind,
   makeScalarBigMapStore,
   makeScalarBigWeakMapStore,
+  watchPromise,
 } from '@agoric/vat-data';
 
 // we set up a lot of ephemeral, merely-virtual, and durable objects
@@ -29,6 +30,20 @@ const holderMethods = {
 const makeVir = defineKind('virtual', initHolder, holderMethods);
 const makeDur = defineDurableKind(durandalHandle, initHolder, holderMethods);
 
+const initWatcher = () => harden({ result: ['unresolved'] });
+const watcherMethods = {
+  onFulfilled: ({ state }, data) => (state.result = harden(['fulfill', data])),
+  onRejected: ({ state }, reason) =>
+    (state.result = harden(['reject', reason])),
+  getResult: ({ state }) => state.result,
+};
+const watcherHandle = makeKindHandle('watcher');
+const makeWatcher = defineDurableKind(
+  watcherHandle,
+  initWatcher,
+  watcherMethods,
+);
+
 // TODO: explore 'export modRetains'
 // eslint-disable-next-line no-unused-vars
 let modRetains;
@@ -134,6 +149,12 @@ const buildExports = (baggage, imp) => {
 export const buildRootObject = (_vatPowers, vatParameters, baggage) => {
   const { promise: p1 } = makePromiseKit();
   const { promise: p2 } = makePromiseKit();
+  const { promise: p3 } = makePromiseKit();
+  const watcher = makeWatcher();
+  watchPromise(p3, watcher);
+  baggage.init('watcher', watcher);
+  baggage.init('wh', watcherHandle);
+
   let heldPromise;
   let counter = 0;
 
@@ -165,6 +186,7 @@ export const buildRootObject = (_vatPowers, vatParameters, baggage) => {
     },
     getEternalPromiseInArray: () => [p1],
     getEternalPromise: () => p2,
+    getWatchedDecidedPromise: () => p3,
 
     makeLostKind: () => {
       makeKindHandle('unhandled');

packages/SwingSet/test/upgrade/vat-ulrik-2.js

@@ -61,6 +61,18 @@ export const buildRootObject = (_vatPowers, vatParameters, baggage) => {
     }
   }
 
+  const watcherHandle = baggage.get('wh');
+  const initWatcher = () => harden({ result: ['unresolved'] });
+  const watcherMethods = {
+    onFulfilled: ({ state }, data) =>
+      (state.result = harden(['fulfill', data])),
+    onRejected: ({ state }, reason) =>
+      (state.result = harden(['reject', reason])),
+    getResult: ({ state }) => state.result,
+  };
+  defineDurableKind(watcherHandle, initWatcher, watcherMethods);
+  const watcher = baggage.get('watcher');
+
   const root = Far('root', {
     getVersion: () => 'v2',
     getParameters: () => vatParameters,
@@ -89,6 +101,7 @@ export const buildRootObject = (_vatPowers, vatParameters, baggage) => {
       return E(handler).ping(`ping ${counter}`);
     },
     getNewDurandal: () => newDur,
+    getWatcherResult: () => watcher.getResult(),
   });
   // buildRootObject() is allowed to return a Promise, as long as it fulfills
   // promptly (we added this in #5246 to allow ZCF to use the async