fix: invalidate merge_cache on scrollback-capacity drain (#405)

freminal-buffer/benches/buffer_row_bench.rs

@@ -362,6 +362,99 @@ fn bench_lf_heavy(c: &mut Criterion) {
     group.finish();
 }
 
+// ---------------------------------------------------------------
+// Benchmark: steady-state LF at scrollback capacity — `merge_cache`
+// rotation invalidation cost (Task #405).
+//
+// `enforce_scrollback_limit` (`resize_and_alt.rs`) now sets
+// `self.merge_cache = None` every time a line feed's push+drain rotation
+// happens with scrollback already at capacity (see
+// `Buffer::merge_cache`'s field doc and the
+// `incremental_merge_matches_oracle_after_scrollback_capacity_rotation`
+// regression test in `flatten.rs`) — this is the correctness fix, but it
+// also means the very next `visible_as_tchars_and_tags` flatten can no
+// longer take the incremental fast path and must fully re-merge the whole
+// visible window instead. This benchmark isolates that worst case: a
+// buffer already sitting at its scrollback cap, then one LF (which always
+// rotates) immediately followed by one flatten, repeated every iteration
+// so the flatten never gets to reuse a warm cache. Two window heights are
+// measured to show the cost scales with `height` (a full-window re-merge),
+// not with `scrollback_limit`.
+// ---------------------------------------------------------------
+fn build_buffer_at_scrollback_capacity(
+    width: usize,
+    height: usize,
+    scrollback_limit: usize,
+) -> Buffer {
+    let mut buf = Buffer::new(width, height).with_scrollback_limit(scrollback_limit);
+    // Fill well past capacity so scrollback is already at its steady-state
+    // cap before any timed iteration runs (pre-fill happens outside the
+    // timed closure via `iter_batched`'s setup callback).
+    let total_lines = height + scrollback_limit + 8;
+    for i in 0..total_lines {
+        let text: Vec<TChar> = format!("line{i:06}")
+            .bytes()
+            .cycle()
+            .take(width)
+            .map(TChar::Ascii)
+            .collect();
+        buf.insert_text(&text);
+        buf.handle_lf();
+        buf.handle_cr();
+    }
+    buf
+}
+
+fn bench_lf_flatten_at_capacity(c: &mut Criterion) {
+    const WIDTH: usize = 200;
+    const SCROLLBACK_LIMIT: usize = 4_000;
+
+    let mut group = c.benchmark_group("bench_lf_flatten_at_capacity");
+
+    for height in [24usize, 100usize] {
+        group.bench_with_input(
+            BenchmarkId::new("lf_then_flatten_steady_state", height),
+            &height,
+            |b, &height| {
+                // `iter_batched_ref` (not `iter_batched`): the routine takes
+                // `&mut Buffer`, so the ~`height + SCROLLBACK_LIMIT`-row
+                // buffer's destructor runs when the batch's input `Vec` is
+                // dropped — OUTSIDE the timed span — instead of inside the
+                // routine on every iteration. With by-value `iter_batched`
+                // the per-iteration buffer drop dominated the measurement
+                // (thousands of `Row` frees), swamping the single LF+flatten
+                // this bench isolates and making the result track
+                // `SCROLLBACK_LIMIT` rather than `height`. The buffer stays at
+                // capacity across the reused iterations (each LF is a
+                // push+drain that nets `rows.len()` unchanged), so every timed
+                // iteration remains in the exact steady state under test.
+                b.iter_batched_ref(
+                    || {
+                        let mut buf =
+                            build_buffer_at_scrollback_capacity(WIDTH, height, SCROLLBACK_LIMIT);
+                        // Warm the merge cache once, outside the timed
+                        // section, so the timed rotation+flatten pair below
+                        // starts from a populated (not merely absent) cache
+                        // — matching the real steady-state PTY loop, where
+                        // the previous frame already warmed it.
+                        let _ = buf.visible_as_tchars_and_tags(0);
+                        buf
+                    },
+                    |buf| {
+                        buf.insert_text(&[TChar::Ascii(b'x')]);
+                        buf.handle_lf();
+                        buf.handle_cr();
+                        std::hint::black_box(buf.visible_as_tchars_and_tags(0));
+                    },
+                    BatchSize::SmallInput,
+                );
+            },
+        );
+    }
+
+    group.finish();
+}
+
 // ---------------------------------------------------------------
 // Benchmark: erase display (ED) on a full buffer
 // ---------------------------------------------------------------
@@ -1024,6 +1117,7 @@ criterion_group!(
         bench_cursor_ops,
         bench_move_cursor_relative,
         bench_lf_heavy,
+        bench_lf_flatten_at_capacity,
         bench_erase_display,
         bench_scrollback_render,
         bench_alternate_screen_switch,

freminal-buffer/src/buffer/flatten.rs

@@ -3101,6 +3101,98 @@ mod incremental_merge_tests {
         );
     }
 
+    /// Task #405 regression guard: `Buffer::enforce_scrollback_limit`
+    /// (`resize_and_alt.rs`) is a fourth rotation site in the same family as
+    /// `scroll_slice_up`/`_down` and whole-buffer `scroll_up` proven above.
+    /// Once scrollback is at capacity, every LF pushes a new row at the
+    /// bottom and then drains `overflow` rows from the front
+    /// (`self.rows.drain(0..overflow)` + `self.row_cache.drain(0..overflow)`),
+    /// netting `rows.len()` unchanged. Since `visible_window_bounds` derives
+    /// purely from `rows.len()` and `height` (both unchanged), the resulting
+    /// `MergeWindowFp` is numerically IDENTICAL to the fingerprint the
+    /// previous call's `merge_cache` was built against, even though every
+    /// row's identity within that window just rotated down by `overflow` —
+    /// so `enforce_scrollback_limit` now contains its own explicit
+    /// `self.merge_cache = None;` at the rotation point, mirroring the other
+    /// three sites (see `Buffer::merge_cache`'s field doc).
+    ///
+    /// This test warms `merge_cache`, then drives many LFs past the
+    /// scrollback cap, re-flattening after every single one (so the
+    /// fingerprint never gets a chance to visibly change between
+    /// rotations) and checks each result against the independent oracle. It
+    /// is the regression guard for that fix: if the `merge_cache = None;`
+    /// line in `enforce_scrollback_limit` is ever removed or bypassed, this
+    /// test fails.
+    #[test]
+    fn incremental_merge_matches_oracle_after_scrollback_capacity_rotation() {
+        let width = 20;
+        let height = 5;
+        let scrollback_limit = 5;
+        let mut buf = Buffer::new(width, height).with_scrollback_limit(scrollback_limit);
+
+        let total_lines = 60;
+        for i in 0..total_lines {
+            buf.insert_text(&text(&format!("line{i:04}")));
+            buf.handle_lf();
+            buf.handle_cr();
+
+            // Warm + immediately re-read the merge cache after every single
+            // line feed, so the fingerprint check in
+            // `rows_as_tchars_and_tags_incremental` is exercised on every
+            // rotation, not just after several have accumulated.
+            let actual = buf.visible_as_tchars_and_tags(0);
+            let oracle = independent_oracle(&mut buf);
+            assert_eq!(
+                actual, oracle,
+                "iteration {i}: flatten diverged from the full-merge oracle \
+                 after a scrollback-capacity push+drain rotation"
+            );
+        }
+
+        // Belt-and-suspenders: independently verify the visible window's
+        // content directly via `Buffer::extract_text`, which reads
+        // `self.rows` / `row_cells_for_read` directly and goes through
+        // NEITHER `row_cache` NOR `merge_cache` — a completely separate
+        // code path from the one exercised (and cross-checked against the
+        // oracle) above.
+        //
+        // The loop above calls `handle_lf()` after EVERY line, including the
+        // last (`i == total_lines - 1`). That final LF advances the cursor
+        // onto a fresh blank row — exactly as a real terminal does — so the
+        // buffer's tail is `line{total_lines-1}` followed by ONE trailing
+        // blank row. The visible window's bottom row is therefore that blank
+        // row, and each content row `k` above it holds
+        // `line{total_lines - (height - 1) + k}`. Deriving the expectation
+        // from that geometry (rather than a `total_lines - height + k`
+        // formula that ignores the trailing blank) keeps this check an
+        // independent oracle over `extract_text` without baking in an
+        // off-by-one.
+        let (visible_start, visible_end) = buf.visible_window_bounds(0, 0);
+        assert_eq!(
+            visible_end - visible_start,
+            height,
+            "sanity: window must be exactly `height` rows"
+        );
+        let content_rows = height - 1;
+        for (k, absolute_row) in (visible_start..visible_end).enumerate() {
+            let actual_row_text = buf.extract_text(absolute_row, 0, absolute_row, width - 1);
+            let expected = if k < content_rows {
+                // Bottom-most content line is `line{total_lines-1}`, sitting
+                // one row above the trailing blank; walk upward from there.
+                let expected_line_index = total_lines - 1 - (content_rows - 1 - k);
+                format!("line{expected_line_index:04}")
+            } else {
+                // Final visible row is the blank row created by the loop's
+                // last `handle_lf()`.
+                String::new()
+            };
+            assert_eq!(
+                actual_row_text, expected,
+                "row {absolute_row} (window position {k}): expected {expected:?} via extract_text"
+            );
+        }
+    }
+
     // ────────────────────────────────────────────────────────────────
     // Property test
     // ────────────────────────────────────────────────────────────────

freminal-buffer/src/buffer/mod.rs

@@ -159,23 +159,30 @@ pub struct Buffer {
     ///      was built from, without marking every affected row dirty or
     ///      `None`: [`Buffer::full_reset`], [`Buffer::reflow_to_width`],
     ///      [`Buffer::enter_alternate`], and [`Buffer::leave_alternate`].
-    ///    - **Confined in-place row rotation**, in `scroll.rs`:
-    ///      `scroll_slice_up`, `scroll_slice_down`, and `scroll_up`. These
+    ///    - **Confined in-place row rotation**: `scroll_slice_up`,
+    ///      `scroll_slice_down`, and `scroll_up` (`scroll.rs`), and
+    ///      `enforce_scrollback_limit` (`resize_and_alt.rs`). These
     ///      relocate already-clean, non-`None` cache entries between row
     ///      indices (a moved row keeps its cached representation, only its
     ///      index changes) without marking the moved rows dirty, and
-    ///      without changing `self.rows.len()` (a shift or a
-    ///      `remove`+`push` nets to the same length) — so *neither* `fp`
-    ///      *nor* `first_rebuilt_row` observes that row content moved to a
-    ///      different index. Each of these three functions contains its own
+    ///      without changing `self.rows.len()`: the three `scroll.rs` sites
+    ///      net to the same length via a shift or a `remove`+`push`, and
+    ///      `enforce_scrollback_limit` nets to the same length because its
+    ///      front `drain` of `overflow` rows is paired, in the same line
+    ///      feed, with a `push_row` of one new row at the bottom once
+    ///      scrollback is at capacity — so *neither* `fp` *nor*
+    ///      `first_rebuilt_row` observes that row content moved to a
+    ///      different index. Each of these four functions contains its own
     ///      `self.merge_cache = None;` at the point where the rotation
     ///      happens; that line is load-bearing, **not** a redundant
     ///      leftover safe to delete — removing it would let a cached
     ///      incremental merge serve stale, pre-rotation content at the
     ///      rotated indices. See [`flatten::MergeCache`]'s doc comment for
     ///      the debug-only oracle cross-check that backstops this case, and
     ///      `incremental_merge_tests` in `flatten.rs` for the regression
-    ///      tests exercising it directly.
+    ///      tests exercising it directly (including
+    ///      `incremental_merge_matches_oracle_after_scrollback_capacity_rotation`
+    ///      for the `enforce_scrollback_limit` site specifically).
     ///
     /// The `_columns` scroll variants (`scroll_slice_up_columns`/
     /// `_down_columns`, used when DECLRMM confines a scroll horizontally)
@@ -185,14 +192,18 @@ pub struct Buffer {
     /// an existing cache entry to a different index, so mechanism (2)
     /// already catches them.
     ///
-    /// Deliberately NOT invalidated here (relying on `fp` instead): the
-    /// row-count-changing sites in `scroll.rs`
-    /// (`enforce_scrollback_limit`'s and `erase_scrollback`'s front
-    /// `drain`s) and the row-append sites in `lines.rs`. Every one of
-    /// those changes `self.rows.len()` without a compensating removal
-    /// elsewhere, which changes the *absolute* `visible_start`/`visible_end`
-    /// bounds `visible_window_bounds` returns, so `fp` mismatches and a
-    /// full merge is forced.
+    /// Deliberately NOT invalidated here (relying on `fp` instead):
+    /// `erase_scrollback`'s front drain (`scroll.rs`) and the row-append
+    /// sites in `lines.rs`. `erase_scrollback` only runs its drain when
+    /// `visible_start > 0` and always collapses `visible_start` to `0`
+    /// (everything above the live view is discarded, not rotated in
+    /// place), so the *absolute* `visible_start`/`visible_end` bounds
+    /// `visible_window_bounds` returns always change — unlike
+    /// `enforce_scrollback_limit`, there is no case where the window
+    /// bounds net out unchanged. The `lines.rs` append sites only ever grow
+    /// `self.rows.len()` with no compensating removal, which likewise
+    /// always changes the window bounds. Both cases make `fp` mismatch, so
+    /// a full merge is forced without an explicit `None`.
     pub(in crate::buffer) merge_cache: Option<MergeCache>,
 
     /// Width and height of the terminal grid.

freminal-buffer/src/buffer/resize_and_alt.rs

@@ -1006,6 +1006,27 @@ impl Buffer {
         self.gc_unreferenced_blocks();
         self.adjust_prompt_rows(overflow);
 
+        // Task #405 fix: once scrollback is at capacity, every line feed
+        // pushes one row at the bottom (via `push_row`/`handle_lf`) and this
+        // drain then removes `overflow` rows from the front in the same
+        // logical step, netting `self.rows.len()` unchanged. Because
+        // `visible_window_bounds` (and therefore `MergeWindowFp`) derives
+        // purely from `rows.len()` and `height` — both unchanged here — the
+        // fingerprint the next flatten computes is numerically IDENTICAL to
+        // the one the stale `merge_cache` was built against, even though
+        // every surviving row's identity just shifted down by `overflow`.
+        // This is the same confined-rotation bug class as `scroll_up`
+        // (`scroll.rs`, see its own `merge_cache = None` and the
+        // explanation in `Buffer::merge_cache`'s field doc): the fingerprint
+        // and `first_rebuilt_row` checks cannot observe an index shift that
+        // isn't accompanied by a `rows.len()` change or a dirty/`None`
+        // marking. Null the cache to force a full re-merge next flatten.
+        //
+        // Only reachable here, inside the `overflow > 0` branch — the
+        // early-return above (`self.rows.len() <= max_rows`) drains nothing
+        // and must not pay this cost on every line feed.
+        self.merge_cache = None;
+
         // --- Garbage-collect images no longer referenced by any row ---
         if !self.image_store.is_empty() {
             self.image_store