fix(sync-service): prevent permanent flush-boundary stalls from unexpected consumer lifecycle transitions#4730
Open
alco wants to merge 7 commits into
Open
fix(sync-service): prevent permanent flush-boundary stalls from unexpected consumer lifecycle transitions#4730alco wants to merge 7 commits into
alco wants to merge 7 commits into
Conversation
Adds last_progress_at (caller-injected monotonic ms) to incomplete flush entries, refreshed only by flush notifications; new stalled_shapes/3 and touch/3 API; handle_txn_fragment reports newly-tracked shapes. Groundwork for SLC writer monitors and stall detection (spec electric-2026-07-16-flush-tracker-writer-monitors). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… suspend contract
Shrinks the benign clause of handle_writer_termination to exactly
:normal/:killed/:shutdown so untagged {:shutdown, x} reasons trigger
shape removal; maps tagged dependency-materializer shutdowns to
stop_and_clean instead of propagating the reason verbatim; suspend now
additionally requires all txn state flushed AND notified; Consumer.stop
escalates a wedged consumer to a kill after the stop call times out.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lled shapes The SLC now holds a monitor on every consumer with an incomplete FlushTracker entry: crash DOWNs unpin the flush boundary immediately and trigger shape invalidation; deliberate-cleanup DOWNs unpin only; bare :shutdown/:killed DOWNs are left to the grace-period stall tick, which invalidates shapes whose entries make no flush progress for ELECTRIC_FLUSH_STALL_GRACE_PERIOD (default 60s). Fixes the permanent confirmed_flush_lsn stall when a consumer dies without cleanup on a quiet table. Includes changeset and telemetry events. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…acking
- Re-issue the ShapeLogCollector removal from the ShapeCleaner when the
shape is already gone from ShapeStatus: an earlier removal chain that
died between the ShapeStatus removal and the SLC removal used to leave
the shape's flush entry pinned forever, because every stall-check
re-fire short-circuited at {:error, :data_removed}. The SLC removal is
idempotent, so the retry now issues it unconditionally.
- Stop the stall check from invalidating healthy shapes that deliberately
defer flush notifications. A consumer buffering transactions ahead of
PG snapshot info, or sitting in a subquery move-in awaiting splice, can
legitimately produce no flush notifications for longer than the grace
period. While in either deferral phase the consumer now periodically
casts notify_flush_deferred to the SLC (at a third of the grace
period), which touches its FlushTracker entry and re-arms the grace
window. The tick stops itself once the deferral phase ends, and a
wedged consumer can't send it, so the stall backstop stays intact.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4730 +/- ##
==========================================
+ Coverage 60.04% 60.18% +0.13%
==========================================
Files 397 412 +15
Lines 43766 44396 +630
Branches 12586 12587 +1
==========================================
+ Hits 26281 26720 +439
- Misses 17403 17594 +191
Partials 82 82
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:
|
…t is delivered to
Monitors were only established for shapes newly tracked by a commit, so
when the suspend-retry path delivered a commit for an already-tracked
shape to a fresh consumer pid, the monitor stayed on the suspended
predecessor. Its queued {:shutdown, :suspend} DOWN was then classified
as a contract-violating crash, spuriously invalidating a healthy shape
and unpinning its boundary while the fresh consumer was still writing.
Monitor every delivered shape that is still tracked after the commit
instead: the swap clause hands the monitor over and flushes the old
pid's queued DOWN. This also makes FlushTracker.handle_txn_fragment's
newly-tracked return value redundant, so it returns just the tracker
again.
Also derive the stall-check re-arm interval from the configured grace
period (clamped to [1s, 10s]) so a short grace period is enforced at
matching granularity, and add test coverage for the monitor hand-over
and for crash classification of exits observed at publish time.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
alco
marked this pull request as ready for review
July 20, 2026 09:42
Closed
1 task
![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: 2828324e3a
ℹ️ 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".
…allenge-response The deferral heartbeat had every consumer in a deferral phase cast notify_flush_deferred to the ShapeLogCollector every third of the grace period — a per-shape periodic message stream converging on the singleton SLC, deviating from the single-stall-timer design. Invert the initiative: consumers are silent, and the stall check challenges suspects instead. The first time a shape shows up stalled with a live monitored writer, the SLC sends the writer a :verify_flush_progress message and holds off. A consumer answers only while deliberately deferring its flushes (buffering ahead of PG snapshot info, or a subquery move-in awaiting splice) with the existing notify_flush_deferred cast, which touches the entry and clears the suspicion. Only a suspect still stalled at the next check — or a shape whose writer is already dead and demonitored (the self-heal path, which keeps its single-check latency) — is invalidated. Any flush progress also clears suspect status, so a shape that answered is re-challenged rather than invalidated if it stalls again (relevant when the grace period is shorter than the 1s check-interval floor). Net effect: zero steady-state messaging (challenges only fire on the rare stall-suspect path), the backstop stays fully armed — a wedged consumer cannot answer — at the cost of one extra check interval of latency before a genuinely wedged-alive shape is culled. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e-based interval The initial timer armed in init/1 used the fixed 10s interval while every re-arm clamps to the configured grace period, so a sub-10s ELECTRIC_FLUSH_STALL_GRACE_PERIOD was not enforced at its own granularity for the first window after startup. Extract the clamp into schedule_stall_check/1 and use it from init/1 too. Addresses #4730 (comment) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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.
Problem
The FlushTracker pins
confirmed_flush_lsnfor the whole stack on the minimum incomplete flush entry, and cleanup of entries is purely event-driven: either new traffic routed to the same shape (publish-time DOWN detection) or an explicitShapeLogCollector.remove_shape/2arriving via the ShapeCleaner chain. Several real-world paths break both — a storage error that crashes the consumer and aborts itsterminate/2beforehandle_writer_terminationruns, a removal chain that dies between theShapeStatusremoval and the SLC removal (unretryable: the retry short-circuits at{:error, :data_removed}), untagged{:shutdown, x}exit reasons classified as benign, and a consumer that stays alive but stops flushing. On a subsequently-quiet table any of these pins the WAL ack forever, causing unbounded WAL growth on Postgres. PR #4713 has a minimal reproduction of the stall.Solution
The SLC becomes the authority on consumer-death handling for flush accounting. Invariant: an incomplete FlushTracker entry always has a live monitor watching the pid responsible for completing it, and a timestamp bounding how long it may sit without flush progress.
{:shutdown, :cleanup}) unpin only; bare:shutdown/:killedare assumed to be supervisor teardown and left alone.last_progress_attimestamp refreshed only by flush notifications. A periodic tick finds shapes whose entries make no flush progress forELECTRIC_FLUSH_STALL_GRACE_PERIOD(default 60s) and invalidates them via the normal ShapeCleaner path, re-arming on each fire so a lost cleanup chain retries instead of pinning forever. Misclassified DOWNs self-heal through the same mechanism.:verify_flush_progress) before being acted on. A consumer answers only while legitimately deferring flush notifications (buffering ahead of PG snapshot info, subquery move-in awaiting splice); the answer re-arms the entry's grace period so healthy deferral phases are not misread as stalls. Only a suspect still stalled and silent at the next check — or a shape whose writer is already dead and demonitored, which skips the challenge — is invalidated. Consumers stay silent in steady state: no periodic messaging, and a wedged consumer can't answer, so the backstop stays armed.handle_writer_terminationshrinks to exactly:normal/:killed/:shutdown, so untagged{:shutdown, x}reasons now trigger removal; tagged dependency-materializer shutdowns map tostop_and_clean; suspend additionally requires all txn state flushed and notified (guaranteeing a suspended consumer never leaves an incomplete entry behind);Consumer.stopescalates a wedged consumer to a kill after the stop call times out.ShapeStatus, healing pins left by removal chains that died partway through.Telemetry: new
[:electric, :flush_tracker, :writer_down]and[:electric, :flush_tracker, :stall_detected]events.🤖 Generated with Claude Code