Close non-stdio file handles when daemonizing

[ntrrgc]

Otherwise, when the compiler wrapper spawns the sccache server, the server may end up with unintended file descriptors, which can lead to unexpected problems.

This is particularly problematic if any of those file descriptors that accidentally end up in the server process is a pipe, as the pipe will only be closed when all the processes with that file descriptor close it or exit.

This was causing sccache to hang ninja, as ninja uses the EOF of a pipe given to the subprocess to detect when that subprocess has exited: ninja-build/ninja#2444 (comment)

This patch adds a dependency on the
close_fds crate, which automatically chooses an appropriate mechanism to close a range of file descriptors. On Linux 5.9+ that mechanism will be libc::close_range().

Fixes #2313

[ntrrgc]

The test test_rust_cargo_cmd_readonly_preemtive_block was failing. After investigation, I found the reason:

if env::var("SCCACHE_ERROR_LOG").is_ok() {
    let f = create_error_log()?;
    // Can't report failure here, we're already daemonized.
    daemonize()?;
    redirect_error_log(f)?;

That code was opening a log file, daemonizing, then swapping stderr with it. We could pass the file descriptor of f to the list of exceptions of close_open_fds(), but in this particular case there is no need, as swapping stderr with a file is already a built-in feature of Daemonize.

I'm updating the patch to make daemonize accept a log file as argument and do the file handle swap at that point, which also removes the need for the redirect_stderr() and redirect_error_log() functions in sccache.

[ntrrgc]

Several CI jobs are consistently failing, so probably there are still file handles unaccounted. Will have to reproduce the failures locally and find the missing handles.

[ntrrgc]

I checked the failures now and it seems it is just formatting nits and failures to build on Windows. If there was something more serious, I can't see it now...

Oh well, I'm updating the patch with the formatting nits applied and with the close_fd crate marked for use only in cfg(unix).

[ntrrgc]

Any thoughts on the patch? Can someone approve the CI workflows so they can run?

[sylvestre]

sorry for the latency
it seems that windows is failing :)

[ntrrgc]

error[E0061]: this function takes 0 arguments but 1 argument was supplied
   --> src\commands.rs:645:17
    |
645 |                 daemonize(Some(f))?;
    |                 ^^^^^^^^^ ------- unexpected argument of type `std::option::Option<fs_err::File>`
    |
note: function defined here
   --> src\util.rs:942:8
    |
942 | pub fn daemonize() -> Result<()> {
    |        ^^^^^^^^^
help: remove the extra argument

This is very odd because daemonize() is modified by this patch to add that extra argument, and you can see that in the diff with those same line numbers:

diff --git a/src/util.rs b/src/util.rs
index 7174681..04d348c 100644
--- a/src/util.rs
+++ b/src/util.rs
@@ -841,7 +841,7 @@ impl<'a> Hasher for HashToDigest<'a> {
 
 /// Pipe `cmd`'s stdio to `/dev/null`, unless a specific env var is set.
 #[cfg(not(windows))]
-pub fn daemonize() -> Result<()> {
+pub fn daemonize(log_file: Option<File>) -> Result<()> {

It's as if the patch had been only partially applied by the CI server. The same error is happening in many of them.

[ntrrgc]

Oooooh, because they're the Windows builds! I keep forgetting the #[cfg(not(windows))]. There has to be a Windows specific variant that I need to update.

[ntrrgc]

toml_format was complaining about the packages not being in alphabetical order (it didn't explicitly say it, but I could confirm it locally).

error: function `redirect_stderr` is never used
   --> src\commands.rs:130:4
    |
130 | fn redirect_stderr(f: File) {
    |    ^^^^^^^^^^^^^^^

redirect_stderr() should actually be used. daemonize() in Windows used to be a full no-op, but now that it takes the log file as argument, it should also set it as stderr in Windows, so the API is the same for all platforms. Fixed it by moving redirect_stderr() to util.rs and using it in the Windows version of daemonize():

/// Daemonizing is a no-op on Windows, but we still must set the log file as
/// stderr.
#[cfg(windows)]
pub fn daemonize(log_file: Option<File>) -> Result<()> {
    if let Some(log_file) = log_file {
        redirect_stderr(log_file);
    }
    Ok(())
}

The failures of the test_rust_cargo_cmd_readonly_preemtive_block in Windows are just a consequence of that log redirection missing in Windows, so that should be fixed now too:

    let log_contents = fs::read_to_string(sccache_log)?;
    assert!(predicates::str::contains("server has setup with ReadOnly").eval(log_contents.as_str()));

Hopefully no more remaining failures!

[AJIOB]

Hi @sylvestre,

Is it ok for you now?

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.

Project coverage is 65.63%. Comparing base (733688f) to head (8125bae).

Files with missing lines	Patch %	Lines
src/bin/sccache-dist/main.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2314      +/-   ##
==========================================
- Coverage   65.66%   65.63%   -0.04%     
==========================================
  Files          65       65              
  Lines       35496    35531      +35     
==========================================
+ Hits        23310    23321      +11     
- Misses      12186    12210      +24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.