Dependents not restarted after SIGHUP reload of service

When 'initctl reload' is called after marking a service in a dependency
chain dirty, Finit fails to restart (unfreeze) affected services.

This patch updates the pidfile plugin to watch for IN_ATTRIB changes,
e.g. when a process uses utimensat() to update its pidfile, and adds
service_step_all() at end of reload cycle to guarantee convergence
after conditions are reasserted.

Issue #476

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>

plugins/pidfile.c

@@ -59,7 +59,7 @@ static int pidfile_add_path(struct iwatch *iw, char *path)
 		}
 	}
 
-	return iwatch_add(iw, path, IN_ONLYDIR | IN_CLOSE_WRITE);
+	return iwatch_add(iw, path, IN_ONLYDIR | IN_CLOSE_WRITE | IN_ATTRIB);
 }
 
 static void pidfile_update_conds(char *dir, char *name, uint32_t mask)
@@ -276,6 +276,7 @@ static void pidfile_reconf(void *arg)
 		if (cond_get(cond) == COND_ON)
 			continue;
 
+		dbg("Reassert condition %s", cond);
 		cond_set_path(cond_path(cond), COND_ON);
 	}
 

src/sm.c

@@ -510,15 +510,27 @@ restart:
 		/* Cleanup stale services */
 		svc_clean_dynamic(service_unregister);
 
+		dbg("Step all services waiting for reasserted conditions ...");
+		service_step_all(SVC_TYPE_ANY);
+
 		dbg("Calling reconf hooks ...");
 		plugin_run_hooks(HOOK_SVC_RECONF);
 
+		dbg("Step all services waiting for reasserted conditions ...");
+		service_step_all(SVC_TYPE_ANY);
+
 		dbg("Update configuration generation of device conditions ...");
 		devmon_reconf();
 
+		dbg("Step all services waiting for reasserted conditions ...");
+		service_step_all(SVC_TYPE_ANY);
+
 		dbg("Update configuration generation of unmodified non-native services ...");
 		service_notify_reconf();
 
+		dbg("Step all services waiting for reasserted conditions ...");
+		service_step_all(SVC_TYPE_ANY);
+
 		dbg("Reconfiguration done");
 		sm.state = SM_RUNNING_STATE;
 		break;

test/dep-chain-reload.sh

@@ -1,26 +1,32 @@
 #!/bin/sh
 # Verify transitive dependency chain during reload and crash
 #
-# Three services in a chain: A → B → C, where B depends on pid/A
+# Four services in a chain: A → B → C → D, all using notify:pid.
-# and C depends on pid/B.  B is placed in a sub-config file so
+# B and C are placed in separate sub-config files so we can use
-# we can use 'initctl touch' on it.  B uses <!pid/svc_a> (with
+# 'initctl touch' on them independently.  B supports SIGHUP (reload),
-# leading '!') so it does not support SIGHUP, causing a full
+# while C and D use the '!' condition prefix (noreload), matching
-# stop/start cycle on 'initctl touch svc_b.conf' + reload.
+# a common pattern in real-world setups (e.g., FRR daemons on Infix).
 #
-# Test 1 - touch + reload:
+# Chain: A ← B <pid/A> ← C <!pid/B> ← D <!pid/C>
-#   After 'initctl touch svc_b.conf' + 'initctl reload':
+#
-#     - A should be unaffected (same PID)
+# Test 1 - touch C + reload (the "Infix" scenario):
-#     - B is restarted (config was touched, noreload)
+#   C is in the middle of the chain.  After 'initctl touch svc_c.conf'
-#     - C must be restarted (transitive, depends on pid/B)
+#   + 'initctl reload':
+#     - A and B should be unaffected (same PID)
+#     - C is stopped/started (config was touched, noreload)
+#     - D must be restarted (reverse dep of C)
 #
 # Test 2 - crash (kill -9):
