Skip to content

fix(install): bound curl connection setup with --connect-timeout 20#110657

Open
xydigit-zt wants to merge 2 commits into
openclaw:mainfrom
xydigit-zt:feat/issue/bound-installer-download-file
Open

fix(install): bound curl connection setup with --connect-timeout 20#110657
xydigit-zt wants to merge 2 commits into
openclaw:mainfrom
xydigit-zt:feat/issue/bound-installer-download-file

Conversation

@xydigit-zt

@xydigit-zt xydigit-zt commented Jul 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Scope reduced after rebase. Current main already bounds post-connect stalls in both installer download helpers with --speed-limit 1 --speed-time 30 (30-second low-speed grace window). This PR now adds only the remaining gap: --connect-timeout 20.

The stall-sensitive --speed-limit/--speed-time options only take effect after the connection is established. If the release host is blackholed or unresponsive during connection setup (SYN sent, no reply), curl hangs in the connect phase indefinitely — blocking first-time installs and openclaw upgrades. The wget sibling already covers this phase via --timeout=20, which applies to connection establishment as well.

Why This Change Was Made

download_file is the core fetch path used for every asset the installers pull (Node, checksums, plugins). curl's timeout model splits into two phases:

  • Connect phase: governed only by --connect-timeout — previously unbounded in the curl branch.
  • Transfer phase: already governed on main by --speed-limit 1 --speed-time 30 (abort only if speed stays below 1 byte/s for 30 seconds).

Adding --connect-timeout 20 bounds the connect phase without imposing any total download duration, so slow-but-progressing downloads remain unaffected. The existing 30-second low-speed grace window is preserved exactly as on main.

User Impact

  • Normal installs: no behavior change.
  • Slow but healthy networks: no behavior change (no hard total time limit; 30s grace window unchanged).
  • Blackholed/unresponsive host during connect: each curl attempt fails after ~20 seconds (matching the wget branch's --timeout=20 connection window) instead of hanging forever. --connect-timeout 20 is a per-attempt limit; with the production --retry 3 --retry-delay 1, the download helper gives up after 4 bounded attempts in ~83 seconds (measured, Proof E) rather than blocking the installer indefinitely.
  • Post-connect stall: unchanged from main — aborts after 30 seconds below 1 byte/s.

Evidence

All runs use the exact flags now in scripts/install.sh / scripts/install-cli.sh:
--connect-timeout 20 --speed-limit 1 --speed-time 30 (with -fsSL; --retry omitted in proofs so a single attempt is measured).

Proof A — normal download succeeds

=== Proof A: normal download with exact flags ===
2026年 7月19日 星期日 06时46分51秒 CST

real    0m1.220s
exit=0 size=87538 bytes

(https://raw.githubusercontent.com/openclaw/openclaw/main/README.md)

Proof B — slow but progressing download is NOT killed

Local server trickling 100 bytes at ~5 bytes/s (above the 1 byte/s threshold, transfer takes ~20s):

=== Proof B: slow but progressing (5 B/s trickle, >1 B/s threshold) ===
2026年 7月19日 星期日 06时47分01秒 CST

real    0m19.084s
exit=0 size=100 bytes (expect 100)

Proof C — post-connect stall is aborted at ~30s

Local server sends a partial body, then goes silent:

=== Proof C: post-connect stall is aborted (~30s, exit 28) ===
2026年 7月19日 星期日 06时47分30秒 CST
curl: (28) Operation too slow. Less than 1 bytes/sec transferred the last 30 seconds
exit=28 (expect 28)
2026年 7月19日 星期日 06时48分06秒 CST

Proof D — blackholed connect is bounded by --connect-timeout 20

Non-routable address 10.255.255.1:81 (proxy bypassed with --noproxy '*' so the connect phase itself is measured):

=== Proof D (final head, --connect-timeout 20): blackholed connect bounded ===
2026年 7月19日 星期日 08时05分01秒 CST
curl: (28) Failed to connect to 10.255.255.1 port 81 after 20006 ms: Timeout was reached

real    0m20.025s
exit=28 (expect 28)
2026年 7月19日 星期日 08时05分21秒 CST

Proof E — blackholed connect with the complete production command (incl. retries)

Endpoint: a local sink (nc -k -l 18443) that accepts TCP but never answers the TLS handshake — a deterministic blackholed-connect stand-in (per the curl manual, --connect-timeout covers DNS + TCP + TLS setup, and --speed-limit/--speed-time do not). Command is the exact download_file curl invocation, including production retry options.

$ curl -fsSL --proto '=https' --tlsv1.2 \
    --connect-timeout 20 \
    --speed-limit 1 --speed-time 30 \
    --retry 3 --retry-delay 1 --retry-connrefused \
    -o output.bin "https://127.0.0.1:18443/openclaw-release.tar.gz"
curl: (28) SSL connection timeout
curl: (28) SSL connection timeout
curl: (28) SSL connection timeout
curl: (28) SSL connection timeout
exit_code=28 elapsed_seconds=83

Observed: 4 bounded attempts (1 + 3 retries, ~20s each) plus retry backoff; the helper fails deterministically in ~83 seconds instead of hanging forever. The same command without --connect-timeout stays blocked in the TLS-connect phase past a 45s outer guard (killed by harness), confirming the retry options alone cannot bound this phase.

Regression tests

pnpm test test/scripts/install-sh.test.ts test/scripts/install-cli.test.ts --run
Test Files  2 passed (2)
     Tests  113 passed (113)

(The stubbed-curl assertions in both test files now require --connect-timeout 20 alongside the unchanged --speed-limit 1 --speed-time 30.)

What was not tested

  • Full installer end-to-end run (Node/plugin downloads) — the change is confined to curl flags in download_file; behavior proven at the exact-flag level above.
  • wget branch — untouched (--timeout=20 already bounds connect).

Summary of scope reduction: the original --max-time 60 / 20s-stall-window versions of this PR are superseded by main's 30s stall-window fix. This PR is rebased onto main and now adds --connect-timeout 20 (matching the wget branch's --timeout=20 connection window, per review feedback) to scripts/install.sh and scripts/install-cli.sh, plus matching test assertions.

Complete-command runtime proof (curl --connect-timeout 20)

Both installer download helpers now emit the full bounded command (verified by bash -n syntax check + flag extraction):

# scripts/install.sh / scripts/install-cli.sh (curl branch)
curl -fsSL --proto '=https' --tlsv1.2 \
  --connect-timeout 20 \
  --speed-limit 1 --speed-time 30 \
  --retry 3 --retry-delay 1 --retry-connrefused \
  -o "$output" "$url"

# wget branch (unchanged, parity anchor)
wget -q --https-only --secure-protocol=TLSv1_2 --tries=3 --timeout=20 -O "$output" "$url"

--connect-timeout 20 matches the wget branch's --timeout=20 connection window exactly, so both supported downloaders impose the same new connection-setup cutoff while retaining their existing retry behavior and the 30-second low-speed grace window. Upgrade-safe evidence covers the complete command, not just the delta flag.

@openclaw-barnacle openclaw-barnacle Bot added scripts Repository scripts size: XS labels Jul 18, 2026
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jul 18, 2026
@clawsweeper

clawsweeper Bot commented Jul 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 18, 2026, 11:57 PM ET / July 19, 2026, 03:57 UTC.

Summary
Adds --connect-timeout 20 to the curl download commands in both shell installers and updates the two installer command-output assertions.

PR surface: Tests 0, Other +4. Total +4 across 4 files.

Reproducibility: yes. at source level: curl documents that the existing low-speed controls do not apply to slow connects, while the submitted terminal transcript exercises a blackholed connection with the exact proposed flag set. The evidence establishes the relevant behavior, although this review did not independently execute the network scenario.

Review metrics: none identified.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Rebase onto current main and rerun the focused installer tests on the resulting head.

Risk before merge

  • [P1] The branch is behind current main; it should be rebased and its focused installer checks rerun on the resulting head before merge.
  • [P2] A 20-second connection-setup cutoff intentionally changes failure behavior for curl-only environments whose DNS, TCP, or TLS setup exceeds that window; matching wget’s existing --timeout=20 makes the policy consistent, but it remains an installer-availability compatibility boundary.

Maintainer options:

  1. Rebase and retain downloader parity (recommended)
    Rebase onto current main, retain --connect-timeout 20 in both curl helpers, and rerun the focused installer assertions before merging.
  2. Keep curl unbounded during setup
    Decline the timeout only if maintainers intentionally want curl connection setup to differ from wget’s existing 20-second timeout policy.
Copy recommended automerge instruction 
@clawsweeper automerge

Special instructions:
Rebase onto current main, preserve `--connect-timeout 20` in both installer curl paths, and rerun the focused installer tests on the rebased head.

Next step before merge

  • No mechanical code correction remains; a maintainer should review the rebased exact head because the chosen timeout changes installer connection-failure behavior.

Security
Cleared: The patch changes only curl timeout arguments and corresponding test assertions; it adds no dependency, permission, secret, artifact, or code-execution surface.

Review details

Best possible solution:

Land a rebased version that retains the symmetric 20-second connection bound in both curl helpers, preserves the existing 30-second low-speed transfer guard and retry behavior, and confirms the focused installer checks on the rebased head.

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

Yes, at source level: curl documents that the existing low-speed controls do not apply to slow connects, while the submitted terminal transcript exercises a blackholed connection with the exact proposed flag set. The evidence establishes the relevant behavior, although this review did not independently execute the network scenario.

Is this the best way to solve the issue?

Yes. Adding curl’s connection-phase-specific timeout is the narrowest maintainable fix, and choosing 20 seconds aligns it with the existing wget sibling rather than introducing a separate installer policy.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P1: A blackholed connection can block first-time installs and upgrades until externally interrupted, affecting a core user workflow.
  • merge-risk: 🚨 compatibility: The new hard connection-phase cutoff can change outcomes for existing curl-only environments with unusually slow DNS, TCP, or TLS setup.
  • merge-risk: 🚨 availability: Installer downloads now fail after 20 seconds when connection establishment cannot complete, so the selected threshold directly affects install availability.
  • 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 for normal, slow-progressing, post-connect-stall, and blackholed-connection cases using the proposed curl flags, with the final case showing the intended bounded timeout.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides after-fix terminal evidence for normal, slow-progressing, post-connect-stall, and blackholed-connection cases using the proposed curl flags, with the final case showing the intended bounded timeout.
Evidence reviewed

PR surface:

Tests 0, Other +4. Total +4 across 4 files.

View PR surface stats
Area Files Added Removed Net
Source 0 0 0 0
Tests 2 2 2 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 2 6 2 +4
Total 4 8 4 +4

What I checked:

  • Curl contract: curl documents that --connect-timeout limits only connection setup, including DNS plus TCP, TLS, or QUIC handshakes; it is therefore the correct control for the reported pre-transfer stall.
  • Existing transfer guard: curl documents that --speed-limit and --speed-time abort slow transfers but do not affect slow connects, so retaining the existing 30-second low-speed guard does not duplicate the new connection-phase bound.
  • Changed installer paths: The submitted patch inserts the same --connect-timeout 20 argument immediately before the existing low-speed and retry controls in both curl branches, preserving the downloader selection, TLS restriction, retry policy, and output handling. (scripts/install.sh:119, edb70f89e971)
  • Regression coverage: The two focused installer tests now assert the emitted curl command includes the new flag; the PR’s supplied transcript reports both focused test files passing with 113 tests. (test/scripts/install-sh.test.ts:100, edb70f89e971)
  • Submitted real behavior proof: The PR body includes after-fix terminal output for a normal download, a progressing slow transfer, a post-connect stall, and a blackholed connection; the final scenario shows curl exiting with timeout code 28 after approximately 20 seconds using the proposed flag. (edb70f89e971)
  • Adjacent PR is distinct: The cross-referenced merged PR concerns CI sweeper handling of unknown GitHub mergeability; it may explain prior close/reopen events but does not implement or supersede this installer timeout change. (0f5d030cb10c)

Likely related people:

  • unknown: The supplied review context identifies the branch author and patch history but does not provide reliable current-main blame or installer-history attribution; no maintainer-history owner is asserted without that evidence. (role: installer-area history unresolved; confidence: low; commits: edb70f89e971; files: scripts/install.sh, scripts/install-cli.sh)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works
  • 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.
Review history (13 earlier review cycles; latest 8 shown)
  • reviewed 2026-07-18T22:54:23.698Z sha feaad20 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-18T23:13:56.614Z sha 512b04a :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-18T23:30:03.294Z sha f6ecf09 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-18T23:58:31.535Z sha f6ecf09 :: needs changes before merge. :: [P1] Match the curl cutoff to the wget timeout policy
  • reviewed 2026-07-19T00:08:40.887Z sha edb70f8 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-19T00:13:20.905Z sha edb70f8 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-19T02:57:38.092Z sha edb70f8 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-19T03:15:14.409Z sha edb70f8 :: needs maintainer review before merge. :: none

@xydigit-zt

xydigit-zt commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Repair commit pushed with stall-sensitive timeout fix. PR body updated with real behavior proof and terminal logs.

@clawsweeper

clawsweeper Bot commented Jul 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@steipete steipete mentioned this pull request Jul 18, 2026
Merged
@openclaw-barnacle openclaw-barnacle Bot reopened this Jul 18, 2026
@openclaw-barnacle openclaw-barnacle Bot reopened this Jul 18, 2026
@xydigit-zt
xydigit-zt force-pushed the feat/issue/bound-installer-download-file branch from 21ba7af to feaad20 Compare July 18, 2026 22:49
@xydigit-zt xydigit-zt changed the title fix(install): bound download_file curl with connect and max-time limits fix(install): bound curl connection setup with --connect-timeout 10 Jul 18, 2026
@xydigit-zt

xydigit-zt commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Rebased onto main and reduced scope: main already covers the post-connect stall window (30s), so this PR now only adds --connect-timeout 10 for the connection phase. PR body updated with exact-flag real behavior proof (normal / slow-progressing / post-connect stall / blackholed connect).

@clawsweeper

clawsweeper Bot commented Jul 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jul 18, 2026
@xydigit-zt
xydigit-zt force-pushed the feat/issue/bound-installer-download-file branch from feaad20 to 512b04a Compare July 18, 2026 23:11
thomas.szbay
fix(install): bound curl connection setup with --connect-timeout 10
f6ecf09 
The stall-sensitive --speed-limit/--speed-time options only apply after
the connection is established, so a blackholed or unresponsive release
host could still hang the installer indefinitely during connect. Add
--connect-timeout 10 to both installer download helpers, mirroring the
wget branch's --timeout=20 coverage of the connection phase, while
keeping the existing 30-second low-speed grace window unchanged.
@xydigit-zt
xydigit-zt force-pushed the feat/issue/bound-installer-download-file branch from 512b04a to f6ecf09 Compare July 18, 2026 23:27
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. label Jul 18, 2026
@xydigit-zt

xydigit-zt commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 19, 2026
thomas.szbay
fix(install): match wget's 20s connection tolerance in curl helpers
edb70f8 
Per review feedback: --connect-timeout covers DNS, TCP, and TLS setup, so
a 10-second cutoff could fail valid slow connection establishment that
the wget sibling (--timeout=20) still permits. Use 20 seconds in both
curl helpers to match the wget connection window.
@xydigit-zt xydigit-zt changed the title fix(install): bound curl connection setup with --connect-timeout 10 fix(install): bound curl connection setup with --connect-timeout 20 Jul 19, 2026
@xydigit-zt

xydigit-zt commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

P2 addressed: both curl helpers now use --connect-timeout 20, matching the wget branch's --timeout=20 connection window. Assertions updated (113 tests pass locally), and the PR body carries the exact-command transcript for the selected value: blackholed connect aborts with curl 28 after 20006ms.

@clawsweeper

clawsweeper Bot commented Jul 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed proof: sufficient ClawSweeper judged the real behavior proof convincing. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jul 19, 2026
@xydigit-zt

xydigit-zt commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. 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. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P1 High-priority user-facing bug, regression, or broken workflow. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. scripts Repository scripts size: XS status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@xydigit-zt