diff --git a/e2e/tests/sync/supersync.spec.ts b/e2e/tests/sync/supersync.spec.ts index 136b465e26..eabd28eabd 100644 --- a/e2e/tests/sync/supersync.spec.ts +++ b/e2e/tests/sync/supersync.spec.ts @@ -308,21 +308,14 @@ test.describe('@supersync SuperSync E2E', () => { * Actions: * 1. Client A creates task, syncs * 2. Client B syncs (download) - * 3. Client A marks task done (no sync yet) - * 4. Client B adds notes to task (no sync yet) + * 3. Client A renames the task (no sync yet) + * 4. Client B marks the task done (no sync yet) * 5. Client A syncs * 6. Client B syncs * - * Expected: Conflict detected or auto-merged, final state consistent - * - * WEAK — do not trust this as conflict coverage. Both clients change the SAME - * field (isDone), so there is no disjoint merge to get wrong, and the only - * assertion is that the task still exists. Strengthening it to rename-vs-done - * uncovered real data loss: the rename is committed as an op and then lost in - * sync. The stronger version lives in #9095 and stays out of this PR because it - * fails for a pre-existing reason unrelated to the fixes here. + * Expected: The disjoint title and completion changes are both preserved */ - test('3.1 Concurrent edits handled gracefully', async ({ + test('3.1 Concurrent disjoint task edits merge without field loss', async ({ browser, baseURL, testRunId, @@ -350,10 +343,11 @@ test.describe('@supersync SuperSync E2E', () => { await waitForTask(clientA.page, taskName); await waitForTask(clientB.page, taskName); - // Client A marks done (creates local op) - await markTaskDone(clientA, taskName); + // Client A changes the title while Client B changes a disjoint field. + const renamedTaskName = `${taskName}-renamed-by-A`; + await renameTask(clientA, taskName, renamedTaskName); - // Client B marks done too (concurrent edit) + // Client B has not seen the rename and marks the original task done. await markTaskDone(clientB, taskName); // Client A syncs first @@ -365,11 +359,11 @@ test.describe('@supersync SuperSync E2E', () => { // Client A syncs again to converge await clientA.sync.syncAndWait(); - // Verify both clients have consistent state + // Verify both independent fields survived conflict resolution on both clients. await expectEqualTaskCount([clientA, clientB]); - - // Task should exist on both - await expectTaskOnAllClients([clientA, clientB], taskName); + await expectTaskOnAllClients([clientA, clientB], renamedTaskName); + await expectTaskDone(clientA, renamedTaskName); + await expectTaskDone(clientB, renamedTaskName); } finally { if (clientA) await closeClient(clientA); if (clientB) await closeClient(clientB); diff --git a/src/app/op-log/sync/conflict-resolution.disjoint-merge.spec.ts b/src/app/op-log/sync/conflict-resolution.disjoint-merge.spec.ts index 0d58c5c032..221ea67696 100644 --- a/src/app/op-log/sync/conflict-resolution.disjoint-merge.spec.ts +++ b/src/app/op-log/sync/conflict-resolution.disjoint-merge.spec.ts @@ -320,6 +320,44 @@ describe('ConflictResolutionService — SPAP-14 disjoint-field merge', () => { expect((await journal.list('unreviewed')).length).toBe(0); }); + // ── (a0) #9095 regression: rename vs mark-done → merge both ──────────────── + // With disjoint merge disabled this pair resolves by whole-entity LWW: the + // later mark-done side wins a full 'replace' snapshot carrying its stale + // title, and the rename is permanently lost on every client. + it('(a0) merges a remote rename with a later local mark-done, losing neither (#9095)', async () => { + mockStore.select.and.returnValue( + of({ id: 'task-1', title: 'Original title', isDone: true }), + ); + + const localOp = op({ + id: 'local-done', + clientId: 'B', + vectorClock: { B: 1 }, + timestamp: 2000, + payload: { task: { id: 'task-1', changes: { isDone: true } } }, + }); + const remoteOp = op({ + id: 'remote-rename', + clientId: 'A', + vectorClock: { A: 1 }, + timestamp: 1000, + payload: { task: { id: 'task-1', changes: { title: 'Renamed by A' } } }, + }); + + await service.autoResolveConflictsLWW([conflictOf([localOp], [remoteOp])]); + + const merged = mergedOpArgs(); + expect(merged).toBeDefined(); + const payload = extractActionPayload(merged!.payload); + expect(payload['title']).toBe('Renamed by A'); + expect(payload['isDone']).toBe(true); + expect((merged!.payload as { lwwUpdateMode?: string }).lwwUpdateMode).toBe('patch'); + + const rejected = mockOpLogStore.markRejected.calls.allArgs().flat(2); + expect(rejected).toContain('local-done'); + expect(rejected).toContain('remote-rename'); + }); + it('(a1) fails closed before mutating the op log for a legacy remote bulk op', async () => { mockStore.select.and.returnValue( of({ id: 'task-2', title: 'Local title', timeSpent: 0 }), diff --git a/src/app/op-log/sync/remote-ops-processing.service.spec.ts b/src/app/op-log/sync/remote-ops-processing.service.spec.ts index 3b5fa45c72..e7720593e9 100644 --- a/src/app/op-log/sync/remote-ops-processing.service.spec.ts +++ b/src/app/op-log/sync/remote-ops-processing.service.spec.ts @@ -441,11 +441,14 @@ describe('RemoteOpsProcessingService', () => { expect(JSON.stringify(summary)).not.toContain('private'); }); - // Producer freeze for the conflict-review rollback: this is the only - // production entry point into autoResolveConflictsLWW, so if these two flags - // are ever dropped the stable fleet silently starts persisting the discarded - // side of every conflict verbatim again. Delete this test with the freeze. - it('should freeze both conflict-review producers on the production resolve path', async () => { + // Producer freeze (journal half) for the conflict-review rollback: this is + // the only production entry point into autoResolveConflictsLWW, so if the + // flag is ever dropped the stable fleet silently starts persisting the + // discarded side of every conflict verbatim again. Delete that half of this + // test with the freeze. The disjoint-merge half is pinned the OTHER way: + // freezing it made concurrent disjoint-field edits lose one side by + // whole-entity LWW (#9095), so the merge must stay enabled here. + it('should freeze the journal producer but keep disjoint merge enabled on the production resolve path', async () => { const localOp = { id: 'local-op', entityType: 'TASK', @@ -482,10 +485,12 @@ describe('RemoteOpsProcessingService', () => { jasmine.any(Array), jasmine.any(Array), jasmine.objectContaining({ - disableDisjointMerge: true, disableConflictJournal: true, }), ); + const resolveOptions = + conflictResolutionServiceSpy.autoResolveConflictsLWW.calls.mostRecent().args[2]; + expect(resolveOptions?.disableDisjointMerge).toBeFalsy(); }); it('should drop operations if migrateOperation returns null', async () => { diff --git a/src/app/op-log/sync/remote-ops-processing.service.ts b/src/app/op-log/sync/remote-ops-processing.service.ts index e431364bd8..424baa8201 100644 --- a/src/app/op-log/sync/remote-ops-processing.service.ts +++ b/src/app/op-log/sync/remote-ops-processing.service.ts @@ -463,23 +463,26 @@ export class RemoteOpsProcessingService { // Piggyback non-conflicting ops so they're applied with resolved conflicts. // Validation failure is surfaced via the session-validation latch. // - // PRODUCER FREEZE for the conflict-review rollback. Both flags are set - // here, at the only production entry point, so the feature's two - // producers stop at the fleet boundary while the service keeps the - // capability intact for its own tests and for a one-line revert: - // - disableDisjointMerge: conflicts resolve by whole-entity LWW, which - // is the behaviour of every released version to date, so the stable - // fleet gains nothing it would later have to be migrated off. + // PRODUCER FREEZE (journal half) for the conflict-review rollback, set + // here at the only production entry point so the producer stops at the + // fleet boundary while the service keeps the capability intact: // - disableConflictJournal: stop persisting the discarded side of a // conflict verbatim, so that device-local data obligation does not // expand beyond the edge/internal builds that already carry it. - // Reverting the freeze = drop these two lines. + // Reverting the freeze = drop that line. + // + // The disjoint-merge half of the original #9061 freeze was UNFROZEN for + // #9095: with the merge disabled, concurrent edits to DIFFERENT fields + // of one entity resolve by whole-entity LWW and the earlier side's edit + // is silently and permanently lost on every client (a rename dies when + // another device marks the task done). The merged op is a standard LWW + // Update whose 'patch' payload released clients apply via updateOne, so + // re-enabling it ships no wire format they cannot handle. const lwwResult = await this.conflictResolutionService.autoResolveConflictsLWW( conflicts, nonConflicting, { callerHoldsOperationLogLock: true, - disableDisjointMerge: true, disableConflictJournal: true, }, );