Skip to content

fix(cli): discard owned notification backlog on first Ctrl+C - #123137

Open
Wenfengcheng wants to merge 4 commits into
NousResearch:mainfrom
Wenfengcheng:fix/123114-interrupt-notification-backlog
Open

Wenfengcheng wants to merge 4 commits into
NousResearch:mainfrom
Wenfengcheng:fix/123114-interrupt-notification-backlog

Conversation

@Wenfengcheng

@Wenfengcheng Wenfengcheng commented Sep 25, 2026 •

Copy link
Copy Markdown

Problem

Part of #123114. In the classic CLI, interrupting a running turn leaves already-pending background completion notifications in the process registry. The post-turn drain immediately turns that backlog into another agent turn.

Root cause

_tui_handle_ctrl_c requests the interrupt but never disposes of the owned notification backlog. Merely removing queue entries would permit duplicate process events or durable delegation replay to resurrect the reports.

Changes

  • First Ctrl+C discards the current owned registry backlog before interrupting, with an explicit count. The second-press force exit is unchanged.
  • Add bare /purge for on-demand disposal of the current session's pending registry reports, including during an active turn. It bypasses the busy input queue, does not interrupt running work, retains user prompts, and works even when Ctrl+C purge is opted out. Unsupported all/kill arguments print usage without discarding anything.
  • Reuse existing ownership routing, including the CLI compression-lineage resolver.
  • Mark purged process completions consumed/observed; claim and terminally drop durable delegation completions using the existing ledger API. Results/output are retained, foreign ownership and live claims are preserved, interim notices do not consume final results.
  • Add display.ctrl_c_purge_notifications (default true); false preserves interrupt-only behavior. Ledger failure retains the event and does not prevent interruption.

Verification

Base: 34343e79ab603f2a6f1c1b7597a44c00c6e2ce6f.

  • RED via scripts/run_tests.sh tests/hermes_cli/test_ctrl_c_notification_backlog.py -q: 1 expected assertion failure, 3 passed. Actual Ctrl+C followed by the real drain queued another input.
  • Focused GREEN: 9 passed. Includes real ProcessRegistry and SQLite delegation ledger, restart restoration, duplicate copies, foreign session, claim contention, interim notice, empty backlog, error fallback and opt-out.
  • Native Windows, existing Python environment; all HOME/USERPROFILE/HERMES_HOME/APPDATA/LOCALAPPDATA paths isolated before imports; no installs or production state used.
  • Exact head 09404606731dee87ab403d62e5c79be380737427: focused suite 13 passed; broader CLI notification + async-delegation suites 51 passed, 2 platform skips via scripts/run_tests.sh. A separate process-registry notification subset passed 5 tests on the preceding head (unchanged routing).
  • A real local Python child was adopted by the registry, completed, then Ctrl+C and the real drain suppressed its notification while read_log retained its output. SQLite restoration retained foreign results and did not resurrect dropped results.
  • Two independent-review findings (failed-drop claim release and legacy events without a durable row) were reproduced as assertion REDs, fixed in follow-up commits on this PR, and included in the final GREEN.
  • Same-PR follow-through at cbef31b6b4800942920bc8c303d1f001fdd9c5f8: the /purge public-dispatch and busy-Enter regression tests produced 2 expected assertion REDs against current main 20cf14fd6bfa248388c83a3c6c1b665e032be707. Focused GREEN: 22 passed. Broader notification/delegation suites: 60 passed, 2 platform skips; command registry and slash-interrupt suites: 52 passed, all through scripts/run_tests.sh.
  • Public /purge E2E: a real local child completed and a real SQLite delegation completion was queued; the actual command and drain suppressed both, retained process output and the durable result, and restoration did not replay the dropped completion.
  • Final exact-head Codex v0.153.4 (gpt-5.6-sol): STATIC PASS. Claude Code 2.1.280 / Opus 5.5 again returned HTTP 503 and reached its bounded timeout; that independent review remains unresolved, not PASS.

Non-goals / remaining acceptance

Delivered: scoped registry purge, durable claim/drop, configurable first Ctrl+C, and bare on-demand /purge with busy-command admission. Remaining: /purge all and /purge kill are explicitly unsupported, not silently treated as bare purge. Continuation decision: retain these on this PR's acceptance ledger, but stop this increment at the independently useful non-destructive boundary. all needs an explicit ownership scope; kill needs cancellation/exit-event coordination and real lifecycle validation rather than reusing global /stop and risking foreign work. No pre-existing exact purge owner was found in the fresh search/timeline checks. This change does not cancel running processes, discard future completions, change Ctrl+Q, gateway, Ink/Desktop, or already-admitted input turns. Global background_process_notifications: off remains independently owned by pre-existing #123123. This is Part of, not Fixes, the issue.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard comp/tools Tool registry, model_tools, toolsets tool/terminal Terminal execution and process management area/config Config system, migrations, profiles sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Sep 25, 2026
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference; please use your judgment.

The ownership predicate is copied verbatim from drain_notifications, so routing stays consistent. Three things break.

1. Blocker — the committed desktop registry dump is stale; CI is red. commands.py:94 adds the /purge CommandDef, but apps/desktop/src/lib/desktop-slash-registry.json is a committed dump of COMMAND_REGISTRY and was not regenerated. test_desktop_slash_registry.py::test_committed_desktop_dump_matches_registry fails here with Right contains 1 more item: {'/purge': 'terminal'}; it passes at merge-base 34343e79ab. Run scripts/dump_desktop_slash_registry.py.

2. Major — "owned" is session_key equality, and it degrades to fail-open when the key is absent. _owns_process_notification (cli_process_notifications.py:4) is pure key comparison, so another profile's backlog is safe and PID reuse is irrelevant (the key is a session id, not a pid). But _owns_event (process_registry.py:1820) consults owns_event only when the event carries a session_key or origin_ui_session_id. Verified: with a real adopt_local(session_key="") completion queued, _owns_event(evt, "OWNER", lambda e: False, False) returns True and purge deletes it — an unkeyed event is adopted by any purging session, even one whose owns_event rejects it. session_key arrives via terminal_tool.py:1299, so empty is reachable. The docstring's "fail closed — never leak on a broken check" does not hold here.

3. Major — a discarded durable delegation is terminally gone, but the message says the opposite. A claimed row goes through drop_completion_delivery → delivery_state='dropped'. Verified: restore_undelivered_completions then replays 0 rows (async_delegation.py:327 selects only pending), list_async_delegations() is empty, and the row is gone from _records. result_json survives in sqlite but no user-facing surface reads a dropped row. Yet cli_process_notifications.py:54 prints "output remains available." For a one-hour run that is silent, unrecoverable loss behind a reassuring prompt.

4. Minor — the "new completions remain notifications" invariant is untested. Mutating the bound to qsize() * 2 leaves all 22 tests green.

Only the TUI path purges (cli_tui_mixin.py:1032); shutdown and SIGINT do not. A second Ctrl+C force-exits without purging; a repeat /purge correctly returns 0.

This branch has not been deployed

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

Labels

area/config Config system, migrations, profiles comp/cli CLI entry point, hermes_cli/, setup wizard comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades tool/terminal Terminal execution and process management type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants