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..04eeed93 100644 --- a/src/runner/hooks.sh +++ b/src/runner/hooks.sh @@ -339,6 +339,57 @@ 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+. + # + # 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. + 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")" +}