Draft
Conversation
Add comprehensive simulation tests verifying delta buffering behavior during state-based sync (Invariant I6: deltas arriving during sync MUST be preserved and applied after sync completes). Changes: - Enhanced DeltaBuffer with FIFO eviction (oldest-first drop policy) - Added drops counter and configurable capacity - Integrated cancel_sync_session for proper error handling - Added 7 simulation test scenarios covering: - Deltas buffered during active sync - Buffered deltas replayed on completion - Immediate application when idle - Buffer cleared on crash - Multiple deltas preserved in FIFO order - Convergence blocked while buffer non-empty - Complex snapshot sync with concurrent writes All 176 sync_sim tests pass.
Addresses all review comments from PR #1959: **Bug Fixes:** - Fix zero capacity edge case in DeltaBuffer::push - now correctly drops incoming delta and returns its ID instead of phantom drops - Change push() return type to Option<[u8; 32]> to return the evicted delta's ID (fixes misleading log message issue) - Add record_write() metrics in SyncComplete replay loop for consistent observability **Code Quality:** - Remove dead code get_buffer_metrics() with #[allow(dead_code)] - Add finish_sync() method on SimNode to atomically drain buffered ops, clear buffer, and reset sync state (prevents partial cleanup) - Extract rate-limit duration as DELTA_BUFFER_DROP_WARNING_RATE_LIMIT_S constant **Test Coverage:** - Add test_zero_capacity_drops_immediately unit test - Add test_buffer_overflow_fifo_eviction simulation test - Add test_buffer_drops_accumulated simulation test - Add buffer_drops metric to EffectMetrics All 178 sync_sim tests pass, 7 delta_buffer unit tests pass.
Add explicit early-return guard for zero-capacity edge case in SimNode::buffer_delta to match production DeltaBuffer::push behavior. Previously, when buffer_capacity was 0: - The condition '0 >= 0' was true, entering the eviction branch - But first() returned None on the empty buffer - The if-let block was skipped (buffer_drops not incremented) - The push() call still executed, incorrectly adding the delta Now, when buffer_capacity is 0: - We immediately increment buffer_drops and return false - The buffer remains empty (matching production behavior) - The drop is properly counted
Contributor
Author
|
Cursor Agent can help with this pull request. Just |
3 tasks
|
Your PR title does not adhere to the Conventional Commits convention: Common errors to avoid:
|
![meroreviewer[bot]](https://avatars.githubusercontent.com/in/2793509?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 3 agents | Quality score: 100% | Review time: 203.6s
💡 1 suggestions. See inline comments.
🤖 Generated by AI Code Reviewer | Review ID: review-f7e6a03f
| /// | ||
| /// Implements FIFO eviction when buffer is full. Returns `true` if added without | ||
| /// eviction, `false` if oldest delta was evicted. | ||
| /// eviction, `false` if oldest delta was evicted or dropped. |
There was a problem hiding this comment.
💡 Consider adding explicit unit test for zero-capacity edge case
Since this bug was discovered in test infrastructure, adding a regression test specifically for buffer_delta with buffer_capacity == 0 would prevent future regressions.
Suggested fix:
Add a test like `#[test] fn buffer_delta_zero_capacity_drops_immediately() { let mut node = SimNode::with_buffer_capacity("n1", 0); assert!(!node.buffer_delta(DeltaId::ZERO)); assert_eq!(node.buffer_drops, 1); }`
|
This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
node: Fix SimNode buffer_delta zero capacity edge case
Description
This PR fixes a bug in
SimNode::buffer_deltawhere it mishandled the zero-capacity edge case, diverging from the productionDeltaBuffer::pushbehavior.Specifically, when
buffer_capacitywas 0,SimNode::buffer_deltawould incorrectly add deltas to the buffer and fail to incrementbuffer_drops.This change adds an explicit zero-capacity guard to
SimNode::buffer_deltato match the productionDeltaBuffer::pushimplementation. This ensures that deltas are immediately dropped andbuffer_dropsis incremented when capacity is zero, aligning the simulation with production behavior for accurate testing of delta buffer invariants.Fixes bug
2e1fad8b-f396-4a24-89d0-15b1c51e48ad: SimNode buffer_delta mishandles zero capacity edge case.Test plan
The existing
cargo testsuite was run.cargo checkandcargo clippypassed, confirming syntactic correctness and no new warnings. The fix directly addresses the identified logic divergence, ensuring existing simulation tests now correctly reflect production behavior for zero-capacity buffers.Documentation update
None. This is an internal simulation fix.