Add an enqueue time to the send queue system #4385

zzorba · 2024-12-06T17:23:47Z

Add a new created_at to the send_queue_events and dependent_send_queue_events stored records. This will allow clients to understand how stale a pending message might be in the event that the queue encounters and error and becomes wedged.

This change is exposed through the FFI on the EventTimelineItem struct as a new optional field named local_created_at. It will be None for any Remote event, and Some for Local events (except for those that were enqueued before the migrations were run).

Signed-off-by: Daniel Salinas

codecov · 2024-12-06T17:38:41Z

Codecov Report

Attention: Patch coverage is 95.90164% with 5 lines in your changes missing coverage. Please review.

Project coverage is 85.44%. Comparing base (519f281) to head (f3a1334).
Report is 25 commits behind head on main.

Files with missing lines	Patch %	Lines
...rates/matrix-sdk-ui/src/timeline/event_item/mod.rs	0.00%	4 Missing ⚠️
crates/matrix-sdk/src/send_queue.rs	95.45%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4385      +/-   ##
==========================================
+ Coverage   85.41%   85.44%   +0.03%     
==========================================
  Files         283      283              
  Lines       31559    31659     +100     
==========================================
+ Hits        26955    27052      +97     
- Misses       4604     4607       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bnjbvr

Looks like this is on the right path! It would be nice to expose the value in the local echoes themselves then, and test this all works fine.

crates/matrix-sdk-sqlite/src/state_store.rs

crates/matrix-sdk-base/src/store/send_queue.rs

crates/matrix-sdk-indexeddb/src/state_store/mod.rs

crates/matrix-sdk-base/src/store/send_queue.rs

zzorba · 2024-12-09T22:09:02Z

@bnjbvr thank you for the review. Made your suggested corrections (one pending question), and then in a second commit I have attempted to expose this timestamp through the api layer.

It was more involved than I thought, but I think there was only one spot where I wasn't quite sure which timestamp to use (related to retry of a media message I believe, I left a comment). Right now I have the storage layer assigning the timestamp and returning it to be used by the SendHandle system. If you think it would be cleaner to pass in a timestamp I am open to changing this.

If you think this API shape is reasonable, I will work to add some tests (or extend the existing tests) to exercise it.

bnjbvr

Looks like this is going in the right direction 👍 I haven't reviewed in depth, but left a few high-level comments in places. Let's go ahead and add some tests 💪

crates/matrix-sdk/src/send_queue.rs

crates/matrix-sdk-base/src/store/traits.rs

bindings/matrix-sdk-ffi/src/timeline/mod.rs

crates/matrix-sdk-sqlite/migrations/state_store/010_send_queue_enqueue_time.sql

bindings/matrix-sdk-ffi/src/timeline/mod.rs

zzorba · 2024-12-11T16:33:02Z

@bnjbvr I had to mark the push_media method as a clippy exception because the additional argument pushed it over the line. Thank you again for all your feedback on this feature, I think the feature has been greatly improved.

zzorba · 2024-12-12T16:20:00Z

Rebased to resolve merge conflicts and flattened the commits.

zzorba · 2024-12-18T18:00:23Z

Unclear to me what the current build break is related to in this PR.

bnjbvr · 2024-12-19T10:23:24Z

Unclear to me what the current build break is related to in this PR.

Yep, it's an unrelated intermittent failure, nothing to do with your PR. I'll take another look today 👍

bnjbvr

Looks like we're getting super close to land this. Sorry to ask a bit for more changes, I spotted a few ways to make this even cleaner.

(I will be on holidays starting today for a few weeks, so there might be a bit of delay before the next round of review. Happy end of the year, and thanks for bearing with us :))

bnjbvr · 2024-12-19T14:37:57Z

crates/matrix-sdk-ui/src/timeline/event_handler.rs

