[kixelated]

Summary

Finishes quest/m1/dispatch-cancel-race.md, the follow-up from moq-dev/moq#8138.

A trapped Bash shell defers its trap while a foreground command runs. A TERM that lands after the shell's last pending-trap check (execute_cmd.c:643 in 5.3) but before make_child blocks signals never reaches the child: the child is forked after the group signal was delivered. In sh/dispatch.sh, a lane's phase subshell (trap 'exit 143' TERM) could then run its whole phase before the cancel took effect. The lane shell had the same window around forking the phase subshell.

The fix forks every command a trapped lane shell runs as a job:

• run CMD... launches the command as a job and waits for it. wait runs a trap at once, and Bash runs a trap left pending across an async fork as soon as the fork returns (execute_cmd.c:797, :1230).
• stop, the body of both TERM traps, ignores further TERMs, resends TERM to the lane's process group (which reaches a job forked after the first TERM), then waits for every job. It uses until wait; do :; done for the same stale wait_signal_received reason as the EXIT trap from fix(ci): a terminated dispatch run waits for every lane and reports it #8138, because its own resend can land while Bash still records wait as the current builtin.
• The lane forks the phase subshell as a job too. The phase subshell and run restore INT/QUIT, which Bash ignores in async lists without job control, so phase commands keep today's dispositions. run keeps the caller's stdin explicitly (<&0).
• Every step command goes through run. The two shapes that forked in the foreground are rewritten:
  • files now writes the listing to $logs/<module>.files, and the lints run run xargs ... <"$listed". Python's check and fix now list the files once instead of twice.
  • flake's probe captures its output to a file instead of a command substitution.

The guarantee from moq-dev/moq#6617 stays: a cancel returns only after the phase's command has exited.

Evidence

All on the pinned Nix Bash 5.3.20.

• Reproduction: test_terminating_just_before_a_phase_forks_its_command_stops_it sets BASH_ENV to an xtrace hook. PS4 expands in the phase shell after the DEBUG trap and the last pending-trap check, right before it forks a foreground command. At rs's lint line, the hook's command substitution TERMs the lane's process group (exactly what a cancel delivers), then blocks until the lane's group TERM kills it, so the TERM is pending when the shell forks. Against main's dispatcher, both subtests (1 and 3 jobs) hang until the test's 60 s guard expires. With this PR they exit 143 at once with check rs (cancelled). A DEBUG-trap hook cannot force this window, because Bash runs the pending TERM trap inside the DEBUG trap.
• The fixed code has no hookable point between its last check and an async fork (an async simple command forks before expansion). So the "pending across the fork" path rests on the Bash source cited above.
• The three cancel tests (forced window, terminate, fail-fast) passed 30 of 30 back-to-back runs.

Choices made without asking

