Skip to content

Guard against homeservers stuck in a /messages pagination loop #903

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gamesguru wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Guard against homeservers stuck in a /messages pagination loop #903

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion tests/csapi/room_messages_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,18 @@ func _sendAndTestMessageHistory(
testCase.numberOfMessagesToSend,
)
fromToken := ""
for {
// 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.
Comment on lines +430 to +434

@MadLittleMods MadLittleMods Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

seenTokens := map[string]bool{fromToken: true}
maxIterations := testCase.numberOfMessagesToSend + 1

@MadLittleMods MadLittleMods Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
maxIterations := testCase.numberOfMessagesToSend + 1
maxPaginationAttempts := testCase.numberOfMessagesToSend + 1

for iteration := 0; ; iteration++ {

@MadLittleMods MadLittleMods Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for iteration := 0; ; iteration++ {
for paginationAttempt := 0; ; paginationAttempt++ {

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)

@MadLittleMods MadLittleMods Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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),
)

}
Comment on lines +436 to +440

@MadLittleMods MadLittleMods Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having maxIterations can make sense 👍


messageQueryParams := url.Values{
"dir": []string{"b"},
"limit": []string{strconv.Itoa(testCase.messagesRequestLimit)},
Expand Down Expand Up @@ -472,6 +483,11 @@ func _sendAndTestMessageHistory(
break
}
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)

@MadLittleMods MadLittleMods Jul 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

}
seenTokens[fromToken] = true
}

// Put them in chronological order to match the expected list
Expand Down