Fix PAUSED → RUNNING not stepping dependent services

When a service resumes from PAUSED to RUNNING during reload its
pid condition is reasserted using cond_set_path(), which only
updates the condition file.  Unlike cond_set(), it does not call
cond_update(), so services further down the dependency chain are
never stepped and remain stuck in PAUSED with cond:flux.

Example: in a chain A → B → C → D, touching B and reloading
causes C to go PAUSED → RUNNING when pid/B is reasserted by the
pidfile plugin (via cond_set).  But C's own reassertion of pid/C
used cond_set_path(), so D was never notified and stayed PAUSED.

This bug has been present since dddd45e (Finit 3.2-rc1, 2018),
which added the reassertion but used the lower-level function.

Fixes #476

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

Author: troglobit

src/service.c

@@ -3154,7 +3154,7 @@ int service_step(svc_t *svc)
 
 					mkcond(svc, name, sizeof(name));
 					dbg("Reassert condition %s", name);
-					cond_set_path(cond_path(name), COND_ON);
+					cond_set(name);
 				}
 
 				dbg("Reassert %s ready condition", svc_ident(svc, NULL, 0));

test/dep-chain-reload.sh

@@ -1,17 +1,22 @@
 #!/bin/sh
 # Verify transitive dependency chain during reload and crash
 #
-# Three services in a chain: A → B → C, where B depends on pid/A
-# and C depends on pid/B.  B is placed in a sub-config file so
-# we can use 'initctl touch' on it.  B uses <!pid/svc_a> (with
-# leading '!') so it does not support SIGHUP, causing a full
-# stop/start cycle on 'initctl touch svc_b.conf' + reload.
+# Four services in a chain: A → B → C → D, where B depends on
+# pid/A and C depends on pid/B and D depends on pid/C.  B is
+# placed in a sub-config file so we can use 'initctl touch' on
+# it.  B uses <!pid/svc_a> (with leading '!') so it does not
+# support SIGHUP, causing a full stop/start cycle on 'initctl
+# touch svc_b.conf' + reload.
 #
 # Test 1 - touch + reload:
 #   After 'initctl touch svc_b.conf' + 'initctl reload':
 #     - A should be unaffected (same PID)
 #     - B is restarted (config was touched, noreload)
-#     - C must be restarted (transitive, depends on pid/B)
+#     - C goes PAUSED (pid/B in flux), then PAUSED → RUNNING
+#       once B reasserts pid/B via the pidfile plugin
+#     - D must also resume: C's PAUSED → RUNNING handler must
+#       call cond_set() (not just cond_set_path()) so that
+#       cond_update() steps D out of PAUSED
 #
 # Test 2 - crash (kill -9):
 #   When B is killed with SIGKILL the crash path (RUNNING →
@@ -20,7 +25,7 @@
 #   IN_CLOSE_WRITE, so neither the pidfile removal (IN_DELETE)
 #   nor a pidfile touch (IN_ATTRIB) triggers an inotify event.
 #   Without the fix in service_cleanup(), pid/B is never
-#   invalidated and C is never restarted.
+#   invalidated and C (and D) are never restarted.
 
 set -eu
 
@@ -42,8 +47,11 @@ 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 "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
@@ -57,7 +65,7 @@ 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"
@@ -70,28 +78,32 @@ sep "Test 1: Touch B 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 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"
 
 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
 # 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 2: crash (kill -9), bypasses STOPPING
@@ -100,23 +112,27 @@ 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
 
 return 0