fix(gateway): give in-flight cron work its own drain floor - #82224
JoaoMarcos44 wants to merge 2 commits into
Conversation
`agent.restart_drain_timeout` defaults to 0 and governed every class of in-flight work at once. That default is deliberate for chat turns: the gateway announces the restart to the user and pre-marks the session resume_pending, so interrupting one is cheap and recoverable. A cron run has neither property. Nobody is waiting on it, it is written to jobs.json as a permanent failure, and a recurring job simply skips to its next schedule. Sharing the chat budget meant `_drain_active_agents()` short-circuited on `timeout <= 0` before entering the wait loop, so the drain reported `drain took 0.00s, timed_out=True, cron_at_start=1, cron_now=1` — it detected the job and killed it anyway. Cron work now drains on its own deadline, `agent.cron_drain_timeout` (default 30s, 0 opts out). The floor is clamped to the shutdown-watchdog leash minus a teardown reserve, so the longer wait can never consume the post-drain cleanup window: being SIGKILLed mid-cleanup would leave the job wedged at `last_status=running`, strictly worse than the bug. Being bounded also means a cron-triggered restart cannot deadlock on itself. The `timeout <= 0` special case is gone — an expired deadline expresses the legacy "interrupt immediately" behaviour, so `timed_out` is always computed from real state instead of asserted up front. The drain-timeout warning now reports the elapsed wait rather than the configured budget, which is what made "timed out after 0.0s" so confusing in the report. Chat-only shutdowns are unchanged: `restart_drain_timeout: 0` still interrupts chat turns immediately. Relates to NousResearch#82161 (complements NousResearch#82195, which removes the `hermes update` self-deadlock that triggered the reported instance).
… doubles CI slice 5/12 caught two ways the new cron budget broke `_stop_impl_body` for callers that are not real GatewayRunner instances: - `_FakeGateway` in test_shutdown_cache_cleanup.py borrows `_stop_impl` without subclassing, so it never picked up the class-level `_cron_drain_timeout` default and raised AttributeError. Read it through the getattr-guard convention the same function already uses for its liveness-guard machinery. - The same double overrides `_drain_active_agents(self, timeout)`, so passing the cron budget raised "takes 2 positional arguments but 3 were given". The double now mirrors the real optional parameter. It is the only override in the tree; test_startup_restart_race.py uses AsyncMock, which accepts any signature. Verified against a stashed clean tree: the 22 gateway test files that still fail locally fail identically with and without this branch (80 = 80, empty set difference both ways) — they are pre-existing Windows-only failures (setsid, POSIX modes) unrelated to this change.
|
This was generated by AI during triage. Summary: Problems:
Solution: Checked against |
…nect When the shutdown drain times out and kills an in-flight cron job, the job's owner is never told. The cron worker does try: `_is_interrupted()` forces the failure path with an honest "interrupted by gateway shutdown" error, and failed jobs always deliver. But that worker is a thread, it reaches `_deliver_result()` asynchronously, and by then `_bounded_adapter_teardown()` has closed the transport. The reporter of Worse, the loss is silent twice over: `_consume_interrupted_flag()` returns True — the gateway already wrote `last_status` — so `mark_job_run()` is skipped, and the `delivery_error` from the failed send is discarded with it. The run's only trace is a generic line in jobs.json. The gateway already owns the right window. `_notify_active_sessions_of_ shutdown()` runs while adapters are up, precisely so shutdown messages can be sent — but it iterates `_running_agents`, and cron work lives on the scheduler's own thread pool. Same structural blindness already fixed for counting (#60432) and draining (#63529), never fixed for notifying. So notify from the post-interrupt phase, which is the last point where the transport is still up: `_kill_tool_subprocesses()` now returns the job IDs it marked, and `_notify_interrupted_cron_jobs()` sends each one's owner a notice on the job's own resolved delivery targets. Adapter teardown order is untouched — it is load-bearing for #53175 and #8202. Jobs with `deliver: local`, and `deliver: origin` jobs with no resolvable origin (#43014), resolve to zero targets and stay silent. Per-platform `gateway_restart_notification: false` is honoured, matching the chat path. Every failure is swallowed so a wedged adapter cannot extend shutdown. Second, when the interrupted flag short-circuits `mark_job_run()`, the delivery failure is now persisted on its own via `update_job()`, so a notice that still cannot be sent is at least recorded. `update_job()` rather than a second `mark_job_run()`: the latter also advances `next_run_at` and the repeat counter, and running that twice for one run would skip a fire or auto-delete the job early. Fixes #82232. Related: #82161, #82224.
|
Merged via #86684 — both commits were cherry-picked onto current main with your authorship preserved in git log. The #82244 delivery-error recovery was rewoven into main's owner-fenced |
…nect When the shutdown drain times out and kills an in-flight cron job, the job's owner is never told. The cron worker does try: `_is_interrupted()` forces the failure path with an honest "interrupted by gateway shutdown" error, and failed jobs always deliver. But that worker is a thread, it reaches `_deliver_result()` asynchronously, and by then `_bounded_adapter_teardown()` has closed the transport. The reporter of Worse, the loss is silent twice over: `_consume_interrupted_flag()` returns True — the gateway already wrote `last_status` — so `mark_job_run()` is skipped, and the `delivery_error` from the failed send is discarded with it. The run's only trace is a generic line in jobs.json. The gateway already owns the right window. `_notify_active_sessions_of_ shutdown()` runs while adapters are up, precisely so shutdown messages can be sent — but it iterates `_running_agents`, and cron work lives on the scheduler's own thread pool. Same structural blindness already fixed for counting (NousResearch#60432) and draining (NousResearch#63529), never fixed for notifying. So notify from the post-interrupt phase, which is the last point where the transport is still up: `_kill_tool_subprocesses()` now returns the job IDs it marked, and `_notify_interrupted_cron_jobs()` sends each one's owner a notice on the job's own resolved delivery targets. Adapter teardown order is untouched — it is load-bearing for NousResearch#53175 and NousResearch#8202. Jobs with `deliver: local`, and `deliver: origin` jobs with no resolvable origin (NousResearch#43014), resolve to zero targets and stay silent. Per-platform `gateway_restart_notification: false` is honoured, matching the chat path. Every failure is swallowed so a wedged adapter cannot extend shutdown. Second, when the interrupted flag short-circuits `mark_job_run()`, the delivery failure is now persisted on its own via `update_job()`, so a notice that still cannot be sent is at least recorded. `update_job()` rather than a second `mark_job_run()`: the latter also advances `next_run_at` and the repeat counter, and running that twice for one run would skip a fire or auto-delete the job early. Fixes NousResearch#82232. Related: NousResearch#82161, NousResearch#82224.
…nect When the shutdown drain times out and kills an in-flight cron job, the job's owner is never told. The cron worker does try: `_is_interrupted()` forces the failure path with an honest "interrupted by gateway shutdown" error, and failed jobs always deliver. But that worker is a thread, it reaches `_deliver_result()` asynchronously, and by then `_bounded_adapter_teardown()` has closed the transport. The reporter of Worse, the loss is silent twice over: `_consume_interrupted_flag()` returns True — the gateway already wrote `last_status` — so `mark_job_run()` is skipped, and the `delivery_error` from the failed send is discarded with it. The run's only trace is a generic line in jobs.json. The gateway already owns the right window. `_notify_active_sessions_of_ shutdown()` runs while adapters are up, precisely so shutdown messages can be sent — but it iterates `_running_agents`, and cron work lives on the scheduler's own thread pool. Same structural blindness already fixed for counting (NousResearch#60432) and draining (NousResearch#63529), never fixed for notifying. So notify from the post-interrupt phase, which is the last point where the transport is still up: `_kill_tool_subprocesses()` now returns the job IDs it marked, and `_notify_interrupted_cron_jobs()` sends each one's owner a notice on the job's own resolved delivery targets. Adapter teardown order is untouched — it is load-bearing for NousResearch#53175 and NousResearch#8202. Jobs with `deliver: local`, and `deliver: origin` jobs with no resolvable origin (NousResearch#43014), resolve to zero targets and stay silent. Per-platform `gateway_restart_notification: false` is honoured, matching the chat path. Every failure is swallowed so a wedged adapter cannot extend shutdown. Second, when the interrupted flag short-circuits `mark_job_run()`, the delivery failure is now persisted on its own via `update_job()`, so a notice that still cannot be sent is at least recorded. `update_job()` rather than a second `mark_job_run()`: the latter also advances `next_run_at` and the repeat counter, and running that twice for one run would skip a fire or auto-delete the job early. Fixes NousResearch#82232. Related: NousResearch#82161, NousResearch#82224.
Summary
The shutdown drain detects in-flight cron work and then kills it anyway, reporting
drain took 0.00s, timed_out=True, cron_at_start=1, cron_now=1. This gives cron work its own bounded drain floor so a restart no longer destroys a running job.This is the drain-contract half of #82161 and is complementary to #82195, not a duplicate — zero file overlap. #82195 removes the
hermes updateself-deadlock that triggered the reported instance. This PR fixes the behaviour the reporter asked for explicitly:After #82195 alone, a plain
systemctl restart hermes-gateway/launchctl kickstart -k/hermes gateway stopwith any cron job in flight still kills it at 0.00s. That is what this PR closes.Root cause
agent.restart_drain_timeoutdefaults to0and governed every class of in-flight work through a single budget in_drain_active_agents().That default is deliberate and correct for chat turns:
_notify_active_sessions_of_shutdown()tells the user the gateway is restarting, and the session is pre-markedresume_pending, so the next message resumes it. Interrupting a chat turn is cheap and recoverable.A cron run has neither property:
mark_running_jobs_interrupted()writes it tojobs.jsonas a permanent failure;compute_next_run()just moves to the next schedule, so the work is lost;Because the budget was shared, the drain hit
if timeout <= 0: return snapshot, Truebefore entering the wait loop at all. It never waited a short time — it never waited. Hencedrain took 0.00swhile simultaneously reportingcron_at_start=1, and a "timed out after 0.0s" warning that printed the configured budget instead of the elapsed one.%%{init: {'theme': 'dark', 'themeVariables': { 'primaryColor': '#8b0000', 'mainBkg': '#0a0204', 'primaryTextColor': '#ffccd5', 'primaryBorderColor': '#ff0038', 'lineColor': '#ff0038'}}}%% graph TD A[Shutdown Signal] --> B[stop / drain phase] B --> C{In-Flight Work Detected} C -->|Chat Turn| D[Announced + resume_pending] C -->|Cron Run| E[No User, No Retry] subgraph BEFORE ["BEFORE - One Shared Budget"] F[restart_drain_timeout = 0] --> G[timeout less-or-equal 0 short-circuit] G --> H[Wait Loop Never Entered] H --> I[drain took 0.00s / timed_out=True] I --> J[Tool Subprocess Killed] J --> K[jobs.json: Permanent Failure] K --> L[Alert Lost to Adapter Teardown] end subgraph AFTER ["AFTER - Split Deadlines"] M[Chat Deadline = restart_drain_timeout] --> N[Unchanged: Immediate Interrupt] O[Cron Deadline = cron_drain_timeout] --> P[Clamp to Watchdog Leash minus Reserve] P --> Q[Bounded Grace Window] Q --> R{Job Finished?} R -->|Yes| S[Normal Completion + Delivery] R -->|No| T[Interrupt + Recorded, Cleanup Intact] end D --> M E --> OInfographic :
The fix
Cron work drains on its own deadline, independent of the chat budget.
agent.cron_drain_timeout(default30s;0opts out and restores the previous behaviour). Reads from config orHERMES_CRON_DRAIN_TIMEOUT, following the exact loader/parse conventions of the neighbouringrestart_after_turn_timeout— including "0is a deliberate value, blank falls back to the default".Two design points that make this safe rather than merely longer:
1. The floor is clamped, not trusted.
resolve_cron_drain_budget()caps the configured value atwatchdog_delay − elapsed − CRON_DRAIN_CLEANUP_RESERVE_S. The shutdown watchdog hard-exits atrestart_drain_timeout + 60s, andTimeoutStopSecis generated asmax(60, drain + 30)from the same knob. Waiting past that leash would swap a job that is killed and recorded for one that is SIGKILLed mid-write and left wedged atlast_status=runningforever — strictly worse than the bug being fixed. The 10s reserve is the post-drain teardown: interrupt agents,process_registry.kill_all(),mark_running_jobs_interrupted(), disconnect adapters. With shipped defaults the effective budget is 30s inside a 60s leash.2. Bounded, so it cannot deadlock. A cron job that triggers its own restart (the reporter's
hermes updatecase) still loses after the floor expires. Unlikerestart_after_turn_timeout's 6-hour cap, this can never wedge the shutdown.The floor only ever extends the wait —
resolve_cron_drain_budget()never returns less thandrain_timeout, so an operator who deliberately configured a longrestart_drain_timeoutkeeps it.Secondary cleanups, both from the report
timeout <= 0special case is removed. An already-expired deadline expresses "interrupt immediately" naturally, sotimed_outis always computed from real state instead of asserted up front. This deletes a branch rather than adding one."Gateway drain timed out after 0.0s"was the line that made this look like a phantom.Behaviour preserved
restart_drain_timeout: 0still interrupts chat turns immediately — covered by a dedicated test asserting the drain returns in< 1seven with a 30s cron floor configured._drain_active_agents(timeout)with one argument keeps pre-Gateway drain exits after 0.00s with in-flight cron job, killing it mid-run #82161 semantics (cron_timeoutdefaults totimeout), so every existing caller and test is untouched.Validation
tests/gateway/test_cron_drain_floor.py(new, 13 tests)test_cron_active_work_drain,test_update_cron_drain,test_restart_drain,test_restart_after_turn,test_api_server_active_work_drain,test_cron_shutdown_draintest_gateway_shutdown,test_external_drain_control,test_restart_resume_pending,test_restart_notification,test_stuck_loop,test_restart_redelivery_dedup,test_restart_service_detectiontests/cron/test_shutdown_interrupt.py,tests/hermes_cli/test_gateway_service.pygit diff --checkNew coverage includes the exact repro (0s drain + cron in flight now waits and completes), the bounded-floor guarantee, the chat-only no-regression case, watchdog clamping, and parse/opt-out edge cases.
One existing assertion changed:
test_in_flight_cron_job_marked_interrupted_on_forced_killnow sets_cron_drain_timeout = 0.01alongside_restart_drain_timeout = 0.01, since forcing the interrupt path requires expiring both budgets. The behaviour it asserts is unchanged.Known gap, deliberately out of scope
The undelivered-notification race the reporter noted (adapters disconnect before the cron failure is written) is a separate defect in delivery ordering. This PR reduces its blast radius — jobs that finish inside the floor deliver normally — but does not reorder teardown. Worth its own issue.
Relates to #82161.