fix(proxy): retry with backoff on transient accept errors instead of exiting by politerealism · Pull Request #2369 · NVIDIA/OpenShell

politerealism · 2026-07-20T15:45:06Z

Summary

Replace the unconditional break in the proxy accept loop with a sleep-and-continue pattern so transient errors (EMFILE, ENFILE, etc.) no longer permanently kill the proxy
EMFILE/ENFILE errors get exponential backoff (100ms → 5s) to let file descriptors drain; all other accept errors get a fixed 100ms backoff matching existing metadata_server and edge_tunnel patterns
OCSF severity raised from Low to Medium for FD exhaustion errors; log messages now include the retry delay

Related Issue

Closes #2337

Changes

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

Replaced break on accept error with tokio::time::sleep(backoff).await
Added FD exhaustion detection via raw_os_error() (EMFILE=24, ENFILE=23)
Added exponential backoff counter that resets on successful accept
Updated OCSF event message to include retry delay

Testing

cargo check -p openshell-supervisor-network — compiles clean
cargo test -p openshell-supervisor-network — all tests pass
Manual verification with the reproduction steps from bug(proxy): EMFILE stops the sandbox proxy while phase remains Ready #2337 (lower RLIMIT_NOFILE, trigger concurrent connections, verify the proxy recovers)

Checklist

Conventional commit format
Signed-off for DCO compliance
No secrets or credentials included
Scoped to the issue at hand

Note: This is step 1 of the fix for #2337. Step 2 is PR #2370 (terminate sandbox when the proxy dies unexpectedly).

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

copy-pr-bot · 2026-07-20T15:45:09Z

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.

maxamillion · 2026-07-20T15:54:36Z

/ok to test 8d0099f

- Use libc::EMFILE / libc::ENFILE instead of raw errno values; promote libc from dev-dependency to regular dependency - Extract accept_backoff() and is_fd_exhaustion_error() into testable helpers - Fix unreachable 5s cap: bump exponent limit from min(6) to min(7) so the 5_000ms ceiling is reachable (100 * 2^6 = 6400, capped to 5000) - Add 6 unit tests covering exponential progression, counter reset, saturation at cap, EMFILE/ENFILE detection, and non-FD error rejection Signed-off-by: Sean Burdine <sburdine@nvidia.com> Signed-off-by: politerealism <burdcat17@gmail.com>

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

Follow-up filed: #2372 — the SSH server accept loop (ssh.rs:141) has the same terminate-on-transient-error vulnerability this PR fixes in the proxy. Since the proxy and SSH server share the supervisor's FD table, the same FD pressure can kill the SSH accept loop even while the proxy recovers via backoff. The fix pattern from this PR (retry with backoff) should be applied there as well.

johntmyers

gator-agent

PR Review Status

Validation: This is a concentrated network-supervisor fix for the reproducible, linked bug in #2337.
Head SHA: 629fd7d4cef2348aea55394598c47d2600df1ccc

Review findings:

Warning — missing regression coverage: The new unit tests cover errno classification and backoff arithmetic, but they never execute the accept loop or prove that it accepts a new connection after EMFILE/ENFILE pressure clears. Please add an isolated Unix subprocess/integration test that lowers RLIMIT_NOFILE, induces the accept failure, releases a descriptor, and verifies a subsequent proxied connection succeeds. Subprocess isolation avoids changing the test runner's process-wide limit.
See the inline warning about distinguishing terminal listener errors from retryable failures.

Thanks @politerealism, I also checked the SSH accept-loop gap you called out in #2372. That is appropriately tracked as separate follow-up work and does not expand or block this focused proxy fix.

Docs: Fern docs are not needed because this changes internal recovery and operational logging, not a user-facing command, configuration, API, or workflow.

Next state: gator:in-review while these findings await an author update. The sandbox-network scope will require test:e2e after review feedback is resolved.

Three-way error classification for the accept loop: transient errors (EMFILE, ECONNABORTED) retry with backoff, terminal errors (EBADF, EINVAL, ENOTSOCK) exit immediately, and unknown errors exit after 5 consecutive failures. Adds unit tests for the classifier and error handler, plus a subprocess-isolated integration test that validates EMFILE recovery by lowering RLIMIT_NOFILE. 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 concentrated network-supervisor fix for the reproducible, linked bug in #2337.
Head SHA: abfe555af0e45201a1d52e57643622aae401ceb8

Review findings:

Resolved: the new classification exits immediately for EBADF, EINVAL, and ENOTSOCK, bounds unknown errors to five retries, and resets both counters after a successful accept.
Warning — production retry coverage remains incomplete: see the inline finding on the new FD-exhaustion test.

Thanks @politerealism, I checked the error-classification and subprocess-test changes in your new commit. The terminal-listener concern is resolved, but the requested production-path regression is not yet covered because the test uses a separate blocking listener and can pass without observing EMFILE.

