fix(sync): suppress effects during the resume-trigger gap

A long-suspended Android client could produce dozens of LWW conflicts
on TAG:TODAY when it resumed mid-day. Trace from the incident log:

  +0ms    androidInterface.onResume$ emits (I_RESUME_APP)
  +106ms  visibilitychange → todayDateStr$ → DAY_CHANGE
  +112ms  setTodayString action dispatched
  +117ms  repairTodayTagConsistency$ fires with stale local taskIds
  +124ms  ...creating a [Tag] Update Tag op in the local op log
  +241ms  SyncWrapperService.sync() actually runs and downloads
          ~600 remote ops, 22 of them TAG:TODAY updates from other
          clients during the day → 22-way LWW conflict

The repair effect is gated on `HydrationStateService.isInSyncWindow`,
but on resume that window opened too late: the trigger pipeline has
`debounceTime(100)` plus exhaustMap latency, so the synchronous
visibility-change → repair cascade finishes before sync() runs.

Add a third phase (`_isSyncWindowOpen`) to `isInSyncWindow`. Open it
via `openSyncWindow()` from three places that all fire ahead of the
cascade:

  1. The tap right after `_immediateSyncTrigger$` in
     `SyncTriggerService.getSyncTrigger$()`, before debounceTime.
  2. Direct subscriptions to `androidInterface.onResume$` and
     `ipcResume$` in the SyncTriggerService constructor — covers
     the cold-start case where (1)'s switchMap chain is not yet
     subscribed.
  3. `SyncWrapperService.sync()` itself, before any async work.

`closeSyncWindow()` runs in `SyncWrapperService.sync()`'s finally
block. The 2s failsafe inside `openSyncWindow()` handles triggers
that get debounced/throttled out before reaching `sync()`. When
`sync()` opens the window itself, it passes failsafeMs=0 to opt out
of the timer — its own `finally` 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.

Also adds a verbose log in `skipDuringSyncWindow` so silent drops
of legitimate emissions during the wider window are observable in
the field.

Tests: 29 hydration-state, 100 sync-wrapper, 14 tag.effects (incl. an
integration test that drives the real HydrationStateService through
the gate end-to-end), 15 sync-trigger, 11 skip-during-sync-window —
all green. Verified against multi-agent code review (6 Claude
reviewers + Codex CLI).
This commit is contained in:
Johannes Millan 2026-04-29 21:15:26 +02:00
parent 68db1ab229
commit a27be2ac51
6 changed files with 341 additions and 12 deletions

View file

@ -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<typeof updateTag> | null = null;
sub.unsubscribe();
const sub2 = realEffects.repairTodayTagConsistency$.subscribe((action) => {
emittedAfterClose = action as ReturnType<typeof updateTag>;
});
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);
});
});
});

View file

@ -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

View file

@ -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) {

View file

@ -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

View file

@ -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<typeof setTimeout> | null = null;
private _syncWindowFailsafeTimer: ReturnType<typeof setTimeout> | 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);
}
}

View file

@ -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 = <T>(): MonoTypeOperatorFunction<T> => {
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;
});
};