WIP: Fix rapid empty sync responses #841

nex · 2025-05-26T01:34:28Z

nex commented

2025-05-26 01:34:28 +00:00

This PR fixes the issue where /sync returns immediately with no valuable data, yet still increments the sync token.

Currently, this fixes the issue where the sync timeout does not default properly. All that needs to be done before this is ready for merge, is preventing the sync token from incrementing (which also coincidentally brings us in line with the Synapse behaviour)

This PR fixes the issue where /sync returns immediately with no valuable data, yet still increments the sync token. Currently, this fixes the issue where the sync timeout does not default properly. All that needs to be done before this is ready for merge, is preventing the sync token from incrementing (which also coincidentally brings us in line with the Synapse behaviour)

nex added this to the 0.5.0-old milestone

2025-05-26 01:34:28 +00:00

nex added the

Bug

Matrix/Client

labels

2025-05-26 01:34:28 +00:00

nex added 1 commit

2025-05-26 01:34:29 +00:00

force 0ns timeouts to default appropriately in syncv3

Release Docker Image / define-variables (push) Successful in 12s

Details

Rust Checks / Format (push) Successful in 32s

Details

Documentation / Build and Deploy Documentation (pull_request) Successful in 31s

Details

Rust Checks / Clippy (push) Failing after 3m53s

Details

Rust Checks / Cargo Test (push) Successful in 4m53s

Details

Release Docker Image / build-image (linux/amd64, linux-amd64) (push) Failing after 14m9s

Details

Release Docker Image / build-image (linux/arm64, linux-arm64) (push) Failing after 15m27s

Details

Release Docker Image / merge (push) Has been skipped

Details

cdbe5d52c2

nex reviewed

2025-05-26 01:35:01 +00:00

src/api/client/sync/v3.rs

					
				@ -135,3 +135,3 @@

					let watcher = services.sync.watch(sender_user, sender_device);

					let response = build_sync_events(&services, &body).await?;

					trace!("sync body.body.full_state: {}", body.body.full_state);

These traces are temporary, probably not necessary now. Remove before merge.

Jade requested changes

2025-05-26 13:05:17 +00:00

src/api/client/sync/v3.rs

					
				@ -150,3 +156,1 @@

					let default = Duration::from_secs(30);

					let duration = cmp::min(body.body.timeout.unwrap_or(default), default);

					_ = tokio::time::timeout(duration, watcher).await;

					let default_timeout = Duration::from_secs(30);

Set this as a constant at the top of the file if possible

👍 1

src/api/client/sync/v3.rs

					
				@ -153,0 +157,4 @@

					let user_timeout = body.body.timeout.unwrap_or(default_timeout);

					trace!("User timeout: {user_timeout:?}, default timeout: {default_timeout:?}");

					// If the user default resolves to 0ns or less, use the default timeout

					let timeout = if user_timeout <= Duration::from_secs(0) {

Perhaps we also want a minimum timeout? Then doing a min on those values. On the other hand, a timeout of 0 could be legitimate

Perhaps we also want a minimum timeout? Then doing a min on those values. On the other hand, a timeout of 0 *could* be legitimate

Was the bug here that not setting a value gave a timeout of 0?

The bug is that body.body.timeout.unwrap_or(default_timeout) was not using the default timeout, but instead unwrapping to zero

Now that I've slept I've also remembered that a timeout of zero means "return immediately", so technically it is actually valid. I should figure out why the unwrap doesn't work as expected instead of this

The bug is that `body.body.timeout.unwrap_or(default_timeout)` was not using the default timeout, but instead unwrapping to zero Now that I've slept I've also remembered that [a timeout of zero means "return immediately"](https://spec.matrix.org/v1.14/client-server-api/#get_matrixclientv3sync), so technically it is actually valid. I should figure out why the unwrap doesn't work as expected instead of this

@Jade wrote in #841 (comment):

Perhaps we also want a minimum timeout? Then doing a min on those values. On the other hand, a timeout of 0 could be legitimate

We might want to special-case zero, but have an actual minimum timeout otherwise. Not sure how useful that'll be though

@Jade wrote in https://forgejo.ellis.link/continuwuation/continuwuity/pulls/841#issuecomment-16019: > Perhaps we also want a minimum timeout? Then doing a min on those values. On the other hand, a timeout of 0 _could_ be legitimate We might want to special-case zero, but have an actual minimum timeout otherwise. Not sure how useful that'll be though

nex commented

2025-05-26 14:18:34 +00:00

Looks like the fix is a red herring, the issue is actually the timeout simply being skipped as the watcher already claims it has data, when it in fact does not

Jade commented

2025-05-26 14:22:29 +00:00

Yeah that's what I was trying to say last night - Related to #652/files
but the way to 'fix' this is to either use a loop to keep checking or to ensure that we never trigger the watcher unnecessarily. Actual solution is probably a combination of both, with some debugging tools to figure out what's triggering the watcher unnecessarily.

Yeah that's what I was trying to say last night - Related to https://forgejo.ellis.link/continuwuation/continuwuity/pulls/652/files but the way to 'fix' this is to either use a loop to keep checking or to ensure that we never trigger the watcher unnecessarily. Actual solution is probably a combination of both, with some debugging tools to figure out what's triggering the watcher unnecessarily.

❤️ 1

Aranjedeath commented

2025-05-26 14:25:15 +00:00

the only non-zero thing in the sync responses is the one-time-key count, is that causing it?

nex commented

2025-05-26 14:25:26 +00:00

We can't really do a loop here in an efficient manner while also respecting the timeout. When I get some more time, I'll go through and properly trace the watcher

nex commented

2025-05-26 14:26:44 +00:00

@Aranjedeath wrote in #841 (comment):

the only non-zero thing in the sync responses is the one-time-key count, is that causing it?

No, the issue is this function returning prematurely, essentially. Need to figure out why specifically, since it doesn't actually return anything, meaning it's a bit tricker to see what's caused it to return in the first place

@Aranjedeath wrote in https://forgejo.ellis.link/continuwuation/continuwuity/pulls/841#issuecomment-16027: > the only non-zero thing in the sync responses is the one-time-key count, is that causing it? No, the issue is [this function](https://forgejo.ellis.link/continuwuation/continuwuity/src/commit/d8311a5ff672fdc4729d956af5e3af8646b0670d/src/service/sync/watch.rs#L7) returning prematurely, essentially. Need to figure out *why* specifically, since it doesn't actually return anything, meaning it's a bit tricker to see what's caused it to return in the first place

nex commented

2025-06-28 19:07:18 +00:00

This doesn't fix the issue, a followup PR will. Closing this for now.

👍 1

nex

2025-06-28 19:07:18 +00:00

closed this pull request
added the
Status
Invalid
label

Release Docker Image / define-variables (push) Successful in 12s

Details

Rust Checks / Format (push) Successful in 32s

Details

Documentation / Build and Deploy Documentation (pull_request) Successful in 31s

Details

Rust Checks / Clippy (push) Failing after 3m53s

Details

Rust Checks / Cargo Test (push) Successful in 4m53s

Details

Release Docker Image / build-image (linux/amd64, linux-amd64) (push) Failing after 14m9s

Details

Release Docker Image / build-image (linux/arm64, linux-arm64) (push) Failing after 15m27s

Details

Release Docker Image / merge (push) Has been skipped

Details

Checks / *

Required

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

No reviewers

No milestone

No project

No assignees

3 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!841

Rows
Columns