Skip to content

test: deflake pty_session wall-clock waits into predicate witnesses - #735

Merged
Kyzcreig merged 1 commit into
mainfrom
fix/deflake-pty-reaper-loop
Sep 19, 2026
Merged

Kyzcreig merged 1 commit into
mainfrom
fix/deflake-pty-reaper-loop

Conversation

@Kyzcreig

Copy link
Copy Markdown
Collaborator

Defect

tests/test_pty_session.py::test_reaper_loop_invokes_reap is a wall-clock flake: it starts the reaper at interval=0.01, sleeps 0.05, then asserts the reap counter >= 2. On a loaded self-hosted runner (ace-ai slot 8, run 35439419317, slice 12/16) it observed assert 1 >= 2 and went red on PR #730, which touches agent/credential_pool.py only. The stopwatch was measuring the machine, not the reaper.

Conversion

Tests only — hermes_cli/pty_session.py is untouched, no production seam was needed.

  • test_reaper_loop_invokes_reap now awaits an asyncio.Event the fake reap_idle sets on its second call, under a 10s wait_for hang guard. The witness is the iteration count reached, so load costs time, never correctness.
  • The four sibling drain tests in the same file replaced fixed asyncio.sleep(0.05) budgets with a wait_until(predicate) helper that polls the exact fact each test asserts (buffer contents / socket close code). Converted in one pass per wall-clock-flake-conversion — a single converted test in a required-check file keeps taxing every other PR.

RED proofs

Each mutation's red names the new witness, not an incidental error:

mutation red
run_reaper while True → for _ in range(1) AssertionError: reaper loop only invoked reap_idle 1 time(s)
_drain self.buffer.append(chunk) → pass 3 failures, all timed out after 10.0s waiting for the drain task to buffer ...
EOF branch stalled with sleep(3600) timed out after 10.0s waiting for the drain task to observe EOF and close the socket

Production restored and re-verified green after each.

Load proof

scripts/run_tests.sh tests/test_pty_session.py → 9 passed.

20x loops with 64 CPU burners on 32 cores (load average 14 → 75):

  • reaper test alone: 20/20 PASS
  • whole file: 20/20 PASS (9 passed each run)

Upstream

The identical five assertions exist upstream at tests/hermes_cli/test_pty_session.py (our fork carries the file at tests/test_pty_session.py). Upstream PR opened separately and linked in a comment; not waiting on it.

test_reaper_loop_invokes_reap slept 0.05s at interval=0.01 and asserted the
reap counter >= 2. On a loaded self-hosted runner it observed "assert 1 >= 2"
and went red on PR #730, which touches agent/credential_pool.py only. The
stopwatch measured the machine, not the reaper.

Conversions (tests only; hermes_cli/pty_session.py is untouched):

- test_reaper_loop_invokes_reap now awaits an asyncio.Event the fake reap sets
  on its second call, under a 10s wait_for hang guard. The witness is the
  iteration COUNT reached, so load costs time, never correctness.
- The four sibling drain tests replaced fixed asyncio.sleep(0.05) budgets with
  a wait_until(predicate) helper that polls the exact fact each test asserts
  (buffer contents / socket close code). Same family, same file, converted in
  one pass so the file stops taxing unrelated PRs.

RED proofs, each red naming the new witness (not an incidental error):
- run_reaper "while True" -> "for _ in range(1)":
  AssertionError: reaper loop only invoked reap_idle 1 time(s)
- _drain "self.buffer.append(chunk)" -> pass: 3 failures, all
  "timed out after 10.0s waiting for the drain task to buffer ..."
- EOF branch stalled with sleep(3600):
  "timed out after 10.0s waiting for the drain task to observe EOF and close
  the socket"

GREEN: scripts/run_tests.sh tests/test_pty_session.py -> 9 passed.
Load proof: 20/20 green for the reaper test and 20/20 for the whole file with
64 CPU burners on 32 cores (load average 14 -> 75).
@Kyzcreig

Copy link
Copy Markdown
Collaborator Author

Upstream port opened: NousResearch#116117 (same five assertions, upstream path tests/hermes_cli/test_pty_session.py; verified green 16/16 and both mutations RED-by-name on a clean origin/main worktree). Not waiting on it.

@Kyzcreig

Copy link
Copy Markdown
Collaborator Author

Slice 6/16 red is a pre-existing load flake, not this diff

tests/gateway/test_browser_control_broker_hardening.py::test_failed_deferred_cancel_flush_keeps_controller_offline → Failed: DID NOT RAISE ConnectionError (job 105909008161).

This PR's commit touches one file: tests/test_pty_session.py (git diff --stat fork/main..HEAD → 45 insertions, 10 deletions, that file only). Zero gateway code.

Control, run on a clean fork/main worktree (6aedf5c969) with no PR diff applied, Mac Studio 32 cores:

condition result
idle, 10x 10/10 PASS
64 CPU burners (load avg ~13), 10x 5/10 PASS, 5/10 FAIL

Failures on the clean base reproduce CI exactly:

  • test_failed_deferred_cancel_flush_keeps_controller_offline — DID NOT RAISE ConnectionError (3/10)
  • test_timeout_while_disconnected_flushes_cancel_before_new_dispatch (2/10)

Cause is the same family this PR fixes: BrowserControlBroker(command_timeout=0.01) + a worker thread + thread.join(timeout=1.0) — a 10 ms wall-clock budget the deferred-cancel path must hit while the runner is busy. Filed as its own card for conversion; deliberately not widening this verified branch.

Re-running the slice.

@Kyzcreig
Kyzcreig added this pull request to the merge queue Sep 19, 2026
Merged via the queue into main with commit 27c6276 Sep 19, 2026
95 of 97 checks passed
@Kyzcreig
Kyzcreig deleted the fix/deflake-pty-reaper-loop branch September 19, 2026 17:02
Kyzcreig added a commit that referenced this pull request Sep 20, 2026
FleetReview scored the previous head 2/5 with four P1s. Each was
reproduced with a failing test before being fixed; the repros are kept as
regression tests.

P1 publish-e2e-evidence.yml:41 — masked failures. The blanket
`continue-on-error: true` suppressed missing artifacts, bad credentials
and a broken gh extension too, so the workflow could report success while
attaching nothing. The step body moves to scripts/ci/publish_evidence_step.sh,
which tolerates ONLY the rate-limit class and exits non-zero on everything
else. Both directions are pinned: a mutation that tolerates everything
fails 3 tests, one that tolerates nothing fails the rate-limit test.

P1 live_comment.py:1048 — lost reporting. Merge-queue membership is not
terminal (a required check can fail, a human can dequeue) and merge-group
runs never comment, so exiting on a queued PR left a later dequeue with no
reporter. The check moves from main() into the poll loop as a pause +
recheck, throttled to one GraphQL call per 300s so it does not become a new
per-cycle charge. The throttle is itself regression-tested (unthrottled:
15 checks over 13 cycles).

P1 live_comment.py:140 — stale backoff. `retry_after` accumulated with
max() and was never cleared, so the largest delay ever seen outlived its
response: after a 900s limit, a later 5s limit still slept 900s, which
_nap clamps to the remaining timeout. It now reflects only the response
being observed, and a response carrying no delay clears it.

P1 live_comment.py:507 — lost comment. A 404 on PATCH cleared the cached
id and deferred recreation to "next cycle", which is a no-op when the
quiet grace is spent and the loop is about to break. The POST is now
issued immediately, bounded to one retry. A 500 still does not recreate
(that would post a duplicate).

P2 test_live_comment_rate_limit.py:373 — tautological test. It built a
look-alike parser with the expected default hardcoded. The parser is
extracted to build_parser() and asserted against; a second test pins that
main() actually uses it.

P3 — artifact download/extract paths were keyed by name while the cache
was keyed by id, so an `overwrite: true` re-upload (every review-status-*)
could serve a stale extract for a new id. Paths are now keyed by name+id.

P3 — dropped the dead `list_key` parameter on `_conditional_get`
(`_api_get_paginated` applies it itself).

Verified:
- tests/ci 304/304 green under the runtime venv with the worktree on
  PYTHONPATH and a sandboxed HOME.
- 8 single-behaviour mutations, each failing exactly its own test(s) and
  nothing else.
- Live run against CI run 35481474867: steady-state cycles charge 0
  requests (19 served as free 304s over 4 cycles); installation remaining
  moved 4953 -> 4950 across the whole session.
- pr_is_in_merge_queue probed against real PRs #736/#738/#735.
- shellcheck clean on the new script.
@Kyzcreig
Kyzcreig restored the fix/deflake-pty-reaper-loop branch September 21, 2026 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant