fix: honor context cancel during bridge reconnect backoff - #5
Open
SebTardif wants to merge 1 commit into
Open
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. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
P2
|
Codex review: needs maintainer review before merge. Reviewed September 5, 2026, 6:59 AM ET / 10:59 UTC. ClawSweeper reviewWhat this changesMakes both bridge reconnect waits respond to shutdown signals and adds three tests for the cancellation-aware wait helper. Merge readiness✅ Ready for maintainer review Current main still has both uninterruptible waits, so this PR remains useful. The focused patch has sufficient real-process proof and no blocking findings. Priority: P2 Review scores
Verification
How this fits togetherClawgo is a headless client that connects local voice input and speech output to the gateway bridge. Its reconnect loop retries failed connections; shutdown signals should interrupt those waits and release discovery resources. flowchart TD
A[Bridge connection attempt] --> B{Connection succeeds?}
B -->|Yes| C[Active bridge session]
B -->|No| D[Reconnect backoff]
C -->|Connection drops| D
D -->|Timer expires| A
E[Shutdown signal] --> D
D -->|Canceled| F[Discovery cleanup and exit]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Shutdown should exit promptly during reconnect backoff while retaining the existing retry schedule and resource cleanup. Do we have a high-confidence way to reproduce the issue? Yes, from source: SIGINT during either current-main backoff cannot interrupt time.Sleep. The contributor supplies a matching before/after process trace; this read-only review did not execute a reproduction. Is this the best way to solve the issue? Yes. Selecting on the existing signal context is a narrow repair that preserves retry timing, exit status, and cleanup without adding configuration or changing the bridge protocol. AGENTS.md: not found in the target repository. Codex review notes: model internal, reasoning high; reviewed against c6e46796a1c8. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (30 earlier review cycles; latest 8 shown)
|
clawsweeper
Bot
added
rating: 🦞 diamond lobster
This was referenced Aug 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.