fix(gateway): add hard-exit watchdog to prevent drain hang on stop/restart - #35676
fix(gateway): add hard-exit watchdog to prevent drain hang on stop/restart#35676shengting wants to merge 1 commit into
Conversation
…start When the gateway is stopped or restarted while sessions are active, _stop_impl() may hang indefinitely if zombie agent threads cannot be interrupted — causing launchd/systemd to escalate to SIGKILL, which bypasses log flushing and DB close. Two layered watchdogs are added: **Layer 1 — Unconditional stop watchdog** (fires at shutdown entry): Daemon thread that calls os._exit(0) after drain_timeout + 20s, guaranteeing the process exits regardless of where the shutdown sequence gets stuck (drain, interrupt wait, adapter.disconnect, DB close). Covers the blind spot of the per-zombie watchdog when _running_agents is cleared before blocking threads finish. **Layer 2 — Per-zombie watchdog** (fires only when zombie sessions remain after the interrupt + 5s wait): Daemon thread that calls os._exit(0) after 10s, allowing the rest of _stop_impl() to continue (log flush, DB close) while guaranteeing exit even if adapter.disconnect() hangs. os._exit(0) is intentional: sys.exit() raises SystemExit which asyncio and atexit machinery may catch or defer. os._exit() is the correct last-resort in a daemon/service context. Observed on: macOS launchd, Linux systemd. Triggered by: blocking subprocess in terminal() tool, synchronous network call that ignores asyncio cancellation.
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Overview
Two-layer hard-exit watchdog to prevent gateway _stop_impl() from hanging indefinitely when zombie sessions can't be interrupted. A belt-and-suspenders approach covering both the per-zombie blind spot and the catastrophic hang case.
Looks Good
- Layer 1: unconditional exit after
drain_timeout + 20sat start of shutdown - Layer 2: per-zombie guard fires 10s after active sessions remain post-interrupt
- Correct use of
os._exit(0)for last-resort (bypasses cleanup handlers intentionally) - Daemon threads that auto-exit on clean shutdown
- Well-documented rationale and behavior matrix
# noqa: SIM115annotated with explanation
Reviewed by Hermes Agent
|
Hi @tonydwb, the CI workflow runs for this PR are awaiting approval (fork PR security policy). Could you please approve them when you get a chance? The PR has already been approved by you — just need the CI checks to pass before merge. Thanks! |
|
Hi @tonydwb, gentle ping — the CI workflow runs for this PR are still awaiting approval (fork PR security policy). It's been about two weeks since your approval. Could you approve the CI runs when you get a chance? Thanks! |
|
Hi @tonydwb, another gentle ping — this PR was approved on 5/31 but the CI workflow runs are still awaiting approval (fork PR security policy). Could you approve the CI runs when you get a chance so we can get this merged? It's been nearly 3 weeks now. Thanks! |
|
Hi @tonydwb, another ping — it's been over 4 weeks since approval on 5/31 and the CI runs still haven't been triggered. The fork PR security policy requires maintainer approval for CI. Could you please approve the CI workflow runs so this can be merged? Thanks! |
|
Hi @tonydwb 👋 — following up on the four previous pings from @shengting (6/9, 6/13, 6/20, 6/29). This PR was approved on 5/31 but the CI workflow runs are still awaiting maintainer approval due to the fork PR security policy. Could you please approve the CI runs so this can be merged? I also wanted to note why this fix remains necessary even after the recent upstream drain work:
The two approaches are complementary, not redundant. Thanks for your time! |
|
@tonydwb — thanks for taking a look at this PR. As @terry197913 beautifully articulated, the hard-exit watchdog (#35676) sits underneath the drain coordination layer (#52937) and addresses a complementary failure mode: stuck blocking calls that make _stop_impl() hang indefinitely. The PR is approved and the analysis from an independent contributor confirms the design rationale. Could you approve the CI workflow runs so we can get this merged? It has been 6 weeks since initial approval. |
|
Thanks for the shutdown-safety work. An automated hermes-sweeper review found that current
|
Problem
When the gateway receives SIGUSR1 (restart) or is stopped while agent
sessions are active,
_stop_impl()may hang indefinitely if any sessionthread cannot be interrupted ("zombie" sessions — e.g. a blocking
subprocess in
terminal(), or a synchronous network call that ignoresasyncio cancellation).
On systems managed by launchd (macOS) or systemd (Linux), the
process is then killed with SIGKILL after
TimeoutStopSec, which bypasseslog flushing and DB close.
Reproducing scenario:
terminal()callSIGUSR1to restart the gateway (hermes restart)_drain_active_agents()times out;_interrupt_running_agents()firesbut the blocking thread doesn't respond
_stop_impl()blocks in the 5s interrupt wait loop — or until theservice manager's SIGKILL
Fix
Two layered watchdogs in
_stop_impl(), both using daemon threads:Layer 1 — Unconditional stop watchdog
Added at the very start of the shutdown sequence (before drain), fires
after
drain_timeout + 20sregardless of where the sequence gets stuck:This covers the blind spot of Layer 2: when
_running_agentsis clearedbefore blocking threads actually finish, the per-zombie check below sees
an empty dict and doesn't fire — but those threads may still be holding
resources. Layer 1 catches this case.
Layer 2 — Per-zombie watchdog
Triggered only when zombie sessions remain after the interrupt + 5s wait.
Fires after 10s, allowing
_stop_impl()to continue (log flush, DBclose, adapter disconnect) while guaranteeing exit:
Why
os._exit(0)notsys.exit()sys.exit()raisesSystemExitwhich asyncio and Python'satexitmachinery may catch or defer — the process might still not exit.
os._exit(0)bypasses all cleanup handlers and is the correctlast-resort in a daemon/service process. The
# noqa: SIM115silencesthe linter's preference for
sys.exit(), which is inappropriate here.Behavior
Normal shutdown (no active sessions):
Both watchdog threads start but never fire — they are daemon threads and
exit automatically when the main process terminates cleanly.
Shutdown with interruptible sessions:
Sessions are cancelled and drained within
drain_timeout. Process exitscleanly before either watchdog fires.
Shutdown with zombie sessions (threads blocked, cannot be interrupted):
Layer 2 watchdog fires after 10s — process hard-exits via
os._exit(0).Layer 1 was already running but superseded.
Catastrophic hang (drain loop itself stuck,
_running_agentscleared early):Layer 1 watchdog fires unconditionally at
drain_timeout + 20s.Testing
Reproduced hang by starting a session running
sleep 120interminal()tool, then issuing
hermes restart(SIGUSR1).drain_timeout + 20s, log and DBclose complete normally in the non-zombie path.