fix(mesh): wire EpochMaxWins into CRDT merge#1469
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a per-prefix MergeStrategy registry (longest-prefix match), introduces MergeStrategy, refactors EpochMaxWins to normalized shard/tombstone state, and threads strategy-aware compaction/merge through OperationLog and CrdtOrMap; tests and MeshKV/adapter wiring updated. ChangesPer-Prefix Merge Strategies for CRDT with Shard-Aware EpochMaxWins
Sequence Diagram(s)sequenceDiagram
participant Client
participant CrdtOrMap
participant OperationLog
participant EpochMaxModule
Client->>CrdtOrMap: insert(key, value)
CrdtOrMap->>CrdtOrMap: get_strategy_for_key(key)
alt EpochMaxWins
CrdtOrMap->>EpochMaxModule: record_epoch_insert_metadata(value, metadata)
EpochMaxModule->>CrdtOrMap: maybe update stored value/metadata
else LastWriterWins
CrdtOrMap->>CrdtOrMap: record_lww_insert_metadata(value, metadata)
end
CrdtOrMap->>OperationLog: append_with_strategy(operation, strategy)
OperationLog->>OperationLog: compact_with_strategy(strategy)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6149e363e0
ℹ️ 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.
Code Review
This pull request introduces a configurable MergeStrategy framework for the CRDT key-value store, enabling prefix-based conflict resolution policies such as LastWriterWins and EpochMaxWins. The implementation includes updates to the OperationLog for strategy-aware compaction and the addition of record_epoch_insert_metadata to handle epoch-based versioning. Review feedback highlights several performance optimization opportunities, including avoiding Vec allocations during value comparisons by decoding into structs and reducing lock contention by hoisting read locks out of tight loops during log compaction. Additionally, a correction was suggested for tombstone handling in metadata records to ensure consistency.
There was a problem hiding this comment.
Solid PR — the EpochMaxWins strategy is correctly wired through the CRDT merge, compaction, and operation-log paths. The candidate_wins logic correctly falls through to LWW for Remove operations and non-Insert pairs, and the record_epoch_insert_metadata correctly only suppresses on duplicate entries or newer tombstones (not newer non-tombstone inserts, which is the right call for value-based merge).
Summary: 0 🔴 Important · 1 🟡 Nit · 0 🟣 Pre-existing
The one nit: snapshot_and_truncate was not updated with a _with_strategy variant like compact and append were. No current callers, but it's a public method that would silently use LWW for EpochMaxWins keys.
Tests cover the core epoch-wins-over-stale-count and tombstone-wins-over-stale-insert scenarios well.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/mesh/src/crdt_kv/crdt.rs (1)
181-191:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRoute local insert-like writes through the strategy-aware merge path.
These methods still call
record_insert_metadata()and overwriteself.storedirectly before theEpochMaxWinslogic inapply_insert()ever runs. For example, on anrl:key a later local write ofencode(5, 100)will replace an existingencode(6, 0)even though merge/compaction would keep epoch 6. The local write path needs to reuse the same strategy-specific resolution as replicated inserts, and it needs a regression test for that case.Also applies to: 220-230, 260-270, 299-310
🤖 Prompt for 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. In `@crates/mesh/src/crdt_kv/crdt.rs` around lines 181 - 191, The local insert path currently calls record_insert_metadata() and directly updates self.store via self.store.upsert before the strategy-specific merge logic in apply_insert()/EpochMaxWins runs; change the local insert-like handlers (the blocks that call record_insert_metadata and self.store.upsert, and then append_operation) to instead build the same Operation::insert and route it through the same merge/application path used for replicated ops (e.g., call apply_insert or the common apply_operation path after append_operation) so the EpochMaxWins strategy and rl: key semantics are respected; update the code blocks around the current self.store.upsert usage (and similar blocks at the other noted ranges) to stop writing the store directly and reuse apply_insert, and add a regression test that performs a local write encode(5,100) against an existing encode(6,0) on an rl: key to assert epoch 6 is preserved.
🤖 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/merge_strategy.rs`:
- Around line 8-9: The public enum variant MergeStrategy::MaxValueWins must not
be usable until its semantics are implemented; either remove the MaxValueWins
variant from the public MergeStrategy API or add a validation check wherever
MergeStrategy is instantiated/configured to reject MaxValueWins with an explicit
error. Locate the MergeStrategy definition and either make MaxValueWins
non-public/temporary (remove or comment out) or add a guard in the
configuration/constructor path (the code that parses or returns a MergeStrategy)
that returns an Err or validation failure if MergeStrategy::MaxValueWins is
selected; also add a unit test to assert the new validation so callers cannot
silently pick MaxValueWins while apply/compaction still treat it like
LastWriterWins.
In `@crates/mesh/src/crdt_kv/operation.rs`:
- Around line 252-255: snapshot_and_truncate() currently hardcodes
MergeStrategy::LastWriterWins when calling
latest_operations_by_key_with_strategy; change it to look up the per-key
registered merge strategy (the same lookup logic used by
compact_with_strategy()) so each key uses its configured MergeStrategy (e.g.,
EpochMaxWins vs LastWriterWins) during snapshotting and compaction; update the
closure passed to latest_operations_by_key_with_strategy in
snapshot_and_truncate() to retrieve the strategy for the given key instead of
always returning LastWriterWins.
---
Outside diff comments:
In `@crates/mesh/src/crdt_kv/crdt.rs`:
- Around line 181-191: The local insert path currently calls
record_insert_metadata() and directly updates self.store via self.store.upsert
before the strategy-specific merge logic in apply_insert()/EpochMaxWins runs;
change the local insert-like handlers (the blocks that call
record_insert_metadata and self.store.upsert, and then append_operation) to
instead build the same Operation::insert and route it through the same
merge/application path used for replicated ops (e.g., call apply_insert or the
common apply_operation path after append_operation) so the EpochMaxWins strategy
and rl: key semantics are respected; update the code blocks around the current
self.store.upsert usage (and similar blocks at the other noted ranges) to stop
writing the store directly and reuse apply_insert, and add a regression test
that performs a local write encode(5,100) against an existing encode(6,0) on an
rl: key to assert epoch 6 is preserved.
🪄 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: 3f208cf6-27bf-43d1-854a-329467fc10b7
📒 Files selected for processing (7)
crates/mesh/src/crdt_kv/crdt.rscrates/mesh/src/crdt_kv/merge_strategy.rscrates/mesh/src/crdt_kv/mod.rscrates/mesh/src/crdt_kv/operation.rscrates/mesh/src/crdt_kv/tests.rscrates/mesh/src/kv.rscrates/mesh/src/lib.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf16fcaf0d
ℹ️ 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.
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/crdt.rs`:
- Around line 623-656: The merge logic currently treats any incoming bytes equal
to the incoming value as a "candidate wins" and rewrites stored epoch metadata
even when the merged bytes equal the existing local value, which can rewind the
stored (timestamp, replica_id); update the decision so we only replace
versions/new_metadata when the merged bytes actually differ from the current
stored bytes or when there is no current value. Concretely, change the
computation of candidate_wins_value and the subsequent check around
current/is_some() so that if current.as_deref() == Some(merged.as_slice()) you
do not clear/replace versions (i.e., preserve existing versions), referencing
the symbols candidate_wins_value, merged, current, versions, new_metadata,
epoch_max_wins::merge, and Self::compact_key_metadata to locate the code to
modify.
In `@crates/mesh/src/crdt_kv/operation.rs`:
- Around line 169-195: candidate_wins currently mixes epoch-based insert
comparisons with timestamp-based insert/remove comparisons causing
non-transitive ordering; change the MergeStrategy::EpochMaxWins branch (function
candidate_wins and the caller latest_operations_by_key_with_strategy) to stop
doing pairwise epoch-vs-timestamp reductions. Instead, for a given key compute
the epoch-max Insert winner across all Insert operations using
epoch_max_wins::winner semantics, separately compute the newest Tombstone/Remove
by (timestamp, replica_id), then compare those two deterministic winners
(epoch-max insert vs newest tombstone) using a single consistent rule to decide
survival; update candidate_wins to call those helpers or delegate to a new
function so the EpochMaxWins path makes a holistic decision rather than falling
back to the (timestamp, replica_id) pairwise comparison.
🪄 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: 4125fe15-96f3-4ed9-9df7-ff4d0d30ad4f
📒 Files selected for processing (6)
crates/mesh/src/crdt_kv/crdt.rscrates/mesh/src/crdt_kv/epoch_max_wins.rscrates/mesh/src/crdt_kv/kv_store.rscrates/mesh/src/crdt_kv/merge_strategy.rscrates/mesh/src/crdt_kv/operation.rscrates/mesh/src/crdt_kv/tests.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c2b791e87
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8287ddf4bb
ℹ️ 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.
Actionable comments posted: 1
🤖 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/tests.rs`:
- Around line 552-586: The test function name
test_operation_log_epoch_max_wins_equal_insert_uses_newer_timestamp is
misleading because a tombstone removes the older insert rather than exercising
equal-epoch LWW tie-breaking; rename the test to
test_operation_log_epoch_max_wins_post_tombstone_insert_wins_over_pre_tombstone_equal_epoch
(update the function identifier and any references) to reflect the scenario
being tested (the code paths around OperationLog::append,
OperationLog::snapshot_and_truncate and MergeStrategy::EpochMaxWins remain
unchanged), or alternatively modify the test to remove the tombstone (keeping
the original name) if you want to directly test equal-epoch LWW tie-breaking.
🪄 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: d46d3ccc-bc31-465f-908e-131fc429edff
📒 Files selected for processing (2)
crates/mesh/src/crdt_kv/operation.rscrates/mesh/src/crdt_kv/tests.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/epoch_max_wins.rs`:
- Around line 342-369: compact_operations currently uses the try-operator on
state_from_insert_value(...) which returns None on a malformed `rl:` insert and
causes the whole compaction to drop the key; change the loop in
compact_operations so that decoding errors from state_from_insert_value are
handled locally (for Insert only) by skipping that operation (optionally
logging) and continuing to merge other operations instead of returning None,
then proceed to merge via current.merge(operation_state) and finally call
state.into_operation(key?) as before; reference the compact_operations function,
the match arm that calls state_from_insert_value, and the merge/into_operation
paths to locate where to replace the `?`-style early-return with a
continue-on-error behavior.
🪄 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: f94d3fef-6561-4add-8bc2-e32b5760d43a
📒 Files selected for processing (6)
crates/mesh/src/crdt_kv/crdt.rscrates/mesh/src/crdt_kv/epoch_max_wins.rscrates/mesh/src/crdt_kv/operation.rscrates/mesh/src/crdt_kv/replica.rscrates/mesh/src/crdt_kv/tests.rsmodel_gateway/src/mesh/adapters/rate_limit_sync.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15e781f06d
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00dd0ba3cf
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c57f1ec86b
ℹ️ 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".
The re-exported `merge_epoch_max_wins` operates on the normalized stored shard payload, but `encode_epoch_count` returns the raw 16-byte wire payload. Calling `merge_epoch_max_wins(encode(5,30), encode(6,0))` therefore treats both inputs as malformed shards and returns local unchanged — epoch 5 wins when epoch 6 should. Production merges go through `merge_live_value` (called from `CrdtOrMap::record_epoch_insert_metadata`), not this byte-only form. Grep confirms zero callers outside the unit tests in `epoch_max_wins.rs` itself. Gate `fn merge` with `#[cfg(test)]` and drop the `merge_epoch_max_wins` re-exports from `lib.rs` and `crdt_kv/mod.rs`. The wire-side public helpers (`encode_epoch_count`, `decode_epoch_count`, `EpochCount`, `EPOCH_MAX_WINS_ENCODED_LEN`) stay public for the gateway adapter. Flagged by Codex (P2) on PR #1469. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
…Wins
When the source replica compacts a log of [pre-tombstone Insert,
Remove, post-tombstone Insert] into a single shard Insert, a peer
that merges only that snapshot never sees a Remove op. The
shard's embedded `tombstone_version` is the only thing standing
between the receiver and resurrection by a delayed pre-tombstone
high-epoch insert from a different peer.
Add a regression test that drives the exact path:
1. Source merges all three ops and compacts to one Insert.
2. Receiver applies that single op.
3. A late peer gossips its still-pre-tombstone high-epoch
Insert. Receiver must reject it.
Also document the tombstone duality on
`record_epoch_insert_metadata` so the next maintainer doesn't
treat the embedded `tombstone_version` as dead weight: it lives
alongside the `ValueMetadata { is_tombstone: true }` entry
because the two cover different propagation paths (local LWW
ordering + GC vs. cross-replica snapshot propagation).
Identified during PR #1469 design review.
Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
The re-exported `merge_epoch_max_wins` operates on the normalized stored shard payload, but `encode_epoch_count` returns the raw 16-byte wire payload. Calling `merge_epoch_max_wins(encode(5,30), encode(6,0))` therefore treats both inputs as malformed shards and returns local unchanged — epoch 5 wins when epoch 6 should. Production merges go through `merge_live_value` (called from `CrdtOrMap::record_epoch_insert_metadata`), not this byte-only form. Grep confirms zero callers outside the unit tests in `epoch_max_wins.rs` itself. Gate `fn merge` with `#[cfg(test)]` and drop the `merge_epoch_max_wins` re-exports from `lib.rs` and `crdt_kv/mod.rs`. The wire-side public helpers (`encode_epoch_count`, `decode_epoch_count`, `EpochCount`, `EPOCH_MAX_WINS_ENCODED_LEN`) stay public for the gateway adapter. Flagged by Codex (P2) on PR #1469. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
…Wins
When the source replica compacts a log of [pre-tombstone Insert,
Remove, post-tombstone Insert] into a single shard Insert, a peer
that merges only that snapshot never sees a Remove op. The
shard's embedded `tombstone_version` is the only thing standing
between the receiver and resurrection by a delayed pre-tombstone
high-epoch insert from a different peer.
Add a regression test that drives the exact path:
1. Source merges all three ops and compacts to one Insert.
2. Receiver applies that single op.
3. A late peer gossips its still-pre-tombstone high-epoch
Insert. Receiver must reject it.
Also document the tombstone duality on
`record_epoch_insert_metadata` so the next maintainer doesn't
treat the embedded `tombstone_version` as dead weight: it lives
alongside the `ValueMetadata { is_tombstone: true }` entry
because the two cover different propagation paths (local LWW
ordering + GC vs. cross-replica snapshot propagation).
Identified during PR #1469 design review.
Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
7a9f07a to
dfb1e6f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfb1e6fcad
ℹ️ 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".
Teach the CRDT OR-map to choose merge behavior by key prefix instead of always applying timestamp LWW semantics. Rate-limit namespaces register EpochMaxWins so counter resets are compared by epoch first and count second, matching the v2 protocol contract. Update operation-log compaction to use the same merge strategy as apply-time merge. Without this, a compacted log could keep the wrong rate-limit operation and later peers would rehydrate stale counter state. Add focused CRDT tests for EpochMaxWins merge, compaction, and tombstone behavior so direct v2 cutover does not silently regress rate-limit correctness. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
Route local insert/upsert paths through the same strategy-aware insert resolver used by replicated operations so stale old-epoch writes cannot overwrite newer rate-limit windows. Keep EpochMaxWins metadata aligned with the semantic value winner instead of the highest Lamport timestamp, and reject duplicate operation ids regardless of whether the existing entry is a tombstone. This lets tombstones newer than the actual epoch-winning insert remove the shard even when a stale higher-timestamp insert was seen earlier. Add regression coverage for local stale-epoch writes and tombstones after an older-timestamp epoch winner. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
Drop the public MaxValueWins variant because the CRDT apply and compaction paths do not implement max-value semantics. Keeping it configurable would let callers select a strategy that silently behaves like LastWriterWins. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
Change snapshot_and_truncate to require the same per-key merge-strategy callback used by append and compaction, removing the hidden LastWriterWins fallback for EpochMaxWins keys. Add a regression test showing an older-timestamp higher-epoch rate-limit value survives snapshot truncation. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
Add an allocation-free winner helper for EpochMaxWins and use it when operation-log compaction only needs to decide which operation survives. This keeps the merge API for actual value writes while avoiding temporary Vec allocations in the hot compaction comparison path. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
Avoid taking the merge-strategy read lock once per key while compacting the operation log. The compaction path now clones the sorted prefix registry once and performs all per-key lookups against that stable snapshot. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
Avoid repeated merge-strategy read-lock acquisition when an append crosses the operation-log compaction threshold. The append path now takes one strategy snapshot under the operation-log write guard and reuses it if compaction runs. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
Avoid treating an older incoming insert with unchanged EpochMaxWins bytes as a semantic winner. When merged bytes match the current store value, preserve or advance metadata by timestamp instead of clearing versions, so tombstone ordering cannot be rewound by delayed equal-value inserts. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
Replace pairwise mixed ordering for EpochMaxWins operation-log snapshots with a per-key survivor selection. The log now selects the epoch-winning insert, selects the newest tombstone, and compares those two once, avoiding order-dependent cycles between stale inserts, resets, and removes. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
Treat the newest tombstone as the cutoff for EpochMaxWins operation-log compaction. Pre-tombstone inserts no longer compete with later inserts by epoch/count, so a valid post-delete write can revive the key even when its epoch is lower than a deleted historical value. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
Replace the raw EpochMaxWins compaction path for rl: keys with a RateLimitShard state that stores a normalized live frontier plus the newest tombstone boundary. This keeps operation-log compaction from losing the post-tombstone value needed to reject stale pre-delete updates later. Normalize rl: writes before storing them, decode both raw epoch/count payloads and normalized shard state for the gateway adapter, and apply incoming operations before compacting the local log so synthetic compacted operations cannot hide unseen updates behind an existing operation id. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
Remove the compatibility magic/version header from normalized rl: shard values now that v2 does not need to read old stored payloads. Raw 16-byte epoch/count payloads are accepted only at the insert boundary; stored and gossiped values are serialized RateLimitShard state. Rename the rate-limit Lamport wrapper and ValueMetadata conversion helpers so CRDT metadata code says RateLimitVersion explicitly instead of a generic version() / from_live_version pair. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
The re-exported `merge_epoch_max_wins` operates on the normalized stored shard payload, but `encode_epoch_count` returns the raw 16-byte wire payload. Calling `merge_epoch_max_wins(encode(5,30), encode(6,0))` therefore treats both inputs as malformed shards and returns local unchanged — epoch 5 wins when epoch 6 should. Production merges go through `merge_live_value` (called from `CrdtOrMap::record_epoch_insert_metadata`), not this byte-only form. Grep confirms zero callers outside the unit tests in `epoch_max_wins.rs` itself. Gate `fn merge` with `#[cfg(test)]` and drop the `merge_epoch_max_wins` re-exports from `lib.rs` and `crdt_kv/mod.rs`. The wire-side public helpers (`encode_epoch_count`, `decode_epoch_count`, `EpochCount`, `EPOCH_MAX_WINS_ENCODED_LEN`) stay public for the gateway adapter. Flagged by Codex (P2) on PR #1469. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
…Wins
When the source replica compacts a log of [pre-tombstone Insert,
Remove, post-tombstone Insert] into a single shard Insert, a peer
that merges only that snapshot never sees a Remove op. The
shard's embedded `tombstone_version` is the only thing standing
between the receiver and resurrection by a delayed pre-tombstone
high-epoch insert from a different peer.
Add a regression test that drives the exact path:
1. Source merges all three ops and compacts to one Insert.
2. Receiver applies that single op.
3. A late peer gossips its still-pre-tombstone high-epoch
Insert. Receiver must reject it.
Also document the tombstone duality on
`record_epoch_insert_metadata` so the next maintainer doesn't
treat the embedded `tombstone_version` as dead weight: it lives
alongside the `ValueMetadata { is_tombstone: true }` entry
because the two cover different propagation paths (local LWW
ordering + GC vs. cross-replica snapshot propagation).
Identified during PR #1469 design review.
Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
The original branch exported `Operation` and `ReplicaId` from `crdt_kv::mod` in anticipation of d-3 needing them at the wire-layer boundary. Post-rebase against main neither type has an external caller (clippy `unused_imports`). Drop them; they can be re-added in the d-3 PR that actually needs them. No behaviour change. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
The live remove path rejected a tombstone wholesale if any single ValueMetadata entry was newer (LWW shape), but `compact_operations` filters per-point against the tombstone. For a shard frontier holding both a pre-tombstone high-epoch point and a post-tombstone low-epoch point, the two paths disagreed: the live store kept both (reporting the high epoch), while the compacted operation log kept only the survivor (reporting the low epoch). Replicas could diverge permanently after sequential gossip. Branch `apply_remove`/`remove` on the per-key merge strategy. For EpochMaxWins keys, a new `apply_tombstone` helper in `epoch_max_wins` runs the same per-point filter the compactor uses and returns either the filtered shard (with embedded tombstone_version) or an Empty marker when every point was killed. Metadata holds the surviving live version when the shard remains, or the tombstone when the key is fully gone - the latter so a delayed pre-tombstone insert cannot resurrect. Matches spec §2.5 / §5.8: post-tombstone inserts survive and compete by EpochMaxWins; pre-tombstone inserts are dropped. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
`OperationLog::merge` and `CrdtOrMap::merge` both dedupe incoming operations by `(replica_id, timestamp)`. That works for LWW where op-id uniquely identifies the payload, but `epoch_max_wins::into_operation` emits a compacted Insert using the newest live version's op-id while folding earlier operations (including tombstones) into the value bytes. A receiver that already saw the post-tombstone raw Insert at the same op-id silently drops the compacted snapshot, never learns the embedded tombstone_version, and a delayed pre-tombstone Insert can resurrect the deleted shard. Violates spec §5.8: "pre-tombstone inserts cannot resurrect a deleted shard". Make both layers strategy-aware: - `OperationLog::merge_with_strategy`: on op-id collision for an EpochMaxWins key, replace the local op with `epoch_max_wins::compact_operations([local, incoming])` so the richer payload sticks in the local log. - `CrdtOrMap::merge`: per-key filter for `unseen_operations`. LWW keys dedupe by op-id as before; EpochMaxWins keys always replay, with `merge_live_value.changed` gating the store update so identical bytes remain a no-op. - Drop the `(timestamp, replica_id)` short-circuit in `record_epoch_insert_metadata` for the same reason - the gate belongs on the merged bytes, not on the op-id. Regression test exercises receiver-has-raw / source-sends-compacted at the same op-id followed by a delayed pre-tombstone insert. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
Both `apply_epoch_remove_locked` and `record_remove_metadata` returned false in their Vacant arm when `self.store.contains_key(key) == false`, silently dropping the tombstone. For a never-seen key the operation log still kept the Remove (added by `local_log.merge_with_strategy`), but metadata stayed empty - so a delayed pre-tombstone Insert that arrived later hit the Vacant insert path with `current_tombstone = None` and resurrected the key. The compacted log said "dead", the live store said "alive": permanent divergence inside a single replica. Drop the `contains_key` short-circuit in both arms. A tombstone for a key the local store never had still records ordering metadata so a delayed older Insert is suppressed - standard CRDT OR-map semantics. For EpochMaxWins the metadata path runs `apply_tombstone(None, None, incoming)`, which yields an Empty marker and records the tombstone; the store stays untouched. For LWW the metadata path inserts the tombstone entry directly. Spec §2.5: "a tombstone partitions history by (timestamp, replica_id); pre-tombstone inserts cannot resurrect a deleted shard." §5.8: same invariant on partition heal. Regression tests cover both strategies with the exact codex repro sequence (Remove for never-seen key, then delayed older Insert). Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
The public LWW-default convenience methods on OperationLog were a footgun. `merge` silently keeps the older raw payload on an op-id collision, which is correct for LWW but loses the embedded tombstone_version when an EpochMaxWins compacted snapshot collides with a raw insert at the same op-id (the resurrection bug class fixed in 6a24d13). `compact` had no callers at all in the codebase. Remove both. Production callers go through `compact_with_strategy` / `merge_with_strategy`, which take a per-key strategy callback and do the right thing for EpochMaxWins. The one test that exercised `OperationLog::merge` directly now passes the explicit LWW strategy. OperationLog remains in the public re-export for now since no non-mesh callers were found, but this PR keeps reducing its "easy to misuse" surface area. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
Three small cleanups bundled together: 1. Derive `Copy` for `MergeStrategy`. It's a 2-variant fieldless enum (1-byte discriminant) - `Copy` is the right derive and lets callers drop `.clone()` noise at every usage site. 2. Switch `merge_strategies` from `Arc<RwLock<Vec<(...)>>>` to `Arc<RwLock<StrategyTable>>` where `StrategyTable = Arc<[...]>`. `register_merge_strategy` builds a new snapshot copy-on-write (rare, startup-only); `compact`, `append`, and `merge` take a cheap `Arc::clone` of the current snapshot via `merge_strategies_snapshot`, replacing the per-call `Vec` clone (3-5 String allocations per gossip round in steady state). Type alias keeps the field shape readable and silences clippy's `type_complexity`. 3. `apply_tombstone` no longer routes the `None` arm through `state.merge(...)`. Directly constructing `RateLimitState::Tombstone` makes intent obvious and removes the previously-needed `unwrap_or` defensive fallback for an unreachable branch. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
Two small follow-ups to the coderabbit review. `decode_shard` now reads through `bincode::DefaultOptions` with a 64 KiB size limit instead of the default unbounded `bincode::deserialize`. Real `rl:` shards are dozens of bytes (one writer per per-node key, one live point plus optional tombstone), and the spec consumer (`rl:`) never needs more. The cap prevents a malformed or hostile peer from forging a multi-MB allocation via a large `live_points` length prefix. Decode failures are already handled as "skip malformed" by callers, so an oversize input falls through the same path as a corrupt one. `MergeStrategy::EpochMaxWins` doc previously said values "MUST be exactly 16 bytes" - true for the raw write payload at the put boundary, but stored and replicated values are normalized into a serialized `RateLimitShard` and run larger. Narrow the wording so the contract describes both shapes. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
…moves `apply_epoch_remove_locked`'s Empty branch unconditionally cleared and re-pushed `ValueMetadata::tombstone(..)`, which constructs `created_at` as `Instant::now()`. When an older delayed Remove arrives, `apply_tombstone` correctly returns the existing dominant tombstone version, so the (timestamp, replica_id) doesn't change - but the new metadata entry's `created_at` does. A steady stream of stale Removes from a lagging peer keeps refreshing that clock; `gc_tombstones_with_grace` keys off `created_at`, so the tombstone never gets collected. Skip the clear-and-push when the merged tombstone version already matches an existing tombstone metadata entry. The on-disk state was already correct (store entry removed, tombstone metadata recorded); the only previous side effect was the spurious `created_at` reset. Regression test merges the dominant Remove, sleeps past the GC grace, merges an older delayed Remove, then asserts the tombstone still GCs. Without the fix the test fails because the delayed Remove reset the clock. Not reachable for the current `rl:` consumer (RateLimitSyncAdapter never calls delete), but a real correctness gap on the EpochMaxWins remove path for any future caller. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
2c32ef4 to
cf38daf
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/mesh/src/crdt_kv/kv_store.rs (1)
20-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale doc comment references removed method.
The comment mentions
upsertbut this method was removed. Update to reflect current behavior.📝 Suggested fix
- /// Monotonically increasing counter, bumped on every insert/remove/upsert. + /// Monotonically increasing counter, bumped on every insert/remove.🤖 Prompt for 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. In `@crates/mesh/src/crdt_kv/kv_store.rs` around lines 20 - 21, Update the stale doc comment for the generation field (generation: Arc<AtomicU64>) to remove the reference to the removed upsert method and reflect current mutation operations that bump the counter (e.g., "bumped on every insert/remove/update" or list the exact methods that now change generation), so the comment accurately describes when generation is incremented.
🤖 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.
Outside diff comments:
In `@crates/mesh/src/crdt_kv/kv_store.rs`:
- Around line 20-21: Update the stale doc comment for the generation field
(generation: Arc<AtomicU64>) to remove the reference to the removed upsert
method and reflect current mutation operations that bump the counter (e.g.,
"bumped on every insert/remove/update" or list the exact methods that now change
generation), so the comment accurately describes when generation is incremented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 57da17a6-ffbf-474f-a82d-d58250b790ed
📒 Files selected for processing (10)
crates/mesh/src/crdt_kv/crdt.rscrates/mesh/src/crdt_kv/epoch_max_wins.rscrates/mesh/src/crdt_kv/kv_store.rscrates/mesh/src/crdt_kv/merge_strategy.rscrates/mesh/src/crdt_kv/mod.rscrates/mesh/src/crdt_kv/operation.rscrates/mesh/src/crdt_kv/tests.rscrates/mesh/src/kv.rscrates/mesh/src/lib.rsmodel_gateway/src/mesh/adapters/rate_limit_sync.rs
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
`upsert` was removed from `CrdtOrMap` earlier in the v2 cleanup; the doc comments on `KvStore.generation` and `CrdtOrMap::generation` still referenced it. Tighten to insert/remove. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
Description
Problem
Mesh v2 routes rate-limit counters through the generic CRDT OR-map (
CrdtOrMap), with a per-prefixMergeStrategy. Therl:namespace is specified to useEpochMaxWins(spec §2.5): higher epoch wins, equal epoch takes the max count, and a tombstone partitions history so pre-tombstone inserts cannot resurrect a deleted shard. Before this PR:CrdtOrMap::mergeonly knewLastWriterWinssemantics. Anyrl:traffic gossiped over the eventualCRDT_BATCHwire path would merge incorrectly — a node that resets to{epoch: N+1, count: 0}could be overwritten by a stale gossip of{epoch: N, count: 100}because the older message has a higher Lamport timestamp.append_with_strategy/compact/snapshot_and_truncate) was hardcoded to LWW. A single auto-compaction over 10K entries would collapse anrl:key to the wrong winner, and a snapshot pulled by a joining peer would replay the wrong value.rl:payload that survived a tombstone after compaction. Withouttombstone_versionembedded in the stored value, a peer receiving only the compacted Insert (the Remove having been folded away) had no way to know that earlier inserts at the same key are pre-tombstone and must be suppressed.Solution
Wire
EpochMaxWinsthrough the CRDT merge end-to-end and normalizerl:payloads into a self-describing shard format that carries both the live-point frontier and the newest tombstone boundary. Every place that selects "the winner" — local apply, operation-log compaction, log snapshot, and remote merge — consults the per-key strategy through the same registry. The gateway adapter only seesEpochCount; the shard format is an internal CRDT detail.Changes
Per-prefix merge strategy registry
MergeStrategyenum surfaces as acrdt_kvexport with two variants:LastWriterWins(default;worker:,policy:,config:) andEpochMaxWins(rl:). The earlierMaxValueWinsplaceholder was removed because exposing a configurable strategy with no implementation is a footgun.MeshKV::configure_crdt_prefix(prefix, strategy)now registers the strategy on the sharedCrdtOrMapso apply and compaction both pick it up. Keeps the mesh crate application-agnostic — gateway adapters select the strategy at namespace setup time, not via mesh-side hardcoding.merge_strategy_for_key_from(strategies, key)does longest-prefix match against a snapshotted strategy table. The snapshot is taken once permerge/compact/appendcall to avoid reacquiring the read lock for every key inside the loop.Apply-path: live store always reflects the configured strategy
apply_insert_lockedbranches on the per-key strategy. LWW uses the existingrecord_insert_metadatapath. EpochMaxWins routes throughrecord_epoch_insert_metadata, which runs the merge against the current stored shard (with its embeddedtombstone_version) plus any tombstone metadata, and only updates the store when the merged bytes actually change.apply_remove/removelikewise branch on the strategy. For EpochMaxWins,apply_epoch_remove_lockedfilters the stored shard per-point against the merged (existing ∪ incoming) tombstone — matching whatcompact_operationsdoes in the log. If any live points survive, the filtered shard is written back with the new tombstone embedded; if all points are killed, the key is removed and metadata holds the tombstone for GC.tombstone_versionis the source of truth for the boundary), or the newest tombstone when the key is fully gone (so a delayed pre-tombstone insert cannot resurrect).Stored shape:
RateLimitShardas a self-describing CRDT staterl:values are no longer stored as the raw 16-byte(epoch, count)write payload. They are normalized into aRateLimitShard { live_points: Vec<LivePoint>, tombstone_version: Option<RateLimitVersion> }and bincode-serialized. The frontier holds the live points that could become the correct value after a future tombstone boundary, computed via suffix-best.decode_epoch_countaccepts both the normalized shard form (what callers see onget()and via gossip) and the raw 16-byte payload (what local namespace subscribers can observe before CRDT normalization runs). The gateway adapter only ever seesEpochCountand never has to know about CRDT internals.state_from_insert_valueaccepts either shape on the insert boundary too, so a local subscriber that round-trips throughputthensubscribedoesn't blow up on its own raw payload.RateLimitState::into_operationis the serialized shard (carrying any embeddedtombstone_version), tagged with the newest live version's(timestamp, replica_id). That choice keeps the local op-log compact (one op per key after compaction) and preserves the original Lamport ordering for downstream merge.Operation-log compaction: holistic per-key, allocation-free
latest_epoch_max_wins_operationevaluates each key holistically: it folds all operations into a singleRateLimitStateand emits one canonical op. Earlier pairwise comparators mixed epoch ordering with timestamp ordering and produced order-dependent winners.Vec<u8>allocation per pairwise compare with the holistic single-fold, dropping compaction allocations from O(N) to O(1) for an EpochMaxWins key.compact_operationsrecognizes tombstone cutoffs: pre-tombstone inserts are ignored, post-tombstone inserts can recreate the key and compete by EpochMaxWins. Malformed insert payloads are skipped (logged and continued) rather than aborting the whole compaction.append_with_strategy,compact_with_strategy, andsnapshot_and_truncateall take a per-key strategy callback. None of these paths can accidentally rewrite an EpochMaxWins winner as an LWW winner.Cross-replica merge: tombstone boundary must survive same-op-id collisions
OperationLog::merge_with_strategyis strategy-aware. For LWW keys it dedups by(replica_id, timestamp)as before. For EpochMaxWins keys, on op-id collision it replaces the local op withepoch_max_wins::compact_operations([local, incoming])so a compacted snapshot carrying an embeddedtombstone_versionoverwrites the older raw payload at the same op-id.CrdtOrMap::mergefiltersunseen_operationsper-key. LWW dedups by op-id; EpochMaxWins always replays, withmerge_live_value.changedgating the store update so identical bytes remain a no-op. Without this, a receiver that already saw the post-tombstone raw insert would silently drop a compacted snapshot at the same op-id, never learn the tombstone boundary, and let a delayed pre-tombstone insert resurrect the deleted shard (spec §5.8 violation).(timestamp, replica_id)short-circuit inrecord_epoch_insert_metadatawas removed. The acceptance gate now belongs on the merged bytes (changed: bool), not on operation identity, because the same op-id can carry semantically different payloads under EpochMaxWins.Equal-value insert no longer rewinds metadata
merge_live_valueproduces bytes equal to the current stored value (changed: false),record_epoch_insert_metadatareturns without clearing the version vec. Previously, an older equal-value insert could erase newer metadata and let an intermediate tombstone delete the live value — silent data loss for therl:reset path.Strategy snapshotting at the right granularity
compact_operation_logandappend_operationsnapshot the strategy table once and pass it into the operation-log helpers as a closure. The read lock onmerge_strategiesis acquired exactly once per gossip round, not once per key. Keeps per-key lookups lock-free under the hot path.Test Plan
cargo test -p smg-mesh --lib— 141 tests pass, including the new EpochMaxWins-specific cases:test_epoch_max_wins_tombstone_filters_pre_tombstone_inserts_per_point— three operations (insert ε=5 @ ts=100, insert ε=6 @ ts=90, remove @ ts=95) arriving in separate merges. The post-tombstone live point survives (ε=5, count=100), the pre-tombstone high-epoch point is dropped. Matches spec §2.5.test_epoch_max_wins_live_store_matches_compacted_log_after_tombstone— same sequence, asserts thatdecode(store.get(key))equalsdecode(value_of(compacted_log)). Catches any future drift between live apply and operation-log compaction.test_epoch_max_wins_tombstone_kills_all_points_removes_key— tombstone newer than every live point removes the key from the store, and a delayed pre-tombstone insert cannot resurrect it.test_epoch_max_wins_snapshot_only_propagation_preserves_tombstone_boundary— source replica compacts{I60 high-ε, T65, I70 low-ε}into one Insert at op-id (70, c) with embeddedtombstone_version=65. Receiver applies snapshot, sees ε=6 count=1. A late peer's pre-tombstone insert at op-id (60, _) is filtered.test_epoch_max_wins_compacted_snapshot_applies_when_op_id_already_seen— the codex P1 case. Receiver already has the raw post-tombstone insert at op-id (70, c). Source sends compacted snapshot at the same op-id with the richer payload. Merge correctly applies it, embedding the tombstone, and a delayed pre-tombstone insert is then suppressed.compacted_live_state_remembers_tombstone_boundary,compacted_live_state_uses_newest_live_version,compact_operations_skips_malformed_inserts— direct tests for the holistic compaction logic.Plus the existing 130+ tests covering LWW behavior, basic CRDT semantics, merge convergence, tombstone GC, and chunking — none regressed.
cargo clippy --workspace --all-targets --all-features -- -D warningsclean.Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspasses.claude/docs/mesh/mesh-v2-implementation-spec.md§2.5 and §5.8 was the source of truth and is unchanged. Module-level doc onepoch_max_wins.rsdocuments the stored-vs-raw shape distinction.Summary by CodeRabbit
New Features
Tests
Documentation