fix(swingset): delete unused snapshots (#3505)

In addition to maintaining a mapping from vatID to snapshotID,
vatKeeper maintains a reverse mapping. When the reverse mapping
is empty, `snapStore.prepareToDelete()` queues the snapshot for deletion.

When the host calls `swingstore.commit()`, swingstore calls `snapStore.commitDeletes()`.

fixes #3374
refs #3431

* feat(swing-store-lmdb): host commit() deletes snapshots
  * feat(snapstore): prepare/commit deletes
  * feat(swing-store-lmdb): provide snapStore with kvStore, streamStore
  * chore: move snapStore to swing-store-lmdb package
  * refactor: factor out makeSnapStoreIO
 - vatKeeper.saveSnapshot() prepares deletes
   - removeFromSnapshot() returns consumers.length
* test: use tmp directories for snapStore in tests
* test(swingset): move c.shutdown to caller of runSteps:
  runSteps didn't create it; its caller still has a reference
* chore(cosmis-swingset): use consolidated snapStore API
* chore(solo): use consolidated snapStore API
* refactor(swingset): static type for kvStore in vatKeeper
 - getRequired() asserts that get() does not return undefined
 - fix addHelpers() return type by declaring arg type
 - where kvStore.get() is ensured by getKeys() or has(),
   mark the alternative with assert.fail().
* docs(swingset): mark DB keys excluded from consensus
* chore(snapstore): assert hash has no path separators
* test(snapstore): robust prepare / commit delete
* docs(swingstore): document snapStore commit constraint
* test: move xsnap/snapstore integration test to solo
   to avoid @agoric/xsnap in swing-store-lmdb devDependencies
* test(snapstore): re-save between prepare and commit delete
  plus one more /etc/passwd test

packages/SwingSet/src/kernel/state/kernelKeeper.js

@@ -53,12 +53,16 @@ const enableKernelGC = true;
 // v$NN.nextDeliveryNum = $NN
 // v$NN.t.endPosition = $NN
 // v$NN.vs.$key = string
-// v$NN.lastSnapshot = JSON({ snapshotID, startPos })
 // v$NN.meter = m$NN
+// exclude from consensus
+// v$NN.lastSnapshot = JSON({ snapshotID, startPos })
 
 // m$NN.remaining = $NN // remaining capacity (in computrons) or 'unlimited'
 // m$NN.threshold = $NN // notify when .remaining first drops below this
 
+// exclude from consensus
+// snapshot.$id = [vatID, ...]
+
 // d$NN.o.nextID = $NN
 // d$NN.c.$kernelSlot = $deviceSlot = o-$NN/d+$NN/d-$NN
 // d$NN.c.$deviceSlot = $kernelSlot = ko$NN/kd$NN
@@ -535,6 +539,11 @@ export default function makeKernelKeeper(
     const promisePrefix = `${vatID}.c.p`;
     const kernelPromisesToReject = [];
 
+    const old = vatKeeper.getLastSnapshot();
+    if (old) {
+      vatKeeper.removeFromSnapshot(old.snapshotID);
+    }
+
     // Note: ASCII order is "+,-./", and we rely upon this to split the
     // keyspace into the various o+NN/o-NN/etc spaces. If we were using a
     // more sophisticated database, we'd keep each section in a separate
@@ -570,7 +579,7 @@ export default function makeKernelKeeper(
       assert(k.startsWith(importPrefix), k);
       // abandoned imports: delete the clist entry as if the vat did a
       // drop+retire
-      const kref = kvStore.get(k);
+      const kref = kvStore.get(k) || assert.fail('getKeys ensures get');
       const vref = k.slice(`${vatID}.c.`.length);
       vatKeeper.deleteCListEntry(kref, vref);
       // that will also delete both db keys
@@ -803,7 +812,7 @@ export default function makeKernelKeeper(
     assert.typeof(name, 'string');
     const k = `vat.name.${name}`;
     assert(kvStore.has(k), X`vat name ${name} must exist, but doesn't`);
-    return kvStore.get(k);
+    return kvStore.get(k) || assert.fail('.has() ensures .get()');
   }
 
   function allocateUnusedVatID() {
@@ -822,7 +831,7 @@ export default function makeKernelKeeper(
       names.push(name);
       kvStore.set('vat.names', JSON.stringify(names));
     }
-    return kvStore.get(k);
+    return kvStore.get(k) || assert.fail('.has() ensures .get()');
   }
 
   function addDynamicVatID(vatID) {
@@ -837,7 +846,7 @@ export default function makeKernelKeeper(
     const result = [];
     for (const k of kvStore.getKeys('vat.name.', 'vat.name/')) {
       const name = k.slice(9);
-      const vatID = kvStore.get(k);
+      const vatID = kvStore.get(k) || assert.fail('getKeys ensures get');
       result.push([name, vatID]);
     }
     return result;
@@ -847,7 +856,7 @@ export default function makeKernelKeeper(
     const result = [];
     for (const k of kvStore.getKeys('device.name.', 'device.name/')) {
       const name = k.slice(12);
-      const deviceID = kvStore.get(k);
+      const deviceID = kvStore.get(k) || assert.fail('getKeys ensures get');
       result.push([name, deviceID]);
     }
     return result;

packages/SwingSet/src/kernel/state/storageWrapper.js

@@ -110,6 +110,9 @@ export function buildCrankBuffer(kvStore) {
   return harden({ crankBuffer, commitCrank, abortCrank });
 }
 
+/**
+ * @param {KVStore} kvStore
+ */
 export function addHelpers(kvStore) {
   // these functions are built on top of the DB interface
   insistStorageAPI(kvStore);
@@ -135,7 +138,7 @@ export function addHelpers(kvStore) {
 
   function* getPrefixedValues(prefix, start = 0) {
     for (const key of enumeratePrefixedKeys(prefix, start)) {
-      yield kvStore.get(key);
+      yield kvStore.get(key) || assert.fail('enumerate ensures get');
     }
   }
 

packages/SwingSet/src/kernel/state/vatKeeper.js

@@ -85,23 +85,35 @@ export function makeVatKeeper(
   insistVatID(vatID);
   const transcriptStream = `transcript-${vatID}`;
 
+  function getRequired(key) {
+    const value = kvStore.get(key);
+    assert(value !== undefined, `missing: ${key}`);
+    return value;
+  }
+
+  /**
+   * @param {SourceOfBundle} source
+   * @param {ManagerOptions} options
+   */
   function setSourceAndOptions(source, options) {
     // take care with API change
     assert(options.managerType, X`vat options missing managerType`);
     assert(source);
-    assert(source.bundle || source.bundleName);
+    assert('bundle' in source || 'bundleName' in source);
     assert.typeof(options, 'object');
     kvStore.set(`${vatID}.source`, JSON.stringify(source));
     kvStore.set(`${vatID}.options`, JSON.stringify(options));
   }
 
   function getSourceAndOptions() {
-    const source = JSON.parse(kvStore.get(`${vatID}.source`));
-    const options = JSON.parse(kvStore.get(`${vatID}.options`));
+    const source = JSON.parse(getRequired(`${vatID}.source`));
+    /** @type { ManagerOptions } */
+    const options = JSON.parse(kvStore.get(`${vatID}.options`) || '{}');
     return harden({ source, options });
   }
 
   function getOptions() {
+    /** @type { ManagerOptions } */
     const options = JSON.parse(kvStore.get(`${vatID}.options`) || '{}');
     return harden(options);
   }
@@ -241,7 +253,7 @@ export function makeVatKeeper(
         assert.fail(X`unknown vatSlot ${q(vatSlot)}`);
       }
     }
-    const kernelSlot = kvStore.get(vatKey);
+    const kernelSlot = getRequired(vatKey);
 
     if (setReachable) {
       if (allocatedByVat) {
@@ -396,7 +408,7 @@ export function makeVatKeeper(
    * @yields { TranscriptEntry } a stream of transcript entries
    */
   function* getTranscript(startPos = streamStore.STREAM_START) {
-    const endPos = JSON.parse(kvStore.get(`${vatID}.t.endPosition`));
+    const endPos = JSON.parse(getRequired(`${vatID}.t.endPosition`));
     for (const entry of streamStore.readStream(
       transcriptStream,
       startPos,
@@ -412,7 +424,7 @@ export function makeVatKeeper(
    * @param {Object} entry  The transcript entry to append.
    */
   function addToTranscript(entry) {
-    const oldPos = JSON.parse(kvStore.get(`${vatID}.t.endPosition`));
+    const oldPos = JSON.parse(getRequired(`${vatID}.t.endPosition`));
     const newPos = streamStore.writeStreamItem(
       transcriptStream,
       JSON.stringify(entry),
@@ -452,6 +464,48 @@ export function makeVatKeeper(
     return { totalEntries, snapshottedEntries };
   }
 
+  /**
+   * Add vatID to consumers of a snapshot.
+   *
+   * @param {string} snapshotID
+   */
+  function addToSnapshot(snapshotID) {
+    const key = `snapshot.${snapshotID}`;
+    const consumers = JSON.parse(kvStore.get(key) || '[]');
+    assert(Array.isArray(consumers));
+
+    // We can't completely rule out the possibility that
+    // a vat will use the same snapshot twice in a row.
+    //
+    // PERFORMANCE NOTE: we assume consumer lists are short;
+    // usually length 1. So O(n) search here is better
+    // than keeping the list sorted.
+    if (!consumers.includes(vatID)) {
+      consumers.push(vatID);
+      kvStore.set(key, JSON.stringify(consumers));
+      // console.log('addToSnapshot result:', { vatID, snapshotID, consumers });
+    }
+  }
+
+  /**
+   * Remove vatID from consumers of a snapshot.
+   *
+   * @param {string} snapshotID
+   */
+  function removeFromSnapshot(snapshotID) {
+    const key = `snapshot.${snapshotID}`;
+    const consumersJSON = kvStore.get(key);
+    assert(consumersJSON, X`cannot remove ${vatID}: ${key} key not defined`);
+    const consumers = JSON.parse(consumersJSON);
+    assert(Array.isArray(consumers));
+    const ix = consumers.indexOf(vatID);
+    assert(ix >= 0);
+    consumers.splice(ix, 1);
+    // console.log('removeFromSnapshot done:', { vatID, snapshotID, consumers });
+    kvStore.set(key, JSON.stringify(consumers));
+    return consumers.length;
+  }
+
   /**
    * Store a snapshot, if given a snapStore.
    *
@@ -464,11 +518,18 @@ export function makeVatKeeper(
     }
 
     const snapshotID = await manager.makeSnapshot(snapStore);
+    const old = getLastSnapshot();
+    if (old) {
+      if (removeFromSnapshot(old.snapshotID) === 0) {
+        snapStore.prepareToDelete(old.snapshotID);
+      }
+    }
     const endPosition = getTranscriptEndPosition();
     kvStore.set(
       `${vatID}.lastSnapshot`,
       JSON.stringify({ snapshotID, startPos: endPosition }),
     );
+    addToSnapshot(snapshotID);
     return true;
   }
 
@@ -481,7 +542,7 @@ export function makeVatKeeper(
     const objectCount = getCount(`${vatID}.o.nextID`, FIRST_OBJECT_ID);
     const promiseCount = getCount(`${vatID}.p.nextID`, FIRST_PROMISE_ID);
     const deviceCount = getCount(`${vatID}.d.nextID`, FIRST_DEVICE_ID);
-    const transcriptCount = JSON.parse(kvStore.get(`${vatID}.t.endPosition`))
+    const transcriptCount = JSON.parse(getRequired(`${vatID}.t.endPosition`))
       .itemCount;
 
     // TODO: Fix the downstream JSON.stringify to allow the counts to be BigInts
@@ -506,7 +567,8 @@ export function makeVatKeeper(
         const slot = k.slice(prefix.length);
         if (!slot.startsWith('k')) {
           const vatSlot = slot;
-          const kernelSlot = kvStore.get(k);
+          const kernelSlot =
+            kvStore.get(k) || assert.fail('getKeys ensures get');
           /** @type { [string, string, string] } */
           const item = [kernelSlot, vatID, vatSlot];
           res.push(item);
@@ -538,5 +600,6 @@ export function makeVatKeeper(
     dumpState,
     saveSnapshot,
     getLastSnapshot,
+    removeFromSnapshot,
   });
 }

packages/SwingSet/src/types.js

@@ -37,6 +37,7 @@
  *   compareSyscalls?: (originalSyscall: {}, newSyscall: {}) => Error | undefined,
  *   vatConsole: Console,
  *   liveSlotsConsole?: Console,
+ *   meterID?: string,
  * } & (HasBundle | HasSetup)} ManagerOptions
  */
 
@@ -131,7 +132,7 @@
  *              makeSnapshot?: (ss: SnapStore) => Promise<string>,
  *              shutdown: () => Promise<void>,
  *            } } VatManager
- * @typedef { ReturnType<typeof import('@agoric/xsnap').makeSnapStore> } SnapStore
+ * @typedef { ReturnType<typeof import('@agoric/swing-store-lmdb').makeSnapStore> } SnapStore
  * @typedef { () => Promise<void> } WaitUntilQuiescent
  */
 

packages/SwingSet/test/vat-warehouse/test-reload-snapshot.js

@@ -2,10 +2,9 @@
 // eslint-disable-next-line import/order
 import { test } from '../../tools/prepare-test-env-ava.js';
 
-import fs from 'fs';
 import path from 'path';
-import { tmpName } from 'tmp';
-import { makeSnapStore } from '@agoric/xsnap';
+import tmp from 'tmp';
+import { makeSnapStore, makeSnapStoreIO } from '@agoric/swing-store-lmdb';
 import { provideHostStorage } from '../../src/hostStorage.js';
 import { initializeSwingset, makeSwingsetController } from '../../src/index.js';
 import { capargs } from '../util.js';
@@ -20,22 +19,9 @@ test('vat reload from snapshot', async t => {
     },
   };
 
-  const snapstorePath = path.resolve(
-    __dirname,
-    './fixture-test-reload-snapshot/',
-  );
-  fs.mkdirSync(snapstorePath, { recursive: true });
-  t.teardown(() => fs.rmdirSync(snapstorePath, { recursive: true }));
+  const snapstorePath = tmp.dirSync({ unsafeCleanup: true }).name;
 
-  const snapStore = makeSnapStore(snapstorePath, {
-    tmpName,
-    existsSync: fs.existsSync,
-    createReadStream: fs.createReadStream,
-    createWriteStream: fs.createWriteStream,
-    rename: fs.promises.rename,
-    unlink: fs.promises.unlink,
-    resolve: path.resolve,
-  });
+  const snapStore = makeSnapStore(snapstorePath, makeSnapStoreIO());
   const hostStorage = { snapStore, ...provideHostStorage() };
 
   const argv = [];

packages/SwingSet/test/vat-warehouse/test-warehouse.js

@@ -3,12 +3,10 @@
 
 // eslint-disable-next-line import/order
 import { test } from '../../tools/prepare-test-env-ava';
-import path from 'path';
 import fs from 'fs';
-import { tmpName } from 'tmp';
-import { makeSnapStore } from '@agoric/xsnap';
+import tmp from 'tmp';
+import { initLMDBSwingStore } from '@agoric/swing-store-lmdb';
 import { loadBasedir, buildVatController } from '../../src/index.js';
-import { provideHostStorage } from '../../src/hostStorage.js';
 import { makeLRU } from '../../src/kernel/vatManager/vat-warehouse.js';
 
 async function makeController(managerType, runtimeOptions) {
@@ -74,8 +72,6 @@ const steps = [
 ];
 
 async function runSteps(c, t) {
-  t.teardown(c.shutdown);
-
   await c.run();
   for (const { vat, online } of steps) {
     t.log('sending to vat', vat);
@@ -101,29 +97,51 @@ test('4 vats in warehouse with 2 online', async t => {
   const c = await makeController('xs-worker', {
     warehousePolicy: { maxVatsOnline },
   });
+  t.teardown(c.shutdown);
+
   await runSteps(c, t);
 });
 
+function unusedSnapshotsOnDisk(kvStore, snapstorePath) {
+  const inUse = [];
+  for (const k of kvStore.getKeys(`snapshot.`, `snapshot/`)) {
+    const consumers = JSON.parse(kvStore.get(k));
+    if (consumers.length > 0) {
+      const id = k.slice(`snapshot.`.length);
+      inUse.push(id);
+    }
+  }
+  const onDisk = fs.readdirSync(snapstorePath);
+  const extra = [];
+  for (const snapshotPath of onDisk) {
+    const id = snapshotPath.slice(0, -'.gz'.length);
+    if (!inUse.includes(id)) {
+      extra.push(id);
+    }
+  }
+  return { inUse, onDisk, extra };
+}
+
 test('snapshot after deliveries', async t => {
-  const snapstorePath = path.resolve(__dirname, './fixture-test-warehouse/');
-  fs.mkdirSync(snapstorePath, { recursive: true });
-  t.teardown(() => fs.rmdirSync(snapstorePath, { recursive: true }));
+  const swingStorePath = tmp.dirSync({ unsafeCleanup: true }).name;
 
-  const snapStore = makeSnapStore(snapstorePath, {
-    tmpName,
-    existsSync: fs.existsSync,
-    createReadStream: fs.createReadStream,
-    createWriteStream: fs.createWriteStream,
-    rename: fs.promises.rename,
-    unlink: fs.promises.unlink,
-    resolve: path.resolve,
-  });
-  const hostStorage = { snapStore, ...provideHostStorage() };
+  const { kvStore, streamStore, commit } = initLMDBSwingStore(swingStorePath);
+  const hostStorage = { kvStore, streamStore };
   const c = await makeController('xs-worker', {
     hostStorage,
     warehousePolicy: { maxVatsOnline, snapshotInterval: 1 },
   });
+  t.teardown(c.shutdown);
+
   await runSteps(c, t);
+  commit();
+
+  const { inUse, onDisk, extra } = unusedSnapshotsOnDisk(
+    kvStore,
+    `${swingStorePath}/xs-snapshots`,
+  );
+  t.log({ inUse, onDisk, extra });
+  t.deepEqual(extra, [], `inUse: ${inUse}, onDisk: ${onDisk}`);
 });
 
 test('LRU eviction', t => {