Conversation
Summary by CodeRabbit
WalkthroughThis PR introduces support for a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/reservation-core/src/snapshot.rs (1)
546-558:⚠️ Potential issue | 🟡 MinorMake the fixture reflect the post-extension state.
Line 550 still leaves the hold at
Slot(5), while Lines 556-558 record a successfulExtendHoldtoSlot(7). That fixture cannot come from the live state machine, so the restore test no longer validates the extended-deadline state this PR is adding.As per coding guidelines, "Write extensive tests for every meaningful behavior change. Favor invariant tests, negative-path tests, recovery tests, and regression tests over shallow happy-path coverage."🧪 Minimal fix
holds: vec![HoldRecord { hold_id: HoldId(21), pool_id: PoolId(11), quantity: 2, - deadline_slot: Slot(5), + deadline_slot: Slot(7), state: HoldState::Held, }],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/reservation-core/src/snapshot.rs` around lines 546 - 558, The fixture's held record does not reflect the successful ExtendHold operation: update the HoldRecord used in the snapshot (the holds vec containing HoldRecord with HoldId(21), PoolId(11), etc.) so its deadline_slot is Slot(7) instead of Slot(5) to match the OperationRecord that records Command::ExtendHold { hold_id: HoldId(21), deadline_slot: Slot(7) }; ensure HoldState::Held remains unchanged and no other fields are altered.
🧹 Nitpick comments (1)
crates/reservation-core/src/command_codec.rs (1)
198-205: Keep the old variant coverage when adding the new round-trip case.Swapping the single test input to
ExtendHolddrops direct regression coverage forCreatePool. A small table-driven test over allCommandvariants would keep both existing tags and the new tag pinned.As per coding guidelines, "Write extensive tests for every meaningful behavior change. Favor invariant tests, negative-path tests, recovery tests, and regression tests over shallow happy-path coverage."🧪 Possible expansion
#[test] -fn internal_command_round_trips() { - let command = Command::ExtendHold { - hold_id: HoldId(7), - deadline_slot: Slot(9), - }; - - let decoded = decode_internal_command(&encode_internal_command(command)).unwrap(); - assert_eq!(decoded, command); +fn internal_commands_round_trip() { + let commands = [ + Command::CreatePool { + pool_id: PoolId(5), + total_capacity: 9, + }, + Command::PlaceHold { + pool_id: PoolId(5), + hold_id: HoldId(6), + quantity: 2, + deadline_slot: Slot(7), + }, + Command::ConfirmHold { hold_id: HoldId(6) }, + Command::ReleaseHold { hold_id: HoldId(6) }, + Command::ExtendHold { + hold_id: HoldId(6), + deadline_slot: Slot(8), + }, + Command::ExpireHold { hold_id: HoldId(6) }, + ]; + + for command in commands { + let decoded = decode_internal_command(&encode_internal_command(command)).unwrap(); + assert_eq!(decoded, command); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/reservation-core/src/command_codec.rs` around lines 198 - 205, Replace the single-case test in internal_command_round_trips with a table-driven loop that iterates over all Command variants (including CreatePool and the new ExtendHold), calling encode_internal_command and decode_internal_command for each and asserting equality to preserve regression coverage; locate the test in internal_command_round_trips and build a Vec or array of Command instances, then for each element call decode_internal_command(&encode_internal_command(cmd)).unwrap() and assert_eq!(decoded, cmd).
🤖 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/reservation-core/src/state_machine.rs`:
- Around line 477-479: The code currently always calls
self.push_hold_expiry(hold_id, deadline_slot) after updating hold.deadline_slot
and self.replace_hold(hold), which enqueues a duplicate entry into
hold_retire_queue and can hit RetireQueueError::Full (e.g., max_holds = 1) when
PlaceHold → ExtendHold occurs; change push_hold_expiry to first check for an
existing queued entry for hold_id and either update its deadline in-place or
skip adding a new entry (i.e., deduplicate keyed by hold_id), or provide an API
on the retire queue to reschedule an existing entry instead of pushing; update
the implementation around self.push_hold_expiry/self.replace_hold to reschedule
rather than append, and add a regression test (max_holds = 1) that PlaceHold
followed by ExtendHold does not panic and results in only a single retire entry
for the hold_id.
---
Outside diff comments:
In `@crates/reservation-core/src/snapshot.rs`:
- Around line 546-558: The fixture's held record does not reflect the successful
ExtendHold operation: update the HoldRecord used in the snapshot (the holds vec
containing HoldRecord with HoldId(21), PoolId(11), etc.) so its deadline_slot is
Slot(7) instead of Slot(5) to match the OperationRecord that records
Command::ExtendHold { hold_id: HoldId(21), deadline_slot: Slot(7) }; ensure
HoldState::Held remains unchanged and no other fields are altered.
---
Nitpick comments:
In `@crates/reservation-core/src/command_codec.rs`:
- Around line 198-205: Replace the single-case test in
internal_command_round_trips with a table-driven loop that iterates over all
Command variants (including CreatePool and the new ExtendHold), calling
encode_internal_command and decode_internal_command for each and asserting
equality to preserve regression coverage; locate the test in
internal_command_round_trips and build a Vec or array of Command instances, then
for each element call
decode_internal_command(&encode_internal_command(cmd)).unwrap() and
assert_eq!(decoded, cmd).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 912c9dea-3d58-45b9-be0d-ea96c35137e2
📒 Files selected for processing (5)
crates/reservation-core/src/command.rscrates/reservation-core/src/command_codec.rscrates/reservation-core/src/recovery.rscrates/reservation-core/src/snapshot.rscrates/reservation-core/src/state_machine.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/reservation-core/src/command_codec.rscrates/reservation-core/src/command.rscrates/reservation-core/src/snapshot.rscrates/reservation-core/src/recovery.rscrates/reservation-core/src/state_machine.rs
🔇 Additional comments (5)
crates/reservation-core/src/command.rs (1)
8-8: LGTM.The new tag and explicit
Command::ExtendHoldvariant keep the command surface and wire tags aligned.Also applies to: 47-50
crates/reservation-core/src/snapshot.rs (1)
369-376: LGTM.
ExtendHoldis encoded and decoded with the same field order, so snapshot round-tripping stays symmetric.Also applies to: 402-405
crates/reservation-core/src/command_codec.rs (1)
82-89: LGTM.The new wire tag is serialized and parsed consistently, so
ExtendHoldround-trips cleanly through the codec.Also applies to: 115-118
crates/reservation-core/src/state_machine.rs (1)
732-734: Good stale-expiry guard.This check prevents an old queued deadline from expiring a hold before its current
deadline_slot.crates/reservation-core/src/recovery.rs (1)
306-339: Nice recovery regression.The helpers keep the scenario deterministic, and the new test proves an extended hold survives replay plus later logical-slot advancement.
Also applies to: 521-620
| hold.deadline_slot = deadline_slot; | ||
| self.replace_hold(hold); | ||
| self.push_hold_expiry(hold_id, deadline_slot); |
There was a problem hiding this comment.
Do not enqueue a second expiry entry for the same hold.
Line 479 appends another retire entry even though the old deadline entry is still in hold_retire_queue. Because that queue is allocated with max_holds, the sequence PlaceHold → ExtendHold already hits the RetireQueueError::Full panic path when max_holds = 1, and the same thing happens any time the queue is otherwise full. This needs an in-place reschedule or dedup keyed by hold_id, not an additional queue entry. Please also pin it with a max_holds = 1 regression.
As per coding guidelines, "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/reservation-core/src/state_machine.rs` around lines 477 - 479, The
code currently always calls self.push_hold_expiry(hold_id, deadline_slot) after
updating hold.deadline_slot and self.replace_hold(hold), which enqueues a
duplicate entry into hold_retire_queue and can hit RetireQueueError::Full (e.g.,
max_holds = 1) when PlaceHold → ExtendHold occurs; change push_hold_expiry to
first check for an existing queued entry for hold_id and either update its
deadline in-place or skip adding a new entry (i.e., deduplicate keyed by
hold_id), or provide an API on the retire queue to reschedule an existing entry
instead of pushing; update the implementation around
self.push_hold_expiry/self.replace_hold to reschedule rather than append, and
add a regression test (max_holds = 1) that PlaceHold followed by ExtendHold does
not panic and results in only a single retire entry for the hold_id.
1 participant
Summary
ExtendHoldtoreservation-corecommands, codecs, snapshot encoding, and live state transitionsVerification
cargo test -p reservation-corecargo clippy -p reservation-core --all-targets --all-features -- -D warningscargo fmt --all --checkcargo testscripts/check_repo.shRefs #128
Closes #129