-#   When B is killed with SIGKILL the crash path (RUNNING →
+#   When B is killed with SIGKILL the crash path (RUNNING → HALTED)
-#   HALTED) bypasses STOPPING, where cond_clear() used to be
+#   bypasses STOPPING.  Without cond_clear() in service_cleanup(),
-#   the only call site.  The pidfile plugin only watches for
+#   pid/B is never invalidated and C, D are never restarted.
-#   IN_CLOSE_WRITE, so neither the pidfile removal (IN_DELETE)
+#
-#   nor a pidfile touch (IN_ATTRIB) triggers an inotify event.
+# Test 3 - touch B + reload:
-#   Without the fix in service_cleanup(), pid/B is never
+#   B supports SIGHUP (no '!' prefix).  After 'initctl touch
-#   invalidated and C is never restarted.
+#   svc_b.conf' + 'initctl reload':
+#     - A should be unaffected (same PID)
+#     - B is SIGHUP'd (same PID, config was touched)
+#     - C and D must be restarted (transitive reverse deps)
 
 set -eu
 
@@ -29,7 +35,7 @@ TEST_DIR=$(dirname "$0")
 test_teardown()
 {
     say "Running test teardown."
-    run "rm -f $FINIT_RCSD/svc_b.conf"
+    run "rm -f $FINIT_RCSD/svc_b.conf $FINIT_RCSD/svc_c.conf"
 }
 
 pidof()
@@ -41,9 +47,14 @@ test_setup()
 {
     run "cat >> $FINIT_CONF" <<EOF
 service log:stdout notify:pid               name:svc_a serv -np -i svc_a -- Chain root
-service log:stdout notify:pid <pid/svc_b>   name:svc_c serv -np -i svc_c -- Needs B
+service log:stdout notify:pid <!pid/svc_c>  name:svc_d serv -np -i svc_d -- Needs C
+EOF
+    run "cat >> $FINIT_RCSD/svc_b.conf" <<EOF
+service log:stdout notify:pid <pid/svc_a>   name:svc_b serv -np -i svc_b -- Needs A
+EOF
+    run "cat >> $FINIT_RCSD/svc_c.conf" <<EOF
+service log:stdout notify:pid <!pid/svc_b>  name:svc_c serv -np -i svc_c -- Needs B
 EOF
-    run "echo 'service log:stdout notify:pid <!pid/svc_a> name:svc_b serv -np -i svc_b -- Needs A' > $FINIT_RCSD/svc_b.conf"
 }
 
 # shellcheck source=/dev/null
@@ -52,31 +63,33 @@ EOF
 sep "Configuration"
 run "cat $FINIT_CONF"
 run "cat $FINIT_RCSD/svc_b.conf"
+run "cat $FINIT_RCSD/svc_c.conf"
 
 say "Reload Finit to start all services"
 run "initctl reload"
 
 say "Wait for full chain to start"
-retry 'assert_status "svc_c" "running"' 10 1
+retry 'assert_status "svc_d" "running"' 10 1
 
 run "initctl status"
 run "initctl cond dump"
 
 # ――――――――――――――――――――――――――――――――――――――――――――――――――――――
-# Test 1: touch + reload
+# Test 1: touch C (middle of chain, noreload) + reload
 # ――――――――――――――――――――――――――――――――――――――――――――――――――――――
-sep "Test 1: Touch B and global reload"
+sep "Test 1: Touch C (middle) and global reload"
 
 pid_a=$(pidof svc_a)
 pid_b=$(pidof svc_b)
 pid_c=$(pidof svc_c)
-say "PIDs before: A=$pid_a B=$pid_b C=$pid_c"
+pid_d=$(pidof svc_d)
+say "PIDs before: A=$pid_a B=$pid_b C=$pid_c D=$pid_d"
 
-run "initctl touch svc_b.conf"
+run "initctl touch svc_c.conf"
 run "initctl reload"
 
 say "Wait for chain to settle"
-retry 'assert_status "svc_c" "running"' 15 1
+retry 'assert_status "svc_d" "running"' 15 1
 
 run "initctl status"
 run "initctl cond dump"
@@ -84,14 +97,17 @@ run "initctl cond dump"
 new_pid_a=$(pidof svc_a)
 new_pid_b=$(pidof svc_b)
 new_pid_c=$(pidof svc_c)
