From 7463db21df09e28093244bf28c3e69891a5dcc72 Mon Sep 17 00:00:00 2001 From: Oz Date: Mon, 13 Jul 2026 21:45:05 +0000 Subject: [PATCH] Fix flaky TUI diff-pipeline test by awaiting layout_complete `diff_pipeline_computes_added_lines_and_ghost_blocks` flaked in CI (e.g. PR #13486) with `assertion left == right failed: left: 0, right: 1` at the `ghosts.len() == 1` check. Root cause: after `expand_diffs`, the removed-line ghost blocks are not stored synchronously. `refresh_diff_state` calls `RenderState::add_temporary_blocks`, which only `try_send`s a `LayoutAction::LayoutTemporaryBlock` onto the render state's async layout channel. That action is applied by a spawned foreground handler that only runs after a background->foreground hop: `App::spawn_stream_local` polls `layout_rx` on the (tokio) background executor and forwards each item to a foreground task that calls `handle_layout_action` -> `set_temporary_blocks`. Until that handler runs, `CharCellState::temporary_blocks` is empty, so `display_lattice(&[]).ghosts()` returns no blocks. The test polled for the ghosts with a fixed `for _ in 0..100 { ...; yield_now().await }` loop. `yield_now` only yields on the foreground executor; it does not drive the background thread that must first forward the queued action. Under CI CPU contention the 100 fast foreground yield cycles can elapse before the background thread is scheduled and the foreground handler stores the blocks, so the loop exits with `ghosts` empty and the assertion fails. This is a timing race, not a logic bug -- under load it reproduces ~50% of the time (40 parallel runs: 20 pass / 20 fail on the unmodified test). Fix: replace the fixed-iteration poll with the render state's `layout_complete()` future, the deterministic sync primitive already used by the GUI editor tests (`app/src/code/editor/model_tests.rs`). `submit_layout_action` increments an `outstanding_layouts` counter for every queued action and `handle_layout_action` decrements it as each is applied; `layout_complete()` awaits until that counter reaches zero, i.e. until every in-flight layout action -- including the ghost-block one -- has been applied. It loops without a cap, so it waits however long the background->foreground hop takes instead of racing it. `layout_complete()` is gated behind the editor crate's `test-util` feature, so enable `warp_editor/test-util` in `warp_tui`'s `[dev-dependencies]` (mirroring how the `app` crate wires it). The feature is empty apart from gating test-only sync primitives. Verified: 160/160 passes under 2x80 parallel CPU-oversubscribed runs (versus 20/20 failures at 40 parallel before); full `warp_tui --lib` suite green (102 tests); `cargo clippy -p warp_tui --all-targets --tests` and `cargo fmt -p warp_tui --check` clean. Co-Authored-By: Oz --- crates/warp_tui/Cargo.toml | 5 ++ .../warp_tui/src/tui_file_edits_view_tests.rs | 49 +++++++++++-------- 2 files changed, 34 insertions(+), 20 deletions(-) diff --git a/crates/warp_tui/Cargo.toml b/crates/warp_tui/Cargo.toml index 40f4171c5dc..de566478112 100644 --- a/crates/warp_tui/Cargo.toml +++ b/crates/warp_tui/Cargo.toml @@ -72,6 +72,11 @@ tempfile.workspace = true # `test-util` exposes `Appearance::mock()`, used to register the `Appearance` # singleton that the editor-backed TUI input view depends on in tests. warp_core = { workspace = true, features = ["test-util"] } +# `test-util` exposes `RenderState::layout_complete()`, the deterministic sync +# primitive the diff-pipeline test awaits so the ghost-block layout action can +# finish draining the render state's async layout channel (see +# `tui_file_edits_view_tests::diff_pipeline_computes_added_lines_and_ghost_blocks`). +warp_editor = { workspace = true, features = ["test-util"] } async-channel.workspace = true # Process-level worker-dispatch regression test. command.workspace = true diff --git a/crates/warp_tui/src/tui_file_edits_view_tests.rs b/crates/warp_tui/src/tui_file_edits_view_tests.rs index d847a7de99f..c3269c4adfb 100644 --- a/crates/warp_tui/src/tui_file_edits_view_tests.rs +++ b/crates/warp_tui/src/tui_file_edits_view_tests.rs @@ -103,26 +103,35 @@ fn diff_pipeline_computes_added_lines_and_ghost_blocks() { editor.update(&mut app, |editor, ctx| editor.expand_diffs(ctx)); - // Ghost blocks land via the render state's async layout channel; poll - // until the spawned handler has stored them. - let mut ghosts = Vec::new(); - for _ in 0..100 { - ghosts = app.read(|app| { - editor - .as_ref(app) - .render_state() - .as_ref(app) - .char_cell() - .expect("TUI editor renders in char-cell mode") - .display_lattice(&[]) - .ghosts() - .to_vec() - }); - if !ghosts.is_empty() { - break; - } - futures_lite::future::yield_now().await; - } + // Ghost blocks land via the render state's async layout channel: + // `expand_diffs` queues a `LayoutTemporaryBlock` action that a spawned + // foreground handler applies only after a background→foreground hop. + // A fixed-iteration `yield_now` poll races that hop under CPU + // contention — the test flaked in CI with `ghosts.len() == 0` when the + // 100 iterations elapsed before the handler had stored the blocks. So + // await the render state's `layout_complete()` future, which resolves + // only once every in-flight layout action (including the ghost-block + // one) has been applied, then read the ghosts once. + app.read(|app| { + editor + .as_ref(app) + .render_state() + .as_ref(app) + .layout_complete() + }) + .await; + + let ghosts = app.read(|app| { + editor + .as_ref(app) + .render_state() + .as_ref(app) + .char_cell() + .expect("TUI editor renders in char-cell mode") + .display_lattice(&[]) + .ghosts() + .to_vec() + }); assert_eq!(ghosts.len(), 1); assert_eq!(ghosts[0].content, "old\n");