Repository navigation
feat(pool): free leases whose declared holder process has exited - #188
Open
m-moravcik wants to merge 8 commits into
Open
m-moravcik wants to merge 8 commits into
m-moravcik wants to merge 8 commits into
Conversation
get --lease --lease-holder-pid (or TREEHOUSE_LEASE_HOLDER_PID) records the holder's PID and a stable start stamp. The holder must be an ancestor of the command, never treehouse itself. Every pool command judges it under the state lock; a holder proven gone (free PID or a different start) has its lease freed only when the slot passes the recovered-entry safety check, otherwise the lease is kept with a reason. Live, undeclared and unreadable holders keep the lease; Windows refuses the option.
[Medium impact] The PR appears safe to merge, with no outstanding findings. Reviews (3) · Last reviewed commit: "no-mistakes(ci): ci-1 (Greptile P2, "Pro..." · Reviewed by Greptile |
… Both bugs were reproduced first: each new test failed before its fix and passes after it. ci-1. Rule: a `get` that is still running its post_create hooks must keep its slot even if the declared holder exits. Only one place creates a lease with a holder: `markAcquired`, which both the reuse and the fresh-slot paths go through. The `lease <name>` takeover never declares a holder. Fix: in internal/pool/pool.go, `markAcquired` keeps the acquiring command's owner reservation (`reserveOwner`) on a lease with a holder. After `hooks.Run`, acquire calls the new `endSetupReservation`, which clears that reservation under the state lock, but only while the entry still has this lease ID and this process's PID. The existing `recoverSafeEntry` already refuses a slot with a live owner (`ownerAlive`), so recovered.go is unchanged. If `get` crashes, the reservation names a dead process, `healState` clears it, and the lease does not stay pinned. Regression test `TestHolderExitingWhilePostCreateHooksRunKeepsTheLease` in internal/pool/holder_test.go: a hook leaves the worktree and blocks, and meanwhile the holder is replaced by one that has exited. The test then checks that `List` still shows the lease, that a second `get` cannot take the slot, that the reservation is gone after the hooks finish, and that the next command frees the slot. ci-2. Rule: an invalid `--lease-holder-pid` (non-positive, self, exited, not an ancestor, or on Windows) must be refused before any write to the pool or the git excludes. Fix: in cmd/get.go, `getRunE` now calls `process.VerifyHolder` in the flag-validation block, before repo resolution and `EnsureExcluded`. This replaces the separate `<= 0` check, which `VerifyHolder` already covers. The check in acquire stays and still records the start time it proves. `TestLeaseHolderPIDIsRefusedWhenItCannotBeHonored` now runs every case with `--root .`, adds "exited" and "negative" cases, and asserts that `<repo>/.treehouse`, its `.gitignore` and `.git/info/exclude` are untouched. Docs: the README holder paragraph and the docs/design.md holder entry now describe the earlier verification and the reservation held while hooks run. No em dashes were added. Verification: `make lint` is clean, `make test` passes in every package, `GOOS=windows go build ./...` succeeds, and the two new tests passed on 5 and 3 repeated runs
…ide"): fixed only as the user decided, with a one-sentence documentation change. Rule violated: AGENTS.md says to use gopsutil for process data. But `internal/process/identity_linux.go` reads `/proc/<pid>/stat` start ticks directly, and `internal/process/identity_unix.go` calls `syscall.Kill(pid, 0)`, and the guide gave no exception for either. I checked every place in `internal/process/identity_*` that touches native process data: `identity_bsd.go` already uses gopsutil `CreateTime`, and `identity_windows.go` is unsupported. So the only two deviations are the Linux `/proc` read and the Unix `kill(pid, 0)`, and the exception names exactly those. Change: AGENTS.md, in the Windows-compatibility gopsutil bullet, now adds this exception: "Exception: holder identity in `internal/process/identity_*` reads Linux `/proc/<pid>/stat` start ticks and uses `kill(pid, 0)` for existence, because gopsutil's Linux `CreateTime` moves with wall-clock steps (see the holder entry in docs/design.md)." The general "use gopsutil" guidance is unchanged. No code, behavior or tests changed, and no em dash was added. Verification: `make lint` (gofmt and go vet) is clean
Author
|
Could a maintainer please rerun the failed Windows job for this unchanged head ( Failed job: https://github.com/kunchenguid/treehouse/actions/runs/39144330969/job/117755250310 |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
A durable lease has no expiry, so an agent that crashes or forgets
treehouse returnblocks its slot forever. This lets a lease name the long-lived process that holds it. Once that process has provably exited, the next pool command frees the lease, but only if the slot passes the same safety check #159 applies to recovered entries.What changes
treehouse get --lease --lease-holder-pid <pid>records the holder's PID and a start stamp in pool state (lease_holder_pid,lease_holder_start, bothomitempty, no state version bump). It is the only input that records a holder.WithStateLockalready runsrecoverQuarantinedEntries, soget,status,return,lease,enter,pruneanddestroyall run it. That pass now also judges leases with a declared holder. If the holder is proven gone, the slot goes through the unchangedrecoverSafeEntrypredicate: no process in the slot (a shell standing in it counts), no tracked changes (submodules and skip-worktree/assume-unchanged included), HEAD contained in a remote-tracking ref or the slot's base, and on Unix untracked files moved into the retained backup first..worktreeinclude/--include-file, as after areturn.recovery_reasonsays why (its holder process 1234 exited, but ...);statusshows it.get --jsonorstatus --json.Holder identity contract
treehouse get --leaseexits as soon as it prints the path, and harnesses that start a fresh shell per command (Claude Code's Bash tool, for one) make$$just as short-lived. Recording the caller or its parent automatically would free a lease between allocation and first use.--lease-holder-pid,treehouse lease <name>, or state written before this change) is never freed by holder liveness; onlytreehouse returnreleases it.process.VerifyHolderaccepts only an ancestor of the running command, refuses treehouse itself, and reads the start stamp before and after the ancestry walk. Ancestry also refuses a PID already recycled to an unrelated process. It runs before the fetch, so a refusal never touches the pool, and the holder is alive when the lease is written.--lease-holder-pid $$names the shell that runs treehouse, so the lease lives until that shell exits. Agent callers: name the agent process ($PPIDin a per-command tool shell).process.IdentityLiveness). Only a free PID, or one now held by a process with a different start stamp, is gone. A live holder, a holder without a recorded start, an unreadable process table and an unsupported platform all keep the lease.p_starttimeon macOS/BSD (gopsutilCreateTime), and the raw start ticks from/proc/<pid>/staton Linux. I did not reuseprocess.StartedAtbecause gopsutil's LinuxCreateTimeadds a boot time recomputed from the wall clock on every read; over a lease's lifetime a stepped clock would make a live holder look recycled. Existence useskill(pid, 0), so EPERM orhidepidnever reads as gone.Limitations
--lease-holder-pidis refused with an explicit error. A parent PID there is never reparented and can name an unrelated process after reuse, so the holder cannot be verified.treehouse return.HEAD.lockchecks. Like any available slot today, a freed slot whose HEAD is merged may be deleted byprune --yes.Related
#187 adds
prune --return. The two do not depend on each other and merge cleanly in either order. Together,prune --return --yesalso returns a landed slot whose holder exited, provided a remote-tracking ref contains its HEAD (for example the pushed PR branch). Prune's snapshot takes the state lock, which frees the lease first, and the kept seed inventory lets the return remove seeded files. A squash-merged HEAD that no remote-tracking ref contains any more fails the containment check above, so that lease stays held with a reason.Tests
internal/process: stable stamp, missing process, alive / exited / recycled / missing identity,VerifyHolder(parent accepted; self, non-ancestor, exited and non-positive refused), Linux stat parsing with spaces and parentheses in the command name, Windows unsupported.internal/pool: exited holder freed; live holder, no holder, PID without start, unreadable liveness and legacy state stay leased; recycled PID freed; tracked edits and unlanded HEAD kept with a reason; untracked files moved to backup, then freed; busy slot freed only after the resident process exits; seeded file removed by the next reuse; a slot returned and re-leased to a live holder keeps the new lease;--if-lease-idof a reclaimed lease refused; six concurrentget --leaseagainst one reclaimable slot in amax_trees = 1pool hand it out exactly once.cmde2e with the built binary: a shell leases with$$and exits, the nextstatusshows the slot available andget --leasereuses it; live holder and no holder stay leased; an exited holder over a dirty slot is kept with the reason in JSON and human output; refusals leave the pool empty.cf7cbdcon macOS (the pipeline re-ran lint and tests on each later fix commit):make lintandmake testpass (uncached, with a globalcore.hooksPathunset; the default 10-minute package timeout was raised to 30 minutes on a heavily loaded machine);GOOS=windows go build ./...andgo vet ./...pass. With feat(prune): add --return to hand landed worktrees back to the pool #187 merged in as well, the full suite passes, plus an ad-hoc end-to-end check of the composition described above (not part of this PR).Risk Assessment
✅ Low: Auto-freeing fails closed: only a provably exited, explicitly declared holder lets a lease go, and only through the existing recovered-entry safety predicate under the state lock. The recorded human decisions are applied (no env var, no public PID output, seed inventory kept), and every intent case (live, exited, recycled, no PID, unknown) has tests through the real package APIs and the CLI.
Testing
I built the treehouse binary and drove 13 scenarios live, using a disposable bare origin, a clone and an isolated HOME with TREEHOUSE_* vars stripped. Covered: a live holder, an exited holder, no holder, a simulated PID reuse, three unsafe-slot cases with their kept-leased reasons, untracked-file backup, the seed inventory being kept and removed on reuse, six invalid-declaration refusals,
getas the freeing command, and four concurrentget --leaseruns. All passed. I also ran the targeted holder/identity unit and e2e tests andGOOS=windows go build ./..., which passed. CLI transcripts are saved in the evidence dir. The temp env is torn down and the worktree is clean. This is CLI-only, so there are no screenshots.treehouse statusshows the slot available and the lease is clearedget --leasewith no holder PID stays leased across commands untiltreehouse return; JSON output has no lease_holder_pidkept leased: its holder process N exited, but tracked changes...reason (text and JSON recovery_reason)getreuse removes .envgetas the next command frees an exited-holder lease and reuses that slot instead of growing the poolget --leaseruns after three holders exit: four distinct paths, three freed slots reused, state consistentEvidence: Live holder keeps lease; exited holder freed by next command
Evidence: Lease without holder stays leased; JSON has no holder PID
Evidence: Recycled PID with different start time treated as gone
Evidence: Unsafe slots kept leased with reason
Evidence: Untracked backup and seed inventory kept, reuse removes seeded file
Evidence: Invalid holder declarations refused
Evidence: get frees and reuses exited-holder slot
Evidence: Concurrent gets reclaim safely
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
internal/pool/recovered.go:51- Every pool command runs recoverQuarantinedEntries first, inside WithStateLock. Take a script that names the agent with--lease-holder-pidand runstreehouse return --if-lease-id <id> <path>after the agent exits. Thereturncommand's own ValidateReleasePreconditions lock frees the lease first, if the slot is safe. The conditional release then fails withlease precondition failed: worktree ... is not leased(exit 1) and does not reset the worktree. The outcome is harmless: the slot is free, it passed the safety predicate, and the next reuse resets it. This also matches the rule that a command which leaves the world unchanged must not exit 0. But aset -ecleanup step could treat it as a failure. Noted for awareness only; no change required.✅ **Test** - passed
✅ No issues found.
treehouse statusshows the slot available and the lease is clearedget --leasewith no holder PID stays leased across commands untiltreehouse return; JSON output has no lease_holder_pidkept leased: its holder process N exited, but tracked changes...reason (text and JSON recovery_reason)getreuse removes .envgetas the next command frees an exited-holder lease and reuses that slot instead of growing the poolget --leaseruns after three holders exit: four distinct paths, three freed slots reused, state consistentgo build -o $T/bin/treehouse .+ isolated HOME/XDG_CONFIG_HOME, bare origin + clonesh -c 'p=$(treehouse get --lease --no-fetch --lease-holder-pid $$); ...; sleep 4' &thentreehouse statuswhile alive and after exittreehouse get --lease --no-fetch --json(no holder) + repeatedtreehouse status+treehouse returnState file lease_holder_pid rewritten to a live unrelatedsleepPID (original start stamp kept) thentreehouse statusHolder exits after tracked edit / local unpushed commit / leaving a residentsleepin the worktree, thentreehouse statusandstatus --jsonHolder exits leaving untracked notes.txt ->treehouse statusbackup moveget --lease --include-file personal.manifest --lease-holder-pid $$, holder exits,status, then plainget --leasereuse checks .env removalRefusals: non-ancestor PID, free PID, 0, -5, without --lease, non-numericExited per-command-shell holder thentreehouse get --lease --jsonfrees and reuses slot 13 exited-holder leases + 4 concurrenttreehouse get --leasego test ./internal/process/ ./internal/pool/ -run 'Holder|Identity|StartStamp|Liveness'go test ./cmd/ -run Holder -vGOOS=windows go build ./...✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.