From debf14bfee450e59a38de128ae7ef9b77cc4316d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 17:37:30 +0000 Subject: [PATCH] Stop the chains on the way out, and update the actions The macOS jobs were not hanging in the tests: the suite finished in six seconds with all examples passing, and then the step sat there until the fifteen-minute job timeout cancelled it. What held it open was a process the suite left behind. The macOS runner does not finish a step while a process that step started is still alive, and it says so at cleanup: Terminate orphan process: pid (22066) (sleep) Terminate orphan process: pid (1682) (bash) The stray comes from the library. A signal ran _finish, which removed the temp directory and exited without touching the chains, so the interrupt test left a chain shell running with nothing to stop it. That is a bug in its own right, since the point of Ctrl-C on a build is that the compiler stops too, so _finish now kills whatever has not reported a status before it cleans up. The Linux runners never minded the stray, which is why this only ever showed on macOS. The fakes that stay busy waited on $WORK/done, a marker teardown deletes moments after it appears, so a fake mid-nap waited on a file that could no longer arrive. They now stop when $WORK goes as well, which is the same signal and cannot be missed. Also, a driver that hangs is now killable: backgrounding start_driver made $! the wrapper subshell rather than the driver, so the watchdog was killing the wrapper and orphaning the driver it meant to stop. The background copy execs, so the pid the watchdog gets is the driver's own, and a timed-out driver says so instead of failing on a mystery status. Alongside that, the workflow moves to actions/checkout@v7, which clears the Node 20 deprecation warning, and lint goes green again: shellcheck needed a source directive to follow the example into the library, shfmt wanted a trailing newline, and checkbashisms read the space inside $(dirname "$0") as a second argument to the dot. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016ZzPaXJV5SX4DKtpQBmJkT --- .github/workflows/ci.yml | 8 ++--- .shellcheckrc | 3 ++ examples/build.sh | 10 ++++-- parallel.sh | 18 ++++++++++ spec/parallel_spec.sh | 29 ++++++++++++++-- spec/spec_helper.sh | 73 +++++++++++++++++++++++++++++++++++----- 6 files changed, 124 insertions(+), 17 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b223f81..880f465 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -12,7 +12,7 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 10 steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v7 - name: Install linters run: | sudo apt-get update @@ -46,7 +46,7 @@ jobs: package: busybox-static path: /usr/local/bin/ash steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v7 - name: Install ${{ matrix.shell }} run: | sudo apt-get update @@ -68,7 +68,7 @@ jobs: matrix: shell: [/bin/sh, /bin/bash] steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v7 - run: make install-dev - run: make test SHELLSPEC_FLAGS="--shell ${{ matrix.shell }}" @@ -76,7 +76,7 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 30 steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v7 - uses: vmactions/freebsd-vm@v1 with: usesh: true diff --git a/.shellcheckrc b/.shellcheckrc index 91ca89c..f655a6d 100644 --- a/.shellcheckrc +++ b/.shellcheckrc @@ -5,3 +5,6 @@ shell=sh # Follow `. parallel.sh` into the library when a source= directive says where # it is, so the example build script is checked as a whole. external-sources=true + +# Resolve `source=` directives relative to the script that carries them. +source-path=SCRIPTDIR diff --git a/examples/build.sh b/examples/build.sh index 04477f4..d71ecf8 100755 --- a/examples/build.sh +++ b/examples/build.sh @@ -1,10 +1,16 @@ #!/bin/sh set -u -. "$(dirname "$0")/../parallel.sh" +# The directory comes out into a variable first so that the dot has a single +# word after it: checkbashisms reads the space inside the substitution as a +# second argument to `.`, which really would be unportable, and shellcheck +# needs the directive to know which file this is. +_here=$(dirname "$0") +# shellcheck source=../parallel.sh +. "$_here/../parallel.sh" chain "composer install --no-dev --no-interaction --prefer-dist --optimize-autoloader" chain "npm ci --audit false" "npm run build" -run \ No newline at end of file +run diff --git a/parallel.sh b/parallel.sh index ad3fa49..7a0921f 100644 --- a/parallel.sh +++ b/parallel.sh @@ -34,10 +34,28 @@ _finish() { 0) ;; *) _ec=$_signal ;; esac + _stop_chains _cleanup exit "$_ec" } +# Take the chains down on the way out. Nothing else does: a chain is its own +# process, so an interrupted build that only cleaned up after itself would +# leave the compiler it started still running, and a script that gave up +# before it reached `run` would leave the lot. Every chain that reported a +# status is finished already, and killing a process that has gone is not an +# error worth reporting, so this cannot fail and cannot abandon the trap +# that called it. +_stop_chains() { + i=1 + while [ "$i" -le "$_count" ]; do + if [ ! -f "$_work/$i.code" ] && [ -f "$_work/$i.pid" ]; then + kill "$(cat "$_work/$i.pid")" 2>/dev/null || : + fi + i=$((i + 1)) + done +} + trap '_finish $?' EXIT trap '_signal=130; _finish 130' INT trap '_signal=143; _finish 143' TERM diff --git a/spec/parallel_spec.sh b/spec/parallel_spec.sh index df80cc6..408b192 100644 --- a/spec/parallel_spec.sh +++ b/spec/parallel_spec.sh @@ -124,7 +124,7 @@ Describe 'parallel.sh' #|. "$LIB" #|chain "slow" \ #| "touch '$WORK/slow-started'" \ - #| "until [ -f '$WORK/done' ]; do sleep 1; done" \ + #| "until [ -f '$WORK/done' ] || [ ! -d '$WORK' ]; do sleep 1; done" \ #| "touch '$WORK/slow-finished'" #|chain "quick" \ #| "until [ -f '$WORK/slow-started' ]; do sleep 1; done" \ @@ -249,7 +249,7 @@ Describe 'parallel.sh' #|printf '%s\n' "$$" >"$WORK/pid" #|chain "slow" \ #| "touch '$WORK/started'" \ - #| "until [ -f '$WORK/done' ]; do sleep 1; done" + #| "until [ -f '$WORK/done' ] || [ ! -d '$WORK' ]; do sleep 1; done" #|run End @@ -257,5 +257,30 @@ Describe 'parallel.sh' The status should equal 130 The directory "$(reported_workdir)" should not be exist End + + # A signal is the one way out that the cancel path never sees, so + # unless the library stops the chains itself an interrupted build + # leaves whatever it started still running. That is its own bug — + # the point of Ctrl-C is that the compiler stops too — and it is + # also what stalls CI: a macOS runner does not finish a step while + # a process the step started is alive, so a chain nobody killed + # holds the job open until it times out, minutes after the suite + # has passed. + It 'stops the chain it started when it is interrupted' + Data + #|set -eu + #|. "$LIB" + #|printf '%s\n' "$$" >"$WORK/pid" + #|chain "slow" \ + #| "touch '$WORK/started'" \ + #| "until [ -f '$WORK/done' ] || [ ! -d '$WORK' ]; do sleep 1; done" + #|printf '%s\n' "$!" >"$WORK/chainpid" + #|run + End + + When call driver_interrupted + The status should equal 130 + The value "$(chain_state)" should equal "stopped" + End End End diff --git a/spec/spec_helper.sh b/spec/spec_helper.sh index 303996b..3d6660d 100644 --- a/spec/spec_helper.sh +++ b/spec/spec_helper.sh @@ -48,6 +48,18 @@ start_driver() { "$SHELLSPEC_SHELL" "$WORK/driver.sh" 3>&- 4>&- 5>&- 6>&- 7>&- 8>&- 9>&- } +# The same driver, but for the background, and `exec` is the whole point of +# the second copy. `start_driver &` forks a subshell that then waits on the +# driver, so $! names the subshell and not the driver: killing it reaps the +# wrapper and leaves the driver running, holding the pipes shellspec reads +# the example's output from. The watchdog below then has nothing to kill, +# and one driver that hangs hangs the whole suite instead of failing its own +# example. Replacing the subshell with the driver keeps the pid the same one +# the watchdog was given. +spawn_driver() { + exec "$SHELLSPEC_SHELL" "$WORK/driver.sh" 3>&- 4>&- 5>&- 6>&- 7>&- 8>&- 9>&- +} + detach() { "$@" >/dev/null 2>&1 3>&- 4>&- 5>&- 6>&- 7>&- 8>&- 9>&- & } @@ -56,12 +68,17 @@ detach() { # being tested. $LIB and $WORK are in its environment. # # When the driver is finished, $WORK/done appears. A fake command that has to -# stay busy for a while waits on that file, so a stray goes away on its own -# shortly after the example that made it. +# stay busy for a while waits for that file to arrive or for $WORK itself to +# go, so a stray goes away on its own shortly after the example that made it. +# Waiting on the file alone is not enough: teardown takes $WORK away moments +# after the marker lands, and a fake that was mid-nap then waits forever for +# a file that can no longer appear. The macOS runner does not finish a step +# while a process it started is still alive, so one immortal stray is a +# fifteen-minute job timeout there, long after the suite itself has passed. driver() { cat >"$WORK/driver.sh" - rm -f "$WORK/done" - start_driver & + rm -f "$WORK/done" "$WORK/timed-out" + spawn_driver & _child=$! detach watchdog "$_child" _watchdog=$! @@ -69,6 +86,7 @@ driver() { _status=$? : >"$WORK/done" stop "$_watchdog" + report_timeout return "$_status" } @@ -81,16 +99,25 @@ driver() { # driver publishes its own pid for the helper to aim at. driver_interrupted() { cat >"$WORK/driver.sh" - rm -f "$WORK/done" + rm -f "$WORK/done" "$WORK/timed-out" detach interrupter _interrupter=$! start_driver _status=$? : >"$WORK/done" stop "$_interrupter" + report_timeout return "$_status" } +# Say so when a driver had to be killed. Without this the example fails on a +# status it never chose, which reads like the library returning the wrong +# code rather than the driver never returning at all. +report_timeout() { + [ -e "$WORK/timed-out" ] || return 0 + printf 'driver did not finish within %ss and was killed\n' "$DRIVER_TIMEOUT" >&2 +} + # Shut a helper down and reap it. Left running, it would still be a child of # the shell shellspec waits on at the end of the example, and every example # would pay the full timeout. @@ -100,15 +127,25 @@ stop() { return 0 } +# The driver here runs in the foreground, so this helper is the only thing +# that can end it: a driver that never reports it started, or that sits +# through the interrupt, has to be killed anyway rather than left to run. interrupter() { - await_file "$WORK/started" || return 0 - kill -INT "$(cat "$WORK/pid")" 2>/dev/null - countdown && kill -9 "$(cat "$WORK/pid" 2>/dev/null)" 2>/dev/null + if await_file "$WORK/started"; then + kill -INT "$(cat "$WORK/pid")" 2>/dev/null + elif [ -e "$WORK/done" ]; then + return 0 # it finished on its own; there is nothing to interrupt + fi + countdown || return 0 + : >"$WORK/timed-out" + kill -9 "$(cat "$WORK/pid" 2>/dev/null)" 2>/dev/null } # Kill a driver that outstays its welcome. watchdog() { - countdown && kill -9 "$1" 2>/dev/null + countdown || return 0 + : >"$WORK/timed-out" + kill -9 "$1" 2>/dev/null } # Sleep out the timeout a second at a time, stopping early once the driver is @@ -140,6 +177,24 @@ reported_workdir() { cat "$WORK/workdir" } +# Whether the chain a finished driver reported through $WORK/chainpid is +# still running. A process that has just been killed can sit as a zombie +# until the shell that started it goes and init reaps it, so this waits a +# few seconds for an answer rather than believing the first one. +chain_state() { + _pid=$(cat "$WORK/chainpid") + _waited=0 + while kill -0 "$_pid" 2>/dev/null; do + if [ "$_waited" -ge 5 ]; then + printf 'running' + return 0 + fi + _waited=$((_waited + 1)) + sleep 1 + done + printf 'stopped' +} + lines_in() { wc -l <"$1" | tr -d ' ' }