Repository navigation
fix(windows): end sandbox processes when the gateway is killed - #5299
ashwind-nvidia wants to merge 2 commits into
Conversation
When the MXC driver runs inside the gateway, a hard-killed gateway left its sandbox processes (wxc-exec, openshell-supervisor and openshell-windows-sandbox) running with no owner. They also kept the gateway's listening socket open, so the port stayed bound and a new gateway failed to start with "Only one usage of each socket address" (os error 10048). There were two causes, so there are two changes: - The gateway now puts itself in a kill-on-close job object before it spawns anything. Every sandbox process joins the job, and Windows ends them all when the gateway process exits for any reason, including a hard kill. If the job cannot be created the gateway logs a warning and continues. - mio creates listening sockets with plain socket(), which leaves the handle inheritable, and child processes are started with handle inheritance enabled, so each sandbox process held a copy of the socket. The gateway, health and metrics listeners now clear the inherit flag right after bind. Either change releases the port on its own; the job object also removes the orphaned processes. Tests: unit tests check that the process is in a kill-on-close job and that a bound gateway listener is not inheritable and still accepts connections. On a native Windows 11 x64 host with three running sandboxes, hard-killing the gateway left 9 processes and an unbindable port in 5 of 5 runs before this change. With it, no process survived, the port could be bound and a new gateway started in 3 of 3 runs on the final build, and in 5 of 5 runs on an earlier build that differed only by two lint-driven edits. The existing MXC end-to-end scenarios give the same results with and without the change. Signed-off-by: ashwind <ashwind@nvidia.com>
|
All contributors have signed the DCO ✍️ ✅ |
|
I have read the DCO document and I hereby sign the DCO. |
shailendra-nv
left a comment
shailendra-nv
left a comment
There was a problem hiding this comment.
The cleanup addresses the orphaned processes in today's in-process MXC setup. I'd like us to keep it compatible with the Windows repository split and make the shutdown behavior clear before carrying it forward. I've left comments on job ownership, lifecycle validation, and the socket fix. This is a code and design review; I haven't run this PR head on ARM64. Separately, linked issue #5298 still lacks state:accepted, and the ready PR has no area label; the existing acceptance and readiness checks need attention.
| #[tokio::main] | ||
| async fn main() -> miette::Result<()> { | ||
| // Must run before anything is spawned so every sandbox process joins the job. | ||
| if let Err(error) = openshell_gateway::process_lifetime::kill_children_on_exit() { |
There was a problem hiding this comment.
Could we have the MXC driver own the jobs, ideally one per sandbox generation? This works while the driver runs inside the gateway, but the split in https://github.com/NVIDIA/OpenShell-Windows/issues/20 starts the external driver through an operator or launcher. That driver and its children would not automatically join this gateway-owned job, so the cleanup guarantee would disappear after extraction. Keeping containment with the driver would let us carry the fix across the split. If gateway loss must also stop an independently running driver’s workloads, we should define that behavior across the authenticated connection rather than rely on the process tree.
| }; | ||
|
|
||
| #[test] | ||
| fn puts_the_process_in_a_kill_on_close_job() { |
There was a problem hiding this comment.
Could we add a graceful gateway shutdown/restart test with persisted state, alongside the hard-kill coverage, and run the lifecycle checks on ARM64 too? This test confirms the job flags, but not what happens to a running workload or its sandbox record. Docker, Podman, and VM use a stop-and-recover lifecycle on graceful gateway restart; MXC still has no restart recovery. We do not need to add recovery in this PR, but the Windows docs and build guidance should make clear that gateway exit terminates workloads without providing the same recovery behavior.
| /// | ||
| /// A failure is logged and otherwise ignored: the listener still works, it just | ||
| /// keeps the old behaviour. This is a no-op on other platforms. | ||
| pub fn disable_handle_inheritance(listener: &TcpListener, name: &str) { |
There was a problem hiding this comment.
Could we first try a targeted Mio update from 1.2.0 to 1.2.1 for this part? Mio 1.2.1 creates Windows sockets with WSA_FLAG_NO_HANDLE_INHERIT: https://github.com/tokio-rs/mio/blob/v1.2.1/src/sys/windows/net.rs#L35. If the listener and port-reuse tests pass with that update, we could avoid carrying this Win32 wrapper and the added server dependency through the split. The non-inheritable listener tests would still be useful. This would address socket inheritance only; we would still need the separate workload cleanup fix.
There was a problem hiding this comment.
Done in 6d3d2ce. mio is updated 1.2.0 → 1.2.1 (lockfile: only the mio entry), and the Win32 wrapper, its call sites and the runtime windows dependency in openshell-server are removed. windows stays as a test-only dependency so the two listener tests can still read the inherit flag.
Verified on Windows: with 1.2.0 tokio's listener, accepted and outbound sockets are all inheritable; with 1.2.1 none are, so this covers more than the wrapper did. With the job object disabled in a temporary build, the nine sandbox processes survive but the port can be bound and a new gateway starts (2/2), so mio alone frees the port. As you said, the job-object fix is still needed.
Windows nextest (whole workspace) 6661/6661 pass; Linux 8062/8079, and the 17 failures pass when not run as root.
shailendra-nv
left a comment
shailendra-nv
left a comment
There was a problem hiding this comment.
Requesting changes based on the comments in my earlier review. Please address job ownership so cleanup stays compatible with the standalone MXC driver, and document and validate the gateway shutdown/restart behavior, including ARM64 coverage. The existing inline comments also describe the possible Mio update to simplify the socket fix.
Addresses review feedback on NVIDIA#5299. mio 1.2.1 creates Windows sockets with WSA_FLAG_NO_HANDLE_INHERIT (1.2.0 used plain socket(), which is inheritable), so a sandbox process the gateway spawns can no longer keep a copy of the gateway's listening socket and hold its port after a gateway crash. Update mio from 1.2.0 to 1.2.1 and remove the Win32 wrapper that cleared the inherit flag after bind, together with the windows dependency it added to openshell-server; windows is now a test-only dependency there. The listener tests stay, so a regression to an inheritable socket still fails the build. Cargo.lock changes only the mio entry (version and checksum); its dependency list is unchanged and tokio 1.52.1 requires mio ^1.2.0. Tests: on Windows x64, bound_gateway_listener_is_not_inheritable and listener_accepts_connections pass without the wrapper. With three sandboxes running, hard-killing the gateway with the job object disabled leaves the nine sandbox processes alive but the port can be bound and a new gateway starts (2 of 2 runs), so the mio update alone frees the port. With the job object enabled no process survives and the port can be bound (3 of 3 runs). cargo fmt, cargo check --locked and clippy with -D warnings pass for openshell-gateway and openshell-server on Linux. Refs NVIDIA#5298 Signed-off-by: ashwind <ashwind@nvidia.com>
Summary
With the MXC driver built into the gateway, hard-killing the gateway left its sandbox processes running with no owner, and they kept the gateway's sockets open so a new gateway could not bind the port. Two changes fix it: the gateway now puts itself in a kill-on-close job object so every sandbox process ends with it, and
miois updated to 1.2.1, which creates Windows sockets as non-inheritable so no child process can keep the gateway's port bound.Related Issue
Closes #5298
Changes
openshell_gateway::process_lifetime::kill_children_on_exit(), called first in the gateway'smain. It puts the process in a job object withJOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE; every sandbox process joins it and Windows ends them when the gateway exits for any reason. If the job cannot be created, a warning is printed and the gateway continues. It does nothing on other platforms.miofrom 1.2.0 to 1.2.1 inCargo.lock(only themioentry changes: version and checksum; its dependencies are unchanged and tokio 1.52.1 requiresmio ^1.2.0).mio1.2.1 creates Windows sockets withWSA_FLAG_NO_HANDLE_INHERIT(1.2.0 used plainsocket(), which leaves the handle inheritable and lets every spawned child keep a copy). On Windows I confirmed that with 1.2.0 tokio's listener, accepted and outbound connection sockets are all inheritable, and with 1.2.1 none are. This replaces the Win32 wrapper from the first commit, which only covered the three listeners (removed in the second commit, following review).openshell-serverkeep the behavior pinned: a bound gateway listener is not inheritable, and it still accepts connections. They fail ifmioever regresses to an inheritable socket. A third test inopenshell-gatewaychecks that the process is in a kill-on-close job.windowsis a Windows-only dependency ofopenshell-gateway(job object) and a test-only dependency ofopenshell-server.Behavior change: sandboxes now end when the gateway process exits, including a normal restart. Before, they were left running without an owner.
Not yet addressed (review follow-ups): moving job ownership into the MXC driver (ideally one job per sandbox) so the cleanup carries over the repository split, a graceful gateway shutdown/restart test with persisted state, Windows documentation of that behavior, and ARM64 validation.
Testing
gateway_listener::tests(2) andprocess_lifetime::tests(1) pass on Windows x64.cargo nextest run --profile ci --workspace): Windows x64, 6661 of 6661 pass (43 skipped); Linux, 8062 of 8079 pass. The 17 Linux failures are all inopenshell-sandboxboundary_server::linuxand come from running the container as root ("workload identity must not contain UID or GID zero"); run as a normal user, all 41 of those tests pass.wxc-exec,openshell-supervisor,openshell-windows-sandbox), the port could not be bound, and a new gateway failed with os error 10048, in 5 of 5 runs. After (this head): no process survived, the port could be bound and a new gateway started, in 3 of 3 runs.mioupdate on its own, using a temporary test build with the job object switched off (not committed): the nine sandbox processes survive, but the port can be bound and a new gateway starts, in 2 of 2 runs.cargo fmt --all -- --check, andcargo check --lockedpluscargo clippy --locked --all-targets -- -D warningsforopenshell-gatewayandopenshell-serveron Linux (Rust 1.95.0) pass. Windows clippy with the CI flags reports 322 diagnostics (319large future), the same as the untouched base, and none in the changed files.cargo deny checkgives identical results with and without this change (the existing advisories are unchanged).run-mxc-e2e.ps1) were run on the first commit: 6 of 7 pass with and without the change;host-servicefails in both. They were not rerun after themioupdate. The runner's port probe did not detect the listening gateway on this host, so a scratch copy with a loopback connect probe was used; the repository copy was not modified.Not tested: ARM64, a graceful gateway shutdown, and the repository's CI for this PR (it has not run yet).
Checklist