Repository navigation
Conversation
Keep recovery and state healing locked, then inspect captured entries without holding up lifecycle state changes. Read each process cwd once per listing, preserve untracked-file detection with cheaper Git output, and report failed cleanliness checks as unverified.
…fer unverified cleanliness
PabloSzx
changed the title
Owner
|
Speaking as Kun's firstmate: thanks, @PabloSzx. I skimmed the diff (no workflow changes, and the only new subprocess is a git wrapper used by a test) and approved fork CI on |
Keep batched process inspection outside the pool lock and validate each slot's marker and registration backlink inside its locked VCS section. Preserve upstream damaged-slot overrides, cwd filtering, HeldOnlyByCwd, and quarantine recovery without repeating recovery for each slot. Report failed batch enumeration with one warning and empty process lists, while retaining upstream status precedence. Preserve the boolean Git dirty check flags and optional-index-lock suppression. Retain upstream pre-checkout acquisition reservations, invalid-marker destroy refusals, same-path registration healing, reftable HEAD transactions, installer changes, releases, and valid-backlink fixtures.
PabloSzx
changed the title
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.
What Changed
treehouse status(pool.List) now runs recovery, healing, and the state write under the pool lock, then releases the lock. It scans processes once for the whole pool using the newprocess.FindProcessesInWorktrees, which reads each process cwd once and still matches symlinked and nested roots. If that scan fails,statusprints one pool-wide warning and marks the affected slotsunverified.withPoolLock, which skips the recovery pass). Inside it,statusre-reads the slot's state entry and checks the marker, branch, and dirty state. Slots that were removed, replaced, or markedDestroyingafter the snapshot are left out of the listing. A slot that only the batch scan would markin-useis scanned again under the lock, so a short-lived Git run from another treehouse command doesn't show up as an occupant.IsDirtynow runsstatus --no-optional-locks --untracked-files=normal --no-renames, so it doesn't write the index or takeindex.lockduring observation. README anddocs/design.mddescribe the new locking behavior. New tests cover status classification, the Unix concurrency cases, batched process detection, and the dirty check.Risk Assessment
✅ Low: I traced the current code for the races raised in earlier rounds. The round-2 fix (re-read state under the per-slot lock) and the round-3 fix (re-scan processes under that lock when they alone would mark a slot in-use) both close their sequences, the batched process matching keeps the old single-path semantics, and the dirty-check flags only lower the risk of locking the index, so the only remaining finding is documentation drift.
Testing
I tested both builds through the CLI in a disposable pool with its own HOME and TREEHOUSE_ROOT, and tore it down afterwards. On a fixture covering leased, dirty (nested untracked file, staged rename, tracked edit), in-use through a symlinked path, you're-here and damaged slots, both builds produced byte-identical
status --json. HEAD'sstatuswas about 37% faster: roughly 815 ms against 1290 ms. HEAD left a stale slot index untouched, while base rewrote it on every run. To show that, I had to unset GIT_OPTIONAL_LOCKS, because the harness exports it as 0 and that hides the difference. With four agents pollingstatus, HEAD's get/return cycles never failed and never skipped a slot. No idle slot was reported in-use and there were no index.lock errors.getlatency fell from 5–23 s to about 0.9 s. When I destroyed a slot during an in-flightstatus, that run omitted the slot instead of reporting it damaged. A slot leased during an in-flightstatuswas reported leased with its lease id when the lease landed early; in the latest-timed run it was reported available. Two timing-dependent paths could not be driven reliably through the CLI: a process that exits between the batched scan and the locked re-scan, and a failed re-scan that should report unverified. I covered those with the targeted unit tests, which pass along with the other listing, process-matching, dirty-check and cmd status tests and the Windows cross-build. Because those two paths were not driven live, that scenario is recorded as untested. Nothing failed. This is a CLI-only change, so the evidence is CLI transcripts, not screenshots.treehouse statuson a pool with leased, dirty (nested untracked file with showUntrackedFiles=no, staged rename only, tracked edit), in-use (process reached via the /tmp symlink), you're-here…status --jsonfrom head and base is IDENTICAL, with GIT_OPTIONAL_LOCKS both set and unsettreehouse statuson a 15-slot pool returns faster than the base buildtreehouse statuson a slot whose index is stale (core.untrackedCache=true, optional locks enabled) does not rewrite the index or take index.lock, and still reports the slot availablestatus, another agent'sget --lease/returncycles always succeed, reuse the available slot without growing the pool, and see no index.lock errorsstatusobservers (and concurrent get/return) never make an idle slot read in-use from another command's short-lived git childget --leaselatency under continuous status polling dropsstatusrun is in flight is omitted from that listing, not reported damagedstatusrun is in flight is reported from fresh state (leased, with its lease id)Evidence: Status classification parity, base vs head (table + JSON diff)
Evidence: Stale index left unwritten by head status (base rewrites it)
Evidence: status latency, base vs head
Evidence: Concurrent status polling vs get/return: correctness and latency
HEAD 64a7cb2: 10x(get --lease + return) wall=19.5s; get latencies ms:[ 931 919 1113 903 917 844 965 858 1034 1381 ] BASE bf61797: 10x(get --lease + return) wall=219.2s; get latencies ms:[ 22966 7254 6433 11334 6201 5750 7758 5221 4964 5386 ]Evidence: Destroy/lease racing an in-flight status
Pipeline
Updates from git push no-mistakes
⏭️ **intent** - skipped
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
internal/pool/pool.go:1254-inspectListedWorktreestakes the lock again for each slot and reads a fresh marker there, but it still classifies the slot from thewtcaptured in the earlierWithStateLocksnapshot. That snapshot suppliesDestroying,Leased/LeaseID/LeaseHolder, the owner fields used byownerAlive(wt), andRecoveryError. Lifecycle operations can now run between the snapshot and a slot's locked section, so that section can mix fresh marker and VCS facts with stale membership and reservation facts. Before this change, List held one lock for the whole pass, so this could not happen. Example:treehouse statuscaptures slot X, and while it scans processes or runs Git on earlier slots,treehouse prune --yesreserves X and removes it under the lock (prune.go:495-562), dropping the entry. When X's locked section runs,WorktreeBackendNameCheckedfails on the missing path, sostatusreports X asdamagedwith marker-inspection guidance even though X is no longer in the pool.enter Xwould also accept it and try to spawn a shell in a missing directory. The same stale data makes a slot leased bytreehouse leaseorget --leaseduring the listing readavailable, which is exactly the sequenceTestListAllowsLeaseDuringProcessInspectiondrives; that test only checks that the state is not overwritten. Fix at the boundary the change already added: inside the per-slotwithPoolLock,ReadStateonce (without healing), look up the entry by name and path, skip it if it is gone orDestroying, and classify from the fresh entry. This costs one small file read per slot, which is cheap next togit status, and keeps recovery and state writes in the singleWithStateLocksection.🔧 Fix applied.
2 warnings still open:
internal/pool/pool.go:1254-inspectListedWorktreestakes the lock again for each slot and reads a fresh marker there, but it still classifies the slot from thewtcaptured in the earlierWithStateLocksnapshot. That snapshot suppliesDestroying,Leased/LeaseID/LeaseHolder, the owner fields used byownerAlive(wt), andRecoveryError. Lifecycle operations can now run between the snapshot and a slot's locked section, so that section can mix fresh marker and VCS facts with stale membership and reservation facts. Before this change, List held one lock for the whole pass, so this could not happen. Example:treehouse statuscaptures slot X, and while it scans processes or runs Git on earlier slots,treehouse prune --yesreserves X and removes it under the lock (prune.go:495-562), dropping the entry. When X's locked section runs,WorktreeBackendNameCheckedfails on the missing path, sostatusreports X asdamagedwith marker-inspection guidance even though X is no longer in the pool.enter Xwould also accept it and try to spawn a shell in a missing directory. The same stale data makes a slot leased bytreehouse leaseorget --leaseduring the listing readavailable, which is exactly the sequenceTestListAllowsLeaseDuringProcessInspectiondrives; that test only checks that the state is not overwritten. Fix at the boundary the change already added: inside the per-slotwithPoolLock,ReadStateonce (without healing), look up the entry by name and path, skip it if it is gone orDestroying, and classify from the fresh entry. This costs one small file read per slot, which is cheap next togit status, and keeps recovery and state writes in the singleWithStateLocksection.internal/pool/pool.go:1238-statuscan report an idle slot asin-usebecause its process scan catches Git processes that another treehouse command runs in that slot. The batched scan (findProcessesInWorktrees, pool.go:1216) now runs outside the pool lock, and List later uses its results as-is under each slot's lock (pool.go:1238 →Processes: procsat 1269 →len(procs) > 0→StatusInUseat 1292). Other commands still run Git in the slot directory while holding that lock: a secondList's per-slotCheckedOutBranch/IsDirty(cmd.Dir = slot),return's reset/detach, andAcquire's reset. Example: agents A and B both runtreehouse status, which is common since agents poll it. B holds the lock while itsgit statusruns in available slot X. A's unlocked scan sees B'sgitchild with its cwd in X. When A gets X's lock, B's git has already exited, yet A reports X asin-usewith a deadgitPID. In the same way, a concurrenttreehouse return Xmakes A report the just-returned slotin-usewith the agent PIDs that return killed or its own git PIDs. Before this change, List held one lock for the scan and the classification, so neither sequence could happen. It also changes behavior:return --all(return_cmd.go:263,returnableStatus) then targets the idle slot and can kill the other observer's git. This defect dates from the original commit 4e982c2. The round-2 fix moved the Git reads back under a per-slot lock but left the scan results unrechecked, so it did not close it. Smallest fix, which keeps the scan outside the lock: inside the per-slotwithPoolLock(pool.go:1247), before classifying, drop scanned processes that are no longer alive (for example withprocess.StartedAt(pid)). Any Git child that ran under another lock holder has exited by the time this section holds the lock, so the class is closed with no new state.🔧 Fix applied.
1 info still open:
docs/design.md:17- Thepool.Listentry in design.md (and the comment oninspectListedWorktreesat internal/pool/pool.go:1202-1208) still describes the design from before the round-2 and round-3 fixes. It says only that marker validation and VCS reads share a per-slot lock section. It does not record two invariants those rounds added. (1) Each slot's lease, owner reservation,Destroying, andRecoveryErrorare re-read from fresh state under that lock (pool.go:1251-1263), and a slot that was removed or reserved for destruction is left out. (2) A slot that would bein-useonly because the batched scan found processes in it is re-scanned under its lock (pool.go:1264-1275), because the unlocked batch can catch Git children that another lock holder ran in the slot. A failed re-scan reports the slotunverified. AGENTS.md sends future agents to design.md before they change an area, so without this rationale someone could 'optimize away' the per-slot ReadState or the re-scan and reopen both races. Fix: add one sentence for each invariant to the design.md entry and to the function comment. Behavior does not change.✅ **Test** - passed
✅ No issues found.
treehouse statuson a pool with leased, dirty (nested untracked file with showUntrackedFiles=no, staged rename only, tracked edit), in-use (process reached via the /tmp symlink), you're-here…status --jsonfrom head and base is IDENTICAL, with GIT_OPTIONAL_LOCKS both set and unsettreehouse statuson a 15-slot pool returns faster than the base buildtreehouse statuson a slot whose index is stale (core.untrackedCache=true, optional locks enabled) does not rewrite the index or take index.lock, and still reports the slot availablestatus, another agent'sget --lease/returncycles always succeed, reuse the available slot without growing the pool, and see no index.lock errorsstatusobservers (and concurrent get/return) never make an idle slot read in-use from another command's short-lived git childget --leaselatency under continuous status polling dropsstatusrun is in flight is omitted from that listing, not reported damagedstatusrun is in flight is reported from fresh state (leased, with its lease id)Builttreehouse-base(bf61797) andtreehouse-head(64a7cb2) into a temp dir. Gave them a separate HOME, TREEHOUSE_ROOT and global git config, a bare origin, and a fresh repoMixed fixture: leased; nested untracked file withstatus.showUntrackedFiles=no;sleeprunning in the slot via the /tmp→/private/tmp symlink; caller cwd; broken.gitmarker; staged rename only; tracked edit. Rantreehouse statusandstatus --jsonwith both builds and diffed the JSON with jqGave a slot's tracked files an old mtime withcore.untrackedCache=trueand GIT_OPTIONAL_LOCKS unset. Compared the slot's index sha before and aftertreehouse statusfor the base and head buildsRan 8 interleavedtreehouse statusruns, 3 rounds per build, on a 15-slot poolFour backgroundstatus --jsonloops plus 20get --lease --no-fetch/returncycles. Checked lifecycle failures, which slot get handed out, pool size, nonzero status exits, idle slots reported in-use, and index.lock errorsSame contention with 10 cycles, timing each get and each status run for both buildsRantreehouse destroy <slot> --yesandtreehouse lease 12at staggered delays while astatus --jsonwas running, and recorded how that status reported the slotgo test -count=1 ./internal/pool/ -run 'TestList|TestStatus' -vgo test -count=1 ./internal/process/ ./internal/vcs/gitvcs/ -run 'Dirty|FindProcesses|Worktree'go test -count=1 ./cmd/ -run StatusGOOS=windows go build ./...✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.