crash: a panic record must not be able to hang the process it is recording

Adversarial re-review, confirmed against std's source and then measured.

`captureCurrentStackTrace` is not the safe half of `writeCurrentStackTrace`.
`StackIterator.init` picks the `.di` strategy whenever `SelfInfo` can unwind,
`stratOk` accepts `.di` regardless of `allow_unsafe_unwind`, and `.di` takes
`SelfInfo`'s rwlock EXCLUSIVELY on its first call — the only kind of call a
panic record makes — across `dl_iterate_phdr`, a DWARF CFI machine and an
allocation. A panic in there (a smashed stack is a leading reason to be in a
panic handler at all) leaves the lock held, because the unlock is a `defer` in
a frame that never returns, and `defaultPanic` then waits on it for the life of
the process. A crash becomes a hang, which is worse than what this file was
added to improve on. The frames stay on stderr, where defaultPanic prints them
under the staging that makes them safe; the record keeps what can be gathered
without asking the process any questions.

ONE record per process, never released. With the guard released on the way out,
one panic wrote two records: the real message, then "reached unreachable code"
under it. That second panic is this handler's own `vaxis.recover()` running a
second time — it closes the vaxis tty and never clears the global saying there
is one, so the double close is `recoverableOsBugDetected` and an `unreachable`
in a Debug build. Guarded now in both the panic and the segfault handler; that
half is a fix older than the crash file.

`clock_gettime`'s return is checked, unlike dump.zig's, because a failure here
leaves `ts` undefined and an undefined large-positive `sec` walks
`calculateYearDay`'s u16 year past 65535 and overflow-panics inside the panic
handler. Debug fills it with 0xaa and lands in 1970, which is why it reads as
harmless.

Verified end to end with a temporary probe: one panic, one record.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_016Q4RATpafkwahrovHQLKRf

5 files changed, 125 insertions, 80 deletions

diff --git a/docs/config.md b/docs/config.md
index b95f207e..448ac763 100644
--- a/docs/config.md
+++ b/docs/config.md
@@ -102,24 +102,24 @@ the one place this program cannot keep a trace: in the TTY shell stderr IS the
 screen, so the trace lands on a grid the terminal is being reset out of; the SDL
 and AppKit shells have no terminal at all; and a `--detach` session's stderr
 goes wherever its launcher left it. The file is appended, never rewritten, and
-each record is one line of build metadata, the panic message, and the return
-addresses behind it:
+each record is two lines — build metadata, then the panic message:
 
 ```text
 pardes 0.0.2 (a1b2c3d) 2026-09-03T11:20:44Z linux-x86_64 pid 48812
 panic: index out of bounds: index 4, len 4
-  0x11ccb5a
-  0x11cc84c
-  0x11cc67a
 ```
 
-`addr2line -e <the pardes binary>` turns those into source lines, against the
-build the metadata line names. They are addresses rather than the symbolised
-trace stderr gets for a measured reason: symbolising from inside a panic
-handler, before `std.debug.defaultPanic` has run, HANGS the process — reading
-DWARF can itself panic, and the staging that turns a nested panic into
-"aborting due to recursive panic" is `defaultPanic`'s own and private. Walking
-frames is safe; symbolising them is not.
+NO STACK TRACE, and that is a measured decision rather than an omission. The
+frames stay on stderr, where `std.debug.defaultPanic` prints them. Collecting
+them here instead HANGS the process: `writeCurrentStackTrace` called from a
+panic handler before `defaultPanic` has run wedges at 0% CPU, and
+`captureCurrentStackTrace` — which looks like the safe half — takes `SelfInfo`'s
+rwlock exclusively on its first call, so a panic inside the walk leaves that
+lock held and `defaultPanic` then waits on it forever. What makes `defaultPanic`
+survive the same hazard is its own private `panic_stage`, which nothing outside
+`std.debug` can reach. A crash that becomes a hang is worse than the crash, so
+this file keeps only what it can gather without asking the process any
+questions: which build, when, where, and what it said.
 
 Everything about it is best effort and silent: no config directory (a launch
 with no `HOME`) means no file, and a directory that cannot be created or opened
