[feat] Reuse Daytona sandboxes across turns instead of deleting them every turn#5225
Conversation
…t, pointer guard, typed teardown Slice 1 of the warm-daytona-sessions plan (PR #5214): the correctness base, with parking still inert (every teardown reason still deletes). - daytona provider wrapper adds pause/reconnect with a bounded state machine over Daytona transitional states; deleteSandbox helper; create-spec fingerprint (snapshot/image/target/env NAMES/network policy). - sandbox-agent package patch: a failed pause keeps the provider handles so the delete fallback works; a reconnected sandbox that fails later attach steps is paused, not leaked. - sessions API: nullable session_states.sandbox_fingerprint column; the sandbox pointer write is guarded by a compare-and-set on data.latest_turn_index (sandbox_turn_index token; tokenless writes keep today's behavior). - runner: pointer write is awaited, carries the fingerprint and guard token, and moved after continuity hydrate so a cold restart sends the right token; reconnect only on fingerprint match, best-effort delete on mismatch. - typed teardown reasons replace the keepWarm boolean; reason-to-disposition mapping with PARK_CLEAN_RESUMABLE_TURNS=false (Slice 2 flips it). Claude-Session: https://claude.ai/code/session_018MaXPNpvzN22kngHno3VMj
…ovider pools, park-to-running, verified live Completes the warm-daytona-sessions plan (PR #5214) on top of the slice-1 correctness base. - Slice 2 (park-to-stopped active): clean resumable Daytona turns and idle shutdown stop the sandbox instead of deleting it; stop timer default 15 minutes; auto-archive removed entirely (create field, env override, compose and helm forwarding); terminal-vs-transient reconnect classification (only not-found/error/destroyed/unknown clear the stored pointer, guarded); clean remount before every remote mount so a reattached sandbox never serves a stale FUSE mount. - Slice 3 (provider-aware pools): one SessionPool per provider; local env vars and behavior unchanged; Daytona configured by AGENTA_RUNNER_DAYTONA_SESSION_IDLE_TTL_MS (0 disables, no separate flag) and AGENTA_RUNNER_DAYTONA_SESSION_MAX_WARM (default 20); pool evictions carry typed teardown reasons; unknown providers fail closed to cold. - Slice 4 (park-to-running warm slots): strict awaited capacity for the Daytona pool (a stopping sandbox keeps its warm slot until the stop confirms, so the billed count cannot overshoot); overflow parks to stopped; one-time activity refresh at live park, no recurring pings. - Slice 5: per-stage [timing] log lines in acquire; Daytona live-window default 120000 ms after the E3 gate passed; the TTL env parse accepts 0 as a valid off switch (a positive-only parse would silently re-enable the default). - Docs and config: the two new env vars forwarded in compose, env examples, and helm; self-host pages updated to the three-state lifecycle. - Live E3 verification: cold 12.5 s, live-warm 1.39 s, stopped-restart 7.7 s; one instance served all three turns; expiry observed as stop; SIGTERM drain observed as stop; zero sandbox leaks. Requires alembic migration core_oss oss000000011 (nullable session_states.sandbox_fingerprint). Carries shared-file hunks from the unmerged lanes feat/keepalive-project-scope (protocol.ts runContext scope and pool key preference) and fix/sessions-continuity-review (models.py replica_id validation, mdx edits) per the coordination board's shared-file rule. Claude-Session: https://claude.ai/code/session_018MaXPNpvzN22kngHno3VMj
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughWarm Daytona sessions now persist sandbox fingerprints and guarded turn pointers, reconnect compatible sandboxes, apply reason-aware stop/delete policies, route keep-alive sessions by provider, and expose matching runtime configuration, documentation, and tests. ChangesSession state persistence
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Runner
participant SessionPool
participant SessionStateAPI
participant Daytona
Client->>Runner: Submit session turn
Runner->>SessionPool: Resolve provider and session scope
SessionPool->>SessionStateAPI: Read guarded sandbox pointer
SessionStateAPI-->>SessionPool: Return sandbox id and fingerprint
SessionPool->>Daytona: Reconnect or create sandbox
Daytona-->>SessionPool: Return active sandbox
SessionPool->>SessionStateAPI: Write pointer with turn index
SessionPool-->>Runner: Resume or park session
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
🤖 The AI agent says: Inline notes at each decision point follow, in the diff's reading order. Suggested reading order for the whole diff: 1) teardown.ts (the reason model, small), 2) daytona-provider.ts (the pause/reconnect state machine and the fingerprint), 3) the package patch (two vendored teardown fixes), 4) sandbox-reconnect.ts plus the sessions API (the guarded pointer), 5) sandbox_agent.ts (reconnect gate, pointer write placement), 6) session-pool.ts and server.ts (per-provider pools, strict capacity), 7) hosting and docs.
| * The destroy path escalates a failed stop to delete. | ||
| */ | ||
|
|
||
| export type TeardownReason = |
There was a problem hiding this comment.
🤖 Why a typed reason instead of the old keepWarm boolean: different endings need different dispositions (a /kill must delete, a clean turn should stop, an idle shutdown should stop, an in-flight shutdown must delete), and a boolean cannot say why the teardown happened. TS note: this union type works like a Python Literal enum; teardownDisposition is the single mapping from reason to stop-or-delete, so review of "can this path leave a sandbox running" happens in one place.
| ]); | ||
| const FAILED_STATES = new Set(["error", "destroyed"]); | ||
|
|
||
| export class DaytonaReconnectTerminalError extends Error { |
There was a problem hiding this comment.
🤖 The terminal-versus-transient split the plan's reviewer asked for: not-found, error, destroyed, and unknown states throw this typed error and the engine clears the stored pointer (guarded). A timeout waiting out a transitional state, or any network failure, stays a plain error and the pointer is retained, so one flaky API call cannot force a permanent cold chain.
| } | ||
|
|
||
| /** Hash only resolved create fields that determine whether a Daytona sandbox is reusable. */ | ||
| export function createSpecFingerprint(create: Record<string, unknown>): string { |
There was a problem hiding this comment.
🤖 The compatibility fingerprint hashes env NAMES only, never values. It deliberately does not reuse the live pool's configFingerprint: that one hashes request fields and misses operator-level settings (DAYTONA_SNAPSHOT, DAYTONA_IMAGE, DAYTONA_TARGET) that also invalidate a stopped sandbox.
| async pauseSandbox() { | ||
| const provider = this.sandboxProvider; | ||
| const rawSandboxId = this.sandboxProviderRawId; | ||
| + let paused = false; |
There was a problem hiding this comment.
🤖 The gap-A fix in one sentence: the vendored pauseSandbox() used to clear the provider handles in a finally that ran even when the pause threw, so the follow-up delete silently did nothing and a RUNNING billed sandbox leaked. Now the handles are cleared only when paused is true. The patch was regenerated canonically with pnpm patch-commit and verified to apply against the pristine 0.4.2 tarball.
| if "data" in upsert.model_fields_set: | ||
| update_values["data"] = stmt.excluded.data | ||
| if "sandbox_id" in upsert.model_fields_set: | ||
| guarded_pointer_write = ( |
There was a problem hiding this comment.
🤖 The pointer guard: when the writer supplies sandbox_turn_index, sandbox_id and sandbox_fingerprint update only under this CASE compare-and-set against the row's latest_turn_index; a tokenless write behaves exactly as before, so existing callers are untouched. SQL note: inside ON CONFLICT DO UPDATE, the table-qualified column refers to the EXISTING row and excluded to the proposed one. The insert path (no existing row) applies unconditionally.
There was a problem hiding this comment.
we removed sandbox_fingerprint is this still relevant
There was a problem hiding this comment.
The fingerprint is gone but this guard stays relevant: it is the turn-index compare-and-set on sandbox_id itself, the multi-runner safety mechanism. It uses only pre-existing columns. Commit 20dbabb removed the fingerprint branch below it.
| : undefined; | ||
| if (sessionForMount && runCred) { | ||
| const liveSandboxId = environment.sandbox?.sandboxId ?? plan.sandboxId; | ||
| const pointerWriteOutcome = await ( |
There was a problem hiding this comment.
🤖 The pointer write moved from right-after-sandbox-start to right-after-continuity-hydrate, and it is now awaited. Reason: the guard token is the conversation's next turn index, and only the hydrated store knows it after a runner restart; the in-memory counter alone would say 0 and the guard would wrongly reject. Live verification showed applied on every turn.
| * TTL uses this because 0 is its documented off switch; with a nonzero shipping default, a | ||
| * positive-only parse would silently turn "0" back into the default. | ||
| */ | ||
| function nonNegativeIntEnv(name: string, fallback: number): number { |
There was a problem hiding this comment.
🤖 A real bug caught during the default flip: the existing positive-only env parse treats "0" as invalid and falls back to the default. With the default now 120000, setting the TTL to 0 would have silently re-enabled the feature. The Daytona TTL is the documented off switch, so it gets this non-negative parse where zero is a valid value.
| constructor( | ||
| private readonly config: Pick<KeepaliveConfig, "poolMax">, | ||
| private readonly logger: (message: string) => void = log, | ||
| private readonly options: { strictCapacity?: boolean } = {}, |
There was a problem hiding this comment.
🤖 strictCapacity is the Daytona pool's billing-safety mode: an evicted entry keeps its map seat (state destroyed) until its teardown RESOLVES, so size() counts stopping sandboxes and a park that races an in-flight stop parks to stopped instead of overshooting the billed cap. The local pool keeps today's fire-and-forget eviction (pinned by a test); its cap budgets host memory, not money.
| return provider === "daytona" ? "daytona" : undefined; | ||
| } | ||
|
|
||
| export function resolveKeepaliveDispatch( |
There was a problem hiding this comment.
🤖 The dispatch fails closed: only the two known provider names route to a pool; anything else runs today's cold path. Local requests resolve exactly as before, with the same env vars, so a local-only deployment is byte-identical.
| DAYTONA_IMAGE: ${DAYTONA_IMAGE:-} | ||
| DAYTONA_AUTOSTOP: ${DAYTONA_AUTOSTOP:-} | ||
| DAYTONA_AUTOARCHIVE: ${DAYTONA_AUTOARCHIVE:-} | ||
| AGENTA_RUNNER_DAYTONA_SESSION_IDLE_TTL_MS: ${AGENTA_RUNNER_DAYTONA_SESSION_IDLE_TTL_MS:-} |
There was a problem hiding this comment.
🤖 The two new knobs, forwarded wherever the DAYTONA_* timers already were (all compose files, env examples, helm): the live-window TTL (default 120000 ms, 0 disables) and the idle-warm cap (default 20). DAYTONA_AUTOARCHIVE is gone everywhere: the archive state is removed from the ladder.
|
🤖 The AI agent says: This implements the approved plan in PR #5214, all five slices. What you need to know before reading: What ran. Runner unit suite green on the exact pushed tree (875 passing, typecheck clean; extracted the pushed commit into a clean directory and ran the gates there, so the published tree is what was tested). API session_states unit tests 14/14, ruff clean. One credit-controlled live E3 pass on the dev stack: cold turn 12.5 s, live-warm second turn 1.39 s, stopped-restart after the window 7.7 s, one instance serving all three turns, expiry and SIGTERM drain both observed as stops, zero sandbox leaks (counted before and after, leftovers deleted). QA smoke chat passes on E2 local and E3 daytona. Browser playground sanity passed. What remains. The API acceptance tests for the guarded write need a live stack in CI. Cap-overflow behavior (more than 20 concurrent warm sessions) is unit-tested, not live-tested. Approval turns on Daytona keep taking the cold path until the F-018 gate plan lands, by design. Deploy note. One migration is required (core_oss Shared hunks. The worktree is shared with two unmerged lanes, so this diff carries their hunks per the coordination board's shared-file rule: Two assumptions from the plan's open questions, both the plan's own proposals: the pointer guard compares against the existing |
|
@coderabbitai review |
dosubot
Bot
added
size:XXL
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/design/agent-workflows/projects/qa/scripts/warm_daytona_probe.py (1)
1-131: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRuff format check is failing on this file.
CI reports
ruff format --checkwould reformat this file. Runruff format docs/design/agent-workflows/projects/qa/scripts/warm_daytona_probe.pyand commit the result.Sources: Coding guidelines, Pipeline failures
🧹 Nitpick comments (2)
services/runner/src/engines/sandbox_agent/sandbox-reconnect.ts (1)
79-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider deduplicating
writeSandboxPointer/clearSandboxPointer.Both functions share the same PUT/error-handling/logging scaffolding, differing only in the request body and the outcome-comparison expression. Extracting a shared
putPointer(sessionId, body, deps, isApplied)helper would reduce the risk of the two diverging on error handling later.♻️ Sketch of a shared helper
+async function putPointer( + sessionId: string, + body: Record<string, unknown>, + deps: SandboxPointerDeps, + isApplied: (sandboxId: string | null | undefined) => boolean, + logPrefix: string, +): Promise<SandboxPointerWriteOutcome> { + const log = deps.log ?? defaultLog; + const doFetch = deps.fetchImpl ?? fetch; + const base = deps.apiBase ?? apiBase(); + try { + const res = await doFetch( + `${base}/sessions/states/?session_id=${encodeURIComponent(sessionId)}`, + { + method: "PUT", + headers: { "content-type": "application/json", authorization: deps.authorization }, + body: JSON.stringify(body), + }, + ); + if (!res.ok) { + log(`${logPrefix} HTTP ${res.status} session=${sessionId}`); + return "failed"; + } + const resBody = (await res.json()) as { session_state?: { sandbox_id?: string | null } | null }; + return isApplied(resBody.session_state?.sandbox_id) ? "applied" : "rejected"; + } catch (err) { + log(`${logPrefix} failed session=${sessionId}: ${String(err instanceof Error ? err.message : err).slice(0, 120)}`); + return "failed"; + } +}services/runner/patches/sandbox-agent@0.4.2.patch (1)
97-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider logging when remote session load fails before falling back to replay.
The empty catch block at lines 116-117 silently swallows all
loadRemoteSessionerrors and falls back to replay-based creation. While the fallback is correct by design, a debug log would help operators diagnose why native session reload isn't being used.♻️ Suggested addition
} catch { + // loadSession failed; fall back to replay-based recreation below }Or with a log:
} catch (err) { + process.stderr.write(`[sandbox-agent] loadSession failed for session ${existing.id}: ${String(err).slice(0, 200)}\n`); }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 129ce70d-9087-4e4c-9a5d-71f115b98f8a
⛔ Files ignored due to path filters (1)
services/runner/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (45)
api/oss/databases/postgres/migrations/core_oss/versions/oss000000011_add_session_state_sandbox_fingerprint.pyapi/oss/src/apis/fastapi/sessions/models.pyapi/oss/src/core/sessions/states/dtos.pyapi/oss/src/dbs/postgres/sessions/states/dao.pyapi/oss/src/dbs/postgres/sessions/states/dbes.pyapi/oss/src/dbs/postgres/sessions/states/mappings.pyapi/oss/tests/pytest/acceptance/session_states/test_session_states_basics.pydocs/design/agent-workflows/projects/qa/scripts/warm_daytona_probe.pydocs/design/agent-workflows/projects/warm-daytona-sessions/implementation-status.mddocs/design/agent-workflows/projects/warm-daytona-sessions/pr-body.mddocs/docs/self-host/02-configuration.mdxdocs/docs/self-host/guides/09-agent-daytona-sandboxes.mdxhosting/docker-compose/ee/docker-compose.dev.ymlhosting/docker-compose/ee/docker-compose.gh.local.ymlhosting/docker-compose/ee/docker-compose.gh.ymlhosting/docker-compose/ee/env.ee.dev.examplehosting/docker-compose/ee/env.ee.gh.examplehosting/docker-compose/oss/docker-compose.dev.ymlhosting/docker-compose/oss/docker-compose.gh.local.ymlhosting/docker-compose/oss/docker-compose.gh.ssl.ymlhosting/docker-compose/oss/docker-compose.gh.ymlhosting/docker-compose/oss/env.oss.dev.examplehosting/docker-compose/oss/env.oss.gh.examplehosting/kubernetes/helm/templates/runner-deployment.yamlhosting/kubernetes/helm/values.yamlservices/runner/patches/sandbox-agent@0.4.2.patchservices/runner/src/engines/sandbox_agent.tsservices/runner/src/engines/sandbox_agent/daytona-provider.tsservices/runner/src/engines/sandbox_agent/mount.tsservices/runner/src/engines/sandbox_agent/provider.tsservices/runner/src/engines/sandbox_agent/sandbox-reconnect.tsservices/runner/src/engines/sandbox_agent/session-pool.tsservices/runner/src/engines/sandbox_agent/teardown.tsservices/runner/src/protocol.tsservices/runner/src/server.tsservices/runner/tests/unit/daytona-provider.test.tsservices/runner/tests/unit/sandbox-agent-acp-interactions.test.tsservices/runner/tests/unit/sandbox-agent-mount.test.tsservices/runner/tests/unit/sandbox-agent-provider.test.tsservices/runner/tests/unit/sandbox-lifecycle.test.tsservices/runner/tests/unit/sandbox-reconnect.test.tsservices/runner/tests/unit/session-keepalive-dispatch.test.tsservices/runner/tests/unit/session-pool.test.tsservices/runner/tests/unit/teardown.test.tsservices/runner/tests/unit/vendored-pause-fallback.test.ts
…connect Trust the stored sandbox pointer instead of comparing a create-spec fingerprint. A resumed Daytona turn reconnects the parked instance by id. Snapshot, image, and target drift is accepted as per-conversation version pinning. Network policy is converged at reconnect: read the live networkBlockAll/networkAllowList and call updateNetworkSettings only when they differ from the run's plan. Per-turn env sync is deferred to the daytona-secret-delivery direction (#5223). Removes the session_states.sandbox_fingerprint column and migration oss000000011, the createSpecFingerprint/buildResolvedDaytonaCreate helpers, and the fingerprint fields on the pointer read/write. Keeps the turn-index compare-and-set guard on sandbox_id, which uses only pre-existing columns. Claude-Session: https://claude.ai/code/session_018MaXPNpvzN22kngHno3VMj
|
Mahmoud Mabrouk seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Rework: dropped the sandbox fingerprint, converge network policy insteadThis pushes one additive commit (20dbabb) that reworks the reconnect path per review. The stored sandbox pointer is now trusted. A resumed turn reconnects the parked instance by id, with no create-spec fingerprint and no delete-and-rebuild compatibility check. The reasoning, field by field:
Verified updateNetworkSettings against a live Daytona sandbox (SDK 0.187.0): an allow-list blocked a public curl, an open update restored it on the running sandbox, the settings survived a stop and restart, and a second update on the restarted sandbox re-applied the block. Created 1 sandbox, deleted 1, final count 0. Removed: the session_states.sandbox_fingerprint column, migration oss000000011, createSpecFingerprint, buildResolvedDaytonaCreate, and the fingerprint fields on the pointer read/write. The dev DB had oss000000011 applied by hand; it was reverted (column dropped, alembic_version_oss reset to oss000000010) so future migrations stay healthy. Kept: the turn-index compare-and-set guard on sandbox_id (multi-runner safety, pre-existing columns only), the warm keepalive pool, the park/stop ladder, typed teardown reasons, the MAX_WARM cap, and the TTL=0 off switch. Before/after latency numbers still stand: cold 12.5s, live-warm 1.39s, stopped-restart 7.7s. |
| } | ||
| const state = await waitForStableState(sandbox, sandboxId, "reconnect"); | ||
| if (RUNNING_STATES.has(state)) { | ||
| await syncNetworkPolicy(sandbox, sandboxId); |
There was a problem hiding this comment.
Policy sync at reconnect. The desired policy comes from options.create (the same daytonaNetworkFields mapping used at create), so there is one source of truth. syncNetworkPolicy reads the live networkBlockAll/networkAllowList and calls updateNetworkSettings only when they differ. Verified against a live sandbox (SDK 0.187.0): the update takes effect on a running sandbox, survives a stop and restart, and re-applies on a restarted sandbox. A failed update logs and leaves the prior policy rather than aborting the reconnect.
| else_=SessionStateDBE.sandbox_id, | ||
| ) | ||
| elif "sandbox_id" in upsert.model_fields_set: | ||
| update_values["sandbox_id"] = stmt.excluded.sandbox_id |
There was a problem hiding this comment.
Kept: the turn-index compare-and-set on sandbox_id (guarded_pointer_write above). This is the multi-runner safety mechanism and it uses only pre-existing columns. Only the fingerprint branch was removed here, so a stale write still cannot overwrite a newer pointer.
| ) | ||
| : undefined; | ||
| if (storedSandboxId) { | ||
| if (storedSandboxPointer) { |
There was a problem hiding this comment.
Trusted-pointer semantics. A stored sandbox id is now reconnected by id with no create-spec fingerprint and no delete-and-rebuild compatibility check. Snapshot, image, and target drift is accepted as per-conversation version pinning. Network policy converges inside reconnect; per-turn env sync is deferred to #5223.
- Gate the sandbox pointer write on the Daytona plan so a local run cannot overwrite a conversation's remote pointer. - Hydrate the continuity store before the guarded pointer clear so a post-restart clear carries the durable turn index. - pause() throws for the error sandbox state so teardown falls back to delete instead of reporting a parked sandbox with a stale pointer. - Catch live-park activity-refresh failures; the hook is best-effort. - Dedupe writeSandboxPointer/clearSandboxPointer into one guarded PUT. - Assert status_code in the tokenless pointer acceptance test. - ruff format on the warm_daytona_probe QA script (CI red). Claude-Session: https://claude.ai/code/session_018MaXPNpvzN22kngHno3VMj
The branch flipped AGENTA_AGENT_MCPS_ENABLED from false to true across the compose files, env examples, and the helm comment. The flip is unrelated to warm Daytona sessions and contradicts the SDK gate and the helm template, which both default off. Restore false everywhere. Claude-Session: https://claude.ai/code/session_018MaXPNpvzN22kngHno3VMj
| if "data" in upsert.model_fields_set: | ||
| update_values["data"] = stmt.excluded.data | ||
| if "sandbox_id" in upsert.model_fields_set: | ||
| guarded_pointer_write = ( |
There was a problem hiding this comment.
we removed sandbox_fingerprint is this still relevant
| # AGENTA_RUNNER_HOST=0.0.0.0 | ||
| # AGENTA_API_KEY= | ||
| # AGENTA_AGENT_MCPS_ENABLED=false | ||
| # AGENTA_AGENT_MCPS_ENABLED=true |
There was a problem hiding this comment.
is this the default? why did we change it
There was a problem hiding this comment.
No, off is the default: the SDK gate and the helm template both default to false. The flip to true rode along in this branch by accident and is unrelated to warm Daytona sessions. Reverted to false everywhere in a588b08.
| const state = await waitForStableState(sandbox, sandboxId, "pause"); | ||
| if (RUNNING_STATES.has(state)) await sandbox.stop(); | ||
| else if ( | ||
| !STOPPED_STATES.has(state) && |
There was a problem hiding this comment.
🤖 The AI agent says:
[P1] Escalate failed Daytona states to deletion
When a sandbox is in Daytona’s error state, this condition treats the pause as successful because FAILED_STATES is excluded from the throwing branch. The caller consequently sets parked = true, skips destroySandbox(), and clears the provider handle, potentially leaving an errored but billable sandbox undeleted. Only already stopped or missing sandboxes should be successful no-ops. An error state must throw so the delete fallback runs.
There was a problem hiding this comment.
Fixed in 59e49b9: pause now throws for the error state, so parked=true is never set and the delete fallback reclaims the errored sandbox. Only stopped, archived, and destroyed remain no-op successes. Same finding as CodeRabbit's, both serviced.
Review-servicing pass: two commits, all outstanding comments answeredCommits 59e49b9 and a588b08 service every open reviewer comment. Each thread has an in-line reply. Changed:
Declined:
Runner suite: 881 passing, typecheck clean. Ruff clean on all touched Python. |
…n matrix Runner respawn+remount and warm/reused sandboxes are merged and the runner remounts the object-store cwd/agent prefixes; update the resume row so the remaining work reads as just the FE 'Resume' affordance.
2 participants
Context
Every turn of a Daytona-backed agent conversation waited about 15 seconds, because the runner
deleted the sandbox at every turn end and rebuilt everything on the next turn. The warm-reuse
plumbing already existed (park at turn end, a stored sandbox id, native session reload), but
the Daytona provider had no pause or reconnect function, so "park" silently fell through to
delete. The plan and measurements behind this change are in PR #5214.
What this adds
The two reuse levels from the plan, plus the correctness work that makes reuse safe:
session for a short window (
AGENTA_RUNNER_DAYTONA_SESSION_IDLE_TTL_MS, default 120000 ms;0 disables it, there is no separate flag). A second turn inside the window continues the
live session. Measured on the dev stack: 1.39 s against 12.5 s cold.
on SIGTERM while idle), the sandbox is stopped, not deleted. The next turn restarts the
same instance and reloads the harness session natively. Measured: 7.7 s against 12.5 s
cold, because
session/load(1.2 s) replaces the 5.2 s session create.AGENTA_RUNNER_DAYTONA_SESSION_MAX_WARM, default20). It bounds idle spend only; an active turn is never blocked. On overflow a finishing
turn parks to stopped. The Daytona pool frees a warm slot only after a stop is confirmed,
so an in-flight stop cannot let the billed count overshoot.
The correctness base underneath:
pauseandreconnectfunctions (a runner-sidewrapper over
sandbox-agent): stop only from a running state, wait out Daytona'stransitional states with a bound, resume stopped or archived instances, and fail cleanly on
error states. Two teardown bugs in the vendored package are fixed in the package patch: a
failed pause used to clear the provider handle so the delete fallback silently did nothing
(a leaked billed sandbox), and a reconnected sandbox that failed later attach steps leaked.
session_states.sandbox_id) is trusted. A resumed turn reconnects theparked instance by id. Snapshot, image, and target drift is accepted as per-conversation
version pinning, like a rolling deploy: an old parked sandbox keeps serving its conversation
until an idle gap hits the delete ladder.
networkBlockAllandnetworkAllowListand calls Daytona'supdateNetworkSettingsonlywhen they differ from the run's plan. That call applies the same runner-side iptables
mechanism as create, so a parked sandbox picks up a policy change without a rebuild. A failed
convergence logs and leaves the prior policy rather than aborting the reconnect.
index is not older than the row's
latest_turn_index(a compare-and-set; tokenless writeskeep today's behavior). The guard uses only pre-existing columns, so this feature adds no
migration.
keepWarmboolean. Kill, failed, and aborteddelete; a clean resumable turn and idle shutdown stop; in-flight shutdown deletes. The
keepalive pool passes its eviction reason through the same mapping.
restoring from archive measured slower than creating fresh. The ladder is stop after 15
idle minutes (was 5), delete after 30.
SessionPoolper provider. Local behavior and env vars are unchanged; the local poolkeeps its fire-and-forget eviction, the Daytona pool gets strict awaited capacity.
[timing]lines (create, install, mounts, workspace, probe,session), so the next latency investigation reads the logs instead of hand-instrumenting.
Environment sync is deferred
Per-turn environment-variable delivery and value rotation are out of scope here. That is
per-turn delivery work and follows the daytona-secret-delivery direction (#5223). Create-time
env baking is unchanged in this PR.
Assumptions from the two open plan questions
latest_turn_indexturn counter (theplan's default wording; no schema migration). Two truly concurrent turns of the same
conversation carry the same index and still race; the guard closes the older-write-lands-last
window. The Redis owner claim remains the stronger future mechanism.
/killstays a harddelete (the plan's proposal).
Tests
reject, tokenless unconditional, missing-row create).
updateNetworkSettingstookeffect on a running sandbox (an allow-list blocked a public curl, then an open update
restored it), the settings survived a stop and restart, and a second update on the restarted
sandbox re-applied the block. Created 1 sandbox, deleted 1, final count 0.
leftovers deleted, final Daytona sandbox count 0): cold 12.5 s, live-warm 1.39 s,
stopped-restart 7.7 s; one instance served all three turns; window expiry observed as a
stop; SIGTERM drain observed stopping the idle parked sandbox; guarded pointer writes
observed applied.
run_matrix.pysmoke_chat_pi PASS on E2 local and E3 daytona after the change.What to QA
The second reply should arrive in about 1 to 3 seconds instead of 10 or more.
stopped restart) and the conversation context should be intact.
latency);
/killstill deletes the sandbox immediately.Session: https://claude.ai/code/session_018MaXPNpvzN22kngHno3VMj
https://claude.ai/code/session_018MaXPNpvzN22kngHno3VMj