Conversation
PR SummaryHigh Risk Overview Lifecycle tracking extends the sandbox map with a lifecycle index ( Shutdown flow in Cleanup fixes: Firecracker stop signals the whole process group and polls until it is gone; network teardown ignores already-removed iptables/routes/links/namespaces and rolls back partial setup on create failure; nftables firewall tables are deleted on close; network slot return is synchronous; the network pool cleans up slots created while closing. Minor shutdown noise handling treats expected Reviewed by Cursor Bugbot for commit 3c8efb1. Bugbot is set up for automated code reviews on this repo. Configure here. |
❌ 4 Tests Failed:
View the full list of 4 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
There was a problem hiding this comment.
Code Review
The use of wg.Go in ForceStopSandboxes will cause a compilation error because sync.WaitGroup does not provide that method. In doStop, returning early when WaitWithContext fails prevents memory resource cleanup, which can lead to leaked userfaultfd processes.
7324b01 to
8c5b901
Compare
8c5b901 to
6443f22
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6443f22c3b
ℹ️ 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".
| live *smap.Map[*Sandbox] | ||
| network *smap.Map[*Sandbox] | ||
| live *smap.Map[*Sandbox] | ||
| lifecycles *smap.Map[lifecycleEntry] |
There was a problem hiding this comment.
Is this a superset of the live and network maps? Can we remove either of those and exclusively use this new map?
There was a problem hiding this comment.
They're all used for different purposes that are disjoint. We can maybe merge the live one at some point, but this requires a bunch of code changes
|
Before merging, we should do the following:
That should give us enough confidence to say "this is fixed" |
6443f22 to
31f1f51
Compare
a07b91d to
4ef20ff
Compare
|
@claude review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ef20ffa5c
ℹ️ 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".
There was a problem hiding this comment.
This is a substantial shutdown/lifecycle refactor touching critical orchestrator paths — the inline finding on the lifecycle map race is worth addressing before merge, and the outstanding architectural threads and your own traffic-simulator soak plan make this a clear human-review case.
Extended reasoning...
Overview
This PR refactors orchestrator shutdown across 13 files: introduces a new sandbox-lifecycle index (Map.lifecycles) with running/stopping states, adds a graceful DrainSandboxes / ForceStopSandboxes path with an in-flight start gate (sandboxStartMu), changes Firecracker teardown to signal the whole process group, hardens iptables/route/link/namespace removal against benign ENOENT/ESRCH/EPERM, removes the nftables firewall table on close, rolls back partial network setup on create failure, and converts the network-slot return from a detached goroutine to synchronous cleanup. Also wires the sandbox drain closer into the shutdown closer chain and ignores benign EINVAL from logger Sync() and the expected serviceDoneError.
Security risks
No direct authn/authz/crypto changes. The closest security-relevant change is signalProcessGroup using kill(-pid, …) — the prior syscall.Kill with a stale PID was already flagged by cursor, and the current code does check processGroupExists before the SIGKILL but still uses a stored PID that could theoretically be reused after waitpid; worth a human eye, though Firecracker's lifecycle makes this narrow in practice. iptables/nft cleanup now treats "not exist" errors as success, which is a behavioral change in the cleanup direction (more lenient) and not in the create direction.
Level of scrutiny
High. This is the orchestrator's hot shutdown path on a host that runs untrusted sandboxes — bugs here translate to leaked kernel resources, stuck drains, or zombie entries that mask real state. The new Map.lifecycles is a parallel index to live/network, and any divergence between the indexes shows up only under shutdown races. The PR author explicitly proposed a 24-hour traffic-simulator soak before merge, which is the right bar.
Other factors
The bug hunter surfaced a concrete non-atomic Get+Insert race in markLifecycleState that can resurrect a lifecycle entry after MarkStopped removed it — permanent zombie, slow leak compounded per occurrence, with a clean fix via the existing smap.Upsert callback that mirrors what MarkStopping already does on the live map. Separately, there are still open threads from djeebus (whether these helpers belong on lifecycle.Manager, dependency-injection-as-strings concern, the "close request ingress first" point doubling down on codex's P1) that don't appear resolved in the timeline. Test coverage is added but is unit-level — the integration soak the author described has not happened yet per the timeline.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 525136ce9f
ℹ️ 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".
525136c to
f1b97d3
Compare
f1b97d3 to
1054a90
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1054a90419
ℹ️ 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".
1054a90 to
038dbc6
Compare
642587e to
3c8efb1
Compare
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 3c8efb1. Configure here.
| err := p.cmd.Process.Kill() | ||
| if err == nil { | ||
| logger.L().Info(ctx, "sent SIGKILL to fc process because it was not responding to SIGTERM for 10 seconds", | ||
| killErr := signalProcessGroup(pid, syscall.SIGKILL) |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: MEDIUM
The shutdown path still sends SIGKILL to -pid after a delay based only on the stored numeric PID. If the original Firecracker process group exits and that PID is reused before the delayed kill path runs, this can terminate an unrelated process group.
Impact: tenant-triggerable sandbox churn can cause cross-sandbox availability impact by killing the wrong process group.
Reviewed by Cursor Security Reviewer for commit 3c8efb1. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c8efb1a71
ℹ️ 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".
| s.StartDraining(ctx) | ||
| // The sandbox factory is shared by API sandboxes and template-build sandboxes. | ||
| // Drain it before waiting so the lifecycle snapshot cannot miss a new build sandbox. | ||
| s.sandboxFactory.StartDraining(ctx) |
There was a problem hiding this comment.
When a graceful shutdown starts while a Checkpoint RPC is already past enterSandboxStart but still snapshotting the old sandbox, this closes the shared sandbox factory before waitSandboxStarts lets that in-flight RPC reach ResumeSandbox. The checkpoint then gets ErrFactoryDraining at the resume call, and because it has already MarkStopping'd the old sandbox and deferred stopSandboxAsync, the accepted checkpoint fails by tearing down the only live sandbox instead of letting the drain wait for it. Drain the factory only after the server start gate has quiesced for graceful shutdown, or otherwise allow already-entered starts to complete.
Useful? React with 👍 / 👎.
Pass 1.
Do some mild lifecycle tracking and then fix all the shutdown cleanup bugs.
Verified by shutting down orchestrator and seeing no leaked kernel resources.
Should enable graceful shutdowns.