Skip to content

Bound shell test suites so a hang reads as a failure - #110

Merged
b-macker merged 1 commit into
masterfrom
claude/naab-inadmissible-action-prevention-4cmn1m
Jul 31, 2026
Merged

Bound shell test suites so a hang reads as a failure#110
b-macker merged 1 commit into
masterfrom
claude/naab-inadmissible-action-prevention-4cmn1m

Conversation

@b-macker

Copy link
Copy Markdown
Owner

Summary

run-all-tests.sh invokes 94 nested test suites as bare bash <script>. Unbounded, one hung suite stalls the entire run until the CI runner gives up — the Windows job on #109 spent ~50 minutes in CLI tests and produced no logs at all, because a killed runner never uploads them.

That is the worst shape a CI failure can take: expensive, and indistinguishable from a runner fault. It got misread as a real regression in the change under test, and only a re-run settled it. A hang is a test failure and should read like one.

Changes

  • run-all-tests.sh — new run_shell_test() wrapper; all 94 if bash "$X_SCRIPT" call sites converted mechanically to if run_shell_test "$X_SCRIPT". Arguments pass to bash unchanged and the suite's exit status returns unchanged, so every pass/fail branch behaves exactly as before — only the bound is new.
  • SIGTERM first, SIGKILL after a grace period, in that order deliberately. The suites install EXIT traps that kill stub servers and restore trust-store state, and TERM lets those run. -k exists for the case that motivated this: a native Windows binary under MSYS2 that never sees TERM, where plain timeout waits forever.
  • timeout presence and -k support are probed once, with graceful fallback. timeout is already a hard dependency of run_test() for .naab tests, but -k is not universal (older busybox), and this wrapper should not be the thing that breaks an exotic platform.
  • Tunable via SHELL_TEST_TIMEOUT / SHELL_TEST_KILL_AFTER.

Choosing the bound

Timed all 99 suites over a full run rather than guessing. Median under 1s, 29 suites above 10s, and a tail set almost entirely by one suite:

95s  test_prescan_canaries.sh   <- the pace-setter, and it runs last
29s  test_propose_commit.sh
21s  test_signal_discrimination.sh
19s  run_property_tests.sh / run_fuzz_smoke.sh

600s is ~6x the 95s ceiling, which holds even if a Windows runner is 2–3x slower than the machine measured. A partial sample taken before the canary suite runs shows a ceiling of 29s and invites a 300s bound that a slow runner would trip on a perfectly healthy run — the comment in the file records why not to tighten it there, since a bound that fails on healthy runs just teaches everyone to raise it.

Deliberately excluded

test_absorption_degenerate.sh already carries its own tighter 120s bound and its own policy of SKIPPING on timeout rather than failing — a knowingly tolerated flake. Routing it through this wrapper would silently convert it into a red build, so it is left alone and the reason is recorded.

Test Plan

  • Ran bash run-all-tests.sh with no new failures — 441 tests, 0 unexpected failures (376 passed, 52 error-behavior, 1 missing-executor, 12 needs-tree-walk)
  • Added/updated tests for new functionality — n/a for the suite runner itself; the wrapper was verified directly instead (below)
  • Tested manually in the REPL — n/a, this is the test harness

The timeout path was exercised rather than assumed. Driving the real wrapper (extracted verbatim) against synthetic suites at a 3s bound:

case result
exits 0 pass-branch — status propagation intact
exits 7 fail-branch, no spurious TIMEOUT line
hangs killed at the bound, TIMEOUT reported, counted as a failure
hangs with an EXIT trap prints its cleanup before dying — SIGTERM-first works
ignores SIGTERM dies at the grace deadline via -k, not at 300s

The last two carry the design claims: the trap case is what keeps stub servers from leaking, and the TERM-ignoring case is the MSYS2 scenario this change exists for.

Related Issues

Follow-up to #109, whose Windows job surfaced the gap.


Generated by Claude Code

run-all-tests.sh invokes 94 nested suites as bare `bash <script>`. Unbounded,
one hung suite stalls the entire run until the CI runner gives up: the Windows
job spent ~50 minutes in `CLI tests` and produced no logs at all, because a
killed runner never uploads them. The failure was indistinguishable from a
runner fault, which is how it got misread as one.

All 94 sites now go through run_shell_test(), which passes arguments to bash
unchanged and returns the suite's exit status unchanged — every pass/fail
branch behaves exactly as before, only the bound is new.

SIGTERM first, SIGKILL after a grace period, in that order deliberately. The
suites install EXIT traps that kill stub servers and restore the trust store,
and TERM lets those run; verified directly, a trap-installing suite prints its
cleanup before dying. -k exists for the case that motivated this — a native
Windows binary under MSYS2 that never sees TERM — and a TERM-ignoring suite
dies at the grace deadline rather than running to completion.

The bound is 600s, ~6x the measured ceiling. Timing all 99 suites over a full
run put the median under 1s and the tail almost entirely on one suite:
test_prescan_canaries.sh at 95s, then 29s, 21s, 19s. Sampling before the canary
suite runs shows a ceiling of 29s and invites a 300s bound that a slower runner
would trip on a healthy run; the comment records why not to tighten it there.

test_absorption_degenerate.sh is deliberately excluded. It already carries its
own 120s bound and its own policy of SKIPPING on timeout rather than failing —
routing it through the wrapper would convert a knowingly tolerated flake into a
red build.

Full suite: 441 tests, 0 unexpected failures.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ELUfjXZvx8kzXo1UJjrAhC
@github-actions

Copy link
Copy Markdown

NAAb Governance Report

Metric Count
Files checked 16
Passed 16
Failed 0

All governance checks passed!

Generated by NAAb Governance Engine v4.0

@b-macker
b-macker marked this pull request as ready for review July 31, 2026 23:50
@b-macker
b-macker merged commit 63180b1 into master Jul 31, 2026
23 checks passed
@b-macker
b-macker deleted the claude/naab-inadmissible-action-prevention-4cmn1m branch July 31, 2026 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants