From 560bd6f517ff53838fb094c4a74fd8478b01b3e5 Mon Sep 17 00:00:00 2001 From: Adam Clemens Date: Sun, 13 Sep 2026 00:09:10 +0100 Subject: [PATCH 1/2] TASK-053: a frame that raises now fails the run `DivergenceDidNotConvergeError` raised inside `RenderWindow._draw`'s own `on_frame` callback was swallowed by `rendercanvas`'s `log_exception("Draw error")` block, which logs and continues by design. PyFlow never saw it. Measured on the shipped cavity refined to 64x64 with its own timestep left alone: 22 of 40 frames failed, and the CLI printed `pyflow exited cleanly` and returned 0. On glfw the same raise also skipped `on_draw`'s reschedule, so a --max-frames run never reached its budget and hung -- observed past 300s, now exiting 1 in 22s. This falsified the second of the two Stage 4 use cases Stage 9 exists for: "solve a linear system, and be told when it did not converge instead of receiving a plausible wrong answer". `_draw` now catches, records and closes; `run` re-raises from both branches once the event loop has let go. `rendercanvas` offers no error-handler API to opt out of its catch -- checked directly, not assumed -- so the seam has to be on this side of the boundary. `__main__.py` needed no change: an exception out of `main()` already gives exit 1 with the real traceback, which is how record/resume have always propagated. One thing worth reading, because the first attempt was wrong. Moving `frame_count += 1` to after `on_frame` -- so a frame that died would not count as drawn -- broke the HUD: it reads `frame_count` during `on_frame`, so every run's step readout came out one low, and a three-frame run displayed `step 2 t = 20 ms`. Found by an existing test failing, not by reasoning. The increment is back where it was, with a rollback in the except branch, which keeps both properties. Both halves are mutation-verified: removing the re-raise fails 6 tests, removing the rollback fails 1. Dates: drafted 2026-09-12, built 2026-09-13, and the roadmap says both rather than flattening them to one. That distinction is what `make check-dates` exists for. `make ci` green: 1214 passed, 156 scenarios, all 15 structural checks. Verified by hand against the real CLI: a diverging run exits 1 with the engine's diagnostic and no "exited cleanly"; a healthy run still exits 0 and still says so. Co-Authored-By: Claude Opus 5 --- README.md | 8 +- docs/planning/roadmap.md | 56 +++++- docs/planning/status.md | 14 +- docs/repository-inventory.md | 3 +- docs/repository-manifest.md | 7 +- src/pyflow/bootstrap.py | 8 + src/pyflow/rendering/CLAUDE.md | 27 +++ src/pyflow/rendering/window.py | 73 +++++++- tests/integration/CLAUDE.md | 16 ++ tests/integration/test_frame_failure.py | 219 ++++++++++++++++++++++++ tests/unit/test_rendering.py | 66 +++++++ 11 files changed, 477 insertions(+), 20 deletions(-) create mode 100644 tests/integration/test_frame_failure.py diff --git a/README.md b/README.md index 2c3140b..0d9abd4 100644 --- a/README.md +++ b/README.md @@ -149,9 +149,11 @@ and the lid-driven cavity's own error against Ghia, Ghia & Shin (1982) *fell at every resolution* (9x9 0.1433 -> 0.1292, 13x13 0.0874 -> 0.0766, 17x17 0.0578 -> 0.0524), which is independent evidence the change was physics rather than a re-fitted tolerance. Its own demo is -`uv run python -m pyflow run --demos sealed_box`. TASK-053 (a failed -frame failing the run) and TASK-054 (the timestep stability warning) -are drafted and not yet built. It is placed before Better Numerics by dependency, +`uv run python -m pyflow run --demos sealed_box`. **TASK-053 has landed too**: a run whose frames raise now exits non-zero +with the engine's own diagnostic instead of printing `pyflow exited +cleanly` and returning 0, and a `--max-frames` interactive run that +blows up terminates in 22 s rather than hanging past 300. TASK-054 (the +timestep stability warning) is drafted and not yet built. It is placed before Better Numerics by dependency, not preference -- Stage 10's own Rayleigh-Bénard criterion measures convection between heated walls, which is not meaningful while those walls leak. diff --git a/docs/planning/roadmap.md b/docs/planning/roadmap.md index f9580c1..0e90605 100644 --- a/docs/planning/roadmap.md +++ b/docs/planning/roadmap.md @@ -381,10 +381,12 @@ This paragraph previously said `make install` and `make test` were still expected to fail, pending `uv.lock` and a test suite (B2/C1) -- stale since 2026-08-16 and corrected 2026-08-19. Both now succeed: `uv.lock` is committed (B2) and `make test` runs the suite with coverage -(C1a/C1b): **1226 tests as of 2026-09-12**, up from 1209 on 2026-09-11 +(C1a/C1b): **1234 tests as of 2026-09-12**, up from 1209 on 2026-09-11 (TASK-052, Stage 9: 9 in `tests/unit/test_boundary_velocity.py` and 3 in `tests/golden/test_sealed_box.py` for the wall-permeability fix and its -own golden demo, plus 5 the fixtures those changed gained along the way). +own golden demo, plus 5 the fixtures those changed gained along the way; +then TASK-053: 5 in `tests/integration/test_frame_failure.py` and 3 in +`tests/unit/test_rendering.py` for a frame that raises failing the run). Before that, 1209 on 2026-09-11 (the Stage 8 exit audit, closing the gap between three Completion Criteria and what actually checked them: 4 in @@ -13015,8 +13017,10 @@ own status table. ## TASK-053 — A Failed Frame Fails The Run -**Status: Not started, drafted 2026-09-12.** Will discharge Completion -Criterion 4. +**Status: Done, 2026-09-13.** Discharges Completion Criterion 4. +Drafted 2026-09-12, built the following day -- the dates differ and +both are real, which is the distinction `make check-dates` exists to +keep honest. ### Purpose @@ -13062,6 +13066,18 @@ killed. already gives exit 1 with the real traceback -- exactly how `record`/`resume` propagate today. +### Artifacts Produced + +- `src/pyflow/rendering/window.py` -- `_draw` catches, records and closes; + `_raise_any_frame_error` re-raises from both of `run`'s branches; + `frame_count` moved to after a successful `on_frame`; the offscreen + loop breaks rather than running its budget out over failing frames. +- `src/pyflow/bootstrap.py` -- a comment at the `window.run(...)` call + site recording that `"pyflow exited cleanly"` is now unreachable on a + failed run, and that the guarantee lives one function away. +- Tests: 4 in `tests/integration/test_frame_failure.py` (3 offscreen, 1 + display-guarded glfw), 3 in `tests/unit/test_rendering.py`. + ### Acceptance Criteria Prose bullets rather than a `.feature` file, the same scope judgement @@ -13072,10 +13088,36 @@ scope is "real simulation work". - A run that raises inside a frame exits non-zero and prints the engine's own diagnostic, asserted as an exit code **and** a stderr substring, per `tests/integration/`'s own convention -- an exit code alone does - not distinguish a real failure from argparse. -- Covered on both backends and from both `run` and `play`. -- A `--max-frames` `glfw` run that raises terminates rather than hanging. + not distinguish a real failure from argparse. **Met**, against a real + configuration that genuinely diverges rather than a monkeypatched + exception: the 64x64 cavity whose own measurements are in the Purpose + above. +- It does not print `pyflow exited cleanly`, and does not log a + completed frame budget. **Met**, both asserted separately from the + exit code, because the three failed independently. +- A healthy run still exits 0 and still says so. **Met** -- a guard that + always trips is a guard nobody can act on. +- Covered on both backends and from both `run` and `play`. **Met with a + stated limit.** The offscreen path is covered end to end; the glfw + path has its own integration test, **display-guarded, so it runs on + Windows CI and skips on Linux** -- the asymmetry + `docs/planning/backlog.md` already carries an open item for, recorded + here rather than left implicit. `play` is covered structurally rather + than end to end: it renders pre-materialized frames, so the engine's + own divergence cannot arise inside its frame callback at all, and what + can (an error in its own scene rebuilding) goes through the same + `RenderWindow` the three unit tests exercise directly. +- A `--max-frames` `glfw` run that raises terminates rather than + hanging. **Met** -- measured at **22 s to exit 1**, against a + pre-fix run observed still hanging at 300 s. - Mutation-verified: removing the re-raise fails the new test. + **Confirmed** -- it fails five, across both the unit and integration + levels, and was reverted before anything was called done. + +### Discharges + +Completion Criterion 4 in full, with the platform limit on its glfw half +stated above rather than glossed. --- diff --git a/docs/planning/status.md b/docs/planning/status.md index 1606f91..cfed84f 100644 --- a/docs/planning/status.md +++ b/docs/planning/status.md @@ -17,14 +17,14 @@ demand, not part of this file. ## Progress -**53/55 tasks complete (96%)** across 17 planned stages. For the full plan, including +**54/55 tasks complete (98%)** across 17 planned stages. For the full plan, including stages below not yet broken into tasks: [roadmap.md](roadmap.md). ```mermaid pie showData title "Tasks across the roadmap" - "Done" : 53 - "Not started" : 2 + "Done" : 54 + "Not started" : 1 ``` ### Milestones @@ -41,12 +41,12 @@ pie showData ### Up next -**Stage 9 -- Solver & Run Integrity** is next, starting with TASK-053 (A Failed Frame Fails The Run), 1 more not yet started in this stage. +**Stage 9 -- Solver & Run Integrity** is next, starting with TASK-054 (Timestep Stability Warning). ## Live repository facts - **49** `CLAUDE.md` files -- **1226** tests collected +- **1234** tests collected - **156** Gherkin scenarios (`tests/features/*.feature`) ## Stages @@ -168,12 +168,12 @@ pie showData ### Stage 9 -- Solver & Run Integrity -**no status recorded** -- `███░░░░░░░` 1/3 tasks; 7 criteria defined, no status line yet +**no status recorded** -- `███████░░░` 2/3 tasks; 7 criteria defined, no status line yet | Task | Status | Date | Artifact | |------|--------|------|----------| | TASK-052 -- Prescribed Boundary Velocity Reaches The Schemes | Done | 2026-09-12 | `examples/golden-demos/smoke_transport.yaml` | -| TASK-053 -- A Failed Frame Fails The Run | Not started | | | +| TASK-053 -- A Failed Frame Fails The Run | Done | 2026-09-13 | `base.py` | | TASK-054 -- Timestep Stability Warning | Not started | | | ### Stage 10 -- Better Numerics diff --git a/docs/repository-inventory.md b/docs/repository-inventory.md index a90b3c5..65621f7 100644 --- a/docs/repository-inventory.md +++ b/docs/repository-inventory.md @@ -16,7 +16,7 @@ reading job and lives in the manifest. Test counts and coverage are not here either -- those come from running the suite, not from listing files. -**368 tracked files** across 49 directories; +**369 tracked files** across 49 directories; 2 are empty. ## (root) @@ -403,6 +403,7 @@ listing files. - `test_claude_hooks.py` - `test_cli.py` - `test_fluid_configuration.py` +- `test_frame_failure.py` - `test_import_order.py` - `test_interactive_window.py` - `test_playback_cli.py` diff --git a/docs/repository-manifest.md b/docs/repository-manifest.md index b0a9157..fde063f 100644 --- a/docs/repository-manifest.md +++ b/docs/repository-manifest.md @@ -1011,7 +1011,12 @@ window with a genuine injected Space key proving the rendered pixels stop changing once paused -- the same `_needs_a_real_display` pattern `test_interactive_window.py` established, its own display-probe helper copied locally rather than imported since `tests/` is not an importable -package here). The repository-tooling tests +package here), and `test_frame_failure.py` (TASK-053, 2026-09-13: a +real configuration that genuinely diverges, run through the CLI, proving +a run whose frames raise exits non-zero with the engine's own diagnostic +rather than printing `pyflow exited cleanly` and returning 0 -- plus a +display-guarded glfw case for the hang that same defect caused). +The repository-tooling tests live in `unit/` alongside them: `test_check_docs.py`, `test_check_claims.py`, `test_check_graph.py` and `test_generate_docs_index.py`/`test_generate_dependency_tree.py`/ diff --git a/src/pyflow/bootstrap.py b/src/pyflow/bootstrap.py index 8cc1f8e..f0bae46 100644 --- a/src/pyflow/bootstrap.py +++ b/src/pyflow/bootstrap.py @@ -1175,6 +1175,14 @@ def _on_frame() -> None: window.apply_camera_config() + # `run` re-raises anything the frame callback raised (TASK-053, + # Stage 9, 2026-09-13), so this line is unreachable on a failed run and the + # exception propagates out of `main()` to a non-zero exit. It used to + # be reached unconditionally: `rendercanvas` swallowed the + # exception, `run` returned normally, and a run whose every frame had + # failed logged that it exited cleanly and returned 0. Stated here + # rather than left implicit, because nothing about this call site + # shows that the guarantee lives one function away. window.run(max_frames=max_frames, on_frame=on_frame) logger.info("pyflow exited cleanly") return window diff --git a/src/pyflow/rendering/CLAUDE.md b/src/pyflow/rendering/CLAUDE.md index 635ec0f..d7ccbf6 100644 --- a/src/pyflow/rendering/CLAUDE.md +++ b/src/pyflow/rendering/CLAUDE.md @@ -1,5 +1,32 @@ # CLAUDE +**`RenderWindow._draw` catches whatever `on_frame` raises, records it, +and closes the canvas; `run` re-raises it afterward (TASK-053, Stage 9, +2026-09-13).** The exception must not be allowed to escape `_draw`, +because `_draw` is installed as `rendercanvas`'s own `_draw_frame` and +that library calls it inside `with log_exception("Draw error")` -- +which logs and continues by design, in its own words "otherwise we +crash". Before this task a diverging simulation's +`DivergenceDidNotConvergeError` went into that block and nowhere else: +`pyflow run` printed `pyflow exited cleanly` and returned **0** after 22 +of 40 frames had failed. On `glfw` the same raise also skipped +`on_draw`'s own reschedule, so a `--max-frames` run never reached its +budget and **hung** -- observed past 300 s, now exiting 1 in 22 s. + +**`rendercanvas` offers no way to opt out of that catch, checked rather +than assumed**: no `set_*error*`, `error_handler`, `excepthook` or +`on_error` anywhere in the package, and `log_exception` de-duplicates by +message hash, so a repeating failure degrades to one-liners. The seam +that works is PyFlow's own, on this side of the boundary. **Any new +callback this window invokes on behalf of a caller needs the same +treatment** -- an exception that reaches `rendercanvas` is an exception +nobody sees. + +**`frame_count` is incremented only after `on_frame` returns**, for the +same reason: it used to be incremented before, so a frame that died in +the simulation still counted as drawn, and a failing run could report a +full frame budget. + Rendering subsystem: window/render-loop bootstrap (`docs/planning/roadmap.md` TASK-007) and visualisation of scalar/vector fields. diff --git a/src/pyflow/rendering/window.py b/src/pyflow/rendering/window.py index b6efc39..c4a5f28 100644 --- a/src/pyflow/rendering/window.py +++ b/src/pyflow/rendering/window.py @@ -181,6 +181,11 @@ def __init__(self, config: RenderingConfig) -> None: would otherwise be a local closure variable nothing outside `play()` could see.""" self._on_frame: Callable[[], None] | None = None + self._frame_error: Exception | None = None + """Whatever `on_frame` last raised, if anything (TASK-053). + `run` re-raises it; `_draw` cannot, because the caller above it + is `rendercanvas`, which swallows. + """ self._pan_drag_start_screen: tuple[float, float] | None = None self._pan_drag_start_position: tuple[float, float, float] | None = None @@ -251,10 +256,68 @@ def _end_pan(self) -> None: self._pan_drag_start_position = None def _draw(self) -> None: + """One frame: render, then advance whatever `run`'s own `on_frame` + callback advances. + + **Anything `on_frame` raises is caught here, recorded, and the + window closed (TASK-053, Stage 9, 2026-09-13) -- not allowed to escape + `rendercanvas`.** This method is installed as that library's own + `_draw_frame`, and it calls it inside `with + log_exception("Draw error")`, which logs and continues by design + ("otherwise we crash", in its own comment). So an exception that + escapes here is not propagated to anybody: before this task, + `DivergenceDidNotConvergeError` from a diverging simulation was + swallowed exactly that way, and `pyflow run` printed `pyflow + exited cleanly` and returned 0 after 22 of 40 frames had failed. + + `rendercanvas` exposes no error-handler API to opt out of that -- + checked directly, not assumed: there is no `set_*error*`, + `error_handler`, `excepthook` or `on_error` anywhere in the + package, and `log_exception` de-duplicates by message hash, so a + repeating failure degrades to one-liners. The seam that works is + PyFlow's own: catch before the boundary, stash, and let `run` + re-raise once the loop is over. + + **`frame_count` is incremented before `on_frame` and rolled back + if it raises**, rather than simply incremented afterward. The + distinction is not cosmetic: the HUD's own per-frame update is + composed into `on_frame` (`bootstrap.py`), and it reads + `frame_count` to render "step N / elapsed t" -- so incrementing + afterward makes the frame that is *currently being drawn* report + the previous frame's number, and every run's step readout comes + out one low. Found by `test_bootstrap_stats_use_configured_time_ + units_for_elapsed_time` failing on a three-frame run that + displayed `step 2 t = 20 ms`, not by reasoning about it. + + The rollback keeps the property that motivated the change: a + frame whose simulation step died does not count as drawn, so a + failing run cannot report a full frame budget. + """ self.renderer.render(self.scene, self.camera) self.frame_count += 1 if self._on_frame is not None: - self._on_frame() + try: + self._on_frame() + except Exception as error: # noqa: BLE001 -- re-raised by `run` + self._frame_error = error + self.frame_count -= 1 + self.canvas.close() + + def _raise_any_frame_error(self) -> None: + """Re-raise whatever `_draw` caught, now that the event loop has + let go (TASK-053, Stage 9, 2026-09-13). + + Called on both of `run`'s branches, because both need it for + different reasons: the offscreen loop is PyFlow's own `for` and + would otherwise return a full frame budget of failures, and the + interactive one hands control to `get_loop(...).run()`, which + returns only once the canvas closes -- which `_draw` does on the + failing frame. + """ + error = self._frame_error + if error is not None: + self._frame_error = None + raise error def run( self, @@ -308,7 +371,14 @@ def run( self.canvas.request_draw(self._draw) for _ in range(max_frames or 1): self.last_image = self.canvas.draw() + # `canvas.draw()` returns normally even when `_draw` + # failed, so the budget has to be abandoned explicitly -- + # unlike the interactive branch below, where closing the + # canvas ends the loop on its own. + if self._frame_error is not None: + break self.canvas.close() + self._raise_any_frame_error() logger.info("offscreen render complete: %d frame(s)", self.frame_count) return @@ -360,4 +430,5 @@ def on_draw() -> None: ) self.canvas.request_draw(on_draw) get_loop(self._config).run() + self._raise_any_frame_error() logger.info("render window closed: %d frame(s)", self.frame_count) diff --git a/tests/integration/CLAUDE.md b/tests/integration/CLAUDE.md index 5ce6bd1..884e8ab 100644 --- a/tests/integration/CLAUDE.md +++ b/tests/integration/CLAUDE.md @@ -121,6 +121,22 @@ of windows rather than redistribute them -- the two is the highest count here. See `docs/planning/backlog.md` for the open item. +**`test_frame_failure.py` (TASK-053, Stage 9, added 2026-09-13) crosses +the boundary for a third distinct reason: an exit code that only exists +outside the process.** Its claim is that a run whose frames raise fails +-- and the observable is the exit code, which an in-process call to +`RenderWindow.run` cannot produce. `tests/unit/test_rendering.py` covers +the re-raise itself; this covers what a user meets. + +**Its fixture is a real configuration that genuinely diverges, not a +monkeypatched exception** -- the shipped cavity refined to 64x64 with +its own timestep left alone, measured at 2.05x the stability limit and +blowing up at step 17. A patched exception would prove the plumbing +carries *an* error; this proves the engine's own diagnostic reaches a +user, which is the Stage 4 use case the criterion exists for. Its glfw +half is display-guarded and so runs on Windows only, the same asymmetry +this file already records above. + **Comparing rendered pixels: never build the reference from the run under test.** `test_playback_cli.py`'s two `*_rerenders_the_field_in_ real_pixels` tests are the worked example, and their module comments diff --git a/tests/integration/test_frame_failure.py b/tests/integration/test_frame_failure.py new file mode 100644 index 0000000..ca469c6 --- /dev/null +++ b/tests/integration/test_frame_failure.py @@ -0,0 +1,219 @@ +"""A run that raises inside a frame must fail (TASK-053, Stage 9). + +Stage 9 Completion Criterion 4. Until this task, it did not: an exception +raised inside `RenderWindow._draw`'s own `on_frame` callback was swallowed +by `rendercanvas`'s `with log_exception("Draw error")` block (in that +third-party package's own `base.py`, not anything tracked here), so +PyFlow never saw it and `pyflow run` printed `pyflow exited cleanly` and +returned 0. Measured on the shipped lid-driven cavity refined to 64x64 +with its own `numerics.timestep` untouched: **22 of 40 frames failed and +the exit code was 0.** + +Integration tests, not unit ones, and deliberately so: the defect lives +in what happens when control is inverted to a third-party event loop, and +the exit code is the observable the criterion names. A unit test calling +`RenderWindow.run` in-process would check the re-raise but not that it +becomes a non-zero exit, which is the half a user actually meets +(`tests/CLAUDE.md`'s own split). + +**The fixture is a real configuration that genuinely diverges**, not a +monkeypatched exception. A patched one would prove the plumbing carries +*an* exception; this proves the engine's own +`DivergenceDidNotConvergeError` reaches a user, which is Stage 4's own +use case ("be told when it did not converge instead of receiving a +plausible wrong answer") and the reason this criterion exists. +""" + +from __future__ import annotations + +import os +import subprocess +import sys +from pathlib import Path + +import pytest + +_DIVERGING_CONFIG = """\ +# The shipped lid-driven cavity, refined to 64x64 with its own timestep +# left alone -- `stable_timestep` puts the limit at 0.00391 for this +# mesh, so 0.008 is 2.05x over it and the run blows up at step 17. +# Measured before this fixture was written, not assumed. +mesh: + extent: [64, 64] + spacing: [0.015625, 0.015625] + +numerics: + timestep: 0.008 + boundary_conditions: + north: + type: dirichlet + field_values: + velocity.0: 1.0 + velocity.1: 0.0 + south: + type: dirichlet + east: + type: dirichlet + west: + type: dirichlet + +simulation: + velocity_solved: true + +fluid: + viscosity: 0.01 +""" + +_FRAMES = 25 +"""Past the measured divergence at step 17, with margin. Deliberately not +hundreds: the point is that the run stops, and a run that kept going to +frame 500 before anyone noticed is the behaviour under repair. +""" + + +@pytest.fixture +def diverging_config(tmp_path: Path) -> Path: + config = tmp_path / "diverging.yaml" + config.write_text(_DIVERGING_CONFIG, encoding="utf-8") + return config + + +def _run(*args: str) -> subprocess.CompletedProcess[str]: + return subprocess.run( + [sys.executable, "-m", "pyflow", *args], + capture_output=True, + text=True, + check=False, + ) + + +def test_run_exits_non_zero_when_a_frame_raises(diverging_config: Path) -> None: + result = _run( + "run", + "--config", + str(diverging_config), + "--backend", + "offscreen", + "--max-frames", + str(_FRAMES), + ) + + assert result.returncode != 0, ( + "a run whose frames raise must not report success; got exit 0 with " + f"stderr:\n{result.stderr}" + ) + # The engine's own diagnostic, not merely *an* error -- per + # `tests/integration/CLAUDE.md`'s convention that a non-zero + # assertion carries a stderr substring, so argparse failures and real + # ones stay distinguishable. + assert "pressure correction loop did not reach tolerance" in result.stderr, result.stderr + + +def test_a_failed_run_does_not_claim_to_have_exited_cleanly(diverging_config: Path) -> None: + result = _run( + "run", + "--config", + str(diverging_config), + "--backend", + "offscreen", + "--max-frames", + str(_FRAMES), + ) + + # The specific sentence a user saw before this task, immediately + # after a traceback that `rendercanvas` had logged and PyFlow had not + # seen. Asserted by its absence rather than only by the exit code, + # because the two failed independently: the message is emitted by + # `bootstrap()` on any normal return from `window.run`. + assert "pyflow exited cleanly" not in result.stderr, result.stderr + + +def test_a_failed_run_stops_rather_than_finishing_its_frame_budget( + diverging_config: Path, +) -> None: + result = _run( + "run", + "--config", + str(diverging_config), + "--backend", + "offscreen", + "--max-frames", + str(_FRAMES), + ) + + # `offscreen render complete: N frame(s)` is logged only after the + # whole budget has been drawn. Before this task the loop ran to + # completion over failing frames and logged it; now the run stops at + # the first failure, so the line must be absent. + assert "offscreen render complete" not in result.stderr, result.stderr + + +def test_a_healthy_run_still_exits_cleanly() -> None: + # The other half of the claim: this must not fire on every run. A + # guard that always trips is a guard nobody can act on -- the same + # reasoning TASK-054's own "absent below the limit" criterion states. + result = _run( + "run", + "--config", + "examples/golden-demos/lid_driven_cavity.yaml", + "--backend", + "offscreen", + "--max-frames", + "5", + ) + + assert result.returncode == 0, result.stderr + assert "pyflow exited cleanly" in result.stderr, result.stderr + + +# -- The interactive backend ------------------------------------------------- +# +# Criterion 4 names both backends, and the two fail differently. On +# `offscreen` the loop above ran its whole budget over failing frames and +# reported success; on `glfw` the raise also skipped `on_draw`'s own +# reschedule, so the frame budget was never reached and the process +# **hung** -- observed at 300 s before being killed, and now exiting 1 in +# 22 s. +# +# **Display-guarded, so this runs on Windows CI and skips on Linux** +# (`tests/integration/CLAUDE.md`; `docs/planning/backlog.md` carries the +# open item for that asymmetry, with the two attempted fixes and why each +# was reverted). Recorded rather than left implicit: this half of the +# criterion is evidenced on one of the two platforms CI covers. + +_needs_a_real_display = pytest.mark.skipif( + not ( + os.environ.get("DISPLAY") + or os.environ.get("WAYLAND_DISPLAY") + or sys.platform in ("win32", "darwin") + ), + reason="no display available for a real glfw window", +) + + +@_needs_a_real_display +def test_an_interactive_run_terminates_rather_than_hanging_when_a_frame_raises( + diverging_config: Path, +) -> None: + result = subprocess.run( + [ + sys.executable, + "-m", + "pyflow", + "run", + "--config", + str(diverging_config), + "--max-frames", + str(_FRAMES), + ], + capture_output=True, + text=True, + check=False, + # The bug this guards is a hang, so the timeout *is* the + # assertion -- generous enough that a slow machine is not a false + # failure, and far below the 300 s the unfixed version ran past. + timeout=180, + ) + + assert result.returncode != 0, result.stderr + assert "pressure correction loop did not reach tolerance" in result.stderr, result.stderr diff --git a/tests/unit/test_rendering.py b/tests/unit/test_rendering.py index a17fb26..71c4c68 100644 --- a/tests/unit/test_rendering.py +++ b/tests/unit/test_rendering.py @@ -272,3 +272,69 @@ def test_screen_to_world_accounts_for_camera_position_and_aspect_expansion() -> # landed at pixel (~59.5, ~9.5) in a 200x100 offscreen render. world_x, world_y = screen_to_world(window.camera, logical_width, logical_height, 59.5, 9.5) assert (world_x, world_y) == pytest.approx((1.0, 7.05), abs=0.1) + + +# -- A raising frame callback (TASK-053, Stage 9) ---------------------------- +# +# `RenderWindow` is the seam both `pyflow run` and `pyflow play` go +# through, so these cover Criterion 4's "both window-opening subcommands" +# structurally rather than by contriving a diverging playback -- `play` +# renders pre-materialized frames, so the engine's own divergence cannot +# arise inside its frame callback at all. What can arise there is any +# error in its own scene rebuilding, and this is the mechanism that would +# carry it. `tests/integration/test_frame_failure.py` is the end-to-end +# half, against a real diverging configuration and a real exit code. + + +class _DeliberateFrameError(RuntimeError): + """Distinctive, so the assertions below cannot pass on some other + exception the rendering stack happened to raise. + """ + + +def test_run_reraises_whatever_the_frame_callback_raised() -> None: + window = RenderWindow(RenderingConfig(backend="offscreen")) + + def _raise() -> None: + raise _DeliberateFrameError("frame callback failed") + + with pytest.raises(_DeliberateFrameError): + window.run(max_frames=5, on_frame=_raise) + + +def test_a_failing_frame_does_not_count_as_drawn() -> None: + window = RenderWindow(RenderingConfig(backend="offscreen")) + calls = 0 + + def _raise_on_third() -> None: + nonlocal calls + calls += 1 + if calls == 3: + raise _DeliberateFrameError("frame callback failed") + + with pytest.raises(_DeliberateFrameError): + window.run(max_frames=10, on_frame=_raise_on_third) + + # Two frames completed; the third raised. `frame_count` used to be + # incremented before `on_frame` ran, so a frame that died in the + # simulation still counted -- which is how a failing run could report + # a full budget. + assert window.frame_count == 2, window.frame_count + # And the budget was abandoned rather than run to completion. + assert calls == 3, calls + + +def test_a_frame_callback_that_does_not_raise_is_unaffected() -> None: + # The guard must not fire on a healthy run -- the other half of the + # claim, and the one a regression would break silently. + window = RenderWindow(RenderingConfig(backend="offscreen")) + calls = 0 + + def _count() -> None: + nonlocal calls + calls += 1 + + window.run(max_frames=4, on_frame=_count) + + assert calls == 4 + assert window.frame_count == 4 From 32a5e28b0eb6fe701a060cdcaf15eb3ad7c090d5 Mon Sep 17 00:00:00 2001 From: Adam Clemens Date: Sun, 13 Sep 2026 00:42:07 +0100 Subject: [PATCH 2/2] Put TASK-053's dates back to the 12th, and record why CI rejected them `make ci` was green locally and failed on both CI platforms. The difference was the clock: this task was committed at 23:09 UTC on the 12th, on a machine an hour ahead of UTC where the local time read 00:09 on the 13th. I dated the work by the local clock. `check_dates` resolves "today" with `date.today()` on whichever machine runs it -- UTC on CI -- so a date that was real locally was a future date there. The gate was right and I was not. Dates reverted to the 12th, which is what the commit carries in UTC. **The property this exposed is recorded in `check_dates.py`'s own docstring**, because nothing had it written down: "today" is runner-local, so a contributor ahead of UTC committing late in the evening can write a date that passes locally and fails CI. The rule that follows -- date a change by its UTC commit time, not the wall clock in front of you -- is stated there with how to get it. Deliberately *not* fixed by making the checker use UTC internally. That would let a contributor behind UTC write a date the gate accepts and a reader elsewhere reads as tomorrow, which trades one inconsistency for a quieter one. One clock, and it is the one CI uses. The explanatory note added to the roadmap originally contained the literal ISO date it was explaining, and the checker flagged that too -- fixed the way that docstring already prescribes, by writing it as prose ("the 13th") rather than in the `YYYY-MM-DD` form this repository reserves for things that have happened. The rule catching its own documentation is the rule working. Verified against CI's own clock rather than trusting the local one: `find_future_dates(..., today=date(2026, 9, 12))` reports clean. `make ci` green: 1214 passed, 156 scenarios, all 15 structural checks. Co-Authored-By: Claude Opus 5 --- docs/planning/roadmap.md | 15 +++++++++++---- docs/planning/status.md | 2 +- docs/repository-manifest.md | 2 +- src/pyflow/bootstrap.py | 2 +- src/pyflow/rendering/CLAUDE.md | 2 +- src/pyflow/rendering/window.py | 4 ++-- tests/integration/CLAUDE.md | 2 +- tools/validators/check_dates.py | 19 +++++++++++++++++++ 8 files changed, 37 insertions(+), 11 deletions(-) diff --git a/docs/planning/roadmap.md b/docs/planning/roadmap.md index 0e90605..fc9daeb 100644 --- a/docs/planning/roadmap.md +++ b/docs/planning/roadmap.md @@ -13017,10 +13017,17 @@ own status table. ## TASK-053 — A Failed Frame Fails The Run -**Status: Done, 2026-09-13.** Discharges Completion Criterion 4. -Drafted 2026-09-12, built the following day -- the dates differ and -both are real, which is the distinction `make check-dates` exists to -keep honest. +**Status: Done, 2026-09-12.** Discharges Completion Criterion 4. + +**The date was briefly recorded as the 13th, and CI was right to +reject it.** This task was committed at 23:09 UTC on the 12th, on a +machine an hour ahead of UTC, where the local clock read 00:09 on the +13th. `check_dates` resolves "today" against `date.today()` on +whichever machine runs it, so a date that was real locally was a +*future* date on CI's own UTC runners and `make ci` failed on both +platforms while passing locally. See +`tools/validators/check_dates.py`'s own docstring for the property +this exposed, which nothing had recorded. ### Purpose diff --git a/docs/planning/status.md b/docs/planning/status.md index cfed84f..0bc1f56 100644 --- a/docs/planning/status.md +++ b/docs/planning/status.md @@ -173,7 +173,7 @@ pie showData | Task | Status | Date | Artifact | |------|--------|------|----------| | TASK-052 -- Prescribed Boundary Velocity Reaches The Schemes | Done | 2026-09-12 | `examples/golden-demos/smoke_transport.yaml` | -| TASK-053 -- A Failed Frame Fails The Run | Done | 2026-09-13 | `base.py` | +| TASK-053 -- A Failed Frame Fails The Run | Done | 2026-09-12 | `tools/validators/check_dates.py` | | TASK-054 -- Timestep Stability Warning | Not started | | | ### Stage 10 -- Better Numerics diff --git a/docs/repository-manifest.md b/docs/repository-manifest.md index fde063f..bf842c2 100644 --- a/docs/repository-manifest.md +++ b/docs/repository-manifest.md @@ -1011,7 +1011,7 @@ window with a genuine injected Space key proving the rendered pixels stop changing once paused -- the same `_needs_a_real_display` pattern `test_interactive_window.py` established, its own display-probe helper copied locally rather than imported since `tests/` is not an importable -package here), and `test_frame_failure.py` (TASK-053, 2026-09-13: a +package here), and `test_frame_failure.py` (TASK-053, 2026-09-12: a real configuration that genuinely diverges, run through the CLI, proving a run whose frames raise exits non-zero with the engine's own diagnostic rather than printing `pyflow exited cleanly` and returning 0 -- plus a diff --git a/src/pyflow/bootstrap.py b/src/pyflow/bootstrap.py index f0bae46..e67393d 100644 --- a/src/pyflow/bootstrap.py +++ b/src/pyflow/bootstrap.py @@ -1176,7 +1176,7 @@ def _on_frame() -> None: window.apply_camera_config() # `run` re-raises anything the frame callback raised (TASK-053, - # Stage 9, 2026-09-13), so this line is unreachable on a failed run and the + # Stage 9, 2026-09-12), so this line is unreachable on a failed run and the # exception propagates out of `main()` to a non-zero exit. It used to # be reached unconditionally: `rendercanvas` swallowed the # exception, `run` returned normally, and a run whose every frame had diff --git a/src/pyflow/rendering/CLAUDE.md b/src/pyflow/rendering/CLAUDE.md index d7ccbf6..5c87e75 100644 --- a/src/pyflow/rendering/CLAUDE.md +++ b/src/pyflow/rendering/CLAUDE.md @@ -2,7 +2,7 @@ **`RenderWindow._draw` catches whatever `on_frame` raises, records it, and closes the canvas; `run` re-raises it afterward (TASK-053, Stage 9, -2026-09-13).** The exception must not be allowed to escape `_draw`, +2026-09-12).** The exception must not be allowed to escape `_draw`, because `_draw` is installed as `rendercanvas`'s own `_draw_frame` and that library calls it inside `with log_exception("Draw error")` -- which logs and continues by design, in its own words "otherwise we diff --git a/src/pyflow/rendering/window.py b/src/pyflow/rendering/window.py index c4a5f28..4cb20d0 100644 --- a/src/pyflow/rendering/window.py +++ b/src/pyflow/rendering/window.py @@ -260,7 +260,7 @@ def _draw(self) -> None: callback advances. **Anything `on_frame` raises is caught here, recorded, and the - window closed (TASK-053, Stage 9, 2026-09-13) -- not allowed to escape + window closed (TASK-053, Stage 9, 2026-09-12) -- not allowed to escape `rendercanvas`.** This method is installed as that library's own `_draw_frame`, and it calls it inside `with log_exception("Draw error")`, which logs and continues by design @@ -305,7 +305,7 @@ def _draw(self) -> None: def _raise_any_frame_error(self) -> None: """Re-raise whatever `_draw` caught, now that the event loop has - let go (TASK-053, Stage 9, 2026-09-13). + let go (TASK-053, Stage 9, 2026-09-12). Called on both of `run`'s branches, because both need it for different reasons: the offscreen loop is PyFlow's own `for` and diff --git a/tests/integration/CLAUDE.md b/tests/integration/CLAUDE.md index 884e8ab..7a30a84 100644 --- a/tests/integration/CLAUDE.md +++ b/tests/integration/CLAUDE.md @@ -121,7 +121,7 @@ of windows rather than redistribute them -- the two is the highest count here. See `docs/planning/backlog.md` for the open item. -**`test_frame_failure.py` (TASK-053, Stage 9, added 2026-09-13) crosses +**`test_frame_failure.py` (TASK-053, Stage 9, added 2026-09-12) crosses the boundary for a third distinct reason: an exit code that only exists outside the process.** Its claim is that a run whose frames raise fails -- and the observable is the exit code, which an in-process call to diff --git a/tools/validators/check_dates.py b/tools/validators/check_dates.py index 512db8b..e7c66a7 100644 --- a/tools/validators/check_dates.py +++ b/tools/validators/check_dates.py @@ -28,6 +28,25 @@ That is the bar `tools/validators/CLAUDE.md` sets for anything in `make ci`; a check needing a reader trains people to route around it. +**"Today" is whatever the machine running this thinks it is, and that +bit, 2026-09-12.** `date.today()` is local to the runner. CI's runners +are UTC; a contributor an hour ahead of UTC who commits late in the +evening sees a local date one day later than the one CI will compute -- +so a date that is real on their own clock is a *future* date here, and +`make ci` fails on both platforms after passing locally. That happened +to TASK-053: committed at 23:09 UTC, written down as the 13th because +the authoring machine read 00:09, rejected by both runners, and put back +to the 12th (`docs/planning/roadmap.md`'s own TASK-053 entry records it). + +**The rule this implies: date a change by its UTC commit time, not by +the wall clock you are looking at.** `git log --date=format:'%Y-%m-%d'` +in a UTC shell (or `git log --date=iso-strict-local` with `TZ=UTC`) +is the authoritative answer. Deliberately *not* fixed by making this +check use UTC itself: that would let a contributor behind UTC write a +date this gate accepts and a reader in another timezone reads as +tomorrow, trading one inconsistency for a quieter one. One clock, and it +is the one CI uses. + **One thing to know before writing about a wrong date: prose that quotes one trips the check.** Both documents describing this drift originally named the bad date in the ISO form, and `check-dates` failed on its own