Skip to content

Deliver realm events to synapse through one bounded, paced queue per process - #6899

Merged
habdelra merged 4 commits into
mainfrom
cs-14189-broadcastrealmevent-is-fire-and-forget-with-no-queue-so-a
Oct 22, 2026
Merged

habdelra merged 4 commits into
mainfrom
cs-14189-broadcastrealmevent-is-fire-and-forget-with-no-queue-so-a

Conversation

@habdelra

@habdelra habdelra commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

What this changes

NodeAdapter.broadcastRealmEvent sends a realm event to every session room subscribed to the realm. Realm calls it without awaiting, so each event ran its own send loop, all at once and with nothing limiting them. A burst of writes became events × rooms sends pushed at synapse as fast as synapse accepted them. Synapse is the same process that serves every user's /sync, message sends and AI assistant streams, so all of those waited behind the burst. Timeouts don't relieve it: a proxy that times out a send does not cancel it, and synapse completes the send anyway.

Every broadcast now goes through one RealmEventDispatcher per process (packages/realm-server/realm-event-dispatcher.ts):

  • Bounded concurrency and rate. At most REALM_EVENT_MAX_SENDS_IN_FLIGHT sends (default 8) are outstanding at once, and sends start no faster than REALM_EVENT_SENDS_PER_SECOND (default 50). The bucket holds one second's worth of sends, and never less than one.
  • Realms take turns. Each send slot goes to the next realm in rotation that has a send ready. The rate limit is checked before a send is chosen, so a realm doesn't lose its turn while the line waits for a token. A realm with a large backlog gets an equal share of the line rather than all of it, so an event from a user's own realm waits behind at most one send per busy realm, however deep the busy realm's backlog is.
  • Per-realm order is kept. A realm's next event starts only after every room has the event before it. Subscribers read incremental-index-initiation and then incremental as the start and end of an indexing pass, so reordering them would leave a client showing "indexing" after the pass has finished.
  • Superseded events merge while queued. A newer incremental-index-initiation replaces a queued one. A newer update merges into a queued one as the union of its path lists. Only the realm's most recently queued event can merge, so a merge never moves an event past one of another kind. Completion events (incremental, full, prerender_html) are never merged, because subscribers reconcile request ids, versions and generations per event.
  • Back-pressure. A send that synapse, or the ALB in front of it, answers with 429/502/503/504, that gets no answer within 30s, or that cannot reach synapse at all, pauses the whole line. A login that fails the same way does too, and the event's rooms are resolved again after the pause rather than the event being skipped. The pause is at least as long as a rate-limit answer's retry_after_ms or Retry-After asks. It starts at 1s and doubles, up to 30s, for each pause in a row that ends in back-pressure again, and resets on a success. Only sends started after the latest pause count toward either, so the sends already in flight when synapse became overloaded add up to one pause rather than doubling it once each. The send is retried, up to 5 attempts, with the same transaction id, so synapse returns the stored event rather than storing a second copy when the timed-out send had in fact completed. A send that starts while a revoked session is being replaced waits for the new login. Other failures go to the existing per-room handling: the stale-room cleanup is unchanged, and other failures log one line with no stack.
  • Bounded memory. Queued bytes are capped at REALM_EVENT_MAX_QUEUED_BYTES (default 64 MiB). Past the cap, the realm with the longest backlog loses its oldest queued event, so the realm causing the backlog is the one that loses events. An event a subscriber recovers from (an initiation, an update, prerender_html) goes before the completion of an indexing pass, because a subscriber that misses a completion keeps showing the realm as indexing and misses that pass's invalidations. Drops are logged as one aggregate warning per 10s.

A realm whose room lookup found no session rooms queries once on its next event instead of waiting and retrying, so a realm nobody has open doesn't pay that wait on every queued event.

broadcastRealmEvent still resolves once the event has gone to every room (or was merged or dropped) and still never rejects, so its callers and the worker-event bridge see the same contract.

MatrixClient.sendEvent takes an optional txnId. On a non-2xx response it throws a MatrixRequestError that carries status, and it reads the body as text first, so an HTML gateway page no longer becomes SyntaxError: Unexpected token '<'. JSON error bodies keep the status <n> - {…} message shape that parseMatrixSendEventError reads.

