fsmonitor: close inherited file descriptors and detach in daemon

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.

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.

Add a 30-second timeout to "fsmonitor--daemon stop" so it does
not block indefinitely if the daemon fails to shut down.

Signed-off-by: Paul Tarjan <github@paulisageek.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

builtin/fsmonitor--daemon.c

@@ -86,6 +86,8 @@ static int do_as_client__send_stop(void)
 {
 	struct strbuf answer = STRBUF_INIT;
 	int ret;
+	int max_wait_ms = 30000;
+	int elapsed_ms = 0;
 
 	ret = fsmonitor_ipc__send_command("quit", &answer);
 
@@ -96,8 +98,16 @@ static int do_as_client__send_stop(void)
 		return ret;
 
 	trace2_region_enter("fsm_client", "polling-for-daemon-exit", NULL);
-	while (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING)
+	while (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING) {
+		if (elapsed_ms >= max_wait_ms) {
+			trace2_region_leave("fsm_client",
+					    "polling-for-daemon-exit", NULL);
+			return error(_("daemon did not stop within %d seconds"),
+				     max_wait_ms / 1000);
+		}
 		sleep_millisec(50);
+		elapsed_ms += 50;
+	}
 	trace2_region_leave("fsm_client", "polling-for-daemon-exit", NULL);
 
 	return 0;
@@ -1431,7 +1441,7 @@ done:
 	return err;
 }
 
-static int try_to_run_foreground_daemon(int detach_console MAYBE_UNUSED)
+static int try_to_run_foreground_daemon(int detach_console)
 {
 	/*
 	 * Technically, we don't need to probe for an existing daemon
@@ -1451,10 +1461,21 @@ static int try_to_run_foreground_daemon(int detach_console MAYBE_UNUSED)
 		fflush(stderr);
 	}
 
+	if (detach_console) {
 #ifdef GIT_WINDOWS_NATIVE
-	if (detach_console)
 		FreeConsole();
+#else
+		/*
+		 * Create a new session so that the daemon is detached
+		 * from the parent's process group.  This prevents
+		 * shells with job control (e.g. bash with "set -m")
+		 * from waiting on the daemon when they wait for a
+		 * foreground command that implicitly spawned it.
+		 */
+		if (setsid() == -1)
+			warning_errno(_("setsid failed"));
 #endif
+	}
 
 	return !!fsmonitor_run_daemon();
 }
@@ -1517,6 +1538,7 @@ static int try_to_start_background_daemon(void)
 	cp.no_stdin = 1;
 	cp.no_stdout = 1;
 	cp.no_stderr = 1;
+	cp.close_fd_above_stderr = 1;
 
 	sbgr = start_bg_command(&cp, bg_wait_cb, NULL,
 				fsmonitor__start_timeout_sec);

fsmonitor-ipc.c

@@ -61,6 +61,9 @@ static int spawn_daemon(void)
 
 	cmd.git_cmd = 1;
 	cmd.no_stdin = 1;
+	cmd.no_stdout = 1;
+	cmd.no_stderr = 1;
+	cmd.close_fd_above_stderr = 1;
 	cmd.trace2_child_class = "fsmonitor";
 	strvec_pushl(&cmd.args, "fsmonitor--daemon", "start", NULL);
 

t/meson.build

@@ -1208,18 +1208,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
 

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
@@ -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
 }
 
 stop_git () {