Uh oh!
There was an error while loading. Please reload this page.
fix(sandbox): bound the Daytona calls that requests wait on - #464
Conversation
The sandbox build-status refresh talks to Daytona and had no cap on the round-trip. It runs on GET /api/v1/capabilities, on GET /api/v1/settings/sandbox-providers, and on turn creation when a fresh sandbox is needed, so a provider that accepted the connection and then stalled held those requests until undici's own multi-minute defaults — once per request, with each one keeping a server connection and an event-loop task alive. A Daytona brownout became piled-up concurrent requests, and nothing was logged while it happened. Only the settings write bounded this, wrapping buildImage in withTimeout at three seconds. That budget now lives beside the refresh as SANDBOX_BUILD_REQUEST_TIMEOUT_MS and both paths import it: they issue the same calls, and a duplicated number is one edit away from disagreeing. On expiry the refresh returns the last persisted status instead of throwing. All three callers are asking what the current status is, not asking to reach Daytona: the capability probe already fails closed on an exception and would report the sandbox unconfigured, the settings read already falls back to the record, and turn creation would take an exception it has no better answer for. A stale status serves each of them better than a hang or a 500 while the provider is unwell. A real error — rejected credentials, a failed build — still propagates, so this does not hide anything except slowness. Snapshot registration also carries its own timeout now. withTimeout is a Promise.race: it abandons the result but the request keeps running, so a caller-side cap alone leaves the socket open for the full undici default. AbortSignal.timeout ends the request itself. Verified: the two stall cases hang and fail on a four-second test timeout with the withTimeout calls removed, and pass with them, so the tests fail without the fix. tests/unit/apis and tests/unit/sandbox: 153 passed. tsc clean for trueforge src, its tests/unit project, and trueforge-core. Six failures elsewhere in the package are a Windows host — unix sockets and symlink paths under LocalSandboxProvider — and fail identically on a clean checkout. Not addressed here: the issue also notes that checkSnapshotStatus mutates persisted state and can reactivate a snapshot from GET handlers, so reads drive external writes. That is a design question about where the refresh belongs rather than a timeout, and changing it would move behaviour this change is trying to leave alone.
🦋 Changeset detectedLatest commit: b91b87e The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
thesujai
left a comment
There was a problem hiding this comment.
We wanted timeout of the calls outside the transaction block to be 1 min
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
The refresh runs outside a transaction, so its budget is a minute rather than the three seconds the settings write uses inside one. The write is untouched and keeps its own constant. On expiry it throws instead of returning the persisted status. Returning it would report 'ready' for a snapshot that may have been deactivated, which is the case the buildImage branch exists to catch. getCapabilities already treats a failure as sandbox-disabled. Registration's request timeout matches the longest caller budget, so the socket outlives no one still waiting on it. Comments trimmed and the test file removed, as asked.
ethanstoner
commented
Aug 27, 2026
Pushed. Summary of what changed:
One thing I pushed back on rather than silently changing, with measurements in the thread: Verified after the changes: |
thesujai
left a comment
There was a problem hiding this comment.
Thanks @ethanstoner for your work here, really appreciate it!
Uh oh!
There was an error while loading. Please reload this page.
Fixes#417.
What hangs, and where
checkSnapshotStatusreaches Daytona with no cap on the round-trip, from three request-handling paths:GET /api/v1/capabilities— the UI's boot probeGET /api/v1/settings/sandbox-providers— the settings pageturns.ts)A provider that accepts the connection and then stalls holds each of those until undici's own multi-minute defaults, once per request, each keeping a server connection and an event-loop task alive. A Daytona brownout turns into piled-up concurrent requests, and nothing is logged while it happens.
Only
PUT /settings/sandbox-providersbounded it, atwithTimeout(provider.buildImage(), 3s).What this does
One budget, one owner.
BUILD_REQUEST_TIMEOUT_MSmoves out ofsandboxProviders.tsand becomesSANDBOX_BUILD_REQUEST_TIMEOUT_MSnext to the refresh; the PUT handler imports it. Both paths issue the same calls, and a duplicated number is one edit from disagreeing — the one-canonical-owner rule inAGENTS.md.The refresh returns the persisted status when the budget expires, rather than throwing. All three callers are asking what the status is, not asking to reach Daytona:
A stale status serves each of them better than a hang or a 500 while the provider is unwell. A real error — rejected credentials, a failed build — still propagates, so this hides nothing except slowness. There is a test for that distinction.
Registration carries its own request timeout. This is the part worth a second look:
withTimeoutis aPromise.race, so it abandons the result while the request keeps running. A caller-side cap alone leaves the socket open for the full undici default, which is most of what the issue is about.AbortSignal.timeouton thefetchends the request itself. Ten seconds there, deliberately looser than the caller's three: it is the backstop for a socket nobody is waiting on any more, not the budget a request is held to.Verified
The two stall cases were run against the code with the
withTimeoutcalls removed — both hang and fail on a four-second test timeout — and pass with them. The tests fail without the fix:tests/unit/apis/+tests/unit/sandbox/— 153 passed, 4 of them new.tsc --noEmitclean forpackages/trueforgesrc, itstests/unitproject, andpackages/trueforge-core.prettier --checkandeslintclean on the files touched.packages/trueforge(LocalSandboxProvider,Code Mode UDS) are a Windows host — unix sockets and symlink paths — and fail identically on a clean checkout.Deliberately not addressed
The issue's closing note:
checkSnapshotStatusmutates persisted status and can reactivate an idle snapshot from GET handlers, so plain reads drive external writes and retrying clients amplify call volume. That is real, but it is a question about where the refresh belongs rather than a timeout, and fixing it would move behaviour this change is trying to leave alone. Happy to open a separate issue or PR for it if you want it split that way..changeset/daytona-request-timeouts.mdcovers both published packages.Note
Medium Risk
Changes how sandbox status refresh behaves under slow or stuck Daytona responses on several API paths; timeouts surface as errors rather than indefinite hangs, which can affect caller error handling (e.g. settings GET vs capabilities fail-closed).
Overview
checkSnapshotStatusno longer waits indefinitely on Daytona when it callsbuildImage()(ready snapshots that may need reactivation) orgetImageBuildStatus()(non-ready builds). Both round-trips are wrapped withwithTimeoutfromtrueforge-core, using a newSTATUS_REFRESH_TIMEOUT_MSof 60 seconds.That refresh runs on request paths such as capabilities boot, sandbox settings GET, and turn creation when status must be refreshed—so a stalled Daytona provider stops holding those handlers for undici’s long default instead of hanging until the client gives up.
A patch changeset notes the behavior for
@truefoundry/trueforgeand@truefoundry/trueforge-core.Reviewed by Cursor Bugbot for commit 3474dee. Bugbot is set up for automated code reviews on this repo. Configure here.