From 953d680d342610574688b703a2b9fe8b3e9d8f1d Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Wed, 15 Jul 2026 17:05:16 +0200 Subject: [PATCH] fix(sync): don't block on repair dialogs while holding sp_op_log (#9026) (#9049) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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. --- .../persistence/sync-hydration.service.ts | 3 + .../sync/operation-log-sync.service.spec.ts | 3 + .../op-log/sync/operation-log-sync.service.ts | 9 +- .../op-log/sync/server-migration.service.ts | 4 +- .../repair-operation.service.spec.ts | 61 +++++- .../validation/repair-operation.service.ts | 52 +++-- ...nc-repair-non-blocking.integration.spec.ts | 183 ++++++++++++++++++ .../validation/validate-state.service.spec.ts | 93 ++++++++- .../validation/validate-state.service.ts | 93 +++++---- 9 files changed, 444 insertions(+), 57 deletions(-) create mode 100644 src/app/op-log/validation/sync-repair-non-blocking.integration.spec.ts diff --git a/src/app/op-log/persistence/sync-hydration.service.ts b/src/app/op-log/persistence/sync-hydration.service.ts index 752f5f0f61..975f03cc62 100644 --- a/src/app/op-log/persistence/sync-hydration.service.ts +++ b/src/app/op-log/persistence/sync-hydration.service.ts @@ -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) { diff --git a/src/app/op-log/sync/operation-log-sync.service.spec.ts b/src/app/op-log/sync/operation-log-sync.service.spec.ts index d61edf097f..ebec3049db 100644 --- a/src/app/op-log/sync/operation-log-sync.service.spec.ts +++ b/src/app/op-log/sync/operation-log-sync.service.spec.ts @@ -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 }, ); }); diff --git a/src/app/op-log/sync/operation-log-sync.service.ts b/src/app/op-log/sync/operation-log-sync.service.ts index 2f402d9569..736349bc67 100644 --- a/src/app/op-log/sync/operation-log-sync.service.ts +++ b/src/app/op-log/sync/operation-log-sync.service.ts @@ -1753,7 +1753,14 @@ export class OperationLogSyncService { stripLocalOnlySyncSettingsFromAppData(snapshotState), localOnlySyncSettings, ) as Record; - 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.', diff --git a/src/app/op-log/sync/server-migration.service.ts b/src/app/op-log/sync/server-migration.service.ts index 42d2f56e1c..a2f3dd1320 100644 --- a/src/app/op-log/sync/server-migration.service.ts +++ b/src/app/op-log/sync/server-migration.service.ts @@ -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); diff --git a/src/app/op-log/validation/repair-operation.service.spec.ts b/src/app/op-log/validation/repair-operation.service.spec.ts index 582bbdee1e..e162423b8e 100644 --- a/src/app/op-log/validation/repair-operation.service.spec.ts +++ b/src/app/op-log/validation/repair-operation.service.spec.ts @@ -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; let repairSyncContext: RepairSyncContextService; let mockStateSnapshotService: jasmine.SpyObj; + let mockSnackService: jasmine.SpyObj; 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 (_name: string, fn: () => Promise) => @@ -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( diff --git a/src/app/op-log/validation/repair-operation.service.ts b/src/app/op-log/validation/repair-operation.service.ts index ae0edaef99..133c346a0c 100644 --- a/src/app/op-log/validation/repair-operation.service.ts +++ b/src/app/op-log/validation/repair-operation.service.ts @@ -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 { 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(), diff --git a/src/app/op-log/validation/sync-repair-non-blocking.integration.spec.ts b/src/app/op-log/validation/sync-repair-non-blocking.integration.spec.ts new file mode 100644 index 0000000000..c10396776d --- /dev/null +++ b/src/app/op-log/validation/sync-repair-non-blocking.integration.spec.ts @@ -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; + let mockSnackService: jasmine.SpyObj; + let confirmSpy: jasmine.Spy; + let alertSpy: jasmine.Spy; + let originalProduction: boolean; + + const createEmptyState = (): Record => ({ + 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 (_name: string, fn: () => Promise): Promise => 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 }), + ); + }); +}); diff --git a/src/app/op-log/validation/validate-state.service.spec.ts b/src/app/op-log/validation/validate-state.service.spec.ts index 7df0357eb3..3039be80bd 100644 --- a/src/app/op-log/validation/validate-state.service.spec.ts +++ b/src/app/op-log/validation/validate-state.service.spec.ts @@ -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; + } + }); }); }); diff --git a/src/app/op-log/validation/validate-state.service.ts b/src/app/op-log/validation/validate-state.service.ts index 968f3d4f40..fea0bef30f 100644 --- a/src/app/op-log/validation/validate-state.service.ts +++ b/src/app/op-log/validation/validate-state.service.ts @@ -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, ); @@ -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, + options?: { interactive?: boolean }, ): Promise { + // 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,