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.

johntmyers · 2026-07-23T22:03:59Z

+            AcceptErrorClass::Transient
+        }
+        Some(libc::EBADF | libc::EINVAL | libc::ENOTSOCK) => AcceptErrorClass::Terminal,
+        _ => AcceptErrorClass::Unknown,


gator-agent

Warning: Tokio consumes WouldBlock, but other OS errors reach this classifier. Linux documents ENETDOWN, EPROTO, ENOPROTOOPT, EHOSTDOWN, ENONET, EHOSTUNREACH, EOPNOTSUPP, and ENETUNREACH as accept errors reliable servers should retry like EAGAIN; ENOBUFS and ENOMEM also indicate resource pressure. These currently become Unknown and terminate the unobserved proxy task after six failures, recreating the stale-Ready failure mode (CWE-755). Add Linux-specific transient cases, use backoff for ENOBUFS/ENOMEM, and add classification tests. See Linux accept(2).

johntmyers · 2026-07-23T22:03:59Z

+
+    // Release FDs.
+    held_fds.clear();
+    drop(backlog_conn);


gator-agent

Warning: On Linux, EMFILE is returned while reserving the output FD before the pending connection is dequeued. backlog_conn can therefore remain first in the accept queue; dropping it and then accepting once may return that closed connection instead of client, making the final read fail or hang. Keep backlog_conn alive and use it to verify recovery, or accept/drain it before creating and validating the fresh connection.

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

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!

johntmyers Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

johntmyers Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

politerealism commented Jul 20, 2026 •

edited

Loading