Skip to content

fix(cli): give runtimeForApp a timed HTTP client - #1881

Closed
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/runtime-timed-http-client
Closed

fix(cli): give runtimeForApp a timed HTTP client#1881
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/runtime-timed-http-client

Conversation

@SebTardif

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes an issue where users running crabbox run, connect, status, and other product commands against HTTP providers could hang forever when the provider control plane stopped responding. Those commands load backends through runtimeForApp, which left Runtime.HTTP unset. Providers such as opencomputer, crownest, superserve, opensandbox, unikraftcloud, and cloudrunsandbox then fell back to http.DefaultClient, which has no timeout.

The same hole exists on RuntimeForProviderOperation, used for provider lifecycle work that is not already bound to a configured backend.

Why This Change Was Made

Give both constructors a shared 60-second http.Client, matching the finite-control clients already used in Azure and Hetzner helpers. A single defaultRuntimeHTTP() helper keeps the two paths from drifting.

This does not clone http.DefaultTransport. #1750 already bounded Daytona control-plane calls inside the Daytona adapter. This change covers the remaining unset Runtime.HTTP constructors on the product CLI path.

User Impact

Stalled provider control-plane requests on the main CLI path now fail after 60 seconds instead of hanging until the process is killed. Operators get an error they can retry or diagnose. No config or flag changes.

Evidence

Live go program calling the exported provider-operation runtime after the patch, plus the built crabbox binary from this tree:

$ go run .
HTTP timeout: 1m0s
HTTP is DefaultClient: false
DefaultClient timeout: 0s
$ crabbox --help
Crabbox leases remote test boxes, syncs your dirty checkout, runs commands, and cleans up.

Usage:
  crabbox <command> [flags]
  crabbox run [flags] -- <command...>

The same helper is used by unexported runtimeForApp, which is what run, connect, and status pass into loadBackend. Before the patch, HTTP was nil on both constructors, so those providers used http.DefaultClient (Timeout 0s, no deadline).

Real behavior proof

  • Behavior or issue addressed: Product CLI runtimes omitted HTTP, so provider control calls could hang forever on a stalled host via http.DefaultClient.

  • Real environment tested: Windows 11, Go 1.27.0 windows/amd64, worktree at fix/runtime-timed-http-client based on origin/main 6172f518.

  • Exact steps or command run after this patch: Built crabbox from this tree with go build, then ran a small go program in the same module that calls cli.RuntimeForProviderOperation and prints the client timeout next to http.DefaultClient.

  • Evidence after fix: terminal output from the patched tree:

    $ go run .
    HTTP timeout: 1m0s
    HTTP is DefaultClient: false
    DefaultClient timeout: 0s
  • Observed result after fix: The exported provider-operation runtime now carries a 60-second HTTP client that is not http.DefaultClient. DefaultClient remains unbounded (0s), which is the hang that providers used when HTTP was nil.

  • What was not tested: Live opencomputer, crownest, or superserve control-plane outage. Multi-minute Daytona archive uploads and E2B envd streams against a real host.

Related

Product run, connect, and status paths load backends through
runtimeForApp, which left Runtime.HTTP unset. Providers then fell
back to http.DefaultClient and could hang on a stalled control plane.

Set a 60s whole-request client on that runtime and on
RuntimeForProviderOperation via a shared helper.

Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
@clawsweeper

clawsweeper Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

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

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. 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. labels Sep 5, 2026
@clawsweeper

clawsweeper Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed September 5, 2026, 8:47 AM ET / 12:47 UTC.

ClawSweeper review

What this changes

Adds a shared 60-second HTTP client to both CLI runtime constructors and tests their client settings.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

Blocked before merge - 6 items remain

Request changes. The patch overrides established provider timeout ownership, and the supplied evidence does not establish an uncovered control-plane hang.