MatrixClient.login() reuses its settled session, so a broadcast logs in only on the first send or after the session is revoked. A failed login throws the same MatrixRequestError, built the same way, rather than JSON-parsing a gateway's HTML page. Room sends and logins give synapse 30s to answer, so a request synapse accepts but never answers holds its realm's line for that long rather than for the HTTP client's multi-minute default.

A realm server stopping on purpose waits up to 10s for queued realm events to go out before it closes its database, and logs what it gives up on if the queue is not empty by then.

Observability

The realm:health line gains these fields: realmEventsQueued, realmEventQueuedKB, realmEventQueueAgeMs, realmEventSendsInFlight, realmEventSends, realmEventSendMs(mean/max), realmEventFailedSends, realmEventBackPressure, realmEventsCoalesced and realmEventsDropped. They are documented in docs/realm-server-health-signals.md.

A new Grafana rule, Realm Server Realm Event Backlog (rs-realm-event-backlog), fires when realmEventQueueAgeMs is above 60s in every 1-minute bin of the last 5 minutes. The rule reduces the window with min, so a single bin under the threshold keeps it quiet. It reads the age of the oldest queued event rather than the queue depth, because a burst that drains keeps the age low however deep the queue gets.

Out of scope

  • The queue is in memory. Beyond the bounded drain on a deliberate stop, a process that crashes or is killed loses what it had queued. That is a deliberate trade, not a deferral: persisting the queue would put a database write in front of every realm event.
  • The number of events a bulk write produces, and the number of rooms each event fans out to, are what drive volume. This change bounds how that volume reaches synapse, not how much of it there is. Both are tracked as their own changes, which compound with this one: fewer events per bulk write and fan-out only to users who have the realm open.
  • The default rates are deliberately conservative and can be tuned per environment with the env vars above.

Tests

  • tests/realm-event-dispatcher-test.ts (new, no database) covers:
    • the in-flight cap;
    • per-realm ordering;
    • rotation across realms;
    • merging, including that nothing merges past an event of another kind;
    • rotation that holds while the rate limit paces sends;
    • back-pressure retries with a doubling pause and the same transaction id, one pause per episode rather than per in-flight send, and a pause that honors a rate-limit answer's requested delay;
    • a rate below one send per second still sending;
    • non-retried failures, and giving up after maxAttempts;
    • the byte bound, which drops from the longest backlog and spares completions;
    • the rate limit;
    • a realm whose rooms can't be resolved not stalling the line, and rooms that can't be resolved because of back-pressure being resolved again after the pause, up to maxAttempts;
    • a send that outlives its deadline being retried with the same transaction id;
    • drain resolving once the queue empties, and reporting a queue that outlasts its wait;
    • isBackPressure and coalesce.
  • tests/node-realm-test.ts adds adapter-level tests: a 504 is retried with the same transaction id and does not clear the session room, and a login answered with a 502 is tried again and the event still reaches the room. The existing stale-room tests run on a dispatcher of their own.

🤖 Generated with Claude Code

NodeAdapter.broadcastRealmEvent started an independent send loop per event,
so a burst of events was pushed at synapse with no limit. Route every
broadcast through a per-process RealmEventDispatcher that caps sends in
flight, rate-limits them, takes realms in turn, keeps each realm's events in
order, merges queued initiation and update events, bounds queued bytes, and
treats 429/502/503/504 or an unreachable homeserver as back-pressure: the line
pauses with exponential backoff and the send is retried with the same
transaction id, so synapse dedupes a retry of a send it already completed.

MatrixClient.sendEvent takes an optional txnId and throws a
MatrixRequestError carrying the status, without JSON-parsing a proxy's HTML
error page. The queue's depth, age, send latency, back-pressure, merges and
drops go on the realm:health line, and a Grafana rule alerts when the oldest
queued event has waited over 60s for 5 minutes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-21T22:27:15.104795Z 1975634 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 197563434c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +433 to +436
this.#backoffMs = Math.min(
this.#maxBackoffMs,
this.#backoffMs ? this.#backoffMs * 2 : this.#initialBackoffMs,
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Honor Matrix's retry delay for 429 responses

When Synapse returns 429 with a retry_after_ms body or Retry-After header longer than this fixed exponential delay, the dispatcher retries too early and exhausts all five attempts in about 15 seconds, permanently dropping the realm event while the rate limit is still active. Matrix's requested delay should be carried through MatrixRequestError and used as the minimum pause; the existing matrixRetryAfterMs handling in packages/realm-server/lib/secure-session.ts demonstrates both response forms.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Fixed in 102f4b8. MatrixClient.sendEvent reads retry_after_ms from the error body, or Retry-After from the headers, onto MatrixRequestError.retryAfterMs. The dispatcher pauses for whichever is longer: that value or the backoff. The test waits at least as long as a rate-limit answer asks pins it.

