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 <stillknotknown@users.noreply.github.com> Co-authored-by: Andy <119136210+AndyMik90@users.noreply.github.com>
2026-01-26 13:48:09 +02:00
parent 988ec0c25b
commit f6b264d562
4 changed files with 88 additions and 16 deletions
@@ -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);

@@ -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();
@@ -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<AgentProcess>): void {
+    const existing = this.processes.get(taskId);
+    if (existing) {
+      this.processes.set(taskId, { ...existing, ...updates });
+    }
+  }
+
  /**
   * Get all processes
   */
@@ -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