Replies are sometimes displayed out of order

[eloquence]

STR 1 - For SendReplyJobTimeoutError

1. Apply this diff so that sometimes a reply fails:

--- a/securedrop_client/api_jobs/uploads.py
+++ b/securedrop_client/api_jobs/uploads.py
@@ -28,6 +28,11 @@ class SendReplyJob(ApiJob):
         database and return the reply uuid string. Otherwise raise a SendReplyJobException so that
         we can return the reply uuid.
         '''
+        import random
+        if random.choice([0, 1]):
+            self.remaining_attempts = 0
+            raise SendReplyJobTimeoutError('test', self.reply_uuid)
+
         try:
             draft_reply_db_object = session.query(DraftReply).filter_by(uuid=self.reply_uuid).one()
             source = session.query(Source).filter_by(uuid=self.source_uuid).one()

2. Send many replies so that there are some successful and once one fails see the conversation reorder like so:

3. Wait until the reply succeeds (because of auto-resume when sync succeeds) and see that the failed reply moves to a successful state

4. Wait until the next sync and see the replies get reordered correctly:

STR 2 - For SendReplyJobError

1. Apply this diff so that sometimes a reply fails:

--- a/securedrop_client/api_jobs/uploads.py
+++ b/securedrop_client/api_jobs/uploads.py
@@ -28,6 +28,11 @@ class SendReplyJob(ApiJob):
         database and return the reply uuid string. Otherwise raise a SendReplyJobException so that
         we can return the reply uuid.
         '''
+        import random
+        if random.choice([0, 1]):
+            self.remaining_attempts = 0
+            raise SendReplyJobError('test', self.reply_uuid)
+
         try:
             draft_reply_db_object = session.query(DraftReply).filter_by(uuid=self.reply_uuid).one()
             source = session.query(Source).filter_by(uuid=self.source_uuid).one()

repeat steps 2-4 above

[redshiftzero]

Problem 1: Replies are prioritized low in the queue

Fixes:

• 462 (we're clogging up the queue with Metadata jobs)

• we prioritize the user-triggered actions higher - https://github.com/freedomofpress/securedrop-client/compare/deprioritize-downloads
  • eventually: allow jobs that can run concurrently to do so

Problem 2: conversation slow to update

Fix:

• As you notice these replies will reorder themselves, but the conversation view might not update immediately. We could have forced the entire conversation view to be redrawn every time a reply is sent but did not due to #473 which needs to be done (we are deleting the entire view and redrawing it but only when a sync completes (if we do it more often we see flicker which is also a bug), which is why you see the reply reorder itself into the right position after a little while).

[eloquence]

462 (we're clogging up the queue with Metadata jobs)

Agreed, but I'm not sure that this is the entire cause of the reply slowness I'm seeing -- I was able to reliably reproduce this over the course of a couple of minutes, where it would alternate between fast (2.5 seconds) and slow (15-20 seconds), without me initiating any sync of my own during this time.

[eloquence]

Performance is significantly better in the latest nightly build, I'm now seeing >1st replies transition all the way to "sent" in the 6-8 second range. Perhaps this will go down further once all the metadata sync changes have landed.

We still have the ordering problem; the order is only "eventually correct", but initially replies appear out of order.

Also worth noting: Out of 20 replies or so I encountered one random failure. I would have expected it to go to "Failed to send" after lots more retries, but it basically transitioned to "Failed" almost immediately.

[ntoll]

Is this not fixed by #688..?

[redshiftzero]

yep you're right, I have not observed this behavior for some time. Closing and let's reopen if we see this again.

[sssoleileraaa]

I just updated the description with the latest STR and reopening because this is not resolved yet.

[ntoll]

Aha..! :smile:

[eloquence]

I've not tested your STR yet Allie, but yay, generally speaking, replies are a lot more reliable now (in Qubes). I'm not seeing the ordering issue during normal use, and replies are sent quite quickly.

[sssoleileraaa]

When someone gets a chance, could you run through the updated STR to confirm that this is a bug we need to fix: https://github.com/freedomofpress/securedrop-client/issues/653#issue-535526020 -- I'm not a 100% sure that @ntoll has already done so

[sssoleileraaa]

today, when running through the STR I saw the same issue:

[sssoleileraaa]

Note: these replies stay out of order even after a pending reply turns to a successful reply. when i close the client and reopen the replies are reordered correctly:

It appears the bug is somewhere within update_conversation

[ntoll]

So... @creviera yes I did manage to recreate but functional tests have taken up my time. However, now they're done I'll look into this.

I've just observed the following (problem) behaviour by following the STR:

• A failed message appears to block the queue of pending messages (i.e. if message 1 is failed, the subsequent messages 2, 3, and 4 all appear in the greyed out pending state). This appears to suggest @redshiftzero is correct about queue priorities / concurrent replies.
• Once the client retries after 15 seconds the failed message is marked as sent and appears in the correct position in the queue -- however, the pending and any subsequent failed messages are still in the wrong place. For instance if message 1 had succeeded, message 2 failed and messages 3 and 4 were pending, then, when message 2 succeeded it would appear immediately after message 1 but messages 3 and 4 might be in the wrong place.

I agree with @creviera that the error is in 'update_conversation' and that it's not so much a bug, as an omission to deal with how to order pending or failed replies (which won't yet be known by the server, so the message order when we next refresh will be correct but won't contain those unknown replies).

My suggested fix (which I'll go ahead and implement so we can at least confirm my hunch is correct -- we can change the fix's behaviour during review of the impending PR) is:

• All replies known to the server retain their position in the order of the conversation.
• All replies unknown to the server (i.e. those in failed or pending states) MUST always be the last replies in the conversation list.

Sound like a plan..?

Give me 10 mins to get a PR in fight with a potential fix / unit tests to prove things behave as expected.

[redshiftzero]

OK having trouble reproducing this due to #820 but looking at the logic: we have logic to reorder drafts to ensure consistent ordering of interleaved successful replies and drafts. This is performed at sync time, and at reply send time when the reply a successful. That means that the local order we have stored should be correct. In order for this order to be shown in the UI, we need to enter update_conversation for the reordering logic to run at the UI level. Is that happening? I'm not seeing where if so - it seems like only on source click and on recreation of the conversation view. That would explain the behavior.

moved the below to issue #821 to focus convo on the reordering bug (my bad)

Question: do we really need add_reply_from_reply_box? This is one path to widgets getting added to the conversation view outside of calls to update_conversation. It seems unnecessary now: in on_reply_sent here we should be able to call update_conversation instead of this additional method add_reply_from_reply_box (source.collection has the DraftReply object which will be created immediately when we click send).

[sssoleileraaa]

add_reply_from_reply_box is called when the user clicks "Send". Without it, we won't immediately see replies in the ConversationView. If we want to remove it, I think we will need to emit a new signal from the Controller after it creates the new DraftReply in response to the user clicking "Send" and then the GUI will have to respond by adding a new reply widget to the conversation view.

I think this is more of a refactor, but I'm not sure it's related to this issue, because update_conversation uses an index to reorder conversation items if the index no longer matches its GUI placement in the conversation, see https://github.com/freedomofpress/securedrop-client/blob/b75db2d9d937060d87dfe54755615f89734b2b0a/securedrop_client/gui/widgets.py#L2758-L2768

The strange reordering behavior only seems to be introduced once a reply is failed, which is why I no longer see STR #1 reported in this issue after making this change: https://github.com/freedomofpress/securedrop-client/pull/818

[redshiftzero]

In order to verify if this was a UI only issue or not I decided to double check using the debug logging here along with logging the current value of source.conversation when drawing the conversation view. I noticed that the interaction_count was not getting updated on the source, which led me to #829. From the comment:

We use the file_counter on each Reply, Message, File, and DraftReply object to order the conversation view. We expect a unique file_counter for Reply, Message, and File objects since they are retrieved from the server. For DraftReply, we don't have that unique constraint, and we instead expect the file_counter to be the interaction_count at the time of send. If we do not update the interaction_count after each successful reply send, future drafts will have an interaction_count that is too low, leading to incorrectly ordered drafts until a metadata sync completes (the metadata sync is the place where the source object is updated from the server).

[rocodes]

I've noticed a sort-of a recurrence of this issue, but no failed reply is required. (Unreliable STR: send lots of replies in quick succession, can observe mis-ordered messages some of the time, order corrects after sync). I'm not sure if it merits re-opening this issue, since it basically involves quickly sending many consecutive replies, and is resolved after a sync, but documenting it here.)

(sometimes the mis-ordering is worse than this, sometimes it doesn't occur at all even with upwards of 15 messages):

(reordered correctly after a sync)

[eloquence]

I saw this once as well during recent QA but didn't write it up, thanks for noting it here. I'd say let's definitely reopen, defer to Allie on priority, but I think it'd be good to know at least what's causing it.