Priority: P2
Reviewed head: bf93cfa3b1cc48085b73ac4de8c2c44ab132090e

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The focused patch overrides established request budgets, while its evidence checks construction rather than the claimed HTTP behavior.
Proof confidence 🦪 silver shellfish (2/6) Needs stronger real behavior proof before merge: The captured Windows terminal output proves RuntimeForProviderOperation constructs a 60-second client, but neither that inspection nor CLI help exercises provider HTTP, stalled responses, or long-operation compatibility. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦪 silver shellfish (2/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs stronger real behavior proof before merge: The captured Windows terminal output proves RuntimeForProviderOperation constructs a 60-second client, but neither that inspection nor CLI help exercises provider HTTP, stalled responses, or long-operation compatibility. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 10 items Verified introduced change: The merge-base comparison contains only the two runtime constructor changes and the new constructor tests: 35 additions and 2 deletions.
Verified merge result: The test merge records the pinned main commit followed by the exact PR head as parents; its comparison against main changes the same three files. The timeout regression therefore survives the merge.
E2B command and upload propagation: The run command passes runtimeForApp into loadBackend. E2B uses an injected HTTP client for both control and data operations; StartProcess and UploadFile pass that client to the real HTTP owners. The new 60-second timeout therefore bounds response-stream reads and uploads.
Findings 1 actionable finding [P1] [P1] Preserve long-operation lifetimes outside control requests
Security None None.

How this fits together

Crabbox’s CLI passes a runtime to provider adapters for remote lifecycle operations, command execution, and file transfers. Those adapters distinguish short control requests from longer commands and uploads.

flowchart TD
  A[CLI commands] --> B[Runtime constructors]
  B --> C[Provider adapters]
  C --> D[Finite control requests]
  C --> E[Commands and file transfers]
  D --> F[Provider request deadlines]
  E --> G[Caller controlled lifetimes]
Loading

Before merge

  • Add real behavior proof - Needs stronger real behavior proof before merge: The captured Windows terminal output proves RuntimeForProviderOperation constructs a 60-second client, but neither that inspection nor CLI help exercises provider HTTP, stalled responses, or long-operation compatibility. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • [P1] Preserve long-operation lifetimes outside control requests (P1) - Every normal CLI backend now receives this 60-second whole-request client. E2B’s e2bHTTPClients returns an injected client for both control calls and envd, so a command such as sleep 90 loses its response stream after 60 seconds despite a longer command budget. Daytona archive uploads also consume b.rt.HTTP, and OpenComputer execution already has a separate one-hour default. Existing adapter deadlines and control/data separation intentionally support these workflows. Keep the shared runtime compatible and fix only a demonstrated unbounded control request inside its adapter.
  • Resolve merge risk (P1) - Compatibility across all runtime consumers remains unverified: merging this default would stop existing commands and uploads exceeding 60 seconds, including workflows whose provider settings permit longer execution.
  • Complete next step (P2) - Remove the global deadline and identify any uncovered finite control request before scoping an adapter fix. Add real HTTP evidence for stalled responses and preserved long commands/uploads; terminal screenshots or recordings are welcome, and logs or copied output also count. Redact credentials, IP addresses, and private endpoints. Updating the PR body should trigger re-review; otherwise ask a maintainer to comment @clawsweeper re-review.
  • Improve patch quality - Identify a genuinely unbounded finite request and preserve existing command, stream, and upload budgets in the repair.
  • Improve patch quality - Provide redacted real-transport evidence showing the uncovered stall is bounded and longer valid operations still complete.

Findings

  • [P1] [P1] Preserve long-operation lifetimes outside control requests — internal/cli/provider_backend.go:1686-1687
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta production +6 net lines; tests +27 lines The stated hardening adds little code, but its constructor placement changes timeout behavior across provider adapters.

Merge-risk options

Maintainer options:

  1. Preserve provider-owned deadlines (recommended)
    Remove the shared whole-request deadline and scope any demonstrated missing bound to its adapter, with long-operation compatibility proof.
  2. Pause pending an uncovered request
    Pause the proposal until a concrete control request is shown to escape the deadlines already present on main.

Technical review

Best possible solution:

Retain adapter-owned control deadlines and caller-owned streaming lifetimes, adding a narrow timeout fix only for a demonstrated uncovered request.

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

Yes for the introduced regression: the CLI-supplied client reaches E2B’s process stream and imposes a 60-second limit on a longer command. The reported current-main hang is not established by the named providers’ deadline-bearing request paths; no runtime reproduction was executed.

Is this the best way to solve the issue?

No. A shared whole-request timeout overrides existing control/data separation; any remaining gap should be repaired at the specific adapter request boundary.

Full review comments:

  • [P1] [P1] Preserve long-operation lifetimes outside control requests — internal/cli/provider_backend.go:1686-1687
    Every normal CLI backend now receives this 60-second whole-request client. E2B’s e2bHTTPClients returns an injected client for both control calls and envd, so a command such as sleep 90 loses its response stream after 60 seconds despite a longer command budget. Daytona archive uploads also consume b.rt.HTTP, and OpenComputer execution already has a separate one-hour default. Existing adapter deadlines and control/data separation intentionally support these workflows. Keep the shared runtime compatible and fix only a demonstrated unbounded control request inside its adapter.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add P2: This is provider timeout hardening without a demonstrated urgent current-main outage.
  • add merge-risk: 🚨 compatibility: The new shared timeout truncates existing long-running commands and uploads despite their longer provider or caller budgets.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦪 silver shellfish.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The captured Windows terminal output proves RuntimeForProviderOperation constructs a 60-second client, but neither that inspection nor CLI help exercises provider HTTP, stalled responses, or long-operation compatibility. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P2: This is provider timeout hardening without a demonstrated urgent current-main outage.
  • merge-risk: 🚨 compatibility: The new shared timeout truncates existing long-running commands and uploads despite their longer provider or caller budgets.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The captured Windows terminal output proves RuntimeForProviderOperation constructs a 60-second client, but neither that inspection nor CLI help exercises provider HTTP, stalled responses, or long-operation compatibility. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

  • Verified introduced change: The merge-base comparison contains only the two runtime constructor changes and the new constructor tests: 35 additions and 2 deletions. (internal/cli/provider_backend.go:1687, bf93cfa3b1cc)
  • Verified merge result: The test merge records the pinned main commit followed by the exact PR head as parents; its comparison against main changes the same three files. The timeout regression therefore survives the merge. (b772a66137aa)
  • E2B command and upload propagation: The run command passes runtimeForApp into loadBackend. E2B uses an injected HTTP client for both control and data operations; StartProcess and UploadFile pass that client to the real HTTP owners. The new 60-second timeout therefore bounds response-stream reads and uploads. (internal/providers/e2b/client.go:125, 834004498309)
  • Established long-operation contract: Existing E2B tests require separate default clients and exercise streams and uploads lasting beyond the control timeout. They construct provider runtimes directly, so they do not cover the newly changed CLI constructor wiring. (internal/providers/e2b/backend_test.go:160, bf93cfa3b1cc)
  • Daytona upload regression: Archive uploads consume b.rt.HTTP directly. Current documentation explicitly reserves caller-controlled lifetimes for uploads and toolbox execution, consistent with the merged fix(daytona): bound control-plane HTTP requests #1750. (internal/providers/daytona/upload.go:35, 834004498309)
  • Existing request-context deadlines: OpenComputer already wraps finite JSON requests in a two-minute context deadline and execution in a separate, normally one-hour budget. Crownest, Superserve, and Unikraft Cloud likewise bound finite requests; Cloud Run Sandbox wraps remote requests in a deadline. A nil Runtime.HTTP does not make these calls unbounded. (internal/providers/opencomputer/apiclient.go:181, 834004498309)

Likely related people:

  • Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • SebTardif: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

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.

@steipete steipete left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for following through on the timeout work. The remaining issue is the scope of the default: a client installed on Runtime.HTTP is also an injected data-plane client, not only a control-plane client.

In current main, E2B's e2bHTTPClients returns the injected client for both control requests and envd operations. Daytona's archive upload and toolbox paths also inherit Runtime.HTTP. A global 60-second http.Client.Timeout therefore caps legitimate longer commands, response streams, and archive uploads even when their own operation budget is longer; that timeout includes reading the response body. This reverses the control/data separation preserved in #1750.

Please keep the runtime injection contract unchanged and narrow the fix to a demonstrated finite provider request that escapes its operation-specific bound. Nil Runtime.HTTP by itself is not evidence of an unbounded operation: adapters also impose request contexts and SDK deadlines. The named examples need to be checked at that boundary against current main.

The regression test should drive the production runtime/backend wiring: a stalled finite request must end, while a stream or upload that legitimately outlives the control timeout must finish, and caller cancellation must still work. Printing the constructor's timeout and running --help do not establish those behaviors.

I cannot land #1881 with this global timeout because of the long-operation regression. A focused adapter fix with that behavior proof remains welcome.

@SebTardif

Copy link
Copy Markdown
Contributor Author

@steipete

I cannot land #1881 with this global timeout because of the long-operation regression. A focused adapter fix with that behavior proof remains welcome.

Agreed. Closing this PR.

A client installed on Runtime.HTTP is also the injected data-plane client (E2B envd, Daytona archive and toolbox). A 60s whole-request Timeout would cut a command such as sleep 90 and long uploads, which is the contract #1750 kept.

I rechecked the named nil-HTTP fallbacks on current main. Finite control calls already have adapter-owned bounds:

  • opencomputer: 2m request context on JSON, separate 1h exec budget
  • crownest: 2m request context except the event stream
  • superserve, unikraftcloud, cloudrunsandbox: request-context deadlines
  • daytona control: 60s client when Runtime.HTTP is nil
  • islo: ResponseHeaderTimeout 30s and Timeout 0 so uploads and exec can still run

I did not find a finite control request that escapes those bounds. If one shows up later, I will send an adapter-only patch with stalled-request and long-operation HTTP proof.

@SebTardif

Copy link
Copy Markdown
Contributor Author

@steipete

Follow-up after the recheck: Nomad control JSON was still unbounded. Regions() dropped context, and job inspect / register used only the parent CLI context (cancel on SIGINT, no deadline) on cleanhttp.DefaultPooledClient().

Adapter-only patch: #1916

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

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants