feat: Track when we last received a message from which contact via which transport - #8544
feat: Track when we last received a message from which contact via which transport#8544Hocuri wants to merge 6 commits into
Conversation
|
@link2xt do you think this is the right approach in general? |
|
The table looks good, at least for collecting statistics and understanding how the knowledge of relays is distributed. As for the approach, it might turn out unnecessary if it happens extremely rarely that we even have contacts that don't know any of our working relays. If we onboard with three relays, then it is unlikely that we will be removing all three relays (because they all went down) any time soon, so for most users any logic based on this might turn out to be dead code. |
d5b5da6 to
58061e0
Compare
There was a problem hiding this comment.
-
as you know there is a "send key updates" concurrent discussion that addresses keeping our contacts up to date from a different, more stateless, angle. I suggest we first finish that other discussion (and work) before engaging with this PR again. As i had been going through the PR already, preparing a review, i post the rest anyway.
-
backward-compat: we may need to have some
tracking_transports_sinceconfig timestamp. Absence in contact/transport tracking may only be meaningful for contacts since we started tracking. Also, setting up a multi-device starts with zero tracking rows. But absence doesn't mean everyone is stranded, neccessarily. If we don't have a "when did we start recording" timestamp, we may struggle to interpret "no/few rows" and conclude everybody is stranded. -
i am dubious about
/transport_awarenessin Saved Messages - a neat hack but also strange (the message still gets send out IISIC). At least it should be "/debug transports" so the "/debug" prefix can be used for also other things, and we don't occupy ever more "/NAMES". (i generally suggest to not use 'awareness' in any name). But it's obvious that this can be discussed endlessly, and leads away from the actual content of the PR, so i stop it here and just register doubts for now :) -
i am confused by the newly introduced namings, here is a suggestion for renaming:
| Current | Suggested |
|---|---|
transport_awareness_by_contacts (SQL table) |
transport_use_by_contacts |
last_seen (SQL column) |
last_used |
last_seen is, also in SQL statements, ambiguous, and there is no need to introduce more ambiguity.
As to "awareness" i find it broad, and would prefer "use" generally:
| Current | Suggested |
|---|---|
record_message_sent_via_transport |
record_transport_use |
record_message_sent_via_transport_by_msg_id |
record_transport_use_by_msg_id |
output_debug_transport_awareness |
output_debug_transport_use |
get_debug_transport_awareness |
get_debug_transport_use |
| } | ||
| chat.prepare_msg_raw(context, msg, update_msg_id).await?; | ||
|
|
||
| info!(context, "dbg sending msg {msg:?}"); |
| } | ||
| } | ||
|
|
||
| pub(crate) async fn record_message_sent_via_transport_by_msg_id( |
There was a problem hiding this comment.
"sent" is misleading i think.
| if let Some(received_msg) = &received_msg { | ||
| record_message_sent_via_transport(context, received_msg.from_id, transport_id) | ||
| .await | ||
| .context("Recording transport")?; |
There was a problem hiding this comment.
if the function fails for some reason, does this not abort message fetching and contradict the function doc promises?
/// If the message is incorrect or there is a failure to write a message to the database,
/// it is skipped and the error is logged.
| .execute( | ||
| "INSERT OR REPLACE INTO transport_awareness_by_contacts(contact_id, transport_id, last_seen) | ||
| VALUES (?,?,?)", | ||
| (contact_id, transport_id, time()), |
There was a problem hiding this comment.
time() is our current device clock time and has nothing to do with the message or when the relay received the message. Maybe we could use INTERNALDATE that we can fetch from dovecot, and which denotes the clock time when the the message arrived at the relay.
| /// Records that the specified contact sent a message via the specified transport, | ||
| /// i.e. that we can be sure that the contact knows about this transport. | ||
| /// This info is saved into the `transport_awareness_by_contacts` table. | ||
| pub(crate) async fn record_message_sent_via_transport( |
There was a problem hiding this comment.
sent is confusing here, and does not fit other terminology in core which uses "sent" and "rcvd" always from the perspective of the profile. We are tracking rather what we received than we sent here.
| /// Records that the specified contact sent a message via the specified transport, | ||
| /// i.e. that we can be sure that the contact knows about this transport. | ||
| /// This info is saved into the `transport_awareness_by_contacts` table. |
There was a problem hiding this comment.
| /// Records that the specified contact sent a message via the specified transport, | |
| /// i.e. that we can be sure that the contact knows about this transport. | |
| /// This info is saved into the `transport_awareness_by_contacts` table. | |
| /// Records that a message from `contact_id` arrived via `transport_id`. | |
| /// | |
| /// Used to tell whether removing a transport would create stranded contacts | |
| /// where we have no indication they have a remaining way to reach our current transports. |
For automatic relay removal and replacement, we want to track which contact knows about which of our transports, and is able to send messages via it.
The prospective idea is that we can use this information for safe deletion of transports: Before deleting a transport, we can check which of our contacts only know about this transport, and would therefore fail to reach us once the transport is removed. Then, we send a hidden message to all of these contacts in order to tell them about our new relays and/or postpone the deletion until all of our contacts know about the current relays.
This PR here only saves the relevant data to the database and doesn't use them yet.
cc @link2xt
For trying this out: With this PR, you can now send
/transport_awarenessinto the Saved Messages chat to get an info message like this sent into Device Messages: