fix(reef): stop leaking the inbox loop across channel crash restarts#110870
Conversation
openclaw-barnacle
Bot
added
gateway
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Codex review: needs real behavior proof before merge. Reviewed July 18, 2026, 1:59 PM ET / 17:59 UTC. Summary PR surface: Source +57, Tests +115. Total +172 across 4 files. Reproducibility: no. independent high-confidence current-main reproduction is available in the supplied evidence. The described failure chain is source-reproducible from the proposed lifecycle and supervisor behavior, but the reported live incident is not accompanied by a reusable reproduction transcript. Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land only after a maintainer reviews the cross-channel abort contract and the contributor supplies redacted after-fix runtime evidence showing a transient reconcile failure does not leave a second Reef inbox connection alive. Do we have a high-confidence way to reproduce the issue? No independent high-confidence current-main reproduction is available in the supplied evidence. The described failure chain is source-reproducible from the proposed lifecycle and supervisor behavior, but the reported live incident is not accompanied by a reusable reproduction transcript. Is this the best way to solve the issue? Likely yes: coupling Reef-owned loops under one lifecycle abort scope while also aborting every crashed supervisor task is a bounded defense at both ownership layers. A real after-fix runtime trace is still needed to confirm the relay behavior and teardown timing. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against e04ea7916c5b. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +57, Tests +115. Total +172 across 4 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
a5de694 to
adeb838
Compare
Yigtwxx
left a comment
Yigtwxx
left a comment
There was a problem hiding this comment.
Traced the cancellation path and it holds up: finally { lifecycle.abort(); ...; await inboxTask; } combined with the loop-head guard in start() (if (this.stopped || signal?.aborted) { await this.processing; return; }) means the old loop genuinely can't run an extra iteration, and abort propagates through abortableSleep, live()'s listener and workAbort with removeEventListener called on every path. The .finally() at server-channels.ts:865 does cover every terminal exit, not just the crash path. The three reef tests are all mutation-sensitive.
Two things inline — one gap in the rate-limit argument, one supervisor test that I don't think pins what its name claims.
On sibling parity, the Promise.all([longRunning, throwingLoop]) shape this fixes doesn't recur in other channels — irc, googlechat, nostr, matrix, slack, mattermost, msteams, feishu, sms, qqbot and the rest all own their cleanup via runStoppablePassiveMonitor / runPassiveAccountLifecycle / waitUntilAbort. Two adjacent spots have the same class of issue if you want them on a follow-up list, both pre-existing:
extensions/clickclack/src/gateway.ts:281— the abort path callsfinishSocketCycle()directly while the close path correctly drains viafinishAfterQueuedMessages(), so an in-flightprocessIncomingEventcan outlivestartAccount. Aborting the signal doesn't drain a turn already in flight, which is exactly what reef'sawait inboxTaskis for.extensions/signal/src/monitor.ts:719—await Promise.all([daemonLifecycle.stop(), monitorTaskRunner.waitForIdle()]): ifstop()rejects,Promise.allrejects immediately andwaitForIdle()is abandoned. The comment above it describes the invariant the code doesn't quite hold.extensions/qa-channel/src/gateway.ts:80has the shape that does.
| await runReefChannelLifecycle({ | ||
| parentSignal: ctx.abortSignal, | ||
| startInbox: (signal) => inbox.start(signal), | ||
| reconcile, |
There was a problem hiding this comment.
The periodic reconcile now gets onReconcileError, but the startup one at line 405 is still a bare await reconcile() outside runReefChannelLifecycle, with no try/catch.
That path is friends.reconcile() → transport.listFriends(), which throws ReefRelayError on a relay 429 (transport.ts:221). So while the relay is rate-limiting, the channel still dies here on every startAccount, and with MAX_RESTARTS = 10 (server-channels.ts:50) Reef gives up permanently after ten attempts.
Harmless for the leak this PR targets — line 405 runs before inbox is constructed, so there's no orphan — but it means the description's "the crash-restart cycle was itself the rate-limit amplifier" argument only half-lands: the WS handshake storm is gone, yet each restart attempt still fires a REST listFriends at the same rate-limited relay.
Making the startup reconcile best-effort too (log and continue, let the inbox start anyway) would close it, or it may be worth saying explicitly that startup reconcile is intentionally fail-fast.
|
|
||
| // A crashed startAccount can leave background work racing on its signal | ||
| // (e.g. a reconnect loop). The replacement must never overlap that lifetime. | ||
| expect(signals[0]?.aborted).toBe(true); |
There was a problem hiding this comment.
This asserts "eventually aborted", not "aborted before the replacement started" as the test name says — the check runs after advanceTimersUntil has already let call #2 through.
The new .finally() block at server-channels.ts:865 aborts the same controller and runs as a microtask right after await startChannelInternal(...), and advanceTimersUntil flushes microtasks each step (:183). So I think reverting the abort.abort() at :847 — the one this test exists to pin — would leave it green, because the .finally() abort still makes signals[0].aborted true by the time it's read.
Capturing the state inside the replacement call would make it load-bearing, e.g. push signals[0]?.aborted from within the second startAccount invocation and assert on that.
Related: of the three abort.abort() calls added, only :847 is covered — :795 (timed-out-stop restart) and :865 (terminal) have no test.
|
Merged via squash.
|
…penclaw#110870) * fix(reef): stop leaking the inbox loop across channel crash restarts * fix(reef): move channel lifecycle helper to its own module for lint/deadcode gates
2 participants
What Problem This Solves
The live
clawdgateway's Reef channel was stuck in a self-sustaining relay rate-limit storm: 2,400+reef inbox connection failederrors and ~140rate_limitedchannel crash/restart cycles per day, withcode=1012 reason=replaced by newer connectioncloses every ~600ms during bursts. Inbound delivery from peers (e.g.molty) was effectively dead while the relay 429'd every request, including WebSocket handshakes.Root cause chain:
reefPlugin.gateway.startAccountranPromise.all([inbox.start(ctx.abortSignal), reconciliationLoop()]). When the 30s friend-reconcile loop threw (e.g. a transient relay 429 →ReefRelayError("rate_limited")),Promise.allrejected and the channel exited — whileinbox.start()kept running in the background.src/gateway/server-channels.ts) never aborts the crashed task'sAbortControllerbefore starting the replacement (it only aborts on manual stop). The orphaned inbox loop therefore reconnected forever.1012 replaced by newer connection), each completed catch-up resets the reconnect backoff to 250ms, and the resulting connect storm drives the relay into rate limiting — which crashes the next reconcile, leaks another loop, and sustains the storm indefinitely.Why This Change Was Made
extensions/reef/src/channel.ts: newrunReefChannelLifecycleowns one abort scope for both account loops. Any exit path aborts the inbox loop and awaits its settlement beforestartAccountsettles, so no reconnect loop can outlive its account instance. It also inherits an already-aborted parent signal (listeners added after abort never fire). Periodic reconcile failures are now logged and retried instead of crashing the channel — the crash-restart cycle was itself the rate-limit amplifier.src/gateway/server-channels.ts: defense-in-depth for every channel plugin — the supervisor aborts the settled task's controller before starting a crash replacement (both restart paths) and on terminal cleanup, so two account instances can never share a live lifetime.User Impact
Reef channels no longer melt down into relay rate limiting after a single transient reconcile failure; inbound agent-to-agent delivery stays up. Any channel plugin that leaves background work racing on its abort signal after a crash is now cut off by the supervisor before the replacement starts.
Evidence
rate_limitedexits on 2026-07-18 (1,109 replacements the prior day), 1012-replacement bursts at ~600ms cadence, escalating to WS handshake 429s.node scripts/run-vitest.mjs extensions/reef— 24 files, 263 passed / 2 skipped (includes new lifecycle tests: reconcile-failure containment, teardown-before-settle, already-aborted parent inheritance).node scripts/run-vitest.mjs src/gateway/server-channels.test.ts— 51 passed (includes new "aborts the crashed task's signal before starting its replacement").gpt-5.6-sol, xhigh): clean after addressing one finding (already-aborted parent signal inheritance).