-say "PIDs after:  A=$new_pid_a B=$new_pid_b C=$new_pid_c"
+new_pid_d=$(pidof svc_d)
+say "PIDs after:  A=$new_pid_a B=$new_pid_b C=$new_pid_c D=$new_pid_d"
 
 # shellcheck disable=SC2086
 assert "A was not restarted"              $new_pid_a -eq $pid_a
 # shellcheck disable=SC2086
-assert "B was restarted (touched)"        $new_pid_b -ne $pid_b
+assert "B was not restarted"              $new_pid_b -eq $pid_b
 # shellcheck disable=SC2086
-assert "C was restarted (transitive dep)" $new_pid_c -ne $pid_c
+assert "C was restarted (touched)"        $new_pid_c -ne $pid_c
+# shellcheck disable=SC2086
+assert "D was restarted (transitive dep)" $new_pid_d -ne $pid_d
 
 # ――――――――――――――――――――――――――――――――――――――――――――――――――――――
 # Test 2: crash (kill -9), bypasses STOPPING
@@ -100,23 +116,65 @@ sep "Test 2: Kill B with SIGKILL (bypasses STOPPING)"
 
 pid_b=$(pidof svc_b)
 pid_c=$(pidof svc_c)
-say "PIDs before: B=$pid_b C=$pid_c"
+pid_d=$(pidof svc_d)
+say "PIDs before: B=$pid_b C=$pid_c D=$pid_d"
 
 run "kill -9 $pid_b"
 
 say "Wait for B to respawn and chain to settle"
-retry 'assert_status "svc_c" "running"' 15 1
+retry 'assert_status "svc_d" "running"' 15 1
 
 run "initctl status"
 run "initctl cond dump"
 
 new_pid_b=$(pidof svc_b)
 new_pid_c=$(pidof svc_c)
-say "PIDs after:  B=$new_pid_b C=$new_pid_c"
+new_pid_d=$(pidof svc_d)
+say "PIDs after:  B=$new_pid_b C=$new_pid_c D=$new_pid_d"
 
 # shellcheck disable=SC2086
 assert "B was restarted (crashed+respawn)" $new_pid_b -ne $pid_b
 # shellcheck disable=SC2086
 assert "C was restarted (transitive dep)"  $new_pid_c -ne $pid_c
+# shellcheck disable=SC2086
+assert "D was restarted (transitive dep)"  $new_pid_d -ne $pid_d
+
+# ――――――――――――――――――――――――――――――――――――――――――――――――――――――
+# Test 3: touch B (supports SIGHUP) + reload
+# ――――――――――――――――――――――――――――――――――――――――――――――――――――――
+sep "Test 3: Touch B (SIGHUP) and global reload"
+
+pid_a=$(pidof svc_a)
+pid_b=$(pidof svc_b)
+pid_c=$(pidof svc_c)
+pid_d=$(pidof svc_d)
+say "PIDs before: A=$pid_a B=$pid_b C=$pid_c D=$pid_d"
+
+run "initctl debug"
+run "initctl touch svc_b.conf"
+run "initctl reload"
+sleep 2
+run "initctl status"
+
+say "Wait for chain to settle"
+retry 'assert_status "svc_d" "running"' 15 1
+
+run "initctl status"
+run "initctl cond dump"
+
+new_pid_a=$(pidof svc_a)
+new_pid_b=$(pidof svc_b)
+new_pid_c=$(pidof svc_c)
+new_pid_d=$(pidof svc_d)
+say "PIDs after:  A=$new_pid_a B=$new_pid_b C=$new_pid_c D=$new_pid_d"
+
+# shellcheck disable=SC2086
+assert "A was not restarted"              $new_pid_a -eq $pid_a
+# shellcheck disable=SC2086
+assert "B was not restarted (SIGHUP)"     $new_pid_b -eq $pid_b
+# shellcheck disable=SC2086
+assert "C was restarted (transitive dep)" $new_pid_c -ne $pid_c
+# shellcheck disable=SC2086
+assert "D was restarted (transitive dep)" $new_pid_d -ne $pid_d
 
 return 0