Time the hang watchdog from the kernel, so disarming it cannot wedge the process (#661)

[lmoresi]

Summary

Closes #661.

unwatch() could hang or crash the process it exists to diagnose. Native stack,
2331 of 2331 samples in one place:

main thread   unwatch() -> faulthandler.cancel_dump_traceback_later()
              -> PyThread_acquire_lock_timed -> cond_wait     [holding the GIL]
faulthandler  faulthandler_thread -> _Py_DumpTracebackThreads
  C thread    -> dump_traceback                               [pinned mid-dump]
Timer thread  PyEval_RestoreThread -> take_gil                [starved]

Load is the trigger, not xdist: idle 0 hangs in 12 runs, 1 of 1 under CPU
burners, and 6/13 with xdist against 6/13 without — that equality is what
ruled xdist out. One defect, two faces: walking frames another thread is
mutating SEGVs the xdist worker in CI and wedges the process locally. It failed
six unrelated PRs in a day.

The fix

The clock moves to the kernel and the dump to a signal handler.
signal.setitimer raises SIGALRM whether or not the interpreter can run;
faulthandler's registered handler dumps on the thread that receives it; and
disarming is setitimer(0), a syscall that cannot wait on anything. Nothing
runs concurrently with the interpreter, so the deadlock is removed structurally
rather than dodged. dump_traceback_later and cancel_dump_traceback_later are
gone from the watchdog entirely.

Measured against the bar the old mechanism set, before the library was
touched:

check	result
200 tight arm/disarm cycles (the pattern that wedges today)	clean
rank blocked in a 4 s allreduce	8 of 8 dumps, every blocked rank
MPI disturbed? 300 allreduce/barrier/bcast rounds at 100 Hz	0 collective errors

Two things this nearly broke

abort was implemented only through dump_traceback_later(exit=True).
Removing that would have silently broken UW_HANG_WATCHDOG_ABORT — which CI
depends on, and which has to work on a rank blocked in MPI where no Python-level
handler runs. It is now done by the kernel too: SIGALRM's disposition is set to
SIG_DFL and the dump chains to it, so the process dumps and is then terminated
by the signal. Verified at np=4: each blocked rank wrote one dump naming
reduce_the_count and died on signal 14, the job ending in 2 s instead of
hanging.

The dump format, which matters because the dumps are a parsed artefact.
dump_traceback_later writes a Timeout ( header before each dump; a signal
dump does not. hang_report keyed on that header, so every dump in a file would
have merged into one. It now starts a new dump at the Current thread line,
which faulthandler writes exactly once per dump in both formats — headed
files parse exactly as before, headerless ones parse correctly.

This is what made test_0054 hang for ten minutes on the first attempt: the
test counted "Timeout (" and waited forever for a dump count that could no
longer rise.

Verification

test_0053 + test_0054	19 passed in 19.25 s
control: test_0054 on unmodified development	8 passed in 18.25 s
test_005* + test_006* batch	74 passed
abort, serial	process died after 1 s watchdog, 1 dump written
abort, np=4 blocked ranks	1 dump each naming reduce_the_count, killed on signal 14

Flagged, not fixed (Charter §9)

test_0053's module docstring points at
tests/parallel/test_0778_hang_watchdog_mpi.py, which does not exist. No test
outside these two files uses the watchdog at all.

Underworld development team with AI support from Claude Code

🤖 Generated with Claude Code

https://claude.ai/code/session_01E87Q7KrpapxeQiLD1RiNXv

[Copilot]

🟡 Changes recommended

The new SIGALRM-based abort path in mpi.py can permanently clobber any pre-existing SIGALRM handler (and repeated re-registering per checkpoint is unnecessarily invasive), so signal state should be preserved/restored to avoid process-wide side effects.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates the hang watchdog to avoid deadlocks/crashes caused by faulthandler.dump_traceback_later() by switching watchdog timing to a kernel interval timer (setitimer/SIGALRM) and adjusting hang-dump parsing/counting to handle headerless signal-triggered dumps.

Changes:

• Rework watchdog arming/disarming to use SIGALRM + signal.setitimer() instead of dump_traceback_later()/cancel_dump_traceback_later().
• Update dump parsing (hang_report.py) and test dump counting (test_0054) to split dumps using the Current thread ... line (works for both headed and headerless dumps).

File summaries

File	Description
src/underworld3/mpi.py	Switch watchdog timing to kernel SIGALRM interval timer and dump via faulthandler’s signal handler.
src/underworld3/utilities/hang_report.py	Treat Current thread ... as a dump boundary so repeated signal dumps don’t merge.
tests/test_0054_hang_report.py	Count dumps using Current thread markers instead of the Timeout ( header.

Review details

Suppressed comments (1)

src/underworld3/mpi.py:474

• cancel() disables the interval timer and unregisters SIGALRM, but in the abort=True path arm() has already overwritten the prior SIGALRM handler with SIG_DFL (so unregister() will restore SIG_DFL, not the original handler). If you save the previous handler in arm(), restore it here so unwatch() leaves process-wide signal state unchanged.

        if _INTERVAL_TIMER_AVAILABLE:
            _signal.setitimer(_signal.ITIMER_REAL, 0.0, 0.0)
            _faulthandler.unregister(_signal.SIGALRM)

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[lmoresi]

Adversarial review of this PR, combined with Copilot's

Copilot raised one issue. Executing it rather than accepting it turned up two
more, and refuted one of its claims. All three real ones are fixed in 78eca78,
each with a regression test.

1. unwatch() did not restore a pre-existing SIGALRM handler — Copilot, CONFIRMED

Reproduced:

before = mine   after = 0        # SIG_DFL

With abort, arm() installed SIG_DFL underneath faulthandler so the dump
could chain to it and terminate. faulthandler.unregister then restored that
SIG_DFL, not the caller's handler. Arming the watchdog anywhere in a program
would have silently discarded a handler it had installed for its own reasons,
and only the next SIGALRM would have revealed it.

2. checkpoint() from a worker thread raised, with abort on — ours, CONFIRMED REGRESSION

this branch : ValueError: signal only works in main thread of the main interpreter
development : ok

signal.signal refuses to run outside the main thread, and the signal setup was
being redone on every arm() — and checkpoint() is arm(). checkpoint is
documented as cheap enough to leave in production code, and abort is CI's
setting, so this is the more serious of the two.

3. watch() consumed a user's ITIMER_REAL in silence — ours, CONFIRMED

user's 3600 s timer after watch(seconds=30):  30.0 s left

Copilot saw the handler; the timer itself is the larger half. There is one
interval timer per process and the watchdog needs it, so it cannot be shared.
It now warns rather than cancelling someone's timer without saying so.

4. "Repeated re-registering per checkpoint is unnecessarily invasive" — Copilot, NOT UPHELD

Measured, same machine, 20 000 checkpoints:

	checkpoint()
this branch	41.8 us
development	66.9 us

It is cheaper than the mechanism it replaces. The re-registering was worth
removing for reason 2, not for cost.

What the fix for 2 then broke, and how it was caught

Moving registration into __init__ broke the resume path. watching cancels
the outer watchdog and restores it with arm(resume=True), which then moved the
clock without re-registering the handler — so the next SIGALRM reached the
SIG_DFL underneath and the kernel killed pytest. Seven tests in, no summary,
exit 1.

That is the failure this whole PR exists to prevent, reintroduced by the fix for
a review comment. It was caught only by running the whole file rather than the
new tests, which is worth recording. Registration is now re-established whenever
a watchdog is armed without one, and test_watching_restores_the_previous_watchdog
covers it.

Verification after the fixes

test_0053 + test_0054	22 passed in 19.03 s
test_005* + test_006*	77 passed
Charter scan	clean
A: checkpoint() off-thread, abort=True	ok
B: pre-existing SIGALRM handler after unwatch()	restored
C: taking ITIMER_REAL	announced by RuntimeWarning