From d7e8d0e33ed20cf0fc651280cfa7f182571f5e98 Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Fri, 12 Jun 2026 15:48:04 +0200 Subject: [PATCH] fix(sync): defer validation on snapshot-download path too (#8279) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The snapshot-download hydration path (first sync / fresh client / USE_REMOTE) called the interactive validateAndRepair, which could still pop the false corruption modal and auto-repair downloaded data — bypassing the Checkpoint D deferral. It now detects-only (validateState): the snapshot loads as-is (benign orphan refs are tolerated by selectors) and the session-validation latch is still flipped on failure. Closes the last modal entry point for the false dialog on receive. --- .../sync-hydration.service.spec.ts | 69 ++++++++++--------- .../persistence/sync-hydration.service.ts | 34 ++++----- .../post-sync-validation.integration.spec.ts | 25 +++---- 3 files changed, 67 insertions(+), 61 deletions(-) diff --git a/src/app/op-log/persistence/sync-hydration.service.spec.ts b/src/app/op-log/persistence/sync-hydration.service.spec.ts index c2fe0c2577..84ceb37f7b 100644 --- a/src/app/op-log/persistence/sync-hydration.service.spec.ts +++ b/src/app/op-log/persistence/sync-hydration.service.spec.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 { ActionType, OpType } from '../core/operation.types'; import { SyncProviderId } from '../sync-providers/provider.const'; @@ -22,6 +23,7 @@ describe('SyncHydrationService', () => { let mockClientIdService: jasmine.SpyObj; let mockVectorClockService: jasmine.SpyObj; let mockValidateStateService: jasmine.SpyObj; + let mockSessionValidation: jasmine.SpyObj; let mockSnackService: jasmine.SpyObj; // Default local sync config for tests @@ -63,7 +65,10 @@ describe('SyncHydrationService', () => { 'getCurrentVectorClock', ]); mockValidateStateService = jasmine.createSpyObj('ValidateStateService', [ - 'validateAndRepair', + 'validateState', + ]); + mockSessionValidation = jasmine.createSpyObj('SyncSessionValidationService', [ + 'setFailed', ]); mockSnackService = jasmine.createSpyObj('SnackService', ['open']); @@ -76,6 +81,7 @@ describe('SyncHydrationService', () => { { provide: ClientIdService, useValue: mockClientIdService }, { provide: VectorClockService, useValue: mockVectorClockService }, { provide: ValidateStateService, useValue: mockValidateStateService }, + { provide: SyncSessionValidationService, useValue: mockSessionValidation }, { provide: SnackService, useValue: mockSnackService }, ], }); @@ -90,9 +96,9 @@ describe('SyncHydrationService', () => { mockOpLogStore.getLastSeq.and.resolveTo(10); mockOpLogStore.saveStateCache.and.resolveTo(undefined); mockOpLogStore.setVectorClock.and.resolveTo(undefined); - mockValidateStateService.validateAndRepair.and.resolveTo({ + mockValidateStateService.validateState.and.resolveTo({ isValid: true, - wasRepaired: false, + typiaErrors: [], }); }; @@ -339,38 +345,42 @@ describe('SyncHydrationService', () => { ); }); - it('should use repaired state when validation detects issues', async () => { + // #8279: snapshot hydration is a sync-receive path — it must NOT auto-repair + // (which can rewrite valid refs across the fleet) nor block with a modal. + // On invalid data it detects-only: loads the snapshot as-is and flags the + // session-validation latch so the wrapper refuses IN_SYNC. + it('loads the snapshot as-is and flags the session when validation fails (defer)', async () => { const downloadedData = { task: { ids: ['t1'] } }; - const repairedState = { task: { ids: ['t1'], repaired: true } } as any; - mockValidateStateService.validateAndRepair.and.resolveTo({ - isValid: true, - wasRepaired: true, - repairedState, + mockValidateStateService.validateState.and.resolveTo({ + isValid: false, + typiaErrors: [], + crossModelError: 'tagId from task not existing', }); await service.hydrateFromRemoteSync(downloadedData); - expect(mockStore.dispatch).toHaveBeenCalledWith( - loadAllData({ - appDataComplete: repairedState as any, - }), - ); - // State cache should also use repaired state - const saveCacheCall = mockOpLogStore.saveStateCache.calls.mostRecent(); - expect(saveCacheCall.args[0].state).toBe(repairedState); + // No repair: the original downloaded data is loaded, not a repaired copy. + const dispatchedLoad = (mockStore.dispatch as jasmine.Spy).calls + .allArgs() + .map((args) => args[0]) + .find((a) => a?.type === loadAllData.type); + expect(dispatchedLoad.appDataComplete.task.ids).toEqual(['t1']); + // And the latch is flagged. + expect(mockSessionValidation.setFailed).toHaveBeenCalled(); }); - it('should use original data when no repair needed', async () => { + it('should use original data when validation passes', async () => { const downloadedData = { task: { ids: ['t1'] } }; - mockValidateStateService.validateAndRepair.and.resolveTo({ + mockValidateStateService.validateState.and.resolveTo({ isValid: true, - wasRepaired: false, + typiaErrors: [], }); await service.hydrateFromRemoteSync(downloadedData); // Should dispatch with the original (merged, stripped) data, not null expect(mockStore.dispatch).toHaveBeenCalled(); + expect(mockSessionValidation.setFailed).not.toHaveBeenCalled(); }); it('should normalize invalid startOfNextDay config before validation and persistence', async () => { @@ -387,7 +397,7 @@ describe('SyncHydrationService', () => { await service.hydrateFromRemoteSync(downloadedData); - const validatedData = mockValidateStateService.validateAndRepair.calls.mostRecent() + const validatedData = mockValidateStateService.validateState.calls.mostRecent() .args[0] as any; expect(validatedData.globalConfig.misc.startOfNextDay).toBe(4); expect(validatedData.globalConfig.misc.startOfNextDayTime).toBe('04:00'); @@ -504,21 +514,16 @@ describe('SyncHydrationService', () => { ); }); - it('should still validate and repair when createSyncImportOp is false', async () => { - const repairedState = { task: { repaired: true } } as any; - mockValidateStateService.validateAndRepair.and.resolveTo({ + it('should still validate when createSyncImportOp is false', async () => { + mockValidateStateService.validateState.and.resolveTo({ isValid: true, - wasRepaired: true, - repairedState, + typiaErrors: [], }); - await service.hydrateFromRemoteSync({ task: {} }, undefined, false); + await service.hydrateFromRemoteSync({ task: { ids: [] } }, undefined, false); - expect(mockStore.dispatch).toHaveBeenCalledWith( - loadAllData({ - appDataComplete: repairedState, - }), - ); + expect(mockValidateStateService.validateState).toHaveBeenCalled(); + expect(mockStore.dispatch).toHaveBeenCalled(); }); }); }); diff --git a/src/app/op-log/persistence/sync-hydration.service.ts b/src/app/op-log/persistence/sync-hydration.service.ts index b377b84806..11e334019a 100644 --- a/src/app/op-log/persistence/sync-hydration.service.ts +++ b/src/app/op-log/persistence/sync-hydration.service.ts @@ -228,34 +228,34 @@ 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). - // 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) + // 7. Validate synced data before dispatching (detect-only). + // #8279: this is a sync-receive path (full snapshot download), so it must + // NOT block with a repair modal and must NOT auto-repair — a cross-device + // merge can leave benign orphan references (tasks pointing at an entity the + // other device deleted) which are tolerated by selectors. Mirroring the + // Checkpoint D deferral, we detect-only: load the snapshot as-is and flip + // the SyncSessionValidationService latch so the wrapper refuses IN_SYNC. + // Without this flag, snapshot hydration would silently accept corrupt remote + // data — a gap not covered by validateAfterSync since this path bypasses + // processRemoteOps entirely. (#7330) const downloadedAppData = locallyReplayableSyncedData as AppDataComplete; const normalizedGlobalConfig = normalizeGlobalConfigStartOfNextDay( downloadedAppData.globalConfig, ); - let dataToLoad = normalizedGlobalConfig + const dataToLoad = normalizedGlobalConfig ? { ...downloadedAppData, globalConfig: normalizedGlobalConfig, } : downloadedAppData; - const validationResult = - await this.validateStateService.validateAndRepair(dataToLoad); - if (validationResult.wasRepaired && validationResult.repairedState) { - // Cast to any since Record doesn't directly map to AppDataComplete - dataToLoad = validationResult.repairedState as any; - OpLog.normal('SyncHydrationService: Repaired synced data before loading'); - } + const validationResult = await this.validateStateService.validateState( + dataToLoad as unknown as Record, + ); if (!validationResult.isValid) { OpLog.err( - 'SyncHydrationService: Validation failed for hydrated remote snapshot — flagging session', - { error: validationResult.error }, + 'SyncHydrationService: Validation failed for hydrated remote snapshot — ' + + 'flagging session (deferring repair, #8279)', + { crossModelError: validationResult.crossModelError }, ); this.sessionValidation.setFailed(); } 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 2b921602a2..fcb099bea7 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 @@ -177,7 +177,7 @@ describe('Post-sync validation latch (#7330) — integration', () => { // Separate TestBed for the hydration test — wider dependency surface. TestBed.resetTestingModule(); validateStateForHydrationSpy = jasmine.createSpyObj('ValidateStateService', [ - 'validateAndRepair', + 'validateState', 'validateAndRepairCurrentState', ]); const opLogStoreHydrationSpy = jasmine.createSpyObj('OperationLogStoreService', [ @@ -239,16 +239,17 @@ describe('Post-sync validation latch (#7330) — integration', () => { hydrationLatch._resetForTest(); }); - // 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 () => { + // Codex review found: hydrateFromRemoteSync was not flipping the latch on + // validation failure. Snapshot hydration (file-based providers, USE_REMOTE + // force-download) would therefore silently accept corrupt remote state. + // #8279: this path now detects-only (no auto-repair/modal) but must still + // flip the latch on failure. + it('flips the latch when validation reports an invalid remote snapshot', async () => { await hydrationLatch.withSession(async () => { - validateStateForHydrationSpy.validateAndRepair.and.resolveTo({ + validateStateForHydrationSpy.validateState.and.resolveTo({ isValid: false, - wasRepaired: false, - error: 'simulated corruption', + typiaErrors: [], + crossModelError: 'simulated corruption', } as never); await hydrationService.hydrateFromRemoteSync( @@ -261,11 +262,11 @@ describe('Post-sync validation latch (#7330) — integration', () => { }); }); - it('leaves the latch reset when validateAndRepair reports a clean snapshot', async () => { + it('leaves the latch reset when validation reports a clean snapshot', async () => { await hydrationLatch.withSession(async () => { - validateStateForHydrationSpy.validateAndRepair.and.resolveTo({ + validateStateForHydrationSpy.validateState.and.resolveTo({ isValid: true, - wasRepaired: false, + typiaErrors: [], } as never); await hydrationService.hydrateFromRemoteSync(