CRDT merge semantics for non-root entities during sync

[xilosada]

Context

Discovered during PR #1972 review (cursor[bot] comment).

Issue

In crates/storage/src/interface.rs:save_internal() (lines 980-983), non-root entities use LWW (Last-Write-Wins) when the incoming timestamp is newer:

} else {
    // Incoming is newer - use it (LWW for non-root entities)
    data.to_vec()
}

This means:

• Root entities: Always CRDT merge ✅
• Non-root + same timestamp: CRDT merge ✅
• Non-root + newer incoming: LWW (no merge) ⚠️

Problem

For CRDT types like GCounter, PnCounter, UnorderedMap, etc., LWW can cause data loss:

Node A: counter = 5 (ts=100)
Node B: counter = 3 (ts=200)
After sync A←B: counter = 3 (lost the 5 increments from A)
Correct CRDT merge: counter = 8

Invariant Affected

I5 - No Silent Data Loss: "initialized nodes MUST CRDT-merge; overwrite ONLY for fresh nodes"

Related Work

• PR feat(storage): add CRDT type-based merge dispatch #1889 (feat/storage-crdt-merge-dispatch) appears to address this by adding merge_by_crdt_type() dispatch
• Issue [Sync Protocol] 016: Snapshot Merge Protection (Invariant I5) #1783 ([Sync Protocol] 016: Snapshot Merge Protection)

Suggested Fix

In save_internal(), check metadata.crdt_type and call CRDT merge for non-LwwRegister types regardless of timestamp order.

Notes

• This is pre-existing behavior, not introduced by feat(sync): SyncProtocolExecutor trait for unified protocol testing #1972
• PR feat(sync): SyncProtocolExecutor trait for unified protocol testing #1972 makes this easier to catch since simulation now uses the same code path as production
• The storage architecture stores CRDT collection elements as separate entities, so the impact may be limited to root-level CRDT fields

/cc @calimero-network/core-team

[xilosada]

Closing: Code Already Fixed

The LWW fallback behavior described in this issue has been addressed. The code in save_internal() (crates/storage/src/interface.rs lines 981-995) now properly calls try_merge_non_root() for all non-root entity conflicts, not just same-timestamp cases:

} else {
    // Incoming is newer - try CRDT merge for non-root entities if possible
    // (Invariant I5: no silent data loss)
    if let Some(existing_data) = S::storage_read(Key::Entry(id)) {
        Self::try_merge_non_root(
            id,
            &existing_data,
            data,
            &metadata,
            *last_metadata.updated_at,
            *metadata.updated_at,
        )?
    } else {
        data.to_vec()
    }
}

try_merge_non_root_internal() properly dispatches to merge_by_crdt_type_with_callback() for built-in CRDTs, ensuring proper merge semantics (e.g., GCounter takes max per executor, not LWW overwrite).

Verification:

• 60 sync compliance tests pass including I5 tests
• 48 storage merge tests pass
• test_i5_gcounter_max_wins, test_i5_pncounter_merge verify correct CRDT merge

The fix was part of the CRDT merge dispatch work (PR #1978 and related).