Skip to content

fix(qqbot): clean up resources after reconnect failure - #41773

Open
k176060444-lgtm wants to merge 2 commits into
NousResearch:mainfrom
k176060444-lgtm:fix/qqbot-reconnect-failure-cleanup
Open

fix(qqbot): clean up resources after reconnect failure#41773
k176060444-lgtm wants to merge 2 commits into
NousResearch:mainfrom
k176060444-lgtm:fix/qqbot-reconnect-failure-cleanup

Conversation

@k176060444-lgtm

Copy link
Copy Markdown

Summary

Clean up stale QQBot WebSocket and aiohttp session resources when a reconnect attempt fails.

Context

The closed-WebSocket CPU busy-loop guard has already been merged upstream. This PR keeps the remaining focused resource-cleanup improvement:

  • capture stale WebSocket and session references;
  • clear adapter fields before awaiting cleanup;
  • close the WebSocket and session independently;
  • keep reconnect failure handling best-effort;
  • ensure cleanup errors do not interrupt later reconnect attempts.

Tests

  • python3 -m pytest tests/gateway/test_qqbot.py -q — 166 passed
  • added regression coverage for:
    • stale WebSocket and session cleanup;
    • missing session handling;
    • session creation followed by WebSocket connect failure;
    • cleanup exceptions;
    • WebSocket close failure without skipping session cleanup.

Related

This extracts the remaining resource-cleanup improvement from #39430 after the primary busy-loop guard was merged upstream.

@liuhao1024

Copy link
Copy Markdown
Contributor

Code Review — Clean ✅

Reviewed the diff (2 files, ~160 lines). Findings:

  1. Resource cleanup pattern is correct — capture refs, null fields early, then close independently in try/except blocks. This prevents: (a) double-close from concurrent reconnect attempts, (b) one close failure skipping the other cleanup.

  2. Test coverage is thorough — 5 test cases covering: normal cleanup, missing session, session created by _open_ws before failure, cleanup exception handling, and ws.close() failure not blocking session.close(). The test_cleanup_ws_close_failure_does_not_skip_session_close test is particularly valuable — it validates the independent cleanup guarantee.

  3. Logging at debug level — cleanup failures logged at debug not warning, which is appropriate since these are best-effort operations during an already-failed reconnect.

No issues found. LGTM.

@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 Jun 8, 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 the focused reconnect cleanup. The premise remains valid on current main: gateway/platforms/qqbot/adapter.py:468 assigns _session before ws_connect(), while the failure handler at gateway/platforms/qqbot/adapter.py:684-686 only logs and returns. The proposed clear-before-close, independent best-effort cleanup addresses that path.

Problems

  • tests/gateway/test_qqbot.py:2253 calls _reconnect(0) with the production delay still active. gateway/platforms/qqbot/constants.py:41 defines the first retry delay as two seconds; the five added tests therefore add about ten seconds of real waiting.

Suggested changes

  • Stub asyncio.sleep or patch the backoff value for this test class.

Automated hermes-sweeper review.

Comment thread tests/gateway/test_qqbot.py
@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
@k176060444-lgtm
k176060444-lgtm requested a review from teknium1 July 14, 2026 10:01
@k176060444-lgtm
k176060444-lgtm force-pushed the fix/qqbot-reconnect-failure-cleanup branch from ffb234b to 2fafd49 Compare August 1, 2026 10:34
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 platform/qqbot QQ Bot adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades 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