Reduce claimer heartbeat churn#213
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughIntroduced an internal ChangesConditional Claimer Lease Refresh
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 0/1 reviews remaining, refill in 36 minutes and 2 seconds.Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@awa/tests/queue_storage_runtime_test.rs`:
- Around line 4144-4152: The test is backdating the claimer lease by 300ms which
is less than the idle_threshold (500ms), so update the SQL bind that sets
last_claimed_at (the Utc::now() - chrono::Duration::milliseconds(300)
expression) to use a duration greater than idle_threshold (e.g., >500ms or use
idle_threshold + some margin) so the lease becomes stale and the refresh branch
is exercised when running the assertions against queue_claimer_leases for
lease.claimer_slot and queue.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7a3d68d-dab0-4f24-a853-02b41bf2616b
📒 Files selected for processing (3)
awa-model/src/queue_storage.rsawa/tests/queue_storage_runtime_test.rsdocs/architecture.md
Summary
mark_queue_claimer_activewhile the lease is still safely freshQueueClaimerLeaseshape unchanged; freshness timestamps stay in an internal row typequeue_claimer_leases, while a half-stale idle heartbeat refreshes before another process can steal itdocs/architecture.md, correcting table names and recovery claims against the current implementation and TLA docsArchitecture review notes
Corrections made while folding in the diagrams:
done_entriesis the terminal table name in queue storage; the oldterminal_entrieswording was removedleasesis the materialized live-attempt table;active_leasesis only a logical/model term in older prosequeue_claim_heads ... FOR UPDATE SKIP LOCKED; ring CAS belongs to rotate/prune safety, not the claim serializerBenchmarks
Production-durability runs on the portable benchmark harness,
PRODUCER_BATCH_MAX=128 PRODUCER_BATCH_MS=25.custom-20260502T035801Z-ab7c8acustom-20260502T053743Z-cd752dcustom-20260502T053958Z-1235c0A SQL-only throttle variant was tested and rejected before this PR because it regressed badly (
custom-20260502T053416Z-2853e7: 4,634/s, p99 2.07s). Statement fusion was also kept out of this PR: it improved the 5k producer-capped case but regressed overload tail latency.Verification
cargo test -p awa --test queue_storage_runtime_test test_queue_storage_claimer_heartbeat_skips_fresh_lease -- --nocapturecargo test -p awa --test queue_storage_runtime_test test_queue_storage_bounded_claimers -- --nocapturecargo clippy -p awa-model --all-targets -- -D warningscargo fmt --check./correctness/run-tlc.sh storage/AwaSegmentedStorage.tlaSummary by CodeRabbit
Bug Fixes
Documentation
Tests