From 8d35a6e97ec0f82ae06ff8ec2343d3a10394e858 Mon Sep 17 00:00:00 2001 From: Glenn Jocher Date: Mon, 17 Aug 2026 13:56:58 +0100 Subject: [PATCH] Enforce per-attempt retry timeouts (#879) Co-authored-by: UltralyticsAssistant --- .github/workflows/test-retry.yml | 50 ++++++++- actions/__init__.py | 2 +- retry/README.md | 9 +- retry/action.yml | 170 ++++++++++++++++++++++++++----- 4 files changed, 201 insertions(+), 30 deletions(-) diff --git a/.github/workflows/test-retry.yml b/.github/workflows/test-retry.yml index 7fcc1ec..7bf6caa 100644 --- a/.github/workflows/test-retry.yml +++ b/.github/workflows/test-retry.yml @@ -12,12 +12,19 @@ on: jobs: test-bash: - runs-on: ubuntu-latest + runs-on: ${{ matrix.os }} + defaults: + run: + shell: bash + strategy: + fail-fast: false + matrix: + os: [ubuntu-latest, macos-latest, windows-latest] steps: - uses: actions/checkout@v7 - name: Cleanup any leftover test files - run: rm -f /tmp/retry_attempt /tmp/retry_attempt_py + run: rm -f /tmp/retry_attempt /tmp/retry_attempt_py /tmp/retry_child - name: Test successful command uses: ./retry @@ -26,6 +33,13 @@ jobs: echo "This should succeed first try" true + - name: Test Python shell on Windows + if: matrix.os == 'windows-latest' + uses: ./retry + with: + shell: python + run: print("Python shell works") + - name: Test retry with eventual success uses: ./retry env: @@ -55,6 +69,38 @@ jobs: rm -f /tmp/retry_attempt echo "Success on attempt $((count + 1))!" + - name: Test hung command retry + uses: ./retry + with: + retries: 1 + retry_delay_seconds: 0 + timeout_minutes: 1 + run: | + if [ ! -f /tmp/retry_attempt ]; then + touch /tmp/retry_attempt + if [ "$RUNNER_OS" = "Windows" ]; then + child_file=$(cygpath -w /tmp/retry_child) + powershell.exe -NoProfile -Command "[IO.File]::WriteAllText('$child_file', [string]\$PID); Start-Sleep 120" & + elif [ "$RUNNER_OS" = "Linux" ]; then + python3 -c 'import os, signal, time; os.fork() and os._exit(0); os.setsid(); os.fork() and os._exit(0); open("/tmp/retry_child", "w").write(str(os.getpid())); os.environ.clear(); signal.signal(signal.SIGTERM, signal.SIG_IGN); time.sleep(120)' + sleep 120 & + else + python3 -c 'import os, signal, time; os.fork() and os._exit(0); os.setsid(); os.fork() and os._exit(0); open("/tmp/retry_child", "w").write(str(os.getpid())); signal.signal(signal.SIGTERM, signal.SIG_IGN); time.sleep(120)' + sleep 120 & + fi + wait $! + fi + if [ "$RUNNER_OS" = "Windows" ]; then + powershell.exe -NoProfile -Command "if (Get-Process -Id $(cat /tmp/retry_child) -ErrorAction SilentlyContinue) { exit 0 } else { exit 1 }" && child_alive=true + elif kill -0 "$(cat /tmp/retry_child)" 2>/dev/null; then + child_alive=true + fi + if [ "$child_alive" = true ]; then + echo "::error::Timed-out child is still running" + exit 1 + fi + rm -f /tmp/retry_attempt /tmp/retry_child + - name: Test multi-line failure uses: ./retry continue-on-error: true diff --git a/actions/__init__.py b/actions/__init__.py index a36ddfd..1ece13d 100644 --- a/actions/__init__.py +++ b/actions/__init__.py @@ -28,4 +28,4 @@ # ├── test_summarize_pr.py # └── ... -__version__ = "0.3.9" +__version__ = "0.3.10" diff --git a/retry/README.md b/retry/README.md index 0097f96..fc81685 100644 --- a/retry/README.md +++ b/retry/README.md @@ -29,7 +29,7 @@ steps: python setup.py install pytest tests/ retries: 2 # Retry twice after initial attempt (3 total runs) - timeout_minutes: 30 # Total timeout across all attempts + timeout_minutes: 30 # Maximum time for each attempt retry_delay_seconds: 10 # Base delay between retries backoff: exponential # exponential (10s, 20s, 40s, ...) or fixed jitter: true # Randomize delay to 80-120% to avoid thundering herd @@ -56,7 +56,7 @@ steps: | --------------------- | ------------------------------------------------------------ | -------- | ------------- | | `run` | Command to run | Yes | - | | `retries` | Number of retry attempts after initial run | No | `3` | -| `timeout_minutes` | Maximum total time in minutes, checked between attempts | No | `360` | +| `timeout_minutes` | Maximum time in minutes for each attempt | No | `360` | | `retry_delay_seconds` | Base delay between retries in seconds | No | `10` | | `backoff` | Backoff strategy: `exponential` (base \* 2^n) or `fixed` | No | `exponential` | | `jitter` | Randomize delay to 80-120% of value to avoid thundering herd | No | `true` | @@ -66,6 +66,9 @@ steps: - Preserves environment variables and step context - Exponential backoff with ±20% jitter (best-practice defaults) -- Configurable total timeout across all attempts (individual sleeps auto-capped to remaining budget) +- Configurable per-attempt timeout that terminates hung process trees - GitHub Actions grouping for retry attempts - Supports both Bash and Python shells + +Timeout supervision requires Python 3 on Linux and macOS. macOS descendants that deliberately detach and clear their +inherited environment are outside the platform's portable containment boundary. diff --git a/retry/action.yml b/retry/action.yml index 83fc6bc..b4437fe 100644 --- a/retry/action.yml +++ b/retry/action.yml @@ -16,7 +16,7 @@ # python setup.py install # pytest tests/ # retries: 2 # Retry twice after initial attempt (3 total runs) -# timeout_minutes: 30 # Total timeout for all attempts combined +# timeout_minutes: 30 # Maximum time for each attempt # retry_delay_seconds: 10 # Base delay between retries in seconds # backoff: exponential # exponential (10s, 20s, 40s, ...) or fixed # jitter: true # Randomize delay to 80-120% to avoid thundering herd @@ -26,7 +26,7 @@ name: "Step-Level Retry" description: "Retries a step while preserving its full context" inputs: timeout_minutes: - description: "Maximum total time in minutes for all attempts (checked between attempts; does not interrupt a running attempt)" + description: "Maximum time in minutes for each attempt" required: false default: "360" retries: @@ -38,7 +38,7 @@ inputs: required: false default: "10" backoff: - description: "Backoff strategy: exponential (base * 2^n) or fixed. Delays are bounded by timeout_minutes." + description: "Backoff strategy: exponential (base * 2^n) or fixed" required: false default: "exponential" jitter: @@ -59,12 +59,12 @@ runs: - name: Execute with retry shell: bash env: - # Note: Workflow env vars are automatically inherited. Only RETRY_COMMANDS is added here. + # Note: Workflow env vars are automatically inherited. RETRY_COMMANDS: ${{ inputs.run }} + RETRY_RUNNER_OS: ${{ runner.os }} run: | set +e # Don't exit on error - we handle errors manually - start_time=$(date +%s) timeout_seconds=$(( ${{ inputs.timeout_minutes }} * 60 )) attempt=1 max_attempts=$(( 1 + ${{ inputs.retries }} )) # Initial run + retries @@ -87,6 +87,11 @@ runs: ''|*[!0-9]*) echo "::error::retries, retry_delay_seconds, timeout_minutes must be non-negative integers"; exit 1 ;; esac + if { [ "$RETRY_RUNNER_OS" != "Windows" ] || [ "$shell_type" = "python" ]; } && ! command -v python3 >/dev/null; then + echo "::error::Python 3 is required for timeout supervision on this runner" + exit 1 + fi + # Create temporary script file with appropriate extension if [ "$shell_type" = "python" ]; then if ! TEMP_SCRIPT=$(mktemp --suffix=.py 2>/dev/null); then @@ -110,7 +115,7 @@ runs: fi # Ensure cleanup on exit - trap 'rm -f "$TEMP_SCRIPT"' EXIT + trap 'rm -f "$TEMP_SCRIPT" "${TEMP_SCRIPT}.timeout"' EXIT # Write the user's commands to the temp script printf '%s\n' "$RETRY_COMMANDS" > "$TEMP_SCRIPT" @@ -126,22 +131,145 @@ runs: echo "::group::Retry $((attempt - 1)) of $retries" fi - current_time=$(date +%s) - elapsed=$((current_time - start_time)) - if [ "$elapsed" -gt "$timeout_seconds" ]; then - echo "::error::Step timed out after $timeout_mins minutes" - [ "$attempt" -gt 1 ] && echo "::endgroup::" - exit 1 - fi - - # Execute the script with appropriate interpreter - if [ "$shell_type" = "python" ]; then - python3 "$TEMP_SCRIPT" + # Execute the script with a deadline, terminating its process group and tracked descendants on timeout. + timeout_marker="${TEMP_SCRIPT}.timeout" + rm -f "$timeout_marker" + if [ "$RETRY_RUNNER_OS" = "Windows" ]; then + retry_python="" + [ "$shell_type" = "python" ] && retry_python=$(cygpath -w "$(command -v python3)") + RETRY_EXECUTABLE=$(cygpath -w "$BASH") \ + RETRY_PYTHON="$retry_python" \ + RETRY_SCRIPT=$(cygpath -w "$TEMP_SCRIPT") \ + RETRY_SHELL_TYPE="$shell_type" \ + RETRY_TIMEOUT_MARKER=$(cygpath -w "$timeout_marker") \ + RETRY_TIMEOUT_SECONDS="$timeout_seconds" \ + powershell.exe -NoProfile -Command ' + $startInfo = New-Object System.Diagnostics.ProcessStartInfo + if ($env:RETRY_SHELL_TYPE -eq "python") { + $startInfo.FileName = $env:RETRY_PYTHON + $startInfo.Arguments = "`"$env:RETRY_SCRIPT`"" + } else { + $startInfo.FileName = $env:RETRY_EXECUTABLE + $startInfo.Arguments = "-e `"$env:RETRY_SCRIPT`"" + } + $startInfo.UseShellExecute = $false + $process = New-Object System.Diagnostics.Process + $process.StartInfo = $startInfo + [void]$process.Start() + if (-not $process.WaitForExit([int]$env:RETRY_TIMEOUT_SECONDS * 1000)) { + [System.IO.File]::WriteAllText($env:RETRY_TIMEOUT_MARKER, "") + $allProcesses = @(Get-CimInstance Win32_Process) + $descendants = @() + $parents = @($process.Id) + while ($parents.Count -gt 0) { + $children = @($allProcesses | Where-Object { $parents -contains [int]$_.ParentProcessId }) + $parents = @($children | ForEach-Object { [int]$_.ProcessId }) + $descendants += $parents + } + foreach ($id in @($process.Id) + $descendants) { + taskkill.exe /PID $id /T /F 2>$null | Out-Null + Stop-Process -Id $id -Force -ErrorAction SilentlyContinue + } + exit 124 + } + $process.WaitForExit() + exit $process.ExitCode + ' else - bash -e "$TEMP_SCRIPT" + python3 - "$shell_type" "$TEMP_SCRIPT" "$timeout_seconds" "$timeout_marker" <<'PY' + import os + import signal + import subprocess + import sys + import time + import uuid + + def descendants(root_pid): + """Return all current descendants of a process.""" + children = {} + for entry in os.scandir("/proc"): + if entry.name.isdigit(): + try: + with open(f"{entry.path}/stat") as file: + stat = file.read() + children.setdefault(int(stat[stat.rfind(")") + 2 :].split()[1]), []).append(int(entry.name)) + except (FileNotFoundError, PermissionError, ValueError): + pass + found, pending = set(), [root_pid] + while pending: + for pid in children.get(pending.pop(), []): + if pid not in found: + found.add(pid) + pending.append(pid) + return found + + def tracked_processes(token): + """Return processes carrying the attempt's unique inherited environment marker.""" + marker = f"ULTRALYTICS_RETRY_TOKEN={token}" + processes = subprocess.check_output(["ps", "axeww", "-o", "pid=", "-o", "command="], text=True) + return {int(line.split(None, 1)[0]) for line in processes.splitlines() if marker in line} + + def send_signal(process_group, process_ids, sig): + """Signal the process group and descendants that detached from it.""" + try: + os.killpg(process_group, sig) + except (PermissionError, ProcessLookupError): + pass + for pid in process_ids: + try: + os.kill(pid, sig) + except (PermissionError, ProcessLookupError): + pass + + shell_type, script, timeout, timeout_marker = sys.argv[1], sys.argv[2], int(sys.argv[3]), sys.argv[4] + command = [sys.executable, script] if shell_type == "python" else ["bash", "-e", script] + token = uuid.uuid4().hex + environment = os.environ.copy() + environment["ULTRALYTICS_RETRY_TOKEN"] = token + if sys.platform.startswith("linux"): + import ctypes + + if ctypes.CDLL(None, use_errno=True).prctl(36, 1, 0, 0, 0) != 0: + raise OSError(ctypes.get_errno(), "Unable to enable child subreaper") + process = subprocess.Popen(command, env=environment, start_new_session=True) + process_ids = set() + try: + return_code = process.wait(timeout=timeout) + sys.exit(128 - return_code if return_code < 0 else return_code) + except subprocess.TimeoutExpired: + open(timeout_marker, "w").close() + if sys.platform.startswith("linux"): + process_ids = descendants(os.getpid()) + process_ids |= tracked_processes(token) + send_signal(process.pid, process_ids, signal.SIGTERM) + deadline = time.monotonic() + 5 + while time.monotonic() < deadline: + if sys.platform.startswith("linux"): + process_ids = descendants(os.getpid()) + else: + process_ids = set() + process_ids |= tracked_processes(token) + process_ids = {pid for pid in process_ids if pid != os.getpid()} + if not process_ids: + break + send_signal(process.pid, process_ids, signal.SIGTERM) + time.sleep(0.1) + if sys.platform.startswith("linux"): + process_ids = descendants(os.getpid()) + else: + process_ids = set() + process_ids |= tracked_processes(token) + send_signal(process.pid, process_ids, signal.SIGKILL) + process.wait() + sys.exit(124) + PY fi exit_code=$? + if [ -f "$timeout_marker" ]; then + echo "::error::Attempt timed out after $timeout_mins minutes" + fi + if [ "$exit_code" -eq 0 ]; then [ "$attempt" -gt 1 ] && echo "::endgroup::" exit 0 @@ -176,12 +304,6 @@ runs: [ "$delay" -lt 1 ] && delay=1 # integer division floor; never collapse to 0 fi - # Cap sleep at remaining timeout budget; no point sleeping past deadline. - # Refresh timestamp — the attempt may have taken significant time. - remaining=$((timeout_seconds - ($(date +%s) - start_time))) - [ "$delay" -gt "$remaining" ] && delay=$remaining - [ "$delay" -lt 0 ] && delay=0 - echo "Retrying in $delay seconds..." sleep "$delay" attempt=$((attempt + 1))