fix(agent): stop Daytona sandbox leak (credit-burner) - #4856
Conversation
A process KILL (docker stop/SIGTERM/SIGKILL/OOM) mid-run skips the per-run `finally` that deletes the sandbox, so it leaks. There was also no server-side backstop: the SDK wrapper hardcodes `autoStopInterval: 0` (auto-stop OFF) while `ephemeral: true` only auto-deletes ON STOP, so they cancel out and a leaked sandbox never self-reaps. - provider.ts: set a non-zero `autoStopInterval` on the Daytona create object (env `SANDBOX_AGENT_DAYTONA_AUTOSTOP_MINUTES`, default 15, clamped >= 1). With auto-stop > 0 the ephemeral auto-delete fires, so a leaked sandbox self-reaps. Extracted `buildDaytonaCreate` so the create object is unit-testable. - server.ts: `registerShutdownHandler` deletes in-flight sandbox(es) on SIGTERM/SIGINT before exit; timeout-bounded + idempotent so it cannot hang shutdown. - sandbox_agent.ts: in-flight sandbox registry (`destroyInFlightSandboxes`) the handler drains; register after start, remove in the `finally`. Resources (cpu/mem/disk) left untouched (snapshot-baked). No /run wire change. Claude-Session: https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc
|
@coderabbitai review |
dosubot
Bot
added
the
size:L
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds Daytona sandbox auto-stop configuration via a new environment variable, documents the setting, and adds shutdown handling that tracks in-flight sandboxes and destroys them on SIGTERM/SIGINT. ChangesSandbox auto-stop and shutdown cleanup
Sequence Diagram(s)sequenceDiagram
participant Process
participant registerShutdownHandler
participant destroyInFlightSandboxes
participant SandboxHandle
participant exit
Process->>registerShutdownHandler: SIGTERM / SIGINT
registerShutdownHandler->>destroyInFlightSandboxes: onCleanup(timeoutMs)
destroyInFlightSandboxes->>SandboxHandle: destroySandbox()
registerShutdownHandler->>exit: exit(0)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
✅ Action performedReview finished.
|
|
Review ask — please focus on:
Runner-only, no |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 07440e73-8a62-44ce-9213-2ed41293a4e7
📒 Files selected for processing (6)
docs/design/agent-workflows/documentation/running-the-agent.mdservices/agent/src/engines/sandbox_agent.tsservices/agent/src/engines/sandbox_agent/provider.tsservices/agent/src/server.tsservices/agent/tests/unit/sandbox-agent-provider.test.tsservices/agent/tests/unit/server.test.ts
| const sweep = Promise.allSettled( | ||
| pending.map((sandbox) => | ||
| Promise.resolve(sandbox.destroySandbox?.()).catch(() => {}), | ||
| ), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node <<'NODE'
const sandboxes = [
{ destroySandbox() { throw new Error("boom"); } },
{ destroySandbox() { console.log("second sandbox reached"); return Promise.resolve(); } },
];
try {
Promise.allSettled(
sandboxes.map((sandbox) =>
Promise.resolve(sandbox.destroySandbox?.()).catch(() => {})
)
);
console.log("allSettled created");
} catch (err) {
console.log("sync throw aborted sweep:", err.message);
}
NODERepository: Agenta-AI/agenta
Length of output: 185
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant implementation around the cited lines and adjacent call sites.
file="services/agent/src/engines/sandbox_agent.ts"
echo "== outline =="
ast-grep outline "$file" --view expanded || true
echo
echo "== relevant lines =="
sed -n '95,125p' "$file" | cat -n
echo
echo "== call sites for destroyInFlightSandboxes =="
rg -n "destroyInFlightSandboxes|destroySandbox\\?\\(" services/agent/src -nRepository: Agenta-AI/agenta
Length of output: 3066
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant implementation around the cited lines and adjacent call sites.
file="services/agent/src/engines/sandbox_agent.ts"
echo "== outline =="
ast-grep outline "$file" --view expanded || true
echo
echo "== relevant lines =="
sed -n '95,125p' "$file" | cat -n
echo
echo "== call sites for destroyInFlightSandboxes =="
rg -n "destroyInFlightSandboxes|destroySandbox\\?\\(" services/agent/src -nRepository: Agenta-AI/agenta
Length of output: 3066
Make each destroy call exception-safe before Promise.allSettled()
Promise.resolve(sandbox.destroySandbox?.()) still invokes destroySandbox() immediately. If one sandbox throws synchronously, map() aborts and the remaining in-flight sandboxes are never attempted.
Suggested fix
const sweep = Promise.allSettled(
pending.map((sandbox) =>
- Promise.resolve(sandbox.destroySandbox?.()).catch(() => {}),
+ Promise.resolve()
+ .then(() => sandbox.destroySandbox?.())
+ .catch(() => {}),
),
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const sweep = Promise.allSettled( | |
| pending.map((sandbox) => | |
| Promise.resolve(sandbox.destroySandbox?.()).catch(() => {}), | |
| ), | |
| const sweep = Promise.allSettled( | |
| pending.map((sandbox) => | |
| Promise.resolve() | |
| .then(() => sandbox.destroySandbox?.()) | |
| .catch(() => {}), | |
| ), |
| void onCleanup() | ||
| .catch(() => {}) | ||
| .finally(() => exit(0)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node <<'NODE'
function run(onCleanup, exit) {
try {
onCleanup().catch(() => {}).finally(() => exit(0));
} catch (err) {
console.log("sync throw bypassed catch/finally:", err.message);
}
}
run(() => { throw new Error("boom"); }, (code) => console.log("exit", code));
NODERepository: Agenta-AI/agenta
Length of output: 194
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== server.ts around shutdown handler ==\n'
sed -n '200,280p' services/agent/src/server.ts
printf '\n== registerShutdownHandler references ==\n'
rg -n "registerShutdownHandler|onCleanup" services/agent/src/server.ts services/agent/src -g '!**/node_modules/**'Repository: Agenta-AI/agenta
Length of output: 4417
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== server.ts around shutdown handler ==\n'
sed -n '200,280p' services/agent/src/server.ts
printf '\n== registerShutdownHandler references ==\n'
rg -n "registerShutdownHandler|onCleanup" services/agent/src/server.ts services/agent/src -g '!**/node_modules/**'Repository: Agenta-AI/agenta
Length of output: 4417
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== server.ts around shutdown handler ==\n'
sed -n '200,280p' services/agent/src/server.ts
printf '\n== registerShutdownHandler references ==\n'
rg -n "registerShutdownHandler|onCleanup" services/agent/src/server.ts services/agent/src -g '!**/node_modules/**'Repository: Agenta-AI/agenta
Length of output: 4417
Normalize onCleanup before chaining the exit path.
onCleanup().catch(...).finally(...) only handles returned promises. A synchronous throw from a custom cleanup callback skips exit(0), so wrap the call in Promise.resolve() first.
Suggested fix
- void onCleanup()
+ void Promise.resolve()
+ .then(() => onCleanup())
.catch(() => {})
.finally(() => exit(0));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void onCleanup() | |
| .catch(() => {}) | |
| .finally(() => exit(0)); | |
| void Promise.resolve() | |
| .then(() => onCleanup()) | |
| .catch(() => {}) | |
| .finally(() => exit(0)); |
1 participant
The leak
Daytona sandboxes the runner created were leaking and burning credit. 10 were found alive long after their runs ended.
The per-run teardown (
finallyinrunSandboxAgent) deletes the sandbox on every normal / error / client-disconnect path — that part works. But a process KILL (docker stop/ SIGTERM / SIGKILL / OOM mid-run) skips thefinallyentirely, so the sandbox is never deleted.There was also no server-side backstop. The
sandbox-agentSDK wrapper hardcodesautoStopInterval: 0(auto-stop OFF), whileephemeral: trueonly auto-deletes a sandbox ON STOP. Those two cancel out: a sandbox that never stops is never auto-deleted, so a leaked one self-reaps never.The fix (runner-only, two-part backstop)
1. Server-side TTL backstop —
provider.ts. The Daytona create object now sets a non-zeroautoStopIntervalalongside the existingephemeral: true. The wrapper spreads our create object after itsautoStopInterval: 0hardcode, so our value wins. With auto-stop > 0, an idle leaked sandbox stops on its own, which then triggers the ephemeral auto-delete — it self-reaps. Configurable via a newSANDBOX_AGENT_DAYTONA_AUTOSTOP_MINUTES(default15min — the Daytona SDK's own documented default; clamped to>= 1so a0cannot re-disable auto-stop and reintroduce the leak).autoStopIntervalmeasures idle time and an actively prompting sandbox is busy, so this does not cut live runs short. ExtractedbuildDaytonaCreateso the create object is unit-testable (the realdaytona()provider closes over it).2. Shutdown signal handler —
server.ts.registerShutdownHandlerdeletes any in-flight sandbox(es) on SIGTERM / SIGINT before exit, so a gracefuldocker stopcleans up immediately instead of waiting on the auto-stop. It drains a new in-flight registry insandbox_agent.ts(destroyInFlightSandboxes; sandboxes register afterstartSandboxAgent, deregister in thefinally). The handler is timeout-bounded (5 s race) and idempotent against a repeated signal, so it can never hang shutdown — and the auto-stop backstop covers the SIGKILL/OOM cases a signal can never reach.Resources (cpu/mem/disk) are left untouched — they are snapshot-baked in
build_snapshot.py. No/runwire change.Tests
daytonaAutoStopMinutesenv parsing: env value, fractional floor, unset/non-numeric/0/negative fall back to the default.buildDaytonaCreatecarries a positiveautoStopInterval+ephemeral(default and env-configured).registerShutdownHandler: registers a listener per signal, runs cleanup then exits, still exits when cleanup rejects, and cleans up only once on a repeated signal.tsc.https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc