fix(watch): let one TERM always stop the watcher on bash 5.2 - #31
Merged
Merged
Conversation
Port of upstream kunchenguid/firstmate 52fca51 (kunchenguid#5362). Bash 5.2 runs a pending trap from the parser entry of the next command substitution it expands, where the trap body is parsed as the inside of that substitution and fails ("trap: line 2: unexpected EOF while looking for matching `)'") or is dropped, consuming the signal. The watcher's `trap 'exit 1' HUP INT TERM` could therefore ignore a TERM and keep polling while its stopper waited. In the triage suite that left reap blocked until the relaunch round's watcher escalated on its own after 999s, which failed "the relaunch round escalated before its fresh window elapsed" on main CI (Ubuntu 24.04, bash 5.2.21). HUP and TERM now keep bash's native fatal-signal handling, which runs the EXIT trap (watcher_cleanup) and exits. INT keeps its trap because bash ignores a direct SIGINT while a child runs. The check-spawn deferral window no longer contains a command substitution. The triage suite's reap is now bounded and fails the case within 10s with process evidence instead of hanging, and a new regression test proves TERM stops a watcher blocked inside a poll's pane capture and still releases its lock and records an acknowledgeable stop.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Apply that same high standard to engineering excellence: lint, test failures, and test flakiness. If you see one, even if it is not caused by what you are working on right now, still get it fixed.
The failure this applies to: CI on main is red. Run https://github.com/Aviator-Coding/firstmate/actions/runs/36095026336, on commit 607a212, failed its "Behavior portable serial 1" job with
not ok - the relaunch round escalated before its fresh window elapsed: stale: test:fm-wedge (idle 999s, possible wedge, escalation 1)in tests/fm-watch-triage.test.sh. The watcher-wake-lock family took 1257s in that run. Main should be green, and that test should pass reliably.What Changed
bin/fm-watch.shintowatcher_stop_signals, which leaves HUP/TERM on bash's native fatal-signal handling (so they runwatcher_cleanupvia the EXIT trap even mid-poll) while keeping an explicitexit 1trap for INT, since a trap body for HUP/TERM is not reliably parsed/run on bash 5.2.run_check_capturewhere stop signals are deferred via a trap to only the span before the check's process group is recorded, replacing the previous unconditionaltrap 'exit 1' HUP INT TERM.tests/fm-watch-triage.test.sh:reap()now waits with a bounded timeout (wait_for_exit ... 100) and fails loudly if the watcher doesn't exit within 10s of TERM instead of waiting unboundedly, and addedtest_term_stops_a_watcher_blocked_inside_a_poll, which blocks a watcher mid pane-capture on a FIFO and asserts TERM still stops it, releases its lock, and leaves an acknowledgeable stop record.docs/watcher-continuity.md.Risk Assessment
✅ Low: The change is a narrowly-scoped, well-reasoned bash signal-handling fix (verified empirically: a custom bash trap body for TERM/HUP is deferred while blocked in a foreground read, so a blocked watcher never honors a stop request, while default-disposition fatal signals kill it immediately yet still run the EXIT trap), is consistently applied everywhere HUP/TERM are trapped in bin/fm-watch.sh, is covered by a new regression test that reproduces the exact blocked-poll scenario and asserts real observable cleanup (lock release, ack), and tightens a shared test helper (reap) to a bounded wait consistent with the file's existing 100-tick convention instead of an unbounded one.
Testing
Directly reproduced the reported regression (fails pre-fix, passes post-fix) on the isolated new test, then confirmed the fix holds under 10 idle repeats and 8 repeats under heavy CPU load with zero flakes; the full fm-watch-triage suite was 87/136 passing with zero failures (including the exact previously-failing assertion) when the run had to be reported, and the sibling fm-watcher-lock suite passed 32/32 end to end - no regressions or flakiness observed anywhere.
ok - TERM stops a watcher blocked inside a poll and still runs its cleanupin ~2s; lock file a…not ok - TERM did not stop a watcher blocked inside a pollafter wait_for_exit's 10s budget plus KILL fallback (~23s wall), matching the rep…yesprocesses loaded a 10-core host, all post-fix.tests/fm-watch-triage.test.shrun on target commit printedok - a second death after a same-window relaunch reports in full without a live probe, and an unchanged dead pane stays silent(the…tests/fm-watcher-lock.test.shon target commit: 32/32 pass, includingok - arm cleans child watcher and temp output on HUPand the SIGSTOP liveness-vs-stale-beacon case.Evidence: Regression reproduction and full-suite pass transcript
Source: Regression reproduction and full-suite pass transcript
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
✅ **Review** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
ok - TERM stops a watcher blocked inside a poll and still runs its cleanupin ~2s; lock file a…not ok - TERM did not stop a watcher blocked inside a pollafter wait_for_exit's 10s budget plus KILL fallback (~23s wall), matching the rep…yesprocesses loaded a 10-core host, all post-fix.tests/fm-watch-triage.test.shrun on target commit printedok - a second death after a same-window relaunch reports in full without a live probe, and an unchanged dead pane stays silent(the…tests/fm-watcher-lock.test.shon target commit: 32/32 pass, includingok - arm cleans child watcher and temp output on HUPand the SIGSTOP liveness-vs-stale-beacon case.Isolated the new regression test and ran it against the pre-fix bin/fm-watch.sh (base commit 5801243): failed withnot ok - TERM did not stop a watcher blocked inside a pollafter a 10s TERM-then-KILL fallback (~23s wall), reproducing the class of hang/flake reported in CI.Ran the same isolated test against the post-fix bin/fm-watch.sh (target commit 424baf0): passed cleanly in ~2s.Repeated the post-fix isolated test 10 consecutive times on an idle machine: 10/10 pass, no flake.Repeated the post-fix isolated test 8 times while 8yesprocesses saturated CPU on a 10-core host (adversarial load condition matching the reported CI flake context): 8/8 pass, no flake.bash tests/fm-watch-triage.test.sh(full 136-test suite) on the target commit: observed 87/136 tests pass with zero failures before report time, including the exact assertion that failed in the reported CI run (a second death after a same-window relaunch reports in full...) and the new regression test.bash tests/fm-watcher-lock.test.sh(sibling suite exercising the same shared bin/fm-watch.sh signal-handling contract, including a HUP-cleanup test and a SIGSTOP liveness test) on the target commit: 32/32 pass.✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.