From df50f69e8421e88061df15c27276a5e5eb648c59 Mon Sep 17 00:00:00 2001 From: Rach Pradhan <54503978+justrach@users.noreply.github.com> Date: Thu, 6 Aug 2026 13:57:13 +0800 Subject: [PATCH] feat(locks): key owner liveness on pid + process start identity (#413) A bare pid is not an owner. Pids are a small recycled namespace, so a crashed holder's number is handed to something unrelated and a lock keyed on the pid alone reads as held forever - or, keyed on a timeout instead, gets stolen from a holder that is very much alive. proc_identity.zig reads the START identity of a pid: /proc//stat field 22 on Linux (counted from the LAST ')', since comm may contain both spaces and parentheses), proc_pidinfo(PROC_PIDTBSDINFO) on macOS - a libSystem call rather than the `ps -o lstart=` subprocess the issue suggested, so there is no fork in a lock path, no locale-dependent date parsing and microsecond rather than one-second resolution - GetProcessTimes on Windows, and pid-only liveness anywhere else. Liveness is now "the pid is alive AND it is still the same process", and only a provable mismatch makes a lock reclaimable. Two rules keep the upgrade safe: a record with no start identity (an older graff's, or an identity-less platform's) keeps the pre-#413 pid-only contract, so an in-flight lock is never bricked; and a probe that FAILS is `.unknown`, which means held - wrongly reclaiming a live lock corrupts, wrongly honouring a dead one only waits. Both lock modules take it up. worktree_lease.Owner.start_ns becomes start_id and gains the producer it never had (selfOwner/probeOwners), with a new live_unverified verdict for a pid we cannot identify. The #289 degraded session write - a filesystem whose advisory locks do not work - stops racing unguarded and brackets itself with an owner record, via the reusable claimOwnerFile/releaseOwnerFile the credential store and any future daemon lease can share. Co-Authored-By: Codegraff --- CHANGELOG.md | 17 +- src/proc_identity.zig | 460 +++++++++++++++++++++++++++++++++++++++++ src/session_lock.zig | 94 +++++++++ src/test_hooks.zig | 7 + src/worktree_lease.zig | 135 +++++++++--- 5 files changed, 681 insertions(+), 32 deletions(-) create mode 100644 src/proc_identity.zig diff --git a/CHANGELOG.md b/CHANGELOG.md index b2f8415b..c23dfdd7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,22 @@ The release workflow uses a tag's section here as its release notes (a hand-written `docs/releases/.md` wins if present), so keeping this file current is part of cutting a release. -## v0.0.240 (unreleased) +## v0.0.241 (unreleased) + +- Cross-process locks stopped trusting a bare pid (#413): an owner record now + carries the holder's process START identity next to its pid (`/proc//stat` + field 22 on Linux, `proc_pidinfo` on macOS, `GetProcessTimes` on Windows), so + liveness is "the pid is alive AND it is still the same process". A recycled + pid can no longer look like a live owner forever, and a crashed holder's lock + is reclaimed because its identity provably mismatches rather than because a + timeout guessed. A record written by an older graff carries no identity and + keeps the pid-only contract exactly as before, so an in-flight lock is never + bricked, and an identity that cannot be read fails safe: held, never stolen. + The #320 worktree lease gets the producer it was missing, and a session save + on a filesystem with no working locks (#289) now coordinates through an owner + record instead of racing unguarded. + +## v0.0.240 (2026-08-06) - The REPL/engine separation began (#422): agent output now flows through a typed event vocabulary and a strict sink boundary (`engine_events.zig` / diff --git a/src/proc_identity.zig b/src/proc_identity.zig new file mode 100644 index 00000000..3d5dfb24 --- /dev/null +++ b/src/proc_identity.zig @@ -0,0 +1,460 @@ +//! Process START identity: the half of a lock owner record that a recycled pid +//! cannot forge (#413). +//! +//! A bare pid is not an owner. Pids are a small recycled namespace, so after a +//! crash the OS hands the dead holder's number to something unrelated, and a +//! lock keyed on the pid alone then reads as "still held" forever — or, if the +//! lock is time-based instead, gets stolen from a holder that is very much +//! alive. Recording the process's START time alongside the pid removes both: +//! the pair is unique for as long as the machine is up, so liveness becomes +//! "the pid is alive AND it is still the same process", and a provable +//! mismatch is what makes a stale lock reclaimable. +//! +//! Where the number comes from, one source per platform: +//! +//! * Linux `/proc//stat` field 22 (`starttime`, clock ticks since +//! boot). Field 2 is `(comm)` and comm may contain BOTH spaces and +//! ')' — `(my )weird( proc)` is a legal name — so the fields are +//! counted from the LAST ')' in the line, never by splitting it. +//! * macOS `proc_pidinfo(PROC_PIDTBSDINFO)` -> `pbi_start_tvsec/tvusec`, +//! the same instant `ps -o lstart= -p ` prints. libproc is a +//! plain libSystem call, so it beats the `ps` subprocess: no fork +//! in a lock path, no locale-dependent date parsing, and +//! microsecond instead of one-second resolution (two graffs +//! started in the same second must not share an identity). +//! * Windows `GetProcessTimes` -> `ftCreationTime` (100 ns ticks). +//! * anything else: no identity is available, so `probe` answers with +//! pid-only liveness and `selfStartId` returns 0. Records stamped there +//! carry `start_id = 0` and are read under the legacy rule below, which is +//! exactly the pre-#413 behaviour. Nothing degrades further than that. +//! +//! Two rules make the upgrade safe, and both live in `ownerState`: +//! +//! * A record with `start_id == 0` — written by a graff older than #413, or +//! on a platform with no identity source — keeps the old pid-only +//! contract. An in-flight lock from an older binary is never bricked, and +//! never stolen just because the new binary cannot verify it. +//! * A probe that FAILS (permissions, no /proc, an unexpected errno) is +//! `.unknown`, and unknown means held. Wrongly reclaiming a live lock +//! corrupts; wrongly honouring a dead one only waits. +//! +//! `claimOwnerFile`/`releaseOwnerFile` package the whole protocol for locks +//! that cannot lean on `flock` — session_lock.zig's degraded path today, the +//! credential store and any daemon lease next — so a future lock inherits the +//! identity rules instead of reinventing a timeout. + +const std = @import("std"); +const builtin = @import("builtin"); +const Io = std.Io; + +/// Opaque per-process value that changes every time the OS starts a process. +/// The units differ per platform, so it is only ever compared against another +/// reading of the SAME pid on the SAME boot — never across machines, never +/// interpreted as a wall clock. 0 means "no identity recorded". +pub const StartId = u64; + +/// What the OS says about one pid right now. +pub const Probe = union(enum) { + /// The pid is alive and this is the start identity of what holds it. + id: StartId, + /// Nothing holds the pid: the recorded owner is provably gone. + gone, + /// The pid could not be resolved to an identity — it may be alive and + /// simply opaque to us (another user's process), or the identity source + /// may be missing. Callers must treat this as live. + unknown, +}; + +pub const OwnerState = enum { + /// Someone still owns this record. Wait, do not take the lock. + held, + /// The recorded owner is gone or the pid now belongs to a different + /// process. The lock is stale and may be taken. + reclaimable, +}; + +/// The identity half of an owner record: what every graff lock writes down so +/// the next process can decide whether the holder is real. +pub const Record = struct { + pid: i32 = 0, + start_id: StartId = 0, +}; + +/// The whole #413 decision, pure so it can be tested without a process to kill. +pub fn ownerState(rec_start_id: StartId, live: Probe) OwnerState { + return switch (live) { + .gone => .reclaimable, + // Fail SAFE: an unreadable identity is never evidence of death. + .unknown => .held, + // rec_start_id == 0 is a legacy (or identity-less platform) record: + // pid alive == held, exactly as before #413. + .id => |v| if (rec_start_id == 0 or v == rec_start_id) .held else .reclaimable, + }; +} + +pub fn selfPid() i32 { + // Mirrors trace.currentPid deliberately: this module is a leaf with no + // graff imports so that any lock can use it without pulling in telemetry. + return if (builtin.os.tag == .windows) + @intCast(std.os.windows.GetCurrentProcessId()) + else + @intCast(std.posix.system.getpid()); +} + +/// This process's start identity, or 0 where the platform has no source. +pub fn selfStartId(io: Io) StartId { + return switch (probe(io, selfPid())) { + .id => |v| v, + else => 0, + }; +} + +/// The owner record to write when taking a lock. +pub fn selfRecord(io: Io) Record { + return .{ .pid = selfPid(), .start_id = selfStartId(io) }; +} + +/// `ownerState` for a record read off disk, probing the OS for the answer. +pub fn stateOf(io: Io, rec: Record) OwnerState { + return ownerState(rec.start_id, probe(io, rec.pid)); +} + +pub fn probe(io: Io, pid: i32) Probe { + if (pid <= 0) return .gone; + return switch (builtin.os.tag) { + .linux => probeLinux(io, pid), + .macos, .ios, .tvos, .watchos, .visionos, .driverkit, .maccatalyst => probeDarwin(io, pid), + .windows => probeWindows(io, pid), + else => probePidOnly(io, pid), + }; +} + +/// Liveness with no identity: alive answers `.unknown` (held, never stolen), +/// only a definitively free pid answers `.gone`. This is the pre-#413 contract +/// and the floor every platform degrades to. +fn probePidOnly(io: Io, pid: i32) Probe { + _ = io; + if (builtin.os.tag == .windows) return .unknown; + std.posix.kill(@intCast(pid), @enumFromInt(0)) catch |err| switch (err) { + error.ProcessNotFound => return .gone, + // PermissionDenied means alive and owned by someone else. + else => return .unknown, + }; + return .unknown; +} + +fn probeLinux(io: Io, pid: i32) Probe { + var path_buf: [64]u8 = undefined; + const path = std.fmt.bufPrint(&path_buf, "/proc/{d}/stat", .{pid}) catch return .unknown; + // procfs reports size 0, so this reads to EOF rather than to a stat size. + var buf: [4096]u8 = undefined; + const text = Io.Dir.cwd().readFile(io, path, &buf) catch |err| switch (err) { + error.FileNotFound => return .gone, + else => return .unknown, + }; + return if (parseLinuxStat(text)) |v| .{ .id = v } else .unknown; +} + +/// Field 22 of a `/proc//stat` line. Field 2 is `(comm)`, which may hold +/// spaces and ')' alike, so the only safe anchor is the LAST ')': everything +/// after it is space separated and positional, starting at field 3. +pub fn parseLinuxStat(text: []const u8) ?StartId { + const close = std.mem.lastIndexOfScalar(u8, text, ')') orelse return null; + var it = std.mem.tokenizeAny(u8, text[close + 1 ..], " \t\r\n"); + var n: usize = 0; + while (it.next()) |tok| { + n += 1; // token 1 is field 3 (state), so field 22 is token 20. + if (n == 20) return std.fmt.parseInt(StartId, tok, 10) catch null; + } + return null; +} + +const darwin = struct { + const PROC_PIDTBSDINFO: c_int = 3; + + /// `struct proc_bsdinfo` from . Only the tail is read, + /// but the whole layout has to be spelled out for the offsets to land; the + /// comptime assert below is what keeps that honest. + const ProcBsdInfo = extern struct { + flags: u32, + status: u32, + xstatus: u32, + pid: u32, + ppid: u32, + uid: u32, + gid: u32, + ruid: u32, + rgid: u32, + svuid: u32, + svgid: u32, + rfu_1: u32, + comm: [16]u8, + name: [32]u8, + nfiles: u32, + pgid: u32, + pjobc: u32, + e_tdev: u32, + e_tpgid: u32, + nice: i32, + start_tvsec: u64, + start_tvusec: u64, + }; + + extern "c" fn proc_pidinfo(pid: c_int, flavor: c_int, arg: u64, buffer: ?*anyopaque, buffersize: c_int) c_int; +}; + +fn probeDarwin(io: Io, pid: i32) Probe { + comptime std.debug.assert(@sizeOf(darwin.ProcBsdInfo) == 136); + comptime std.debug.assert(@offsetOf(darwin.ProcBsdInfo, "start_tvsec") == 120); + var info: darwin.ProcBsdInfo = undefined; + const size: c_int = @sizeOf(darwin.ProcBsdInfo); + const n = darwin.proc_pidinfo(@intCast(pid), darwin.PROC_PIDTBSDINFO, 0, &info, size); + // A short answer means ESRCH (dead) or EPERM (alive, another user's). + // kill(pid, 0) tells those apart without reading errno by hand. + if (n != size) return probePidOnly(io, pid); + const usec = info.start_tvsec *% std.time.us_per_s +% info.start_tvusec; + return if (usec == 0) .unknown else .{ .id = usec }; +} + +const win = struct { + const w = std.os.windows; + const PROCESS_QUERY_LIMITED_INFORMATION: w.DWORD = 0x1000; + + extern "kernel32" fn OpenProcess(dwDesiredAccess: w.DWORD, bInheritHandle: w.BOOL, dwProcessId: w.DWORD) callconv(.winapi) ?w.HANDLE; + extern "kernel32" fn GetProcessTimes( + hProcess: w.HANDLE, + lpCreationTime: *w.FILETIME, + lpExitTime: *w.FILETIME, + lpKernelTime: *w.FILETIME, + lpUserTime: *w.FILETIME, + ) callconv(.winapi) w.BOOL; +}; + +fn probeWindows(io: Io, pid: i32) Probe { + _ = io; + const w = std.os.windows; + // QUERY_LIMITED_INFORMATION is the least privilege that still answers + // GetProcessTimes, and it works across integrity levels. + const handle = win.OpenProcess(win.PROCESS_QUERY_LIMITED_INFORMATION, .FALSE, @intCast(pid)) orelse { + // INVALID_PARAMETER is Windows for "no process has that id"; a denial + // means it exists and is simply out of reach. + return switch (w.GetLastError()) { + .INVALID_PARAMETER => .gone, + else => .unknown, + }; + }; + defer w.CloseHandle(handle); + var created: w.FILETIME = undefined; + var exited: w.FILETIME = undefined; + var kernel: w.FILETIME = undefined; + var user: w.FILETIME = undefined; + if (!win.GetProcessTimes(handle, &created, &exited, &kernel, &user).toBool()) return .unknown; + const ticks = (@as(u64, created.dwHighDateTime) << 32) | @as(u64, created.dwLowDateTime); + return if (ticks == 0) .unknown else .{ .id = ticks }; +} + +/// One line, ASCII, no allocator: a lock file must be writable from a path +/// that is already failing. `formatRecord` needs at least `record_max` bytes. +pub const record_max = 80; + +pub fn formatRecord(buf: *[record_max]u8, rec: Record) []const u8 { + return std.fmt.bufPrint(buf, "graff-owner 1 pid={d} start={d}\n", .{ rec.pid, rec.start_id }) catch unreachable; +} + +/// Tolerant on purpose. `start=` missing (a pre-#413 writer) yields +/// `start_id = 0`, which `ownerState` honours under the legacy rule, and a +/// bare number is read as the classic one-line pidfile. Anything else is null: +/// an unparsable record is no record, never a spurious owner. +pub fn parseRecord(text: []const u8) ?Record { + const line = std.mem.trim(u8, text[0 .. std.mem.indexOfScalar(u8, text, '\n') orelse text.len], " \t\r"); + if (line.len == 0) return null; + if (!std.mem.startsWith(u8, line, "graff-owner")) { + // The classic one-line pidfile: a number and nothing else. + const pid = std.fmt.parseInt(i32, line, 10) catch return null; + return if (pid > 0) .{ .pid = pid } else null; + } + var rec: Record = .{}; + var have_pid = false; + var it = std.mem.tokenizeAny(u8, line, " \t"); + while (it.next()) |tok| { + if (std.mem.startsWith(u8, tok, "pid=")) { + rec.pid = std.fmt.parseInt(i32, tok[4..], 10) catch return null; + have_pid = true; + } else if (std.mem.startsWith(u8, tok, "start=")) { + rec.start_id = std.fmt.parseInt(StartId, tok[6..], 10) catch return null; + } + } + if (!have_pid or rec.pid <= 0) return null; + return rec; +} + +/// A record is worth waiting for only when it names someone ELSE and is still +/// held. Our own record is never contention. +pub fn heldByOther(rec: Record, my_pid: i32, state: OwnerState) bool { + return rec.pid != my_pid and state == .held; +} + +pub fn readOwnerFile(io: Io, dir: Io.Dir, path: []const u8) ?Record { + var buf: [record_max]u8 = undefined; + const text = dir.readFile(io, path, &buf) catch return null; + return parseRecord(text); +} + +/// Take an owner file, or report that a live owner still has it. This is the +/// whole cross-process lock protocol for a lock that cannot use `flock` — +/// a filesystem with no working locks, or a lock that must outlive an open +/// file descriptor — and it is what makes the identity worth recording: a +/// crashed owner's record is RECLAIMED because its identity provably no longer +/// matches, with no timeout to tune and no live owner ever robbed. +/// +/// Failing to read or write the file itself is deliberately not an error. An +/// owner file is a best-effort stand-in for a lock, and it must never become +/// the reason the operation it guards cannot happen at all. +pub fn claimOwnerFile(io: Io, dir: Io.Dir, path: []const u8) error{LockHeld}!void { + if (readOwnerFile(io, dir, path)) |rec| { + if (heldByOther(rec, selfPid(), stateOf(io, rec))) return error.LockHeld; + } + var stamp: [record_max]u8 = undefined; + dir.writeFile(io, .{ .sub_path = path, .data = formatRecord(&stamp, selfRecord(io)) }) catch {}; +} + +pub fn releaseOwnerFile(io: Io, dir: Io.Dir, path: []const u8) void { + dir.deleteFile(io, path) catch {}; +} + +test "ownerState: the same pid holds only while it is the same process (#413)" { + // Live and unchanged: the lock is genuinely held. + try std.testing.expectEqual(OwnerState.held, ownerState(4242, .{ .id = 4242 })); + // Same pid, different process: a recycled pid may never keep the lock. + try std.testing.expectEqual(OwnerState.reclaimable, ownerState(4242, .{ .id = 4243 })); + // Nothing holds the pid at all. + try std.testing.expectEqual(OwnerState.reclaimable, ownerState(4242, .gone)); +} + +test "ownerState: a record with no start identity keeps the pre-#413 contract" { + // An older graff's in-flight lock: pid alive means held, and it is never + // stolen merely because the new binary cannot verify it. + try std.testing.expectEqual(OwnerState.held, ownerState(0, .{ .id = 999 })); + try std.testing.expectEqual(OwnerState.held, ownerState(0, .unknown)); + // Only a provably free pid retires it — the old rule, unchanged. + try std.testing.expectEqual(OwnerState.reclaimable, ownerState(0, .gone)); +} + +test "ownerState: a failed identity read fails safe and never steals a lock" { + try std.testing.expectEqual(OwnerState.held, ownerState(4242, .unknown)); + try std.testing.expectEqual(OwnerState.held, ownerState(0, .unknown)); +} + +test "parseLinuxStat: field 22 survives a comm holding spaces and ')'" { + // comm is `graff (test) x`: a naive whitespace split answers 1 here, and + // any anchor but the LAST ')' lands inside the name. + const weird = "4242 (graff (test) x) S 4241 4242 4242 0 -1 4194304 512 0 0 0 7 3 0 0 20 0 1 0 987654321 12345678 900 18446744073709551615"; + try std.testing.expectEqual(@as(?StartId, 987654321), parseLinuxStat(weird)); + + // The ordinary shape, captured from a real /proc//stat. + const plain = "1 (systemd) S 0 1 1 0 -1 4194560 24512 366 89 0 61 213 0 0 20 0 1 0 12 170201088 3221 18446744073709551615 1 1 0 0 0 0 671173123 4096 1260 0 0 0 17 2 0 0 0 0 0\n"; + try std.testing.expectEqual(@as(?StartId, 12), parseLinuxStat(plain)); + + // Truncated or nonsense input is no identity rather than a wrong one. + try std.testing.expectEqual(@as(?StartId, null), parseLinuxStat("4242 (graff) S 1 2 3")); + try std.testing.expectEqual(@as(?StartId, null), parseLinuxStat("no parens here")); +} + +test "parseRecord: round trips, tolerates a legacy pidfile, rejects garbage" { + var buf: [record_max]u8 = undefined; + const line = formatRecord(&buf, .{ .pid = 4242, .start_id = 1785994345056160 }); + const back = parseRecord(line) orelse return error.ExpectedRecord; + try std.testing.expectEqual(@as(i32, 4242), back.pid); + try std.testing.expectEqual(@as(StartId, 1785994345056160), back.start_id); + + // A record from a graff older than #413: pid, no identity. + const legacy = parseRecord("graff-owner 1 pid=77\n") orelse return error.ExpectedRecord; + try std.testing.expectEqual(@as(i32, 77), legacy.pid); + try std.testing.expectEqual(@as(StartId, 0), legacy.start_id); + // The classic one-line pidfile, same treatment. + const bare = parseRecord("77\n") orelse return error.ExpectedRecord; + try std.testing.expectEqual(@as(i32, 77), bare.pid); + try std.testing.expectEqual(@as(StartId, 0), bare.start_id); + + try std.testing.expect(parseRecord("") == null); + try std.testing.expect(parseRecord("graff-owner 1\n") == null); + try std.testing.expect(parseRecord("graff-owner 1 pid=0 start=5") == null); + try std.testing.expect(parseRecord("graff-owner 1 pid=nonsense") == null); + try std.testing.expect(parseRecord("half a written line") == null); +} + +test "heldByOther: only a live record belonging to someone else is a holder" { + const me = selfPid(); + try std.testing.expect(heldByOther(.{ .pid = me + 1, .start_id = 7 }, me, .held)); + // Our own record is not contention, whatever it says. + try std.testing.expect(!heldByOther(.{ .pid = me, .start_id = 7 }, me, .held)); + // A crashed owner, or one whose pid now belongs to something else. + try std.testing.expect(!heldByOther(.{ .pid = me + 1, .start_id = 7 }, me, .reclaimable)); +} + +test "claimOwnerFile: a crashed owner is reclaimed, a live one is waited for" { + if (builtin.os.tag == .windows) return error.SkipZigTest; // no pid 1 + const io = std.testing.io; + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + const lock = "engine.owner"; + var buf: [record_max]u8 = undefined; + + // Nobody has it: taking it stamps our own record. + try claimOwnerFile(io, tmp.dir, lock); + const mine = readOwnerFile(io, tmp.dir, lock) orelse return error.ExpectedRecord; + try std.testing.expectEqual(selfPid(), mine.pid); + // Re-entrant: our own record never locks us out of our own lock. + try claimOwnerFile(io, tmp.dir, lock); + + // pid 1 (init/launchd) is alive on every unix and is never us. A record + // with NO start identity is what a graff older than #413 wrote, and it + // must still be honoured or the upgrade would brick an in-flight lock. + try tmp.dir.writeFile(io, .{ .sub_path = lock, .data = "graff-owner 1 pid=1\n" }); + try std.testing.expectError(error.LockHeld, claimOwnerFile(io, tmp.dir, lock)); + + switch (probe(io, 1)) { + .id => |live| { + // Same pid, provably a different process: the record is reclaimed. + try tmp.dir.writeFile(io, .{ .sub_path = lock, .data = formatRecord(&buf, .{ .pid = 1, .start_id = live +% 1 }) }); + try claimOwnerFile(io, tmp.dir, lock); + // Same pid AND the same identity: never stolen from. + try tmp.dir.writeFile(io, .{ .sub_path = lock, .data = formatRecord(&buf, .{ .pid = 1, .start_id = live }) }); + try std.testing.expectError(error.LockHeld, claimOwnerFile(io, tmp.dir, lock)); + }, + // pid 1 is opaque to an unprivileged user on macOS; unreadable is + // held, which the legacy assertion above already proved here. + .unknown => {}, + .gone => return error.InitProcessReportedGone, + } + + // An unreadable or truncated record is no record: it must not lock the + // world out, and it must not survive the next claim either. + try tmp.dir.writeFile(io, .{ .sub_path = lock, .data = "half a written l" }); + try claimOwnerFile(io, tmp.dir, lock); + releaseOwnerFile(io, tmp.dir, lock); + try std.testing.expect(readOwnerFile(io, tmp.dir, lock) == null); +} + +test "probe: this process is alive and stably identified; a free pid is gone" { + const io = std.testing.io; + const me = selfPid(); + try std.testing.expect(me > 0); + switch (probe(io, me)) { + // Every supported platform must identify the process it is running in. + .id => |v| { + try std.testing.expect(v != 0); + // Stable: a second reading of the same live pid must agree, or + // every lock would look reclaimable the moment it was checked. + try std.testing.expectEqual(OwnerState.held, stateOf(io, selfRecord(io))); + // And a neighbouring identity must not. + try std.testing.expectEqual(OwnerState.reclaimable, stateOf(io, .{ .pid = me, .start_id = v + 1 })); + }, + // A platform with no identity source degrades to pid-only liveness. + .unknown => try std.testing.expectEqual(OwnerState.held, stateOf(io, .{ .pid = me })), + .gone => return error.LiveProcessReportedGone, + } + // pid 0 and negatives are not processes, so nothing can be held by them. + try std.testing.expectEqual(Probe.gone, probe(io, 0)); + try std.testing.expectEqual(Probe.gone, probe(io, -1)); +} diff --git a/src/session_lock.zig b/src/session_lock.zig index b9fabeee..d47ffcd9 100644 --- a/src/session_lock.zig +++ b/src/session_lock.zig @@ -9,10 +9,50 @@ //! save. Only real contention — another live graff holding this very session — //! is reported, as `error.SessionOpenInAnotherGraff`; /save and the one-shot //! saver already print the error name, so the user sees which failure it was. +//! +//! On that degraded path there is no lock to hold, so since #413 the writers +//! coordinate through a sidecar owner record instead — `.owner`, +//! carrying `{pid, start_id}`. It is stamped for the duration of ONE write and +//! removed after, so an idle graff never blocks anybody; a graff that crashes +//! mid-write leaves it behind, and the start identity is what lets the next +//! writer PROVE the holder is gone instead of guessing with a timeout — a +//! recycled pid would otherwise look like a live owner forever. const std = @import("std"); +const builtin = @import("builtin"); const Io = std.Io; +const proc_identity = @import("proc_identity.zig"); + +/// Suffix of the sidecar that stands in for the advisory lock on a filesystem +/// that has none. Only the degraded path touches it, so the ordinary flock +/// path pays nothing for it. +pub const owner_suffix = ".owner"; + +/// Longest session path we will stamp a sidecar for; a longer one simply +/// writes as it did before #413 rather than failing the save. +const owner_path_max = 512; + +fn ownerPath(buf: *[owner_path_max]u8, path: []const u8) ?[]const u8 { + return std.fmt.bufPrint(buf, "{s}{s}", .{ path, owner_suffix }) catch null; +} + +/// Take the sidecar for one unlocked write, or report the live foreign writer. +/// A path too long to name simply skips the sidecar: it is a best-effort +/// stand-in for a lock the filesystem could not give us, and must never become +/// the reason a session is lost. +fn claimOwner(io: Io, dir: Io.Dir, path: []const u8) error{SessionOpenInAnotherGraff}!void { + var name_buf: [owner_path_max]u8 = undefined; + const owner = ownerPath(&name_buf, path) orelse return; + proc_identity.claimOwnerFile(io, dir, owner) catch return error.SessionOpenInAnotherGraff; +} + +fn releaseOwner(io: Io, dir: Io.Dir, path: []const u8) void { + var name_buf: [owner_path_max]u8 = undefined; + const owner = ownerPath(&name_buf, path) orelse return; + proc_identity.releaseOwnerFile(io, dir, owner); +} + /// Write `data` to `path` under `dir` while holding an exclusive advisory lock /// on the session file, creating the parent directory chain first. /// @@ -37,7 +77,11 @@ pub fn writeSession(io: Io, dir: Io.Dir, path: []const u8, data: []const u8) !vo // #289: locking is advisory. A filesystem with no working locks (some // network mounts report either of these) must still save the session — // degrade to the pre-#289 unlocked write instead of losing the file. + // #413: with no lock to hold, the sidecar owner record is the only + // thing keeping two writers apart, so it brackets the write. error.FileLocksUnsupported, error.SystemResources => { + try claimOwner(io, dir, path); + defer releaseOwner(io, dir, path); return dir.writeFile(io, .{ .sub_path = path, .data = data }); }, else => return err, @@ -77,3 +121,53 @@ test "a second graff reports contention instead of clobbering the session (#289) defer std.testing.allocator.free(now); try std.testing.expectEqualStrings(second, now); } + +test "the unlocked fallback waits for a live writer and reclaims a crashed one (#413)" { + if (builtin.os.tag == .windows) return error.SkipZigTest; // no pid 1 + const io = std.testing.io; + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + const path = "wf.session.json"; + const owner = path ++ owner_suffix; + var buf: [proc_identity.record_max]u8 = undefined; + + // pid 1 (init/launchd) is alive on every unix and is never us. A record + // with NO start identity is what a graff older than #413 wrote: it must + // still block, or the upgrade would brick an in-flight lock. + try tmp.dir.writeFile(io, .{ .sub_path = owner, .data = "graff-owner 1 pid=1\n" }); + try std.testing.expectError(error.SessionOpenInAnotherGraff, claimOwner(io, tmp.dir, path)); + + switch (proc_identity.probe(io, 1)) { + .id => |live| { + // Same pid, provably a different process: a recycled pid may never + // keep the lock, so the crashed writer's record is reclaimed… + const stale = proc_identity.formatRecord(&buf, .{ .pid = 1, .start_id = live +% 1 }); + try tmp.dir.writeFile(io, .{ .sub_path = owner, .data = stale }); + try claimOwner(io, tmp.dir, path); + // …and the sidecar now names us. + var back: [proc_identity.record_max]u8 = undefined; + const rec = proc_identity.parseRecord(try tmp.dir.readFile(io, owner, &back)) orelse return error.ExpectedRecord; + try std.testing.expectEqual(proc_identity.selfPid(), rec.pid); + + // Same pid AND the same identity: a genuinely live holder is never + // stolen from. + const held = proc_identity.formatRecord(&buf, .{ .pid = 1, .start_id = live }); + try tmp.dir.writeFile(io, .{ .sub_path = owner, .data = held }); + try std.testing.expectError(error.SessionOpenInAnotherGraff, claimOwner(io, tmp.dir, path)); + }, + // pid 1 is opaque to an unprivileged user on macOS. Unreadable is + // held, never stolen — which the legacy assertion above already + // proved on this platform. + .unknown => {}, + .gone => return error.InitProcessReportedGone, + } + + // A record nobody owns any more: the write goes through and the sidecar is + // released rather than left behind for the next writer to trip over. + try tmp.dir.deleteFile(io, owner); + try writeSession(io, tmp.dir, path, "{}"); + try claimOwner(io, tmp.dir, path); + releaseOwner(io, tmp.dir, path); + var gone: [proc_identity.record_max]u8 = undefined; + try std.testing.expectError(error.FileNotFound, tmp.dir.readFile(io, owner, &gone)); +} diff --git a/src/test_hooks.zig b/src/test_hooks.zig index cf8aeac5..a7ac6a54 100644 --- a/src/test_hooks.zig +++ b/src/test_hooks.zig @@ -130,6 +130,12 @@ const credential_store = @import("credential_store.zig"); const engine_events = @import("engine_events.zig"); const engine_sink = @import("engine_sink.zig"); +// #413: process START identity, the half of a lock owner record a recycled pid +// cannot forge. session_lock.zig and worktree_lease.zig reach it only through +// calls, and it is the kind of module whose tests must never go quiet: every +// cross-process lock in graff decides "stale or held" with it. +const proc_identity = @import("proc_identity.zig"); + test { _ = learn_holdout; _ = learn_receipt; @@ -169,6 +175,7 @@ test { _ = credential_store; _ = engine_events; _ = engine_sink; + _ = proc_identity; _ = escalation; _ = escalation_tests; _ = edit_contract; diff --git a/src/worktree_lease.zig b/src/worktree_lease.zig index 19f24d0d..547a43c0 100644 --- a/src/worktree_lease.zig +++ b/src/worktree_lease.zig @@ -14,12 +14,17 @@ //! and it already honours #320's rule that a pid alone may never signal an //! owner. Nothing writes records yet, so no root session is warned at startup; //! that needs a registry file plus a call site in startup.zig. +//! +//! #413 supplied the missing half: `proc_identity` reads a pid's START +//! identity, so "is that pid still the process that recorded it" is now a +//! question the OS answers rather than one this file has to be handed. const std = @import("std"); const Io = std.Io; const Allocator = std.mem.Allocator; const process_runner = @import("process_runner.zig"); +const proc_identity = @import("proc_identity.zig"); const runCapped = process_runner.runCapped; const ranOk = process_runner.ranOk; @@ -53,12 +58,13 @@ pub fn canonicalIdentity(git_dir: []const u8, common_dir: []const u8, fallback_p return .{ .id = g, .kind = .linked_worktree }; } -/// One recorded root-session owner of a worktree identity. `start_ns` is the -/// process START time, not just the pid: after a crash the OS may hand the same -/// pid to something unrelated, and #320 requires that never look like an owner. +/// One recorded root-session owner of a worktree identity. `start_id` is the +/// process START identity, not just the pid: after a crash the OS may hand the +/// same pid to something unrelated, and #320 requires that never look like an +/// owner. `proc_identity` produces the value and knows its units. pub const Owner = struct { pid: i32 = 0, - start_ns: u64 = 0, + start_id: proc_identity.StartId = 0, session_id: []const u8 = "", identity: []const u8 = "", last_seen_ms: i64 = 0, @@ -71,38 +77,73 @@ pub const OwnerVerdict = enum { other_worktree, /// Another root session is alive in MY worktree: the #320 warning case. live_foreign, + /// The pid is alive but its identity could not be read, so we cannot prove + /// it is NOT the recorded owner. Warned about like a live one (#413): the + /// cost of a needless warning is a line of text, the cost of staying quiet + /// is two sessions editing one tree. + live_unverified, /// The owner exited (or its pid now belongs to something else). stale_dead, /// Not enough evidence to claim anyone owns it; treated as stale. stale_unverifiable, }; -/// `live_start_ns` is the START time of whatever process currently holds -/// `rec.pid`, or null when no such process exists or it could not be read. -pub fn ownerVerdict(rec: Owner, my_identity: []const u8, my_pid: i32, live_start_ns: ?u64) OwnerVerdict { +/// The record for this process, ready to be written to a registry. +pub fn selfOwner(io: Io, identity: []const u8, session_id: []const u8, now_ms: i64) Owner { + return .{ + .pid = proc_identity.selfPid(), + .start_id = proc_identity.selfStartId(io), + .session_id = session_id, + .identity = identity, + .last_seen_ms = now_ms, + }; +} + +/// `live` is what the OS says about `rec.pid` right now (`proc_identity.probe`). +pub fn ownerVerdict(rec: Owner, my_identity: []const u8, my_pid: i32, live: proc_identity.Probe) OwnerVerdict { if (rec.identity.len == 0 or my_identity.len == 0) return .stale_unverifiable; if (!std.mem.eql(u8, rec.identity, my_identity)) return .other_worktree; // A record with no start identity can only be matched by pid, and pid alone - // is precisely what #320 says must not signal an owner. - if (rec.start_ns == 0) return .stale_unverifiable; - const live = live_start_ns orelse return .stale_dead; - if (live != rec.start_ns) return .stale_dead; // pid reuse, not the owner + // is precisely what #320 says must not signal an owner. Unlike a mutual + // exclusion lock — where honouring a legacy record is the safe answer — + // this one only prints a warning, so the false-positive is the harm. + if (rec.start_id == 0) return .stale_unverifiable; + const unverified = switch (live) { + .gone => return .stale_dead, + .id => |v| blk: { + if (v != rec.start_id) return .stale_dead; // pid reuse, not the owner + break :blk false; + }, + .unknown => true, + }; if (rec.pid == my_pid) return .self; - return .live_foreign; + return if (unverified) .live_unverified else .live_foreign; } /// The #320 preflight: the first live foreign owner of MY worktree, if any. /// Everything else — my own record, another worktree's, a crashed or pid-reused /// one — is silent, so a stale registry can never block a startup. -/// `live_start_ns[i]` pairs with `records[i]`; a short slice reads as "unknown". -pub fn duplicateOwner(records: []const Owner, live_start_ns: []const ?u64, my_identity: []const u8, my_pid: i32) ?Owner { +/// `probes[i]` pairs with `records[i]`; a record past the end of `probes` was +/// never probed at all, which is no evidence of anything and so is skipped. +pub fn duplicateOwner(records: []const Owner, probes: []const proc_identity.Probe, my_identity: []const u8, my_pid: i32) ?Owner { for (records, 0..) |rec, i| { - const live = if (i < live_start_ns.len) live_start_ns[i] else null; - if (ownerVerdict(rec, my_identity, my_pid, live) == .live_foreign) return rec; + if (i >= probes.len) break; + switch (ownerVerdict(rec, my_identity, my_pid, probes[i])) { + .live_foreign, .live_unverified => return rec, + else => {}, + } } return null; } +/// Probe every record's pid once into caller storage; the slice it returns is +/// what `duplicateOwner` expects. +pub fn probeOwners(io: Io, records: []const Owner, out: []proc_identity.Probe) []const proc_identity.Probe { + const n = @min(records.len, out.len); + for (records[0..n], out[0..n]) |rec, *slot| slot.* = proc_identity.probe(io, rec.pid); + return out[0..n]; +} + pub fn duplicateOwnerWarning(arena: Allocator, rec: Owner, age_ms: i64) []const u8 { const mins = @divTrunc(if (age_ms > 0) age_ms else 0, std.time.ms_per_min); return std.fmt.allocPrint( @@ -174,46 +215,78 @@ test "canonicalIdentity: one id per worktree, distinct across linked worktrees ( test "ownerVerdict: only a verified live process in MY worktree is an owner (#320)" { const me = "/repo/.git"; - const rec: Owner = .{ .pid = 4242, .start_ns = 777, .session_id = "s-1", .identity = me }; + const rec: Owner = .{ .pid = 4242, .start_id = 777, .session_id = "s-1", .identity = me }; - try std.testing.expectEqual(OwnerVerdict.live_foreign, ownerVerdict(rec, me, 99, 777)); - try std.testing.expectEqual(OwnerVerdict.self, ownerVerdict(rec, me, 4242, 777)); + try std.testing.expectEqual(OwnerVerdict.live_foreign, ownerVerdict(rec, me, 99, .{ .id = 777 })); + try std.testing.expectEqual(OwnerVerdict.self, ownerVerdict(rec, me, 4242, .{ .id = 777 })); // Separate git worktrees do not conflict. - try std.testing.expectEqual(OwnerVerdict.other_worktree, ownerVerdict(rec, "/repo/.git/worktrees/wt1", 99, 777)); + try std.testing.expectEqual(OwnerVerdict.other_worktree, ownerVerdict(rec, "/repo/.git/worktrees/wt1", 99, .{ .id = 777 })); // Crashed owner: the pid is simply gone. - try std.testing.expectEqual(OwnerVerdict.stale_dead, ownerVerdict(rec, me, 99, null)); + try std.testing.expectEqual(OwnerVerdict.stale_dead, ownerVerdict(rec, me, 99, .gone)); // PID reuse: the pid is live but it is a different process. - try std.testing.expectEqual(OwnerVerdict.stale_dead, ownerVerdict(rec, me, 99, 778)); + try std.testing.expectEqual(OwnerVerdict.stale_dead, ownerVerdict(rec, me, 99, .{ .id = 778 })); // No start identity recorded — unverifiable is stale, never a warning. var no_start = rec; - no_start.start_ns = 0; - try std.testing.expectEqual(OwnerVerdict.stale_unverifiable, ownerVerdict(no_start, me, 99, 777)); + no_start.start_id = 0; + try std.testing.expectEqual(OwnerVerdict.stale_unverifiable, ownerVerdict(no_start, me, 99, .{ .id = 777 })); +} + +test "ownerVerdict: a pid we cannot identify is assumed to be the owner (#413)" { + const me = "/repo/.git"; + const rec: Owner = .{ .pid = 4242, .start_id = 777, .session_id = "s-1", .identity = me }; + // An unreadable identity is not evidence of death: warn rather than treat + // a possibly live session as stale. + try std.testing.expectEqual(OwnerVerdict.live_unverified, ownerVerdict(rec, me, 99, .unknown)); + // …but it is still not somebody else when the pid is mine. + try std.testing.expectEqual(OwnerVerdict.self, ownerVerdict(rec, me, 4242, .unknown)); } test "duplicateOwner: picks the live foreign session and ignores stale records (#320)" { const me = "/repo/.git"; const records = [_]Owner{ - .{ .pid = 1, .start_ns = 10, .session_id = "dead", .identity = me }, - .{ .pid = 2, .start_ns = 20, .session_id = "other-wt", .identity = "/repo/.git/worktrees/wt1" }, - .{ .pid = 3, .start_ns = 30, .session_id = "mine", .identity = me }, - .{ .pid = 4, .start_ns = 40, .session_id = "live", .identity = me }, + .{ .pid = 1, .start_id = 10, .session_id = "dead", .identity = me }, + .{ .pid = 2, .start_id = 20, .session_id = "other-wt", .identity = "/repo/.git/worktrees/wt1" }, + .{ .pid = 3, .start_id = 30, .session_id = "mine", .identity = me }, + .{ .pid = 4, .start_id = 40, .session_id = "live", .identity = me }, }; - const live = [_]?u64{ null, 20, 30, 40 }; + const live = [_]proc_identity.Probe{ .gone, .{ .id = 20 }, .{ .id = 30 }, .{ .id = 40 } }; const found = duplicateOwner(&records, &live, me, 3) orelse return error.ExpectedDuplicate; try std.testing.expectEqualStrings("live", found.session_id); try std.testing.expectEqual(@as(i32, 4), found.pid); // Alone in the worktree: only my own record matches, so no warning. const solo = [_]Owner{records[2]}; - try std.testing.expect(duplicateOwner(&solo, &.{30}, me, 3) == null); + try std.testing.expect(duplicateOwner(&solo, &.{.{ .id = 30 }}, me, 3) == null); // A registry we cannot read at all must not manufacture an owner. try std.testing.expect(duplicateOwner(&records, &.{}, me, 3) == null); } +test "selfOwner + probeOwners: this process records and verifies as itself (#413)" { + const io = std.testing.io; + const me = "/repo/.git"; + const mine = selfOwner(io, me, "s-self", 1234); + try std.testing.expect(mine.pid > 0); + + var probes: [1]proc_identity.Probe = undefined; + const live = probeOwners(io, &.{mine}, &probes); + try std.testing.expectEqual(@as(usize, 1), live.len); + // My own live record is `self`, never a duplicate owner… + try std.testing.expectEqual(OwnerVerdict.self, ownerVerdict(mine, me, mine.pid, live[0])); + try std.testing.expect(duplicateOwner(&.{mine}, live, me, mine.pid) == null); + + // …and a record claiming my pid from a process that no longer exists is + // stale, which is the whole point: a recycled pid cannot hold a lease. + if (mine.start_id != 0) { + var recycled = mine; + recycled.start_id +%= 1; + try std.testing.expectEqual(OwnerVerdict.stale_dead, ownerVerdict(recycled, me, mine.pid, live[0])); + } +} + test "duplicateOwnerWarning: names the pid, the session and an escape hatch (#320)" { var arena_state = std.heap.ArenaAllocator.init(std.testing.allocator); defer arena_state.deinit(); - const text = duplicateOwnerWarning(arena_state.allocator(), .{ .pid = 4242, .start_ns = 1, .session_id = "s-1" }, 5 * std.time.ms_per_min); + const text = duplicateOwnerWarning(arena_state.allocator(), .{ .pid = 4242, .start_id = 1, .session_id = "s-1" }, 5 * std.time.ms_per_min); try std.testing.expect(std.mem.indexOf(u8, text, "4242") != null); try std.testing.expect(std.mem.indexOf(u8, text, "s-1") != null); try std.testing.expect(std.mem.indexOf(u8, text, "5m") != null);