fix(vcs): bound git subprocesses with deadlines and actionable timeout diagnostics - #86
fix(vcs): bound git subprocesses with deadlines and actionable timeout diagnostics#86mvanhorn wants to merge 4 commits into
Conversation
|
Speaking as Kun's firstmate: thanks for this. |
|
Speaking as Kun's firstmate: no new author push. Windows |
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported fallback deadline and linked-worktree lock guidance issues are fixed in the current code. Reviews (3): Last reviewed commit: "no-mistakes(document): update VCS seam c..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Speaking as Kun's firstmate: no new author push (still VISION (per rule):
Please put the merge-content fallback under the same |
|
Pushed 0fb398c. The remaining Greptile blocker was that the merge-base and read-tree helpers still used
|
|
Speaking as Kun's firstmate: thanks for Remaining blocker: |
…ktrees, wrap exec errors
0fb398c to
095ea9d
Compare
mvanhorn
changed the title
|
Speaking as Kun's firstmate: thanks for the re-raise at Inspected:
VISION (per rule):
Greptile is 5/5 on this SHA. I approved CI Require no-mistakes is red: "This PR was not raised through no-mistakes." The body has a |
2 participants
Intent
Bound stalled git commands in treehouse with actionable diagnostics: centralize subprocess construction in internal/git around exec.CommandContext, apply a conservative default deadline to every git command including the merge-ancestor path that bypassed runGit, surface DeadlineExceeded as a timeout-specific error naming the git subcommand and working directory with stale index-lock guidance, and keep IsHeadMergedIntoRef's exit-code-1 unmerged-head meaning intact. Re-pushing through no-mistakes so PR #86 carries the pipeline attestation the maintainer requires to merge.
What Changed
internal/vcs/gitvcsonexec.CommandContextwith a deadline, including the merge-ancestor path that previously bypassedrunGit. Two budgets apply: a 2-minute standard budget for metadata commands and a 30-minute long budget for size- and network-bound commands (fetch,ls-remote,worktree add/remove,checkout,read-tree,clean), overridable viaTREEHOUSE_GIT_TIMEOUTandTREEHOUSE_GIT_LONG_TIMEOUT(an unparseable or non-positive value warns and falls back to the default). A deadline hit now reports the git subcommand, the working directory, stale index-lock / credential-prompt / network guidance, and the env var that raises the budget;IsHeadMergedIntoRef's exit-code-1 unmerged-head meaning is preserved, and non-ExitErrorfailures (includingcmd.WaitDelayexpiry from a background helper holding the output pipe) are wrapped with the same subcommand and directory context.PruneWorktreeAtoperation to thevcs.Backendseam (git implementation plus a documented jj no-op) that prunes stale registrations and additionally unlocks theinitializinglock git leaves on an interruptedworktree add, which plaingit worktree pruneskips. Only that lock reason is cleared, and only while the slot directory is gone, so a user-taken lock is untouched.getnow routes its pre-add prune through it, and on a failedAddWorktreeremoves the partial slot directory it just created and re-prunes, so a timed-out creation no longer wedges the pool on that slot name; the cleanup is skipped when the slot directory already existed.getdocumentation in README and AGENTS.md. Test coverage added ininternal/vcs/gitvcs/gitvcs_test.goandinternal/pool/pool_test.gofor budget selection, timeout error content, lock clearing, and slot reuse after a failed creation.Risk Assessment
✅ Low: The follow-up commit resolves all three actionable round-1 findings with narrowly scoped, documented, and test-pinned changes - split budgets with env overrides, a pre-existence-guarded partial-slot cleanup, and context-carrying error wrapping - while preserving the exit-code-1 unmerged semantics and the origin-unreachable classification the surrounding code depends on.
Testing
Ran the targeted gitvcs and pool test packages for the timeout and partial-worktree-cleanup paths (20 tests, all passing) and then drove the built CLI through four real scenarios: a stalled
git worktree addunder a shortened long budget, recovery of the pool afterwards, a stalledgit merge-base --is-ancestorcompared side by side against a binary built from the base commit, and an unmerged pooled worktree. The timeout diagnostics appear verbatim to the end user with the subcommand, working directory, stale-index-lock guidance, and the right overridable env var for each budget; the base binary hung indefinitely on the merge-ancestor stall while the target binary exits promptly and fails closed. The pool-wedge reported in the previous round no longer reproduces: the interrupted creation's registration is cleared and the freed slot is reused. Scratch repos, shims, and the baseline binary were deleted and the working tree is clean.Evidence: CLI transcript: stalled worktree add times out and the pool recovers
STEP 1 treehouse get --lease with a stalled checkout filter 🌳 Setting up worktree... failed to create worktree: git worktree add --detach .../1/repo main timed out in "/private/tmp/treehouse-e2e/run/repo"; check for a stale index lock (locate it with 'git rev-parse --git-path index.lock'), blocked credential prompts, or network connectivity. Raise TREEHOUSE_GIT_LONG_TIMEOUT if this repository legitimately needs longer --- exit code: 1 (elapsed 13s) STEP 2 git's own view of the pool after the timeout worktree /private/tmp/treehouse-e2e/run/repo HEAD 7f9063936b3ef76bbf862c24acf7dc4848e9cb5b branch refs/heads/main STEP 3 treehouse status 🌳 No worktrees in pool. STEP 4 the stall is over; treehouse get --lease again 🌳 Leased worktree at ~/.treehouse/repo-6a9505/1/repo. --- exit code: 0Evidence: Before/after: stalled merge-ancestor probe (base 5aafb9e vs ee4a57f)
BEFORE base commit 5aafb9e (merge-ancestor used exec.Command, no context, no deadline) ----- BEFORE : treehouse prune --verbose with a stalled merge-ancestor probe STILL RUNNING after 30s -- killed (no deadline; the real stall is 600s) AFTER ee4a57f ----- AFTER : treehouse prune --verbose with a stalled merge-ancestor probe exited after 17s 🌳 Skipped 2 unsafe idle worktrees: cannot verify worktree: 1 cannot prove HEAD is merged into default branch ~/.treehouse/repo-96b6b9/1/repo detail: git merge-base --is-ancestor HEAD refs/heads/main timed out in "..."; check for a stale index lock (locate it with 'git rev-parse --git-path index.lock'), blocked credential prompts, or network connectivity. Raise TREEHOUSE_GIT_TIMEOUT if this repository legitimately needs longerEvidence: CLI transcript: merge-ancestor timeout through get and prune
Evidence: CLI transcript: exit-code-1 still means unmerged, slot preserved
unmerged HEAD in a pooled worktree (merge-base exit code 1) 🌳 No stale worktrees to prune. 🌳 Skipped 1 unsafe idle worktree: unmerged: 1 HEAD not merged into refs/heads/main ~/.treehouse/repo-62341d/1/repo --- prune exit: 0 --- treehouse get must NOT reuse the unmerged slot: 🌳 Leased worktree at ~/.treehouse/repo-62341d/2/repo.Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
🔧 **Rebase** - 2 issues found → auto-fixed ✅
internal/git/git.go- merge conflict rebasing onto origin/maininternal/git/git_test.go- merge conflict rebasing onto origin/main🔧 Fix applied.
✅ Re-checked - no issues remain.
internal/vcs/gitvcs/gitvcs.go:16- The single 2-minute budget now also bounds the only genuinely unbounded git commands:fetch origin(gitvcs.go:205),ls-remote --symref(gitvcs.go:401),worktree add --detach(gitvcs.go:176) andread-tree --reset -u(gitvcs.go:271), with no config/env override. Concrete failing sequence: on a git-lfs or very large repo,treehouse getcallsvcs.AddWorktree(internal/pool/pool.go:235);git worktree addruns the LFS smudge filter and is SIGKILLed at 120s after it has already created<pool>/<n>/<repo>and registered it in.git/worktrees.WriteStateis never reached, so no state entry exists. On the nextget,nextName(internal/pool/pool.go:547) derives the slot name only from state and returns the samen,PruneWorktreescannot clear a registration whose directory still exists, andgit worktree addfails with "already exists" - every subsequentgeton that pool is wedged until a human deletes the directory. A slowfetchsimilarly turnsgetinto a hard "fetch failed" where it previously succeeded slowly. Suggest either a longer (or overridable) budget for the network/checkout commands, or removing the partialwtPathwhenAddWorktreefails so a timeout is self-healing.internal/vcs/gitvcs/gitvcs.go:589- The non-ExitErrorfallback returns the bareerrwith no git subcommand and no working directory, and settingcmd.WaitDelay(gitvcs.go:596) makesexec.ErrWaitDelaynewly reachable on this path. Failing sequence:git status --porcelain --untracked-files=allexits 0 in a repo where a descendant (credential-cache daemon, LFS/filter process, fsmonitor daemon) still holds the inherited stdout pipe; after 250msWaitreturnsErrWaitDelay, the already-copied output is discarded, andIsDirtyfails with the opaque "exec: WaitDelay expired before I/O complete" - the user cannot tell which git command or which worktree produced it, which is exactly the diagnosability this change is meant to add. Wrap this return withfmt.Errorf("git %s in %q: %w", strings.Join(args, " "), dir, err); optionally treatErrWaitDelaywith a zero exit status as success so a lingering descendant does not convert a completed command into a failure.internal/vcs/gitvcs/gitvcs.go:573-runGitRawhas no callers left anywhere in the repo (its only caller,readTree, becamereadTreeContextand callsrunGitRawContextdirectly), and its new doc comment claims it "routes [the ls-tree caller] through the same timeout budget" - a caller it no longer has.go vetwill not flag it, so it will silently rot. Delete it, or drop the misleading comment if it is intentionally kept as an API entry point.internal/vcs/jjvcs/jjvcs.go:551- Noting the remaining boundary, not asking for it in this change: the jj backend still builds subprocesses with plainexec.Commandand no deadline, and jj shells out to git for network work, so the same stall class remains reachable withvcs = "jj". The stated intent scopes this fix to the git backend, so this is deliberate residual scope rather than a gap in the fix.🔧 Fix: separate long git budgets, self-heal partial worktrees, wrap exec errors
2 infos still open:
internal/vcs/gitvcs/gitvcs.go:24- Tradeoff worth a conscious decision, not a blocker:fetchandls-remotenow sit on the 30-minute budget, but a blocked credential prompt - one of the three hang causes the timeout message itself names - is not a slow transfer; git blocks on /dev/tty indefinitely, sotreehouse getstalls for a full 30 minutes before the actionable error appears (internal/pool/pool.go:125 returns "fetch failed"). If interactive credential entry during a pool fetch is not a supported flow, settingGIT_TERMINAL_PROMPT=0on the command environment ingitCommandContextwould make that class fail in seconds while genuinely slow transfers keep the long budget. Leaving it as-is is defensible since the command is still bounded and the override env var is documented.internal/pool/pool.go:559- Acknowledging the deliberate limits of the new self-heal, no action needed: cleanup runs only when the process reaches the error return and only whenslotDirIsNew. Two paths still leave<pool>/<n>behind - a SIGINT/SIGKILL duringgit worktree add(no deferred cleanup runs), and aRemoveAllthat fails because a descendant the context kill did not terminate (git-lfs filter-process, notably on Windows where open files block deletion) still holds files. Both are pre-existing rather than introduced, both now emit a warning naming the slot path, and the resultinggit worktree addfailure quotes the conflicting path, so recovery is discoverable. TheslotDirIsNewguard correctly prioritizes never deleting pre-existing content over always self-healing.🔧 **Test** - 1 issue found → auto-fixed ✅
internal/pool/pool.go:559- Timing outgit worktree addleaves git's registration locked, and the new self-heal cannot clear it, so the pool is permanently wedged. Repro (full transcript in pool-wedged-after-worktree-add-timeout.txt): a repo with a stalling required smudge filter,TREEHOUSE_GIT_LONG_TIMEOUT=10s treehouse get --lease-> the expected actionable timeout error;git worktree listthen shows the slot registered andlocked, because git locks a worktree while checking it out and only unlocks on success. cleanupPartialWorktree removes the slot directory and calls vcs.PruneWorktrees, butgit worktree pruneskips locked entries, so nextName hands out the same slot name and every subsequenttreehouse getdies with "is a missing but locked worktree" even after the stall is gone.treehouse statusreports an empty pool,treehouse prune --yesandtreehouse prune --prune-orphans --yesboth report nothing to prune; only rawgit worktree unlock+git worktree prunerestores the pool. This is the exact case AGENTS.md says the self-heal covers ("A failed vcs.AddWorktree ... (timeout, interrupted checkout, full disk)"), and the new timeout makes it a routine outcome rather than a rare interrupt. Deciding the fix (unlock before pruning, or route cleanup throughgit worktree remove --force) is a product change outside this test phase.go test ./internal/vcs/gitvcs/ -run 'TestRunGit|TestGitCommandTimeoutFor|TestGitTimeoutError|TestIsHeadMergedIntoRef|TestIsHeadContentMergedIntoRefContextReportsTimeout' -v(20 tests, all pass)go test ./internal/pool/ -run 'TestCleanupPartialWorktreeRemovesSlotAndRegistration|TestAcquireReusesSlotNameAfterFailedWorktreeCreation|TestAcquireKeepsPreexistingSlotContentWhenCreationFails' -v(3 tests, all pass)Manual before/after: built the base commit (5aafb9e) viagit archiveand the target commit, rantreehouse get --leasein a repo whose required smudge filter sleeps forever; base hung until an external 40s cap killed it, target failed atTREEHOUSE_GIT_LONG_TIMEOUT=10swith the actionable diagnosticManual metadata-budget check:git config core.fsmonitor <hanging hook>thenTREEHOUSE_GIT_TIMEOUT=5s treehouse prune -v— bounded at 5s, classified "cannot verify", detail namesgit status --porcelain --untracked-files=all, the worktree dir, andTREEHOUSE_GIT_TIMEOUTManual acquire check:TREEHOUSE_GIT_TIMEOUT=5s TREEHOUSE_GIT_LONG_TIMEOUT=20s treehouse get --leasewith a stalled fsmonitor — unverifiable slot skipped (fail closed), whole command boundedManual override validation:TREEHOUSE_GIT_TIMEOUT=banana treehouse statuswarns once and falls back to 2m0sManual unmerged-head check: worktree HEAD carrying a commit not on main,treehouse pruneclassifies itunmerged(merge-base exit 1 semantics intact)Manual defect repro:TREEHOUSE_GIT_LONG_TIMEOUT=10s treehouse get --lease->git worktree list(locked) -> repeatedtreehouse get --lease,treehouse status,treehouse prune --yes,treehouse prune --prune-orphans --yes, thengit worktree unlock+git worktree prunegrep -n 'CombinedOutput()|.Output()|.Run()|.Start()' internal/vcs/gitvcs/gitvcs.go— all three execution sites go throughgitCommandContext🔧 Fix: clear git's initializing lock when worktree creation fails
✅ Re-checked - no issues remain.
go build -o /tmp/treehouse-e2e/treehouse .go test ./internal/vcs/gitvcs/ -run 'Timeout|Git|PruneWorktreeAt|Merged' -v(16 tests, all pass)go test ./internal/pool/ -run 'PartialWorktree|FailedWorktreeCreation|PreexistingSlot|InitializingLock' -v(4 tests, all pass)Manual E2E: repo with a hanging required smudge filter,TREEHOUSE_GIT_LONG_TIMEOUT=10s treehouse get --lease-> actionable timeout in 13sManual E2E: post-timeout recovery -git worktree list --porcelain,treehouse status, thentreehouse get --leasereuses slot 1 and creates slot 2 (round-1 wedge regression check)Manual E2E:treehouse return+ re-getwith orphaned filter processes still alive (lingering processes terminated, slot reused)Manual E2E:gitshim stalling only onmerge-base --is-ancestor,TREEHOUSE_GIT_TIMEOUT=8s treehouse get --lease(fails closed, skips the slot) andtreehouse prune --verbose(surfaces the timeout detail)Manual E2E before/after: same stalled merge-ancestor probe against a binary built from base commit 5aafb9e (still running after 30s, killed) vs. ee4a57f (exits in 17s with the diagnostic)Manual E2E: unmerged commit in a pooled worktree ->treehouse prune --verboseclassifies itunmerged,treehouse getrefuses to reuse itgrep -rn 'exec\.Command' internal/vcs/gitvcs/gitvcs.go internal/vcs/vcs.go internal/pool cmd(singleexec.CommandContextcall site)✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.