Repository navigation
fix(gateway): run the VM compute driver in its own process group - #5068
vineethsai7 wants to merge 1 commit into
Conversation
Ctrl+C on a gateway running in the foreground sends SIGINT to the terminal's whole foreground process group. The VM compute driver and its VM launchers inherited the gateway's group, so they exited on that signal before the gateway's shutdown sweep asked the driver to stop its sandboxes. The sweep then failed (stopped=0 failed=1), an attached `sandbox create --no-keep` reported "ephemeral cleanup failed", and the sandbox came back in Error after a restart. Spawn the driver with process_group(0). Terminal signals now reach only the gateway, which runs its sweep while the driver is still serving and then stops the driver itself with SIGTERM. The launchers inherit the driver's new group, so they are also out of the terminal's group and stay under the driver's control. A gateway that dies without a graceful shutdown still takes the driver with it: the driver arms a parent-death signal at startup, which does not depend on the process group. Closes NVIDIA#4770 Signed-off-by: Vineeth Sai Narajala <vnarajal@cisco.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Thank you for your interest in contributing to OpenShell, @vineethsai7. This project uses a vouch system for first-time contributors. Before submitting a pull request, you need to be vouched by a maintainer. To get vouched:
See CONTRIBUTING.md for details. |
|
Thank you for your submission! We ask that you sign our Developer Certificate of Origin before we can accept your contribution. You can sign the DCO by adding a comment below using this text: I have read the DCO document and I hereby sign the DCO. You can retrigger this bot by commenting recheck in this Pull Request. Posted by the DCO Assistant Lite bot. |
![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: 5b772589e0
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| )) | ||
| } | ||
|
|
||
| /// Base command for the VM compute-driver subprocess. |
There was a problem hiding this comment.
The reviewed commit contains no Signed-off-by trailer even though its checklist marks DCO complete, so it does not satisfy this repository's per-commit DCO requirement and the DCO check cannot accept it; add the author's valid sign-off.
AGENTS.md reference: AGENTS.md:L191-L191
Useful? React with 👍 / 👎.
|
|
||
| /// Base command for the VM compute-driver subprocess. | ||
| /// | ||
| /// The driver gets its own process group. Ctrl+C on a foreground gateway |
There was a problem hiding this comment.
The reviewed commit message ends with a Generated with [Claude Code] attribution, which repository policy explicitly forbids in commit or PR text; remove that footer before this commit is accepted.
AGENTS.md reference: AGENTS.md:L192-L192
Useful? React with 👍 / 👎.
| fn compute_driver_command(driver_bin: &Path) -> Command { | ||
| let mut command = Command::new(driver_bin); | ||
| command.kill_on_drop(true); | ||
| command.process_group(0); |
There was a problem hiding this comment.
When a foreground gateway is started in a terminal configured with stty tostop, this makes the driver a background process group while its stdout and stderr remain attached to that terminal. Its first log write therefore receives SIGTTOU and stops the driver, so wait_for_compute_driver times out and the gateway cannot start; route the child's output through the foreground gateway or otherwise detach it from the controlling terminal while preserving the SIGINT isolation.
Useful? React with 👍 / 👎.
|
I have read the DCO document and I hereby sign the DCO. |
Summary
Run the VM compute driver in its own process group, so Ctrl+C on a gateway running in the foreground reaches only the gateway. The gateway's shutdown sweep then stops sandboxes while the driver is still up, instead of failing with
stopped=0 failed=1and leaving the sandbox inErrorafter a restart.Related Issue
Closes #4770
Downstream report: cisco-ai-defense/defenseclaw#1221
Related: #4409 changes the same shutdown sweep for a different trigger (a pending driver operation on SIGTERM). This PR does not touch the sweep.
Problem
Repro with a foreground VM gateway:
openshell-gatewaywith the VM compute driver in a terminal.openshell sandbox create --no-keep -- sleep 600from another terminal.openshell sandbox list.Observed: the gateway logs
gateway shutdown cleanup failedwithstopped=0 failed=1, the attached CLI exits 255 withephemeral cleanup failed, and after the restart the sandbox is listed inError. A gateway stopped with SIGTERM from systemd or launchd is not affected.Root cause: Ctrl+C makes the terminal send SIGINT to its whole foreground process group.
vm::spawnstartedopenshell-driver-vmin the gateway's group, and the--internal-run-vmlaunchers that the driver starts inherit that group, so the gateway, the driver and every launcher got SIGINT at once. The driver exited through itsshutdown_signalhandler, and each launcher forwarded the signal to its VM worker. When the gateway'scleanup_on_shutdownsweep then asked the driver to stop the persisted sandboxes, the driver was gone.Changes
crates/openshell-gateway/src/vm.rs: move the base driver command (kill_on_drop, stdio) intocompute_driver_commandand addprocess_group(0). Tokio'sCommand::process_groupis available in the workspace's tokio (1.53).vm_driver_command_runs_driver_in_its_own_process_group: spawns a stand-in process throughcompute_driver_commandand checks that it leads its own process group, which differs from the test process's group.Behaviour after the change:
ManagedDriverProcess::shutdownsends SIGTERM to the driver (SIGKILL after 5 seconds).PR_SET_PDEATHSIG(SIGKILL)on Linux and a kqueue parent-exit watcher on macOS at startup (procguard::die_with_parent), and the launchers arm their own. These track the parent process, not the process group, so the driver and its launchers still exit when the gateway dies.kill_on_dropis kept for the drop path.openshell-server/src/credentials.rsis short-lived and not part of shutdown, so it is left alone.This PR does not change the shutdown sweep itself. Deleting rather than stopping ephemeral sandboxes during shutdown, and treating
ProcessExitedduring shutdown as recoverable, can follow separately if maintainers want it.Release Notes
Pressing Ctrl+C on a gateway that runs the VM driver in a terminal now stops its sandboxes cleanly. Before, the driver exited first and sandboxes were left in
Error.Testing
cargo fmt -p openshell-gateway --check;cargo clippy -p openshell-gateway --features vendored-z3 --all-targets -- -D warnings(Linux x86_64, Rust 1.95.0).cargo test -p openshell-gateway --features vendored-z3passes: 36 library tests, including the new one, plus 11 binary tests. The new test fails whenprocess_group(0)is removed (assertion left == right failed: the driver must lead its own process group).e2e/rust/tests/ephemeral_cleanup.rscase that sends SIGINT to the gateway's process group can follow if wanted.Before/after (live)
Tested on RHEL 9.8 x86_64 with nested KVM, with the v0.1.6
openshellCLI andopenshell-driver-vm. Only the gateway binary changed. Each run used a fresh state dir and a real Ctrl+C in a tmux pane on a foreground gateway, withopenshell sandbox create --no-keep -- sleep 600attached in a second pane.--internal-run-vmlauncher, VM and supervisor all in the gateway's group, which is the terminal's foreground groupShutdown signal received; stopping vm compute driver, thenFailed to stop sandbox during gateway shutdown ... service is currently unavailable, thenstopped=0 failed=1andgateway shutdown cleanup failedStopped sandbox during gateway shutdown ... phase=Ready, thenstopped=1 failed=0; the driver is stopped afterwards; no openshell processes leftsandbox listafter restartError, stillError30 s later; only a manual delete removes itrestored inactive sandbox,started=1 failed=0, Provisioning then Ready; neverErrorstopped=1 failed=0stopped=1 failed=0, gateway gone in 2 s, no driver, VM or supervisor leftWhat this PR does not change: the attached CLI still prints
ephemeral cleanup failed: connect error: transport error, because the gateway has already exited when the CLI sends its ephemeral delete. The--no-keepsandbox is stopped and comes back after the restart instead of being deleted. Removing it during shutdown is the second step in #4770 (the sweep deletes, rather than stops, sandboxes whose create session is ephemeral). It needs a maintainer decision, so it is left to a follow-up.Checklist
🤖 Generated with Claude Code