fix(server): close inherited fds after daemonizing to avoid pipe deadlock (supersedes #2314)#2771
fix(server): close inherited fds after daemonizing to avoid pipe deadlock (supersedes #2314)#2771brianmacy wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2771 +/- ##
==========================================
- Coverage 70.90% 70.87% -0.04%
==========================================
Files 71 71
Lines 40393 40479 +86
==========================================
+ Hits 28642 28689 +47
- Misses 11751 11790 +39 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…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>
f598437 to
352e267
Compare
|
Re: the one red check — it is unrelated to this PR and its fix does not belong here.
The specific issue
sccache-dist collects Rust inputs from rustc's dep-info and untars them on the builder. The intermediate directory the Publish times match the pass→fail boundary on
ScopeThis PR touches only the cache-server daemon's FD handling; the failure is entirely server-side in the sccache-dist builder. The fix — either sccache-dist input reconstruction for (Correcting my earlier comment, which loosely called this a "flake" — it is deterministic, just environment/dependency-driven rather than caused by any code here.) |
|
in the future, could you please keep your comments a bit shorter? |
3 participants
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.