Skip to content

fix(client): don't count SSE catch-up responses as short connections - #4841

Open
Nabil372 wants to merge 7 commits into
electric-sql:mainfrom
Nabil372:fix/client-sse-catch-up-not-short
Open

Nabil372 wants to merge 7 commits into
electric-sql:mainfrom
Nabil372:fix/client-sse-catch-up-not-short

Conversation

@Nabil372

Copy link
Copy Markdown

Refs #4137

Why the change

After three SSE responses in a row that end within 1 s without being aborted, the client logs "SSE connections are closing immediately (possibly due to proxy buffering ...)" and long polls for the rest of the stream's life. It decides on duration alone (LiveState.handleSseConnectionClosed).

The server closes a live SSE request that is behind the log end as soon as it has sent the catch-up (do_serve_shape_log in api.ex). During a burst of writes each reconnect lands behind the log end, so three healthy responses end within milliseconds and the stream drops to long polling with nothing buffered. This is the second trigger in the #4137 comments, and it reproduces against a plain local Electric: a shape with liveSse: true, then a few hundred single-row inserts in separate transactions.

We run Electric with TanStack DB in production, in web apps and in Expo apps on iOS and Android, and any write-heavy moment can do this to every open SSE stream. We run a looser version as a patch on 1.5.28 (any short response that carried an event is exempt), which is only safe next to #4807's guard, so this PR also requires the response to have moved the offset.

The fix

A short response is exempt when its body delivered messages and the stream's offset after it differs from the offset the request was made at. An exempt response neither counts nor resets the counter and skips the short-connection backoff, so the next request goes out at once.

Tests

  • shape-stream-state.test.ts: "short connection that advanced the offset leaves the counter unchanged" (fails on main).
  • stream.test.ts, "SSE short-connection fallback": four catch-up responses that each carry a row and an up-to-date stay on SSE with no warning (fails on main); three short responses that replay operations and end back at 0_0 still fall back; three keep-alive-only bodies whose header moves the offset still fall back (fails if the header alone exempts a response, as does the existing integration test "should reset SSE fallback state after shape rotation (409)").
cd packages/typescript-client
pnpm run typecheck
pnpm run stylecheck
pnpm exec vitest run --config vitest.unit.config.ts   # 14 files, 456 tests pass

client.test.ts also passes against a local Electric (electricsql/electric:canary).

A live SSE request that is behind the log end gets the catch-up and is
closed by the server right away. During a burst of writes three such
responses in a row tripped the short-connection heuristic, and the
stream fell back to long polling for its lifetime with the
proxy-buffering warning. Count a short response only when its body
delivered no messages; one that did neither counts nor resets the
counter and skips the short-connection backoff.

Refs electric-sql#4137
A short SSE response that carried messages is not necessarily healthy:
while the client can move its offset back on an up-to-date (electric-sql#4806),
every response replays the same operations and closes at once, and the
three-short-responses fallback with its backoff is what stops that loop.
Exempt a short response only when the stream's offset after it differs
from the offset the request was made at, and test that a replay at one
offset still falls back.
On main the client takes an SSE response's offset header when the response
opens, so a body that ended before any event could still look like it moved
the offset and skip the short-connection count. Require that the body
delivered messages too. The existing integration test "should reset SSE
fallback state after shape rotation (409)" failed without this, and the
keep-alive-only unit test now uses a header that moves the offset.

This branch has not been deployed

No deployments
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.

1 participant