fix: limit and track processing of SendReplyJobs around session and network partitions

[cfm]

• Blocked on data migrations will fail if subsequent versions add columns to back-referenced tables #1500

Description

• Fixes Client retries failed jobs after logging out/in without restart #1420 by clearing the ApiJobQueue when it stops, so that unprocessed jobs (e.g. SendReplyJobs for pending DraftReplys) will not continue to be processed under the session that's just logged out.
• Fixes "Failed" replies may actually succeed after client exits #1457 and fixes update_replies(): purge or mark stale pending ("draft") replies #1493 by having the SendReplyJob take "ownership" of a given DraftReply by marking it with the current PID, so that both the current and a subsequent Client session can infer whether or not the SendReplyJob's API call could still be in flight.

For more details on why all this extra machinery is necessary to address this pair of corner cases, see especially #1486 (comment) and #1457 (comment).

Test Plan

Synthesized from the reproductions of #1420 and #1457:

1. On your test server, watch /var/log/apache2/journalist-access.log.
2. Create a new source and submit a message.
3. Log into the Client and wait for sync.
4. Send a reply and wait for it to send.
5. Observe exactly 1 "POST /api/v1/sources/<uuid>/replies HTTP/1.1" 201.
6. Disconnect sd-whonix from sys-firewall.
7. Send at least n >= 1 more replies.
8. Quit the Client.
9. Reconnect sd-whonix to sys-firewall.
10. Observe exactly 1 "POST /api/v1/sources/<uuid>/replies HTTP/1.1" 201.
11. Wait several seconds.
12. Start the Client again.
13. Log in again and wait for sync.
14. Observe only the first message from step (5) has succeeded...
15. ...and the rest are marked Failed to send.
16. Repeat steps (1)‒(15), but log out (and back in) rather than quitting and restarting the Client.
17. Observe the same results.

Bonus: bootstrapping case

1. Delete your svs.sqlite and reinitialize it with alembic upgrade head.
2. Log into the Client and wait for sync.
3. Sources and submissions are retrieved and displayed as usual.

Checklist

If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:

• I have tested these changes in the appropriate Qubes environment
• I do not have an appropriate Qubes OS workstation set up (the reviewer will need to test these changes)
• These changes should not need testing in Qubes

If these changes add or remove files other than client code, the AppArmor profile may need to be updated. Please check as applicable:

• I have updated the AppArmor profile
• No update to the AppArmor profile is required for these changes
• I don't know and would appreciate guidance

If these changes modify the database schema, you should include a database migration. Please check as applicable:

• I have written a migration and upgraded a test database based on main and confirmed that the migration is self-contained and applies cleanly
• I have written a migration but have not upgraded a test database based on main and would like the reviewer to do so
• I need help writing a database migration
• No database schema changes are needed

[cfm]

@creviera, I'd welcome your early feedback on this overall approach to #1457. The test plan (TK tomorrow) will address whether this also fixes #1420, per your hypothesis that they're actually the same bug only apparently at different layers in the application. Thanks!

[sssoleileraaa]

@cfm, this change is pleasantly small, yet the issue caused so much confusion during QA last release. The only gotcha I can think of is that we actually want to fail the replies in the queue that are not being processed.

[cfm]

Thanks, @creviera! The test plan I've outlined above gives two pieces of good news:

1. Client retries failed jobs after logging out/in without restart #1420 and "Failed" replies may actually succeed after client exits #1457 are indeed the same bug: the first SendReplyJob in flight before logout will succeed when network connectivity is restored, regardless of the Client's state. For that job, the Server behaves identically between the two reproductions, which appear to diverge only because the job queue is not persistent across Client invocations.

2. You wrote that "we actually want to fail the replies in the queue that are not being processed". Yes! In Client retries failed jobs after logging out/in without restart #1420, with this change, we get that behavior for free, because the Server correctly rejects the second through nth SendReplyJobs with HTTP 403.

The bad news: We need a different mechanism to handle this situation in #1457, in which:

1. we have draft replies still marked as pending from a previous Client invocation;
2. we have no SendReplyJobs pending in the queue in this Client invocation; and
3. these replies are not present in the set of replies returned by the Server.

I believe this to be a data-reconciliation problem (storage.py), not a job-processing problem (queue.py). I'll open a follow-up ticket for that problem, so that you can decide whether to review this fix as is or wait to do a stacked review of both.

[cfm]

Visually reviewed (and tested, albeit off-plan :-) with @creviera. The test plan checks out as written; but #1493 represents a regression from the current behavior, not just an impetus for future refactoring. Our new hypothesis, which I'll test next week (realistically):

1. this branch addresses Client retries failed jobs after logging out/in without restart #1420 in the GUI;
2. poor old fix: clear RunnableQueues on ApiJobQueue.stop() #1434 addresses Client retries failed jobs after logging out/in without restart #1420 in the job queue; and

—so the two of them, rebased together, will let us solve #1420 at both layers. (That would then leave #1493's refactoring to help us untangle these layers.)

[cfm]

Our hypothesis in #1486 (comment) is right so far as it goes, but we need a bit more wiring to keep the GUI in sync with both the SendReplyJob set (to which it's reactive) and the DraftReply table (to which it's not, and which currently doubles as a shadow queue of pending replies). That's added as of 9ae4f2d, which by the end of the week I'll have cleaned up and test-planned for review. Some new architectural/refactoring insights will follow in #1493.

[cfm]

Now that this fix is no longer blocked by #1500, I'm interested in getting it back to ready for review. Whether or not we merge it, it's useful as an illustration of some technical debt I'd like us to have a shared understanding of for future architectural discussions.

Necessary changes:

