fix!: keep primary transport device-local - #8510
Conversation
It is ready, waiting for a review. Few comments that are not marked as resolved there are in a discussion state or something for the future. |
3238361 to
ab35e93
Compare
51f2a46 to
4fa5404
Compare
eba0573 to
a5f60bd
Compare
Devices no longer implicitely use the From address of sync messages to determine their primary transport. Receivers have no concept of it and own devices may disagree on which relay is reachable because of VPN or different networks: 1. Make setting a primary transport (`configured_addr`) a per-device non-synced operation. 2. Transport rows (add/remove/unpublish) keep syncing like before. 3. A device reelects a primary if a sync message unpublished/removed the current primary if there is a better candidate. 4. `TransportsModified` event is emitted at most once on an incoming transport sync message. Users will notice the change in that changing primary transport in settings/advanced/relays will not synchronize to other devices anymore.
4fa5404 to
8f47f8a
Compare
thanks for the note, this PR here is independently ready for review as well. I didn't mean to imply that #8345 replaces it, but rather that they play to the same goal of automated send failover. |
| // A published primary transport is still a valid choice. | ||
| if transports | ||
| .iter() | ||
| .any(|(addr, is_published)| *is_published && *addr == configured_addr) | ||
| { | ||
| return Ok(None); | ||
| } |
There was a problem hiding this comment.
I wouldn't understand the comment if I didn't know what it means. Rather "If the primary transport is still available, everything is fine already" or just remove it
| "SELECT addr, is_published FROM transports | ||
| ORDER BY add_timestamp DESC, id DESC", | ||
| )? | ||
| .query_map((), |row| Ok((row.get(0)?, row.get(1)?)))? |
There was a problem hiding this comment.
Ok((row.get(0)?, row.get(1)?)))? can likely be simplified to (row.get(0)?, row.get(1)?)), because Ok(...)? just wraps ... in a Result::Ok, and immediately un-wraps it again
...OTOH we want to get rid of this function again, anyways, because we want to get rid of the whole concept of a primary transport, so, it doesn't matter.
| /// Tests that changing the primary transport propagates to other devices | ||
| /// even if the promoted transport was added within the same second. | ||
| #[tokio::test(flavor = "multi_thread", worker_threads = 2)] | ||
| async fn test_promote_transport_same_second() -> Result<()> { |
There was a problem hiding this comment.
Not sure if this test still tests something? The transport is already synced to alice2, so that the only thing that promote_transport_and_sync does is checking that add_timestamp increased.
3 participants
[branched is based on #8499 for now]
configured_addrnow just carries a local device's current primary relay choice. If another device removes the underlying transport, or unpublishes is, the device selects the next best from available transports. Devices no longer implicitly use theFromaddress of sync messages to determine their primary transport.Note that this PR, just as main, does nothing to prevent SMTP queued outgoing messages from failing/vanishing when the primary changes. But the PR may well cause less trouble as on main any
Fromchange of a sync message corrupts pending messages. In any case, the eventual real fix is in finalizing and merging #8345 and letting send-failover freely choose a "next primary" for sending if the current primary fails, without any multi-device synchronization repercussions.Users may notice the change in that changing a relay as "used for sending" in settings/advanced/relays will not synchronize to other devices anymore. On the positive side, devices may anyway have different connectivity (VPN, different networks etc.) and letting them make per-device primary relay choices (used for sending and iOS background-fetch) is better compared to them constantly disagreeing over what is a working primary "global" transport.