Skip to content

fix(gateway): track unowned watcher tasks in GatewayRunner - #6790

Open
Dusk1e wants to merge 1 commit into
NousResearch:mainfrom
Dusk1e:fix/gateway-untracked-background-tasks
Open

fix(gateway): track unowned watcher tasks in GatewayRunner#6790
Dusk1e wants to merge 1 commit into
NousResearch:mainfrom
Dusk1e:fix/gateway-untracked-background-tasks

Conversation

@Dusk1e

@Dusk1e Dusk1e commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?
I noticed a lifecycle issue in GatewayRunner where several critical watcher tasks were being created as "unowned" asyncio tasks.

Specifically, the tasks for process watching, session expiry loops, and platform reconnection were triggered via bare asyncio.create_task() calls. Without maintaining a strong reference in self._background_tasks, these are at risk of being prematurely reaped by Python's Garbage Collector (GC) during long-running gateway sessions.

This PR introduces a _track_background_task helper to ensure these tasks have a strong reference and are deterministically cancelled during gateway shutdown, following asyncio best practices.

Type of Change
[x] 🐛 Bug fix (non-breaking change that fixes an issue)

[x] ✅ Tests (adding or improving test coverage)

Changes Made
gateway/run.py:

Implemented _track_background_task() which adds tasks to the internal tracking set with an add_done_callback for automatic cleanup (discard).

Migrated 4 callsites (reconnect loops, session flusher, and process watchers) to use this tracking helper.

tests/gateway/test_gateway_shutdown.py:

Added a regression test to verify that background tasks are correctly registered in the runner's tracking set and cleared upon stop().

How to Test
I've verified the code syntax using compileall (since my current env lacks some RL dependencies).

The logic can be validated by running pytest tests/gateway/test_gateway_shutdown.py.

You should see that len(runner._background_tasks) remains stable and doesn't lose tasks during extended idle periods.

Checklist
[x] My commit messages follow [Conventional Commits]

[x] This isn't a duplicate of existing PRs

[x] I've added tests to cover the lifecycle of these watchers

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery labels Apr 29, 2026

@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 identifying the task-ownership issue. The premise still holds on current main, but this patch needs a current-code salvage to cover the expanded watcher surface.

Problems

  • Current startup schedules unowned process, expiry, Kanban, reconnect, handoff, async-delegation, optional scale-to-zero, and drain-control watchers at gateway/run.py:7302-7383; porting only the PR's four historical callsites would leave the same bug class present.
  • The post-turn process-watcher drain still uses a bare task at gateway/run.py:11727-11740 and needs the same ownership treatment.
  • The added test covers the former three-watcher startup shape, not the current set or every scheduling path.

Suggested changes

  • Port the helper and apply it consistently to the current long-lived watcher creation sites, then verify shutdown through gateway/run.py:8273-8283 cancels the retained tasks.
  • Expand lifecycle coverage for both startup and post-turn watcher scheduling.

Automated hermes-sweeper review.

Comment thread gateway/run.py
self._background_tasks: set = set()

def _track_background_task(self, task: asyncio.Task) -> asyncio.Task:
"""Retain a strong reference to a fire-and-forget task until completion."""

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.

Please port this helper across the current watcher surface, not only the four historical callsites. Current start() has additional untracked Kanban, handoff, async-delegation, scale-to-zero, and drain-control watchers at gateway/run.py:7302-7383, plus a post-turn process-watcher drain at gateway/run.py:11727-11740.

@teknium1 teknium1 added 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 12, 2026
@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

Five PRs address or reference the same GatewayRunner background-task ownership issue. #3167 and its merged salvage #3254 track command and memory-flush tasks; #6790, #11630, and #14748 target unowned watcher tasks, with #6790 covering four historical watcher call sites but not the broader current watcher surface identified in review.

Related pull requests

Duplicates

#3167 and #3254 overlap on the command and session memory-flush task tracking, with #3254 the merged salvage of #3167. #14748 is explicitly a duplicate of #6790; #11630 substantially overlaps the watcher-tracking scope but also includes update-notification coverage.

Suggested consolidation

Keep #6790 open with a salvage path: despite the contributor keep_open review, expand its helper-based tracking to every current long-lived watcher creation site named in that review, including the startup sites at gateway/run.py:7302-7383 and the post-turn task at gateway/run.py:11727-11740, then extend lifecycle tests accordingly. Keep #3254 as the merged reference implementation, and leave #3167 and #14748 closed; #14748 can remain closed as a duplicate of #6790, while #11630 should remain closed after its contributor-recorded skip.

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
    subgraph Dup6790 ["PRs duplicating each other"]
        P6790["PR #6790 (open)"]
        P11630["PR #11630 (closed)"]
        P14748["PR #14748 (closed)"]
    end
    class P6790 open
    class P11630 closed
    class P14748 closed
    class P6790 target
    click P6790 "https://github.com/NousResearch/hermes-agent/pull/6790"
    click P11630 "https://github.com/NousResearch/hermes-agent/pull/11630"
    click P14748 "https://github.com/NousResearch/hermes-agent/pull/14748"
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 5 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 29 kB of PR diffs, 11 kB of issue/PR text, 2 kB of discussion (6 comments), 0 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

comp/gateway Gateway runner, session dispatch, delivery 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 type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants