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 29, 2026, 9:59 AM ET / 13:59 UTC.

ClawSweeper review

What this changes

This PR makes the headless Clawgo node stop reconnect backoff promptly on SIGINT or SIGTERM and adds cancellation tests for the wait helper.

Merge readiness

⚠️ Ready for maintainer review - 1 item remains

Keep open for routine maintainer merge review: the focused patch fixes two current reconnect waits that ignore the node shutdown context, with no actionable defect found in the introduced code.

Priority: P2
Reviewed head: 1310d1869d48633854e56067789abe0b5a6d698d

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused reliability fix with direct terminal proof and targeted helper coverage.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): The changed production owner is runNode’s reconnect loop; the supplied macOS terminal trace exercises it through clawgo run with a refused local bridge and records prompt post-fix SIGINT exit.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The changed production owner is runNode’s reconnect loop; the supplied macOS terminal trace exercises it through clawgo run with a refused local bridge and records prompt post-fix SIGINT exit.
Evidence reviewed 5 items Current default-branch behavior: The current main revision creates a signal-backed context, but both failed-connect and post-disconnect paths call uninterruptible time.Sleep(backoff).
Introduced repair: The PR replaces both waits with a context-aware timer and preserves the existing mDNS cleanup before returning on cancellation.
Focused regression coverage: The new helper tests cover an already-canceled context, cancellation during a wait, and normal timer completion.
Findings None None.
Security None None.

How this fits together

clawgo run is the headless node client’s bridge connection loop. It receives process shutdown signals, retries unavailable bridge connections, and then starts the connected bridge session and optional chat routing.

flowchart LR
  A[Operator starts Clawgo node] --> B[Bridge connection loop]
  B --> C{Bridge available?}
  C -- no --> D[Cancelable reconnect wait]
  D --> B
  E[SIGINT or SIGTERM] --> D
  C -- yes --> F[Bridge session and chat routing]
Loading

Before merge

  • Complete next step (P2) - Routine maintainer merge review is the remaining action; no narrow repair-lane work is needed.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test scope production +27, tests +44 across 3 files The implementation is small and accompanied by targeted cancellation coverage.

Technical review

Best possible solution:

Merge the narrow context-aware wait change so bridge reconnection retains its existing backoff while shutdown remains prompt.

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

Yes, by source: current main has two time.Sleep(backoff) calls after a signal-backed context is created, and the PR supplies a closed-port SIGINT terminal trace for that path.

Is this the best way to solve the issue?

Yes. A local context-aware timer is the narrowest maintainable repair and leaves the established exponential-backoff behavior unchanged.

AGENTS.md: not found in the target repository.

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

Labels

Label justifications:

  • P2: This is a bounded reliability fix for a node reconnect workflow, without evidence of an emergency outage or data-loss impact.
  • 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 changed production owner is runNode’s reconnect loop; the supplied macOS terminal trace exercises it through clawgo run with a refused local bridge and records prompt post-fix SIGINT exit.
  • proof: sufficient: Contributor real behavior proof is sufficient. The changed production owner is runNode’s reconnect loop; the supplied macOS terminal trace exercises it through clawgo run with a refused local bridge and records prompt post-fix SIGINT exit.

Evidence

What I checked:

  • Current default-branch behavior: The current main revision creates a signal-backed context, but both failed-connect and post-disconnect paths call uninterruptible time.Sleep(backoff). (cmd/clawgo/main.go:353, 5f1b9d90abe2)
  • Introduced repair: The PR replaces both waits with a context-aware timer and preserves the existing mDNS cleanup before returning on cancellation. (cmd/clawgo/main.go:353, 1310d1869d48)
  • Focused regression coverage: The new helper tests cover an already-canceled context, cancellation during a wait, and normal timer completion. (cmd/clawgo/sleep_test.go:10, 1310d1869d48)
  • Real behavior proof: The supplied PR body records a patched clawgo run against a closed local bridge exiting 0.028 seconds after SIGINT during reconnect backoff, versus 0.919 seconds for the old binary. (1310d1869d48)
  • Feature provenance: The reconnect-loop behavior appears to date to the routing-plugin commit by Mariano Belinky; later audio work established a similar context-aware wait pattern in a separate package. (cmd/clawgo/main.go:280, f60140892c55)

Likely related people:

  • Mariano Belinky: Introduced the current bridge reconnect loop and routing wiring that contains the original uninterruptible waits. (role: feature introducer; confidence: high; commits: f60140892c55; files: cmd/clawgo/main.go)
  • Peter Steinberger: Recent audio work introduced the repository’s existing context-aware delay pattern in the related capture loop. (role: adjacent pattern contributor; confidence: medium; 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.

History

Review history (29 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-25T21:48:27.834Z sha 1310d18 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-26T02:24:44.127Z sha 1310d18 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-26T10:04:18.325Z sha 1310d18 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-26T18:29:23.679Z sha 1310d18 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-27T03:30:54.400Z sha 1310d18 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-28T21:06:22.594Z sha 1310d18 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-29T06:03:56.346Z sha 1310d18 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-29T08:55:29.639Z sha 1310d18 :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. labels Aug 23, 2026
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