Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Complement Adding a push rule wakes up an incremental /sync is consistently failing #14456

Closed
DMRobertson opened this issue Nov 15, 2022 · 6 comments · Fixed by #14468
Closed
Assignees
Labels
O-Frequent Affects or can be seen by most users regularly or impacts most users' first experience S-Major Major functionality / product severely impaired, no satisfactory workaround. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.

Comments

@DMRobertson
Copy link
Contributor

and it's irritating me.

https://github.com/matrix-org/synapse/actions/runs/3472783471/jobs/5804115166 e.g.

From early debugging it looks

  • adding the push rule correctly wakes up a user_stream
  • the user_stream calls its callback to generate a SyncResult
  • we fetch global_account_data and get {} for our troubles
  • presumably it is supposed to be a nonempty dict?
@DMRobertson
Copy link
Contributor Author

"Flakey" might mean that Synapse is broken.

The tests are about a month old (matrix-org/complement#488).

@DMRobertson
Copy link
Contributor Author

Running a git bisect points the finger at #14376

@DMRobertson DMRobertson changed the title Complement Adding a push rule wakes up an incremental /sync is flakey Complement Adding a push rule wakes up an incremental /sync is consistently failing Nov 16, 2022
@DMRobertson
Copy link
Contributor Author

DMRobertson commented Nov 16, 2022

I'm using COMPLEMENT_DIR=../complement scripts-dev/complement.sh -run 'TestPushSync' to reproduce this.

Edit: or the slightly fancier

SYNAPSE_TEST_LOG_LEVEL=DEBUG COMPLEMENT_DIR=../complement scripts-dev/complement.sh -run 'TestPushSync' -json | gotestfmt | tee test | less

@DMRobertson
Copy link
Contributor Author

I think that

changed = self._account_data_stream_cache.has_entity_changed(
user_id, int(stream_id)
)
is now False when it should have been True.

@DMRobertson
Copy link
Contributor Author

DMRobertson commented Nov 16, 2022

According to Erik, this condition

if self._is_writer:

should be if not self._is_writer. If I make that change, the test passes (without the revert in #14463). cc @Fizzadar

I'll see if I can hack together a unit test which would detect this.

@Fizzadar
Copy link
Contributor

Fizzadar commented Nov 16, 2022

Aha great spot @DMRobertson! Wild that this wasn't picked up in the tests earlier! EDIT: thank you both!

@erikjohnston erikjohnston added S-Major Major functionality / product severely impaired, no satisfactory workaround. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. O-Frequent Affects or can be seen by most users regularly or impacts most users' first experience labels Nov 16, 2022
DMRobertson pushed a commit that referenced this issue Nov 16, 2022
DMRobertson pushed a commit that referenced this issue Nov 16, 2022
DMRobertson pushed a commit that referenced this issue Nov 16, 2022
* Add tests for StreamIdGenerator

* Drive-by: annotate all defs

* Revert "Revert "Remove slaved id tracker (#14376)" (#14463)"

This reverts commit d63814f, which in
turn reverted 36097e8. This restores
the latter.

* Fix StreamIdGenerator not handling unpersisted IDs

Spotted by @erikjohnston.

Closes #14456.

* Changelog

Co-authored-by: Nick Mills-Barrett <nick@fizzadar.com>
Co-authored-by: Erik Johnston <erik@matrix.org>
H-Shay pushed a commit that referenced this issue Dec 13, 2022
* Add tests for StreamIdGenerator

* Drive-by: annotate all defs

* Revert "Revert "Remove slaved id tracker (#14376)" (#14463)"

This reverts commit d63814f, which in
turn reverted 36097e8. This restores
the latter.

* Fix StreamIdGenerator not handling unpersisted IDs

Spotted by @erikjohnston.

Closes #14456.

* Changelog

Co-authored-by: Nick Mills-Barrett <nick@fizzadar.com>
Co-authored-by: Erik Johnston <erik@matrix.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
O-Frequent Affects or can be seen by most users regularly or impacts most users' first experience S-Major Major functionality / product severely impaired, no satisfactory workaround. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants