fix(http): bound DefaultClient fallbacks with 60s timeout - #1229
Open
SebTardif wants to merge 2 commits into
Open
fix(http): bound DefaultClient fallbacks with 60s timeout#1229SebTardif wants to merge 2 commits into
SebTardif wants to merge 2 commits into
Conversation
Replace production http.DefaultClient nil-fallbacks with
&http.Client{Timeout: 60 * time.Second}, matching existing DO/OVH/Linode
defaults, so stalled API calls cannot hang forever.
Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
clawsweeper
Bot
added
rating: 🦪 silver shellfish
Contributor
|
Codex review: needs real behavior proof before merge. Reviewed August 9, 2026, 4:25 PM ET / 20:25 UTC. ClawSweeper reviewWhat this changesThe PR replaces unbounded fallback HTTP clients across CLI and provider adapters with 60-second defaults, with a transfer-aware exception for Daytona archive uploads. Merge readiness⛔ Blocked until real behavior proof from a real setup is added - 13 items remain Keep open: the Daytona exception is correct, but six changed fallback clients still serve uploads or streamed execution and would impose a 60-second whole-request cutoff on existing workflows. Priority: P1 Review scores
Verification
How this fits togetherCrabbox provider adapters construct HTTP clients for control-plane API calls, file transfers, and streamed execution. A runtime-supplied client is preserved when present; otherwise the adapter selects its fallback before sending requests to the provider. flowchart LR
A[CLI provider operation] --> B[Provider adapter]
B --> C{Runtime client supplied?}
C -->|Yes| D[Use supplied client]
C -->|No| E[Select fallback client]
E --> F[Control API, upload, or stream]
D --> F
F --> G[Provider result]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest possible solution: Keep 60-second defaults for control-plane-only fallback requests, but make each affected provider’s upload and stream path transfer-aware with bounded connection setup and no whole-request deadline. Do we have a high-confidence way to reproduce the issue? Yes, from source: construct any affected provider without Runtime.HTTP and run its upload or streaming path for longer than 60 seconds; the proposed fallback client will cancel the request. Is this the best way to solve the issue? No: a universal whole-request timeout is not safe for adapters whose existing client also handles uploads or response streams; preserve bounded setup timeouts without imposing an overall deadline on those paths. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 6409db970a7f. LabelsLabel justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (39 earlier review cycles; latest 8 shown)
|
Use a no-overall-Timeout client with dial/TLS/header bounds for streaming archive uploads so multi-minute syncs are not aborted at 60s. Drop the release-owned Unreleased changelog hunk. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
Contributor
Author
|
@clawsweeper re-review Addressed P1: Daytona archive uploads use transferAwareHTTPClient (Timeout 0 + header/dial bounds). Dropped release-owned changelog entry. Terminal proof in body. |
Contributor
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
merge-risk: 🚨 availability
clawsweeper
Bot
added
rating: 🧂 unranked krab
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
Many provider/CLI HTTP constructors fell back to
http.DefaultClientwhenrt.HTTP(or equivalent) was nil. That client has no overall timeout, so a stalled cloud API response can hang the CLI forever. Sibling providers (DigitalOcean, OVH, Linode, Vultr, etc.) already use&http.Client{Timeout: 60 * time.Second}.Evidence
http.DefaultClientfallbacks/uses across providers +aws_ssh_cidr/azure/http_redirect.http.DefaultClientreferences; all use a 60s client matching existing DO/OVH defaults.http.DefaultClientintentionally (test doubles).Real behavior proof
Environment: Go 1.26, macOS; branch
fix/default-http-client-timeoutSetup: module download
Command:
Output:
Why this proves it: Compile + package tests for changed providers pass; production code no longer installs an unbounded default client.
Limits: Does not live-hit provider APIs; timeout value is the established 60s project convention, not newly measured.
Test plan
Review follow-up (Daytona transfer-aware upload)
Claw P1: Daytona archive upload no longer uses a 60s whole-request
Client.Timeout.Nil-runtime fallback is now
transferAwareHTTPClient():Timeout: 0(streaming body can exceed 60s)Removed the release-owned Unreleased CHANGELOG hunk.
Evidence after fix (terminal):