Merge pull request #4979 from Agoric/4978-double-retire-import

fix liveslots double-retire bug by using a Set for importsToDrop/Retire

packages/SwingSet/src/liveslots/liveslots.js

@@ -204,7 +204,9 @@ function build(
     // by a remaining pillar, or the pillar which was dropped might be back
     // (e.g., given a new in-memory manifestation).
 
-    const [importsToDrop, importsToRetire, exportsToRetire] = [[], [], []];
+    const importsToDrop = new Set();
+    const importsToRetire = new Set();
+    const exportsToRetire = new Set();
     let doMore;
     do {
       doMore = false;
@@ -234,7 +236,7 @@ function build(
           // i.e., always drop before retiring
           // eslint-disable-next-line no-use-before-define
           if (!vrm.isVrefRecognizable(vref)) {
-            importsToRetire.push(vref);
+            importsToRetire.add(vref);
           }
         }
       }
@@ -250,42 +252,39 @@ function build(
           // eslint-disable-next-line no-use-before-define
           const [gcAgain, retirees] = vrm.possibleVirtualObjectDeath(baseRef);
           if (retirees) {
-            retirees.map(retiree => exportsToRetire.push(retiree));
+            retirees.map(retiree => exportsToRetire.add(retiree));
           }
           doMore = doMore || gcAgain;
         } else if (allocatedByVat) {
           // Remotable: send retireExport
           // for remotables, vref === baseRef
           if (kernelRecognizableRemotables.has(baseRef)) {
             kernelRecognizableRemotables.delete(baseRef);
-            exportsToRetire.push(baseRef);
+            exportsToRetire.add(baseRef);
           }
         } else {
           // Presence: send dropImport unless reachable by VOM
           // eslint-disable-next-line no-lonely-if, no-use-before-define
           if (!vrm.isPresenceReachable(baseRef)) {
-            importsToDrop.push(baseRef);
+            importsToDrop.add(baseRef);
             // eslint-disable-next-line no-use-before-define
             if (!vrm.isVrefRecognizable(baseRef)) {
               // for presences, baseRef === vref
-              importsToRetire.push(baseRef);
+              importsToRetire.add(baseRef);
             }
           }
         }
       }
     } while (possiblyDeadSet.size > 0 || possiblyRetiredSet.size > 0 || doMore);
 
-    if (importsToDrop.length) {
-      importsToDrop.sort();
-      syscall.dropImports(importsToDrop);
+    if (importsToDrop.size) {
+      syscall.dropImports(Array.from(importsToDrop).sort());
     }
-    if (importsToRetire.length) {
-      importsToRetire.sort();
-      syscall.retireImports(importsToRetire);
+    if (importsToRetire.size) {
+      syscall.retireImports(Array.from(importsToRetire).sort());
     }
-    if (exportsToRetire.length) {
-      exportsToRetire.sort();
-      syscall.retireExports(exportsToRetire);
+    if (exportsToRetire.size) {
+      syscall.retireExports(Array.from(exportsToRetire).sort());
     }
   }
 

packages/SwingSet/test/virtualObjects/double-retire-import/bootstrap-dri.js

@@ -0,0 +1,27 @@
+import { E } from '@endo/eventual-send';
+import { Far } from '@endo/marshal';
+
+export function buildRootObject() {
+  let vatAdmin;
+  let root;
+  const sensor0 = Far(`sensor-0`, {});
+  const sensor1 = Far(`sensor-1`, {});
+
+  return Far('root', {
+    async bootstrap(vats, devices) {
+      vatAdmin = await E(vats.vatAdmin).createVatAdminService(devices.vatAdmin);
+    },
+
+    async build() {
+      // build the target vat
+      const bcap = await E(vatAdmin).getNamedBundleCap('dri');
+      const options = {};
+      options.virtualObjectCacheSize = 0;
+      const res = await E(vatAdmin).createVat(bcap, options);
+      root = res.root;
+      await E(root).buildVir(sensor0, sensor1);
+      await E(root).ping();
+      return 'ok';
+    },
+  });
+}

packages/SwingSet/test/virtualObjects/double-retire-import/test-double-retire-import.js

@@ -0,0 +1,48 @@
+// eslint-disable-next-line import/order
+import { test } from '../../../tools/prepare-test-env-ava.js';
+
+// eslint-disable-next-line import/order
+import { provideHostStorage } from '../../../src/controller/hostStorage.js';
+import {
+  initializeSwingset,
+  makeSwingsetController,
+} from '../../../src/index.js';
+import { capargs } from '../../util.js';
+
+function bfile(name) {
+  return new URL(name, import.meta.url).pathname;
+}
+
+async function testUpgrade(t, defaultManagerType) {
+  const config = {
+    defaultManagerType,
+    bootstrap: 'bootstrap',
+    vats: {
+      bootstrap: { sourceSpec: bfile('bootstrap-dri.js') },
+    },
+    bundles: {
+      dri: { sourceSpec: bfile('vat-dri.js') },
+    },
+  };
+
+  const hostStorage = provideHostStorage();
+  await initializeSwingset(config, [], hostStorage);
+  const c = await makeSwingsetController(hostStorage);
+  c.pinVatRoot('bootstrap');
+  await c.run();
+
+  const kpid = c.queueToVatRoot('bootstrap', 'build', capargs([]));
+  await c.run();
+  t.is(c.kpStatus(kpid), 'fulfilled');
+  // the bug manifests as a fatal vat error (illegal syscall), causing
+  // vat-dri to be killed before it can respond to the ping, causing
+  // bootstrap~.build() to reject.
+}
+
+test('double retire import - local', async t => {
+  return testUpgrade(t, 'local');
+});
+
+test('double retire import - xsnap', async t => {
+  return testUpgrade(t, 'xs-worker');
+});

packages/SwingSet/test/virtualObjects/double-retire-import/vat-dri.js

@@ -0,0 +1,71 @@
+/* global VatData */
+import { Far } from '@endo/marshal';
+// import { defineKind } from '@agoric/vat-data';
+const { defineKind } = VatData;
+
+function initialize(arg) {
+  return harden({ arg });
+}
+function actualize(state) {
+  return {
+    get() {
+      return state.arg;
+    },
+    set(arg) {
+      state.arg = harden(arg);
+    },
+  };
+}
+const makeVir = defineKind('virtual', initialize, actualize);
+
+function buildVirtuals(sensor0, sensor1) {
+  // eslint-disable-next-line no-unused-vars
+  const vir0 = makeVir(sensor0); // gets o+10/1, data holds o-50
+  // If I only make one vir, its Representative doesn't get dropped, I
+  // don't know why. If I make two, the first gets dropped but not the
+  // second.
+
+  // eslint-disable-next-line no-unused-vars
+  const vir1 = makeVir(sensor1); // o+10/2, data holds o-51
+
+  // We drop everything at the same time. The finalizers run on the
+  // vir0 Representative and both Presences (but not vir1,
+  // why??). `possiblyDeadSet` has [o+10/1, o-50, o-51]. The o+10/1
+  // sorts earlier, so we entry the 'for (const baseRef of
+  // deadBaseRefs)' loop with [o+10/1, o-50, o-51]:
+  // * o+10/1 : vrm.possibleVirtualObjectDeath() finds that it is
+  //            unreferenced by exports or vdata, so it is
+  //            deleted. While deleting o+10/1, the o-50 sensor0
+  //            object is decreffed, adding it back to
+  //            `possiblyDeadSet`. The 'gcAgain' return value is true,
+  //            because vdata was deleted (which might have released
+  //            a Remotable or Promise), scheduling another loop pass.
+  // * o-50 : vrm.isPresenceReachable reports no refcount, so o-50 is
+  //          added to importsToDrop. vrm.isVrefRecognizable reports
+  //          no refcount, so o-50 is also added to importsToRetire
+  // * o-51 : vrm.isPresenceReachable reports a non-zero refcount
+  //          because (for some reason) o+10/2 is still alive, so o-51
+  //          is not added to importsToDrop or importsToRetire
+
+  // Now the gcAgain/doMore flag causes the loop to be repeated,
+  // starting with a gcAndFinalize() (which doesn't yield anything
+  // here). However o-50 is in possiblyDeadSet, and makes it into
+  // deadSet, and is examined by vrm.isPresenceReachable, which
+  // reports no refcount, so it is added *again* into importsToDrop
+  // and importsToRetire. Because these are Arrays and not Sets, we
+  // attempt `syscall.dropImports([o-50,o-50])`, which happens to
+  // succeed, and then `syscall.retireImports([o-50,o-50])`, which
+  // fails when the second copy tries to look up a c-list entry that
+  // was deleted by the first copy.
+}
+
+export function buildRootObject() {
+  return Far('root', {
+    ping() {
+      return 0;
+    },
+    buildVir(sensor0, sensor1) {
+      return buildVirtuals(sensor0, sensor1);
+    },
+  });
+}