Skip to content

fix(supervisor): make a watchdog restart resumable, bounded, and sized by the backend it interrupts - #1462

Merged
ZhengGong-amd merged 5 commits into
mainfrom
fix/supervisor-watchdog
Sep 14, 2026
Merged

ZhengGong-amd merged 5 commits into
mainfrom
fix/supervisor-watchdog

Conversation

@lishuoshuo-amd

@lishuoshuo-amd lishuoshuo-amd commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

What this fixes

Two failures the supervisor could not tell apart, and the machinery that got in
the way of telling them apart.

A wedged coordinator is restarted, not ended. The supervisor sends SIGHUP,
and the coordinator reads the signal number off the drain to classify it. A
resumable stop records the leg boundary in leg_ended_ts and writes no
stop_reason, no stop_ts and no terminal artifacts, so the monitor resumes the
same session. The restart budget lives in runtime/supervisor/status.json and is
banked before the signal goes out -- the coordinator's stop path can outlive
the supervisor, and a restart spent only afterwards reads to the next leg as
never spent. After three restarts the next watchdog stop is SIGTERM and follows
the normal terminal path; that terminal ask is not counted as a restart it never
performed.

A hung turn cannot hold the tick open. _await_within_session_bound bounded
inline steps by the session, so one awaited reactor turn could consume the
whole run. Each role turn now carries a wall-clock ceiling -- but read off the
backend that owns it, not from a constant. A backend's call_timeout_s is an
idle budget between streamed messages, and every input to a legal turn (that
timeout, the attempt count, the backoff) is an environment override, so any fixed
number is only right for the defaults it was written against. Each backend
reports the longest turn its own configuration allows; one that declares no
budget is left to the session bound rather than capped at a guess. A turn
cancelled at its ceiling is recorded as a crash, because the tick advances either
way and the crash count is the only channel left that can end a session wedged on
one role.

Also in this PR

  • The robustness monitor imported hyperloom.orchestrator.phase_state, which
    does not exist, and silently fell back to a subset missing 16 terminal stop
    reasons -- so a finished session could be relaunched. It now reads the real
    vocabulary and says on stderr when it cannot.
  • leg_ended_ts is Coordinator-owned (CORE_STATE_FIELDS) like stop_ts: the
    next leg banks phase time up to it, so a forged value bills a phase for time it
    never ran.
  • A SIGTERM that crosses the watchdog's SIGHUP is re-read at the last moment
    before the stop is acted on, so an operator's stop stays terminal.
  • A resumable leg no longer writes the session's close-out package or prints a
    final summary; the close-out steps are collapsed behind one guard.
  • The stall floor goes back to its own argument (1800s) instead of being pinned
    to three copies of a reactor constant, and the env-var reference matches it.

Test plan

  • pytest src/hyperloom/orchestrator/ src/hyperloom/inference_optimizer/tests/ src/hyperloom/agents/ -n auto — 16028 passed; the 19 failures reproduce unchanged on the base commit (local gateway/credential env, missing hypothesis).
  • ruff check src/ and ruff format --check clean.
  • Every new test was run against the pre-fix code first and observed to fail.

@lishuoshuo-amd
lishuoshuo-amd requested a review from a team as a code owner September 9, 2026 11:48
Comment thread src/hyperloom/inference_optimizer/cli/__init__.py Fixed
Comment thread src/hyperloom/inference_optimizer/tests/test_session_lock.py Fixed
Comment thread src/hyperloom/inference_optimizer/cli/__init__.py Fixed
Comment thread src/hyperloom/inference_optimizer/tests/test_session_lock.py Fixed
Comment thread src/hyperloom/orchestrator/actions/executors/_grid_runner.py Fixed
@ZhengGong-amd

Copy link
Copy Markdown
Collaborator

Finding 3 is real and worth fixing. Finding 2 is fixed in the wrong direction, and
Finding 3 is carried over the wrong channel — most of the new code exists to patch
that choice. Please redraw the two mechanisms below rather than adding guards; the
watchdog business code should come out roughly half its current size.

1. One bit, one signal. "Was this SIGTERM the supervisor's?" is one bit, and
SignalDrain already receives it: the bytes it reads off the wakeup pipe are the
signal numbers (signals.py:86), and __init__ already takes an arbitrary
signals: Iterable[int]. Add SIGHUP to STOP_SIGNALS, publish
self.received.update(data) next to self.requested.set(), have the supervisor
os.kill(pid, signal.SIGHUP), and let the coordinator read
signal.SIGHUP in drain.received. That deletes _signal_stop_reason's file read,
the pid comparison, the startswith(f"{WEDGED_STOP_REASON}:") match, the
write-status-before-kill ordering, and the PermissionError rollback
(_asked_unix = 0.0 / asked.pop() / re-write) that only exists because of the
early bookkeeping. It also closes a hole the current form has: stop_asked is
sticky, so after the supervisor has asked once, an operator's own kill is
classified supervisor_restart_requested and the session gets relaunched — the
"manual SIGTERM terminal handling" the description claims to preserve. The new
tests cover empty/mismatched/malformed status, not that case. As a side effect
this also stops the coordinator depending on the supervisor's file format and on
the wording of a log string, which inverts a dependency the docs call out as
one-way.

2. Derive the terminal markers from the monitor, don't enumerate them by hand.
is_terminal_session() reads three things: reports/final.md (:149),
state.phase == "CLOSE" (:160), and stop_reason ∈ STOP_REASON_VOCAB (:178). It
does not read reports/final.json. So the PR gates one marker the monitor
ignores and misses one it honours: a coordinator wedged after the phase machine
reached CLOSE still leaves phase=CLOSE behind and still won't be resumed — the
same failure, one entry condition over. The new paragraph in
environment-variables.md:696-702 states the wrong set on both ends; fix it from
the script.

3. Don't borrow stop_ts. What the next leg needs is where the previous leg's
phase segment ended; stop_ts means "the session stopped", and
sessions.py:511-513 says so in a comment and computes ended_at_utc from it. A
resumable stop still runs write_breakdown_json, so session_breakdown.json now
records an end time for a session that hasn't ended. Add leg_ended_ts, or let
_bank_previous_leg_phase_segment derive the boundary itself — either way the
stop_reason/stop_ts pairing stays intact.

4. Say where the brake is. With over = True if self._asked_unix in the DEAD
branch, and _end_attempted allowing exactly one _end that returns False while
the process is still up, supervisor_tick_stalled and supervisor_coordinator_died
become nearly unreachable — both are still in STOP_REASON_VOCAB. That terminal
record was the only thing stopping wedge → restart → wedge. If watchdog restarts
are now non-terminal, the restart count has to be bounded somewhere: carry it in
status.json and go terminal past a limit.

5. Finding 2: fix the root cause, not the detector. min(tick_age, heartbeat_age) fires only when both are stale, i.e. only when the loop is blocked
in sync code — which running(pid) already almost covers. The common hang is an
await that never returns; the pulse stays fresh through it and WEDGED becomes
unreachable. The false positive's cause is that inline tick steps are bounded by
the session: _await_within_session_bound uses
remaining = self._seconds_until_session_bound(). It already takes stage — give
it a per-stage cap (min(stage_cap, session_remaining), role turns at the
backend's call_timeout_s). That removes the false positive and fixes a real bug:
one step may currently consume the whole run. If you want a second signal instead,
use in-flight task/lease progress (resource_lock.heartbeat_at), which is evidence
of work rather than of a turning loop. Either way, the five-line rationale on
DEFAULT_TICK_STALL_SEC should be replaced by a new argument, not deleted — and
if the detector keeps a composite age, Observation should carry heartbeat_age
so status.json records the input the verdict was made on, not half of it.

6. Collapse what's left. One if not resumable_stop: around the close
sequence and the T4 hook (currently two adjacent guards, plus a third in stop()).
Drop _terminal_reports_enabled — one call site, one comparison. Drop
getattr(state, "close_sequence_done", False); state is a SharedState and your
own test constructs the field. Log the degradation in whatever replaces
_signal_stop_reason: falling back to a terminal stop because a file was
unreadable is exactly the case that needs a line, and the supervisor side already
logs its refusals. return stop_reason if stop_reason == SUPERVISOR_RESTART_REASON else … says "if it is X return X". And final.md moved from after
write_breakdown_json to before it — if that's intended, say why and pin it;
otherwise restore the order.

7. Split the PR. The description says "Findings 2 and 3 only"; the diff carries
five commits and seven unrelated files. packaging (already a module-level import
at base) and the _agentx_variant_timeout extraction (good — it replaces an
except ImportError fallback with a real dependency cut) are each fine on their
own. Two are not, and need their own review:

  • install_baremetal.sh: checkout -q "$SGLANG_REF" is correct when the ref is a
    SHA, which is the only case the new test builds. When it's a branch name, this
    checks out a possibly stale local branch, where FETCH_HEAD was guaranteed to be
    what was just fetched. Resolve FETCH_HEAD to a SHA, or cover the branch case.
  • pre-release probe: moving the real execution to a baremetal leg is defensible,
    but keep the pytest test that actually runs the probe (it skips when the
    framework is absent) instead of replacing it with bootstrap_script.index(...)
    ordering assertions on source text. Also note set -a; . "$envf"; set +a exports
    the whole env file into everything the leg runs afterwards, and a probe failure
    aborts the leg without going through log.

Finally: no CHANGELOG entry, and SKILL.md (the one place that mentions the
supervisor) doesn't learn that a watchdog restart is not a session outcome.

Carry the watchdog restart on the signal itself, bank the interrupted
leg on its own timestamp, and stop after three resumable attempts.

Co-authored-by: Cursor <cursoragent@cursor.com>
lishuoshuo-amd and others added 2 commits September 11, 2026 11:14
A backend's call_timeout_s caps the silence between streamed messages, so
reading it as a turn ceiling cancelled healthy agents and left their retry
budget unreachable. Give the turn its own ceiling above the longest legal
one, widen the stall floor to hold a whole tick of them, and count a turn
cancelled at that ceiling as a crash so an advancing tick cannot hide it.

Co-authored-by: Cursor <cursoragent@cursor.com>
A constant reactor ceiling is only right for the defaults it was written
against: every input to a legal turn -- the per-attempt timeout, the attempt
count, the backoff -- is an environment override, so raising one made the
constant cancel turns the backend still calls healthy. Each backend now
reports the longest turn its own configuration allows, and the tick reads
that; a backend that declares no budget is left to the session bound rather
than capped at a guess. The stall floor goes back to its own argument instead
of being pinned to three copies of that constant.

The watchdog's restart budget is banked before the signal goes out, since the
coordinator's stop path can outlive the supervisor and a restart spent only
afterwards reads as never spent. The terminal ask is no longer counted as a
restart it did not perform.

Also: the monitor imported a stop-reason module that does not exist and
silently fell back to a subset missing 16 terminal reasons; leg_ended_ts is
Coordinator-owned like stop_ts, since the next leg banks phase time up to it;
a SIGTERM crossing the watchdog's SIGHUP is re-read at the last moment so an
operator's stop stays terminal; and a resumable leg no longer writes the
session's close-out package or prints a final summary.

Co-authored-by: Cursor <cursoragent@cursor.com>
@lishuoshuo-amd lishuoshuo-amd changed the title fix(supervisor): treat a live heartbeat as liveness and keep watchdog stops resumable fix(supervisor): make a watchdog restart resumable, bounded, and sized by the backend it interrupts Sep 11, 2026
Use an explicit total reactor deadline and stage progress so idle budgets cannot cancel healthy turns or make the watchdog fire inside legal work. Preserve terminal signal priority and resumable recovery across persistence failures, while falling back to a terminal stop when the restart budget cannot be recorded.

Co-authored-by: Cursor <cursoragent@cursor.com>
#: role turns at their five-minute cap, plus a retry each.
_TICK_STALL_FLOOR_SEC: float = 1800.0
#: The shortest default stall window covers one reactor turn and one poll.
_TICK_STALL_FLOOR_SEC: float = DEFAULT_REACTOR_TURN_TIMEOUT_SEC + DEFAULT_POLL_SEC
#: role turns at their five-minute cap, plus a retry each.
_TICK_STALL_FLOOR_SEC: float = 1800.0
#: The shortest default stall window covers one reactor turn and one poll.
_TICK_STALL_FLOOR_SEC: float = DEFAULT_REACTOR_TURN_TIMEOUT_SEC + DEFAULT_POLL_SEC
Co-authored-by: Cursor <cursoragent@cursor.com>

@ZhengGong-amd ZhengGong-amd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

@ZhengGong-amd
ZhengGong-amd merged commit 42d0ef7 into main Sep 14, 2026
45 of 48 checks passed
@ZhengGong-amd
ZhengGong-amd deleted the fix/supervisor-watchdog branch September 14, 2026 02:25
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.

3 participants