mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-18 00:46:45 +00:00
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
This commit is contained in:
parent
447263afbd
commit
1950ed41b5
6 changed files with 247 additions and 5 deletions
|
|
@ -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)', () => {
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
};
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -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<SnackService>;
|
||||
|
||||
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);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
*
|
||||
|
|
|
|||
|
|
@ -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',
|
||||
|
|
|
|||
|
|
@ -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}}",
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue