Skip to content

matrix-synapse: fix test errors#371815

Merged
Lassulus merged 2 commits intoNixOS:masterfrom
Mic92:matrix-synapse
Jan 12, 2025
Merged

matrix-synapse: fix test errors#371815
Lassulus merged 2 commits intoNixOS:masterfrom
Mic92:matrix-synapse

Conversation

@Mic92
Copy link
Member

@Mic92 Mic92 commented Jan 7, 2025

Closes #369303

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@Mic92 Mic92 requested a review from mweinelt January 7, 2025 14:11
@github-actions github-actions bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jan 7, 2025
@Mic92
Copy link
Member Author

Mic92 commented Jan 7, 2025

Still seeing one other error:

[web01] matrix-synapse> [ERROR]
[web01] matrix-synapse> Traceback (most recent call last):
[web01] matrix-synapse>   File "/nix/store/60yi01wql4dnn1fwzk6ivp9d4h4yqy4y-python3.12-twisted-24.11.0/lib/python3.12/site-packages/twisted/logger/_observer.py", line 81, in __call__
[web01] matrix-synapse>     observer(event)
[web01] matrix-synapse>   File "/nix/store/60yi01wql4dnn1fwzk6ivp9d4h4yqy4y-python3.12-twisted-24.11.0/lib/python3.12/site-packages/twisted/logger/_legacy.py", line 90, in __call__
[web01] matrix-synapse>     self.legacyObserver(event)
[web01] matrix-synapse>   File "/nix/store/60yi01wql4dnn1fwzk6ivp9d4h4yqy4y-python3.12-twisted-24.11.0/lib/python3.12/site-packages/twisted/trial/_dist/workertrial.py", line 44, in emit
[web01] matrix-synapse>     self.protocol.callRemote(managercommands.TestWrite, out=text)
[web01] matrix-synapse>   File "/nix/store/60yi01wql4dnn1fwzk6ivp9d4h4yqy4y-python3.12-twisted-24.11.0/lib/python3.12/site-packages/twisted/protocols/amp.py", line 951, in callRemote
[web01] matrix-synapse>     return co._doCommand(self)
[web01] matrix-synapse>   File "/nix/store/60yi01wql4dnn1fwzk6ivp9d4h4yqy4y-python3.12-twisted-24.11.0/lib/python3.12/site-packages/twisted/protocols/amp.py", line 1973, in _doCommand
[web01] matrix-synapse>     d = proto._sendBoxCommand(
[web01] matrix-synapse>   File "/nix/store/60yi01wql4dnn1fwzk6ivp9d4h4yqy4y-python3.12-twisted-24.11.0/lib/python3.12/site-packages/twisted/protocols/amp.py", line 884, in _sendBoxCommand
[web01] matrix-synapse>     box._sendTo(self.boxSender)
[web01] matrix-synapse>   File "/nix/store/60yi01wql4dnn1fwzk6ivp9d4h4yqy4y-python3.12-twisted-24.11.0/lib/python3.12/site-packages/twisted/protocols/amp.py", line 713, in _sendTo
[web01] matrix-synapse>     proto.sendBox(self)
[web01] matrix-synapse>   File "/nix/store/60yi01wql4dnn1fwzk6ivp9d4h4yqy4y-python3.12-twisted-24.11.0/lib/python3.12/site-packages/twisted/protocols/amp.py", line 2358, in sendBox
[web01] matrix-synapse>     self.transport.write(box.serialize())
[web01] matrix-synapse>   File "/nix/store/60yi01wql4dnn1fwzk6ivp9d4h4yqy4y-python3.12-twisted-24.11.0/lib/python3.12/site-packages/twisted/protocols/amp.py", line 692, in serialize
[web01] matrix-synapse>     raise TooLong(False, True, v, k)
[web01] matr
[web01] ix-synapse> twisted.protocols.amp.TooLong:
[web01] matrix-synapse> tests.storage.databases.main.test_events_worker.DatabaseOutageTestCase.test_recovery

@naturallaw777

This comment was marked as duplicate.

@NickCao
Copy link
Member

NickCao commented Jan 7, 2025

Shall we wait until everything is fixed? I mean this patch is already part of 1.122.0rc1 so we would be dropping it very soon.

@SuperSandro2000
Copy link
Member

for reference element-hq/synapse#17878 (comment)

@wegank wegank added 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. 12.approvals: 1 This PR was reviewed and approved by one person. labels Jan 7, 2025
@naturallaw777
Copy link

My bad for posting the duplicate. I guess I did not see the previous comment. Anyways, thanks again for all your hard work to get this resolved!

@lucasew

This comment was marked as outdated.

@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one person. label Jan 8, 2025
@Mic92
Copy link
Member Author

Mic92 commented Jan 8, 2025

This now works for me.

@chvp
Copy link
Member

chvp commented Jan 8, 2025

I've also been able to deploy with this patch and the one from #369859, but it seems like a dangerous solution to me. The test seems to be related to database recovery. If the actual database recovery functionality is broken as well, I don't know whether it would be a good idea to ship this.

@TheArcaneBrony
Copy link
Member

TheArcaneBrony commented Jan 9, 2025

@chvp for clarity: the test is about recovering from a database outage, not recovering the database itself. Mainly related to fetching events when the db is down.

Test case (third party repo because it was what showed up looking for the test with google): https://mau.dev/leytilera/synapse/-/blob/v1.66.0/tests/storage/databases/main/test_events_worker.py#L197

Edit: just double checked, seems this test has not changed upstream since: https://github.com/element-hq/synapse/blob/HEAD/tests/storage/databases/main/test_events_worker.py#L345

@wegank wegank added the 12.approvals: 2 This PR was reviewed and approved by two persons. label Jan 10, 2025
@naturallaw777
Copy link

I just ran your flake @Mic92 and it works for me too. Thank you for adding in this patch.

@fauxmight
Copy link
Contributor

I just ran your flake @Mic92 and it works for me too. Thank you for adding in this patch.

@naturallaw777 I have beat my head against this for hours today trying to manually modify my own flake with overrides/overlays incorporating the changes from @Mic92 and have had absolutely no effect whatsoever.

Where is the flake you are referencing?

@Lassulus Lassulus merged commit 4046a28 into NixOS:master Jan 12, 2025
31 of 34 checks passed
@naturallaw777
Copy link

I just ran your flake @Mic92 and it works for me too. Thank you for adding in this patch.

@naturallaw777 I have beat my head against this for hours today trying to manually modify my own flake with overrides/overlays incorporating the changes from @Mic92 and have had absolutely no effect whatsoever.

Where is the flake you are referencing?

It has now been merged to the master branch. You should now be good to go for now.

@NickCao
Copy link
Member

NickCao commented Jan 12, 2025

I've got a "twisted" patch that actually fixes the test:

--- a/tests/storage/databases/main/test_events_worker.py
+++ b/tests/storage/databases/main/test_events_worker.py
@@ -372,7 +372,7 @@ class DatabaseOutageTestCase(unittest.HomeserverTestCase):
         )

         self.event_ids: List[str] = []
-        for idx in range(1, 21):  # Stream ordering starts at 1.
+        for idx in range(1, 11):  # Stream ordering starts at 1.
             event_json = {
                 "type": f"test {idx}",
                 "room_id": self.room_id,

My assumption was that the failed tasks created a backtrace that's too long to be serialized with the twisted internal amp protocol, thus reducing the number of events, thus tasks, brings it back into the limit.

@fauxmight
Copy link
Contributor

@naturallaw777 Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 12.approvals: 2 This PR was reviewed and approved by two persons. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Build failure: matrix-synapse (unwrapped)