flaky: SIGTERM forward assertion races the stub's pidfile write

[twistedmelonman]

Summary

test-gh-wrapper-scope-hint.sh fails intermittently. test-git-env-isolation.sh fails with it, because it re-runs every guarded test in a loop and inherits the failure. One root cause, two red tests.

The failing assertion is always the same:

FAIL: SIGTERM forward: gh (pid unknown) still running after the wrapper died

pid unknown is the tell: the pidfile was empty, so the assertion never had a pid to check. It is not reporting a surviving process — it is reporting that it could not tell either way, and failing closed.

Reproduction

Serially it is hard to hit — 5/5 clean in isolation. Concurrency reproduces it:

for i in 1 2 3 4; do
  ( bash bash/tests/test-gh-wrapper-scope-hint.sh >/dev/null 2>"/tmp/load$i"; echo "run$i rc=$?" ) &
done
wait
grep -h FAIL /tmp/load*

Observed 1 of 4 failing. Whole-suite runs on unchanged code went 25/26 then 26/26 back to back, which is the same flake seen from the runner.

Root cause

Two different files are used as the readiness signal, and only one of them is waited for.

The stub writes the pidfile as its first action (bash/tests/test-gh-wrapper-scope-hint.sh:330-335):

echo "$$" >"${OM_SLEEPER_PIDFILE:?}"
echo 'starting' >&2
sleep 30

Before signalling, the test waits for the errfile to appear (:355-362). But the errfile is created by the wrapper's mktemp, not by the stub — so it can exist before the stub process has run its first line. The test then sends SIGTERM and asserts on the pidfile (:398):

gh_pid="$(cat "${OM_SLEEPER_PIDFILE}" 2>/dev/null)"
if [[ -n "${gh_pid}" ]] && ! kill -0 "${gh_pid}" 2>/dev/null; then

Under load, SIGTERM can land in the window after the wrapper's mktemp but before the stub's echo "$$". The pidfile is still empty, gh_pid is empty, the -n guard is false, and the branch reports a surviving orphan that never existed.

This is a race in the test's readiness check, not a defect in gh-wrapper.sh. The wrapper behaved correctly in every observed failure.

Suggested fix

Wait for the signal the assertion actually depends on. Gate on the pidfile being non-empty before kill -TERM, in the same style as the existing errfile poll:

for _ in $(seq 1 20); do
  [[ -s "${OM_SLEEPER_PIDFILE}" ]] && break
  sleep 0.25
done

Then distinguish the two outcomes the assertion currently merges, so a future regression cannot hide behind an empty pidfile:

if [[ -z "${gh_pid}" ]]; then
  _fail "SIGTERM forward: pidfile never written; assertion could not run"
elif ! kill -0 "${gh_pid}" 2>/dev/null; then
  _pass "SIGTERM forward: gh did not survive the wrapper as an orphan"
else
  _fail "SIGTERM forward: gh (pid ${gh_pid}) still running after the wrapper died"
fi

The file already applies this discipline elsewhere — saw_errfile exists precisely so a clean result cannot "prove nothing" (:353-354). This assertion needs the same treatment.

Notes

Found while adding an unrelated test in #316; not fixed there to keep that PR scoped. Both tests passed in that PR's pre-push hook run, which is consistent with a load-sensitive race rather than a persistent failure.