fix(server): close inherited fds after daemonizing to avoid pipe deadlock (supersedes #2314)#2771
Open
brianmacy wants to merge 1 commit into
Open
fix(server): close inherited fds after daemonizing to avoid pipe deadlock (supersedes #2314)#2771brianmacy wants to merge 1 commit into
brianmacy wants to merge 1 commit into
Conversation
…lock The persistent cache server is lazily fork+exec'd by a compile-job client and inherits the client's open file descriptors, including ninja's jobserver pipe and the `cmake ... | tee` stdout pipe. Because the long-lived daemon keeps those write-ends open, the writer never observes EOF, producing a 0%-CPU deadlock (mozilla#1011). After daemonizing, close all inherited file descriptors >= 3 (before the server binds its socket) via the close_fds crate. The sweep is scoped to the cache server (Command::InternalStartServer): sccache-dist has already built a reqwest client that owns runtime/epoll fds at daemonize time, so closing arbitrary fds there would be unsound -- it opts out. daemonize() now takes (Option<File>, bool): the error-log stderr redirect is folded into daemonize (configured through daemonix's Stdio) instead of a separate post-daemonize step, and the bool selects whether to run the fd sweep. SCCACHE_NO_DAEMON=1 (foreground debug mode) skips daemonizing and the sweep; SCCACHE_NO_FD_HYGIENE=1 is an escape hatch that skips only the sweep. On Windows there is no daemon fork, so daemonize() just redirects stderr. Adds a Unix regression test that forks, runs close_open_fds(3, &[]) in the child, and asserts an inherited pipe fd is closed (fcntl F_GETFD returns -1 with errno EBADF) while stderr (fd 2) survives the sweep. Closes mozilla#1011. Supersedes mozilla#2314. Co-authored-by: Alicia Boya García <aboya@igalia.com>
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.
Summary
Fixes the 0%-CPU daemon deadlock in #1011.
The persistent cache server is lazily
fork+exec'd by a compile-job clientand inherits that client's open file descriptors — including ninja's jobserver
pipe and the
cmake ... | teestdout pipe. Because the long-lived daemon keepsthose write-ends open, the writer end never observes EOF, and the build wedges
at 0% CPU (reliably reproducible under a
ninjabuild that pipes throughtee,e.g. an ONNX Runtime link step).
Fix
After daemonizing — and before the server binds its socket — close all inherited
file descriptors
>= 3via theclose_fdscrate.
Command::InternalStartServer).sccache-disthas already constructed a
reqwestclient that owns runtime/epoll fds by thetime it daemonizes, so blindly closing fds there would be unsound — it opts out.
daemonize()now takes(Option<File>, bool): the error-log stderr redirectis folded into
daemonize(configured via daemonix'sStdio) instead of aseparate post-daemonize step, and the
boolselects whether to run the sweep.SCCACHE_NO_DAEMON=1(foreground debug) skips daemonizing andthe sweep;
SCCACHE_NO_FD_HYGIENE=1is an escape hatch that skips only thesweep. On Windows there is no daemon fork, so
daemonize()just redirects stderr.Test
Adds a Unix regression test that forks, runs
close_open_fds(3, &[])in thechild, and asserts an inherited pipe fd is closed (
fcntl(F_GETFD)→EBADF)while stderr (fd 2) survives the sweep.
This has also been running in our own CI at scale (a fork build carrying this
change) across all six release targets with no regressions, and it eliminates
the ONNX-link deadlock we hit consistently without it.
Credit
This finishes off @ntrrgc's #2314, rebased onto current
mainand adapted tothe
daemonize→ daemonix migration (hence thedaemonize(Option<File>, bool)shape and the
close_fds 0.3.2dependency). Co-authored accordingly.Closes #1011. Supersedes #2314.