diff --git a/ralph_loop.sh b/ralph_loop.sh index a7c2f1e0..29e12c2c 100755 --- a/ralph_loop.sh +++ b/ralph_loop.sh @@ -2099,6 +2099,9 @@ execute_claude_code() { # Get PID and monitor progress local claude_pid=$! + # Expose the child to on_signal(): claude_pid is local, and a SIGTERM + # sent to this script does not reach a backgrounded child. + CLAUDE_CHILD_PID=$claude_pid local progress_counter=0 # Early failure detection: if the command doesn't exist or fails immediately, @@ -2174,6 +2177,7 @@ EOF # Wait for the process to finish and get exit code wait $claude_pid exit_code=$? + CLAUDE_CHILD_PID="" fi # Issue #75: pull changed files back from the cloud sandbox BEFORE the @@ -2452,16 +2456,177 @@ cleanup() { # Only record "interrupted" status for abnormal exits (non-zero exit code) # Normal exit (code 0) preserves the status already written by the main loop - if [[ $loop_count -gt 0 && $trap_exit_code -ne 0 ]]; then + # _INTERRUPTED is authoritative for signal paths: in a trap handler $? is the + # status of the last completed command, NOT 128+n, so trap_exit_code alone + # records the interrupt only when the preceding command happened to fail. + if [[ $loop_count -gt 0 && ( $trap_exit_code -ne 0 || "${_INTERRUPTED:-false}" == "true" ) ]]; then log_status "INFO" "Ralph loop interrupted. Cleaning up..." reset_session "manual_interrupt" update_status "$loop_count" "$(cat "$CALL_COUNT_FILE" 2>/dev/null || echo "0")" "interrupted" "stopped" fi - # No exit here — EXIT trap handles natural termination + # No exit here — cleanup() is teardown only, and is called from both the + # normal loop-exit path and on_signal() below. The signal handler owns + # termination; see the warning there. +} + +# ⚠️ WHY STOPPING NEEDS A TREE WALK AND NOT A kill. +# +# `$!` does NOT reliably name the agent. portable_timeout is a shell function, +# so backgrounding it forks a subshell; bash sometimes replaces that subshell +# with an exec of gtimeout and sometimes does not, and which one you get varies +# with the execution context. Measured, mid-work, under a real pty: +# +# loop pid 94095 pgid 94095 <- foreground group; gets the Ctrl-C +# bash pid 94100 pgid 94095 <- $! captured THIS, not the agent +# gtimeout pid 94107 pgid 94107 <- gtimeout setpgid()s ITSELF +# claude pid 94112 pgid 94107 <- the agent, in that other group +# +# Two consequences, both load-bearing: +# - Ctrl-C signals only the foreground group, so the agent never sees it. +# - TERMing $! kills the subshell; gtimeout and the agent are orphaned and +# keep working. Measured: files written 6s, 11s and 17s after the stop, +# and the API call billed in full. +# A `kill -$PGID` is NOT the answer either: the agent is not in our group, so +# that signals the loop itself and spares the agent. Membership must be +# established by parentage before anything is signalled, never assumed. +# +# Targets bash 3.2 (`#!/bin/bash` on macOS): no mapfile, no readarray, no +# associative arrays, no array expansions that break under `set -u`. + +# Print every descendant of $1 (excluding $1), one pid per line, shallowest +# first, proven against the live ppid table. +list_descendants() { + _ld_root="$1" + [ -n "$_ld_root" ] || return 0 + _ld_snapshot=`ps -eo pid=,ppid= 2>/dev/null` + [ -n "$_ld_snapshot" ] || return 0 + + _ld_frontier="$_ld_root" + _ld_found="" + _ld_guard=0 + while [ -n "$_ld_frontier" ]; do + _ld_guard=`expr $_ld_guard + 1` + [ "$_ld_guard" -gt 64 ] && break + _ld_next="" + while read -r _ld_p _ld_pp; do + [ -n "$_ld_p" ] || continue + for _ld_f in $_ld_frontier; do + if [ "$_ld_pp" = "$_ld_f" ]; then + _ld_next="$_ld_next $_ld_p" + break + fi + done + done </dev/null | tr -d ' '` + done + _kt_forbids() { + for _kt_x in $_kt_forbidden; do + [ "$1" = "$_kt_x" ] && return 0 + done + return 1 + } + + # ⛔ ENUMERATE BEFORE SIGNALLING. Once the root dies its descendants are + # reparented to launchd and no later walk can ever find them. + _kt_members="" + for _kt_p in `list_descendants "$_kt_root"`; do + _kt_forbids "$_kt_p" || _kt_members="$_kt_p $_kt_members" + done + _kt_forbids "$_kt_root" || _kt_members="$_kt_members $_kt_root" + [ -n "$_kt_members" ] || return 0 + + for _kt_p in $_kt_members; do kill -TERM "$_kt_p" 2>/dev/null; done + + _kt_waited=0 + _kt_tenths=`expr $_kt_grace \* 10` + while [ "$_kt_waited" -lt "$_kt_tenths" ]; do + _kt_any=no + for _kt_p in $_kt_members; do + if kill -0 "$_kt_p" 2>/dev/null; then _kt_any=yes; break; fi + done + [ "$_kt_any" = "no" ] && return 0 + sleep 0.1 + _kt_waited=`expr $_kt_waited + 1` + done + + # Re-enumerate from SURVIVING members (not the root, which may be gone) to + # catch anything spawned during the grace window. + _kt_late="" + for _kt_p in $_kt_members; do + if kill -0 "$_kt_p" 2>/dev/null; then + for _kt_q in `list_descendants "$_kt_p"`; do + _kt_forbids "$_kt_q" || _kt_late="$_kt_late $_kt_q" + done + fi + done + for _kt_p in $_kt_members $_kt_late; do + kill -0 "$_kt_p" 2>/dev/null && kill -KILL "$_kt_p" 2>/dev/null + done + + sleep 0.3 + for _kt_p in $_kt_members $_kt_late; do + if kill -0 "$_kt_p" 2>/dev/null; then return 1; fi + done + return 0 } # Set up signal handlers -trap cleanup SIGINT SIGTERM +# ⚠️ cleanup() deliberately does NOT exit — it is teardown only. A signal trap +# that merely calls it RETURNS, and bash resumes the main loop: the operator +# sees "interrupted", and the loop keeps running. The handler must terminate +# the process itself, and must also stop the backgrounded Claude child, which +# a SIGTERM sent to this script alone never reaches. +_INTERRUPTED=false +on_signal() { + local sig="$1" + # Restore defaults FIRST so a second ^C always kills, even if cleanup hangs. + trap - SIGINT SIGTERM + _INTERRUPTED=true + log_status "WARN" "Received SIG${sig} — stopping Ralph…" + if [[ -n "${CLAUDE_CHILD_PID:-}" ]] && kill -0 "$CLAUDE_CHILD_PID" 2>/dev/null; then + # NOT `kill -TERM "$CLAUDE_CHILD_PID"`: that pid is usually a wrapper, + # and killing it orphans the agent, which keeps working and keeps + # billing. Walk the tree and terminate every verified member. + if kill_tree "$CLAUDE_CHILD_PID" 5; then + log_status "INFO" "Claude process tree stopped" + else + log_status "ERROR" "⛔ Claude process tree SURVIVED SIGKILL — check for strays before relaunching" + fi + fi + cleanup + # Re-raise so we die of the signal with the correct exit status (128+n). + kill -s "$sig" $$ +} +trap 'on_signal INT' SIGINT +trap 'on_signal TERM' SIGTERM # Global variable for loop count (needed by cleanup function) loop_count=0 diff --git a/tests/integration/test_process_tree_kill.bats b/tests/integration/test_process_tree_kill.bats new file mode 100644 index 00000000..35a041b7 --- /dev/null +++ b/tests/integration/test_process_tree_kill.bats @@ -0,0 +1,173 @@ +#!/usr/bin/env bats +# +# Regression tests for kill_tree() / list_descendants() — the stop control's +# child-kill leg. +# +# The bug these cover: the loop stored `$!` in CLAUDE_CHILD_PID and sent it a +# single SIGTERM. That pid is not reliably the agent. Measured mid-work under a +# real pty: +# +# loop pid 94095 pgid 94095 <- foreground group; receives the Ctrl-C +# bash pid 94100 pgid 94095 <- $! captured THIS, not the agent +# gtimeout pid 94107 pgid 94107 <- gtimeout setpgid()s ITSELF +# claude pid 94112 pgid 94107 <- the agent, in that other group +# +# `portable_timeout` is a shell function, so backgrounding it forks a subshell; +# bash sometimes replaces that subshell with an exec of the timeout binary and +# sometimes does not, varying with execution context. Killing the captured pid +# therefore killed a wrapper and orphaned the agent, which kept working and +# kept billing. A process-group kill is not the answer either — the agent is +# not in the loop's group, so `-$PGID` signals the loop and spares the agent. +# +# These tests use `sleep` trees. They make no API calls and cost nothing. +# +# Each behavioural test that proves kill_tree works is paired with a control +# that must genuinely FAIL first — see "a single-pid TERM leaves the tree +# alive". If that control ever passes, the others prove nothing and it fails +# loudly rather than going green for the wrong reason. +# +# Written for bash 3.2 (`#!/bin/bash` on macOS): no mapfile, no readarray, and +# no array expansion that aborts under `set -u`. + +load ../helpers/test_helper + +RALPH_LOOP="${BATS_TEST_DIRNAME}/../../ralph_loop.sh" + +# Run a snippet with the real ralph_loop.sh sourced, in a subshell so the +# loop's own SIGINT/SIGTERM traps are never installed in the bats shell. +# The functions under test are the ones in the shipped file, not a copy. +in_loop_ctx() { + bash -c "source '$RALPH_LOOP' >/dev/null 2>&1; $1" +} + +@test "list_descendants finds a child and a grandchild through a wrapper" { + run in_loop_ctx ' + bash -c "bash -c \"sleep 30\" & sleep 30" & + root=$! + sleep 1 + n=0 + for p in $(list_descendants "$root"); do n=$((n + 1)); done + kill -KILL $(list_descendants "$root") "$root" 2>/dev/null || true + echo "descendants=$n" + ' + [ "$status" -eq 0 ] + [[ "$output" == *"descendants=2"* ]] +} + +@test "a single-pid TERM leaves the tree alive — the defect this fix exists to stop" { + # KNOWN-FALSE CONTROL. This asserts the OLD behaviour still fails. If this + # ever passes, every kill_tree test below is proving nothing. + run in_loop_ctx ' + bash -c "bash -c \"sleep 30\" & sleep 30" & + root=$! + sleep 1 + kids=$(list_descendants "$root") + kill -TERM "$root" 2>/dev/null # exactly the old child-kill leg + sleep 1 + alive=0 + for p in $kids; do kill -0 "$p" 2>/dev/null && alive=$((alive + 1)); done + kill -KILL $kids 2>/dev/null || true + echo "survivors=$alive" + ' + [ "$status" -eq 0 ] + [[ "$output" == *"survivors=2"* ]] +} + +@test "kill_tree clears the whole tree from a live root" { + run in_loop_ctx ' + bash -c "bash -c \"sleep 30\" & sleep 30" & + root=$! + sleep 1 + kids=$(list_descendants "$root") + kill_tree "$root" 3 + sleep 1 + alive=0 + for p in $root $kids; do kill -0 "$p" 2>/dev/null && alive=$((alive + 1)); done + kill -KILL $root $kids 2>/dev/null || true + echo "alive=$alive" + ' + [ "$status" -eq 0 ] + [[ "$output" == *"alive=0"* ]] +} + +@test "kill_tree reaches a descendant in a DIFFERENT process group" { + # The production shape: the timeout binary puts itself and the agent into + # their own group, so group membership cannot be assumed from the pid the + # loop captured. Parentage is the only test that survives this. + command -v perl >/dev/null || skip "perl required to create a new process group" + run in_loop_ctx ' + bash -c "perl -e \"setpgrp(0,0); exec q(sleep), q(30)\" & sleep 30" & + root=$! + sleep 1 + kids=$(list_descendants "$root") + rootpg=$(ps -o pgid= -p "$root" | tr -d " ") + crossed=no + for p in $kids; do + pg=$(ps -o pgid= -p "$p" 2>/dev/null | tr -d " ") + [ -n "$pg" ] && [ "$pg" != "$rootpg" ] && crossed=yes + done + kill_tree "$root" 3 + sleep 1 + alive=0 + for p in $root $kids; do kill -0 "$p" 2>/dev/null && alive=$((alive + 1)); done + kill -KILL $root $kids 2>/dev/null || true + echo "crossed=$crossed alive=$alive" + ' + [ "$status" -eq 0 ] + # The fixture must actually straddle a group boundary, or it tests nothing. + [[ "$output" == *"crossed=yes"* ]] + [[ "$output" == *"alive=0"* ]] +} + +@test "kill_tree does not kill its own caller" { + # A candidate fix that signalled -$PGID killed the supervisor and left the + # target running. A stop control that kills the supervisor is worse than + # one that does nothing. + run in_loop_ctx ' + bash -c "sleep 30" & + root=$! + sleep 1 + kill_tree "$root" 3 + # The target MUST be dead as well, or this test passes vacuously + # wherever kill_tree does not exist — a test that goes green against + # the unpatched loop is worthless. + if kill -0 "$root" 2>/dev/null; then target=alive; else target=dead; fi + kill -KILL "$root" 2>/dev/null || true + # Cleanup must not be the last command: killing an already-dead pid + # exits non-zero and would fail the test for the wrong reason. + kill -0 $$ 2>/dev/null && echo "caller=alive target=$target" \ + || echo "caller=dead target=$target" + ' + [ "$status" -eq 0 ] + [[ "$output" == *"caller=alive"* ]] + [[ "$output" == *"target=dead"* ]] +} + +@test "kill_tree escalates to SIGKILL past a process that ignores SIGTERM" { + run in_loop_ctx ' + bash -c "trap \"\" TERM; sleep 30" & + stubborn=$! + sleep 1 + kill_tree "$stubborn" 2 + sleep 1 + if kill -0 "$stubborn" 2>/dev/null; then + kill -KILL "$stubborn" 2>/dev/null + echo "stubborn=survived" + else + echo "stubborn=killed" + fi + ' + [ "$status" -eq 0 ] + [[ "$output" == *"stubborn=killed"* ]] +} + +@test "the signal handler stops the tree, not a single pid" { + # Comment lines are excluded deliberately: the handler carries a comment + # naming the old form to explain why it is wrong, and a naive grep counts + # that as the bug still being present. + run bash -c "grep -v '^[[:space:]]*#' '$RALPH_LOOP' | grep -cF 'kill -TERM \"\$CLAUDE_CHILD_PID\"'" + [ "$output" -eq 0 ] + + run bash -c "grep -cF 'kill_tree \"\$CLAUDE_CHILD_PID\"' '$RALPH_LOOP'" + [ "$output" -ge 1 ] +} diff --git a/tests/integration/test_signal_handling.bats b/tests/integration/test_signal_handling.bats new file mode 100644 index 00000000..f160f46a --- /dev/null +++ b/tests/integration/test_signal_handling.bats @@ -0,0 +1,126 @@ +#!/usr/bin/env bats +# +# Regression tests for the SIGINT/SIGTERM stop control. +# +# The bug: `trap cleanup SIGINT SIGTERM` invoked a cleanup() that deliberately +# returns rather than exiting ("No exit here — EXIT trap handles natural +# termination" — but no EXIT trap existed). The handler therefore returned and +# bash resumed the main loop: Ralph logged "Ralph loop interrupted. Cleaning +# up..." and kept running. The _CLEANUP_DONE reentrancy guard then made every +# subsequent signal a silent no-op, so repeated Ctrl-C could not recover. +# +# These tests run the loop with --dry-run, so they make no API calls. +# +# NOTE ON SIGINT: a background job started from a non-interactive shell +# inherits SIGINT as SIG_IGN, and bash cannot re-trap an inherited-ignored +# signal. A naive harness therefore shows the loop surviving Ctrl-C whether or +# not the bug is present. The perl wrapper resets SIGINT to its default +# disposition before exec, which is what an interactive terminal supplies. + +load ../helpers/test_helper + +RALPH_LOOP="${BATS_TEST_DIRNAME}/../../ralph_loop.sh" + +# Build a minimal but valid Ralph project in the per-test temp dir. +setup_ralph_project() { + mkdir -p .ralph/specs src + printf 'Say hello. Do nothing else.\n' > .ralph/PROMPT.md + printf '# fix plan\n- nothing\n' > .ralph/fix_plan.md + printf '# agent\nTest fixture.\n' > .ralph/AGENT.md + printf 'ALLOWED_TOOLS="Read"\n' > .ralphrc +} + +# Start the loop in its own process group with default signal dispositions. +# Sets LOOP_PID. Must NOT be called in a command substitution, or the job +# becomes a child of the subshell and `wait` cannot reach it. +start_loop() { + perl -e '$SIG{INT}="DEFAULT"; $SIG{QUIT}="DEFAULT"; setpgrp(0,0); exec @ARGV' \ + bash "$RALPH_LOOP" --dry-run > loop.log 2>&1 & + LOOP_PID=$! +} + +# Wait until the loop is actually executing an iteration, or fail. +wait_for_loop() { + local waited=0 + while [ "$waited" -lt 40 ]; do + grep -q "DRY RUN" loop.log 2>/dev/null && return 0 + kill -0 "$LOOP_PID" 2>/dev/null || return 1 + sleep 1 + waited=$((waited + 1)) + done + return 1 +} + +# Block until the loop exits, with a hard watchdog so a hung test cannot wedge +# the suite. Sets LOOP_RC to the loop's exit status. `wait` is used rather than +# a kill -0 poll because a dead-but-unreaped child still answers kill -0. +reap_loop() { + ( sleep 15; kill -9 "-$LOOP_PID" 2>/dev/null ) & + local watchdog=$! + # A signalled child returns 128+n; capture it without tripping bats' errexit. + LOOP_RC=0 + wait "$LOOP_PID" 2>/dev/null || LOOP_RC=$? + kill "$watchdog" 2>/dev/null || true + wait "$watchdog" 2>/dev/null || true + return 0 +} + +@test "SIGTERM terminates the loop instead of being swallowed" { + setup_ralph_project + start_loop + wait_for_loop || { kill -9 "-$LOOP_PID" 2>/dev/null; skip "loop did not start"; } + + kill -TERM "$LOOP_PID" + reap_loop + + if [ "$LOOP_RC" -ne 143 ]; then # 128 + SIGTERM + echo "expected exit 143, got $LOOP_RC; log:"; cat loop.log + return 1 + fi +} + +@test "SIGINT terminates the loop instead of being swallowed" { + command -v perl >/dev/null || skip "perl required to reset SIGINT disposition" + setup_ralph_project + start_loop + wait_for_loop || { kill -9 "-$LOOP_PID" 2>/dev/null; skip "loop did not start"; } + + kill -INT "-$LOOP_PID" # process group, as a terminal would + reap_loop + + if [ "$LOOP_RC" -ne 130 ]; then # 128 + SIGINT + echo "expected exit 130, got $LOOP_RC; log:"; cat loop.log + return 1 + fi +} + +@test "repeated signals do not disarm the handler" { + setup_ralph_project + start_loop + wait_for_loop || { kill -9 "-$LOOP_PID" 2>/dev/null; skip "loop did not start"; } + + # Under the reentrancy-guard bug the first signal disarmed cleanup() and + # every later one was a silent no-op. The loop must die on the first. + kill -TERM "$LOOP_PID" + kill -TERM "$LOOP_PID" 2>/dev/null || true + reap_loop + + if [ "$LOOP_RC" -eq 137 ]; then + echo "loop had to be SIGKILLed by the watchdog; log:"; cat loop.log + return 1 + fi +} + +@test "the signal trap is not a bare call to cleanup" { + # cleanup() is teardown only and must never be installed as the handler on + # its own: it returns, and bash then resumes the loop. + run grep -E '^trap[[:space:]]+cleanup[[:space:]]+SIGINT[[:space:]]+SIGTERM' "$RALPH_LOOP" + [ "$status" -ne 0 ] +} + +@test "the child Claude pid is exposed to the signal handler" { + # A SIGTERM addressed to the script does not reach a backgrounded child, + # so the handler needs a non-local pid to stop it. + run grep -q 'CLAUDE_CHILD_PID' "$RALPH_LOOP" + [ "$status" -eq 0 ] +}