diff --git a/CLAUDE.md b/CLAUDE.md index 73db156..23911ef 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -12,7 +12,7 @@ library plus CoreFoundation/Security. Run from the repo root: ```bash -zig build test --summary all # unit tests (~3s, 292 at last count) +zig build test --summary all # unit tests (~3s, 293 at last count) zig build # debug binary → zig-out/bin/lcc zig build -Doptimize=ReleaseFast # what PATH should be serving zig build run -- list # run without installing @@ -142,6 +142,21 @@ Do not "simplify" `build.zig`'s separate `test_mod`: reusing the executable's mo improvement and turns every recovered row into an `unknown_session` error. The ids collide across daemons anyway — `next_id` restarts at 1 — which is also why the state file is named for Claude Code's session UUID rather than for either the lcc id or the worktree path. +- **A dashboard row has to name a directory that still exists.** `sessions.visible` is that + rule, and *both* ways the rows are read have to go through it — the live snapshot in + `collect` / `snapshotOnce` and `sessions.resolved` — because the daemon never drops a + session from its own list and flushes that whole list once more as it exits, so its file + outlives it naming every worktree it ever ran in. `collect` puts the same predicate on + `app.worktreeChoices`, not inside it: git keeps listing a worktree whose directory was + deleted (`prunable`), and that is exactly the row `lcc list` shows in red and `lcc remove` + needs in order to clean the entry up. +- **A registry row reading `unknown` is bookkeeping, not a session.** `collect` collapses it to + `null` through `liveMatch` before `rowFor` ever sees it. Leave it a match and the worktree's + hook-recovered status is never consulted — which after a daemon dies is every worktree it + touched, so `watch_state` recovers nothing and the column reads `unknown` with an age + measured from the epoch. Passing `rowFor` the recovered status *and* the dead session id + instead is worse than either: `attachable` goes true and enter asks the daemon for a session + nothing holds. - **A session's hook settings file is per session, not per daemon.** `watch_paths.hooksFor` names it `hooks-.json` and `watch_hooks.settingsJson` bakes that id into every hook command line, so a report says which session it came from. Collapsing them back into diff --git a/README.md b/README.md index 16409b7..5f15c3a 100644 --- a/README.md +++ b/README.md @@ -123,6 +123,16 @@ A session running in another repository still appears, because an agent working somewhere you are not looking is the one you most need to see. `lcc open xcode` is unchanged and still opens a worktree in Xcode. +What the list never shows is a directory that is not there. A worktree you +removed drops off the moment its directory does — including one `git worktree +prune` has not caught up with yet, which git still lists — and the sessions that +ran in it go with it rather than staying as rows that open nothing. Nothing +cleans up after them otherwise: the background process never drops a session from +its own list and writes that list once more on the way out, so its file goes on +naming every worktree it ever touched until a new one replaces the file. The one +thing you give up is reaching an agent still running in a directory you deleted — +it has no row any more, and `lcc open --stop-all` is what ends it. + `● waiting` is the one that wants you: the agent is blocked on a permission prompt or a question. `◐ active` is a turn in flight, `○ idle` is finished. Those come from Claude Code's own hooks rather than from reading its screen, so @@ -183,6 +193,12 @@ normally clears its file and goes back to `no session`, having nothing left to say. Sessions that died before this shipped left no file behind and stay `no session` until you start them again. +A row the dead process left in the registry does not outrank that file either. It +can only read `unknown` — the one thing that could have said otherwise is gone — +and what the hooks reported is better than that, so the recovered status wins. It +is still not offered for attach: the session behind that row died with its +process, whatever the registry remembers of it. + Sessions also hold the build of `lcc` they started under. After `zig build` the sessions still running can be many commits behind the `lcc` you are typing — older behaviour reached through the same command, and every symptom of it looks diff --git a/src/commands/watch.zig b/src/commands/watch.zig index 4f4d4d6..a493425 100644 --- a/src/commands/watch.zig +++ b/src/commands/watch.zig @@ -76,8 +76,9 @@ fn snapshotOnce(app: app_mod.App, opts: Opts) !void { const outdated = outdatedDaemon(app, app.gpa, exec.selfModified(app.gpa, app.io)); if (live) |list| { - const rows = try app.gpa.alloc(Row, list.len); - for (list, 0..) |s, i| rows[i] = toRow(s, false); + const present = try onDisk(app.io, app.gpa, list); + const rows = try app.gpa.alloc(Row, present.len); + for (present, 0..) |s, i| rows[i] = toRow(s, false); return emit(app, opts, rows, true, outdated, now); } @@ -96,6 +97,19 @@ fn outdatedDaemon(app: app_mod.App, arena: std.mem.Allocator, built: ?i64) bool return sessions.daemonOutdated(sessions.load(arena, app.io, app.environ), built); } +pub fn onDisk( + io: Io, + arena: std.mem.Allocator, + list: []const sessions.Session, +) ![]const sessions.Session { + var out: std.ArrayList(sessions.Session) = .empty; + for (list) |s| { + if (sessions.visible(io, s, true) == null) continue; + try out.append(arena, s); + } + return out.toOwnedSlice(arena); +} + const outdated_warning = "These sessions are running an older build of lcc than this one."; const outdated_hint = "They end on their own 30 minutes after the last one finishes. `lcc open --stop-all` is immediate, but ends them now."; @@ -368,7 +382,7 @@ fn collect(app: app_mod.App, arena: std.mem.Allocator, now: i64) ![]watch_table. var live: []const sessions.Session = &.{}; var stale = false; if (watch_client.snapshot(app) catch null) |list| { - live = list; + live = try onDisk(app.io, arena, list); } else { const state = sessions.load(arena, app.io, app.environ); const resolved = try sessions.resolved(arena, app.io, state, now); @@ -385,8 +399,9 @@ fn collect(app: app_mod.App, arena: std.mem.Allocator, now: i64) ![]watch_table. if (app.repo()) |repo| { for (try app_mod.worktreeChoices(app, repo)) |choice| { + if (!disk.isDirectory(app.io, choice.entry.path)) continue; const branch = choice.entry.branch orelse app_mod.shortHead(choice.entry.head); - const match = findSession(live, choice.entry.path); + const match = liveMatch(findSession(live, choice.entry.path)); const recovered: ?watch_state.Resolved = if (match != null) null else recover: { if (states == null) states = watch_state.load(arena, app.io, app.environ); break :recover watch_state.statusFor( @@ -446,6 +461,11 @@ pub fn rowFor( }; } +pub fn liveMatch(found: ?sessions.Session) ?sessions.Session { + const session = found orelse return null; + return if (session.parsedStatus() == .unknown) null else session; +} + pub fn findSession(list: []const sessions.Session, worktree: []const u8) ?sessions.Session { var fallback: ?sessions.Session = null; for (list) |s| { @@ -637,6 +657,89 @@ test "a live session outranks anything left on disk for the same worktree" { try std.testing.expect(row.attachable()); } +test "a session the daemon is still running in a deleted worktree is dropped too, not just the dead ones" { + const gpa = std.testing.allocator; + const io = std.testing.io; + var arena_state: std.heap.ArenaAllocator = .init(gpa); + defer arena_state.deinit(); + const arena = arena_state.allocator(); + + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + const base = try tmp.dir.realPathFileAlloc(io, ".", arena); + const removed = try std.fs.path.join(arena, &.{ base, "removed" }); + + const present = try onDisk(io, arena, &.{ + .{ .id = "s-here", .worktree = base, .branch = "feature/pe-284", .status = "waiting" }, + .{ .id = "s-gone", .worktree = removed, .branch = "feature/pe-283", .status = "active" }, + .{ .id = "s-done", .worktree = removed, .branch = "feature/pe-286", .status = "exited" }, + }); + + if (present.len != 1) { + std.debug.print( + "{d} of 3 sessions survived a snapshot with two deleted worktrees: the dashboard " ++ + "lists work that has nowhere left to happen, and enter on those rows opens an " ++ + "agent in a directory that is not there.\n", + .{present.len}, + ); + return error.TestExpectedEqual; + } + try std.testing.expectEqualStrings("s-here", present[0].id); + try std.testing.expectEqualStrings("waiting", present[0].status); +} + +test "a row left behind by a dead daemon does not outrank what the hooks reported" { + const gpa = std.testing.allocator; + var arena_state: std.heap.ArenaAllocator = .init(gpa); + defer arena_state.deinit(); + const arena = arena_state.allocator(); + + const leftover: sessions.Session = .{ + .id = "s-00000006", + .worktree = "/w/pe-290", + .branch = "feature/pe-290", + .issue = "PE-290", + .status = "unknown", + .last_activity_at = 1200, + }; + + if (liveMatch(leftover) != null) { + std.debug.print( + "a session the daemon left in the registry still counts as a match: it reports " ++ + "`unknown` for every worktree that daemon ever touched, and the status the hooks " ++ + "wrote to disk is never consulted, which is the whole point of recovering it.\n", + .{}, + ); + return error.TestUnexpectedResult; + } + + const row = rowFor( + arena, + "/w/pe-290", + "feature/pe-290", + liveMatch(leftover), + .{ .status = .waiting, .last_activity_at = 1700 }, + false, + ); + try std.testing.expectEqual(sessions.Status.waiting, row.status.?); + try std.testing.expectEqual(@as(i64, 1700), row.last_activity_at); + + if (row.attachable()) { + std.debug.print( + "the recovered row kept the dead session's id and offers itself for attach: enter " ++ + "asks the daemon for a session nothing holds and comes back unknown_session, " ++ + "instead of starting the work again.\n", + .{}, + ); + return error.TestUnexpectedResult; + } + + var running = leftover; + running.status = "waiting"; + try std.testing.expectEqualStrings("s-00000006", liveMatch(running).?.id); + try std.testing.expect(liveMatch(null) == null); +} + test "a worktree with neither a session nor a report still reads as having none" { const gpa = std.testing.allocator; var arena_state: std.heap.ArenaAllocator = .init(gpa); diff --git a/src/disk.zig b/src/disk.zig index 92ece4b..b7e52f1 100644 --- a/src/disk.zig +++ b/src/disk.zig @@ -38,6 +38,12 @@ pub fn realPath(gpa: std.mem.Allocator, io: Io, target: []const u8) []const u8 { return Io.Dir.cwd().realPathFileAlloc(io, target, gpa) catch target; } +pub fn isDirectory(io: Io, path: []const u8) bool { + if (path.len == 0) return false; + const info = Io.Dir.cwd().statFile(io, path, .{}) catch return false; + return info.kind == .directory; +} + pub fn removeChild(io: Io, parent: []const u8, path: []const u8) !void { const dirname = std.fs.path.dirname(path) orelse return error.RefusingToDelete; const trimmed = std.mem.trimEnd(u8, parent, "/"); @@ -54,6 +60,38 @@ pub fn abbreviate(gpa: std.mem.Allocator, environ: *const std.process.Environ.Ma return std.fmt.allocPrint(gpa, "~{s}", .{path[home.len..]}) catch path; } +test "a worktree is a directory that is there, not a name that used to be one" { + const io = std.testing.io; + const gpa = std.testing.allocator; + + var arena_state: std.heap.ArenaAllocator = .init(gpa); + defer arena_state.deinit(); + const arena = arena_state.allocator(); + + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + const base = try tmp.dir.realPathFileAlloc(io, ".", arena); + + try std.testing.expect(isDirectory(io, base)); + + const gone = try std.fs.path.join(arena, &.{ base, "removed" }); + try std.testing.expect(!isDirectory(io, gone)); + + const file = try std.fs.path.join(arena, &.{ base, "a-file" }); + try Io.Dir.cwd().writeFile(io, .{ .sub_path = file, .data = "" }); + if (isDirectory(io, file)) { + std.debug.print( + "a plain file answered yes: a session whose worktree was replaced by a file of the " ++ + "same name keeps its row, and enter on it starts an agent in a directory that " ++ + "does not exist.\n", + .{}, + ); + return error.TestUnexpectedResult; + } + + try std.testing.expect(!isDirectory(io, "")); +} + test "isInside distinguishes containment from a shared prefix" { const gpa = std.testing.allocator; diff --git a/src/sessions.zig b/src/sessions.zig index 5585481..d4a8242 100644 --- a/src/sessions.zig +++ b/src/sessions.zig @@ -14,7 +14,6 @@ pub const Status = enum { idle, plan, exited, - orphan, unknown, pub fn label(self: Status) []const u8 { @@ -128,37 +127,31 @@ pub fn daemonOutdated(state: State, binary_modified: ?i64) bool { return built > daemon.started_at; } +pub fn visible(io: Io, session: Session, daemon_alive: bool) ?Status { + if (!disk.isDirectory(io, session.worktree)) return null; + return if (daemon_alive) session.parsedStatus() else .unknown; +} + pub fn resolved( gpa: std.mem.Allocator, io: Io, state: State, now: i64, ) ![]const Resolved { - const out = try gpa.alloc(Resolved, state.sessions.len); const daemon_alive = alive(state); const wrote_at = if (state.daemon) |d| d.wrote_at else 0; const age = @max(0, now - wrote_at); - for (state.sessions, 0..) |session, i| { - var status = session.parsedStatus(); - if (!daemon_alive) { - status = .unknown; - } else if (status != .exited and !worktreeExists(io, session.worktree)) { - status = .orphan; - } - out[i] = .{ + var out: std.ArrayList(Resolved) = .empty; + for (state.sessions) |session| { + const status = visible(io, session, daemon_alive) orelse continue; + try out.append(gpa, .{ .session = session, .status = status, .stale = daemon_alive and age > stale_after_seconds, - }; + }); } - return out; -} - -fn worktreeExists(io: Io, worktree: []const u8) bool { - if (worktree.len == 0) return false; - const info = Io.Dir.cwd().statFile(io, worktree, .{}) catch return false; - return info.kind == .directory; + return out.toOwnedSlice(gpa); } pub fn owning(gpa: std.mem.Allocator, state: State, target: []const u8) ?Session { @@ -319,7 +312,7 @@ test "nothing is out of date when there is no daemon to be out of date" { try testing.expect(!daemonOutdated(.{ .daemon = .{ .pid = pid, .started_at = 0 } }, 5_000)); } -test "a worktree that is gone reads as orphan, and the row stays" { +test "a session whose worktree was deleted stops being a row at all" { const gpa = testing.allocator; const io = testing.io; var arena_state: std.heap.ArenaAllocator = .init(gpa); @@ -329,22 +322,62 @@ test "a worktree that is gone reads as orphan, and the row stays" { var tmp = testing.tmpDir(.{}); defer tmp.cleanup(); const base = try tmp.dir.realPathFileAlloc(io, ".", arena); + const removed = try std.fs.path.join(arena, &.{ base, "removed" }); const state: State = .{ .daemon = .{ .pid = 1, .wrote_at = 1000 }, .sessions = &.{ .{ .id = "s-here", .worktree = base, .status = "active" }, - .{ .id = "s-gone", .worktree = try std.fs.path.join(arena, &.{ base, "removed" }), .status = "active" }, - .{ .id = "s-done", .worktree = try std.fs.path.join(arena, &.{ base, "removed" }), .status = "exited" }, - .{ .id = "s-plan", .worktree = try std.fs.path.join(arena, &.{ base, "removed" }), .status = "plan" }, + .{ .id = "s-gone", .worktree = removed, .status = "active" }, + .{ .id = "s-done", .worktree = removed, .status = "exited" }, + .{ .id = "s-plan", .worktree = removed, .status = "plan" }, }, }; const rows = try resolved(arena, io, state, 1001); + if (rows.len != 1) { + std.debug.print( + "{d} rows came back for one surviving worktree: every worktree ever removed keeps a " ++ + "line on the dashboard, so the list stops being what is on disk and starts being " ++ + "everything a daemon once ran.\n", + .{rows.len}, + ); + return error.TestExpectedEqual; + } + try testing.expectEqualStrings("s-here", rows[0].session.id); try testing.expectEqual(Status.active, rows[0].status); - try testing.expectEqual(Status.orphan, rows[1].status); - try testing.expectEqual(Status.exited, rows[2].status); - try testing.expectEqual(Status.orphan, rows[3].status); +} + +test "a deleted worktree is gone from the list whether or not a daemon is still there" { + const gpa = testing.allocator; + const io = testing.io; + var arena_state: std.heap.ArenaAllocator = .init(gpa); + defer arena_state.deinit(); + const arena = arena_state.allocator(); + + var tmp = testing.tmpDir(.{}); + defer tmp.cleanup(); + const base = try tmp.dir.realPathFileAlloc(io, ".", arena); + const removed = try std.fs.path.join(arena, &.{ base, "removed" }); + + const dead: State = .{ + .daemon = .{ .pid = 0x7fff_fffe, .wrote_at = 1000 }, + .sessions = &.{ + .{ .id = "s-gone", .worktree = removed, .status = "waiting" }, + .{ .id = "s-here", .worktree = base, .status = "waiting" }, + }, + }; + try testing.expect(!alive(dead)); + + const rows = try resolved(arena, io, dead, 1001); + try testing.expectEqual(@as(usize, 1), rows.len); + try testing.expectEqualStrings("s-here", rows[0].session.id); + try testing.expectEqual(Status.unknown, rows[0].status); + + try testing.expect(visible(io, .{ .worktree = removed, .status = "waiting" }, true) == null); + try testing.expect(visible(io, .{ .worktree = removed, .status = "waiting" }, false) == null); + try testing.expect(visible(io, .{ .worktree = "", .status = "waiting" }, true) == null); + try testing.expectEqual(Status.waiting, visible(io, .{ .worktree = base, .status = "waiting" }, true).?); } test "plan round-trips as text, like every other status" { diff --git a/src/watch_state.zig b/src/watch_state.zig index ffa9731..85ba0ed 100644 --- a/src/watch_state.zig +++ b/src/watch_state.zig @@ -120,7 +120,7 @@ pub fn load( const file_path = std.fs.path.join(gpa, &.{ base, dirent.name }) catch continue; var record = readAt(gpa, io, file_path) orelse continue; if (record.cwd.len == 0) continue; - if (!worktreeExists(io, record.cwd)) continue; + if (!disk.isDirectory(io, record.cwd)) continue; record.cwd = disk.realPath(gpa, io, record.cwd); out.append(gpa, record) catch continue; @@ -128,11 +128,6 @@ pub fn load( return out.toOwnedSlice(gpa) catch &.{}; } -fn worktreeExists(io: Io, worktree: []const u8) bool { - const info = Io.Dir.cwd().statFile(io, worktree, .{}) catch return false; - return info.kind == .directory; -} - const testing = std.testing; fn testEnviron(arena: std.mem.Allocator, base: []const u8) !std.process.Environ.Map { @@ -182,7 +177,6 @@ test "a recovered status is never one only a live session can be in" { try testing.expect(status != .active); try testing.expect(status != .starting); try testing.expect(status != .exited); - try testing.expect(status != .orphan); try testing.expect(status != .unknown); } } diff --git a/src/watch_status.zig b/src/watch_status.zig index a0e3a6b..b900acc 100644 --- a/src/watch_status.zig +++ b/src/watch_status.zig @@ -33,7 +33,7 @@ pub fn present(current: Status, plan: bool) Status { if (!plan) return current; return switch (current) { .active, .idle => .plan, - .starting, .waiting, .exited, .orphan, .unknown, .plan => current, + .starting, .waiting, .exited, .unknown, .plan => current, }; } @@ -85,7 +85,6 @@ test "every hook event maps to a status a session can actually be in" { for ([_]Event{ .waiting, .active, .idle, .ended }) |event| { const status = apply(.starting, event); try testing.expect(status != .unknown); - try testing.expect(status != .orphan); try testing.expect(status != .plan); } } @@ -97,13 +96,12 @@ test "plan mode replaces working, and never replaces being blocked" { try testing.expectEqual(Status.waiting, present(.waiting, true)); try testing.expectEqual(Status.exited, present(.exited, true)); - try testing.expectEqual(Status.orphan, present(.orphan, true)); try testing.expectEqual(Status.unknown, present(.unknown, true)); try testing.expectEqual(Status.starting, present(.starting, true)); } test "without plan mode, present changes nothing at all" { - for ([_]Status{ .starting, .active, .waiting, .idle, .plan, .exited, .orphan, .unknown }) |status| { + for ([_]Status{ .starting, .active, .waiting, .idle, .plan, .exited, .unknown }) |status| { try testing.expectEqual(status, present(status, false)); } } diff --git a/src/watch_table.zig b/src/watch_table.zig index 089893b..2bfa93d 100644 --- a/src/watch_table.zig +++ b/src/watch_table.zig @@ -18,7 +18,7 @@ pub const Row = struct { pub fn attachable(self: Row) bool { if (self.session_id == null) return false; return switch (self.status orelse return false) { - .starting, .active, .plan, .waiting, .idle, .orphan => true, + .starting, .active, .plan, .waiting, .idle => true, .exited, .unknown => false, }; } @@ -32,7 +32,6 @@ fn glyph(status: ?sessions.Status) []const u8 { .idle => "○", .starting => "◌", .exited => "✗", - .orphan => "⚠", .unknown => "?", }; } @@ -42,7 +41,6 @@ fn paint(status: ?sessions.Status, palette: ui.Palette) []const u8 { .waiting => palette.yellow, .active => palette.green, .plan => palette.cyan, - .orphan => palette.yellow, .exited => palette.red, .idle, .starting, .unknown => palette.dim, }; @@ -357,8 +355,6 @@ test "a row is attachable only when something is actually behind it" { try testing.expect(!row.attachable()); row.status = .exited; try testing.expect(!row.attachable()); - row.status = .orphan; - try testing.expect(row.attachable()); row.status = .plan; try testing.expect(row.attachable());