ADE-69: PR/GitHub subsystem: freshness/ETag correctness, merge-rail bugs, switchBranch stale cleanup, polling cursors, index.lock, decomposition & PR-detail polish#498
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Review limit reached
More reviews will be available in 8 minutes and 42 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (32)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
arul28
force-pushed
the
ade-69-pr-github-subsystem-freshness-etag-correctness-merge-rail-bugs-switchbranch-stale-cleanup-polling-cursors-index-lock-decomposition-pr-detail-polish
branch
from
14da1d3 to
43d8978
Compare
|
@copilot review but do not make fixes |
arul28
force-pushed
the
ade-69-pr-github-subsystem-freshness-etag-correctness-merge-rail-bugs-switchbranch-stale-cleanup-polling-cursors-index-lock-decomposition-pr-detail-polish
branch
from
43d8978 to
a3dd3eb
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function isIndexLockHeldByProcess(lockPath: string): boolean { | ||
| if (activeGitPids.size > 0) return true; | ||
| if (process.platform === "win32") return false; | ||
| try { | ||
| const out = execFileSync("lsof", [lockPath], { | ||
| encoding: "utf8", | ||
| timeout: 2_000, | ||
| stdio: ["ignore", "pipe", "ignore"], | ||
| }); | ||
| return out.trim().split(/\r?\n/).length > 1; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
execFileSync('lsof', …) blocks the event loop for up to 2 seconds
recoverStaleIndexLock is called synchronously inside the git error-recovery path. Introducing a timeout: 2_000 blocking execFileSync here will stall every other pending I/O — IPC messages from the renderer, timers, in-flight promises — for up to 2 seconds whenever the 2-minute age threshold is crossed and activeGitPids is empty.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/services/git/git.ts
Line: 112-125
Comment:
**`execFileSync('lsof', …)` blocks the event loop for up to 2 seconds**
`recoverStaleIndexLock` is called synchronously inside the git error-recovery path. Introducing a `timeout: 2_000` blocking `execFileSync` here will stall every other pending I/O — IPC messages from the renderer, timers, in-flight promises — for up to 2 seconds whenever the 2-minute age threshold is crossed and `activeGitPids` is empty.
How can I resolve this? If you propose a fix, please make it concise.
| const behindBaseBy = status?.behindBaseBy ?? 0; | ||
| if (behindBaseBy > 0) { | ||
| blockers.push({ | ||
| id: "behind", | ||
| label: `The head branch is ${status.behindBaseBy} commit${status.behindBaseBy === 1 ? "" : "s"} behind the base branch.`, | ||
| label: `The head branch is ${behindBaseBy} commit${behindBaseBy === 1 ? "" : "s"} behind the base branch.`, | ||
| }); | ||
| } |
There was a problem hiding this comment.
behindBaseBy silently collapses to 0, hiding unknown compare status
When the GitHub compare API fails, behindBaseBy is now null (the bestEffort fallback in prService.ts). This coercion maps null → 0, so behindBaseBy > 0 is false and no "behind" blocker is shown — effectively displaying "not behind" when the status is actually unknown.
| const behindBaseBy = status?.behindBaseBy ?? 0; | |
| if (behindBaseBy > 0) { | |
| blockers.push({ | |
| id: "behind", | |
| label: `The head branch is ${status.behindBaseBy} commit${status.behindBaseBy === 1 ? "" : "s"} behind the base branch.`, | |
| label: `The head branch is ${behindBaseBy} commit${behindBaseBy === 1 ? "" : "s"} behind the base branch.`, | |
| }); | |
| } | |
| const behindBaseBy = status?.behindBaseBy; | |
| if (behindBaseBy != null && behindBaseBy > 0) { | |
| blockers.push({ | |
| id: "behind", | |
| label: `The head branch is ${behindBaseBy} commit${behindBaseBy === 1 ? "" : "s"} behind the base branch.`, | |
| }); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/components/prs/shared/prMergeRailUtils.ts
Line: 70-76
Comment:
**Null `behindBaseBy` silently collapses to 0, hiding unknown compare status**
When the GitHub compare API fails, `behindBaseBy` is now `null` (the `bestEffort` fallback in `prService.ts`). This coercion maps `null → 0`, so `behindBaseBy > 0` is false and no "behind" blocker is shown — effectively displaying "not behind" when the status is actually unknown.
```suggestion
const behindBaseBy = status?.behindBaseBy;
if (behindBaseBy != null && behindBaseBy > 0) {
blockers.push({
id: "behind",
label: `The head branch is ${behindBaseBy} commit${behindBaseBy === 1 ? "" : "s"} behind the base branch.`,
});
}
```
How can I resolve this? If you propose a fix, please make it concise.
arul28
deleted the
ade-69-pr-github-subsystem-freshness-etag-correctness-merge-rail-bugs-switchbranch-stale-cleanup-polling-cursors-index-lock-decomposition-pr-detail-polish
branch
1 participant
Fixes ADE-69
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Linked Linear issues
Greptile Summary
This PR addresses a broad set of correctness issues in the PR/GitHub subsystem: ETag cache eviction safety, polling cursor freshness after downtime, stale conflict predictions, index.lock recovery hardening, and PR merge-rail UX polish. It also extracts PR row deletion into a shared module and fixes several silent-swallow error patterns across the rebase banner and tab.
inFlightConditionalGetKeysto protect cache entries from eviction while a conditional GET is in flight; the eviction loop and eviction helper are updated to respect this set.sincecursor is available (post-restart), fixing missed events after downtime;pollReviewsgains asinceparameter for filtering old reviews on restart.behindBaseBybecomesnumber | nullthroughout so compare failures don't silently masquerade as "zero commits behind", and thelandpath adds a pre-merge draft/conflict check before calling GitHub's merge API.Confidence Score: 3/5
Safe to merge for the majority of users; one narrow but real scenario in the ETag cache can cause polling errors under high concurrency when the cache is at capacity.
The concurrent-conditional-GET race in
githubService.tsis the main concern: two requests for the same URL both add the protection key, but the first to complete removes it, leaving the second unprotected. If an eviction fires in that window, the 304 handler finds no cached data and throws, surfacing as a polling failure. This is hard to hit in practice (requires ≥200 cached URLs and two overlapping requests to the same endpoint) but the failure mode is a thrown error rather than a graceful degradation. Everything else in the PR — polling cursor fixes, SHA-based staleness, merge-rail guards, error surfacing, PR row cleanup refactor — looks correct and well-tested.apps/desktop/src/main/services/github/githubService.ts (ETag cache race), apps/desktop/src/main/services/git/git.ts (synchronous lsof call), apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts (outdated-thread state inconsistency)
Important Files Changed
inFlightConditionalGetKeys; has a concurrent-request race where two GETs for the same URL can leave the key unprotected, causing a 304-with-empty-cache error at capacity.lsof-based live-process check; theexecFileSynccall blocks the event loop up to 2 s on each recovery attempt.pr_group_members, cascade deletes, and empty-group pruning for both ID-based and lane-based deletions.bestEffort; changes compare-failure fallback tonullinstead of0.behindBaseByhandling; null compare status silently maps to 0 inderiveMergeBlockers, hiding unknown state from the user.sinceparameter topollReviewsfor cursor-based filtering on restart.behindBaseByconservatively — runs base-advance sync when the compare result is unknown, preventing premature merge attempts on stale state.isOutdatedfrom fixed-state condition; fallback path now returns"new"for outdated+unresolved threads, diverging from theissueInventoryServicebehavior that skips them.Sequence Diagram
sequenceDiagram participant Poller as githubPollingService participant GH as githubService.apiRequest participant ECache as etagCache (Map) participant Fly as inFlightConditionalGetKeys (Set) Poller->>GH: GET /repos/... (cached ETag present) GH->>Fly: add(urlKey) GH->>GH: fetch (await - yields event loop) Note over GH,Fly: concurrent eviction window alt Single in-flight (fixed) GH-->>GH: 304 received GH->>Fly: delete(urlKey) [finally] GH->>ECache: get(urlKey) → cached ✓ GH-->>Poller: return cached data else Two concurrent GETs for same key (race) GH-->>GH: Request 1 completes GH->>Fly: delete(urlKey) [finally] — key now unprotected Note over Fly,ECache: eviction can now remove urlKey entry GH-->>GH: Request 2 gets 304 GH->>Fly: delete(urlKey) [no-op] GH->>ECache: get(urlKey) → undefined ✗ GH-->>Poller: throws "GitHub API request failed (HTTP 304)" endComments Outside Diff (2)
apps/desktop/src/main/services/github/githubService.ts, line 633-645 (link)inFlightConditionalGetKeysuses a plainSet, so two concurrent requests for the sameurlKeyboth add the key, but the first to complete removes it infinally, leaving the second request unprotected. If the cache is at capacity and anotherapiRequestfires a new URL between those two completions,evictOldestEtagCacheEntrywill evict the shared key. The second request then receives a 304 and reaches line 641 whereetagCache.get(urlKey)returnsundefined; the code falls through toresponse.text(), parses an empty 304 body as{}, then throws on!response.ok(304is notok):GitHub API request failed (HTTP 304). This would surface as a polling error when the GitHub API cache is at capacity and two overlapping polls for the same resource are in flight.Prompt To Fix With AI
apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts, line 883-885 (link)"new", diverging fromissueInventoryServiceissueInventoryService.tswas updated to skip brand-new outdated threads (thread.isOutdated && !existing) rather than marking them"fixed". The fallback path here still classifies any non-resolved thread (including outdated ones) as"new". When the full DB sync hasn't run yet, the CTO operator may receive an outdated review thread as an open action item and attempt to address it, even though the main sync would have dropped it entirely.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "Fix ADE-69 Linear links and PR freshness" | Re-trigger Greptile