Repository navigation
perf(pool): skip merge-state checks for plain slot listing - #184
jadhavgaurav wants to merge 3 commits into
Conversation
status marked an idle, clean slot available even when get skips it because its HEAD is not merged into the base. Share the reuse proof between List and acquire, report such slots as unlanded, and count them in the pool-full error. Fixes kunchenguid#176 Covers part 1 of kunchenguid#162 Signed-off-by: Gaurav Jadhav <gaurav.vjadhav01@gmail.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
[Medium impact] The PR appears safe to merge; no new actionable issue was found. Reviews (3) · Last reviewed commit: "perf(pool): judge merge state only where..." · Reviewed by Greptile |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92ddab5193
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| branch, err := vcs.DefaultBranchForWorktree(wt.Path) | ||
| if err != nil { | ||
| return false |
There was a problem hiding this comment.
Fall back to the slot's recorded base
In the detached local-repository case covered by TestRelease_WithBaseBranchSurvivesUnresolvableDefault, a slot can be clean and parked on a resolvable explicit base such as develop while DefaultBranchForWorktree errors. This early return never checks wt.BaseBranch, so List newly labels that pristine slot unlanded and says get will not reuse it even though treehouse get with base_branch = "develop" can reuse it; fall back to the recorded/configured base before returning false.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
…`pool.List` callers, `enter` and `return --all`) judged a slot's landed state only against the repository default branch via `landedOnDefaultBase`, ignoring a configured `base_branch`. This let `status` promise a slot "unlanded" (get won't reuse it) while `get` actually resolves the configured base and reuses the same slot - the exact main/develop disagreement Greptile described. Fix: exported `pool.resolveBaseBranch` as `pool.ResolveBaseBranch` (the same resolution `Acquire` already uses) and threaded a `configuredBase` parameter through `pool.List` into `landedOnDefaultBase`, so it checks the configured base when one resolves, falling back to the repository default only when none is configured. Updated all three `pool.List` call sites (`cmd/status.go`, `cmd/enter.go`, `cmd/return_cmd.go`'s `returnHeldWorktrees`) to resolve and pass the same base `get` would use, so `status`/`enter`/`return --all` can no longer disagree with `get`. Updated the ~22 unaffected test call sites to pass `""` (preserving existing behavior) and added `TestList_HonorsConfiguredBaseBranch` reproducing Greptile's exact scenario (slot cut from main, merged only into develop, `base_branch=develop`): verified it fails on the prior logic and passes with the fix. Updated docs/design.md's description of this invariant to match. Verified: `go build ./...` and `go vet ./...` clean; `cmd` package `TestStatus*`, `TestReturn*`, `TestEnter*` all pass; the new regression test passes. Two pre-existing failures in `internal/pool` (`TestAcquireBranchRejectedReferenceTransactionPreservesIgnoredOutput`/`...ChangedSeed`, about git reference-transaction hook output) reproduce identically on the unmodified base commit and are unrelated to this change (environment/git-hook dependent, not caused by this PR)
|
Speaking as Kun's firstmate: thanks for picking up #176, @jadhavgaurav. I approved fork CI for tip One blocker: Main's Windows lane is already close to that limit, but on this morning's main run the same packages finished in about 396s ( |
pool.List now takes no base and never runs the merge-state proof, so `enter` and `return --all`, which treat an unlanded slot exactly like an available one, are back to the git cost they had before. `status` calls the new pool.ListWithMergeState, which resolves the configured base lazily and at most once, and only when a slot is idle and clean. Counted git invocations over `go test ./cmd` on macOS: 8989 on main, 9581 with the previous tip, 9296 now (the remainder is `status`, which needs the proof, and the new tests). Signed-off-by: Gaurav Jadhav <gaurav.vjadhav01@gmail.com>
|
Speaking as Kun's firstmate: thanks for the follow-up, @jadhavgaurav. I read the new commit
Since the cut to |
Intent
Fix #176:
treehouse statusreported an idle, clean slot holding commits that are not merged into the base asavailable, althoughgetskips such a slot.statusnow reports it asunlanded, using the same merge-state proofgetuses (headLandedForReuse) against the same basegetwould resolve, so unproven work is never shown as free and the two commands agree. The pool-full error counts the skipped slots and names the base.Follow-up for the Windows timeout the maintainer reported: the
cmdandinternal/pooltest packages ran into the 10 minute limit. Counting every git invocation over each whole package on macOS, the first version added about 590 calls togo test ./cmd(8989 to 9581) and about 560 to./internal/pool(10552 to 11113), all light read-only calls. That is a 5 to 7 percent increase in invocations, which is real but small next to main's own Windows spread (cmd378 to 552 seconds,internal/pool408 to 587 seconds across three recent main runs). The listing used byenterandreturn --alldoes not need the proof, so it no longer runs it.What Changed
statusreports a clean, idle slot whose HEAD is not proven merged asunlanded(text and--json), with a hint thatgetwill not reuse it.getandstatusshareheadLandedForReuse, and any check that errors counts as unlanded.getresolves (pool.ResolveBaseBranch, including a configuredbase_branch), so a slot merged only into the configured base reads available in both commands.pool.Listkeeps its pre-existing signature and never judges merge state.statuscalls the newpool.ListWithMergeState, which resolves the base lazily, at most once, and only when a slot is idle and clean.enterandreturn --alluse the plain listing, which is also how they always saw such a slot (return --allleaves it alone either way).getcounts unlanded slots and names the base.return --allleaving an unlanded slot alone.docs/design.mdand the README describe the split.Risk Assessment
✅ Low: The change is a well-bounded perf refactor: it extracts the existing merge-state proof into a shared helper, restores pool.List to its pre-PR signature for enter/return --all (which never acted on the unlanded/available distinction anyway), and adds a separate ListWithMergeState with lazy, once-only base resolution for status. The diff matches the stated intent exactly, docs/design.md and README were updated consistently, and the new tests exercise real git repository state (commits, merges, pushes) as independent oracles rather than asserting on source text.
Testing
Built the target-commit binary and exercised it live against a disposable sandbox git repo: simulated an owner-process death leaving a slot with unmerged commits, and confirmed status/get/return --all all treat it as unlanded consistently; also ran the targeted pool and cmd test suites covering List, ListWithMergeState, status, enter, and return --all, all passing. No issues found; sandbox and built binary were torn down after testing.
get(owner-reservation mode) in a background subshell, committed unmerged work on a feature branch inside it, then SIGKILLed the o…treehouse get --leasereturned the pool-full error naming 'main', '1 unlanded', and 'unlanded' explicitly, matching the new message format in pool.go's acquire(…treehouse return --all --forcereturned slot 2 (leased) and reported it skipped the one not-held slot; the subsequentstatusstill showed slot 1 as unlanded with its feature branch intact. See 10_…go test ./internal/pool/... -run TestList -v: TestList_WithoutMergeStateNeverReportsUnlanded and TestListWithMergeState_ResolvesBaseLazilyAndOnce both pass, confirming List's signature reverted…go test ./internal/pool/... -run TestList(includes TestList_HonorsConfiguredBaseBranch) andgo test ./cmd/... -run 'TestStatus|TestReturn|TestEnter'(includes TestStat…Evidence: status shows unlanded slot with explanation
Evidence: get refuses to reuse unlanded slot; pool-full error names it
Evidence: return --all skips the unlanded slot, returns the leased one
Evidence: final status: unlanded slot untouched after return --all
Pipeline
Updates from git push no-mistakes
✅ **Intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
✅ **Review** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
get(owner-reservation mode) in a background subshell, committed unmerged work on a feature branch inside it, then SIGKILLed the o…treehouse get --leasereturned the pool-full error naming 'main', '1 unlanded', and 'unlanded' explicitly, matching the new message format in pool.go's acquire(…treehouse return --all --forcereturned slot 2 (leased) and reported it skipped the one not-held slot; the subsequentstatusstill showed slot 1 as unlanded with its feature branch intact. See 10_…go test ./internal/pool/... -run TestList -v: TestList_WithoutMergeStateNeverReportsUnlanded and TestListWithMergeState_ResolvesBaseLazilyAndOnce both pass, confirming List's signature reverted…go test ./internal/pool/... -run TestList(includes TestList_HonorsConfiguredBaseBranch) andgo test ./cmd/... -run 'TestStatus|TestReturn|TestEnter'(includes TestStat…go build ./...go test ./internal/pool/... -run TestList -vgo test ./cmd/... -run 'TestStatus|TestReturn|TestEnter' -vmanual CLI drive: get (reservation mode) + kill -9 owner process + status/get/return --all against a disposable sandbox git repo and pool✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
Issues
Closes #176