Skip to content

fix(tui): keep background notifications in owning session (#42674) - #42731

Closed
raywcm wants to merge 3 commits into
NousResearch:mainfrom
raywcm:fix/tui-owned-background-notifications
Closed

fix(tui): keep background notifications in owning session (#42674)#42731
raywcm wants to merge 3 commits into
NousResearch:mainfrom
raywcm:fix/tui-owned-background-notifications

Conversation

@raywcm

@raywcm raywcm commented Jun 9, 2026

Copy link
Copy Markdown

What does this PR do?

Fixes a Desktop/TUI cross-session notification path for background process completions.

The poller path already skips events whose session_key belongs to another live session, but the post-turn safety drain still called process_registry.drain_notifications() directly. That method drains the global process completion queue, so a turn finishing in session B could consume and dispatch a notify_on_complete event that was started by session A.

This PR adds a small TUI-owned drain helper that filters drained events with the same ownership check used by the poller, requeues foreign live-session events, and only dispatches notifications owned by the current session (plus global/orphan notifications).

Related Issue

Fixes #42674

Related to #35652, but this targets the post-turn drain path rather than the between-turn poller path.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • tui_gateway/server.py
    • Added _drain_owned_notifications() to drain only notifications owned by the current TUI session.
    • Requeues foreign live-session events so the owning session's poller can handle them.
    • Uses the helper in the post-turn completion-notification drain.
  • tests/test_tui_gateway_server.py
    • Added regression coverage for owned, foreign, and global notification events.

How to Test

  1. Open two TUI/Desktop sessions.
  2. In session A, start a bounded background process with notify_on_complete=True.
  3. While it is running, send a normal message in session B.
  4. When the process completes, the notification should remain routed to session A instead of being consumed by session B's post-turn drain.

Local verification performed:

python3 -m py_compile /tmp/hermes-fix/tui_gateway/server.py
python3 -m py_compile /tmp/hermes-fix/tests/test_tui_gateway_server.py

I could not run the full pytest suite in this PRoot environment because cloning/extracting the full repository repeatedly timed out; the patch was prepared via GitHub Contents API against main and syntax-checked locally.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Linux / PRoot (syntax checks)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

For New Skills

N/A

Screenshots / Logs

Syntax checks:

python3 -m py_compile /tmp/hermes-fix/tui_gateway/server.py
python3 -m py_compile /tmp/hermes-fix/tests/test_tui_gateway_server.py

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/tui Terminal UI (ui-tui/ + tui_gateway/) tool/delegate Subagent delegation labels Jun 9, 2026
yingliang-zhang added a commit to yingliang-zhang/hermes-agent that referenced this pull request Jul 9, 2026
…aned events

Two complementary fixes for cross-session background-process notification
leakage in the TUI/Desktop multi-session path (NousResearch#42674, NousResearch#35652).

1. Poller orphan guard: after _notification_event_belongs_elsewhere
   returns False, check whether the event has a non-empty session_key
   that differs from the current session.  If so the owner session is
   gone — drop the event instead of hijacking it into an unrelated
   session transcript.

2. Post-turn drain filter: the existing drain_notifications() pops every
   event from the global queue regardless of ownership.  Added
   _drain_owned_notifications() which applies the same ownership routing
   used by the poller (consume own, requeue foreign-live, drop orphan),
   and wired it into the post-turn safety drain.

Complementary to PR NousResearch#42731 which addresses a separate code path in the
same bug class.  Together they close NousResearch#42674.
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for isolating the post-turn drain path. The premise is still valid on current main: tui_gateway/server.py:9408 invokes the post-turn drain, while tools/process_registry.py:1200 applies ownership filtering only to async_delegation, so ordinary completion events still bypass the callback.

Problems

  • The patch predates the current ownership API: current tui_gateway/server.py:8545 defines _notification_event_belongs_elsewhere(sid, session, evt), while the proposed helper calls it with two arguments.
  • Filtering after drain_notifications() is too late for events suppressed inside that method: tools/process_registry.py:1195 runs completion suppression before the proposed helper would receive an event.
  • The added test directly calls the helper rather than exercising _run_prompt_submit() with a foreign ordinary completion.

Suggested changes

  • Put addressed-event ownership routing in ProcessRegistry.drain_notifications() before completion suppression, using the current compression-aware _session_owns_notification_event() callback from the TUI.
  • Add real post-turn A/B tests for successful and failed completions, plus compression-lineage ownership.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
yingliang-zhang added a commit to yingliang-zhang/hermes-agent that referenced this pull request Jul 15, 2026
…aned events

Two complementary fixes for cross-session background-process notification
leakage in the TUI/Desktop multi-session path (NousResearch#42674, NousResearch#35652).

1. Poller orphan guard: after _notification_event_belongs_elsewhere
   returns False, check whether the event has a non-empty session_key
   that differs from the current session.  If so the owner session is
   gone — drop the event instead of hijacking it into an unrelated
   session transcript.

2. Post-turn drain filter: the existing drain_notifications() pops every
   event from the global queue regardless of ownership.  Added
   _drain_owned_notifications() which applies the same ownership routing
   used by the poller (consume own, requeue foreign-live, drop orphan),
   and wired it into the post-turn safety drain.

Complementary to PR NousResearch#42731 which addresses a separate code path in the
same bug class.  Together they close NousResearch#42674.
teknium1 pushed a commit that referenced this pull request Jul 16, 2026
…aned events

Two complementary fixes for cross-session background-process notification
leakage in the TUI/Desktop multi-session path (#42674, #35652).

1. Poller orphan guard: after _notification_event_belongs_elsewhere
   returns False, check whether the event has a non-empty session_key
   that differs from the current session.  If so the owner session is
   gone — drop the event instead of hijacking it into an unrelated
   session transcript.

2. Post-turn drain filter: the existing drain_notifications() pops every
   event from the global queue regardless of ownership.  Added
   _drain_owned_notifications() which applies the same ownership routing
   used by the poller (consume own, requeue foreign-live, drop orphan),
   and wired it into the post-turn safety drain.

Complementary to PR #42731 which addresses a separate code path in the
same bug class.  Together they close #42674.
@teknium1

Copy link
Copy Markdown
Contributor

Closing as superseded by #65419 (merged) — the post-turn drain now passes session_key + the compression-chain-aware owns_event callback + skip_poll_observed=False, which covers everything _drain_owned_notifications() did plus lineage resolution and a requeue-when-running guard. Your diagnosis of the bare drain_notifications() call was correct a month before the rest of us got there — thanks @raywcm, and sorry this sat long enough to be fixed out from under it.

@teknium1 teknium1 closed this Jul 16, 2026
@teknium1 teknium1 added the area/sessions Session lifecycle, resume, persistence, history label Jul 19, 2026
Gravezzz pushed a commit to Gravezzz/hermes-agent that referenced this pull request Jul 21, 2026
…aned events

Two complementary fixes for cross-session background-process notification
leakage in the TUI/Desktop multi-session path (NousResearch#42674, NousResearch#35652).

1. Poller orphan guard: after _notification_event_belongs_elsewhere
   returns False, check whether the event has a non-empty session_key
   that differs from the current session.  If so the owner session is
   gone — drop the event instead of hijacking it into an unrelated
   session transcript.

2. Post-turn drain filter: the existing drain_notifications() pops every
   event from the global queue regardless of ownership.  Added
   _drain_owned_notifications() which applies the same ownership routing
   used by the poller (consume own, requeue foreign-live, drop orphan),
   and wired it into the post-turn safety drain.

Complementary to PR NousResearch#42731 which addresses a separate code path in the
same bug class.  Together they close NousResearch#42674.
randlee pushed a commit to randlee/hermes-agent that referenced this pull request Aug 11, 2026
…aned events

Two complementary fixes for cross-session background-process notification
leakage in the TUI/Desktop multi-session path (NousResearch#42674, NousResearch#35652).

1. Poller orphan guard: after _notification_event_belongs_elsewhere
   returns False, check whether the event has a non-empty session_key
   that differs from the current session.  If so the owner session is
   gone — drop the event instead of hijacking it into an unrelated
   session transcript.

2. Post-turn drain filter: the existing drain_notifications() pops every
   event from the global queue regardless of ownership.  Added
   _drain_owned_notifications() which applies the same ownership routing
   used by the poller (consume own, requeue foreign-live, drop orphan),
   and wired it into the post-turn safety drain.

Complementary to PR NousResearch#42731 which addresses a separate code path in the
same bug class.  Together they close NousResearch#42674.
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/tui Terminal UI (ui-tui/ + tui_gateway/) P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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 tool/delegate Subagent delegation type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Background process notify_on_complete leaks into wrong TUI session (cross-session bleed)

3 participants