fix: don't trip over your shoelaces

packages/swing-store/src/snapStore.js

@@ -106,7 +106,6 @@ export function makeSnapStore(
   noteExport = () => {},
   { keepSnapshots = false } = {},
 ) {
-  ensureTxn();
   db.exec(`
     CREATE TABLE IF NOT EXISTS snapshots (
       vatID TEXT,
@@ -304,7 +303,6 @@ export function makeSnapStore(
    * @returns {AsyncIterable<Uint8Array>}
    */
   function exportSnapshot(name, includeHistorical) {
-    ensureTxn();
     typeof name === 'string' || Fail`artifact name must be a string`;
     const parts = name.split('.');
     const [type, vatID, pos] = parts;
@@ -344,7 +342,6 @@ export function makeSnapStore(
     const cleanup = [];
     return aggregateTryFinally(
       async () => {
-        ensureTxn();
         const loadInfo = sqlLoadSnapshot.get(vatID);
         loadInfo || Fail`no snapshot available for vat ${q(vatID)}`;
         const { hash, compressedSnapshot } = loadInfo;
@@ -397,6 +394,7 @@ export function makeSnapStore(
     WHERE vatID = ?
     ORDER BY endPos
   `);
+  sqlGetSnapshotList.pluck(true);
 
   /**
    * Delete all snapshots for a given vat (for use when, e.g., a vat is terminated)
@@ -405,8 +403,12 @@ export function makeSnapStore(
    */
   function deleteVatSnapshots(vatID) {
     ensureTxn();
-    for (const rec of sqlGetSnapshotList.iterate(vatID)) {
-      const exportRec = snapshotRec(vatID, rec.endPos, undefined);
+    const deletions = [];
+    for (const endPos of sqlGetSnapshotList.iterate(vatID)) {
+      deletions.push(endPos);
+    }
+    for (const endPos of deletions) {
+      const exportRec = snapshotRec(vatID, endPos, undefined);
       noteExport(snapshotMetadataKey(exportRec), undefined);
     }
     noteExport(currentSnapshotMetadataKey({ vatID }), undefined);
@@ -428,7 +430,6 @@ export function makeSnapStore(
    * @returns {SnapshotInfo}
    */
   function getSnapshotInfo(vatID) {
-    ensureTxn();
     return /** @type {SnapshotInfo} */ (sqlGetSnapshotInfo.get(vatID));
   }
 
@@ -451,7 +452,6 @@ export function makeSnapStore(
    * @returns {boolean}
    */
   function hasHash(vatID, hash) {
-    ensureTxn();
     return !!sqlHasHash.get(vatID, hash);
   }
 
@@ -502,7 +502,6 @@ export function makeSnapStore(
    * @returns {Iterable<[key: string, value: string]>}
    */
   function* getExportRecords(includeHistorical = true) {
-    ensureTxn();
     for (const rec of sqlGetSnapshotMetadata.iterate(1)) {
       const exportRec = snapshotRec(rec.vatID, rec.endPos, rec.hash, 1);
       const exportKey = snapshotMetadataKey(rec);
@@ -518,7 +517,6 @@ export function makeSnapStore(
   }
 
   async function* getArtifactNames(includeHistorical) {
-    ensureTxn();
     for (const rec of sqlGetSnapshotMetadata.iterate(1)) {
       yield snapshotArtifactName(rec);
     }
@@ -594,13 +592,6 @@ export function makeSnapStore(
    * @param {boolean} [includeHistorical]
    */
   function dumpSnapshots(includeHistorical = true) {
-    // We do not do ensureTxn() here, because tests run this after a
-    // commit(), and we don't want to leave the DB inside a txn
-    // afterwards. I *think* that's ok, but it means we should not be
-    // using this from with normal code, especially if it might
-    // somehow run as the first DB thing in a controller.run(), in
-    // which case it would create an auto-committing txn, and that
-    // would be bad.
     const sql = includeHistorical
       ? sqlDumpAllSnapshots
       : sqlDumpCurrentSnapshots;

packages/swing-store/src/swingStore.js

@@ -174,7 +174,7 @@ export function makeSwingStoreExporter(dirPath, exportMode = 'current') {
 
   // Execute the data export in a (read) transaction, to ensure that we are
   // capturing the state of the database at a single point in time.
-  const sqlBeginTransaction = db.prepare('BEGIN IMMEDIATE TRANSACTION');
+  const sqlBeginTransaction = db.prepare('BEGIN TRANSACTION');
   sqlBeginTransaction.run();
 
   // ensureTxn can be a dummy, we just started one

packages/swing-store/src/transcriptStore.js

@@ -261,7 +261,12 @@ export function makeTranscriptStore(
    * @param {string} vatID
    */
   function deleteVatTranscripts(vatID) {
+    ensureTxn();
+    const deletions = [];
     for (const startPos of sqlGetVatSpans.iterate(vatID)) {
+      deletions.push(startPos);
+    }
+    for (const startPos of deletions) {
       const exportRec = spanRec(vatID, startPos);
       noteExport(historicSpanMetadataKey(exportRec), undefined);
     }

packages/swing-store/test/test-deletion.js

@@ -8,7 +8,7 @@ async function dosnap(filePath) {
   fs.writeFileSync(filePath, 'abc');
 }
 
-test.failing('delete snapshots with export callback', async t => {
+test('delete snapshots with export callback', async t => {
   const exportLog = [];
   const exportCallback = exports => {
     for (const [key, value] of exports) {
@@ -26,7 +26,7 @@ test.failing('delete snapshots with export callback', async t => {
   // nothing is written to exportCallback until endCrank() or commit()
   t.deepEqual(exportLog, []);
 
-  commit();
+  await commit();
 
   t.is(exportLog.length, 4);
   t.is(exportLog[0][0], 'snapshot.v1.10');
@@ -38,7 +38,7 @@ test.failing('delete snapshots with export callback', async t => {
   // in a previous version, deleteVatSnapshots caused overlapping SQL
   // queries, and failed
   snapStore.deleteVatSnapshots('v1');
-  commit();
+  await commit();
 
   t.deepEqual(exportLog, [
     ['snapshot.v1.10', null],
@@ -48,3 +48,47 @@ test.failing('delete snapshots with export callback', async t => {
   ]);
   exportLog.length = 0;
 });
+
+test('delete transcripts with export callback', async t => {
+  const exportLog = [];
+  const exportCallback = exports => {
+    for (const [key, value] of exports) {
+      exportLog.push([key, value]);
+    }
+  };
+  const store = initSwingStore(null, { exportCallback });
+  const { kernelStorage, hostStorage } = store;
+  const { transcriptStore } = kernelStorage;
+  const { commit } = hostStorage;
+
+  transcriptStore.initTranscript('v1');
+  transcriptStore.addItem('v1', 'aaa');
+  transcriptStore.addItem('v1', 'bbb');
+  transcriptStore.addItem('v1', 'ccc');
+  transcriptStore.rolloverSpan('v1');
+  transcriptStore.addItem('v1', 'ddd');
+  transcriptStore.addItem('v1', 'eee');
+  transcriptStore.addItem('v1', 'fff');
+  // nothing is written to exportCallback until endCrank() or commit()
+  t.deepEqual(exportLog, []);
+
+  await commit();
+
+  t.is(exportLog.length, 2);
+  t.is(exportLog[0][0], 'transcript.v1.0');
+  t.is(exportLog[1][0], 'transcript.v1.current');
+  exportLog.length = 0;
+
+  // in a previous version, deleteVatTranscripts caused overlapping SQL
+  // queries, and failed
+  transcriptStore.deleteVatTranscripts('v1');
+  await commit();
+
+  t.deepEqual(exportLog, [
+    ['transcript.v1.0', null],
+    ['transcript.v1.3', null],
+    ['transcript.v1.current', null],
+  ]);
+
+  exportLog.length = 0;
+});

packages/swing-store/test/test-exportImport.js

@@ -114,7 +114,8 @@ test('crank abort leaves no debris in export log', async t => {
         crankNum % 3 === 0,
       );
     }
-    ssOut.hostStorage.commit();
+    // eslint-disable-next-line no-await-in-loop
+    await ssOut.hostStorage.commit();
   }
 
   const exporter = makeSwingStoreExporter(dbDir, 'current');
@@ -204,7 +205,8 @@ async function testExportImport(
       // eslint-disable-next-line no-await-in-loop
       await fakeAVatSnapshot(vats[block % 2], kernelStorage);
     }
-    ssOut.hostStorage.commit();
+    // eslint-disable-next-line no-await-in-loop
+    await ssOut.hostStorage.commit();
   }
 
   const exporter = makeSwingStoreExporter(dbDir, exportMode);
@@ -304,7 +306,7 @@ async function testExportImport(
     throw e;
   }
   t.is(failureMode, 'none');
-  ssIn.hostStorage.commit();
+  await ssIn.hostStorage.commit();
   const dumpsShouldMatch =
     runMode !== 'debug' || (exportMode === 'debug' && importMode !== 'current');
   if (dumpsShouldMatch) {