mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-17 16:37:43 +00:00
* fix(sync): don't block on repair dialogs while holding sp_op_log (#9026) Post-remote-apply validation failure ran the data-repair flow — a native confirm() before repair AND a native alert() after — from inside the sp_op_log lock. During unattended background sync the dialog sat open for ~10 min, freezing the event loop and starving every sp_op_log contender (snapshot compaction, write-flush, upload, backup) until the 30s LockAcquisitionTimeout, and widening the file-based recovery-snapshot clobber window (#9023). Make data-repair non-interactive by default (fail-safe), so no automatic or in-lock caller can block on a native dialog: - ValidateStateService.validateAndRepair gains `interactive` (default false): the confirm-before-repair and the repair-failed alert only run for interactive callers. - RepairOperationService.createRepairOperation / _notifyUser gain the same flag: the post-repair "data repaired" acknowledge alert (the common-case blocker the first cut missed) is skipped when non-interactive, and the record is logged non-blockingly instead of via devError (which itself pops a dialog in dev builds). - Since essentially every caller runs automatically and/or inside the lock, the safe behavior is the default and cannot be forgotten. Only the user-initiated, pre-lock USE_REMOTE recovery opts into interactive:true. Automatic in-lock callers (validateAfterSync / conflict-resolution via validateAndRepairCurrentState, snapshot hydration, server migration) all inherit the non-interactive default. The REPAIR op is built identically (dataRepair unchanged), so replay determinism is preserved; repair still re-validates and refuses to dispatch a still-invalid state. Failures stay surfaced via the session-validation latch (#7330) and error snack. Note: silent auto-repair no longer signals the user that data was modified — a gated non-blocking "data repaired" notification is a possible follow-up. * feat(sync): surface a non-blocking snack when background repair changes data (#9026) Making automatic repair non-interactive removed the only signal that the user's data was auto-modified. Restore awareness without reintroducing a blocking dialog: when a non-interactive repair actually changes something (totalFixes > 0), RepairOperationService._notifyUser opens a single non-blocking WARNING snack per session (reusing T.F.SYNC.D_DATA_REPAIRED), so a silent, cross-device-propagating data change isn't wholly invisible. The once-per-session latch mirrors the version-block snack guard so a repeat-repair loop can't spam it. Interactive (foreground) callers keep the existing blocking acknowledge alert. * test(sync): stitched proof the background-sync repair path is dialog-free (#9026) Adds an integration test that drives the full path with BOTH ValidateState- Service and RepairOperationService real — validateAndRepairCurrentState ('sync', { callerHoldsLock: true }) → real dataRepair → real createRepairOperation → real _notifyUser — and asserts it never reaches a blocking window.confirm/window.alert, while still creating the REPAIR op and surfacing the non-blocking "data repaired" snack. Closes the coverage seam the unit specs (which mock RepairOperationService) left open.
This commit is contained in:
Johannes Millan
GitHub
parent
f75346613d
commit
953d680d34
9 changed files with 444 additions and 57 deletions
|
|
@ -287,6 +287,9 @@ export class SyncHydrationService {
|
|||
globalConfig: normalizedGlobalConfig,
|
||||
}
|
||||
: downloadedAppData;
|
||||
// Runs inside the sp_op_log lock (flushThenRunExclusive) during automatic
|
||||
// snapshot hydration, so rely on the non-interactive default — a blocking
|
||||
// dialog on an invalid remote snapshot would starve lock contenders (#9026).
|
||||
const validationResult =
|
||||
await this.validateStateService.validateAndRepair(dataToLoad);
|
||||
if (validationResult.wasRepaired && validationResult.repairedState) {
|
||||
|
|
|
|||
|
|
@ -4979,8 +4979,10 @@ describe('OperationLogSyncService', () => {
|
|||
service.forceDownloadRemoteState(mockProvider),
|
||||
).toBeRejectedWithError(/snapshot is invalid/);
|
||||
|
||||
// USE_REMOTE is a foreground, user-initiated recovery → interactive (#9026).
|
||||
expect(validateStateServiceSpy.validateAndRepair).toHaveBeenCalledOnceWith(
|
||||
snapshotState,
|
||||
{ interactive: true },
|
||||
);
|
||||
expect(backupServiceSpy.captureImportBackup).not.toHaveBeenCalled();
|
||||
expect(opLogStoreSpy.runRemoteStateReplacement).not.toHaveBeenCalled();
|
||||
|
|
@ -5032,6 +5034,7 @@ describe('OperationLogSyncService', () => {
|
|||
}),
|
||||
}),
|
||||
}),
|
||||
{ interactive: true },
|
||||
);
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -1753,7 +1753,14 @@ export class OperationLogSyncService {
|
|||
stripLocalOnlySyncSettingsFromAppData(snapshotState),
|
||||
localOnlySyncSettings,
|
||||
) as Record<string, unknown>;
|
||||
const validation = await this.validateStateService.validateAndRepair(snapshotState);
|
||||
// User-initiated USE_REMOTE recovery: this validates in PHASE 1, before
|
||||
// the destructive replace acquires sp_op_log, and the user is in the
|
||||
// foreground — so keep the interactive confirm/acknowledge dialogs. Every
|
||||
// automatic/in-lock repair path uses the non-interactive default (#9026).
|
||||
const validation = await this.validateStateService.validateAndRepair(
|
||||
snapshotState,
|
||||
{ interactive: true },
|
||||
);
|
||||
if (!validation.isValid) {
|
||||
throw new Error(
|
||||
'USE_REMOTE aborted: remote snapshot is invalid and could not be repaired.',
|
||||
|
|
|
|||
|
|
@ -244,7 +244,9 @@ export class ServerMigrationService {
|
|||
|
||||
// Validate and repair state before creating SYNC_IMPORT
|
||||
// This prevents corrupted state (e.g., orphaned menuTree references) from
|
||||
// propagating to other clients via the full state import.
|
||||
// propagating to other clients via the full state import. Runs inside the
|
||||
// sp_op_log lock (flushThenRunExclusive) during automatic sync, so rely on
|
||||
// the non-interactive default — no blocking dialog under the lock (#9026).
|
||||
const validationResult =
|
||||
await this.validateStateService.validateAndRepair(currentState);
|
||||
|
||||
|
|
|
|||
|
|
@ -8,6 +8,8 @@ import { CURRENT_SCHEMA_VERSION } from '../persistence/schema-migration.service'
|
|||
import { TranslateService } from '@ngx-translate/core';
|
||||
import { RepairSyncContextService } from './repair-sync-context.service';
|
||||
import { StateSnapshotService } from '../backup/state-snapshot.service';
|
||||
import { SnackService } from '../../core/snack/snack.service';
|
||||
import { T } from '../../t.const';
|
||||
|
||||
describe('RepairOperationService', () => {
|
||||
let service: RepairOperationService;
|
||||
|
|
@ -17,6 +19,7 @@ describe('RepairOperationService', () => {
|
|||
let mockVectorClockService: jasmine.SpyObj<VectorClockService>;
|
||||
let repairSyncContext: RepairSyncContextService;
|
||||
let mockStateSnapshotService: jasmine.SpyObj<StateSnapshotService>;
|
||||
let mockSnackService: jasmine.SpyObj<SnackService>;
|
||||
let alertSpy: jasmine.Spy;
|
||||
let confirmSpy: jasmine.Spy;
|
||||
|
||||
|
|
@ -51,6 +54,7 @@ describe('RepairOperationService', () => {
|
|||
mockStateSnapshotService = jasmine.createSpyObj('StateSnapshotService', [
|
||||
'getStateSnapshotAsync',
|
||||
]);
|
||||
mockSnackService = jasmine.createSpyObj('SnackService', ['open']);
|
||||
|
||||
// Default mock implementations
|
||||
mockLockService.request.and.callFake(async <T>(_name: string, fn: () => Promise<T>) =>
|
||||
|
|
@ -94,6 +98,7 @@ describe('RepairOperationService', () => {
|
|||
{ provide: TranslateService, useValue: mockTranslateService },
|
||||
{ provide: VectorClockService, useValue: mockVectorClockService },
|
||||
{ provide: StateSnapshotService, useValue: mockStateSnapshotService },
|
||||
{ provide: SnackService, useValue: mockSnackService },
|
||||
],
|
||||
});
|
||||
|
||||
|
|
@ -258,25 +263,67 @@ describe('RepairOperationService', () => {
|
|||
).toBeRejectedWithError('clientId is required - cannot create repair operation');
|
||||
});
|
||||
|
||||
it('should notify user when fixes were made', async () => {
|
||||
it('should notify user when fixes were made (interactive)', async () => {
|
||||
const summary = createRepairSummary({
|
||||
entityStateFixed: 2,
|
||||
orphanedEntitiesRestored: 3,
|
||||
});
|
||||
|
||||
await service.createRepairOperation(mockRepairedState, summary, 'test-client');
|
||||
await service.createRepairOperation(mockRepairedState, summary, 'test-client', {
|
||||
interactive: true,
|
||||
});
|
||||
|
||||
expect(mockTranslateService.instant).toHaveBeenCalled();
|
||||
expect(alertSpy).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('should always notify user even when no fixes were made', async () => {
|
||||
// Always show alert since user already confirmed the repair
|
||||
it('should always alert an interactive caller even when no fixes were made', async () => {
|
||||
const summary = createRepairSummary(); // All zeros
|
||||
|
||||
await service.createRepairOperation(mockRepairedState, summary, 'test-client', {
|
||||
interactive: true,
|
||||
});
|
||||
|
||||
expect(alertSpy).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
// #9026: the default is non-interactive (automatic/in-lock repair). It must
|
||||
// never reach the blocking "data repaired" alert() — that would hold
|
||||
// sp_op_log open during background sync — but a non-blocking snack still
|
||||
// surfaces the silent data change, and the REPAIR op is still created.
|
||||
it('shows a non-blocking snack (not the blocking alert) for a non-interactive repair', async () => {
|
||||
const summary = createRepairSummary({ entityStateFixed: 2 });
|
||||
|
||||
await service.createRepairOperation(mockRepairedState, summary, 'test-client');
|
||||
|
||||
expect(mockOpLogStore.appendWithVectorClockUpdate).toHaveBeenCalled();
|
||||
// translateService.instant + alert() are only reached by the blocking path.
|
||||
expect(mockTranslateService.instant).not.toHaveBeenCalled();
|
||||
expect(alertSpy).not.toHaveBeenCalled();
|
||||
expect(mockSnackService.open).toHaveBeenCalledWith(
|
||||
jasmine.objectContaining({
|
||||
msg: T.F.SYNC.D_DATA_REPAIRED.MSG,
|
||||
translateParams: { count: 2 },
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it('does not snack a non-interactive repair when nothing changed', async () => {
|
||||
const summary = createRepairSummary(); // All zeros
|
||||
|
||||
await service.createRepairOperation(mockRepairedState, summary, 'test-client');
|
||||
|
||||
expect(alertSpy).toHaveBeenCalled();
|
||||
expect(mockSnackService.open).not.toHaveBeenCalled();
|
||||
expect(alertSpy).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('snacks a non-interactive repair only once per session', async () => {
|
||||
const summary = createRepairSummary({ entityStateFixed: 1 });
|
||||
|
||||
await service.createRepairOperation(mockRepairedState, summary, 'test-client');
|
||||
await service.createRepairOperation(mockRepairedState, summary, 'test-client');
|
||||
|
||||
expect(mockSnackService.open).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('should generate unique operation ID', async () => {
|
||||
|
|
@ -334,7 +381,9 @@ describe('RepairOperationService', () => {
|
|||
typeErrorsFixed: 6,
|
||||
});
|
||||
|
||||
await service.createRepairOperation(mockRepairedState, summary, 'test-client');
|
||||
await service.createRepairOperation(mockRepairedState, summary, 'test-client', {
|
||||
interactive: true,
|
||||
});
|
||||
|
||||
// Total fixes = 1+2+3+4+5+6 = 21
|
||||
expect(mockTranslateService.instant).toHaveBeenCalledWith(
|
||||
|
|
|
|||
|
|
@ -20,6 +20,7 @@ import { LOCK_NAMES } from '../core/operation-log.const';
|
|||
import { alertDialog } from '../../util/native-dialogs';
|
||||
import { RepairSyncContextService } from './repair-sync-context.service';
|
||||
import { StateSnapshotService } from '../backup/state-snapshot.service';
|
||||
import { SnackService } from '../../core/snack/snack.service';
|
||||
|
||||
export interface RebaseStaleRepairOptions {
|
||||
staleRepairOpId: string;
|
||||
|
|
@ -44,6 +45,11 @@ export class RepairOperationService {
|
|||
private vectorClockService = inject(VectorClockService);
|
||||
private repairSyncContext = inject(RepairSyncContextService);
|
||||
private stateSnapshotService = inject(StateSnapshotService);
|
||||
private snackService = inject(SnackService);
|
||||
|
||||
// Once-per-session guard for the non-interactive "data repaired" snack, so a
|
||||
// repeat-repair loop can't spam it (mirrors the version-block snack latch).
|
||||
private _hasShownRepairSnackThisSession = false;
|
||||
|
||||
/**
|
||||
* Creates a REPAIR operation with the repaired state and saves it to the operation log.
|
||||
|
|
@ -53,13 +59,16 @@ export class RepairOperationService {
|
|||
* @param repairSummary - Summary of what was repaired (counts by category)
|
||||
* @param clientId - The client ID for the operation (passed by caller to avoid circular dependency)
|
||||
* @param options.skipLock - If true, skip acquiring sp_op_log lock. Use when caller already holds the lock.
|
||||
* @param options.interactive - If true, show the blocking "data repaired" acknowledge
|
||||
* dialog. Defaults to false (fail-safe): automatic/in-lock repair must never block
|
||||
* on a native dialog, which would hold sp_op_log open during background sync (#9026).
|
||||
* @returns The sequence number of the created operation
|
||||
*/
|
||||
async createRepairOperation(
|
||||
repairedState: unknown,
|
||||
repairSummary: RepairSummary,
|
||||
clientId: string,
|
||||
options?: { skipLock?: boolean },
|
||||
options?: { skipLock?: boolean; interactive?: boolean },
|
||||
): Promise<number> {
|
||||
if (!clientId) {
|
||||
throw new Error('clientId is required - cannot create repair operation');
|
||||
|
|
@ -101,8 +110,8 @@ export class RepairOperationService {
|
|||
await this.lockService.request(LOCK_NAMES.OPERATION_LOG, doCreateOperation);
|
||||
}
|
||||
|
||||
// Notify user that repair happened
|
||||
this._notifyUser(repairSummary);
|
||||
// Notify user that repair happened (non-blocking unless interactive — #9026).
|
||||
this._notifyUser(repairSummary, options?.interactive ?? false);
|
||||
|
||||
return seq;
|
||||
}
|
||||
|
|
@ -173,18 +182,39 @@ export class RepairOperationService {
|
|||
}
|
||||
|
||||
/**
|
||||
* Shows a blocking alert and logs devError when repair happens.
|
||||
* Uses native alert() to prevent reload until user acknowledges.
|
||||
* Always shows alert since user already confirmed the repair.
|
||||
* Records that a repair happened and, for interactive (foreground) callers,
|
||||
* shows a blocking native alert() acknowledgement.
|
||||
*
|
||||
* The `interactive` gate is load-bearing for #9026: automatic/in-lock repair
|
||||
* (background sync) must never reach a blocking dialog — neither this alert()
|
||||
* nor `devError`, which itself pops a blocking alert/confirm in dev builds —
|
||||
* or it would hold the sp_op_log lock open for as long as the dialog sits
|
||||
* there. The summary is six numeric counts (no user content), safe to log.
|
||||
*/
|
||||
private _notifyUser(summary: RepairSummary): void {
|
||||
private _notifyUser(summary: RepairSummary, interactive: boolean): void {
|
||||
const totalFixes = this._getTotalFixes(summary);
|
||||
const logMsg = `Data repair executed: ${totalFixes} issues fixed. Summary: ${JSON.stringify(summary)}`;
|
||||
|
||||
// Always log
|
||||
const errorMsg = `Data repair executed: ${totalFixes} issues fixed. Summary: ${JSON.stringify(summary)}`;
|
||||
devError(errorMsg);
|
||||
if (!interactive) {
|
||||
// Automatic/in-lock repair (background sync): never a native dialog — that
|
||||
// would hold sp_op_log open (#9026). Record it, and surface a single
|
||||
// non-blocking snack per session so a silent data change (auto-repair can
|
||||
// drop entities/refs and propagate cross-device) isn't wholly invisible.
|
||||
// Only when something actually changed.
|
||||
OpLog.err(logMsg);
|
||||
if (totalFixes > 0 && !this._hasShownRepairSnackThisSession) {
|
||||
this._hasShownRepairSnackThisSession = true;
|
||||
this.snackService.open({
|
||||
type: 'WARNING',
|
||||
msg: T.F.SYNC.D_DATA_REPAIRED.MSG,
|
||||
translateParams: { count: totalFixes },
|
||||
});
|
||||
}
|
||||
return;
|
||||
}
|
||||
|
||||
// Always show alert after repair completes (user already confirmed)
|
||||
// Foreground: loud dev diagnostic + user acknowledgement.
|
||||
devError(logMsg);
|
||||
const title = this.translateService.instant(T.F.SYNC.D_DATA_REPAIRED.TITLE);
|
||||
const msg = this.translateService.instant(T.F.SYNC.D_DATA_REPAIRED.MSG, {
|
||||
count: totalFixes.toString(),
|
||||
|
|
|
|||
|
|
@ -0,0 +1,183 @@
|
|||
import { TestBed } from '@angular/core/testing';
|
||||
import { provideMockStore } from '@ngrx/store/testing';
|
||||
import { TranslateService } from '@ngx-translate/core';
|
||||
import { ValidateStateService } from './validate-state.service';
|
||||
import { StateSnapshotService } from '../backup/state-snapshot.service';
|
||||
import { OperationLogStoreService } from '../persistence/operation-log-store.service';
|
||||
import { LockService } from '../sync/lock.service';
|
||||
import { VectorClockService } from '../sync/vector-clock.service';
|
||||
import { CLIENT_ID_PROVIDER } from '../util/client-id.provider';
|
||||
import { HydrationStateService } from '../apply/hydration-state.service';
|
||||
import { SnackService } from '../../core/snack/snack.service';
|
||||
import { DEFAULT_GLOBAL_CONFIG } from '../../features/config/default-global-config.const';
|
||||
import { plannerInitialState } from '../../features/planner/store/planner.reducer';
|
||||
import { initialTimeTrackingState } from '../../features/time-tracking/store/time-tracking.reducer';
|
||||
import { initialMetricState } from '../../features/metric/store/metric.reducer';
|
||||
import { menuTreeInitialState } from '../../features/menu-tree/store/menu-tree.reducer';
|
||||
import {
|
||||
MenuTreeKind,
|
||||
MenuTreeState,
|
||||
} from '../../features/menu-tree/store/menu-tree.model';
|
||||
import { environment } from '../../../environments/environment';
|
||||
import { T } from '../../t.const';
|
||||
|
||||
/**
|
||||
* #9026 regression — stitched, real-service integration.
|
||||
*
|
||||
* The unit specs verify each layer separately (validate-state mocks
|
||||
* RepairOperationService; repair-operation is tested in isolation). This test
|
||||
* drives the FULL background-sync repair path with BOTH services real —
|
||||
* `validateAndRepairCurrentState('sync', { callerHoldsLock: true })` →
|
||||
* real `validateAndRepair` (invalid state → real dataRepair) → real
|
||||
* `createRepairOperation` → real `_notifyUser` — and asserts the whole chain
|
||||
* never reaches a blocking native `confirm()`/`alert()` (which would hold the
|
||||
* sp_op_log lock during background sync), while still creating the REPAIR op
|
||||
* and surfacing the non-blocking "data repaired" snack.
|
||||
*/
|
||||
describe('#9026 background-sync repair is non-blocking (stitched)', () => {
|
||||
let service: ValidateStateService;
|
||||
let mockOpLogStore: jasmine.SpyObj<OperationLogStoreService>;
|
||||
let mockSnackService: jasmine.SpyObj<SnackService>;
|
||||
let confirmSpy: jasmine.Spy;
|
||||
let alertSpy: jasmine.Spy;
|
||||
let originalProduction: boolean;
|
||||
|
||||
const createEmptyState = (): Record<string, unknown> => ({
|
||||
task: { ids: [], entities: {}, currentTaskId: null },
|
||||
project: { ids: [], entities: {} },
|
||||
tag: { ids: [], entities: {} },
|
||||
note: { ids: [], entities: {}, todayOrder: [] },
|
||||
section: { ids: [], entities: {} },
|
||||
simpleCounter: { ids: [], entities: {} },
|
||||
issueProvider: { ids: [], entities: {} },
|
||||
taskRepeatCfg: { ids: [], entities: {} },
|
||||
metric: initialMetricState,
|
||||
boards: { boardCfgs: [] },
|
||||
planner: plannerInitialState,
|
||||
menuTree: menuTreeInitialState,
|
||||
globalConfig: DEFAULT_GLOBAL_CONFIG,
|
||||
timeTracking: initialTimeTrackingState,
|
||||
reminders: [],
|
||||
pluginUserData: [],
|
||||
pluginMetadata: [],
|
||||
archiveYoung: {
|
||||
task: { ids: [], entities: {} },
|
||||
timeTracking: initialTimeTrackingState,
|
||||
lastTimeTrackingFlush: 0,
|
||||
},
|
||||
archiveOld: {
|
||||
task: { ids: [], entities: {} },
|
||||
timeTracking: initialTimeTrackingState,
|
||||
lastTimeTrackingFlush: 0,
|
||||
},
|
||||
});
|
||||
|
||||
beforeEach(() => {
|
||||
const invalidState = createEmptyState();
|
||||
// Orphaned project reference in menuTree → cross-model validation fails,
|
||||
// dataRepair prunes it (a real, repairable inconsistency).
|
||||
invalidState.menuTree = {
|
||||
...(invalidState.menuTree as MenuTreeState),
|
||||
projectTree: [{ id: 'ORPHAN_PROJECT', k: MenuTreeKind.PROJECT }],
|
||||
};
|
||||
|
||||
const mockStateSnapshotService = jasmine.createSpyObj('StateSnapshotService', [
|
||||
'getStateSnapshot',
|
||||
'getStateSnapshotForOperationLogAsync',
|
||||
'getStateSnapshotAsync',
|
||||
]);
|
||||
mockStateSnapshotService.getStateSnapshot.and.returnValue(invalidState as never);
|
||||
mockStateSnapshotService.getStateSnapshotForOperationLogAsync.and.resolveTo(
|
||||
invalidState as never,
|
||||
);
|
||||
|
||||
mockOpLogStore = jasmine.createSpyObj('OperationLogStoreService', [
|
||||
'appendWithVectorClockUpdate',
|
||||
'saveStateCache',
|
||||
]);
|
||||
mockOpLogStore.appendWithVectorClockUpdate.and.resolveTo(1);
|
||||
mockOpLogStore.saveStateCache.and.resolveTo();
|
||||
|
||||
const mockLockService = jasmine.createSpyObj('LockService', ['request']);
|
||||
mockLockService.request.and.callFake(
|
||||
async <T>(_name: string, fn: () => Promise<T>): Promise<T> => fn(),
|
||||
);
|
||||
|
||||
const mockVectorClockService = jasmine.createSpyObj('VectorClockService', [
|
||||
'getCurrentVectorClock',
|
||||
]);
|
||||
mockVectorClockService.getCurrentVectorClock.and.resolveTo({});
|
||||
|
||||
const mockHydrationStateService = jasmine.createSpyObj('HydrationStateService', [
|
||||
'isApplyingRemoteOps',
|
||||
'startApplyingRemoteOps',
|
||||
'endApplyingRemoteOps',
|
||||
'startPostSyncCooldown',
|
||||
]);
|
||||
mockHydrationStateService.isApplyingRemoteOps.and.returnValue(false);
|
||||
|
||||
mockSnackService = jasmine.createSpyObj('SnackService', ['open']);
|
||||
|
||||
const mockTranslate = { instant: (k: string): string => k };
|
||||
const mockClientIdProvider = {
|
||||
loadClientId: jasmine.createSpy('loadClientId').and.resolveTo('test-client'),
|
||||
};
|
||||
|
||||
TestBed.configureTestingModule({
|
||||
providers: [
|
||||
provideMockStore(),
|
||||
// ValidateStateService + RepairOperationService + RepairSyncContextService
|
||||
// are all providedIn: 'root' — left REAL so the chain is genuinely stitched.
|
||||
{ provide: StateSnapshotService, useValue: mockStateSnapshotService },
|
||||
{ provide: OperationLogStoreService, useValue: mockOpLogStore },
|
||||
{ provide: LockService, useValue: mockLockService },
|
||||
{ provide: VectorClockService, useValue: mockVectorClockService },
|
||||
{ provide: HydrationStateService, useValue: mockHydrationStateService },
|
||||
{ provide: SnackService, useValue: mockSnackService },
|
||||
{ provide: TranslateService, useValue: mockTranslate },
|
||||
{ provide: CLIENT_ID_PROVIDER, useValue: mockClientIdProvider },
|
||||
],
|
||||
});
|
||||
service = TestBed.inject(ValidateStateService);
|
||||
|
||||
// production=true → devError only logs (cannot pop a dialog), so any dialog
|
||||
// would have to come from the repair path itself — which is what we assert against.
|
||||
originalProduction = environment.production;
|
||||
(environment as unknown as { production: boolean }).production = true;
|
||||
spyOn(localStorage, 'setItem'); // don't touch the shared Karma localStorage
|
||||
|
||||
confirmSpy = jasmine.isSpy(window.confirm)
|
||||
? (window.confirm as jasmine.Spy)
|
||||
: spyOn(window, 'confirm');
|
||||
confirmSpy.and.returnValue(false);
|
||||
confirmSpy.calls.reset();
|
||||
alertSpy = jasmine.isSpy(window.alert)
|
||||
? (window.alert as jasmine.Spy)
|
||||
: spyOn(window, 'alert');
|
||||
alertSpy.and.stub();
|
||||
alertSpy.calls.reset();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
// Restore the shared module-global so this spec can't pollute others.
|
||||
(environment as unknown as { production: boolean }).production = originalProduction;
|
||||
});
|
||||
|
||||
it('repairs a corrupt state under the lock without any blocking dialog, and snacks it', async () => {
|
||||
const isValid = await service.validateAndRepairCurrentState('sync', {
|
||||
callerHoldsLock: true,
|
||||
});
|
||||
|
||||
// Repaired and reported valid.
|
||||
expect(isValid).toBeTrue();
|
||||
// No blocking native dialog anywhere on the path.
|
||||
expect(confirmSpy).not.toHaveBeenCalled();
|
||||
expect(alertSpy).not.toHaveBeenCalled();
|
||||
// A REPAIR op was actually created (real createRepairOperation ran).
|
||||
expect(mockOpLogStore.appendWithVectorClockUpdate).toHaveBeenCalled();
|
||||
// ...and the non-blocking awareness snack was surfaced.
|
||||
expect(mockSnackService.open).toHaveBeenCalledWith(
|
||||
jasmine.objectContaining({ msg: T.F.SYNC.D_DATA_REPAIRED.MSG }),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
|
@ -130,8 +130,8 @@ describe('ValidateStateService', () => {
|
|||
],
|
||||
};
|
||||
|
||||
// Should repair
|
||||
const result = await service.validateAndRepair(state);
|
||||
// Should repair (interactive: user is prompted and confirms)
|
||||
const result = await service.validateAndRepair(state, { interactive: true });
|
||||
|
||||
expect(result.isValid).toBeTrue();
|
||||
expect(result.wasRepaired).toBeTrue();
|
||||
|
|
@ -176,8 +176,8 @@ describe('ValidateStateService', () => {
|
|||
],
|
||||
};
|
||||
|
||||
// Should not repair when user declines
|
||||
const result = await service.validateAndRepair(state);
|
||||
// Should not repair when user declines (interactive: prompt is shown)
|
||||
const result = await service.validateAndRepair(state, { interactive: true });
|
||||
|
||||
expect(result.isValid).toBeFalse();
|
||||
expect(result.wasRepaired).toBeFalse();
|
||||
|
|
@ -187,6 +187,47 @@ describe('ValidateStateService', () => {
|
|||
}
|
||||
});
|
||||
|
||||
it('auto-repairs without prompting when interactive is false (#9026)', async () => {
|
||||
// Non-interactive repair (automatic sync path) must never reach the blocking
|
||||
// native confirm()/alert(): during background sync it would freeze the event
|
||||
// loop and hold sp_op_log open for as long as the dialog sits there.
|
||||
const originalEnvProduction = environment.production;
|
||||
(environment as any).production = false;
|
||||
|
||||
const alertSpy = jasmine.isSpy(window.alert)
|
||||
? (window.alert as jasmine.Spy)
|
||||
: spyOn(window, 'alert');
|
||||
alertSpy.and.stub();
|
||||
alertSpy.calls.reset();
|
||||
const confirmSpy = jasmine.isSpy(window.confirm)
|
||||
? (window.confirm as jasmine.Spy)
|
||||
: spyOn(window, 'confirm');
|
||||
// If confirm were reached it would decline the repair, failing the
|
||||
// assertions below loudly rather than silently.
|
||||
confirmSpy.and.returnValue(false);
|
||||
confirmSpy.calls.reset();
|
||||
|
||||
try {
|
||||
const state = createEmptyState();
|
||||
state.menuTree = {
|
||||
...(state.menuTree as MenuTreeState),
|
||||
projectTree: [{ id: 'NON_EXISTENT_PROJECT_ID', k: MenuTreeKind.PROJECT }],
|
||||
};
|
||||
|
||||
const result = await service.validateAndRepair(state, { interactive: false });
|
||||
|
||||
expect(confirmSpy).not.toHaveBeenCalled();
|
||||
expect(alertSpy).not.toHaveBeenCalled();
|
||||
expect(result.isValid).toBeTrue();
|
||||
expect(result.wasRepaired).toBeTrue();
|
||||
expect((result.repairedState!.menuTree as MenuTreeState).projectTree!.length).toBe(
|
||||
0,
|
||||
);
|
||||
} finally {
|
||||
(environment as any).production = originalEnvProduction;
|
||||
}
|
||||
});
|
||||
|
||||
describe('records the critical-error signal (rating-prompt cooldown)', () => {
|
||||
let setItemSpy: jasmine.Spy;
|
||||
|
||||
|
|
@ -453,5 +494,49 @@ describe('ValidateStateService', () => {
|
|||
.args[0] as any;
|
||||
expect(repairedState.archiveYoung.task.ids).toEqual([archivedTaskId]);
|
||||
});
|
||||
|
||||
// #9026: the sync repair path runs inside the sp_op_log lock during
|
||||
// background sync, so it must never show a blocking native dialog. Drive a
|
||||
// real repair (invalid menuTree) and assert neither confirm nor alert fires
|
||||
// while the state is still repaired and a REPAIR op created.
|
||||
it('never shows a blocking dialog on the sync repair path', async () => {
|
||||
const originalEnvProduction = environment.production;
|
||||
(environment as any).production = false;
|
||||
|
||||
const alertSpy = jasmine.isSpy(window.alert)
|
||||
? (window.alert as jasmine.Spy)
|
||||
: spyOn(window, 'alert');
|
||||
alertSpy.and.stub();
|
||||
alertSpy.calls.reset();
|
||||
const confirmSpy = jasmine.isSpy(window.confirm)
|
||||
? (window.confirm as jasmine.Spy)
|
||||
: spyOn(window, 'confirm');
|
||||
confirmSpy.and.returnValue(false);
|
||||
confirmSpy.calls.reset();
|
||||
|
||||
try {
|
||||
const invalidState = createEmptyState();
|
||||
invalidState.menuTree = {
|
||||
...(invalidState.menuTree as MenuTreeState),
|
||||
projectTree: [{ id: 'ORPHAN', k: MenuTreeKind.PROJECT }],
|
||||
};
|
||||
mockStateSnapshotService.getStateSnapshot.and.returnValue(invalidState as any);
|
||||
mockStateSnapshotService.getStateSnapshotForOperationLogAsync.and.resolveTo(
|
||||
invalidState as any,
|
||||
);
|
||||
|
||||
const result = await service.validateAndRepairCurrentState('sync', {
|
||||
callerHoldsLock: true,
|
||||
});
|
||||
|
||||
expect(result).toBeTrue();
|
||||
expect(confirmSpy).not.toHaveBeenCalled();
|
||||
expect(alertSpy).not.toHaveBeenCalled();
|
||||
// Repair still happened silently — the REPAIR op is created.
|
||||
expect(mockRepairService.createRepairOperation).toHaveBeenCalled();
|
||||
} finally {
|
||||
(environment as any).production = originalEnvProduction;
|
||||
}
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -121,6 +121,10 @@ export class ValidateStateService {
|
|||
const currentState =
|
||||
await this.stateSnapshotService.getStateSnapshotForOperationLogAsync();
|
||||
|
||||
// This whole method runs inside the sp_op_log lock during background sync,
|
||||
// so repair must stay non-interactive: validateAndRepair and (below)
|
||||
// createRepairOperation both default to non-interactive, so no blocking
|
||||
// native dialog is shown while the lock is held (#9026).
|
||||
const result = await this.validateAndRepair(
|
||||
currentState as unknown as Record<string, unknown>,
|
||||
);
|
||||
|
|
@ -150,7 +154,9 @@ export class ValidateStateService {
|
|||
return false;
|
||||
}
|
||||
|
||||
// Create REPAIR operation first (before dispatching state)
|
||||
// Create REPAIR operation first (before dispatching state). Non-interactive
|
||||
// by default, so its "data repaired" acknowledge dialog never blocks the
|
||||
// held lock during background sync (#9026).
|
||||
await this.repairOperationService.createRepairOperation(
|
||||
result.repairedState,
|
||||
result.repairSummary,
|
||||
|
|
@ -250,25 +256,29 @@ export class ValidateStateService {
|
|||
* Validates state and repairs if necessary.
|
||||
* Returns the (possibly repaired) state and repair summary.
|
||||
*
|
||||
* Shows a confirmation dialog before executing repair to give users
|
||||
* explicit control over when repair runs.
|
||||
* ## Interactive vs. non-interactive (`options.interactive`, default FALSE)
|
||||
* The default is non-interactive (auto-repair, no dialog) — a deliberate
|
||||
* fail-safe default. Interactive repair uses native `confirm()`/`alert()`,
|
||||
* which block the JS thread; shown while the `sp_op_log` lock is held during
|
||||
* background sync, a blocking dialog freezes the event loop and starves lock
|
||||
* contenders (e.g. snapshot compaction) for as long as the dialog sits open
|
||||
* (#9026). Since essentially every caller runs automatically and/or inside
|
||||
* that lock, the safe behavior is the default and can't be forgotten: the
|
||||
* state is already invalid, repair is the safe recovery, so we auto-repair
|
||||
* and surface any failure via the caller's non-blocking session-validation
|
||||
* latch + snack. Only genuinely foreground, non-lock callers (the
|
||||
* user-initiated USE_REMOTE flow) pass `interactive: true` to keep the
|
||||
* confirm-before-repair and acknowledge dialogs.
|
||||
*
|
||||
* ## Note on Blocking confirm()
|
||||
* Uses native `confirm()` which blocks the JS thread. This is intentional:
|
||||
* - Prevents race conditions during repair
|
||||
* - Ensures user explicitly acknowledges before data modification
|
||||
* However, this could cause issues if called during background sync with
|
||||
* user not actively looking at the app. Consider deferring repair to app
|
||||
* foreground if this becomes problematic.
|
||||
*
|
||||
* ## TOCTOU Limitation
|
||||
* ## TOCTOU Limitation (interactive path)
|
||||
* The state snapshot passed to this method is validated, then user confirms,
|
||||
* then repair runs on that same snapshot. If the actual NgRx state changed
|
||||
* during the confirm dialog (via another tab, service worker, or user action),
|
||||
* we'll repair and dispatch the older snapshot, potentially overwriting recent
|
||||
* changes. This is an accepted tradeoff to keep the API simple. The repair
|
||||
* operation will still be valid and the REPAIR op in the log reflects what
|
||||
* was applied.
|
||||
* was applied. The non-interactive path has no dialog wait, so this window
|
||||
* shrinks to the surrounding synchronous repair work.
|
||||
*
|
||||
* ## Repair Summary
|
||||
* The `dataRepair()` function returns a `RepairSummary` with accurate
|
||||
|
|
@ -276,7 +286,13 @@ export class ValidateStateService {
|
|||
*/
|
||||
async validateAndRepair(
|
||||
state: Record<string, unknown>,
|
||||
options?: { interactive?: boolean },
|
||||
): Promise<ValidateAndRepairResult> {
|
||||
// Fail-safe default: non-interactive (auto-repair, no blocking dialog). See
|
||||
// the method doc — a native dialog while sp_op_log is held during background
|
||||
// sync starves lock contenders (#9026); only foreground callers opt in.
|
||||
const interactive = options?.interactive ?? false;
|
||||
|
||||
// First, validate the state
|
||||
const validationResult = await this.validateState(state);
|
||||
|
||||
|
|
@ -287,9 +303,9 @@ export class ValidateStateService {
|
|||
};
|
||||
}
|
||||
|
||||
// State is invalid - ask user for confirmation before repair
|
||||
// (the rating-prompt suppression is recorded centrally in validateState).
|
||||
OpLog.log('[ValidateStateService] State invalid, asking user for confirmation...');
|
||||
// State is invalid (the rating-prompt suppression is recorded centrally in
|
||||
// validateState). Interactive callers confirm below; automatic callers repair.
|
||||
OpLog.log('[ValidateStateService] State invalid — repairing');
|
||||
|
||||
// Check if repair is possible
|
||||
if (!isDataRepairPossible(state as AppDataComplete)) {
|
||||
|
|
@ -302,20 +318,26 @@ export class ValidateStateService {
|
|||
};
|
||||
}
|
||||
|
||||
// Show confirmation dialog using translated message
|
||||
const confirmTitle = this.translateService.instant(
|
||||
T.F.SYNC.D_DATA_REPAIR_CONFIRM.TITLE,
|
||||
);
|
||||
const confirmMsg = this.translateService.instant(T.F.SYNC.D_DATA_REPAIR_CONFIRM.MSG);
|
||||
const userConfirmed = confirmDialog(`${confirmTitle}\n\n${confirmMsg}`);
|
||||
// Interactive callers only — see the method doc for why automatic/in-lock
|
||||
// repair must not block on a native dialog (#9026).
|
||||
if (interactive) {
|
||||
// Show confirmation dialog using translated message
|
||||
const confirmTitle = this.translateService.instant(
|
||||
T.F.SYNC.D_DATA_REPAIR_CONFIRM.TITLE,
|
||||
);
|
||||
const confirmMsg = this.translateService.instant(
|
||||
T.F.SYNC.D_DATA_REPAIR_CONFIRM.MSG,
|
||||
);
|
||||
const userConfirmed = confirmDialog(`${confirmTitle}\n\n${confirmMsg}`);
|
||||
|
||||
if (!userConfirmed) {
|
||||
OpLog.warn('[ValidateStateService] User declined repair');
|
||||
return {
|
||||
isValid: false,
|
||||
wasRepaired: false,
|
||||
error: 'User declined repair',
|
||||
};
|
||||
if (!userConfirmed) {
|
||||
OpLog.warn('[ValidateStateService] User declined repair');
|
||||
return {
|
||||
isValid: false,
|
||||
wasRepaired: false,
|
||||
error: 'User declined repair',
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
// User confirmed - proceed with repair
|
||||
|
|
@ -330,11 +352,14 @@ export class ValidateStateService {
|
|||
const revalidationResult = await this.validateState(repairedState);
|
||||
if (!revalidationResult.isValid) {
|
||||
OpLog.err('[ValidateStateService] State still invalid after repair');
|
||||
// Notify user that repair failed - they confirmed but it didn't work
|
||||
alertDialog(
|
||||
'Repair attempted but failed to fully fix data issues. ' +
|
||||
'Please try restoring from a backup or contact support.',
|
||||
);
|
||||
// Interactive callers only; automatic callers surface failure via the
|
||||
// session-validation latch + non-blocking error snack (#9026).
|
||||
if (interactive) {
|
||||
alertDialog(
|
||||
'Repair attempted but failed to fully fix data issues. ' +
|
||||
'Please try restoring from a backup or contact support.',
|
||||
);
|
||||
}
|
||||
return {
|
||||
isValid: false,
|
||||
wasRepaired: true,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue