fix(pr-autotrack): harden daemon PR-branch auto-tracking#377
Conversation
Address 7 adversarially-confirmed findings in daemon PR-branch auto-tracking (adversarial review on #364/#367): 1. [major] Failed discovery is now distinguishable from zero open PRs. discover_open_prs returns Result; a git/gh command failure surfaces as Err and poll_project skips the whole reconcile cycle (logging outcome=error) instead of reconciling against an empty discovery and mass-untracking every managed PR. 2. [major] Interrupted track + advanced head no longer wedges forever. prepare_pr_worktree drops the erroring show-ref guard and resets the collision-proof synthetic branch with `worktree add -B`, adopting an orphan left by a mid-track crash. Orphan worktrees are swept on each (complete) reconcile so stale checkouts don't leak. 3. [major] remove_tracked_branch_store now serializes on the branch-add lock (matching prepare/gc) so it can't race a concurrent `branch add` and clobber or resurrect a branch-meta entry. 4. [minor] Per-cycle new-track cap uses `continue` not `break`, so an already-managed PR whose head advanced is still refreshed even when new tracks are capped. 5. [minor] All spawned git/gh subprocesses set GIT_TERMINAL_PROMPT=0 and GIT_ASKPASS=echo so a credential prompt can't hang the single poll loop. 6. [minor] gh discovery raises --limit and flags the result `partial` when it reaches the limit; partial discoveries suppress removals so PRs beyond the page aren't untracked as closed. 7. [minor] Disabling the setting now tears down managed PR state: the daemon runs a removals-only reconcile once (teardown_disabled_project) and the disable CLI output documents it. Tests: extend daemon_suite/pr_autotrack_test.rs and pr_autotrack/tests.rs covering failed discovery, the wedged-track recovery, the cap head-update starvation, disable teardown, the branch-add lock, and partial-discovery removal suppression. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52b306ee72
ℹ️ 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".
| let Some(_lock) = acquire_branch_add_lock_blocking(tracedecay_dir) else { | ||
| return false; |
There was a problem hiding this comment.
When the branch-add lock is held longer than these retries (for example, a user is running branch add while the daemon is closing/refreshing a PR or doing disabled teardown), this returns false without removing branch-meta.json or the DB. The PR autotrack callers ignore that false and reconcile_project still removes the managed state/worktree/ref, so no later cycle retries and the stale branch store remains visible indefinitely; keep the managed state until removal succeeds or propagate a distinct contention result.
Useful? React with 👍 / 👎.
1 participant
Fixes 7 adversarially-confirmed findings in daemon PR-branch auto-tracking (from the adversarial review of #364 / #367). Each has a regression test.
Findings & fixes
discover_open_prsnow returnsResult. Agit/ghcommand failure surfaces asErr, andpoll_projectskips the entire reconcile cycle (logsaction=poll outcome=error) instead of reconciling against an empty discovery. Untrack happens only on positive evidence a PR closed.prepare_pr_worktreedrops the erroringshow-refguard and adopts/resets the collision-proof synthetic branch withgit worktree add -B. A stale-worktree sweep on each (complete) reconcile removes leakedpr-worktrees/pr-<N>checkouts.remove_tracked_branch_storemutatedbranch-meta.jsonwithout the branch-add lock. It now serializes on the same lockprepare_branch_tracking_in_layoutandgc_dead_branch_storeshold; skips (returns false) on sustained contention so the daemon retries.breakskipped head-SHA updates for managed PRs. Changed tocontinue— the cap bounds only new tracks; refreshes always run.GIT_TERMINAL_PROMPT=0on daemon git/gh subprocesses. Every spawnedgit/ghcommand now setsGIT_TERMINAL_PROMPT=0andGIT_ASKPASS=echo, so a credential prompt can't hang the single sequential poll loop.gh --limittruncation treated PR refactor: split tracedecay.rs into focused submodules #201+ as closed. Limit raised and truncation detected (count == limit → partial); apartialdiscovery suppresses removals so unlisted-but-open PRs aren't untracked.teardown_disabled_project) when the feature is off but managed entries remain; thebranch autotrack disableoutput documents it.Tests
Extended
tests/daemon_suite/pr_autotrack_test.rsandsrc/daemon/pr_autotrack/tests.rs:failed_discovery_returns_error_and_never_mass_untracksinterrupted_track_with_advanced_head_recovers_instead_of_wedgingcap_does_not_starve_head_updates_for_managed_prsteardown_removes_all_managed_state_on_disableremove_tracked_branch_store_respects_branch_add_lockpartial_discovery_suppresses_removals,gh_pr_list_flags_partial_when_result_reaches_limitGates
cargo fmt --check,cargo clippy --workspace --all-targets --locked -- -D warnings, andnextestdaemon_suite + branch/config lib tests all green (full lib suite running).🤖 Generated with Claude Code