Re: [PATCH v7 10/10] fsmonitor: close inherited file descriptors and detach in daemon

From: Paul Tarjan <paul@paultarjan.com>
Date: Mar 4, 2026, 18:17 UTC
Message-ID: <20260304181753.25787-1-github@paulisageek.com>
In-Reply-To: <aafilb7FnvWBTQ0J@pks.im>

On Tue, Mar 4, 2026, Patrick Steinhardt wrote:

> Hm. We already have related logic in `daemonize()`. Should we maybe
> reuse that function, and potentially expand it to handle closing all FDs
> up to the maximum file descriptor?

daemonize() does a double-fork with setsid, which is the classic Unix daemon pattern. But fsmonitor uses start_bg_command(), which forks the daemon and waits for it to signal readiness over IPC. If we called daemonize() inside the child, start_bg_command() would lose track of the PID.

So instead we just use setsid() to detach from the terminal, and close_fd_above_stderr in run-command to close inherited FDs before exec.

> This feels like a "while-at-it" change to me. Should it maybe be moved
> into a separate commit?

Done. Split the 30-second stop timeout into its own commit (patch 10, "fsmonitor: add timeout to daemon stop command").

> Might make sense to reorder commits a bit so that the fix comes first.
> In that case we wouldn't ever have to introduce the timeouts in the
> first place.

Done. Reordered in v8 so run-command and daemon detach come before the test commit. The meson timeout never appears now.

> This change feels unrelated and is not mentioned in the commit message.

--start-timeout=10 is now in the tests commit (patch 11) and documented in that commit message.

> I thought with our call to setsid() we're not part of the same process
> group anymore. So why is this change here still needed?

setsid() runs inside the daemon after it's already been forked. The set -m in the test is about the shell putting `git pull &` into its own process group. Without it, the background job inherits the test shell's pgid and `wait` stalls. Moved to the tests commit (patch 11) with a note in the commit message explaining why it's still needed.