wtclient: ensure correct disk mode for overflow queue

[ellemouton]

Before this commit, in the watchtower client's DiskOverflowQueue, there could be a situation if no consumer was reading from the queue that could lead to an in-memory build up of backup tasks instead of the expected disk overflow. This commit fixes this by ensuring that if a task is read from disk, then the mode is set to disk mode to ensure that any new items are persisted to disk in this scenario.

More detailed explanation

This order of events could trigger the flake in question:

1. We start out with toDisk= false (ie, we are in memory-mode)
2. Enough items get pushed to the queue to fill up memQueue to its capacity.
3. Add 2 more items in quick succession to the queue.
4. For the first item, drainInputList will try push it to inputChan but because memQueue is full, it will return success=false and the mode will be switched to disk-mode (toDisk=true).
5. drainInputList will then instead write the item to disk & push a notification on the newDiskItemSignal channel.
6. feedMemQueue then responds to this signal and calls feedFromDisk which will pop the first item from disk and will block on pushing the item to the currently full memQueue.
   ⚠️ Here is where things get race-y:
7. drainInputList starts handling the second item - since the mode is still "disk-mode" it preps writing the item to disk (but doesnt yet!!)
8. One item gets popped from memQueue meaning that feedMemQueue manages to push the first item to memQueue and then goes onto the next iteration of the feedFromDisk loop - Here, it will try read an item from disk but will see that there are no more items items & so will set the mode to memory-mode.
9. At this point, drainInputList actually writes the second item to disk and sends a new newDiskItemSignal.
10. feedMemQueue will then respond to this signal by reading the new item from disk and blocking on adding it to the memQueue.
11. Now, any new items that drainInputList gets - it will try to push those to inputChan because the mode is "memory-mode" BUT feedMemQueue wont be listening on that channel because it is blocking on pushing an item to memQueue.

We want to make sure that if feedMemQueue is ever blocking on pushing to memQueue, then the mode MUST be disk-mode.

The Fix

The fix is straight forward: basically, if feedMemQueue successfully pops an item from disk (and is therefore about to potentially block on memQueue then it first sets the mode to disk-mode.

Confirmation of the correctness of the fix

Before this commit, I ran the test 10k times & ran into the flake 4 times.
After this commit, I ran the test 10k times and did not run into the flake at all.

Fixes #8355

[guggero]

Great investigation! And nice that the fix is so small :)

Could we perhaps add a reproduction as a unit test? Given we now know how it happened in the first place?

[ellemouton]

Could we perhaps add a reproduction as a unit test? Given we now know how it happened in the first place?

Good idea @guggero 👍

It was a bit more tricky than usual though since this is a race issue. So what i've done is first added a few temporary variables to allow more easy stop/go control from a test & then i've added the reproduction test. Then I add the commit that fixes the issue and finally, I remove all the stop/go code and also the test.

[guggero]

Nice fix, LGTM 🎉

[bitromortac]

Thank you for the nice write-up, demo, and fix! LGTM ⚡