diff --git a/docs/audits/local-log-tail-lifetime/README.md b/docs/audits/local-log-tail-lifetime/README.md new file mode 100644 index 00000000000..ac2f6a09c55 --- /dev/null +++ b/docs/audits/local-log-tail-lifetime/README.md @@ -0,0 +1,28 @@ +# Local log-tail watchers outliving their renderer + +Main can receive a log-tail subscription, await path authorization, and finish installing its native watcher after the requesting renderer has gone away. Previously, the destroyed listener was registered only after authorization. Installed watchers also survived a renderer crash or a new document loaded into the same WebContents. The map retained each watcher and its sender callback; callbacks suppressed notifications to destroyed senders without releasing resources. This handler is present in `v1.4.198`. + +The fix gives each sender one owner using the existing `abortWhenRendererGone` policy: destruction, renderer process loss, or committed document navigation closes its live watches and invalidates pending authorization. Same-document and canceled navigation preserve the owner. For a reused subscription ID, the latest pending request wins. Each pending subscription has an identity token; old completions and old watcher errors cannot replace or close newer subscriptions. Failed authorization preserves an existing installed watch. The last pending/live release removes all owner listeners. + +This is a reproduced native-handle and small metadata leak. Watchers do not retain file-content chunks. It does not establish the input frequency or memory scale in [#19768](https://github.com/stablyai/orca/issues/19768) or [#19831](https://github.com/stablyai/orca/issues/19831). + +## Reproduce + +From the repository root with existing dependencies: + +```sh +ORCA_BACKGROUND_LAUNCH=1 node docs/audits/local-log-tail-lifetime/reproduce.mjs +``` + +The script runs the actual IPC handlers against temporary files, real `fs.watch` handles, controlled authorization promises, and EventEmitter senders. The existing IPC tests use watcher doubles to deliver an error from a retired watcher. No Electron window, real user log, process inventory, or network request is used. Test cleanup releases all watchers. + +The baseline reverses only `fix.patch` in a temporary Vite transform. Working sources remain unchanged; source hashes and exact failed cases are recorded in `results.json`. Child test runners use the shared cross-platform process runner. + +| Version | Passed | Failed | +| ------------------- | -----: | -----: | +| Before lifetime fix | 9 | 10 | +| With lifetime fix | 19 | 0 | + +The twenty-owner case retained twenty native watcher owners before the fix and zero afterward. The broader cases cover destruction during authorization, active-plus-pending replacement, process loss/navigation, failed replacement, explicit stop, idle listener disposal, superseded success/error, and failed native installation. Ordinary tab cancellation already waited for start before stop; that behavior remains covered by the renderer hook tests. + +Additional validation: Node typecheck, direct lint, and the existing renderer-lifetime and local-log-tail hook suites. This endpoint only watches renderer-authorized local logs. SSH/paired-runtime execution ownership and wire schemas do not change; the local editor eligibility check already excludes runtime-environment files. Folder workspaces follow the existing path authorization policy. diff --git a/docs/audits/local-log-tail-lifetime/fix.patch b/docs/audits/local-log-tail-lifetime/fix.patch new file mode 100644 index 00000000000..398f2c3f820 --- /dev/null +++ b/docs/audits/local-log-tail-lifetime/fix.patch @@ -0,0 +1,210 @@ +diff --git a/src/main/ipc/local-log-tail.ts b/src/main/ipc/local-log-tail.ts +index 430882b4e0..0892665ad5 100644 +--- a/src/main/ipc/local-log-tail.ts ++++ b/src/main/ipc/local-log-tail.ts +@@ -9,35 +9,83 @@ import type { + } from '../../shared/local-log-tail-types' + import { readLocalLogTailRange } from '../ai-vault/local-log-tail-reader' + import { resolveAuthorizedPath } from './filesystem-auth' ++import { abortWhenRendererGone } from './renderer-lifetime-abort' + +-type TailWatch = { ++type TailSenderOwner = { + senderId: number ++ pending: Map ++ watchKeys: Set ++ signal: AbortSignal ++ dispose: () => void ++} ++ ++type TailWatch = { ++ owner: TailSenderOwner + watcher: FSWatcher + } + + const tailWatches = new Map() +-const senderCleanupRegistered = new Set() ++const senderOwners = new Map() + + function watchKey(senderId: number, subscriptionId: string): string { + return `${senderId}:${subscriptionId}` + } + +-function closeWatch(key: string): void { ++function releaseIdleOwner(owner: TailSenderOwner): void { ++ if (owner.pending.size > 0 || owner.watchKeys.size > 0) { ++ return ++ } ++ if (senderOwners.get(owner.senderId) === owner) { ++ senderOwners.delete(owner.senderId) ++ } ++ owner.dispose() ++} ++ ++function closeWatch(key: string, expected?: TailWatch): void { + const subscription = tailWatches.get(key) +- if (!subscription) { ++ if (!subscription || (expected && subscription !== expected)) { + return + } + tailWatches.delete(key) +- subscription.watcher.close() ++ subscription.owner.watchKeys.delete(key) ++ try { ++ subscription.watcher.close() ++ } finally { ++ releaseIdleOwner(subscription.owner) ++ } + } + +-function closeSenderWatches(senderId: number): void { +- senderCleanupRegistered.delete(senderId) +- for (const [key, subscription] of tailWatches) { +- if (subscription.senderId === senderId) { +- closeWatch(key) ++function closeSenderWatches(owner: TailSenderOwner): void { ++ owner.pending.clear() ++ for (const key of owner.watchKeys) { ++ const subscription = tailWatches.get(key) ++ if (subscription?.owner === owner) { ++ closeWatch(key, subscription) ++ } ++ } ++ releaseIdleOwner(owner) ++} ++ ++function getSenderOwner(sender: WebContents): TailSenderOwner { ++ const existing = senderOwners.get(sender.id) ++ if (existing) { ++ return existing ++ } ++ const lifetime = abortWhenRendererGone(sender) ++ const onAbort = (): void => closeSenderWatches(owner) ++ const owner: TailSenderOwner = { ++ senderId: sender.id, ++ pending: new Map(), ++ watchKeys: new Set(), ++ signal: lifetime.signal, ++ dispose: () => { ++ lifetime.signal.removeEventListener('abort', onAbort) ++ lifetime.dispose() + } + } ++ senderOwners.set(sender.id, owner) ++ lifetime.signal.addEventListener('abort', onAbort, { once: true }) ++ return owner + } + + function validateSubscriptionId(value: unknown): string { +@@ -47,12 +95,52 @@ function validateSubscriptionId(value: unknown): string { + return value + } + +-function registerSenderCleanup(sender: WebContents): void { +- if (senderCleanupRegistered.has(sender.id)) { ++async function startWatch( ++ sender: WebContents, ++ args: LocalLogTailWatchArgs, ++ store: Store ++): Promise { ++ const subscriptionId = validateSubscriptionId(args.subscriptionId) ++ if (sender.isDestroyed()) { + return + } +- senderCleanupRegistered.add(sender.id) +- sender.once('destroyed', () => closeSenderWatches(sender.id)) ++ const key = watchKey(sender.id, subscriptionId) ++ const owner = getSenderOwner(sender) ++ const pending = Symbol(subscriptionId) ++ owner.pending.set(key, pending) ++ try { ++ const filePath = await resolveAuthorizedPath(args.filePath, store) ++ if ( ++ sender.isDestroyed() || ++ owner.signal.aborted || ++ senderOwners.get(sender.id) !== owner || ++ owner.pending.get(key) !== pending ++ ) { ++ return ++ } ++ closeWatch(key) ++ const sendChange = (eventType: 'change' | 'rename'): void => { ++ if (tailWatches.get(key) !== subscription || sender.isDestroyed()) { ++ return ++ } ++ const payload: LocalLogTailChangedPayload = { subscriptionId, eventType } ++ sender.send('fs:localLogTailChanged', payload) ++ } ++ const watcher = watch(filePath, (eventType) => sendChange(eventType)) ++ const subscription: TailWatch = { owner, watcher } ++ watcher.on('error', () => { ++ // Rotation needs one final drain before releasing this exact watcher. ++ sendChange('rename') ++ closeWatch(key, subscription) ++ }) ++ tailWatches.set(key, subscription) ++ owner.watchKeys.add(key) ++ } finally { ++ if (owner.pending.get(key) === pending) { ++ owner.pending.delete(key) ++ } ++ releaseIdleOwner(owner) ++ } + } + + export function registerLocalLogTailHandlers(store: Store): void { +@@ -64,43 +152,25 @@ export function registerLocalLogTailHandlers(store: Store): void { + } + ) + +- ipcMain.handle( +- 'fs:startLocalLogTail', +- async (event, args: LocalLogTailWatchArgs): Promise => { +- const subscriptionId = validateSubscriptionId(args.subscriptionId) +- const filePath = await resolveAuthorizedPath(args.filePath, store) +- const key = watchKey(event.sender.id, subscriptionId) +- closeWatch(key) +- +- const sendChange = (eventType: 'change' | 'rename'): void => { +- if (!tailWatches.has(key) || event.sender.isDestroyed()) { +- return +- } +- const payload: LocalLogTailChangedPayload = { subscriptionId, eventType } +- event.sender.send('fs:localLogTailChanged', payload) +- } +- const watcher = watch(filePath, (eventType) => sendChange(eventType)) +- watcher.on('error', () => { +- // Why: an error commonly accompanies rotation. Signal one final drain so +- // the renderer can detect identity change, then release the dead handle. +- sendChange('rename') +- closeWatch(key) +- }) +- tailWatches.set(key, { senderId: event.sender.id, watcher }) +- registerSenderCleanup(event.sender) +- } ++ ipcMain.handle('fs:startLocalLogTail', (event, args: LocalLogTailWatchArgs): Promise => ++ startWatch(event.sender, args, store) + ) + + ipcMain.handle('fs:stopLocalLogTail', (event, args: { subscriptionId: string }): void => { +- closeWatch(watchKey(event.sender.id, validateSubscriptionId(args.subscriptionId))) ++ const key = watchKey(event.sender.id, validateSubscriptionId(args.subscriptionId)) ++ const owner = senderOwners.get(event.sender.id) ++ owner?.pending.delete(key) ++ closeWatch(key) ++ if (owner) { ++ releaseIdleOwner(owner) ++ } + }) + } + + export function closeAllLocalLogTailWatchers(): void { +- for (const key of Array.from(tailWatches.keys())) { +- closeWatch(key) ++ for (const owner of senderOwners.values()) { ++ closeSenderWatches(owner) + } +- senderCleanupRegistered.clear() + } + + /** Test-only: verifies tab/window teardown does not retain native watchers. */ diff --git a/docs/audits/local-log-tail-lifetime/reproduce.mjs b/docs/audits/local-log-tail-lifetime/reproduce.mjs new file mode 100644 index 00000000000..4d683bb09e8 --- /dev/null +++ b/docs/audits/local-log-tail-lifetime/reproduce.mjs @@ -0,0 +1,146 @@ +import { createHash } from 'node:crypto' +import { mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' +import { createRequire } from 'node:module' +import { tmpdir } from 'node:os' +import { join, resolve } from 'node:path' +import { fileURLToPath, pathToFileURL } from 'node:url' +import { applyPatch, parsePatch, reversePatch } from 'diff' +import { build } from 'esbuild' + +if (process.env.ORCA_BACKGROUND_LAUNCH !== '1') { + throw new Error('Run with ORCA_BACKGROUND_LAUNCH=1.') +} + +const root = fileURLToPath(new URL('../../../', import.meta.url)) +const patch = await readFile(new URL('./fix.patch', import.meta.url), 'utf8') +const beforeSources = {} +const sourceHashes = {} +for (const parsed of parsePatch(patch)) { + const path = parsed.newFileName.replace(/^b\//, '') + const absolute = resolve(root, path) + const current = await readFile(absolute, 'utf8') + const before = applyPatch(current, reversePatch(parsed)) + if (before === false) { + throw new Error(`Source changed; review the proof patch: ${path}`) + } + beforeSources[absolute.replaceAll('\\', '/')] = before + sourceHashes[path] = { + before: createHash('sha256').update(before).digest('hex'), + after: createHash('sha256').update(current).digest('hex') + } +} + +for (const path of [ + 'src/main/ipc/local-log-tail-lifetime.test.ts', + 'src/main/ipc/local-log-tail.test.ts' +]) { + sourceHashes[path] = { + current: createHash('sha256') + .update(await readFile(resolve(root, path))) + .digest('hex') + } +} + +const scratch = await mkdtemp(join(tmpdir(), 'orca-local-log-tail-lifetime-')) +const require = createRequire(import.meta.url) +let runnerModuleId +try { + const runnerPath = join(scratch, 'run-process.cjs') + await build({ + absWorkingDir: root, + entryPoints: [resolve(root, 'src/shared/child-process/run-process.ts')], + outfile: runnerPath, + bundle: true, + platform: 'node', + format: 'cjs', + logLevel: 'silent' + }) + runnerModuleId = require.resolve(runnerPath) + const { runProcess } = require(runnerModuleId) + const baselineConfig = join(scratch, 'before.config.mjs') + const fixedConfig = join(scratch, 'after.config.mjs') + const includes = [ + 'src/main/ipc/local-log-tail-lifetime.test.ts', + 'src/main/ipc/local-log-tail.test.ts' + ] + const configImport = JSON.stringify(pathToFileURL(resolve(root, 'config/vitest.config.ts')).href) + await writeFile( + baselineConfig, + `import base from ${configImport}; +const beforeSources = ${JSON.stringify(beforeSources)}; +export default {...base, test: {...base.test, include: ${JSON.stringify(includes)}}, plugins: [{ + name: 'local-log-lifetime-before-fix', enforce: 'pre', + transform(_code, id) { + const before = beforeSources[id.replaceAll('\\\\', '/').split('?')[0]]; + return before === undefined ? null : {code: before, map: null}; + } +}]};\n` + ) + + await writeFile( + fixedConfig, + `import base from ${configImport};\nexport default {...base, test: {...base.test, include: ${JSON.stringify(includes)}}};\n` + ) + + async function run(label, config) { + const report = join(scratch, `${label}.json`) + const result = await runProcess({ + program: process.execPath, + args: [ + resolve(root, 'node_modules/vitest/vitest.mjs'), + 'run', + '--config', + config, + '--reporter=json', + `--outputFile=${report}` + ], + cwd: root, + env: process.env, + timeoutMs: 90_000, + maxOutputBytes: 4 * 1024 * 1024 + }) + let parsed + try { + parsed = JSON.parse(await readFile(report, 'utf8')) + } catch (error) { + throw new Error(`${label} runner failed: ${result.stderr || result.stdout}`, { cause: error }) + } + return { + exitCode: result.code, + passed: parsed.numPassedTests, + failed: parsed.numFailedTests, + failedCases: parsed.testResults.flatMap((suite) => + suite.assertionResults + .filter((test) => test.status === 'failed') + .map((test) => test.fullName) + ) + } + } + + const before = await run('before', baselineConfig) + const after = await run('after', fixedConfig) + const passed = + before.failed === 10 && before.passed === 9 && after.passed === 19 && after.failed === 0 + console.log( + JSON.stringify( + { + comparison: + 'Actual local log IPC lifecycle tests with real temporary-file watchers and controlled sender events; before reverses only fix.patch in a temporary Vite transform', + sourceHashes, + before, + after, + passed + }, + null, + 2 + ) + ) + if (!passed) { + process.exitCode = 1 + } +} finally { + if (runnerModuleId) { + delete require.cache[runnerModuleId] + } + await rm(scratch, { recursive: true, force: true }) +} diff --git a/docs/audits/local-log-tail-lifetime/results.json b/docs/audits/local-log-tail-lifetime/results.json new file mode 100644 index 00000000000..c4e77d95759 --- /dev/null +++ b/docs/audits/local-log-tail-lifetime/results.json @@ -0,0 +1,39 @@ +{ + "comparison": "Actual local log IPC lifecycle tests with real temporary-file watchers and controlled sender events; before reverses only fix.patch in a temporary Vite transform", + "sourceHashes": { + "src/main/ipc/local-log-tail.ts": { + "before": "6c7b9912fdab5be8b219eacc5000f2e11097219832212ec2f532879222292c02", + "after": "e5db5f0256dd1c2d8d6b42039f2ad5f2cf46faa9e2edeb6f522a962fa58fbf81" + }, + "src/main/ipc/local-log-tail-lifetime.test.ts": { + "current": "2ed1a7f9a1a0ddaf724b429aaa2ec1f9c531dda1c6e82c9884e8d85f25877ef6" + }, + "src/main/ipc/local-log-tail.test.ts": { + "current": "eafa0ccdf60d7adbc14ed9b14ca04c27e775a55c77996e5b2894f448a126637c" + } + }, + "before": { + "exitCode": 1, + "passed": 9, + "failed": 10, + "failedCases": [ + "does not install a watcher after its sender is destroyed during authorization", + "does not revive an existing subscription while a replacement is authorizing at destruction", + "rejects both overlapping same-ID admissions after renderer destruction", + "releases installed and pending watches on render-process-gone and permits a new document owner", + "releases installed and pending watches on did-navigate and permits a new document owner", + "shares lifecycle listeners and releases them when the last watch stops", + "explicit stop invalidates pending authorization without retaining idle listeners", + "late success from an older same-ID request preserves the newer installed watch", + "does not accumulate watchers across twenty destroyed renderer owners", + "local log tail IPC ignores errors from a retired watcher after a same-ID replacement" + ] + }, + "after": { + "exitCode": 0, + "passed": 19, + "failed": 0, + "failedCases": [] + }, + "passed": true +} diff --git a/src/main/ipc/local-log-tail-lifetime.test.ts b/src/main/ipc/local-log-tail-lifetime.test.ts new file mode 100644 index 00000000000..4abf283b9ec --- /dev/null +++ b/src/main/ipc/local-log-tail-lifetime.test.ts @@ -0,0 +1,249 @@ +import { afterEach, beforeEach, expect, it, vi } from 'vitest' +import { EventEmitter } from 'node:events' +import { mkdtemp, rm, writeFile } from 'node:fs/promises' +import { join } from 'node:path' +import { tmpdir } from 'node:os' + +const { handlers, authorize } = vi.hoisted(() => ({ + handlers: new Map unknown>(), + authorize: vi.fn() +})) +vi.mock('electron', () => ({ + ipcMain: { + handle: (name: string, handler: (...args: unknown[]) => unknown) => handlers.set(name, handler) + } +})) +vi.mock('./filesystem-auth', () => ({ resolveAuthorizedPath: authorize })) +import { + closeAllLocalLogTailWatchers, + getActiveLocalLogTailWatcherCount, + registerLocalLogTailHandlers +} from './local-log-tail' + +class Sender extends EventEmitter { + dead = false + send = vi.fn() + constructor(readonly id: number) { + super() + } + isDestroyed() { + return this.dead + } + destroy() { + this.dead = true + this.emit('destroyed') + } +} +let directory = '' +let filePath = '' +beforeEach(async () => { + directory = await mkdtemp(join(tmpdir(), 'orca-log-admission-test-')) + filePath = join(directory, 'fixture.log') + await writeFile(filePath, 'test\n') + authorize.mockReset().mockResolvedValue(filePath) + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: authorization is mocked; the handler never reads Store in this isolated fixture. + registerLocalLogTailHandlers({} as never) +}) +afterEach(async () => { + closeAllLocalLogTailWatchers() + await rm(directory, { force: true, recursive: true }) +}) +function start(sender: Sender, subscriptionId = 'tail') { + return handlers.get('fs:startLocalLogTail')!({ sender }, { filePath, subscriptionId }) +} +function deferAuthorization() { + let resolve!: (path: string) => void + let reject!: (error: Error) => void + const promise = new Promise((onResolve, onReject) => { + resolve = onResolve + reject = onReject + }) + authorize.mockReturnValueOnce(promise) + return { resolve: (path = filePath) => resolve(path), reject: () => reject(new Error('denied')) } +} + +it('does not install a watcher after its sender is destroyed during authorization', async () => { + const sender = new Sender(1) + const admission = deferAuthorization() + const pending = start(sender) + sender.destroy() + admission.resolve() + await pending + expect(getActiveLocalLogTailWatcherCount()).toBe(0) + expect(sender.listenerCount('destroyed')).toBe(0) +}) +it('does not revive an existing subscription while a replacement is authorizing at destruction', async () => { + const sender = new Sender(2) + await start(sender) + const admission = deferAuthorization() + const pending = start(sender) + sender.destroy() + expect(getActiveLocalLogTailWatcherCount()).toBe(0) + admission.resolve() + await pending + expect(getActiveLocalLogTailWatcherCount()).toBe(0) + expect(sender.listenerCount('destroyed')).toBe(0) +}) +it('rejects both overlapping same-ID admissions after renderer destruction', async () => { + const sender = new Sender(3) + const first = deferAuthorization() + const pendingFirst = start(sender) + const second = deferAuthorization() + const pendingSecond = start(sender) + sender.destroy() + second.resolve() + await pendingSecond + first.resolve() + await pendingFirst + expect(getActiveLocalLogTailWatcherCount()).toBe(0) + expect(sender.listenerCount('destroyed')).toBe(0) +}) +it('replaces a live same-ID subscription and keeps one sender cleanup listener', async () => { + const sender = new Sender(4) + await start(sender) + await start(sender) + expect(getActiveLocalLogTailWatcherCount()).toBe(1) + expect(sender.listenerCount('destroyed')).toBe(1) + sender.destroy() + expect(getActiveLocalLogTailWatcherCount()).toBe(0) +}) +it('preserves a live subscription when replacement authorization fails', async () => { + const sender = new Sender(5) + await start(sender) + const admission = deferAuthorization() + const pending = Promise.resolve(start(sender)) + const rejection = expect(pending).rejects.toThrow('denied') + admission.reject() + await rejection + expect(getActiveLocalLogTailWatcherCount()).toBe(1) + sender.destroy() + expect(getActiveLocalLogTailWatcherCount()).toBe(0) +}) +it('a failed older admission cannot remove a newer successful same-ID watch', async () => { + const sender = new Sender(6) + const admission = deferAuthorization() + const pending = Promise.resolve(start(sender)) + const rejection = expect(pending).rejects.toThrow('denied') + await start(sender) + admission.reject() + await rejection + expect(getActiveLocalLogTailWatcherCount()).toBe(1) + sender.destroy() + expect(getActiveLocalLogTailWatcherCount()).toBe(0) +}) + +it.each(['render-process-gone', 'did-navigate'])( + 'releases installed and pending watches on %s and permits a new document owner', + async (event) => { + const sender = new Sender(7) + await start(sender, 'installed') + const admission = deferAuthorization() + const pending = start(sender, 'pending') + sender.emit(event) + expect(getActiveLocalLogTailWatcherCount()).toBe(0) + expect(sender.listenerCount('destroyed')).toBe(0) + await start(sender, 'pending') + admission.resolve() + await pending + expect(getActiveLocalLogTailWatcherCount()).toBe(1) + expect(sender.listenerCount('destroyed')).toBe(1) + sender.destroy() + expect(getActiveLocalLogTailWatcherCount()).toBe(0) + } +) + +it('keeps live watches for same-document and canceled navigation', async () => { + const sender = new Sender(8) + await start(sender) + sender.emit('did-start-navigation', {}, 'https://blocked.example', false, true) + sender.emit('did-navigate-in-page', {}, 'app://index.html#route', true) + expect(getActiveLocalLogTailWatcherCount()).toBe(1) +}) + +it('shares lifecycle listeners and releases them when the last watch stops', async () => { + const sender = new Sender(9) + await Promise.all(Array.from({ length: 20 }, (_, index) => start(sender, `tail-${index}`))) + for (const event of ['destroyed', 'render-process-gone', 'did-navigate']) { + expect(sender.listenerCount(event)).toBe(1) + } + for (let index = 0; index < 20; index++) { + handlers.get('fs:stopLocalLogTail')!({ sender }, { subscriptionId: `tail-${index}` }) + } + expect(getActiveLocalLogTailWatcherCount()).toBe(0) + for (const event of ['destroyed', 'render-process-gone', 'did-navigate']) { + expect(sender.listenerCount(event)).toBe(0) + } +}) + +it('explicit stop invalidates pending authorization without retaining idle listeners', async () => { + const sender = new Sender(10) + const admission = deferAuthorization() + const pending = start(sender) + handlers.get('fs:stopLocalLogTail')!({ sender }, { subscriptionId: 'tail' }) + expect(sender.listenerCount('destroyed')).toBe(0) + admission.resolve() + await pending + expect(getActiveLocalLogTailWatcherCount()).toBe(0) +}) + +it('late success from an older same-ID request preserves the newer installed watch', async () => { + const sender = new Sender(11) + const admission = deferAuthorization() + const pending = start(sender) + await start(sender) + const listeners = sender.rawListeners('destroyed') + admission.resolve(join(directory, 'retired-file-no-longer-exists.log')) + await pending + expect(getActiveLocalLogTailWatcherCount()).toBe(1) + expect(sender.rawListeners('destroyed')).toEqual(listeners) +}) + +it('a failed initial authorization releases all lifecycle listeners', async () => { + const sender = new Sender(12) + authorize.mockRejectedValueOnce(new Error('denied')) + await expect(start(sender)).rejects.toThrow('denied') + expect(getActiveLocalLogTailWatcherCount()).toBe(0) + for (const event of ['destroyed', 'render-process-gone', 'did-navigate']) { + expect(sender.listenerCount(event)).toBe(0) + } +}) + +it('close-all invalidates pending admission and leaves replacement ownership intact', async () => { + const sender = new Sender(13) + const admission = deferAuthorization() + const pending = start(sender) + closeAllLocalLogTailWatchers() + await start(sender) + admission.resolve() + await pending + expect(getActiveLocalLogTailWatcherCount()).toBe(1) + expect(sender.listenerCount('destroyed')).toBe(1) +}) + +it('releases a failed native watcher installation after authorization', async () => { + const sender = new Sender(14) + authorize.mockResolvedValueOnce(join(directory, 'missing.log')) + await expect(start(sender)).rejects.toThrow() + expect(getActiveLocalLogTailWatcherCount()).toBe(0) + for (const event of ['destroyed', 'render-process-gone', 'did-navigate']) { + expect(sender.listenerCount(event)).toBe(0) + } +}) + +it('does not accumulate watchers across twenty destroyed renderer owners', async () => { + let authorizeNow!: (path: string) => void + authorize.mockReturnValue( + new Promise((resolve) => { + authorizeNow = resolve + }) + ) + const senders = Array.from({ length: 20 }, (_, index) => new Sender(index + 20)) + const pending = senders.map((sender) => start(sender)) + for (const sender of senders) { + sender.destroy() + } + authorizeNow(filePath) + await Promise.all(pending) + expect(getActiveLocalLogTailWatcherCount()).toBe(0) + expect(senders.every((sender) => sender.listenerCount('destroyed') === 0)).toBe(true) +}) diff --git a/src/main/ipc/local-log-tail.test.ts b/src/main/ipc/local-log-tail.test.ts index beac464f4a4..5f7f46ab0f5 100644 --- a/src/main/ipc/local-log-tail.test.ts +++ b/src/main/ipc/local-log-tail.test.ts @@ -1,4 +1,5 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { EventEmitter } from 'node:events' const { handlers, watchMock, resolveAuthorizedPathMock, readRangeMock } = vi.hoisted(() => ({ handlers: new Map unknown>(), @@ -49,18 +50,13 @@ function makeWatcher(): FakeWatcher { } function makeSender(id: number) { - let destroyedListener: (() => void) | undefined - return { + const sender = new EventEmitter() + return Object.assign(sender, { id, send: vi.fn(), isDestroyed: vi.fn(() => false), - once: vi.fn((event: string, listener: () => void) => { - if (event === 'destroyed') { - destroyedListener = listener - } - }), - destroy: () => destroyedListener?.() - } + destroy: () => sender.emit('destroyed') + }) } beforeEach(() => { @@ -124,4 +120,20 @@ describe('local log tail IPC', () => { expect(second.close).toHaveBeenCalledTimes(1) expect(getActiveLocalLogTailWatcherCount()).toBe(0) }) + it('ignores errors from a retired watcher after a same-ID replacement', async () => { + const first = makeWatcher() + const second = makeWatcher() + watchMock.mockReturnValueOnce(first).mockReturnValueOnce(second) + const sender = makeSender(10) + const args = { filePath: '/logs/session.jsonl', subscriptionId: 'tail' } + await handlers.get('fs:startLocalLogTail')?.({ sender }, args) + await handlers.get('fs:startLocalLogTail')?.({ sender }, args) + + first.emitError() + + expect(first.close).toHaveBeenCalledTimes(1) + expect(second.close).not.toHaveBeenCalled() + expect(sender.send).not.toHaveBeenCalled() + expect(getActiveLocalLogTailWatcherCount()).toBe(1) + }) }) diff --git a/src/main/ipc/local-log-tail.ts b/src/main/ipc/local-log-tail.ts index 430882b4e07..0892665ad50 100644 --- a/src/main/ipc/local-log-tail.ts +++ b/src/main/ipc/local-log-tail.ts @@ -9,35 +9,83 @@ import type { } from '../../shared/local-log-tail-types' import { readLocalLogTailRange } from '../ai-vault/local-log-tail-reader' import { resolveAuthorizedPath } from './filesystem-auth' +import { abortWhenRendererGone } from './renderer-lifetime-abort' + +type TailSenderOwner = { + senderId: number + pending: Map + watchKeys: Set + signal: AbortSignal + dispose: () => void +} type TailWatch = { - senderId: number + owner: TailSenderOwner watcher: FSWatcher } const tailWatches = new Map() -const senderCleanupRegistered = new Set() +const senderOwners = new Map() function watchKey(senderId: number, subscriptionId: string): string { return `${senderId}:${subscriptionId}` } -function closeWatch(key: string): void { +function releaseIdleOwner(owner: TailSenderOwner): void { + if (owner.pending.size > 0 || owner.watchKeys.size > 0) { + return + } + if (senderOwners.get(owner.senderId) === owner) { + senderOwners.delete(owner.senderId) + } + owner.dispose() +} + +function closeWatch(key: string, expected?: TailWatch): void { const subscription = tailWatches.get(key) - if (!subscription) { + if (!subscription || (expected && subscription !== expected)) { return } tailWatches.delete(key) - subscription.watcher.close() + subscription.owner.watchKeys.delete(key) + try { + subscription.watcher.close() + } finally { + releaseIdleOwner(subscription.owner) + } } -function closeSenderWatches(senderId: number): void { - senderCleanupRegistered.delete(senderId) - for (const [key, subscription] of tailWatches) { - if (subscription.senderId === senderId) { - closeWatch(key) +function closeSenderWatches(owner: TailSenderOwner): void { + owner.pending.clear() + for (const key of owner.watchKeys) { + const subscription = tailWatches.get(key) + if (subscription?.owner === owner) { + closeWatch(key, subscription) } } + releaseIdleOwner(owner) +} + +function getSenderOwner(sender: WebContents): TailSenderOwner { + const existing = senderOwners.get(sender.id) + if (existing) { + return existing + } + const lifetime = abortWhenRendererGone(sender) + const onAbort = (): void => closeSenderWatches(owner) + const owner: TailSenderOwner = { + senderId: sender.id, + pending: new Map(), + watchKeys: new Set(), + signal: lifetime.signal, + dispose: () => { + lifetime.signal.removeEventListener('abort', onAbort) + lifetime.dispose() + } + } + senderOwners.set(sender.id, owner) + lifetime.signal.addEventListener('abort', onAbort, { once: true }) + return owner } function validateSubscriptionId(value: unknown): string { @@ -47,12 +95,52 @@ function validateSubscriptionId(value: unknown): string { return value } -function registerSenderCleanup(sender: WebContents): void { - if (senderCleanupRegistered.has(sender.id)) { +async function startWatch( + sender: WebContents, + args: LocalLogTailWatchArgs, + store: Store +): Promise { + const subscriptionId = validateSubscriptionId(args.subscriptionId) + if (sender.isDestroyed()) { return } - senderCleanupRegistered.add(sender.id) - sender.once('destroyed', () => closeSenderWatches(sender.id)) + const key = watchKey(sender.id, subscriptionId) + const owner = getSenderOwner(sender) + const pending = Symbol(subscriptionId) + owner.pending.set(key, pending) + try { + const filePath = await resolveAuthorizedPath(args.filePath, store) + if ( + sender.isDestroyed() || + owner.signal.aborted || + senderOwners.get(sender.id) !== owner || + owner.pending.get(key) !== pending + ) { + return + } + closeWatch(key) + const sendChange = (eventType: 'change' | 'rename'): void => { + if (tailWatches.get(key) !== subscription || sender.isDestroyed()) { + return + } + const payload: LocalLogTailChangedPayload = { subscriptionId, eventType } + sender.send('fs:localLogTailChanged', payload) + } + const watcher = watch(filePath, (eventType) => sendChange(eventType)) + const subscription: TailWatch = { owner, watcher } + watcher.on('error', () => { + // Rotation needs one final drain before releasing this exact watcher. + sendChange('rename') + closeWatch(key, subscription) + }) + tailWatches.set(key, subscription) + owner.watchKeys.add(key) + } finally { + if (owner.pending.get(key) === pending) { + owner.pending.delete(key) + } + releaseIdleOwner(owner) + } } export function registerLocalLogTailHandlers(store: Store): void { @@ -64,43 +152,25 @@ export function registerLocalLogTailHandlers(store: Store): void { } ) - ipcMain.handle( - 'fs:startLocalLogTail', - async (event, args: LocalLogTailWatchArgs): Promise => { - const subscriptionId = validateSubscriptionId(args.subscriptionId) - const filePath = await resolveAuthorizedPath(args.filePath, store) - const key = watchKey(event.sender.id, subscriptionId) - closeWatch(key) - - const sendChange = (eventType: 'change' | 'rename'): void => { - if (!tailWatches.has(key) || event.sender.isDestroyed()) { - return - } - const payload: LocalLogTailChangedPayload = { subscriptionId, eventType } - event.sender.send('fs:localLogTailChanged', payload) - } - const watcher = watch(filePath, (eventType) => sendChange(eventType)) - watcher.on('error', () => { - // Why: an error commonly accompanies rotation. Signal one final drain so - // the renderer can detect identity change, then release the dead handle. - sendChange('rename') - closeWatch(key) - }) - tailWatches.set(key, { senderId: event.sender.id, watcher }) - registerSenderCleanup(event.sender) - } + ipcMain.handle('fs:startLocalLogTail', (event, args: LocalLogTailWatchArgs): Promise => + startWatch(event.sender, args, store) ) ipcMain.handle('fs:stopLocalLogTail', (event, args: { subscriptionId: string }): void => { - closeWatch(watchKey(event.sender.id, validateSubscriptionId(args.subscriptionId))) + const key = watchKey(event.sender.id, validateSubscriptionId(args.subscriptionId)) + const owner = senderOwners.get(event.sender.id) + owner?.pending.delete(key) + closeWatch(key) + if (owner) { + releaseIdleOwner(owner) + } }) } export function closeAllLocalLogTailWatchers(): void { - for (const key of Array.from(tailWatches.keys())) { - closeWatch(key) + for (const owner of senderOwners.values()) { + closeSenderWatches(owner) } - senderCleanupRegistered.clear() } /** Test-only: verifies tab/window teardown does not retain native watchers. */ diff --git a/src/renderer/src/components/right-sidebar/AiVaultSessionSubagents.test.tsx b/src/renderer/src/components/right-sidebar/AiVaultSessionSubagents.test.tsx index a39b732b28a..821636f8ce7 100644 --- a/src/renderer/src/components/right-sidebar/AiVaultSessionSubagents.test.tsx +++ b/src/renderer/src/components/right-sidebar/AiVaultSessionSubagents.test.tsx @@ -1,7 +1,7 @@ // @vitest-environment happy-dom import type { ComponentProps, JSX } from 'react' -import { act, fireEvent, render } from '@testing-library/react' +import { act, cleanup, fireEvent, render } from '@testing-library/react' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { SubagentExpansionProvider } from './ai-vault-subagent-expansion' import { TooltipProvider } from '@/components/ui/tooltip' @@ -30,7 +30,7 @@ beforeEach(() => { }) afterEach(() => { - document.body.replaceChildren() + cleanup() }) function makeSession(overrides: Partial = {}): AiVaultSession {