fix(daytona): allow long-running SDK commands - #1457
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
Codex review: found issues before merge. Reviewed August 24, 2026, 2:56 AM ET / 06:56 UTC. ClawSweeper reviewWhat this changesThe PR makes direct Daytona process commands and toolbox requests honor the caller’s context budget instead of fixed SDK HTTP or one-hour limits. Merge readinessKeep open: the Daytona timeout fix remains necessary, has sufficient exact-head behavior proof, and is functionally correct aside from one non-blocking release-note ownership cleanup. Priority: P2 Review scores
Verification
Live VerificationCommand: Result: FAIL (failed) — execution before step 1 Assertions:
How this fits togetherCrabbox’s direct Daytona provider creates or resolves a sandbox, synchronizes workspace content through the toolbox API, and executes a remote process. The caller context flows into these operations and determines when the request and remote command should stop. flowchart LR
A[CLI run request] --> B[Caller execution context]
B --> C[Daytona provider]
C --> D[Workspace toolbox sync]
C --> E[Remote process request]
D --> F[Daytona sandbox]
E --> F
F --> G[Command result and cleanup]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Land the caller-owned timeout behavior with its focused SDK serialization and cancellation coverage, while leaving release-note ownership to the release process. Do we have a high-confidence way to reproduce the issue? Yes—source reproducibly identifies direct process calls on current main without caller-derived execution timeouts, and the exact-head SDK trace demonstrates the corrected request values and cancellation path. Is this the best way to solve the issue? Yes—the wrapper keeps the caller context as the authority, covers setup, extraction, and user commands, and avoids adding public configuration; remove only the release-owned changelog entry. Full review comments:
Overall correctness: patch is correct AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against dcbea5cf10a8. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (3 earlier review cycles)
|
|
Maintainer follow-up pushed at The original fixed 60-minute process timeout and 61-minute HTTP timeout still truncated valid longer workloads. The provider now derives Daytona's remote timeout from the caller's actual context deadline, rounds fractional seconds upward, and uses the maximum supported 32-bit duration when the caller has no deadline. Toolbox HTTP requests remain governed by their request context instead of a second hard wall-clock limit. Verification:
The regression covers an unbounded caller, a 90-minute deadline, and upward rounding of a 1.5-second deadline. Existing live Daytona create/use/destroy proof from this PR still applies to the transport path; exact-head CI should complete before landing. |
Co-authored-by: Arianit Sylafeta <ari.sylafeta@gmail.com>
65d521a to
e8d9e57
Compare
|
Maintainer verification for exact head One real Daytona sandbox was provisioned, executed successfully, and released: The pinned Daytona SDK's actual serialized process request and cancellation behavior were independently observed on the same head: The hosted smoke used the available coordinator-managed SSH path; the SDK request trace used a local instrumented HTTP server. A hosted direct-SDK run longer than one hour is not claimed because no direct Daytona credential, authenticated CLI profile, or direct-mode snapshot is available. Provider race tests, repository-wide vet, CLI build, the complete docs gate, and project autoreview all passed. Contributor authorship and co-author credit are preserved. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
steipete
left a comment
There was a problem hiding this comment.
Approved after verifying exact head e8d9e57b73fb83b337df2db73018466d0df5b46f against the complete green CI matrix, the cleanup-verified hosted Daytona lifecycle, and the pinned SDK's actual serialized 5400-second process request plus caller-context cancellation.
The Unreleased changelog entry thanking @arisylafeta is intentional and must remain: this is a user-visible contributor fix, and repository guidance assigns changelog selection and contributor attribution to the maintainer. The automated suggestion to remove that maintainer-added entry conflicts with that requirement and is rejected.
The hosted smoke exercised coordinator-managed SSH, while the direct SDK timeout and cancellation were proven against an instrumented local HTTP endpoint. No hosted direct-SDK execution longer than one hour is claimed.
What Problem This Solves
Fixes an issue where direct Daytona SDK setup, sync extraction, and user commands could be cut short by the SDK's default 60-second HTTP timeout. The original proposed 60-minute process timeout and 61-minute HTTP timeout would also have imposed a new fixed ceiling on otherwise valid longer workloads.
Why This Change Was Made
Daytona process requests now receive the remaining caller-context budget, rounded upward to whole seconds and capped at the SDK's signed 32-bit maximum. A caller without a deadline receives that same maximum. The run-owned toolbox HTTP client no longer imposes an independent wall-clock timeout; its request still stops when the caller context is canceled. Existing provider-owned sandbox cleanup remains separately bounded.
User Impact
Direct Daytona commands follow the execution budget already selected by the caller instead of being stopped by an unrelated SDK HTTP deadline or a new one-hour cap. No public CLI flag or configuration option changes. A caller without a deadline relies on explicit context cancellation, the provider's maximum execution timeout, and ordinary sandbox lifecycle cleanup.
Evidence
Validated exact head:
e8d9e57b73fb83b337df2db73018466d0df5b46f.Hosted Daytona lifecycle
The exact-head CLI created one real Daytona sandbox, executed a bounded command successfully, and released it:
The coordinator initially reported asynchronous release, so cleanup was explicitly reconciled and independently verified afterward:
These last two outputs are sanitized, narrowed excerpts from the inspected JSON responses. The available hosted account is coordinator-managed and therefore runs commands over SSH, not through the direct Daytona SDK command runner changed here; the hosted smoke proves real Daytona provisioning, execution, release, and absence of leftover claims, not the direct SDK transport.
Direct SDK request and cancellation boundary
The exact-head regression exercises the pinned Daytona Go SDK against an instrumented local HTTP server and inspects the actual serialized
/process/executerequest:This proves that a 90-minute caller budget transmits
5400seconds, beyond the superseded3600-second cap, while request cancellation still interrupts an in-flight HTTP exchange. It does not claim a hosted direct-SDK workload actually ran for more than one hour. No direct Daytona API credential, authenticated Daytona CLI profile, or direct-mode snapshot was available; hosted direct-SDK execution would require that separate access.Additional exact-head validation:
The original contributor commit retains Arianit Sylafeta's authorship, the follow-up carries a matching
Co-authored-bytrailer, and the current Unreleased changelog thanks @arisylafeta.