fix: reap script/PHP children without blocking the poller, drop USING_TPOPEN (#633)

## What
- `nft_pclose()` (`nft_popen.c`) and `php_terminate_and_reap()`
(`php.c`) no longer escalate through a spin/sleep/SIGTERM/SIGKILL
sequence. Each now does one bounded, non-blocking check (~20ms allowance
for normal fork/exec/exit scheduling latency), kills anything still
running, and hands it to the existing abandoned-pid sweep instead of
waiting on it in the calling (polling) thread.
- Removed the `USING_TPOPEN`/`--enable-popen` native-`popen()` fallback
in `exec_poll()` (`poller.c`, `configure.ac`). It could not track or
kill a timed-out child (no pid available from a real `popen()`), so a
timeout there could only ever abandon the descriptor without terminating
the process.
- Extended `tests/regression/test_child_process_safety.sh` with
structural guards against reintroducing the blocking spin/sleep
escalation in either function.
- Updated `tests/unit/test_linked.c`'s `nft_pclose()` coverage for the
new, faster contract (the old test asserted a graceful-SIGTERM-then-wait
exit code that the new design intentionally no longer provides).

## Why
Investigating a reported poller slowdown surfaced that `nft_pclose()`
(added by #614) could block a poller thread for up to ~5 seconds per
script whose process hadn't exited by the time its pipe was closed, and
`php_terminate_and_reap()` had the same shape (up to ~2s), reachable
mid-poll-cycle via `php_fail_read()`, not just at shutdown. Separately,
the `USING_TPOPEN` fallback's inability to kill a timed-out child was
flagged as out of scope in #574. Tracked in #632; design principles
written up in #630.

## What was tested
- `./bootstrap && ./configure --enable-warnings && make -j` — clean
build, no new warnings.
- `make check` — all 11 suites pass (unit, regression, integration),
including the extended `test_child_process_safety.sh` guard and the
updated `test_linked` case.
- `cppcheck --enable=all --std=c11` on the three changed `.c` files — no
new errors (pre-existing style-only notes unrelated to this change).

Signed-off-by: TheWitness <thewitness@cacti.net>

CHANGELOG

@@ -63,6 +63,8 @@ The Cacti Group | spine
 -feature: Add the unit, fuzz and integration test suites
 -feature: Make spine work with Stream feature
 -feature: Seed spine.conf from spine.conf.dist on first install without overwriting an existing config
+-issue#632: Reap nft_pclose() and php_terminate_and_reap() with a single bounded non-blocking check, killing and handing off to the abandoned-pid sweep instead of a blocking spin/SIGTERM/SIGKILL escalation that could pin a poller thread for seconds
+-issue#632: Remove the USING_TPOPEN/--enable-popen native popen() fallback in exec_poll(), which could not track or kill a timed-out child
 
 1.2.31
 -issue#365: Removing Backtrace Support

configure.ac

@@ -438,21 +438,6 @@ AC_ARG_WITH(max-mysql-buffer,
 AC_DEFINE_UNQUOTED(MAX_MYSQL_BUF_SIZE, $max_mysql_buffer, The maximum MySQL buffer size to insert)
 AC_MSG_RESULT(checking for the maximum MySQL buffer size... $max_mysql_buffer)
 
-# ****************** Traditional Popen Check ***********************
-# If we should use the system popen or nifty popen
-AC_MSG_CHECKING(whether we are using traditional popen)
-AC_ARG_ENABLE(popen,
-    [  --enable-popen          Enable the traditional popen implementation of nifty popen (default: disabled)],
-    [ ENABLED_TPOPEN=$enableval ],
-    [ ENABLED_TPOPEN=no ]
-    )
-if test "$ENABLED_TPOPEN" = "yes"; then
-  AC_MSG_RESULT([yes])
-  AC_DEFINE(USING_TPOPEN, 1, If traditional popen should be enabled by default)
-else
-  AC_MSG_RESULT([no])
-fi
-
 # ****************** Force Net-SNMP Version Checks ***********************
 # If we should use the system popen or nifty popen
 AC_MSG_CHECKING(whether to verify net-snmp library vs header versions)

nft_popen.c

@@ -145,28 +145,19 @@ static __attribute__((noinline)) struct pid *pid_list_close_and_take(int fd)
 
 	return cur;
 }
-
-/* nft_pclose() must not block a poller thread indefinitely. A script that
-   writes its value and then lingers, or that ignores SIGPIPE, would otherwise
-   pin the thread across polling cycles while holding its available_scripts
-   token. Poll with WNOHANG, then escalate to SIGKILL. */
-/* Budget to SIGKILL is about five seconds on both platforms. The counts differ
- * because the granularity does: everywhere else in the tree usleep() is simply
- * skipped under SOLAR_THREAD rather than replaced, so the coarsest wait
- * available there is a whole second and the attempt count scales to match.
- * Leaving the counts equal made the Solaris path roughly a hundred seconds,
- * longer than a polling cycle, while nft_pclose() holds an available_scripts
- * token throughout. */
+/* nft_pclose() must not block a poller thread: a script that writes its
+   value and then lingers, or that ignores signals, would otherwise pin the
+   thread across polling cycles while holding its available_scripts token.
+   It gets a brief (~20ms), non-escalating WNOHANG allowance via
+   spine_reap_child_bounded() below - just enough to absorb ordinary
+   fork/exec/exit scheduling latency (measured flaky under virtualized/loaded
+   CI at the original 2ms budget), not to wait out a script that is actually
+   still working. Anything still running past that is killed outright and
+   handed to nft_abandon_child() so nft_sweep_abandoned() reaps it on a
+   later, unrelated nft_popen() call instead of this thread waiting for it. */
 #define NFT_PCLOSE_REAP_USEC 50000
 #define NFT_PCLOSE_SPIN_USEC 200
-#define NFT_PCLOSE_SPIN_ATTEMPTS 10
-#ifndef SOLAR_THREAD
-#define NFT_PCLOSE_TERM_ATTEMPTS 100
-#define NFT_PCLOSE_KILL_ATTEMPTS 20
-#else
-#define NFT_PCLOSE_TERM_ATTEMPTS 5
-#define NFT_PCLOSE_KILL_ATTEMPTS 2
-#endif
+#define NFT_PCLOSE_SPIN_ATTEMPTS 100
 
 int spine_set_cloexec(int fd) {
 	int flags;
@@ -303,15 +294,13 @@ int spine_reap_child_bounded(pid_t pid, int *pstat, int attempts) {
 		}
 
 		/* The delay is load-bearing: without it the attempts are spent in
-		   nanoseconds and SIGKILL lands before the child can exit.
+		   nanoseconds and the caller's kill lands before the child can exit.
 
 		   Starting at the full 50ms charged that to every script that exits a
 		   moment after closing stdout, which is the common case for anything
-		   that flushes or tears down an interpreter. nft_pclose() runs while
-		   the caller still holds an available_scripts token, so that delay
-		   costs poller capacity rather than one thread. Spin briefly first,
-		   then settle. The attempt count and so the time to SIGKILL are
-		   unchanged. */
+		   that flushes or tears down an interpreter. Spin briefly first, then
+		   settle, so a caller that opts into this bounded wait does not pay
+		   the full interval on every reap. */
 		#ifndef SOLAR_THREAD
 		if (attempt < NFT_PCLOSE_SPIN_ATTEMPTS) {
 			usleep(NFT_PCLOSE_SPIN_USEC);
@@ -609,7 +598,10 @@ int nft_pchild(int fd) {
  *
  *  nft_pclose
  *
- *  Close the pipe and wait for the status of the child process.
+ *  Close the pipe and check the child's status with a brief (~20ms),
+ *  non-escalating bounded waitpid(). A child still running past that point
+ *  is killed and handed to the abandoned-pid sweep rather than waited for
+ *  here, so this call never blocks the caller on a lingering script.
  *
  *  On success, the exit status of the child process is returned.
  *  On failure, nft_pclose() returns -1, with errno set to:
@@ -646,26 +638,22 @@ nft_pclose(int fd)
 
 	pthread_setcancelstate(cancel_state, NULL);
 
-	/* Give a child a brief chance to observe pipe EOF, then request graceful
-	 * termination before escalating to SIGKILL. */
+	/* The script already had its one chance to write and its pipe is now
+	 * closed. Give it a brief (~20ms), non-escalating allowance to catch the
+	 * common case where it has already exited or is about to on seeing EOF -
+	 * without it, ordinary fork/exec/exit scheduling latency would flag a
+	 * script that is not actually misbehaving. Anything still running past
+	 * that is killed and handed to the abandoned-pid sweep instead of this
+	 * thread waiting for it. */
 	switch (spine_reap_child_bounded(cur->pid, &pstat, NFT_PCLOSE_SPIN_ATTEMPTS)) {
 	case 0:
 		pid = cur->pid;
 		break;
 	case 1:
-		(void)kill(cur->pid, SIGTERM);
-		if (spine_reap_child_bounded(cur->pid, &pstat, NFT_PCLOSE_TERM_ATTEMPTS) == 0) {
-			pid = cur->pid;
-		} else {
-			(void)kill(cur->pid, SIGKILL);
-			if (spine_reap_child_bounded(cur->pid, &pstat, NFT_PCLOSE_KILL_ATTEMPTS) == 0) {
-				pid = cur->pid;
-			} else {
-				nft_abandon_child(cur->pid, "kill budget expired");
-				errno = ETIMEDOUT;
-				pid = -1;
-			}
-		}
+		(void)kill(cur->pid, SIGKILL);
+		nft_abandon_child(cur->pid, "did not exit before pipe close");
+		errno = ETIMEDOUT;
+		pid = -1;
 		break;
 	default:
 		nft_abandon_child(cur->pid, "waitpid failed");

php.c

@@ -801,38 +801,26 @@ cleanup:
 	return rc;
 }
 static int php_terminate_and_reap(pid_t pid) {
-	int attempts;
-	int phase;
 	int status;
-	int signal_number = SIGTERM;
 	pid_t waited;
 
-	for (phase = 0; phase < 2; phase++) {
-		if (kill(pid, signal_number) < 0 && errno != ESRCH) {
-			SPINE_LOG(("WARNING: Unable to signal PHP Script Server PID[%ld]: %s", (long)pid, strerror(errno)));
-		}
+	do {
+		waited = waitpid(pid, &status, WNOHANG);
+	} while (waited < 0 && errno == EINTR);
 
-		for (attempts = 0; attempts < 20; attempts++) {
-			do {
-				waited = waitpid(pid, &status, WNOHANG);
-			} while (waited < 0 && errno == EINTR);
+	if (waited == pid || (waited < 0 && errno == ECHILD)) {
+		return TRUE;
+	}
 
-			if (waited == pid || (waited < 0 && errno == ECHILD)) {
-				return TRUE;
-			}
+	if (waited < 0) {
+		SPINE_LOG(("WARNING: Unable to reap PHP Script Server PID[%ld]: %s", (long)pid, strerror(errno)));
+		return FALSE;
+	}
 
-			if (waited < 0) {
-				SPINE_LOG(("WARNING: Unable to reap PHP Script Server PID[%ld]: %s", (long)pid, strerror(errno)));
-				return FALSE;
-			}
-
-			/* The delay is load-bearing: without it both phases burn twenty
-			 * WNOHANG polls in nanoseconds, so SIGKILL lands immediately and
-			 * the child is never reaped. */
-			usleep(50000);
-		}
-
-		signal_number = SIGKILL;
+	/* Still running after its grace period: kill outright rather than
+	 * waiting to see whether a gentler signal works. */
+	if (kill(pid, SIGKILL) < 0 && errno != ESRCH) {
+		SPINE_LOG(("WARNING: Unable to signal PHP Script Server PID[%ld]: %s", (long)pid, strerror(errno)));
 	}
 
 	return FALSE;
@@ -847,8 +835,11 @@ static int php_terminate_and_reap(pid_t pid) {
  *  information is will close and/or terminate the child PHP Script Server
  *  process and then return to the calling function.
  *
-	 *  Child shutdown is bounded: SIGTERM receives a grace period before SIGKILL.
-	 *  Both phases use non-blocking reaping so a wedged child cannot hang Spine.
+	 *  Shutdown gives the server one grace period (the "quit" write plus a
+	 *  fixed delay below) to exit on its own, then checks once with a
+	 *  non-blocking waitpid(). A child still running at that point is killed
+	 *  outright and handed to the abandoned-pid sweep instead of waiting for
+	 *  it here.
  */
 void php_close(int php_process) {
 	int i;

poller.c

@@ -2422,11 +2422,6 @@ char *exec_poll(host_t *current_host, char *command, int id, const char *type) {
 	int cmd_fd;
 	int pid;
 
-	#ifdef USING_TPOPEN
-	FILE *fd;
-	int close_fd = TRUE;
-	#endif
-
 	int bytes_read;
 	fd_set fds;
 	double begin_time = 0;
@@ -2518,22 +2513,12 @@ char *exec_poll(host_t *current_host, char *command, int id, const char *type) {
 		SPINE_LOG_DEBUG(("The executable is '%s' in \'%s\'", executable, proc_command));
 
 		if (access(executable, X_OK | F_OK) != -1) {
-			#ifdef USING_TPOPEN
-			fd = popen((char *)proc_command, "r");
-			cmd_fd = fileno(fd);
-			if (is_debug_device(current_host->id)) {
-				SPINE_LOG(("Device[%i] DEBUG: The POPEN returned the following File Descriptor %i", current_host->id, cmd_fd));
-			} else {
-				SPINE_LOG_DEBUG(("Device[%i] DEBUG: The POPEN returned the following File Descriptor %i", current_host->id, cmd_fd));
-			}
-			#else
 			cmd_fd = nft_popen(proc_command, "r");
 			if (is_debug_device(current_host->id)) {
 				SPINE_LOG(("Device[%i] DEBUG: The NIFTY POPEN returned the following File Descriptor %i", current_host->id, cmd_fd));
 			} else {
 				SPINE_LOG_DEBUG(("Device[%i] DEBUG: The NIFTY POPEN returned the following File Descriptor %i", current_host->id, cmd_fd));
 			}
-			#endif
 
 			if (cmd_fd > 0) {
 				retry:
@@ -2550,10 +2535,6 @@ char *exec_poll(host_t *current_host, char *command, int id, const char *type) {
 								SPINE_LOG(("Device[%i] ERROR: One or more of the file descriptor sets specified a file descriptor that is not a valid open file descriptor.", current_host->id));
 								SET_UNDEFINED(result_string);
 
-								#ifdef USING_TPOPEN
-								close_fd = FALSE;
-								#endif
-
 								break;
 							case EINTR:
 								#ifndef SOLAR_THREAD
@@ -2581,34 +2562,20 @@ char *exec_poll(host_t *current_host, char *command, int id, const char *type) {
 								} else {
 									SPINE_LOG(("WARNING: A script timed out while processing EINTR's."));
 									SET_UNDEFINED(result_string);
-									#ifdef USING_TPOPEN
-									close_fd = FALSE;
-									#endif
 								}
 								break;
 							case EINVAL:
 								SPINE_LOG(("Device[%i] ERROR: Possible invalid timeout specified in select() statement.", current_host->id));
 								SET_UNDEFINED(result_string);
-								#ifdef USING_TPOPEN
-								close_fd = FALSE;
-								#endif
 								break;
 							default:
 								SPINE_LOG(("Device[%i] ERROR: The script/command select() failed", current_host->id));
 								SET_UNDEFINED(result_string);
-								#ifdef USING_TPOPEN
-								close_fd = FALSE;
-								#endif
 								break;
 						}
 
 						break;
 				case 0:
-					#ifdef USING_TPOPEN
-					SPINE_LOG_MEDIUM(("Device[%i] ERROR: The POPEN timed out", current_host->id));
-
-					close_fd = FALSE;
-					#else
 					SPINE_LOG_MEDIUM(("Device[%i] ERROR: The NIFTY POPEN timed out", current_host->id));
 
 					pid = nft_pchild(cmd_fd);
@@ -2617,7 +2584,6 @@ char *exec_poll(host_t *current_host, char *command, int id, const char *type) {
 					} else {
 						SPINE_LOG(("Device[%i] ERROR: Unable to find the timed-out POPEN child", current_host->id));
 					}
-					#endif
 
 					SET_UNDEFINED(result_string);
 					break;
@@ -2637,14 +2603,7 @@ char *exec_poll(host_t *current_host, char *command, int id, const char *type) {
 				}
 
 				/* close pipe */
-				#ifdef USING_TPOPEN
-				/* we leave the old fd open if it timed out. It will have to exit on it's own */
-				if (close_fd) {
-					pclose(fd);
-				}
-				#else
 				nft_pclose(cmd_fd);
-				#endif
 			} else {
 				SPINE_LOG(("Device[%i] ERROR: Problem executing POPEN [%s]: '%s'", current_host->id, current_host->hostname, command));
 				SET_UNDEFINED(result_string);

tests/regression/test_child_process_safety.sh

@@ -48,6 +48,23 @@ if grep -q 'waitpid(phpp->php_pid, &wstatus, 0)' php.c; then
 	fail "php_close() must not block in waitpid()"
 fi
 
+# PR #614 replaced the blocking waitpid() above with a bounded-but-still-
+# blocking spin/sleep/SIGTERM/SIGKILL escalation, which reproduced the same
+# poller slowdown this guard exists to prevent. nft_pclose() must reap with a
+# single non-blocking check and hand anything still running to the abandoned-
+# pid sweep instead of spinning/sleeping in the calling thread.
+nft_pclose_body=$(awk '/^nft_pclose\(int fd\)/,/^}/' nft_popen.c)
+
+printf '%s\n' "$nft_pclose_body" | grep -qE 'usleep|NFT_PCLOSE_(TERM|KILL)_ATTEMPTS' &&
+	fail "nft_pclose() must not spin/sleep waiting for a child to exit"
+
+# php_terminate_and_reap() must hold the same invariant: one non-blocking
+# check, then kill and move on.
+php_terminate_body=$(awk '/^static int php_terminate_and_reap/,/^}/' php.c)
+
+printf '%s\n' "$php_terminate_body" | grep -qE 'usleep|for \(' &&
+	fail "php_terminate_and_reap() must not spin/sleep waiting for a child to exit"
+
 echo "PASS: child process safety invariants"
 
 # php_init() has one cleanup path that closes every descriptor it still holds,

tests/unit/test_linked.c

@@ -1042,19 +1042,33 @@ static void test_reap_reports_an_already_reaped_child(void **state) {
 	assert_int_equal(pstat, 0);
 }
 
-static void test_nft_pclose_requests_graceful_termination_before_kill(void **state) {
+static void test_nft_pclose_does_not_block_on_a_lingering_child(void **state) {
 	char ready[6] = {0};
 	int fd;
-	int status;
+	double start;
+	int attempts;
 
 	(void) state;
-	fd = nft_popen("trap 'exit 0' TERM; printf ready; while :; do sleep 1; done", "r");
+	fd = nft_popen("printf ready; while :; do sleep 1; done", "r");
 	assert_true(fd >= 0);
 	assert_int_equal(read(fd, ready, 5), 5);
 	assert_string_equal(ready, "ready");
-	status = nft_pclose(fd);
-	assert_true(WIFEXITED(status));
-	assert_int_equal(WEXITSTATUS(status), 0);
+
+	start = get_time_as_double();
+	assert_int_equal(nft_pclose(fd), -1);
+
+	/* The removed escalating spin/sleep/SIGTERM/SIGKILL sequence took up to
+	 * ~5 seconds here. nft_pclose() must kill the child and hand it to the
+	 * abandoned-pid sweep instead of blocking waiting for it to exit. */
+	assert_true(get_time_as_double() - start < 0.5);
+
+	for (attempts = 0; attempts < 50; attempts++) {
+		if (nft_abandoned_pending() == 0) {
+			break;
+		}
+		usleep(20000);
+	}
+	assert_int_equal(nft_abandoned_pending(), 0);
 }
 
 int main(void) {
@@ -1113,7 +1127,7 @@ int main(void) {
 		cmocka_unit_test(test_reap_returns_still_running_rather_than_blocking),
 		cmocka_unit_test(test_reap_collects_an_exited_child),
 		cmocka_unit_test(test_reap_reports_an_already_reaped_child),
-		cmocka_unit_test(test_nft_pclose_requests_graceful_termination_before_kill),
+		cmocka_unit_test(test_nft_pclose_does_not_block_on_a_lingering_child),
 		cmocka_unit_test(test_abandoned_children_are_swept_and_capacity_is_bounded),
 	};