Repository navigation
Conversation
Signed-off-by: Shane Jaroch <chown_tee@proton.me>
| fromToken = endTokenRes.Str | ||
|
|
||
| if seenTokens[fromToken] { | ||
| t.Fatalf("homeserver returned a pagination token (%s) we've already seen -- it appears to be stuck in loop or repeating elements", fromToken) |
There was a problem hiding this comment.
I think it's valid for a homeserver to return the same pagination token if it doesn't have anything to return yet (still backfilling).
(we should get rid of this check)
There was a problem hiding this comment.
I agree it's allowed in the spec. However, having run this test against Synapse over 200 times and never seen a failure in this test, I would question what potential value allowing duplicate tokens has? Can you defend that?
Given the overwhelming empirical evidence that no homeserver duplicates tokens under this test, it seems a reasonable enforcement. (That could be totally invalid in other tests, but it strongly appears to hold here.)
My complement run failed for an unrelated flake:
❌ TestRoomSummaryAllowedRoomIDs/restricted_room_includes_allowed_room_ids (0s)
room_summary_test.go:60: CSAPI.MustDo GET http://127.0.0.1:32784/_matrix/client/v1/room_summary/%21HrEM4o3rDaGUCMUP:hs1 returned non-2xx code: 404 Not Found - body: {"errcode":"M_UNRECOGNIZED","error":"Unrecognized request"}
There was a problem hiding this comment.
@MadLittleMods Since you haven't replied in over two months, I'm not sure your response to the fact that "Synapse never does it"
I'm not particularly concerned how the check is maintained upstream. Happy to let you guys do it your way 👍
There was a problem hiding this comment.
@gamesguru Please re-request review if this is ready to look at again
| maxIterations := testCase.numberOfMessagesToSend + 1 | ||
| for iteration := 0; ; iteration++ { | ||
| if iteration >= maxIterations { | ||
| t.Fatalf("paginated %d times without reaching the start of the room (no `end` token) -- homeserver may be stuck in a pagination loop", iteration) | ||
| } |
There was a problem hiding this comment.
Having maxIterations can make sense 👍
Co-authored-by: Eric Eastwood <madlittlemods@gmail.com>
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
Fixes a couple 1 hour deadlock scenarios observed in Complement during a federation/timeline refactor of homeserver code.
Enhances checks and emits more helpful failure messages.
Additionally asserts no element is served twice (lessening the chances of future regressions or TOCTOU bugs).
Pull Request Checklist