fix: TimeCache handle key collision to prevent leak (#358)

* TimeCache handle key collision to prevent leak

* Move unit tests into time-cache

Author: dapplion

src/index.ts

@@ -1140,7 +1140,12 @@ export class GossipSub extends EventEmitter<GossipsubEvents> implements PubSub<G
     const messageId = { msgId, msgIdStr }
 
     // Add the message to the duplicate caches
-    if (fastMsgIdStr !== undefined) this.fastMsgIdCache?.put(fastMsgIdStr, msgIdStr)
+    if (fastMsgIdStr !== undefined && this.fastMsgIdCache) {
+      const collision = this.fastMsgIdCache.put(fastMsgIdStr, msgIdStr)
+      if (collision) {
+        this.metrics?.fastMsgIdCacheCollision.inc()
+      }
+    }
 
     if (this.seenCache.has(msgIdStr)) {
       return { code: MessageStatus.duplicate, msgIdStr }

src/metrics.ts

@@ -498,6 +498,11 @@ export function getMetrics(
       help: 'Current mcache msg count not validated'
     }),
 
+    fastMsgIdCacheCollision: register.gauge({
+      name: 'gossipsub_fastmsgid_cache_collision_total',
+      help: 'Total count of key collisions on fastmsgid cache put'
+    }),
+
     topicStrToLabel: topicStrToLabel,
 
     toTopic(topicStr: TopicStr): TopicLabel {

src/utils/time-cache.ts

@@ -27,8 +27,19 @@ export class SimpleTimeCache<T> {
     return this.entries.size
   }
 
-  put(key: string | number, value: T): void {
+  /** Returns true if there was a key collision and the entry is dropped */
+  put(key: string | number, value: T): boolean {
+    if (this.entries.has(key)) {
+      // Key collisions break insertion order in the entries cache, which break prune logic.
+      // prune relies on each iterated entry to have strictly ascending validUntilMs, else it
+      // won't prune expired entries and SimpleTimeCache will grow unexpectedly.
+      // As of Oct 2022 NodeJS v16, inserting the same key twice with different value does not
+      // change the key position in the iterator stream. A unit test asserts this behaviour.
+      return true
+    }
+
     this.entries.set(key, { value, validUntilMs: Date.now() + this.validityMs })
+    return false
   }
 
   prune(): void {
@@ -38,7 +49,8 @@ export class SimpleTimeCache<T> {
       if (v.validUntilMs < now) {
         this.entries.delete(k)
       } else {
-        // sort by insertion order
+        // Entries are inserted with strictly ascending validUntilMs.
+        // Stop early to save iterations
         break
       }
     }

test/time-cache.spec.ts

@@ -40,4 +40,28 @@ describe('SimpleTimeCache', () => {
     expect(timeCache.has('bFirst')).to.be.false()
     expect(timeCache.has('cFirst')).to.be.false()
   })
+
+  it('Map insertion order', () => {
+    const key1 = 'key1'
+    const key2 = 'key2'
+    const key3 = 'key3'
+
+    const map = new Map<string, number>()
+    map.set(key1, Date.now())
+    map.set(key2, Date.now())
+    map.set(key3, Date.now())
+
+    expect(Array.from(map.keys())).deep.equals([key1, key2, key3], 'Map iterator order')
+
+    // Does not change key position
+    map.set(key2, Date.now())
+
+    expect(Array.from(map.keys())).deep.equals([key1, key2, key3], 'Map iterator order after re-set')
+
+    // Changes key position
+    map.delete(key2)
+    map.set(key2, Date.now())
+
+    expect(Array.from(map.keys())).deep.equals([key1, key3, key2], 'Map iterator order after delete set')
+  })
 })