stop the daemon instances from piling up

Two defects let a second daemon start beside a running one, and once
that happened the init script could not stop either of them again.

pid_save() wrote its pid over whatever was in brisk.pid, and pid_remove()
deleted the file without looking at whose pid was in it. So an instance
exiting after another one had taken the file over left the survivor
unrecorded, and from then on "stop" found no pid to kill: every restart
added an orphan instead of replacing it. The orphan was not idle - a
starting daemon unlinks the socket files and binds its own, so it takes
every new connection while the old one stays alive on its own shared
memory, and nothing says so.

brisk.pid is now opened once and held under an exclusive non blocking
lock for the whole life of the daemon. The kernel drops the lock when the
process dies, however it dies, so neither a stale file left by a crash
nor two instances starting in the same instant can get through - and
checking the recorded pid for liveness could not have covered the second
case. A daemon that cannot take the lock says who holds it and exits 3.
pid_remove() only unlinks the file while it still holds the lock.

In the init script the pipe into grep was written "\|", so it never was
a pipe: the daemon was run with "|", "grep" and "IN LOOP" as three extra
arguments, and the loop that restarts it watched the wrong exit status.
Harmless in itself - Sac_a_push::create only looks for -d and --daemon -
but the loop never worked as intended and the junk showed up in ps.

"stop" now also quits our screen sessions. Killing the process alone was
never enough, because each session carries the loop that respawns it; and
a session left behind by a start that was refused would sit there and
grab the daemon at the next stop. The loop also sleeps a second between
attempts, so a daemon that cannot start does not spin.

Checked in the container: stop brings a deliberately dirty state (two
daemons, four screens) back to nothing; three restarts in a row leave
exactly one daemon with a pid file that matches it; a second start is
refused naming the holder; a kill -9 leaves the file behind and the next
start takes it over anyway. 100 players with gzip afterwards: no errors,
stream integrity clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014M1jiEq9cHdE5SE5j6vFuE

diff --git a/bin/brisk-init.sh b/bin/brisk-init.sh
index e95805793d523029198a843f701d5b0fae8fbfdf..0c29c0eeceade316ab02df2b70f423eec98782c4 100755 (executable)
--- a/bin/brisk-init.sh
+++ b/bin/brisk-init.sh
@@ -49,6 +49,18 @@ case "$1" in
                 kill -KILL $pid_old 2>/dev/null || true
             fi
         fi
+        #
+        #  Then close our screen sessions. Each one carries the loop that
+        #  respawns the daemon, so killing the process alone is not enough;
+        #  and a session left behind by a start that was refused, because the
+        #  daemon was already running, would wait there and take the daemon
+        #  over at the next stop. That is how the instances used to pile up.
+        su -s /bin/bash - ${BUSER} -c "screen -ls" 2>/dev/null \
+          | sed -n "s/^[[:space:]]*\([0-9][0-9]*\.${SSUFF}\)[[:space:]].*/\1/p" \
+          | while read scr ; do
+                su -s /bin/bash - ${BUSER} -c "screen -S $scr -X quit" >/dev/null 2>&1 || true
+            done
+        su -s /bin/bash - ${BUSER} -c "screen -wipe" >/dev/null 2>&1 || true
         ;;
 
     devstart)
@@ -56,7 +68,7 @@ case "$1" in
         ;;
 
     start)
-        su -s /bin/bash - ${BUSER} -c 'cd '"$BPATH"'/spush ; screen -d -m -S '"${SSUFF}"' bash -c '"'"'while [ 1 ]; do cd . ; ./brisk-spush.php \| grep "IN LOOP" ; if [ $? -eq 0 ]; then break ; fi ; done'"'"
+        su -s /bin/bash - ${BUSER} -c 'cd '"$BPATH"'/spush ; screen -d -m -S '"${SSUFF}"' bash -c '"'"'while [ 1 ]; do cd . ; ./brisk-spush.php | grep "IN LOOP" ; if [ $? -eq 0 ]; then break ; fi ; sleep 1 ; done'"'"
         ;;
     restart)
         $0 stop

diff --git a/web/Obj/sac-a-push.phh b/web/Obj/sac-a-push.phh
index 393490b3fda811e2569522ac3756b136699731fa..41d122e8d53bfcfb050b9231c03d53b6d51232fb 100644 (file)
--- a/web/Obj/sac-a-push.phh
+++ b/web/Obj/sac-a-push.phh
@@ -96,24 +96,60 @@ function global_dump()
 
 }
 
+$G_pid_fp = FALSE;
+
 function pid_save()
 {
-    $pid = getmypid();
+    GLOBAL $G_pid_fp;
+
     $fname = LEGAL_PATH."/brisk.pid";
 
-    if (file_exists($fname)) {
-        log_crit("WARN: brisk.pid already exists");
+    if (($fp = @fopen($fname, 'c+')) == FALSE) {
+        log_crit("REFUSING TO START: cannot open ".$fname);
+        fprintf(STDERR, "REFUSING TO START: cannot open %s\n", $fname);
+        return (FALSE);
     }
-    file_put_contents($fname, sprintf("%d\n", $pid));
+
+    /* The lock is taken for the whole life of the daemon and the kernel drops
+       it when the process dies, however it dies. It is the only check that
+       neither a stale file left by a crash nor two instances starting in the
+       same instant can fool, and both used to get through: the newcomer would
+       unlink the socket files and bind its own, taking every new connection
+       while the first one stayed alive, orphaned, on its own shared memory. */
+    if (flock($fp, LOCK_EX | LOCK_NB) == FALSE) {
+        rewind($fp);
+        $old = intval(trim(stream_get_contents($fp)));
+        log_crit(sprintf("REFUSING TO START: %s is held by %d", $fname, $old));
+        fprintf(STDERR, "REFUSING TO START: %s is held by %d\n", $fname, $old);
+        fclose($fp);
+        return (FALSE);
+    }
+
+    ftruncate($fp, 0);
+    rewind($fp);
+    fwrite($fp, sprintf("%d\n", getmypid()));
+    fflush($fp);
+
+    /* kept open on purpose: closing it would drop the lock */
+    $G_pid_fp = $fp;
+
+    return (TRUE);
 }
 
 function pid_remove()
 {
-    $fname = LEGAL_PATH."/brisk.pid";
+    GLOBAL $G_pid_fp;
 
-    if (file_exists($fname)) {
-        unlink($fname);
+    if ($G_pid_fp == FALSE) {
+        /* the file was never ours: leaving it alone is what keeps the daemon
+           that does own it recorded, and the init script able to stop it */
+        return;
     }
+
+    unlink(LEGAL_PATH."/brisk.pid");
+    flock($G_pid_fp, LOCK_UN);
+    fclose($G_pid_fp);
+    $G_pid_fp = FALSE;
 }
 
 function post_manage(&$post, $line)

diff --git a/web/spush/brisk-spush.php b/web/spush/brisk-spush.php
index 00d3e64c8fb875f858198a624087707683b63217..d2eb027e19e3b8a4cb3738fcf3c5db67ae2281f1 100755 (executable)
--- a/web/spush/brisk-spush.php
+++ b/web/spush/brisk-spush.php
@@ -64,7 +64,9 @@ function main($argv)
 {
     GLOBAL $G_ban_list, $G_black_list, $G_cloud_smasher, $G_provider_proxy;
 
-    pid_save();
+    if (pid_save() == FALSE) {
+        exit(3);
+    }
     do {
         if (($brisk = Brisk::create(LEGAL_PATH."/brisk-crystal.data", $G_ban_list, $G_black_list, $G_cloud_smasher)) == FALSE) {
             log_crit("Brisk::create failed");