1. Rebase from main, resolving conflicts with recent refactoring of securedrop.logic (easy) and securedrop.queue (harder) and their tests
2. Rebase Alembic migration onto current head d7c8af95bc8e (new base from fix: squash Alembic migrations; eliminate non-self-contained data-migration tests #1517)
3. Retest
4. Write new test plan

[eloquence]

This is marked as "in development", is that accurate or should we bump it back for now?

[cfm]

I'm finally resuscitating this branch against current main and on the current project board—and aiming to propose a clear way forward on this proposal by the end of next week.

[gonzalo-bulnes]

For the record: @cfm and I walked through this PR and #1493. The vision makes sense to me. I'll summarize it later :P

Edit to add: The gist of the vision beyond this PR is:

• making a distinction between user intent (primarily a GUI concern) and operations tracking (that is: how much have we acted upon that intent so far?)
• being intentional in not introducing unnecessary fragmentation (e.g. by having two actors update distinct records of the same event) and actively looking for it
• working on a technical level on unifying concepts, for example, towards dealing with queues in a near-standard manner, whichever that is, instead of having a collection of distinct queue implementations and behaviors

[gonzalo-bulnes]

@cfm It may be obvious to you, but the CI pipeline definition has changed significantly since you opened this PR, so you won't get anything close to a meaningful CI failure until you update onto main. 🍎 🍏

[cfm]

@gonzalo-bulnes, I've been able to trace this bug to the discrepancy described and fixed in e313fc9. As I note there, this turns out not to be a data race, strictly speaking. Rather, the Reply and the ReplyWidget have different (implicit) state machines, and only certain signals propagate changes from the former to the latter.

This could be fixed in the controller instead, with something like:

--- a/securedrop_client/logic.py
+++ b/securedrop_client/logic.py
@@ -892,6 +892,7 @@ class Controller(QObject):
         """
         self.session.commit()  # Needed to flush stale data.
         reply = storage.get_reply(self.session, uuid)
+        self.reply_succeeded.emit(reply.source.uuid, reply.uuid, reply.content)
         self.reply_ready.emit(reply.source.uuid, reply.uuid, reply.content)
 
     def on_reply_download_failure(self, exception: DownloadException) -> None:

(Emitting this signal in Controller.download_new_replies, where it would arguably make more sense, unfortunately creates the risk of an equal, but different, discrepancy as a side effect.)

I favor the change proposed in e313fc9 simply because the fix and the test are each a single-line change at the GUI level, while this approach would require constructing a dedicated cross-layer test case.

I don't love it, though. More motivation for #1493 (comment), unless you can think of a cleaner way for now!

[gonzalo-bulnes]

The explanation in the commit message makes sense to me @cfm, and I agree that while no ideal (a somewhat unintelligible without context), received_from_the_sever => successfully_sent_to_the_server is sound.

I think you're making the right decision when favoring the smaller, simpler change over another one that requires additional design. We'll be in a better place for design when we can take a few steps back, rather than designing a solution to a problem that might not even occur anymore after we look at the process sin its entirety. 👍 👍

I'll do a round of testing tomorrow. (And might ask you for a refresher on the onion addresses config, let's see how precisely I remember the procedure!)

[gonzalo-bulnes]

1. On your test server, watch /var/log/apache2/journalist-access.log.
2. Create a new source and submit a message. ℹ️ The source is called ethnological remark.
3. Log into the Client and wait for sync.
4. Send a reply and wait for it to send.
5. Observe exactly 1 "POST /api/v1/sources/<uuid>/replies HTTP/1.1" 201.
6. Disconnect sd-whonix from sys-firewall.
7. Send at least n >= 1 more replies. ℹ️ I sent two.
8. Quit the Client.
9. Reconnect sd-whonix to sys-firewall.
10. Observe exactly 1 "POST /api/v1/sources/<uuid>/replies HTTP/1.1" 201. ℹ️ That reply, the first of the two I sent, made it to the server and was visible to the source, as expected.
11. Wait several seconds.
12. Start the Client again.
13. Log in again and wait for sync.
14. Observe only the first message from step (5) has succeeded... 🍊 Like in the previous test, the first message -that gets sent- never get "seen" check marks until the client is restarted. - which, again, I wouldn't worry about in the scope of this PR.
15. ...and the rest are marked Failed to send.
16. Repeat steps (1)‒(15), but log out (and back in) rather than quitting and restarting the Client.
17. Observe the same results.

---

However: When the network is reconnected, the reply can be shown as failed for some time, until a "sync" turns the body and avatar purple.. but leaves the "(i) Failed to send" label in place 😞

I'm skipping detailing steps to reproduce because I'm not sure I understand the basic connection behavior. (The GUI messaging around connection state doesn't seems to match what I know of the connection between sd-whonix and sys-firewall. I'm not sure if that's related to this branch, and I suspect it might not be. I think the "sync" sometimes gets stuck until new source messages come in... either way I see flashed of "failed" replies that then turn "sent". Let's talk about that, and maybe we can 🍐 pair together on characterizing those scenarios errors better @cfm.

[cfm]

#1486 (comment):

However: When the network is reconnected, the reply can be shown as failed for some time, until a "sync" turns the body and avatar purple.. but leaves the "(i) Failed to send" label in place disappointed

Tomorrow @gonzalo-bulnes and I will try to reproduce this case with some instrumentation patched in. Depending on our findings, I have a local branch already rebased against current main that I can force-push here for a quick turnaround.

[cfm]

f0040bc is rebased to clear conflicts after #1604, after extensive testing with @gonzalo-bulnes to make sure that e313fc9, from which it does not differ substantively, was actually the head under test.

[gonzalo-bulnes]

The unexpected and inconsistent states are cleared by f0040bc. 💮

I'll approve the PR based on extensive testing with @cfm.

[gonzalo-bulnes]

🎩 @cfm!