fix(realtime): keep the file-doc store reconnecting instead of dying quietly - #6661
Conversation
…quietly A relay that lost Redis for longer than its retry budget did not degrade — it went silently split-brain and stayed that way. The reconnect strategy returned an `Error` after ten attempts, which tells node-redis to give up and CLOSE the client, and a closed client rejects every command with "The client is closed" for the rest of the process's life. From that point the task kept serving clients while its rooms stopped receiving other tasks' updates, its own edits stopped reaching the shared stream (also the crash buffer between persists), and seeds, locks and the persist If-Match token all failed. The tail loop then treated that as a transient read error — `running` is only false during shutdown, so it retried every 500ms forever, one warning per attempt. A tab left open overnight produced thousands of identical lines, which is how the actual failure stayed invisible. - Never stop reconnecting. This process holds live documents whose only convergence path is that connection, so a connection it can rebuild is always worth rebuilding. Same capped backoff, now via the shared `backoffWithJitter`, and no error return. - Back the reader off after a failed read (500ms → 10s) instead of retrying at the read cadence, re-open a client that was CLOSED — node-redis reconnects a dropped client, never a closed one — and log the first failure of a streak then one in twenty, carrying the streak length, so an outage stays visible without burying itself. Pinned by a test that models a closed connection: six read attempts in three seconds before, about three after, and proof the reader is re-opened rather than abandoned.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview Redis clients now use capped The multiplexed reader tail loop no longer retries failed reads every 500ms with a warning each time. It backs off (500ms up to 10s), resets the failure streak when any read succeeds—including idle blocking timeouts with no new messages—so a later blip does not start at max backoff, re-opens the read client when Tests extend the in-memory Redis fake for closed-client behavior and add coverage for backoff/reconnect and streak reset on idle reads. Reviewed by Cursor Bugbot for commit 064b4aa. Configure here. |
Greptile SummaryThe PR keeps the Redis-backed file-document store recoverable during extended connection outages.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/realtime/src/handlers/file-doc-store.ts | Introduces indefinite Redis reconnection, backed-off reader recovery, closed-client reopening, and correct failure-streak reset after any successful read. |
| apps/realtime/src/handlers/file-doc-store.test.ts | Adds closed-reader recovery and idle-read streak-reset coverage while consistently using the shared sleep helper. |
Sequence Diagram
sequenceDiagram
participant Reader as FileDocStore reader
participant Redis as Redis client
Reader->>Redis: xRead()
Redis-->>Reader: read failure
Reader->>Reader: Increment failure streak
Reader->>Reader: Sleep with capped jittered backoff
alt Client is closed
Reader->>Redis: connect()
end
Reader->>Redis: xRead()
Redis-->>Reader: messages or idle timeout
Reader->>Reader: Reset failure streak
Reviews (3): Last reviewed commit: "test(realtime): assert the retry delay, ..." | Re-trigger Greptile
… busy one Review findings, both accurate. The streak reset sat after the entries were applied, so a blocking read that timed out with nothing new — the idle steady state — skipped it via `continue`. A healed outage's count therefore survived through normal polling, and the next unrelated blip opened at the backoff cap: minutes of avoidable split-brain, and a log line claiming a failure count it never earned. The streak now ends on the read RETURNING, which is what proves the connection works. Also: the new test built a raw `setTimeout` promise instead of the shared `sleep`, which CLAUDE.md calls out by name.
|
@cursor review |
The streak-reset guard could pass on the very regression it exists to catch. It counted read attempts inside a 1900ms window, and the jittered delay for a carried streak is 1600–2400ms — so whenever jitter landed below about 0.95, a second read fell inside the window and the assertion held even though the idle reads had never cleared `failures`. A single falsification run happened to draw a long delay, which is exactly how a guard like this goes quiet. Assert the delay itself instead. The first retry after a reset is 500ms ±20% (400–600ms); carried over it is the third, 2000ms ±20% (1600–2400ms). Those ranges are disjoint, so the check no longer depends on which jitter is drawn: against the old placement it now fails every time (measured 2160ms, 1925ms, 2046ms against the 1000ms bound).
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 064b4aa. Configure here.
A relay that lost Redis for longer than its retry budget did not degrade — it went silently split-brain and stayed that way until the process restarted.
What was happening
Leaving a tab open long enough for the Upstash connection to drop produced this, twice a second, forever:
Two bugs stacked:
The client gave up permanently. The reconnect strategy returned an
Errorafter ten attempts:Returning an
Errorthere tells node-redis to stop reconnecting and close the client, and a closed client rejects every command with"The client is closed"for the rest of the process's life. So an outage longer than roughly a minute did not degrade the task — it took it out, quietly. The task kept serving clients while:If-Matchtoken all failed.The tail loop spun on it.
runReader's catch only breaks on!this.running, which is true solely during shutdown — so a permanently-closed client was retried every 500ms forever, one warning per attempt. A tab left open overnight yields thousands of identical lines, which is how the actual failure stayed invisible.Changes
backoffWithJitter, and no error return.Verification
A test models a closed connection (
xReadthrowsThe client is closedand flipsisOpen) and asserts the loop slowed down and tried to recover:connect()278 realtime tests,
tscand biome clean.This is independent of the collaborative-files work it was found alongside: any long-lived Redis drop hits it.