terminal: update page_serial_min properly when erasing a page to avoid crash in search (#11965)

Fixes #11957

erasePage now updates page_serial_min when the first page is erased, and
asserts that only front or back pages are erased since page_serial_min
cannot represent serial gaps from middle erasure.

eraseRows can still technically destroy middle pages but no caller does
that today. We'll have to rethink this eventually.

src/terminal/PageList.zig

@@ -3734,12 +3734,36 @@ pub fn eraseRowBounded(
     node.data.clearCells(&rows[node.data.size.rows - 1], 0, node.data.size.cols);
 }
 
-/// Erase the rows from the given top to bottom (inclusive). Erasing
-/// the rows doesn't clear them but actually physically REMOVES the rows.
-/// If the top or bottom point is in the middle of a page, the other
-/// contents in the page will be preserved but the page itself will be
-/// underutilized (size < capacity).
-pub fn eraseRows(
+/// Erase all history rows, optionally up to a bottom-left bound.
+/// This always starts from the beginning of the history area.
+pub fn eraseHistory(
+    self: *PageList,
+    bl_pt: ?point.Point,
+) void {
+    self.eraseRows(.{ .history = .{} }, bl_pt);
+}
+
+/// Erase active area rows, from the top of the active area to the
+/// given row (inclusive).
+pub fn eraseActive(
+    self: *PageList,
+    y: size.CellCountInt,
+) void {
+    assert(y < self.rows);
+    self.eraseRows(.{ .active = .{} }, .{ .active = .{ .y = y } });
+}
+
+/// Erase rows from tl_pt to bl_pt (inclusive), physically removing
+/// them rather than just clearing their contents. If a point falls
+/// in the middle of a page, remaining rows in that page are shifted
+/// and the page becomes underutilized (size < capacity).
+///
+/// Callers must ensure that the erased range only removes pages from
+/// the front or back of the linked list, never the middle. Middle-page
+/// erasure would create serial gaps that page_serial_min cannot
+/// represent, leaving dangling references in consumers such as search.
+/// Use the public eraseHistory/eraseActive wrappers which enforce this.
+fn eraseRows(
     self: *PageList,
     tl_pt: point.Point,
     bl_pt: ?point.Point,
@@ -3843,16 +3867,29 @@ pub fn eraseRows(
     self.fixupViewport(erased);
 }
 
-/// Erase a single page, freeing all its resources. The page can be
-/// anywhere in the linked list but must NOT be the final page in the
-/// entire list (i.e. must not make the list empty).
+/// Erase a single page, freeing all its resources. The page must be
+/// at the front or back of the linked list (not the middle) and must
+/// NOT be the final page in the entire list (i.e. must not make the
+/// list empty).
 ///
 /// IMPORTANT: This function does NOT update `total_rows`. The caller is
 /// responsible for accounting for the removed rows before or after calling
 /// this function.
 fn erasePage(self: *PageList, node: *List.Node) void {
+    // Must not be the final page.
     assert(node.next != null or node.prev != null);
 
+    // We only support erasing from the front or back, never the middle.
+    // Middle erasure would create serial gaps that page_serial_min can't
+    // represent. If this ever needs to change, we'll need a more
+    // sophisticated invalidation mechanism.
+    assert(node.prev == null or node.next == null);
+
+    // If we're erasing the first page, update page_serial_min so that
+    // any external references holding this page's serial will know it
+    // has been invalidated.
+    if (node.prev == null) self.page_serial_min = node.next.?.serial;
+
     // Update any tracked pins to move to the previous or next page.
     const pin_keys = self.tracked_pins.keys();
     for (pin_keys) |p| {
@@ -7140,10 +7177,7 @@ test "PageList eraseRows invalidates viewport offset cache" {
     // This removes rows from before our pin, which changes its absolute
     // offset from the top, but the cache is not invalidated.
     const rows_to_erase = page.capacity.rows / 2;
-    s.eraseRows(
-        .{ .history = .{} },
-        .{ .history = .{ .y = rows_to_erase - 1 } },
-    );
+    s.eraseHistory(.{ .history = .{ .y = rows_to_erase - 1 } });
 
     try testing.expectEqual(Scrollbar{
         .total = s.total_rows,
@@ -9318,7 +9352,7 @@ test "PageList erase" {
     try testing.expect(s.total_rows > s.rows);
 
     // Erase the entire history, we should be back to just our active set.
-    s.eraseRows(.{ .history = .{} }, null);
+    s.eraseHistory(null);
     try testing.expectEqual(s.rows, s.total_rows);
 
     // We should be back to just one page
@@ -9349,7 +9383,7 @@ test "PageList erase reaccounts page size" {
     try testing.expect(s.page_size > start_size);
 
     // Erase the entire history, we should be back to just our active set.
-    s.eraseRows(.{ .history = .{} }, null);
+    s.eraseHistory(null);
     try testing.expectEqual(start_size, s.page_size);
 }
 
@@ -9381,7 +9415,7 @@ test "PageList erase row with tracked pin resets to top-left" {
     defer s.untrackPin(p);
 
     // Erase the entire history, we should be back to just our active set.
-    s.eraseRows(.{ .history = .{} }, null);
+    s.eraseHistory(null);
     try testing.expectEqual(s.rows, s.total_rows);
 
     // Our pin should move to the first page
@@ -9402,7 +9436,7 @@ test "PageList erase row with tracked pin shifts" {
     defer s.untrackPin(p);
 
     // Erase only a few rows in our active
-    s.eraseRows(.{ .active = .{} }, .{ .active = .{ .y = 3 } });
+    s.eraseActive(3);
     try testing.expectEqual(s.rows, s.total_rows);
 
     // Our pin should move to the first page
@@ -9423,7 +9457,7 @@ test "PageList erase row with tracked pin is erased" {
     defer s.untrackPin(p);
 
     // Erase the entire history, we should be back to just our active set.
-    s.eraseRows(.{ .active = .{} }, .{ .active = .{ .y = 3 } });
+    s.eraseActive(3);
     try testing.expectEqual(s.rows, s.total_rows);
 
     // Our pin should move to the first page
@@ -9457,7 +9491,7 @@ test "PageList erase resets viewport to active if moves within active" {
     try testing.expect(s.viewport == .top);
 
     // Erase the entire history, we should be back to just our active set.
-    s.eraseRows(.{ .history = .{} }, null);
+    s.eraseHistory(null);
     try testing.expect(s.viewport == .active);
 }
 
@@ -9486,7 +9520,7 @@ test "PageList erase resets viewport if inside erased page but not active" {
     try testing.expect(s.viewport == .top);
 
     // Erase the entire history, we should be back to just our active set.
-    s.eraseRows(.{ .history = .{} }, .{ .history = .{ .y = 2 } });
+    s.eraseHistory(.{ .history = .{ .y = 2 } });
     try testing.expect(s.viewport == .top);
 }
 
@@ -9514,7 +9548,7 @@ test "PageList erase resets viewport to active if top is inside active" {
     s.scroll(.{ .top = {} });
 
     // Erase the entire history, we should be back to just our active set.
-    s.eraseRows(.{ .history = .{} }, null);
+    s.eraseHistory(null);
     try testing.expect(s.viewport == .active);
 }
 
@@ -9525,7 +9559,7 @@ test "PageList erase active regrows automatically" {
     var s = try init(alloc, 80, 24, null);
     defer s.deinit();
     try testing.expect(s.totalRows() == s.rows);
-    s.eraseRows(.{ .active = .{} }, .{ .active = .{ .y = 10 } });
+    s.eraseActive(10);
     try testing.expect(s.totalRows() == s.rows);
 }
 
@@ -9547,7 +9581,7 @@ test "PageList erase a one-row active" {
         };
     }
 
-    s.eraseRows(.{ .active = .{} }, .{ .active = .{} });
+    s.eraseActive(0);
     try testing.expectEqual(s.rows, s.total_rows);
 
     // The row should be empty
@@ -13842,7 +13876,7 @@ test "PageList resize (no reflow) more cols remaps pins in backfill path" {
 
     // Trim a history row so the first page has spare capacity.
     // This triggers the backfill path in resizeWithoutReflowGrowCols.
-    s.eraseRows(.{ .history = .{} }, .{ .history = .{ .y = 0 } });
+    s.eraseHistory(.{ .history = .{ .y = 0 } });
     try testing.expect(first_page.data.size.rows < first_page.data.capacity.rows);
 
     // Ensure the resize takes the slow path (new capacity > current capacity).

src/terminal/Screen.zig

@@ -1302,19 +1302,21 @@ pub inline fn viewportIsBottom(self: Screen) bool {
 /// Erase the region specified by tl and br, inclusive. This will physically
 /// erase the rows meaning the memory will be reclaimed (if the underlying
 /// page is empty) and other rows will be shifted up.
-pub inline fn eraseRows(
+pub inline fn eraseHistory(
     self: *Screen,
-    tl: point.Point,
     bl: ?point.Point,
 ) void {
     defer self.assertIntegrity();
+    self.pages.eraseHistory(bl);
+    self.cursorReload();
+}
 
-    // Erase the rows
-    self.pages.eraseRows(tl, bl);
-
-    // Just to be safe, reset our cursor since it is possible depending
-    // on the points that our active area shifted so our pointers are
-    // invalid.
+pub inline fn eraseActive(
+    self: *Screen,
+    y: size.CellCountInt,
+) void {
+    defer self.assertIntegrity();
+    self.pages.eraseActive(y);
     self.cursorReload();
 }
 
@@ -1761,7 +1763,7 @@ pub inline fn resize(
     // erase our history. This is because PageList always keeps at least
     // a page size of history.
     if (self.no_scrollback) {
-        self.pages.eraseRows(.{ .history = .{} }, null);
+        self.pages.eraseHistory(null);
     }
 
     // If our cursor was updated, we do a full reload so all our cursor
@@ -3880,7 +3882,7 @@ test "Screen eraseRows history" {
         try testing.expectEqualStrings("1\n2\n3\n4\n5\n6", str);
     }
 
-    s.eraseRows(.{ .history = .{} }, null);
+    s.eraseHistory(null);
 
     {
         const str = try s.dumpStringAlloc(alloc, .{ .active = .{} });
@@ -3914,7 +3916,7 @@ test "Screen eraseRows history with more lines" {
         try testing.expectEqualStrings("A\nB\nC\n1\n2\n3\n4\n5\n6", str);
     }
 
-    s.eraseRows(.{ .history = .{} }, null);
+    s.eraseHistory(null);
 
     {
         const str = try s.dumpStringAlloc(alloc, .{ .active = .{} });
@@ -3943,7 +3945,7 @@ test "Screen eraseRows active partial" {
         try testing.expectEqualStrings("1\n2\n3", str);
     }
 
-    s.eraseRows(.{ .active = .{} }, .{ .active = .{ .y = 1 } });
+    s.eraseActive(1);
 
     {
         const str = try s.dumpStringAlloc(alloc, .{ .active = .{} });
@@ -5655,7 +5657,7 @@ test "Screen: clear history with no history" {
     defer s.deinit();
     try s.testWriteString("4ABCD\n5EFGH\n6IJKL");
     try testing.expect(s.pages.viewport == .active);
-    s.eraseRows(.{ .history = .{} }, null);
+    s.eraseHistory(null);
     try testing.expect(s.pages.viewport == .active);
     {
         // Test our contents rotated
@@ -5689,7 +5691,7 @@ test "Screen: clear history" {
         try testing.expectEqualStrings("1ABCD\n2EFGH\n3IJKL", contents);
     }
 
-    s.eraseRows(.{ .history = .{} }, null);
+    s.eraseHistory(null);
     try testing.expect(s.pages.viewport == .active);
     {
         // Test our contents rotated

src/terminal/Terminal.zig

@@ -2601,7 +2601,7 @@ pub fn eraseDisplay(
             assert(!self.screens.active.cursor.pending_wrap);
         },
 
-        .scrollback => self.screens.active.eraseRows(.{ .history = .{} }, null),
+        .scrollback => self.screens.active.eraseHistory(null),
     }
 }
 

src/terminal/search/screen.zig

@@ -1506,3 +1506,47 @@ test "reloadActive partial history cleanup on loop append error" {
     // because FlattenedHighlight items weren't cleaned up.
     try testing.expectError(error.OutOfMemory, search.select(.next));
 }
+
+test "select after clearing scrollback" {
+    // Regression test for: https://github.com/ghostty-org/ghostty/issues/11957
+    // After clearing scrollback (CSI 3J), selecting next/prev should not crash.
+    const alloc = testing.allocator;
+    var t: Terminal = try .init(alloc, .{
+        .cols = 10,
+        .rows = 2,
+        .max_scrollback = std.math.maxInt(usize),
+    });
+    defer t.deinit(alloc);
+    const list: *PageList = &t.screens.active.pages;
+
+    var s = t.vtStream();
+    defer s.deinit();
+
+    // Write enough content to push matches into scrollback history.
+    s.nextSlice("error\r\n");
+    while (list.totalPages() < 3) s.nextSlice("error\r\n");
+    for (0..list.rows) |_| s.nextSlice("\r\n");
+    s.nextSlice("error.");
+
+    // Start search and find all matches.
+    var search: ScreenSearch = try .init(alloc, t.screens.active, "error");
+    defer search.deinit();
+    try search.searchAll();
+
+    // Should have matches in both history and active areas.
+    try testing.expect(search.history_results.items.len > 0);
+    try testing.expect(search.active_results.items.len > 0);
+
+    // Select a match first (so we have a selection).
+    _ = try search.select(.next);
+    try testing.expect(search.selected != null);
+
+    // Clear scrollback (equivalent to CSI 3J / Cmd+K erasing scrollback).
+    t.eraseDisplay(.scrollback, false);
+
+    // Selecting next/prev after clearing scrollback should not crash.
+    // Before the fix, this would hit an assertion in trackPin because
+    // the FlattenedHighlight contained dangling node pointers.
+    _ = try search.select(.next);
+    _ = try search.select(.prev);
+}

src/termio/Termio.zig

@@ -577,9 +577,8 @@ pub fn clearScreen(self: *Termio, td: *ThreadData, history: bool) !void {
         // If we're not at a prompt, we just delete above the cursor.
         if (!self.terminal.cursorIsAtPrompt()) {
             if (self.terminal.screens.active.cursor.y > 0) {
-                self.terminal.screens.active.eraseRows(
-                    .{ .active = .{ .y = 0 } },
-                    .{ .active = .{ .y = self.terminal.screens.active.cursor.y - 1 } },
+                self.terminal.screens.active.eraseActive(
+                    self.terminal.screens.active.cursor.y - 1,
                 );
             }