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()