Preserve data messages during replay suppression in the TypeScript client - #4782
Merged
Merged
Conversation
…-date A replayed up-to-date that is suppressed during replay mode still carries a real global_last_seen_lsn. Run the snapshot tracker's lastSeenUpdate for it before filtering it out, so suppression only affects delivery to subscribers and not snapshot retirement. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PCuaAMyknYeVkfWMkyGuuS
The server only sets electric-cursor on live responses and the client is never in ReplayingState for a live request, so the cursor match that triggers suppression cannot occur against a conforming server. Mark the C9 test and spec entry as pinning the state-machine invariant rather than describing a server scenario. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PCuaAMyknYeVkfWMkyGuuS
Drop the early return on an empty filtered batch. It skipped the subscriber callback not only for a suppressed replay marker but also for any batch whose changes were all rejected by the snapshot tracker, which previously published an empty array. Restore the pre-existing behaviour so this change is limited to the replay path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PCuaAMyknYeVkfWMkyGuuS
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4782 +/- ##
==========================================
+ Coverage 60.02% 60.07% +0.05%
==========================================
Files 397 397
Lines 43772 43772
Branches 12588 12591 +3
==========================================
+ Hits 26272 26294 +22
+ Misses 17418 17397 -21
+ Partials 82 81 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a2ab2f380
ℹ️ 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".
When a replayed batch carries nothing but the duplicate up-to-date, the suppression filter leaves it empty and #publish still invoked every subscriber with []. Subscribers are promised one or more messages per notification, and the pre-suppression code never notified for a suppressed replay, so return early in that case. The early return is gated on transition.suppressUpToDate so that batches emptied by the snapshot tracker alone keep their pre-existing behaviour of publishing an empty array. Addresses the Codex review on PR #4782. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qn6oVFdmDPXtTfioKPjHua
Contributor
|
This PR has been released! 🚀 The following packages include changes from this PR:
Thanks for contributing to Electric! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #4723 by @joshdchang, rebased on
mainwith follow-up commits. Josh's original commit is preserved as-is.Problem
When a fresh
ShapeStreamenters 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,ReplayingStaterequested suppression of the whole batch:#onMessagesreturned before publishing anything. A batch of[insert, …, up-to-date]therefore lost its rows while the stream's offset still advanced past them.Fix
suppressUpToDateinstead ofsuppressBatch. The snapshot-tracker filtering runs as usual and only the duplicateup-to-datecontrol message is removed before notifying subscribers.global_last_seen_lsnbookkeeping for a suppressed up-to-date before dropping it, so suppression affects delivery to subscribers only, not snapshot retirement.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-cursoris only set onlive=trueresponses, the client only sendslive=truefromLiveState, andReplayingStateis 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.ai/code/session_01PCuaAMyknYeVkfWMkyGuuS