Skip to content

fix(client): preserve data during replay suppression - #4723

Closed
joshdchang wants to merge 1 commit into
electric-sql:mainfrom
ai-sapien:josh/preserve-replayed-shape-data
Closed

joshdchang wants to merge 1 commit into
electric-sql:mainfrom
ai-sapien:josh/preserve-replayed-shape-data

Conversation

@joshdchang

Copy link
Copy Markdown
Contributor

Summary

  • preserve row-bearing messages when a cached response is replayed for a previously acknowledged cursor
  • suppress only the duplicate up-to-date control message while the tracker advances to the live cursor
  • add a regression test and document the replay invariant in the TypeScript client specification

Root cause

The replay transition was represented as suppressBatch, and the client returned before publishing the entire response. A valid replay response can contain both data messages and the duplicate up-to-date marker, so this discarded rows together with the marker.

Fix

Represent the transition as suppressUpToDate, run the normal snapshot-tracker filtering, and remove only up-to-date messages before notifying subscribers. The client still avoids an empty subscriber callback when the response contains only the suppressed marker.

Fixes #4722.

Verification

  • regression test fails on current main before the fix and passes after it
  • corepack pnpm exec vitest --config vitest.unit.config.ts --run (438 tests)
  • corepack pnpm run typecheck
  • corepack pnpm run stylecheck
  • Prettier check and git diff --check

@alco

alco commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Thanks for the fix, @joshdchang — suppressing only the up-to-date marker and keeping the data messages is the right change, and it's going in.

To land it I've rebased your commit onto current main (kept as-is, with your authorship) and stacked a few follow-ups on top, opened as #4782 which supersedes this PR:

  • LSN bookkeeping ordering. main now feeds global_last_seen_lsn from each up-to-date into the snapshot tracker inside the same filter. With your early return false ahead of that, a suppressed up-to-date would skip snapshot retirement. Moved the check after the LSN update so suppression only affects delivery to subscribers.
  • Annotated the C9 test and spec entry as pinning a state-machine invariant rather than a server scenario. While tracing when the branch actually fires I found it can't against a conforming server: electric-cursor is only set on live=true responses, the client only sends live=true from LiveState, and ReplayingState is only entered around a non-live request. The same synthetic fixture shape (cursor headers on non-live responses) is what the original tracker tests from Fix multiple renders from cached up-to-date messages on page refresh #3358 use, which is why it wasn't obvious. Details in the new PR's description.
  • Removed if (messagesToProcess.length === 0) return. It's a behaviour change outside replay mode too — a batch whose every change is rejected by the snapshot tracker previously produced a #publish([]) and with the guard produces nothing — so the new PR keeps the pre-existing behaviour. If you'd like to reintroduce it, you're welcome to comment on the rationale and implications over there and we can take it from there.

Closing this in favour of #4782.

@alco alco closed this Aug 27, 2026
alco added a commit that referenced this pull request Sep 1, 2026
…ient (#4782)

Supersedes #4723 by @joshdchang, rebased on `main` with follow-up
commits. Josh's original commit is preserved as-is.

## Problem

When a fresh `ShapeStream` enters replay mode (the up-to-date tracker
holds a recent cursor for the shape) and the first up-to-date it sees
carries that same cursor, `ReplayingState` requested suppression of the
*whole batch*: `#onMessages` returned before publishing anything. A
batch of `[insert, …, up-to-date]` therefore lost its rows while the
stream's offset still advanced past them.

## Fix

- Represent the transition as `suppressUpToDate` instead of
`suppressBatch`. The snapshot-tracker filtering runs as usual and only
the duplicate `up-to-date` control message is removed before notifying
subscribers.
- Apply the `global_last_seen_lsn` bookkeeping for a suppressed
up-to-date before dropping it, so suppression affects delivery to
subscribers only, not snapshot retirement.
- Document the invariant as C9 in the client `SPEC.md`.

The original PR also skipped the subscriber callback when the filtered
batch was empty. That applied outside replay mode as well (a batch whose
changes were all rejected by the snapshot tracker previously published
an empty array), so it is not included here; the pre-existing behaviour
is kept.

## Reachability

The suppression branch cannot fire against a conforming server:
`electric-cursor` is only set on `live=true` responses, the client only
sends `live=true` from `LiveState`, and `ReplayingState` is only entered
around a non-live request. The first up-to-date a replaying stream sees
therefore never carries a cursor. The state-machine semantics were still
wrong and are fixed here; the C9 test and spec entry are annotated as
pinning the invariant with a synthetic cursor header rather than
describing a server scenario. Whether the replay mechanism should exist
at all is left for a follow-up.

Fixes #4722.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_01PCuaAMyknYeVkfWMkyGuuS

---------

Co-authored-by: joshdchang <joshchang04@gmail.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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.

ShapeStream replay suppression can discard data messages

2 participants