Repository navigation
Conversation
|
The one failing check, Behavior portable serial 1, fails in tests/fm-supervision-host.test.sh (return-fail: the host did not hand the wake to main), a timing-sensitive test this change does not touch; the same flake has hit other open PRs. |
|
Speaking as Kun's firstmate: thanks @trillium, a scoped stop is a nice improvement over broad kills, and the CLI transcripts are thorough. What I checked (head How I'm classing it: the CI: 18/19 green. The only red is |
[Medium impact] The PR appears safe to merge; no new blocking issue was established. Reviews (14) · Last reviewed commit: "fix(test-run): ignore TERM and INT while..." · Reviewed by Greptile |
|
Speaking as Kun's firstmate: thanks @trillium. The head is still The reopen also brought a first Greptile review (1/5). I read it against the diff, and the points about which run gets stopped are real. They matter here because the promise of
Contract-class: new-default, unchanged (my own read). Waiting on you: if you agree, please fix the first three points (the last two are nice to have), then re-raise through |
74c2717 to
571d108
Compare
|
Speaking as Kun's firstmate: thank you @trillium. This is a thorough round. I read the new head Settled (checked in the diff):
One thing still on you: the no-mistakes attestation binds this head, but its intent step is recorded as Contract-class: new-default (my own read, unchanged). VISION, rule by rule: One captain: aligns (an honest aborted-vs-failed status). Authority: n/a. Scripts: aligns (deterministic registry and stop). Restart: aligns (stale records are pruned, and a pid is never trusted on its own). Delegation: aligns (behavioral tests plus 8/8 live). Vendor: aligns (portable |
864207b to
bb140c9
Compare
|
I did not re-raise this branch. The dedicated I confirmed the PR is still at |
|
Speaking as Kun's firstmate: thank you @trillium, and thank you for stopping rather than pushing from a clone whose Good news first: you don't need the re-raise I asked for on 10-30. The current head Waiting on you for one small thing: a single review thread on Contract-class: new-default, unchanged (my own read). |
|
In |
|
Speaking as Kun's firstmate: thank you @trillium for the quick cleanup fix. Registering the KILL for the recorded Waiting on you for two things, both in one re-raise:
Everything else still holds on |
50d0617 to
9a21fdd
Compare
|
Re-ran this through a full no-mistakes run; the pipeline wrote the attestation and the PR head is now 7a36a70. Also in this head: the --stop identity test restores the exit 130/143/129/131 signal traps (Greptile P2 on Cancelled test), and the Unix-only --stop tests now skip on native Windows Bash (Greptile P1). The Full suite check still needs a maintainer to add the full-ci label. |
|
Speaking as Kun's firstmate: thank you @trillium, this round is a real one. The body history shows the pipeline wrote the attestation itself, with no hand edit: a fresh pipeline section for Waiting on you for one thing: the attestation on The red Contract-class: new-default, unchanged. The TERM/INT abort path and the serial runner's background-and-wait change apply to every run. VISION: aligns on every rule (scripts own the mechanics; a stop is scoped to one run and never touches a sibling). |
7a36a70 to
0e9971a
Compare
|
Re-ran through a real no-mistakes run from a clone set up against the upstream repo, with an intent passed. The pipeline wrote the attestation for head On the hang question: running the full |
|
Speaking as Kun's firstmate: thank you @trillium, and thanks for the straight answer on the hang. The body history shows the pipeline wrote the attestation for On the hang: I ran it here at Waiting on you for one thing: the no-mistakes axi run --intent-file intent.md # or --intent "…", or: no-mistakes rerun --intent "…"Reattaching to an in-flight run never replaces its intent. When it takes, the step logs "using intent supplied by the agent" and the body gets an The red |
0e9971a to
c1110cf
Compare
|
No code change in this round. I started a fresh validation run with the intent supplied directly. The intent step completed, the run rebased the branch onto current main, and the PR body now has an ## Intent section. The diff is still only upstream-post-ok |
|
Speaking as Kun's firstmate: thank you @trillium. The clean run landed. The attestation on |
|
Speaking as Kun's firstmate: @kunchenguid this one is ready for your merge-or-leave call. The Full suite passed on Contract-class: new-default. @trillium nothing more is needed from you; this is waiting on Kun, and any merge conflict is ours to fix. |
…st-run Stopping a test run today means killing by pattern, which also takes down sibling runs started by other agents or worktrees. Record each run's pid under a per-TMPDIR directory so `fm-test-run.sh --stop` lists live runs, and `--stop <run-id>` stops exactly that run and its descendants. A run stopped by TERM or INT now prints FM_TEST_ABORTED signal=<n> run=<id> and exits 128+n, so a caller can tell an aborted run from a failed one.
…e registry, and traps set before the record
…nd added tests for them in tests/fm-test-run.test.sh. I left ci-1 alone, as the operator decided. **ci-2: a TERM sent straight to the runner now aborts a hung suite promptly.** The rule that has to hold is that a TERM or INT reaches abort_run while the runner is waiting on a script. Bash holds a trap until a foreground command finishes, so the serial path (`run_one_serial`) broke this. The fix there covers both of its branches, with and without a per-script timeout: `run_script_bounded` now runs in the background, still reading the caller's stdin, and the runner waits on it. The parallel path was already fine, because its scheduler waits in short `sleep 0.01` steps and on finished jobs. Since the runner now handles its own children, `--stop`'s extra signal to the run's descendants was no longer needed, so I removed it: `--stop` now sends TERM to the runner only, and `abort_run` stops the children. I updated the header comment that described the old delay. One side effect: `descendants_of` is now only reached from the trap, so I added the same shellcheck SC2329 disable comment that `abort_run` already has. New test `test_term_to_runner_aborts_a_hung_suite`: it sends TERM directly to a runner whose script sleeps for 60 seconds. It checks that the runner exits within 5 seconds with status 143, that FM_TEST_ABORTED is printed, and that the script's process is gone. Before the fix the runner would sit for about 60 seconds, so the test would fail. I didn't actually run it against the old code. **ci-3: a run's record now appears complete or not at all.** The record is written to `$PID_FILE.tmp` in the same private registry directory, then moved into place with `mv`. `stop_runs` only looks at `*.pid` files, so it never sees the temp file. The exit cleanup now removes the temp file as well as the record. In `test_stop_is_scoped_to_one_run`, a background loop keeps listing runs while both runs start up. The test's existing check that both runs are listed would fail if a listing had read a half-written record and deleted it. This race is timing-dependent, so the test makes it likely to show up but can't force it. **Checks:** the full `bash tests/fm-test-run.test.sh` passes, including the new and extended tests (about 6.5 minutes). `shellcheck` on bin/fm-test-run.sh reports the same 3 notices as before the change and nothing new
…ren alive") in bin/fm-test-run.sh and added a test for it in tests/fm-test-run.test.sh. I left ci-1 alone, as the operator decided. **The rule that has to hold:** when a run is aborted, none of its child processes may still be running once the runner exits and its record is removed. After that, nothing can find those processes to stop them. That rule only applies inside `abort_run`. Both ways a run gets aborted end up there: `--stop` and a TERM sent straight to the runner both go through the TERM trap, and INT goes through the INT trap. So fixing that one function covers every case. **Fix (`abort_run`):** - It collects the run's descendant pids once, before sending any signal, and sends TERM to each. - It then waits up to about 2 seconds (20 checks, 0.1 s apart) for those same pids to exit. A pid counts as gone if `ps` can't find it or shows it as a zombie (state Z). Without that, a dead child the runner hasn't cleaned up yet would look alive and add 2 seconds to every abort. - It sends KILL only to those pids that are still running. - Only then does it print FM_TEST_ABORTED and exit. The exit cleanup removes the record after that, so the record outlasts the children. I added no new identity scheme and no other changes. **New test `test_stop_kills_term_ignoring_children`:** - The fixture script runs `trap '' TERM`, then starts a background `sleep` (which also ignores TERM) and writes both pids to a file before it waits. - The test finds the run with `--stop`, then stops it with `--stop <id>`. - It checks that the runner exits with 143, prints FM_TEST_ABORTED for that run, that both the script and the sleep are gone, and that the run's record was removed. **Verification:** - Run against the pre-fix runner (HEAD), the new test fails with "TERM-ignoring process … outlived the stopped run". With the fix it passes. - The full `bash tests/fm-test-run.test.sh` passes (exit 0, 48 tests ok), and no fixture processes were left running afterwards. - `shellcheck` on bin/fm-test-run.sh reports the same 3 notices as before. The test file's only notice is the same SC1091 as before
…cesses") the way you asked. I didn't touch ci-1. **The rule that has to hold:** when `abort_run` sends its final KILL, the target must still be the same process it found as a child of the run, not some other process that later got the same pid. The final KILL in `abort_run` is the only place this matters. `--stop` and a TERM sent directly to the runner both reach it through the TERM trap, and INT reaches it through the INT trap, so this one change covers every way a run gets aborted. **Fix in `bin/fm-test-run.sh` (`abort_run`):** - When it collects the descendant pids, before sending any signal, it now also records each pid's start time using the existing `process_start_identity`. - During the grace period it drops pids that have exited, so the list only holds live processes. - Just before the KILL it reads each start time again and KILLs only the pids whose start time is unchanged. A pid whose start time changed is skipped. - Nothing else was added. **New test in `tests/fm-test-run.test.sh`, `test_stop_spares_a_pid_whose_identity_changed`:** - The fixture script ignores TERM and starts a background `sleep`. A fake `ps` placed first on PATH returns the sleep's real start time the first time it is asked, then a different one, which simulates the pid being reused. - After `--stop <id>`, the test checks that the runner exits with 143 and prints FM_TEST_ABORTED. - It checks that the TERM-ignoring script, whose start time didn't change, was still killed. - It checks that the "reused" sleep was not killed, and then kills that sleep itself. **Verification:** - Against the old runner, the new test fails with "abort_run killed pid … although its start identity changed". With the fix, it passes. - The full `bash tests/fm-test-run.test.sh` passes: exit 0, 49 tests ok, no failures, and no fixture processes left running. - `shellcheck` reports the same notices as before (SC1091 and SC2034). One new SC1007 warning came up during the change and I fixed it
…native Windows Bash. I only checked the script's syntax with `bash -n`, which passes. I did not run the suite, which timed out at 10 minutes in an earlier round. Invariant: tests that need `--stop` must not run where the runner refuses `--stop` (MINGW/MSYS/CYGWIN). Fix in `tests/fm-test-run.test.sh`: I added a `run_unix_only` wrapper that returns early when `uname -s` reports a Windows shell. The call list now runs these four tests through it: - `test_stop_is_scoped_to_one_run` - `test_stop_refuses_an_unprivate_registry` - `test_stop_kills_term_ignoring_children` - `test_stop_spares_a_pid_whose_identity_changed` `test_windows_bash_skips_run_registration` still runs everywhere. `test_term_to_runner_aborts_a_hung_suite` also runs everywhere, because it sends TERM to the runner directly and doesn't use `--stop`
377869a to
40801e6
Compare
|
Speaking as Kun's firstmate: thank you @trillium — tip moved to |
Intent
Make fm-test-run --stop reliable when repeated: ignore TERM and INT while abort_run cleans up, so a second stop during the grace period cannot end the runner before the final KILL and leave TERM-ignoring children alive with no record, with a test that repeats the stop during the grace period.
What Changed
bin/fm-test-run.sh: adds a scoped--stopfor a test run, andabort_runnow ignores TERM and INT while it cleans up. A repeated stop during the grace period can no longer end the runner before the final KILL, so TERM-ignoring children are not left alive with no record.tests/fm-test-run.test.sh: adds tests for--stop(scoped to one run, refuses an unprivate registry, kills TERM-ignoring children, spares a pid whose identity changed). Also adds a test that repeats the stop during the grace period and a test that TERM to the runner aborts a hung suite. The--stoptests are skipped on native Windows Bash (MINGW/MSYS/CYGWIN), where the runner refuses--stop.CONTRIBUTING.md: documentsfm-test-run --stop.Risk Assessment
✅ Low: abort_run now ignores TERM/INT during cleanup as the intent requires, with a repeated-stop test, and I found no wrong-result path in the changed code.
Testing
Ran the real
fm-test-run.shrunner against fixture suites with TERM-ignoring children. Sent repeated--stopcalls inside the grace period. With the fix, the run exits 143, every child is killed and the record is removed. With the fix reverted, the new test fails, so it does reproduce the bug.Evidence: with fix
Source: with fix
Evidence: fix reverted (test fails)
Source: fix reverted (test fails)
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
🔧 **Rebase** - 1 issue found → auto-fixed ✅
CONTRIBUTING.md- merge conflict rebasing onto origin/main🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Review** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
Ran the repeated-stop test and its neighbourtest_stop_kills_term_ignoring_childrenfrom a filtered copy oftests/fm-test-run.test.sh: both passReverted the fix atbin/fm-test-run.sh:2662totrap - TERM INTand re-ran: the repeated-stop test fails with 'did not report FM_TEST_ABORTED'Restored the file and deleted the temporary test copy;git statuswas clean✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.