mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-17 16:37:43 +00:00
Drop disableDisjointMerge from the production conflict-resolution entry point. With the merge frozen (#9061), concurrent edits to different fields of one entity resolve by whole-entity LWW: the later side wins a full 'replace' snapshot with a dominating clock and the other side's edit is silently and permanently lost on every client (a rename dies when another device marks the task done). The journal half of the freeze stays. Wire-safe: v18.14.0 predates lwwUpdateMode entirely and applies LWW Update payloads via updateOne, so the merge's 'patch' union-delta merges cleanly on released clients (#8874's back-compat envelope design). Restores the strengthened 3.1 disjoint-merge E2E parked out of #9089 (it fails deterministically while the merge is frozen) and adds a unit regression for the exact rename-vs-done pair; the caller guard spec now pins that the merge stays enabled on the production resolve path.
This commit is contained in:
parent
b50d5f6d96
commit
e5a9ff7866
4 changed files with 73 additions and 33 deletions
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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 }),
|
||||
|
|
|
|||
|
|
@ -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 () => {
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
},
|
||||
);
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue