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 26, 2026, 11:30 PM ET / August 27, 2026, 03:30 UTC.

ClawSweeper review

What this changes

The PR replaces fixed bridge-reconnect sleeps in clawgo run with cancellation-aware waits and adds focused helper tests.

Merge readiness

⚠️ Ready for maintainer review - 1 item remains

This remains a focused, correct fix: current main still has two uninterruptible reconnect waits, while the unchanged PR head makes both waits respect the signal context and includes credible live terminal proof.

Priority: P2
Reviewed head: 1310d1869d48633854e56067789abe0b5a6d698d

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A small, focused patch with direct terminal proof, regression tests, and no concrete review finding.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (terminal): The PR body provides an after-fix terminal run of the built client against a closed local bridge port, showing SIGINT exit during retry backoff; it also supplies focused supplemental tests.
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 an after-fix terminal run of the built client against a closed local bridge port, showing SIGINT exit during retry backoff; it also supplies focused supplemental tests.
Evidence reviewed 5 items Current-main behavior: Current main creates a signal-cancelable context but then calls time.Sleep(backoff) after an initial bridge connection failure and after a dropped connection, leaving those waits uninterruptible.
PR implementation: The proposed helper selects between the retry timer and ctx.Done(), and both current-main sleep sites are replaced with it while retaining the existing backoff progression.
Established local pattern: The audio path already uses the same timer-or-context pattern for cancellation-aware waits, supporting this narrow implementation approach without changing the bridge protocol.
Findings None None.
Security None None.

Live Verification

Command: go build -o /tmp/clawgo-proof ./cmd/clawgo

Result: FAIL (failed) — execution before step 1 run: sh -lc pnpm install --ignore-scripts --frozen-lockfile failed: ! Corepack is about to download https://registry.npmjs.org/pnpm/-/pnpm-11.24.0.tgz

sh -lc pnpm install --ignore-scripts --frozen-lockfile failed: ! Corepack is about to download https://registry.npmjs.org/pnpm/-/pnpm-11.24.0.tgz

Assertions:

  • FAIL expect_output: bridge connect failed

How this fits together

clawgo run connects a local node to the gateway bridge and retries failed connections. SIGINT or SIGTERM cancels its run context, which should stop a pending retry before process exit.

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"]
Loading

Before merge

  • Complete next step (P2) - No repair-lane action is needed: there is no actionable defect in the proposed patch, and routine maintainer merge review remains.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta production +29, tests +44 The small production helper and two call-site substitutions are covered by more focused test code than production growth.

Technical review

Best 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.

Labels

Label justifications:

  • P2: This is a bounded shutdown-responsiveness bug in bridge retry handling, with limited operational blast radius.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit 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 an after-fix terminal run of the built client against a closed local bridge port, showing SIGINT exit during retry backoff; it also supplies focused supplemental tests.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides an after-fix terminal run of the built client against a closed local bridge port, showing SIGINT exit during retry backoff; it also supplies focused supplemental tests.

Evidence

What I checked:

  • Current-main behavior: Current main creates a signal-cancelable context but then calls time.Sleep(backoff) after an initial bridge connection failure and after a dropped connection, leaving those waits uninterruptible. (cmd/clawgo/main.go:353, 5f1b9d90abe2)
  • PR implementation: The proposed helper selects between the retry timer and ctx.Done(), and both current-main sleep sites are replaced with it while retaining the existing backoff progression. (cmd/clawgo/sleep.go:8, 1310d1869d48)
  • Established local pattern: The audio path already uses the same timer-or-context pattern for cancellation-aware waits, supporting this narrow implementation approach without changing the bridge protocol. (modules/audio/line_capture.go:180, a86cdbb8c5ee)
  • Review continuity: The three changed files are identical to the previous ClawSweeper-reviewed head, which had no outstanding findings. (cmd/clawgo/main.go:353, 1310d1869d48)
  • Feature history: The historical routing feature commit is authored by Mariano Belinky, making that author the strongest available routing-path contact. (cmd/clawgo/main.go, f60140892c55)

Likely related people:

  • Mariano Belinky: The available feature-history record identifies Mariano Belinky as author of the early routing feature commit associated with the bridge run path. (role: routing feature author; confidence: medium; commits: f60140892c55; files: cmd/clawgo/main.go)
  • Peter Steinberger: Peter Steinberger added the current-main cancellation-aware wait pattern in the audio capture loop, a directly adjacent implementation precedent. (role: recent adjacent 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 (25 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-24T21:02:39.909Z sha 1310d18 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-25T00:07:52.014Z sha 1310d18 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-25T06:04:08.661Z sha 1310d18 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-25T12:22:44.683Z sha 1310d18 :: needs maintainer review before merge. :: none
  • 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

@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