fix(liveslots): collection deletion didn't free key objects

Fix collection deletion. Previously, objects (passables/vrefs) used as
collection keys were not de-referenced when the collection was
dropped/deleted or unconditionally cleared (`collection.clear()`
without keyPatt or valuePatt). Strong collections would leak a
reachable-refcount to the key object, and weak collections would leak
a recognizable-refcount, leading to the object being kept alive (or
kept unretired) forever. In addition, each entry would leak a vatstore
key until the collection was finally deleted.

The code in `clearInternalFull()` confused dbKeys, encodedKeys, and
vrefs, and gave the wrong kind of key to `isEncodedRemotable()`, so
the answer was always "no".

fixes #8756

packages/swingset-liveslots/src/collectionManager.js

@@ -293,6 +293,17 @@ export function makeCollectionManager(
       return `${dbKeyPrefix}${dbEntryKey}`;
     }
 
+    // a "vref" is a string like "o-4"
+    // an "EncodedKey" is the output of encode-passable:
+    // * strings become `s${string}`, like "foo" -> "sfoo"
+    // * positive BigInts become `p${len}:${digits}`, 47n -> "p2:47"
+    // * refs are assigned an "ordinal" and encode as "r0000000001:o-4"
+    // a "DBKey" is used to index the vatstore
+    // * DBKeys for collection entries join a collection prefix to an EncodedKey
+    // * "foo" -> "vc.5.sfoo"
+    // * 47n -> "vc.5.p2:47"
+    // * vref(o-4) -> "vc.5.r0000000001:o-4"
+
     const encodeRemotable = remotable => {
       // eslint-disable-next-line no-use-before-define
       const ordinal = getOrdinal(remotable);
@@ -308,10 +319,11 @@ export function makeCollectionManager(
     // the resulting function will encode only `Key` arguments.
     const encodeKey = makeEncodePassable({ encodeRemotable });
 
-    const vrefFromDBKey = dbKey => dbKey.substring(BIGINT_TAG_LEN + 2);
+    const vrefFromEncodedKey = encodedKey =>
+      encodedKey.substring(1 + BIGINT_TAG_LEN + 1);
 
     const decodeRemotable = encodedKey =>
-      convertSlotToVal(vrefFromDBKey(encodedKey));
+      convertSlotToVal(vrefFromEncodedKey(encodedKey));
 
     // `makeDecodePassable` has three named options:
     // `decodeRemotable`, `decodeError`, and `decodePromise`.
@@ -347,10 +359,15 @@ export function makeCollectionManager(
     }
 
     function dbKeyToKey(dbKey) {
+      // convert e.g. vc.5.r0000000001:o+v10/1 to r0000000001:o+v10/1
       const dbEntryKey = dbKey.substring(dbKeyPrefix.length);
       return decodeKey(dbEntryKey);
     }
 
+    function dbKeyToEncodedKey(dbKey) {
+      return dbKey.substring(dbKeyPrefix.length);
+    }
+
     function has(key) {
       const { keyShape } = getSchema();
       if (!matches(key, keyShape)) {
@@ -563,8 +580,9 @@ export function makeCollectionManager(
         doMoreGC =
           value.slots.map(vrm.removeReachableVref).some(b => b) || doMoreGC;
         syscall.vatstoreDelete(dbKey);
-        if (isEncodedRemotable(dbKey)) {
-          const keyVref = vrefFromDBKey(dbKey);
+        const encodedKey = dbKeyToEncodedKey(dbKey);
+        if (isEncodedRemotable(encodedKey)) {
+          const keyVref = vrefFromEncodedKey(encodedKey);
           if (hasWeakKeys) {
             vrm.removeRecognizableVref(keyVref, `${collectionID}`, true);
           } else {

packages/swingset-liveslots/test/storeGC/test-clear-collection.js

@@ -0,0 +1,192 @@
+// @ts-nocheck
+import test from 'ava';
+
+import { Far } from '@endo/marshal';
+import { kser, kslot } from '@agoric/kmarshal';
+import { makeLiveSlots } from '../../src/liveslots.js';
+import { buildSyscall } from '../liveslots-helpers.js';
+import { makeMessage, makeStartVat, makeBringOutYourDead } from '../util.js';
+import { makeMockGC } from '../mock-gc.js';
+
+// When a virtual collection is the only reference to a virtual
+// object, collection.clear() should let them be deleted. Bug #8756
+// caused them to be retained by mistake.
+
+test('collection.clear() deletes contents', async t => {
+  const { syscall, log } = buildSyscall();
+  const gcTools = makeMockGC();
+  const COUNT = 5;
+
+  function build(vatPowers) {
+    const { defineKind, makeScalarBigSetStore } = vatPowers.VatData;
+    const make = defineKind('target', () => ({}), {});
+    const holder = makeScalarBigSetStore('holder');
+    const root = Far('root', {
+      create() {
+        for (let i = 0; i < COUNT; i += 1) {
+          // vrefs are all `o+v10/${N}`, N=1..10
+          const target = make();
+          holder.add(target);
+          // we immediately delete the presence, but the finalizers
+          // won't run until gcTools.flushAllFRs()
+          gcTools.kill(target);
+        }
+      },
+      clear() {
+        holder.clear();
+      },
+    });
+    return root;
+  }
+
+  const ls = makeLiveSlots(syscall, 'vatA', {}, {}, gcTools, undefined, () => ({
+    buildRootObject: build,
+  }));
+  const { dispatch } = ls;
+  await dispatch(makeStartVat(kser()));
+  log.length = 0;
+
+  const rootA = 'o+0';
+
+  await dispatch(makeMessage(rootA, 'create', []));
+  log.length = 0;
+
+  // Collect the representatives, leaving only the virtual-data
+  // pillar. This BOYD finds non-zero virtual-data refcounts for all
+  // five VOs, so they are not deleted.
+  gcTools.flushAllFRs();
+  const boyd1 = await dispatch(makeBringOutYourDead());
+  t.is(boyd1.possiblyDeadSet, 0);
+  t.is(boyd1.possiblyRetiredSet, 0);
+  log.length = 0;
+
+  // clearing the collections should delete both the data key and the
+  // ordinal key for each entry, but it won't delete the values, that
+  // is deferred until BOYD
+  await dispatch(makeMessage(rootA, 'clear', []));
+
+  // The .clear() will do an initial get(vc.5.) to start the iterator,
+  // then for each collection entry it will getNextKey()/get(), then a
+  // delete() of the data row, a get()+delete() of the refcount, and a
+  // delete() of the ordinal row (vc.5|o+v10/1). Then we'll see an
+  // extra getNextKey() (which stops the iteration). At the end of the
+  // loop, it will write out the new "|entryCount" value with one last
+  // set(). Total: 1*COUNT+1 getNextKey, 2*COUNT get, 3*COUNT delete,
+  // plus 1 set.
+  t.is(log.filter(l => l.type === 'vatstoreGetNextKey').length, 1 * COUNT + 1);
+  t.is(log.filter(l => l.type === 'vatstoreGet').length, 2 * COUNT + 1);
+  t.is(log.filter(l => l.type === 'vatstoreDelete').length, 3 * COUNT);
+  t.is(log.filter(l => l.type === 'vatstoreSet').length, 1);
+  t.is(log.length, 6 * COUNT + 3);
+  log.length = 0;
+
+  // this ought to delete the VOs. bug #8756
+  await dispatch(makeBringOutYourDead());
+
+  // We expect to see the objects get deleted. The scanForDeadObjects
+  // does a get(rc)/get(es) on each to decide the object is really
+  // dead. Then deleting the objects does a get(vom) to see what
+  // references need to be decremented, then a redundant
+  // get(rc)/get(es), and a delete(rc)/delete(es) (redundant, in this
+  // case, since the RAM pillar was the only one left, and rc/es keys
+  // didn't exist). A delete(vom) is emitted for each, but held in the
+  // data cache until end of crank. Then the retirement check does a
+  // getNextKey(ir) scan.
+  //
+  // Total: 5*COUNT get, 1*COUNT getNextKey, 3*COUNT delete
+  t.is(log.filter(l => l.type === 'vatstoreGet').length, 5 * COUNT);
+  t.is(log.filter(l => l.type === 'vatstoreSet').length, 0 * COUNT);
+  t.is(log.filter(l => l.type === 'vatstoreGetNextKey').length, 1 * COUNT);
+  t.is(log.filter(l => l.type === 'vatstoreDelete').length, 3 * COUNT);
+  t.is(log.length, 9 * COUNT);
+});
+
+test('weak collection deletion will retire contents', async t => {
+  const { syscall, log } = buildSyscall();
+  const gcTools = makeMockGC();
+  const COUNT = 5;
+  const allVrefs = [];
+  const allKslots = [];
+  for (let i = 0; i < COUNT; i += 1) {
+    const vref = `o-${i + 1}`;
+    allVrefs.push(vref);
+    allKslots.push(kslot(vref, 'imported'));
+  }
+
+  // Import a bunch of Presences and hold them in a weakset. Drop the
+  // imports, but retain recognition, until we drop the weakset, which
+  // should delete the collection and retire the keys.
+  function build(vatPowers) {
+    const { makeScalarBigWeakSetStore } = vatPowers.VatData;
+    const recognizer = makeScalarBigWeakSetStore('recognizer');
+    const root = Far('root', {
+      create(presences) {
+        for (const p of presences) {
+          recognizer.add(p);
+          // we immediately delete the presence, but the finalizers
+          // won't run until gcTools.flushAllFRs()
+          gcTools.kill(p);
+        }
+      },
+      drop() {
+        gcTools.kill(recognizer);
+      },
+    });
+    return root;
+  }
+
+  const ls = makeLiveSlots(syscall, 'vatA', {}, {}, gcTools, undefined, () => ({
+    buildRootObject: build,
+  }));
+  const { dispatch } = ls;
+  await dispatch(makeStartVat(kser()));
+  log.length = 0;
+
+  const rootA = 'o+0';
+
+  await dispatch(makeMessage(rootA, 'create', [allKslots]));
+  log.length = 0;
+
+  // Collect the representatives, leaving only the virtual-data
+  // pillar. This BOYD finds non-zero virtual-data refcounts for all
+  // five VOs, so they are not deleted.
+  gcTools.flushAllFRs();
+  await dispatch(makeBringOutYourDead());
+  t.deepEqual(log.at(-1), { type: 'dropImports', slots: allVrefs });
+  log.length = 0;
+
+  // dropping the collection makes it UNREACHABLE but it won't be
+  // COLLECTED until BOYD
+  await dispatch(makeMessage(rootA, 'drop', []));
+  t.is(log.length, 0);
+
+  // this will delete the collection and should retire the imports
+  gcTools.flushAllFRs();
+  await dispatch(makeBringOutYourDead());
+
+  // Collection deletion starts with scanForDeadObjects doing a
+  // get(rc)+get(es) on the collection vref, to decide if it's really
+  // dead. Then, the deletion phase does another get(rc)+get(es), then
+  // iterates over rows. This starts with the weird get(vc.5.), then
+  // for each entry, we see getNextKey(), get+delete of the data row,
+  // delete(ir), and delete(reverse-mapping). One more getNextKey()
+  // ends the iteration. Then we get(schemata) to see if the
+  // keyshape/valueshape has references that need deletion. Then we
+  // find and delete metadata:
+
+  // * getNextKey, delete(|nextOrdinal)
+  // * getNextKey (deletion of |schemata is deferred)
+  // * getNextKey ends the iteration
+  // Then delete(rc)+delete(es) for the collection vref, even though
+  // they didn't exist. Then getNextKey to scan for ir records
+  // (none). Then finally delete(|schemata)
+  //
+  // Total: 5+1*COUNT+1 get, 1*COUNT+1+3+1 getNextKey, 3*COUNT+4 delete
+
+  t.is(log.filter(l => l.type === 'vatstoreGetNextKey').length, COUNT + 5);
+  t.is(log.filter(l => l.type === 'vatstoreGet').length, COUNT + 6);
+  t.is(log.filter(l => l.type === 'vatstoreDelete').length, 3 * COUNT + 4);
+  t.is(log.filter(l => l.type === 'vatstoreSet').length, 0);
+  t.is(log.length, 5 * COUNT + 15);
+  log.length = 0;
+});