From 1daec3e890515175743474bd009fb62094065986 Mon Sep 17 00:00:00 2001
From: Elle Mouton <elle.mouton@gmail.com>
Date: Mon, 27 Nov 2023 12:17:55 +0200
Subject: [PATCH] wtclient: use the new filter options to fix the demo'd bug

In this commit, we use the newly added session listing options to ensure
that we only see a session as exhausted if it does not have any un-acked
updates on disk. This fixes the bug previously demonstrated.
---
 watchtower/wtclient/client.go      | 10 +++++---
 watchtower/wtclient/client_test.go | 39 ++++++++++++++++++------------
 2 files changed, 30 insertions(+), 19 deletions(-)

diff --git a/watchtower/wtclient/client.go b/watchtower/wtclient/client.go
index 3383de6ab..fa76ebd58 100644
--- a/watchtower/wtclient/client.go
+++ b/watchtower/wtclient/client.go
@@ -66,10 +66,14 @@ func (c *client) genSessionFilter(
 }
 
 // ExhaustedSessionFilter constructs a wtdb.ClientSessionFilterFn filter
-// function that will filter out any sessions that have been exhausted.
+// function that will filter out any sessions that have been exhausted. A
+// session is considered exhausted only if it has no un-acked updates and the
+// sequence number of the session is equal to the max updates of the session
+// policy.
 func ExhaustedSessionFilter() wtdb.ClientSessWithNumCommittedUpdatesFilterFn {
-	return func(session *wtdb.ClientSession, _ uint16) bool {
-		return session.SeqNum < session.Policy.MaxUpdates
+	return func(session *wtdb.ClientSession, numUnAcked uint16) bool {
+		return session.SeqNum < session.Policy.MaxUpdates ||
+			numUnAcked > 0
 	}
 }
 
diff --git a/watchtower/wtclient/client_test.go b/watchtower/wtclient/client_test.go
index c1b40be40..c2380dbff 100644
--- a/watchtower/wtclient/client_test.go
+++ b/watchtower/wtclient/client_test.go
@@ -2418,13 +2418,13 @@ var clientTests = []clientTest{
 		},
 	},
 	{
-		// Demonstrate that a client is un-able to remove a tower if
-		// there are persisted un-acked updates in a session that is
-		// not loaded as a candidate session on start-up.
-		//
-		// NOTE: This is a bug and will be fixed in a followup commit.
-		name: "can't remove tower with un-acked update in " +
-			"non-candidate session",
+		// Previously we would not load a session into memory if its
+		// seq num was equal to it's max-updates. This meant that we
+		// would then not properly handle any committed updates for the
+		// session meaning that we would then not remove the tower if
+		// needed. This test demonstrates that this has been fixed.
+		name: "can remove tower with an un-acked update in " +
+			"an exhausted session after a restart",
 		cfg: harnessCfg{
 			localBalance:  localBalance,
 			remoteBalance: remoteBalance,
@@ -2462,8 +2462,7 @@ var clientTests = []clientTest{
 			// update which will cause a CommittedUpdate to be
 			// persisted which will also mean that the SeqNum of the
 			// session is now equal to MaxUpdates of the session
-			// policy. This means that on start-up of the client,
-			// the session will not be loaded as a candidate.
+			// policy.
 			h.backupStates(chanID, numUpdates-1, numUpdates, nil)
 
 			tower, err := h.clientDB.LoadTower(
@@ -2493,19 +2492,27 @@ var clientTests = []clientTest{
 			require.NoError(h.t, err)
 
 			// Now restart the client. On restart, the previous
-			// session will no longer be loaded as a candidate since
-			// it is now seen as exhausted.
+			// session should still be loaded even though it is
+			// exhausted since it has an un-acked update.
 			require.NoError(h.t, h.clientMgr.Stop())
 			h.startClient()
 
-			// Attempt to remove the tower. This will fail due to
-			// there still being un-acked updates for the tower
-			// since these are only (currently) properly handled for
-			// sessions loaded as candidate sessions on start-up.
+			// Now remove the tower.
 			err = h.clientMgr.RemoveTower(
 				h.server.addr.IdentityKey, nil,
 			)
-			require.ErrorIs(h.t, err, wtdb.ErrTowerUnackedUpdates)
+			require.NoError(h.t, err)
+
+			// Add a new tower.
+			server2 := newServerHarness(
+				h.t, h.net, towerAddr2Str, nil,
+			)
+			server2.start()
+			h.addTower(server2.addr)
+
+			// Now we assert that the backups are backed up to the
+			// new tower.
+			server2.waitForUpdates(hints[numUpdates-1:], waitTime)
 		},
 	},
 	{