Docs: Fern docs are not needed because this changes internal recovery and operational logging, not a user-facing command, configuration, API, or workflow.

Next state: gator:in-review while the remaining regression-test finding awaits an author update. The sandbox-network scope will require test:e2e after review feedback is resolved.

johntmyers · 2026-07-23T19:17:49Z

Warning — production retry coverage remains incomplete: see the inline finding on the new FD-exhaustion test.

We can ignore this and accept the new unit and integration tests. cc @gator-agent

johntmyers · 2026-07-23T19:18:26Z

/ok to test abfe555

github-actions · 2026-07-23T19:27:34Z

Label test:e2e applied for abfe555. 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.

Signed-off-by: Quinn Burdine <sburdine@redhat.com> Signed-off-by: politerealism <burdcat17@gmail.com>

johntmyers · 2026-07-23T21:53:48Z

/ok to test 39705af

johntmyers

gator-agent

PR Review Status

Validation: This remains a concentrated network-supervisor fix for the reproducible, linked bug in #2337.
Head SHA: 39705af9f2d2b7732c13a0b606be0898082badba

Review findings:

Warning: the Linux transient accept-error allowlist is incomplete, so repeated retryable errors can still terminate the unobserved proxy task.
Warning: the FD-exhaustion test can accept the earlier queued connection after dropping it, rather than the fresh connection it later validates.

Thanks @politerealism, I checked your explanation that the unit and integration tests divide the retry coverage. Thanks @johntmyers; I treated your maintainer waiver as resolving the earlier request to exercise the exact production async wiring. The two inline findings below are distinct issues found in the current cumulative diff.

Docs: Fern docs are not needed because this changes internal recovery and operational logging, not a user-facing command, configuration, API, or workflow.

E2E: test:e2e is already applied, and /ok to test was posted for this exact head SHA.

Next state: gator:in-review while these findings await an author update.

Widen the accept-error classification to cover all Linux accept(2) transient errnos (ENETDOWN, EPROTO, ENOPROTOOPT, EHOSTDOWN, EHOSTUNREACH, EOPNOTSUPP, ENETUNREACH, ENONET behind cfg gate). Rename fd-exhaustion helpers to resource-pressure to reflect ENOBUFS and ENOMEM coverage. Fix clippy lints in the integration test (borrow_as_ptr, collection_is_never_read, while_let_loop) and make post-EMFILE recovery verification cross-platform by handling Linux's stale backlog behavior. 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 concentrated network-supervisor resilience fix for the reproducible, linked bug in #2337.
Head SHA: ddae69bebea51b6dbd0f28afe8d3134effe388ac

Thanks @politerealism. I checked the current head against the two findings you acknowledged: the documented Linux pending-network errors and ENOBUFS/ENOMEM now retry with the intended backoff, and the FD-exhaustion test now handles the stale queued connection before validating the fresh client.

Review findings:

Warning: one additional Linux-only accept-error group can still exhaust the bounded unknown retry budget and terminate the proxy listener.
Suggestion: the FD-exhaustion test can still return successfully without observing EMFILE.

Docs: Fern docs are not needed because this is internal recovery and telemetry behavior with no new command, configuration, or user workflow.

Security: retrying does not bypass OPA, identity, L7, or default-deny enforcement.

Next state: gator:in-review pending the warning below. The test suggestion is non-blocking.

johntmyers · 2026-07-24T22:15:01Z

/ok to test ddae69b

…est skip explicit Classify ENOSR, ESOCKTNOSUPPORT, EPROTONOSUPPORT, and ETIMEDOUT as transient accept errors per Linux accept(2) documentation. Treat ENOSR as resource pressure (exponential backoff). Make the EMFILE integration test skip path emit a diagnostic instead of silently returning success. 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 concentrated network-supervisor resilience fix for the reproducible, linked bug in #2337.
Head SHA: e8b4aabe434d2e7736415fa1845a63ded3bd8f07

Thanks @politerealism. I checked the current head against the warning and suggestion from the previous review. The remaining Linux accept(2) errnos are now classified as transient, and the stale queued connection is handled safely. The attempted explicit EMFILE skip diagnostic is still swallowed by the parent process, however; see the inline warning.

Review findings:

Warning: CI can still report the FD-exhaustion test as passing without observing EMFILE or displaying the intended skip diagnostic.

Docs: Fern docs are not needed because this changes internal resilience and operational logging, not a user-facing command, configuration field, API, or workflow.

Security: the change preserves OPA, identity, L7, and default-deny enforcement.

E2E: test:e2e remains applied. The current head should enter pipeline watch after the review warning is resolved.

Next state: gator:in-review while the warning awaits an author update.

The integration test child runs inside Command::output() which captures stderr. A silent return on the Ok path meant CI would report a passing test without any diagnostic. Panic instead so the parent test fails visibly if the platform cannot induce EMFILE. 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 concentrated network-supervisor resilience fix for the reproducible, linked bug in #2337.
Head SHA: f13c4544607e226ae0f3ebd8b2497e4f510109f6

Thanks @politerealism. I checked the current head against the previous warning: the subprocess now fails visibly when it does not induce EMFILE, so CI can no longer pass without exercising that precondition. I also reviewed the cumulative retry classification, backoff state, recovery test, and its virtual merge with newer main proxy plumbing.

Review findings:

No blocking or actionable findings remain.

Docs: Fern docs are not needed because this changes internal resilience and operational telemetry, not a user-facing command, configuration field, API, or workflow.

Security: the network-policy trust boundary is unchanged; accepted connections still enter the existing policy-enforced handler.

E2E: test:e2e is applied. The current head requires /ok to test so the mirror-backed required checks can run.

Next state: gator:watch-pipeline.

johntmyers · 2026-07-27T17:24:49Z

/ok to test f13c454

johntmyers · 2026-07-27T18:29:19Z

gator-agent

Monitoring Complete

Monitoring is complete because this PR has merged.

Final status: Gator review found no remaining actionable issues on head f13c4544607e226ae0f3ebd8b2497e4f510109f6; test:e2e completed successfully, required checks were green, and maintainer approval was present before merge.

I removed the active gator:* label because there is nothing left for gator to monitor on this PR.

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

politerealism mentioned this pull request Jul 20, 2026

fix(sandbox): terminate sandbox when proxy accept loop exits unexpectedly #2370

Open

5 tasks

politerealism mentioned this pull request Jul 20, 2026

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

Open

johntmyers self-assigned this Jul 21, 2026

johntmyers added the gator:in-review Gator is reviewing or awaiting PR review feedback label Jul 21, 2026

johntmyers reviewed Jul 21, 2026

View reviewed changes

Comment thread crates/openshell-supervisor-network/src/proxy.rs Outdated

politerealism requested a review from johntmyers July 21, 2026 15:31

johntmyers reviewed Jul 23, 2026

View reviewed changes

Comment thread crates/openshell-supervisor-network/tests/accept_fd_exhaustion.rs

johntmyers added the test:e2e Requires end-to-end coverage label Jul 23, 2026

fix(proxy): use nested or-patterns to satisfy clippy lint

39705af

Signed-off-by: Quinn Burdine <sburdine@redhat.com> Signed-off-by: politerealism <burdcat17@gmail.com>

johntmyers reviewed Jul 23, 2026

View reviewed changes

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

Comment thread crates/openshell-supervisor-network/tests/accept_fd_exhaustion.rs

johntmyers reviewed Jul 24, 2026

View reviewed changes

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

Comment thread crates/openshell-supervisor-network/tests/accept_fd_exhaustion.rs

johntmyers previously approved these changes Jul 27, 2026

View reviewed changes

politerealism dismissed johntmyers’s stale review via e8b4aab July 27, 2026 14:50

johntmyers reviewed Jul 27, 2026

View reviewed changes

Comment thread crates/openshell-supervisor-network/tests/accept_fd_exhaustion.rs Outdated

johntmyers reviewed Jul 27, 2026

View reviewed changes

johntmyers approved these changes Jul 27, 2026

View reviewed changes

johntmyers added this pull request to the merge queue Jul 27, 2026

Merged via the queue into NVIDIA:main with commit 79bcf29 Jul 27, 2026
52 of 54 checks passed

johntmyers removed the gator:approval-needed Gator completed review; maintainer approval needed label Jul 27, 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!

maxamillion 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!

Uh oh!

johntmyers left a comment

Choose a reason for hiding this comment

PR Review Status

Uh oh!

Uh oh!

johntmyers commented Jul 23, 2026

Uh oh!

johntmyers commented Jul 23, 2026

Uh oh!

github-actions Bot commented Jul 23, 2026

Uh oh!

johntmyers commented Jul 23, 2026

Uh oh!

johntmyers left a comment

Choose a reason for hiding this comment

PR Review Status

Uh oh!

Uh oh!

Uh oh!

johntmyers left a comment

Choose a reason for hiding this comment

PR Review Status

Uh oh!

Uh oh!

Uh oh!

johntmyers commented Jul 24, 2026

Uh oh!

johntmyers left a comment

Choose a reason for hiding this comment

PR Review Status

Uh oh!

Uh oh!

johntmyers left a comment

Choose a reason for hiding this comment

PR Review Status

Uh oh!

johntmyers commented Jul 27, 2026

Uh oh!

Uh oh!

johntmyers commented Jul 27, 2026

Monitoring Complete

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

politerealism commented Jul 20, 2026 •

edited

Loading