ReviewOS

also looking at this

craft-native/craft

feat(log): --log-file, and a logger that survives being used

#78
Merged chris wants to merge feat/log-file into main
5 files +691 -37
11 days ago

Closes #70.

src/log.zig was written complete — levels, JSON, a filter, a mutex, file output — and then never configured by anything. The issue asks to put a flag in front of it.

Doing only that would have produced an empty file. The module has two callers (devmode.zig, hotreload.zig); craft's actual diagnostics go through std.log and logging.zig, neither of which it has ever seen.

What lands in the file, and how

sourcemechanismin the file?
std.log.* (~200 sites, 50 files)pub const std_options → craftLogFn in the exe rootyes — across module boundaries
logging.zig scoped loggersits existing .target = .callback hookyes — zero call sites touched
log.zig direct callersalready directyes
std.debug.printnone — goes through std.Options.debug_iono, and --help says so

The log_level = .debug already declared in minimal.zig turns out to be load-bearing: std.log.debug is compiled out at comptime below the threshold, and most of craft's logging is debug level. Without it --log-level debug would be a flag that could not do anything.

Wiring it up is what made its defects reachable

Seven, all on the --log-file path, none previously reachable because nothing configured the module:

  1. Messages over 2048 bytes were dropped whole, not truncated — bufPrint(...) catch return. The longest records are usually the ones worth reading, and nothing said anything was missing.
  2. The file was written from offset zero every run. CreateFileOptions has no append flag in this Zig and writeStreamingAll starts at position 0, so each launch overwrote the previous one and left anything longer stranded past the end. Now seeded from stat().size and advanced with writePositionalAll.
  3. init kept the caller's slices for output_file and filter_pattern, and minimal.zig frees the command-line strings during startup — a use-after-free that had simply never been reachable.
  4. A reentrancy hang. compat_mutex falls back to a non-recursive spinlock that never yields (std.Thread.Mutex doesn't exist in this Zig), and log holds it across the write. A std.log call from inside log would spin against a lock the same thread holds, forever — no panic, no stack. Inert while nothing logged through it; live the moment 200 call sites do.
  5. The timestamp had no date. --log-file exists to leave a trail read afterwards; a time alone can't say which day. Now ISO-8601 UTC — and the comment claiming localtime_r (which isn't in std.c here, and whose result the code discarded via a dead _ = t;) no longer claims it.
  6. Bytes over 0x7f went into JSON verbatim, so one bad byte from a filename or truncated frame made the line unparseable.
  7. std.c.clock_gettime doesn't compile for Windows here. The file only ever built because nothing instantiated log().

That's the third time this session that wiring something up is what exposed it. It is the argument for the flag and the fixes landing together rather than the flag landing alone.

Tests

The module did have tests — test/log_test.zig — which reach it across a module boundary and therefore cannot touch the config storage or the formatter, the three places it was wrong. Twelve more now run against the source directly, covering truncation, append across runs, string ownership, level filtering, JSON under hostile input, invalid UTF-8, stderr suppression, re-init, the date, and the reentrancy guard.

Verified end to end, not just compiled

$ craft --html-file p.html --headless --log-file f.log
$ cat f.log
[2026-08-27T12:02:08Z] WARN refused to open a new window for a URL the page
cannot send to the browser: craft-probe://payload

That record originates from a std.log.warn in macos.zig — a different module from the root — which is what proves the cross-module routing works. Also confirmed by running the binary:

behaviourresult
two runs append2 records (the old code left 1)
--log-jsonparses as JSON
--log-quietin file: 1, on stderr: 0
--log-level errorthe warn is filtered out

An earlier probe produced an empty file and I nearly reported success on "the flag works" — the file existed and the build was green. It was empty because the handler override hadn't actually landed. Forcing a known std.log.warn and finding it still absent is what caught it.

Verified

zig build · zig build test · zig build test-js · x86_64-windows cross-build · zig fmt --check · tsc --noEmit · 511 bun tests · pickier 38 warnings, unchanged from main.

5 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.