Skip to content

test: stabilize websocket broadcast test - #31039

Closed
josephnilo wants to merge 2 commits into
NousResearch:mainfrom
josephnilo:fix/websocket-broadcast-test-timeout
Closed

test: stabilize websocket broadcast test#31039
josephnilo wants to merge 2 commits into
NousResearch:mainfrom
josephnilo:fix/websocket-broadcast-test-timeout

Conversation

@josephnilo

Copy link
Copy Markdown

Summary

  • keep the /api/pub websocket open briefly after sending the broadcast frame
  • receive the event on the same TestClient thread instead of a background receiver thread
  • removes the CI-hanging receive race in TestPtyWebSocket

Test Plan

  • python -m pytest tests/hermes_cli/test_web_server.py::TestPtyWebSocket::test_pub_broadcasts_to_events_subscribers -q -o 'addopts='
  • repeated the targeted test 5x locally
  • python -m pytest tests/hermes_cli/test_web_server.py -q -o 'addopts='

Context: PR #30948's test slice failed on this flaky websocket broadcast test; this patch is independent of the curator change and intended to unblock release CI.

@alt-glitch alt-glitch added type/test Test coverage or test infrastructure comp/gateway Gateway runner, session dispatch, delivery P3 Low — cosmetic, nice to have labels May 23, 2026
@josephnilo

josephnilo commented May 23, 2026

Copy link
Copy Markdown
Author

Release-unblock context from triage:

This PR now addresses two independent CI blockers seen across current release-candidate PRs:

  1. Stabilizes the websocket broadcast test that failed on fix(curator): add post-archive safety guard to prevent unverified skill archival #30948 (tests/hermes_cli/test_web_server.py::TestPtyWebSocket::test_pub_broadcasts_to_events_subscribers). Verified locally with the targeted test repeated 5x plus the full tests/hermes_cli/test_web_server.py file.
  2. Fixes contributor-check false negatives / missing mappings seen on gateway: quiet Telegram operational chatter #31034, fix(memory): refuse mutations when on-disk file changed since last read (#26045) #30993, docs(xai-oauth): confirm token propagation is correct, add E2E regression tests (#29344) #30992, and fix(providers): warn on duplicate name/alias registration (#30921) #30984:
    • accepts both GitHub noreply forms (123+user@users.noreply.github.com and user@users.noreply.github.com)
    • maps linux2010@github.com, root@srv1626652.hstgr.cloud, and this fork branch's mike@hilomedia.local author email.

Local verification after the second commit:

  • python -m pytest tests/hermes_cli/test_web_server.py::TestPtyWebSocket::test_pub_broadcasts_to_events_subscribers tests/gateway/test_verbose_command.py::TestVerboseCommand::test_defaults_to_all_when_no_tool_progress_set tests/gateway/test_verbose_command.py::TestVerboseCommand::test_per_platform_isolation tests/providers/test_provider_profiles.py::TestNvidiaProfile tests/acp/test_server.py::TestSlashCommands::test_model_switch_uses_requested_provider -q -o 'addopts=' → 8 passed
  • python -m py_compile scripts/release.py
  • git diff --check

Note: GitHub currently reports no checks for this fork PR, so the local verification above is the available signal from this machine.

@alt-glitch alt-glitch added comp/dashboard Web dashboard / control panel UI (dashboard/, landing) and removed comp/gateway Gateway runner, session dispatch, delivery labels Jun 26, 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 addressing both CI concerns. The contributor-attribution portion remains useful, but the websocket-test change should not be transplanted as written.

Problems

  • tests/hermes_cli/test_web_server.py:2329 replaces the bounded receiver-thread wait with time.sleep(0.05) followed by unbounded receive_text(). A starved TestClient/ASGI thread can still miss that fixed delay and hang.
  • Current main already has a deterministic replacement at tests/hermes_cli/test_web_server.py:6865-6920: it calls _broadcast_event directly with fake subscribers and verifies both same-channel fan-out and channel isolation. The production fan-out is in hermes_cli/web_server.py:14788-14800.

Suggested changes

  • Preserve the current-main websocket test; do not salvage this PR's test hunk.
  • Salvage the attribution change separately. The CI workflow still only exempts numeric-plus noreply forms at .github/workflows/contributor-check.yml:39-44, although scripts/release.py:2154-2162 already resolves both noreply forms. Revalidate the three proposed AUTHOR_MAP identities while applying that focused change.

Automated hermes-sweeper review.

if isinstance(received, Exception):
raise received
# Give the server-side publisher coroutine a chance to fan
# out the frame before the publisher websocket is closed.

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.

A fixed 50 ms sleep does not establish that the ASGI broadcast completed; sub.receive_text() remains unbounded if that thread is delayed. Current main avoids this scheduling race by testing _broadcast_event directly with fake subscribers, so this hunk should be dropped during salvage.

@teknium1 teknium1 added sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 13, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks @josephnilo — closing as resolved on main: the websocket broadcast flake was fixed with a different approach (driving _broadcast_event directly with fake subscribers, no portal). This branch also carries unrelated workflow/release.py changes that would need their own PRs.

@teknium1 teknium1 closed this Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/dashboard Web dashboard / control panel UI (dashboard/, landing) P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants