fix(sandbox): terminate sandbox when proxy accept loop exits unexpectedly by politerealism · Pull Request #2370 · NVIDIA/OpenShell

politerealism · 2026-07-20T16:10:24Z

Summary

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 AppLifecycle error at High severity if the proxy dies
Prevents the sandbox from continuing to report Ready with a dead proxy

Related Issue

Note: This is step 2 of the fix for #2337, building on PR #2369 (step 1: accept loop retry with backoff). Step 1 prevents the proxy from dying on transient errors; step 2 catches any unexpected death as defense-in-depth.

Changes

crates/openshell-supervisor-network/src/proxy.rs

Added exited_rx oneshot receiver field to ProxyHandle
Created exited_tx sender inside the spawned proxy task as _proxy_exit_guard — dropped on task exit for any reason
Added take_exit_receiver() method to extract the receiver for monitoring

crates/openshell-sandbox/src/lib.rs

Extract proxy exit receiver before the main wait section
Wrap it in a boxed future (std::future::pending() when no proxy exists)
Added proxy-death arm to all 5 tokio::select! wait paths:
- Process-enabled + sidecar control
- Process-enabled, no sidecar
- Network-only + sidecar control (Linux)
- Network-only, no sidecar (Linux)
- Non-Linux network-only

Testing

cargo check -p openshell-supervisor-network — compiles clean, no warnings
cargo check -p openshell-sandbox — compiles clean
cargo test -p openshell-supervisor-network -p openshell-sandbox — all tests pass

Checklist

Conventional commit format
Signed-off for DCO compliance
No secrets or credentials included
Scoped to the issue at hand
Follows existing sidecar control channel pattern (lib.rs:692-710)

Note: This PR depends on PR #2369 (step 1: accept loop retry with backoff) and should be merged after it.

copy-pr-bot · 2026-07-20T16:10:28Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

politerealism · 2026-07-20T16:37:12Z

Follow-up filed: #2372 — the SSH server task is spawned fire-and-forget (run.rs:238), so if it exits unexpectedly the sandbox has no way to detect it. The same exit-notification pattern from this PR (oneshot drop-guard + tokio::select! arm) should be applied to the SSH server task to enforce the "Ready implies exec relay operational" invariant.

politerealism · 2026-07-20T16:38:22Z

Rebase note: This PR was branched off #2369. Once #2369 merges, this branch needs a rebase to pick up the final version of the proxy backoff code (libc constants, extracted helpers, fixed 5s cap). No code changes required — just a rebase.

johntmyers

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.

johntmyers · 2026-07-21T00:13:36Z

+                        // back off exponentially to let connections drain.
+                        let is_fd_exhaustion = matches!(
+                            err.raw_os_error(),
+                            Some(24) | Some(23)


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.

johntmyers · 2026-07-21T00:13:36Z

+                            consecutive_fd_errors = consecutive_fd_errors.saturating_add(1);
+                            let backoff_ms = 100u64
+                                .saturating_mul(
+                                    1u64 << consecutive_fd_errors.min(6).saturating_sub(1),


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.

johntmyers · 2026-07-21T00:13:36Z

+    /// 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();


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.

johntmyers · 2026-07-21T00:13:36Z

+
+    #[tokio::test]
+    async fn test_exit_receiver_fires_when_task_exits() {
+        let (exited_tx, exited_rx) = tokio::sync::oneshot::channel::<()>();


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 · 2026-07-21T15:43:44Z

gator-agent

Blocked

Head SHA: 6a1fe36ae0357d7bc0d22b91323ff8ae296b54a2

Gator is blocked because GitHub reports this PR as having merge conflicts with main (mergeStateStatus: DIRTY).

Next action: @politerealism, please rebase the branch onto the current main, resolve the conflicts, and push the updated head. Gator will review the new head and decide the required E2E label and /ok to test action after the conflict is resolved.

politerealism · 2026-07-21T18:48:32Z

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>

johntmyers

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-aware libc constants.
gator-inline-3618433663: still open; a second take_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.

johntmyers · 2026-08-03T18:04:01Z

-    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 {


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

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.

github-actions · 2026-08-03T18:30:33Z

Label test:e2e applied, but pull-request/2370 does not exist yet. A maintainer needs to comment /ok to test fe8298791181db1dd5f606e4914de4f9bd406e85 to mirror this PR. Once the mirror exists, re-apply the label or re-run Branch E2E Checks from the Actions tab.

johntmyers · 2026-08-03T18:30:45Z

/ok to test fe82987

github-actions · 2026-08-03T18:31:31Z

Label test:e2e applied for fe82987. Open the existing run and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

politerealism requested review from a team, derekwaynecarr, maxamillion and mrunalp as code owners July 20, 2026 16:10

This was referenced Jul 20, 2026

fix(proxy): retry with backoff on transient accept errors instead of exiting #2369

Merged

bug: SSH server accept loop exits permanently on transient errors (same class as #2337) #2372

Open

johntmyers self-assigned this Jul 21, 2026

johntmyers reviewed Jul 21, 2026

View reviewed changes

johntmyers added gator:in-review Gator is reviewing or awaiting PR review feedback gator:blocked Gator is blocked by process or repository gates and removed gator:in-review Gator is reviewing or awaiting PR review feedback labels Jul 21, 2026

politerealism added 3 commits August 3, 2026 13:37

politerealism force-pushed the fix/proxy-exit-detection branch from 6a1fe36 to 00dc53e Compare August 3, 2026 17:50

johntmyers reviewed Aug 3, 2026

View reviewed changes

johntmyers added gator:in-review Gator is reviewing or awaiting PR review feedback and removed gator:blocked Gator is blocked by process or repository gates labels Aug 3, 2026

politerealism requested a review from johntmyers August 3, 2026 18:08

johntmyers reviewed Aug 3, 2026

View reviewed changes

johntmyers added the test:e2e Requires end-to-end coverage label Aug 3, 2026

johntmyers added gator:watch-pipeline Gator is monitoring PR CI/CD status test:e2e Requires end-to-end coverage and removed gator:in-review Gator is reviewing or awaiting PR review feedback test:e2e Requires end-to-end coverage labels Aug 3, 2026

johntmyers added gator:in-review Gator is reviewing or awaiting PR review feedback and removed gator:watch-pipeline Gator is monitoring PR CI/CD status labels Aug 3, 2026

Uh oh!

Conversation

politerealism commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Related Issue

Changes

Testing

Checklist

Uh oh!

copy-pr-bot Bot commented Jul 20, 2026

Uh oh!

politerealism commented Jul 20, 2026

Uh oh!

politerealism commented Jul 20, 2026

Uh oh!

johntmyers left a comment

Choose a reason for hiding this comment

PR Review Status

Uh oh!

johntmyers Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

johntmyers Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

johntmyers Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

johntmyers Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

johntmyers commented Jul 21, 2026

Blocked

Uh oh!

politerealism commented Jul 21, 2026

Uh oh!

johntmyers left a comment

Choose a reason for hiding this comment

PR Review Status

Uh oh!

johntmyers Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

johntmyers left a comment

Choose a reason for hiding this comment

PR Review Status

Uh oh!

github-actions Bot commented Aug 3, 2026

Uh oh!

johntmyers commented Aug 3, 2026

Uh oh!

github-actions Bot commented Aug 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

politerealism commented Jul 20, 2026 •

edited

Loading