Skip to content

fix(cron): finalize kanban worker sessions with ended_at (#76914) - #76995

Open
webtecnica wants to merge 1 commit into
NousResearch:mainfrom
webtecnica:fix/76914-kanban-session-ended-at-null
Open

fix(cron): finalize kanban worker sessions with ended_at (#76914)#76995
webtecnica wants to merge 1 commit into
NousResearch:mainfrom
webtecnica:fix/76914-kanban-session-ended-at-null

Conversation

@webtecnica

Copy link
Copy Markdown
Contributor

Summary

Kanban worker sessions completed their tasks but left sessions.ended_at NULL in state.db (reproduced 2/2 runs). The worker exits via the quiet single-query CLI path, which never finalized the SQLite session.

Root Cause

Kanban workers are spawned as hermes -p <profile> --cli chat -q "work kanban task <id>" (hermes_cli/kanban_db.py:8964). The quiet path in cli.py (_finalize_single_query, ~L1342) only notifies session-finalize hooks and runs cleanup — it never calls agent.close(), which is what invokes session_db.end_session(session_id, "agent_close") (run_agent.py:4150-4157, gated by _end_session_on_close, default True).

The interactive run() loop closes the session in its finally block (cli.py:17718-17721), but the quiet path bypasses it entirely. Compare hermes_cli/oneshot.py:461-481 (hermes -z calls agent.close() in its own finally) and cron/scheduler.py:3829-3832 (end_session(..., "cron_complete") explicitly).

Change

cli.py_finalize_single_query now calls agent.close() (defensively, via getattr/callable so stub agents in tests are unaffected) before releasing the active session lease. close() is idempotent in the run loop path and triggers end_sessionended_at set.

Verification

  • pytest tests/cli/test_single_query_session_finalize.py tests/cli/test_chat_q_exit_clear.py — 9 passed
  • pytest tests/hermes_cli/test_kanban_lifecycle_hooks.py::test_claim_fires_hook tests/hermes_cli/test_kanban_write_guard.py — 4 passed
  • 7 kanban-collection failures on the full -k kanban run reproduce on unpatched upstream/main (pre-existing order-dependent flakes, not caused by this change)

Closes #76914

@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.

Thanks for tracing the Kanban worker lifecycle. The premise is confirmed on current main: hermes_cli/kanban_db.py:9131-9134 launches workers through chat -q, while cli.py:1341-1347 does not call agent.close(); run_agent.py:4096-4100 is the path that ends an owned SQLite session.

Problems

  • The new close() is after _run_cleanup() (cli.py:1352 in this diff). If cleanup raises, it is never reached. tests/cli/test_single_query_session_finalize.py:16-34 already exercises a cleanup exception and proves the finalizer must continue its teardown path.

Suggested changes

  • Put the guarded agent.close() in a finally before _release_active_session(), with release protected by its own finally.
  • Extend tests/cli/test_single_query_session_finalize.py to assert close() runs on both normal and cleanup-failure paths.

This is an automated hermes-sweeper review.

Comment thread cli.py
# workers / cron pipes / `hermes chat -q` exit with ended_at NULL.
agent = getattr(cli, "agent", None)
close = getattr(agent, "close", None)
if callable(close):

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.

_run_cleanup() can raise (see tests/cli/test_single_query_session_finalize.py:16-34), which skips this call and leaves the owned session open. Move this guarded close into a finally that runs before the lease-release finally, and add coverage for that error path.

@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard comp/cron Cron scheduler and job management area/sessions Session lifecycle, resume, persistence, history P3 Low — cosmetic, nice to have sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 2, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #50881 covers the broader one-shot session-finalization path, including explicit DB finalization and WAL flushing. This PR is the focused agent.close() repair for the #76914 ended_at regression.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Aug 2, 2026
@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

One PR addresses #76914. #76995 targets the reported quiet-path lifecycle gap by adding agent.close(), but the call remains after cleanup and can therefore be skipped when cleanup raises.

Related pull requests

Suggested consolidation

Keep #76995 open with a salvage path: move the guarded agent.close() into a finally that executes before the lease-release finally, and add coverage showing that close() runs after both successful cleanup and cleanup failure. The current revision should not be merged because the contributor's keep_open review identifies a documented error path in which the reported session-finalization defect remains.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I76914(["issue #76914 (open)"])
    P76995["PR #76995 (open)"]
    P76995 -->|best fix| I76914
    class I76914 open
    class P76995 open
    class P76995 best
    class P76995 target
    click I76914 "https://github.com/NousResearch/hermes-agent/issues/76914"
    click P76995 "https://github.com/NousResearch/hermes-agent/pull/76995"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 1 pull request and 1 issue in this complex. Each diff was read against this issue; Assessment working set: <1 kB of PR diffs, 4 kB of issue/PR text, 1 kB of discussion (2 comments), 2 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/cli CLI entry point, hermes_cli/, setup wizard comp/cron Cron scheduler and job management P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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.

Kanban worker sessions finish tasks but leave sessions.ended_at NULL

4 participants