From f6b264d5626adfff47294edc5787916f57d88179 Mon Sep 17 00:00:00 2001 From: StillKnotKnown <192589389+StillKnotKnown@users.noreply.github.com> Date: Mon, 26 Jan 2026 13:48:09 +0200 Subject: [PATCH] fix: resolve flaky subprocess-spawn test on Windows CI (ACS-392) (#1494) * fix: resolve flaky subprocess-spawn test on Windows CI (ACS-392) The test "should track running tasks" was failing intermittently on Windows CI because vi.waitFor() would timeout before tasks were added to tracking. The root cause was that state.addProcess() was called AFTER async operations (profile init, Python env ready, API profile env) which could be slower on Windows CI. Changes: - Move state.addProcess() to execute immediately at spawnProcess() start, before async operations - Update AgentProcess.process type to ChildProcess | null to handle the initialization state - Add updateProcess() method to set the actual ChildProcess after spawn() - Add null checks in killProcess() and killAllProcesses() for the async setup window - Add setImmediate() wait in test to ensure exit handlers are attached Refs: ACS-392 * fix: address PR review feedback - orphan process prevention and error handling Addresses feedback from Gemini Code Assist and CodeRabbitAI: 1. Add check if task was killed during async setup - if spawn() completes after killProcess() was called, the new childProcess will be terminated to prevent orphaned processes (Gemini HIGH) 2. Add try/catch around spawn() to handle synchronous failures (command not found, permission denied). Without this, the tracking entry with process: null would remain orphaned indefinitely (CodeRabbitAI HIGH) 3. Update misleading comments in killProcess() and killAllProcesses() to reflect actual behavior - spawnProcess() checks and terminates the process if it was killed during setup, rather than being "abandoned" (Gemini MEDIUM) * fix: use wasSpawnKilled() check instead of hasProcess() for orphan prevention Per Auto Claude PR review feedback: - Use wasSpawnKilled() check AFTER updateProcess() instead of hasProcess() check, because killProcess() marks the spawn as killed before deleting tracking - Update comments to accurately reflect that spawn() still executes but the spawned process will be terminated by the post-spawn wasSpawnKilled() check * docs: add comments explaining race condition handling mechanisms Per Auto Claude follow-up review LOW issues: - Add detailed comment explaining why the `?? spawnId` fallback is critical for handling the race condition when killProcess() is called during async setup - Add JSDoc comment to updateProcess() explaining why it silently ignores non-existent taskIds (intentional for race condition handling) --------- Co-authored-by: StillKnotKnown Co-authored-by: Andy <119136210+AndyMik90@users.noreply.github.com> --- .../integration/subprocess-spawn.test.ts | 3 + apps/frontend/src/main/agent/agent-process.ts | 85 +++++++++++++++---- apps/frontend/src/main/agent/agent-state.ts | 14 +++ apps/frontend/src/main/agent/types.ts | 2 +- 4 files changed, 88 insertions(+), 16 deletions(-) diff --git a/apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts b/apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts index 86e9c6e7..fe0f7808 100644 --- a/apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts +++ b/apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts @@ -395,6 +395,9 @@ describe('Subprocess Spawn Integration', () => { expect(manager.getRunningTasks()).toHaveLength(2); }, { timeout: 5000 }); + // Wait for spawn to complete (ensures exit handlers are attached) + await new Promise(resolve => setImmediate(resolve)); + // Both tasks share the same mock process, so emit exit once triggers both handlers mockProcess.emit('exit', 0); diff --git a/apps/frontend/src/main/agent/agent-process.ts b/apps/frontend/src/main/agent/agent-process.ts index 5bdac535..d1491bd5 100644 --- a/apps/frontend/src/main/agent/agent-process.ts +++ b/apps/frontend/src/main/agent/agent-process.ts @@ -512,6 +512,18 @@ export class AgentProcessManager { this.killProcess(taskId); const spawnId = this.state.generateSpawnId(); + + // IMPORTANT: Add to tracking IMMEDIATELY, before async operations. + // This ensures getRunningTasks() returns the task right away, preventing + // flaky tests on slower Windows CI where async setup may take longer than + // vi.waitFor timeout (ACS-392). + this.state.addProcess(taskId, { + taskId, + process: null, // Will be set after spawn() call completes below + startedAt: new Date(), + spawnId + }); + const env = this.setupProcessEnvironment(extraEnv); // Get Python environment (PYTHONPATH for bundled packages, etc.) @@ -531,22 +543,48 @@ export class AgentProcessManager { // Parse Python commandto handle space-separated commands like "py -3" const [pythonCommand, pythonBaseArgs] = parsePythonCommand(this.getPythonPath()); - const childProcess = spawn(pythonCommand, [...pythonBaseArgs, ...args], { - cwd, - env: { - ...env, // Already includes process.env, extraEnv, profileEnv, PYTHONUNBUFFERED, PYTHONUTF8 - ...pythonEnv, // Include Python environment (PYTHONPATH for bundled packages) - ...oauthModeClearVars, // Clear stale ANTHROPIC_* vars when in OAuth mode - ...apiProfileEnv // Include active API profile config (highest priority for ANTHROPIC_* vars) - } - }); + let childProcess; + try { + childProcess = spawn(pythonCommand, [...pythonBaseArgs, ...args], { + cwd, + env: { + ...env, // Already includes process.env, extraEnv, profileEnv, PYTHONUNBUFFERED, PYTHONUTF8 + ...pythonEnv, // Include Python environment (PYTHONPATH for bundled packages) + ...oauthModeClearVars, // Clear stale ANTHROPIC_* vars when in OAuth mode + ...apiProfileEnv // Include active API profile config (highest priority for ANTHROPIC_* vars) + } + }); + } catch (err) { + // spawn() failed synchronously (e.g., command not found, permission denied) + // Clean up tracking entry and propagate error + this.state.deleteProcess(taskId); + this.emitter.emit('error', taskId, err instanceof Error ? err.message : String(err)); + throw err; + } - this.state.addProcess(taskId, { - taskId, - process: childProcess, - startedAt: new Date(), - spawnId - }); + // Update the tracked process with the actual spawned ChildProcess + this.state.updateProcess(taskId, { process: childProcess }); + + // Check if this spawn was killed during async setup (before spawn() completed). + // If so, terminate the newly created process immediately to prevent orphaned processes. + // Note: wasSpawnKilled() is checked AFTER updateProcess() because killProcess() + // marks the spawn as killed before deleting the tracking entry. + // + // CRITICAL: The `?? spawnId` fallback is essential here because if killProcess() + // was called during the async setup window, the taskId entry may have been deleted + // from the process map. In that case, getProcess(taskId) returns undefined, so we + // fall back to the local spawnId variable to check if this specific spawn was killed. + const currentSpawnId = this.state.getProcess(taskId)?.spawnId ?? spawnId; + if (this.state.wasSpawnKilled(currentSpawnId)) { + console.log(`[AgentProcess] Task ${taskId} was killed during spawn setup. Terminating newly created process.`); + killProcessGracefully(childProcess, { + debugPrefix: '[AgentProcess]', + debug: process.env.DEBUG === 'true' || process.env.NODE_ENV === 'development' + }); + this.state.deleteProcess(taskId); + this.state.clearKilledSpawn(currentSpawnId); + return; // Do not proceed with this spawn + } let currentPhase: ExecutionProgressData['phase'] = isSpecRunner ? 'planning' : 'planning'; let phaseProgress = 0; @@ -743,6 +781,14 @@ export class AgentProcessManager { // Mark this specific spawn as killed so its exit handler knows to ignore this.state.markSpawnAsKilled(agentProcess.spawnId); + // If process hasn't been spawned yet (still in async setup phase, before spawn() returns), + // just remove from tracking. The spawn() call will still complete, but the spawned process + // will be terminated by the post-spawn wasSpawnKilled() check (see spawnProcess() after updateProcess). + if (!agentProcess.process) { + this.state.deleteProcess(taskId); + return true; + } + // Use shared platform-aware kill utility killProcessGracefully(agentProcess.process, { debugPrefix: '[AgentProcess]', @@ -768,6 +814,15 @@ export class AgentProcessManager { return; } + // If process hasn't been spawned yet (still in async setup phase before spawn() returns), + // just resolve immediately. The spawn() call will still complete, but the spawned process + // will be terminated by the post-spawn wasSpawnKilled() check (see spawnProcess() after updateProcess). + if (!agentProcess.process) { + this.killProcess(taskId); + resolve(); + return; + } + // Set up timeout to not block forever const timeoutId = setTimeout(() => { resolve(); diff --git a/apps/frontend/src/main/agent/agent-state.ts b/apps/frontend/src/main/agent/agent-state.ts index 9c571679..379d95f0 100644 --- a/apps/frontend/src/main/agent/agent-state.ts +++ b/apps/frontend/src/main/agent/agent-state.ts @@ -84,6 +84,20 @@ export class AgentState { this.killedSpawnIds.delete(spawnId); } + /** + * Update a process's properties (e.g., after spawn completes) + * + * Note: Silently ignores updates if taskId doesn't exist. This is intentional for + * race condition handling in spawnProcess() - if a task is killed during async setup, + * the tracking entry is deleted before spawn() completes, and we don't want to fail here. + */ + updateProcess(taskId: string, updates: Partial): void { + const existing = this.processes.get(taskId); + if (existing) { + this.processes.set(taskId, { ...existing, ...updates }); + } + } + /** * Get all processes */ diff --git a/apps/frontend/src/main/agent/types.ts b/apps/frontend/src/main/agent/types.ts index f409db34..640043d9 100644 --- a/apps/frontend/src/main/agent/types.ts +++ b/apps/frontend/src/main/agent/types.ts @@ -10,7 +10,7 @@ export type QueueProcessType = 'ideation' | 'roadmap'; export interface AgentProcess { taskId: string; - process: ChildProcess; + process: ChildProcess | null; // null during async spawn setup before ChildProcess is created startedAt: Date; projectPath?: string; // For ideation processes to load session on completion spawnId: number; // Unique ID to identify this specific spawn