Repository navigation
fix(box): close inherited descriptors in one syscall, not an RLIMIT_NOFILE loop - #37
Open
csantanapr wants to merge 1 commit into
Open
csantanapr wants to merge 1 commit into
csantanapr wants to merge 1 commit into
Conversation
csantanapr
force-pushed
the
fix/alias-close-range
branch
2 times, most recently
from
October 9, 2026 17:58
b35abfc to
424c08f
Compare
…OFILE loop The socket alias closes every inherited descriptor above stderr before it connects to the broker. It did this with a loop from STDERR_FILENO + 1 up to getdtablesize(), which returns the soft RLIMIT_NOFILE, so the cost scaled with the ambient descriptor limit rather than the number of descriptors actually open. When a runtime hands the process a very large limit (for example about 1e9, which a process inherits from a service manager that sets LimitNOFILE=infinity) the alias made roughly a billion close() calls, nearly all EBADF, before every mediated command. Close the range in one call instead, falling back in order: 1. close_range, issued via libc::syscall(SYS_close_range, ...) rather than libc::close_range. The glibc wrapper exists only as the versioned symbol close_range@GLIBC_2.34, so going straight to the syscall number adds no glibc-version requirement, builds on musl, and needs no target_env gate. 2. On any close_range error, close the descriptors listed in /proc/self/fd, which costs one close() per open descriptor. 3. Only if /proc is also unavailable, the historical bounded loop (best-effort; it cannot see a descriptor above a limit later lowered by setrlimit). macOS and other non-Linux unix targets keep the bounded loop unchanged. glibc note: this removes close_range's own version dependency, but the alias still needs glibc 2.34 for an unrelated reason. glibc 2.34 merged libpthread into libc, and this binary creates threads via its Tokio runtime, so it already pulls in pthread_create@GLIBC_2.34 regardless of this change. Tests: a new integration test (box_close_range) drives a dedicated probe binary in a fresh process per phase (fork is unsafe in the multithreaded test harness and in-process would wipe the harness's own descriptors). It covers the close_range fast path (asserting close_range specifically ran, with an explicit skip when it is unavailable, promoted to a hard failure when BOX_REQUIRE_CLOSE_RANGE=1), the /proc/self/fd fallback, the bounded loop, a descriptor near the soft limit, and the exact lower boundary (fd 3). The bounded and soft-limit phases cap their own soft limit so they run the same way and in seconds on every host. Fixes strands-agents#36
csantanapr
force-pushed
the
fix/alias-close-range
branch
from
October 9, 2026 18:06
424c08f to
3374f1d
Compare
This branch has not been deployed
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.
Description
The socket alias closes every inherited file descriptor above stderr before it connects to the
broker. It did this with a loop from
STDERR_FILENO + 1up togetdtablesize(), andgetdtablesize()returns the softRLIMIT_NOFILE. So the cost of the close scaled with theambient descriptor limit, not with the number of descriptors actually open.
That is cheap when the limit is small, but some runtimes hand a process a very large
RLIMIT_NOFILE(for example about 1e9, which a process inherits from a service manager that setsLimitNOFILE=infinity). At that limit the alias makes roughly a billionclose()calls, nearlyall returning
EBADF, before it does anything else, and it runs this loop before every mediatedcommand.
This change closes the whole range in one call, with two fallbacks:
close_range, issued vialibc::syscall(SYS_close_range, ...)rather thanlibc::close_range(see Notes). Its cost does not depend on the limit.close_rangeerror, close the descriptors listed in/proc/self/fd, which costs onlyas many
close()calls as there are open descriptors./procis also unavailable, the historical bounded loop, kept best-effort.macOS and every other non-Linux unix target keep the bounded loop unchanged (that includes
FreeBSD, which has its own
close_range, but this change does not call it there).On success the two primary paths guarantee that no descriptor above stderr survives. The bounded
last resort keeps its original best-effort behavior and names its one limitation in its own doc:
getdtablesize()returns the current soft limit, andsetrlimit(2)may lower it below adescriptor already open, which keeps its number and stays open above the ceiling this loop walks.
The two primary paths both handle that case; a cheap, complete check for it in the last resort
would need the same mechanisms that have already failed by the time it runs, or a sweep to the
kernel's
fs.nr_openceiling (about 1e9), which would reintroduce the exact cost this changeremoves.
The descriptor-closing code lives in its own file
(
crates/box/src/bin/strands-box-sock-alias/close_descriptors.rs), depending on nothing butlibcandstd. A test-support probe binary (box-close-range-probe) includes that same fileand is driven by a new integration test (
box_close_range).Notes
close_rangeis called vialibc::syscall(SYS_close_range, ...)rather thanlibc::close_range. The latter binds glibc's own C wrapper, exported only as the versioneddynamic symbol
close_range@GLIBC_2.34(added when glibc 2.34 first wrapped the kernelsyscall). Calling the syscall by number avoids that specific version dependency, needs no
target_env = "gnu"gate, and builds on musl. The alias binary as a whole still requires glibc2.34 for an unrelated, pre-existing reason: glibc 2.34 merged
libpthreadintolibc, and thisbinary creates threads via its Tokio runtime, so it already pulls in
pthread_create@GLIBC_2.34and similar symbols regardless of this change (confirmed with
objdump -Tagainst both thisbranch and an unmodified
mainbuild; the pthread symbol set is identical either way). So thischange removes one specific, avoidable version dependency; it does not by itself change which
glibc versions can load the alias.
(
close_rangespecifically, not merely that some mechanism did). It reports an explicit skiprather than failing or silently passing through the fallback if
close_rangeis genuinelyunavailable on the test host. Setting
BOX_REQUIRE_CLOSE_RANGE=1on a runner that is known tohave
close_rangeturns a skip there into a hard failure, so a runner that quietly stoppedexercising the fast path does not pass unnoticed. It is set on the stock Linux test leg.
ClosePathand the*_reportingfunctions stay in the production build rather than behind atest-only feature. The cost is zero, and gating them behind test-support would mean the probe
tests a different build of the function than the alias ships.
Related Issues
Fixes #36
Type of Change
Bug fix
Testing
cargo fmt --all --check: clean.cargo clippy --workspace --all-targets --all-features -- -D warnings -A clippy::too_many_arguments -A clippy::only_used_in_recursion -A clippy::await_holding_refcell_ref -A clippy::explicit_auto_deref -A clippy::chunks_exact_to_as_chunks -A dead_code -A non_snake_case:clean. The bare
cargo clippy --workspace --all-targets --all-features -- -D warningsreportswarnings only in the vendored
strands-shelltree and incrates/containment, both pre-existingon an unmodified base checkout and in no file this change touches; those are the allowances the
CI recipe above is built around.
crates/box/tests/box_close_range.rs(5 tests, each driving a freshinstance of the test-support probe binary so every phase runs in its own process):
the_close_range_fast_path_closes_above_stdio_and_keeps_stdiothe_proc_self_fd_fallback_closes_above_stdio_and_keeps_stdiothe_bounded_loop_closes_above_stdio_and_keeps_stdioa_descriptor_near_the_soft_limit_is_closedthe_exact_lower_boundary_descriptor_is_closedboundedandsoft-limitphases lower their own softRLIMIT_NOFILEto 4096 before theyrun, so they behave the same way and finish in the same (short) time on every host rather than
depending on the ambient limit the issue is about. The probe is its own process, so this affects
nothing else.
--no-runand ran it in a container with--ulimit nofile=1073741816:1073741816(confirmed soft and hard both at that value inside). All5 tests passed in about 0.01s. As a control, the same binary built from the pre-change code, run
the same way, failed the
soft-limitphase withF_DUPFD: Too many open filesand spent about253s in the
boundedphase, which is the behavior this change removes.cargo test --workspace --all-features: passes except for 3 pre-existing failures instrands-box-containment::backend::linux::namespace::view::tests, which need namespacecapabilities the test environment does not grant and are skipped by name in CI. They fail
identically on an unmodified base checkout;
crates/containmentis byte-identical to base and tocurrent
main, and this change touches onlycrates/box.cargo test --workspace --all-features)cargo fmt --allandcargo clippy --workspace --all-targets --all-features -- -D warningsChecklist
AI tools, and I can explain why it works
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.