fix(sync-service): shed live shape subscribers that stop draining their mailbox#4672
fix(sync-service): shed live shape subscribers that stop draining their mailbox#4672robacourt wants to merge 2 commits into
Conversation
…ir mailbox A live `GET /v1/shape` request whose client can't keep up — a stalled or dead socket while changes keep streaming — registered as a change subscriber and was notified via a fire-and-forget `send/2` per transaction, with no backpressure to the producer and no cap on the subscriber's mailbox. The request process, blocked writing to the socket, stopped draining its mailbox while the consumer kept enqueuing `:new_changes`, so the message queue (and the reference-counted binary each message pins) grew without bound until the node OOMed. Observed in production as a single `GET /v1/shape` process with a ~200k message queue and ~3.9 GB of `vm.memory.binary`. Fix (producer-side mailbox watermark): before notifying each subscriber, `Consumer.notify_clients_of_new_changes/2` checks the subscriber's `message_queue_len`; if it exceeds `:slow_subscriber_max_queue_len` the subscriber is terminated instead of notified. The client reconnects and resumes from its last offset (the live protocol is resumable), so shedding is safe. This is the only mechanism that catches a handler blocked inside `Plug.Conn.chunk` — it can't wake to check itself, but the producer inspects it externally. The watermark is always on (a positive integer, default 10,000; non-positive values are rejected at stack startup) and tunable via the `ELECTRIC_SLOW_SUBSCRIBER_MAX_QUEUE_LEN` env var. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4672 +/- ##
==========================================
- Coverage 60.15% 60.01% -0.15%
==========================================
Files 410 395 -15
Lines 44348 43747 -601
Branches 12582 12583 +1
==========================================
- Hits 26677 26254 -423
+ Misses 17593 17414 -179
- Partials 78 79 +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:
|
Claude Code ReviewSummaryThis PR adds producer-side mailbox-watermark shedding to Previous Review StatusThe one Important issue from iteration 1 — "the reproduction test does not exercise the actual production failure state" — is resolved by The rewritten
Nicely done — this is a more convincing regression test than I suggested. Issues FoundCritical (Must Fix)None. Important (Should Fix)None. (The iteration-1 Important item is resolved above; no new ones introduced.) Suggestions (Nice to Have)New, on the added test (both very minor):
Still open from iteration 1 (implementation unchanged, all nice-to-have — repeating briefly, not re-litigating):
Issue ConformanceNo linked issue (flagged per convention), but the PR description thoroughly documents the production incident, Honeycomb evidence, and the rationale for producer-side shedding over the deferred alternatives (coalescing / send-timeout). The implementation matches the described solution, the changeset ( Review iteration: 2 | 2026-07-02 |
Address review: the reproduction simulated the stuck subscriber with Process.sleep(:infinity) (idle in receive), which is a different scheduler state from the production failure — a handler blocked inside Plug.Conn.chunk. It proved the watermark comparison, not that the mechanism fires against a genuinely socket-blocked handler, and didn't cover the risk that Process.info(_, :message_queue_len) could itself stall on the blocked target and freeze the per-shape consumer. The subscriber is now blocked in a real :gen_tcp.send to a peer that never reads — the actual production state (`:waiting` in the inet_reply receive, verified on OTP 28). The shed still firing proves process_info did not block the consumer; an added assertion registers a healthy subscriber afterwards and confirms it is still notified, explicitly guarding consumer liveness. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| # A live subscriber (e.g. a `GET /v1/shape` request) that can't drain its | ||
| # mailbox — a stalled or dead client socket while changes keep streaming — | ||
| # would otherwise accumulate one `:new_changes` message per transaction | ||
| # without bound, pinning reference-counted binary until the node runs out of | ||
| # memory. Once its mailbox crosses the watermark we stop notifying it and | ||
| # terminate it; the client reconnects and resumes from its last offset. |
There was a problem hiding this comment.
Here's what looks suspicios to me in this approach:
When processing a shape request, the request handler process enters the receive state until it receive a single :new_changes message. At that point it serves the shape log from storage until the end and closes the HTTP response. There's no looping and there's no expectation that the request handler process can do anything with the 2nd or any subsequent :new_changes message.
So how does a handler process end up with 10k messages in its inbox? To me the more likely explanation is that this is a bug, partly caused by a leaky abstraction: the subscription for changes is initiated by a call to Registry.register() which subscribes the current process. A single request's lifetime is usually smaller than that of a process because the same Bandit process may handle multiple requests. So the process ends up receiving messages from a consumer after it has already finished processing the request and has returned to Bandit's pool of request handlers.
This also highlights another problem: once the current request finishes and the same process picks up a new request, it will get a new subscription ref and will use it for pattern matching in its receive expression. So any messages from the previous subscription will remain in the mailbox and will cause repeated compute tax on the request handler process while it is doing selective receive using a different ref value.
There was a problem hiding this comment.
Thanks @alco , I should have spotted that! As for your proposed alternative cause, I'm looking into it.
2 participants
Summary
Fixes an unbounded-memory / OOM failure mode in the live shape-streaming path. A live
GET /v1/shapesubscriber whose client can't keep up would grow its process mailbox without bound until the node ran out of memory.Problem
A live request registers as a change subscriber. On every transaction the consumer notifies each subscriber with a fire-and-forget
send/2(Consumer.notify_clients_of_new_changes/2→Registry.dispatch), with no backpressure to the producer and no cap on the subscriber's mailbox. The only backpressure is the socket write (Plug.Conn.chunk), which blocks the request process on a stalled/dead client — so the process stops draining its mailbox while the producer keeps enqueuing:new_changes. The message queue, and the reference-counted binary each queued message pins, grows without bound.Observed in production (Honeycomb):
GET /v1/shapeprocess withvm.monitor.long_message_queue.length≈ 200k (MAX == SUMfor thatprocess_type→ one process).Solution
Producer-side mailbox watermark. Before notifying each subscriber, the consumer checks its
message_queue_len; if it exceeds:slow_subscriber_max_queue_len, the subscriber is terminated instead of notified. The client reconnects and resumes from its last offset (the live protocol is resumable), so shedding is safe.This is the only mechanism that catches a handler blocked inside
Plug.Conn.chunk— it can't wake to check itself, but the producer inspects it externally. The watermark is always on (a positive integer, default 10,000; non-positive values are rejected at stack startup) and tunable viaELECTRIC_SLOW_SUBSCRIBER_MAX_QUEUE_LEN.Changes
lib/electric/shapes/consumer.ex— watermark check + shed in the notify loop.ELECTRIC_SLOW_SUBSCRIBER_MAX_QUEUE_LEN(runtime.exs,config.ex,application.ex,stack_supervisor.ex,stack_config.ex), mirroring the existingELECTRIC_CONSUMER_GC_HEAP_THRESHOLDpath.Test Plan
consumer_test.exs,config_test.exs,stack_supervisor_test.exs,shape_cache_test.exs,shape_log_collector_test.exs,partitioned_tables_test.exsall pass (no regressions on the notify path).mix compile --warnings-as-errorsclean.Generated with Claude Code