We also need this for the case where we have a very large number of either sources or submissions: any remaining jobs in the queue when the next sync kicks off with the current logic get added again, resulting in the unnecessary repeated redownload of items, potentially several times (for 15 second sync time and 1000 sources with 4 items per source conversation, this happened multiple times).

@creviera in standup the other day made an important point (h/t to @rmol for pointing me to the relevant bit of code after standup) - for download jobs, the actual downloads won’t happen due to this logic, although the download jobs do get added (what I was seeing in my comment above).

As such, I was considering whether I should close or defer this PR, even though imho having duplicate jobs like this in the network queue is not great design, but before doing so I thought I’d do a quick investigation to determine - just how many unnecessary jobs will get added and how long does it take for them to get cleared out after initial sync in a realistic scenario?

For 1000 sources with 2 submissions and 2 replies each, here’s what happens (note this is not over tor and this is using the 0.1.6 tag):

11:04:09,775 - I login to the client with a clean database
11:04:23,093 - sync begins
11:05:08,105 - download jobs start getting added to the queue and begin to be processed
11:20:17,087 - all data is downloaded, now the duplicate jobs start getting processed
11:25:51,754 - duplicate jobs are finished processing, the queue is now available again for legitimate message/reply downloads

So the queue was blocked to message/reply download for ~5 minutes (in my test, 21220 unnecessary jobs were processed) while all these duplicated jobs were cleared out. The length of time that the queue is blocked to message/reply download will be longer if there were many more messages/replies, or if the time to download an object is longer as the number of jobs processed per sync period is lower, so more duplicate jobs will be added, which would be true if this were happening over tor.

As such, we should still enforce the constraint of disallowing duplicate jobs else legitimate message/reply downloads can be significant delayed (potentially many minutes) due to duplicate jobs consuming resources.

For the interested observer if you want to repeat this test I used this diff:

diff --git a/securedrop_client/api_jobs/downloads.py b/securedrop_client/api_jobs/downloads.py
index aab51d6..930491e 100644
--- a/securedrop_client/api_jobs/downloads.py
+++ b/securedrop_client/api_jobs/downloads.py
@@ -123,9 +123,11 @@ class DownloadJob(ApiJob):
         db_object = self.get_db_object(session)

         if db_object.is_decrypted:
+            logger.debug('OBJECT {} IS DECRYPTED, SKIPPING'.format(db_object.uuid))
             return db_object.uuid

         if db_object.is_downloaded:
+            logger.debug('OBJECT {} IS DOWNLOADED, SKIPPING'.format(db_object.uuid))
             self._decrypt(db_object.location(self.data_dir), db_object, session)
             return db_object.uuid

Given the potential for negative side effects of duplicate jobs on instances with large source counts, we're still treating this as a stretch goal for the 0.2.0 release during the 4/22-5/6 sprint, with @redshiftzero continuing to lead improvement of this PR.

(This is ready for review, but since it's a larger change, we should release 0.2.0 first)

^ Per the above, priority is on release (e.g. fixes for any issues discovered in QA); otherwise we've committed to a time-boxed (4 hour) review of this PR during the 5/20-6/3 sprint, which may or may not result in it getting merged.