From d58b18bf2af2f293b713fd7bb590801b33d3a5db Mon Sep 17 00:00:00 2001 From: Oz Date: Mon, 13 Jul 2026 22:23:45 +0000 Subject: [PATCH] Fix flaky diff_pipeline_computes_added_lines_and_ghost_blocks TUI test Two races made this test flaky: 1. The test's bounded ghost wait (100x yield_now) raced the async layout channel: spawn_stream_local polls the layout stream on the background executor before dispatching back to the foreground, so ghost-block delivery latency is unbounded while the yield loop spins out in microseconds. The waits are now event-driven (DiffUpdated / LayoutInvalidated) instead of bounded spinning. 2. DiffModel::compute_diff aborts a superseded computation only best-effort; a computation that already completed on the background thread still delivered its stale result, clobbering a newer diff and re-triggering refresh_diff_state with the stale status. The completion callback now drops results that are no longer the latest requested computation. Verified with 1100 runs of the test (800 under full CPU load): 0 failures, versus 20/300 failures for the original test under the same load. Co-Authored-By: Oz --- app/src/code/editor/diff.rs | 13 +++++ .../warp_tui/src/tui_file_edits_view_tests.rs | 49 ++++++++++++++----- 2 files changed, 49 insertions(+), 13 deletions(-) diff --git a/app/src/code/editor/diff.rs b/app/src/code/editor/diff.rs index a2a36d2b35d..451a5120818 100644 --- a/app/src/code/editor/diff.rs +++ b/app/src/code/editor/diff.rs @@ -609,6 +609,19 @@ impl DiffModel { .spawn( async move { Self::compute_diff_internal(&base_text, &new).await }, move |model, (change_mapping, deletion_mapping), ctx| { + // Aborting an outdated computation is best-effort: the + // background future may already have completed by the time + // a newer computation (or a `set_base` call) aborts it, in + // which case its result still arrives here. Only apply the + // result if this is still the latest requested computation, + // so stale results never clobber a newer diff. + let is_latest = model + .abort_handle + .as_ref() + .is_some_and(|(_, latest_version)| *latest_version == version); + if !is_latest { + return; + } model.status.change_mapping = change_mapping; model.status.deletion_mapping = deletion_mapping; log::debug!("diff status updated: {:#?}", &model.status); 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..1fb7f6402cf 100644 --- a/crates/warp_tui/src/tui_file_edits_view_tests.rs +++ b/crates/warp_tui/src/tui_file_edits_view_tests.rs @@ -1,7 +1,8 @@ use std::path::PathBuf; use ai::diff_validation::{DiffDelta, DiffType}; -use futures::channel::oneshot; +use futures::channel::mpsc; +use futures::StreamExt; use warp::appearance::Appearance; use warp::editor::{CodeEditorModel, CodeEditorModelEvent}; use warp::tui_export::FileDiff; @@ -76,14 +77,19 @@ fn diff_pipeline_computes_added_lines_and_ghost_blocks() { app.add_singleton_model(|_| Appearance::mock()); let editor = app.add_model(|ctx| CodeEditorModel::new_tui(80, ctx)); - let (tx, rx) = oneshot::channel(); + let (tx, mut rx) = mpsc::unbounded(); app.update(|ctx| { - let mut tx = Some(tx); ctx.subscribe_to_model(&editor, move |_, event, _| { - if matches!(event, CodeEditorModelEvent::DiffUpdated) { - if let Some(tx) = tx.take() { - let _ = tx.send(()); - } + // `DiffUpdated` signals a recomputed diff status; + // `LayoutInvalidated` signals the render state processed a + // layout action (e.g. stored the ghost blocks). Forward both + // so the waits below are event-driven instead of bounded + // spinning, which raced with the async pipeline. + if matches!( + event, + CodeEditorModelEvent::DiffUpdated | CodeEditorModelEvent::LayoutInvalidated + ) { + let _ = tx.unbounded_send(()); } }); editor.update(ctx, |editor, ctx| { @@ -99,14 +105,31 @@ fn diff_pipeline_computes_added_lines_and_ghost_blocks() { ); }); }); - rx.await.expect("diff computation should complete"); + + // The buffer edits above also emit `LayoutInvalidated` events, and + // `reset_content` schedules a diff computation of its own (the buffer + // reset emits `ContentChanged`) that the `apply_diffs` recompute only + // best-effort aborts. An incoming notification therefore doesn't + // guarantee that the applied hunk has been computed — wait until the + // diff model actually contains it. + loop { + rx.next().await.expect("diff computation should complete"); + let hunk_computed = + app.read(|app| editor.as_ref(app).diff().as_ref(app).diff_hunk_count() > 0); + if hunk_computed { + break; + } + } 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 { + // Ghost blocks land via the render state's async layout channel, + // which round-trips through the background executor, so their + // arrival time is unbounded. Every delivery emits `LayoutInvalidated` + // after the blocks are stored, so re-check after each event instead + // of spinning a fixed number of times. + let mut ghosts; + loop { ghosts = app.read(|app| { editor .as_ref(app) @@ -121,7 +144,7 @@ fn diff_pipeline_computes_added_lines_and_ghost_blocks() { if !ghosts.is_empty() { break; } - futures_lite::future::yield_now().await; + rx.next().await.expect("ghost blocks should be stored"); } assert_eq!(ghosts.len(), 1);