fix: close short-lived connections first when pruning by tag value (#…

…1517)

When choosing connections to close, choose shorter lived connections over longer lived ones.

Closes #1509

doc/LIMITS.md

@@ -53,7 +53,7 @@ const node = await createLibp2pNode({
 
 ## Closing connections
 
-When choosing connections to close the connection manager sorts the list of connections by the value derived from the tags given to each peer. The values of all tags are summed and connections with lower valued peers are eligible for closing first.
+When choosing connections to close the connection manager sorts the list of connections by the value derived from the tags given to each peer. The values of all tags are summed and connections with lower valued peers are eligible for closing first. If there are tags with equal values, the shortest-lived connection will be closed first.
 
 ```js
 // tag a peer

src/connection-manager/index.ts

@@ -631,6 +631,18 @@ export class DefaultConnectionManager extends EventEmitter<ConnectionManagerEven
         return -1
       }
 
+      // if the peers have an equal tag value then we want to close short-lived connections first
+      const connectionALifespan = a.stat.timeline.open
+      const connectionBLifespan = b.stat.timeline.open
+
+      if (connectionALifespan < connectionBLifespan) {
+        return 1
+      }
+
+      if (connectionALifespan > connectionBLifespan) {
+        return -1
+      }
+
       return 0
     })
 

test/connection-manager/index.spec.ts

@@ -114,6 +114,61 @@ describe('Connection Manager', () => {
     expect(lowestSpy).to.have.property('callCount', 1)
   })
 
+  it('should close shortest-lived connection if the tag values are equal', async () => {
+    const max = 5
+    libp2p = await createNode({
+      config: createBaseOptions({
+        connectionManager: {
+          maxConnections: max,
+          minConnections: 2
+        }
+      }),
+      started: false
+    })
+
+    await libp2p.start()
+
+    const connectionManager = libp2p.connectionManager as DefaultConnectionManager
+    const connectionManagerMaybeDisconnectOneSpy = sinon.spy(connectionManager, '_pruneConnections')
+    const spies = new Map<string, sinon.SinonSpy<[], Promise<void>>>()
+
+    const createConnection = async (value: number, open: number = Date.now(), peerTag: string = 'test-tag') => {
+      // #TODO: Mock the connection timeline to simulate an older connection
+      const connection = mockConnection(mockMultiaddrConnection({ ...mockDuplex(), timeline: { open } }, await createEd25519PeerId()))
+      const spy = sinon.spy(connection, 'close')
+
+      // The lowest tag value will have the longest connection
+      spies.set(peerTag, spy)
+      await libp2p.peerStore.tagPeer(connection.remotePeer, peerTag, {
+        value
+      })
+
+      await connectionManager._onConnect(new CustomEvent('connection', { detail: connection }))
+    }
+
+    // Create one short of enough connections to iniate pruning
+    for (let i = 1; i < max; i++) {
+      const value = i * 10
+      await createConnection(value)
+    }
+
+    const value = 0 * 10
+    // Add a connection with the lowest tag value BUT the longest lived connection
+    await createConnection(value, 18000, 'longest')
+    // Add one more connection with the lowest tag value BUT the shortest-lived connection
+    await createConnection(value, Date.now(), 'shortest')
+
+    // get the lowest tagged value, but this would be also the longest lived connection
+    const longestLivedWithLowestTagSpy = spies.get('longest')
+
+    // Get lowest tagged connection but with a shorter-lived connection
+    const shortestLivedWithLowestTagSpy = spies.get('shortest')
+
+    expect(connectionManagerMaybeDisconnectOneSpy.callCount).to.equal(1)
+    expect(longestLivedWithLowestTagSpy).to.have.property('callCount', 0)
+    expect(shortestLivedWithLowestTagSpy).to.have.property('callCount', 1)
+  })
+
   it('should close connection when the maximum has been reached even without tags', async () => {
     const max = 5
     libp2p = await createNode({