fix(display): rebuild the GL window surface on resize (CI GUI wipeout) - #400
Conversation
The CI GUI wipeout distilled to its smallest witness: a bare winit+wgpu window that presents red, resizes, and presents blue must show blue at the new geometry. It passes on Vulkan (incl. lavapipe) and fails on the GL backend -- 0.67 blue after resize, exactly the stale-height fraction -- with no editor, redisplay, or scheduler involved, so the fault is in the GL present path itself and the fix belongs behind a backend quirk, not in the scheduler. The backend follows WGPU_BACKEND exactly as CI selects it, the window runs on the infra harness's isolated Xvfb (whose session env becomes the process env, because an in-process event loop must not see the operator's Wayland), and captures go through import like the rest of the suite. The session outlives wgpu teardown: dropping it earlier trips Xlib's fatal IO handler and takes the process with it.
wgpu's GL backend emulates the swapchain, and after a window resize its presents land a stale-geometry buffer: the new present-path contract test measures exactly the stale-height fraction (0.67 blue after a 480x360 resize from 320x240) with no editor involved, while the same test passes on Vulkan including lavapipe. CI runners have no Vulkan, so every GUI scenario there presents through GL -- the root of the multi-month GUI wipeouts that the scheduler and ingest logs kept exonerating. GuiFrameNativeWindowState now records the surface's backend (wgpu surfaces do not expose it) and handle_resize routes through one policy: GL rebuilds the window surface from the instance at the new geometry, every other backend reconfigures in place as before. The contract test encodes the same policy so it guards both paths on whatever backend the host selects.
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It changes the GUI frame present/resize path and its correctness depends on runtime GPU backend behavior that cannot be verified without executing the GUI tests, so it warrants human GUI verification.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
This PR fixes a long-standing CI GUI "wipeout" where, on runners without Vulkan, wgpu's GL backend presents a stale-geometry buffer after a window resize because its emulated swapchain does not survive a reconfigure-in-place. The fix records each surface's backend and, on GL only, rebuilds the window surface at the new geometry during resize; all other backends keep reconfiguring in place. A new bare winit+wgpu contract test encodes and guards the same policy on whichever backend the host selects.
Changes:
- Add
surface_backendtoGuiFrameNativeWindowStateand route resize throughsurface_configure_or_rebuild, which rebuilds the GL surface but reconfigures every other backend in place. - Thread
wgpu::Instanceintohandle_resize(needed to recreate the surface) and update the caller and both construction sites. - Add a self-contained
present_contractintegration test (Xvfb + winit + wgpu) that renders, resizes, and asserts the second frame shows at the new geometry.
| File | Description |
|---|---|
| crates/neomacs-display-runtime/src/render_thread/frame_windows.rs | Adds surface_backend field + surface_configure_or_rebuild; GL rebuilds surface on resize, others reconfigure in place. |
| crates/neomacs-display-runtime/src/render_thread/surface_resize.rs | Passes gpu.instance alongside gpu.device into the updated handle_resize. |
| crates/neomacs-display-runtime/src/render_thread/bootstrap.rs | Populates surface_backend from adapter_info.backend at the primary-window construction site. |
| crates/neomacs-gui-tests/tests/present_contract.rs | New present-path contract test that reproduces/guards the GL resize quirk. |
| crates/neomacs-gui-tests/Cargo.toml | Adds winit/wgpu/raw-window-handle/pollster deps for the new test. |
| Cargo.lock | Records the new dependency edges for neomacs-gui-tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Review follow-ups: the app no longer owns (and never drops) the DisplaySession -- that was a leak of one Xvfb per run. It carries only the session's env pairs; the session stays in test scope, where it outlives the event loop's teardown (dropping it inside run_app trips Xlib's fatal IO handler, which exits the process instead of unwinding) and still cleans up deterministically at test end. The winit/wgpu/pollster deps move to dev-dependencies per crate convention, and the unused direct raw-window-handle dependency goes: the contract reaches the handle types through winit.
|
Both review findings are addressed in 427e988:
The contract is re-verified green on both GL (CI's path) and Vulkan. @copilot please re-review. |
The previous commit moved winit/wgpu/pollster into neomacs-gui-tests' dev-dependencies but staged only the manifest; the lockfile encoding that move stayed local, so every --locked CI job (check-dependency- coherence first among them) refused to run.
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The change is backend-specific GPU surface-lifecycle code validated mainly through GL/Vulkan-dependent GUI tests, and the branch description references further unshown changes, so it warrants human verification alongside fixing the stale Cargo.lock entry.
Review effort: Balanced
Findings: None
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The change alters the GPU surface present/resize hot path with backend-specific behavior whose correctness depends on real GL/Vulkan runtime results that cannot be fully verified statically.
Review effort: Balanced
Findings: None
Root cause
CI runners have no Vulkan, so every GUI scenario presents through wgpu's GL backend — whose emulated swapchain lands a stale-geometry buffer after a window resize. A new minimal present-path contract test (bare winit+wgpu, no editor) proves it: 0.67 blue after a 480×360 resize from 320×240 — exactly the stale-height fraction — while the identical test passes on Vulkan (incl. lavapipe). This was the root of the long-running GUI wipeouts that scheduler/ingest logs kept exonerating.
The fix
GuiFrameNativeWindowStaterecords the surface's backend (wgpu surfaces don't expose it)handle_resizeroutes through one policy: GL rebuilds the window surface from the instance at the new geometry; every other backend reconfigures in place (unchanged)Verification
Also on this branch: sway session extraction into
neomacs-infra::display,LC_ALL=C.UTF-8pinned per display session, frame-outcome/occlusion logging, and the GSettings failure probe.