From d89c0b532df23ed5b48495f83725893d5d82042b Mon Sep 17 00:00:00 2001 From: Gabriel Schneider Date: Thu, 3 Sep 2026 15:39:43 -0300 Subject: errors: a save that could not happen, and two panics on an ordinary click MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A review of what this program does when the environment says no. The finding that reframes it: there were almost NO panics on ordinary paths — the rule already held — but there was a great deal of silence, and one case worse than any panic. SILENT DATA LOSS ON SAVE. `saveFile` marked the pane saved the moment it QUEUED the effect, before any host had tried; `host_io.writeFd` returned void, so a short or failed write was indistinguishable from a complete one; and `writeFileBytes` returned true regardless. A save to a read-only file, or into a directory removed under the pane, therefore cleared the tag's ` *` and posted nothing — and `Del` makes no dirty check, so the next click threw the edits away with the screen saying they were safe. On a full disk it was worse: the file is already `O_TRUNC`'d when `write` fails, so the message row said `saved` over a file that had just been emptied. Now: `writeFd` reports, `writeFileBytes` returns WHY (`PermissionDenied`, `NoSpaceLeft`, `ReadOnlyFilesystem`, …) including a failed `close`, which is where write-back filesystems report at all; the core marks the pane saved around `perform` rather than at emit, which is also where the bytes are read; and a host that could not write calls `Pardes.saveFailed`, which puts the reason on the message row and takes the clean mark back. That is a CALL and not a return value because host.zig enforces, at comptime, that a `push_` method reaching every host in a fan-out cannot have one answer — the first attempt at this changed the signature and the compiler was right to refuse it. TWO PANICS ON AN ORDINARY KEYSTROKE, in look.zig's number scans. `v = v * 10 + d` over caller-supplied digits, reached from `parsePathLine` and the `@pN` scan — which every Look, every right-click and every n/N motion runs on whatever word is under the pointer. A hash in a log, a CSV column, any output shaped `foo:99999999999999999999`, and the editor died with "integer overflow". Both saturate now, the same way acmefs.zig's address parser already did; a saturated line is refused by `file_pane.open`'s `line <= total` and a saturated pane id by `focusPaneLine`'s `id < MAX_PANES`, so nothing addressable changes. A BOOT FILE THAT WILL NOT OPEN joins the missing-name case in the `+Errors` pane instead of taking the launch down: `pardes /root` resolves as a `.file`, could not be read, and left `error: PermissionDenied` and a return trace. `look.readFile` now says which errno it was, so the pane can say "permission denied" rather than a word from the source code. The tag-marker test drained no effects and passed anyway, which is exactly the defect; it drains now. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016Q4RATpafkwahrovHQLKRf --- src/pardes.zig | 144 +++++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 135 insertions(+), 9 deletions(-) (limited to 'src/pardes.zig') diff --git a/src/pardes.zig b/src/pardes.zig index 55e111e6..4956150a 100644 --- a/src/pardes.zig +++ b/src/pardes.zig @@ -1384,6 +1384,12 @@ test "an unsaved file marker sits between its path and builtins until Save" { try std.testing.expect(marker_at < save_at); try std.testing.expect(p.executeBuiltinLine(0, "Save")); + // DRAINED FIRST, and the drain is the point rather than ceremony: the + // marker now clears when the write LANDS, not when the effect is queued. + // This test used to pass without it, which is exactly what was wrong — a + // save the host could not do cleared the marker anyway. No frame is + // affected, because `pump` drains before it renders. + while (p.nextEffect()) |effect| p.perform(effect); const saved = try p.tagText(p.scratch.allocator(), pane); try std.testing.expect(std.mem.indexOf(u8, saved, "/hxcase.txt *") == null); @@ -1903,6 +1909,43 @@ test "Save takes the path as an argument, relative to the pane's own directory" try std.testing.expect(drainForSavePath(p, &buf) == null); } +test "a save the host could not do leaves the pane dirty" { + const gpa = std.testing.allocator; + // A host that refuses every write: a read-only file, a directory that was + // removed under the pane, a full disk. The core cannot tell those apart and + // does not need to — the host says why on the message row, and this is the + // other half, which is that the pane must NOT come clean. + const Refusing = struct { + fn writeFile(ctx: ?*anyopaque, pane: u8, _: []const u8, _: []const u8) void { + const core: *Pardes = @ptrCast(@alignCast(ctx.?)); + core.saveFailed(pane, "save", error.PermissionDenied); + } + }; + const p = try Pardes.init(gpa, .{ .tty_only = true, .cols = 100, .rows = 30 }); + defer p.deinit(); + while (p.nextEffect()) |_| {} + const pane = try p.hxOpenFileContent("before\n"); + file_pane.setContent(p, &pane.file.?, try gpa.dupe(u8, "after\n")); + try std.testing.expect(pane.file.?.revision != pane.file.?.saved_revision); + + p.host = .{ .ctx = p, .vtable = &.{ .push_write_file = Refusing.writeFile } }; + try std.testing.expect(p.executeBuiltinLine(0, "Save")); + while (p.nextEffect()) |effect| p.perform(effect); + // STILL DIRTY. Until this, `saveFile` marked the pane saved the moment it + // QUEUED the effect, so the tag's ` *` cleared on a save that never + // happened — and `Del` makes no dirty check, so the next click threw the + // edits away with the screen saying they were safe. + try std.testing.expect(pane.file.?.revision != pane.file.?.saved_revision); + + // ...and the same save against a host that CAN write does come clean, so + // the guard above is not simply "never clean". + p.host = .{}; + try std.testing.expect(p.executeBuiltinLine(0, "Save")); + while (p.nextEffect()) |effect| p.perform(effect); + try std.testing.expectEqual(pane.file.?.revision, pane.file.?.saved_revision); + try std.testing.expectEqualStrings("after\n", p.fallback.get("/hxcase.txt").?); +} + test "Save elsewhere copies a file's bytes and keeps the pane on its own file" { const gpa = std.testing.allocator; const p = try Pardes.init(gpa, .{ .tty_only = true, .cols = 100, .rows = 30 }); @@ -6328,13 +6371,42 @@ pub const Pardes = struct { // is asking to READ it, not to be handed a shell you did not ask // for and have to close — and the launch directory is one Newcol // away when it is wanted. Doc, PDF and image all boot the same way. - _ = initial_doc: { + const opened = initial_doc: { if (comptime pdf_enabled) if (look.isPdfPath(path)) - break :initial_doc try pdf_pane.openPane(p, 0, path, opts.file_line); + break :initial_doc pdf_pane.openPane(p, 0, path, opts.file_line); if (look.isImagePath(path)) - break :initial_doc try image_pane.create(p, 0, path, &.{}); - break :initial_doc try file_pane.open(p, 0, path, opts.file_line); + break :initial_doc image_pane.create(p, 0, path, &.{}); + break :initial_doc file_pane.open(p, 0, path, opts.file_line); }; + // ...AND IF IT WILL NOT OPEN, SAY SO IN THE WINDOW. `pardes /root` + // is a directory that resolves and cannot be read, so it arrives + // here as a `.file` and used to take the whole launch down with + // `error: PermissionDenied` and a return trace out of `main` — a + // crash, to the human, for asking to read something they are not + // allowed to read. Every environment reason lands in the same + // `+Errors` pane a missing name does, because they are the same + // event to whoever typed it: pardes cannot show you that. + // + // OUT OF MEMORY IS NOT ONE OF THEM and goes back to the caller. + // A core that could not allocate a file cannot allocate the pane + // explaining it, and pretending otherwise turns a clean failure + // into a second one. Every opener does its fallible IO BEFORE it + // claims a pane slot (`look.readFile`, then `newDocPane`), so slot + // 0 is still free here — which is what makes this legal. + if (opened) |_| {} else |err| { + if (err == error.OutOfMemory) return err; + const why = switch (err) { + error.PermissionDenied => "permission denied", + error.FileNotFound => "no file of that name", + error.IsDirectory => "that is a directory, and not one that could be read", + error.PathTooLong => "that path is too long", + error.FileTooLarge => "that file is too large to open", + else => @errorName(err), + }; + const content = try std.fmt.allocPrint(gpa, "cannot open\n\n\t{s}\n\n{s}\n", .{ path, why }); + errdefer gpa.free(content); + _ = try output_pane.open(p, 0, std.fs.path.dirname(path) orelse "/", .errors, "", content); + } p.ncol = 1; p.col_n[0] = 1; p.col_terms[0][0] = 0; @@ -7117,7 +7189,22 @@ pub const Pardes = struct { fn hostWriteFile(p: *Pardes, pane: u8, path: []const u8, bytes: []const u8) void { if (p.host.vtable.push_write_file) |f| return f(p.host.ctx, pane, path, bytes); - p.fallback.writeFile(path, bytes); + // The in-process filesystem reports the same way a real host does, so + // an OOM here leaves the pane dirty rather than looking saved. + if (!p.fallback.writeFile(path, bytes)) p.saveFailed(pane, "save", error.OutOfMemory); + } + + /// A HOST'S ANSWER TO `save_file`, and the only one it needs to give: the + /// write did not happen. Puts the reason on the pane's message row and + /// takes back the optimistic clean mark `perform` made, so the tag keeps + /// its ` *` and the edits keep being edits. See host.zig + /// `push_write_file` for why this is a call and not a return value. + pub fn saveFailed(p: *Pardes, id: u8, what: []const u8, err: anyerror) void { + if (p.panes[id]) |pane| if (pane.file) |*f| { + // The `-%` spelling acmefs.zig already uses for "make this dirty". + f.saved_revision = f.revision -% 1; + }; + p.reportError(id, what, err); } /// The path a watch is about: a real file's, or a PDF's. @@ -7171,12 +7258,25 @@ pub const Pardes = struct { p.fallback.setLink(u.slice()), .save_file => |sf| { const pane = p.panes[sf.pane] orelse return; - const f = pane.file orelse return; + const f = if (pane.file) |*file| file else return; + // CLEAN HERE rather than where the effect was queued, and + // BEFORE the call so the host can take it back. `saveFile` used + // to set `saved_revision` at emit time, so a write that could + // not happen still cleared the tag's ` *` and left the edits one + // `Del` from gone — `Del` makes no dirty check. Here is also + // where `f.content` is read, so the revision recorded is the + // revision of the bytes that actually went out. + f.saved_revision = f.revision; p.hostWriteFile(sf.pane, f.path, f.content); }, .save_text => |st| { const pane = p.panes[st.pane] orelse return; if (pane.serial != st.serial) return; // a recycled slot: not ours + // `Save ` is a COPY: it does not clean this pane, + // because the file the pane has open is not the file that was + // written. The one case that does clean is a scratch buffer + // adopting the path, and `saveTo` handles that by emitting + // `save_file` for the pane's own path instead. if (pane.file) |f| return p.hostWriteFile(st.pane, st.path.slice(), f.content); if (!pane.isTerminal()) return; const text = term_pane.screenTextAlloc(pane, p.gpa) catch return; @@ -7191,7 +7291,7 @@ pub const Pardes = struct { // A real host reports where it landed, which is what puts // `Restore ` in the topbar; the virtual one owes the // same, or the bytes it holds are unreachable. - p.fallback.writeFile(fallback_dump_path, out); + _ = p.fallback.writeFile(fallback_dump_path, out); p.setLastDump(fallback_dump_path); } }, @@ -10306,8 +10406,10 @@ pub const Pardes = struct { const pane = p.panes[id] orelse return; const f = if (pane.file) |*file| file else return; if (f.output != null) return; // nothing behind it yet: saveTo, with a path + // NOT marked saved here: the effect has only been QUEUED. `perform`'s + // `.save_file` arm cleans the pane if and only if the host says the + // bytes landed — see there. p.emit(.{ .save_file = .{ .pane = @intCast(id) } }); - f.saved_revision = f.revision; } /// Commit a path — prompted, typed after the word, or chorded onto it. @@ -10356,7 +10458,6 @@ pub const Pardes = struct { f.path = owned; f.output = null; // an ordinary file pane from here on pane.cwd = .none; // its directory is now its own path's dirname - f.saved_revision = f.revision; pane.tag_init = false; // re-derive the tag as a plain file pane.tag_tail_len = 0; p.emit(.{ .save_file = .{ .pane = @intCast(id) } }); @@ -16460,6 +16561,31 @@ test "Esc alternates between two panes of the SAME kind" { } } +test "a boot file that will not open boots an errors pane rather than failing the launch" { + if (platform == .web) return; + const gpa = std.testing.allocator; + // A path `look.resolve` would have accepted and `readFile` cannot open. + // Spelled as a name that is simply not there rather than by chmod-ing a + // fixture to 000, because the second answers differently when the suite + // runs as root and this must fail the same way everywhere. + const p = try Pardes.init(gpa, .{ + .cols = 80, + .rows = 24, + .file = "/definitely/not/here/notes.md", + }); + defer p.deinit(); + p.update(.{ .resize = .{ .cols = 80, .rows = 24 } }); + + const pane = p.panes[0].?; + const f = pane.file.?; + try std.testing.expectEqual(output_pane.Origin.errors, f.output.?.from); + // The reason IN WORDS, not an error name: `PermissionDenied` on a screen + // is jargon, and the whole point of this pane is that a human reads it. + try std.testing.expect(std.mem.indexOf(u8, f.content, "cannot open") != null); + try std.testing.expect(std.mem.indexOf(u8, f.content, "no file of that name") != null); + try std.testing.expect(std.mem.indexOf(u8, f.content, "/definitely/not/here/notes.md") != null); +} + test "argv naming nothing boots an errors pane rather than failing the launch" { const gpa = std.testing.allocator; const p = try Pardes.init(gpa, .{ -- cgit v1.3