Skip to content

watch: serialize job runs and terminate in-flight jobs on shutdown - #119

Draft
rsanheim wants to merge 1 commit into
mainfrom
rjs/watch-job-lifecycle
Draft

watch: serialize job runs and terminate in-flight jobs on shutdown#119
rsanheim wants to merge 1 commit into
mainfrom
rjs/watch-job-lifecycle

Conversation

@rsanheim

@rsanheim rsanheim commented Aug 8, 2026

Copy link
Copy Markdown
Owner

Found while auditing plur watch shutdown/cleanup on the common user paths. Three P1s, all reproduced against a real watch session before and after.

The problem

plur kept no handle on the job it spawned and never signalled it. Ctrl-C only appeared to work, because the tty signals the whole foreground process group — plur itself did nothing. RSpec and Minitest both rescue Interrupt to print a summary, so for real suites the job survives and keeps writing to your terminal after plur is gone.

Verified with a rescue Interrupt Ruby job, old binary:

### OLD (main) binary, SIGTERM:
  RESULT: >>> ORPHAN SURVIVED <<<  63342  1  ruby slowjob.rb
### OLD (main) binary, SIGINT:
  RESULT: >>> ORPHAN SURVIVED <<<  63464  1  ruby slowjob.rb

Reparented to PID 1 in both cases. exit and reload leak the same way; after syscall.Exec the job is still a child of the same pid, and the new image never waits on it, so it becomes a zombie — one per reload-with-running-job.

Changes

  1. Terminate in-flight jobs. ExecuteJob now Start/Waits and records the process; TerminateRunningJob sends SIGTERM, waits a 2s grace, then SIGKILLs. Called from the watch loop's defer (covers exit, signals, timeout, error returns) and explicitly in reload() before syscall.Exec, where defers don't run.

  2. Serialize runs. The debouncer resets its timer but can't cancel an already-firing callback, and time.AfterFunc runs each on a fresh goroutine — so saves during a run started concurrent suites against the same test DB. Five concurrent runs were observed from five saves 2s apart. A package-level mutex in ExecuteJob covers both the debounced and [Enter] paths.

  3. [Enter] runs off the select loop. It ran inline, so sigChan wasn't serviced for the job's duration and watch mode was unkillable from its own terminal — three genuine Ctrl-Cs over 12s did nothing.

After

### FIXED binary, SIGTERM:  clean, no survivor
### FIXED binary, SIGINT:   clean, no survivor
### FIXED, SIGTERM during [Enter] run:  plur exited in 1s / orphan: none

(old binary on that last case: still alive after 12s, orphan survived)

Notes

  • The serialization test is red/green verified — without the mutex it produces start start start start end end end end instead of alternating pairs.
  • The SIGKILL escalation test takes exactly the 2s grace period, confirming the escalation actually fires rather than the job dying on SIGTERM.
  • Full suite: 375 examples, 6 failures — the same 6 that fail on main (stdout streaming ×5, auto bundle install ×1), unrelated to this change.
  • Two watch specs updated: exit is now honored immediately during a manual run instead of queuing behind it, so they delay it. Their assertions are unchanged; added one covering the new prompt-exit behavior.

Deliberately not done

Setpgid + kill(-pgid) would also reap grandchildren of a shell-wrapper job, but it stops the tty from delivering Ctrl-C to the job at all and makes plur responsible for forwarding — a much larger behavior change. This is the smaller fix; the grandchild limitation is noted in the code comment and written up separately.

plur watch kept no handle on the job it spawned and never signalled it, so
the job outlived plur on every shutdown path. Ctrl-C appeared to work only
because the tty signals the whole foreground process group -- RSpec and
Minitest both rescue Interrupt to print a summary, so in practice the suite
survived and kept writing to the terminal after plur was gone. Verified
against a `rescue Interrupt` Ruby job: SIGTERM and SIGINT both left it
reparented to PID 1.

Three related fixes:

- ExecuteJob now Start/Waits and records the process, and TerminateRunningJob
  sends SIGTERM, waits a short grace, then SIGKILLs. It is called from the
  watch loop's defer (covering exit, signals, timeout, and error returns) and
  explicitly in reload() before syscall.Exec, where defers do not run and the
  exec'd image would otherwise inherit a child it never waits on.

- A package-level mutex serializes runs. The debouncer resets its timer but
  cannot cancel an already-firing callback, and time.AfterFunc runs each on a
  fresh goroutine, so saves during a run started concurrent suites against the
  same test database -- five were observed from five saves two seconds apart.

- The [Enter] run moves off the select loop. It ran inline, so signals were
  not serviced for the duration of the job and watch mode was unkillable from
  its own terminal; three genuine Ctrl-Cs over twelve seconds did nothing.

The watch specs for manual-run output now delay `exit`, since it is honored
immediately during a run rather than queuing behind it.

Not addressed: grandchildren of a shell-wrapper job are still not reached,
which would need process groups and signal forwarding.
@rsanheim

rsanheim commented Aug 8, 2026

Copy link
Copy Markdown
Owner Author

Do not read the description above as accurate — the central claim is wrong and the code has a known regression. Not ready for review.

What's wrong

The description claims Ctrl-C never really worked, on the grounds that RSpec and Minitest rescue Interrupt and therefore survive. That's false, and the evidence I cited for it was worthless: the fixture behind the ORPHAN SURVIVED block trapped INT and then kept looping, which is not how either runner behaves. I built a fixture that encoded my assumption and then confirmed the assumption against it.

Reading the actual sources, both of which were sitting on the machine:

  • rspec-core/lib/rspec/core/runner.rb:175-190 — traps INT, sets wants_to_quit, prints "RSpec is shutting down and will print the summary report...", exits at the next example boundary. Second INT → exit!(1).
  • minitest/lib/minitest.rb:336-340 — no INT trap; rescue Interrupt around run_all_suites, prints Interrupted. Exiting..., then still calls reporter.report.

Ctrl-C is the runners' own clean shutdown and it works today on main.

The regression this commit introduces

TerminateRunningJob sends SIGTERM immediately on every shutdown path, including Ctrl-C — where the tty has already delivered SIGINT and the runner is mid-shutdown. Measured against a real Minitest suite:

  • main: Interrupted. Exiting... + 0 runs, 0 assertions, ...
  • this commit: neither. Killed before it could report.

So as it stands this fixes the SIGTERM path and breaks the Ctrl-C path.

What still holds, verified independently of that mistake

  • Overlapping concurrent runs. Reproduced three ways plus a red/green unit test; without the mutex the log reads start start start start end end end end. Unrelated to runner signal handling.
  • [Enter] blocks the select loop, so SIGTERM is ignored for the whole job and watch mode is unkillable from its own terminal.
  • Orphans on SIGTERM / exit / reload. Confirmed with a real RSpec suite: kill -TERM on plur left rspec running to full completion, printing 10 examples, 0 failures into the bare shell after plur was gone.
  • Orphans on Ctrl-C with a long example. Confirmed with real RSpec: stuck in sleep 40, it can't reach its quit check, and the pid outlived plur.

So the three P1s are real; what I got wrong was the trigger conditions for the orphan one, and the fix built on that wrong model.

Next

Rework (uncommitted): wait when the terminal has already interrupted the job, send SIGINT ourselves when nothing has, escalate to SIGTERM then SIGKILL only if it doesn't stop. Verification is being moved onto the repo's existing tmux/PTY integration harness (spec/support/tmux_helper.rb) instead of the ad-hoc scripts I used here. Description and the corresponding plur-internal writeup both need correcting.

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.

1 participant