diff --git a/src/app/features/tag/store/tag.effects.spec.ts b/src/app/features/tag/store/tag.effects.spec.ts index 43791e4d5e..19d2849344 100644 --- a/src/app/features/tag/store/tag.effects.spec.ts +++ b/src/app/features/tag/store/tag.effects.spec.ts @@ -390,4 +390,138 @@ describe('TagEffects', () => { }); }); }); + + /** + * Integration test for the resume-time conflict regression. + * + * Uses the REAL `HydrationStateService` instead of the spy, so the gate + * is exercised through the actual signal-based state machine. Proves the + * end-to-end chain: `openSyncWindow()` → `isInSyncWindow()` returns true + * → `skipDuringSyncWindow()` filters → `repairTodayTagConsistency$` is + * suppressed. + */ + describe('resume-time conflict regression (real HydrationStateService)', () => { + let realHydration: HydrationStateService; + let realEffects: TagEffects; + let realStore: MockStore; + + beforeEach(() => { + // Tear down the spy-based TestBed and rebuild with the real service. + TestBed.resetTestingModule(); + + const syncTriggerSpy = jasmine.createSpyObj('SyncTriggerService', [ + 'isInitialSyncDoneSync', + ]); + syncTriggerSpy.isInitialSyncDoneSync.and.returnValue(true); + + const snackServiceSpy = jasmine.createSpyObj('SnackService', ['open']); + const tagServiceSpy = jasmine.createSpyObj('TagService', ['updateTag']); + const workContextSpy = jasmine.createSpyObj('WorkContextService', [], { + activeWorkContextId: 'test-id', + activeWorkContextTypeAndId$: of({ + activeType: WorkContextType.TAG, + activeId: 'test-id', + }), + mainListTasks$: of([]), + }); + const routerSpy = jasmine.createSpyObj('Router', ['navigate']); + const translateSpy = jasmine.createSpyObj('TranslateService', ['instant']); + translateSpy.instant.and.returnValue('Today'); + const plannerSpy = jasmine.createSpyObj('PlannerService', ['getSnackExtraStr']); + plannerSpy.getSnackExtraStr.and.returnValue(Promise.resolve('')); + + TestBed.configureTestingModule({ + providers: [ + TagEffects, + provideMockActions(() => EMPTY), + provideMockStore({ + selectors: [ + { selector: selectTodayTagRepair, value: null }, + { selector: selectTodayTaskIds, value: [] }, + { selector: selectAllTasksDueToday, value: [] }, + ], + }), + // Real HydrationStateService (providedIn: 'root', no spy) + HydrationStateService, + { provide: SyncTriggerService, useValue: syncTriggerSpy }, + { provide: SnackService, useValue: snackServiceSpy }, + { provide: TagService, useValue: tagServiceSpy }, + { provide: WorkContextService, useValue: workContextSpy }, + { provide: Router, useValue: routerSpy }, + { provide: TranslateService, useValue: translateSpy }, + { provide: PlannerService, useValue: plannerSpy }, + { provide: LOCAL_ACTIONS, useValue: EMPTY }, + ], + }); + + realEffects = TestBed.inject(TagEffects); + realStore = TestBed.inject(MockStore); + realHydration = TestBed.inject(HydrationStateService); + + // Clean any stale state from a previous test + realHydration.endApplyingRemoteOps(); + realHydration.clearPostSyncCooldown(); + realHydration.closeSyncWindow(); + }); + + afterEach(() => { + realHydration.closeSyncWindow(); + realStore.resetSelectors(); + }); + + it('suppresses repair while window is open, dispatches once closed', (done) => { + // Simulate the resume race: openSyncWindow() runs first (synchronously, + // from the SyncTriggerService constructor subscription), then the + // selectTodayTagRepair selector emits a needsRepair=true value. + realHydration.openSyncWindow(); + expect(realHydration.isInSyncWindow()).toBeTrue(); + + realStore.overrideSelector(selectTodayTagRepair, { + needsRepair: true, + repairedTaskIds: ['stale-task-1', 'stale-task-2'], + }); + realStore.refreshState(); + + let emittedDuringWindow = false; + const sub = realEffects.repairTodayTagConsistency$.subscribe(() => { + emittedDuringWindow = true; + }); + + // After 50ms with the window open, the effect must NOT have dispatched. + setTimeout(() => { + expect(emittedDuringWindow).toBeFalse(); + + // Now close the window (simulating the post-sync flow) and emit a + // fresh repair value — the effect should dispatch this time. + realHydration.closeSyncWindow(); + expect(realHydration.isInSyncWindow()).toBeFalse(); + + let emittedAfterClose: ReturnType | null = null; + sub.unsubscribe(); + const sub2 = realEffects.repairTodayTagConsistency$.subscribe((action) => { + emittedAfterClose = action as ReturnType; + }); + + realStore.overrideSelector(selectTodayTagRepair, { + needsRepair: true, + repairedTaskIds: ['fresh-task-1'], + }); + realStore.refreshState(); + + setTimeout(() => { + expect(emittedAfterClose).toEqual( + updateTag({ + tag: { + id: TODAY_TAG.id, + changes: { taskIds: ['fresh-task-1' as never] }, + }, + isSkipSnack: true, + }), + ); + sub2.unsubscribe(); + done(); + }, 50); + }, 50); + }); + }); }); diff --git a/src/app/imex/sync/sync-trigger.service.ts b/src/app/imex/sync/sync-trigger.service.ts index 90e25b6ebf..ca81b40abb 100644 --- a/src/app/imex/sync/sync-trigger.service.ts +++ b/src/app/imex/sync/sync-trigger.service.ts @@ -33,6 +33,7 @@ import { ipcResume$, ipcSuspend$ } from '../../core/ipc-events'; import { IS_TOUCH_PRIMARY } from '../../util/is-mouse-primary'; import { DataInitStateService } from '../../core/data-init/data-init-state.service'; import { SyncLog } from '../../core/log'; +import { HydrationStateService } from '../../op-log/apply/hydration-state.service'; import { SyncWrapperService } from './sync-wrapper.service'; import { SyncProviderId } from '../../op-log/sync-exports'; @@ -48,6 +49,7 @@ export class SyncTriggerService { private readonly _dataInitStateService = inject(DataInitStateService); private readonly _idleService = inject(IdleService); private readonly _syncWrapperService = inject(SyncWrapperService); + private readonly _hydrationState = inject(HydrationStateService); constructor() { // When sync is disabled, set initialSyncDone immediately so UI shows @@ -57,6 +59,18 @@ export class SyncTriggerService { this.setInitialSyncDone(true); } }); + + // Open the sync window directly on every resume source, bypassing the + // gated `getSyncTrigger$()` chain (which is only subscribed once + // dataInitState + config are ready). This handles the cold-start edge + // case where a resume arrives before the trigger pipeline is wired. + // The failsafe in `openSyncWindow()` cleans up if no sync follows. + if (IS_ANDROID_WEB_VIEW) { + androidInterface.onResume$.subscribe(() => this._hydrationState.openSyncWindow()); + } + if (IS_ELECTRON) { + ipcResume$.subscribe(() => this._hydrationState.openSyncWindow()); + } } // Note: This was previously connected to PFAPI's onLocalMetaUpdate$, which was a no-op. @@ -254,7 +268,17 @@ export class SyncTriggerService { ); return merge( // once immediately - _immediateSyncTrigger$.pipe(tap((v) => SyncLog.log('immediate sync trigger', v))), + _immediateSyncTrigger$.pipe( + tap((v) => { + SyncLog.log('immediate sync trigger', v); + // Open BEFORE the downstream debounceTime(100) so the resume → DAY_CHANGE + // → TODAY_TAG repair cascade (which fires inside that 100ms) sees the + // window already open and is suppressed by skipDuringSyncWindow(). + // Failsafe in openSyncWindow() handles triggers that get debounced/ + // throttled out before reaching SyncWrapperService.sync(). + this._hydrationState.openSyncWindow(); + }), + ), // and once we reset the sync interval for all other triggers // we do this to reset the audit time to avoid sync checks in short succession diff --git a/src/app/imex/sync/sync-wrapper.service.ts b/src/app/imex/sync/sync-wrapper.service.ts index ad65d2497b..bc11d3217b 100644 --- a/src/app/imex/sync/sync-wrapper.service.ts +++ b/src/app/imex/sync/sync-wrapper.service.ts @@ -75,6 +75,7 @@ import { OperationLogStoreService } from '../../op-log/persistence/operation-log import { OperationLogSyncService } from '../../op-log/sync/operation-log-sync.service'; import { WrappedProviderService } from '../../op-log/sync-providers/wrapped-provider.service'; import { SuperSyncProvider } from '../../op-log/sync-providers/super-sync/super-sync'; +import { HydrationStateService } from '../../op-log/apply/hydration-state.service'; /** * Identifies which error or UI path triggered a destructive forceUpload. @@ -109,6 +110,7 @@ export class SyncWrapperService { private _opLogStore = inject(OperationLogStoreService); private _opLogSyncService = inject(OperationLogSyncService); private _wrappedProvider = inject(WrappedProviderService); + private _hydrationState = inject(HydrationStateService); syncState$ = this._providerManager.syncStatus$; @@ -263,10 +265,16 @@ export class SyncWrapperService { return 'HANDLED_ERROR'; } this._isSyncInProgress$.next(true); + // Open before any async work — see `HydrationStateService.isInSyncWindow`. + // Pass 0 to disable the failsafe: the `finally` block below is the + // authoritative close, and a slow sync (provider I/O > 2s) would + // otherwise expire the timer mid-sync and leave a stale-state gap. + this._hydrationState.openSyncWindow(0); // Set SYNCING status so ImmediateUploadService knows not to interfere this._providerManager.setSyncStatus('SYNCING'); const result = await this._sync().finally(() => { this._isSyncInProgress$.next(false); + this._hydrationState.closeSyncWindow(); // Safeguard: if _sync() threw or completed without setting a final status, // reset from SYNCING to UNKNOWN_OR_CHANGED to avoid getting stuck in SYNCING state if (this._providerManager.isSyncInProgress) { diff --git a/src/app/op-log/apply/hydration-state.service.spec.ts b/src/app/op-log/apply/hydration-state.service.spec.ts index dbe164c81d..f855e7abc5 100644 --- a/src/app/op-log/apply/hydration-state.service.spec.ts +++ b/src/app/op-log/apply/hydration-state.service.spec.ts @@ -14,6 +14,7 @@ describe('HydrationStateService', () => { // Ensure clean state - meta-reducer flag may be dirty from previous tests service.endApplyingRemoteOps(); service.clearPostSyncCooldown(); + service.closeSyncWindow(); }); describe('initial state', () => { @@ -197,6 +198,104 @@ describe('HydrationStateService', () => { }); }); + describe('explicit sync window', () => { + // Regression coverage for the I_RESUME_APP gap (see `isInSyncWindow` JSDoc). + it('should set isInSyncWindow to true when openSyncWindow is called', () => { + expect(service.isInSyncWindow()).toBeFalse(); + + service.openSyncWindow(); + + expect(service.isInSyncWindow()).toBeTrue(); + }); + + it('should clear isInSyncWindow when closeSyncWindow is called', () => { + service.openSyncWindow(); + expect(service.isInSyncWindow()).toBeTrue(); + + service.closeSyncWindow(); + + expect(service.isInSyncWindow()).toBeFalse(); + }); + + it('should stay open across openSyncWindow → applying → close', () => { + service.openSyncWindow(); + expect(service.isInSyncWindow()).toBeTrue(); + + service.startApplyingRemoteOps(); + expect(service.isInSyncWindow()).toBeTrue(); + + service.endApplyingRemoteOps(); + // closeSyncWindow hasn't been called yet — still open + expect(service.isInSyncWindow()).toBeTrue(); + + service.closeSyncWindow(); + expect(service.isInSyncWindow()).toBeFalse(); + }); + + it('closeSyncWindow is idempotent when no window is open', () => { + expect(service.isInSyncWindow()).toBeFalse(); + expect(() => service.closeSyncWindow()).not.toThrow(); + expect(service.isInSyncWindow()).toBeFalse(); + }); + + it('failsafe auto-closes the window if closeSyncWindow is never called', (done) => { + service.openSyncWindow(50); // 50ms failsafe for fast test + expect(service.isInSyncWindow()).toBeTrue(); + + setTimeout(() => { + expect(service.isInSyncWindow()).toBeFalse(); + done(); + }, 100); + }); + + it('closeSyncWindow cancels a pending failsafe', (done) => { + service.openSyncWindow(50); + service.closeSyncWindow(); + expect(service.isInSyncWindow()).toBeFalse(); + + // Wait past failsafe; should remain closed + setTimeout(() => { + expect(service.isInSyncWindow()).toBeFalse(); + done(); + }, 100); + }); + + it('failsafeMs=0 skips the failsafe entirely', (done) => { + // Simulates `SyncWrapperService.sync()` opening the window without a + // wall-clock timer because its `finally` is authoritative. The window + // must stay open until explicitly closed, regardless of duration. + service.openSyncWindow(0); + expect(service.isInSyncWindow()).toBeTrue(); + + // After what would have been the default failsafe interval, the window + // must still be open. + setTimeout(() => { + expect(service.isInSyncWindow()).toBeTrue(); + service.closeSyncWindow(); + expect(service.isInSyncWindow()).toBeFalse(); + done(); + }, 50); + }); + + it('reopening restarts the failsafe', (done) => { + service.openSyncWindow(50); + // Re-open with a longer failsafe before the first one fires + setTimeout(() => service.openSyncWindow(200), 25); + + // At 80ms the original 50ms timer would have fired but the new 200ms + // timer (started at 25ms, fires at 225ms) still holds the window open + setTimeout(() => { + expect(service.isInSyncWindow()).toBeTrue(); + }, 80); + + // At 250ms the second timer should have fired + setTimeout(() => { + expect(service.isInSyncWindow()).toBeFalse(); + done(); + }, 280); + }); + }); + describe('edge cases', () => { it('should handle concurrent start calls without issues', () => { // Multiple starts should just keep it true diff --git a/src/app/op-log/apply/hydration-state.service.ts b/src/app/op-log/apply/hydration-state.service.ts index 26c708a2de..1a0b4f162d 100644 --- a/src/app/op-log/apply/hydration-state.service.ts +++ b/src/app/op-log/apply/hydration-state.service.ts @@ -3,6 +3,15 @@ import { toObservable } from '@angular/core/rxjs-interop'; import { setIsApplyingRemoteOps } from '../capture/operation-capture.meta-reducer'; import { POST_SYNC_COOLDOWN_MS } from '../core/operation-log.const'; +/** + * Failsafe for the explicit sync window. Triggers can be debounced, throttled, + * or dropped (e.g. exhaustMap busy) before reaching `SyncWrapperService.sync()`, + * meaning `closeSyncWindow()` may never run. The timer guarantees the window + * cannot stay open indefinitely. 2s is well past the typical handoff to + * `_isApplyingRemoteOps` while keeping the silent-drop window short. + */ +const SYNC_WINDOW_FAILSAFE_MS = 2000; + /** * Tracks whether the application is currently applying remote operations * (hydration replay or sync). This allows selector-based effects to skip @@ -54,20 +63,28 @@ import { POST_SYNC_COOLDOWN_MS } from '../core/operation-log.const'; export class HydrationStateService { private _isApplyingRemoteOps = signal(false); private _isInPostSyncCooldown = signal(false); + private _isSyncWindowOpen = signal(false); private _cooldownTimer: ReturnType | null = null; + private _syncWindowFailsafeTimer: ReturnType | null = null; /** - * Computed signal: true when in the extended sync window where selector-based - * effects that modify shared state (like TODAY_TAG) should be suppressed. + * True when selector-based effects that modify shared state (e.g. TODAY_TAG + * repair) should be suppressed. Three phases contribute: * - * This includes: - * - During remote op application (isApplyingRemoteOps) - * - During post-sync cooldown period + * 1. **Open**: `openSyncWindow()` is called by `SyncTriggerService` the + * moment a sync is triggered (e.g. `I_RESUME_APP`), *before* the trigger + * pipeline's `debounceTime(100)`. Closes the race on app resume where + * the visibility-change → DAY_CHANGE → TODAY_TAG-repair cascade fires + * inside that debounce window and emits ops on stale local state. + * 2. **Applying**: remote ops are being replayed into the store. + * 3. **Cooldown**: short post-sync window for state to settle. * - * Use `skipDuringSyncWindow()` operator for effects that should be - * suppressed during this extended window. + * Use `skipDuringSyncWindow()` (drop) or `waitForSyncWindow()` (defer). */ isInSyncWindow = computed( - () => this._isApplyingRemoteOps() || this._isInPostSyncCooldown(), + () => + this._isSyncWindowOpen() || + this._isApplyingRemoteOps() || + this._isInPostSyncCooldown(), ); /** @@ -144,4 +161,39 @@ export class HydrationStateService { } this._isInPostSyncCooldown.set(false); } + + /** + * Opens the sync window. Restartable: each call resets the failsafe timer. + * + * Failsafe ensures the window auto-closes if no `closeSyncWindow()` follows + * (e.g. trigger debounced, throttled, or dropped before reaching `sync()`). + * + * Pass `failsafeMs: 0` to skip the timer entirely. Use this when the caller + * has its own deterministic close path (`SyncWrapperService.sync()`'s + * `finally` block) and the timer would otherwise close the window + * prematurely during a slow sync (longer than the default 2s). + */ + openSyncWindow(failsafeMs: number = SYNC_WINDOW_FAILSAFE_MS): void { + this._isSyncWindowOpen.set(true); + + if (this._syncWindowFailsafeTimer) { + clearTimeout(this._syncWindowFailsafeTimer); + this._syncWindowFailsafeTimer = null; + } + if (failsafeMs > 0) { + this._syncWindowFailsafeTimer = setTimeout(() => { + this._isSyncWindowOpen.set(false); + this._syncWindowFailsafeTimer = null; + }, failsafeMs); + } + } + + /** Closes the sync window and clears the failsafe timer. */ + closeSyncWindow(): void { + if (this._syncWindowFailsafeTimer) { + clearTimeout(this._syncWindowFailsafeTimer); + this._syncWindowFailsafeTimer = null; + } + this._isSyncWindowOpen.set(false); + } } diff --git a/src/app/util/skip-during-sync-window.operator.ts b/src/app/util/skip-during-sync-window.operator.ts index b8898641dc..20f9df26ca 100644 --- a/src/app/util/skip-during-sync-window.operator.ts +++ b/src/app/util/skip-during-sync-window.operator.ts @@ -3,6 +3,7 @@ import { MonoTypeOperatorFunction } from 'rxjs'; import { filter } from 'rxjs/operators'; import { HydrationStateService } from '../op-log/apply/hydration-state.service'; import { SyncTriggerService } from '../imex/sync/sync-trigger.service'; +import { SyncLog } from '../core/log'; /** * RxJS operator that skips emissions during the full sync window, @@ -64,7 +65,18 @@ import { SyncTriggerService } from '../imex/sync/sync-trigger.service'; export const skipDuringSyncWindow = (): MonoTypeOperatorFunction => { const hydrationState = inject(HydrationStateService); const syncTrigger = inject(SyncTriggerService); - return filter( - () => syncTrigger.isInitialSyncDoneSync() && !hydrationState.isInSyncWindow(), - ); + return filter(() => { + const initialSyncDone = syncTrigger.isInitialSyncDoneSync(); + const inSyncWindow = hydrationState.isInSyncWindow(); + const allow = initialSyncDone && !inSyncWindow; + if (!allow) { + // Verbose-level so it doesn't spam normal logs. Visible when investigating + // whether the wider sync window is silently dropping legitimate emissions. + SyncLog.verbose('skipDuringSyncWindow drop', { + initialSyncDone, + inSyncWindow, + }); + } + return allow; + }); };