From c387d26e379ac621883db5e702db74495b191d33 Mon Sep 17 00:00:00 2001 From: Chemaclass Date: Fri, 21 Aug 2026 13:33:02 +0200 Subject: [PATCH 1/2] fix(runner): release both teardowns when Ctrl-C stops a parallel run Under `--parallel`, Ctrl-C ran neither the per-test `tear_down` nor the file's `tear_down_after_script`. `main::cleanup` pkills the runner's children, which kills the file worker outright: since #1320 that worker owns the file hook, and the parent cannot run it in its place because several files are in flight and the hook is unset and redefined as the loop advances. The worker now traps the signal itself and settles the debt the parent recorded before dispatch. The debt is cleared before the hook runs, so a signal landing while the worker is already inside its teardown cannot run it twice. Measured rather than assumed, and three things differ from the issue's model: - The test body subshell is a great-grandchild of the runner, not a grandchild (runner, file worker, run_test fork, body subshell, leaf command). - SIGINT cannot be covered here. A shell sets SIGINT to SIG_IGN in a job it backgrounds, and a signal ignored on entry can be neither trapped nor reset, on bash 3.2 and 5.3 alike. Ctrl-C arrives as the SIGTERM the parent pkills with, so TERM is the only disposition worth a handler. - A per-pid TERM does not reach a test body: bash defers the trap until the running foreground command returns, so a body inside the test's own `sleep` never reaches the EXIT trap where `tear_down` lives. Each test therefore gets its own process group, and the handler signals the group, which is the idiom run_with_timeout already uses for the same reason. The handler deliberately does not sweep with `pkill -P $$`. `$$` stays the runner's pid inside a subshell, so the sweep signalled the runner's children, this worker among them, and killed the handler before it reached the hook. Bash 3.2 won that race and bash 5 lost it every time. `BASHPID` would name the worker and is Bash 4+. The cost is that a file opting out of per-test parallelism runs its bodies unforked, with no group of their own, so their `tear_down` is missed. Best effort, as in #1323: a hook that never returns does not hold the run, which still prints its message and exits 1 promptly. Closes #1331 --- CHANGELOG.md | 1 + src/runner/discovery.sh | 10 +++ src/runner/exec.sh | 7 ++ src/runner/hooks.sh | 46 +++++++++++++ tests/unit/runner/worker_cleanup_test.sh | 84 ++++++++++++++++++++++++ 5 files changed, 148 insertions(+) create mode 100644 tests/unit/runner/worker_cleanup_test.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index 322d8c84..ffe0fe5f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ - A test killed by `--test-timeout` runs its `tear_down`, so a per-test resource is released. Best effort within the watchdog's grace before it sends SIGKILL, so a hook cannot outlive the timeout it cleans up after (#1324) - A test file that fails to source sweeps its script temp files, so a `bashunit::temp_file` it created at top level no longer survives the run. `bashunit bench` already did this (#1325) - A malformed `@timeout` or `@retry` runs `tear_down_after_script` before it aborts the run, so the file releases what `set_up_before_script` acquired. Sequential and `--parallel` both leaked it (#1329) +- Ctrl-C releases what an interrupted `--parallel` run acquired: the file's `tear_down_after_script` and the `tear_down` of a test in flight. The worker that owns the file's hook now handles the signal and reaches its test bodies, which a kill from the parent could not (#1331) ## [0.50.0](https://github.com/TypedDevs/bashunit/compare/0.49.0...0.50.0) - 2026-08-18 diff --git a/src/runner/discovery.sh b/src/runner/discovery.sh index f213fddc..22909fbc 100644 --- a/src/runner/discovery.sh +++ b/src/runner/discovery.sh @@ -245,11 +245,21 @@ function bashunit::runner::load_test_files() { # call_test_functions waits for its own per-test workers before it # returns, which is what makes this ordering hold. { + # Ctrl-C reaches this frame as the SIGTERM main::cleanup pkills it with, + # and this is the only frame holding this file's hook (#1331). + trap 'bashunit::runner::cleanup_worker_on_signal' TERM + # A group per test, so the handler above can signal a body subshell + # together with the command it is blocked on. Same reason and same idiom + # as run_with_timeout's watchdog. + set -m # An aborting annotation returns non-zero here, and this shell runs with # errexit off, so the hook below still runs. The status is dropped: the # worker has no channel to abort the parent with, which is a separate # exit-code defect and not this teardown leak (#1329). bashunit::runner::call_test_functions "$test_file" "$_cached_fns" + # Settles the debt the parent recorded before dispatch, so a signal that + # lands from here on cannot run the hook a second time. + _BASHUNIT_FILE_TEARDOWN_PENDING="" bashunit::runner::run_tear_down_after_script "$test_file" # A hook failure recorded in here dies with the subshell (#1147), so # publish it the way a test publishes its result. diff --git a/src/runner/exec.sh b/src/runner/exec.sh index ef0f7803..ea5b064d 100644 --- a/src/runner/exec.sh +++ b/src/runner/exec.sh @@ -139,6 +139,11 @@ function bashunit::runner::call_test_functions() { allow_test_parallel=false fi + # Reset before the first dispatch below: the worker's signal handler group-kills + # every pid recorded here, and a pid left over from the previous file may belong + # to something else by now (#1331). + _BASHUNIT_WORKER_TEST_PIDS="" + # Pre-create the file's result dir before spawning test workers: they all # publish into it, and checking `[ -d ]` inside a worker races its siblings # (every worker would still pay the mkdir fork). @@ -160,6 +165,7 @@ function bashunit::runner::call_test_functions() { _test_ordinal=$((_test_ordinal + 1)) _BASHUNIT_RUNNER_RESULT_ORDINAL=$_test_ordinal bashunit::runner::run_test "$script" "$fn_name" & + _BASHUNIT_WORKER_TEST_PIDS="$_BASHUNIT_WORKER_TEST_PIDS $!" else bashunit::runner::run_test "$script" "$fn_name" fi @@ -206,6 +212,7 @@ function bashunit::runner::call_test_functions() { _test_ordinal=$((_test_ordinal + 1)) _BASHUNIT_RUNNER_RESULT_ORDINAL=$_test_ordinal bashunit::runner::run_test "$script" "$fn_name" ${parsed_data+"${parsed_data[@]}"} & + _BASHUNIT_WORKER_TEST_PIDS="$_BASHUNIT_WORKER_TEST_PIDS $!" else bashunit::runner::run_test "$script" "$fn_name" ${parsed_data+"${parsed_data[@]}"} fi diff --git a/src/runner/hooks.sh b/src/runner/hooks.sh index 30062341..757b65c0 100644 --- a/src/runner/hooks.sh +++ b/src/runner/hooks.sh @@ -339,6 +339,52 @@ function bashunit::runner::run_pending_file_teardown() { bashunit::runner::run_tear_down_after_script "$test_file" } +# The process groups of the tests this worker still has in flight, empty when it +# has none. Written by call_test_functions as it dispatches, read by the handler +# below, which is the only frame that can reach them. +_BASHUNIT_WORKER_TEST_PIDS="" + +## +# The per-file worker's SIGTERM handler. +# +# Since #1320 the worker owns the file's tear_down_after_script, and nothing above +# it can run the hook: several files are in flight under --parallel and the hook is +# unset and redefined as the loop advances, so by interrupt time the parent no +# longer holds the right function body (#1331). +# +# TERM only, and not INT. A shell sets SIGINT to SIG_IGN in a job it backgrounds, +# and a signal ignored on entry can be neither trapped nor reset, so a `trap ... +# INT` in here would be dead code -- measured the same on bash 3.2 and 5.3. Ctrl-C +# reaches this frame as the SIGTERM that main::cleanup pkills it with. +## +function bashunit::runner::cleanup_worker_on_signal() { + # Back to the default disposition first: the hook below is user code and may + # never return, and a handler that cannot itself be interrupted would leave no + # way out but SIGKILL, the same reasoning as main::cleanup. + trap - TERM + # Whole group per test, so the signal reaches the body subshell AND the command + # it is blocked on. Signalling the body alone is not enough: bash defers a trap + # until the running foreground command returns, so a body sitting in the test's + # own `sleep` never reaches the EXIT trap where tear_down lives. The body is a + # great-grandchild of the runner under --parallel, which is why a single + # `pkill -P` from anywhere above cannot do this. + local test_pid + for test_pid in $_BASHUNIT_WORKER_TEST_PIDS; do + kill -TERM -"$test_pid" 2>/dev/null + done + # No `pkill -P $$` sweep to go with the loop. `$$` stays the runner's pid inside + # a subshell, so the sweep signalled the runner's children -- this worker among + # them -- and killed the handler before it reached the hook below. On bash 5 it + # lost that race every time. `$BASHPID` would name the worker, and it is Bash + # 4+. The cost is that a file opting out of per-test parallelism runs its bodies + # unforked, with no group of their own, so their tear_down is missed here. + # + # After the kills, so the per-test tear_down that each body's EXIT trap runs + # comes first, as it does in a normal run. + bashunit::runner::run_pending_file_teardown || true + exit 143 +} + function bashunit::runner::run_tear_down_after_script() { local test_file="$1" bashunit::internal_log "run_tear_down_after_script" diff --git a/tests/unit/runner/worker_cleanup_test.sh b/tests/unit/runner/worker_cleanup_test.sh new file mode 100644 index 00000000..a6badb93 --- /dev/null +++ b/tests/unit/runner/worker_cleanup_test.sh @@ -0,0 +1,84 @@ +#!/usr/bin/env bash + +# bashunit::runner::cleanup_worker_on_signal is the per-file worker's SIGTERM +# handler. Since #1320 the worker owns the file's tear_down_after_script, and the +# parent cannot run it: several files are in flight under --parallel and the hook +# is unset and redefined as the loop advances, so by interrupt time the parent no +# longer holds the right function body (#1331). +# +# Every call runs inside a subshell: the function ends in `exit`, and kill and +# pkill have to be replaced before they run for real. +# +# No signal is sent here. Delivering one to a real run depends on job control and +# on which frame the shell is in when it lands, which under a loaded --parallel +# suite is not reproducible: an acceptance test doing that failed 2 runs in 3 +# while #1323 was being written. These pin the handler's body instead. +# +# Arguments: $1 - order log, $2 - "owed" to record a pending file teardown +# +# The definitions below shadow what the handler calls, so shellcheck sees two +# functions nothing in this file invokes. +# shellcheck disable=SC2329 +function _run_worker_cleanup() { + local order=$1 + local owed=${2:-} + ( + function kill() { printf 'kill %s\n' "$*" >>"$order"; return 0; } + function tear_down_after_script() { printf 'file-teardown\n' >>"$order"; } + + _BASHUNIT_WORKER_TEST_PIDS="111 222" + + if [ "$owed" = owed ]; then + bashunit::runner::mark_file_teardown_pending "some_test.sh" + fi + + bashunit::runner::cleanup_worker_on_signal + ) >/dev/null 2>&1 || true +} + +function test_the_worker_handler_kills_each_test_group_before_the_file_teardown() { + local order + order="$(bashunit::temp_dir worker_cleanup_owed)/order" + + _run_worker_cleanup "$order" owed + + # A negative pid signals the whole group, which is what reaches the test body + # subshell and the command it blocks on. A plain per-pid TERM leaves the body a + # live orphan, and a TERM the body defers behind its own foreground command + # never runs its EXIT trap, which is where tear_down lives. + # + # Kills first, so the per-test tear_down each body's EXIT trap runs comes + # before the file hook, as it does in a normal run. + assert_same "kill -TERM -111 +kill -TERM -222 +file-teardown" "$(cat "$order")" +} + +function test_the_worker_handler_runs_no_file_teardown_when_the_file_owes_none() { + local order + order="$(bashunit::temp_dir worker_cleanup_not_owed)/order" + + _run_worker_cleanup "$order" + + assert_same "kill -TERM -111 +kill -TERM -222" "$(cat "$order")" +} + +function test_the_worker_handler_runs_the_file_teardown_only_once() { + local order + order="$(bashunit::temp_dir worker_cleanup_twice)/order" + + # Settling the debt before the hook runs is what keeps a second delivery, or a + # hook that re-enters this path, from releasing the same resource twice. + ( + function kill() { return 0; } + function tear_down_after_script() { printf 'file-teardown\n' >>"$order"; } + function exit() { :; } + + bashunit::runner::mark_file_teardown_pending "some_test.sh" + bashunit::runner::cleanup_worker_on_signal + bashunit::runner::cleanup_worker_on_signal + ) >/dev/null 2>&1 || true + + assert_same "file-teardown" "$(cat "$order")" +} From d1162a33afa5926f6f8a8ff0840599d01fd03d98 Mon Sep 17 00:00:00 2001 From: Chemaclass Date: Fri, 21 Aug 2026 14:36:21 +0200 Subject: [PATCH 2/2] docs(runner): correct the no-parallel-tests note on the worker handler Measured rather than assumed: such a file runs its bodies unforked, so the worker sits in a command substitution and bash defers the trap until it returns. Both hooks still run, when the body finishes rather than when the signal lands, so nothing is leaked and the interrupt only fails to cut that test short. --- src/runner/hooks.sh | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/runner/hooks.sh b/src/runner/hooks.sh index 757b65c0..04eeed93 100644 --- a/src/runner/hooks.sh +++ b/src/runner/hooks.sh @@ -375,9 +375,14 @@ function bashunit::runner::cleanup_worker_on_signal() { # No `pkill -P $$` sweep to go with the loop. `$$` stays the runner's pid inside # a subshell, so the sweep signalled the runner's children -- this worker among # them -- and killed the handler before it reached the hook below. On bash 5 it - # lost that race every time. `$BASHPID` would name the worker, and it is Bash - # 4+. The cost is that a file opting out of per-test parallelism runs its bodies - # unforked, with no group of their own, so their tear_down is missed here. + # lost that race every time. `$BASHPID` would name the worker, and it is Bash 4+. + # + # A file that opted out of per-test parallelism runs its bodies unforked, so it + # never reaches this handler while a test is running: the worker sits in a + # command substitution, and bash defers a trap until the running foreground + # command returns. Both hooks still run for such a file, when the body finishes + # rather than when the signal lands, so nothing is leaked and the interrupt just + # does not cut that test short. # # After the kills, so the per-test tear_down that each body's EXIT trap runs # comes first, as it does in a normal run.