fix(proxy): retry with backoff on transient accept errors instead of exiting#2369
fix(proxy): retry with backoff on transient accept errors instead of exiting#2369politerealism wants to merge 3 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>
|
/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>
|
Follow-up filed: #2372 — the SSH server accept loop ( |
johntmyers
added
the
gator:in-review
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 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.
| let (severity, backoff) = if is_fd_exhaustion { | ||
| consecutive_fd_errors = consecutive_fd_errors.saturating_add(1); | ||
| (SeverityId::Medium, accept_backoff(consecutive_fd_errors)) | ||
| } else { |
There was a problem hiding this comment.
gator-agent
Warning: Retrying every non-FD error indefinitely also catches terminal listener failures such as EBADF, EINVAL, and ENOTSOCK. This leaves ProxyHandle.join alive while the proxy is permanently unusable, preventing supervisor monitoring such as #2370 from observing task termination (CWE-755). Retry known transient errors; emit the failure and exit for terminal errors, or place a bounded retry limit on unknown errors.
There was a problem hiding this comment.
@johntmyers the fixes that I did shoudl resolve your feedback. Would you mind taking a look at it?
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
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
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, andENOTSOCK, 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.
| //! | ||
| //! Uses blocking I/O in the child to surface EMFILE reliably across | ||
| //! platforms (tokio's async accept may swallow EMFILE on kqueue-based | ||
| //! systems). The proxy's retry logic is validated by the unit tests for |
There was a problem hiding this comment.
gator-agent
Warning: This test never executes the production listener.accept().await → handle_accept_error → backoff path; it only proves that a separate blocking listener can accept after descriptors are released. A regression that removes or breaks the production retry wiring would still pass, and Ok(_) => return at line 79 can make the test succeed without observing EMFILE. Please exercise the actual async production retry sequence in the isolated child (gate it to a platform where EMFILE delivery is reliable if needed) and fail rather than return when exhaustion is not induced.
There was a problem hiding this comment.
Hey @johntmyers So the unit tests cover the retry logic and the integration test covers the OS precondition together *i think they provide full coverage without needing to stand up the full proxy stack in a subprocess. I mean I could also Wire up a minimal ProxyHandle in the subprocess child that actually runs the async accept loop, trigger EMFILE, and verify it recovers. This is more complex but do you think it's the better option for testing the production path?
3 participants
Summary
breakin the proxy accept loop with a sleep-and-continue pattern so transient errors (EMFILE, ENFILE, etc.) no longer permanently kill the proxymetadata_serverandedge_tunnelpatternsRelated Issue
Closes #2337
Changes
crates/openshell-supervisor-network/src/proxy.rsbreakon accept error withtokio::time::sleep(backoff).awaitraw_os_error()(EMFILE=24, ENFILE=23)Testing
cargo check -p openshell-supervisor-network— compiles cleancargo test -p openshell-supervisor-network— all tests passChecklist