diff --git a/src/CHANGELOG.md b/src/CHANGELOG.md
index 4cef906d..bb7e9ad5 100644
--- a/src/CHANGELOG.md
+++ b/src/CHANGELOG.md
@@ -15,14 +15,22 @@
   fact. `src/crash.zig` runs inside the panic handler, so it takes no lock of
   this program's, allocates nothing of its own, and every failure is swallowed:
   a crash file that could not be written must not become the crash, and stderr
-  still gets its own copy either way. The file gets RETURN ADDRESSES rather
-  than the symbolised trace, and that is measured rather than chosen —
-  `writeCurrentStackTrace` called from a panic handler *before* `defaultPanic`
-  wedges the process at 0% CPU: symbolising reads DWARF, that read can panic,
-  and the staging which turns a nested panic into "aborting due to recursive
-  panic" is `defaultPanic`'s own and private. Walking frames is safe, so the
-  addresses go in the file and `addr2line -e` against the build named on the
-  line above them finishes the job. The AppKit shell got a panic handler of its
+  still gets its own copy either way. The file carries NO STACK TRACE, and that
+  is measured rather than chosen: `writeCurrentStackTrace` called from a panic
+  handler *before* `defaultPanic` wedges the process at 0% CPU, and
+  `captureCurrentStackTrace` — which looks like the safe half of it — takes
+  `SelfInfo`'s rwlock exclusively on its first call, so a panic inside the walk
+  leaves that lock held and `defaultPanic` waits on it for the life of the
+  process. A crash that becomes a hang is worse than the crash. What makes
+  `defaultPanic` itself survive that is its private `panic_stage`, reachable
+  from nowhere outside `std.debug`, so the frames stay on stderr where they
+  already work. ONE record per process, because the nested panic std's trace
+  printer raises comes back through the handler: the first version of this
+  wrote the real message and then "reached unreachable code" underneath it,
+  which is the panic handler's own second `vaxis.recover()` double-closing the
+  tty — `recover()` never cleared the global saying there was one. That call is
+  guarded now too, in both the panic and the segfault handler, which is a fix
+  older than this file. The AppKit shell got a panic handler of its
   own in the process: the macOS build roots at `macos.zig`, so the one in
   `main.zig` had never run there — in the shell with the least useful stderr of
   the four.
diff --git a/src/crash.zig b/src/crash.zig
index 8de2abed..d9c47715 100644
--- a/src/crash.zig
+++ b/src/crash.zig
@@ -10,9 +10,10 @@
 //! that produced it: a bug report needs the version and the commit, and a user
 //! who runs pardes already knows where its config lives.
 //!
-//! The record is the metadata line, the panic message, and the RETURN
-//! ADDRESSES — not the symbolised trace stderr gets. `record` says why in as
-//! many words: symbolising from out here hangs the process, measured.
+//! The record is the metadata line and the panic message, and NOT the frames:
+//! every way of collecting those from out here can wedge the process instead of
+//! ending it. `record` says exactly why, at length, because the reasoning is
+//! not visible from the call it does not make.
 //!
 //! Panics only. A SIGSEGV never reaches a panic handler (main.zig `debug`
 //! hooks that path separately) and unwinding one needs the signal's saved cpu
@@ -56,20 +57,18 @@ else
 /// the fd is opened `O_APPEND` and one `write(2)` of the lot is what keeps two
 /// crashing processes from interleaving their records line by line. Static
 /// rather than a local: a panic handler may be running on a thread stack of a
-/// few pages.
+/// few pages. Two lines fit several times over — the panic message is the only
+/// part with no bound of its own, and a longer one is simply cut.
 var scratch: [8 << 10]u8 = undefined;
 
