refactor(sync): Rewrite simplified sliding sync #2280

Open
eleboucher wants to merge 3 commits from eleboucher/continuwuity:fix/sync-v5-left-rooms into main
Contributor

Rewrites the sliding sync endpoint to follow the merged MSC4186 and its extension MSCs, keeping the unstable wire format current clients speak.

  • Left, kicked and banned rooms and rejected invites now update the room list
  • required_state is sent as a delta, with MSC4186 lazy-loading rules ($LAZY/$ME mapped from the unstable format)
  • is_dm, bump_stamp for invites and new rooms, and heroes follow the MSC
  • Receipts, typing, account data, e2ee, to-device and sticky events follow their MSCs, with no lost to-device messages
  • Positions are retry-safe and work as /messages tokens; connection state is bounded
  • Compatible with matrix-rust-sdk (Element X, sable) and matrix-js-sdk

Tested with Complement (same as main) and matrix-rust-sdk's integration suite (46 passed vs 41 on main, no regressions).

Pull request checklist:

  • This pull request targets the main branch, and the branch is named something other than
    main.
  • I have written an appropriate pull request title and my description is clear.
  • I understand I am responsible for the contents of this pull request.
  • I have followed the contributing guidelines:
<!-- In order to help reviewers know what your pull request does at a glance, you should ensure that 1. Your PR title is a short, single sentence describing what you changed 2. You have described in more detail what you have changed, why you have changed it, what the intended effect is, and why you think this will be beneficial to the project. If you have made any potentially strange/questionable design choices, but didn't feel they'd benefit from code comments, please don't mention them here - after opening your pull request, go to "files changed", and click on the "+" symbol in the line number gutter, and attach comments to the lines that you think would benefit from some clarification. --> Rewrites the sliding sync endpoint to follow the merged MSC4186 and its extension MSCs, keeping the unstable wire format current clients speak. - Left, kicked and banned rooms and rejected invites now update the room list - `required_state` is sent as a delta, with MSC4186 lazy-loading rules (`$LAZY`/`$ME` mapped from the unstable format) - `is_dm`, `bump_stamp` for invites and new rooms, and heroes follow the MSC - Receipts, typing, account data, e2ee, to-device and sticky events follow their MSCs, with no lost to-device messages - Positions are retry-safe and work as `/messages` tokens; connection state is bounded - Compatible with matrix-rust-sdk (Element X, sable) and matrix-js-sdk Tested with Complement (same as `main`) and matrix-rust-sdk's integration suite (46 passed vs 41 on `main`, no regressions). <!-- Example: This pull request allows us to warp through time and space ten times faster than before by double-inverting the warp drive with hyperheated jump fluid, both making the drive faster and more efficient. This resolves the common issue where we have to wait more than 10 milliseconds to engage, use, and disengage the warp drive when travelling between galaxies. --> <!-- Closes: #... --> <!-- Fixes: #... --> <!-- Uncomment the above line(s) if your pull request fixes an issue or closes another pull request by superseding it. Replace `#...` with the issue/pr number, such as `#123`. --> **Pull request checklist:** <!-- You need to complete these before your PR can be considered. If you aren't sure about some, feel free to ask for clarification in #dev:continuwuity.org. --> - [x] This pull request targets the `main` branch, and the branch is named something other than `main`. - [x] I have written an appropriate pull request title and my description is clear. - [x] I understand I am responsible for the contents of this pull request. - I have followed the [contributing guidelines][c1]: - [x] My contribution follows the [code style][c2], if applicable. - [x] I ran [pre-commit checks][c1pc] before opening/drafting this pull request. - [x] I have [tested my contribution][c1t] (or proof-read it for documentation-only changes) myself, if applicable. This includes ensuring code compiles. - [x] My commit messages follow the [commit message format][c1cm] and are descriptive. <!-- Notes on these requirements: - While not required, we encourage you to sign your commits with GPG or SSH to attest the authenticity of your changes. - While we allow LLM-assisted contributions, we do not appreciate contributions that are low quality, which is typical of machine-generated contributions that have not had a lot of love and care from a human. Please do not open a PR if all you have done is asked ChatGPT to tidy up the codebase with a +-100,000 diff. - In the case of code style violations, reviewers may leave review comments/change requests indicating what the ideal change would look like. For example, a reviewer may suggest you lower a log level, or use `match` instead of `if/else` etc. - In the case of code style violations, pre-commit check failures, minor things like typos/spelling errors, and in some cases commit format violations, reviewers may modify your branch directly, typically by making changes and adding a commit. Particularly in the latter case, a reviewer may rebase your commits to squash "spammy" ones (like "fix", "fix", "actually fix"), and reword commit messages that don't satisfy the format. - Pull requests MUST pass the `Checks` CI workflows to be capable of being merged. This can only be bypassed in exceptional circumstances. If your CI flakes, let us know in matrix:r/dev:continuwuity.org. - Pull requests have to be based on the latest `main` commit before being merged. If the main branch changes while you're making your changes, you should make sure you rebase on main before opening a PR. Your branch will be rebased on main before it is merged if it has fallen behind. - We typically only do fast-forward merges, so your entire commit log will be included. Once in main, it's difficult to get out cleanly, so put on your best dress, smile for the cameras! --> [c1]: https://forgejo.ellis.link/continuwuation/continuwuity/src/branch/main/CONTRIBUTING.md [c2]: https://forgejo.ellis.link/continuwuation/continuwuity/src/branch/main/docs/development/code_style.mdx [c1pc]: https://forgejo.ellis.link/continuwuation/continuwuity/src/branch/main/CONTRIBUTING.md#pre-commit-checks [c1t]: https://forgejo.ellis.link/continuwuation/continuwuity/src/branch/main/CONTRIBUTING.md#running-tests-locally [c1cm]: https://forgejo.ellis.link/continuwuation/continuwuity/src/branch/main/CONTRIBUTING.md#commit-messages
eleboucher force-pushed fix/sync-v5-left-rooms from 637ba1764b
Some checks failed
Documentation / Build and Deploy Documentation (pull_request) Has been skipped
Auto Labeler / Apply labels based on changed files (pull_request_target) Successful in 3s
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 7s
Checks / Prek / Check changed files (pull_request) Successful in 8s
Checks / Prek / Pre-commit & Formatting (pull_request) Failing after 1m41s
Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 7m23s
to e5bb1f06fa
All checks were successful
Documentation / Build and Deploy Documentation (pull_request) Has been skipped
Checks / Prek / Check changed files (pull_request) Successful in 6s
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 7s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m13s
Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 7m15s
2026-09-23 12:25:26 +00:00
Compare
eleboucher force-pushed fix/sync-v5-left-rooms from e5bb1f06fa
All checks were successful
Documentation / Build and Deploy Documentation (pull_request) Has been skipped
Checks / Prek / Check changed files (pull_request) Successful in 6s
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 7s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m13s
Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 7m15s
to 20c7b32008
All checks were successful
Documentation / Build and Deploy Documentation (pull_request) Has been skipped
Checks / Prek / Check changed files (pull_request) Successful in 6s
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 8s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m18s
Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 7m22s
2026-09-23 13:51:24 +00:00
Compare
Owner

required_state is sent as a delta; $LAZY and $ME work

I'm getting a sense of de-ja-vu here. These don't exist: #1533, #1661, #1690, #2237.

Given they used to exist, I have to ask, does this actually implement MSC4186 as merged, or does it implement a compatibility hybrid that will break when clients update to their next set of quirks next week, requiring another refactor?

> `required_state` is sent as a delta; `$LAZY` and `$ME` work I'm getting a sense of de-ja-vu here. These *don't exist*: #1533, #1661, #1690, #2237. Given they *used to* exist, I have to ask, does this actually implement MSC4186 as merged, or does it implement a compatibility hybrid that will break when clients update to their next set of quirks next week, requiring another refactor?
Author
Contributor

@nex wrote in #2280 (comment):

required_state is sent as a delta; $LAZY and $ME work

I'm getting a sense of de-ja-vu here. These don't exist: #1533, #1661, #1690, #2237.

Given they used to exist, I have to ask, does this actually implement MSC4186 as merged, or does it implement a compatibility hybrid that will break when clients update to their next set of quirks next week, requiring another refactor?

yes sorry i removed it i forgot to update the description my local matrix spec was out of date

@nex wrote in https://forgejo.ellis.link/continuwuation/continuwuity/pulls/2280#issuecomment-36069: > > `required_state` is sent as a delta; `$LAZY` and `$ME` work > > I'm getting a sense of de-ja-vu here. These _don't exist_: #1533, #1661, #1690, #2237. > > Given they _used to_ exist, I have to ask, does this actually implement MSC4186 as merged, or does it implement a compatibility hybrid that will break when clients update to their next set of quirks next week, requiring another refactor? yes sorry i removed it i forgot to update the description my local matrix spec was out of date
Owner

Ah makes sense. Second comment still stands though, is this implemented per the latest revision of the MSC?

Ah makes sense. Second comment still stands though, is this implemented per the latest revision of the MSC?
Author
Contributor

@nex wrote in #2280 (comment):

Ah makes sense. Second comment still stands though, is this implemented per the latest revision of the MSC?

It implements MSC4186's server semantics as merged (connection state, room inclusion, required_state deltas and lazy-loading rules, left/kicked/banned rooms, extension scoping), but on the unstable org.matrix.simplified_msc3575 wire format. Because the sdk's still use the the unstable. When clients move to v4, supporting it is a request/response adapter over the same core, not another rewrite. The compatibility shims are documented in the code and limited to the unstable format: unread counts, sticky parameters, and conn_id-less subscriptions for js-sdk.

@nex wrote in https://forgejo.ellis.link/continuwuation/continuwuity/pulls/2280#issuecomment-36079: > Ah makes sense. Second comment still stands though, is this implemented per the latest revision of the MSC? It implements MSC4186's server semantics as merged (connection state, room inclusion, required_state deltas and lazy-loading rules, left/kicked/banned rooms, extension scoping), but on the unstable org.matrix.simplified_msc3575 wire format. Because the sdk's still use the the unstable. When clients move to v4, supporting it is a request/response adapter over the same core, not another rewrite. The compatibility shims are documented in the code and limited to the unstable format: unread counts, sticky parameters, and conn_id-less subscriptions for js-sdk.
@ -0,0 +68,4 @@
.update_device_last_seen(syncing_user, Some(syncing_device), client_ip)
.await;
// MSC3575's way of dropping subscriptions, which ruma does not model
Owner

What's this doing here? We don't care to support MSC3575.

What's this doing here? We don't care to support MSC3575.
Author
Contributor

removing it, it just that the javascript sdk still wait for some stuff for this MSC

removing it, it just that the javascript sdk still wait for some stuff for this MSC
eleboucher marked this conversation as resolved
Owner

okay awesome, works for me, thanks!

okay awesome, works for me, thanks!
eleboucher force-pushed fix/sync-v5-left-rooms from 20c7b32008
All checks were successful
Documentation / Build and Deploy Documentation (pull_request) Has been skipped
Checks / Prek / Check changed files (pull_request) Successful in 6s
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 8s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m18s
Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 7m22s
to 958e85cea2
All checks were successful
Documentation / Build and Deploy Documentation (pull_request) Has been skipped
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 7s
Checks / Prek / Check changed files (pull_request) Successful in 6s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m36s
Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 7m16s
2026-09-23 15:01:36 +00:00
Compare
eleboucher changed title from refactor(sync): Rewrite simplified sliding sync to fix(pusher): Unify retry backoff and fix proxied or LAN push gateways 2026-09-23 19:25:39 +00:00
eleboucher changed title from fix(pusher): Unify retry backoff and fix proxied or LAN push gateways to refactor(sync): Rewrite simplified sliding sync 2026-09-23 19:25:54 +00:00
eleboucher force-pushed fix/sync-v5-left-rooms from 958e85cea2
All checks were successful
Documentation / Build and Deploy Documentation (pull_request) Has been skipped
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 7s
Checks / Prek / Check changed files (pull_request) Successful in 6s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m36s
Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 7m16s
to 70061f680f
Some checks failed
Documentation / Build and Deploy Documentation (pull_request) Has been skipped
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 6s
Checks / Prek / Check changed files (pull_request) Successful in 7s
Update flake hashes / update-flake-hashes (pull_request) Successful in 1m24s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m50s
Checks / Prek / Clippy and Cargo Tests (pull_request) Has been cancelled
2026-09-28 06:18:26 +00:00
Compare
eleboucher force-pushed fix/sync-v5-left-rooms from 70061f680f
Some checks failed
Documentation / Build and Deploy Documentation (pull_request) Has been skipped
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 6s
Checks / Prek / Check changed files (pull_request) Successful in 7s
Update flake hashes / update-flake-hashes (pull_request) Successful in 1m24s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m50s
Checks / Prek / Clippy and Cargo Tests (pull_request) Has been cancelled
to 634d44c64e
Some checks failed
Documentation / Build and Deploy Documentation (pull_request) Has been skipped
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 7s
Checks / Prek / Check changed files (pull_request) Successful in 8s
Update flake hashes / update-flake-hashes (pull_request) Successful in 1m20s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 2m0s
Checks / Prek / Clippy and Cargo Tests (pull_request) Failing after 5m34s
2026-09-28 06:20:43 +00:00
Compare
Jade force-pushed fix/sync-v5-left-rooms from 634d44c64e
Some checks failed
Documentation / Build and Deploy Documentation (pull_request) Has been skipped
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 7s
Checks / Prek / Check changed files (pull_request) Successful in 8s
Update flake hashes / update-flake-hashes (pull_request) Successful in 1m20s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 2m0s
Checks / Prek / Clippy and Cargo Tests (pull_request) Failing after 5m34s
to 5efd5e9a11
Some checks failed
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 7s
Documentation / Build and Deploy Documentation (pull_request) Has been skipped
Checks / Prek / Check changed files (pull_request) Successful in 7s
Checks / Prek / Pre-commit & Formatting (pull_request) Failing after 1m13s
Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 10m18s
2026-10-04 22:31:43 +00:00
Compare
Jade force-pushed fix/sync-v5-left-rooms from 5efd5e9a11
Some checks failed
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 7s
Documentation / Build and Deploy Documentation (pull_request) Has been skipped
Checks / Prek / Check changed files (pull_request) Successful in 7s
Checks / Prek / Pre-commit & Formatting (pull_request) Failing after 1m13s
Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 10m18s
to da7a22c01e
All checks were successful
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 10s
Documentation / Build and Deploy Documentation (pull_request) Has been skipped
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m49s
Checks / Prek / Check changed files (pull_request) Successful in 7s
Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 10m28s
2026-10-04 23:40:05 +00:00
Compare
fix(sync): Wake sliding sync connections only for changes they report
All checks were successful
Documentation / Build and Deploy Documentation (pull_request) Has been skipped
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 6s
Checks / Prek / Check changed files (pull_request) Successful in 8s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m41s
Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 7m19s
3fd08aa593
eleboucher force-pushed fix/sync-v5-left-rooms from 3fd08aa593
All checks were successful
Documentation / Build and Deploy Documentation (pull_request) Has been skipped
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 6s
Checks / Prek / Check changed files (pull_request) Successful in 8s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m41s
Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 7m19s
to d99f9fa9fd
All checks were successful
Documentation / Build and Deploy Documentation (pull_request) Has been skipped
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 6s
Checks / Prek / Check changed files (pull_request) Successful in 8s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m47s
Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 7m17s
2026-10-05 19:31:01 +00:00
Compare
nex left a comment

Just clearing old partial review, pls ignore (why doesnt forgejo have a way to do this lol)

Just clearing old partial review, pls ignore (why doesnt forgejo have a way to do this lol)
Contributor

support for MSC4559 as is blocking impl. of the Presence v2 MSCs in SNext would be cool as well :)

support for [MSC4559](https://github.com/matrix-org/matrix-spec-proposals/pull/4559) as is blocking impl. of the Presence v2 MSCs in SNext would be cool as well :)
Owner

For the sake of getting this PR over the review line given our current limited bandwidth I think it's fair to ask that it doesn't have its scope increased - a follow-up PR is always welcome

For the sake of getting this PR over the review line given our current limited bandwidth I think it's fair to ask that it doesn't have its scope increased - a follow-up PR is always welcome
nex requested changes 2026-10-17 02:08:12 +00:00
nex left a comment

This PR is overall very complicated, poorly documented, and has a plethora of codestyle issues. Frankly, I think it might be a better idea to break this PR down into several smaller PRs so that changes can be merged incrementally, starting with the core sliding-sync implementation, and then one PR per extension.

Just for the record: this took me 2.5hrs to review

This PR is overall very complicated, poorly documented, and has a plethora of codestyle issues. Frankly, I think it might be a better idea to break this PR down into several smaller PRs so that changes can be merged incrementally, starting with the core sliding-sync implementation, and then one PR per extension. Just for the record: this took me 2.5hrs to review
@ -184,2 +220,4 @@
.await
}
/// `None` if nothing happened in the room since `count`.
Owner

This docstring doesn't actually describe anything about the function (the first sentence of a doc string at the very least should summarise what the function does)

This docstring doesn't actually describe anything about the function (the first sentence of a doc string at the very least should summarise what the function does)
@ -185,1 +221,4 @@
}
/// `None` if nothing happened in the room since `count`.
async fn shortstatehash_after(
Owner

nit: shortstatehash_after_count would be more a intuitive name

nit: `shortstatehash_after_count` would be more a intuitive name
@ -186,0 +252,4 @@
.map_err(|err| err!("PDU after count has no shortstatehash: {err}"))
}
/// `state_events` are the room's state changes in the range.
Owner

This docstring doesn't actually describe anything about the function (the first sentence of a doc string at the very least should summarise what the function does)

This docstring doesn't actually describe anything about the function (the first sentence of a doc string at the very least should summarise what the function does)
@ -0,0 +11,5 @@
use super::{SyncContext, scope, scope_update};
use crate::client::sync::v5::lists::RoomSelection;
/// MSC4548: a room entering the scope for the first time gets all of its
/// account data.
Owner

nit: references to an MSC should include a link to said MSC, either hyperlinked or referenced

nit: references to an MSC should include a link to said MSC, either hyperlinked or referenced
@ -0,0 +19,4 @@
v5::room::added_state,
};
/// MSC4543: device lists are only sent once the client has a position.
Owner

nit: references to an MSC should include a link to said MSC, either hyperlinked or referenced

nit: references to an MSC should include a link to said MSC, either hyperlinked or referenced
@ -0,0 +139,4 @@
.await
}
async fn room_device_list_changes(
Owner

How does this differ from mod.rs' function of the same name? (might be worth noting that in a comment or docstring)

How does this differ from mod.rs' function of the same name? (might be worth noting that in a comment or docstring)
@ -0,0 +65,4 @@
}
/// MSC4508: rooms in range of the named lists and the named subscriptions,
/// all of either by default.
Owner

nit: references to an MSC should include a link to said MSC, either hyperlinked or referenced

nit: references to an MSC should include a link to said MSC, either hyperlinked or referenced
@ -0,0 +66,4 @@
/// MSC4508: rooms in range of the named lists and the named subscriptions,
/// all of either by default.
fn scope(
Owner

What does this actually do? The docstring doesn't explain it very well and the function body itself is match soup

What does this actually do? The docstring doesn't explain it very well and the function body itself is match soup
@ -0,0 +104,16 @@
scope
}
/// A disabled extension forgets its scope, so enabling it again starts over.
fn scope_update(
context: SyncContext<'_>,
previous: &ExtensionScope,
enabled: bool,
scope: HashSet<OwnedRoomId>,
) -> Option<ExtensionScope> {
if !enabled {
return previous.enabled.then(ExtensionScope::default);
}
previous.advance(scope, context.since())
}
Owner

Does this need to be a function? it looks small enough to inline to me

Does this need to be a function? it looks small enough to inline to me
@ -0,0 +17,5 @@
room::{BuiltRooms, SentTimeline},
};
/// A room entering the scope only gets the receipts for its returned timeline
/// and the user's own, rather than one for every member.
Owner

This docstring is written more like a comment for a specific condition rather than describing what the function does

This docstring is written more like a comment for a specific condition rather than describing what the function does
@ -0,0 +92,7 @@
read_receipt
.readreceipts_since(room_id, Some(scan_from))
.ready_filter(|(read_user, count, receipt)| {
(!entering && *count > since)
|| (read_user != syncing_user
&& sent_timeline
.is_some_and(|timeline| receipts_any(receipt, &timeline.event_ids)))
Owner

this should probably be un-nested because it's hard to follow

this should probably be un-nested because it's hard to follow
@ -0,0 +22,5 @@
const DEFAULT_LIMIT: usize = 100;
/// MSC4480 covers every joined room a list or subscription matches, including
/// rooms outside the lists' ranges.
Owner

nit: references to an MSC should include a link to said MSC, either hyperlinked or referenced

This docstring also doesn't really describe what the function actually does

nit: references to an MSC should include a link to said MSC, either hyperlinked or referenced This docstring also doesn't really describe what the function actually does
@ -0,0 +71,4 @@
let found: Vec<_> = joined_rooms
.into_iter()
.stream()
.wide_then(async |room_id| {
Owner

nit: This closure is pretty long, it probably wants making into a real function

nit: This closure is pretty long, it probably wants making into a real function
@ -0,0 +164,4 @@
push_sticky_event(services, &mut response, syncing_user, room_id, pdu).await;
}
// MSC4480 requires it, and it keeps the next request from scanning again
Owner

nit: "it"?

nit: "it"?
@ -0,0 +211,4 @@
services: &Services,
context: SyncContext<'_>,
) -> (
Vec<(OwnedRoomId, MembershipState, Option<u64>)>,
Owner

nit: This tuple probably wants to be a named type

nit: This tuple probably wants to be a named type
@ -0,0 +300,4 @@
.map_or(MembershipState::Leave, |content| content.membership)
}
async fn candidate(
Owner

This looks like a complication function and should probably have a docstring explaining what it does

This looks like a complication function and should probably have a docstring explaining what it does
@ -0,0 +346,4 @@
}
}
/// Invalid `m.direct` data counts as no DMs rather than failing every sync.
Owner

This is probably more of a comment than a docstring

This is probably more of a comment than a docstring
@ -0,0 +385,5 @@
.is_some()
}
/// Stripped state is only recommended to carry `m.room.create`, so a room
/// without one is treated as untyped rather than hidden from every list.
Owner

what? Also I think this is supposed to be a comment(?) instead of a docstring

what? Also I think this is supposed to be a comment(?) instead of a docstring
@ -0,0 +417,6 @@
}
}
fn is_stripped(membership: &MembershipState) -> bool {
matches!(membership, MembershipState::Invite | MembershipState::Knock)
}
Owner

This doesn't need to be a function

This doesn't need to be a function
@ -0,0 +429,7 @@
event_type: &StateEventType,
) -> Option<Raw<AnyStrippedStateEvent>> {
let state_cache = &services.rooms.state_cache;
let state = match membership {
| MembershipState::Knock => state_cache.knock_state(user_id, room_id).await,
| _ => state_cache.invite_state(user_id, room_id).await,
};
Owner

This could be an if/else instead of match

This could be an if/else instead of match
@ -0,0 +444,5 @@
})
}
/// Rooms the user left are visible even if they never joined, e.g. after
/// rejecting an invite.
Owner

This docstring doesn't describe what this function actually does

This docstring doesn't describe what this function actually does
@ -0,0 +498,10 @@
let is_invite = candidate.membership == MembershipState::Invite;
let type_filtered = !filters.room_types.is_empty() || !filters.not_room_types.is_empty();
matches_bool_filter(candidate.is_dm, filters.is_dm)
&& matches_bool_filter(candidate.is_encrypted, filters.is_encrypted)
&& matches_bool_filter(is_invite, filters.is_invite)
&& (!type_filtered
|| candidate.room_type.as_ref().is_some_and(|room_type| {
matches_room_type(room_type, &filters.room_types, &filters.not_room_types)
}))
Owner

This is a really complication calculation and should probably be un-nested for readabilty

This is a really complication calculation and should probably be un-nested for readabilty
@ -0,0 +507,6 @@
}))
}
fn matches_bool_filter(value: bool, filter: Option<bool>) -> bool {
filter.is_none_or(|expected| value == expected)
}
Owner

This doesn't need to be a function

This doesn't need to be a function
@ -0,0 +511,11 @@
filter.is_none_or(|expected| value == expected)
}
fn matches_room_type(
room_type: &RoomTypeFilter,
room_types: &[RoomTypeFilter],
not_room_types: &[RoomTypeFilter],
) -> bool {
!not_room_types.contains(room_type)
&& (room_types.is_empty() || room_types.contains(room_type))
}
Owner

This doesn't need to be a function imo

This doesn't need to be a function imo
@ -0,0 +520,11 @@
&& (room_types.is_empty() || room_types.contains(room_type))
}
fn sort_by_activity(candidates: &mut [Candidate]) {
candidates.sort_unstable_by(|left, right| {
right
.activity
.cmp(&left.activity)
.then_with(|| left.room_id.cmp(&right.room_id))
});
}
Owner

Is this actually referenced in multiple places? It doesn't need to be a function if not

Is this actually referenced in multiple places? It doesn't need to be a function if not
@ -0,0 +529,4 @@
});
}
#[allow(clippy::single_range_in_vec_init, reason = "these are lists of index ranges")]
Owner

This should go below the docstring

This should go below the docstring
@ -0,0 +531,4 @@
#[allow(clippy::single_range_in_vec_init, reason = "these are lists of index ranges")]
/// The disjoint index ranges a list's inclusive `[start, end]` ranges cover.
/// MSC4186: without ranges the list holds every matching room.
Owner

nit: references to an MSC should include a link to said MSC, either hyperlinked or referenced

nit: references to an MSC should include a link to said MSC, either hyperlinked or referenced
@ -0,0 +22,4 @@
};
use crate::{Ruma, client_ip::ClientIp};
const MAX_TIMEOUT: Duration = Duration::from_secs(30);
Owner

Why 30 seconds? Is this actually specced?

Why 30 seconds? Is this actually specced?
@ -0,0 +27,4 @@
/// A response that takes this long to build is reported at `info`.
const SLOW_BUILD: Duration = Duration::from_millis(100);
/// MSC4186 allows at most this many lists and room subscriptions; the same
Owner

nit: references to an MSC should include a link to said MSC, either hyperlinked or referenced

nit: references to an MSC should include a link to said MSC, either hyperlinked or referenced
@ -0,0 +25,4 @@
const MAX_TIMEOUT: Duration = Duration::from_secs(30);
/// A response that takes this long to build is reported at `info`.
const SLOW_BUILD: Duration = Duration::from_millis(100);
Owner

100ms is a low threshold for large accounts 🤔

100ms is a low threshold for large accounts 🤔
@ -0,0 +121,15 @@
let (mut response, update) = build_response(services, context).await?;
// a changed list count is news even without room data, and clients
// replenish their one-time keys as soon as the counts move, and send
// encryption requests between syncs of a connection without rooms
let respond = !is_empty(&response)
|| update.list_counts.is_some()
|| match timeout_at(deadline, wake.woken()).await {
| Ok(Some(woken)) => {
debug!(conn_id = ?request.conn_id, ?woken, "sliding sync woken");
!has_rooms(&request) || woken.contains(&WakeKind::Keys)
},
| Ok(None) | Err(_) => true,
};
Owner

This is a complicated calculation that could probably just do with being split up

This is a complicated calculation that could probably just do with being split up
@ -0,0 +172,6 @@
.collect()
}
fn has_rooms(request: &v5::Request) -> bool {
!request.lists.is_empty() || !request.room_subscriptions.is_empty()
}
Owner

This doesn't need to be a function

This doesn't need to be a function
@ -0,0 +260,4 @@
/// `phases` are the elapsed times after selecting rooms, building them and
/// building the extensions.
fn log_build(context: SyncContext<'_>, rooms: usize, phases: [Duration; 3]) {
Owner

Given this is only interesting for debug builds, it might be a good idea to feature-gate this behind debug_assertions

Given this is only interesting for debug builds, it might be a good idea to feature-gate this behind debug_assertions
@ -0,0 +170,4 @@
.await
}
/// `None` if the room must be sent in full.
Owner

