Skip to content

Bound each browser probe's own answer so a wedged gate names the probe - #100

Merged
max-sixty merged 1 commit into
mainfrom
fix/ci-33220366482
Aug 29, 2026
Merged

Bound each browser probe's own answer so a wedged gate names the probe#100
max-sixty merged 1 commit into
mainfrom
fix/ci-33220366482

Conversation

@leaf-agent

Copy link
Copy Markdown
Collaborator

Problem

Run 33220366482 (db3f5d0) spent the step's whole 45 minutes and left no failure to read. Under the -v #81 added, both workers name what stopped them, and they stopped in two different places. gw0 printed tests/test_render_widgets.py::test_accept_all_decides_every_pending_suggestion at 23:48:35 — that is the Response.json shape #85 diagnosed, and #85 has since landed on main as e626f53. gw1 printed tests/test_smoke.py::test_a_shipped_page_passes_the_real_browser_gate at 23:49:53, at 99%, and never reported it. That second one is left over: it is one line — render_gate_model.render_version(...) — so the wait is on the gate's own path, in shipped code rather than in tests/, and it has now stopped three runs.

page.evaluate takes no timeout in any binding. render_checks.py already races the probe module's load against call.timeoutMs; the probe it then invokes was awaited with nothing bounding it. Two probes await a promise the page supplies rather than a fact it states, and both are reached that way:

  • nextFrame is new Promise(requestAnimationFrame), awaited at readings.py:188 as the last thing each scheme reads. requestAnimationFrame stops in a page the compositor has stopped drawing.
  • retiredSlots awaits Promise.allSettled over each holder's animations — a.finished, the same promise bb8b7e0 had to bound on the test side.

They are the only two: no other export under render-checks/ contains an await or a .then. #84 named nextFrame and scoped it out as product rather than suite behaviour, which is right — it belongs here rather than in tests/render_harness.py, and this PR does not touch anything #84 does.

Solution

Race each probe's answer against the deadline _INVOKE_PROBE already carries for the load, in the one place every probe goes through. Nothing new is declared: call.timeoutMs is the gate's served_timeout_ms, defaulting to SERVED_TIMEOUT_MS, which is Playwright's own 30 seconds — so a probe now runs out where every other browser wait in the gate and the suite runs out. The rejection comes back through evaluate, and render_version already turns a PlaywrightError from its probe module into a gate finding, so version check --render reports which probe stopped and the bound it passed instead of hanging.

Bounding at _INVOKE_PROBE rather than at the two probes is what keeps the layer open: a probe added later that awaits the page is bounded by existing, and the suite's own evaluate_probe callers are covered by the same edit. tests/CLAUDE.md's bounded-await rule is corrected to say so, since it currently names only the loader and would send an author to write a second race around a call that already has one.

This does not claim to bound every unbounded await in the payload — only the ones reached through evaluate_probe. An await written directly into a page.evaluate argument elsewhere is still a wait nothing bounds.

Testing

test_a_probe_that_never_answers_is_a_gate_finding, beside the loader's own deadline test. It serves the real probe facade with nextFrame replaced by a promise that never settles, runs the whole gate, and asserts the finding names the probe and the bound. The replacement holds its own resolver, as requestAnimationFrame holds the callback it was handed — the first version let it be collected, and the driver ended the wait itself with Resulting promise was garbage collected, which is a different arrangement and not the one that stopped these runs.

Bug-back on this runner, same test both ways:

this branch      1 passed in 9.51s   (failure: "probe nextFrame did not answer within 3000ms")
main's _INVOKE_PROBE   exit=143      (still waiting when a 120s cap cut it off)

On the rebased tree (b25a9e0), every suite file that reaches evaluate_probe, plus the one the wedge landed on: test_render_gate.py and test_smoke.py (66 passed), test_render_pages.py, test_render_startup.py and test_render_options.py (135 passed), test_render_widgets.py (61 passed), and test_render_export.py with test_render_projection.py (97 passed, run against 6045970 before the rebase, along with test_render_commands.py). The everyday suite uv run pytest tests is 690 passed, 6 skipped, and pre-commit run over the three changed files is clean. The full --run-nightly suite has not been run here; CI runs it on this branch.

The three runs this wait stopped

