From 4a1e7220695ca4c5875b6df16a090b19910bf306 Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Sun, 16 Aug 2026 03:23:20 +0200 Subject: [PATCH] fix(on-device-install): daemon restart never replaced the running process Three stacked bugs, found and confirmed on real hardware while downgrading a speaker: the new binary landed on disk correctly, but the running service kept reporting the old version indefinitely. - install.sh called `/etc/init.d/aftertouch start`, not `restart`, after installing. start-stop-daemon silently refuses to launch a second instance when one is already running, and the init script never checked its exit status, so the old process was never replaced. - The init script started the daemon through a `sh -c "exec ... | logger"` pipeline, on the assumption that `exec` lets --make-pidfile record the daemon's own PID. POSIX forks each side of a pipe into its own process, so the wrapper shell (not the daemon) was the one actually tracked. `stop` killed the wrapper, which doesn't forward SIGTERM to its children, orphaning the real daemon to keep running and keep holding :8000 forever. - Once the wrapper correctly tracked the daemon's own PID, a further race surfaced: start-stop-daemon's own "already running?" check matched on generic `/bin/sh` identity, so a `restart`'s `start` phase could catch the previous wrapper still mid-teardown and silently refuse to launch a new one (masked by --quiet, looking like a 120s hang). Fixed by calling `restart` instead of `start` in install.sh, and by having the wrapper shell record the daemon's real PID itself (via $!) while keying start-stop-daemon's own check on that same pidfile instead of process identity. Verified on hardware: three consecutive restart cycles, each fast, each with the pidfile matching the live daemon PID and daemon output flowing through syslog again via logread. Co-Authored-By: Claude Sonnet 5 --- scripts/on-device-install/aftertouch | 56 ++++++++++++++++++++++++---- scripts/on-device-install/install.sh | 14 ++++++- 2 files changed, 61 insertions(+), 9 deletions(-) diff --git a/scripts/on-device-install/aftertouch b/scripts/on-device-install/aftertouch index ccb04b3..3a51e10 100644 --- a/scripts/on-device-install/aftertouch +++ b/scripts/on-device-install/aftertouch @@ -42,25 +42,67 @@ case "$1" in mkdir -p "$DATADIR" - # Pipe stdout + stderr through `logger -t $LOG_TAG` so the + # Route stdout + stderr through `logger -t $LOG_TAG` so the # daemon's output lands in busybox syslog (bounded ring buffer, # never grows on disk). Users diagnose with: # # logread | grep aftertouch | tail -20 # logread -f | grep aftertouch # live tail # - # `exec` on the daemon replaces /bin/sh so --make-pidfile records - # the daemon's own PID (not the shell wrapper). The `logger` - # process sits on the read end of the pipe and exits cleanly - # when the daemon dies and closes its end. + # This used to be a `--startas "/bin/sh" -- -c "exec $DAEMON | logger"` + # pipeline, on the theory that `exec` replaces /bin/sh so --make-pidfile + # records the daemon's own PID. That's wrong for a *piped* command: + # POSIX requires each side of a pipe to run in its own forked process, + # so the top-level /bin/sh forks two children (one execs into the + # daemon, one becomes logger) and stays alive itself, blocked in + # wait() -- --make-pidfile recorded *that* wrapper's PID, not the + # daemon's. `stop` then killed the wrapper, which doesn't forward + # SIGTERM to its children, orphaning the real daemon (reparented to + # init) to keep running -- and keep holding :8000 -- forever, silently + # surviving every later stop/start/restart. + # + # A first fix attempt dropped the wrapper shell entirely in favor of + # `--exec "$DAEMON"` directly, with a plain shell-level `>FIFO` + # redirection on the start-stop-daemon invocation. That broke logging + # instead: this busybox's `--background` resets the backgrounded + # child's own stdio, ignoring the outer redirection, so the daemon's + # output never reached the FIFO -- confirmed on hardware (`logger` + # exited immediately with nothing to read, `logread` showed nothing + # new). + # + # This version keeps a wrapper shell -- its *own* FIFO redirection, + # set up by its own script logic rather than inherited from outside, + # isn't affected by whatever --background did to its stdio -- but has + # the wrapper record the daemon's real PID itself instead of trusting + # --make-pidfile. $! after a single, non-piped backgrounded command is + # portably that command's own PID; --make-pidfile can only ever see + # whatever process start-stop-daemon directly forked (the wrapper), + # never a PID from inside it. + LOGFIFO="/tmp/$NAME.fifo" + rm -f "$LOGFIFO" + mkfifo "$LOGFIFO" + + # --pidfile (without --make-pidfile, since the wrapper writes it itself + # once it knows the daemon's real PID) makes start-stop-daemon's own + # "already running?" check keyed on *our* pidfile, not on "/bin/sh" + # identity. Without this, --startas "/bin/sh" is itself the match + # criterion -- and since the wrapper stays alive for the daemon's whole + # lifetime (blocked in its own `wait`), and `stop` only confirms the + # *daemon* PID died (not that the wrapper has finished tearing down), + # a `restart` firing `start` right after `stop` can catch the previous + # wrapper still mid-teardown. start-stop-daemon then silently refuses + # ("/bin/sh is already running", swallowed by --quiet) while the + # script burns its full 120s timeout waiting for a daemon that was + # never launched. Confirmed on hardware: a bare + # `start-stop-daemon --startas "/bin/sh" -- -c "echo hi"` was refused + # with exactly that message while a prior wrapper was still alive. start-stop-daemon --start \ --quiet \ --pidfile "$PIDFILE" \ --background \ - --make-pidfile \ --chuid "$USER" \ --startas "/bin/sh" \ - -- -c "exec \"$DAEMON\" --data-dir '$DATADIR' --record-interactions=false --discovery-interval=60m 2>&1 | logger -t $LOG_TAG" + -- -c "logger -t $LOG_TAG <'$LOGFIFO' & \"$DAEMON\" --data-dir '$DATADIR' --record-interactions=false --discovery-interval=60m >'$LOGFIFO' 2>&1 & echo \$! >'$PIDFILE'; wait" tries=0 max_tries=60 diff --git a/scripts/on-device-install/install.sh b/scripts/on-device-install/install.sh index 1fad690..372ade7 100644 --- a/scripts/on-device-install/install.sh +++ b/scripts/on-device-install/install.sh @@ -138,8 +138,18 @@ mv "$UPDATE_TMP_DIR/init-script" /etc/init.d/aftertouch chmod +x /etc/init.d/aftertouch update-rc.d aftertouch defaults -echo "Installation complete. Running initial startup..." -/etc/init.d/aftertouch start +echo "Installation complete. (Re)starting the service..." +# Use `restart`, not `start`: if AfterTouch is already running (the normal +# case for an in-place upgrade or downgrade), `start` calls start-stop-daemon +# with a pidfile that still points at a live PID. start-stop-daemon then +# refuses to launch a second instance and exits non-zero -- but this script +# has no `set -e` here and never checked that exit status, so the old +# process kept running untouched while the new binary sat unused on disk. +# The post-install curl check below couldn't catch it either, since the old +# process kept answering on :8000 throughout. `restart` stops the old +# process first (a no-op if nothing was running yet, e.g. on a fresh +# install), guaranteeing the newly-installed binary is the one that starts. +/etc/init.d/aftertouch restart /etc/init.d/aftertouch status