feat: add revoke and reclaim lease transitions#92
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 (18)
WalkthroughAdds explicit Revoke and Reclaim commands with new Revoking/Revoked states, updates fingerprints and wire/snapshot codecs, implements state-machine handlers and helpers for revoke/reclaim, extends invariants and slot checks, and adds recovery/replication/tests and API/replicated-simulation coverage. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant SM as StateMachine
participant RS as Reservation
participant R as Resources
participant Q as RetirementQueue
Client->>SM: Command::Revoke(reservation_id)
SM->>RS: validate exists && state == Confirmed
Note right of RS: Confirmed → Revoking\n(lease_epoch++)
SM->>R: mark_member_resources(reservation_id, Revoking)
SM-->>Client: ResultCode::Ok
Client->>SM: Command::Reclaim(reservation_id)
SM->>RS: validate state == Revoking && not retired
Note right of RS: Revoking → Revoked\n(set released_lsn, retire_after_slot)
SM->>R: release_member_resources(reservation_id) -> Available
SM->>Q: enqueue_retirement(reservation_id, retire_after_slot)
SM-->>Client: ResultCode::Ok
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/allocdb-core/src/state_machine_revoke_apply.rs (1)
35-38: Invariant check only validates anchor resource, not all bundle members.The
assert!on line 36 only validates thatreservation.resource_id(the anchor resource) exists. For bundle reservations with multiple members, if any non-anchor member resource is unexpectedly missing, the panic will occur later inmark_member_resourceswith a less specific error message.This is likely acceptable since all member resources should always exist if the anchor exists (structural invariant), but consider adding a comment explaining that
mark_member_resourceswill also assert member presence, or expanding validation here for clearer diagnostics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/allocdb-core/src/state_machine_revoke_apply.rs` around lines 35 - 38, The current assert only checks that self.resources contains reservation.resource_id (the anchor) which can hide missing non-anchor bundle members until mark_member_resources runs; either extend this validation here to iterate reservation.members (or the bundle member list used by mark_member_resources) and assert each member resource exists in self.resources, or add a concise comment on the assert mentioning that mark_member_resources will perform/assume per-member presence checks and will panic with a more specific message if a non-anchor member is missing (reference the assert! on reservation.resource_id and the mark_member_resources function to locate the logic).crates/allocdb-core/src/state_machine_revoke_tests.rs (1)
1-306: Consider adding edge-case tests for additional negative paths.The test coverage is solid for the core acceptance criteria. For completeness, consider adding tests for:
revokeon a non-existent reservation (expectReservationNotFound)revokeon alreadyRevokingorRevokedstates (expectInvalidState)- Double
reclaimon same reservation (expectReservationRetiredorInvalidState)These would strengthen regression coverage. Based on learnings: "Favor invariant tests, negative-path tests, recovery tests, and regression tests over shallow happy-path coverage."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/allocdb-core/src/state_machine_revoke_tests.rs` around lines 1 - 306, Add negative-path tests in state_machine_revoke_tests.rs that call AllocDb::apply_client with Command::Revoke and Command::Reclaim to exercise these edge cases: (1) revoke a non-existent reservation id (use ReservationId(999) or similar) and assert ResultCode::ReservationNotFound; (2) revoke a reservation already in ReservationState::Revoking and one already in ReservationState::Revoked (drive to those states using existing reserve/confirm/revoke/reclaim flows) and assert ResultCode::InvalidState; and (3) call Reclaim twice on the same reservation and assert the second call returns ReservationRetired or ResultCode::InvalidState as appropriate. Use the same helpers (create, reserve_bundle, context, AllocDb::new, ClientRequest, apply_client) and existing symbols (Command::Revoke, Command::Reclaim, ReservationId, ResultCode, ReservationState) so tests integrate consistently with the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/allocdb-core/src/command_codec.rs`:
- Around line 112-125: The wire tag for Command::Expire was changed causing old
WAL frames to mis-decode; revert/ensure the Expire variant uses the legacy tag
value (6) in the encoder/decoder where Command::Revoke and Command::Reclaim are
serialized (the push byte values around the Command::Revoke/Reclaim/Expire match
in the encoder) and update the decoder branch in decode_internal_command so tag
6 maps to Command::Expire (consuming reservation_id and deadline_slot) while
Revoke uses its original tag; add a fixed-input regression test that feeds a raw
legacy tag-6 Expire payload (reservation_id + deadline_slot bytes) into your
decoder to assert it produces Command::Expire and does not leave unread bytes or
return InvalidLayout.
In `@crates/allocdb-node/src/bin/allocdb-jepsen/events.rs`:
- Around line 350-358: Add a focused unit test that exercises the mapping of
ReservationState->JepsenReservationState: create a Reservation (or the minimal
input expected by the function that contains the shown match/convert logic) with
state=ReservationState::Revoking and assert the converter returns
JepsenReservationState::Active with confirmed == true and correct
resource_id/holder_id/deadline_slot; also create a Reservation with
state=ReservationState::Revoked and assert the converter returns
JepsenReservationState::Released with the expected released_lsn. Locate and call
the function that performs the match (the code containing
ReservationState::Confirmed | ReservationState::Revoking => ... and
ReservationState::Released | ReservationState::Expired |
ReservationState::Revoked => ...) and add these two assertions as unit tests to
the same module so regression changes fail fast.
---
Nitpick comments:
In `@crates/allocdb-core/src/state_machine_revoke_apply.rs`:
- Around line 35-38: The current assert only checks that self.resources contains
reservation.resource_id (the anchor) which can hide missing non-anchor bundle
members until mark_member_resources runs; either extend this validation here to
iterate reservation.members (or the bundle member list used by
mark_member_resources) and assert each member resource exists in self.resources,
or add a concise comment on the assert mentioning that mark_member_resources
will perform/assume per-member presence checks and will panic with a more
specific message if a non-anchor member is missing (reference the assert! on
reservation.resource_id and the mark_member_resources function to locate the
logic).
In `@crates/allocdb-core/src/state_machine_revoke_tests.rs`:
- Around line 1-306: Add negative-path tests in state_machine_revoke_tests.rs
that call AllocDb::apply_client with Command::Revoke and Command::Reclaim to
exercise these edge cases: (1) revoke a non-existent reservation id (use
ReservationId(999) or similar) and assert ResultCode::ReservationNotFound; (2)
revoke a reservation already in ReservationState::Revoking and one already in
ReservationState::Revoked (drive to those states using existing
reserve/confirm/revoke/reclaim flows) and assert ResultCode::InvalidState; and
(3) call Reclaim twice on the same reservation and assert the second call
returns ReservationRetired or ResultCode::InvalidState as appropriate. Use the
same helpers (create, reserve_bundle, context, AllocDb::new, ClientRequest,
apply_client) and existing symbols (Command::Revoke, Command::Reclaim,
ReservationId, ResultCode, ReservationState) so tests integrate consistently
with the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 50dce838-cf6b-4275-ab51-32e3c3a7b71d
📒 Files selected for processing (18)
crates/allocdb-core/src/command.rscrates/allocdb-core/src/command_codec.rscrates/allocdb-core/src/recovery.rscrates/allocdb-core/src/recovery_revoke_tests.rscrates/allocdb-core/src/snapshot_codec.rscrates/allocdb-core/src/snapshot_tests.rscrates/allocdb-core/src/state_machine.rscrates/allocdb-core/src/state_machine_apply.rscrates/allocdb-core/src/state_machine_invariants.rscrates/allocdb-core/src/state_machine_reservation_invariants.rscrates/allocdb-core/src/state_machine_revoke_apply.rscrates/allocdb-core/src/state_machine_revoke_tests.rscrates/allocdb-core/src/state_machine_slots.rscrates/allocdb-node/src/api_codec.rscrates/allocdb-node/src/api_tests.rscrates/allocdb-node/src/bin/allocdb-jepsen/events.rscrates/allocdb-node/src/replicated_simulation_tests.rsdocs/status.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: semgrep-cloud-platform/scan
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/bin/allocdb-jepsen/events.rscrates/allocdb-core/src/state_machine_invariants.rscrates/allocdb-core/src/state_machine_slots.rscrates/allocdb-core/src/snapshot_codec.rscrates/allocdb-core/src/snapshot_tests.rscrates/allocdb-core/src/recovery.rscrates/allocdb-core/src/command.rscrates/allocdb-node/src/api_codec.rscrates/allocdb-core/src/state_machine.rscrates/allocdb-core/src/state_machine_reservation_invariants.rscrates/allocdb-core/src/command_codec.rscrates/allocdb-node/src/api_tests.rscrates/allocdb-core/src/recovery_revoke_tests.rscrates/allocdb-core/src/state_machine_apply.rscrates/allocdb-node/src/replicated_simulation_tests.rscrates/allocdb-core/src/state_machine_revoke_tests.rscrates/allocdb-core/src/state_machine_revoke_apply.rs
**/*.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
🧠 Learnings (2)
📚 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 **/*.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.
Applied to files:
crates/allocdb-core/src/recovery.rscrates/allocdb-core/src/recovery_revoke_tests.rscrates/allocdb-core/src/state_machine_revoke_tests.rs
📚 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
🧬 Code graph analysis (4)
crates/allocdb-node/src/bin/allocdb-jepsen/events.rs (1)
crates/allocdb-core/src/state_machine_metrics.rs (1)
reservation(48-68)
crates/allocdb-node/src/replicated_simulation_tests.rs (5)
crates/allocdb-core/src/command_codec.rs (1)
encode_client_request(12-19)crates/allocdb-node/src/engine_checkpoint_tests.rs (2)
reserve(55-65)confirm(67-77)crates/allocdb-node/src/engine_tests.rs (2)
reserve(62-77)confirm(79-89)crates/allocdb-node/src/simulation_tests.rs (1)
reserve(51-66)crates/allocdb-node/src/engine_issue_31_tests.rs (1)
reserve(65-80)
crates/allocdb-core/src/state_machine_revoke_tests.rs (4)
crates/allocdb-core/src/recovery_revoke_tests.rs (1)
config(24-36)crates/allocdb-core/src/snapshot_tests.rs (2)
config(11-23)context(25-30)crates/allocdb-core/src/state_machine.rs (1)
new(160-199)crates/allocdb-core/src/state_machine_metrics.rs (2)
reservation(48-68)resource(36-38)
crates/allocdb-core/src/state_machine_revoke_apply.rs (2)
crates/allocdb-core/src/state_machine_metrics.rs (2)
reservation(48-68)resource(36-38)crates/allocdb-core/src/state_machine_slots.rs (1)
slot_overflow_outcome(38-49)
🔇 Additional comments (25)
crates/allocdb-core/src/snapshot_tests.rs (1)
125-204: LGTM! Comprehensive snapshot round-trip test for the new revoke/reclaim lifecycle.The test correctly validates that both
RevokingandRevokedstates survive snapshot encode/decode cycles. The progression throughCreateResource→ReserveBundle→Confirm→Revoke→ snapshot/restore →Reclaim→ snapshot/restore ensures the full lifecycle is exercised.crates/allocdb-core/src/state_machine.rs (2)
63-79: LGTM! State enum extensions follow the existing pattern.The new
Revokingstate for resources andRevoking/Revokedstates for reservations are correctly positioned in the enums, maintaining the logical state progression.
224-225: LGTM! Command dispatch correctly delegates to new apply methods.The
Revokecommand correctly omitscontext(since it doesn't need slot-derived fields), whileReclaimpassescontextfor retirement scheduling. This aligns with the pattern used byReleaseandExpire.crates/allocdb-node/src/api_tests.rs (2)
178-222: LGTM! API codec test coverage for new revoke/reclaim states.The test cases correctly model the expected field states:
Revokingreservation:released_lsn: None,retire_after_slot: None(not yet terminal)Revokedreservation:released_lsn: Some(...),retire_after_slot: Some(...)(terminal state with retirement scheduled)This aligns with the lifecycle semantics.
271-282: LGTM! Clean refactoring improves test organization.Splitting response cases into
submit_response_cases,read_response_cases, andmaintenance_response_casesimproves maintainability without changing test semantics.crates/allocdb-core/src/state_machine_reservation_invariants.rs (2)
13-15: LGTM! Invariant classification correctly models the revoke lifecycle.
Revokingis correctly treated as active (noretire_after_slot) since the resource is still held pending explicit reclaim.Revokedis correctly terminal with requiredretire_after_slotsince the reservation is complete and awaiting history window expiry.Also applies to: 24-24
161-173: LGTM! Resource-reservation state coupling invariant for Revoking.The invariant correctly requires that when a reservation is in
Revokingstate, all member resources must be inResourceState::Revokingwith matchingcurrent_reservation_id. This prevents state inconsistencies during the revoke phase.docs/status.md (1)
36-40: LGTM! Documentation correctly tracks implementation progress.The status file accurately reflects:
- New
revoke/reclaimoperations in the allocator surface- Active branch validation scope
- Clear T08/T09 boundary for deferred work
Based on learnings: "Keep
docs/status.mdcurrent as the single-file progress snapshot for the repository."Also applies to: 50-50, 209-219
crates/allocdb-core/src/state_machine_apply.rs (3)
176-186: LGTM! Release correctly rejects reservations in revoke lifecycle.Adding
RevokingandRevokedto the invalid-state rejection is correct: once revocation has begun (or completed), the holder cannot release since they've already been fenced out by the revoke operation.
244-258: LGTM! Expire correctly treats revoke states as noop.
RevokingandRevokedreservations are correctly excluded from TTL expiration since:
Revokingtransitioned fromConfirmedwhich already unscheduled the expirationRevokedis terminal and shouldn't be expired
114-114: LGTM! Helper method refactoring improves maintainability.Extracting
mark_member_resourcesandrelease_member_resourceshelpers reduces duplication between confirm/release/expire paths and the new revoke/reclaim paths.Also applies to: 208-208, 285-285
crates/allocdb-core/src/recovery_revoke_tests.rs (2)
47-104: LGTM! Recovery test correctly validates revoking state preservation.The test verifies critical revoking-state invariants after WAL replay:
lease_epochbumped to 2 (revoke increments epoch)released_lsnisNone(revoking is not terminal)- Resource remains in
Revokingstate with reservation link intactThis ensures the "late-not-early" reuse guarantee survives recovery.
106-172: LGTM! Recovery test correctly validates revoked state preservation.The test verifies terminal revoked-state semantics after WAL replay:
released_lsnset to the reclaim LSNretire_after_slot = Slot(7)(request_slot=3 + history_window=4)- Resource transitions to
Availablewith cleared reservation linkBased on learnings: "Favor invariant tests, negative-path tests, recovery tests, and regression tests over shallow happy-path coverage."
crates/allocdb-node/src/replicated_simulation_tests.rs (3)
54-101: LGTM! Payload helpers follow established patterns.The new
reserve_payload,confirm_payload,revoke_payload, andreclaim_payloadhelpers mirror the existingcreate_payloadpattern and provide clean test construction for reservation lifecycle commands.
343-351: LGTM! Helper for full backup commit.
commit_to_all_backupscorrectly chains the prepare/ack/commit delivery to backup 2 and the prepare/commit delivery to backup 3. This ensures majority-committed state before failover scenarios.
1359-1460: Excellent regression test for the core safety property.This test directly validates the PR acceptance criteria: "Revoked ownership must not be reusable early under crash, retry, or failover scenarios." The test flow:
- Establishes confirmed reservation across all replicas
- Commits revoke to majority (backup 2), prepares to backup 3
- Crashes primary, completes view change
- Verifies
ResourceState::Revokingpersists withReservationIdlink- Critically: early reserve attempt returns
ResourceBusy- Only after explicit
Reclaimdoes resource becomeAvailableThis is exactly the "late-not-early" reuse guarantee the issue requires.
crates/allocdb-core/src/state_machine_revoke_tests.rs (6)
7-53: LGTM! Well-structured test helpers.The helper functions provide clean abstractions for creating test fixtures. The
config()function appropriately setsmax_bundle_size: 4to support bundle tests, differing from other test modules that usemax_bundle_size: 1.
55-105: LGTM! Comprehensive revoke transition test.Good invariant coverage: verifies epoch bump (1→2), state transitions for both reservation and member resources, and confirms
released_lsn/retire_after_slotremainNoneuntil reclaim.
107-162: LGTM! Critical negative-path coverage for stale epoch rejection.This test directly validates the acceptance criteria that stale holder commands are rejected after revoke. Both
ConfirmandReleasewith the pre-revokelease_epochcorrectly returnStaleEpoch.
164-234: LGTM! Validates the core "no early reuse" safety guarantee.This test directly addresses the acceptance criteria: "Revoked ownership must not be reusable early." The sequence demonstrates that resources in
Revokingstate correctly returnResourceBusy, and onlyReclaimtransitions them toAvailable.
236-265: LGTM! Negative-path test for state ordering.Validates that the state machine enforces the required
Confirmed → Revoking → Revokedtransition order.
267-306: LGTM! Validates deterministic duplicate handling.Directly addresses the acceptance criteria: "Duplicate revoke requests must be handled deterministically." The test confirms that replaying the same
operation_idwith different LSN contexts returns cached results.crates/allocdb-core/src/state_machine_revoke_apply.rs (3)
57-114: LGTM! Correct reclaim implementation.The function properly:
- Validates preconditions (exists, not retired, state is
Revoking)- Handles slot overflow gracefully
- Sets
released_lsnfrom context for WAL recovery support- Queues retirement for eventual cleanup
- Follows logging guidelines (
warn!for rejections,debug!for success)
116-138: LGTM! Clean reusable helper for member resource state updates.The version increment on each resource update is important for change detection. The parameterized
stateallows reuse across different transitions (Revoking, etc.).
140-159: LGTM! Correctly releases all member resources.The function properly clears
current_reservation_idand sets state toAvailable, enabling safe reuse after reclaim.
|
Addressed CodeRabbit feedback:
Validation rerun: @coderabbitai review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/allocdb-core/src/command_codec.rs (1)
273-286:⚠️ Potential issue | 🟡 MinorPin the new wire tags with fixed-byte tests.
This only proves encoder/decoder self-consistency. If
7and8are renumbered together later, the test still passes while WAL frames written by this release become unreadable after upgrade. Please add raw-byte assertions for both variants.🔧 Suggested test tightening
#[test] fn revoke_and_reclaim_round_trip() { - for command in [ - Command::Revoke { - reservation_id: ReservationId(41), - }, - Command::Reclaim { - reservation_id: ReservationId(42), - }, - ] { - let decoded = decode_internal_command(&encode_internal_command(&command)).unwrap(); + for (command, reservation_id, tag) in [ + ( + Command::Revoke { + reservation_id: ReservationId(41), + }, + ReservationId(41), + 7, + ), + ( + Command::Reclaim { + reservation_id: ReservationId(42), + }, + ReservationId(42), + 8, + ), + ] { + let encoded = encode_internal_command(&command); + let mut expected = vec![tag]; + expected.extend_from_slice(&reservation_id.get().to_le_bytes()); + assert_eq!(encoded, expected); + let decoded = decode_internal_command(&expected).unwrap(); assert_eq!(decoded, command); } }As per coding guidelines,
**/*.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."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/allocdb-core/src/command_codec.rs` around lines 273 - 286, The existing test revoke_and_reclaim_round_trip only checks encoder/decoder round-trip; add deterministic raw-byte assertions to pin the wire tags so future renumbering breaks tests. For Command::Revoke and Command::Reclaim (the variants used in the test) compute their encoded bytes via encode_internal_command and assert equality against hard-coded byte slices that reflect the current wire tags (e.g., the exact bytes that encode reservation_id and the tag values currently used, including the tag bytes for 7 and 8); keep the round-trip decode assertions but add assert_eq!(encoded, expected_bytes) for each variant to lock in the on-wire format for encode_internal_command and ensure decode_internal_command still parses those exact bytes. Use the same helper functions encode_internal_command/decode_internal_command and update test names/comments accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/allocdb-core/src/command_codec.rs`:
- Around line 273-286: The existing test revoke_and_reclaim_round_trip only
checks encoder/decoder round-trip; add deterministic raw-byte assertions to pin
the wire tags so future renumbering breaks tests. For Command::Revoke and
Command::Reclaim (the variants used in the test) compute their encoded bytes via
encode_internal_command and assert equality against hard-coded byte slices that
reflect the current wire tags (e.g., the exact bytes that encode reservation_id
and the tag values currently used, including the tag bytes for 7 and 8); keep
the round-trip decode assertions but add assert_eq!(encoded, expected_bytes) for
each variant to lock in the on-wire format for encode_internal_command and
ensure decode_internal_command still parses those exact bytes. Use the same
helper functions encode_internal_command/decode_internal_command and update test
names/comments accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7429367c-2df1-4fb0-a901-35a657fc7545
📒 Files selected for processing (2)
crates/allocdb-core/src/command_codec.rscrates/allocdb-node/src/bin/allocdb-jepsen/events.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/allocdb-node/src/bin/allocdb-jepsen/events.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: semgrep-cloud-platform/scan
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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-core/src/command_codec.rs
🔇 Additional comments (2)
crates/allocdb-core/src/command_codec.rs (2)
112-119: Good call on preserving the legacy tag layout.Appending
RevokeandReclaimafterExpirekeeps existing tag-6frames decodable during replay.Also applies to: 164-173
288-303: Nice regression guard for legacyExpirepayloads.Feeding a raw tag-
6frame through the decoder is exactly the right coverage for replay compatibility.
|
Addressed the remaining CodeRabbit feedback by pinning the Validation rerun: @coderabbitai review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
1 participant
Summary
RevokeandReclaimcommands withactive -> revoking -> revokedsemantics and lease-epoch bumps on revokeCloses #85.
Validation
./scripts/preflight.sh