Each row is the nodeid a worker printed and never reported, found by pairing -v's start lines against its [gwN] … PASSED/FAILED lines over the whole job log:

Run Commit Unpaired nodeid
33169923393, attempt 1 16a94ad (#95's branch) test_smoke.py::test_a_shipped_page_passes_the_real_browser_gate, and test_render_options.py::test_a_question_owns_one_thread_in_the_page_and_panel
33209352445 1c5edad test_smoke.py::test_a_shipped_page_passes_the_real_browser_gate, and test_render_conversations.py::test_a_resolved_thread_can_be_reopened
33220366482 db3f5d0 test_smoke.py::test_a_shipped_page_passes_the_real_browser_gate, and test_render_widgets.py::test_accept_all_decides_every_pending_suggestion

The second nodeid on each row is #85's shape rather than this one — all three reach Traffic through round_trip. This PR does not claim them; e626f53 does.

That first row is attempt 1 because its failed jobs were re-run afterwards, and the re-run's log is what gh run view --job returns now; attempt 1's own log is still readable through actions/jobs/98844279758/logs, and that is the one parsed here.

The diagnosis on #95 named both of this PR's awaits after the first of these three runs, but #95 was a test-side fix and could not carry them. That comment is now on a closed PR, so this restates it rather than linking to it as if it were pending.


Automated fix for failed run

`page.evaluate` takes no timeout in any binding. `render_checks.py` already races
the probe module's load against `call.timeoutMs`, but the probe it then invokes
was awaited with nothing bounding it — and two probes await a promise the page
supplies rather than a fact it states: `nextFrame` waits on a rendering turn, and
`retiredSlots` on each holder's animations. A page whose compositor has stopped
drawing settles neither, so the reading ran for as long as the process lived,
with no probe named and nothing printed.

Race the answer against the same deadline the load already states. The rejection
comes back through `evaluate`, and `render_version` already turns a
`PlaywrightError` from its probe module into a gate finding, so a probe that
stops now says which one stopped and the bound it passed.
@leaf-agent

Copy link
Copy Markdown
Collaborator Author

ci on 7221ee5: lint green, test red on both attempts — but on a different single test each time, and neither is on this diff's path.

Attempt Result Failure
1 1 failed, 1455 passed, 6 skipped in 24:56 test_render_projection.py::test_claims_and_reports_share_one_canonical_update_feedassert 'effective' == 'settled', the report's disposition read after wait_for_revision(page, 2) and BOTH_STAMPS
2 1 failed, 1455 passed, 6 skipped in 21:30 test_render_outbox.py::test_a_newer_queued_action_survives_an_older_refusalBrowser.new_page: Response has been disposed, on the test's first line

Each passed in the attempt the other failed in. Neither test calls evaluate_probe, so neither reaches the four lines this PR changes: the first is a settlement read taken before the page has finished judging the note, and the second is the driver failing to hand out a page at all. test_claims_and_reports_share_one_canonical_update_feed passed 5/5 at -n0 on this runner, which classifies it no further than "did not reproduce without the load" — I have not measured a rate.

What the two attempts do say is the thing this PR is for: both ran the whole suite to a reported result — 24:56 and 21:30 — rather than spending the 45-minute step bound and naming nothing.

I have not opened a fix for either failure: they are separate concerns from this diff, and main reddens on a different test most days.

@max-sixty
max-sixty merged commit 891a2d4 into main Aug 29, 2026
4 of 6 checks passed
@max-sixty
max-sixty deleted the fix/ci-33220366482 branch August 29, 2026 03:08
max-sixty added a commit that referenced this pull request Aug 29, 2026
PR #100 bounded probe promises so CI named a stopped probe, but the
operations and their deadlines still lived inside the page. This change
removes the underlying waits: shipped probes publish synchronous
readings or readiness facts, module and frame progress is observed
through Playwright's bounded driver waits, and redundant per-holder
animation waits are gone.

It also closes the two lifecycle races exposed by PR #100's own CI:
complete-revision waits now include the server reading applied to that
document, and response-transforming route handlers are drained before
their page is disposed. The remaining image-event wait follows the same
publish-and-poll pattern.

Tests: `uv run pytest tests --run-nightly -q` (1484 passed, 1 skipped);
`uv run pre-commit run --all-files`.

> _This was written by Codex on behalf of max-sixty_
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.

2 participants