mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-17 16:37:43 +00:00
fix(sync): defer validation on snapshot-download path too (#8279)
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.
This commit is contained in:
Johannes Millan
parent
a78d388af0
commit
d7e8d0e33e
3 changed files with 67 additions and 61 deletions
|
|
@ -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<ClientIdService>;
|
||||
let mockVectorClockService: jasmine.SpyObj<VectorClockService>;
|
||||
let mockValidateStateService: jasmine.SpyObj<ValidateStateService>;
|
||||
let mockSessionValidation: jasmine.SpyObj<SyncSessionValidationService>;
|
||||
let mockSnackService: jasmine.SpyObj<SnackService>;
|
||||
|
||||
// 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();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<string, unknown> 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<string, unknown>,
|
||||
);
|
||||
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();
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue