From 5c641030519dcaf57dd03b00eccc72beb49cff4b Mon Sep 17 00:00:00 2001 From: foxnne Date: Sat, 17 Oct 2026 00:19:36 -0500 Subject: [PATCH] layout: a pane writing its tabs back no longer reads freed ids `Layout.State.assign` freed a region's old id strings (`setAssignment`), then took the new ids out of every other region by reading the caller's list. A pane adds or drops a tab by reading its assignment, editing it and writing it back (`Workspace.addTab`), so that list borrows the very strings just freed. Fizzy crashed there on 2026-10-16 (`mem.eql` in `evictIdFromOthers`), on an agent's `pixi.create` opening beside the person into a pane that had tabs. `assign` now evicts by the region's new, owned copy, so its input may borrow from any assignment, its own included. It takes that copy's key and list out of the map first: rewriting the other regions can grow the map (`getOrPut` makes room before it looks) and move its entries. The SDK's `assignedSurfaces` doc says so: read, edit, write. `evictIdFromOthers` kept at most 32 regions and 32 tabs in fixed buffers and swallowed its errors: a pane of 40 tabs that gave one up came back with 32. It now uses lists, and its errors reach `assign`'s caller. Co-Authored-By: Claude Opus 5.5 --- app/layout/State.zig | 43 ++++++++++---------- sdk/src/EditorAPI.zig | 4 +- sdk/src/Host.zig | 4 +- tests/integration.zig | 94 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 119 insertions(+), 26 deletions(-) diff --git a/app/layout/State.zig b/app/layout/State.zig index e75ca4058..cc6acba3f 100644 --- a/app/layout/State.zig +++ b/app/layout/State.zig @@ -473,9 +473,17 @@ pub fn assignment(self: *State, name: []const u8) ?[]const []const u8 { /// Set what region `name` shows. An empty list is a real choice — "nothing here" — distinct from /// `unassign`, which hands the region back to its keywords. A surface lives in one /// place: putting it here takes it out of every other assignment. +/// +/// `surfaces` may borrow from any assignment, this region's included — read, edit, write back +/// is how a pane adds a tab. So past `setAssignment`, which frees the old lists, only this +/// region's new, owned copy is read: its key and list, taken out of the map first, since +/// rewriting the other regions can grow the map and move its entries. pub fn assign(self: *State, gpa: std.mem.Allocator, name: []const u8, surfaces: []const []const u8) !void { try self.setAssignment(gpa, name, surfaces); - for (surfaces) |id| self.evictIdFromOthers(gpa, name, id); + const owned = self.assignments.getEntry(name).?; + const key = owned.key_ptr.*; + const ids = owned.value_ptr.*; + for (ids) |id| try self.evictIdFromOthers(gpa, key, id); } fn setAssignment(self: *State, gpa: std.mem.Allocator, name: []const u8, surfaces: []const []const u8) !void { @@ -500,33 +508,24 @@ fn setAssignment(self: *State, gpa: std.mem.Allocator, name: []const u8, surface gop.value_ptr.* = owned; } -fn evictIdFromOthers(self: *State, gpa: std.mem.Allocator, keep: []const u8, id: []const u8) void { - var names: [32][]const u8 = undefined; - var n: usize = 0; +fn evictIdFromOthers(self: *State, gpa: std.mem.Allocator, keep: []const u8, id: []const u8) !void { + var names: std.ArrayListUnmanaged([]const u8) = .empty; + defer names.deinit(gpa); var it = self.assignments.iterator(); while (it.next()) |e| { if (std.mem.eql(u8, e.key_ptr.*, keep)) continue; - for (e.value_ptr.*) |x| { - if (!std.mem.eql(u8, x, id)) continue; - if (n < names.len) { - names[n] = e.key_ptr.*; - n += 1; - } + for (e.value_ptr.*) |x| if (std.mem.eql(u8, x, id)) { + try names.append(gpa, e.key_ptr.*); break; - } + }; } - for (names[0..n]) |region| { + var kept: std.ArrayListUnmanaged([]const u8) = .empty; + defer kept.deinit(gpa); + for (names.items) |region| { const ids = self.assignment(region) orelse continue; - var kept: [32][]const u8 = undefined; - var k: usize = 0; - for (ids) |x| { - if (std.mem.eql(u8, x, id)) continue; - if (k < kept.len) { - kept[k] = x; - k += 1; - } - } - self.setAssignment(gpa, region, kept[0..k]) catch {}; + kept.clearRetainingCapacity(); + for (ids) |x| if (!std.mem.eql(u8, x, id)) try kept.append(gpa, x); + try self.setAssignment(gpa, region, kept.items); } } diff --git a/sdk/src/EditorAPI.zig b/sdk/src/EditorAPI.zig index 6ded8415c..ed07e9559 100644 --- a/sdk/src/EditorAPI.zig +++ b/sdk/src/EditorAPI.zig @@ -306,8 +306,8 @@ pub const VTable = struct { /// plugin can address a region it is not drawing right now: a tab dropped on another pane, /// a pane that does not exist yet, a session being restored before anything has drawn. assignSurfaces: *const fn (ctx: *anyopaque, region: []const u8, ids: ?[]const []const u8) anyerror!void, - /// The assignment for a region name, or null if the user never chose. Borrowed from the app; - /// copy before the next assignment write. + /// The assignment for a region name, or null if the user never chose. Borrowed from the app + /// until the next assignment write, which may itself be given these ids: read, edit, write. assignedSurfaces: *const fn (ctx: *anyopaque, region: []const u8) ?[]const []const u8, /// Every region name that has an assignment, arena-allocated. How a plugin finds the panes /// it declared last session before it has declared any this one. diff --git a/sdk/src/Host.zig b/sdk/src/Host.zig index d881e227f..6cdd7996b 100644 --- a/sdk/src/Host.zig +++ b/sdk/src/Host.zig @@ -606,8 +606,8 @@ pub fn assignSurfaces(self: *Host, region_name: []const u8, ids: ?[]const []cons if (self.fizzy_api) |a| return a.assignSurfaces(region_name, ids); } -/// The assignment under `region_name`, or null when the user never chose. Borrowed; copy before -/// the next `assignSurfaces`. +/// The assignment under `region_name`, or null when the user never chose. Borrowed until the next +/// `assignSurfaces`, which may itself be given these ids: read, edit, write. pub fn assignedSurfaces(self: *Host, region_name: []const u8) ?[]const []const u8 { return if (self.fizzy_api) |a| a.assignedSurfaces(region_name) else null; } diff --git a/tests/integration.zig b/tests/integration.zig index f99366bfc..17c65ba61 100644 --- a/tests/integration.zig +++ b/tests/integration.zig @@ -1901,6 +1901,72 @@ test "assigning a region overrides keyword matching, duplicates and empties" { try std.testing.expectEqualStrings("test.other", layout.matching(sidebar)[0].id); } +// A pane adds or drops a tab by reading its assignment, editing the list and writing it back +// (`Workspace.addTab`), so the list it writes borrows the ids it replaces. On 2026-10-16 that +// crashed fizzy in `evictIdFromOthers`, reading ids the write had just freed, on a document an +// agent opened beside the person. The page allocator unmaps every free, so a read of one faults +// here every time rather than whenever the freed slot's page happened to go. +test "assigning a region a list borrowed from the assignments it replaces" { + const gpa = std.heap.page_allocator; + var state: fizzy.Editor.Layout.State = .{}; + defer state.deinitAssignments(gpa); + + try state.assign(gpa, "pane.1", &.{ "doc.a", "doc.b" }); + try state.assign(gpa, "pane.2", &.{ "doc.c", "doc.d" }); + + // Pane 1 takes doc.c as a tab: its own ids, then the new one borrowed from pane 2's list. + const one = state.assignment("pane.1").?; + const two = state.assignment("pane.2").?; + try state.assign(gpa, "pane.1", &.{ one[0], one[1], two[0] }); + try std.testing.expectEqual(@as(usize, 3), state.assignment("pane.1").?.len); + try std.testing.expectEqualStrings("doc.c", state.assignment("pane.1").?[2]); + try std.testing.expectEqual(@as(usize, 1), state.assignment("pane.2").?.len); + try std.testing.expectEqualStrings("doc.d", state.assignment("pane.2").?[0]); + + // Pane 2 drops back to its first tab, written from its own list, under its own key. + const keyed = state.assignments.getEntry("pane.2").?; + try state.assign(gpa, keyed.key_ptr.*, keyed.value_ptr.*[0..1]); + try std.testing.expectEqualStrings("doc.d", state.assignment("pane.2").?[0]); +} + +// Taking a region's ids out of the others rewrites them, and rewriting even an existing entry +// can grow the map (`getOrPut` makes room first), moving every entry. Whatever `assign` holds of +// its own entry must outlive that. Some count of regions leaves the map full at the rewrite; +// trying every count up to well past a resize reaches it, and the page allocator faults on a +// stale read. +test "assigning a region whose ids other regions hold, whatever the map's fill" { + const gpa = std.heap.page_allocator; + for (1..64) |count| { + var state: fizzy.Editor.Layout.State = .{}; + defer state.deinitAssignments(gpa); + var names: [64][16]u8 = undefined; + var ids: [64][]const u8 = undefined; + for (0..count) |i| { + const name = try std.fmt.bufPrint(&names[i], "pane.{d}", .{i}); + try state.assign(gpa, name, &.{name}); + ids[i] = name; + } + // One region takes every id: each other region is emptied, none twice. + try state.assign(gpa, "all", ids[0..count]); + try std.testing.expectEqual(count, state.assignment("all").?.len); + for (ids[0..count]) |name| try std.testing.expectEqual(@as(usize, 0), state.assignment(name).?.len); + } +} + +// A place of many tabs keeps every one when another place takes one of them. +test "taking one id out of a region of many keeps the rest" { + const gpa = std.testing.allocator; + var state: fizzy.Editor.Layout.State = .{}; + defer state.deinitAssignments(gpa); + var bufs: [40][16]u8 = undefined; + var ids: [40][]const u8 = undefined; + for (&ids, 0..) |*id, i| id.* = try std.fmt.bufPrint(&bufs[i], "doc.{d}", .{i}); + try state.assign(gpa, "pane.1", &ids); + try state.assign(gpa, "pane.2", &.{ids[0]}); + try std.testing.expectEqual(@as(usize, 39), state.assignment("pane.1").?.len); + try std.testing.expectEqualStrings("doc.39", state.assignment("pane.1").?[38]); +} + // -- regions inside regions ---------------------------------------------------------------------- // A region declared inside another names a place *within* it: a document pane inside the main area @@ -7475,6 +7541,34 @@ test "headless: the whole editor comes up, opens a file, plays the tour and goes try std.testing.expect(std.mem.indexOf(u8, try docText(host), ".text = \"typed\"") != null); } + // Documents a command creates open beside the person (`Host.openBeside`), as an agent's + // `pixi.create` does, into panes whose tabs have come and gone. Each pane's tab list is + // rewritten from itself (`Workspace.addTab`), the crash of 2026-10-16 (`Layout.State.assign`). + { + const host = &editor.app.host; + const here = editor.workbench.open_workspace_grouping; + const grid: sdk.EditorAPI.NewDocGrid = .{ .column_width = 16, .row_height = 16 }; + var made: [4]u64 = undefined; + for (&made, 0..) |*id, i| { + const p = try std.fmt.allocPrint(gpa, "{s}/beside-{d}.txt", .{ project, i }); + defer gpa.free(p); + host.openBeside(); + defer host.endOpenBeside(); + const doc = try editor.newFile(p, grid); + id.* = doc.id; + try dvui.testing.settle(headlessFrame); + try std.testing.expectEqual(here, editor.workbench.open_workspace_grouping); + // Every other one goes again, leaving its pane's list shorter for the next. + if (i % 2 == 1) try editor.closeFileID(made[i - 1]); + try dvui.testing.settle(headlessFrame); + } + const last = editor.app.docById(made[made.len - 1]) orelse return error.TestUnexpectedResult; + const beside = for (editor.workbench.workspaces.values()) |ws| { + if (ws.grouping != here) if (ws.active) |a| if (a.id == last.id) break true; + } else false; + try std.testing.expect(beside); + } + // A document's history as a line (`Plugin.historyLine`): each step keeps its id through // undo and redo, and going to one (`historyGoTo`) is so many undos or redos. {