Comment on lines +491 to +494
"relativeTimeRange": {
"from": 300,
"to": 0
},

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Evaluate backlog persistence over the current sample

The rule queries a trailing five-minute window and then reduces it with max, while also requiring the resulting condition for five minutes. Consequently, a backlog that clears after roughly a minute can remain above threshold solely because its old samples stay in the lookback window long enough to satisfy for: 5m, producing a stale alert despite the summary claiming five continuous minutes of backlog. Query only the current sampling interval before applying for, or aggregate the window in a way that requires every interval to exceed the threshold.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Fixed in 102f4b8. The rule now reduces the 5-minute window with min and uses for: 0s, like its sibling rules. It fires only when every 1-minute bin in the window is over 60s, so a backlog that clears stops matching within a minute. The summary says the same.

@habdelra habdelra left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] This review covered the dispatcher's state machine (rotation, token bucket, pause and backoff, per-realm order, coalescing, drop policy), the adapter and MatrixClient retry path, the alert rule, and whether the timing tests can flake. It did not run the realm-server DB suite, so the adapter-level test in node-realm-test.ts and any pacing effects on other suites are unverified.

Bottom line: one blocking issue. At the rate limit, the rotation can starve whole realms, which defeats the "realms take turns" guarantee. The rest is non-blocking. The backoff and the zero-subscriber prepare cost should be fixed before this carries production load.

  1. Rotation starves realms when out of tokens: take the token before picking. Blocking. See #pump in realm-event-dispatcher.ts.
  2. The backoff jumps to the cap on one burst of concurrent failures, and successes from before the pause reset it. See the backoff handling in #run.
  3. REALM_EVENT_SENDS_PER_SECOND below 1 stalls the line forever. See the constructor.
  4. prepare runs once per event per realm and pays the waitForSessionRooms retry sleeps, so unwatched realms drain at about 6–7 events/s and can trigger the backlog alert. See prepareRoomSends in node-realm.ts.
  5. Decision: the drop policy can discard an incremental and leave hosts showing "indexing". See #enforceBound.
  6. Pre-existing: a send that starts during a re-login fails permanently. See the send closure in node-realm.ts.
  7. The alert's max reducer combined with for: 5m doesn't match its summary. See the comment on the rule.

The timing tests assert lower bounds only, and the pause is checked against now() when the timer fires, so they cannot flake toward failure. The retry with the same transaction id holds across the 401 re-login, because the device id is fixed.

Adjacent, out of scope: the PR description's "no longer becomes SyntaxError" is temporal wording. The current contract is "a non-JSON error body is reported by size, not parsed".

Comment on lines +314 to +326
let next = this.#nextSend();
if (!next) {
return;
}
if (this.#tokens < 1) {
// Put it back at the front of its realm's sends; the rotation has
// already moved on, so this realm is not favored when the line wakes.
next.active.ready!.unshift(next.pending);
this.#wakeAt(
now + Math.ceil(((1 - this.#tokens) * 1000) / this.#sendsPerSecond),
);
return;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] When the line hits the rate limit, the rotation can starve whole realms. That is exactly the backlog case "realms take turns" exists for.

#nextSend() moves #cursor past the realm it picks. When #tokens < 1 sends that pick back, the realm has lost its turn even though nothing went out. Every pump while out of tokens does this: the one after a send in the loop, and each pump from a completion's finally. So between wakes the cursor moves by however many pumps ran, not by one. Whenever that step is a multiple of the number of busy realms, the same realm wins every wake.

I checked this against the branch: 3 realms each with a 200-room event, sendsPerSecond: 20, maxInFlight: 8, 4s run. Past the initial bucket, instant sends gave r0 all 79 paced sends. With 30ms sends, r0 got none (r1 33, r2 46).

The fix is to take the token before choosing:

this.#refill(now);
if (this.#tokens < 1) {
  this.#wakeAt(now + Math.ceil(((1 - this.#tokens) * 1000) / this.#sendsPerSecond));
  return;
}
let next = this.#nextSend();
if (!next) {
  return;
}

With that change, the same probe splits 27/27/26 in both send timings, and the 12 tests in realm-event-dispatcher-test.ts still pass. The rotation test runs at sendsPerSecond: 1000 and maxInFlight: 1, so it never reaches the token branch. Please add a case with three backlogged realms at a low rate that asserts the split.

Regression, blocking: it breaks the fairness guarantee the PR is built around.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Fixed in 102f4b8. #pump checks for a token before #nextSend chooses a send, so waiting on the rate limit no longer moves the rotation past a realm. The new test keeps realms taking turns while the rate limit holds sends back runs 3 realms at 100 sends/s and checks that the paced sends split about evenly. Run against the previous dispatcher, it fails at 0/30/30.

Comment on lines +428 to +440
this.#backoffMs = 0;
},
async (error: unknown) => {
if (isBackPressure(error)) {
this.#backPressure++;
this.#backoffMs = Math.min(
this.#maxBackoffMs,
this.#backoffMs ? this.#backoffMs * 2 : this.#initialBackoffMs,
);
this.#pausedUntil = Math.max(
this.#pausedUntil,
this.#now() + this.#backoffMs,
);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] The backoff follows how many sends are in flight, not how many overload episodes there have been. It goes wrong in both directions:

  • One episode goes straight to the cap. Each of the up to maxInFlight sends that were already in flight when synapse tipped over doubles #backoffMs again. I ran 8 concurrent 503s with initialBackoffMs: 10 and maxBackoffMs: 1000: every retry started at about 1006ms. With the defaults, one burst of 502s from a restarting synapse pauses every realm in the process for 30s.
  • A success from before the pause resets it. A send that started before the pause and then succeeds sets #backoffMs = 0. I ran one room that always returns 503 beside successful rooms (maxInFlight: 2, initial 50ms). The gaps between its attempts were 110, 51, 50 and 100ms, not 50, 100, 200 and 400. A synapse that is partly overloaded never backs off further.

To fix this, stamp each pause with an epoch. Double only when the failing send started at or after the current pause ended, and reset only on the success of such a send. #run already records startedAt, so the comparison is against #pausedUntil. A test with two or more concurrent failures would pin it, and so would a test with a failure plus a concurrent success.

Regression, non-blocking. I'd still fix it before relying on the pause in production.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Fixed in 102f4b8. The backoff now escalates once per episode. #pausedAt records when the latest pause was set, and only a send started after it can double the backoff, on back-pressure, or reset it, on success. So the sends already in flight add up to one pause, and a success from a send that predates the pause doesn't cut the next one short. The test sends that were in flight when synapse became overloaded add up to one pause, not one doubling each fails against the previous dispatcher at 401ms and now passes at about 50ms.

Comment on lines +177 to +180
// The bucket holds one second's worth of sends, so an idle line can start
// a short burst at once and a long burst settles to the steady rate.
this.#tokens = this.#sendsPerSecond;
this.#tokensAt = this.#now();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] positiveNumberFromEnv accepts any value above 0. When REALM_EVENT_SENDS_PER_SECOND is below 1 (say 0.5), the bucket's capacity is below one token. #tokens then never reaches 1, the line re-arms its timer forever, and every broadcast() promise in the process stays pending. I checked this on the branch: with sendsPerSecond: 0.5, nothing was sent in 4s.

Either cap the bucket at Math.max(1, this.#sendsPerSecond) or reject values below 1 in the env parsing. Regression, non-blocking, since only a misconfiguration reaches it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Fixed in 102f4b8. The bucket's size is max(1, sendsPerSecond), so a rate under one a second refills to one token and sends at that rate. The test a rate under one send a second still sends pins it.

Comment thread packages/realm-server/node-realm.ts Outdated
return [];
}

let dmRooms = await this.waitForSessionRooms(dbAdapter, realmUrl);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] prepare now runs once per event, one event at a time per realm. That puts waitForSessionRooms on the realm's critical path. For a realm with no subscribed session rooms, it makes four queries with three 50ms sleeps between them before it returns {}.

So an unwatched realm drains at about 6–7 events/s with no sends at all. I checked the serialization with a prepare that waits 150ms and returns []: 20 events took 3.0s, and oldestQueuedMs was 1500 after 1.5s. A bulk write of a few hundred files into a realm nobody has open therefore holds realmEventQueueAgeMs above the 60s alert threshold while synapse sits idle. The first browser to open that realm mid-burst also waits behind the whole backlog. Before this change, each broadcast paid that wait concurrently.

The retry loop came from a test-flake fix. Please make the dispatcher path a single fetchRealmSessionRooms call, where an empty result means no sends, and move any waiting the tests need into the tests. Regression, non-blocking.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Fixed in 102f4b8. NodeAdapter remembers whether the realm's last room lookup found no session rooms. When it did, the next lookup queries once instead of waiting and retrying, and a lookup that finds rooms re-enables the wait. A realm nobody has open pays the ~150ms once rather than once per queued event, and the retry still covers the moment a first session room appears.

Comment on lines +269 to +281
let longest: Line | undefined;
for (let line of this.#lines.values()) {
if (
line.pending.length > 0 &&
(!longest || line.pending.length > longest.pending.length)
) {
longest = line;
}
}
if (!longest) {
return;
}
let dropped = longest.pending.shift()!;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Should the drop policy spare completion events? It drops the oldest queued event whatever its kind. When that event is an incremental whose incremental-index-initiation already went out, two things go wrong in the host:

  • services/realm.ts leaves info.isIndexing = true (and the indexing test waiter held) until another completion arrives. That is the misleading "still indexing" state the ordering comment at the top of this file is written to prevent.
  • Open tabs never re-read that event's invalidations.

Dropping the initiations and updates first, or merging a dropped initiation into the next event, would keep the bound and keep the completion events intact. This needs a decision, not a fix. It is non-blocking, since 64 MiB is many maximum-size events.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Yes, and done in 102f4b8. In the realm with the longest backlog, #enforceBound drops the oldest event that isn't a completion (isCompletion: incremental or full). It drops the oldest completion only when every queued event is one. Losing an initiation, an update or a prerender_html costs a subscriber little, since a later event covers it. Losing a completion leaves the realm shown as indexing and drops that pass's invalidations. The test past the byte bound, drops an event a subscriber can lose before a completion pins it.

Comment on lines +495 to 501
send: async (txnId: string) => {
await matrixClient.sendEvent(
roomId,
APP_BOXEL_REALM_EVENT_TYPE,
eventWithRealmURL,
event,
txnId,
);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Retrying with the same transaction id holds across the 401 re-login, because the device stays boxel-server. There is one gap next to it, though. While MatrixClient.request re-logs in after an M_UNKNOWN_TOKEN, it holds access = undefined. Any send the dispatcher starts in that window throws a plain Missing matrix access token from sendEvent. That error isn't back-pressure, so the send goes to onFailure and is lost.

An await matrixClient.login() at the top of send closes the gap. It costs nothing on a settled session, and it waits on the in-flight re-login. Pre-existing (sendEvent had the same check before), non-blocking. Since the PR adds the retry path, this is the natural place to cover it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Fixed in 102f4b8. The send closure calls matrixClient.login() first when isLoggedIn() is false. During a re-login that joins the in-flight loginPromise, so the send waits for the new token instead of failing with "Missing matrix access token".

],
"noDataState": "OK",
"execErrState": "Alerting",
"for": "5m",

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] The rule doesn't do what its summary says. The summary says "at every 1-minute sample for 5 minutes". The rule takes max over the 5-minute window (C) and then requires for: 5m. A single 5s health sample above 60s therefore keeps B true for as long as it stays in the window, about 5 minutes. Two spikes a few minutes apart will fire the alert, though the backlog never held.

The sibling rules use for: 0s with a max reducer and say "at least once in the last 5 minutes". For "held for 5 minutes", set C's reducer to min (every 1-minute bin's max is above 60s) and for to 0s. Otherwise, keep the summary and narrow relativeTimeRange.from to 60. The JSON parses and passes prettier. Regression, non-blocking.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Fixed in 102f4b8. The rule reduces with min and uses for: 0s, so it fires only when every 1-minute bin of the last 5 minutes is over 60s, which matches the summary. The summary now says so explicitly.

Address review findings on the realm event dispatcher:

- Check for a token before choosing a send, so waiting on the rate limit no
  longer moves the rotation past a realm and starves it.
- Escalate the backoff once per overload episode: only a send started after
  the latest pause doubles it or resets it.
- Honor a rate-limit answer's retry_after_ms / Retry-After, carried on
  MatrixRequestError.retryAfterMs, as the minimum pause.
