fix(reef): contain startup friend reconcile failures#110918
Conversation
openclaw-barnacle
Bot
added
channel: reef
|
@clawsweeper re-review Rewrote the description against the PR template after the initial |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
proof: sufficient
|
Codex review: needs maintainer review before merge. Reviewed July 18, 2026, 6:10 PM ET / 22:10 UTC. Summary PR surface: Source +95, Tests +232, Docs +1. Total +328 across 6 files. Reproducibility: yes. source inspection and the submitted live relay-429 transcript provide a high-confidence path: make the startup friends request return 429 and compare account activation before and after the shared lifecycle path. Review metrics: 1 noteworthy metric.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review detailsBest possible solution: Land the shared lifecycle path only after the Reef owner confirms that using persisted peer keys during a retryable startup refresh failure is the intended availability policy, with definitive failures still stopping startup. Do we have a high-confidence way to reproduce the issue? Yes, source inspection and the submitted live relay-429 transcript provide a high-confidence path: make the startup friends request return 429 and compare account activation before and after the shared lifecycle path. Is this the best way to solve the issue? Unclear until a Reef owner confirms the intended stale-peer-key policy; the shared lifecycle is the narrowest implementation if that policy is accepted, but it is not solely a mechanical bug fix. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 01c64793115d. Label changesLabel justifications:
Evidence reviewedPR surface: Source +95, Tests +232, Docs +1. Total +328 across 6 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
Review history (6 earlier review cycles)
|
The periodic reconcile treats a transient relay failure as non-fatal, but the startup reconcile was a bare await outside the lifecycle. A relay 429 there rejects startAccount, and the supervisor restart that follows is itself what escalates relay rate limiting -- so Reef gives up permanently after MAX_RESTARTS while the relay is still throttling. Move the startup reconcile into runReefChannelLifecycle so both paths share one failure policy, and activate through an onReady hook to preserve the existing ordering: refresh peer keys, activate, then start the inbox.
e5f83f8 to
8b8235f
Compare
clawsweeper
Bot
added
rating: 🦞 diamond lobster
|
CI note: I force-pushed once to refresh the checks (the earlier
The assertion that fails is I have not pushed again to re-roll it, since that only adds churn and risks a different flake. Happy to rebase if you would rather see a clean run before merging. |
clawsweeper
Bot
added
rating: 🐚 platinum hermit
Co-authored-by: Yigtwxx <[email protected]>
# Conflicts: # CHANGELOG.md
|
Merged via squash.
|
github-project-automation
Bot
moved this from Todo
to Done
in Upstream OSS contributions
* fix(reef): contain startup friend reconcile failures The periodic reconcile treats a transient relay failure as non-fatal, but the startup reconcile was a bare await outside the lifecycle. A relay 429 there rejects startAccount, and the supervisor restart that follows is itself what escalates relay rate limiting -- so Reef gives up permanently after MAX_RESTARTS while the relay is still throttling. Move the startup reconcile into runReefChannelLifecycle so both paths share one failure policy, and activate through an onReady hook to preserve the existing ordering: refresh peer keys, activate, then start the inbox. * fix(reef): harden reconcile recovery Co-authored-by: Yigtwxx <[email protected]> * docs(changelog): credit Reef reconcile fix * test(reef): satisfy exact-head validation * test(reef): satisfy lint contract --------- Co-authored-by: Peter Steinberger <[email protected]>
2 participants
Related: #110870
What Problem This Solves
Fixes an issue where operators running the Reef channel against a relay that is rate limiting would see Reef stop permanently and stay down until the process was restarted by hand, instead of riding out the throttling and recovering on its own.
While the relay returns 429, starting the Reef account fails outright. The gateway supervisor restarts it, each restart hits the same throttled relay again, and after ten attempts Reef gives up for the lifetime of the process — logging
giving up after 10 restart attempts. Because every attempt issues another relay request, the restart cycle also feeds the throttling that caused it.#110870 fixed exactly this amplifier for the periodic reconcile that runs every 30s, but the reconcile that runs once at account startup was not covered by that change and still fails the whole account.
Why This Change Was Made
The startup and periodic reconcile are the same relay call with two different failure policies: the periodic one reports the error and keeps the channel alive, the startup one rejects
startAccount. This change gives them one shared policy, in the module that already owns the account lifecycle.The startup reconcile moves into
runReefChannelLifecycle, and activation moves behind a newonReadyhook so the original ordering is preserved exactly — refresh peer keys, activate, then start the inbox — since an agent turn must not use Reef outbound before peer keys are refreshed. Both call sites now route through one internalrunReconcileStephelper.Deliberate boundary: on a startup failure the account continues with the peer keys already persisted, which is the same staleness the periodic path already tolerates between intervals, and the next interval retries. Two adjacent fixes fall out of the restructure — the abort listener on
parentSignalis no longer leaked if a startup step throws, and an already-aborted account no longer issues a relay request on its way out.User Impact
Reef survives relay rate limiting instead of dying to it. When the relay throttles at startup, the account now comes up, logs
reef friend reconcile failed: ..., serves inbound and outbound messages with the peer keys it already has, and refreshes them on the next 30s reconcile once the relay recovers. Operators no longer need to restart the process to get Reef back, and Reef stops adding restart traffic to a relay that is already throttling.No configuration change. The only behavior difference on the success path is that peer keys may briefly be one interval stale after a failed startup refresh.
Evidence
Live run against a local relay returning
429 {"error":"rate_limited"}onGET /v1/friends, driving the realReefTransportClient— real request signing, realReefRelayError— through both startup sequences:Harness scope: it drives the real transport, signing and lifecycle, and substitutes the trust-store bookkeeping that wraps
listFriendsinsidefriends.reconcile(). That bookkeeping is not what throws, and this machine cannot open the plugin state DB (see the note below).Tests — two added to
extensions/reef/src/channel.test.ts:activates and starts the inbox when the startup reconcile failscaptures the reconcile and error counts insideonReady, so it pins the startup reconcile specifically rather than "some reconcile eventually failed" after the periodic loop has had a chance to run.refreshes peer keys before activating and before the inbox startspins thereconcile → ready → inboxordering.Both are load-bearing — restoring a bare
await params.reconcile()at startup fails them:tears down the inbox loop before settling when a loop branch throwsneeded one adjustment. It drives its failure throughonReconcileErrorthrowing, which now also fires on the startup step, so the inbox would never start and there would be nothing to tear down. Its startup reconcile now succeeds and the failure lands on the periodic loop with the inbox running, which is the case that test was written to cover.Local checks:
oxlintandoxfmt --checkclean on the three files,tsgo --noEmitclean,extensions/reef/src/channel.test.ts7 passed / 1 failed.That one failure is
exposes locally trusted peers as routable directory entries, which fails identically on unmodifiedorigin/mainhere (1 failed / 5 passed): this machine runs Node 22.20.0, whose embedded SQLite 3.50.4 is below the WAL-safety floor the plugin state store requires. Unrelated to this change. Themax-linesratchet likewise reportsui/src/components/lobster-pet.tson this base with and without this change; not touched here.AI-assisted.