From 1950ed41b5d95acdf3b69d5492d668603db7205e Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 12 Jun 2026 22:56:37 +0000 Subject: [PATCH] fix(sync): notify user when deferred actions are dropped and raise buffer cap Implements phase 1 of #8297: the deferred-action buffer overflow during a sync window previously evicted the oldest buffered action with only a devError (invisible in production), silently losing the operation behind an already-applied state change. - show a sticky ERROR snackbar (once per sync window) with a Reload action when a deferred action is dropped; snack is deferred out of the reducer call stack via setTimeout - log dropped action type and running count only (never payloads) - raise MAX_DEFERRED_ACTIONS_HARD_LIMIT from 100 to 1000; overflow now only occurs in stuck-window scenarios, which are reported loudly - resolve SnackService lazily via Injector so the APP_INITIALIZER-created service does not pull in the snack/translate stack at bootstrap Drop semantics (evict oldest) and flush behavior are unchanged; only observability is added. https://claude.ai/code/session_01QFEBa6TZLoZ6nQFvh5Vt99 --- .../operation-capture.meta-reducer.spec.ts | 95 +++++++++++++++++++ .../capture/operation-capture.meta-reducer.ts | 46 ++++++++- .../capture/operation-capture.service.spec.ts | 48 ++++++++++ .../capture/operation-capture.service.ts | 61 +++++++++++- src/app/t.const.ts | 1 + src/assets/i18n/en.json | 1 + 6 files changed, 247 insertions(+), 5 deletions(-) diff --git a/src/app/op-log/capture/operation-capture.meta-reducer.spec.ts b/src/app/op-log/capture/operation-capture.meta-reducer.spec.ts index 7789abd6e3..b4c77cecb3 100644 --- a/src/app/op-log/capture/operation-capture.meta-reducer.spec.ts +++ b/src/app/op-log/capture/operation-capture.meta-reducer.spec.ts @@ -7,6 +7,7 @@ import { bufferDeferredAction, getDeferredActions, clearDeferredActions, + MAX_DEFERRED_ACTIONS_HARD_LIMIT, } from './operation-capture.meta-reducer'; import { OperationCaptureService } from './operation-capture.service'; import { Action } from '@ngrx/store'; @@ -56,6 +57,7 @@ describe('operationCaptureMetaReducer', () => { 'dequeue', 'getQueueSize', 'clear', + 'notifyDeferredActionsDropped', ]); mockReducer = jasmine.createSpy('reducer').and.returnValue(mockModifiedState); @@ -269,6 +271,99 @@ describe('operationCaptureMetaReducer', () => { expect(getDeferredActions()).toEqual([]); }); }); + + describe('buffer overflow', () => { + beforeEach(() => { + // devError fires native dialogs in non-production builds; stub them so + // filling the buffer past the warning threshold stays silent and never + // throws (the global test.ts stub returns true for confirm, which would + // make devError throw). alert/confirm may already be spies from test.ts. + if (!jasmine.isSpy(window.alert)) { + spyOn(window, 'alert'); + } + if (jasmine.isSpy(window.confirm)) { + (window.confirm as jasmine.Spy).and.returnValue(false); + } else { + spyOn(window, 'confirm').and.returnValue(false); + } + }); + + const fillBufferToLimit = (): PersistentAction[] => { + const actions: PersistentAction[] = []; + for (let i = 0; i < MAX_DEFERRED_ACTIONS_HARD_LIMIT; i++) { + const action = createMockAction({ type: `[TaskShared] Update Task ${i}` }); + actions.push(action); + bufferDeferredAction(action); + } + return actions; + }; + + it('should drop the oldest action and keep the newest when over the limit', () => { + const initial = fillBufferToLimit(); + const overflowAction = createMockAction({ type: '[TaskShared] Overflow' }); + + bufferDeferredAction(overflowAction); + + const buffered = getDeferredActions(); + expect(buffered.length).toBe(MAX_DEFERRED_ACTIONS_HARD_LIMIT); + expect(buffered[0]).toBe(initial[1]); + expect(buffered[buffered.length - 1]).toBe(overflowAction); + }); + + it('should NOT notify below the limit', () => { + fillBufferToLimit(); + + expect(mockCaptureService.notifyDeferredActionsDropped).not.toHaveBeenCalled(); + }); + + it('should notify exactly once per window with dropped type and count', () => { + const initial = fillBufferToLimit(); + + bufferDeferredAction(createMockAction({ type: '[TaskShared] Overflow 1' })); + bufferDeferredAction(createMockAction({ type: '[TaskShared] Overflow 2' })); + bufferDeferredAction(createMockAction({ type: '[TaskShared] Overflow 3' })); + + expect(mockCaptureService.notifyDeferredActionsDropped).toHaveBeenCalledTimes(1); + expect(mockCaptureService.notifyDeferredActionsDropped).toHaveBeenCalledWith( + initial[0].type, + 1, + ); + }); + + it('should notify again after the buffer is consumed (new window)', () => { + fillBufferToLimit(); + bufferDeferredAction(createMockAction({ type: '[TaskShared] Overflow 1' })); + expect(mockCaptureService.notifyDeferredActionsDropped).toHaveBeenCalledTimes(1); + + // Consuming the buffer ends the window and resets the notification flag + getDeferredActions(); + + fillBufferToLimit(); + bufferDeferredAction(createMockAction({ type: '[TaskShared] Overflow 2' })); + expect(mockCaptureService.notifyDeferredActionsDropped).toHaveBeenCalledTimes(2); + }); + + it('should reset the notification flag on clearDeferredActions', () => { + fillBufferToLimit(); + bufferDeferredAction(createMockAction({ type: '[TaskShared] Overflow 1' })); + expect(mockCaptureService.notifyDeferredActionsDropped).toHaveBeenCalledTimes(1); + + clearDeferredActions(); + + fillBufferToLimit(); + bufferDeferredAction(createMockAction({ type: '[TaskShared] Overflow 2' })); + expect(mockCaptureService.notifyDeferredActionsDropped).toHaveBeenCalledTimes(2); + }); + + it('should not throw when no capture service is set', () => { + setOperationCaptureService(null as any); + fillBufferToLimit(); + + expect(() => + bufferDeferredAction(createMockAction({ type: '[TaskShared] Overflow' })), + ).not.toThrow(); + }); + }); }); describe('sync buffering (user interaction during sync)', () => { diff --git a/src/app/op-log/capture/operation-capture.meta-reducer.ts b/src/app/op-log/capture/operation-capture.meta-reducer.ts index f2618bc4ca..81bc82ca0e 100644 --- a/src/app/op-log/capture/operation-capture.meta-reducer.ts +++ b/src/app/op-log/capture/operation-capture.meta-reducer.ts @@ -141,8 +141,26 @@ const MAX_DEFERRED_ACTIONS_WARNING = 10; /** * Hard limit for deferred actions buffer. * If reached, oldest actions are dropped to prevent unbounded memory growth. + * + * Buffered actions are small plain objects, so the memory cost of a large cap is + * negligible. A healthy apply window is sub-second and per-second tracking ticks + * are not persistent actions, so reaching this limit in practice means the apply + * window is stuck — an error condition that is reported loudly (see + * notifyDeferredActionsDropped), not a normal-load scenario to size for. */ -const MAX_DEFERRED_ACTIONS_HARD_LIMIT = 100; +export const MAX_DEFERRED_ACTIONS_HARD_LIMIT = 1000; + +/** + * Number of actions dropped from the buffer during the current sync window. + * Reset when the buffer is consumed (getDeferredActions) or cleared. + */ +let droppedActionsInWindowCount = 0; + +/** + * Ensures the user-facing drop notification fires at most once per sync window. + * Reset when the buffer is consumed (getDeferredActions) or cleared. + */ +let hasNotifiedDropInWindow = false; /** * Buffers an action for processing after sync completes. @@ -152,14 +170,30 @@ export const bufferDeferredAction = (action: PersistentAction): void => { // Hard limit: drop oldest action if buffer is full (sync stuck scenario). // NOTE: The shifted action remains in deferredActionSet (WeakSet has no delete-by-value). // The effect filters it via isDeferredAction(), and getDeferredActions() won't return it, - // so it is silently lost. This is acceptable: the hard limit is itself an error condition - // (sync stuck), and dropping the oldest action is the lesser evil vs unbounded growth. + // so it is permanently lost. The inner reducer has already applied it to local state, + // so a drop means on-screen state with no operation behind it — report it loudly. if (deferredActions.length >= MAX_DEFERRED_ACTIONS_HARD_LIMIT) { + const dropped = deferredActions.shift(); + droppedActionsInWindowCount++; + // Rule 9: log action type and count only, never payloads/user content. + OpLog.err( + `operationCaptureMetaReducer: Deferred actions buffer exceeded ${MAX_DEFERRED_ACTIONS_HARD_LIMIT} items - dropping oldest action`, + { + droppedActionType: dropped?.type, + droppedCountInWindow: droppedActionsInWindowCount, + }, + ); devError( `[operationCaptureMetaReducer] Deferred actions buffer exceeded ${MAX_DEFERRED_ACTIONS_HARD_LIMIT} items. ` + `Dropping oldest action. Sync may be stuck - consider reloading the app.`, ); - deferredActions.shift(); + if (!hasNotifiedDropInWindow) { + hasNotifiedDropInWindow = true; + operationCaptureService?.notifyDeferredActionsDropped( + dropped?.type ?? 'unknown', + droppedActionsInWindowCount, + ); + } } deferredActions.push(action); @@ -180,6 +214,8 @@ export const bufferDeferredAction = (action: PersistentAction): void => { export const getDeferredActions = (): PersistentAction[] => { const actions = deferredActions; deferredActions = []; + droppedActionsInWindowCount = 0; + hasNotifiedDropInWindow = false; return actions; }; @@ -194,6 +230,8 @@ export const getDeferredActions = (): PersistentAction[] => { */ export const clearDeferredActions = (): void => { deferredActions = []; + droppedActionsInWindowCount = 0; + hasNotifiedDropInWindow = false; }; /** diff --git a/src/app/op-log/capture/operation-capture.service.spec.ts b/src/app/op-log/capture/operation-capture.service.spec.ts index b3777f9030..4a38c80241 100644 --- a/src/app/op-log/capture/operation-capture.service.spec.ts +++ b/src/app/op-log/capture/operation-capture.service.spec.ts @@ -1,6 +1,9 @@ +import { TestBed } from '@angular/core/testing'; import { OperationCaptureService } from './operation-capture.service'; import { OpType, EntityType } from '../core/operation.types'; import { PersistentAction } from '../core/persistent-action.interface'; +import { SnackService } from '../../core/snack/snack.service'; +import { T } from '../../t.const'; describe('OperationCaptureService', () => { let service: OperationCaptureService; @@ -266,4 +269,49 @@ describe('OperationCaptureService', () => { expect(service.getQueueSize()).toBe(1); // Still in queue }); }); + + describe('notifyDeferredActionsDropped', () => { + let mockSnackService: jasmine.SpyObj; + + beforeEach(() => { + jasmine.clock().install(); + mockSnackService = jasmine.createSpyObj('SnackService', ['open']); + TestBed.configureTestingModule({ + providers: [{ provide: SnackService, useValue: mockSnackService }], + }); + }); + + afterEach(() => { + jasmine.clock().uninstall(); + }); + + it('should open a sticky ERROR snack, deferred out of the reducer call stack', () => { + // Created via DI so the service picks up the injection context + const serviceWithInjector = TestBed.inject(OperationCaptureService); + + serviceWithInjector.notifyDeferredActionsDropped('[TaskShared] Update Task', 1); + + // Must NOT open synchronously - the call originates inside a reducer pass + expect(mockSnackService.open).not.toHaveBeenCalled(); + + jasmine.clock().tick(0); + + expect(mockSnackService.open).toHaveBeenCalledTimes(1); + const cfg = mockSnackService.open.calls.mostRecent().args[0] as { + type: string; + msg: string; + config: { duration: number }; + }; + expect(cfg.type).toBe('ERROR'); + expect(cfg.msg).toBe(T.F.SYNC.S.DEFERRED_ACTIONS_DROPPED); + expect(cfg.config.duration).toBe(0); + }); + + it('should not throw when constructed without an injector', () => { + expect(() => + service.notifyDeferredActionsDropped('[TaskShared] Update Task', 1), + ).not.toThrow(); + jasmine.clock().tick(0); + }); + }); }); diff --git a/src/app/op-log/capture/operation-capture.service.ts b/src/app/op-log/capture/operation-capture.service.ts index 5898ff0e2c..e92183dedf 100644 --- a/src/app/op-log/capture/operation-capture.service.ts +++ b/src/app/op-log/capture/operation-capture.service.ts @@ -1,7 +1,9 @@ -import { Injectable } from '@angular/core'; +import { inject, Injectable, Injector } from '@angular/core'; import { EntityChange, OpType } from '../core/operation.types'; import { PersistentAction } from '../core/persistent-action.interface'; import { OpLog } from '../../core/log'; +import { SnackService } from '../../core/snack/snack.service'; +import { T } from '../../t.const'; /** * Captures action payloads and queues them for persistence using a simple FIFO queue. @@ -27,6 +29,21 @@ import { OpLog } from '../../core/log'; providedIn: 'root', }) export class OperationCaptureService { + /** + * Used to resolve SnackService lazily — this service is instantiated in an + * APP_INITIALIZER, before the snack/translate stack should be pulled in. + * Falls back to null when constructed outside an injection context (several + * specs use `new OperationCaptureService()`); notification then degrades to + * log-only. + */ + private readonly _injector: Injector | null = (() => { + try { + return inject(Injector); + } catch { + return null; + } + })(); + /** * Warning threshold for queue size. * If effects fail to dequeue, we log a warning. @@ -125,6 +142,48 @@ export class OperationCaptureService { this.hasWarnedAboutQueueSize = false; } + /** + * Notifies the user that deferred actions were dropped from the buffer. + * Called (at most once per sync window) by the operation-capture meta-reducer + * when the deferred-action buffer overflows: the dropped change is on screen + * but will never be written as an operation, so it silently diverges across + * devices and may be lost on the next hydration. The user's only recovery is + * a reload, hence the sticky error snack. + * + * Logs action type and count only — never payloads/user content (rule 9). + */ + notifyDeferredActionsDropped( + droppedActionType: string, + droppedCountInWindow: number, + ): void { + OpLog.err( + 'OperationCaptureService: Deferred action(s) dropped during sync window - data loss', + { droppedActionType, droppedCountInWindow }, + ); + + const injector = this._injector; + if (!injector) { + return; + } + + // Defer out of the reducer call stack: this method is invoked synchronously + // from within a reducer pass, and opening a Material snackbar (change + // detection, possible follow-up dispatches) inside a reducer is unsafe. + setTimeout(() => { + injector.get(SnackService).open({ + type: 'ERROR', + msg: T.F.SYNC.S.DEFERRED_ACTIONS_DROPPED, + actionStr: T.PS.RELOAD, + actionFn: (): void => { + window.location.reload(); + }, + config: { + duration: 0, // Sticky - data loss requires user action + }, + }); + }); + } + /** * Extracts entity changes from action payload. * diff --git a/src/app/t.const.ts b/src/app/t.const.ts index f461a8630d..3b2e2e4022 100644 --- a/src/app/t.const.ts +++ b/src/app/t.const.ts @@ -1567,6 +1567,7 @@ const T = { DATA_REPAIRED: 'F.SYNC.S.DATA_REPAIRED', DECRYPTION_FAILED: 'F.SYNC.S.DECRYPTION_FAILED', DEFERRED_ACTION_FAILED: 'F.SYNC.S.DEFERRED_ACTION_FAILED', + DEFERRED_ACTIONS_DROPPED: 'F.SYNC.S.DEFERRED_ACTIONS_DROPPED', DIALOG_RESULT_ERROR: 'F.SYNC.S.DIALOG_RESULT_ERROR', DISABLE_ENCRYPTION_FAILED: 'F.SYNC.S.DISABLE_ENCRYPTION_FAILED', ENABLE_ENCRYPTION_FAILED: 'F.SYNC.S.ENABLE_ENCRYPTION_FAILED', diff --git a/src/assets/i18n/en.json b/src/assets/i18n/en.json index 24c9112321..bb8a2f8f41 100644 --- a/src/assets/i18n/en.json +++ b/src/assets/i18n/en.json @@ -1522,6 +1522,7 @@ "DATA_REPAIRED": "Sync cleanup: {{count}} reference(s) resolved", "DECRYPTION_FAILED": "Failed to decrypt synced data. Please check your encryption password.", "DEFERRED_ACTION_FAILED": "Some changes made during sync could not be saved. Please reload to recover.", + "DEFERRED_ACTIONS_DROPPED": "Sync is taking unusually long and some recent changes could not be recorded. Please reload the app to avoid losing data.", "DIALOG_RESULT_ERROR": "An error occurred while processing the sync dialog. Please try again.", "DISABLE_ENCRYPTION_FAILED": "Failed to disable encryption: {{message}}", "ENABLE_ENCRYPTION_FAILED": "Failed to enable encryption: {{message}}",