-/// Deep enough to reach `main` through any of this program's loops, and the
-/// only sizing decision here: the addresses are 8 bytes each and cost a line.
-var addr_buf: [64]usize = undefined;
-
-/// One at a time, and never inside itself. `defaultPanic` gets both properties
-/// from a private `panic_stage` and from the stderr lock, and neither is
-/// reachable from out here: two threads panicking at once would interleave
-/// `@memcpy`s into one `scratch` and each write out the mixture, and a panic
-/// raised INSIDE this function — the walk below is the plausible place — would
-/// re-enter it with the fd still open. Both end here instead, and the loser
-/// falls through to `defaultPanic`, which still says everything on stderr.
+/// One record per process, taken by whoever panics first. `defaultPanic` gets
+/// the same property from a private `panic_stage` and from the stderr lock, and
+/// neither is reachable from out here: two threads panicking at once would
+/// interleave `@memcpy`s into one `scratch` and each write out the mixture, and
+/// the nested panic `defaultPanic`'s own trace printer can raise comes back
+/// through the handler and appends its wreckage under the real message. Both
+/// end here instead, and the loser falls through to `defaultPanic`, which still
+/// says everything on stderr.
 var recording: std.atomic.Value(bool) = .init(false);
 
 /// Append one crash record, or silently do nothing. Called FROM the panic
@@ -77,12 +76,15 @@ var recording: std.atomic.Value(bool) = .init(false);
 /// own, and does its own file I/O: a static buffer, one `open`, one `write`.
 /// Every failure is swallowed — a crash file that could not be written must not
 /// become the crash, and stderr is about to get the message either way.
-pub fn record(msg: []const u8, first_trace_addr: ?usize) void {
+pub fn record(msg: []const u8) void {
     if (dir_len == 0) return;
+    // ONCE PER PROCESS, and never released. A process panics once and dies; a
+    // SECOND record is always the wreckage of the first. Observed with the
+    // flag released on the way out: `defaultPanic`'s own trace printing raised
+    // a nested panic, which came back through the handler and appended
+    // "panic: reached unreachable code" under the real message — burying the
+    // one line the file exists to keep. A test resets it deliberately.
     if (recording.swap(true, .seq_cst)) return;
-    // Released on the way out, which matters to nobody at panic time — the
-    // process is about to abort — and lets a test call this twice.
-    defer recording.store(false, .seq_cst);
     const config_dir = dir_buf[0..dir_len];
     var path_buf: [1024:0]u8 = undefined;
     if (config_dir.len + 1 + name.len >= path_buf.len) return;
@@ -113,8 +115,14 @@ pub fn record(msg: []const u8, first_trace_addr: ?usize) void {
     // The metadata line. Same clock and the same civil-time arithmetic as
     // dump.zig's filename, printed as UTC because a crash file is read by
     // whoever the reporter sends it to and their zone is not the reporter's.
-    var ts: std.c.timespec = undefined;
-    _ = std.c.clock_gettime(.REALTIME, &ts);
+    //
+    // The return IS checked, unlike dump.zig's, because this one runs where a
+    // panic cannot be afforded: a failed `clock_gettime` leaves `ts` undefined,
+    // and an undefined large-positive `sec` walks `calculateYearDay`'s `u16`
+    // year past 65535 and overflow-panics INSIDE the panic handler. Debug fills
+    // it with 0xaa and lands in 1970, which is why it reads as harmless.
+    var ts: std.c.timespec = .{ .sec = 0, .nsec = 0 };
+    if (std.c.clock_gettime(.REALTIME, &ts) != 0) ts = .{ .sec = 0, .nsec = 0 };
     const es: std.time.epoch.EpochSeconds = .{ .secs = @intCast(@max(0, ts.sec)) };
     const yd = es.getEpochDay().calculateYearDay();
     const md = yd.calculateMonthDay();
@@ -134,34 +142,43 @@ pub fn record(msg: []const u8, first_trace_addr: ?usize) void {
         @as(u32, @intCast(std.c.getpid())),
     }) catch {};
 
-    // ...and under it the message stderr is about to print, then the return
-    // addresses behind it.
+    // ...and under it the line stderr is about to print.
+    //
+    // NO STACK TRACE, AND NOT EVEN THE RAW RETURN ADDRESSES. This is the whole
+    // design decision in this file, and it was arrived at by measurement, so it
+    // is written down here rather than left to be rediscovered.
+    //
+    // `std.debug.writeCurrentStackTrace` — the call `defaultPanic` makes a
+    // moment later, into a writer of its own — HANGS THE PROCESS when it is
+    // made from a panic handler BEFORE `defaultPanic` has run. Observed at 0%
+    // CPU in `futex_do_wait` with libc's debug info half-open, identically in a
+    // test binary and in a standalone build carrying this program's
+    // `std_options_debug_io`. Calling `debug_io.vtable.crashHandler` first does
+    // not help, and neither does holding `lockStderr` around it.
     //
-    // ADDRESSES AND NOT THE SYMBOLISED TRACE, which is the one thing here that
-    // is not simply "the same bytes stderr gets". `std.debug.writeCurrentStack
-    // Trace` — the call `defaultPanic` makes a moment later, into a writer of
-    // its own — HANGS THE PROCESS when it is made from a panic handler before
-    // `defaultPanic` has run. Symbolising reads DWARF, and on a machine with
-    // debug info to fetch that read can itself panic; `defaultPanic` survives
-    // that because its private `panic_stage` turns the nested panic into
-    // "aborting due to recursive panic" and an abort. Outside it there is no
-    // such staging, and the recursion ends in a futex nobody will post: a
-    // panicking pardes stopped dead instead of dying. Measured, not guessed —
-    // with the stack-trace call the process wedged at 0% CPU with libc's debug
-    // info half-open, and it did it identically inside a test binary and in a
-    // standalone build with this program's `std_options_debug_io`.
+    // `captureCurrentStackTrace` looks like the safe half of that and is NOT.
+    // `StackIterator.init` picks the `.di` strategy whenever `SelfInfo` can
+    // unwind, `stratOk` accepts `.di` no matter what `allow_unsafe_unwind`
+    // says, and `.di` unwinding takes `SelfInfo`'s rwlock EXCLUSIVELY on its
+    // first call — the only kind of call a panic record ever makes — while it
+    // runs `dl_iterate_phdr` and a DWARF CFI machine and allocates. A panic in
+    // there (a smashed stack is a leading cause of getting here at all) leaves
+    // that lock held forever, because the unlock is a `defer` in a frame that
+    // never returns. `defaultPanic` then asks for the same lock and waits on it
+    // for the life of the process: A CRASH BECOMES A HANG, which is strictly
+    // worse than the behaviour this file was added to improve on. What makes
+    // `defaultPanic` itself survive that is its own PRIVATE `panic_stage`,
+    // reachable from nowhere out here.
     //
-    // `captureCurrentStackTrace` only walks frames, so it is safe here, and the
-    // addresses resolve with `addr2line -e <the pardes binary>` against the
-    // build the line above names. stderr still gets the symbolised trace from
-    // `defaultPanic`, unchanged.
+    // So the frames stay on stderr, where `defaultPanic` prints them under the
+    // staging that makes them safe, and this file keeps what it can gather
+    // without asking the process any questions: which build, when, where, and
+    // what it said. That is the half a reporter cannot reconstruct afterwards.
     w.print("panic: {s}\n", .{msg}) catch {};
-    const trace = std.debug.captureCurrentStackTrace(.{
-        .first_address = first_trace_addr orelse @returnAddress(),
-        .allow_unsafe_unwind = true,
-    }, &addr_buf);
-    for (trace.return_addresses) |a| w.print("  0x{x}\n", .{a}) catch {};
 
+    // One `write(2)`, and the loop is for a short write rather than for a
+    // second record: `O_APPEND` puts it at the end whatever else is writing,
+    // and nothing above here can fail in a way that leaves it half-built.
     const written = w.buffered();
     var off: usize = 0;
     while (off < written.len) {
@@ -187,8 +204,12 @@ test "a crash record names the build, appends, and carries the panic message" {
     defer dir_len = saved_len;
     setDir(config_dir);
 
-    record("first boom", null);
-    record("second boom", null);
+    record("first boom");
+    // Deliberately released, which the panic path never does — see `recording`.
+    // Two records is how the APPEND is proved, and this is the only caller
+    // allowed to ask for a second one.
+    recording.store(false, .seq_cst);
+    record("second boom");
 
     const path = try std.fs.path.join(gpa, &.{ config_dir, name });
     defer gpa.free(path);
@@ -207,19 +228,20 @@ test "a crash record names the build, appends, and carries the panic message" {
     try std.testing.expect(stamp < second);
     try std.testing.expect(std.mem.indexOf(u8, bytes, "1970-") == null);
 
-    // ...and at least one return address under the FIRST message, which is the
-    // only part of this that can quietly produce nothing: an unwinder that
-    // refuses the strategy hands back an empty trace and the loop writes no
-    // lines at all. Asserted between the two records so it cannot be satisfied
-    // by the second one's.
-    const first_msg = std.mem.indexOf(u8, bytes, "panic: first boom\n").?;
-    const addr = std.mem.indexOfPos(u8, bytes, first_msg, "\n  0x").?;
-    try std.testing.expect(addr < second);
+    // Two lines per record and no third: the frames deliberately do NOT come
+    // here (see `record`), and a future edit that starts collecting them again
+    // is the thing this pins. Counted rather than pattern-matched, because the
+    // failure being guarded is "something extra appeared", which no assertion
+    // about known content can see.
+    // Three newlines a record: the blank one that separates records, the end of
+    // the metadata line, and the end of the `panic:` line.
+    try std.testing.expectEqual(@as(usize, 6), std.mem.count(u8, bytes, "\n"));
 
     // No directory, no file: a core that never resolved a config directory
     // panics exactly as it did before this existed.
     dir_len = 0;
-    record("unrecorded", null);
+    recording.store(false, .seq_cst);
+    record("unrecorded");
     const again = try std.Io.Dir.cwd().readFileAlloc(io, path, gpa, .limited(1 << 20));
     defer gpa.free(again);
     try std.testing.expectEqual(bytes.len, again.len);
diff --git a/src/macos.zig b/src/macos.zig
index a9c9d0f3..f2e68a85 100644
--- a/src/macos.zig
+++ b/src/macos.zig
@@ -50,7 +50,7 @@ const crash = @import("crash.zig");
 /// terminal to restore either, which is the rest of what main.zig's does.
 pub const panic = std.debug.FullPanic(struct {
     fn call(msg: []const u8, ret_addr: ?usize) noreturn {
-        crash.record(msg, ret_addr);
+        crash.record(msg);
         std.debug.defaultPanic(msg, ret_addr);
     }
 }.call);
diff --git a/src/main.zig b/src/main.zig
index 60491052..92b59bc2 100644
--- a/src/main.zig
+++ b/src/main.zig
@@ -53,18 +53,33 @@ fn logFn(
 // exactly the behaviour that was here before.
 pub const panic = if (is_emscripten) std.debug.FullPanic(std.debug.defaultPanic) else std.debug.FullPanic(struct {
     fn call(msg: []const u8, ret_addr: ?usize) noreturn {
-        @import("vaxis").recover();
-        @import("crash.zig").record(msg, ret_addr);
+        // ONCE. This handler is re-entered whenever something panics while it
+        // runs, and std's own trace printer does exactly that — `defaultPanic`
+        // survives its own recursion through a private `panic_stage`, and
+        // everything up here is in front of that guard. `recover()` is not
+        // idempotent: it closes the vaxis tty and never clears the global that
+        // says there is one, so a second call double-closes, which std answers
+        // with `recoverableOsBugDetected` and an `unreachable` in a Debug
+        // build. Measured with the crash file in place: one panic left TWO
+        // records, the real message and then "reached unreachable code" from
+        // this line under it.
+        if (!recovering.swap(true, .seq_cst)) @import("vaxis").recover();
+        @import("crash.zig").record(msg);
         std.debug.defaultPanic(msg, ret_addr);
     }
 }.call);
 
+/// Whether this process has already restored its terminal — see `panic` above,
+/// and note that `debug.handleSegfault` below shares it: a SIGSEGV raised while
+/// the panic handler runs must not double-close either.
+var recovering: std.atomic.Value(bool) = .init(false);
+
 // Fatal signals (SIGSEGV/SIGILL/SIGBUS/SIGFPE) bypass the panic handler and
 // no defer/errdefer ever runs — hook std.debug's segfault path the same way
 // so the terminal is restored before the trace prints.
 pub const debug = if (is_emscripten) struct {} else struct {
     pub fn handleSegfault(addr: ?usize, name: []const u8, opt_ctx: anytype) noreturn {
-        @import("vaxis").recover();
+        if (!recovering.swap(true, .seq_cst)) @import("vaxis").recover();
         return std.debug.defaultHandleSegfault(addr, name, opt_ctx);
     }
 };