Better assert when TestDelayedEvents/delayed_state_events_are_kept_on_server_restart take too long to run for our assumptions to be correct
#829
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MadLittleMods
commented
Dec 17, 2025
| "Test took too long to run, cannot guarantee delayed event timings. " + | ||
| "More than 10 seconds elapsed between scheduling the delayed event and now when we're about to check for it.", | ||
| ) | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Actually fixing this and making this kind of test consistent is tricky.
If we just increase the delay time, then the test will just take longer to run even in the fast scenarios. Ideally, the second delayed event should be delayed by at-least COMPLEMENT_SPAWN_HS_TIMEOUT_SECS (defaults to 30 seconds) since we're stopping and starting servers here 🤔 - although we use COMPLEMENT_SPAWN_HS_TIMEOUT_SECS=120 with the Synapse workers variant and two minutes sounds painful to wait for.
Collaborator
Author
There was a problem hiding this comment.
We could setup a series of delayed events at 10 second intervals up to the COMPLEMENT_SPAWN_HS_TIMEOUT_SECS. Then we just want to look for a decrement after the server restart. This keeps the test as fast as before while being lenient for however long it takes to restart.
I'm going to address this in a follow-up PR.
MadLittleMods
changed the title
TestDelayedEvents take too long to run for our assumptions to be incorrectTestDelayedEvents take too long to run for our assumptions to be correct
MadLittleMods
changed the title
TestDelayedEvents take too long to run for our assumptions to be correctTestDelayedEvents/delayed_state_events_are_kept_on_server_restart take too long to run for our assumptions to be correct
1 task
devonh
approved these changes
Dec 19, 2025
Co-authored-by: Devon Hudson <devon.dmytro@gmail.com>
Collaborator
Author
|
Thanks for the review @devonh 🦘 |
MadLittleMods
added a commit
that referenced
this pull request
Dec 22, 2025
…t` to account for slow servers (de-flake) (#830) Follow-up to #829 Part of element-hq/synapse#18537 ### What does this PR do? Fix `TestDelayedEvents/delayed_state_events_are_kept_on_server_restart` to account for slow servers (de-flake). Previously, this was naively using a single delayed event with a 10 second delay. But because we're stopping and starting servers here, it could take up `deployment.GetConfig().SpawnHSTimeout` (defaults to 30 seconds) for the server to start up again so by the time the server is back up, the delayed event may have already been sent, invalidating our assertions below (which expect some delayed events to still be pending and then see one of them be sent after the server is back up). We could account for this by setting the delayed event delay to be longer than `deployment.GetConfig().SpawnHSTimeout` but that would make the test suite take longer to run in all cases even for homeservers that are quick to restart because we have to wait for that large delay. We instead account for this by scheduling many delayed events at short intervals (we chose 10 seconds because that's what the test naively chose before). Then whenever the servers comes back, we can just check until it decrements by 1. ### Experiencing the flaky failure As experienced when running this test against the worker-based Synapse setup we use alongside the Synapse Pro Rust apps, element-hq/synapse-rust-apps#344 (comment). We probably experience this heavily in the private project because GitHub runners are less than half as powerful as those for public projects and that single container with a share of the 2 CPU cores available is just not powerful enough to run all 16 workers effectively. <details> <summary>For reference, the CI runners provided by GitHub for private projects are less than half as powerful as those for public projects.</summary> > #### Standard GitHub-hosted runners for public repositories > > Virtual machine | Processor (CPU) | Memory (RAM) | Storage (SSD) | Architecture | Workflow label > --- | --- | --- | --- | --- | --- > Linux | 4 | 16 GB | 14 GB | x64 | ubuntu-latest, ubuntu-24.04, ubuntu-22.04 > > *-- [Standard GitHub-hosted runners for public repositories](https://docs.github.com/en/actions/reference/runners/github-hosted-runners#standard-github-hosted-runners-for-public-repositories)* --- > #### Standard GitHub-hosted runners for private repositories > > Virtual Machine | Processor (CPU) | Memory (RAM) | Storage (SSD) | Architecture | Workflow label > --- | --- | --- | --- | --- | --- > Linux | 2 | 7 GB | 14 GB | x64 | ubuntu-latest, ubuntu-24.04, ubuntu-22.04 > > *-- [Standard GitHub-hosted runners for private repositories](https://docs.github.com/en/actions/reference/runners/github-hosted-runners#standard-github-hosted-runners-for-public-repositories)* </details> And for the same slow reasons, why we're also experiencing this as an occasional [flake](element-hq/synapse#18537) with `(workers, postgres)` in the public Synapse CI as well. ### Reproduction instructions I can easily reproduce this problem if I use #827 to limit the number of CPU's available for the homeserver containers to use: `COMPLEMENT_CONTAINER_CPUS=0.5`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Better assert when
TestDelayedEvents/delayed_state_events_are_kept_on_server_restarttake too long to run for our assumptions to be correct.As experienced when running this test against the worker-based Synapse setup we use alongside the Synapse Pro Rust apps, https://github.com/element-hq/synapse-rust-apps/pull/344#discussion_r2620714306. We probably experience this heavily in the private project because GitHub runners are less than half as powerful as those for public projects and that single container with a share of the 2 CPU cores available is just not powerful enough to run all 16 workers effectively.
For reference, the CI runners provided by GitHub for private projects are less than half as powerful as those for public projects.
And for the same slow reasons, why we're also experiencing this as an occasional flake with
(workers, postgres)in the public Synapse CI as well.Reproduction instructions
I can easily reproduce this problem if I use #827 to limit the number of CPU's available for the homeserver containers to use:
COMPLEMENT_CONTAINER_CPUS=0.5Pull Request Checklist