mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-18 00:46:45 +00:00
fix(sync): make disjoint-merge convergent + close flip stale-guard blind spot
Two confirmed cross-device data defects found in the SPAP-14 whole-PR review: 1. Full-entity merged op diverges (CRITICAL). The merged op was a full-entity snapshot of each client's CURRENT state, so any field NEITHER conflicting side touched rode along. Under ordinary staggered sync (one client already applied a third device's edit to that field, the other not yet), the two clients synthesize different snapshots that tie under LWW at the identical max(timestamp) and diverge PERMANENTLY. Fix: synthesize a PARTIAL delta (union of the two sides' changed fields only), derived purely from the ops so both clients compute the byte-identical map; lwwUpdateMetaReducer applies it via updateOne (shallow merge), leaving untouched fields alone. synthesizeMergedEntity -> synthesizeMergedChanges. Also: detectConflicts emits one conflict per remote op with no per-entity aggregation, so an entity with >=2 concurrent remote ops synthesized multiple merged ops whose clocks dominate one another -> a dominated sibling's field is silently dropped, falsely journaled as 'kept both'. Fix: refuse merge for any entity with >1 conflict this batch and fall back to whole-entity LWW. 2. Flip stale-guard blind to loser-only fields (HIGH). getStaleState only compared WINNER-changed fields, but flip writes LOSER-changed fields. A post-resolution edit to a loser-only field was undetected and silently overwritten. Fix: also flag stale when a loser-only field flip will write diverged from the value flip would write; bulk flipAllToSide now SKIPS stale entries instead of silently applying them. Adds regression specs for the un-conflicted-field ride-along, multi-remote-op refusal, and loser-only stale detection (per-entry + bulk). Full conflict suite green (disjoint-merge 12, ui 24, conflict-resolution 138, journal 20, hook 6, util 14, review-util 13, superseded 42, banner 3, page 6).
This commit is contained in:
Johannes Millan
parent
90544d7d73
commit
efe66d7875
6 changed files with 370 additions and 64 deletions
|
|
@ -99,20 +99,34 @@ kept by synthesizing a single merged UPDATE op. Eligibility
|
|||
|
||||
- neither side has a DELETE op, and the plan is not an archive plan;
|
||||
- neither side has opaque ops (their changes could not be carried into the
|
||||
synthesized entity — merging would silently drop them and the two clients
|
||||
would synthesize DIFFERENT entities);
|
||||
synthesized delta — merging would silently drop them and the two clients
|
||||
would synthesize DIFFERENT results);
|
||||
- both sides changed at least one real (non-noise) field;
|
||||
- the two sides' non-noise changed-field sets are disjoint.
|
||||
- the two sides' non-noise changed-field sets are disjoint;
|
||||
- the entity has only ONE conflict in this batch. `detectConflicts` emits one
|
||||
conflict per remote op with no per-entity aggregation, so an entity with ≥2
|
||||
concurrent remote ops would synthesize multiple merged ops whose clocks
|
||||
dominate one another — a dominated sibling can be superseded and its field
|
||||
silently dropped. Such entities fall back to whole-entity LWW (honest refusal;
|
||||
per-entity aggregation into one op is a possible future improvement).
|
||||
|
||||
**Convergence contract:** both clients must synthesize the byte-identical
|
||||
merged entity regardless of which one performs the merge. Each client starts
|
||||
from its own current state (`base + ownChanges`) and overlays the other side's
|
||||
non-noise fields (disjoint, so nothing is clobbered); noise fields both sides
|
||||
changed resolve via a deterministic `(timestamp, clientId)` tiebreak.
|
||||
merged **changes delta** regardless of which one performs the merge. The delta
|
||||
is the union of both sides' non-noise fields (disjoint, so nothing is clobbered)
|
||||
plus the noise fields either side changed, resolved via a deterministic
|
||||
`(timestamp, clientId)` tiebreak. Crucially the delta is derived ONLY from the
|
||||
two sides' ops — **not** from either client's current entity snapshot. A
|
||||
full-entity snapshot would drag along fields NEITHER side touched; if such an
|
||||
untouched field momentarily differs between the two clients (an ordinary
|
||||
staggered-sync race — e.g. one client already applied a third device's edit the
|
||||
other has not), the two snapshots would differ, tie under LWW at the identical
|
||||
`max(timestamp)`, and diverge PERMANENTLY. See `synthesizeMergedChanges`.
|
||||
|
||||
**Atomicity / no-re-merge contract:** the merged resolution is exactly ONE new
|
||||
UPDATE op with a **flat full-entity payload**, layered on top of both sides'
|
||||
history like a normal edit — there is no history rewind. Because the payload is
|
||||
UPDATE op carrying a **flat PARTIAL delta** (only the changed fields), layered
|
||||
on top of both sides' history like a normal edit — there is no history rewind.
|
||||
`lwwUpdateMetaReducer` applies it via `updateOne` (a shallow merge), so fields
|
||||
outside the delta keep their own values on each client. Because the payload is
|
||||
flat (not `{ changes }`-shaped), `extractUpdateChanges` yields `{}` for it, so
|
||||
a merged op can never itself become disjoint-merge eligible: merges do not
|
||||
cascade or re-merge on later syncs. Merged resolutions are journaled with
|
||||
|
|
@ -133,9 +147,15 @@ Per-entry actions (`SyncConflictUiService`):
|
|||
action — the same action a manual edit dispatches — so the operation-capture
|
||||
meta-reducer turns it into a synced op that propagates everywhere. No history
|
||||
rewind; a flip is a brand-new edit on top of current state. Before applying,
|
||||
a **stale guard** compares the entity's current values to the journaled
|
||||
winner values and asks for confirmation if the entity was edited after the
|
||||
conflict resolved.
|
||||
a **stale guard** asks for confirmation if the entity was edited after the
|
||||
conflict resolved. It checks every field the flip will actually write: a
|
||||
winner-changed field whose current value diverged from the journaled winner
|
||||
value, OR a **loser-only** field (one the flip writes but the winner never
|
||||
changed, so it is invisible to the winner values) whose current value is not
|
||||
already what the flip would write. Without the loser-only check the guard is
|
||||
blind to exactly the fields flip overwrites, and a post-resolution edit there
|
||||
would be silently lost. The bulk flip path shows no dialog, so it **skips**
|
||||
stale entries entirely rather than overwriting them.
|
||||
|
||||
**Flip capability is deliberately narrow** (`canFlip`); everything else
|
||||
returns `unsupported`, keeps the entry `unreviewed`, and shows an error snack —
|
||||
|
|
|
|||
|
|
@ -7,9 +7,9 @@
|
|||
* both sides' changed fields.
|
||||
*
|
||||
* No Angular, no I/O — deterministic, so the merge decision and the synthesized
|
||||
* entity are unit-testable in isolation. Determinism is the whole point: both
|
||||
* clients must arrive at the byte-identical merged entity regardless of which
|
||||
* one performs the merge (see `synthesizeMergedEntity`).
|
||||
* changes delta are unit-testable in isolation. Determinism is the whole point:
|
||||
* both clients must arrive at the byte-identical merged delta regardless of
|
||||
* which one performs the merge (see `synthesizeMergedChanges`).
|
||||
*/
|
||||
|
||||
import { OpType } from '../core/operation.types';
|
||||
|
|
@ -157,34 +157,45 @@ export const isDisjointMergeEligible = (params: {
|
|||
};
|
||||
|
||||
/**
|
||||
* Synthesizes the merged entity — the SINGLE source of truth both clients must
|
||||
* converge on.
|
||||
* Synthesizes the merged CHANGES DELTA — the union of both sides' changed
|
||||
* fields, applied on top of each client's current entity by `updateOne` (a
|
||||
* shallow MERGE, not a replace). This is the SINGLE source of truth both clients
|
||||
* must converge on.
|
||||
*
|
||||
* `currentEntity` is THIS client's current entity state, i.e. `base + localChanges`.
|
||||
* We overlay the OTHER side's non-noise changed fields (guaranteed disjoint from
|
||||
* local's, so nothing local is clobbered), then resolve every noise field either
|
||||
* side changed via the deterministic `(timestamp, clientId)` tiebreak.
|
||||
* IMPORTANT — why a delta and NOT a full-entity snapshot: the delta is derived
|
||||
* purely from the two conflicting sides' ops, so both clients compute the
|
||||
* byte-identical map regardless of the rest of their entity state. A full-entity
|
||||
* snapshot (`{...currentEntity}`) would drag along fields NEITHER side touched;
|
||||
* if such an untouched field momentarily differs between the two clients (an
|
||||
* ordinary staggered-sync race — e.g. one client already applied a third
|
||||
* device's edit the other has not), the two synthesized snapshots differ, tie
|
||||
* under LWW at the identical `max(timestamp)`, and diverge PERMANENTLY. Carrying
|
||||
* only the changed fields makes the merged ops identical and leaves every
|
||||
* untouched field to its own op/LWW.
|
||||
*
|
||||
* Convergence: client A starts from `base+A` and overlays B's fields; client B
|
||||
* starts from `base+B` and overlays A's fields. For every non-noise field the
|
||||
* value is the same (disjoint sets → each field owned by exactly one side); for
|
||||
* every unchanged field the value is `base` on both; for every noise field both
|
||||
* pick the same global tiebreak winner. Therefore `mergedA === mergedB`.
|
||||
* Convergence: for every non-noise field the value is the same (disjoint sets →
|
||||
* each field owned by exactly one side); for every noise field both pick the
|
||||
* same global `(timestamp, clientId)` tiebreak winner. Therefore the delta is
|
||||
* identical on both clients.
|
||||
*/
|
||||
export const synthesizeMergedEntity = (
|
||||
currentEntity: Record<string, unknown>,
|
||||
export const synthesizeMergedChanges = (
|
||||
localChanges: Record<string, unknown>,
|
||||
remoteChanges: Record<string, unknown>,
|
||||
localMeta: MergeSideMeta,
|
||||
remoteMeta: MergeSideMeta,
|
||||
): Record<string, unknown> => {
|
||||
const merged: Record<string, unknown> = { ...currentEntity };
|
||||
const changes: Record<string, unknown> = {};
|
||||
|
||||
// Overlay the remote side's real (non-noise) fields. Local's real fields are
|
||||
// already present in `currentEntity` and are disjoint from these.
|
||||
// Union of both sides' real (non-noise) fields. The two sets are guaranteed
|
||||
// disjoint (isDisjointMergeEligible), so neither overwrites the other.
|
||||
for (const [key, value] of Object.entries(localChanges)) {
|
||||
if (!NOISE_FIELDS.has(key)) {
|
||||
changes[key] = value;
|
||||
}
|
||||
}
|
||||
for (const [key, value] of Object.entries(remoteChanges)) {
|
||||
if (!NOISE_FIELDS.has(key)) {
|
||||
merged[key] = value;
|
||||
changes[key] = value;
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -200,15 +211,15 @@ export const synthesizeMergedEntity = (
|
|||
const localHas = field in localChanges;
|
||||
const remoteHas = field in remoteChanges;
|
||||
if (localHas && remoteHas) {
|
||||
merged[field] = winner === 'local' ? localChanges[field] : remoteChanges[field];
|
||||
changes[field] = winner === 'local' ? localChanges[field] : remoteChanges[field];
|
||||
} else if (localHas) {
|
||||
merged[field] = localChanges[field];
|
||||
changes[field] = localChanges[field];
|
||||
} else {
|
||||
merged[field] = remoteChanges[field];
|
||||
changes[field] = remoteChanges[field];
|
||||
}
|
||||
}
|
||||
|
||||
return merged;
|
||||
return changes;
|
||||
};
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -18,7 +18,7 @@ import {
|
|||
VectorClockComparison,
|
||||
} from '../../core/util/vector-clock';
|
||||
import {
|
||||
synthesizeMergedEntity,
|
||||
synthesizeMergedChanges,
|
||||
isDisjointMergeEligible,
|
||||
} from './conflict-disjoint-merge.util';
|
||||
|
||||
|
|
@ -220,6 +220,90 @@ describe('ConflictResolutionService — SPAP-14 disjoint-field merge', () => {
|
|||
expect(result.localWinOpsCreated).toBe(1);
|
||||
});
|
||||
|
||||
// ── (a3) SPAP-14 fix: partial-delta merged op, no un-conflicted ride-along ──
|
||||
it('(a3) synthesizes a partial-delta merged op that excludes un-conflicted fields', async () => {
|
||||
// Current entity carries a field NEITHER side touched (timeSpentOnDay). A
|
||||
// full-entity snapshot would embed it and diverge across clients whose
|
||||
// current state differs (staggered third-device sync); the delta must carry
|
||||
// ONLY the two sides' changed fields.
|
||||
mockStore.select.and.returnValue(
|
||||
of({
|
||||
id: 'task-1',
|
||||
title: 'Local title',
|
||||
notes: 'base notes',
|
||||
// An un-conflicted field present in current state (value shape is
|
||||
// irrelevant — the delta must not read current state at all).
|
||||
timeSpentOnDay: {},
|
||||
}),
|
||||
);
|
||||
const localOp = op({
|
||||
id: 'local-1',
|
||||
clientId: 'A',
|
||||
vectorClock: { A: 1 },
|
||||
timestamp: 2000,
|
||||
payload: { task: { id: 'task-1', changes: { title: 'Local title' } } },
|
||||
});
|
||||
const remoteOp = op({
|
||||
id: 'remote-1',
|
||||
clientId: 'B',
|
||||
vectorClock: { B: 1 },
|
||||
timestamp: 1000,
|
||||
payload: { task: { id: 'task-1', changes: { notes: 'Remote notes' } } },
|
||||
});
|
||||
|
||||
await service.autoResolveConflictsLWW([conflictOf([localOp], [remoteOp])]);
|
||||
|
||||
const merged = mergedOpArgs();
|
||||
expect(merged).toBeDefined();
|
||||
const payload = merged!.payload as Record<string, unknown>;
|
||||
expect(payload['title']).toBe('Local title');
|
||||
expect(payload['notes']).toBe('Remote notes');
|
||||
// The un-conflicted field must NOT ride along in the synthesized op.
|
||||
expect('timeSpentOnDay' in payload).toBe(false);
|
||||
});
|
||||
|
||||
// ── (a4) SPAP-14 fix: refuse merge when the entity has >1 conflict this batch
|
||||
it('(a4) refuses disjoint-merge for an entity with multiple conflicts (falls back to LWW)', async () => {
|
||||
mockStore.select.and.returnValue(
|
||||
of({ id: 'task-1', title: 'base', notes: 'base', timeEstimate: 5 }),
|
||||
);
|
||||
const localEst = op({
|
||||
id: 'local-est',
|
||||
clientId: 'A',
|
||||
vectorClock: { A: 1 },
|
||||
timestamp: 3000,
|
||||
payload: { task: { id: 'task-1', changes: { timeEstimate: 9 } } },
|
||||
});
|
||||
const remoteTitle = op({
|
||||
id: 'remote-title',
|
||||
clientId: 'B',
|
||||
vectorClock: { B: 1 },
|
||||
timestamp: 3100,
|
||||
payload: { task: { id: 'task-1', changes: { title: 'B title' } } },
|
||||
});
|
||||
const remoteNotes = op({
|
||||
id: 'remote-notes',
|
||||
clientId: 'B',
|
||||
vectorClock: { B: 2 },
|
||||
timestamp: 3200,
|
||||
payload: { task: { id: 'task-1', changes: { notes: 'B notes' } } },
|
||||
});
|
||||
|
||||
// detectConflicts emits one conflict per remote op → two conflicts, same
|
||||
// entity. Merging each independently would let the clock-dominating sibling
|
||||
// silently drop the other's field, falsely journaled as "kept both".
|
||||
await service.autoResolveConflictsLWW([
|
||||
conflictOf([localEst], [remoteTitle]),
|
||||
conflictOf([localEst], [remoteNotes]),
|
||||
]);
|
||||
|
||||
// No merged op was synthesized; both conflicts fell back to whole-entity LWW.
|
||||
const entries = await journal.list('history');
|
||||
expect(entries.length).toBeGreaterThan(0);
|
||||
expect(entries.every((e) => e.winner !== 'merged')).toBe(true);
|
||||
expect((await journal.list('unreviewed')).length).toBeGreaterThan(0);
|
||||
});
|
||||
|
||||
// ── (b) title vs title → LWW unchanged ─────────────────────────────────────
|
||||
it('(b) leaves same-field (title-vs-title) conflicts to LWW (journal unreviewed)', async () => {
|
||||
mockStore.select.and.returnValue(of({ id: 'task-1', title: 'Local title' }));
|
||||
|
|
@ -376,27 +460,22 @@ describe('ConflictResolutionService — SPAP-14 disjoint-field merge', () => {
|
|||
|
||||
// ── (e) two-client convergence ─────────────────────────────────────────────
|
||||
describe('(e) two-client convergence', () => {
|
||||
const base = { id: 'task-1', title: 'base', notes: 'base', modified: 100 };
|
||||
// side1 authored by client A; side2 authored by client B.
|
||||
const side1Changes = { title: 'A-title', modified: 1500 };
|
||||
const side2Changes = { notes: 'B-notes', modified: 1600 };
|
||||
const side1Meta = { timestamp: 1500, clientId: 'A' };
|
||||
const side2Meta = { timestamp: 1600, clientId: 'B' };
|
||||
const currentA = { ...base, ...side1Changes };
|
||||
const currentB = { ...base, ...side2Changes };
|
||||
|
||||
it('both clients synthesize the byte-identical merged entity (either ordering)', () => {
|
||||
it('both clients synthesize the byte-identical merged DELTA (either ordering)', () => {
|
||||
// Client A: local = side1, remote = side2.
|
||||
const mergedA = synthesizeMergedEntity(
|
||||
currentA,
|
||||
const mergedA = synthesizeMergedChanges(
|
||||
side1Changes,
|
||||
side2Changes,
|
||||
side1Meta,
|
||||
side2Meta,
|
||||
);
|
||||
// Client B: local = side2, remote = side1 (mirror).
|
||||
const mergedB = synthesizeMergedEntity(
|
||||
currentB,
|
||||
const mergedB = synthesizeMergedChanges(
|
||||
side2Changes,
|
||||
side1Changes,
|
||||
side2Meta,
|
||||
|
|
@ -404,15 +483,37 @@ describe('ConflictResolutionService — SPAP-14 disjoint-field merge', () => {
|
|||
);
|
||||
|
||||
expect(mergedA).toEqual(mergedB);
|
||||
// Explicit expected entity: both real fields kept; noise → newer (side2).
|
||||
// Explicit expected delta: both real fields kept; noise → newer (side2).
|
||||
// No `id` and NO untouched fields — the delta carries ONLY changed fields.
|
||||
expect(mergedA).toEqual({
|
||||
id: 'task-1',
|
||||
title: 'A-title',
|
||||
notes: 'B-notes',
|
||||
modified: 1600,
|
||||
});
|
||||
});
|
||||
|
||||
it("the merged DELTA is independent of each client's divergent current state (SPAP-14 divergence fix)", () => {
|
||||
// The two clients' current entities differ on an UN-conflicted field
|
||||
// (timeSpentOnDay) — e.g. one already applied a third device's edit the
|
||||
// other has not. A full-entity snapshot would drag that field along and
|
||||
// diverge forever; the delta is derived only from the two sides' ops, so
|
||||
// it is identical regardless. Neither delta may contain timeSpentOnDay.
|
||||
const mergedA = synthesizeMergedChanges(
|
||||
side1Changes,
|
||||
side2Changes,
|
||||
side1Meta,
|
||||
side2Meta,
|
||||
);
|
||||
const mergedB = synthesizeMergedChanges(
|
||||
side2Changes,
|
||||
side1Changes,
|
||||
side2Meta,
|
||||
side1Meta,
|
||||
);
|
||||
expect(mergedA).toEqual(mergedB);
|
||||
expect('timeSpentOnDay' in mergedA).toBe(false);
|
||||
});
|
||||
|
||||
it('both merged clocks dominate BOTH original ops', () => {
|
||||
const clockSide1 = { clientA: 2 };
|
||||
const clockSide2 = { clientB: 2 };
|
||||
|
|
|
|||
|
|
@ -70,7 +70,7 @@ import { buildConflictJournalEntry } from './conflict-journal-emission.util';
|
|||
import {
|
||||
isDisjointMergeEligible,
|
||||
mergeChangedFields,
|
||||
synthesizeMergedEntity,
|
||||
synthesizeMergedChanges,
|
||||
} from './conflict-disjoint-merge.util';
|
||||
|
||||
/**
|
||||
|
|
@ -748,13 +748,39 @@ export class ConflictResolutionService {
|
|||
toEntityKey(entityType as EntityType, entityId),
|
||||
});
|
||||
|
||||
// SPAP-14 hardening: disjoint-merge is only safe for a SINGLE remote op per
|
||||
// entity per batch. detectConflicts emits one conflict per remote op with no
|
||||
// per-entity aggregation, so an entity with ≥2 concurrent remote ops (e.g.
|
||||
// one device edited title then notes offline) would synthesize multiple
|
||||
// merged ops for the same entity; their clocks dominate one another, so a
|
||||
// dominated sibling can be superseded and its field silently dropped —
|
||||
// falsely journaled as a successful "kept both" merge. Refuse the merge for
|
||||
// any entity with >1 conflict this batch and fall back to whole-entity LWW
|
||||
// (baseline behaviour, no false merge). Per-entity aggregation into one op is
|
||||
// a possible future improvement; refusal is the safe floor.
|
||||
const conflictCountByEntity = new Map<string, number>();
|
||||
for (const plan of plans) {
|
||||
const key = toEntityKey(
|
||||
plan.conflict.entityType as EntityType,
|
||||
plan.conflict.entityId,
|
||||
);
|
||||
conflictCountByEntity.set(key, (conflictCountByEntity.get(key) ?? 0) + 1);
|
||||
}
|
||||
|
||||
for (const plan of plans) {
|
||||
// SPAP-14: BEFORE the whole-entity LWW plan, try a disjoint-field merge —
|
||||
// when both sides edited the same entity but DIFFERENT real fields, keep
|
||||
// BOTH instead of discarding the loser. Delete/archive and same-field
|
||||
// (overlapping) conflicts are NOT eligible and fall through to the exact
|
||||
// LWW + SPAP-13 path below, byte-unchanged.
|
||||
const mergedOp = await this._tryCreateDisjointMergeOp(plan);
|
||||
// BOTH instead of discarding the loser. Delete/archive, same-field
|
||||
// (overlapping), and multi-remote-op-per-entity conflicts are NOT eligible
|
||||
// and fall through to the exact LWW + SPAP-13 path below, byte-unchanged.
|
||||
const entityKey = toEntityKey(
|
||||
plan.conflict.entityType as EntityType,
|
||||
plan.conflict.entityId,
|
||||
);
|
||||
const mergedOp =
|
||||
(conflictCountByEntity.get(entityKey) ?? 0) > 1
|
||||
? undefined
|
||||
: await this._tryCreateDisjointMergeOp(plan);
|
||||
if (mergedOp) {
|
||||
mergedResolutions.push({ conflict: plan.conflict, mergedOp });
|
||||
await this._journalMergedResolution(plan);
|
||||
|
|
@ -860,11 +886,14 @@ export class ConflictResolutionService {
|
|||
* path unchanged.
|
||||
*
|
||||
* The merged op is deterministic and CONVERGENT: both clients synthesize the
|
||||
* byte-identical merged entity (union of both sides' disjoint real fields, with
|
||||
* noise fields resolved by a deterministic `(timestamp, clientId)` tiebreak —
|
||||
* see `synthesizeMergedEntity`) and a vector clock that DOMINATES both sides
|
||||
* (via `mergeAndIncrementClocks`, mirroring `_createLocalWinUpdateOp`). The op
|
||||
* uses the standard LWW Update action type and the max timestamp across both
|
||||
* byte-identical merged CHANGES DELTA (union of both sides' disjoint real
|
||||
* fields, with noise fields resolved by a deterministic `(timestamp, clientId)`
|
||||
* tiebreak — see `synthesizeMergedChanges`) and a vector clock that DOMINATES
|
||||
* both sides (via `mergeAndIncrementClocks`, mirroring `_createLocalWinUpdateOp`).
|
||||
* The op carries a PARTIAL delta (not a full-entity snapshot), so untouched
|
||||
* fields that momentarily differ between the two clients can't ride along and
|
||||
* diverge; `lwwUpdateMetaReducer` applies it via `updateOne` (a shallow merge).
|
||||
* It uses the standard LWW Update action type and the max timestamp across both
|
||||
* sides, so when two independently-synthesized merged ops meet they carry
|
||||
* identical payloads and resolve by ordinary LWW — never re-merging.
|
||||
*
|
||||
|
|
@ -916,8 +945,14 @@ export class ConflictResolutionService {
|
|||
const localTs = Math.max(...conflict.localOps.map((op) => op.timestamp));
|
||||
const remoteTs = Math.max(...conflict.remoteOps.map((op) => op.timestamp));
|
||||
|
||||
const mergedEntity = synthesizeMergedEntity(
|
||||
currentEntityState as Record<string, unknown>,
|
||||
// The merged op carries ONLY the union of both sides' changed fields (a
|
||||
// partial delta), NOT a full-entity snapshot of `currentEntityState`. The
|
||||
// delta is derived purely from the two sides' ops, so both clients compute
|
||||
// the byte-identical map — a full snapshot would drag along untouched fields
|
||||
// that can differ between clients under staggered sync and diverge forever.
|
||||
// The lwwUpdateMetaReducer applies it via `updateOne` (a shallow merge), so
|
||||
// fields outside the delta keep their own values. See `synthesizeMergedChanges`.
|
||||
const mergedChanges = synthesizeMergedChanges(
|
||||
localChanges,
|
||||
remoteChanges,
|
||||
{ timestamp: localTs, clientId: conflict.localOps[0]?.clientId ?? clientId },
|
||||
|
|
@ -939,7 +974,7 @@ export class ConflictResolutionService {
|
|||
return this.createLWWUpdateOp(
|
||||
conflict.entityType,
|
||||
conflict.entityId,
|
||||
mergedEntity,
|
||||
mergedChanges,
|
||||
clientId,
|
||||
newClock,
|
||||
mergedTimestamp,
|
||||
|
|
|
|||
|
|
@ -133,6 +133,123 @@ describe('SyncConflictUiService', () => {
|
|||
expect((await journal.getEntry('e1'))?.status).toBe('unreviewed');
|
||||
});
|
||||
|
||||
it('getStaleState() flags stale when a LOSER-ONLY field was edited after resolution', async () => {
|
||||
// Overlapping-field LWW conflict: loser changed title+notes, winner changed
|
||||
// only title (remote won). `notes` is a loser-only field — winnerChangesFor
|
||||
// cannot see it, so the guard was blind to a post-resolution notes edit and
|
||||
// flip would silently overwrite it. getStaleState must now detect it.
|
||||
const entry = makeEntry({
|
||||
id: 'lo1',
|
||||
fieldDiffs: [
|
||||
{
|
||||
field: 'title',
|
||||
localVal: 'Local title',
|
||||
remoteVal: 'Remote title',
|
||||
localChanged: true,
|
||||
remoteChanged: true,
|
||||
pickedSide: 'remote',
|
||||
},
|
||||
{
|
||||
field: 'notes',
|
||||
localVal: 'Loser notes',
|
||||
remoteVal: undefined,
|
||||
localChanged: true,
|
||||
remoteChanged: false,
|
||||
pickedSide: 'remote',
|
||||
},
|
||||
],
|
||||
});
|
||||
// Winner field (title) unchanged, but notes was edited to a value that is
|
||||
// neither the kept value nor what flip would write.
|
||||
store.overrideSelector(selectTaskById, {
|
||||
id: 'task-1',
|
||||
title: 'Remote title',
|
||||
notes: 'USER-EDIT',
|
||||
} as Task);
|
||||
store.refreshState();
|
||||
|
||||
const stale = await service.getStaleState(entry);
|
||||
expect(stale.isStale).toBe(true);
|
||||
});
|
||||
|
||||
it('flip() shows the stale confirm for a post-resolution loser-only edit (previously silent)', async () => {
|
||||
const entry = makeEntry({
|
||||
id: 'lo2',
|
||||
fieldDiffs: [
|
||||
{
|
||||
field: 'title',
|
||||
localVal: 'Local title',
|
||||
remoteVal: 'Remote title',
|
||||
localChanged: true,
|
||||
remoteChanged: true,
|
||||
pickedSide: 'remote',
|
||||
},
|
||||
{
|
||||
field: 'notes',
|
||||
localVal: 'Loser notes',
|
||||
remoteVal: undefined,
|
||||
localChanged: true,
|
||||
remoteChanged: false,
|
||||
pickedSide: 'remote',
|
||||
},
|
||||
],
|
||||
});
|
||||
await journal.record(entry);
|
||||
store.overrideSelector(selectTaskById, {
|
||||
id: 'task-1',
|
||||
title: 'Remote title',
|
||||
notes: 'USER-EDIT',
|
||||
} as Task);
|
||||
store.refreshState();
|
||||
setDialogResult(false); // user cancels → flip must NOT silently overwrite
|
||||
|
||||
const result = await service.flip(entry);
|
||||
|
||||
expect(matDialog.open).toHaveBeenCalled();
|
||||
expect(result).toBe('cancelled');
|
||||
expect(dispatchSpy).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('flipAllToSide() skips stale entries (never silently overwrites in bulk)', async () => {
|
||||
// Remote-won entry whose loser-only notes field diverged after resolution.
|
||||
// Bulk flip-to-local must SKIP it (leave unreviewed), not silently overwrite
|
||||
// the newer notes — the bulk path shows no per-entry confirm.
|
||||
const entry = makeEntry({
|
||||
id: 'bulk1',
|
||||
winner: 'remote',
|
||||
fieldDiffs: [
|
||||
{
|
||||
field: 'title',
|
||||
localVal: 'Local title',
|
||||
remoteVal: 'Remote title',
|
||||
localChanged: true,
|
||||
remoteChanged: true,
|
||||
pickedSide: 'remote',
|
||||
},
|
||||
{
|
||||
field: 'notes',
|
||||
localVal: 'Loser notes',
|
||||
remoteVal: undefined,
|
||||
localChanged: true,
|
||||
remoteChanged: false,
|
||||
pickedSide: 'remote',
|
||||
},
|
||||
],
|
||||
});
|
||||
await journal.record(entry);
|
||||
store.overrideSelector(selectTaskById, {
|
||||
id: 'task-1',
|
||||
title: 'Remote title',
|
||||
notes: 'USER-EDIT',
|
||||
} as Task);
|
||||
store.refreshState();
|
||||
|
||||
await service.flipAllToSide([entry], 'local');
|
||||
|
||||
expect(dispatchSpy).not.toHaveBeenCalled();
|
||||
expect((await journal.getEntry('bulk1'))?.status).toBe('unreviewed');
|
||||
});
|
||||
|
||||
it('flip() suppresses short-syntax parsing on the re-applied title', async () => {
|
||||
// The canonical flip is a rename conflict → changes is exactly { title },
|
||||
// which is precisely the shape shortSyntax$ re-parses in replace mode. A
|
||||
|
|
|
|||
|
|
@ -193,8 +193,12 @@ export class SyncConflictUiService {
|
|||
/**
|
||||
* Bulk FLIP toward one side: applies to rows where that side LOST (so flipping
|
||||
* makes it win). `side='local'` targets remote-won entries; `side='remote'`
|
||||
* targets local-won entries. Merged entries are never touched. The per-entry
|
||||
* stale confirm is skipped for the bulk path (explicit power action).
|
||||
* targets local-won entries. Merged entries are never touched.
|
||||
*
|
||||
* Stale entries are SKIPPED (left unreviewed), never silently flipped: a bulk
|
||||
* action must not overwrite an edit made after the conflict resolved. Because
|
||||
* the bulk path shows no per-entry dialog, we cannot ask — so we refuse the
|
||||
* risky ones and leave them for per-entry flip (which surfaces the confirm).
|
||||
*/
|
||||
async flipAllToSide(
|
||||
entries: readonly ConflictJournalEntry[],
|
||||
|
|
@ -207,6 +211,10 @@ export class SyncConflictUiService {
|
|||
entry.winner === loserIsSide &&
|
||||
this.canFlip(entry)
|
||||
) {
|
||||
const { isStale } = await this.getStaleState(entry);
|
||||
if (isStale) {
|
||||
continue;
|
||||
}
|
||||
await this.flip(entry, { skipStaleConfirm: true });
|
||||
}
|
||||
}
|
||||
|
|
@ -222,11 +230,25 @@ export class SyncConflictUiService {
|
|||
if (!current) {
|
||||
return { isStale: false, current: undefined };
|
||||
}
|
||||
// A field the WINNER changed diverged from its kept value → entity edited
|
||||
// since resolution.
|
||||
const winnerVals = winnerChangesFor(entry);
|
||||
const isStale = Object.keys(winnerVals).some(
|
||||
const winnerStale = Object.keys(winnerVals).some(
|
||||
(field) => !this._valueEquals(current[field], winnerVals[field]),
|
||||
);
|
||||
return { isStale, current };
|
||||
// Loser-only fields: FLIP WILL write these, but the winner never changed
|
||||
// them, so `winnerChangesFor` cannot see them — leaving getStaleState blind
|
||||
// to exactly the fields flip overwrites. If the current value is not already
|
||||
// the value flip would write, a post-resolution edit lives there and flip
|
||||
// would silently destroy it, so force the stale confirm. (Overlaps with
|
||||
// winner-changed fields, e.g. a both-changed `title`, are already covered
|
||||
// above; only loser-ONLY fields need this extra pass.)
|
||||
const flipVals = loserChangesFor(entry);
|
||||
const loserOnlyStale = Object.keys(flipVals).some(
|
||||
(field) =>
|
||||
!(field in winnerVals) && !this._valueEquals(current[field], flipVals[field]),
|
||||
);
|
||||
return { isStale: winnerStale || loserOnlyStale, current };
|
||||
}
|
||||
|
||||
private _buildUpdateAction(
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue