From: Patrick Steinhardt <ps@pks.im>
Date: Wed, 04 Mar 2026 07:43:17 GMT
Subject: Re: [PATCH v7 10/10] fsmonitor: close inherited file descriptors and detach in daemon
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:
> 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.

> 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?

> 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.

> 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.

> @@ -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