Bound the held fold's finished wait so a stall fails the test, not the job - #95
Bound the held fold's finished wait so a stall fails the test, not the job#95leaf-agent wants to merge 1 commit into
Conversation
|
This branch's first That test is one line —
So the rule this PR writes into The rerun's reading: |
|
Landed on
|
#100) ## Problem [Run 33220366482](https://github.com/max-sixty/leaf/actions/runs/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](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`](https://github.com/max-sixty/leaf/blob/b25a9e06f74cc1f994de6dc48a1e0a2ada873a05/plugins/leaf/skills/leaf/scripts/leaf/render_checks.py#L23) 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`](https://github.com/max-sixty/leaf/blob/b25a9e06f74cc1f994de6dc48a1e0a2ada873a05/plugins/leaf/skills/leaf/scripts/leaf/render-checks/runtime.js#L47) is `new Promise(requestAnimationFrame)`, awaited at [`readings.py:188`](https://github.com/max-sixty/leaf/blob/b25a9e06f74cc1f994de6dc48a1e0a2ada873a05/plugins/leaf/skills/leaf/scripts/leaf/render_gate/readings.py#L188) as the last thing each scheme reads. `requestAnimationFrame` stops in a page the compositor has stopped drawing. - [`retiredSlots`](https://github.com/max-sixty/leaf/blob/b25a9e06f74cc1f994de6dc48a1e0a2ada873a05/plugins/leaf/skills/leaf/scripts/leaf/render-checks/widgets.js#L166) 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`](https://github.com/max-sixty/leaf/blob/b25a9e06f74cc1f994de6dc48a1e0a2ada873a05/plugins/leaf/skills/leaf/scripts/leaf/render_gate/version.py#L94) 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. <details><summary>The three runs this wait stopped</summary> 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](https://github.com/max-sixty/leaf/actions/runs/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](https://github.com/max-sixty/leaf/actions/runs/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](https://github.com/max-sixty/leaf/actions/runs/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. </details> --- Automated fix for [failed run](https://github.com/max-sixty/leaf/actions/runs/33220366482) Co-authored-by: leaf-agent <318509791+leaf-agent@users.noreply.github.com>
…the job (#154) ## Problem The review on #153 ([review](#153 (review))) raised this while that PR was in flight; it merged before the point was actioned, so the shape is on `main` now. `margins_laid_out` awaits the `/runtime/margin-layout.js` import inside `page.evaluate`, which `tests/CLAUDE.md` names as the unbounded case: "`page.evaluate` takes no timeout in any binding, so a promise awaited inside it — an animation's `finished`, a module's load, a listener's next call — is a wait nothing bounds. It does not fail in thirty seconds naming its test: it spends the job's whole step, and the share of the suite already handed to that worker never runs." That is the class #84 and #95 bound at eleven other sites; this helper was added after them and reintroduces it at three call sites — `test_render_pages.py` twice and `test_render_margin.py` once — all of them on the render suite's ordinary path. ## Solution Poll the import from the driver with `wait_for_function` against `render_checks.SERVED_TIMEOUT_MS`, the same patience the payload's own probes carry. The deadline moves to the driver side, which is the shape `tests/CLAUDE.md` prefers; the round trip count and the frame the layout runs in are unchanged. The docstring now says why the wait is bounded from outside, so the next reader does not have to re-derive it. ## Testing On this branch, rebased on `6d68a1b`: - `uv run pytest tests` — 740 passed, 6 skipped. That is the gate CI runs on this PR. - `uv run pytest tests/test_render_pages.py::test_a_left_sidebar_uses_the_margin_until_the_page_needs_it_back --run-nightly -n0` — passes. This is the test #153 fixed, and the one the helper was written for. - `uv run pytest tests/test_render_margin.py tests/test_render_pages.py --run-nightly -n2` — 62 passed, 2 failed. Both failures are pre-existing and untouched here (below). - `ruff check` and `ruff format --check` over the changed file pass. <details><summary>The two nightly failures, and why they are not this change</summary> `test_render_margin.py::test_g_addresses_the_page_map_prefix_in_its_announced_order` and `test_one_margin_item_owns_a_targets_controls_information_and_more_actions` both wait for `m\s*page-map items` on the key line and read `gm1–9page-map items` / `gm1–4page-map items` instead. I reran both at `-n0` with this diff stashed, on `6d68a1b` unmodified: they fail identically. They belong to `aa41f7a` ("Keep chord hints stable through progress"), which changed the chord row to carry the item count, and #153's body already named them on its own base. No open issue tracks them yet; the next `ci` on `main` should route them to `tend-ci-fix`. </details> --------- Co-authored-by: leaf-agent <318509791+leaf-agent@users.noreply.github.com>
Problem
The
testjob on run 33165671503 spent the step's whole 45 minutes and left no failure to read. Under the-v#81 added, the log names what stopped:gw1printedtests/test_render_conversations.py::test_a_thread_reopened_mid_fold_folds_again_when_it_settlesat 11:11:32 and never reported it, whilegw0drained the rest and stopped at 81%, twenty-six minutes before the bound cut the job off.Every wait that test makes is Playwright's and runs out in thirty seconds naming its nodeid, except one: it awaits the held fold's
finishedinsidepage.evaluate, which takes no timeout in any binding. An animation that never settles is therefore a wait nothing bounds, and what should have been one test failing became the job's whole cap plus the share of the suite xdist had already handed that worker.Solution
Race that await against a
setTimeoutthat rejects naming the motion, so the wait states its end as well as its fact. Thirty seconds, because that is where every other browser wait in the suite runs out. The rejection comes back throughevaluateas the failure of the test that asked for it, which is the naming a wedge has never had.This is the same class #84 bounds for rendering-frame promises, and the instance its sweep did not reach —
finishedis not a frame promise, and this is the suite's only await on one. The two are independent: #84 ownspyproject.toml's note about the class, so this branch leaves it alone.tests/CLAUDE.mdgains the rule under A wait consumes a fact the system states, since the shape recurs wherever a test awaits a page promise rather than a fact the page states.Testing
The edited test, 5 runs at
-n 0:1 passedeach, ~4.2s. Whole file at the suite's own-n 2 --dist loadgroupwith--run-nightly:41 passed in 46.8s.ruff checkclean overtests/;ruff formatleaves this diff untouched.Bug-back — the deadline fires, and it names what stopped:
Against
main's expression the same page runs until whatever cap is above it — which is the run this fixes.Why the wedge has to be this wait, when it did not reproduce
62 runs of the test locally — 30 sequential at
-n 0, then 32 across four concurrent loops on a four-core runner — all passed. So the classification is by elimination rather than by repro, and the elimination is complete: every other call the test makes carries a deadline.clickandwait_for_functiontake Playwright's 30s default (nothing here callsset_default_timeout; the one place that does istest_render_gate.py, and it lowers it),expecttakes 5s,round_tripandundogo through_until's explicit 30s,toldcarries its own 30s, andevents_modelreads and appends are Python-side file work. Had any of them blown, the run would show a failure with the nodeid within thirty seconds. It showed nothing for thirty-seven minutes, so the wait was one that cannot run out — andpage.evaluateis the only unbounded call in the test, withawait m.finishedthe only thing in it that can block.The one-in-ten rate fits:
testwas green on the nine otherciruns onmainthis morning, each of which ran this test once.The other half of this run: `lint`
lintwas also red, on theruff formatrewrite of the two assertsdbfaeaaadded to this same file. That is #94, which is open and green, and it is the failure on all ten ofmain's red runs this morning. It is deliberately not duplicated here — solinton this PR is red for that pre-existing reason until #94 or an equivalent lands, andtestis the check this PR is answerable for.Automated fix for failed run