Skip to content

fix: honor context cancel during bridge reconnect backoff - #5

Open
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/reconnect-sleep-context
Open

fix: honor context cancel during bridge reconnect backoff#5
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/reconnect-sleep-context

Conversation

@SebTardif

Copy link
Copy Markdown

What Problem This Solves

clawgo run reconnects to the gateway bridge with exponential backoff (1s, doubling, capped at 15s). After a connect failure, and again on the reconnect path, the loop called time.Sleep(backoff). That sleep cannot be interrupted.

The process already installs signal.NotifyContext for SIGINT and SIGTERM. The inner select already returns on ctx.Done(). The two backoff sleeps did not, so run stayed stuck until the current sleep finished (up to 15 seconds).

Evidence

Live go run of the old Sleep versus the new helper. Context already canceled. Requested wait 1500ms:

$ go run /tmp/sleep-context-demo.go
canceled context, requested wait 1500ms
old time.Sleep err=<nil> elapsed=1.503s
sleepContext    err=context canceled elapsed=0s

Live clawgo run against a closed port. SIGINT sent after bridge connect failed (during the first reconnect backoff):

$ /tmp/clawgo-old run -bridge 127.0.0.1:1 -mdns=false -tts-engine none -chat-subscribe=false
bridge connect failed: dial tcp 127.0.0.1:1: connect: connection refused
SIGINT: process exited 0.919s later (remainder of the 1s Sleep)

$ /tmp/clawgo-fixed run -bridge 127.0.0.1:1 -mdns=false -tts-engine none -chat-subscribe=false
bridge connect failed: dial tcp 127.0.0.1:1: connect: connection refused
SIGINT: process exited 0.028s later

Canceled helper behavior from go test ./cmd/clawgo -run TestSleepContext -v (supplemental):

$ go test ./cmd/clawgo -run TestSleepContext -count=1 -timeout 15s -v
=== RUN   TestSleepContextCanceledReturnsCanceled
--- PASS: TestSleepContextCanceledReturnsCanceled (0.00s)
=== RUN   TestSleepContextCancelDuringWait
--- PASS: TestSleepContextCancelDuringWait (0.00s)
=== RUN   TestSleepContextCompletesWhenContextStaysOpen
--- PASS: TestSleepContextCompletesWhenContextStaysOpen (0.00s)
PASS
ok  	github.com/clawdbot/clawgo/cmd/clawgo	2.021s

Real behavior proof

  • Behavior or issue addressed: clawgo run reconnect backoff used time.Sleep, so SIGINT could not stop the process until the current 1s-15s sleep finished.
  • Real environment tested: macOS, Go 1.26.5, branch fix/reconnect-sleep-context, binary built from ./cmd/clawgo to /tmp/clawgo-fixed, down bridge 127.0.0.1:1.
  • Exact steps or command run after this patch: Built the binary. Started clawgo run -bridge 127.0.0.1:1 -mdns=false -tts-engine none -chat-subscribe=false. Waited for bridge connect failed. Sent SIGINT and measured time to exit. Also ran go run /tmp/sleep-context-demo.go and go test ./cmd/clawgo -run TestSleepContext -v.
  • Evidence after fix: terminal output from the patched binary and helper. After the patch, SIGINT during backoff returned in 0.028s. On an already-canceled context the helper returned context canceled in 0s instead of sleeping 1.503s.
  • Observed result after fix: reconnect backoff now returns when ctx is canceled, matching the existing case <-ctx.Done() path. Backoff math (1s, double, cap 15s) is unchanged.
  • What was not tested: pairing against a live remote gateway, and SIGINT after backoff has already reached the 15s cap.

Summary

Call chain: main -> run -> runNode -> connect failure or reconnect: label -> time.Sleep(backoff).

runNode creates ctx with signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM). The inner select already handles ctx.Done(). The two Sleep calls did not.

This has been present since f601408 (2026-01-04, 223 days).

Related work:

  • Closed #2 mentioned cancelable backoff but changed modules/audio and the queue, not this reconnect loop. The audio helper landed on main as a86cdbb (sleepWithContext). This PR applies the same idea to cmd/clawgo.
  • kubernetes/kubernetes#53245 (context-aware backoff)

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>
@clawsweeper

clawsweeper Bot commented Aug 15, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Aug 15, 2026
@clawsweeper

clawsweeper Bot commented Aug 15, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed August 15, 2026, 5:16 PM ET / 21:16 UTC.

ClawSweeper review

What this changes

This PR replaces the two bridge reconnect time.Sleep calls with a context-aware timer and adds cancellation-focused tests.

Merge readiness

⚠️ Ready for maintainer review - 1 item remains

Keep open: current main still uses uninterruptible backoff sleeps in both bridge reconnect paths, while this focused patch makes those waits honor the run context and includes credible live proof.

Priority: P2
Reviewed head: 1310d1869d48633854e56067789abe0b5a6d698d

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused, source-consistent repair with direct live terminal proof and targeted helper coverage.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): The PR body provides after-fix terminal evidence from a real binary against a closed local bridge port, showing prompt SIGINT exit during reconnect backoff.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The PR body provides after-fix terminal evidence from a real binary against a closed local bridge port, showing prompt SIGINT exit during reconnect backoff.
Evidence reviewed 5 items Current main still blocks shutdown during backoff: The current reconnect loop creates a signal-derived context but calls time.Sleep(backoff) after a connection failure and after a connected bridge fails, so cancellation is not observed until the sleep ends.
Patch covers both affected branches: The PR replaces both blocking waits with sleepContext, returning after cleanup when the signal context is canceled; the helper selects between the context and timer.
Adjacent established pattern: Current main already uses the same timer-and-context-select pattern for audio path-loop retries, so the PR follows an established local cancellation approach without coupling packages.
Findings None None.
Security None None.

How this fits together

clawgo run maintains a TCP connection to the gateway bridge. Connection failures and dropped connections enter exponential backoff; SIGINT or SIGTERM should cancel that wait and allow the command to exit promptly.

flowchart LR
  Signals[Shutdown signals] --> Run[clawgo run]
  Run --> Connect[Bridge connection]
  Connect --> Failure[Connection failure]
  Failure --> Wait[Cancelable backoff wait]
  Wait --> Connect
  Wait --> Exit[Prompt shutdown]
Loading

Before merge

  • Complete next step (P2) - No repair lane is needed because this open PR already contains the focused, evidence-backed fix.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Focused change size production +29/-2, tests +44 across 3 files The patch is narrowly scoped to the two blocking waits and directly exercises the new cancellation helper.

Technical review

Best possible solution:

Merge the narrow context-aware wait so signal cancellation interrupts both bridge reconnect backoff paths while preserving the existing one-second-to-fifteen-second backoff schedule.

Do we have a high-confidence way to reproduce the issue?

Yes—source inspection shows the signal context is established before both time.Sleep(backoff) calls on current main, and the PR supplies a concrete closed-port SIGINT reproduction.

Is this the best way to solve the issue?

Yes—the local context-aware timer is the narrowest maintainable repair, preserves the existing backoff calculation, and matches the established audio retry pattern.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning high; reviewed against 5f1b9d90abe2.

Labels

Label changes:

  • add P2: Uninterruptible reconnect backoff delays normal CLI shutdown by up to fifteen seconds, but the patch has a limited, localized blast radius.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides after-fix terminal evidence from a real binary against a closed local bridge port, showing prompt SIGINT exit during reconnect backoff.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body provides after-fix terminal evidence from a real binary against a closed local bridge port, showing prompt SIGINT exit during reconnect backoff.

Label justifications:

  • P2: Uninterruptible reconnect backoff delays normal CLI shutdown by up to fifteen seconds, but the patch has a limited, localized blast radius.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body provides after-fix terminal evidence from a real binary against a closed local bridge port, showing prompt SIGINT exit during reconnect backoff.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides after-fix terminal evidence from a real binary against a closed local bridge port, showing prompt SIGINT exit during reconnect backoff.

Evidence

What I checked:

  • Current main still blocks shutdown during backoff: The current reconnect loop creates a signal-derived context but calls time.Sleep(backoff) after a connection failure and after a connected bridge fails, so cancellation is not observed until the sleep ends. (cmd/clawgo/main.go:353, 5f1b9d90abe2)
  • Patch covers both affected branches: The PR replaces both blocking waits with sleepContext, returning after cleanup when the signal context is canceled; the helper selects between the context and timer. (cmd/clawgo/sleep.go:9, 1310d1869d48)
  • Adjacent established pattern: Current main already uses the same timer-and-context-select pattern for audio path-loop retries, so the PR follows an established local cancellation approach without coupling packages. (modules/audio/line_capture.go:180, 5f1b9d90abe2)
  • Feature history: History for the current bridge command traces the reconnect-bearing file to Mariano Belinky's January routing and command work; the later audio cancellation helper was added by Peter Steinberger. (cmd/clawgo/main.go, f60140892c55)
  • Real behavior proof: The PR body records a patched binary against a closed local bridge port: SIGINT during reconnect backoff exited in 0.028s, compared with 0.919s for the prior binary, plus focused helper test output. (1310d1869d48)

Likely related people:

  • Mariano Belinky: The available history for cmd/clawgo/main.go places the reconnect flow in Mariano Belinky's January command and routing work. (role: original bridge-command contributor; confidence: high; commits: f60140892c55, 36d4909cbd34; files: cmd/clawgo/main.go)
  • Peter Steinberger: Peter Steinberger added the current-main context-aware timer pattern in the audio retry loop, which this patch mirrors locally. (role: recent adjacent contributor; confidence: high; commits: a86cdbb8c5ee; files: modules/audio/line_capture.go)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant