Cover concurrent Gemini tier-fetch coalescing - #150
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds deterministic concurrent Gemini usage coverage. It instruments tier-cache waiter enrollment, updates isolated URL protocol support for asynchronous cancellation, and verifies shared tier fetching and tier-derived quota project routing. ChangesGemini concurrency coverage
Sequence Diagram(s)sequenceDiagram
participant RequestA
participant RequestB
participant GeminiUsageProvider
participant GeminiTierCache
participant TestURLProtocol
RequestA->>GeminiUsageProvider: Start usage fetch
RequestB->>GeminiUsageProvider: Start overlapping usage fetch
GeminiUsageProvider->>GeminiTierCache: Fetch or await tier
GeminiTierCache->>TestURLProtocol: Send one tier request
TestURLProtocol-->>GeminiTierCache: Return tier and project
GeminiTierCache-->>GeminiUsageProvider: Release both callers
GeminiUsageProvider->>TestURLProtocol: Send quota request for resolved project
GeminiUsageProvider->>TestURLProtocol: Send quota request for resolved project
TestURLProtocol-->>RequestA: Return usage
TestURLProtocol-->>RequestB: Return usage
Possibly related issues
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
@macroscope-app review |
|
@codex review |
|
Manual reviews triggered for commit All prior checks · these links stay valid even if you push more commits. |
|
Review is in progress. Results will be posted as check runs when complete. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CodexBarMacTests/NetworkTestSupport.swift`:
- Around line 71-80: Add explicit empty deinit declarations to both
IsolatedTestURLProtocol and its nested LoadContext class, matching the existing
test-target convention and satisfying SwiftLint’s required_deinit rule.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6add99d8-2b83-4635-966d-5b7d87fd8389
📒 Files selected for processing (4)
CHANGELOG.mdCodexBarMac/Services/GeminiUsageProvider.swiftCodexBarMacTests/GeminiProviderTests.swiftCodexBarMacTests/NetworkTestSupport.swift
ApprovabilityVerdict: Would Approve This PR adds test coverage for concurrent Gemini tier-fetch coalescing. Production code changes are limited to adding an optional test synchronization hook that's always nil in production, with no runtime behavior impact. The bulk of changes are test code and test infrastructure. Macroscope would have approved this PR. Enable approvability here. |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@macroscope-app review |
|
@codex review |
|
Review is in progress. Results will be posted as check runs when complete. |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@macroscope-app review |
|
@codex review |
|
Review is in progress. Results will be posted as check runs when complete. |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Closes #149
Summary
Verification
./test.sh(changelog, release-artifact, and app-runner smoke tests; 352 XCTest tests passed)xcodebuild ... -enableThreadSanitizer YES test(352 tests passed; no Thread Sanitizer diagnostics)xccov(the existingfetchInProgressfalse return and continuation enqueue each executed once)xcodebuild ... analyzegit diff --checkResidual risk
Summary by cubic
Adds deterministic coverage for concurrent Gemini tier-fetch coalescing and confirms a second usage refresh waits on the in-flight request and uses the tier-derived project for quota routing. No production behavior change.
tierWaiterEnqueuedhook toGeminiUsageProviderand forwarded it toGeminiTierCache.waitForInFlightFetchIfNeededvia a new package-internal initializer.URLSessionto async handlers and let cancellation interrupt and stop callbacks oncestopLoadingbegins.Written for commit 712a5cf. Summary will update on new commits.
Note
Add deterministic test coverage for concurrent Gemini tier-fetch coalescing
testGeminiUsageProviderCoalescesConcurrentTierFetchesto verify that two concurrent fetches share a single tier HTTP request and derive the same quota project.tierWaiterEnqueuedcallback onGeminiUsageProviderand threads it throughGeminiTierCache.waitForInFlightFetchIfNeededso the test can synchronize precisely when the second fetch joins the waiter queue.IsolatedTestURLProtocolin NetworkTestSupport.swift to support async handlers with safe cancellation, allowing the test to gate the tier response until both fetches are in-flight.Macroscope summarized 712a5cf.
Greptile Summary
The PR adds deterministic coverage for concurrent Gemini tier-fetch coalescing and extends isolated URL loading to support asynchronous handlers.
Confidence Score: 4/5
The PR is not yet safe to merge because isolated URL protocol callbacks can still execute after cancellation begins.
The reply stating the callback race was fixed is contradicted by the current implementation:
activeClient()releases its lock before invoking each client callback, sostopLoading()can begin after the check while the callback still proceeds.Files Needing Attention: CodexBarMacTests/NetworkTestSupport.swift
Important Files Changed
Sequence Diagram
sequenceDiagram participant F1 as First usage refresh participant Cache as Gemini tier cache participant Tier as Tier endpoint participant F2 as Second usage refresh participant Quota as Quota endpoint F1->>Cache: Fetch tier Cache->>Tier: Start request F2->>Cache: Request tier Cache-->>F2: Enqueue waiter Tier-->>Cache: Project and tier Cache-->>F1: Complete fetch Cache-->>F2: Resume waiter F1->>Quota: Request using returned project F2->>Quota: Request using returned projectReviews (3): Last reviewed commit: "Let cancellation interrupt test callback..." | Re-trigger Greptile