Storage operation correctness#1964
Storage operation correctness#1964cursor[bot] wants to merge 1 commit intofeat/sim-node-storage-infrafrom
Conversation
…d writes - Fix hierarchical insertion creating self-referencing cycles when entity key bytes after position depth are all zeros. Now skips intermediate nodes that would equal the entity's storage_id. - Fix replayed buffered writes in SyncComplete handler not being recorded in work metrics. Now calls record_write() for each replayed operation, consistent with the GossipDelta handler.
|
Cursor Agent can help with this pull request. Just |
|
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: 196.3s
🟡 1 warnings, 💡 1 suggestions. See inline comments.
🤖 Generated by AI Code Reviewer | Review ID: review-f4946ce9
| let mut intermediate_key = [0u8; 32]; | ||
| intermediate_key[..d].copy_from_slice(&key[..d]); | ||
|
|
||
| let intermediate_id = Id::new(intermediate_key); |
There was a problem hiding this comment.
🟡 Edge case fix lacks dedicated test coverage
The PR description acknowledges that 'a dedicated test case for EntityId::from_u64(0) with depth >= 1 could be added' but doesn't include one; this self-referencing cycle fix is a critical correctness guard that should have explicit test coverage.
Suggested fix:
Add a unit test that explicitly creates an entity with EntityId::from_u64(0) at depth >= 1 and verifies no cycle is created and the hierarchy is valid.
| let mut intermediate_key = [0u8; 32]; | ||
| intermediate_key[..d].copy_from_slice(&key[..d]); | ||
|
|
||
| let intermediate_id = Id::new(intermediate_key); |
There was a problem hiding this comment.
💡 Comment could clarify resulting hierarchy behavior
The comment explains that it prevents self-referencing cycles but doesn't mention that skipping these intermediate nodes results in a shallower hierarchy than the requested depth.
Suggested fix:
Expand comment to: '// Skip if this intermediate ID equals the entity's storage ID. This prevents self-referencing cycles when key[depth..] are all zeros, resulting in a shallower hierarchy at the matching entity's storage_id level.'
2 participants
Node Simulation: Fix hierarchical insertion cycle and buffered write metrics
Description
This PR addresses two issues in the node simulation:
insert_entity_hierarchicalcould create a self-referencing cycle if an entity'sstorage_idwas identical to an intermediate node ID (e.g.,EntityId::from_u64(0)withdepth >= 1), leading to infinite recursion. The fix prevents an intermediate node from being created if its ID matches the entity'sstorage_id, thus avoiding the entity becoming its own parent.SyncCompletehandler failed to recordself.metrics.work.record_write()for storage operations replayed from the buffer, causing an undercount of actual storage writes. The fix adds therecord_write()call for each replayed buffered operation, ensuring consistent metric tracking.Test plan
The changes are within the
crates/node/tests/sync_simmodule.SyncCompleteevents and buffered operations should now correctly reflect thework.writescount.EntityId::from_u64(0)withdepth >= 1could be added to explicitly verify this fix. The current simulation tests are expected to pass without issues.Documentation update
No public or internal documentation updates are required as these changes are confined to test utilities.