This isn't a helpful docstring

This isn't a helpful docstring
@ -0,0 +172,4 @@
/// `None` if the room must be sent in full.
fn since(previous: Option<&RoomState>, selected: &SelectedRoom) -> Option<u64> {
let previous = previous?;
Owner

Why not just make this function take an &RoomState and just not call it if the value would be None?

Why not just make this function take an &RoomState and just not call it if the value would be None?
@ -0,0 +182,4 @@
(comparable && !selected.config.expands(&previous.config)).then_some(previous.sent_at)
}
fn room_update(
Owner

Vague function name lacking a docstring

Vague function name lacking a docstring
@ -0,0 +375,4 @@
if join_pdu
.get_content::<RoomMemberEventContent>()
.is_ok_and(|content| content.membership != MembershipState::Join)
Owner

I think if this fails to deserialise, the user is implicitly just allowed to see history since the bad membership event?

I think if this fails to deserialise, the user is implicitly just allowed to see history since the bad membership event?
@ -0,0 +396,9 @@
build_timeline_room(services, context, room_id, config, since, view).await
}
fn leave_only(since: Option<u64>, leave_pdu: PduEvent) -> v5::response::Room {
assign!(v5::response::Room::new(), {
initial: Some(since.is_none()),
required_state: vec![leave_pdu.into_format()],
})
}
Owner

I don't think this needs to be a function

I don't think this needs to be a function
@ -0,0 +531,4 @@
required_state.push(leave);
}
// clients drop their pagination token without one
Owner

without one what?

without one what?
@ -163,3 +163,4 @@
type Err = Error;
fn from_str(token: &str) -> Result<Self, Self::Err> { Ok(Self::from_signed(token.parse()?)) }
/// Also accepts a sliding sync `pos`, which MSC4186 lets clients paginate
/// from; its count comes before the `_`.
Owner

This docstring seems partial/incomplete?

This docstring seems partial/incomplete?
@ -0,0 +10,4 @@
use super::RoomConfig;
/// Responses a client may still acknowledge after it was sent a newer one.
const MAX_PENDING: usize = 8;
Owner

why eight? I thought the proposal said this was immediate?

why eight? I thought the proposal said this was immediate?
@ -0,0 +73,5 @@
pub typing: Option<ExtensionScope>,
}
/// The sequence number keeps a retried request from committing a response the
/// client never received, even when the stream count has not moved.
Owner

Shouldn't this be on the doc for the seq property?

Shouldn't this be on the doc for the seq property?
@ -0,0 +154,4 @@
}
}
/// Returns false unless `pos` is current or still pending.
Owner

What does this function do though? (bad docstring)

What does this function do though? (bad docstring)
@ -0,0 +6,5 @@
const MAX_TIMELINE_LIMIT: usize = 100;
const WILDCARD: &str = "*";
const LAZY: &str = "$LAZY";
const ME: &str = "$ME";
Owner

These were removed ages ago, I thought you said this was built based on the merged spec proposal?

These were removed ages ago, I thought you said this was built based on the merged spec proposal?
@ -0,0 +65,8 @@
let event_type = (event_type.to_string() != WILDCARD).then(|| event_type.clone());
let state_key = match state_key.as_str() {
| WILDCARD => StateKeyPattern::Any,
| ME => StateKeyPattern::Me,
| LAZY if event_type == Some(StateEventType::RoomMember) => {
this.lazy_members = true;
continue;
},
Owner

Again these don't exist

Again these don't exist
All checks were successful
Documentation / Build and Deploy Documentation (pull_request) Has been skipped
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 6s
Required
Details
Checks / Prek / Check changed files (pull_request) Successful in 8s
Required
Details
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m47s
Required
Details
Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 7m17s
Required
Details
This pull request has changes conflicting with the target branch.
  • src/service/rooms/read_receipt/mod.rs
  • src/service/rooms/typing/mod.rs
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u fix/sync-v5-left-rooms:eleboucher-fix/sync-v5-left-rooms
git switch eleboucher-fix/sync-v5-left-rooms
Sign in to join this conversation.
No reviewers
nex
No milestone
No assignees
5 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
continuwuation/continuwuity!2280
No description provided.