Re: [PATCH v7 10/10] fsmonitor: close inherited file descriptors and detach in daemon
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Mar 4, 2026, 07:43 UTC
- Message-ID
- <aafilb7FnvWBTQ0J@pks.im>
- In-Reply-To
- <4987a009a2e74a04aa1a8deb0508c971a3df0549.1772065643.git.gitgitgadget@gmail.com>
On Thu, Feb 26, 2026 at 12:27:23AM +0000, Paul Tarjan via GitGitGadget wrote:
Show 8 quoted lines
> From: Paul Tarjan <github@paulisageek.com> > > When the fsmonitor daemon is spawned as a background process, it may > inherit file descriptors from its parent that it does not need. In > particular, when the test harness or a CI system captures output through > pipes, the daemon can inherit duplicated pipe endpoints. If the daemon > holds these open, the parent process never sees EOF and may appear to > hang.
Oh, interesting.
Show 12 quoted lines
> Set close_fd_above_stderr on the child process at daemon startup so > that file descriptors 3 and above are closed before any daemon work > begins. This ensures the daemon does not inadvertently hold open > descriptors from its launching environment. > > Additionally, call setsid() when the daemon starts with --detach to > create a new session and process group. Without this, shells that > enable job control (e.g. bash with "set -m") treat the daemon as part > of the spawning command's job. Their "wait" builtin then blocks until > the daemon exits, which it never does. This specifically affects > systems where /bin/sh is bash (e.g. Fedora), since dash only waits for > the specific PID rather than the full process group.
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?
> Add a 30-second timeout to "fsmonitor--daemon stop" so it does > not block indefinitely if the daemon fails to shut down.
This feels like a "while-at-it" change to me. Should it maybe be moved into a separate commit?
Show 24 quoted lines
> diff --git a/t/meson.build b/t/meson.build
> index 85ef2ae2fa..19e8306298 100644
> --- a/t/meson.build
> +++ b/t/meson.build
> @@ -1210,18 +1210,12 @@ test_environment = script_environment
> test_environment.set('GIT_BUILD_DIR', git_build_dir)
>
> foreach integration_test : integration_tests
> - per_test_kwargs = test_kwargs
> - # The fsmonitor tests start daemon processes that in some environments
> - # can hang. Set a generous timeout to prevent CI from blocking.
> - if fs.stem(integration_test) == 't7527-builtin-fsmonitor'
> - per_test_kwargs += {'timeout': 1800}
> - endif
> test(fs.stem(integration_test), shell,
> args: [ integration_test ],
> workdir: meson.current_source_dir(),
> env: test_environment,
> depends: test_dependencies + bin_wrappers,
> - kwargs: per_test_kwargs,
> + kwargs: test_kwargs,
> )
> endforeach
> 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.
Show 13 quoted lines
> diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh > index 774da5ac60..d7e64bcb7a 100755 > --- a/t/t7527-builtin-fsmonitor.sh > +++ b/t/t7527-builtin-fsmonitor.sh > @@ -766,7 +766,7 @@ do > else > test_expect_success "Matrix[uc:$uc_val][fsm:$fsm_val] enable fsmonitor" ' > git config core.fsmonitor true && > - git fsmonitor--daemon start && > + git fsmonitor--daemon start --start-timeout=10 && > git update-index --fsmonitor > ' > fi
This change feels unrelated and is not mentioned in the commit message.
Show 16 quoted lines
> @@ -997,7 +997,17 @@ start_git_in_background () {
> nr_tries_left=$(($nr_tries_left - 1))
> done >/dev/null 2>&1 3>&- 4>&- 5>&- 6>&- 7>&- &
> watchdog_pid=$!
> +
> + # Disable job control before wait. With "set -m", bash treats
> + # "wait $pid" as waiting for the entire job (process group),
> + # which blocks indefinitely if the fsmonitor daemon was spawned
> + # into the same process group and is still running. Turning off
> + # job control makes "wait" only wait for the specific PID.
> + set +m &&
> wait $git_pid
> + wait_status=$?
> + set -m
> + return $wait_status
> }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?
Patrick