• I made every phase command a run job instead of dropping the phase subshell's trap. An untrapped phase dies on TERM with no race, but it orphans the running command, which breaks ci: run check and ci modules in concurrent lanes #6617's "returns only after the tree exits". Bash does not exec-optimize a function's last command, so even single-command phases would orphan.
• A cancelled command can get up to three TERMs (the dispatcher's, plus one resend each from the lane and the phase). That is harmless for the tools here. Telling a resend that is needed apart from one that isn't would require knowing whether the TERM landed before the fork, which Bash does not expose.
• New step entries must use run. A foreground command would bring this latency race back for that phase only. The forced test guards rs's line.

Public API and wire

None. sh/ CI tooling only.

Validation

• python3 sh/dispatch-query.test.py: 38 tests OK. The new tests are the forced window, and test_formatters_get_the_listed_files, which covers files listing (deleted tracked files skipped) reaching xargs.
• shellcheck sh/dispatch.sh, shfmt -d sh/dispatch.sh, ruff check / ruff format --check on the test: clean.
• quest check: ok.
• quest gate just check (selects everything, since sh/dispatch.sh is root orchestration):
  • 063ff29: passed, exit 0.
  • 9bf391f: everything passed except test js. Its one failure was lite-07 answers a bounded window that resolves empty and FINs in reply, the known flake moq-dev/moq#8150 quarantines, unrelated to sh/.

Suggested follow-ups

None.

Review

Codex gpt-6.1-sol, medium effort, via quest review --base origin/main (base 9920a3f550).

• Round 1 on 9bf391fe481f8ba5c802dc02286c94a181bad0d0, 1 finding: run's async block could drop a redirected stdin (run xargs ... <"$listed"). Fixed in 063ff29 with } <&0 &. It was latent in the dispatcher, where run_lane ... </dev/null & sets Bash's stdin_redir (execute_cmd.c:2837), but real in plain Bash.
• Round 2 on 063ff29ec1969238ef2cef58cdfc754be37f24bb: no findings.
• Pullfrog thread: the forced test TERMed the dispatcher, which on Bash 5.2 could land mid lane-launch and fail the exit 143 trap parse (exit 2). Fixed in c15dc50: the hook now sends kill -TERM 0; 10/10 local passes, still fails against main's dispatcher.
• After merging origin/main, round 1 (fresh session) on c15dc50431916c6b65d1d1dc139b6874d84a6ca9: no findings. sh/dispatch-query.test.py: 38 tests OK.

🤖 Generated with Claude Code

(Written by Claude Opus 5.5 with low effort)

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 8 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: dd4a329f-4505-409c-a5de-cccf95998d72

Commits

Reviewing files that changed from the base of the PR and between fe00d62 and c15dc50.

Files selected for processing: 4

• quest/m1/README.md
• quest/m1/dispatch-cancel-race.md
• sh/dispatch-query.test.py
• sh/dispatch.sh

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[kixelated]

Automated review by review (OpenAI)

Reviewed head 063ff29.

No actionable findings. Using asynchronous jobs plus interruptible waits is a sound direction for the deferred-TERM window, while the resend-and-wait traps retain ownership of the running phase. The conversion covers the phase fork and all step commands; the file-list/probe rewrites avoid the old foreground pipelines and command substitution. Explicit stdin forwarding preserves the formatter inputs, and normal command exit statuses still reach the lane report.

Verification limits: GitHub-only static review of the dispatcher, changed tests and adjacent termination/flake checks. I did not execute the Bash race reproducer, the test suite or the reported gate, and did not independently verify the cited Bash implementation line numbers. The forced-window and repeated-run results remain author-reported.

[pullfrog]

Important

The new forced-window test is flaky: on bash 5.2.21 its jobs="3" subtest failed 8 of 30 back-to-back runs (and 1 of 4 full-suite runs), always with the dispatcher exiting 2 instead of 143. Details and a validated fix direction are inline.

Reviewed changes

• Every phase command is a job — run forks the command as an async job with the caller's stdin and waits for it, so a pending TERM trap runs at once; stop ignores further TERMs, resends TERM to the lane's process group, and waits for every job before the lane exits.
• Phase subshell becomes a lane job — the phase is forked with set +e; wait "$!", keeps its stop; exit 143 trap, and restores INT/QUIT; run_lane's trap becomes stop; cancelled; exit 143.
• Listing and probe rewrites — files writes $logs/<module>.files for xargs (one git ls-files per phase), and the flake probe captures to $logs/flake.probe instead of a command substitution.
• Tests and quest — a forced pre-fork-window test and a files-listing test are added; the completed quest file and its m1 README line are removed.

No implementation defect found in sh/dispatch.sh: I checked the stdin hand-off, the errexit paths, $listed dynamic scoping, the probe-output equivalence, and the process-group invariant (set -m puts each lane in its own group, and bash disables job control in subshells so a lane's jobs stay in that group). The forced test does hang against origin/main, so it genuinely detects the bug.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏}

[pullfrog]

✅ No new issues found.

Reviewed changes

• Forced-window test signals the lane group — the BASH_ENV xtrace hook now sends kill -TERM 0 (the lane's process group, which is what a cancel delivers) instead of kill -TERM $$, keeping the dispatcher's own lane-launch window out of the test.
• Comment updated — the comment above the hook now states the group target and why it avoids the dispatcher's window.

Validated on bash 5.2.21: the forced-window test passed 40/40 back-to-back runs (against 8/30 failures before the change, always jobs="3" with exit 2), and the full sh/dispatch-query.test.py suite passed 38/38. The delta since the prior review contains no sh/dispatch.sh change; the quest/m1/README.md churn in the range-diff is the upstream merge, and the PR's only README change remains removing the completed quest line. The earlier implementation review stands.

  ^{｜ View workflow run ｜ Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏}

[kixelated]

Automated review by review (OpenAI)

Reviewed c15dc50, comparing with 063ff29 and accounting for merged main changes.

No new actionable findings. The forced-window hook now signals its own lane process group rather than Bash's inherited $$ dispatcher PID. That targets the cancellation condition being tested without also racing the dispatcher's lane launch. The job-control boundary keeps the signal inside the lane; the dispatcher implementation and cancellation/child-exit assertions retain their reviewed behavior. This addresses the published test-flakiness concern.

Verification limits: GitHub-only base-aware comparison of all four changed files and inspection of the dispatcher and regression hook. No Bash reproducer, repeated-run trials or tests executed; reported Bash-version results remain author-provided. Rechecked open/non-draft state, full head/base and published reviews immediately before posting.