Repository navigation
fix(relay): never close connections when cross-node fan-out falls behind - #9302
Conversation
Multi-node fan-out closed every recipient of an event it delivered late (after a 30 s deadline), dropped from a full worker queue, or could not access-check, and every subscribed connection when the dispatcher lagged. A database stall slows every node's deliveries at once, so these closes landed fleet-wide together, and the reconnects (AUTH, membership reads, NIP-11, REST catch-up) hit the same database while it was still slow. Keep the dispatcher with 16 ordered workers and the bulk membership read. Remove the delivery deadline, the live fence, the rate-limited closer, the queue-full closes and the access-check retry loop. A slow delivery now arrives late. A queue-full drop or dispatcher lag is counted and resyncs the affected artifact syncs (NIP-AR Sync rule 5), as before the closer. Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Luke Tornquist <[email protected]>
🔐 Codex Security Review
Review SummaryOverall Risk: MEDIUM
Findings[MEDIUM]
|
Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Luke Tornquist <[email protected]>
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Approving. Read the full diff at aa2bf78: the new multinode_fanout.rs, the deliver_pubsub_event change back to the shared route_fanout_matches path, and the ConnectionManager / SubscriptionRegistry removals.
- The Redis path and the local path now run the same access gate, change-signal and divert steps, so a sync can't be skipped on either; an unverified recipient's sync still gets
resync: access check failed. - Queue-full drops resync only the syncs the event would have reached, and the local-echo check runs before queueing, so echoes aren't counted as drops. Dispatcher lag resyncs every sync. Nothing closes a connection except the existing per-connection backpressure limit, which is back to its pre-#9195 form.
- No leftover references to the removed fence,
LagCloser,received_atonChannelEvent, orsubscribed_connections.
On the Codex comment: its first point (a plain subscription isn't told about a dropped event) is the trade-off the description states, and I agree with it. Paced closes still all fire at once across the fleet during a database stall, which is worse than an occasional gap that the counters will surface.
Nonblocking: its second point is narrow but real. With the delivery deadline gone, a worker waits on its database read with no client-side limit. Reader connections have no statement_timeout, and a half-open TCP connection after checkout isn't covered by the acquire timeout. In that case the 1/16 of channels hashed to that worker stop until the kernel gives up on the socket; meanwhile their queue fills and drops (counted, syncs resync). The local fan-out path has the same exposure, so this isn't new. If you want to close it, wrap deliver_pubsub_event in the worker with a generous tokio::time::timeout that counts a drop and resyncs the matched syncs on expiry, without closing anything.
…e-mode * origin/main: ci(docker): skip relay image publishes for main pushes outside the build context (#9310) Add manual-only relay image publishing (#9303) fix(mobile): bump Flutter to 3.47.9 for Android TLS fix (#9309) Serve video and poster previews for invite links (#9306) docs: point the README to Buzz 1.0 in block/buzz-app (#9296) fix(db): give the replica-fence probe its own pool and instrument it (#9297) fix(relay): never close connections when cross-node fan-out falls behind (#9302) test: fix timing-dependent unit tests in buzz-cli, buzz-agent and buzz-admin (#9273) fix(ci): classify hermit retry timeouts by watchdog kill, not whole seconds (#9274) test(ios): wait for a key, foreground-active window before presenting in RunnerTests (#9275) test(desktop): wait on real readiness signals in audience and community-rail specs (#9277) Signed-off-by: Will Pfleger <[email protected]>
**Context:** #9302 (merged) stopped the relay from closing connections when cross-node fan-out falls behind. It also removed the 30 s delivery deadline. This PR is the fix from the review of #9302. **Problem:** Since #9302, a fan-out worker waits for the access-gate database read with no time limit. If a database connection stops answering in the middle of that read, the worker never continues. Its queue fills, and one sixteenth of the node's channels get no live events from other nodes until the relay restarts. Before #9302 the deadline cut that delivery off after 30 s. **Fix:** Each delivery attempt has a 30 s limit (`MAX_DELIVERY_ATTEMPT`), measured from when the worker starts it. When the limit passes, the relay counts the event in `buzz_multinode_fanout_dropped_total{reason="stale"}`, resyncs the artifact syncs the event matched, and continues with the next event. It closes no connection. An event that only waits in the queue is still delivered late, as #9302 intends. The queue-full drop and the new stale drop share one helper. ### Validation - Added test: `hung_delivery_is_dropped_at_the_bound_and_the_worker_moves_on` drives the production worker with a database that accepts the connection and never answers. It checks that the delivery stops at the limit, is counted, resyncs the matched sync, closes nothing, and that the next event on the same worker is delivered. With the limit removed it fails (the worker waits out the pool's one-hour acquire timeout instead). - The other `handlers::multinode_fanout` tests pass. The full local `buzz-relay` suite was not run; CI on this head covers it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Tom Brow <[email protected]> Co-authored-by: Tom Brow <[email protected]>
Summary
Cross-node fan-out no longer closes connections when it falls behind.
Before this change, an event closed its local recipients when:
Dispatcher lag also closed every subscribed connection. A rate-limited closer spread the closes out, at 1% of a node's connections per second with a floor.
The trouble is that a database stall slows every node's deliveries at once. So the closes landed on every node together. Each reconnect then does AUTH, membership reads, NIP-11 and REST catch-up against the database that was already slow. That turned a short stall into a reconnect storm and bursts of 503s. On small nodes the closer's floor dominated, so a node could close all of its connections within tens of seconds. Review-Proven Rule 9 (design every client-driven path for the whole fleet running it at once) is the reason: a server-forced close starts a fleet-wide reconnect.
What changes
Kept from #9195 and #9235:
(community, topic). Each channel stays in order and channels deliver concurrently.Removed:
ChannelEvent.received_at.fence_live_delivery,live_fenced).LagCloserand its rate limit.buzz_multinode_fanout_gate_retries_total).Behavior now:
buzz_multinode_fanout_dropped_total{reason="queue_full"}orbuzz_multinode_fanout_lag_total, and the affected artifact syncs getCLOSED "resync: live delivery lagged"(NIP-AR Sync rule 5). This is the same as before the closer existed.Trade-off: a plain subscription that misses a dropped event is not told. Its client may replay the event on its next reconnect, but not if later deliveries moved its resume cursor past the event by more than its replay slack (5 s for the ACP client). That was the behavior before #9195. Queue-full and lag drops stay visible in the counters, so we can see whether they actually happen before adding any recovery mechanism.
What got simpler
Net −700 lines.
multinode_fanout.rsdrops from ~940 lines to ~350.deliver_pubsub_eventloses its retry loop and goes back to the sharedroute_fanout_matchespath. The Redis path and the local path once again apply the access gate the same way.ConnectionManagerloses the fence flag and the split send helper.SubscriptionRegistrylosessubscribed_connections.Testing
handlers::multinode_fanout::tests:handlers::event::testsis unchanged apart from removingreceived_at.🤖 Generated with Claude Code