Skip to content

fix(tests): drain blocker completions in the async-delegation capacity test - #64369

Closed
Soju06 wants to merge 1 commit into
NousResearch:mainfrom
Soju06:upstream-pr/fix-async-delegation-flake
Closed

fix(tests): drain blocker completions in the async-delegation capacity test#64369
Soju06 wants to merge 1 commit into
NousResearch:mainfrom
Soju06:upstream-pr/fix-async-delegation-flake

Conversation

@Soju06

@Soju06 Soju06 commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Problem

tests/tools/test_async_delegation.py::test_crashed_runner_produces_error_completion fails intermittently in CI (and locally) with:

AssertionError: assert 'completed' == 'error'

test_dispatch_rejected_at_capacity releases its two blocker workers with ev.set() and returns immediately. The workers' completed events land on the shared process_registry.completion_queue asynchronously — often after the next test's autouse start-of-test drain has already run on a still-empty queue. The crashed-runner test's _drain_one() then picks up a leaked completed event from the previous test instead of its own crash's error event.

Reproduction (running just the two tests in sequence): 6/15 runs fail on main, 0/15 with this fix.

This flake currently trips Run tests slices on unrelated PRs (e.g. it failed three different PR runs today on three different slices).

Change

Test-only: the capacity test consumes both blockers' completion events (_drain_one() × 2) before returning, so nothing leaks past the next test's drain. Assertions unchanged.

Tests

  • Full module: 27 passed (×3 consecutive runs).
  • Targeted pair stress: 15/15 clean with the fix vs 9/15 on main.

🤖 Generated with Claude Code

@alt-glitch alt-glitch added type/test Test coverage or test infrastructure tool/delegate Subagent delegation sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation P3 Low — cosmetic, nice to have labels Jul 14, 2026
@Soju06

Soju06 commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Noting #64431 removed the flaky test outright — this PR is the alternative that keeps the coverage: the flake was the capacity test leaking its blockers' late completion events into the next test's drain, not a defect in the crashed-runner test itself (0/15 failures with the drain vs 6/15 on the old main). Happy to rebase this to restore the removed test alongside the drain fix if you'd rather keep the assertion; equally fine closing this if the deletion is the intended end state.

test_dispatch_rejected_at_capacity releases its two blocker workers
with ev.set() and returns; their completion events enqueue
asynchronously after the next test's start-of-test drain has already
run. test_crashed_runner_produces_error_completion then drains a
leaked 'completed' event instead of its own crash's 'error' event —
reproduced 6/15 runs on main, 0/15 with the drain. Consume both
completions before the test returns.
@Soju06
Soju06 force-pushed the upstream-pr/fix-async-delegation-flake branch from 0bab7ed to 09f7c64 Compare July 14, 2026 14:43
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for tracing the completion-queue race and documenting the alternative to #64431. This is an automated hermes-sweeper review; current main has superseded this patch.

  • f5c2ea49a4716ccd377c99511ee1bb9be1b56275 added module-level teardown at tests/tools/test_async_delegation.py:28-37, which waits for async workers to finish before draining the shared completion queue. This covers the late-completion leak for the capacity test and other tests in the module.
  • The wait covers the enqueue window: active_count() includes finalizing records at tools/async_delegation.py:410-413, and completion events are enqueued before the record becomes terminal at tools/async_delegation.py:815-826.
  • c084085a3e520178fb3aa27c6ba7a411df8c79a0 (test: remove flaky test_crashed_runner_produces_error_completion #64431) removed test_crashed_runner_produces_error_completion, the test this PR was intended to preserve. As noted in the discussion, restoring deterministic coverage would be a separate follow-up if desired.

Closing as implemented on main.

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

Labels

P3 Low — cosmetic, nice to have sweeper:implemented-on-main Sweeper: behavior already present on current main sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation tool/delegate Subagent delegation type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants