fix: address various stability and reliability issues

- Cache stderr TTY check in Config to optimize log hot path.
- Cap signal handler attempts during no-hard-exit.
- Always duplicate URL segments to prevent invalid frees.
- Write live file before backup in atomic writes.
- Settle microtasks before verifying DOM elements.
This commit is contained in:
Adrià Arrufat
2026-05-25 18:05:11 +02:00
parent 4bc5baf7ee
commit fbec560292
6 changed files with 69 additions and 19 deletions

View File

@@ -382,10 +382,23 @@ pub fn logLevel(self: *const Config) ?log.Level {
/// --task on a TTY default to .low.
pub fn agentVerbosity(opts: Agent) AgentVerbosity {
if (opts.verbosity) |v| return v;
const piped_one_shot = opts.task != null and !std.posix.isatty(std.posix.STDERR_FILENO);
const piped_one_shot = opts.task != null and !stderrIsTty();
return if (piped_one_shot) .high else .low;
}
/// `isatty(STDERR)` is a syscall and `agentVerbosity` is on the log hot
/// path (every gate check resolves through it). Cache once — the fd
/// doesn't change after process start.
var stderr_tty_cached: bool = undefined;
var stderr_tty_once = std.once(initStderrTty);
fn initStderrTty() void {
stderr_tty_cached = std.posix.isatty(std.posix.STDERR_FILENO);
}
fn stderrIsTty() bool {
stderr_tty_once.call();
return stderr_tty_cached;
}
pub fn logFormat(self: *const Config) ?log.Format {
return switch (self.mode) {
inline .serve, .fetch, .mcp, .agent => |opts| opts.log_format,

View File

@@ -136,7 +136,10 @@ fn sighandle(self: *SigHandler) noreturn {
self.mutex.unlock();
std.process.exit(1);
}
self.attempt += 1;
// While `no_hard_exit` is set, cap the counter so a later
// flip back to false (e.g. agent-suspend/resume) doesn't
// immediately hard-exit on the next signal.
if (self.no_hard_exit) self.attempt = 1 else self.attempt += 1;
log.info(.app, "Received termination signal...", .{});
for (self.listeners.items) |*item| {

View File

@@ -401,7 +401,9 @@ pub fn processQueuedNavigation(self: *Session) !void {
// First pass: process async navigations (non-about:blank)
for (navigations.items) |frame| {
const qn = frame._queued_navigation orelse {
log.debug(.frame, "skipped null queued nav", .{});
// Was previously an assert; downgraded so prod can recover, but
// kept at warn so the invariant violation isn't silently lost.
log.warn(.frame, "skipped null queued nav", .{});
continue;
};
@@ -425,7 +427,9 @@ pub fn processQueuedNavigation(self: *Session) !void {
// siblings whose _queued_navigation stays set).
for (about_blank_queue.items) |frame| {
const qn = frame._queued_navigation orelse {
log.debug(.frame, "skipped null queued nav", .{});
// Was previously an assert; downgraded so prod can recover, but
// kept at warn so the invariant violation isn't silently lost.
log.warn(.frame, "skipped null queued nav", .{});
continue;
};
self.processFrameNavigation(frame, qn) catch |err| {

View File

@@ -293,7 +293,9 @@ pub fn percentEncodeSegment(allocator: Allocator, segment: []const u8, comptime
}
}
if (!needs_encoding) {
return segment;
// Always dupe — the signature returns owned bytes, so a caller doing
// `defer allocator.free(out)` mustn't crash on the no-op path.
return allocator.dupe(u8, segment);
}
var buf = try std.ArrayList(u8).initCapacity(allocator, segment.len + 10);

View File

@@ -180,15 +180,19 @@ pub fn writeAtomic(
if (std.mem.eql(u8, new_content, content)) return;
var bak_buf: [std.fs.max_path_bytes]u8 = undefined;
const bak_path = try std.fmt.bufPrint(&bak_buf, "{s}.bak", .{path});
try dir.writeFile(.{ .sub_path = bak_path, .data = content });
// Rewrite the live file first; only refresh `.bak` once the new content
// is committed. Reversed order left a stale `.bak == live` snapshot on
// any atomic-rewrite failure, which a later successful run would then
// overwrite — wiping the only record of the pre-heal state.
var write_buf: [4096]u8 = undefined;
var af = try dir.atomicFile(path, .{ .write_buffer = &write_buf });
defer af.deinit();
try af.file_writer.interface.writeAll(new_content);
try af.finish();
var bak_buf: [std.fs.max_path_bytes]u8 = undefined;
const bak_path = try std.fmt.bufPrint(&bak_buf, "{s}.bak", .{path});
try dir.writeFile(.{ .sub_path = bak_path, .data = content });
}
/// Replacement body: either parsed Commands (agent self-heal) or pre-rendered
@@ -423,15 +427,21 @@ test "writeAtomic: writes content and creates .bak" {
try std.testing.expectEqualStrings("/goto 'https://x'\n/click selector='old'\n", buf[0..m]);
}
test "writeAtomic: leaves original untouched when .bak write fails" {
test "writeAtomic: commits rewrite even when .bak write fails" {
// The live rewrite is committed before `.bak` is refreshed — a `.bak`
// failure surfaces as an error but the heal itself is already in place.
// The previous order (.bak first) left useless `.bak == live` snapshots
// on failure, which a later successful run could overwrite with stale
// pre-heal state.
var tmp = std.testing.tmpDir(.{});
defer tmp.cleanup();
const original = "/click selector='old'\n";
const updated = "/click selector='new'\n";
try tmp.dir.writeFile(.{ .sub_path = "script.lp", .data = original });
const replacements = [_]Replacement{
.{ .original_span = original[0..], .new_text = "/click selector='new'\n" },
.{ .original_span = original[0..], .new_text = updated },
};
// Force the .bak write to fail by putting a directory at the .bak path.
@@ -445,7 +455,7 @@ test "writeAtomic: leaves original untouched when .bak write fails" {
const live = tmp.dir.openFile("script.lp", .{}) catch unreachable;
defer live.close();
const n = live.readAll(&buf) catch unreachable;
try std.testing.expectEqualStrings(original, buf[0..n]);
try std.testing.expectEqualStrings(updated, buf[0..n]);
}
test "isPathSafe: relative paths without traversal are accepted" {

View File

@@ -93,7 +93,11 @@ fn verifyFill(self: *Verifier, arena: std.mem.Allocator, selector: []const u8, e
// Secret env-var references can't be compared literally — just
// verify the field isn't empty after substitution.
if (std.mem.indexOf(u8, expected_value, "$LP_") != null) {
const actual = self.queryElementProperty(arena, selector, .value) orelse return .inconclusive;
var actual = self.queryElementProperty(arena, selector, .value) orelse return .inconclusive;
if (actual.len == 0) {
self.settle();
actual = self.queryElementProperty(arena, selector, .value) orelse return .inconclusive;
}
if (actual.len == 0)
return .{ .failed = "element value is empty after fill (expected non-empty for secret)" };
return .passed;
@@ -117,12 +121,26 @@ const Check = struct {
};
fn verifyElementValue(self: *Verifier, arena: std.mem.Allocator, selector: []const u8, check: Check) VerifyResult {
const actual = self.queryElementProperty(arena, selector, check.property) orelse return .inconclusive;
if (!std.mem.eql(u8, actual, check.expected)) {
const msg = std.fmt.allocPrint(arena, "element {s} is \"{s}\" (expected \"{s}\")", .{ check.label, actual, check.expected }) catch failed_reason_oom;
return .{ .failed = msg };
}
return .passed;
var actual = self.queryElementProperty(arena, selector, check.property) orelse return .inconclusive;
if (std.mem.eql(u8, actual, check.expected)) return .passed;
// Frameworks (React, Vue) reflect state changes through a microtask /
// re-render. Reading inside the same tick can miss the update — drain
// one runner tick and try again before declaring failure.
self.settle();
actual = self.queryElementProperty(arena, selector, check.property) orelse return .inconclusive;
if (std.mem.eql(u8, actual, check.expected)) return .passed;
const msg = std.fmt.allocPrint(arena, "element {s} is \"{s}\" (expected \"{s}\")", .{ check.label, actual, check.expected }) catch failed_reason_oom;
return .{ .failed = msg };
}
/// Drain pending microtasks / macrotasks so a same-tick re-render
/// reflects in DOM state before the next query. Best-effort; failures
/// to acquire the runner fall through silently.
fn settle(self: *Verifier) void {
var runner = self.session.runner(.{}) catch return;
runner.wait(.{ .ms = 50, .until = .done }) catch {};
}
/// Returns the property value, or `null` when the element is missing or the