Repository navigation
perf(git): only schedule the upstream-ref poll for the repo that has one - #16443
Conversation
📝 WalkthroughWalkthroughThe status-ref watch now exposes binding-change subscriptions and notifies listeners after applying active bindings. The common primary watch tracks polling generations, rejects stale or invalid status-ref pollers, and unsubscribes during teardown. Tests cover repeated binding changes, updated visibility listener counts, disposal cleanup, and asynchronous filesystem reconciliation. Merge Risk: 🔵 Low · up to This change makes upstream-ref polling demand-driven, but an unresolved teardown path may leave a poller running after watcher failure. The PR is mergeable with explicit owner awareness or follow-up on that bounded runtime/resource risk. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the background, bug, fix, performance result, and test updates. However, it does not follow the required template and omits the Visual Proof, Testing, Review, Agent skill upstream boundary, Notes, and Checklist sections. Resolution Add the missing template sections. Include
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4494b0f8-c5fd-428b-b177-256576057bf6
📒 Files selected for processing (3)
src/main/ipc/worktree-git-common-primary-watch.tssrc/main/ipc/worktree-git-common-watch.test.tssrc/main/ipc/worktree-git-status-ref-watch.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/ipc/worktree-git-common-primary-watch.ts (1)
176-176:⚠️ Potential issue | 🟠 MajorSerialize initial poller adoption with binding reconciliation.
The initial
startStatusRefPollingIfSelected()call can overlapsyncStatusRefPolling(). If bindings change during setup, Line 199 can overwrite a poller adopted bysyncStatusRefPolling, or adopt a poller after selection becomes empty. The lost poller remains active after teardown. Route both paths through one reconciliation mechanism, or recheck selection and ownership before assigningstatusRefPollingand unsubscribe the losing poller.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e6b6f09-68e7-4323-988c-1b2cfce4636b
📒 Files selected for processing (1)
src/main/ipc/worktree-git-common-primary-watch.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/ipc/worktree-git-common-primary-watch.ts (1)
46-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShorten the new explanatory comments.
Keep each comment to one short statement of intent.
src/main/ipc/worktree-git-common-primary-watch.ts#L46-L50: reduce the generation ownership explanation to one line.src/main/ipc/worktree-git-common-primary-watch.ts#L89-L92: reduce the adoption rule explanation to one line.src/main/ipc/worktree-git-common-primary-watch.ts#L205-L207: reduce the startup race explanation to one line.As per coding guidelines,
**/*.{ts,tsx,js,jsx}must “DO NOT: be verbose, explain the obvious, walk through the code.”Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 32f9bdbe-828e-41ae-8268-2e9d598a1d67
📥 Commits
Reviewing files that changed from the base of the PR and between 3ca0d5e and 8bddbd9120f4f6f9dbb7ed226a2251f3d03543e1.
📒 Files selected for processing (2)
src/main/ipc/worktree-git-common-primary-watch.tssrc/main/ipc/worktree-git-common-watch.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
| await adoptStatusRefPolling(generation, nextStatusRefPolling) | ||
| backstopPolling = nextBackstop |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Assign the backstop before the awaited adoption.
If watcher teardown occurs during await adoptStatusRefPolling(...), teardown sees backstopPolling === null. Line 218 then stores nextBackstop after teardown, leaving its timer and visibility listener active.
Proposed fix
- await adoptStatusRefPolling(generation, nextStatusRefPolling)
backstopPolling = nextBackstop
+ await adoptStatusRefPolling(generation, nextStatusRefPolling)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await adoptStatusRefPolling(generation, nextStatusRefPolling) | |
| backstopPolling = nextBackstop | |
| backstopPolling = nextBackstop | |
| await adoptStatusRefPolling(generation, nextStatusRefPolling) |
A single global binding means at most one worktree holds a selected upstream ref at a time, so every other repo's 2s poll woke only to stat an empty set. Rebinding is synchronous and in-process, so reacting to it detects a newly selected ref exactly as fast as polling did — the wake-ups were pure waste. Measured at 100 repos with no selected ref: 0.811 -> 0.320 CPU-ms/s idle.
A rebind can flip back while the poller is being constructed. The concurrent unbind sees statusRefPolling still null and correctly does nothing, so without re-reading selection the in-flight build adopts a poller for a repo that no longer holds a ref — reinstating the idle wake-ups this change removes.
…token Startup and each rebind could be mid-build at once, and the startup path adopted its poller unconditionally. A rebind that won the slot first was then overwritten without being unsubscribed, stranding that poller's timer and visibility listener for the process lifetime. The generation names which attempt still owns the slot so every loser tears down what it built. Adds a regression test driving the real binding path; it fails if the poller is dropped without unsubscribing. Reported by CodeRabbit on #16443.
8bddbd9 to
05c21a1
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/ipc/worktree-git-status-ref-watch.ts (1)
77-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShorten the rationale comment.
Keep this explanation to one line where possible.
Proposed revision
-// Why: at most one worktree holds a selected upstream ref at a time, so every -// other repo's ref poll would wake only to stat nothing. Publishing the -// transition lets those pollers stay unscheduled until they actually have a ref, -// with no detection delay — rebinding is an in-process event, not a timer. +// Notify pollers immediately because rebinding is synchronous and timer-free.As per coding guidelines,
**/*.{ts,tsx,js,jsx}: “DO NOT: be verbose; BE CONCISE. 1 LINE if possible.”Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ec371e4-1da7-4779-b218-85af61c6fea4
📒 Files selected for processing (3)
src/main/ipc/worktree-git-common-primary-watch.tssrc/main/ipc/worktree-git-common-watch.test.tssrc/main/ipc/worktree-git-status-ref-watch.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/ipc/worktree-git-common-primary-watch.ts
- src/main/ipc/worktree-git-common-watch.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.
…one (stablyai#16443) * perf(git): only schedule the upstream-ref poll for the repo that has one A single global binding means at most one worktree holds a selected upstream ref at a time, so every other repo's 2s poll woke only to stat an empty set. Rebinding is synchronous and in-process, so reacting to it detects a newly selected ref exactly as fast as polling did — the wake-ups were pure waste. Measured at 100 repos with no selected ref: 0.811 -> 0.320 CPU-ms/s idle. * fix(git-watch): re-read ref selection after building the poller A rebind can flip back while the poller is being constructed. The concurrent unbind sees statusRefPolling still null and correctly does nothing, so without re-reading selection the in-flight build adopts a poller for a repo that no longer holds a ref — reinstating the idle wake-ups this change removes. * fix(git-watch): fence concurrent ref-poller starts with a generation token Startup and each rebind could be mid-build at once, and the startup path adopted its poller unconditionally. A rebind that won the slot first was then overwritten without being unsubscribed, stranding that poller's timer and visibility listener for the process lifetime. The generation names which attempt still owns the slot so every loser tears down what it built. Adds a regression test driving the real binding path; it fails if the poller is dropped without unsubscribing. Reported by CodeRabbit on stablyai#16443.
…one (stablyai#16443) * perf(git): only schedule the upstream-ref poll for the repo that has one A single global binding means at most one worktree holds a selected upstream ref at a time, so every other repo's 2s poll woke only to stat an empty set. Rebinding is synchronous and in-process, so reacting to it detects a newly selected ref exactly as fast as polling did — the wake-ups were pure waste. Measured at 100 repos with no selected ref: 0.811 -> 0.320 CPU-ms/s idle. * fix(git-watch): re-read ref selection after building the poller A rebind can flip back while the poller is being constructed. The concurrent unbind sees statusRefPolling still null and correctly does nothing, so without re-reading selection the in-flight build adopts a poller for a repo that no longer holds a ref — reinstating the idle wake-ups this change removes. * fix(git-watch): fence concurrent ref-poller starts with a generation token Startup and each rebind could be mid-build at once, and the startup path adopted its poller unconditionally. A rebind that won the slot first was then overwritten without being unsubscribed, stranding that poller's timer and visibility listener for the process lifetime. The generation names which attempt still owns the slot so every loser tears down what it built. Adds a regression test driving the real binding path; it fails if the poller is dropped without unsubscribing. Reported by CodeRabbit on stablyai#16443. (cherry picked from commit 5a8b4af)
…one (stablyai#16443) * perf(git): only schedule the upstream-ref poll for the repo that has one A single global binding means at most one worktree holds a selected upstream ref at a time, so every other repo's 2s poll woke only to stat an empty set. Rebinding is synchronous and in-process, so reacting to it detects a newly selected ref exactly as fast as polling did — the wake-ups were pure waste. Measured at 100 repos with no selected ref: 0.811 -> 0.320 CPU-ms/s idle. * fix(git-watch): re-read ref selection after building the poller A rebind can flip back while the poller is being constructed. The concurrent unbind sees statusRefPolling still null and correctly does nothing, so without re-reading selection the in-flight build adopts a poller for a repo that no longer holds a ref — reinstating the idle wake-ups this change removes. * fix(git-watch): fence concurrent ref-poller starts with a generation token Startup and each rebind could be mid-build at once, and the startup path adopted its poller unconditionally. A rebind that won the slot first was then overwritten without being unsubscribed, stranding that poller's timer and visibility listener for the process lifetime. The generation names which attempt still owns the slot so every loser tears down what it built. Adds a regression test driving the real binding path; it fails if the poller is dropped without unsubscribing. Reported by CodeRabbit on stablyai#16443.
Background: what "holding a ref" means
To show ahead/behind for a branch, Orca has to notice when the branch's upstream ref file changes — e.g.
.git/refs/remotes/origin/mainmoving when someone else pushes. So it watches that one file.It only does this for one worktree at a time: the one whose ahead/behind is actually on screen. That's
activeGitStatusRefBinding— a single module-level value inworktree-git-status-ref-watch.ts, set as you move around the UI:applyActiveGitStatusRefBinding()then gives that one worktree a one-element set of paths to watch, and clears it for everyone else:So "repo N holds a ref" just means: N is the one worktree currently being shown, so it has one upstream ref file worth watching. Every other repo's set is empty.
The bug
Every repo scheduled its own 2-second poll over that set — including the repos whose set is empty. Those pollers woke twice a second, allocated a
Set,awaitedPromise.all([]), diffed two empty maps, and went back to sleep. With 100 repos open, 99 of them were doing this forever and could never find anything, because a repo only gets a path when it becomes the active one.The fix
Only schedule the poll for the repo that actually has a path, and start/stop it from the rebinding event rather than from a timer.
This costs nothing in detection latency. Rebinding is synchronous and in-process —
applyBindingToWatches()runs when the active worktree changes — so a newly selected ref starts its poll on the same tick it would previously have been noticed. The wake-ups weren't a safety margin; they were pure waste. The repo that does hold a ref polls exactly as before.Measured
100 repos, none holding a ref, 20s idle window:
The 0.320 is the measurement harness itself, so the poll's own cost effectively goes to zero.
Test changes, and why
vi.waitForfor exactly this reason; this makes the two consistent.Follows #16404. Independent of the metadata poll scheduler.