From 8e810edbe72f00af9886f28f454788b85c08163b Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Tue, 14 Jul 2026 19:58:33 +0200 Subject: [PATCH] fix(sync): make marked project deletions win LWW conflicts (#9009) deleteProject cascade-deletes a project's tasks, notes, sections, repeat config, and archive data in one reducer pass. When that op lost an LWW conflict to a concurrent project edit, only the PROJECT entity was reversed: every client resurrected an empty project and the winning client's status-blind hydration replay cascaded its tasks away after a restart (live state != post-restart replay). Rather than recreate every cascaded entity (payload scales with project size and cannot restore every side effect safely), give schema-v4 deleteProject operations explicit delete-wins precedence: - new deleteProject actions carry a shared PROJECT_DELETE_WINS_MARKER; the shared LWW planner accepts a host-supplied delete-wins classifier. A marked remote delete is applied regardless of timestamps; a marked local delete is replaced with one op whose vector clock dominates both sides. - historical unmarked (schema-v3) deletions keep timestamp-based LWW; the absence of the marker (never added by the no-op v3->v4 migration) is the real discriminator, and a schema v3->v4 barrier (mirroring v2->v3) makes older clients block on the newer-schema gate instead of mis-resolving. Delete-wins plans reuse the archive-win resolution pipeline, so they inherit its atomic persistence and losing-op rejection, and disjoint merge leaves them untouched (the delete must win the whole entity). Hardening from multi-agent review: - union allTaskIds/noteIds across multiple concurrent marked deletes for the same project, so a single replacement cannot leave orphan tasks on clients that only receive it (the task reducer removes by allTaskIds). - gate the classifier on the AUTHENTICATED payload projectId matching the plaintext entityId, so a tampered/replayed delete retargeted onto a live entity cannot silently drop a concurrent edit. - guard a null/undefined delete payload in the classifier instead of throwing and wedging the conflict pass. - pin the server's legacy-misc conflict alias to the fixed v1->v2 split boundary, not CURRENT_SCHEMA_VERSION, so this bump does not fabricate false GLOBAL_CONFIG:misc/tasks conflicts during rollout. - bind the marker with a shared const (compiler-checked on producer and consumer) and rename _isArchivePlan -> _isWholeEntityWinPlan. Documents the policy as ARCHITECTURE-DECISIONS.md #7. Addresses #8997. --- ARCHITECTURE-DECISIONS.md | 64 +++++ packages/shared-schema/src/index.ts | 6 +- .../shared-schema/src/migrations/index.ts | 2 + .../project-delete-wins-barrier-v3-to-v4.ts | 17 ++ packages/shared-schema/src/schema-version.ts | 3 +- packages/shared-schema/tests/migrate.spec.ts | 41 +++ .../lww-replacement-barrier-v2-to-v3.spec.ts | 2 +- ...oject-delete-wins-barrier-v3-to-v4.spec.ts | 49 ++++ .../super-sync-server/src/sync/conflict.ts | 12 +- .../super-sync-server/tests/conflict.spec.ts | 14 + packages/sync-core/src/conflict-resolution.ts | 37 ++- .../tests/conflict-resolution.spec.ts | 86 +++++++ .../schema-migration.service.spec.ts | 4 - .../sync/conflict-resolution.service.spec.ts | 241 ++++++++++++++++++ .../sync/conflict-resolution.service.ts | 98 ++++++- .../root-store/meta/task-shared.actions.ts | 10 + .../meta/task-shared.reducer.spec.ts | 10 + 17 files changed, 679 insertions(+), 17 deletions(-) create mode 100644 packages/shared-schema/src/migrations/project-delete-wins-barrier-v3-to-v4.ts create mode 100644 packages/shared-schema/tests/migrations/project-delete-wins-barrier-v3-to-v4.spec.ts diff --git a/ARCHITECTURE-DECISIONS.md b/ARCHITECTURE-DECISIONS.md index 431d00f8e2..d00af733ea 100644 --- a/ARCHITECTURE-DECISIONS.md +++ b/ARCHITECTURE-DECISIONS.md @@ -259,6 +259,70 @@ promoted to the user's active `Passkey` set only when that token is consumed. --- +### 7. Versioned Delete-Wins Semantics for Project Deletion + +**Status**: ✅ Active (since July 2026) + +**Decision**: Project deletions created with schema v4 or newer carry an explicit +`projectDeleteWins` marker and beat concurrent project updates. Historical, +unmarked deletions keep timestamp-based LWW semantics. + +This is a deliberate semantic trade-off: a concurrent project rename or field +edit that is vector-clock CONCURRENT with a marked delete **loses**, regardless +of which has the newer wall-clock timestamp. Deleting an entity another device is +editing wins over the edit — the alternative (timestamp LWW) resurrects an empty +project shell and silently loses its task subtree. The lost edit is only +recoverable via local undo, not via sync. + +**Rationale**: + +- `deleteProject` is one user intent whose reducer cascade removes the project, + active tasks, notes, sections, repeat configuration, and related archive data. + Reversing only the project entity after that operation loses data and violates + replay determinism. +- Capturing every cascaded entity in the delete payload or emitting restoration + sidecars makes payload size scale with project size and still cannot restore + every side effect safely. +- Deletion is the only complete, deterministic result already represented by the + operation. A concurrent rename or project-field edit must not partially undo it. +- The schema-v4 barrier makes clients that do not understand this conflict policy + stop before applying the operation (they block on the newer-schema gate rather + than mis-resolving). The **absence** of the payload marker on historical + deletions — never added by the no-op v3→v4 migration — is what preserves their + timestamp-LWW semantics; the marker, not the version number, is the real + discriminator. The classifier additionally requires the marked delete's + plaintext `entityId` to match its authenticated payload `projectId`, so a + tampered/replayed delete retargeted onto a live entity cannot win. + +**Implementation**: + +- New `deleteProject` actions include `projectDeleteWins: true`; replacement + delete operations preserve that payload. +- The shared LWW planner accepts a host-supplied delete-wins classifier. A remote + marked delete is applied regardless of timestamps. A local marked delete is + replaced with one operation whose vector clock dominates both conflict sides. +- SuperSync keeps its generic conflict protocol: if the first delete upload is + rejected, the existing retry path uploads the causally dominant replacement. + File-based providers use the same client planner and marker. +- Do not add per-task/note restoration operations or project-sized snapshots to + compensate a losing marked project delete. + +**Key Files**: + +- [`task-shared.actions.ts`](src/app/root-store/meta/task-shared.actions.ts) — the `PROJECT_DELETE_WINS_MARKER` producer +- [`conflict-resolution.ts`](packages/sync-core/src/conflict-resolution.ts) +- [`conflict-resolution.service.ts`](src/app/op-log/sync/conflict-resolution.service.ts) — the delete-wins classifier +- [`schema-version.ts`](packages/shared-schema/src/schema-version.ts) +- [`project-delete-wins-barrier-v3-to-v4.ts`](packages/shared-schema/src/migrations/project-delete-wins-barrier-v3-to-v4.ts) (registered in [`migrations/index.ts`](packages/shared-schema/src/migrations/index.ts)) + +**When to Update This Pattern**: + +- Changing the cascade performed by `deleteProject` +- Adding another operation with delete-wins conflict semantics +- Changing schema compatibility or LWW replacement behavior + +--- + ## How to Use This Document ### When Making Architectural Changes diff --git a/packages/shared-schema/src/index.ts b/packages/shared-schema/src/index.ts index ee17caaf40..ee623e8a6a 100644 --- a/packages/shared-schema/src/index.ts +++ b/packages/shared-schema/src/index.ts @@ -1,5 +1,9 @@ // Schema version constants -export { CURRENT_SCHEMA_VERSION, MIN_SUPPORTED_SCHEMA_VERSION } from './schema-version'; +export { + CURRENT_SCHEMA_VERSION, + MIN_SUPPORTED_SCHEMA_VERSION, + PROJECT_DELETE_WINS_SCHEMA_VERSION, +} from './schema-version'; // Types export type { diff --git a/packages/shared-schema/src/migrations/index.ts b/packages/shared-schema/src/migrations/index.ts index d9785d29fc..375931b272 100644 --- a/packages/shared-schema/src/migrations/index.ts +++ b/packages/shared-schema/src/migrations/index.ts @@ -1,6 +1,7 @@ import type { SchemaMigration } from '../migration.types'; import { MiscToTasksSettingsMigration_v1v2 } from './misc-to-tasks-settings-migration-v1-to-v2'; import { LwwReplacementBarrierMigration_v2v3 } from './lww-replacement-barrier-v2-to-v3'; +import { ProjectDeleteWinsBarrierMigration_v3v4 } from './project-delete-wins-barrier-v3-to-v4'; /** * Registry of all schema migrations. @@ -30,4 +31,5 @@ import { LwwReplacementBarrierMigration_v2v3 } from './lww-replacement-barrier-v export const MIGRATIONS: SchemaMigration[] = [ MiscToTasksSettingsMigration_v1v2, LwwReplacementBarrierMigration_v2v3, + ProjectDeleteWinsBarrierMigration_v3v4, ]; diff --git a/packages/shared-schema/src/migrations/project-delete-wins-barrier-v3-to-v4.ts b/packages/shared-schema/src/migrations/project-delete-wins-barrier-v3-to-v4.ts new file mode 100644 index 0000000000..7a53eb9314 --- /dev/null +++ b/packages/shared-schema/src/migrations/project-delete-wins-barrier-v3-to-v4.ts @@ -0,0 +1,17 @@ +import type { SchemaMigration } from '../migration.types'; + +/** + * Compatibility barrier for delete-wins project deletions. + * + * The state shape and historical operations are unchanged. New schema-v4 + * deleteProject operations carry an explicit payload marker; leaving older + * operations untouched ensures they retain their original timestamp-based LWW + * semantics after receiver-side migration. + */ +export const ProjectDeleteWinsBarrierMigration_v3v4: SchemaMigration = { + fromVersion: 3, + toVersion: 4, + description: 'Gate marked project delete-wins conflict semantics', + requiresOperationMigration: false, + migrateState: (state: unknown): unknown => state, +}; diff --git a/packages/shared-schema/src/schema-version.ts b/packages/shared-schema/src/schema-version.ts index 8abbdfdcb4..8585567172 100644 --- a/packages/shared-schema/src/schema-version.ts +++ b/packages/shared-schema/src/schema-version.ts @@ -2,7 +2,8 @@ * Current schema version for all operations and state snapshots. * Increment this BEFORE adding a new migration. */ -export const CURRENT_SCHEMA_VERSION = 3; +export const PROJECT_DELETE_WINS_SCHEMA_VERSION = 4; +export const CURRENT_SCHEMA_VERSION = PROJECT_DELETE_WINS_SCHEMA_VERSION; /** * Minimum schema version that this codebase can still handle. diff --git a/packages/shared-schema/tests/migrate.spec.ts b/packages/shared-schema/tests/migrate.spec.ts index e8f97cafd9..0d3eed72ff 100644 --- a/packages/shared-schema/tests/migrate.spec.ts +++ b/packages/shared-schema/tests/migrate.spec.ts @@ -11,11 +11,22 @@ import { import { CURRENT_SCHEMA_VERSION, MIN_SUPPORTED_SCHEMA_VERSION, + PROJECT_DELETE_WINS_SCHEMA_VERSION, } from '../src/schema-version'; import type { OperationLike, SchemaMigration } from '../src/migration.types'; import { MIGRATIONS } from '../src/migrations'; describe('shared-schema migration functions', () => { + it('includes the schema-v4 project-delete conflict-policy barrier', () => { + expect(CURRENT_SCHEMA_VERSION).toBe(4); + expect(PROJECT_DELETE_WINS_SCHEMA_VERSION).toBe(CURRENT_SCHEMA_VERSION); + expect(MIGRATIONS.at(-1)).toMatchObject({ + fromVersion: 3, + toVersion: 4, + requiresOperationMigration: false, + }); + }); + describe('getCurrentSchemaVersion', () => { it('returns the current schema version', () => { expect(getCurrentSchemaVersion()).toBe(CURRENT_SCHEMA_VERSION); @@ -117,6 +128,36 @@ describe('shared-schema migration functions', () => { }); describe('migrateOperation', () => { + it('keeps a historical project delete unmarked while crossing the v4 barrier', () => { + const op: OperationLike = { + id: 'legacy-project-delete', + opType: 'DEL', + entityType: 'PROJECT', + entityId: 'project-1', + payload: { + actionPayload: { + projectId: 'project-1', + noteIds: [], + allTaskIds: [], + }, + entityChanges: [], + }, + schemaVersion: 3, + }; + + const result = migrateOperation(op); + + expect(result).toMatchObject({ success: true, migratedToVersion: 4 }); + expect(result.data).toEqual({ ...op, schemaVersion: 4 }); + expect( + ( + (result.data as OperationLike).payload as { + actionPayload: { projectDeleteWins?: boolean }; + } + ).actionPayload.projectDeleteWins, + ).toBeUndefined(); + }); + it('returns unchanged operation when already at target version', () => { const op: OperationLike = { id: 'op1', diff --git a/packages/shared-schema/tests/migrations/lww-replacement-barrier-v2-to-v3.spec.ts b/packages/shared-schema/tests/migrations/lww-replacement-barrier-v2-to-v3.spec.ts index 318b3f43d0..85ec181921 100644 --- a/packages/shared-schema/tests/migrations/lww-replacement-barrier-v2-to-v3.spec.ts +++ b/packages/shared-schema/tests/migrations/lww-replacement-barrier-v2-to-v3.spec.ts @@ -6,7 +6,7 @@ import type { OperationLike } from '../../src/migration.types'; describe('LWW replacement compatibility barrier v2 -> v3', () => { it('makes replacement-mode operations visible as a new schema generation', () => { - expect(CURRENT_SCHEMA_VERSION).toBe(3); + expect(CURRENT_SCHEMA_VERSION).toBeGreaterThanOrEqual(3); expect(LwwReplacementBarrierMigration_v2v3.fromVersion).toBe(2); expect(LwwReplacementBarrierMigration_v2v3.toVersion).toBe(3); }); diff --git a/packages/shared-schema/tests/migrations/project-delete-wins-barrier-v3-to-v4.spec.ts b/packages/shared-schema/tests/migrations/project-delete-wins-barrier-v3-to-v4.spec.ts new file mode 100644 index 0000000000..2e1b3057ae --- /dev/null +++ b/packages/shared-schema/tests/migrations/project-delete-wins-barrier-v3-to-v4.spec.ts @@ -0,0 +1,49 @@ +import { describe, expect, it } from 'vitest'; +import { + CURRENT_SCHEMA_VERSION, + PROJECT_DELETE_WINS_SCHEMA_VERSION, +} from '../../src/schema-version'; +import { migrateOperation, migrateState } from '../../src/migrate'; +import { ProjectDeleteWinsBarrierMigration_v3v4 } from '../../src/migrations/project-delete-wins-barrier-v3-to-v4'; +import type { OperationLike } from '../../src/migration.types'; + +describe('project delete-wins compatibility barrier v3 -> v4', () => { + it('makes marked project deletions visible as a new schema generation', () => { + expect(CURRENT_SCHEMA_VERSION).toBe(4); + expect(PROJECT_DELETE_WINS_SCHEMA_VERSION).toBe(4); + expect(ProjectDeleteWinsBarrierMigration_v3v4.fromVersion).toBe(3); + expect(ProjectDeleteWinsBarrierMigration_v3v4.toVersion).toBe(4); + }); + + it('leaves v3 state data unchanged', () => { + const state = { project: { ids: ['project-1'] } }; + + const result = migrateState(state, 3, 4); + + expect(result.success).toBe(true); + expect(result.data).toBe(state); + }); + + it('preserves historical project-delete semantics while stamping schema v4', () => { + const operation: OperationLike = { + id: 'legacy-project-delete', + opType: 'DEL', + entityType: 'PROJECT', + entityId: 'project-1', + payload: { + actionPayload: { + projectId: 'project-1', + noteIds: [], + allTaskIds: [], + }, + entityChanges: [], + }, + schemaVersion: 3, + }; + + const result = migrateOperation(operation, 4); + + expect(result.success).toBe(true); + expect(result.data).toEqual({ ...operation, schemaVersion: 4 }); + }); +}); diff --git a/packages/super-sync-server/src/sync/conflict.ts b/packages/super-sync-server/src/sync/conflict.ts index cc08460a37..5a8148f78e 100644 --- a/packages/super-sync-server/src/sync/conflict.ts +++ b/packages/super-sync-server/src/sync/conflict.ts @@ -416,8 +416,18 @@ export const getConflictEntityIds = (op: Operation): string[] => { return Array.from(new Set(rawEntityIds)); }; +/** + * The misc→tasks settings split happened in the v1→v2 migration + * (`MiscToTasksSettingsMigration_v1v2`), so ONLY pre-v2 `GLOBAL_CONFIG:misc` + * writes also touched what is now `GLOBAL_CONFIG:tasks`. Gate on that fixed + * boundary, not the moving `CURRENT_SCHEMA_VERSION`: otherwise every schema bump + * (e.g. v3→v4) newly aliases already-split misc writes to tasks and fabricates + * conflicts between disjoint settings during rollout. + */ +const MISC_TASKS_SPLIT_SCHEMA_VERSION = 2; + const isLegacyMiscConfigOperation = (op: Operation): boolean => - op.schemaVersion < CURRENT_SCHEMA_VERSION && + op.schemaVersion < MISC_TASKS_SPLIT_SCHEMA_VERSION && op.entityType === 'GLOBAL_CONFIG' && op.entityId === 'misc'; diff --git a/packages/super-sync-server/tests/conflict.spec.ts b/packages/super-sync-server/tests/conflict.spec.ts index ad505e20af..23d5a7cdaf 100644 --- a/packages/super-sync-server/tests/conflict.spec.ts +++ b/packages/super-sync-server/tests/conflict.spec.ts @@ -91,6 +91,20 @@ describe('conflict helpers', () => { }, ); + it.each([2, 3, 4])( + 'does NOT alias a post-split (v%i) misc write to tasks', + (schemaVersion) => { + // The misc→tasks split was the v1→v2 migration; v2+ misc writes touch only + // misc. The legacy boundary must stay fixed at v2 so schema bumps do not + // fabricate conflicts between disjoint settings (regression: v3→v4 bump). + expect( + getConflictEntityIds( + op({ entityType: 'GLOBAL_CONFIG', entityId: 'misc', schemaVersion }), + ), + ).toEqual(['misc']); + }, + ); + it('accepts matching duplicate operations regardless of JSON key order', () => { const incoming = op({ payload: { title: 'A', nested: { b: 2, a: 1 } }, diff --git a/packages/sync-core/src/conflict-resolution.ts b/packages/sync-core/src/conflict-resolution.ts index de4fe468fc..18c597b559 100644 --- a/packages/sync-core/src/conflict-resolution.ts +++ b/packages/sync-core/src/conflict-resolution.ts @@ -11,11 +11,13 @@ import type { SyncLogger } from './sync-logger'; export type ConflictResolutionSuggestion = 'local' | 'remote' | 'manual'; export type LwwConflictResolutionWinner = 'local' | 'remote'; -export type LwwLocalWinOperationKind = 'archive-win' | 'update'; +export type LwwLocalWinOperationKind = 'archive-win' | 'delete-win' | 'update'; export type LwwConflictResolutionReason = | 'remote-archive' | 'local-archive' | 'local-archive-sibling' + | 'remote-delete-wins' + | 'local-delete-wins' | 'local-timestamp' | 'remote-timestamp-or-tie'; @@ -42,6 +44,7 @@ export interface LwwConflictResolutionPlanningOptions< TOperation extends Operation = Operation, > { isArchiveAction: (op: TOperation) => boolean; + isDeleteWinsAction?: (op: TOperation) => boolean; toEntityKey?: (entityType: string, entityId: string) => string; } @@ -334,9 +337,10 @@ export const suggestConflictResolution = >( * Plans last-write-wins conflict resolution without looking up host state or * creating operations. * - * The host supplies archive-action detection because archive semantics are - * domain-specific. The returned plan tells the host whether a local-win op must - * be created and which app-side factory should create it. + * The host supplies archive-action detection and may opt specific operations + * into delete-wins precedence because both policies are domain-specific. The + * returned plan tells the host whether a local-win op must be created and which + * app-side factory should create it. */ export const planLwwConflictResolutions = < TOperation extends Operation = Operation, @@ -386,6 +390,31 @@ export const planLwwConflictResolutions = < }; } + const isDeleteWinsAction = options.isDeleteWinsAction; + const remoteHasDeleteWins = isDeleteWinsAction + ? conflict.remoteOps.some(isDeleteWinsAction) + : false; + const localHasDeleteWins = isDeleteWinsAction + ? conflict.localOps.some(isDeleteWinsAction) + : false; + + if (remoteHasDeleteWins) { + return { + conflict, + winner: 'remote', + reason: 'remote-delete-wins', + }; + } + + if (localHasDeleteWins) { + return { + conflict, + winner: 'local', + reason: 'local-delete-wins', + localWinOperationKind: 'delete-win', + }; + } + const localMaxTimestamp = Math.max(...conflict.localOps.map((op) => op.timestamp)); const remoteMaxTimestamp = Math.max(...conflict.remoteOps.map((op) => op.timestamp)); diff --git a/packages/sync-core/tests/conflict-resolution.spec.ts b/packages/sync-core/tests/conflict-resolution.spec.ts index 212e6f9ddf..86be7e7af4 100644 --- a/packages/sync-core/tests/conflict-resolution.spec.ts +++ b/packages/sync-core/tests/conflict-resolution.spec.ts @@ -192,6 +192,92 @@ describe('suggestConflictResolution', () => { }); describe('planLwwConflictResolutions', () => { + const isDeleteWinsAction = (op: Operation): boolean => + op.actionType === '[Test] Delete Wins'; + + it('lets a remote delete-wins action beat a newer local update', () => { + const conflict = createConflict( + [createOp({ id: 'local-update', timestamp: 2_000 })], + [ + createOp({ + id: 'remote-delete', + actionType: '[Test] Delete Wins', + opType: OpType.Delete, + timestamp: 1_000, + }), + ], + ); + + expect( + planLwwConflictResolutions([conflict], { + isArchiveAction, + isDeleteWinsAction, + }), + ).toEqual([ + { + conflict, + winner: 'remote', + reason: 'remote-delete-wins', + }, + ]); + }); + + it('plans a replacement delete when a local delete-wins action is older', () => { + const conflict = createConflict( + [ + createOp({ + id: 'local-delete', + actionType: '[Test] Delete Wins', + opType: OpType.Delete, + timestamp: 1_000, + }), + ], + [createOp({ id: 'remote-update', timestamp: 2_000 })], + ); + + expect( + planLwwConflictResolutions([conflict], { + isArchiveAction, + isDeleteWinsAction, + }), + ).toEqual([ + { + conflict, + winner: 'local', + reason: 'local-delete-wins', + localWinOperationKind: 'delete-win', + }, + ]); + }); + + it('keeps timestamp semantics for an unmarked delete', () => { + const conflict = createConflict( + [ + createOp({ + id: 'local-delete', + opType: OpType.Delete, + timestamp: 1_000, + }), + ], + [createOp({ id: 'remote-update', timestamp: 2_000 })], + ); + + expect( + planLwwConflictResolutions([conflict], { + isArchiveAction, + isDeleteWinsAction, + }), + ).toEqual([ + { + conflict, + winner: 'remote', + reason: 'remote-timestamp-or-tie', + localMaxTimestamp: 1_000, + remoteMaxTimestamp: 2_000, + }, + ]); + }); + it('lets a remote archive win over local non-archive operations', () => { const conflict = createConflict( [createOp({ id: 'local', timestamp: 2_000 })], diff --git a/src/app/op-log/persistence/schema-migration.service.spec.ts b/src/app/op-log/persistence/schema-migration.service.spec.ts index f4a7ffdf06..da2681fda0 100644 --- a/src/app/op-log/persistence/schema-migration.service.spec.ts +++ b/src/app/op-log/persistence/schema-migration.service.spec.ts @@ -47,10 +47,6 @@ describe('SchemaMigrationService', () => { it('should return the current schema version', () => { expect(service.getCurrentVersion()).toBe(CURRENT_SCHEMA_VERSION); }); - - it('should return 3 for the LWW replacement compatibility barrier', () => { - expect(service.getCurrentVersion()).toBe(3); - }); }); describe('getMigrations', () => { 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 fd9bac6d3f..baaa7c1c56 100644 --- a/src/app/op-log/sync/conflict-resolution.service.spec.ts +++ b/src/app/op-log/sync/conflict-resolution.service.spec.ts @@ -449,6 +449,27 @@ describe('ConflictResolutionService', () => { schemaVersion: 1, }); + const createProjectDelete = ( + id: string, + clientId: string, + timestamp: number, + marked: boolean = true, + ): Operation => ({ + ...createOpWithTimestamp(id, clientId, timestamp, OpType.Delete, 'project-1'), + actionType: ActionType.TASK_SHARED_DELETE_PROJECT, + entityType: 'PROJECT', + schemaVersion: CURRENT_SCHEMA_VERSION, + payload: { + actionPayload: { + projectId: 'project-1', + noteIds: ['note-1'], + allTaskIds: ['task-1'], + ...(marked ? { projectDeleteWins: true } : {}), + }, + entityChanges: [], + }, + }); + // Helper to create conflict with suggestedResolution const createConflict = ( entityId: string, @@ -1093,6 +1114,226 @@ describe('ConflictResolutionService', () => { expect(mockOpLogStore.markRejected).toHaveBeenCalledWith(['local-del']); }); + it('keeps a marked local project deletion when a concurrent update is newer', async () => { + const localDelete = createProjectDelete( + 'local-project-delete', + 'client-a', + 1_000, + ); + const remoteUpdate: Operation = { + ...createOpWithTimestamp( + 'remote-project-update', + 'client-b', + 2_000, + OpType.Update, + 'project-1', + ), + entityType: 'PROJECT', + }; + const conflict: EntityConflict = { + entityType: 'PROJECT', + entityId: 'project-1', + localOps: [localDelete], + remoteOps: [remoteUpdate], + suggestedResolution: 'manual', + }; + + const result = await service.autoResolveConflictsLWW([conflict]); + + const replacementDelete = getFirstMixedLocalOp(); + expect(replacementDelete.opType).toBe(OpType.Delete); + expect(replacementDelete.actionType).toBe(ActionType.TASK_SHARED_DELETE_PROJECT); + expect(replacementDelete.payload).toEqual(localDelete.payload); + expect(replacementDelete.timestamp).toBe(localDelete.timestamp); + expect(replacementDelete.vectorClock['client-a']).toBeGreaterThanOrEqual(1); + expect(replacementDelete.vectorClock['client-b']).toBeGreaterThanOrEqual(1); + expect(replacementDelete.vectorClock[TEST_CLIENT_ID]).toBeGreaterThanOrEqual(1); + expect(result.localWinOpsCreated).toBe(1); + }); + + it('applies a marked remote project deletion even when the local update is newer', async () => { + const localUpdate: Operation = { + ...createOpWithTimestamp( + 'local-project-update', + 'client-a', + 2_000, + OpType.Update, + 'project-1', + ), + entityType: 'PROJECT', + }; + const remoteDelete = createProjectDelete( + 'remote-project-delete', + 'client-b', + 1_000, + ); + const conflict: EntityConflict = { + entityType: 'PROJECT', + entityId: 'project-1', + localOps: [localUpdate], + remoteOps: [remoteDelete], + suggestedResolution: 'manual', + }; + mockOperationApplier.applyOperations.and.resolveTo({ + appliedOps: [remoteDelete], + }); + + const result = await service.autoResolveConflictsLWW([conflict]); + + expect(mockOpLogStore.appendBatchSkipDuplicates).toHaveBeenCalledWith( + [remoteDelete], + 'remote', + { pendingApply: true }, + ); + expect(getMixedLocalOps()).toEqual([]); + const appliedOps = mockOperationApplier.applyOperations.calls.mostRecent() + .args[0] as Operation[]; + expect(appliedOps).toContain(remoteDelete); + expect(result.localWinOpsCreated).toBe(0); + }); + + it('keeps timestamp LWW for a migrated project deletion without the marker', async () => { + const localDelete = createProjectDelete( + 'legacy-project-delete', + 'client-a', + 1_000, + false, + ); + const remoteUpdate: Operation = { + ...createOpWithTimestamp( + 'remote-project-update', + 'client-b', + 2_000, + OpType.Update, + 'project-1', + ), + entityType: 'PROJECT', + }; + const conflict: EntityConflict = { + entityType: 'PROJECT', + entityId: 'project-1', + localOps: [localDelete], + remoteOps: [remoteUpdate], + suggestedResolution: 'manual', + }; + mockOperationApplier.applyOperations.and.resolveTo({ + appliedOps: [remoteUpdate], + }); + + await service.autoResolveConflictsLWW([conflict]); + + expect(mockOpLogStore.appendBatchSkipDuplicates).toHaveBeenCalledWith( + [remoteUpdate], + 'remote', + { pendingApply: true }, + ); + }); + + it('unions allTaskIds/noteIds across multiple concurrent marked deletes (#8997)', async () => { + // Concurrent tabs each captured a deleteProject with a different cascade + // set; the local store applied BOTH. The single replacement must carry + // the union or another client keeps entities only the other delete removed. + const makeDelete = ( + id: string, + taskIds: string[], + noteIds: string[], + ): Operation => ({ + ...createOpWithTimestamp(id, 'client-a', 1_000, OpType.Delete, 'project-1'), + actionType: ActionType.TASK_SHARED_DELETE_PROJECT, + entityType: 'PROJECT', + schemaVersion: CURRENT_SCHEMA_VERSION, + payload: { + actionPayload: { + projectId: 'project-1', + noteIds, + allTaskIds: taskIds, + projectDeleteWins: true, + }, + entityChanges: [], + }, + }); + const deleteA = makeDelete('local-delete-a', ['task-1'], ['note-1']); + const deleteB = makeDelete('local-delete-b', ['task-2'], ['note-2']); + const remoteUpdate: Operation = { + ...createOpWithTimestamp( + 'remote-project-update', + 'client-b', + 2_000, + OpType.Update, + 'project-1', + ), + entityType: 'PROJECT', + }; + const conflict: EntityConflict = { + entityType: 'PROJECT', + entityId: 'project-1', + localOps: [deleteA, deleteB], + remoteOps: [remoteUpdate], + suggestedResolution: 'manual', + }; + + await service.autoResolveConflictsLWW([conflict]); + + const replacementPayload = getFirstMixedLocalOp().payload as { + actionPayload: { allTaskIds: string[]; noteIds: string[] }; + }; + expect(new Set(replacementPayload.actionPayload.allTaskIds)).toEqual( + new Set(['task-1', 'task-2']), + ); + expect(new Set(replacementPayload.actionPayload.noteIds)).toEqual( + new Set(['note-1', 'note-2']), + ); + }); + + it('ignores the marker when entityId does not match the authenticated projectId', async () => { + // Tampered/replayed delete retargeted onto a live entity: marker present + // but op.entityId ('project-1') != payload.projectId ('project-original'). + // Must NOT win delete-wins; falls back to timestamp LWW, so the newer + // local update wins and the retargeted delete is not applied. + const retargetedDelete: Operation = { + ...createProjectDelete('remote-retargeted-delete', 'client-b', 1_000), + payload: { + actionPayload: { + projectId: 'project-original', + noteIds: [], + allTaskIds: [], + projectDeleteWins: true, + }, + entityChanges: [], + }, + }; + const localUpdate: Operation = { + ...createOpWithTimestamp( + 'local-project-update', + 'client-a', + 2_000, + OpType.Update, + 'project-1', + ), + entityType: 'PROJECT', + }; + const conflict: EntityConflict = { + entityType: 'PROJECT', + entityId: 'project-1', + localOps: [localUpdate], + remoteOps: [retargetedDelete], + suggestedResolution: 'manual', + }; + mockOperationApplier.applyOperations.and.resolveTo({ appliedOps: [] }); + + await service.autoResolveConflictsLWW([conflict]); + + // Delete-wins did NOT fire (would apply the delete as a remote winner). + expect(mockOpLogStore.appendBatchSkipDuplicates).not.toHaveBeenCalledWith( + [retargetedDelete], + 'remote', + { pendingApply: true }, + ); + expect(mockOpLogStore.markRejected).toHaveBeenCalledWith([ + 'remote-retargeted-delete', + ]); + }); + it('restamps a converted remote update to the current schema version (#8990)', async () => { // Conversion wraps an older-schema remote update in the v3-only // replacement envelope; the stored row's version must match its diff --git a/src/app/op-log/sync/conflict-resolution.service.ts b/src/app/op-log/sync/conflict-resolution.service.ts index f0df599c58..c456a3b27e 100644 --- a/src/app/op-log/sync/conflict-resolution.service.ts +++ b/src/app/op-log/sync/conflict-resolution.service.ts @@ -19,6 +19,7 @@ import { type LwwConflictResolutionPlan, type LwwResolvedConflict, } from '@sp/sync-core'; +import { PROJECT_DELETE_WINS_SCHEMA_VERSION } from '@sp/shared-schema'; import { findLwwContentConflicts, type LwwContentConflict, @@ -39,6 +40,7 @@ import { VectorClock, } from '../core/operation.types'; import { toLwwUpdateActionType } from '../core/lww-update-action-types'; +import { PROJECT_DELETE_WINS_MARKER } from '../../root-store/meta/task-shared.actions'; import { OperationApplierService } from '../apply/operation-applier.service'; import { HydrationStateService } from '../apply/hydration-state.service'; import { @@ -114,6 +116,68 @@ interface AutoResolveConflictsLwwOptions { remoteApplyLifecycleOwnedByCaller?: boolean; } +const isProjectDeleteWinsOperation = (operation: Operation): boolean => { + // `!(x >= n)` (not `x < n`) so a malformed op with an undefined schemaVersion + // is treated as pre-v4 rather than slipping through. The `!operation.payload` + // guard prevents a null/undefined-payload DEL op (the server permits one) from + // throwing inside `extractActionPayload` and wedging the whole conflict pass. + if ( + !(operation.schemaVersion >= PROJECT_DELETE_WINS_SCHEMA_VERSION) || + operation.actionType !== ActionType.TASK_SHARED_DELETE_PROJECT || + operation.opType !== OpType.Delete || + !operation.payload + ) { + return false; + } + const actionPayload = extractActionPayload(operation.payload); + // Gate on the AUTHENTICATED `projectId` (inside the E2EE GCM auth tag), and + // require it to match the plaintext `entityId` used to group the conflict. + // A tampered/replayed marked delete retargeted onto a live entity therefore + // fails to win delete-wins, so it cannot silently drop the victim's concurrent + // edit — it falls back to timestamp LWW. (GHSA-8pxh metadata-tampering class.) + return ( + actionPayload[PROJECT_DELETE_WINS_MARKER] === true && + operation.entityId === actionPayload['projectId'] + ); +}; + +/** + * Concurrent tabs can capture more than one marked `deleteProject` for the same + * project before syncing, and the local store has applied EVERY one's cascade + * (the task reducer removes entities by explicit `allTaskIds`, not by + * `projectId`). The single winning replacement must therefore carry the UNION of + * all their cascaded `allTaskIds`/`noteIds`, or a client that only receives that + * replacement keeps entities a later local delete already removed. Only the id + * arrays are widened — `projectId` and every other field are identical across + * same-project deletes, so the first op is a safe base. + */ +const mergeMarkedProjectDeleteOps = (localOps: Operation[]): Operation | undefined => { + const deletes = localOps.filter(isProjectDeleteWinsOperation); + if (deletes.length <= 1) { + return deletes[0]; + } + const unionIds = (key: string): string[] => { + const merged = new Set(); + for (const op of deletes) { + const value = extractActionPayload(op.payload)[key]; + if (Array.isArray(value)) { + value.forEach((id) => merged.add(id as string)); + } + } + return [...merged]; + }; + const base = deletes[0]; + const mergedActionPayload: Record = { + ...extractActionPayload(base.payload), + allTaskIds: unionIds('allTaskIds'), + noteIds: unionIds('noteIds'), + }; + const mergedPayload = isMultiEntityPayload(base.payload) + ? { ...base.payload, actionPayload: mergedActionPayload } + : mergedActionPayload; + return { ...base, payload: mergedPayload }; +}; + const getTaskProjectMoveEntityIds = (operation: Operation): string[] | undefined => { if ( operation.actionType === toLwwUpdateActionType('TASK') && @@ -1444,6 +1508,7 @@ export class ConflictResolutionService { const plans = planLwwConflictResolutions(conflicts, { isArchiveAction: (op) => op.actionType === ActionType.TASK_SHARED_MOVE_TO_ARCHIVE, + isDeleteWinsAction: isProjectDeleteWinsOperation, toEntityKey: (entityType, entityId) => toEntityKey(entityType as EntityType, entityId), }); @@ -1506,6 +1571,15 @@ export class ConflictResolutionService { if (plan.localWinOperationKind === 'archive-win') { localWinOp = await this._createArchiveWinOp(plan.conflict); + } else if (plan.localWinOperationKind === 'delete-win') { + const deleteOp = mergeMarkedProjectDeleteOps(plan.conflict.localOps); + if (!deleteOp) { + throw new Error( + `ConflictResolutionService: Missing delete-wins operation for ` + + `${plan.conflict.entityType}:${plan.conflict.entityId}`, + ); + } + localWinOp = await this._createReplacementDeleteOp(plan.conflict, deleteOp); } else if (plan.localWinOperationKind === 'update') { localWinOp = await this._createLocalWinUpdateOp(plan.conflict); } @@ -1527,6 +1601,15 @@ export class ConflictResolutionService { `(${plan.reason === 'remote-archive' ? 'remote' : 'local'} archive) for ` + `${plan.conflict.entityType}:${plan.conflict.entityId}`, ); + } else if ( + plan.reason === 'remote-delete-wins' || + plan.reason === 'local-delete-wins' + ) { + OpLog.normal( + `ConflictResolutionService: Project deletion wins over concurrent update ` + + `(${plan.winner} delete) for ` + + `${plan.conflict.entityType}:${plan.conflict.entityId}`, + ); } else if (plan.winner === 'local') { OpLog.normal( `ConflictResolutionService: LWW resolved ${plan.conflict.entityType}:${plan.conflict.entityId} as LOCAL ` + @@ -1845,15 +1928,20 @@ export class ConflictResolutionService { } /** - * SPAP-14: whether this plan is an archive plan. Archive/delete-wins semantics - * are left 100% untouched by disjoint-merge — the archive must win the whole - * entity, never be partially merged with a concurrent edit. + * SPAP-14: whether this plan must win the WHOLE entity and so is excluded from + * disjoint-field merge. Both archive and project-delete-wins have this + * property — the winner replaces the entity outright, never partially merged + * with a concurrent edit. */ - private _isArchivePlan(plan: LwwConflictResolutionPlan): boolean { + private _isWholeEntityWinPlan( + plan: LwwConflictResolutionPlan, + ): boolean { return ( plan.reason === 'remote-archive' || plan.reason === 'local-archive' || plan.reason === 'local-archive-sibling' || + plan.reason === 'remote-delete-wins' || + plan.reason === 'local-delete-wins' || plan.localWinOperationKind === 'archive-win' ); } @@ -1881,7 +1969,7 @@ export class ConflictResolutionService { private async _tryCreateDisjointMergeOp( plan: LwwConflictResolutionPlan, ): Promise { - if (this._isArchivePlan(plan)) { + if (this._isWholeEntityWinPlan(plan)) { return undefined; } diff --git a/src/app/root-store/meta/task-shared.actions.ts b/src/app/root-store/meta/task-shared.actions.ts index 278b1a6425..1e4ffc6382 100644 --- a/src/app/root-store/meta/task-shared.actions.ts +++ b/src/app/root-store/meta/task-shared.actions.ts @@ -7,6 +7,15 @@ import { BatchOperation } from '@super-productivity/plugin-api'; import { PersistentActionMeta } from '../../op-log/core/persistent-action.interface'; import { OpType } from '../../op-log/core/operation.types'; +/** + * Payload marker stamped on every new `deleteProject` operation so the LWW + * conflict planner can give it delete-wins precedence. Shared with the sync + * classifier (`conflict-resolution.service.ts`) so a rename is compiler-checked + * on both ends — dropping/renaming it silently would regress to resurrecting an + * empty project (see ARCHITECTURE-DECISIONS.md #7). + */ +export const PROJECT_DELETE_WINS_MARKER = 'projectDeleteWins'; + /** * Shared actions that affect multiple reducers (tasks, projects, tags) * These actions are handled by the task-shared meta-reducer @@ -310,6 +319,7 @@ export const TaskSharedActions = createActionGroup({ allTaskIds: string[]; }) => ({ ...taskProps, + [PROJECT_DELETE_WINS_MARKER]: true as const, meta: { isPersistent: true, entityType: 'PROJECT', diff --git a/src/app/root-store/meta/task-shared.reducer.spec.ts b/src/app/root-store/meta/task-shared.reducer.spec.ts index 6415920dd6..4c6dfe3080 100644 --- a/src/app/root-store/meta/task-shared.reducer.spec.ts +++ b/src/app/root-store/meta/task-shared.reducer.spec.ts @@ -1859,6 +1859,16 @@ describe('taskSharedMetaReducer', () => { }); describe('deleteProject action', () => { + it('marks new project deletions with delete-wins conflict semantics', () => { + const action = TaskSharedActions.deleteProject({ + projectId: 'project-1', + noteIds: [], + allTaskIds: [], + }); + + expect(action.projectDeleteWins).toBeTrue(); + }); + it('should remove all project tasks from all tags', () => { const testState = createStateWithExistingTasks( [],