a73x

15b5fbcc

fix: collectLeafIds propagates the allocation failure it was swallowing

claude   2026-09-04 13:22

Commit message
fix: collectLeafIds propagates the allocation failure it was swallowing

`ids.append(alloc, t) catch {}` turned an OOM into a SHORT list, and a short
list of leaf ids is not a smaller tree — it is a wrong answer about this one.
Both callers read the list positionally, so both had a silent wrong-answer
path:

- `Tree.serialize` maps the focused leaf to its index in the depth-first
  walk. A dropped leaf shifts every later index, so the sidecar recorded
  `focus K` naming a different pane, or no focus line at all. The next
  attach then restored the wall focused on the wrong tile, and nothing on
  the way there was an error anyone could see.
- `Tree.remapLeaves` drives its removal pass off the list. A dropped id is a
  leaf whose `map` entry is null and which is never removed, so a pane the
  wall no longer has survived the heal and went back into the saved tree.

`collectLeafIds` now returns `Allocator.Error!void`. `serialize` already
returned an error union and just adds a `try`. `remapLeaves` gains one, and
the collection is still taken before the first mutation, so the error leaves
the tree exactly as the caller handed it in.

Both call sites in `wall_layout.zig` already degrade on allocation failure
and keep doing so: `saveLayoutTo` prints `wall layout not saved: OutOfMemory`
and writes nothing, and `seedLayout` returns null for the default cut. That
matches the sidecar's rule — it is derived convenience, so every failure
degrades to the default cut rather than persisting a tree that misdescribes
the wall.

Two tests pin it, each swapping the tree's allocator for a
`FailingAllocator` after the tree is built so the failure lands on exactly
the allocation under test: serialize refuses instead of writing a focus
line, and a failed remap leaves all three leaves under their original ids.

(cherry picked from commit 63752b67691c166fba98233dece8ce568d657e1a)
src/client/layout.zig
Old New
@@ -282,7 +282,7 @@ pub const Tree = struct {
282 if (focus) |fid| { 282 if (focus) |fid| {
283 var ids: std.ArrayListUnmanaged(u8) = .{}; 283 var ids: std.ArrayListUnmanaged(u8) = .{};
284 defer ids.deinit(self.alloc); 284 defer ids.deinit(self.alloc);
285 collectLeafIds(self.alloc, self.root, &ids); 285 try collectLeafIds(self.alloc, self.root, &ids);
286 for (ids.items, 0..) |id, k| { 286 for (ids.items, 0..) |id, k| {
287 if (id == fid) { 287 if (id == fid) {
288 try writer.print("focus {d}\n", .{k}); 288 try writer.print("focus {d}\n", .{k});
@@ -314,12 +314,16 @@ pub const Tree = struct {
314 /// containers collapse. Removals ALL happen before any rewrite, so old ids 314 /// containers collapse. Removals ALL happen before any rewrite, so old ids
315 /// stay addressable through the removal pass; the rewrite is one pass, so a 315 /// stay addressable through the removal pass; the rewrite is one pass, so a
316 /// new id cannot collide with a not-yet-rewritten old one. 316 /// new id cannot collide with a not-yet-rewritten old one.
317 pub fn remapLeaves(self: *Tree, map: []const ?u8) void { 317 ///
318 /// `error.OutOfMemory` leaves the tree UNTOUCHED: the only allocation is
319 /// the id list, and it is taken before the first mutation, so a caller that
320 /// gives up on the error is giving up on a tree it never modified.
321 pub fn remapLeaves(self: *Tree, map: []const ?u8) std.mem.Allocator.Error!void {
318 // Collect ids first because removal mutates the tree and may 322 // Collect ids first because removal mutates the tree and may
319 // collapse containers, invalidating node pointers. 323 // collapse containers, invalidating node pointers.
320 var ids: std.ArrayListUnmanaged(u8) = .{}; 324 var ids: std.ArrayListUnmanaged(u8) = .{};
321 defer ids.deinit(self.alloc); 325 defer ids.deinit(self.alloc);
322 collectLeafIds(self.alloc, self.root, &ids); 326 try collectLeafIds(self.alloc, self.root, &ids);
323 327
324 // Pass 1: removals. All null-mapped leaves are removed before any 328 // Pass 1: removals. All null-mapped leaves are removed before any
325 // id rewrite, so the old ids remain addressable throughout. 329 // id rewrite, so the old ids remain addressable throughout.
@@ -732,12 +736,17 @@ fn countLeadingSpaces(line: []const u8) usize {
732 return i; 736 return i;
733 } 737 }
734 738
735 fn collectLeafIds(alloc: std.mem.Allocator, node: ?*const Node, ids: *std.ArrayListUnmanaged(u8)) void { 739 /// Depth-first leaf ids, in encounter order. The allocation failure is
740 /// PROPAGATED, never swallowed: a short list is not a smaller tree, it is a
741 /// wrong answer about this one. Dropping a leaf here made `serialize` write a
742 /// `focus K` naming a different pane, and made `remapLeaves` skip a removal so
743 /// a pane the wall no longer has stayed in the saved tree.
744 fn collectLeafIds(alloc: std.mem.Allocator, node: ?*const Node, ids: *std.ArrayListUnmanaged(u8)) std.mem.Allocator.Error!void {
736 const n = node orelse return; 745 const n = node orelse return;
737 switch (n.*) { 746 switch (n.*) {
738 .leaf => |t| ids.append(alloc, t) catch {}, 747 .leaf => |t| try ids.append(alloc, t),
739 .container => |c| { 748 .container => |c| {
740 for (c.children.items) |child| collectLeafIds(alloc, child, ids); 749 for (c.children.items) |child| try collectLeafIds(alloc, child, ids);
741 }, 750 },
742 } 751 }
743 } 752 }
@@ -1324,7 +1333,7 @@ test "remapLeaves: null removes, containers collapse, ids rewrite in one pass" {
1324 try t.splitBelow(1, 2); 1333 try t.splitBelow(1, 2);
1325 // Leaf 1 has no wall line; 0 and 2 map to tiles 2 and 0 (a swap, the 1334 // Leaf 1 has no wall line; 0 and 2 map to tiles 2 and 0 (a swap, the
1326 // collision-prone case a two-pass rewrite gets wrong). 1335 // collision-prone case a two-pass rewrite gets wrong).
1327 t.remapLeaves(&[_]?u8{ 2, null, 0 }); 1336 try t.remapLeaves(&[_]?u8{ 2, null, 0 });
1328 try std.testing.expectEqual(@as(usize, 2), t.count()); 1337 try std.testing.expectEqual(@as(usize, 2), t.count());
1329 const f = try t.flatten(alloc, 24, 80, .{ .rows = 2, .cols = 3 }, null); 1338 const f = try t.flatten(alloc, 24, 80, .{ .rows = 2, .cols = 3 }, null);
1330 defer f.deinit(alloc); 1339 defer f.deinit(alloc);
@@ -1333,6 +1342,58 @@ test "remapLeaves: null removes, containers collapse, ids rewrite in one pass" {
1333 try std.testing.expect(f.rectOf(1) == null); 1342 try std.testing.expect(f.rectOf(1) == null);
1334 } 1343 }
1335 1344
1345 test "serialize: a failed leaf-id collection refuses rather than misnaming the focus" {
1346 const alloc = std.testing.allocator;
1347 var t = Tree.init(alloc);
1348 defer t.deinit();
1349 try t.addFirst(0);
1350 try t.splitRight(0, 1);
1351 try t.splitBelow(1, 2); // encounter order: 0, 1, 2
1352 const spellings = [_][]const u8{ "a", "b", "c" };
1353 var buf: std.ArrayListUnmanaged(u8) = .{};
1354 defer buf.deinit(alloc);
1355
1356 // The tree is built through the test allocator and only the id list is
1357 // taken through the failing one, so the failure lands on exactly the
1358 // allocation under test. `buf` keeps the test allocator: the node lines
1359 // are written before the id walk and are not what this pins.
1360 var failing = std.testing.FailingAllocator.init(alloc, .{ .fail_index = 0 });
1361 t.alloc = failing.allocator();
1362 const err = t.serialize(&spellings, 2, buf.writer(alloc));
1363 t.alloc = alloc;
1364
1365 try std.testing.expectError(error.OutOfMemory, err);
1366 // Not "focus 0" or a missing focus line pointing the next attach at the
1367 // wrong pane: a truncated walk finds leaf 2 at no index, or at the index
1368 // of whichever leaf survived the truncation.
1369 try std.testing.expect(std.mem.indexOf(u8, buf.items, "focus") == null);
1370 }
1371
1372 test "remapLeaves: a failed leaf-id collection leaves the tree untouched" {
1373 const alloc = std.testing.allocator;
1374 var t = Tree.init(alloc);
1375 defer t.deinit();
1376 try t.addFirst(0);
1377 try t.splitRight(0, 1);
1378 try t.splitBelow(1, 2);
1379
1380 var failing = std.testing.FailingAllocator.init(alloc, .{ .fail_index = 0 });
1381 t.alloc = failing.allocator();
1382 const err = t.remapLeaves(&[_]?u8{ 2, null, 0 });
1383 t.alloc = alloc;
1384
1385 try std.testing.expectError(error.OutOfMemory, err);
1386 // All three leaves still there under their original ids: the collection
1387 // is taken before the first removal, so the caller that gives up gets the
1388 // tree it handed in, not one with an arbitrary prefix of the map applied.
1389 try std.testing.expectEqual(@as(usize, 3), t.count());
1390 const f = try t.flatten(alloc, 24, 80, .{ .rows = 2, .cols = 3 }, null);
1391 defer f.deinit(alloc);
1392 try std.testing.expect(f.rectOf(0) != null);
1393 try std.testing.expect(f.rectOf(1) != null);
1394 try std.testing.expect(f.rectOf(2) != null);
1395 }
1396
1336 test "parse: a zero-weight child degrades to null, not a divide by zero" { 1397 test "parse: a zero-weight child degrades to null, not a divide by zero" {
1337 const alloc = std.testing.allocator; 1398 const alloc = std.testing.allocator;
1338 // Root cells are read and discarded, so only a non-root zero is a 1399 // Root cells are read and discarded, so only a non-root zero is a
src/tui/wall_layout.zig
Old New
@@ -474,7 +474,11 @@ fn seedAttempt(
474 parsed.deinit(alloc); 474 parsed.deinit(alloc);
475 return null; 475 return null;
476 } 476 }
477 parsed.tree.remapLeaves(map); 477 parsed.tree.remapLeaves(map) catch {
478 plan.deinit(alloc);
479 parsed.deinit(alloc);
480 return null;
481 };
478 if (base == 1 and entry_at == null) { 482 if (base == 1 and entry_at == null) {
479 const anchor: u8 = if (focus) |f| @intCast(f) else @intCast(base); 483 const anchor: u8 = if (focus) |f| @intCast(f) else @intCast(base);
480 parsed.tree.insert(anchor, 0) catch { 484 parsed.tree.insert(anchor, 0) catch {