refactor(mesh): make OperationLog strategy-free; engines own merge/compact - #1560
Conversation
…d compact After the per-namespace engine split, every callsite of OperationLog's `*_with_strategy` methods passed a constant `|_| MergeStrategy::Foo` closure - the parameter existed for a world where one shared log served multiple strategies and had to dispatch per key, but each engine's log now holds only its own strategy's ops. The closure became dead architecture (PR #1540 migration step 6). Strip the strategy-aware surface from `OperationLog`: - Removed: `append_with_strategy`, `merge_with_strategy`, `compact_with_strategy`, `latest_operations_by_key_with_strategy`, `latest_lww_operation`, `latest_epoch_max_wins_operation`, `snapshot_and_truncate` (dead public API - no production callers). - `OperationLog::append` is now strategy-free with no auto-compaction; engines manage their own compaction policy. - New `OperationLog::compact_by_key(fold)` helper performs the group-by-key + per-key fold step that both engines need, taking the fold function as a parameter so the log itself stays strategy-agnostic. - New `OperationLog::operations_mut()` exposes the underlying vec for in-place merging by engines. - `MergeStrategy` no longer appears in `OperationLog`. The enum stays on `CrdtOrMap::register_merge_strategy` as the engine-selection knob, but is no longer threaded through the log on every operation. `LwwEngine` and `RateLimitEngine` now own the strategy decision end to end. Both define a private per-key fold (LWW: max by (timestamp, replica_id); EpochMaxWins: `compact_operations` over the group), have a `compact_log` method that runs `compact_by_key` with their own fold plus the same truncate-oldest safety valve for the many-thousand-unique-key pathology, and run their own op-id collision policy inline in `apply_remote_ops`: LWW dedups by op-id (skip if already present); EpochMaxWins folds via `compact_operations` so a compacted snapshot replaces a previously-seen raw payload at the same op-id (the bug class addressed in #1469). Test updates: - The 4 `snapshot_and_truncate` test sites in `tests.rs` use the new `compact_by_key` helper directly with `epoch_max_wins::compact_operations` as the fold. The properties tested (tombstone selection order independence, post-tombstone insert revival, etc.) are unchanged. - `test_operation_log_merge_deduplicates` (which tested LWW dedup via the deleted `merge_with_strategy`) is replaced by `test_lww_apply_remote_ops_is_idempotent` which exercises the same property through the public `CrdtOrMap::merge` API now that LWW dedup lives in `LwwEngine::apply_remote_ops`. Net: -30 lines. 150 mesh tests pass unchanged in semantics. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughOperationLog made append-only and strategy-agnostic. LwwEngine and RateLimitEngine now append ops directly and perform per-key compaction externally (LWW / EpochMaxWins folds), with safety truncation fallbacks. Tests updated to use direct per-key compaction helpers and assert winners after compaction. ChangesCRDT Operation Log Architecture
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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 |
There was a problem hiding this comment.
Code Review
This pull request refactors the CRDT operation log and engine merge/compaction logic to be strategy-agnostic. The OperationLog now acts as a simple append-only log, delegating the merge collision policies and compaction folding rules to the specific engines (LwwEngine and RateLimitEngine). Feedback focuses on optimizing performance: first, by avoiding redundant filtering of already-unseen operations under the write lock in LwwEngine::apply_remote_ops; second, by eliminating heap allocations during compact_by_key in OperationLog by sorting and grouping operations in-place instead of using a HashMap with cloned keys.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 348346d567
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Clean, well-structured refactoring. The strategy logic is correctly lifted from OperationLog into each engine where the typed knowledge already lives. Key observations from review:
compact_by_keyis correct: drains ops, groups by key, folds per-group via caller-supplied closure, sorts by(timestamp, replica_id). No data loss paths (fold only returnsNonefor empty groups, which can't happen).- LWW merge policy (dedup by op-id) is correctly implemented under the write lock with fresh
local_op_ids, avoiding TOCTOU issues from the earlier read-lock pass. - EpochMaxWins merge policy (fold collisions via
compact_operations) correctly preserves the #1469 fix — compacted snapshots replace raw payloads at the same op-id. - Safety valve (truncate-oldest when compaction isn't enough) is faithfully moved to both engines with the same 3/4 keep ratio.
- Tests are updated to exercise the real code paths through
CrdtOrMapandcompact_by_keyinstead of the deleted methods, with unchanged correctness properties.
Both engine `compact_log` helpers previously did `compact_by_key` plus a truncate-oldest safety valve in one step. `append_op` called this on local writes, and `apply_remote_ops` reused it after absorbing remote batches. That second call introduced a behavior regression versus the pre-refactor `OperationLog::merge_with_strategy` / `compact_with_strategy` pair, which compacted but never truncated. When a remote log carries more than 10K distinct keys, the truncating path on `apply_remote_ops` drops the oldest entries from the local operation log even though the live store just accepted them. Since `CrdtOrMap::get_operation_log` exports only the log, any downstream peer syncing from this node would silently miss those keys unless it could also reach a peer that still had them. Split each engine's compaction into: - `compact_log`: pure compact (no truncate). Called by `apply_remote_ops` after absorbing the incoming batch. Cannot drop remotely-learned keys. - `compact_log_and_truncate`: compact plus truncate-oldest safety valve. Called only by `append_op` after a local write. The truncate there is the same as before; the local-write path was already responsible for it pre-refactor. Also drop the redundant write-lock `local_op_ids` rebuild in `LwwEngine::apply_remote_ops`: the `unseen` Vec computed under the read lock already excludes ops the local log has seen, so append it directly instead of re-filtering `ops` under the write lock. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
The HashMap<String, Vec<Operation>> grouping required cloning each op's key into an owned `String` to use as a map key. At the auto-compact threshold (10K ops), that's 10K throwaway allocations per compaction. Sort the operation vector in place by key (`sort_unstable_by` against `&str`, no allocation), then scan contiguous runs. The fold is applied to each contiguous slice directly. Keys are compared as borrowed `&str`; the only remaining allocation is the result vec. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
The previous `compact_log` / `compact_log_and_truncate` helper pair hid a meaningful behavior difference behind a name: one collapsed duplicates, the other also dropped keys. Readers had to know which helper each callsite used to understand whether the path could drop remotely-learned keys. Inline the truncate block directly inside `append_op` so the asymmetry between the local-write path (compact + safety-valve truncate) and the remote-apply path (compact only) is visible at the code level. The "why" comment lives next to the truncate where it actually matters, not behind a method name. `compact_log` is now the single helper for compaction, used by both `append_op` and `apply_remote_ops`. The structural shape - "`apply_remote_ops` literally has no truncate code in its path" - is now visible by inspection. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
Two small wins on the remote-merge path: - The `unseen` filter now consumes `ops` via `into_iter` instead of borrowing and cloning each surviving op. Each `Operation::Insert` carries a `Vec<u8>` payload; the previous `iter().filter(...).cloned()` cloned the payload bytes for every op that made it past the dedup check. Net effect: zero clones in the filter step. - Add an early return when `unseen` is empty. A remote apply that learns nothing new (every op already in the local log) previously still acquired the log write lock, ran a full `sort_unstable_by(key)` + scan + re-sort over the entire log via `compact_log`, and entered the per-op clock/state replay loop. None of that is needed when there's nothing to absorb. No semantic change. All 150 mesh tests pass unchanged. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/mesh/src/crdt_kv/engine/lww.rs`:
- Around line 237-254: The truncation can remove the authoritative winner for a
key causing stale LWW exports; before performing the drain in the LwwEngine code
(the block using compact_log, OperationLog::AUTO_COMPACT_THRESHOLD and
log.operations_mut().drain), compute the current per-key winning operation
(using the same tie-breaker logic as
compact_log/record_insert_metadata/record_remove_metadata) and ensure at least
that winning entry is retained when selecting which oldest entries to drop (skip
draining any entry that is the winner for its key); alternatively, set a
"non_authoritative" flag on the OperationLog when truncation happens and ensure
apply_remote_ops/export/gossip checks that flag and refuses to advertise entries
from a truncated log. Ensure you update the drain logic to preserve winner
entries (or set the flag) so apply_remote_ops and record_* semantics remain
consistent.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d55006b7-2fee-44f1-b79a-c63803fe2e9c
📒 Files selected for processing (2)
crates/mesh/src/crdt_kv/engine/lww.rscrates/mesh/src/crdt_kv/engine/rate_limit.rs
Both engines had a near-verbatim drain block in `append_op`: after compaction, if the log is still over `AUTO_COMPACT_THRESHOLD`, drop the oldest entries down to 75% of the threshold. The only difference between the two copies was the engine name in the log message. Extract the drain into `OperationLog::truncate_oldest_over_threshold`, which returns the number of entries dropped so each engine can log with its own context. The drain itself is strategy-agnostic (it just trims the op vector); only `compact_log` (the per-key fold) stays engine-specific, so this keeps OperationLog strategy-free. The helper's docs also state the two constraints that were previously buried in duplicated comments: (1) it's a memory backstop that only fires when distinct live keys exceed the threshold - far beyond the design's expected key count - and trades convergence for the dropped keys against unbounded growth; (2) callers must invoke it only on the local-write path, never on remote-merge, or it would shed remotely-learned live keys and break downstream sync. No behavior change. 150 mesh tests pass. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
Description
Problem
After #1539 split CRDT logic into per-namespace engines and #1549 made the rate-limit engine typed, every callsite of
OperationLog's strategy-aware methods (append_with_strategy,merge_with_strategy,compact_with_strategy) passed a constant closure:The closure parameter existed for a world where one shared log served multiple strategies and had to dispatch per-key. Now each engine's log only ever holds its own strategy's ops, so the parameter is dead architecture — but every callsite still has to remember to pass it, and
OperationLogstill carries strategy knowledge it doesn't need.Plus
OperationLog::snapshot_and_truncatewas public API with zero production callers (only 4 test sites referenced it).Solution
Make
OperationLogstrategy-free; move merge and compact policy into each engine, where the typed knowledge already lives. This closes migration step 6 from #1540 ("MakeOperationLog::mergenon-public or engine-owned, so there is no default-to-LWW path for non-LWW data").Old vs new callsite flow
Before this PR — strategy knowledge threaded through OperationLog on every call:
After this PR — strategy knowledge lives in the engine; OperationLog is pure data:
Two structural properties the new shape makes visible:
Strategy lives with the engine. Each engine names its own per-key
foldfunction (lww_foldorepoch_max_wins_fold) and its own op-id collision policy. No closure of strategy enums threaded through anything. Adding a third engine doesn't touchOperationLog.Truncate-oldest fires only on the local-write path. The safety valve is inlined inside
append_op, not behind a helper thatapply_remote_opscould accidentally call. Truncating inapply_remote_opswould silently drop remotely-learned keys from the log (still live in state) and break downstream sync from this node — the asymmetry is now structural, not just policy.Changes
crates/mesh/src/crdt_kv/operation.rs—OperationLogbecomes a thin strategy-free log:append_with_strategy,merge_with_strategy,compact_with_strategy,latest_operations_by_key_with_strategy,latest_lww_operation,latest_epoch_max_wins_operation,snapshot_and_truncate,operation_id(unused).append(op)is now strategy-free, no auto-compact.compact_by_key(fold): strategy-agnostic group-by-key + per-key fold helper, using in-place sort-and-scan to avoid per-opStringallocations (~10K allocations saved per compaction at threshold).operations_mut()for in-place merging by engines.AUTO_COMPACT_THRESHOLDexposed aspub(super) constfor engines to gate their own auto-compact policy.MergeStrategyno longer appears in this file.crates/mesh/src/crdt_kv/engine/lww.rs— owns LWW merge and compaction:lww_fold: per-key fold returns the op with max(timestamp, replica_id).compact_log: runscompact_by_key(lww_fold). Used by bothappend_opandapply_remote_ops. Never drops keys.append_op: appends + if oversized, compacts + inline safety-valve truncate.apply_remote_ops: pre-filtersunseenops by op-id under the read lock, appends them under the write lock, compacts (no truncate). LWW op-id collision policy is dedup.crates/mesh/src/crdt_kv/engine/rate_limit.rs— owns EpochMaxWins merge and compaction:epoch_max_wins_fold: per-key fold delegates toepoch_max_wins::compact_operations.compact_log: runscompact_by_key(epoch_max_wins_fold). Same shape as LWW. Never drops keys.append_op: same auto-compact pattern with inline safety-valve truncate.apply_remote_ops: EMW op-id collision policy is fold, not dedup — when an incoming op matches a local op's(replica_id, timestamp), fold the pair viacompact_operationsso a compacted snapshot replaces a previously-seen raw payload at the same op-id. Preserves the fix(mesh): wire EpochMaxWins into CRDT merge #1469 fix.crates/mesh/src/crdt_kv/tests.rs— test updates:snapshot_and_truncatesites use the newcompact_by_keyhelper directly withepoch_max_wins::compact_operationsas the fold. Properties tested unchanged.test_operation_log_merge_deduplicates(tested LWW dedup via the deletedmerge_with_strategy) is replaced bytest_lww_apply_remote_ops_is_idempotentexercising the same property throughCrdtOrMap::merge.Test Plan
cargo test -p smg-mesh --lib— 150 passed, 0 failedcargo clippy -p smg-mesh --all-targets— cleancargo check --workspace— cleanNet: +222 / -250 ≈ -28 lines across the four files.
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
Bug Fixes
Performance & Reliability
Tests