Repository navigation
fix: drain embedded Kanban workers before gateway restart - #103961
sycamoregroupltd wants to merge 2 commits into
Conversation
alt-glitch
added
type/bug
There was a problem hiding this comment.
🟡 Changes recommended
There are a few correctness/operability issues (notably failing-open accounting, worker handling when run_id is NULL, and misleading SIGTERM logging) that could cause the restart drain to miss or misreport live Kanban workers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates the gateway restart “after-turn” drain logic to account for embedded Kanban worker subprocesses, preventing systemd KillMode escalation from force-killing workers without a durable handoff marker.
Changes:
- Adds Kanban worker accounting to gateway active-work and restart wait calculations, plus a force-interrupt hook for restart deadlines.
- Introduces DB-backed worker discovery and a force-path helper to durably transition running tasks before signaling worker PIDs.
- Adds regression tests covering wedged exclusion, force-path ordering, and active-work totals.
File summaries
| File | Description |
|---|---|
| tests/hermes_cli/test_gateway_restart_kanban_workers.py | New regression tests for worker discovery, wedged handling, and force-path behavior. |
| hermes_cli/kanban_db.py | Re-exports new dispatcher helper functions from kanban_db_dispatch. |
| hermes_cli/kanban_db_dispatch.py | Adds worker listing and force-restart preparation (durable transition + SIGTERM). |
| gateway/run_shutdown.py | Incorporates Kanban workers into active-work and awaitable-work logic; invokes Kanban interrupt hook on force paths. |
| gateway/kanban_watchers.py | Enumerates live workers across boards, pauses dispatch while draining, and implements force cleanup during restart drain. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| try: | ||
| from hermes_cli import kanban_db as _kb | ||
| from hermes_cli import kanban_db_connect as _kbc | ||
| from hermes_cli import kanban_db_dispatch as _kbd | ||
| boards = _kb.list_boards(include_archived=False) | ||
| except Exception: | ||
| logger.exception("kanban restart drain: cannot enumerate boards") | ||
| return [] | ||
|
|
||
| workers: list[dict] = [] | ||
| for board_meta in boards: | ||
| slug = board_meta.get("slug") or _kb.DEFAULT_BOARD | ||
| conn = None | ||
| try: | ||
| conn = _kbc.connect(board=slug) | ||
| workers.extend(_kbd.list_running_workers(conn, include_wedged=include_wedged)) | ||
| except Exception: | ||
| logger.debug("kanban restart drain: worker scan failed on board %s", slug, exc_info=True) | ||
| finally: | ||
| if conn is not None: | ||
| try: | ||
| conn.close() | ||
| except Exception: | ||
| pass | ||
| return workers |
| count_kanban = getattr(self, "_active_kanban_worker_count", None) | ||
| try: | ||
| active_kanban = max(0, int(count_kanban())) if callable(count_kanban) else 0 | ||
| except Exception: | ||
| active_kanban = 0 | ||
| return ( |
| prepared: list[dict[str, Any]] = [] | ||
| for worker in list_running_workers(conn, include_wedged=True): | ||
| run_id = worker["run_id"] | ||
| if run_id is None: | ||
| continue |
| if prepared: | ||
| logger.warning( | ||
| "kanban restart drain: blocked and SIGTERM'd %d worker(s) on board %s", | ||
| len(prepared), slug, | ||
| ) |
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head 1cbd5910479f3be130c2c4427aefc312e9d34743 against current main 245e48008fa814b3251f50755eb656bd9fb86cb1 (merge base 96ed0e71ea2d75d325cc05c3e6468d213de2408d; this branch is currently 3 commits behind main). I read all five changed files, the existing restart-handoff test on current main, the shutdown/dispatcher ownership paths, the worker spawn/scope helper, current review threads, #77184, merged #101940, open #101911, and the Kanban decomposition owner #78632.
The incident evidence is valuable, and the core ordering instinct here is good: if a run really must be interrupted, durable state must win before an external kill can be interpreted as a crash. The new tests also pin the intended block-before-signal ordering rather than merely checking an exception. Exact-head CI is green: CI run 34001954780, Docker 34001954495, and Nix 34001954467 all succeeded; this PR has one commit, so every PR commit currently has a green exact-object receipt.
I do not think this head is safe to land yet, for three architecture-level reasons beyond the existing Copilot inline findings.
1. This currently reverses the restart-survival contract already on main and competes directly with #101911
Merged #101940 deliberately changed managed-systemd Kanban spawn to restart_safe_gateway_child_argv(). Current-main tests/hermes_cli/test_kanban_gateway_restart_handoff.py::test_managed_gateway_worker_is_spawned_in_restart_safe_scope asserts that the worker runs in its own .scope and specifically not in hermes-gateway.service; the fail-closed tests refuse a managed spawn when that scope cannot be established. In other words, for the topology this PR is trying to protect, current main already says the worker's effect lifetime is not the gateway process lifetime.
This head then counts those restart-safe workers as gateway-owned work and, on every force path, calls block_task(... kind="transient", expected_run_id=run_id) and SIGTERMs them. That turns "survive the gateway restart" into "planned gateway restart terminalizes the Kanban run." It avoids charging a crash retry, but it discards the durable-work contract that #101940 intentionally established.
Open #101911 is the existing full carrier for exactly the unresolved other half: worker self-registration, PID/start-time identity, cgroup-truth liveness, re-adoption after gateway restart, verified whole-scope teardown, and an explicit worker_isolation_stop_on_shutdown policy whose default is survive + re-adopt. It also collides with this PR in gateway/kanban_watchers.py and hermes_cli/kanban_db.py. That is not merely adjacent work; these are competing lifecycle policies for the same defect class.
Required resolution: preserve one semantic owner. Either fold this production incident + focused regression evidence into #101911, preserving its contributor history, or reduce this PR to a genuinely non-competing delta that handles only a proven remainder (for example, legacy/unisolated workers) without terminating the restart-safe scoped workers current main intentionally detached. Please make the merge order explicit rather than landing two different answers to "who owns a Kanban worker across gateway restart?"
2. A gateway that does not own the machine-global dispatcher can currently block and signal another process's workers
GatewayKanbanWatchersMixin._kanban_dispatcher_boot() acquires the machine-global .dispatcher.lock for the process lifetime. A sibling gateway that sees the lock contended explicitly returns without becoming the dispatcher. But the new _active_kanban_worker_count(), _wedged_kanban_worker_count(), and _interrupt_kanban_workers_for_restart() paths do not require _owns_kanban_dispatcher_lock() at all.
Worse, list_running_workers() accepts any claim whose lock merely starts with _host_prefix(). Current main defines _claimer_id() as <host>:<pid> and _host_prefix() as only <host>:. So a profile gateway that never acquired dispatch authority can restart, scan every board, wait on workers claimed by the actual dispatcher, and eventually block_task() + SIGTERM those workers. Its own _draining flag does not pause the actual owner's watcher, either. That is a cross-process mutation-authority breach, not just an accounting bug.
Required fix: bind restart handling to exact worker authority, not host co-location. At minimum the regression needs two gateway/claimer identities on one host: the non-owner restart must neither treat the owner's worker as its own drain target nor mutate/signal it. The implementation should consume the existing dispatcher ownership/claim identity (or the durable adoption authority that #101911 introduces), rather than authorizing lifecycle mutation from _host_prefix().
3. worker_pid + _pid_alive() is not sufficient authority for a destructive signal, and one PID is not the whole scoped effect
The new force path reads a durable PID, checks only that some process currently occupies it, commits the task away from running, and then calls os.kill(pid, SIGTERM). There is no process-generation/start-time proof. A stale PID can be recycled between persistence and restart; in that case this path can block the Hermes task and signal an unrelated host process. The repository already has the stronger shape in tools/process_registry.ProcessSession (host_start_time, systemd_unit), and #101911 carries the corresponding Kanban design (worker_pid_started_at, worker_scope, cgroup truth).
There is also a settlement-order problem for process trees: current-main managed workers live in a transient systemd scope. Signalling one PID does not prove that the run's descendants are gone. This PR makes the durable task terminal-ish first and never verifies whole-effect teardown, so a worker can be recorded blocked while a descendant process from that run remains live and continues side effects.
Required fix: do not authorize a destructive signal from a bare PID. For workers that should survive, re-adopt instead of terminating. For workers that truly must stop, use generation-bound process identity and verified whole-scope/process-tree teardown before releasing the run into a state that can later be retried.
The four existing Copilot findings remain additive (cross-board scan rate, fail-open accounting, NULL-run handling, and the SIGTERM telemetry mismatch); I am not duplicating those here.
Interlocks / landing gate
- #77184 is the historical after-turn-drain defect and remains useful provenance; this PR is a follow-up, not a replacement for that closed fix.
- #101940 is merged current behavior, not historical noise: its Kanban scope contract has to be preserved.
- #101911 is the direct competing/superseding carrier for Kanban worker restart ownership. Its prior work and authorship need to survive whichever implementation lands.
- #78632 remains the canonical
hermes_cli/kanban_db.pydecomposition owner. This PR modifies that file at the re-export seam around line 4191, so it still touches an owner above the repository's 2K ceiling. Treat that as a landing-order/topology gate rather than growing the compatibility godfile again.
Before a clear/ready verdict I would want the ownership test above, a managed-systemd survival/re-adoption test, a recycled-PID negative test, a leaked-descendant/whole-scope settlement test, the existing scan-failure cases, and then a rebase to current main with CI/Docker/Nix green again at the new exact head. The present head is green, but it is not current-main proof.
There is strong incident work here. The important thing now is to keep that evidence and the useful ordering tests while landing them into the repository's one durable worker-lifetime model instead of creating a second one.
…vity, sync exit_kind contract - DispatchResult.supervisor_restarts: new field, plumbed from detect_crashed_workers._last_supervisor_restart in _run_reclaim_phase — dispatch_once callers (CLI output, telemetry, on_kanban_dispatch_tick) can now see neutral gateway-restart releases. - _TICK_ACTIVITY_FIELDS: count supervisor_restarts as tick activity so a restart releasing N pre-boot claims no longer reports outcome='idle'. - plugins.py on_kanban_worker_exited contract: document the new 'supervisor_restart' exit_kind value. - _supervisor_restart_collateral docstring: prove only what durable state proves (unobserved-by-any-live-dispatcher) and name the residual one-shot-dispatcher ambiguity explicitly (NousResearch#83066, NousResearch#103961). - Regression tests: tick-level red/green test for the plumb + whitelist. Co-authored-by: crazyief <8566250+crazyief@users.noreply.github.com>
…vity, sync exit_kind contract - DispatchResult.supervisor_restarts: new field, plumbed from detect_crashed_workers._last_supervisor_restart in _run_reclaim_phase — dispatch_once callers (CLI output, telemetry, on_kanban_dispatch_tick) can now see neutral gateway-restart releases. - _TICK_ACTIVITY_FIELDS: count supervisor_restarts as tick activity so a restart releasing N pre-boot claims no longer reports outcome='idle'. - plugins.py on_kanban_worker_exited contract: document the new 'supervisor_restart' exit_kind value. - _supervisor_restart_collateral docstring: prove only what durable state proves (unobserved-by-any-live-dispatcher) and name the residual one-shot-dispatcher ambiguity explicitly (NousResearch#83066, NousResearch#103961). - Regression tests: tick-level red/green test for the plumb + whitelist. Co-authored-by: crazyief <8566250+crazyief@users.noreply.github.com>
Managed-worker turns run in child processes, outside _running_agents, so _active_work_count()/_drain_work_counts() saw none: with a managed turn running, _drain_active_agents reported no work and timed_out=False, no Stop was ever sent, and settle_gateway_runtime() awaited the authority task with an unbounded gather. A stalled worker bypassed the configured bounded drain. - run_runtime.managed_turn_count counts live managed workers across every served authority; it rides the chat slot of the drain (chat deadline), the active-work total, the restart wait and the post-interrupt grace loop, and appears in the draining status as kind=managed. A worker registers before its reservation, and a draining owner refuses that reservation, so nothing unbootstrapped can start work behind the count. - the timed-out path (_interrupt_running_agents) sends each managed worker the same Stop a user's Stop sends (stop_managed_turns); the worker acknowledges or is terminated within STOP_ACK_SECONDS and the admission settles through its own fenced path (interrupted, or durable unknown). - settle_gateway_runtime keeps the #121813 authority-task settlement wait but sends Stop first and bounds it (TURN_SETTLE_SECONDS); a task past the bound is cancelled, which leaves its admission for recovery, never settled. Donor test pattern: #103961 test_active_work_count_includes_kanban_workers / test_restart_wait_excludes_wedged_kanban_workers_and_interrupts_them, adapted to canonical managed workers with real subprocess children. Review: #106742 (comment)
Follow-up to #77184.
Problem
The gateway restart after-turn wait counted messaging turns but not embedded Kanban worker subprocesses. The observed restart series on 2026-09-03 was 00:13, 00:29, 00:57, 07:20, 07:38, 09:27, 21:17, and 21:28 BST (the incident report labels this as seven restarts but lists eight timestamps); each was recorded as a crash wave. The two late restarts were legitimate (Slack integration at 21:16 and dashboard activity), and left 7 jarvis-os plus 6 sycode-trading workers under systemd KillMode=mixed; all 13 were SIGKILLed and re-dispatched after pid-not-alive crashes.
Fix
gateway restartand run-id CAS before sending SIGTERM, so supervisor escalation cannot charge a crash retry.Verification
python -m pytest tests/hermes_cli/test_gateway_restart_kanban_workers.py tests/hermes_cli/test_kanban_gateway_restart_handoff.py -q -o addopts=-> 9 passed.This is a fork branch only; no live gateway or production install was restarted.