Address review findings on merged test-infra PRs#339
Conversation
Three post-merge review findings from #334 and #336: - The test-concurrency semaphore now budgets connections, not tests: setup_pool acquires permits equal to the pool's max_connections against an 80-permit budget, so eight 20-connection pools can no longer exceed the server's max_connections=100 (CodeRabbit, #336). A per-test floor of budget/8 keeps effective test concurrency near 8 — without it, small-pool tests admitted ~20 concurrent client runtimes and CPU starvation made wait-for-state assertions time out (reproduced: 5-6 failures per run at unfloored weights). - setup_pool returns (guard, pool) so the destructured locals drop pool-first and the permit outlives the pool handle (CodeRabbit nitpick, #336). - test_weight_proportionality waits on snapshot publication rather than the completion counter, which increments a beat before the snapshot stores (CodeRabbit, #334). Validated: 8 consecutive green queue_storage_runtime_test runs, 3 consecutive green weighted_test runs.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
More reviews will be available in 46 minutes and 7 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughConnection budgeting semaphore introduced to cap concurrent Postgres connections across integration tests. Queue storage test harness ChangesTest Infrastructure and Flakiness Fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
🤖 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 `@awa/tests/weighted_test.rs`:
- Line 627: The steady-state loop currently checks only snapshot_a against
SNAPSHOT_UNSET, allowing the loop to exit before snapshot_b is set and causing
unsafe/flaky calculations; update the loop's exit condition to wait for both
snapshot_a and snapshot_b to be set (i.e., ensure snapshot_a.load(...) !=
SNAPSHOT_UNSET && snapshot_b.load(...) != SNAPSHOT_UNSET) so the worker's
sequential stores for snapshot_a and snapshot_b complete before computing
snapshot_b - base_b.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro
Run ID: 1a3247a0-a558-404b-a1eb-1ad14a936beb
📒 Files selected for processing (2)
awa/tests/queue_storage_runtime_test.rsawa/tests/weighted_test.rs
The loop exited once snapshot_a was published, but the worker stores base_a, base_b, snapshot_a, snapshot_b sequentially; gate on all four and use the same condition to choose the measurement window, so a 20s-timeout edge can never subtract from an unset baseline.
1 participant
Summary
Follow-up for the post-merge review comments on #334 and #336, per the CodeRabbit findings:
setup_poolnow acquires permits equal to the pool'smax_connectionsagainst an 80-permit budget, so the aggregate connection worst case is actually bounded below the server'smax_connections=100— the previous one-permit-per-test cap allowed 8 × 20-connection pools. A per-test floor of budget/8 keeps effective test concurrency near 8: at unfloored weights, small-pool tests admitted ~20 concurrent client runtimes and CPU starvation produced 5–6 wait-for-state timeouts per run (reproduced locally before adding the floor).setup_poolreturns(guard, pool)so destructured locals drop the pool handle before releasing the permit.test_weight_proportionalitywaits on the snapshot store itself instead of the completion counter, which increments a beat before the stores.The codex lockfile finding on #334 was already fixed pre-merge (
9e400ae).Validation
cargo fmt --all;cargo clippy -p awa --tests -- -D warningscleanqueue_storage_runtime_testruns (incl. the floor-regression check), 3 consecutive greenweighted_testrunsRefs #334, #336, #335.
Summary by CodeRabbit