From 9132ab67224cfa4e3d59e65c14aa9c62f425810c Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Wed, 15 Jul 2026 13:03:09 +0200 Subject: [PATCH] fix(sync-core): break whole-entity LWW timestamp ties by clientId (#9035) * fix(sync-core): break whole-entity LWW timestamp ties by clientId An exact-millisecond timestamp tie on the same field of one entity fell to remote-wins with no tiebreak. Because local/remote are swapped between devices, each kept the other's value and diverged permanently. Fall back to a deterministic larger-clientId compare on the tie (mirrors noiseTiebreakSide), routing a local tie-win through the existing localWinOperationKind: 'update' path so the compensating op preserves the local value and dominates the loser's clock. * refactor(sync-core): tidy LWW tiebreak helper + strengthen tie tests Multi-review follow-up (no behavior change): - drop unreachable `?? ops[0]` fallback and the unused generic in winningClientId; correct the doc comment's "mirrors" overstatement. - rewrite the mislabeled service-level tie test that never exercised the tiebreak and add the local-clientId-larger direction (compensating op). * test(sync): cover LWW client-ID tie-win over a concurrent remote DELETE The tiebreak reaches _createLocalWinUpdateOp's delete-recreation branch via a tie (not just a newer timestamp); assert the entity is still recreated. --- packages/sync-core/src/conflict-resolution.ts | 30 +++++++- .../tests/conflict-resolution.spec.ts | 64 +++++++++++++++- .../sync/conflict-resolution.service.spec.ts | 73 ++++++++++++++++++- 3 files changed, 162 insertions(+), 5 deletions(-) diff --git a/packages/sync-core/src/conflict-resolution.ts b/packages/sync-core/src/conflict-resolution.ts index 18c597b559..ee954d957e 100644 --- a/packages/sync-core/src/conflict-resolution.ts +++ b/packages/sync-core/src/conflict-resolution.ts @@ -333,6 +333,22 @@ export const suggestConflictResolution = >( return 'manual'; }; +/** + * The clientId of the op carrying a side's winning (max) timestamp. Used only as + * a deterministic tiebreak when both sides share the same max timestamp: the two + * devices see "local" and "remote" swapped, so comparing timestamps alone makes + * each keep the other's value and diverge permanently. Comparing the winning + * clientIds instead — over the same unordered pair on both devices — makes them + * converge, following the same `(timestamp, clientId)` principle as the client's + * `noiseTiebreakSide`. A genuine cross-device tie always has exactly one op at + * the max timestamp per side (same-client ops on one entity are never + * vector-clock-concurrent, so they never reach here as a conflict). + */ +const winningClientId = ( + ops: readonly Operation[], + maxTimestamp: number, +): string => ops.find((op) => op.timestamp === maxTimestamp)?.clientId ?? ''; + /** * Plans last-write-wins conflict resolution without looking up host state or * creating operations. @@ -418,7 +434,19 @@ export const planLwwConflictResolutions = < const localMaxTimestamp = Math.max(...conflict.localOps.map((op) => op.timestamp)); const remoteMaxTimestamp = Math.max(...conflict.remoteOps.map((op) => op.timestamp)); - if (localMaxTimestamp > remoteMaxTimestamp) { + // On an exact-millisecond tie, fall back to a deterministic clientId compare + // (larger wins) so both devices converge instead of each keeping the other's + // value. A local tie-win must carry `localWinOperationKind: 'update'` exactly + // like a timestamp win: the host rejects the original local ops regardless of + // winner, and only the resulting compensating op (dominating clock) both + // preserves the local value and stops the loser from resurfacing. + const localWins = + localMaxTimestamp > remoteMaxTimestamp || + (localMaxTimestamp === remoteMaxTimestamp && + winningClientId(conflict.localOps, localMaxTimestamp) > + winningClientId(conflict.remoteOps, remoteMaxTimestamp)); + + if (localWins) { return { conflict, winner: 'local', diff --git a/packages/sync-core/tests/conflict-resolution.spec.ts b/packages/sync-core/tests/conflict-resolution.spec.ts index 86be7e7af4..dbba22fc90 100644 --- a/packages/sync-core/tests/conflict-resolution.spec.ts +++ b/packages/sync-core/tests/conflict-resolution.spec.ts @@ -529,10 +529,13 @@ describe('planLwwConflictResolutions', () => { ]); }); - it('lets remote win timestamp ties', () => { + it('defaults a timestamp tie to remote when both sides share a clientId', () => { + // Degenerate case only: same-client ops on one entity are never + // vector-clock-concurrent, so a real cross-device tie never has equal + // clientIds. Keeping the default deterministic is enough here. const conflict = createConflict( - [createOp({ id: 'local', timestamp: 1_000 })], - [createOp({ id: 'remote', timestamp: 1_000 })], + [createOp({ id: 'local', timestamp: 1_000, clientId: 'client-1' })], + [createOp({ id: 'remote', timestamp: 1_000, clientId: 'client-1' })], ); expect(planLwwConflictResolutions([conflict], { isArchiveAction })).toEqual([ @@ -545,6 +548,61 @@ describe('planLwwConflictResolutions', () => { }, ]); }); + + it('breaks a timestamp tie for local when its winning clientId is larger', () => { + const conflict = createConflict( + [createOp({ id: 'local', timestamp: 1_000, clientId: 'client-z' })], + [createOp({ id: 'remote', timestamp: 1_000, clientId: 'client-a' })], + ); + + expect(planLwwConflictResolutions([conflict], { isArchiveAction })).toEqual([ + { + conflict, + winner: 'local', + reason: 'local-timestamp', + localWinOperationKind: 'update', + localMaxTimestamp: 1_000, + remoteMaxTimestamp: 1_000, + }, + ]); + }); + + it('breaks a timestamp tie for remote when its winning clientId is larger', () => { + const conflict = createConflict( + [createOp({ id: 'local', timestamp: 1_000, clientId: 'client-a' })], + [createOp({ id: 'remote', timestamp: 1_000, clientId: 'client-z' })], + ); + + expect(planLwwConflictResolutions([conflict], { isArchiveAction })).toEqual([ + { + conflict, + winner: 'remote', + reason: 'remote-timestamp-or-tie', + localMaxTimestamp: 1_000, + remoteMaxTimestamp: 1_000, + }, + ]); + }); + + it('resolves a timestamp tie to the same physical client when sides are swapped', () => { + // Commutativity: whichever side "client-b" is on, it wins — so both devices + // converge instead of each keeping the other's value. + const opA = createOp({ id: 'a', timestamp: 1_000, clientId: 'client-a' }); + const opB = createOp({ id: 'b', timestamp: 1_000, clientId: 'client-b' }); + + const [deviceA] = planLwwConflictResolutions([createConflict([opA], [opB])], { + isArchiveAction, + }); + const [deviceB] = planLwwConflictResolutions([createConflict([opB], [opA])], { + isArchiveAction, + }); + + // Device A sees opB as remote and adopts it; device B sees opB as local and + // keeps it. Different winner label, same winning op — convergent. + expect(deviceA.winner).toBe('remote'); + expect(deviceB.winner).toBe('local'); + expect(deviceB.localWinOperationKind).toBe('update'); + }); }); describe('partitionLwwResolutions', () => { diff --git a/src/app/op-log/sync/conflict-resolution.service.spec.ts b/src/app/op-log/sync/conflict-resolution.service.spec.ts index 1df4655e6e..86e8c85404 100644 --- a/src/app/op-log/sync/conflict-resolution.service.spec.ts +++ b/src/app/op-log/sync/conflict-resolution.service.spec.ts @@ -1080,6 +1080,45 @@ describe('ConflictResolutionService', () => { ).toBeTrue(); }); + it('should recreate a locally-winning UPDATE over a concurrent remote DELETE on a client-ID tie (#9024)', async () => { + const now = Date.now(); + mockStore.select.and.returnValue( + of({ id: 'task-1', title: 'Local winning task' }), + ); + // Exact-timestamp tie against a remote DELETE. Local's clientId + // (client-z) is the larger, so the deterministic tiebreak makes the + // local UPDATE win — reaching the SAME delete-recreation path as the + // "UPDATE is newer" case above, just via the tie rather than the + // timestamp. Guards that the #9024 tiebreak doesn't bypass entity + // recreation when the loser was a delete. + const conflicts: EntityConflict[] = [ + createConflict( + 'task-1', + [ + { + ...createOpWithTimestamp('local-upd', 'client-z', now), + opType: OpType.Update, + }, + ], + [ + { + ...createOpWithTimestamp('remote-del', 'client-a', now), + opType: OpType.Delete, + }, + ], + ), + ]; + + await service.autoResolveConflictsLWW(conflicts); + + expect(mockOpLogStore.markRejected).toHaveBeenCalledWith(['local-upd']); + expect(mockOpLogStore.markRejected).toHaveBeenCalledWith(['remote-del']); + expect( + (getFirstMixedLocalOp().payload as { recreatesEntityAfterDelete?: boolean }) + .recreatesEntityAfterDelete, + ).toBeTrue(); + }); + it('should resolve DELETE vs UPDATE conflict when DELETE is older (remote UPDATE wins)', async () => { const now = Date.now(); const conflicts: EntityConflict[] = [ @@ -5185,7 +5224,8 @@ describe('ConflictResolutionService', () => { { entityType: 'TASK', entityId: 'task-1', - // client-a < client-b alphabetically, but we test that remote wins on tie + // Remote's clientId (client-b) is lexicographically larger, so the + // deterministic tiebreak makes remote win the exact-timestamp tie. localOps: [createOpWithTimestamp('local-1', 'client-a', now)], remoteOps: [createOpWithTimestamp('remote-1', 'client-b', now)], suggestedResolution: 'remote', // Remote wins on tie @@ -5210,6 +5250,37 @@ describe('ConflictResolutionService', () => { ); expect(mockOpLogStore.markRejected).toHaveBeenCalledWith(['local-1']); }); + + it('should let local win the tie when its client ID is larger', async () => { + const now = Date.now(); + + // Same exact-millisecond tie, but now LOCAL's clientId (client-z) is the + // larger, so the deterministic tiebreak flips the winner to local. This is + // the direction the pre-existing tie tests never exercised (#9024): both + // devices see the sides swapped yet pick the same physical client, so they + // converge instead of each keeping the other's value. + const conflicts: EntityConflict[] = [ + { + entityType: 'TASK', + entityId: 'task-1', + localOps: [createOpWithTimestamp('local-1', 'client-z', now)], + remoteOps: [createOpWithTimestamp('remote-1', 'client-a', now)], + suggestedResolution: 'remote', + }, + ]; + + mockOpLogStore.hasOp.and.resolveTo(false); + mockOpLogStore.append.and.resolveTo(1); + mockOpLogStore.markApplied.and.resolveTo(undefined); + mockOpLogStore.markRejected.and.resolveTo(undefined); + mockOperationApplier.applyOperations.and.resolveTo({ appliedOps: [] }); + + await service.autoResolveConflictsLWW(conflicts); + + // Local wins the tie → the remote op is rejected (mirrors the local-win + // timestamp cases in this block). + expect(mockOpLogStore.markRejected).toHaveBeenCalledWith(['remote-1']); + }); }); // =========================================================================