From 45c384d108ea91c065fac459e43385dae60ef2ba Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sat, 14 Nov 2026 00:59:54 -0800 Subject: [PATCH] perf: save browser extension storage off the main thread (#42189) * perf: save browser extension storage off the main thread chrome.storage.sync and declarativeNetRequest state saved each dirty extension with two synchronous fsyncs on Electron's main thread after a 250 ms debounce, freezing the UI ~10-25 ms per extension per burst. Writes and forget deletes now share one ordered background queue (the common pattern): memory stays the source of truth, deletes complete before later writes of the same key, failed deletes keep blocking them, and callers await deletes before recording cleanup. Quit closes the stores last and waits for their final save, as the old synchronous will-quit save did. Co-Authored-By: Claude Opus 5.5 (1M context) * fix: settle every extension storage delete before the next save A multi-file forget now waits for all its deletes before reporting, so a still-running delete can't erase a later save; file deletes get Node's busy-file retries; finished deletes retire in one pass. Co-Authored-By: Claude Opus 5.5 (1M context) * fix: skip stray files when deleting an extension's storage A .DS_Store beside the profile folders made every removal fail with ENOTDIR, which now also held that extension's saves back. Co-Authored-By: Claude Opus 5.5 (1M context) * test: check extension storage saves leave the caller's turn Co-Authored-By: Claude Opus 5.5 (1M context) --------- Co-authored-by: Claude Opus 5.5 (1M context) --- config/main-sync-fs-baseline.txt | 2 +- .../browser-extension-management.ts | 7 +- .../session/browser-extension-dnr-store.ts | 8 +- .../browser-extension-session-loader.ts | 6 +- .../browser-extension-data-removal.test.ts | 62 ++++- .../browser-extension-data-removal.ts | 15 +- ...-extension-persistence-electron-fixture.ts | 9 +- ...r-extension-profile-data-directory.test.ts | 4 +- ...rowser-extension-profile-document-store.ts | 263 ++++++++++++++---- ...owser-extension-profile-document-sweeps.ts | 92 ++++++ ...owser-extension-sync-storage-store.test.ts | 150 +++++++++- .../browser-extension-sync-storage-store.ts | 8 +- src/main/startup/quit/main-process-quit.ts | 6 +- 13 files changed, 545 insertions(+), 87 deletions(-) create mode 100644 src/main/browser/extensions/profile-storage/browser-extension-profile-document-sweeps.ts diff --git a/config/main-sync-fs-baseline.txt b/config/main-sync-fs-baseline.txt index c3df8dee752..3c2ca3e6d86 100644 --- a/config/main-sync-fs-baseline.txt +++ b/config/main-sync-fs-baseline.txt @@ -58,7 +58,7 @@ 8 src/main/browser/extensions/installation/browser-extension-install-store.ts 1 src/main/browser/extensions/installation/browser-extension-load-directory.ts 2 src/main/browser/extensions/profile-storage/browser-extension-data-removal.ts -8 src/main/browser/extensions/profile-storage/browser-extension-profile-document-store.ts +1 src/main/browser/extensions/profile-storage/browser-extension-profile-document-store.ts 4 src/main/browser/extensions/profile-storage/browser-extension-profile-state-store.ts 3 src/main/browser/extensions/profile-storage/browser-extension-retired-profile-storage.ts 1 src/main/browser/guest-lifecycle/popups/compatibility/browser-popup-background-cdp-probe.ts diff --git a/src/main/browser/extensions/browser-extension-management.ts b/src/main/browser/extensions/browser-extension-management.ts index c25e81a3775..315533d7e5e 100644 --- a/src/main/browser/extensions/browser-extension-management.ts +++ b/src/main/browser/extensions/browser-extension-management.ts @@ -142,8 +142,11 @@ export function removeBrowserExtension(extensionId: string): Promise created.flush()) + closeBrowserExtensionProfileDocumentStoreOnQuit(created) store = created } return store diff --git a/src/main/browser/extensions/profile-sessions/browser-extension-session-loader.ts b/src/main/browser/extensions/profile-sessions/browser-extension-session-loader.ts index 15ef8a8a457..0515d02629b 100644 --- a/src/main/browser/extensions/profile-sessions/browser-extension-session-loader.ts +++ b/src/main/browser/extensions/profile-sessions/browser-extension-session-loader.ts @@ -159,8 +159,10 @@ export async function detachBrowserExtensionsFromProfileSession( } finally { forgetBrowserExtensionActionProfile(profile.id) getBrowserExtensionProfileStateStore().forgetProfile(profile.id) - getBrowserExtensionSyncStorageStore().forgetProfile(profile.id) - getBrowserExtensionDnrStore().forgetProfile(profile.id) + await Promise.all([ + getBrowserExtensionSyncStorageStore().forgetProfile(profile.id), + getBrowserExtensionDnrStore().forgetProfile(profile.id) + ]) } } diff --git a/src/main/browser/extensions/profile-storage/browser-extension-data-removal.test.ts b/src/main/browser/extensions/profile-storage/browser-extension-data-removal.test.ts index edd07c1f6e4..bc0bead212b 100644 --- a/src/main/browser/extensions/profile-storage/browser-extension-data-removal.test.ts +++ b/src/main/browser/extensions/profile-storage/browser-extension-data-removal.test.ts @@ -8,13 +8,14 @@ type WindowRecord = { options: unknown; url: string; script: string; destroyed: const mocks = vi.hoisted(() => { const windows: WindowRecord[] = [] - return { windows, userData: '', readdir: vi.fn() } + return { windows, userData: '', readdir: vi.fn(), rm: vi.fn() } }) vi.mock('node:fs/promises', async (importOriginal) => { const fs = await importOriginal() mocks.readdir.mockImplementation(fs.readdir) - return { ...fs, readdir: mocks.readdir } + mocks.rm.mockImplementation(fs.rm) + return { ...fs, readdir: mocks.readdir, rm: mocks.rm } }) vi.mock('electron', () => ({ @@ -117,7 +118,7 @@ describe('uninstall while another Orca profile is inactive', () => { })) personal.sync.write('default', ID, { token: 'old secret' }) personal.states.flush() - personal.sync.flush() + await personal.sync.flush() personal.installed.save(record) const work = await openOrcaProfile(workDirectory) @@ -135,7 +136,7 @@ describe('uninstall while another Orca profile is inactive', () => { reopened.sync.write('default', ID, { token: 'new secret' }) await reopened.sweep(sess, 'default', new Set([ID])) expect(reopened.sync.read('default', ID)).toEqual({ token: 'new secret' }) - reopened.sync.flush() + await reopened.sync.flush() }) }) @@ -232,5 +233,56 @@ it("preserves another pending unpacked reinstall's data when reconciling one ext expect(sess.clearStorageData).toHaveBeenCalledExactlyOnceWith({ origin: `chrome-extension://${OTHER}` }) - sync.flush() + await sync.flush() +}) + +it('records reinstall cleanup only after the old sync storage file is deleted', async () => { + const store = getBrowserExtensionInstallStore() + const record = { + id: ID, + name: 'Vault', + version: '1', + path: store.installDirectory(ID, '1'), + source: 'webstore' as const, + iconPath: null, + permissions: [], + hostPermissions: [], + optionsPage: null + } + store.save(record) + await store.remove(ID) + store.save(record) + const profileId = 'held-delete' + const sync = getBrowserExtensionSyncStorageStore() + const states = getBrowserExtensionProfileStateStore() + sync.write(profileId, ID, { token: 'old secret' }) + await sync.flush() + let releaseDelete: () => void = () => {} + let deleteStarted: () => void = () => {} + const started = new Promise((resolve) => (deleteStarted = resolve)) + const held = new Promise((resolve) => (releaseDelete = resolve)) + mocks.rm.mockImplementation(async (...args: Parameters) => { + if (String(args[0]).endsWith(`${ID}.json`)) { + deleteStarted() + await held + } + return realFsPromises.rm(...args) + }) + try { + const cleanup = removeReinstalledBrowserExtensionData(fakeSession(false), profileId, ID) + await started + // A marker saved before the delete would skip it after a crash, bringing the old items back. + expect(states.read(profileId, ID).removalGeneration).toBe(0) + releaseDelete() + await cleanup + expect(states.read(profileId, ID).removalGeneration).toBeGreaterThan(0) + expect( + existsSync( + join(mocks.userData, 'browser-extensions', 'sync-storage', profileId, `${ID}.json`) + ) + ).toBe(false) + } finally { + releaseDelete() + mocks.rm.mockImplementation(realFsPromises.rm) + } }) diff --git a/src/main/browser/extensions/profile-storage/browser-extension-data-removal.ts b/src/main/browser/extensions/profile-storage/browser-extension-data-removal.ts index 0e5605f9b91..0859e71186a 100644 --- a/src/main/browser/extensions/profile-storage/browser-extension-data-removal.ts +++ b/src/main/browser/extensions/profile-storage/browser-extension-data-removal.ts @@ -188,11 +188,14 @@ async function removeUninstalledData( const storagePath = sess.getStoragePath() const folder = storagePath ? join(storagePath, EXTENSION_SETTINGS_FOLDER) : null const removed = new Set(stale) + const documentDeletes: Promise[] = [] if ('extensionId' in scope) { if (stale.has(scope.extensionId)) { states.forgetProfileExtension(profileId, scope.extensionId) - getBrowserExtensionSyncStorageStore().forgetProfileExtension(profileId, scope.extensionId) - getBrowserExtensionDnrStore().forgetProfileExtension(profileId, scope.extensionId) + documentDeletes.push( + getBrowserExtensionSyncStorageStore().forgetProfileExtension(profileId, scope.extensionId), + getBrowserExtensionDnrStore().forgetProfileExtension(profileId, scope.extensionId) + ) } } else { const retained = new Set([...scope.installedIds].filter((id) => !stale.has(id))) @@ -200,8 +203,10 @@ async function removeUninstalledData( profileId, new Set([...retained, ...[...removals.keys()].filter((id) => !stale.has(id))]) ) - getBrowserExtensionSyncStorageStore().forgetExtensionsExcept(profileId, retained) - getBrowserExtensionDnrStore().forgetExtensionsExcept(profileId, retained) + documentDeletes.push( + getBrowserExtensionSyncStorageStore().forgetExtensionsExcept(profileId, retained), + getBrowserExtensionDnrStore().forgetExtensionsExcept(profileId, retained) + ) const entries = folder ? await readdir(folder).catch(() => []) : [] for (const id of [ ...previousIds.filter((id) => !scope.installedIds.has(id) && !removals.has(id)), @@ -225,6 +230,8 @@ async function removeUninstalledData( } }) ) + // Why before the markers: a marker saved ahead of the deletes would skip them after a crash. + await Promise.all(documentDeletes) if ('installedIds' in scope) { scope.onUninstalled?.(stale) } diff --git a/src/main/browser/extensions/profile-storage/browser-extension-persistence-electron-fixture.ts b/src/main/browser/extensions/profile-storage/browser-extension-persistence-electron-fixture.ts index ef5aaf52ada..cdfa5314b7e 100644 --- a/src/main/browser/extensions/profile-storage/browser-extension-persistence-electron-fixture.ts +++ b/src/main/browser/extensions/profile-storage/browser-extension-persistence-electron-fixture.ts @@ -13,6 +13,7 @@ import { whenBrowserExtensionSessionsLoaded } from '../profile-sessions/browser-extension-session-loader' import { addUnpackedBrowserExtension } from '../browser-extension-management' +import { closeBrowserExtensionProfileDocumentStores } from './browser-extension-profile-document-store' import { buildBrowserExtensionPageContextMenu } from '../context-menus/browser-extension-context-menu-host' import { recordActiveBrowserPage } from '../tabs-and-windows/browser-extension-tab-model' import { trackBrowserExtensionMainWindow } from '../tabs-and-windows/browser-extension-window-model' @@ -81,9 +82,13 @@ function capture(message: string): void { status[key] = key === 'commands' ? [...(Array.isArray(status[key]) ? status[key] : []), value] : value writeStatus() - // Why quit, not exit: will-quit flushes the extension state the second launch replays. + // Why quit, not exit: will-quit flushes the extension state the second launch replays; sync + // storage saves last, as Orca's quit does. if (key === 'setup' && phase === 'first') { - setTimeout(() => app.quit(), 300) + setTimeout( + () => void closeBrowserExtensionProfileDocumentStores().then(() => app.quit()), + 300 + ) } } } diff --git a/src/main/browser/extensions/profile-storage/browser-extension-profile-data-directory.test.ts b/src/main/browser/extensions/profile-storage/browser-extension-profile-data-directory.test.ts index e380885729b..f26f39f0758 100644 --- a/src/main/browser/extensions/profile-storage/browser-extension-profile-data-directory.test.ts +++ b/src/main/browser/extensions/profile-storage/browser-extension-profile-data-directory.test.ts @@ -39,9 +39,9 @@ describe('browser extension data per Orca profile', () => { personal.dnr.updateBrowserExtensionDnrState('default', extension, { dynamicRules: [{ id: 1 }] }) - personal.sync.flush() + await personal.sync.flush() personal.state.flush() - personal.dnr.getBrowserExtensionDnrStore().flush() + await personal.dnr.getBrowserExtensionDnrStore().flush() vi.resetModules() const work = await openStoresAs('work') diff --git a/src/main/browser/extensions/profile-storage/browser-extension-profile-document-store.ts b/src/main/browser/extensions/profile-storage/browser-extension-profile-document-store.ts index 9a1f6a8b0bb..d52a3250b3e 100644 --- a/src/main/browser/extensions/profile-storage/browser-extension-profile-document-store.ts +++ b/src/main/browser/extensions/profile-storage/browser-extension-profile-document-store.ts @@ -1,7 +1,18 @@ -import { mkdirSync, readFileSync, readdirSync, rmSync } from 'node:fs' +import { readFileSync } from 'node:fs' +import { mkdir } from 'node:fs/promises' import { join } from 'node:path' -import { isChromeExtensionId } from '../../../../shared/browser/extensions/browser-extension-url' -import { durableWriteTempPath, writeFileDurableSync } from '../../../filesystem/durable-file-write' +import { + durableWriteTempPath, + writeFileDurableIfCurrent +} from '../../../filesystem/durable-file-write' +import { + deleteSweptDocuments, + documentFileName, + documentProfileDirectory, + sweepCovers, + type Sweep, + type SweepScope +} from './browser-extension-profile-document-sweeps' const SYNC_STORAGE_DIRECTORY = 'sync-storage' @@ -13,12 +24,47 @@ function isItems(value: unknown): value is BrowserExtensionProfileDocumentItems return value !== null && typeof value === 'object' && !Array.isArray(value) } +/** A forget after the final save didn't delete anything, so its caller must not record it as done. */ +function closedError(): Promise { + const rejected = Promise.reject(new Error('Extension storage is closed for quit')) + rejected.catch(() => {}) + return rejected +} + +const storesToCloseOnQuit = new Set() +let quitClosing = false + +/** Production stores join the final save on quit; one created after it starts is closed at once. */ +export function closeBrowserExtensionProfileDocumentStoreOnQuit( + store: BrowserExtensionProfileDocumentStore +): void { + storesToCloseOnQuit.add(store) + if (quitClosing) { + void store.close() + } +} + +/** Final save of every production store this run created; never rejects. */ +export function closeBrowserExtensionProfileDocumentStores(): Promise { + quitClosing = true + return Promise.allSettled([...storesToCloseOnQuit].map((store) => store.close())).then(() => {}) +} + +/** + * Memory is the source of truth; disk work (writes and forget deletes, in request order) runs off + * the main thread, one pass at a time, because durable writes fsync twice per file. + */ export class BrowserExtensionProfileDocumentStore { /** Profile → extension → items. */ private readonly cache = new Map>() /** Profile → extensions whose items are not on disk yet. */ private readonly dirty = new Map>() + /** Forgets not on disk yet, oldest first; a running pass keeps its sweeps here until done. */ + private sweeps: Sweep[] = [] private timer: NodeJS.Timeout | null = null + private running: Promise | null = null + private passRequested = false + private closed = false constructor( private readonly rootDirectory: string, @@ -41,42 +87,44 @@ export class BrowserExtensionProfileDocumentStore { } write(profileId: string, extensionId: string, items: BrowserExtensionProfileDocumentItems): void { + // Closed for quit: changing memory now would veto the final save of earlier writes. + if (this.closed) { + return + } this.read(profileId, extensionId) this.cache.get(profileId)?.set(extensionId, items) - const dirty = this.dirty.get(profileId) ?? new Set() - dirty.add(extensionId) - this.dirty.set(profileId, dirty) + this.markDirty(profileId, extensionId) this.schedulePersist() } - forgetExtension(extensionId: string): void { + /** Settles once the files are deleted; memory forgets at once. */ + forgetExtension(extensionId: string): Promise { + if (this.closed) { + return closedError() + } for (const byExtension of this.cache.values()) { byExtension.delete(extensionId) } for (const dirty of this.dirty.values()) { dirty.delete(extensionId) } - let profileDirectories: string[] - try { - profileDirectories = readdirSync(join(this.rootDirectory, this.directoryName)) - } catch { - return - } - for (const directory of profileDirectories) { - rmSync(join(this.rootDirectory, this.directoryName, directory, this.fileName(extensionId)), { - force: true - }) - } + return this.queueSweep({ kind: 'extension', extensionId }) } - forgetProfileExtension(profileId: string, extensionId: string): void { + forgetProfileExtension(profileId: string, extensionId: string): Promise { + if (this.closed) { + return closedError() + } this.cache.get(profileId)?.delete(extensionId) this.dirty.get(profileId)?.delete(extensionId) - rmSync(join(this.profileDirectory(profileId), this.fileName(extensionId)), { force: true }) + return this.queueSweep({ kind: 'profileExtension', profileId, extensionId }) } /** Drops one profile's items of every extension not in `extensionIds`. */ - forgetExtensionsExcept(profileId: string, extensionIds: ReadonlySet): void { + forgetExtensionsExcept(profileId: string, extensionIds: ReadonlySet): Promise { + if (this.closed) { + return closedError() + } const cached = this.cache.get(profileId) for (const extensionId of cached?.keys() ?? []) { if (!extensionIds.has(extensionId)) { @@ -84,44 +132,67 @@ export class BrowserExtensionProfileDocumentStore { this.dirty.get(profileId)?.delete(extensionId) } } - let files: string[] - try { - files = readdirSync(this.profileDirectory(profileId)) - } catch { - return - } - for (const file of files) { - const extensionId = file.replace(/\.json$/, '') - if (isChromeExtensionId(extensionId) && !extensionIds.has(extensionId)) { - rmSync(join(this.profileDirectory(profileId), file), { force: true }) - } - } + return this.queueSweep({ + kind: 'profileExcept', + profileId, + extensionIds: new Set(extensionIds) + }) } - forgetProfile(profileId: string): void { + forgetProfile(profileId: string): Promise { + if (this.closed) { + return closedError() + } this.cache.delete(profileId) this.dirty.delete(profileId) - rmSync(this.profileDirectory(profileId), { recursive: true, force: true }) + return this.queueSweep({ kind: 'profile', profileId }) } - flush(): void { + /** Saves what is pending; stops early only if a pass that tried work saved nothing (e.g. disk full). */ + async flush(): Promise { if (this.timer) { clearTimeout(this.timer) this.timer = null } - for (const [profileId, extensionIds] of this.dirty) { - const directory = this.profileDirectory(profileId) - mkdirSync(directory, { recursive: true }) - for (const extensionId of extensionIds) { - const target = join(directory, this.fileName(extensionId)) - const items = this.cache.get(profileId)?.get(extensionId) ?? {} - writeFileDurableSync(durableWriteTempPath(target), target, JSON.stringify(items)) - extensionIds.delete(extensionId) + for (;;) { + while (this.running) { + await this.running + } + if (!this.hasPendingWork() || !(await this.runPass())) { + return } - this.dirty.delete(profileId) } } + /** Final save on quit; later writes are ignored and forgets refused, as after the old last quit save. */ + close(): Promise { + this.closed = true + return this.flush() + } + + private queueSweep(scope: SweepScope): Promise { + let settle!: Sweep['settle'] + const done = new Promise((resolve, reject) => { + settle = (error) => (error === undefined ? resolve() : reject(error)) + }) + // Callers that don't await still get the deletion; only the first failure is reported. + done.catch(() => {}) + this.sweeps.push({ scope, done, settle }) + // Callers wait for the delete, so don't debounce it. + void this.runPass() + return done + } + + private markDirty(profileId: string, extensionId: string): void { + const dirty = this.dirty.get(profileId) ?? new Set() + dirty.add(extensionId) + this.dirty.set(profileId, dirty) + } + + private hasPendingWork(): boolean { + return this.sweeps.length > 0 || [...this.dirty.values()].some((ids) => ids.size > 0) + } + // Why debounce: extensions often write settings key by key in a burst. private schedulePersist(): void { if (this.timer) { @@ -129,15 +200,99 @@ export class BrowserExtensionProfileDocumentStore { } this.timer = setTimeout(() => { this.timer = null - try { - this.flush() - } catch (error) { - console.warn('[browser-extensions] Could not save extension sync storage:', error) - } + void this.runPass() }, PERSIST_DELAY_MS) } + /** One pass at a time; a request during a pass runs one more pass after it. Resolves false when nothing succeeded. */ + private runPass(): Promise { + if (this.running) { + this.passRequested = true + return this.running + } + this.running = this.persist().finally(() => { + this.running = null + if (this.passRequested) { + this.passRequested = false + void this.runPass() + } + }) + return this.running + } + + private async persist(): Promise { + let progressed = false + let failure: unknown = null + // Take this pass's work in one step: a forget dropped every earlier dirty key it covers, so these + // documents were all written after the queued sweeps, which therefore run first. + const sweeps = [...this.sweeps] + const jobs: { + profileId: string + extensionId: string + items: BrowserExtensionProfileDocumentItems + }[] = [] + for (const [profileId, extensionIds] of this.dirty) { + for (const extensionId of extensionIds) { + const items = this.cache.get(profileId)?.get(extensionId) + if (items) { + jobs.push({ profileId, extensionId, items }) + } + } + } + this.dirty.clear() + const deleted = new Set() + for (const sweep of sweeps) { + try { + await deleteSweptDocuments(sweep.scope, this.storageDirectory()) + deleted.add(sweep) + sweep.settle() + progressed = true + } catch (error) { + sweep.settle(error) + failure = error + } + } + this.sweeps = this.sweeps.filter((sweep) => !deleted.has(sweep)) + for (const { profileId, extensionId, items } of jobs) { + // A newer write or a forget replaces the cached document; that write or delete owns the file. + const isCurrent = () => this.cache.get(profileId)?.get(extensionId) === items + if (!isCurrent()) { + continue + } + // Written after a forget whose delete failed: keep the delete first. + if (this.sweeps.some((sweep) => sweepCovers(sweep.scope, profileId, extensionId))) { + this.markDirty(profileId, extensionId) + continue + } + const directory = this.profileDirectory(profileId) + const target = join(directory, this.fileName(extensionId)) + try { + await mkdir(directory, { recursive: true }) + await writeFileDurableIfCurrent( + durableWriteTempPath(target), + target, + JSON.stringify(items), + isCurrent + ) + progressed = true + } catch (error) { + failure = error + if (isCurrent()) { + this.markDirty(profileId, extensionId) + } + } + } + if (failure !== null) { + console.warn('[browser-extensions] Could not save extension storage:', failure) + } + return progressed || failure === null + } + private load(profileId: string, extensionId: string): BrowserExtensionProfileDocumentItems { + // A queued forget will delete this file; don't bring its items back meanwhile. + if (this.sweeps.some((sweep) => sweepCovers(sweep.scope, profileId, extensionId))) { + return {} + } try { const raw: unknown = JSON.parse( readFileSync(join(this.profileDirectory(profileId), this.fileName(extensionId)), 'utf8') @@ -148,11 +303,15 @@ export class BrowserExtensionProfileDocumentStore { } } + private storageDirectory(): string { + return join(this.rootDirectory, this.directoryName) + } + private profileDirectory(profileId: string): string { - return join(this.rootDirectory, this.directoryName, encodeURIComponent(profileId)) + return documentProfileDirectory(this.storageDirectory(), profileId) } private fileName(extensionId: string): string { - return `${encodeURIComponent(extensionId)}.json` + return documentFileName(extensionId) } } diff --git a/src/main/browser/extensions/profile-storage/browser-extension-profile-document-sweeps.ts b/src/main/browser/extensions/profile-storage/browser-extension-profile-document-sweeps.ts new file mode 100644 index 00000000000..6c2f523cbab --- /dev/null +++ b/src/main/browser/extensions/profile-storage/browser-extension-profile-document-sweeps.ts @@ -0,0 +1,92 @@ +import { readdir, rm } from 'node:fs/promises' +import { join } from 'node:path' +import { isChromeExtensionId } from '../../../../shared/browser/extensions/browser-extension-url' + +/** Files a forget removes; queued with the writes so an older write can never land after it. */ +export type SweepScope = + | { kind: 'extension'; extensionId: string } + | { kind: 'profileExtension'; profileId: string; extensionId: string } + | { kind: 'profileExcept'; profileId: string; extensionIds: ReadonlySet } + | { kind: 'profile'; profileId: string } + +export type Sweep = { scope: SweepScope; done: Promise; settle: (error?: unknown) => void } + +export function documentProfileDirectory(storageDirectory: string, profileId: string): string { + return join(storageDirectory, encodeURIComponent(profileId)) +} + +export function documentFileName(extensionId: string): string { + return `${encodeURIComponent(extensionId)}.json` +} + +export function sweepCovers(sweep: SweepScope, profileId: string, extensionId: string): boolean { + switch (sweep.kind) { + case 'extension': + return sweep.extensionId === extensionId + case 'profileExtension': + return sweep.profileId === profileId && sweep.extensionId === extensionId + case 'profileExcept': + return sweep.profileId === profileId && !sweep.extensionIds.has(extensionId) + case 'profile': + return sweep.profileId === profileId + } +} + +/** A missing directory has nothing to delete; any other read error fails the sweep. */ +async function ifPresent(read: () => Promise): Promise { + try { + return await read() + } catch (error) { + if (error instanceof Error && 'code' in error && error.code === 'ENOENT') { + return [] + } + throw error + } +} + +// Node retries busy files only in recursive mode, which also removes a plain file. +const REMOVE_OPTIONS = { force: true, recursive: true, maxRetries: 3 } + +/** Settles every removal before reporting a failure, so none outlives its sweep. */ +async function removeAll(paths: string[]): Promise { + const results = await Promise.allSettled(paths.map((path) => rm(path, REMOVE_OPTIONS))) + const failed = results.find((result) => result.status === 'rejected') + if (failed) { + throw failed.reason + } +} + +export async function deleteSweptDocuments( + sweep: SweepScope, + storageDirectory: string +): Promise { + if (sweep.kind === 'profile') { + await rm(documentProfileDirectory(storageDirectory, sweep.profileId), REMOVE_OPTIONS) + return + } + if (sweep.kind === 'profileExtension') { + const directory = documentProfileDirectory(storageDirectory, sweep.profileId) + await rm(join(directory, documentFileName(sweep.extensionId)), REMOVE_OPTIONS) + return + } + if (sweep.kind === 'extension') { + const entries = await ifPresent(() => readdir(storageDirectory, { withFileTypes: true })) + // Skip stray files like .DS_Store: a path through one fails with ENOTDIR on every retry. + await removeAll( + entries + .filter((entry) => entry.isDirectory()) + .map((entry) => join(storageDirectory, entry.name, documentFileName(sweep.extensionId))) + ) + return + } + const directory = documentProfileDirectory(storageDirectory, sweep.profileId) + const files = await ifPresent(() => readdir(directory)) + await removeAll( + files + .filter((file) => { + const extensionId = file.replace(/\.json$/, '') + return isChromeExtensionId(extensionId) && !sweep.extensionIds.has(extensionId) + }) + .map((file) => join(directory, file)) + ) +} diff --git a/src/main/browser/extensions/profile-storage/browser-extension-sync-storage-store.test.ts b/src/main/browser/extensions/profile-storage/browser-extension-sync-storage-store.test.ts index 427f19ce811..0ffc0ff0db9 100644 --- a/src/main/browser/extensions/profile-storage/browser-extension-sync-storage-store.test.ts +++ b/src/main/browser/extensions/profile-storage/browser-extension-sync-storage-store.test.ts @@ -1,8 +1,40 @@ -import { mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { existsSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import type * as FsPromises from 'node:fs/promises' import { tmpdir } from 'node:os' -import { join } from 'node:path' +import { join, sep } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +const io = vi.hoisted(() => { + const state: { + heldMkdir: Promise | null + failingRemovals: number + beforeRemove: ((path: string) => Promise) | null + } = { + heldMkdir: null, + failingRemovals: 0, + beforeRemove: null + } + return state +}) + +vi.mock('node:fs/promises', async (importOriginal) => { + const fs = await importOriginal() + return { + ...fs, + mkdir: async (...args: Parameters) => { + await io.heldMkdir + return fs.mkdir(...args) + }, + rm: async (...args: Parameters) => { + await io.beforeRemove?.(String(args[0])) + if (io.failingRemovals > 0) { + io.failingRemovals-- + throw Object.assign(new Error('resource busy'), { code: 'EBUSY' }) + } + return fs.rm(...args) + } + } +}) vi.mock('electron', () => ({ app: { getPath: () => tmpdir(), on: vi.fn() } })) const { BrowserExtensionProfileDocumentStore } = @@ -14,37 +46,42 @@ let root: string beforeEach(() => { root = mkdtempSync(join(tmpdir(), 'orca-extension-sync-')) + vi.spyOn(console, 'warn').mockImplementation(() => {}) }) afterEach(() => { + io.heldMkdir = null + io.failingRemovals = 0 + io.beforeRemove = null + vi.restoreAllMocks() rmSync(root, { recursive: true, force: true }) }) describe('BrowserExtensionProfileDocumentStore', () => { - it('retries acknowledged writes after a failed flush', () => { + it('retries acknowledged writes after a failed save', async () => { const store = new BrowserExtensionProfileDocumentStore(root) store.write('default', EXTENSION, { token: 'saved after recovery' }) writeFileSync(join(root, 'sync-storage'), 'blocks the storage directory') - expect(() => store.flush()).toThrow() + await store.flush() rmSync(join(root, 'sync-storage')) - store.flush() + await store.flush() expect(new BrowserExtensionProfileDocumentStore(root).read('default', EXTENSION)).toEqual({ token: 'saved after recovery' }) }) - it('forgets a removed extension in every profile and a deleted profile entirely', () => { + it('forgets a removed extension in every profile and a deleted profile entirely', async () => { const store = new BrowserExtensionProfileDocumentStore(root) for (const profile of ['default', 'work']) { store.write(profile, EXTENSION, { kept: profile }) store.write(profile, OTHER, { kept: profile }) } - store.flush() + await store.flush() + writeFileSync(join(root, 'sync-storage', '.DS_Store'), 'Finder metadata') // A pending write must not bring a forgotten extension back. store.write('default', EXTENSION, { pending: true }) - store.forgetExtension(EXTENSION) - store.forgetProfile('work') - store.flush() + await Promise.all([store.forgetExtension(EXTENSION), store.forgetProfile('work')]) + await store.flush() expect(store.read('default', EXTENSION)).toEqual({}) const reopened = new BrowserExtensionProfileDocumentStore(root) @@ -52,4 +89,97 @@ describe('BrowserExtensionProfileDocumentStore', () => { expect(reopened.read('default', OTHER)).toEqual({ kept: 'default' }) expect(reopened.read('work', OTHER)).toEqual({}) }) + + it('keeps a reinstalled extension’s new items when it was forgotten during an older save', async () => { + const store = new BrowserExtensionProfileDocumentStore(root) + let release: () => void = () => {} + io.heldMkdir = new Promise((resolve) => (release = resolve)) + store.write('default', EXTENSION, { token: 'old' }) + const saving = store.flush() + // The save waits on its own I/O instead of writing on the caller's turn. + expect(existsSync(join(root, 'sync-storage', 'default', `${EXTENSION}.json`))).toBe(false) + + const forgotten = store.forgetProfileExtension('default', EXTENSION) + expect(store.read('default', EXTENSION)).toEqual({}) + store.write('default', EXTENSION, { token: 'new' }) + release() + await Promise.all([saving, forgotten]) + await store.flush() + + expect(new BrowserExtensionProfileDocumentStore(root).read('default', EXTENSION)).toEqual({ + token: 'new' + }) + }) + + it('saves a write made after a forget only once the forget’s delete succeeds', async () => { + const store = new BrowserExtensionProfileDocumentStore(root) + store.write('default', EXTENSION, { token: 'old' }) + await store.flush() + io.failingRemovals = 2 + + await expect(store.forgetProfileExtension('default', EXTENSION)).rejects.toThrow( + 'resource busy' + ) + store.write('default', EXTENSION, { token: 'new' }) + await store.flush() + await store.flush() + + expect(new BrowserExtensionProfileDocumentStore(root).read('default', EXTENSION)).toEqual({ + token: 'new' + }) + }) + + it('finishes every delete of a failed forget before a later write can land', async () => { + const seeded = new BrowserExtensionProfileDocumentStore(root) + seeded.write('first', EXTENSION, { token: 'old' }) + seeded.write('second', EXTENSION, { token: 'old' }) + await seeded.flush() + let release: () => void = () => {} + let secondStarted: () => void = () => {} + const started = new Promise((resolve) => (secondStarted = resolve)) + const held = new Promise((resolve) => (release = resolve)) + io.beforeRemove = async (path) => { + if (path.includes(`${sep}first${sep}`)) { + throw Object.assign(new Error('resource busy'), { code: 'EBUSY' }) + } + if (path.includes(`${sep}second${sep}`)) { + secondStarted() + await held + } + } + + const store = new BrowserExtensionProfileDocumentStore(root) + let settled = false + const forgotten = store.forgetExtension(EXTENSION).finally(() => (settled = true)) + // Never read before, yet it must not load the file its pending delete removes. + expect(store.read('second', EXTENSION)).toEqual({}) + await started + await new Promise((resolve) => setImmediate(resolve)) + expect(settled).toBe(false) + release() + await expect(forgotten).rejects.toThrow('resource busy') + io.beforeRemove = null + store.write('second', EXTENSION, { token: 'new' }) + await store.flush() + + expect(new BrowserExtensionProfileDocumentStore(root).read('second', EXTENSION)).toEqual({ + token: 'new' + }) + }) + + it('saves writes made before close, not after', async () => { + const store = new BrowserExtensionProfileDocumentStore(root) + let release: () => void = () => {} + io.heldMkdir = new Promise((resolve) => (release = resolve)) + store.write('default', EXTENSION, { token: 'before quit' }) + const closing = store.close() + store.write('default', EXTENSION, { token: 'after quit' }) + await expect(store.forgetProfileExtension('default', EXTENSION)).rejects.toThrow() + release() + await closing + + expect(new BrowserExtensionProfileDocumentStore(root).read('default', EXTENSION)).toEqual({ + token: 'before quit' + }) + }) }) diff --git a/src/main/browser/extensions/profile-storage/browser-extension-sync-storage-store.ts b/src/main/browser/extensions/profile-storage/browser-extension-sync-storage-store.ts index 569bdd6b60c..03215c61f22 100644 --- a/src/main/browser/extensions/profile-storage/browser-extension-sync-storage-store.ts +++ b/src/main/browser/extensions/profile-storage/browser-extension-sync-storage-store.ts @@ -1,5 +1,7 @@ -import { BrowserExtensionProfileDocumentStore } from './browser-extension-profile-document-store' -import { app } from 'electron' +import { + BrowserExtensionProfileDocumentStore, + closeBrowserExtensionProfileDocumentStoreOnQuit +} from './browser-extension-profile-document-store' import { getBrowserExtensionProfileDataDirectory } from './browser-extension-profile-data-directory' let syncStorageStore: BrowserExtensionProfileDocumentStore | null = null @@ -14,7 +16,7 @@ export function getBrowserExtensionSyncStorageStore(): BrowserExtensionProfileDo const store = new BrowserExtensionProfileDocumentStore( getBrowserExtensionProfileDataDirectory() ) - app.on('will-quit', () => store.flush()) + closeBrowserExtensionProfileDocumentStoreOnQuit(store) syncStorageStore = store } return syncStorageStore diff --git a/src/main/startup/quit/main-process-quit.ts b/src/main/startup/quit/main-process-quit.ts index 68ea61e20a9..e704aff8c34 100644 --- a/src/main/startup/quit/main-process-quit.ts +++ b/src/main/startup/quit/main-process-quit.ts @@ -42,6 +42,7 @@ import { isDevParentShutdownRequested } from '../configure-process' import { getCanonicalUserDataPath } from '../../persistence/loading-store/user-data-path' import { flushUnsavedCloudSessions } from '../../orca-profiles/cloud-session/profile-cloud-session-owner' import { getWorkspaceCleanupRunner } from '../../workspaces/cleanup/workspace-cleanup-runner-registry' +import { closeBrowserExtensionProfileDocumentStores } from '../../browser/extensions/profile-storage/browser-extension-profile-document-store' // First will-quit pass awaits teardown; the re-fired pass must exit without starting writes again. let daemonDisconnectDone = false @@ -284,7 +285,8 @@ function installWillQuitHandler(): void { const daemonTeardown = removalQuiescence.then(() => isDevParentShutdownRequested() ? shutdownDaemon() : disconnectDaemon() ) - // Bound unsettled transports and final writes so quit cannot hang (#9447). Failures still exit; atomic swaps preserve old files if the final debounce is lost. + // Bound unsettled transports and these final writes so quit cannot hang (#9447). Failures still exit; atomic swaps preserve old files if the final debounce is lost. + // Extension storage's final save runs after this, unbounded, as its old synchronous will-quit save was. settleTeardownWithinDeadline([ { name: 'worktree-removals', promise: removalShutdown }, { name: 'state-checkpoint', promise: state.profileShutdownCheckpoint }, @@ -317,6 +319,8 @@ function installWillQuitHandler(): void { .catch(() => { /* swallow — telemetry must never prevent app.quit() */ }) + // Last before quitting, so it saves every extension write accepted during teardown. + .then(() => closeBrowserExtensionProfileDocumentStores()) .then(() => { daemonDisconnectDone = true app.quit()