From 9075f983d8fa9a13c18a63451a78bed5912e78d0 Mon Sep 17 00:00:00 2001
From: Andre Staltz <andre@staltz.com>
Date: Tue, 20 Feb 2024 16:51:06 +0200
Subject: [PATCH] dont erase a trail msg redundantly

---
 lib/index.js            | 62 ++++++++++++++++++++++++-----------------
 test/feed-holes.test.js | 12 ++++++++
 2 files changed, 49 insertions(+), 25 deletions(-)

diff --git a/lib/index.js b/lib/index.js
index 036be1b..bde88da 100644
--- a/lib/index.js
+++ b/lib/index.js
@@ -55,12 +55,12 @@ function initGC(peer, config) {
     const done = multicb({ pluck: 1 })
 
     /**
-     * @param {string} explanation
+     * @param {string} errExplanation
      */
-    function makeRecCB(explanation) {
+    function makeRecCB(errExplanation) {
       const cb = done()
       return (/**@type {Error=}*/ err) => {
-        if (err) debug('%s: %s', explanation, flattenCauseChain(err))
+        if (err) debug('%s: %s', errExplanation, flattenCauseChain(err))
         cb()
       }
     }
@@ -70,29 +70,41 @@ function initGC(peer, config) {
       if (!rec.msg) continue
       const { id: msgID, msg } = rec
       const [purpose, details] = peer.goals.getMsgPurpose(msgID, msg)
-      if (purpose === 'goal') continue // don't cleanup
-      if (purpose === 'none') {
-        const recCB = makeRecCB('Failed to delete msg when cleaning up')
-        debug('Deleting msg %s with purpose=none', msgID)
-        peer.db.del(msgID, recCB)
-        waiting = true
-      } else if (purpose === 'ghost') {
-        const { tangleID, span } = details
-        const recCB = makeRecCB('Failed to delete ghost msg when cleaning up')
-        // TODO: Could one msg be a ghostable in MANY tangles? Or just one?
-        debug('Deleting and ghosting msg %s with purpose=ghost', msgID)
-        peer.db.ghosts.add({ tangleID, msgID, span }, (err) => {
-          if (err) return recCB(err)
+      switch (purpose) {
+        case 'goal': {
+          continue // don't cleanup
+        }
+        case 'none': {
+          const recCB = makeRecCB('Failed to delete msg when cleaning up')
+          debug('Deleting msg %s with purpose=none', msgID)
           peer.db.del(msgID, recCB)
-        })
-        waiting = true
-      } else if (purpose === 'trail') {
-        const recCB = makeRecCB('Failed to erase trail msg when cleaning up')
-        debug('Erasing msg %s with purpose=trail', msgID)
-        peer.db.erase(msgID, recCB)
-        waiting = true
-      } else {
-        cb(new Error('Unreachable'))
+          waiting = true
+          continue
+        }
+        case 'ghost': {
+          const { tangleID, span } = details
+          const recCB = makeRecCB('Failed to delete ghost msg when cleaning up')
+          // TODO: Could one msg be a ghostable in MANY tangles? Or just one?
+          debug('Deleting and ghosting msg %s with purpose=ghost', msgID)
+          peer.db.ghosts.add({ tangleID, msgID, span }, (err) => {
+            if (err) return recCB(err)
+            peer.db.del(msgID, recCB)
+          })
+          waiting = true
+          continue
+        }
+        case 'trail': {
+          if (!msg.data) continue // it's already erased
+          const recCB = makeRecCB('Failed to erase trail msg when cleaning up')
+          debug('Erasing msg %s with purpose=trail', msgID)
+          peer.db.erase(msgID, recCB)
+          waiting = true
+          continue
+        }
+        default: {
+          cb(new Error('Unreachable'))
+          return
+        }
       }
     }
 
diff --git a/test/feed-holes.test.js b/test/feed-holes.test.js
index 863c512..3eba8ed 100644
--- a/test/feed-holes.test.js
+++ b/test/feed-holes.test.js
@@ -52,7 +52,16 @@ test('Feed holes', async (t) => {
   alice.goals.set(postFeedID, 'newest-4')
   assert('alice set a goal for newest-4 of post feed')
 
+  // Mock db.erase so we can inspect how many times it was called
+  const prevErase = alice.db.erase
+  const calledErase = []
+  alice.db.erase = (msgID, cb) => {
+    calledErase.push(msgID)
+    prevErase(msgID, cb)
+  }
+
   await p(alice.gc.forceImmediately)()
+  assert.deepEqual(calledErase, [posts[2]], 'erased A2')
 
   assert.deepEqual(
     getTexts([...alice.db.msgs()]),
@@ -60,5 +69,8 @@ test('Feed holes', async (t) => {
     'alice has only the end of the feed'
   )
 
+  await p(alice.gc.forceImmediately)()
+  assert.deepEqual(calledErase, [posts[2]], 'no double erasing')
+
   await p(alice.close)(true)
 })