- Size the token bucket at least one, so a rate under one a second still sends.
- Past the byte bound, drop an event a subscriber can lose before an indexing
  pass's completion.
- Skip the room-lookup retries after a realm's last lookup found no rooms.
- Wait for an in-flight re-login before a send instead of failing it.
- The backlog alert reduces its window with min and drops `for`, so it fires
  only when every 1-minute bin is over the threshold.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@habdelra
habdelra requested a review from a team October 21, 2026 22:39
@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Host Test Results

    1 files  ±0      1 suites  ±0   2h 10m 42s ⏱️ + 4m 16s
5 842 tests +1  5 831 ✅ +1  11 💤 ±0  0 ❌ ±0 
5 857 runs  +1  5 846 ✅ +1  11 💤 ±0  0 ❌ ±0 

Results for commit fb6cdcc. ± Comparison against earlier commit 4774110.

Realm Server Test Results

    1 files  ±0    360 suites  ±0   2h 2m 44s ⏱️ -6s
5 999 tests +5  5 999 ✅ +5  0 💤 ±0  0 ❌ ±0 
6 062 runs  +5  6 062 ✅ +5  0 💤 ±0  0 ❌ ±0 

Results for commit fb6cdcc. ± Comparison against earlier commit 4774110.

@habdelra

Copy link
Copy Markdown
Contributor Author

[Claude Code 🤖] The red Host Tests (10, 16) check fails on Acceptance | code submode | file-tree tests: scroll position is restored when returning to file view but only for one file per realm. That test fails on every run of its shard on Chrome 156, on any branch; #6890 fixes it. This PR doesn't touch the host's code mode. Once #6890 lands, I'll merge main into this branch to turn the shard green.

…lmevent-is-fire-and-forget-with-no-queue-so-a

@FadhlanR FadhlanR left a comment

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.

[Claude Code 🤖] This review looked at what the line does when a send never returns, when re-login fails under overload, and at shutdown, on top of the earlier rounds on rotation, backoff and the drop policy. It did not run anything: the realm-server suites and synapse behaviour under load are unverified.

Bottom line: no blocking issues. The per-realm barrier makes an unanswered send much more expensive than before, since sendEvent has no client-side timeout. That, and draining the queue on shutdown, are worth settling before this carries production load.

  1. A send with no answer holds its realm's line, and one of the 8 slots, for undici's 300s headers timeout per attempt. Add a send timeout that counts as back-pressure. See the comment on sendEvent in matrix-client.ts.
  2. A login that fails under the same overload (HTML 502, 429) isn't back-pressure, so the event is lost for that room, or skipped for every room when prepare logs in. See the comment on the send closure in node-realm.ts.
  3. Shutdown doesn't drain the queue, so a rolling deploy loses everything queued at that moment. Under a backlog that can include completions. See the comment on the in-memory note in realm-event-dispatcher.ts.

The description's Out of scope section lists deferred work (in-memory queue loss, fewer events per bulk write, narrower fan-out) with no tracking tickets. The repo's PR rules ask for each deferral to have a ticket linked from the description before merge.

Comment on lines 367 to 371
let response = await this.request(
`_matrix/client/v3/rooms/${roomId}/send/${type}/${txnId}`,
'PUT',
{ body: JSON.stringify(content) },
);

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.

[Claude Code 🤖] This fetch has no client-side timeout, and the dispatcher now makes a slow answer cost a lot more. #finishIfDone holds a realm's next event until outstanding is 0, so one send synapse accepts but doesn't answer stalls every subscriber of that realm, and holds one of the maxInFlight slots. Undici's default headers timeout is 300s. It then rejects with TypeError: fetch failed, which isBackPressure retries, up to 5 attempts. So one wedged room can hold its realm's line for about 25 minutes. Eight of them stop realm events for the whole process. Before this change, an unanswered send held up only the rest of its own broadcast.

Suggest passing signal: AbortSignal.timeout(…) here (something like 30s, well under the 300s default), and having isBackPressure also treat a TimeoutError/AbortError as back-pressure. The retry is safe because it reuses the transaction id. A dispatcher test with a send that never settles would pin it.

Regression, non-blocking.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Fixed in fb6cdccb07. sendEvent and the login request both pass signal: AbortSignal.timeout(30_000) (MATRIX_WRITE_TIMEOUT_MS). isBackPressure treats an error named TimeoutError or AbortError as back-pressure, so a send synapse never answers holds its realm's line for at most 30s per attempt. The retry pauses the line and reuses the transaction id. realm-event-dispatcher-test.ts covers the classification and the same-txnId retry of a send that outlived its deadline.

