Fix Session Owner Tracking and Finalization
Here's trail 583:
Session liveness: detect crashed 'active' sessions via PID + start-time
- Branch:
soph/session-liveness-pid(basemain) · Status: draft · Author: Soph · created 2026-06-17 - No body/description written yet
- 3 open findings from the
trail-pr-reviewagent (0 high, 1 medium, 2 low) — all on the resume code paths, oddly enough, not on session-liveness code (the review ran against this branch's diff which touchesresume.go/resume_picker.go):
| Sev | Location | Issue |
|---|---|---|
| Medium | resume_picker.go:264 buildCheckpointBranchIndex | Walks up to 50 commits per branch with no dedup → O(branches × commits) on repos with hundreds of branches. Suggests a seen map / early-stop. |
| Low | resume_picker.go branchCheckedOutElsewhere | Silently returns ("", false) on git failure/timeout; a stale curPath from a prior iteration could cause a false match. Suggests clearing curPath on parse errors. |
| Low | resume.go:174 resumeByCheckpointID | Reads checkpoint metadata before verifying existence, triggering a possibly-unnecessary remote metadata check. Suggests a cheaper existence check first, or documenting it as intentional. |
Want me to dig into any of these? I can pull up the actual diff on that branch and assess whether the medium finding is worth fixing, or just walk through the code at those locations. Let me know how deep you want to go.
I think that code is actually merged, can we reset the branch to latest main and then look at the actual implementation. You can dismiss the findings
Done. Summary of what happened:
Branch reset — soph/session-liveness-pid was a stale branch holding 3 early commits of the resume-picker work (and 196 commits behind). That work was already merged into main via PR #1445 (soph/resume-session-picker), with follow-up commits that addressed review feedback. I reset the branch to origin/main (f7fe32a3c).
Implementation check — all 3 findings are resolved in the merged code:
- Medium
buildCheckpointBranchIndex— now seeds adefaultCommitsstop-set so feature-branch walks halt at shared history, plus first-branch-wins dedup (if _, ok := index[key]; !ok). The doc comment explicitly calls out avoiding the O(branches × history) blowup the finding flagged. - Low
branchCheckedOutElsewhere— parsing extracted into block-scopedparseWorktreeForBranch(commit5ef184290, literally tagged "trail finding");curPathresets at each block boundary, so no stale-path false match. - Low
resumeByCheckpointID— the local-read-fails →checkRemoteMetadatapath is the intended fallback and now carries an explicit debug log; by design.
Findings dismissed — all 3 dismissed with reasons recording where each was fixed. Trail 583 now has 0 open findings.
One note: the trail is titled "Session liveness: detect crashed 'active' sessions via PID + start-time" but the branch only ever contained resume-picker code, and it's still in draft status. So the PID/start-time session-liveness work it's named for doesn't appear to exist on this branch — if that's still something you intend to build, the trail is essentially empty and ready for it. Want me to do anything with the trail itself (close it, or leave it for the liveness work)?
The plan is revised with Codex's feedback incorporated. Here's a summary of what changed:
Codex review — accepted and folded in:
- Darwin: use the existing
golang.org/x/sys/unixdep's typedSysctlKinfoProc(confirmed present at v0.46.0) instead of hand-decoding rawkinfo_procbytes — much safer. - Linux
/proc/statparsing: split on the last)(comm can contain spaces/parens), post-comm field indices spelled out (ppid=1, starttime=19), plus a dedicated parser test; use/proc/sys/kernel/random/boot_idas the reboot discriminator. - doctor inaccuracies corrected: there's no top-level confirm (it prompts per-session), and
CondenseSessionByIDsalvages toPhaseIdle, not "ended" — wording fixed. Crashed sessions now flow through the existing fix loop, defaulting to "Condense" rather than silently skipping the prompt (per Codex's safety advice). - status
--json: added —sessionStatusLabel(~line 670) must emitcrashedtoo, not just the human path. - Owner-resolution risk called out explicitly with the "resolve to Unknown rather than a guessed/transient PID" rule, plus per-agent manual validation.
- Tests: OS build tags,
Kill()+Wait(), unsupported-platform asserts Unknown.
One open item for you (Codex agreed with my refinement, but it overrides your earlier selection): you chose "status itself transitions the crashed session out of ACTIVE." I've kept status read-only (it reports crashed immediately, no 1-hour wait) and put the actual finalize in doctor, because entire status runs constantly and possibly alongside a live agent — mutating session state from a display command risks races. The plan reflects this; let me know if you'd rather status mutate.
The updated plan is at /Users/soph/.claude/plans/radiant-percolating-karp.md. Ready for your approval to start implementing.
"entire status" is not run constantly, also I wonder if "crashed" is really the right term especially if for example in claude a single "exit" can cause this too
The trail is built and verified. Summary:
Session liveness — implemented on soph/session-liveness-pid
What it does: an ACTIVE session whose owning agent process is gone (clean /exit, crash, kill, closed terminal, reboot) is now detected immediately and finalized, instead of lingering as "active" for the 1-hour StuckActiveThreshold.
New cmd/entire/cli/proclive package (stdlib + x/sys/unix leaf, no import cycle):
- Records the owner's
{PID, start-time fingerprint, boot, host}by walking up the process tree from the hook to the first non-shell, non-entireancestor. Check()returns Alive / Dead / Unknown — Dead on missing PID, start mismatch (PID reuse), or reboot; Unknown on cross-host or unsupported platform (Windows degrades cleanly to the old timeout).- Per-platform seam:
proc_linux.go(last-)/proc/statparse +boot_id),proc_darwin.go(typedunix.SysctlKinfoProc),proc_other.go(Unknown).
Wiring:
session.StategainsOwner+OwnerLiveness()/OwnerExited().- Captured each turn start in
InitializeSession(alongsidecaptureSessionBranch). - A shared
finalizeExitedSessionshelper replays the missingEventSessionStop(→PhaseEnded+ condense), called up front by bothentire status(with a "Finalized N exited session(s)" note) andentire doctor.status --jsonand the human/doctor paths also carry anexitedlabel as a fallback.
Verification: mise run fmt && lint clean; mise run test:ci green (unit + integration + 59 Vogon + 4 external-agent canary); plus a live smoke test confirming entire status finalized a planted exited session (phase: ended, fully_condensed: true).
Trail 583 now has a body describing the feature and is moved from draft → open.
Changes are on the branch, uncommitted — want me to commit them (and/or open a PR)? Note Windows liveness and entire session list labeling are documented as out-of-scope follow-ups.
/simplify → 4 cleanup agents in parallel → apply the fixes
You are improving the quality of the changed code, not hunting for bugs. Review
it for reuse, simplification, efficiency, and altitude issues, then fix what you
find. Do not look for correctness bugs — that is what /code-review is for.
Phase 0 — Gather the diff
Run git diff @{upstream}...HEAD (or git diff main...HEAD / git diff HEAD~1
if there's no upstream) to get the unified diff under review. If there are
uncommitted changes, or the range diff is empty, also run git diff HEAD and
include the working-tree changes in scope — the review often runs before the
commit. If a PR number, branch name, or file path was passed as an argument,
review that target instead. Treat this diff as the review scope.
Phase 1 — Review (4 cleanup agents in parallel)
Launch 4 independent review agents via the Agent tool, all in a
single message so they run concurrently. Pass each agent the diff and one of
the four angles below. Each returns its findings with file, line, a
one-line summary, and the concrete cost (what is duplicated, wasted, or
harder to maintain).
Reuse
Flag new code that re-implements something the codebase already has — Grep shared/utility modules and files adjacent to the change, and name the existing helper to call instead.
Simplification
Flag unnecessary complexity the diff adds: redundant or derivable state, copy-paste with slight variation, deep nesting, dead code left behind. Name the simpler form that does the same job.
Efficiency
Flag wasted work the diff introduces: redundant computation or repeated I/O, independent operations run sequentially, blocking work added to startup or hot paths. Also flag long-lived objects built from closures or captured environments — they keep the entire enclosing scope alive for the object's lifetime (a memory leak when that scope holds large values); prefer a class/struct that copies only the fields it needs. Name the cheaper alternative.
Altitude
Check that each change is implemented at the right depth, not as a fragile bandaid. Special cases layered on shared infrastructure are a sign the fix isn't deep enough — prefer generalizing the underlying mechanism over adding special cases.
Phase 2 — Apply the fixes
Wait for all four agents to complete, dedup findings that point at the same line or mechanism, and fix each remaining one directly. Skip any finding whose fix would change intended behavior, require changes well outside the reviewed diff, or that you judge to be a false positive — note the skip rather than arguing with it. Finish with a brief summary of what was fixed and what was skipped (or confirm the code was already clean).
Lint clean (0 issues), all affected tests green, cross-compiles on linux/windows. Cleanup complete.
/simplify summary
Four cleanup agents (reuse, simplification, efficiency, altitude) reviewed the session-liveness diff. Two findings were worth fixing:
Fixed:
- Efficiency — redundant process reads in
proclive.ResolveOwner(proclive.go). The walk calledprocStat(pid)for the parent, thenprocStat(ppid)again on the next iteration — reading each ancestor's/proc/sysctl entry twice. Restructured to read each ancestor exactly once (procStatalready returns parent + name + start together), roughly halving the syscalls on the per-turn hot path. Behavior-preserving. - Altitude — duplicated session-end sequence (
lifecycle.go,session_finalize.go).finalizeExitedSessionsreplicated themarkSessionEnded+CondenseAndMarkFullyCondensedpair that theSessionStophook already runs. Extracted a sharedendSessionNow(ctx, event, sessionID)inlifecycle.go; both the hook handler and the exited-session sweep now call it, so they can't drift (and the issue-#591 zombie-session rationale now lives in one place).
Skipped (with reasons):
Liveness.String()case+default both return "unknown" — not real redundancy: the explicitLivenessUnknowncase is required by theexhaustivelinter, and thedefaultis required to handle out-of-range values. Both are structurally necessary.- Move the sweep into
ListSessionStates(altitude) — would make a read/list path perform git writes (condensation) as a side effect; that's a behavioral change in the wrong direction, and the status+doctor placement was a deliberate design decision. - Consolidate the "exited" label into a
State.DisplayStatus()(altitude) — the predicate is already a single method (OwnerExited()); a full display-status enum would reach outside the diff into the pre-existing active/stale/idle/ended labeling. Out of scope. - Caching
os.Hostname/bootID/owner-resolution, batching the twoMutateSessionStateround-trips (efficiency) — micro-optimizations on rare/cheap paths that trade clarity for negligible gains.
Reuse review found nothing to change — the code already reuses markSessionEnded, CondenseAndMarkFullyCondensed, and the shared testutil/setupAttachTestRepo helpers, and proclive is correctly a stdlib-only leaf.
Changes remain uncommitted on soph/session-liveness-pid.
Full CI suite passed (exit 0). Done.
Summary
Four meaningful, dependency-ordered commits (each builds independently) on soph/session-liveness-pid:
proclive: add process-liveness packagesession: record owning process and detect exited sessionsstrategy: capture session owner at each turn startstatus, doctor: finalize sessions whose agent has exited
Codex re-review (second pass) confirmed the earlier fixes held and found 4 new issues — all fixed and folded into the relevant commits:
| # | Issue | Fix |
|---|---|---|
| 1 | captureSessionOwner left a stale (dead) Owner when resolution failed → could finalize a now-live session | Clear Owner first, set only on success (commit 3) |
| 2 | TOCTOU race: sweep acted on a stale list snapshot, ending a session a concurrent turn could have revived | Threaded an optional guard through markSessionEnded/endSessionNow; the sweep re-checks OwnerExited() on the freshly-loaded state under the lock (commit 4) |
| 3 | Local-dev go run wrapper recorded go as owner (exits immediately → false finalize in dev) | Added go to the transient-skip set (commit 1) |
| 4 | Check fell through to Alive when host/boot lookups failed | Fail closed: unverifiable host/boot → Unknown (commit 1) |
Added regression tests for the under-lock revalidation (TestFinalizeExitedSessions_RevalidatesUnderLock) and the go-transient case.
Verification: each commit compiles; mise run lint → 0 issues; mise run test:ci green (unit + integration + Vogon canary + external-agent canary).
Codex's one remaining observation (#3's alternative) was to change scripts/entire-dev to build-and-exec instead of go run — I addressed it on the proclive side instead (skipping go), which fixes it without touching the dev harness.
Nothing is pushed. Want me to push the branch and open a PR?
Done.
Pushed soph/session-liveness-pid (force-with-lease, replacing the stale already-merged resume-picker commits) and opened PR #1488: https://github.com/entireio/cli/pull/1488
The PR contains the four logical commits, a description of the problem/approach/testing, and notes the out-of-scope follow-ups (Windows liveness, session list labeling). CI will run on it now — let me know if you'd like me to watch the run or address anything that comes back.
Note: the push also synced the entire/checkpoints/v1 branch (this repo's own checkpoint data) to its remote, as expected from the pre-push hook.
CI lint now passes. Here's everything I did:
PR review comments (PR #1488)
Two bots reviewed; all findings fixed:
- Cursor Bugbot — JSON status skips finalize (Medium):
entire status --jsonlisted sessions and labeled themexitedbut never ran the sweep, leaving them orphaned ACTIVE on disk. →runStatusJSONnow callsfinalizeExitedSessionslike the human path. - Cursor + Copilot —
FullyCondensedset optimistically (Low): the sweep unconditionally setFullyCondensed=truein memory even though condense is fail-open. → It now reloads the session from disk after finalizing, so the snapshot reflects reality. (This also fixed a latent bug where stale in-memoryStepCountcould makedoctorre-flag a just-finalized session.) - Copilot —
ResolveOwnerempty Host on hostname failure: recording an owner with no host disabled the cross-machine guard. → Now fails closed (records no owner ifos.Hostname()fails).
Trail #583 finding (Medium — macOS kern.boottime / NTP)
Real bug: kern.boottime is recomputed on wall-clock steps (NTP), so the boot guard could falsely declare a live session dead. → darwin now records no boot guard and relies on its absolute P_starttime (fixed at process creation, already distinguishes a reused PID across reboots); Linux keeps the NTP-stable boot_id. Resolved on the trail with a note.
Failing lint
proc_linux.go returned unwrapped os.ReadFile/strconv.Atoi errors (wrapcheck). My local mise run lint runs on darwin and never compiled the linux file — I missed it. → Wrapped them, and I now lint both GOOS locally. CI lint is green.
All fixes were folded into the four logical commits (force-pushed), each builds on darwin/linux/windows, lint clean on both platforms, and I posted a summary comment on the PR. Remaining CI (test-core/integration/canary) is running — lint, binary-size, and license checks have already passed. Want me to keep watching the test jobs?