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 5 commits into
Conversation
|
Follow-up filed: #2372 — the SSH server task is spawned fire-and-forget ( |
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.
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 |
|
Waiting to test and recheck this one until 2369 is approved and merges so I can rebase then go through checks. |
…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>
…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>
…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>
6a1fe36 to
00dc53e
Compare
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This remains a project-valid, concentrated fix for the stale-Ready proxy failure in #2337.
Head SHA: 00dc53e89455857e75b8dcceb2f91cd788ad228b
Base SHA: 0a3ec7a1126aad908ffb742435db25bea1c19c25
Merge base SHA: 0a3ec7a1126aad908ffb742435db25bea1c19c25
Patch ID: 287b2741b08a8605dfb9f7325b7d7e284227be8a
Gator payload: 2
Review mode: follow_up
Previous reviewed SHA: 4e915eddb6d4c5111e95d44fff5c1b3e539f20b6
Thanks @politerealism, I checked your rebase note now that #2369 has merged. The merge conflict is resolved, and the 5-second backoff cap finding (gator-inline-3618433659) is fixed. The latest author delta also replaces parts of #2369's finalized classifier, which introduces the new inline blocker below.
Blocking findings:
GATOR-00dc53e8-01: documented transient accept errors now consume the unknown-error budget and terminate the proxy on the tenth occurrence.
Carried findings:
gator-inline-3618433657: still open; numeric errno literals remain instead of target-awarelibcconstants.gator-inline-3618433663: still open; a secondtake_exit_receiver()call still manufactures an immediate false exit signal.gator-inline-3618433665: still open; tests still bypass the production proxy task and sandbox lifecycle selection.
Non-blocking suggestions: None.
Docs: Fern docs are not needed because this restores an existing readiness invariant without changing commands, configuration, APIs, or intentional user workflows.
Next state: gator:in-review
Please preserve #2369's portable transient-error classifier while layering the proxy-exit notification on its terminal outcome, and address the carried obligations above. E2E remains required after review feedback is resolved.
| message: String, | ||
| backoff: Option<std::time::Duration>, | ||
| *consecutive_unknown_errors = consecutive_unknown_errors.saturating_add(1); | ||
| if *consecutive_unknown_errors >= MAX_CONSECUTIVE_UNKNOWN_ACCEPT_ERRORS { |
There was a problem hiding this comment.
gator-agent
Warning — GATOR-00dc53e8-01
Invariant: Kernel-documented transient accept errors must remain retryable and must not consume the bounded unknown-error budget that terminates the required proxy.
Prerequisite: The host experiences ten consecutive transient accept failures, such as ENOBUFS, ENOMEM, ENOSR, ECONNABORTED, or ENETDOWN.
Entry point → sink: A network-enabled sandbox's listener.accept() loop → this fallback increments consecutive_unknown_errors, returns Terminal on the tenth error, drops the exit guard, and makes run_sandbox fail.
Base → head: Current main classifies the documented transient resource/network errno set with target-aware libc constants and retries it indefinitely; this head recognizes only EMFILE/ENFILE, so every other known transient error reaches this terminal threshold.
Impact: Temporary resource or network pressure that main survives now terminates an otherwise recoverable sandbox and disrupts its workload.
Reproducer: Call classify_accept_error(Error::from_raw_os_error(libc::ENOBUFS), &mut fd, &mut unknown) ten times. This head returns Terminal on call 10, while the base path returns retry every time; repeat over the documented transient errno table.
PR ownership: Commit 00dc53e8 deletes the portable transient classification from the rebased #2369 base and routes those errors through this new terminal budget.
Requested change: Keep main's target-aware transient/terminal/unknown classifier and backoff helpers, layer this PR's exit notification only on the existing terminal outcome, and retain table-driven coverage for the known transient errno set.
Replace raw errno numbers with libc:: constants behind #[cfg(unix)] gates in classify_accept_error and its tests. Refactor ProxyHandle::exited_rx to Option<Receiver<()>> so take_exit_receiver uses .take() instead of allocating a dummy sender. Add a ProxyHandle-level test that verifies the full drop → abort → receiver fires path. Signed-off-by: Quinn Burdine <sburdine@redhat.com> Signed-off-by: politerealism <burdcat17@gmail.com>
The refactored classifier only recognized EMFILE/ENFILE as retryable, routing all other documented transient accept errors (ENOBUFS, ENOMEM, ECONNABORTED, ECONNRESET, EINTR, ENETDOWN, etc.) through the unknown- error budget which terminates the proxy after 10 consecutive failures. Restore the complete transient errno table from main so these errors retry indefinitely with appropriate backoff. Signed-off-by: Quinn Burdine <sburdine@redhat.com> Signed-off-by: politerealism <burdcat17@gmail.com>
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This remains a project-valid, concentrated fix for the stale-Ready proxy failure in #2337.
Head SHA: fe8298791181db1dd5f606e4914de4f9bd406e85
Base SHA: 0a3ec7a1126aad908ffb742435db25bea1c19c25
Merge base SHA: 0a3ec7a1126aad908ffb742435db25bea1c19c25
Patch ID: 8ebd81958c1bf8492797c1af6c62754d1ebd732d
Gator payload: 2
Review mode: follow_up
Previous reviewed SHA: 00dc53e89455857e75b8dcceb2f91cd788ad228b
Thanks @politerealism, I checked your latest commit restoring the complete transient errno classifier. GATOR-00dc53e8-01 is resolved: the supported transient family now bypasses the unknown-error budget, resource-pressure errors retain capped backoff, and the added coverage exercises repeated transient classification. The Option-based receiver change also resolves the double-take contract issue.
Blocking findings:
- No blocking findings remain.
Carried findings:
- None.
Docs: Fern docs are not needed because this restores an existing readiness invariant without changing commands, configuration, APIs, or intentional user workflows.
Next state: gator:watch-pipeline
This sandbox lifecycle and network-proxy change requires test:e2e; gator will enable that gate and monitor the required checks.
|
Label |
|
/ok to test fe82987 |
|
Label |
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)