fix(sandbox): terminate sandbox when proxy accept loop exits unexpectedly#2370
fix(sandbox): terminate sandbox when proxy accept loop exits unexpectedly#2370politerealism wants to merge 4 commits into
Conversation
…exiting The proxy accept loop unconditionally broke on any accept() error, permanently killing the proxy while the sandbox continued to report Ready. Replace the break with a sleep-and-continue pattern: EMFILE/ENFILE errors get exponential backoff (100ms to 3.2s) to let file descriptors drain, and all other accept errors get a fixed 100ms backoff matching the existing metadata_server and edge_tunnel patterns. Closes NVIDIA#2337 Signed-off-by: Sean Burdine <sburdine@nvidia.com> Signed-off-by: politerealism <burdcat17@gmail.com>
…edly Add a oneshot notification channel to ProxyHandle that fires when the proxy task exits for any reason (panic, abort, or loop break). The sandbox main loop now races this signal alongside the entrypoint process and shutdown signals, terminating with a clear OCSF error if the proxy dies. This prevents the sandbox from continuing to report Ready with a dead proxy. Follows the existing sidecar control channel pattern. All five wait paths (process+sidecar, process-only, network+sidecar, network-only on Linux, and non-Linux) now monitor proxy liveness. Refs NVIDIA#2337 Signed-off-by: Sean Burdine <sburdine@nvidia.com> Signed-off-by: politerealism <burdcat17@gmail.com>
|
Follow-up filed: #2372 — the SSH server task is spawned fire-and-forget ( |
…it_receiver Verify the oneshot drop-guard pattern fires on normal task exit and abort, and document the take_exit_receiver contract including the double-call hazard. Signed-off-by: politerealism <burdcat17@gmail.com>
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This is a concentrated sandbox/network-supervisor correctness fix for the reproducible stale-Ready failure in #2337, and no duplicate implementation was found.
Head SHA: 4e915eddb6d4c5111e95d44fff5c1b3e539f20b6
Thanks @politerealism, I checked your rebase note against PR #2369. Its current head differs from the retry commit stacked here; rebasing after #2369 lands should pick up the portable errno helper and corrected 5-second cap covered by the first two inline findings.
Review findings:
- Four actionable warnings are attached inline.
- The fail-closed proxy-exit signal is wired into every sandbox wait path and does not weaken the sandbox security boundary.
Docs: Fern docs are not needed because this restores the documented readiness invariant without changing commands, configuration, APIs, or an intentional user workflow.
Next state: gator:in-review
Please rebase after #2369 lands and address the exit-receiver contract and production-path test coverage. E2E will be required after review feedback is resolved.
| // back off exponentially to let connections drain. | ||
| let is_fd_exhaustion = matches!( | ||
| err.raw_os_error(), | ||
| Some(24) | Some(23) |
There was a problem hiding this comment.
gator-agent
Warning: Replace numeric errno values with libc::EMFILE and libc::ENFILE through a platform-aware helper. raw_os_error() values are platform-specific, so non-Linux targets may miss FD exhaustion and use the wrong backoff and severity. Rebasing onto the final #2369 helper should address this.
| consecutive_fd_errors = consecutive_fd_errors.saturating_add(1); | ||
| let backoff_ms = 100u64 | ||
| .saturating_mul( | ||
| 1u64 << consecutive_fd_errors.min(6).saturating_sub(1), |
There was a problem hiding this comment.
gator-agent
Warning: consecutive_fd_errors.min(6) limits the exponent to 5, so this tops out at 3,200 ms and the 5,000 ms cap is unreachable. Allow the next exponent before applying the cap and add a unit test covering the complete backoff sequence.
| /// Take the exit notification receiver for health monitoring. | ||
| /// Resolves when the proxy accept loop task exits for any reason. | ||
| pub fn take_exit_receiver(&mut self) -> tokio::sync::oneshot::Receiver<()> { | ||
| let (_tx, dummy_rx) = tokio::sync::oneshot::channel(); |
There was a problem hiding this comment.
gator-agent
Warning: Store exited_rx as an Option and make take_exit_receiver() return Option or Result. The dropped dummy sender makes a second call indistinguishable from a real proxy exit, which can falsely terminate a healthy sandbox; the added test currently codifies that footgun.
|
|
||
| #[tokio::test] | ||
| async fn test_exit_receiver_fires_when_task_exits() { | ||
| let (exited_tx, exited_rx) = tokio::sync::oneshot::channel::<()>(); |
There was a problem hiding this comment.
gator-agent
Warning: These tests recreate standalone Tokio channels, so they still pass if the production exit guard or run_sandbox selection is removed. Exercise an actual ProxyHandle task exit and add a focused sandbox-level test proving proxy notification returns the expected error and cancels the workload.
johntmyers
added
gator:in-review
…unrecoverable failures Extract accept-loop error handling into classify_accept_error() with AcceptAction enum so the decision logic is independently testable. Terminal errors (EBADF/EINVAL/ENOTSOCK) now exit the loop immediately, allowing the supervisor to detect proxy death. Unknown errors exit after 10 consecutive failures. FD exhaustion retries with exponential backoff capped at 5s (fix unreachable cap from min(6) to min(7)). Adds 9 unit tests covering classification, counter interactions, backoff progression, and bounded exit behavior. Signed-off-by: Quinn Burdine <sburdine@redhat.com>
BlockedHead SHA: Gator is blocked because GitHub reports this PR as having merge conflicts with Next action: @politerealism, please rebase the branch onto the current |
2 participants
Summary
ProxyHandlethat fires when the proxy task exits for any reason (panic, abort, or loop break)AppLifecycleerror atHighseverity if the proxy diesReadywith a dead proxyRelated Issue
Refs #2337
Changes
crates/openshell-supervisor-network/src/proxy.rsexited_rxoneshot receiver field toProxyHandleexited_txsender inside the spawned proxy task as_proxy_exit_guard— dropped on task exit for any reasontake_exit_receiver()method to extract the receiver for monitoringcrates/openshell-sandbox/src/lib.rsstd::future::pending()when no proxy exists)tokio::select!wait paths:Testing
cargo check -p openshell-supervisor-network— compiles clean, no warningscargo check -p openshell-sandbox— compiles cleancargo test -p openshell-supervisor-network -p openshell-sandbox— all tests passChecklist
lib.rs:692-710)