test: preserve lease primitives across replication and failover#94
test: preserve lease primitives across replication and failover#94
Conversation
|
Warning Rate limit exceeded
⌛ 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe PR adds bundle reservation test infrastructure and a new failover simulation test to verify bundle membership state preservation across quorum replication and view change, alongside documentation updates reflecting completion of M9-T09 and shift to M9-T10 objectives. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 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)
📝 Coding Plan
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
![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
🧹 Nitpick comments (1)
crates/allocdb-node/src/replicated_simulation_tests.rs (1)
1467-1481: Avoid hardcoded reservation identity/slot in assertions.
ReservationId(3)andcurrent_slot=3make this test brittle to unrelated sequencing changes. Prefer deriving the reservation id from the published result of"bundle-reserve"and using that in membership checks.Suggested refactor
- assert_eq!(first_member.current_reservation_id, Some(ReservationId(3))); + let bundle_result = harness + .published_result(bundle.lsn) + .expect("bundle reserve should publish after quorum commit"); + let reservation_id = bundle_result + .outcome + .reservation_id + .expect("bundle reserve should allocate a reservation id"); + assert_eq!(first_member.current_reservation_id, Some(reservation_id)); - assert_eq!(second_member.current_reservation_id, Some(ReservationId(3))); + assert_eq!(second_member.current_reservation_id, Some(reservation_id)); - assert_eq!( - replica_reservation_member_ids(&harness, 2, 3, 3), + assert_eq!( + replica_reservation_member_ids(&harness, 2, reservation_id.get(), 3), vec![ResourceId(81), ResourceId(82)] ); - assert_eq!( - replica_reservation_member_ids(&harness, 3, 3, 3), + assert_eq!( + replica_reservation_member_ids(&harness, 3, reservation_id.get(), 3), vec![ResourceId(81), ResourceId(82)] );Also applies to: 1487-1489
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/allocdb-node/src/replicated_simulation_tests.rs` around lines 1467 - 1481, The assertions hardcode ReservationId(3) and slot 3 making the test brittle; instead extract the actual reservation id/slot from the bundle-reserve publish result (the value produced when running the bundle reservation) and use that variable in comparisons for first_member.current_reservation_id, second_member.current_reservation_id, and the call to replica_reservation_member_ids; update the test to capture that reservation id (e.g. reserve_id or reservation_id) from the published bundle result (where bundle.lsn is produced) and substitute that symbol in place of ReservationId(3) and the literal slot 3 so membership checks reference the dynamically obtained reservation id/slot.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/status.md`:
- Around line 211-220: Update the earlier milestone mention that still reads "M9
... T08 implementation in progress" in docs/status.md so the milestone state
matches the new block (which marks PR `#93` as M9-T09 merged and `#87` as M9-T10
active); locate the inconsistent milestone text and change the T08 reference to
T10 (and adjust any adjacent phrasing that implies T08 work) so the single-file
snapshot consistently reflects M9-T10 as the active implementation slice and
M9-T09 as merged.
---
Nitpick comments:
In `@crates/allocdb-node/src/replicated_simulation_tests.rs`:
- Around line 1467-1481: The assertions hardcode ReservationId(3) and slot 3
making the test brittle; instead extract the actual reservation id/slot from the
bundle-reserve publish result (the value produced when running the bundle
reservation) and use that variable in comparisons for
first_member.current_reservation_id, second_member.current_reservation_id, and
the call to replica_reservation_member_ids; update the test to capture that
reservation id (e.g. reserve_id or reservation_id) from the published bundle
result (where bundle.lsn is produced) and substitute that symbol in place of
ReservationId(3) and the literal slot 3 so membership checks reference the
dynamically obtained reservation id/slot.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7b977214-2a7d-4921-8d76-c4279cccdcdb
📒 Files selected for processing (2)
crates/allocdb-node/src/replicated_simulation_tests.rsdocs/status.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Keep documentation up to date with the code and design. If a change affects behavior, invariants, failure modes, operational semantics, testing strategy, or implementation sequencing, update the relevant docs in the same task or PR.
Files:
docs/status.md
docs/status.md
📄 CodeRabbit inference engine (AGENTS.md)
Keep
docs/status.mdcurrent as the single-file progress snapshot for the repository. Update it whenever milestone state, implementation coverage, or the recommended next step materially changes.
Files:
docs/status.md
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Write extensive tests for every meaningful behavior change. Favor invariant tests, negative-path tests, recovery tests, and regression tests over shallow happy-path coverage.
Add extensive logging where it materially improves debuggability or operational clarity. Use the right log level:errorfor invariant breaks, corruption, and failed operations that require intervention;warnfor degraded but expected conditions such as overload, lag, or rejected requests;infofor meaningful lifecycle and state-transition events;debugfor detailed execution traces useful in development;traceonly for very high-volume diagnostic detail.
Logging must be structured and purposeful. Do not add noisy logs that obscure signal or hide bugs.
Files:
crates/allocdb-node/src/replicated_simulation_tests.rs
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: skel84/allocdb PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-12T15:18:53.086Z
Learning: Treat CodeRabbit as part of the required review path when it is enabled on the repository. Wait for its status to complete before merge. If it completes without a visible review comment or review thread, request visible output with `coderabbitai summary`. Address every substantive CodeRabbit comment explicitly before merge by either applying the change or documenting why it is not being applied. Apply correctness, safety, recovery, test, and docs-alignment feedback by default; document why you reject suggestions that would weaken determinism, boundedness, or trusted-core discipline.
📚 Learning: 2026-03-12T15:18:53.086Z
Learnt from: CR
Repo: skel84/allocdb PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-12T15:18:53.086Z
Learning: Applies to docs/status.md : Keep [`docs/status.md`](./docs/status.md) current as the single-file progress snapshot for the repository. Update it whenever milestone state, implementation coverage, or the recommended next step materially changes.
Applied to files:
docs/status.md
📚 Learning: 2026-03-12T15:18:53.086Z
Learnt from: CR
Repo: skel84/allocdb PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-12T15:18:53.086Z
Learning: Applies to **/*.md : Keep documentation up to date with the code and design. If a change affects behavior, invariants, failure modes, operational semantics, testing strategy, or implementation sequencing, update the relevant docs in the same task or PR.
Applied to files:
docs/status.md
📚 Learning: 2026-03-12T15:18:53.086Z
Learnt from: CR
Repo: skel84/allocdb PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-12T15:18:53.086Z
Learning: Use the GitHub Project `AllocDB` as the operational work board. Keep planned work on the board, not only in milestone pages or local docs.
Applied to files:
docs/status.md
🧬 Code graph analysis (1)
crates/allocdb-node/src/replicated_simulation_tests.rs (4)
crates/allocdb-core/src/command_codec.rs (2)
encode_client_request(12-19)new(184-186)crates/allocdb-node/src/api_codec.rs (1)
new(671-673)crates/allocdb-node/src/replicated_simulation.rs (3)
new(481-525)replica(535-540)current_slot(527-529)crates/allocdb-core/src/state_machine_metrics.rs (1)
reservation(48-68)
🔇 Additional comments (3)
crates/allocdb-node/src/replicated_simulation_tests.rs (3)
32-37: Good helper extraction for bundle/failover test setup.These additions keep the new replication tests focused and readable while reusing the existing request-encoding path.
Also applies to: 73-83, 102-117, 276-282
1423-1504: Strong failover regression coverage for bundle semantics.This test meaningfully exercises commit, promotion, suffix-only rejoin, and post-failover conflict handling for bundle members.
1572-1588: Nice extension of revoke-failover semantics with stale release rejection.The added stale-holder
Releaseassertion plus operation-id progression improves negative-path confidence for M9-T10 without widening scope.Also applies to: 1593-1593, 1610-1610
1 participant
Summary
Validation
Closes #87