Guard against homeservers stuck in a /messages pagination loop - #903
Open
gamesguru wants to merge 1 commit into
Open
Guard against homeservers stuck in a /messages pagination loop#903gamesguru wants to merge 1 commit into
/messages pagination loop#903gamesguru wants to merge 1 commit into
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) |
Contributor
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)
Author
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"}
Comment on lines
+436
to
+440
| 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) | ||
| } |
Contributor
There was a problem hiding this comment.
Having maxIterations can make sense 👍
Comment on lines
+430
to
+434
| // Guard against a buggy homeserver that never terminates pagination (e.g. it | ||
| // keeps returning an `end` token, possibly oscillating between a small set of | ||
| // tokens like `[-1, 1]` -> `[1, -1]` -> ...). Without this, the test would spin | ||
| // until the overall test timeout (which can be as long as 3600s) instead of | ||
| // failing fast with a useful error. |
Contributor
There was a problem hiding this comment.
Suggested change
| // Guard against a buggy homeserver that never terminates pagination (e.g. it | |
| // keeps returning an `end` token, possibly oscillating between a small set of | |
| // tokens like `[-1, 1]` -> `[1, -1]` -> ...). Without this, the test would spin | |
| // until the overall test timeout (which can be as long as 3600s) instead of | |
| // failing fast with a useful error. | |
| // Guard against a buggy homeserver that never terminates pagination (e.g. it keeps | |
| // returning an `end` token). Without this, the test could spin until the overall test | |
| // timeout instead of failing fast with a useful error. | |
| // | |
| // We chose `testCase.numberOfMessagesToSend + 1` as it allows the homeserver to | |
| // return events one by one with an extra request for good measure (no more events). |
| // failing fast with a useful error. | ||
| seenTokens := map[string]bool{fromToken: true} | ||
| maxIterations := testCase.numberOfMessagesToSend + 1 | ||
| for iteration := 0; ; iteration++ { |
Contributor
There was a problem hiding this comment.
Suggested change
| for iteration := 0; ; iteration++ { | |
| for paginationAttempt := 0; ; paginationAttempt++ { |
| // until the overall test timeout (which can be as long as 3600s) instead of | ||
| // failing fast with a useful error. | ||
| seenTokens := map[string]bool{fromToken: true} | ||
| maxIterations := testCase.numberOfMessagesToSend + 1 |
Contributor
There was a problem hiding this comment.
Suggested change
| maxIterations := testCase.numberOfMessagesToSend + 1 | |
| maxPaginationAttempts := testCase.numberOfMessagesToSend + 1 |
| 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) |
Contributor
There was a problem hiding this comment.
Suggested change
| t.Fatalf("paginated %d times without reaching the start of the room (no `end` token) -- homeserver may be stuck in a pagination loop", iteration) | |
| t.Fatalf( | |
| "paginated %d times without reaching the start of the room (no `end` token) "+ | |
| "(saw %d out of %d expected events) -- homeserver may be stuck in a pagination loop", | |
| iteration, | |
| len(filterEventIDs(t, actualEventIDsFromRequest, eventIDs)), | |
| len(eventIDs), | |
| ) |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
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