@@ -1033,6 +1033,7 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> {
                send_state: EventSendState::NotSentYet,
                transaction_id: txn_id.to_owned(),
                send_handle: send_handle.clone(),
+                created_at: send_handle.clone().and_then(|h| h.created_at),


Probably the cloning is not required here:

Suggested change

created_at: send_handle.clone().and_then(|h| h.created_at),

created_at: send_handle.created_at,

(or created_at.clone(), if borrowck complains)

So send_handle is an Option so I can't access created_at directly. And if I try to map and clone, it seems to dislike the Option being mapped (since self.ctx_flow is a reference). I think the clone is needed?

I think the field is Copy so it should work with send_handle.as_ref().and_then(|h| h.created_at).

crates/matrix-sdk-base/src/store/send_queue.rs

crates/matrix-sdk-indexeddb/src/state_store/mod.rs

bnjbvr · 2024-12-19T14:42:09Z

crates/matrix-sdk-base/src/store/send_queue.rs

@@ -131,6 +132,9 @@ pub struct QueuedRequest {
    /// The bigger the value, the higher the priority at which this request
    /// should be handled.
    pub priority: usize,
+
+    /// The time that the request was original attempted.
+    pub created_at: Option<MilliSecondsSinceUnixEpoch>,


Could this be non-optional, since this is not serialized we could always provide a value here? There might be places where we upgrade a DependentQueuedRequest with no created_at (because they were created before this change is introduced) to a QueuedRequest, but it seems fine to use Milliseconds::now() in those cases, maybe?

Yeah, so if we are okay with injecting Milliseconds::now when it fails to parse coming out of the DB (either due to a previous migration or due to ... cosmic rays, then I think we can safely make this non optional.

IMO that's totally fine to do as you explained, just wasn't sure about injecting false data.

I've updated it to use a ::now in most places and make it required everywhere except when it is exposed in the FFI (because SendHandle might not be present for the Local items)

crates/matrix-sdk-ui/src/timeline/event_item/mod.rs

crates/matrix-sdk-ui/src/timeline/tests/echo.rs

crates/matrix-sdk-ui/tests/integration/timeline/queue.rs

bnjbvr · 2024-12-19T14:54:53Z

crates/matrix-sdk/src/send_queue.rs

+        let created_at = MilliSecondsSinceUnixEpoch::now();
+        let transaction_id = self.inner.queue.push(content.clone().into(), created_at).await?;


The PR mixes two ways of feeding the created_at:

sometimes it's created at the RoomSendQueue level, and passed as a parameter to a QueueStorage function,

sometimes it's the QueueStorage that will call now() and pass it directly to the underlying store functions.

We should make this a bit more uniform; I think I prefer the latter where the QueueStorage internally sets it. It's nicer, because it implies having fewer parameters to the QueueStorage external functions too.

So the original version had it being injected directly in the send_queue to minimize the API changes.

But this meant though that the timestamp would have to be returned to be used in constructing a SendHandle, which we did not like. Sometimes the SendHandles come from the persisted store, but sometimes they are constructed directly by people calling the send_queue method.

Let me know which way you prefer. Personally I think the current method is cleaner, and also makes it easier to write tests against.

bnjbvr · 2024-12-19T14:56:17Z

crates/matrix-sdk/src/send_queue.rs

+
+    /// The time that this send handle was first created
+    pub created_at: Option<MilliSecondsSinceUnixEpoch>,


I see that the only place where this is read is in the timeline code, correct? As a last high-level comment: I think this could be moved to the LocalEchoContent::Event variant, even — we might not care about a reaction's created_at (or do we? if so, we could move it to the LocalEcho struct more generally).

So the issue is that it needs to be exposed in the LocalEventTimelineItem in crates/matrix-sdk-ui/src/timeline/event_handler.rs which doesn't appear to have access to LocalEchos. Thats why I put it in the SendHandle.

bnjbvr · 2024-12-19T15:01:43Z

Unclear to me what the current build break is related to in this PR.

Yep, it's an unrelated intermittent failure, nothing to do with your PR. I'll take another look today 👍

(If you rebase on top of main it should be fixed now.)

Co-authored-by: Benjamin Bouvier <[email protected]> Signed-off-by: Daniel Salinas <[email protected]>

zzorba requested a review from a team as a code owner December 6, 2024 17:23

zzorba requested review from poljar and removed request for a team December 6, 2024 17:23

zzorba force-pushed the add_enqueue_time_to_send_queues branch 7 times, most recently from e0b7108 to b1ef4ca Compare December 7, 2024 00:57

bnjbvr requested review from bnjbvr and removed request for poljar December 9, 2024 08:51

bnjbvr reviewed Dec 9, 2024

View reviewed changes

zzorba force-pushed the add_enqueue_time_to_send_queues branch from 0208b4e to f06e529 Compare December 9, 2024 21:53

zzorba force-pushed the add_enqueue_time_to_send_queues branch from f06e529 to 0530eb0 Compare December 9, 2024 22:11

bnjbvr self-requested a review December 10, 2024 10:51

bnjbvr reviewed Dec 10, 2024

View reviewed changes

zzorba force-pushed the add_enqueue_time_to_send_queues branch 2 times, most recently from 7f01da5 to 6d799dd Compare December 10, 2024 22:09

bnjbvr reviewed Dec 11, 2024

View reviewed changes

bindings/matrix-sdk-ffi/src/timeline/mod.rs Outdated Show resolved Hide resolved

zzorba force-pushed the add_enqueue_time_to_send_queues branch 3 times, most recently from c3726a0 to df7fc6e Compare December 11, 2024 16:11

zzorba force-pushed the add_enqueue_time_to_send_queues branch from df7fc6e to 12d9609 Compare December 12, 2024 16:19

zzorba force-pushed the add_enqueue_time_to_send_queues branch from 12d9609 to 9ac86a1 Compare December 18, 2024 16:51

zzorba requested a review from bnjbvr December 18, 2024 17:08

zzorba force-pushed the add_enqueue_time_to_send_queues branch from 9ac86a1 to d47e45a Compare December 18, 2024 17:15

bnjbvr requested changes Dec 19, 2024

View reviewed changes

Add created_at time persisted send_queue system

b5e4b99

zzorba force-pushed the add_enqueue_time_to_send_queues branch from d47e45a to b5e4b99 Compare December 19, 2024 23:09

Apply suggestions from code review

1e6bfa3

Co-authored-by: Benjamin Bouvier <[email protected]> Signed-off-by: Daniel Salinas <[email protected]>

zzorba force-pushed the add_enqueue_time_to_send_queues branch from 71b057d to 1e6bfa3 Compare December 20, 2024 15:20

Expose created_at from SendHandle directly instead of unpacking

f3a1334

zzorba force-pushed the add_enqueue_time_to_send_queues branch from 8d4a9e7 to f3a1334 Compare December 20, 2024 15:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an enqueue time to the send queue system #4385

Add an enqueue time to the send queue system #4385

zzorba commented Dec 6, 2024 •

edited

Loading

codecov bot commented Dec 6, 2024 •

edited

Loading

bnjbvr left a comment

zzorba commented Dec 9, 2024

bnjbvr left a comment

zzorba commented Dec 11, 2024

zzorba commented Dec 12, 2024

zzorba commented Dec 18, 2024

bnjbvr commented Dec 19, 2024

bnjbvr left a comment

bnjbvr Dec 19, 2024

zzorba Dec 20, 2024

jplatte Dec 20, 2024

bnjbvr Dec 19, 2024

zzorba Dec 20, 2024

zzorba Dec 20, 2024

bnjbvr Dec 19, 2024

zzorba Dec 20, 2024

bnjbvr Dec 19, 2024

zzorba Dec 20, 2024

bnjbvr commented Dec 19, 2024

	created_at: send_handle.clone().and_then(\|h\| h.created_at),
	created_at: send_handle.created_at,

		let created_at = MilliSecondsSinceUnixEpoch::now();
		let transaction_id = self.inner.queue.push(content.clone().into(), created_at).await?;


		/// The time that this send handle was first created
		pub created_at: Option<MilliSecondsSinceUnixEpoch>,

Add an enqueue time to the send queue system #4385

Are you sure you want to change the base?

Add an enqueue time to the send queue system #4385

Conversation

zzorba commented Dec 6, 2024 • edited Loading

codecov bot commented Dec 6, 2024 • edited Loading

Codecov Report

bnjbvr left a comment

Choose a reason for hiding this comment

zzorba commented Dec 9, 2024

bnjbvr left a comment

Choose a reason for hiding this comment

zzorba commented Dec 11, 2024

zzorba commented Dec 12, 2024

zzorba commented Dec 18, 2024

bnjbvr commented Dec 19, 2024

bnjbvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnjbvr commented Dec 19, 2024

zzorba commented Dec 6, 2024 •

edited

Loading

codecov bot commented Dec 6, 2024 •

edited

Loading