From e65c5a38897bf4e14e22e9c7b20e569fb88dc3ab Mon Sep 17 00:00:00 2001 From: rakibulism <40rakib70@gmail.com> Date: Sat, 25 Jul 2026 13:41:57 +0600 Subject: [PATCH] Close the pty write-after-respawn race and bound the pty PATH resolve stall ptySpawn now returns a versioned PtyHandle{key, generation}; ptyWriteChecked refuses a write whose generation no longer matches the current occupant of that key, closing a misdelivery hazard where a stale write could land on a respawned pty that reused the same key. ts_core_host.zig's spawn/write path now uses it. resolveExecutable's PATH walk (blocking access() calls with no timeout) now runs on a bounded worker thread via resolveExecutableBounded, capped at 300ms before the worker is abandoned rather than stalling the loop thread indefinitely on a wedged NFS/autofs mount. Also adds a real-executor test for the deferred (non-inline) host-request answer path racing cancelHostRequest, closing a coverage gap in effects_host_tests.zig. Co-Authored-By: Claude Sonnet 5 --- examples/terminal/src/main.zig | 2 +- examples/workbench/src/main.zig | 2 +- src/runtime/effects.zig | 127 ++++++++++++++++++++++++++--- src/runtime/effects_host_tests.zig | 81 ++++++++++++++++++ src/runtime/effects_pty_tests.zig | 92 ++++++++++----------- src/runtime/pty.zig | 100 ++++++++++++++++++++++- src/runtime/ts_core_host.zig | 34 ++++++-- 7 files changed, 369 insertions(+), 69 deletions(-) diff --git a/examples/terminal/src/main.zig b/examples/terminal/src/main.zig index da6843b3d..7fa7535ce 100644 --- a/examples/terminal/src/main.zig +++ b/examples/terminal/src/main.zig @@ -230,7 +230,7 @@ fn spawnShell(model: *Model, fx: *Fx) void { // session that just ended. (A no-op on the first spawn.) model.session.reset(); model.session.refreshScreenText(); - fx.ptySpawn(.{ + _ = fx.ptySpawn(.{ .key = shell_key, .argv = default_shell_argv, .cols = model.cols, diff --git a/examples/workbench/src/main.zig b/examples/workbench/src/main.zig index fe8079cbe..c8aaf8760 100644 --- a/examples/workbench/src/main.zig +++ b/examples/workbench/src/main.zig @@ -135,7 +135,7 @@ pub fn boot(model: *Model, fx: *Effects) void { model.history_index = 0; model.address_field.set(home_url); } - fx.ptySpawn(.{ + _ = fx.ptySpawn(.{ .key = shell_effect_key, .argv = default_shell_argv, .cols = 80, diff --git a/src/runtime/effects.zig b/src/runtime/effects.zig index 2b3bf76df..eaeddfe3b 100644 --- a/src/runtime/effects.zig +++ b/src/runtime/effects.zig @@ -2911,6 +2911,20 @@ pub fn Effects(comptime Msg: type) type { on_event: PtyMsgFn, }; + /// A versioned reference to one `ptySpawn` occupancy, returned + /// by `ptySpawn` and accepted by `ptyWriteChecked`. `key` alone + /// is reusable (a later spawn may claim the same key once this + /// occupancy's exit has delivered and its slot retired); + /// `generation` is stamped fresh on every spawn and is what + /// `ptyWriteChecked` actually pins a write to. Prefer this over + /// the bare-key `ptyWrite` whenever a write could otherwise + /// race a respawn under the same key (see `ptyWrite`'s + /// key-reuse-misdelivery note). + pub const PtyHandle = struct { + key: u64, + generation: u64, + }; + /// A recorded fake-pty spawn request, exposed for test /// assertions (`pendingPtyAt`). Strings borrow the slot's /// storage — valid until the slot retires. @@ -6840,10 +6854,11 @@ pub fn Effects(comptime Msg: type) type { /// per-drain Msgs (and journal records), never per-read ones. /// Under session replay nothing spawns — the journaled batches /// and exit ARE the session (`feedPtyOutput`/`feedPtyExit`). - pub fn ptySpawn(self: *Self, options: PtySpawnOptions) void { + pub fn ptySpawn(self: *Self, options: PtySpawnOptions) ?PtyHandle { self.reclaimSlots(); if (options.argv.len == 0 or options.argv.len > max_effect_argv) { - return self.rejectPty(options.key, options.on_event, true); + self.rejectPty(options.key, options.on_event, true); + return null; } var total_bytes: usize = 0; for (options.argv) |arg| { @@ -6853,26 +6868,46 @@ pub fn Effects(comptime Msg: type) type { // a cut argument (validated here so the fake executor // and replay refuse identically, before the real // transport's own guard). - if (std.mem.indexOfScalar(u8, arg, 0) != null) return self.rejectPty(options.key, options.on_event, true); + if (std.mem.indexOfScalar(u8, arg, 0) != null) { + self.rejectPty(options.key, options.on_event, true); + return null; + } + } + if (total_bytes > max_effect_argv_bytes) { + self.rejectPty(options.key, options.on_event, true); + return null; + } + if (options.cols == 0 or options.rows == 0) { + self.rejectPty(options.key, options.on_event, true); + return null; } - if (total_bytes > max_effect_argv_bytes) return self.rejectPty(options.key, options.on_event, true); - if (options.cols == 0 or options.rows == 0) return self.rejectPty(options.key, options.on_event, true); if (options.term.len == 0 or options.term.len > max_effect_pty_term_bytes) { - return self.rejectPty(options.key, options.on_event, true); + self.rejectPty(options.key, options.on_event, true); + return null; } // TERM rides the child environment; an embedded NUL would // truncate it at the C boundary, so a spawn that "succeeded" // would hand the child a different TERM than requested. - if (std.mem.indexOfScalar(u8, options.term, 0) != null) return self.rejectPty(options.key, options.on_event, true); - if (self.keyOccupiedUntilDelivery(options.key)) return self.rejectPty(options.key, options.on_event, true); - const slot_index = self.findIdlePtySlot() orelse return self.rejectPty(options.key, options.on_event, true); + if (std.mem.indexOfScalar(u8, options.term, 0) != null) { + self.rejectPty(options.key, options.on_event, true); + return null; + } + if (self.keyOccupiedUntilDelivery(options.key)) { + self.rejectPty(options.key, options.on_event, true); + return null; + } + const slot_index = self.findIdlePtySlot() orelse { + self.rejectPty(options.key, options.on_event, true); + return null; + }; // Table capacity obeys the replay-hold invariant, the // channel argument verbatim: executor-truth start failures // park a real slot under replay until the journaled // terminal feeds, so live holds the open counted through // the staged window too. if (self.idlePtySlotCount() <= self.stagedPtyReservationCount()) { - return self.rejectPty(options.key, options.on_event, true); + self.rejectPty(options.key, options.on_event, true); + return null; } // A build without a pty transport (a libc-free Linux // build; a target outside the support matrix) refuses up @@ -6887,7 +6922,10 @@ pub fn Effects(comptime Msg: type) type { // spawn), instead of the replayed spawn parking with no // journaled terminal to retire it. if (comptime !pty_transport.supported) { - if (self.executor != .fake) return self.rejectPty(options.key, options.on_event, false); + if (self.executor != .fake) { + self.rejectPty(options.key, options.on_event, false); + return null; + } } const slot = &self.pty_slots[slot_index]; @@ -6911,6 +6949,7 @@ pub fn Effects(comptime Msg: type) type { } @memcpy(slot.term_storage[0..options.term.len], options.term); slot.term_len = options.term.len; + const handle: PtyHandle = .{ .key = options.key, .generation = generation }; if (slot.fake) { // Session replay: PARK — the fake-slot discipline. The @@ -6922,9 +6961,16 @@ pub fn Effects(comptime Msg: type) type { slot.park_seq = self.nextPendingSeq(); slot.park_state = .reserved; } - return; + return handle; } self.startRealPty(slot, options); + // `startRealPty` may fail synchronously and release the slot + // (staging `.spawn_failed` instead) — the returned handle is + // still correct either way: `ptyWriteChecked` re-validates + // the generation against whatever currently occupies `key` + // at write time, so a handle for an already-released slot + // simply reads back as "unknown" rather than misdelivering. + return handle; } /// Write bytes toward the pty child's stdin, all-or-nothing. @@ -6950,6 +6996,43 @@ pub fn Effects(comptime Msg: type) type { /// exact: a verdict is journaled (and consumed) iff the key names /// a live occupancy and the payload is non-empty — both /// deterministic across the replayed dispatch stream. + // NOTE(key-reuse misdelivery): `findPtySlot` below matches purely + // on the bare `u64` key. Once a key's slot is retired + // (`retirePtySlot`, state -> `.idle`) it becomes eligible for + // reuse by a *new* `ptySpawn` under the same key, so a `ptyWrite` + // intended for the OLD occupant but issued after a NEW occupant + // has already spawned under that same key silently lands on the + // new occupant's stdin instead of being dropped. + // + // `ts_core_host.zig`'s `issuePtySpawn`/`runPtyWrite` owns both the + // spawn and the write for its keys, so it now stamps and reuses + // `PtyHandle.generation` via `ptyWriteChecked` below — closing the + // gap outright rather than just relying on its `entry.used` + // liveness gate. + // + // `ui_app.zig`'s `terminalGatewayWrite` (the built-in `` + // element's write path, via `terminal_session.zig`'s + // `TerminalSessions`) still uses this bare-key form. That module + // does not own the spawn — the embedding app calls `ptySpawn` and + // merely binds the resulting key to a `` element, and + // deliberately keeps the SAME session object across a respawn + // under the same key (`notePtyEvent`'s `.output` handler calls + // `resetForRespawn` rather than allocating a new session) — so + // adopting `ptyWriteChecked` there would mean threading a + // generation through the public `EffectPtyEvent` contract + // `session_journal.zig`/`session_replay.zig` serialize, a wire- + // format change well beyond this fix's scope. It remains safe + // under the same single-threaded-dispatch argument verified for + // `ts_core_host.zig`, via a different mechanism: `keyEvent`/ + // `textInput` (the only two paths that call `gateway.write`) both + // gate on `Session.acceptsInput()` (`!session.ended`), and + // `session.ended` is set at the same exit-delivery instant that + // frees the key for reuse, only clearing again once the new + // spawn's first output batch arrives — so no write can reach the + // gateway for a key between an old occupant's exit and a new + // occupant's first output. New callers that own both the spawn + // and the write should prefer `ptyWriteChecked` over this + // bare-key form. pub fn ptyWrite(self: *Self, key: u64, bytes: []const u8) bool { const slot = self.findPtySlot(key) orelse { // A spawn whose transport failed SYNCHRONOUSLY released @@ -6985,6 +7068,26 @@ pub fn Effects(comptime Msg: type) type { return accepted; } + /// `ptyWrite`, but pinned to the exact occupancy `handle` names + /// instead of whatever currently holds `handle.key`. Closes the + /// key-reuse-misdelivery gap `ptyWrite` documents: if `handle`'s + /// occupancy has already retired and the key was reused by a + /// newer `ptySpawn`, `handle.generation` no longer matches the + /// slot's current generation, so the write is refused exactly + /// like an unknown key rather than landing on the new occupant. + /// The mismatch case is deliberately NOT counted into + /// `dropped_writes` (that tally belongs to the CURRENT occupant, + /// which never saw this write) and is not journaled — from + /// `handle`'s perspective this occupancy is simply gone, the same + /// silent-refuse contract `ptyWrite` already gives a key with no + /// spawn at all. + pub fn ptyWriteChecked(self: *Self, handle: PtyHandle, bytes: []const u8) bool { + if (self.findPtySlot(handle.key)) |slot| { + if (slot.generation != handle.generation) return false; + } + return self.ptyWrite(handle.key, bytes); + } + /// The admission decision behind `ptyWrite` — the live half the /// journaled verdict records. fn ptyWriteAdmit(self: *Self, slot: *PtySlot, bytes: []const u8) bool { diff --git a/src/runtime/effects_host_tests.zig b/src/runtime/effects_host_tests.zig index 6431a4bf7..9b7743d87 100644 --- a/src/runtime/effects_host_tests.zig +++ b/src/runtime/effects_host_tests.zig @@ -6,6 +6,13 @@ //! pin: a session driving requests and fx timers through a full UiApp //! journals `.host` results and replays to identical state without a //! host call. +//! +//! The real-executor cases include the genuinely async path +//! `HostCallBinding`'s own doc comment calls out: `request_fn` returns +//! without answering, and `feedHostResult` arrives LATER from a separate, +//! host-marshaled call (outside `request_fn`'s own call stack) — plus +//! that path racing a `cancel_fn` notice, mirroring the fake executor's +//! "cancelHostRequest drops silently" coverage for the real binding. const std = @import("std"); const geometry = @import("geometry"); @@ -450,6 +457,80 @@ test "real-mode host calls ride the binding and answer through the feed" { try std.testing.expectEqual(@as(u32, 1), h.app_state.model.result_count); } +test "real-mode: request_fn defers the answer, which arrives later and can race a cancel" { + var h = try Harness.create(); + defer h.destroy(); + const fx = &h.app_state.effects; + + const Stub = struct { + var bound: ?*HostEffects = null; + var deferred_key: ?u64 = null; + var cancel_count: usize = 0; + var last_cancelled: u64 = 0; + fn send(context: *anyopaque, name: []const u8, payload: []const u8) void { + _ = context; + _ = name; + _ = payload; + } + // Neither branch answers inline: both requests stay in flight + // until the test itself feeds a result from outside this + // function's call stack, standing in for a host that marshals + // the answer back through its own event loop instead of + // resolving synchronously. + fn request(context: *anyopaque, name: []const u8, key: u64, payload: []const u8) void { + _ = context; + _ = name; + _ = payload; + deferred_key = key; + } + fn cancelNotice(context: *anyopaque, key: u64) void { + _ = context; + cancel_count += 1; + last_cancelled = key; + } + }; + Stub.bound = fx; + Stub.deferred_key = null; + Stub.cancel_count = 0; + var context: u8 = 0; + fx.bindHostCalls(.{ + .context = &context, + .send_fn = Stub.send, + .request_fn = Stub.request, + .cancel_fn = Stub.cancelNotice, + }); + + // The request returns with no answer yet: nothing has delivered. + test_payload = "hello"; + try h.app_state.dispatch(&h.harness.runtime, 1, .ask); + try h.wake(); + try std.testing.expectEqual(ask_key, Stub.deferred_key.?); + try std.testing.expectEqual(@as(u32, 0), h.app_state.model.result_count); + + // The host answers later, off `request_fn`'s own call stack: the + // deferred result still delivers on the next wake. + try fx.feedHostResult(ask_key, true, "deferred-answer"); + try h.wake(); + try std.testing.expectEqual(@as(u32, 1), h.app_state.model.result_count); + try std.testing.expectEqual(@as(u32, 1), h.app_state.model.ok_count); + try std.testing.expectEqualStrings("deferred-answer", h.app_state.model.bytesPrefix()); + + // A second deferred request races a cancel: the host is notified, + // and a late answer that arrives after the cancel notice reports + // `error.EffectNotFound` for the real executor too (the fake + // executor's equivalent is pinned by "cancelHostRequest drops + // silently, and the generic cancel routes to it" above). + try h.app_state.dispatch(&h.harness.runtime, 1, .ask_other); + try h.wake(); + try std.testing.expectEqual(other_key, Stub.deferred_key.?); + fx.cancelHostRequest(other_key); + try h.wake(); + try std.testing.expectEqual(@as(usize, 1), Stub.cancel_count); + try std.testing.expectEqual(other_key, Stub.last_cancelled); + try std.testing.expectError(error.EffectNotFound, fx.feedHostResult(other_key, true, "too-late")); + try std.testing.expectEqual(@as(u32, 1), h.app_state.model.result_count); +} + test "real-mode requests without bound host services reject through the err route" { var h = try Harness.create(); defer h.destroy(); diff --git a/src/runtime/effects_pty_tests.zig b/src/runtime/effects_pty_tests.zig index a92c59f67..d5bbde3bb 100644 --- a/src/runtime/effects_pty_tests.zig +++ b/src/runtime/effects_pty_tests.zig @@ -70,7 +70,7 @@ test "fake pty lifecycle: spawn parks the request, feeds deliver, exit retires t defer fx.deinit(); fx.executor = .fake; - fx.ptySpawn(.{ + _ = fx.ptySpawn(.{ .key = 9, .argv = &.{ "sh", "-l" }, .cols = 120, @@ -98,7 +98,7 @@ test "fake pty lifecycle: spawn parks the request, feeds deliver, exit retires t // The exit delivery freed the key: a fresh spawn under it is // accepted (the families' shared instant). - fx.ptySpawn(.{ .key = 9, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 9, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); try testing.expectEqual(@as(usize, 1), fx.pendingPtyCount()); } @@ -108,41 +108,41 @@ test "pty admission: every refused spawn delivers exactly one rejected exit" { fx.executor = .fake; // Empty argv. - fx.ptySpawn(.{ .key = 1, .argv = &.{}, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 1, .argv = &.{}, .on_event = DirectFx.ptyMsg(.pty) }); _ = try expectExit(&fx, 1, .rejected); // argv over the entry budget. var too_many: [effects_mod.max_effect_argv + 1][]const u8 = undefined; for (&too_many) |*arg| arg.* = "x"; - fx.ptySpawn(.{ .key = 2, .argv = &too_many, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 2, .argv = &too_many, .on_event = DirectFx.ptyMsg(.pty) }); _ = try expectExit(&fx, 2, .rejected); // argv over the byte budget. const big = "y" ** (effects_mod.max_effect_argv_bytes + 1); - fx.ptySpawn(.{ .key = 3, .argv = &.{big}, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 3, .argv = &.{big}, .on_event = DirectFx.ptyMsg(.pty) }); _ = try expectExit(&fx, 3, .rejected); // A zero dimension. - fx.ptySpawn(.{ .key = 4, .argv = &.{"sh"}, .cols = 0, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 4, .argv = &.{"sh"}, .cols = 0, .on_event = DirectFx.ptyMsg(.pty) }); _ = try expectExit(&fx, 4, .rejected); // TERM over its bound. const long_term = "t" ** (effects_mod.max_effect_pty_term_bytes + 1); - fx.ptySpawn(.{ .key = 5, .argv = &.{"sh"}, .term = long_term, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 5, .argv = &.{"sh"}, .term = long_term, .on_event = DirectFx.ptyMsg(.pty) }); _ = try expectExit(&fx, 5, .rejected); // A duplicate active key. - fx.ptySpawn(.{ .key = 6, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); - fx.ptySpawn(.{ .key = 6, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 6, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 6, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); _ = try expectExit(&fx, 6, .rejected); // Table exhaustion: the table already holds key 6; filling the // remaining slots leaves the next spawn refused. var key: u64 = 7; while (key < 7 + effects_mod.max_effect_ptys - 1) : (key += 1) { - fx.ptySpawn(.{ .key = key, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = key, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); } - fx.ptySpawn(.{ .key = 99, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 99, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); _ = try expectExit(&fx, 99, .rejected); try testing.expectEqual(@as(usize, effects_mod.max_effect_ptys), fx.pendingPtyCount()); } @@ -152,7 +152,7 @@ test "fake pty write capture, resize mirror, and kill mirror" { defer fx.deinit(); fx.executor = .fake; - fx.ptySpawn(.{ .key = 11, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 11, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); try testing.expect(fx.ptyWrite(11, "ls -la")); try testing.expect(fx.ptyWrite(11, "\r")); try testing.expectEqualStrings("ls -la\r", fx.ptyWrittenBytes(11)); @@ -195,7 +195,7 @@ test "a signaled exit carries its signal; a signaled feed with no signal is refu // that names `.signaled` with signal 0 could never come from the live // transport and would journal a record replay's damage gate refuses — // so the feed boundary refuses it loudly, before it can be recorded. - fx.ptySpawn(.{ .key = 51, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 51, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); try testing.expectError(error.ReplayDamagedRecord, fx.feedPtyExit(51, -1, 0, .signaled, 0)); // The slot is untouched by the refusal: a well-formed signaled exit // then delivers, carrying its signal and the -1 code sentinel. @@ -208,7 +208,7 @@ test "a signaled exit carries its signal; a signaled feed with no signal is refu // 0..255 (plus the -1 externally-reaped sentinel) and signals // 1..127 — a feed outside those could never come from the live // transport and is refused before it can be journaled. - fx.ptySpawn(.{ .key = 52, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 52, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); try testing.expectError(error.ReplayDamagedRecord, fx.feedPtyExit(52, 300, 0, .exited, 0)); try testing.expectError(error.ReplayDamagedRecord, fx.feedPtyExit(52, -2, 0, .exited, 0)); try testing.expectError(error.ReplayDamagedRecord, fx.feedPtyExit(52, -1, 128, .signaled, 0)); @@ -225,7 +225,7 @@ test "replay-mode ptyWrite returns the journaled verdicts, never a recomputed gu fx.armReplay(); // The replayed spawn parks; the parked occupancy holds the key. - fx.ptySpawn(.{ .key = 81, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 81, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); // The journal recorded refuse-then-accept (a full FIFO that later // drained). The replayed writes must return exactly that — the @@ -276,7 +276,7 @@ test "a write against a synchronously failed spawn journals the verdict replay's var fx = DirectFx.init(testing.allocator); defer fx.deinit(); fx.bindJournal(capture.journal()); - fx.ptySpawn(.{ .key = 91, .argv = &.{"/nonexistent-binary-for-this-test"}, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 91, .argv = &.{"/nonexistent-binary-for-this-test"}, .on_event = DirectFx.ptyMsg(.pty) }); try testing.expect(!fx.ptyWrite(91, "same dispatch")); try testing.expectEqual(@as(u32, 1), capture.write_records); try testing.expectEqual(@as(i32, 0), capture.last_code); @@ -300,7 +300,7 @@ test "a write against a synchronously failed spawn journals the verdict replay's var fx = DirectFx.init(testing.allocator); defer fx.deinit(); fx.armReplay(); - fx.ptySpawn(.{ .key = 91, .argv = &.{"/nonexistent-binary-for-this-test"}, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 91, .argv = &.{"/nonexistent-binary-for-this-test"}, .on_event = DirectFx.ptyMsg(.pty) }); try fx.pushReplayPtyWriteVerdict(91, false); try testing.expect(!fx.ptyWrite(91, "same dispatch")); try fx.feedPtyExit(91, -1, 0, .spawn_failed, 1); @@ -315,7 +315,7 @@ test "the replay verdict queue grows past its inline window and stays keyed" { var fx = DirectFx.init(testing.allocator); defer fx.deinit(); fx.armReplay(); - fx.ptySpawn(.{ .key = 84, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 84, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); // Every verdict a dispatch recorded feeds BEFORE that dispatch // replays, so one update that wrote thousands of chunks (a giant @@ -351,7 +351,7 @@ test "settle refuses replay write-count divergence in both directions" { var fx = DirectFx.init(testing.allocator); defer fx.deinit(); fx.armReplay(); - fx.ptySpawn(.{ .key = 82, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 82, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); try testing.expect(fx.ptyWrite(82, "past the recording")); try testing.expectError(error.ReplayDivergence, fx.settleReplayFeeds()); } @@ -362,8 +362,8 @@ test "settle refuses replay write-count divergence in both directions" { var fx = DirectFx.init(testing.allocator); defer fx.deinit(); fx.armReplay(); - fx.ptySpawn(.{ .key = 82, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); - fx.ptySpawn(.{ .key = 83, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 82, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 83, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); try fx.pushReplayPtyWriteVerdict(82, true); try testing.expect(fx.ptyWrite(83, "wrong session")); try testing.expectError(error.ReplayDivergence, fx.settleReplayFeeds()); @@ -392,7 +392,7 @@ test "settle refuses replay write-count divergence in both directions" { var fx = DirectFx.init(testing.allocator); defer fx.deinit(); fx.armReplay(); - fx.ptySpawn(.{ .key = 85, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 85, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); try fx.feedPtyOutput(85, "undelivered"); try testing.expectError(error.ReplayDivergence, fx.settleReplayFeeds()); // Delivered, the same feeds settle clean. @@ -428,7 +428,7 @@ test "settle refuses replay write-count divergence in both directions" { var fx = DirectFx.init(testing.allocator); defer fx.deinit(); fx.armReplay(); - fx.ptySpawn(.{ .key = 83, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 83, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); try fx.pushReplayPtyWriteVerdict(83, false); try fx.pushReplayClock(1_000); try testing.expect(!fx.ptyWrite(83, "recorded")); @@ -504,7 +504,7 @@ test "a write after the fed exit is staged refuses and counts - the live rule on defer fx.deinit(); fx.executor = .fake; - fx.ptySpawn(.{ .key = 58, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 58, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); try testing.expect(fx.ptyWrite(58, "before")); try fx.feedPtyExit(58, 0, 0, .exited, 0); // The exit is staged but undelivered: the session is already over, @@ -522,7 +522,7 @@ test "a fake pty refuses a second feed after its exit is queued" { defer fx.deinit(); fx.executor = .fake; - fx.ptySpawn(.{ .key = 71, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 71, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); try fx.feedPtyExit(71, 0, 0, .exited, 0); // One terminal per spawn: a feed before the exit drains is refused // loudly, never enqueued and silently dropped. @@ -536,7 +536,7 @@ test "a fed output batch over the chunk bound is refused, never truncated" { defer fx.deinit(); fx.executor = .fake; - fx.ptySpawn(.{ .key = 72, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 72, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); var over: [effects_mod.max_effect_pty_chunk_bytes + 1]u8 = undefined; @memset(&over, 'x'); try testing.expectError(error.PtyChunkTooLarge, fx.feedPtyOutput(72, &over)); @@ -547,7 +547,7 @@ test "a fed output batch past the inline entry bound rides a heap payload intact defer fx.deinit(); fx.executor = .fake; - fx.ptySpawn(.{ .key = 21, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 21, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); var big: [effects_mod.max_effect_line_bytes + 128]u8 = undefined; for (&big, 0..) |*byte, index| byte.* = @intCast('a' + (index % 26)); try fx.feedPtyOutput(21, &big); @@ -561,7 +561,7 @@ test "pty keys share the keyed families' space" { defer fx.deinit(); fx.executor = .fake; - fx.ptySpawn(.{ .key = 31, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 31, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); // A channel open under the pty's key is refused (and vice versa — // one key space across every keyed family). const handle = fx.openChannel(.{ .key = 31, .on_event = undefined }); @@ -594,7 +594,7 @@ test "replay never spawns: an armed channel parks the spawn and feeds deliver th defer fx.deinit(); fx.armReplay(); - fx.ptySpawn(.{ .key = 41, .argv = &.{ "sh", "-c", "echo hi" }, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 41, .argv = &.{ "sh", "-c", "echo hi" }, .on_event = DirectFx.ptyMsg(.pty) }); // Parked as a fake: no process, no io thread — the journal is the // whole world. try testing.expectEqual(@as(usize, 1), fx.pendingPtyCount()); @@ -606,13 +606,13 @@ test "replay never spawns: an armed channel parks the spawn and feeds deliver th // A fed start failure retires a park at the spawn's dispatch // position (the reserved pending-order stamp). - fx.ptySpawn(.{ .key = 42, .argv = &.{"missing-binary"}, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 42, .argv = &.{"missing-binary"}, .on_event = DirectFx.ptyMsg(.pty) }); try fx.feedPtyExit(42, effects_mod.effect_error_exit_code, 0, .spawn_failed, 0); _ = try expectExit(&fx, 42, .spawn_failed); try testing.expectEqual(@as(usize, 0), fx.pendingPtyCount()); // Nothing feeds past a terminal: one exit per spawn. - fx.ptySpawn(.{ .key = 43, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 43, .argv = &.{"sh"}, .on_event = DirectFx.ptyMsg(.pty) }); try fx.feedPtyExit(43, 0, 0, .exited, 0); try testing.expectError(error.ReplayDamagedRecord, fx.feedPtyOutput(43, "late")); } @@ -657,7 +657,7 @@ test "live pty end to end: output, coalescing, and the exit code" { var fx = DirectFx.init(testing.allocator); defer fx.deinit(); - fx.ptySpawn(.{ + _ = fx.ptySpawn(.{ .key = 51, // 200 one-byte writes; the staging ring coalesces whatever // lands between drains, so waiting for the child first proves @@ -685,7 +685,7 @@ test "live pty back-pressure is lossless past the staging ring" { // the ring fills and resumes as the drain frees room; every byte // arrives, each record within the chunk bound. const total: usize = 1024 * 1024; - fx.ptySpawn(.{ + _ = fx.ptySpawn(.{ .key = 52, .argv = &.{ "/bin/sh", "-c", "dd if=/dev/zero bs=4096 count=256 2>/dev/null | tr '\\0' 'a'" }, .on_event = DirectFx.ptyMsg(.pty), @@ -702,7 +702,7 @@ test "live pty write and kill: input reaches the child, the exit reports cancell var fx = DirectFx.init(testing.allocator); defer fx.deinit(); - fx.ptySpawn(.{ + _ = fx.ptySpawn(.{ .key = 53, .argv = &.{"/bin/cat"}, .on_event = DirectFx.ptyMsg(.pty), @@ -741,7 +741,7 @@ test "a write refused after the exit is staged still counts into dropped_writes" // A child that exits immediately and prints nothing: the io thread // stages the exit; the loop has not delivered it yet. - fx.ptySpawn(.{ + _ = fx.ptySpawn(.{ .key = 57, .argv = &.{ "/bin/sh", "-c", "exit 0" }, .on_event = DirectFx.ptyMsg(.pty), @@ -769,10 +769,10 @@ test "a fed output batch over the chunk bound and NUL-bearing term/argv are refu // TERM with an embedded NUL: refused (a truncated TERM would reach // the child as a different value than requested). - fx.ptySpawn(.{ .key = 81, .argv = &.{"sh"}, .term = "xterm\x00evil", .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 81, .argv = &.{"sh"}, .term = "xterm\x00evil", .on_event = DirectFx.ptyMsg(.pty) }); _ = try expectExit(&fx, 81, .rejected); // argv with an embedded NUL: refused (fake and real agree). - fx.ptySpawn(.{ .key = 82, .argv = &.{ "sh", "a\x00b" }, .on_event = DirectFx.ptyMsg(.pty) }); + _ = fx.ptySpawn(.{ .key = 82, .argv = &.{ "sh", "a\x00b" }, .on_event = DirectFx.ptyMsg(.pty) }); _ = try expectExit(&fx, 82, .rejected); } @@ -781,7 +781,7 @@ test "live pty resize lands as the child's window size" { var fx = DirectFx.init(testing.allocator); defer fx.deinit(); - fx.ptySpawn(.{ + _ = fx.ptySpawn(.{ .key = 54, // Report the size the kernel line discipline hands back after // the spawn declared 91x33 — proof the initial TIOCSWINSZ took. @@ -807,7 +807,7 @@ test "live conpty end to end: rendered output carries the child's text, the exit if (comptime !live_windows) return; var fx = DirectFx.init(testing.allocator); defer fx.deinit(); - fx.ptySpawn(.{ + _ = fx.ptySpawn(.{ .key = 51, .argv = &.{ "cmd.exe", "/d", "/c", "echo pty-effects-live-marker& exit 4" }, .on_event = DirectFx.ptyMsg(.pty), @@ -830,7 +830,7 @@ test "live conpty back-pressure survives output past the staging ring" { // exact byte total non-deterministic; the property under test is // that the session stays live through the parking (arrives, keeps // flowing, exits cleanly) and every record obeys the chunk bound. - fx.ptySpawn(.{ + _ = fx.ptySpawn(.{ .key = 52, .argv = &.{ "cmd.exe", @@ -851,7 +851,7 @@ test "live conpty write and kill: input reaches the child, the exit reports canc var fx = DirectFx.init(testing.allocator); defer fx.deinit(); - fx.ptySpawn(.{ + _ = fx.ptySpawn(.{ .key = 53, .argv = &.{"cmd.exe"}, .on_event = DirectFx.ptyMsg(.pty), @@ -891,7 +891,7 @@ test "live conpty initial grid size reaches the child console" { var fx = DirectFx.init(testing.allocator); defer fx.deinit(); - fx.ptySpawn(.{ + _ = fx.ptySpawn(.{ .key = 54, // `mode con` reports the console geometry the pseudoconsole // was created with; labels are localized, digits are not. @@ -924,7 +924,7 @@ else test "teardown with a live pty returns promptly and reaps the child" { if (comptime !pty_transport.supported) return; var fx = DirectFx.init(testing.allocator); - fx.ptySpawn(.{ + _ = fx.ptySpawn(.{ .key = 55, .argv = live_long_lived_argv, .on_event = DirectFx.ptyMsg(.pty), @@ -1008,7 +1008,7 @@ test "a failed bind-site snapshot publication never strands a live pty's exit" { // shape where a running pty can meet a failed publication — with a // write in flight, so the io thread's outbound machinery is live // across the abandon. - fx.ptySpawn(.{ + _ = fx.ptySpawn(.{ .key = 56, .argv = live_exec_long_lived_argv, .on_event = DirectFx.ptyMsg(.pty), @@ -1104,7 +1104,7 @@ const session_pty_key: u64 = 61; fn ptySessionUpdate(model: *PtySessionModel, msg: PtySessionMsg, fx: *PtySessionApp.Effects) void { switch (msg) { - .spawn => fx.ptySpawn(.{ + .spawn => _ = fx.ptySpawn(.{ .key = session_pty_key, .argv = &.{ "sh", "-i" }, .cols = 100, @@ -1113,7 +1113,7 @@ fn ptySessionUpdate(model: *PtySessionModel, msg: PtySessionMsg, fx: *PtySession }), // The duplicate spawn: refused loop-side on BOTH sides — the // journaled `.rejected` exit regenerates at replay. - .spawn_dup => fx.ptySpawn(.{ + .spawn_dup => _ = fx.ptySpawn(.{ .key = session_pty_key, .argv = &.{"sh"}, .on_event = PtySessionApp.Effects.ptyMsg(.event), diff --git a/src/runtime/pty.zig b/src/runtime/pty.zig index cec0b6b0d..079f125fe 100644 --- a/src/runtime/pty.zig +++ b/src/runtime/pty.zig @@ -440,7 +440,7 @@ pub fn spawn(gpa: std.mem.Allocator, options: SpawnOptions) Error!Pty { // only post-fork calls are login_tty/execve/_exit — execve needs a // resolved path (it does no PATH search). var path_buf: [std.fs.max_path_bytes]u8 = undefined; - const resolved = resolveExecutable(options.argv[0], options.env, &path_buf) orelse + const resolved = resolveExecutableBounded(gpa, options.argv[0], options.env, &path_buf) orelse return error.PtyCommandNotFound; // Build the NUL-terminated argv/envp arrays the child hands execve. @@ -775,14 +775,19 @@ fn decodeStatus(status: c_int) Exit { return .{ .code = -1, .signal = sig }; } -fn resolveExecutable(arg0: []const u8, env: ?[]const EnvVar, buf: []u8) ?[]const u8 { +/// The synchronous walk: every `executableAt` call is a blocking libc +/// `access()` with no timeout of its own. Never call this directly from +/// `spawn()` — it runs unbounded on whatever thread calls it, which is +/// exactly the hazard `resolveExecutableBounded` below exists to cap. +/// Kept as a plain, allocation-free function so it can run standalone on +/// a worker thread with nothing but its own two owned buffers. +fn resolveExecutable(arg0: []const u8, path: []const u8, buf: []u8) ?[]const u8 { if (std.mem.indexOfScalar(u8, arg0, '/') != null) { if (!executableAt(buf, arg0)) return null; if (arg0.len >= buf.len) return null; @memcpy(buf[0..arg0.len], arg0); return buf[0..arg0.len]; } - const path = lookupEnv(env, "PATH") orelse "/usr/bin:/bin:/usr/sbin:/sbin"; var scratch: [std.fs.max_path_bytes]u8 = undefined; var it = std.mem.splitScalar(u8, path, ':'); while (it.next()) |component| { @@ -795,6 +800,95 @@ fn resolveExecutable(arg0: []const u8, env: ?[]const EnvVar, buf: []u8) ?[]const return null; } +/// How long `resolveExecutableBounded` waits for the worker thread +/// before abandoning it. Generous relative to the fast common case (a +/// local filesystem resolves every PATH entry in well under a +/// millisecond) but short enough that a stalled NFS/autofs mount costs +/// the loop thread a bounded stall instead of an indefinite one — the +/// same trade `execStatus`'s 500ms exec-probe deadline already makes for +/// the exec call right after this one. +const path_resolve_deadline_ns: u64 = 300 * std.time.ns_per_ms; +const path_resolve_poll_interval_us: c_uint = 2_000; + +/// One resolution attempt's owned inputs/output, heap-allocated so it +/// can safely outlive an abandoned worker thread. Every field the worker +/// touches is either owned here or immutable static data — nothing +/// borrows from the caller's stack — because on abandonment the caller +/// returns and its stack frame is gone while the worker may still be +/// running (see `resolveExecutableBounded`'s timeout path, the same +/// bounded-abandon shape `effects.zig` uses for stuck file/pty workers). +const PathResolveJob = struct { + arg0: []u8, + path: []u8, + result_buf: [std.fs.max_path_bytes]u8 = undefined, + result_len: ?usize = null, + done: std.atomic.Value(bool) = std.atomic.Value(bool).init(false), + + fn run(job: *PathResolveJob) void { + if (resolveExecutable(job.arg0, job.path, &job.result_buf)) |resolved| { + job.result_len = resolved.len; + } + // Publish the result before the completion flag so the polling + // thread never observes `done` before `result_len`/`result_buf`. + job.done.store(true, .release); + } +}; + +/// `resolveExecutable`, bounded: runs the walk on a dedicated worker +/// thread and waits up to `path_resolve_deadline_ns` for it. Within the +/// deadline this behaves exactly like the direct call (same result, +/// just carried through `buf`); past the deadline it abandons the +/// worker — detached, its job struct intentionally leaked, since the +/// worker may still be mid-`access()` against the stalled mount and +/// could write into it at any point — and reports not-found, so a +/// wedged PATH entry costs the loop thread one bounded stall instead of +/// an unbounded one. `gpa` backs only the two small owned copies the job +/// needs (`arg0`, `path`); nothing else allocates. +fn resolveExecutableBounded(gpa: std.mem.Allocator, arg0: []const u8, env: ?[]const EnvVar, buf: []u8) ?[]const u8 { + const path = lookupEnv(env, "PATH") orelse "/usr/bin:/bin:/usr/sbin:/sbin"; + const job = gpa.create(PathResolveJob) catch return null; + const owned_arg0 = gpa.dupe(u8, arg0) catch { + gpa.destroy(job); + return null; + }; + const owned_path = gpa.dupe(u8, path) catch { + gpa.free(owned_arg0); + gpa.destroy(job); + return null; + }; + job.* = .{ .arg0 = owned_arg0, .path = owned_path }; + + const thread = std.Thread.spawn(.{}, PathResolveJob.run, .{job}) catch { + gpa.free(owned_arg0); + gpa.free(owned_path); + gpa.destroy(job); + return null; + }; + + const deadline = clock.monotonicNanoseconds() +| path_resolve_deadline_ns; + while (!job.done.load(.acquire)) { + // A missing monotonic clock (not expected on macOS/Linux) reports + // not-found rather than risk waiting forever on a job we can + // never confirm finished — the same unresolved-favors-safety + // call `execStatus` makes for its own deadline clock read. + const now = clock.monotonicNanoseconds(); + if (now == 0 or now >= deadline) { + thread.detach(); + return null; + } + _ = c.usleep(path_resolve_poll_interval_us); + } + thread.join(); + defer { + gpa.free(owned_arg0); + gpa.free(owned_path); + gpa.destroy(job); + } + const len = job.result_len orelse return null; + @memcpy(buf[0..len], job.result_buf[0..len]); + return buf[0..len]; +} + /// X_OK access check through libc; `scratch` holds the NUL-terminated copy. fn executableAt(scratch: []u8, path: []const u8) bool { if (path.len + 1 > scratch.len) return false; diff --git a/src/runtime/ts_core_host.zig b/src/runtime/ts_core_host.zig index 33562b992..87e145743 100644 --- a/src/runtime/ts_core_host.zig +++ b/src/runtime/ts_core_host.zig @@ -599,6 +599,13 @@ pub fn TsCoreHost(comptime core: type) type { key_len: usize = 0, key: [max_wire_key_bytes]u8 = undefined, event_tag: u8 = 0, + /// The engine occupancy this entry currently names, stamped + /// from `ptySpawn`'s returned handle. Carried into every + /// `ptyWriteChecked` call so a write queued against a table + /// index the bridge is about to reuse for a fresh spawn + /// cannot land on the new occupant (see `effects.zig`'s + /// `PtyHandle`/`ptyWriteChecked`). + generation: u64 = 0, fn wireKey(entry: *const PtyEntry) []const u8 { return entry.key[0..entry.key_len]; @@ -1197,8 +1204,17 @@ pub fn TsCoreHost(comptime core: type) type { const bytes = takeLongBytes(cmd, &at); // TS `Cmd.ptyWrite` is fire-and-forget: a refusal // counts into the exit's dropped_writes, so the - // acceptance result is ignored here. - if (findPty(key)) |index| _ = fx.ptyWrite(pty_key_base + index, bytes); + // acceptance result is ignored here. Checked + // against the entry's stamped generation so a + // write still in this dispatch's command buffer + // cannot land on a fresh spawn that reused this + // table index (see `PtyEntry.generation`). + if (findPty(key)) |index| { + _ = fx.ptyWriteChecked(.{ + .key = pty_key_base + index, + .generation = ptys[index].generation, + }, bytes); + } }, // pty_resize [op][key_len][key][cols f64 LE][rows f64 LE] 0x1B => { @@ -1847,7 +1863,13 @@ pub fn TsCoreHost(comptime core: type) type { entry.key_len = key.len; @memcpy(entry.key[0..key.len], key); entry.event_tag = event_tag; - if (term.len == 0) { + // The returned handle's generation is stamped onto the entry + // so `pty_write` command handling can address this exact + // occupancy (`ptyWriteChecked`) rather than the bare, reusable + // table index. A `null` return (synchronous rejection) leaves + // the entry at its zero default: the engine never assigned a + // slot, so no write can find one under this key either way. + const handle = if (term.len == 0) // Wire "" = "the engine's default TERM" — the record // never bakes the default in (the fetch-timeout rule). fx.ptySpawn(.{ @@ -1856,8 +1878,8 @@ pub fn TsCoreHost(comptime core: type) type { .cols = ptyDimension(cols), .rows = ptyDimension(rows), .on_event = ptyEventMsg, - }); - } else { + }) + else fx.ptySpawn(.{ .key = pty_key_base + index, .argv = argv, @@ -1866,7 +1888,7 @@ pub fn TsCoreHost(comptime core: type) type { .term = term, .on_event = ptyEventMsg, }); - } + if (handle) |h| entry.generation = h.generation; } /// The wire carries the app's f64; the transport's grid is u16.