fix(orchestrator): enforce sandbox TTL on the node via WaitForExit#3194
fix(orchestrator): enforce sandbox TTL on the node via WaitForExit#3194AdaAibaby wants to merge 2 commits into
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Using time.Until(s.GetEndAt()) > 0 to detect if the deadline was extended can cause a busy loop or unnecessary timer re-creations if the timer fires slightly early. Comparing the current deadline against the previously observed deadline using s.GetEndAt().After(endAt) ensures the loop only continues if the deadline was actually extended. Additionally, logging 'sandbox TTL expired, stopping' is misleading when the exit wait fails because the Firecracker process exited prematurely; a generic message like 'sandbox exit wait failed, stopping' should be used instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
WaitForExit was defined but never called, leaving the API-layer evictor as the sole mechanism to kill expired sandboxes. If the API service restarts the evictor goroutine stops and VMs accumulate indefinitely on nodes. Two changes: 1. setupSandboxLifecycle (sandboxes.go): replace sbx.Wait with sbx.WaitForExit so the lifecycle goroutine races against the sandbox TTL. On timeout, Stop() is called explicitly before the normal Close/cleanup path runs. 2. WaitForExit (sandbox.go): replace the one-shot time.After with a time.NewTimer loop that re-checks endAt after each fire. This means a KeepAlive that calls SetEndAt mid-flight correctly resets the node-side deadline instead of being silently ignored. Fixes e2b-dev#3193
5 cases covering the two code changes in the parent commit: - AlreadyExpired: endAt in the past returns error immediately - ExitBeforeTTL: clean FC exit before TTL returns nil - ExitWithError: FC exits with error, error is wrapped and returned - KeepAliveExtendsTTL: SetEndAt extension resets the timer loop; sandbox is NOT killed at the original deadline - ContextCancelled: ctx cancel returns nil without killing All pass with -race.
AdaAibaby
force-pushed
the
fix/orchestrator-sandbox-timeout-enforcement
branch
from
4b62c59 to
4826a8e
Compare
2 participants
Problem
WaitForExitwas defined insandbox.gobut never called anywhere in the codebase. The API-layer evictor (polling Redis every 50 ms) was the sole mechanism to kill expired sandboxes. When the API service restarts, the evictor stops and Firecracker VMs accumulate on nodes indefinitely.Closes #3193
Changes
packages/orchestrator/pkg/server/sandboxes.goReplace
sbx.Waitwithsbx.WaitForExitinsetupSandboxLifecycle. The lifecycle goroutine now races against the sandbox TTL. On timeout,Stop()is called explicitly before the existingClose/cleanup path.packages/orchestrator/pkg/sandbox/sandbox.goReplace the one-shot
time.AfterinWaitForExitwith atime.NewTimerloop that re-checksendAtafter each fire. A KeepAlive that callsSetEndAtmid-flight now correctly resets the node-side deadline.packages/orchestrator/pkg/sandbox/wait_for_exit_test.go(new)5 unit tests covering all branches of
WaitForExit:TestWaitForExit_AlreadyExpired— past endAt returns error immediatelyTestWaitForExit_ExitBeforeTTL— clean exit before TTL returns nilTestWaitForExit_ExitWithError— FC error is wrapped and returnedTestWaitForExit_KeepAliveExtendsTTL—SetEndAtextension resets the timer; sandbox is not killed at original deadlineTestWaitForExit_ContextCancelled— ctx cancel returns nilAll pass with
-race.Behaviour after this fix
endAtindependentlyendAtTesting