Skip to content

fix(kanban): make run finalization shutdown-safe - #66915

Open
chrismaddern wants to merge 2 commits into
NousResearch:mainfrom
chrismaddern:wt/t_822e83e0
Open

fix(kanban): make run finalization shutdown-safe#66915
chrismaddern wants to merge 2 commits into
NousResearch:mainfrom
chrismaddern:wt/t_822e83e0

Conversation

@chrismaddern

Copy link
Copy Markdown

Summary

  • finalize complete/block/reclaim/worker-exit races exactly once
  • reconcile orphaned active runs before dispatcher claims new work
  • track gateway Kanban DB threads and boundedly drain real SQLite writes on shutdown, including with a zero-second agent drain
  • add concurrent race, restart reconciliation, and SIGTERM transaction fault-injection coverage

This branch includes the prerequisite DB-health commit from #65899 because that dependency is not yet on main.

Test plan

  • scripts/run_tests.sh tests/hermes_cli/test_kanban_db.py tests/hermes_cli/test_kanban_db_health.py tests/hermes_cli/test_kanban_dispatch_lock.py tests/hermes_cli/test_kanban_run_finalization.py tests/gateway/test_kanban_db_circuit.py tests/gateway/test_kanban_watchers_mixin.py -q (256 passed)
  • uv run ruff check gateway/kanban_watchers.py gateway/run.py hermes_cli/kanban_db.py tests/gateway/test_kanban_watchers_mixin.py tests/hermes_cli/test_kanban_run_finalization.py
  • python -m compileall -q ...
  • git diff --check

@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management comp/gateway Gateway runner, session dispatch, delivery P3 Low — cosmetic, nice to have needs-decision Awaiting maintainer decision before any implementation sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Jul 18, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.
Related to #65068 (detached-worker exit finalization) and same-author sibling #66912 (shared Kanban DB-health prerequisite). This PR adds a distinct gateway shutdown-drain/orphan-reconciliation path; please split or rebase the shared slice so the changes can be reviewed independently.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The finalization and shutdown-tracking direction addresses gaps still present on current main: _end_run does not inspect its run-row update result (hermes_cli/kanban_db.py:3262-3290), dispatch does not reconcile orphaned runs before reclaim/claim work (hermes_cli/kanban_db.py:7551-7556), and gateway shutdown does not track Kanban to_thread work (gateway/run.py:8452; gateway/kanban_watchers.py:298).

Problems

  • The shared DB-health prerequisite (e97692b) overlaps the active consolidation in #66912, as the MEMBER comment notes. Please split that prerequisite from the targeted finalization slice so it can be reviewed independently.
  • tests/hermes_cli/test_kanban_core_functionality.py:3749 renames an existing test to _legacy_gateway_dispatcher_retries_after_ttl; pytest will no longer collect it, leaving obsolete dead test code.

Suggested changes

  • Keep the e7639f2 finalization/shutdown slice separate from the DB-health prerequisite and consolidate the latter through #66912.
  • Remove the uncollected legacy helper, or make it an executed assertion of the supported recovery behavior.

Automated hermes-sweeper review.

@@ -3756,25 +3749,23 @@ async def _sleep(_delay):
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This rename removes the previous test from pytest collection but retains its obsolete implementation as dead code. Please delete it, or replace it with an executed test for the supported recovery contract.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management comp/gateway Gateway runner, session dispatch, delivery needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants