diff --git a/android/app/src/main/java/com/superproductivity/superproductivity/webview/JavaScriptInterface.kt b/android/app/src/main/java/com/superproductivity/superproductivity/webview/JavaScriptInterface.kt index 7ad51be23a..fb58be5d14 100644 --- a/android/app/src/main/java/com/superproductivity/superproductivity/webview/JavaScriptInterface.kt +++ b/android/app/src/main/java/com/superproductivity/superproductivity/webview/JavaScriptInterface.kt @@ -93,29 +93,36 @@ class JavaScriptInterface( @JavascriptInterface fun saveToDb(requestId: String, key: String, value: String) { (activity.application as App).keyValStore.set(key, value) - callJavaScriptFunction(FN_PREFIX + "saveToDbCallback('" + requestId + "')") + callJavaScriptFunction(FN_PREFIX + "saveToDbCallback(" + JSONObject.quote(requestId) + ")") } + // #7925: quote every arg so a stored value with ' / \ / newlines / + // can't break out of the JS literal. (JSON.stringify does not escape + // apostrophes — pre-fix, a backup blob with one silently corrupted the load.) @Suppress("unused") @JavascriptInterface fun loadFromDb(requestId: String, key: String) { val r = (activity.application as App).keyValStore.get(key, "") - // NOTE: ' are important as otherwise the json messes up - callJavaScriptFunction(FN_PREFIX + "loadFromDbCallback('" + requestId + "', '" + key + "', '" + r + "')") + callJavaScriptFunction( + FN_PREFIX + "loadFromDbCallback(" + + JSONObject.quote(requestId) + ", " + + JSONObject.quote(key) + ", " + + JSONObject.quote(r) + ")" + ) } @Suppress("unused") @JavascriptInterface fun removeFromDb(requestId: String, key: String) { (activity.application as App).keyValStore.set(key, null) - callJavaScriptFunction(FN_PREFIX + "removeFromDbCallback('" + requestId + "')") + callJavaScriptFunction(FN_PREFIX + "removeFromDbCallback(" + JSONObject.quote(requestId) + ")") } @Suppress("unused") @JavascriptInterface fun clearDb(requestId: String) { (activity.application as App).keyValStore.clearAll(activity) - callJavaScriptFunction(FN_PREFIX + "clearDbCallback('" + requestId + "')") + callJavaScriptFunction(FN_PREFIX + "clearDbCallback(" + JSONObject.quote(requestId) + ")") } @Suppress("unused") diff --git a/docs/sync-and-op-log/sqlite-migration-followup.md b/docs/sync-and-op-log/sqlite-migration-followup.md index ce47c6b86d..c95d53be67 100644 --- a/docs/sync-and-op-log/sqlite-migration-followup.md +++ b/docs/sync-and-op-log/sqlite-migration-followup.md @@ -15,9 +15,17 @@ ordered so each item is independently shippable and reviewable. - ✅ `SqliteOpLogAdapter` fully implemented against a minimal `SqliteDb` port, unit-tested against an in-memory SQLite stand-in. **Not wired to any platform; no native plugin dependency.** +- ✅ App-private local backup shipped (#7924): `LocalBackupService` writes a + JSON snapshot every 5 min on Android (`KeyValStore` rows `backup` / + `backup_prev`) and iOS (`Directory.Data` `super-productivity-backup.json` / + `.prev.json`), with an empty-state write guard and a two-generation ring so + one bad/evicted write cycle can't erase the only good copy. Fresh-launch + restore prompt is informed (`summarizeBackupStr` shows task / project + counts). Electron continues to use its own rotated backup folder. -Nothing below changes runtime behavior for existing users until step B3 flips a -platform to the SQLite backend. +Nothing in the SQLite tracks below changes runtime behavior for existing users +until step B3 flips a platform to the SQLite backend. The #7924 local-backup +work is already live on Android/iOS. --- @@ -32,28 +40,38 @@ SQLite work. Highest user value per unit effort; do these first. native and logs nothing when `persist()` resolves `false`. On Android WebView the grant is often not honored, so today a report like #7892 carries no signal. -- **Do:** `Log.log({ persisted, granted })` on every branch (incl. native), and - surface the result in the exported logs / "About" diagnostics. +- **Do:** `Log.log({ persisted, granted })` on every branch (incl. native), so + exported logs always carry the durability state of the WebView store. + Optionally surface in About diagnostics as a follow-up. - **Size:** ~1 file, a few lines. **Risk:** none (logging only). -- **Payoff:** the next #7892-style report is conclusive instead of a mystery. +- **Payoff:** the next #7892-style report is conclusive instead of a mystery, + and the telemetry decides whether the next protective steps (e.g. the + near-empty write guard below) are worth the added complexity. -### A2. Periodic local auto-backup outside the WebView store (native) +### A2 (shipped). Debounced on-data-change backup trigger -A second copy of the op-log/state in app-private storage that OS WebView -eviction cannot touch. +✅ Shipped in #7925: `LocalBackupService._triggerBackupSave$` merges a +`LOCAL_ACTIONS`-driven trigger with the existing 5-min interval — any local +action settles into a backup after a 30s quiet period. `LOCAL_ACTIONS` +already filters out remote/hydration replays, and the existing empty-state +guard in `_backup()` prevents writing a degraded post-eviction snapshot +over a good backup, so the trigger strictly adds frequency without spam. -- **Do:** on native, periodically (and on a debounced "data changed") write a - JSON snapshot via `@capacitor/filesystem` to `Directory.Data`; keep the last - N. Restore offered on a wholly-fresh launch when a backup exists. Hook into - the existing `_initBackups()` / `loadStateCache()` flow that already has the - native restore-prompt scaffolding. -- **Size:** medium, isolated feature. **Risk:** low (additive; never deletes the - live store). -- **Payoff:** survives the exact overnight-eviction scenario in #7892 even - before SQLite lands. +### A3 (shipped). Near-empty write-time overwrite guard -> A1 + A2 are the recommended near-term fix for #7892. SQLite (Track B) is the -> durable architectural fix but is weeks of on-device work behind these. +✅ Shipped in #7925: `LocalBackupService._backupAndroid()` and `_backupIOS()` +each read the existing primary slot before promoting/overwriting, and bail +when a near-empty snapshot (< 3 tasks) would clobber a substantial existing +backup (≥ 10 tasks). Counts include active + young-archived + old-archived +tasks via the shared `countAllTasks` helper, so the threshold is the same +on the read side (`summarizeBackupStr`) and the write side. Electron is +unchanged — its rotated, timestamped backup chain isn't a single-slot +overwrite. Fail-safe: skipping never loses data; the guard self-clears +once the store grows back past 3 tasks, so a legitimate bulk-delete is +captured on the next tick. + +> A1, A2, and A3 have shipped — Track A is complete. SQLite (Track B) is +> the durable architectural fix and is tracked in #7931. --- @@ -130,9 +148,47 @@ and the `adoptConnection` bridge once SQLite is the sole native backend. ## Suggested order -1. **A1** (trivial, unblocks diagnosis) → **A2** (the real near-term #7892 fix). -2. **B1 → B2 → B3** (gets SQLite runnable + validated behind a flag). -3. **C1 → C2** (migrate real users' data, staged). -4. **D** (tidy up once SQLite is the native default). +1. ✅ Track A complete — **A1** (storage-persistence diagnostics) → **A2** + (debounced data-change trigger) → **A3** (near-empty write-time overwrite + guard) all shipped. +2. **B1 → B2 → B3** (gets SQLite runnable + validated behind a flag) — + tracked in #7931. +3. **C1 → C2** (migrate real users' data, staged) — tracked in #7931. +4. **D** (tidy up once SQLite is the native default) — tracked in #7931. -Tracks A and B/C/D are independent — A can ship while B is still in progress. +Tracks A and B/C/D are independent — A shipped while B/C/D moves at its own +device-gated cadence. + +## Cross-cutting / hardening + +These don't belong to a single track but were surfaced by the #7924 review and +should land alongside the next time the area is touched. + +- **`JavaScriptInterface.kt` JS-literal injection** (Android bridge). The + `loadFromDbCallback(...)` call is built by raw single-quote interpolation of + the stored value into `evaluateJavascript`. Beyond the security smell, it is + a real functional bug: `JSON.stringify` does not escape `'`, so a backup + blob containing an apostrophe terminates the JS string literal and + load-from-DB returns garbage. Fix is to use `JSONObject.quote()` for the + arguments (the same primitive already used by + `emitForegroundServiceStartFailed`). +- **Backup-date in the restore prompt** (strengthens the informed-restore UX + from #7924). iOS has `Filesystem.stat.mtime` for free; Android needs a + bridge change to surface the (now-real) `KEY_CREATED_AT` — + `loadFromDbWrapped(key)` returns only the value, so add a meta-aware reader + (e.g. `loadFromDbWithMeta` → `{ value, createdAt }`). This gives + `KEY_CREATED_AT` its first reader; the column is behaviorally inert today. +- **Robust restore on empty/degraded boot** (was #7901 item 4). Today + `_initBackups()` only offers restore when there is no `stateCache` at all. + Extend the trigger to also fire when the loaded state is degraded per + `hasMeaningfulStateData`. Needs a decision on auto-restore vs prompt and a + guard against resurrecting an intentional wipe (the informed-restore prompt + shipped in #7924 already lets the user decline knowingly). +- **"Last backup" visibility on mobile** (was #7901 item 5). Surface the + most recent successful backup time in About / a settings panel so no-sync + users can see they are protected. Pairs naturally with the backup-date + bridge change. +- **No-sync onboarding nudge** (was #7901 item 6). On a no-sync mobile + install, surface that local-only data is at risk and recommend enabling + sync. Default-on local backup (since #7924) already protects them; this is + the awareness piece. diff --git a/src/app/core/startup/startup.service.ts b/src/app/core/startup/startup.service.ts index 33e654aab2..e986820bfa 100644 --- a/src/app/core/startup/startup.service.ts +++ b/src/app/core/startup/startup.service.ts @@ -353,46 +353,48 @@ export class StartupService { } private _requestPersistence(): void { - if (navigator.storage) { - // try to avoid data-loss - Promise.all([navigator.storage.persisted()]) - .then(([persisted]) => { - if (!persisted) { - return navigator.storage.persist().then((granted) => { - if (granted) { - Log.log('Persistent store granted'); - } - // NOTE: we never show this warning for native mobile apps or Electron, - // because persistence is managed by the OS and not subject to browser eviction. - // Also suppress during active onboarding to avoid confusing first-time users. - else if ( - !this._platformService.isNative && - !IS_ELECTRON && - !OnboardingHintService.isOnboardingInProgress() - ) { - const msg = T.GLOBAL_SNACK.PERSISTENCE_DISALLOWED; - Log.warn('Persistence not allowed'); - this._snackService.open({ msg }); - } - }); - } else { - Log.log('Persistence already allowed'); - return; - } - }) - .catch((e) => { - Log.log(e); - const err = e && e.toString ? e.toString() : 'UNKNOWN'; - const msg = T.GLOBAL_SNACK.PERSISTENCE_ERROR; - this._snackService.open({ - type: 'ERROR', - msg, - translateParams: { - err, - }, - }); - }); + // A1 (#7925): always log the outcome so a #7892-style report carries the + // durability state of the WebView store. Snack gating below is unchanged. + const isNative = this._platformService.isNative; + if (!navigator.storage) { + Log.log('Persistence: navigator.storage unavailable', { isNative, IS_ELECTRON }); + return; } + navigator.storage + .persisted() + .then((persisted) => { + if (persisted) { + Log.log('Persistence: already granted', { isNative, IS_ELECTRON }); + return; + } + return navigator.storage.persist().then((granted) => { + Log.log('Persistence: persist() resolved', { + granted, + isNative, + IS_ELECTRON, + }); + // Native + Electron persistence is OS-managed (not subject to browser + // eviction); also suppress during onboarding. + if ( + !granted && + !isNative && + !IS_ELECTRON && + !OnboardingHintService.isOnboardingInProgress() + ) { + Log.warn('Persistence not allowed'); + this._snackService.open({ msg: T.GLOBAL_SNACK.PERSISTENCE_DISALLOWED }); + } + }); + }) + .catch((e) => { + Log.log('Persistence: error', { isNative, IS_ELECTRON, error: e }); + const err = e && e.toString ? e.toString() : 'UNKNOWN'; + this._snackService.open({ + type: 'ERROR', + msg: T.GLOBAL_SNACK.PERSISTENCE_ERROR, + translateParams: { err }, + }); + }); } private _checkAvailableStorage(): void { diff --git a/src/app/imex/local-backup/backup-ring.util.spec.ts b/src/app/imex/local-backup/backup-ring.util.spec.ts index ebecebeec7..153dff6742 100644 --- a/src/app/imex/local-backup/backup-ring.util.spec.ts +++ b/src/app/imex/local-backup/backup-ring.util.spec.ts @@ -1,4 +1,6 @@ import { + countAllTasks, + countAllTasksInBackupStr, isUsableBackupStr, selectBestBackupStr, summarizeBackupStr, @@ -96,4 +98,53 @@ describe('backup-ring.util', () => { expect(selectBestBackupStr('', '')).toBeNull(); }); }); + + describe('countAllTasks (A3 / #7925)', () => { + it('returns 0 for null / undefined / non-object input', () => { + expect(countAllTasks(null)).toBe(0); + expect(countAllTasks(undefined)).toBe(0); + expect(countAllTasks(42)).toBe(0); + expect(countAllTasks('str')).toBe(0); + }); + + it('counts active + young-archived + old-archived tasks', () => { + expect( + countAllTasks({ + task: { ids: ['t1', 't2'] }, + archiveYoung: { task: { ids: ['a1', 'a2'] } }, + archiveOld: { task: { ids: ['a3'] } }, + }), + ).toBe(5); + }); + + it('treats missing slots as zero rather than throwing', () => { + expect(countAllTasks({})).toBe(0); + expect(countAllTasks({ task: { ids: ['t1'] } })).toBe(1); + }); + + it('matches summarizeBackupStr.taskCount on a representative blob', () => { + // Single-source-of-truth invariant: both call sites must agree on + // "how many tasks?" so the A3 write-time threshold lines up with + // what summarizeBackupStr shows in the restore prompt. + expect(summarizeBackupStr(WITH_ARCHIVE_AND_INBOX)?.taskCount).toBe(5); + expect(countAllTasksInBackupStr(WITH_ARCHIVE_AND_INBOX)).toBe(5); + }); + }); + + describe('countAllTasksInBackupStr (A3 / #7925)', () => { + it('returns null for empty / corrupt blobs (= no existing backup to compare against)', () => { + expect(countAllTasksInBackupStr(null)).toBeNull(); + expect(countAllTasksInBackupStr(undefined)).toBeNull(); + expect(countAllTasksInBackupStr('')).toBeNull(); + expect(countAllTasksInBackupStr('{broken')).toBeNull(); + }); + + it('returns 0 for a parseable but task-less blob', () => { + expect(countAllTasksInBackupStr(EMPTY_STATE)).toBe(0); + }); + + it('returns the active+archive sum for a normal backup', () => { + expect(countAllTasksInBackupStr(MEANINGFUL)).toBe(1); + }); + }); }); diff --git a/src/app/imex/local-backup/backup-ring.util.ts b/src/app/imex/local-backup/backup-ring.util.ts index 229ccc1d5e..f575478541 100644 --- a/src/app/imex/local-backup/backup-ring.util.ts +++ b/src/app/imex/local-backup/backup-ring.util.ts @@ -9,6 +9,47 @@ const entityCount = (val: unknown): number => { const archiveTaskCount = (archive: unknown): number => entityCount((archive as { task?: unknown })?.task); +/** + * Counts active + young-archived + old-archived tasks on a backup-shaped + * object. Single source of truth for the "how many tasks?" question used + * by both the informed restore prompt (`summarizeBackupStr`) and the A3 + * near-empty write-time overwrite guard (#7925), so "near-empty" means the + * same thing on the read side and the write side. + */ +export const countAllTasks = (data: unknown): number => { + if (!data || typeof data !== 'object') { + return 0; + } + const d = data as { + task?: unknown; + archiveYoung?: unknown; + archiveOld?: unknown; + }; + return ( + entityCount(d.task) + + archiveTaskCount(d.archiveYoung) + + archiveTaskCount(d.archiveOld) + ); +}; + +/** + * Parse-and-count for the A3 guard: returns null when the stored blob is + * empty/corrupt (treat as "no existing backup", so we don't skip the write + * and lose the chance to capture a real first backup). + */ +export const countAllTasksInBackupStr = ( + str: string | null | undefined, +): number | null => { + if (!str) { + return null; + } + try { + return countAllTasks(JSON.parse(str)); + } catch { + return null; + } +}; + /** A human-meaningful summary of a backup blob, used for the restore prompt. */ export interface BackupSummary { taskCount: number; @@ -37,10 +78,7 @@ export const summarizeBackupStr = ( const s = JSON.parse(str) as Record; const projectIds = (s.project as { ids?: unknown })?.ids; return { - taskCount: - entityCount(s.task) + - archiveTaskCount(s.archiveYoung) + - archiveTaskCount(s.archiveOld), + taskCount: countAllTasks(s), projectCount: Array.isArray(projectIds) ? projectIds.filter((id) => id !== INBOX_PROJECT.id).length : 0, diff --git a/src/app/imex/local-backup/local-backup.service.spec.ts b/src/app/imex/local-backup/local-backup.service.spec.ts index 7a077ffabb..badfda3476 100644 --- a/src/app/imex/local-backup/local-backup.service.spec.ts +++ b/src/app/imex/local-backup/local-backup.service.spec.ts @@ -1,5 +1,5 @@ import { fakeAsync, TestBed, tick } from '@angular/core/testing'; -import { BehaviorSubject, of } from 'rxjs'; +import { BehaviorSubject, of, Subject } from 'rxjs'; import { LocalBackupService } from './local-backup.service'; import { GlobalConfigService } from '../../features/config/global-config.service'; import { StateSnapshotService } from '../../op-log/backup/state-snapshot.service'; @@ -11,8 +11,11 @@ import { initialTimeTrackingState } from '../../features/time-tracking/store/tim import { CapacitorPlatformService } from '../../core/platform/capacitor-platform.service'; import { AppDataComplete } from '../../op-log/model/model-config'; import { T } from '../../t.const'; +import { LOCAL_ACTIONS } from '../../util/local-actions.token'; +import { Action } from '@ngrx/store'; const BACKUP_INTERVAL = 5 * 60 * 1000; +const DATA_CHANGE_DEBOUNCE = 30 * 1000; type LocalBackupServiceWithPrivate = { _backup: () => Promise; @@ -118,6 +121,8 @@ describe('LocalBackupService', () => { { provide: SnackService, useValue: snackServiceSpy }, { provide: TranslateService, useValue: translateServiceSpy }, { provide: CapacitorPlatformService, useValue: platformServiceSpy }, + // Default is an inert stream; the data-change trigger specs override. + { provide: LOCAL_ACTIONS, useValue: new Subject() }, ], }); @@ -297,6 +302,7 @@ describe('LocalBackupService', () => { { provide: SnackService, useValue: snackServiceSpy }, { provide: TranslateService, useValue: translateServiceSpy }, { provide: CapacitorPlatformService, useValue: platformServiceSpy }, + { provide: LOCAL_ACTIONS, useValue: new Subject() }, ], }); service = TestBed.inject(LocalBackupService); @@ -321,6 +327,209 @@ describe('LocalBackupService', () => { })); }); + describe('near-empty overwrite guard (A3, #7925)', () => { + type LocalBackupServiceWithA3 = { + _isNearEmptyOverwrite: ( + newData: AppDataComplete, + existingRaw: string | null, + ) => boolean; + }; + + type LocalBackupServiceWithIosRing = { + _readIOSFileOrNull: (path: string) => Promise; + _writeIOSFile: (path: string, data: string) => Promise; + }; + + const makeData = (taskCount: number): AppDataComplete => + ({ + task: { + ids: Array.from({ length: taskCount }, (_, i) => `t${i}`), + entities: {}, + }, + archiveYoung: { task: { ids: [], entities: {} } }, + archiveOld: { task: { ids: [], entities: {} } }, + }) as unknown as AppDataComplete; + + const makeStoredBackup = (activeTasks: number, archivedTasks: number = 0): string => + JSON.stringify({ + task: { + ids: Array.from({ length: activeTasks }, (_, i) => `t${i}`), + entities: {}, + }, + archiveYoung: { + task: { + ids: Array.from({ length: archivedTasks }, (_, i) => `a${i}`), + entities: {}, + }, + }, + archiveOld: { task: { ids: [], entities: {} } }, + }); + + describe('_isNearEmptyOverwrite (pure)', () => { + it('blocks when new < 3 tasks AND existing >= 10 tasks', () => { + const guard = (service as unknown as LocalBackupServiceWithA3) + ._isNearEmptyOverwrite; + expect(guard.call(service, makeData(0), makeStoredBackup(10))).toBe(true); + expect(guard.call(service, makeData(2), makeStoredBackup(10))).toBe(true); + expect(guard.call(service, makeData(2), makeStoredBackup(50))).toBe(true); + }); + + it('counts archived tasks toward the "substantial" threshold', () => { + // 4 active + 8 archived = 12 → still substantial. + const guard = (service as unknown as LocalBackupServiceWithA3) + ._isNearEmptyOverwrite; + expect(guard.call(service, makeData(1), makeStoredBackup(4, 8))).toBe(true); + }); + + it('allows when the new snapshot is not near-empty', () => { + const guard = (service as unknown as LocalBackupServiceWithA3) + ._isNearEmptyOverwrite; + expect(guard.call(service, makeData(3), makeStoredBackup(100))).toBe(false); + expect(guard.call(service, makeData(10), makeStoredBackup(10))).toBe(false); + }); + + it('allows when the existing backup is not substantial', () => { + // A legitimate fresh-start scenario: existing has only a few tasks too. + const guard = (service as unknown as LocalBackupServiceWithA3) + ._isNearEmptyOverwrite; + expect(guard.call(service, makeData(1), makeStoredBackup(9))).toBe(false); + expect(guard.call(service, makeData(0), makeStoredBackup(0))).toBe(false); + }); + + it('allows when there is no existing backup or it is corrupt', () => { + // First-ever write must not be blocked, and a corrupt slot must not + // pretend to be a substantial backup. + const guard = (service as unknown as LocalBackupServiceWithA3) + ._isNearEmptyOverwrite; + expect(guard.call(service, makeData(0), null)).toBe(false); + expect(guard.call(service, makeData(0), '')).toBe(false); + expect(guard.call(service, makeData(0), '{broken')).toBe(false); + }); + }); + + describe('integration via _backupIOS', () => { + beforeEach(() => { + ( + service as unknown as { + _platformService: Pick; + } + )._platformService = { isIOS: () => true }; + }); + + it('skips the overwrite when a near-empty snapshot would clobber a substantial backup', async () => { + stateSnapshotServiceSpy.getAllSyncModelDataFromStoreAsync.and.resolveTo( + makeData(1) as any, + ); + spyOn( + service as unknown as LocalBackupServiceWithIosRing, + '_readIOSFileOrNull', + ).and.resolveTo(makeStoredBackup(20)); + const writeSpy = spyOn( + service as unknown as LocalBackupServiceWithIosRing, + '_writeIOSFile', + ).and.resolveTo(); + + await (service as unknown as LocalBackupServiceWithPrivate)._backup(); + + // Guard fired before any write — neither the prev promotion nor the + // primary overwrite happened, so the good backup is preserved. + expect(writeSpy).not.toHaveBeenCalled(); + }); + + it('writes normally when the new snapshot is not near-empty', async () => { + stateSnapshotServiceSpy.getAllSyncModelDataFromStoreAsync.and.resolveTo( + makeData(5) as any, + ); + spyOn( + service as unknown as LocalBackupServiceWithIosRing, + '_readIOSFileOrNull', + ).and.resolveTo(makeStoredBackup(20)); + const writeSpy = spyOn( + service as unknown as LocalBackupServiceWithIosRing, + '_writeIOSFile', + ).and.resolveTo(); + + await (service as unknown as LocalBackupServiceWithPrivate)._backup(); + + // Prev-promotion + primary write = 2 calls. + expect(writeSpy).toHaveBeenCalledTimes(2); + }); + }); + }); + + describe('debounced data-change trigger (A2, #7925)', () => { + const setup = ( + isEnabled: boolean, + ): { actions$: Subject; backupSpy: jasmine.Spy } => { + const actions$ = new Subject(); + const cfg$ = new BehaviorSubject({ localBackup: { isEnabled } }); + globalConfigServiceSpy = jasmine.createSpyObj('GlobalConfigService', [], { + cfg$, + }); + + TestBed.resetTestingModule(); + TestBed.configureTestingModule({ + providers: [ + LocalBackupService, + { provide: GlobalConfigService, useValue: globalConfigServiceSpy }, + { provide: StateSnapshotService, useValue: stateSnapshotServiceSpy }, + { provide: BackupService, useValue: backupServiceSpy }, + { provide: SnackService, useValue: snackServiceSpy }, + { provide: TranslateService, useValue: translateServiceSpy }, + { provide: CapacitorPlatformService, useValue: platformServiceSpy }, + { provide: LOCAL_ACTIONS, useValue: actions$ }, + ], + }); + service = TestBed.inject(LocalBackupService); + const backupSpy = spyOn( + service as unknown as LocalBackupServiceWithPrivate, + '_backup', + ).and.resolveTo(); + service.init(); + return { actions$, backupSpy }; + }; + + it('fires one backup after the debounce settles, regardless of action count', fakeAsync(() => { + const { actions$, backupSpy } = setup(true); + + actions$.next({ type: 'SomeAction' }); + actions$.next({ type: 'AnotherAction' }); + actions$.next({ type: 'YetAnother' }); + + // Inside the debounce window — no backup yet. + tick(DATA_CHANGE_DEBOUNCE - 1); + expect(backupSpy).not.toHaveBeenCalled(); + + // Window settles — exactly one backup. + tick(1); + expect(backupSpy).toHaveBeenCalledTimes(1); + })); + + it('resets the debounce on each new action', fakeAsync(() => { + const { actions$, backupSpy } = setup(true); + + actions$.next({ type: 'A' }); + tick(DATA_CHANGE_DEBOUNCE - 1); + // New action just before the window closes — debounce resets. + actions$.next({ type: 'B' }); + tick(DATA_CHANGE_DEBOUNCE - 1); + expect(backupSpy).not.toHaveBeenCalled(); + + tick(1); + expect(backupSpy).toHaveBeenCalledTimes(1); + })); + + it('does not fire when isEnabled is false', fakeAsync(() => { + const { actions$, backupSpy } = setup(false); + + actions$.next({ type: 'SomeAction' }); + tick(DATA_CHANGE_DEBOUNCE); + tick(BACKUP_INTERVAL); + + expect(backupSpy).not.toHaveBeenCalled(); + })); + }); + describe('informed mobile restore prompt (#7901)', () => { type LocalBackupServiceWithPrompt = { _restoreMobilePromptMsg: (backupData: string) => string; diff --git a/src/app/imex/local-backup/local-backup.service.ts b/src/app/imex/local-backup/local-backup.service.ts index 3c0e1866d1..d7dfc3a7ec 100644 --- a/src/app/imex/local-backup/local-backup.service.ts +++ b/src/app/imex/local-backup/local-backup.service.ts @@ -1,9 +1,10 @@ import { DestroyRef, inject, Injectable } from '@angular/core'; import { takeUntilDestroyed } from '@angular/core/rxjs-interop'; import { GlobalConfigService } from '../../features/config/global-config.service'; -import { EMPTY, interval, Observable } from 'rxjs'; +import { EMPTY, interval, merge, Observable } from 'rxjs'; import { LocalBackupConfig } from '../../features/config/global-config.model'; -import { map, switchMap, tap } from 'rxjs/operators'; +import { debounceTime, map, switchMap, tap } from 'rxjs/operators'; +import { LOCAL_ACTIONS } from '../../util/local-actions.token'; import { LocalBackupMeta } from './local-backup.model'; import { IS_ANDROID_WEB_VIEW } from '../../util/is-android-web-view'; import { IS_ELECTRON } from '../../app.constants'; @@ -14,7 +15,12 @@ import { T } from '../../t.const'; import { TranslateService } from '@ngx-translate/core'; import { AppDataComplete } from '../../op-log/model/model-config'; import { hasMeaningfulStateData } from '../../op-log/validation/has-meaningful-state-data.util'; -import { selectBestBackupStr, summarizeBackupStr } from './backup-ring.util'; +import { + countAllTasks, + countAllTasksInBackupStr, + selectBestBackupStr, + summarizeBackupStr, +} from './backup-ring.util'; import { SnackService } from '../../core/snack/snack.service'; import { Log } from '../../core/log'; import { confirmDialog } from '../../util/native-dialogs'; @@ -22,15 +28,19 @@ import { CapacitorPlatformService } from '../../core/platform/capacitor-platform import { Directory, Encoding, Filesystem } from '@capacitor/filesystem'; const DEFAULT_BACKUP_INTERVAL = 5 * 60 * 1000; +// A2 (#7925): high enough that a flurry of UI actions settles into one backup; +// low enough that a real change is captured before the user backgrounds the app. +const DATA_CHANGE_BACKUP_DEBOUNCE = 30 * 1000; const ANDROID_DB_KEY = 'backup'; -// Previous-generation slot for the two-generation ring (#7901): the current -// `backup` is promoted here before being overwritten, so one bad/corrupt write -// cycle can never erase the only good copy. +// Previous-generation slot for the two-generation ring (#7901). const ANDROID_DB_KEY_PREV = 'backup_prev'; const IOS_BACKUP_FILENAME = 'super-productivity-backup.json'; const IOS_BACKUP_PREV_FILENAME = 'super-productivity-backup.prev.json'; -// const DEFAULT_BACKUP_INTERVAL = 6 * 1000; +// A3 (#7925) near-empty write-time overwrite guard thresholds. Starting point +// — revisit once A1 telemetry shows what a real post-eviction boot looks like. +const NEAR_EMPTY_NEW_TASKS = 3; +const SUBSTANTIAL_EXISTING_TASKS = 10; @Injectable({ providedIn: 'root', @@ -43,12 +53,24 @@ export class LocalBackupService { private _snackService = inject(SnackService); private _translateService = inject(TranslateService); private _platformService = inject(CapacitorPlatformService); + private _localActions$ = inject(LOCAL_ACTIONS); private _cfg$: Observable = this._configService.cfg$.pipe( map((cfg) => cfg.localBackup), ); + // A2 (#7925): the empty-state guard in `_backup()` plus A3's near-empty + // guard keep degraded data out — `bulkApplyOperations` / `loadAllData` + // do transit LOCAL_ACTIONS (they aren't tagged `meta.isRemote`), and + // that's fine because the downstream guards handle it. private _triggerBackupSave$: Observable = this._cfg$.pipe( - switchMap((cfg) => (cfg.isEnabled ? interval(DEFAULT_BACKUP_INTERVAL) : EMPTY)), + switchMap((cfg) => + cfg.isEnabled + ? merge( + interval(DEFAULT_BACKUP_INTERVAL), + this._localActions$.pipe(debounceTime(DATA_CHANGE_BACKUP_DEBOUNCE)), + ) + : EMPTY, + ), tap(() => this._backup()), ); @@ -81,20 +103,14 @@ export class LocalBackupService { } async loadBackupAndroid(): Promise { - // Restore from the newest usable ring slot (#7901). The Android bridge can - // hand back literal newlines, so we escape them here (the single escape site) - // and judge usability on that parse-ready form; the returned string is ready - // for JSON.parse. Returns '' when nothing usable exists (degrades to the - // existing import-error snack rather than throwing on the startup path). - const [primaryRaw, prevRaw] = await Promise.all([ + // Restore from the newest usable ring slot (#7901). Returns '' when nothing + // usable exists (degrades to the existing import-error snack rather than + // throwing on the startup path). + const [primary, prev] = await Promise.all([ androidInterface.loadFromDbWrapped(ANDROID_DB_KEY), androidInterface.loadFromDbWrapped(ANDROID_DB_KEY_PREV), ]); - const best = selectBestBackupStr( - this._escapeAndroidNewlines(primaryRaw), - this._escapeAndroidNewlines(prevRaw), - ); - return best ?? ''; + return selectBestBackupStr(primary, prev) ?? ''; } async loadBackupIOS(): Promise { @@ -181,52 +197,73 @@ export class LocalBackupService { const data = (await this._stateSnapshotService.getAllSyncModelDataFromStoreAsync()) as AppDataComplete; - // GUARD (#7901/#7892): never overwrite a good on-device backup with an - // empty/degraded store. The local backups live in durable, non-evictable - // storage (Android SQLite KeyValStore, iOS file, Electron file), but after a - // WebView IndexedDB eviction the live NgRx store can boot empty — and the - // 5-min timer would then clobber the last good backup with nothing. Skipping - // the write is always safe: the previous backup stays intact (this mirrors - // the snapshot/compaction empty-overwrite guard). Trade-off: a deliberate - // full wipe is not captured in the local backup until real data exists again. + // #7901/#7892: don't overwrite a good backup with an empty store (a + // post-eviction WebView can boot blank). A3 (#7925) extends this to + // "near-empty over substantial" inside each platform writer. if (!hasMeaningfulStateData(data)) { - Log.warn( - 'LocalBackupService: Skipping backup — current state has no meaningful ' + - 'data (refusing to overwrite backup with empty state)', - ); + Log.warn('LocalBackupService: skipping backup — empty state'); return; } if (IS_ELECTRON) { - // Electron keeps its own rotated, timestamped backups (electron/backup.ts), - // so it needs no ring here. + // Electron has its own rotated, timestamped chain — no ring or A3 guard + // needed (the bug class A3 protects against doesn't apply). window.ea.backupAppData(data); } if (IS_ANDROID_WEB_VIEW) { - await this._backupAndroid(JSON.stringify(data)); + await this._backupAndroid(data); } if (this._platformService.isIOS()) { await this._backupIOS(data); } } - /** - * Android two-generation ring (#7901): promote the current backup to the prev - * slot before overwriting it, so a single bad write can't erase the only copy. - */ - private async _backupAndroid(dataStr: string): Promise { + // A3 (#7925) pure predicate — kept on its own so the threshold logic is + // testable independent of the platform I/O paths. + private _isNearEmptyOverwrite( + newData: AppDataComplete, + existingRaw: string | null, + ): boolean { + if (countAllTasks(newData) >= NEAR_EMPTY_NEW_TASKS) { + return false; + } + const existingTaskCount = countAllTasksInBackupStr(existingRaw); + if (existingTaskCount === null) { + return false; + } + return existingTaskCount >= SUBSTANTIAL_EXISTING_TASKS; + } + + // Single source of the A3 skip log; returns true when the caller should bail. + private _guardNearEmptyOverwrite( + data: AppDataComplete, + existing: string | null, + platform: 'Android' | 'iOS', + ): boolean { + if (!this._isNearEmptyOverwrite(data, existing)) { + return false; + } + Log.warn( + `LocalBackupService: skipping ${platform} backup — near-empty ` + + `(${countAllTasks(data)} tasks) over substantial backup ` + + `(${countAllTasksInBackupStr(existing)} tasks). #7925 A3.`, + ); + return true; + } + + private async _backupAndroid(data: AppDataComplete): Promise { const existing = await androidInterface.loadFromDbWrapped(ANDROID_DB_KEY); + if (this._guardNearEmptyOverwrite(data, existing, 'Android')) return; if (existing) { await androidInterface.saveToDbWrapped(ANDROID_DB_KEY_PREV, existing); } - await androidInterface.saveToDbWrapped(ANDROID_DB_KEY, dataStr); + await androidInterface.saveToDbWrapped(ANDROID_DB_KEY, JSON.stringify(data)); } private async _backupIOS(data: AppDataComplete): Promise { try { - // Two-generation ring (#7901): promote the current backup file to the prev - // slot before overwriting, so a single bad write can't erase the only copy. const existing = await this._readIOSFileOrNull(IOS_BACKUP_FILENAME); + if (this._guardNearEmptyOverwrite(data, existing, 'iOS')) return; if (existing) { await this._writeIOSFile(IOS_BACKUP_PREV_FILENAME, existing); } @@ -246,11 +283,6 @@ export class LocalBackupService { }); } - /** Re-escapes literal newlines from the Android bridge so the blob parses as JSON. */ - private _escapeAndroidNewlines(raw: string | null): string | null { - return raw === null ? null : raw.replace(/\n/g, '\\n'); - } - private async _readIOSFileOrNull(path: string): Promise { try { const result = await Filesystem.readFile({