Comment on lines +504 to +509
send: async (txnId: string) => {
// A send can start while a revoked session is being replaced, when
// the client holds no token; wait for the new one rather than fail.
if (!matrixClient.isLoggedIn()) {
await matrixClient.login();
}

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.

[Claude Code 🤖] The back-pressure classification doesn't cover the login this closure now waits on. doLogin still calls response.json() before it checks ok and throws a plain Error. An ALB 502/503 HTML page therefore becomes a SyntaxError, and a synapse 429 on /login becomes an Error with no status. Neither counts as back-pressure, so the room's send goes to onFailure and is lost, during exactly the overload the line is meant to ride out. The same failure inside prepareRoomSends returns [], which skips the event for every room. The 401 re-login inside MatrixClient.request takes the same path.

Throwing MatrixRequestError from doLogin, built the way sendEvent now builds it, would let both paths pause and retry. In prepare, that means letting the error reach the dispatcher rather than returning [].

Pre-existing, now load-bearing. Non-blocking.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Fixed in fb6cdccb07. doLogin checks ok before it reads the body, and a failed login throws MatrixRequestError from the same helper sendEvent uses. So an ALB 502 page or a synapse 429 carries its status and any retry_after_ms/Retry-After, and isBackPressure recognises it.

That covers both paths:

  • The send closure, including the 401 re-login inside MatrixClient.request: the error reaches the dispatcher as back-pressure, so it pauses and retries the send.
  • prepareRoomSends: it rethrows a back-pressure login failure instead of returning []. The dispatcher then puts the event back at the head of its realm's line and prepares it again after the pause, up to maxAttempts. Any other prepare failure still skips the event.

New tests in realm-event-dispatcher-test.ts cover the re-prepare and the give-up after maxAttempts. node-realm-test.ts covers a login answered with a 502 that is tried again and still delivers to the room.

Comment on lines +51 to +52
// The queue is in memory, so a process that exits with events queued does not
// deliver them.

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.

[Claude Code 🤖] Can shutdown drain the line before it exits? stopRealmServer in main.ts closes the reconciler and the DB, then calls process.exit(0), and never waits on realmEventDispatcher().idle(). Before this change, only sends already on the wire could be lost at exit. Now everything queued is lost too, and the queue is designed to hold a backlog: the alert only fires past 60s. On a rolling deploy that can include an incremental completion, which leaves open tabs showing the realm as indexing, the outcome isCompletion exists to prevent. A bounded wait in stopRealmServer before dbAdapter.close(), like Promise.race([realmEventDispatcher().idle(), timeout(10_000)]), would cover ordinary deploys, since prepare still needs the DB. If this stays out of scope, it needs a tracking ticket.

Regression, non-blocking.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Fixed in fb6cdccb07. RealmEventDispatcher.drain(timeoutMs) resolves true once nothing is queued or in flight, and false if that takes longer. stopRealmServer awaits drain(10_000) after the listeners shut down and before dbAdapter.close(), since prepare still needs the database. If the queue isn't empty by then, it logs the queued and in-flight counts it is giving up on.

A process that crashes or is killed still loses its queue. The description now records that as a deliberate trade rather than a deferral. The two volume deferrals link their tracking tickets.

…ain on shutdown

Room sends and logins now give synapse 30s to answer, and a timeout counts
as back-pressure, so one unanswered send no longer holds its realm's line
for the HTTP client's multi-minute default. A login that synapse or its
proxy answers with 429/502/503/504 now throws MatrixRequestError, and the
dispatcher prepares the event again after the pause instead of skipping it.
A server stopping on purpose waits up to 10s for queued realm events
before closing its database.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@habdelra

Copy link
Copy Markdown
Contributor Author

[Claude Code 🤖] On the review summary's point about deferrals: the description's Out of scope section now links the tracking ticket for each one, fewer events per bulk write and fan-out only to users who have the realm open. The in-memory queue is no longer listed as a deferral. A deliberate stop drains it for up to 10s (fb6cdccb07), and losing it on a crash is stated as an accepted trade.

@habdelra
habdelra merged commit 00ccfe9 into main Oct 22, 2026
87 of 88 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants