From 41b47be49d0ce0306acfb929080253e8d57e73ff Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Fri, 8 May 2026 16:29:29 +0200 Subject: [PATCH] fix(sync): close 3 latch-bypass gaps from codex review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. SyncHydrationService.hydrateFromRemoteSync runs validateAndRepair() directly and previously dropped the result on failure. Snapshot hydration (file-based providers, USE_REMOTE force-download) would silently accept corrupt remote data — the wrapper would see a clean latch and report IN_SYNC. Now flips SyncSessionValidationService.setFailed() when isValid is false. 2. WsTriggeredDownloadService called downloadRemoteOps() outside the wrapper session contract, so any validation failure during a realtime apply was either dropped (next sync()'s reset cleared it) or leaked into the next session. The service now resets the latch up-front and reads it after the download, surfacing failures as setSyncStatus('ERROR'). 3. convertOpToAction only backfilled payload.id when missing. A malformed/older remote LWW op with payload.id \!= op.entityId would slip through and update the WRONG entity in lwwUpdateMetaReducer (which trusts entityData.id). Now forces id from op.entityId for non-singleton LWW ops, making "entityId is canonical" a hard invariant at the apply boundary. Adds regression tests for each: integration test for the snapshot path, two new tests on ws-triggered-download (latch flip → ERROR; latch reset between sessions), and a converter test for the entityId-mismatch case. --- .../apply/operation-converter.util.spec.ts | 23 +++- .../op-log/apply/operation-converter.util.ts | 18 +-- .../persistence/sync-hydration.service.ts | 18 ++- .../ws-triggered-download.service.spec.ts | 55 +++++++++ .../sync/ws-triggered-download.service.ts | 17 +++ .../post-sync-validation.integration.spec.ts | 112 ++++++++++++++++++ 6 files changed, 230 insertions(+), 13 deletions(-) diff --git a/src/app/op-log/apply/operation-converter.util.spec.ts b/src/app/op-log/apply/operation-converter.util.spec.ts index bbb6034c3c..d8fcede151 100644 --- a/src/app/op-log/apply/operation-converter.util.spec.ts +++ b/src/app/op-log/apply/operation-converter.util.spec.ts @@ -466,16 +466,31 @@ describe('operation-converter utility', () => { expect((action as any).title).toBe('Recovered'); }); - it('preserves an explicitly-set payload id', () => { + it('preserves payload id when it already matches op.entityId', () => { const op = createMockOperation({ actionType: '[TASK] LWW Update' as ActionType, entityId: 'task-canonical', - payload: { id: 'task-explicit', title: 'Explicit' }, + payload: { id: 'task-canonical', title: 'Match' }, }); const action = convertOpToAction(op); - // Explicit id wins; converter only fills when missing. - expect((action as any).id).toBe('task-explicit'); + expect((action as any).id).toBe('task-canonical'); + }); + + // Regression net (#7330 / codex review): a malformed or older remote + // LWW op whose payload.id disagrees with op.entityId would otherwise + // update the WRONG entity in lwwUpdateMetaReducer (which trusts + // entityData.id). The converter must enforce op.entityId as canonical. + it('overrides a payload id that disagrees with op.entityId', () => { + const op = createMockOperation({ + actionType: '[TASK] LWW Update' as ActionType, + entityId: 'task-canonical', + payload: { id: 'task-malicious-or-stale', title: 'Drift' }, + }); + const action = convertOpToAction(op); + + // op.entityId is the canonical identifier — payload.id is overridden. + expect((action as any).id).toBe('task-canonical'); }); it('does NOT inject id for singleton LWW Update (entityId === "*")', () => { diff --git a/src/app/op-log/apply/operation-converter.util.ts b/src/app/op-log/apply/operation-converter.util.ts index b7ac7e1334..99b5ff7ab5 100644 --- a/src/app/op-log/apply/operation-converter.util.ts +++ b/src/app/op-log/apply/operation-converter.util.ts @@ -63,12 +63,16 @@ export const convertOpToAction = (op: Operation): PersistentAction => { ? extractFullStatePayload(op.payload) : (extractActionPayload(op.payload) as Record); - // Backfill `payload.id` for LWW Update ops at the apply boundary. Producers - // also force this in their own creation paths (so the on-disk op shape is - // explicit), but doing it here means every applied LWW op has the id set — - // removing the need for downstream defense-in-depth and covering ops that - // pre-date the producer fixes. Singletons use `SINGLETON_ENTITY_ID` and - // have no `id` field. Issue #7330. + // Force `payload.id = op.entityId` for non-singleton LWW Update ops. The + // op's `entityId` is the canonical identifier — producers also enforce + // this when creating ops, but a malformed/older remote op (or any path + // that ever drifts) could carry a payload.id that disagrees with + // op.entityId, in which case the consumer reducer at + // task-shared-meta-reducers/lww-update.meta-reducer.ts trusts payload.id + // and would update the WRONG entity. Forcing here makes "entityId is + // canonical" a hard invariant at the apply boundary regardless of + // producer or wire shape. Singletons use `SINGLETON_ENTITY_ID` and have + // no `id` field. Issue #7330. if ( !isFullStateOp && isLwwUpdateActionType(actionType) && @@ -76,7 +80,7 @@ export const convertOpToAction = (op: Operation): PersistentAction => { !isSingletonEntityId(op.entityId) && actionPayload && typeof actionPayload === 'object' && - !actionPayload['id'] + actionPayload['id'] !== op.entityId ) { actionPayload = { ...actionPayload, id: op.entityId }; } diff --git a/src/app/op-log/persistence/sync-hydration.service.ts b/src/app/op-log/persistence/sync-hydration.service.ts index 3ad05e9ef7..f7eb8fbb2e 100644 --- a/src/app/op-log/persistence/sync-hydration.service.ts +++ b/src/app/op-log/persistence/sync-hydration.service.ts @@ -7,6 +7,7 @@ import { StateSnapshotService } from '../backup/state-snapshot.service'; import { ClientIdService } from '../../core/util/client-id.service'; import { VectorClockService } from '../sync/vector-clock.service'; import { ValidateStateService } from '../validation/validate-state.service'; +import { SyncSessionValidationService } from '../sync/sync-session-validation.service'; import { loadAllData } from '../../root-store/meta/load-all-data.action'; import { Operation, OpType, ActionType, SyncImportReason } from '../core/operation.types'; import { uuidv7 } from '../../util/uuid-v7'; @@ -43,6 +44,7 @@ export class SyncHydrationService { private clientIdService = inject(ClientIdService); private vectorClockService = inject(VectorClockService); private validateStateService = inject(ValidateStateService); + private sessionValidation = inject(SyncSessionValidationService); private snackService = inject(SnackService); private archiveDbAdapter = inject(ArchiveDbAdapter); @@ -214,8 +216,13 @@ export class SyncHydrationService { lastSeq = await this.opLogStore.getLastSeq(); } - // 7. Validate and repair synced data before dispatching - // This fixes stale task references (e.g., tags/projects referencing deleted tasks) + // 7. Validate and repair synced data before dispatching. + // This fixes stale task references (e.g., tags/projects referencing deleted tasks). + // If the validator reports the data is *not* valid (and repair didn't + // succeed), flip the SyncSessionValidationService latch so the wrapper + // can refuse IN_SYNC. Without this, snapshot hydration would silently + // accept corrupt remote data — a gap not covered by validateAfterSync + // since this path bypasses processRemoteOps entirely. (#7330) let dataToLoad = syncedData as AppDataComplete; const validationResult = await this.validateStateService.validateAndRepair(dataToLoad); @@ -224,6 +231,13 @@ export class SyncHydrationService { dataToLoad = validationResult.repairedState as any; OpLog.normal('SyncHydrationService: Repaired synced data before loading'); } + if (!validationResult.isValid) { + OpLog.err( + 'SyncHydrationService: Validation failed for hydrated remote snapshot — flagging session', + { error: validationResult.error }, + ); + this.sessionValidation.setFailed(); + } // 7b. Restore local-only sync settings into dataToLoad // This ensures the snapshot and NgRx state have the correct local settings, diff --git a/src/app/op-log/sync/ws-triggered-download.service.spec.ts b/src/app/op-log/sync/ws-triggered-download.service.spec.ts index 930d5a5382..da86f913ff 100644 --- a/src/app/op-log/sync/ws-triggered-download.service.spec.ts +++ b/src/app/op-log/sync/ws-triggered-download.service.spec.ts @@ -8,6 +8,7 @@ import { OperationLogSyncService } from './operation-log-sync.service'; import { SyncProviderManager } from '../sync-providers/provider-manager.service'; import { WrappedProviderService } from '../sync-providers/wrapped-provider.service'; import { WsTriggeredDownloadService } from './ws-triggered-download.service'; +import { SyncSessionValidationService } from './sync-session-validation.service'; import { AuthFailSPError, MissingCredentialsSPError } from '../sync-exports'; describe('WsTriggeredDownloadService', () => { @@ -177,4 +178,58 @@ describe('WsTriggeredDownloadService', () => { expect(mockSyncService.downloadRemoteOps).toHaveBeenCalledTimes(1); })); + + // Codex review: WS-triggered downloads run outside the wrapper session + // contract. Without an explicit reset+read here, validation failures + // from realtime sync would be either silently dropped (next sync()'s + // reset clears them) or leak into the next session. The service must + // be its own session boundary. + it('sets sync status ERROR when the download flips the validation latch', fakeAsync(() => { + if (mockProviderManager.setSyncStatus === undefined) { + mockProviderManager.setSyncStatus = jasmine.createSpy('setSyncStatus'); + } + const latch = TestBed.inject(SyncSessionValidationService); + mockSyncService.downloadRemoteOps.and.callFake(async () => { + latch.setFailed(); + return { kind: 'ops_processed' as const, newOpsCount: 1, localWinOpsCreated: 0 }; + }); + + service.start(); + notification$.next({ latestSeq: 1 }); + tick(500); + flushMicrotasks(); + + expect(mockProviderManager.setSyncStatus).toHaveBeenCalledWith('ERROR'); + })); + + it('does not flag ERROR when the download leaves the latch reset', fakeAsync(() => { + if (mockProviderManager.setSyncStatus === undefined) { + mockProviderManager.setSyncStatus = jasmine.createSpy('setSyncStatus'); + } + const latch = TestBed.inject(SyncSessionValidationService); + latch.reset(); + + service.start(); + notification$.next({ latestSeq: 1 }); + tick(500); + flushMicrotasks(); + + expect(mockProviderManager.setSyncStatus).not.toHaveBeenCalledWith('ERROR'); + })); + + // Defense against stale latch from a prior path: the WS service resets + // before its download runs, so the read at the end reflects only this + // session's outcome. + it('resets the latch before each WS download', fakeAsync(() => { + const latch = TestBed.inject(SyncSessionValidationService); + latch.setFailed(); // simulate stale failure from a prior path + + service.start(); + notification$.next({ latestSeq: 1 }); + tick(500); + flushMicrotasks(); + + // After the reset and a clean download, the latch should be back to false. + expect(latch.hasFailed()).toBe(false); + })); }); diff --git a/src/app/op-log/sync/ws-triggered-download.service.ts b/src/app/op-log/sync/ws-triggered-download.service.ts index 4adbe860e3..7c2b4535d2 100644 --- a/src/app/op-log/sync/ws-triggered-download.service.ts +++ b/src/app/op-log/sync/ws-triggered-download.service.ts @@ -5,6 +5,7 @@ import { SuperSyncWebSocketService } from './super-sync-websocket.service'; import { OperationLogSyncService } from './operation-log-sync.service'; import { SyncProviderManager } from '../sync-providers/provider-manager.service'; import { WrappedProviderService } from '../sync-providers/wrapped-provider.service'; +import { SyncSessionValidationService } from './sync-session-validation.service'; import { SyncLog } from '../../core/log'; import { AuthFailSPError, MissingCredentialsSPError } from '../sync-exports'; @@ -26,6 +27,7 @@ export class WsTriggeredDownloadService implements OnDestroy { private _syncService = inject(OperationLogSyncService); private _providerManager = inject(SyncProviderManager); private _wrappedProvider = inject(WrappedProviderService); + private _sessionValidation = inject(SyncSessionValidationService); private _subscription: Subscription | null = null; @@ -62,6 +64,14 @@ export class WsTriggeredDownloadService implements OnDestroy { return; } + // WS-triggered downloads are their own session boundary. Reset the + // latch up-front so the read at the end reflects only this session, + // and a leaked-failed latch from a prior path can't masquerade as a + // failure here. (#7330 — codex review found that without this the + // validation result of a realtime apply was silently dropped before + // the next user-initiated sync() reset the latch.) + this._sessionValidation.reset(); + try { const rawProvider = this._providerManager.getActiveProvider(); if (!rawProvider) { @@ -87,6 +97,13 @@ export class WsTriggeredDownloadService implements OnDestroy { const result = await this._syncService.downloadRemoteOps(syncCapableProvider); SyncLog.log(`WsTriggeredDownloadService: Download complete. kind=${result.kind}`); + + if (this._sessionValidation.hasFailed()) { + SyncLog.err( + 'WsTriggeredDownloadService: Post-sync validation failed during WS download — reporting ERROR', + ); + this._providerManager.setSyncStatus('ERROR'); + } } catch (err) { if (err instanceof AuthFailSPError || err instanceof MissingCredentialsSPError) { SyncLog.warn('WsTriggeredDownloadService: Auth failure during download', err); diff --git a/src/app/op-log/testing/integration/post-sync-validation.integration.spec.ts b/src/app/op-log/testing/integration/post-sync-validation.integration.spec.ts index 3de6f89d2c..1e9b905836 100644 --- a/src/app/op-log/testing/integration/post-sync-validation.integration.spec.ts +++ b/src/app/op-log/testing/integration/post-sync-validation.integration.spec.ts @@ -5,8 +5,13 @@ import { of, BehaviorSubject } from 'rxjs'; import { RemoteOpsProcessingService } from '../../sync/remote-ops-processing.service'; import { ConflictResolutionService } from '../../sync/conflict-resolution.service'; import { SyncSessionValidationService } from '../../sync/sync-session-validation.service'; +import { SyncHydrationService } from '../../persistence/sync-hydration.service'; import { ValidateStateService } from '../../validation/validate-state.service'; import { OperationLogStoreService } from '../../persistence/operation-log-store.service'; +import { StateSnapshotService } from '../../backup/state-snapshot.service'; +import { ClientIdService } from '../../../core/util/client-id.service'; +import { VectorClockService } from '../../sync/vector-clock.service'; +import { ArchiveDbAdapter } from '../../../core/persistence/archive-db-adapter.service'; import { SnackService } from '../../../core/snack/snack.service'; import { CLIENT_ID_PROVIDER } from '../../util/client-id.provider'; @@ -149,6 +154,113 @@ describe('Post-sync validation latch (#7330) — integration', () => { }); }); + describe('SyncHydrationService snapshot path', () => { + let hydrationService: SyncHydrationService; + let validateStateForHydrationSpy: jasmine.SpyObj; + let hydrationLatch: SyncSessionValidationService; + + beforeEach(() => { + // Separate TestBed for the hydration test — wider dependency surface. + TestBed.resetTestingModule(); + validateStateForHydrationSpy = jasmine.createSpyObj('ValidateStateService', [ + 'validateAndRepair', + 'validateAndRepairCurrentState', + ]); + const opLogStoreHydrationSpy = jasmine.createSpyObj('OperationLogStoreService', [ + 'getLastSeq', + 'getUnsynced', + 'markRejected', + 'saveStateCache', + 'setVectorClock', + 'append', + 'loadStateCache', + ]); + opLogStoreHydrationSpy.getLastSeq.and.resolveTo(0); + opLogStoreHydrationSpy.getUnsynced.and.resolveTo([]); + opLogStoreHydrationSpy.loadStateCache.and.resolveTo(null); + const stateSnapshotSpy = jasmine.createSpyObj('StateSnapshotService', [ + 'getStateSnapshot', + 'getAllSyncModelDataFromStoreAsync', + ]); + stateSnapshotSpy.getStateSnapshot.and.returnValue({}); + stateSnapshotSpy.getAllSyncModelDataFromStoreAsync.and.resolveTo({}); + const clientIdSpy = jasmine.createSpyObj('ClientIdService', [ + 'getClientId', + 'getOrGenerateClientId', + ]); + clientIdSpy.getClientId.and.resolveTo('clientTest'); + clientIdSpy.getOrGenerateClientId.and.resolveTo('clientTest'); + const vectorClockSpy = jasmine.createSpyObj('VectorClockService', [ + 'getCurrentVectorClock', + ]); + vectorClockSpy.getCurrentVectorClock.and.resolveTo({}); + const archiveDbSpy = jasmine.createSpyObj('ArchiveDbAdapter', ['load']); + archiveDbSpy.load.and.resolveTo(undefined); + const storeForHydrationSpy = jasmine.createSpyObj('Store', ['dispatch', 'select']); + storeForHydrationSpy.select.and.returnValue( + of({ syncProvider: null, isEnabled: false }), + ); + + TestBed.configureTestingModule({ + providers: [ + SyncSessionValidationService, + SyncHydrationService, + { provide: ValidateStateService, useValue: validateStateForHydrationSpy }, + { provide: OperationLogStoreService, useValue: opLogStoreHydrationSpy }, + { provide: StateSnapshotService, useValue: stateSnapshotSpy }, + { provide: ClientIdService, useValue: clientIdSpy }, + { provide: VectorClockService, useValue: vectorClockSpy }, + { provide: ArchiveDbAdapter, useValue: archiveDbSpy }, + { provide: SnackService, useValue: jasmine.createSpyObj('S', ['open']) }, + { provide: Store, useValue: storeForHydrationSpy }, + { + provide: TranslateService, + useValue: { instant: (k: string): string => k }, + }, + ], + }); + + hydrationService = TestBed.inject(SyncHydrationService); + hydrationLatch = TestBed.inject(SyncSessionValidationService); + hydrationLatch.reset(); + }); + + // Codex review found: hydrateFromRemoteSync runs validateAndRepair + // directly and was not flipping the latch on failure. Snapshot + // hydration (file-based providers, USE_REMOTE force-download) would + // therefore silently accept corrupt remote state. + it('flips the latch when validateAndRepair reports an unrepairable remote snapshot', async () => { + validateStateForHydrationSpy.validateAndRepair.and.resolveTo({ + isValid: false, + wasRepaired: false, + error: 'simulated corruption', + } as never); + + await hydrationService.hydrateFromRemoteSync( + { task: { ids: [], entities: {} } as never }, + { clientRemote: 1 }, + false, + ); + + expect(hydrationLatch.hasFailed()).toBe(true); + }); + + it('leaves the latch reset when validateAndRepair reports a clean snapshot', async () => { + validateStateForHydrationSpy.validateAndRepair.and.resolveTo({ + isValid: true, + wasRepaired: false, + } as never); + + await hydrationService.hydrateFromRemoteSync( + { task: { ids: [], entities: {} } as never }, + { clientRemote: 1 }, + false, + ); + + expect(hydrationLatch.hasFailed()).toBe(false); + }); + }); + describe('latch session semantics', () => { it('multiple validateAfterSync calls within one session keep the latch flipped', async () => { validateStateSpy.validateAndRepairCurrentState.and.resolveTo(false);