refactor(orchestrator): Refactor orchestrator shutdown to not leak resources#2990
refactor(orchestrator): Refactor orchestrator shutdown to not leak resources#2990wj-e2b wants to merge 9 commits into
Conversation
PR SummaryHigh Risk Overview Reviewed by Cursor Bugbot for commit f927e79. Bugbot is set up for automated code reviews on this repo. Configure here. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![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
The wg variable in packages/orchestrator/pkg/server/main.go is declared as a sync.WaitGroup but incorrectly calls a non-existent Go method, which will cause a compilation error; this should be replaced with wg.Add(1) and a standard goroutine. Additionally, the isIPTablesNotExist helper in packages/orchestrator/pkg/sandbox/network/network.go incorrectly attempts to use errors.As with a custom notExistError interface, but errors from the go-iptables library do not implement this interface, meaning the check will always return false and fail to detect non-existent rules.
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.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f98b580. Configure here.
wj-e2b
force-pushed
the
orchestrator-fix
branch
2 times, most recently
from
eebb237 to
6aa4d64
Compare
Use stdlib pidfd-backed signaling (os.Process is pidfd-backed since Go 1.23) so FC processes cannot be signaled after PID reuse, kill the sandbox cgroup on stop and wait for it to empty via poll on cgroup.events instead of polling cgroup.procs, and keep memory shutdown running even when the exit wait context is canceled so UFFD can exit.
Track sandbox lifecycle entries in the sandbox map so shutdown can observe live sandboxes, and mark sandboxes stopped on Close so the map reflects reality even when cleanup partially fails.
Return network slots synchronously during sandbox cleanup instead of fire-and-forget goroutines, guard firewall teardown against missing iptables state, and preserve ErrClosed when returning slots to a closed pool.
Introduce a shared draingate.Gate (counter plus notification channel) and use it in the sandbox factory and gRPC server to reject new sandbox starts while draining, wait for in-flight starts, and drain or force-stop live sandboxes before closers run. Forced shutdown preserves buffered close errors on context cancellation and avoids duplicate final-pass errors. Adds utils.WaitGroupWait to wait on a WaitGroup with context cancellation. The graceful drain phase is bounded by SHUTDOWN_DRAIN_TIMEOUT; when it expires the drain escalates to a forced sandbox shutdown. By default the drain waits forever, until sandboxes exit on their own or a force-stop API call empties the node.
Gate new template build, delete, and layer upload requests behind the shared drain gate, cancel or await in-flight builds via the build cache, and split ServerStore shutdown into Wait (graceful) and ForceStop so callers choose the escalation explicitly. If the graceful template drain is cut short (for example by SHUTDOWN_DRAIN_TIMEOUT), shutdown escalates to a bounded template force stop instead of abandoning in-flight builds.
Make ServiceInfo status transitions atomic via TransitionStatus so a drain override cannot race a concurrent transition back to healthy, reject drain reversal (including via an Unhealthy detour), guard ForceStop with sync.Once while preserving drain-to-force-stop escalation, and plumb a forceStop flag from the API through the node status change so draining can force stop sandboxes and template builds.
Reject forceStop requests unless the target status is draining, and map orchestrator InvalidArgument and FailedPrecondition errors to 400 and 409. FailedPrecondition intentionally diverges from grpc-gateway's 400 mapping because drain reversal is a node state conflict, not malformed input.
ServerStore.Close returned early when its context was already done, skipping the registered closers (such as the Docker Hub repository client). During forced shutdown (FORCE_STOP) the shutdown context is canceled before closers run, and with SHUTDOWN_DRAIN_TIMEOUT it can be expired after a timed-out drain, so the template server closer always failed and leaked its clients in those paths. Waiting for builds is handled by Wait and ForceStop during the drain phase, so the early exit no longer guards anything. Always run the closers regardless of context state.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f927e79731
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if req.GetForceStop() { | ||
| s.forceStopOnce.Do(func() { |
There was a problem hiding this comment.
When the first force_stop request hits a timeout or any cleanup error from drainController.ForceStop, this sync.Once is still consumed, so later admin requests with forceStop=true while the node remains Draining return success but never launch another cleanup attempt. Since the forced stop path has bounded 30s waits and can report errors, this can leave sandboxes/builds running with no API retry path short of restarting the process.
Useful? React with 👍 / 👎.
2 participants
Uh oh!
There was an error while loading. Please reload this page.