Skip to content

fix(qqbot): prevent listener busy-loop after closed-WebSocket reconnect failure (#31771) - #31774

Closed
xxxigm wants to merge 3 commits into
NousResearch:mainfrom
xxxigm:fix/31771-qqbot-busy-loop-on-closed-ws
Closed

fix(qqbot): prevent listener busy-loop after closed-WebSocket reconnect failure (#31771)#31774
xxxigm wants to merge 3 commits into
NousResearch:mainfrom
xxxigm:fix/31771-qqbot-busy-loop-on-closed-ws

Conversation

@xxxigm

@xxxigm xxxigm commented May 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What does this PR do?

Stops the QQBot adapter from pegging a Hermes Gateway process at 99-100% CPU after the WebSocket closes and the next reconnect attempt fails.

When _ensure_token or _get_gateway_url raised inside _reconnect (e.g. the transient Failed to get QQ Bot gateway URL: [Errno 8] nodename nor servname provided from the issue trace), the previous session's self._ws was never cleared because _open_ws — the only place that nulls the reference — never ran. The next _listen_loop iteration re-entered _read_events, where the while not self._ws.closed: loop body never ran for a closed socket, the function returned None immediately, and _listen_loop reset backoff_idx = 0 and looped again. Result: a hot busy-loop with no further reconnect logs, platforms.qqbot.state stuck at disconnected, and one Python core fully pinned.

This PR adds two layers of defense:

  1. _read_events raises on entry when the socket is missing / already-closed, and again at the bottom if the read loop exited silently while the listener still wants to run, so a closed-socket return is never mistaken for a successful read cycle.
  2. _reconnect clears self._ws on its exception path (only when the socket is actually closed — never killing a still-live one) so the next iteration sees a missing socket rather than a closed one.

Either fix on its own breaks the loop; together they make the contract explicit at both ends.

Related Issue

Closes #31771.

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

  • gateway/platforms/qqbot/adapter.py — _read_events raises RuntimeError(\"WebSocket closed before read\") on entry with a closed socket and RuntimeError(\"WebSocket closed during read\") after the loop if self._running is still True; _reconnect drops a stale closed self._ws reference on the exception path.
  • tests/gateway/test_qqbot_busy_loop_31771.py — 11 new regression tests:
    • TestReadEventsRaisesOnClosedSocket (4 cases) — entry with stale closed socket / self._ws=None, the post-loop guard for socket-disappears-mid-read, and clean-shutdown silent return.
    • TestReconnectClearsStaleClosedSocket (3 cases) — failed reconnect drops a closed reference, a successful reconnect leaves the new socket set, a still-open socket is never killed.
    • TestListenLoopNoBusyLoopAfterReconnectFailure (2 cases) — end-to-end repro pinned with asyncio.wait_for so a regression that re-introduces the hot loop will trip the test runner's timeout.
    • TestReadEventsSourceGuards (2 cases) — inspect.getsource-based guards so an accidental refactor removing the defensive checks is loud during code review.

How to Test

  1. Check out this branch and ensure .venv is set up: python3 -m venv .venv && source .venv/bin/activate && pip install -e \".[all,dev]\"
  2. Run the new regression suite on its own:
    ```
    scripts/run_tests.sh tests/gateway/test_qqbot_busy_loop_31771.py
    ```
    Expected: 11 passed.
  3. Run the full QQBot test suite to confirm no cross-file regressions:
    ```
    scripts/run_tests.sh tests/gateway/test_qqbot.py tests/gateway/test_qqbot_busy_loop_31771.py
    ```
    Expected: 170 passed (159 pre-existing + 11 new).

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(qqbot): / test(qqbot):)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix (no unrelated commits)
  • I've run scripts/run_tests.sh tests/gateway/test_qqbot_busy_loop_31771.py and all 11 tests pass
  • I've added tests for my changes (11 new cases across 4 classes)
  • I've tested on my platform: macOS 15.2 (Darwin 24.6.0), Python 3.12

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — N/A (defensive guard, no public-API change; rationale captured in code comments + commit bodies)
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — pure asyncio control-flow, no platform-specific calls
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

Screenshots / Logs

```
$ scripts/run_tests.sh tests/gateway/test_qqbot_busy_loop_31771.py
24 workers [11 items]
============================== 11 passed in 0.5s ==============================

$ scripts/run_tests.sh tests/gateway/test_qqbot.py tests/gateway/test_qqbot_busy_loop_31771.py
=== Summary: 2 files, 170 tests passed, 0 failed (100% complete) in 2.6s ===
```

After applying this PR, the exact failure trace from #31771:

```
2026-05-25 02:30:33,484 WARNING gateway.platforms.qqbot.adapter: [QQBot:1903695542] WebSocket error: WebSocket closed
2026-05-25 02:30:33,488 INFO gateway.platforms.qqbot.adapter: [QQBot:1903695542] Reconnecting in 2s (attempt 1)...
2026-05-25 02:30:35,522 WARNING gateway.platforms.qqbot.adapter: [QQBot:1903695542] Reconnect failed: Failed to get QQ Bot gateway URL: ...
```

continues on through the normal backoff/retry path (WebSocket closed before read from _read_events → except Exception → next _reconnect with bumped backoff_idx) instead of looping silently at 100% CPU.

xxxigm added 3 commits May 25, 2026 08:37
@xxxigm
fix(qqbot): raise from _read_events on closed/missing socket (NousRes…
4136bea 
…earch#31771)

After a reconnect attempt failed (e.g. ``_get_gateway_url`` raised on
a transient DNS glitch before ``_open_ws`` could clear ``self._ws``),
the next ``_listen_loop`` iteration re-entered ``_read_events`` with a
stale closed socket reference.  The ``while not self._ws.closed:`` body
never ran, the function returned ``None`` immediately, and
``_listen_loop`` reset the backoff and looped again — pegging the
process at 99-100% CPU with no further reconnect logs.

Make ``_read_events`` raise on entry when the socket is missing /
already-closed, and again at the bottom if the read loop exited
silently while the listener still wants to run.  ``_listen_loop`` now
sees a ``RuntimeError`` and applies its reconnect/backoff path instead
of treating the silent return as a successful read cycle.
@xxxigm
fix(qqbot): clear stale closed _ws on failed reconnect (NousResearch#…
39c6225 
…31771)

Defense in depth for the busy-loop scenario in NousResearch#31771: when
``_ensure_token`` or ``_get_gateway_url`` raises inside ``_reconnect``
before ``_open_ws`` could clear the previous session's ``self._ws``,
the closed reference would survive and leak into the next listen-loop
iteration.

Drop the reference on the failure path (only when it's actually closed
— never null out a still-live socket) so a subsequent ``_read_events``
call surfaces ``WebSocket not connected`` rather than the closed
socket silently short-circuiting the read loop.  Pairs with the
``_read_events`` pre-flight raise from the previous commit; either
defense alone would already prevent the hot loop, but together they
make the contract explicit at both ends.
@xxxigm
test(qqbot): regression coverage for busy-loop on closed websocket (N…
cc68bdc 
…ousResearch#31771)

Add 11 regression tests pinning both the ``_read_events`` and
``_reconnect`` defenses against the NousResearch#31771 hot-loop:

- ``TestReadEventsRaisesOnClosedSocket`` (4 cases) — covers entry with
  a stale closed socket, entry with ``self._ws=None``, the
  socket-disappears-mid-read post-loop guard, and the clean-shutdown
  case where silent return is correct.
- ``TestReconnectClearsStaleClosedSocket`` (3 cases) — closes the
  stale-state path: failed reconnect drops a closed reference, a
  successful reconnect leaves the new socket set, and a still-open
  socket is never killed by the cleanup branch.
- ``TestListenLoopNoBusyLoopAfterReconnectFailure`` (2 cases) — the
  end-to-end repro: ``_listen_loop`` now ticks ``backoff_idx`` after
  each silent failure and gives up at ``MAX_RECONNECT_ATTEMPTS``
  instead of looping forever.  Wrapped in ``asyncio.wait_for`` so the
  test runner's timeout catches a regression even when the loop body
  is actually hot.
- ``TestReadEventsSourceGuards`` (2 cases) — pin the defensive checks
  in source via ``inspect.getsource`` so an accidental refactor
  removing them is loud during code review.
@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery platform/qqbot QQ Bot adapter P2 Medium — degraded but workaround exists labels May 25, 2026
@alt-glitch

alt-glitch commented May 25, 2026

Copy link
Copy Markdown
Collaborator

This is another competing fix for root issue #17703 (QQBot busy-loop on closed WebSocket). Multiple open PRs address this: #31333, #29057, #27821, #20994, #30285. The issue #31771 that this closes is itself a duplicate of #17703.

@alt-glitch alt-glitch mentioned this pull request Jun 6, 2026
Closed
2 tasks
@teknium1

teknium1 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Thanks for the detailed reproduction and regression coverage. This is already implemented on current main via the canonical QQBot busy-loop fix.

Automated hermes-sweeper review evidence:

The linked competing-fix discussion also points to the same salvage path, so this PR is redundant with main.

@teknium1 teknium1 closed this Jul 13, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists platform/qqbot QQ Bot adapter sweeper:implemented-on-main Sweeper: behavior already present on current main type/bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: QQBot adapter busy-loops after WebSocket reconnect failure, causing 100% CPU

3 participants

@xxxigm @alt-glitch @teknium1