From e9c04fb8d95254f0cb29addf669d4a597f8ca9a7 Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Thu, 17 Sep 2026 20:11:55 -0700 Subject: [PATCH] fix(ai-vault): ignore cancellations after request settlement (#20980) Co-authored-by: m4air --- docs/audits/scanner-late-cancel/README.md | 51 +++++ docs/audits/scanner-late-cancel/reproduce.mjs | 94 +++++++++ docs/audits/scanner-late-cancel/results.json | 27 +++ ...ssion-scanner-service-cancellation.test.ts | 187 ++++++++++++++++++ .../ai-vault/session-scanner-service-entry.ts | 3 + 5 files changed, 362 insertions(+) create mode 100644 docs/audits/scanner-late-cancel/README.md create mode 100644 docs/audits/scanner-late-cancel/reproduce.mjs create mode 100644 docs/audits/scanner-late-cancel/results.json create mode 100644 src/main/ai-vault/session-scanner-service-cancellation.test.ts diff --git a/docs/audits/scanner-late-cancel/README.md b/docs/audits/scanner-late-cancel/README.md new file mode 100644 index 00000000000..1d4a9a7044b --- /dev/null +++ b/docs/audits/scanner-late-cancel/README.md @@ -0,0 +1,51 @@ +# AI Vault scanner late cancellation + +The scanner child kept cancellation IDs after their requests had already settled. +Its response can still be in transit when the parent sends a cancellation, so this +does not require an invalid caller. The completed request has already run its +cleanup; nothing remains to delete the newly inserted ID. + +The fix admits cancellation only while the existing `pending` set owns the request. +That set includes both queued and running requests. Their cancellation and cleanup +remain unchanged. No protocol or history-retention policy changes. + +## Proof + +Run from the repository root with dependencies installed: + +```sh +ORCA_BACKGROUND_LAUNCH=1 node --max-old-space-size=128 docs/audits/scanner-late-cancel/reproduce.mjs +``` + +The script loads the checked-out production entry and derives the before version by +removing only the three-line `pending` membership guard in memory. It checks that +the guard occurs exactly once; no historical commit or Git access is needed. +Esbuild strips TypeScript before both versions run in separate VM contexts. Only +imported collaborators are stubbed: the production message handler, request lanes, +sets, and cleanup run. After each synthetic first-prompt request completes, its +matching cancel arrives. + +The script asserts the counts and emits JSON with both source SHA-256 hashes and +Node/platform/heap-limit provenance. [results.json](./results.json) records a run on +Node v26.6.0 with a 128 MiB old-space limit. This measures retained entries, not RSS. + +| Source | Requests/responses | Pending | Controllers | Retained cancel IDs | +| ------ | -----------------: | ------: | ----------: | ------------------: | +| Before | 1,000 / 1,000 | 0 | 0 | 1,000 | +| After | 1,000 / 1,000 | 0 | 0 | 0 | + +The regression test imports the production entry and directly observes its existing +cancellation set through an admitted cancellation. It repeats late cancels after +both successful and failed requests, verifies queued/running cancellation, and +checks shutdown cleanup. No production diagnostics or test-only exports were added. + +```sh +ORCA_BACKGROUND_LAUNCH=1 node node_modules/vitest/vitest.mjs run --config config/vitest.config.ts src/main/ai-vault/session-scanner-service-cancellation.test.ts src/main/ai-vault/session-scanner-service-entry.test.ts src/main/ai-vault/session-scanner-service-client.test.ts +``` + +## Incident scope + +The same unconditional insertion exists in release `v1.4.198`. This retains numeric +IDs in the scanner child, not transcript contents in Electron main. It is a concrete +small leak; it does not explain the reported roughly 26 MB/s main-process growth in +#19768 or establish the cause of #19831's scope-level peak memory measurements. diff --git a/docs/audits/scanner-late-cancel/reproduce.mjs b/docs/audits/scanner-late-cancel/reproduce.mjs new file mode 100644 index 00000000000..e0a0b56a2a3 --- /dev/null +++ b/docs/audits/scanner-late-cancel/reproduce.mjs @@ -0,0 +1,94 @@ +import assert from 'node:assert/strict' +import { createHash } from 'node:crypto' +import { EventEmitter } from 'node:events' +import { readFileSync } from 'node:fs' +import { fileURLToPath } from 'node:url' +import { getHeapStatistics } from 'node:v8' +import { runInNewContext } from 'node:vm' +import { transform } from 'esbuild' + +if (process.env.ORCA_BACKGROUND_LAUNCH !== '1') { + throw new Error('Run with ORCA_BACKGROUND_LAUNCH=1') +} + +const sourcePath = 'src/main/ai-vault/session-scanner-service-entry.ts' +const source = readFileSync(new URL(`../../../${sourcePath}`, import.meta.url), 'utf8') +const guard = ' if (!pending.has(raw.id)) {\n return\n }\n' +const requestCount = 1000 +assert.equal(source.split(guard).length, 2, 'Review changed baseline transform') + +async function run(version) { + const original = version === 'before' ? source.replace(guard, '') : source + const { code } = await transform(original, { loader: 'ts', format: 'esm', target: 'es2022' }) + const entry = code.replace(/^import[\s\S]*?from ['"][^'"]+['"];?\n/gm, '') + assert.equal(/^import\b/m.test(entry), false, 'Unexpected production import shape') + const processStub = new EventEmitter() + let responses = 0 + processStub.send = (message) => { + if (message.type === 'result') { + responses++ + } + } + processStub.pid = 1 + const context = { + process: processStub, + performance, + AbortController, + requestSessionSearchRoots: () => undefined, + SessionScannerServiceSearch: class { + handles() { + return false + } + }, + AI_VAULT_SERVICE_PROTOCOL_VERSION: 1, + aiVaultServiceLane: () => 'interactive', + isAiVaultServiceRequest: (raw) => raw.type === 'request', + readAiVaultFirstUserPrompt: async () => ({ prompt: null }), + inspect: undefined + } + runInNewContext( + `${entry}\ninspect = () => ({ pending: pending.size, controllers: controllers.size, cancelled: cancelled.size })`, + context, + { timeout: 1000, filename: fileURLToPath(new URL(`../../../${sourcePath}`, import.meta.url)) } + ) + try { + processStub.emit('message', { type: 'init', protocol: 1 }) + for (let id = 1; id <= requestCount; id++) { + processStub.emit('message', { + type: 'request', + id, + operation: 'firstPrompt', + request: { agent: 'claude', filePath: '/synthetic' } + }) + await new Promise(setImmediate) + processStub.emit('message', { type: 'cancel', id }) + } + const retained = context.inspect() + assert.equal(responses, requestCount) + assert.equal(retained.pending, 0) + assert.equal(retained.controllers, 0) + assert.equal(retained.cancelled, version === 'before' ? requestCount : 0) + return { + source: version === 'before' ? 'working tree without pending guard' : 'working tree', + sourceSha256: createHash('sha256').update(original).digest('hex'), + requests: requestCount, + responses, + ...retained + } + } finally { + processStub.removeAllListeners() + } +} + +const results = { + node: process.version, + platform: process.platform, + architecture: process.arch, + heapLimitBytes: getHeapStatistics().heap_size_limit, + sourcePath, + baselineTransform: 'Remove only the three-line pending-membership guard in memory', + harness: 'Production entry in isolated VM contexts; imported collaborators stubbed', + before: await run('before'), + after: await run('after') +} +process.stdout.write(`${JSON.stringify(results, null, 2)}\n`) diff --git a/docs/audits/scanner-late-cancel/results.json b/docs/audits/scanner-late-cancel/results.json new file mode 100644 index 00000000000..71df02a9ccd --- /dev/null +++ b/docs/audits/scanner-late-cancel/results.json @@ -0,0 +1,27 @@ +{ + "node": "v26.6.0", + "platform": "darwin", + "architecture": "arm64", + "heapLimitBytes": 234881024, + "sourcePath": "src/main/ai-vault/session-scanner-service-entry.ts", + "baselineTransform": "Remove only the three-line pending-membership guard in memory", + "harness": "Production entry in isolated VM contexts; imported collaborators stubbed", + "before": { + "source": "working tree without pending guard", + "sourceSha256": "846f9af9577dbaf14d05aa5a2eea9e16a44a28ef7993e6b04c43b65db1f1f44f", + "requests": 1000, + "responses": 1000, + "pending": 0, + "controllers": 0, + "cancelled": 1000 + }, + "after": { + "source": "working tree", + "sourceSha256": "e300d4922da94da09abb3c14bbb240ef9593d8a4834a37598c1c2f77b14dcbf5", + "requests": 1000, + "responses": 1000, + "pending": 0, + "controllers": 0, + "cancelled": 0 + } +} diff --git a/src/main/ai-vault/session-scanner-service-cancellation.test.ts b/src/main/ai-vault/session-scanner-service-cancellation.test.ts new file mode 100644 index 00000000000..8863006017e --- /dev/null +++ b/src/main/ai-vault/session-scanner-service-cancellation.test.ts @@ -0,0 +1,187 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { + AiVaultServiceChildMessage, + AiVaultServiceParentMessage +} from './session-scanner-service-protocol' +import { AI_VAULT_SERVICE_PROTOCOL_VERSION } from './session-scanner-service-protocol' + +const scanAiVaultSessions = vi.hoisted(() => vi.fn()) +const flushSessionParseCachePersist = vi.hoisted(() => vi.fn(async () => undefined)) +const closeSearch = vi.hoisted(() => vi.fn()) + +vi.mock('./session-scanner', () => ({ scanAiVaultSessions })) +vi.mock('./session-scanner-parse-cache', () => ({ invalidateSessionParseCacheEntry: vi.fn() })) +vi.mock('./session-parse-cache-persistence', () => ({ + flushSessionParseCachePersist, + initSessionParseCachePersistence: vi.fn() +})) +vi.mock('./session-scanner-service-search', () => ({ + SessionScannerServiceSearch: class { + handles(): boolean { + return false + } + close(): void { + closeSearch() + } + } +})) + +const result = { sessions: [], issues: [], scannedAt: '2026-09-15' } +const sent: AiVaultServiceChildMessage[] = [] +const disconnect = vi.fn() +let restoreProcess = (): void => undefined + +function emit(message: AiVaultServiceParentMessage): void { + process.emit('message', message) +} + +function scan(id: number): void { + emit({ type: 'request', id, operation: 'scan', options: {} }) +} + +function settle(): Promise { + return new Promise((resolve) => setImmediate(resolve)) +} + +function cancelAndObserveSet(id: number): Set { + const add = vi.spyOn(Set.prototype, 'add') + try { + emit({ type: 'cancel', id }) + const index = add.mock.calls.findIndex(([value]) => value === id) + const retained = add.mock.contexts[index] + if (!(retained instanceof Set)) { + throw new Error('Expected an admitted cancellation.') + } + return retained + } finally { + add.mockRestore() + } +} + +beforeEach(async () => { + vi.resetModules() + vi.clearAllMocks() + sent.length = 0 + scanAiVaultSessions.mockResolvedValue(result) + const sendDescriptor = Object.getOwnPropertyDescriptor(process, 'send') + const disconnectDescriptor = Object.getOwnPropertyDescriptor(process, 'disconnect') + const messageListeners = new Set(process.listeners('message')) + const disconnectListeners = new Set(process.listeners('disconnect')) + Object.defineProperty(process, 'send', { + configurable: true, + value: (message: AiVaultServiceChildMessage) => { + sent.push(message) + return true + } + }) + Object.defineProperty(process, 'disconnect', { configurable: true, value: disconnect }) + restoreProcess = () => { + for (const listener of process.listeners('message')) { + if (!messageListeners.has(listener)) { + process.removeListener('message', listener) + } + } + for (const listener of process.listeners('disconnect')) { + if (!disconnectListeners.has(listener)) { + process.removeListener('disconnect', listener) + } + } + if (sendDescriptor) { + Object.defineProperty(process, 'send', sendDescriptor) + } else { + Reflect.deleteProperty(process, 'send') + } + if (disconnectDescriptor) { + Object.defineProperty(process, 'disconnect', disconnectDescriptor) + } else { + Reflect.deleteProperty(process, 'disconnect') + } + } + await import('./session-scanner-service-entry') + emit({ + type: 'init', + protocol: AI_VAULT_SERVICE_PROTOCOL_VERSION, + sessionParseCache: null, + sessionSearch: null + }) +}) + +afterEach(async () => { + emit({ type: 'shutdown' }) + await settle() + restoreProcess() +}) + +describe('AI Vault service cancellation ownership', () => { + it.each(['result', 'error'] as const)( + 'does not retain late cancellations after a %s response', + async (responseType) => { + const first = Promise.withResolvers() + scanAiVaultSessions.mockReturnValueOnce(first.promise) + scan(1) + await settle() + const cancelled = cancelAndObserveSet(1) + first.resolve(result) + await settle() + expect(cancelled.size).toBe(0) + + if (responseType === 'error') { + scanAiVaultSessions.mockRejectedValue(new Error('Synthetic parse failure')) + } + for (let id = 2; id <= 65; id++) { + scan(id) + await settle() + expect(sent).toContainEqual(expect.objectContaining({ type: responseType, id })) + // The parent can cancel while this completed response is still in transit. + emit({ type: 'cancel', id }) + } + emit({ type: 'cancel', id: 999 }) + expect(cancelled.size).toBe(0) + } + ) + + it('cancels running and queued requests and releases both IDs when they settle', async () => { + const first = Promise.withResolvers() + const signals: AbortSignal[] = [] + scanAiVaultSessions.mockImplementation(({ signal }: { signal: AbortSignal }) => { + signals.push(signal) + return signals.length === 1 ? first.promise : Promise.resolve(result) + }) + scan(1) + scan(2) + await settle() + const cancelled = cancelAndObserveSet(1) + emit({ type: 'cancel', id: 2 }) + expect(signals).toHaveLength(1) + expect(signals[0]?.aborted).toBe(true) + expect(cancelled.size).toBe(2) + first.resolve(result) + await settle() + expect(signals).toHaveLength(2) + expect(signals[1]?.aborted).toBe(true) + expect(cancelled.size).toBe(0) + expect(sent.filter((message) => message.type === 'result')).toHaveLength(2) + }) + + it('aborts the running request and closes the service before ignoring later cancels', async () => { + const first = Promise.withResolvers() + let signal: AbortSignal | undefined + scanAiVaultSessions.mockImplementation((options: { signal: AbortSignal }) => { + signal = options.signal + return first.promise + }) + scan(1) + await settle() + const cancelled = cancelAndObserveSet(1) + emit({ type: 'shutdown' }) + expect(signal?.aborted).toBe(true) + first.resolve(result) + await settle() + expect(cancelled.size).toBe(0) + expect(closeSearch).toHaveBeenCalledOnce() + expect(flushSessionParseCachePersist).toHaveBeenCalledOnce() + expect(disconnect).toHaveBeenCalledOnce() + emit({ type: 'cancel', id: 1 }) + expect(cancelled.size).toBe(0) + }) +}) diff --git a/src/main/ai-vault/session-scanner-service-entry.ts b/src/main/ai-vault/session-scanner-service-entry.ts index 7458db74e3a..d1a9da14a61 100644 --- a/src/main/ai-vault/session-scanner-service-entry.ts +++ b/src/main/ai-vault/session-scanner-service-entry.ts @@ -187,6 +187,9 @@ process.on('message', (raw: AiVaultServiceParentMessage) => { return } if (raw?.type === 'cancel') { + if (!pending.has(raw.id)) { + return + } cancelled.add(raw.id) controllers.get(raw.id)?.abort() return