fix(pool): back off when accept() fails on descriptor exhaustion - #718
Open
gimballock wants to merge 1 commit into
Open
fix(pool): back off when accept() fails on descriptor exhaustion#718gimballock wants to merge 1 commit into
gimballock wants to merge 1 commit into
Conversation
The accept error arm logged the failure and immediately retried, with no errno discrimination and no delay. When the process or the system is out of file descriptors, `accept()` fails without consuming the pending connection, so the listener stays readable and the next iteration fails identically. The loop then spins at CPU speed for as long as the shortage lasts, writing one `error!` line per iteration. Two costs follow: a core is burned doing nothing, and the log grows without bound. The second is the dangerous one, because a pool log can already reach hundreds of megabytes at scale, so a sustained spin can fill the volume and take the pool down rather than merely failing some connects. Discriminate descriptor exhaustion and delay the retry: EMFILE and ENFILE back off starting at 5 ms and doubling to a 1 s ceiling, while every other error retries immediately as before, because those are per-connection faults and delaying them would slow admission for every miner. A successful accept, or any non-exhaustion error, resets the delay. The wait sits inside a `select!` on the cancellation token so shutdown is never held up by a pending backoff. The schedule follows the precedent in Go's `net/http.Server`. Discrimination goes through `raw_os_error()` because both errnos map to `ErrorKind::Uncategorized`, which is why the existing arm cannot tell them apart. This adds no dependency: two `#[cfg(unix)]` constants, whose values agree across Linux and the BSDs including macOS. Adds four unit tests over the classification and the schedule, which are pure functions. They construct `io::Error::from_raw_os_error(24)` directly rather than exhausting descriptors, because `setrlimit` is process-wide and unreliable under parallel test execution. The spin has not been observed in practice: our own runs peaked at 75% of a 400,000 descriptor limit, so the exhaustion path was never reached. This is read from the source rather than reproduced.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When the pool runs out of file descriptors, the accept loop spins at CPU speed and writes a log line per iteration until the shortage clears.
Problem
The accept error arm logs and immediately retries, with no errno discrimination and no delay (
channel_manager/mod.rs:412-414):When the process or the system is out of descriptors,
accept()fails without consuming the pending connection, so the listener stays readable and the next iteration fails identically. Two costs follow: a core is burned doing nothing, and the log grows without bound.The second cost is the dangerous one. Our pool log reached 227 MB at 200,000 connections in normal operation, on a volume without rotation, so a sustained spin turns "some connections fail" into "the disk fills and the pool stops."
Fix
EMFILE/ENFILE→ delay the retry, starting at 5 ms and doubling to a 1 s ceilingselect!on the cancellation token, so shutdown is never held up by a pending backoffDiscrimination goes through
raw_os_error()because both errnos map toErrorKind::Uncategorized— there is no stable variant to match on, which is precisely why the current arm cannot distinguish them. This needs no new dependency: two#[cfg(unix)]constants, whose values agree across Linux and the BSDs including macOS.The 5 ms → 1 s schedule follows the precedent in Go's
net/http.Server, which applies the same doubling bounds to temporary accept errors.Tests
Four unit tests, deterministic, 0.00 s, with no descriptor or filesystem manipulation:
descriptor_exhaustion_is_recognisedEMFILE(24) andENFILE(23) select the backoff pathother_failures_are_not_delayedConnectionAborted,PermissionDenied,Interrupteddo notbackoff_starts_at_the_floor_then_doubles_to_the_ceilingbackoff_is_idempotent_at_the_ceilingThe classification and the schedule are pure functions, so the tests construct
io::Error::from_raw_os_error(24)directly rather than exhausting descriptors.setrlimitwould be process-wide and unreliable under parallel test execution, and a listener trait would be invasive relative to what it buys.What this does not demonstrate
We have not observed the spin. Our runs peaked at 75% of a 400,000 descriptor limit at 1.000 fd/conn, so the exhaustion path was never reached — this is read from the source, not reproduced. The 227 MB figure reflects our deployment and its rotation policy rather than yours.
Validation
Against
mainon the pinned 1.85.0 toolchain:cargo +nightly fmt --all --manifest-path=pool-apps/Cargo.toml -- --check— passcargo clippy --manifest-path=pool-apps/Cargo.toml -- -D warnings— passcargo test --manifest-path=pool-apps/pool/Cargo.toml— 5 passedpool_sv2is 0.7.0 with 0.6.0 published, so it is already bumped since the last release, and this change is API-compatible because both helpers are privateRelated but independent: #717 covers the listen backlog on this same accept path.