fix: honor context cancel during bridge reconnect backoff - #5
Conversation
Reconnect used time.Sleep(backoff) after connect failure and on reconnect, so SIGINT could not interrupt up to 15s. Replace both sleeps with sleepContext so run returns on ctx cancel. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
Codex review: needs maintainer review before merge. Reviewed August 26, 2026, 11:30 PM ET / August 27, 2026, 03:30 UTC. ClawSweeper reviewWhat this changesThe PR replaces fixed bridge-reconnect sleeps in Merge readinessThis remains a focused, correct fix: current Priority: P2 Review scores
Verification
Live VerificationCommand: Result: FAIL (failed) — execution before step 1 Assertions:
How this fits together
flowchart LR
Signal["SIGINT or SIGTERM"] --> Context["Run context"]
Config["Bridge address"] --> Connect["Bridge connection attempt"]
Connect --> Failed{"Connection failed?"}
Failed --> Wait["Cancelable retry wait"]
Context --> Wait
Wait --> Connect
Context --> Exit["Clean process exit"]
Before merge
Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Merge the focused cancellation fix after routine maintainer review so both bridge retry paths exit promptly on the existing signal context. Do we have a high-confidence way to reproduce the issue? Yes. Current main visibly enters fixed sleeps after both bridge failure paths, and the PR supplies an after-fix closed-local-port terminal run showing SIGINT exit during that wait. Is this the best way to solve the issue? Yes. A local timer-or-context helper is the narrowest maintainable repair and follows the existing cancellation-aware wait pattern already used in audio capture. AGENTS.md: not found in the target repository. Codex review notes: model internal, reasoning high; reviewed against 5f1b9d90abe2. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (25 earlier review cycles; latest 8 shown)
|
What Problem This Solves
clawgo runreconnects to the gateway bridge with exponential backoff (1s, doubling, capped at 15s). After a connect failure, and again on the reconnect path, the loop calledtime.Sleep(backoff). That sleep cannot be interrupted.The process already installs
signal.NotifyContextfor SIGINT and SIGTERM. The inner select already returns onctx.Done(). The two backoff sleeps did not, sorunstayed stuck until the current sleep finished (up to 15 seconds).Evidence
Live
go runof the old Sleep versus the new helper. Context already canceled. Requested wait 1500ms:Live
clawgo runagainst a closed port. SIGINT sent afterbridge connect failed(during the first reconnect backoff):Canceled helper behavior from
go test ./cmd/clawgo -run TestSleepContext -v(supplemental):Real behavior proof
clawgo runreconnect backoff usedtime.Sleep, so SIGINT could not stop the process until the current 1s-15s sleep finished.fix/reconnect-sleep-context, binary built from./cmd/clawgoto/tmp/clawgo-fixed, down bridge127.0.0.1:1.clawgo run -bridge 127.0.0.1:1 -mdns=false -tts-engine none -chat-subscribe=false. Waited forbridge connect failed. Sent SIGINT and measured time to exit. Also rango run /tmp/sleep-context-demo.goandgo test ./cmd/clawgo -run TestSleepContext -v.context canceledin 0s instead of sleeping 1.503s.ctxis canceled, matching the existingcase <-ctx.Done()path. Backoff math (1s, double, cap 15s) is unchanged.Summary
Call chain:
main->run->runNode-> connect failure orreconnect:label ->time.Sleep(backoff).runNodecreatesctxwithsignal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM). The inner select already handlesctx.Done(). The two Sleep calls did not.This has been present since
f601408(2026-01-04, 223 days).Related work:
modules/audioand the queue, not this reconnect loop. The audio helper landed on main asa86cdbb(sleepWithContext). This PR applies the same idea tocmd/clawgo.