From d1a2ca6ccc1a7b1392f80f8e3a12ee4dd67b14ac Mon Sep 17 00:00:00 2001 From: Matthew Meszaros Date: Fri, 14 Aug 2026 05:24:34 +0200 Subject: [PATCH] fix: stop discarding errors that had somewhere to go An audit of the 194 `catch {}` sites in src. Most are in component `view` functions, where the signature genuinely cannot report anything -- those need the fallible-view treatment and are left for a follow-up. These are the ones where an error was being dropped despite having a caller that could handle it. `LayerStack.render` returned `""` when the cell grid could not be allocated and swallowed every write failure after that, so an out-of-memory frame reached the screen as a truncated one, looking like a compositing bug. It is fallible now. The `.cache_image` and `.delete_image` commands dropped every error, while `.image_file` right next to them propagated. An unsupported protocol already returns false rather than erroring, so what was being discarded was genuine I/O failure -- and a cache_image that quietly failed left the following place_cached_image drawing nothing, with no signal anywhere. `dev_console` swallowed an append that cannot fail: the slice is a subset of what was just cleared, so the capacity is already there. `appendSliceAssumeCapacity` says so. The rest -- terminal cleanup, the suspend path -- are best-effort by nature: they run while unwinding, with nobody left to report to. Those now carry a comment saying why, so the next reader does not have to work it out again. --- src/core/dev_console.zig | 4 +++- src/core/program.zig | 24 +++++++++++++++--------- src/layout/layer.zig | 20 ++++++++++++-------- src/terminal/terminal.zig | 7 +++++++ tests/layout_tests.zig | 35 +++++++++++++++++++++++++++++++---- 5 files changed, 68 insertions(+), 22 deletions(-) diff --git a/src/core/dev_console.zig b/src/core/dev_console.zig index 1167c9b..b162b0c 100644 --- a/src/core/dev_console.zig +++ b/src/core/dev_console.zig @@ -237,8 +237,10 @@ pub const DevConsole = struct { } if (!still_alive) conn.close(io); } + // `keep` is a subset of what was just cleared, so the + // capacity is already there and this cannot fail. tcp.connections.clearRetainingCapacity(); - tcp.connections.appendSlice(keep.items) catch {}; + tcp.connections.appendSliceAssumeCapacity(keep.items); } }, } diff --git a/src/core/program.zig b/src/core/program.zig index 261c7be..041d0f5 100644 --- a/src/core/program.zig +++ b/src/core/program.zig @@ -468,13 +468,16 @@ pub fn Program(comptime Model: type) type { term.cleanup(); } - // Raise SIGTSTP to suspend process + // Raise SIGTSTP to suspend process. If the signal cannot be + // raised we simply never stop; carrying on is better than dying. if (builtin.os.tag != .windows) { const posix = std.posix; _ = posix.raise(posix.SIG.TSTP) catch {}; } - // When we resume (after `fg`), re-setup terminal + // When we resume (after `fg`), re-setup terminal. A failure here + // leaves the terminal in the shell's mode, which renders badly but + // still runs -- and there is no caller to report it to. if (self.terminal) |*term| { term.setup() catch {}; } @@ -626,27 +629,30 @@ pub fn Program(comptime Model: type) type { }, .cache_image => |cache| { if (self.terminal) |*term| { + // An unsupported protocol returns false rather than an + // error, so anything that does surface here is a real + // I/O failure, the same as in `flushPendingImage`. switch (cache.source) { .file => |path| { - _ = term.transmitKittyImageFromFile(path, .{ + _ = try term.transmitKittyImageFromFile(path, .{ .image_id = cache.image_id, .format = @enumFromInt(@intFromEnum(cache.format)), .quiet = cache.quiet, .pixel_width = cache.pixel_width, .pixel_height = cache.pixel_height, - }) catch {}; + }); }, .data => |data| { - _ = term.transmitKittyImage(data, .{ + _ = try term.transmitKittyImage(data, .{ .image_id = cache.image_id, .format = @enumFromInt(@intFromEnum(cache.format)), .quiet = cache.quiet, .pixel_width = cache.pixel_width, .pixel_height = cache.pixel_height, - }) catch {}; + }); }, } - term.flush() catch {}; + try term.flush(); } }, .place_cached_image => |place| { @@ -659,8 +665,8 @@ pub fn Program(comptime Model: type) type { .by_placement => |bp| .{ .by_placement = .{ .image_id = bp.image_id, .placement_id = bp.placement_id } }, .all => .all, }; - _ = term.deleteKittyImage(target) catch {}; - term.flush() catch {}; + _ = try term.deleteKittyImage(target); + try term.flush(); } }, } diff --git a/src/layout/layer.zig b/src/layout/layer.zig index 6ef1e6c..e8a7eee 100644 --- a/src/layout/layer.zig +++ b/src/layout/layer.zig @@ -54,13 +54,17 @@ pub const LayerStack = struct { } /// Composite all layers and return the final rendered string. - pub fn render(self: *const LayerStack, allocator: std.mem.Allocator) []const u8 { + /// + /// Fallible on purpose: an allocation failure part-way through used to + /// return an empty or truncated frame, which reaches the screen looking + /// like a rendering bug rather than an error. + pub fn render(self: *const LayerStack, allocator: std.mem.Allocator) ![]const u8 { const w: usize = self.width; const h: usize = self.height; // Create cell buffer: each cell stores a byte slice (content) and ANSI state // For simplicity, we use a 2D grid of cells that stores display characters - const grid = allocator.alloc(Cell, w * h) catch return ""; + const grid = try allocator.alloc(Cell, w * h); // Fill with background const bg = [1]u8{self.background}; @@ -69,7 +73,7 @@ pub const LayerStack = struct { } // Sort layers by z-index - const sorted = allocator.alloc(Layer, self.layers.items.len) catch return ""; + const sorted = try allocator.alloc(Layer, self.layers.items.len); @memcpy(sorted, self.layers.items); std.mem.sort(Layer, sorted, {}, struct { fn lessThan(_: void, a: Layer, b: Layer) bool { @@ -87,17 +91,17 @@ pub const LayerStack = struct { const writer = &result.writer; for (0..h) |row| { - if (row > 0) writer.writeByte('\n') catch {}; + if (row > 0) try writer.writeByte('\n'); for (0..w) |col| { const cell = grid[row * w + col]; // Empty content marks the second column of a wide character if (cell.content.len == 0) continue; if (cell.ansi_prefix.len > 0) { - writer.writeAll(cell.ansi_prefix) catch {}; - writer.writeAll(cell.content) catch {}; - writer.writeAll("\x1b[0m") catch {}; + try writer.writeAll(cell.ansi_prefix); + try writer.writeAll(cell.content); + try writer.writeAll("\x1b[0m"); } else { - writer.writeAll(cell.content) catch {}; + try writer.writeAll(cell.content); } } } diff --git a/src/terminal/terminal.zig b/src/terminal/terminal.zig index b32263a..a8dfaef 100644 --- a/src/terminal/terminal.zig +++ b/src/terminal/terminal.zig @@ -315,6 +315,8 @@ pub const Terminal = struct { pub fn setup(self: *Terminal) !void { // Setup signal handlers + // Resize notifications are a nicety: without the handler the program + // still runs, it just stops noticing the window changing size. platform.setupSignals() catch {}; // Enable raw mode @@ -357,6 +359,11 @@ pub const Terminal = struct { try self.flush(); } + /// Put the terminal back the way it was found. + /// + /// Every write here is best-effort. This runs on the way out, often while + /// already unwinding from a failure, and there is nobody left to report to + /// — so a broken pipe must not stop the remaining modes from being reset. pub fn cleanup(self: *Terminal) void { // Disable Kitty keyboard protocol if (self.config.kitty_keyboard) { diff --git a/tests/layout_tests.zig b/tests/layout_tests.zig index 32f2f6d..dca3586 100644 --- a/tests/layout_tests.zig +++ b/tests/layout_tests.zig @@ -173,7 +173,7 @@ test "layer.LayerStack - multibyte UTF-8 chars occupy one cell" { try stack.push(.{ .content = "╭─╮", .transparent = false }); - try testing.expectEqualStrings("╭─╮ ", stack.render(allocator)); + try testing.expectEqualStrings("╭─╮ ", try stack.render(allocator)); } test "layer.LayerStack - styled multibyte border chars stay intact" { @@ -189,7 +189,7 @@ test "layer.LayerStack - styled multibyte border chars stay intact" { try testing.expectEqualStrings( "\x1b[36m─\x1b[0m\x1b[36m│\x1b[0m ", - stack.render(allocator), + try stack.render(allocator), ); } @@ -205,7 +205,7 @@ test "layer.LayerStack - overlay aligns on UTF-8 background" { try stack.push(.{ .content = "──────", .z = 0, .transparent = false }); try stack.push(.{ .content = "AB", .x = 2, .z = 1 }); - try testing.expectEqualStrings("──AB──", stack.render(allocator)); + try testing.expectEqualStrings("──AB──", try stack.render(allocator)); } test "layer.LayerStack - wide characters cover two cells" { @@ -219,5 +219,32 @@ test "layer.LayerStack - wide characters cover two cells" { try stack.push(.{ .content = "你a", .transparent = false }); - try testing.expectEqualStrings("你a ", stack.render(allocator)); + try testing.expectEqualStrings("你a ", try stack.render(allocator)); +} + +test "layer.LayerStack - reports allocation failure instead of truncating" { + // The old signature had no way to say an allocation failed, so it returned + // a short frame — which reaches the screen looking like a rendering bug. + // Fail at each allocation in turn and check the result is always either a + // whole frame or an error, never something in between. + var fail_index: usize = 0; + while (fail_index < 8) : (fail_index += 1) { + var arena = std.heap.ArenaAllocator.init(testing.allocator); + defer arena.deinit(); + + var failing = std.testing.FailingAllocator.init( + arena.allocator(), + .{ .fail_index = fail_index }, + ); + const allocator = failing.allocator(); + + var stack = zz.layout.layer.LayerStack.init(allocator); + defer stack.deinit(); + stack.setSize(40, 10); + + stack.push(.{ .content = "hello", .transparent = false }) catch continue; + + const out = stack.render(allocator) catch continue; + try testing.expectEqual(@as(usize, 10), zz.height(out)); + } }