fix(agent): ensure Python env is ready before spawning tasks (ACS-254) (#1061)

* fix(agent): ensure Python env is ready before spawning tasks (ACS-254) Fixes race condition where task creation fails with exit code 127 when Python environment initialization hasn't completed. The issue occurred because AgentManager.startSpecCreation() and startTaskExecution() spawned Python processes without ensuring pythonEnvManager.isEnvReady() was true. This caused getPythonPath() to fall back to findPythonCommand() which could return an invalid path during the async initialization window. Changes: - Add pythonEnvManager import to agent-manager.ts - Add ensurePythonEnvReady() private method (mirrors agent-queue.ts pattern) - Call ensurePythonEnvReady() in startSpecCreation() before spawning - Call ensurePythonEnvReady() in startTaskExecution() before spawning The fix ensures that if a task is started before Python venv is ready, the task will wait for initialization to complete rather than failing with "command not found" (exit code 127). Refs: https://linear.app/stillknotknown/issue/ACS-254 * refactor(agent): extract shared ensurePythonEnvReady to AgentProcessManager Address PR review feedback about code duplication between AgentManager and AgentQueueManager.ensurePythonEnvReady(). Changes: - Add AgentProcessManager.ensurePythonEnvReady() as shared method - Remove duplicated private method from AgentManager - Update AgentManager to use processManager.ensurePythonEnvReady() - Simplify AgentQueueManager.ensurePythonEnvReady() to delegate to shared method - Add unit tests for ensurePythonEnvReady covering all scenarios The shared method returns { ready: boolean; error?: string } to allow callers to handle error emission in their own way (AgentManager emits 'error' event, AgentQueueManager emits specific event types). Test coverage added for: - Python environment already ready (no initialization needed) - Python environment not ready (initializes successfully) - autoBuildSource not found (returns error) - Python initialization fails with error message - Python initialization fails without error message Reduces code duplication by ~55 lines while maintaining same behavior. Refs: https://linear.app/stillknotknown/issue/ACS-254 * refactor(agent): add Python env check to startQAProcess for consistency Address CodeRabbit review suggestion to add ensurePythonEnvReady check to startQAProcess, providing consistent protection against the race condition for all Python process spawning methods. Now all three process-spawning methods in AgentManager have the check: - startSpecCreation - startTaskExecution - startQAProcess (newly added) This prevents edge-case failures where QA might be triggered before Python environment initialization completes. Refs: https://linear.app/stillknotknown/issue/ACS-254 * test: fix pythonEnvManager mock to include getPythonEnv method The test mock was missing the getPythonEnv() method that spawnProcess() calls, causing 14 test failures. Added getPythonEnv mock returning empty object to match production usage. Signed-off-by: StillKnotKnown <stillknotknown@users.noreply.github.com> * test: add python-env-manager mock for integration tests Integration tests were timing out because python-env-manager wasn't mocked. The ensurePythonEnvReady changes added calls to pythonEnvManager.isEnvReady() and getPythonEnv() which weren't mocked in the integration test suite. Fixes 13 timeout failures in subprocess-spawn.test.ts Signed-off-by: StillKnotKnown <stillknotknown@users.noreply.github.com> --------- Signed-off-by: StillKnotKnown <stillknotknown@users.noreply.github.com> Co-authored-by: StillKnotKnown <stillknotknown@users.noreply.github.com>
2026-01-14 21:18:55 +02:00
parent 71a9fc848f
commit 7dc54f235a
5 changed files with 198 additions and 17 deletions
@@ -58,6 +58,16 @@ vi.mock('../../main/python-detector', async (importOriginal) => {
  };
 });

+// Mock python-env-manager for ensurePythonEnvReady (ACS-254)
+vi.mock('../../main/python-env-manager', () => ({
+  pythonEnvManager: {
+    isEnvReady: vi.fn(() => true),
+    initialize: vi.fn(() => Promise.resolve({ ready: true })),
+    getPythonEnv: vi.fn(() => ({}))
+  },
+  getConfiguredPythonPath: vi.fn(() => DETECTED_PYTHON_CMD)
+}));
+
 // Auto-claude source path (for getAutoBuildSourcePath to find)
 const AUTO_CLAUDE_SOURCE = path.join(TEST_DIR, 'auto-claude-source');

@@ -110,6 +110,13 @@ export class AgentManager extends EventEmitter {
      return;
    }

+    // Ensure Python environment is ready before spawning process (prevents exit code 127 race condition)
+    const pythonStatus = await this.processManager.ensurePythonEnvReady('AgentManager');
+    if (!pythonStatus.ready) {
+      this.emit('error', taskId, `Python environment not ready: ${pythonStatus.error || 'initialization failed'}`);
+      return;
+    }
+
    const autoBuildSource = this.processManager.getAutoBuildSourcePath();

    if (!autoBuildSource) {
@@ -196,6 +203,13 @@ export class AgentManager extends EventEmitter {
      return;
    }

+    // Ensure Python environment is ready before spawning process (prevents exit code 127 race condition)
+    const pythonStatus = await this.processManager.ensurePythonEnvReady('AgentManager');
+    if (!pythonStatus.ready) {
+      this.emit('error', taskId, `Python environment not ready: ${pythonStatus.error || 'initialization failed'}`);
+      return;
+    }
+
    const autoBuildSource = this.processManager.getAutoBuildSourcePath();

    if (!autoBuildSource) {
@@ -250,6 +264,13 @@ export class AgentManager extends EventEmitter {
    projectPath: string,
    specId: string
  ): Promise<void> {
+    // Ensure Python environment is ready before spawning process (prevents exit code 127 race condition)
+    const pythonStatus = await this.processManager.ensurePythonEnvReady('AgentManager');
+    if (!pythonStatus.ready) {
+      this.emit('error', taskId, `Python environment not ready: ${pythonStatus.error || 'initialization failed'}`);
+      return;
+    }
+
    const autoBuildSource = this.processManager.getAutoBuildSourcePath();

    if (!autoBuildSource) {
@@ -95,18 +95,46 @@ vi.mock('../python-detector', () => ({
  parsePythonCommand: vi.fn(() => ['python', []])
 }));

+// Mock python-env-manager for ensurePythonEnvReady tests
+vi.mock('../python-env-manager', () => ({
+  pythonEnvManager: {
+    isEnvReady: vi.fn(() => true),
+    initialize: vi.fn(() => Promise.resolve({ ready: true })),
+    getPythonEnv: vi.fn(() => ({}))
+  },
+  getConfiguredPythonPath: vi.fn(() => 'python3')
+}));
+
 vi.mock('electron', () => ({
  app: {
    getAppPath: vi.fn(() => '/fake/app/path')
  }
 }));

+// Mock fs.existsSync for getAutoBuildSourcePath path validation
+vi.mock('fs', async (importOriginal) => {
+  const actual = await importOriginal<typeof import('fs')>();
+  return {
+    ...actual,
+    existsSync: vi.fn((path: string) => {
+      // Return true for the fake auto-build path and its expected files
+      if (path === '/fake/auto-build' ||
+          path === '/fake/auto-build/runners' ||
+          path === '/fake/auto-build/runners/spec_runner.py') {
+        return true;
+      }
+      return false;
+    })
+  };
+});
+
 // Import AFTER all mocks are set up
 import { AgentProcessManager } from './agent-process';
 import { AgentState } from './agent-state';
 import { AgentEvents } from './agent-events';
 import * as profileService from '../services/profile';
 import * as rateLimitDetector from '../rate-limit-detector';
+import { pythonEnvManager } from '../python-env-manager';

 describe('AgentProcessManager - API Profile Env Injection (Story 2.3)', () => {
  let processManager: AgentProcessManager;
@@ -491,4 +519,105 @@ describe('AgentProcessManager - API Profile Env Injection (Story 2.3)', () => {
      expect(envArg.ANTHROPIC_BASE_URL).toBe('');
    });
  });
+
+  describe('ensurePythonEnvReady - Python Environment Readiness (ACS-254)', () => {
+    let testProcessManager: AgentProcessManager;
+
+    beforeEach(() => {
+      // Reset all mocks
+      vi.clearAllMocks();
+      spawnCalls.length = 0;
+
+      // Create fresh process manager for these tests
+      state = new AgentState();
+      events = new AgentEvents();
+      emitter = new EventEmitter();
+      testProcessManager = new AgentProcessManager(state, events, emitter);
+    });
+
+    it('should return ready: true when Python environment is already ready', async () => {
+      vi.mocked(pythonEnvManager.isEnvReady).mockReturnValue(true);
+
+      // Configure with valid autoBuildSource
+      testProcessManager.configure(undefined, '/fake/auto-build');
+
+      const result = await testProcessManager.ensurePythonEnvReady('TestContext');
+
+      expect(result.ready).toBe(true);
+      expect(result.error).toBeUndefined();
+      expect(pythonEnvManager.initialize).not.toHaveBeenCalled();
+    });
+
+    it('should initialize Python environment when not ready', async () => {
+      vi.mocked(pythonEnvManager.isEnvReady).mockReturnValue(false);
+      vi.mocked(pythonEnvManager.initialize).mockResolvedValue({
+        ready: true,
+        pythonPath: '/fake/python',
+        sitePackagesPath: '/fake/site-packages',
+        venvExists: true,
+        depsInstalled: true,
+        usingBundledPackages: false
+      });
+
+      testProcessManager.configure(undefined, '/fake/auto-build');
+
+      const result = await testProcessManager.ensurePythonEnvReady('TestContext');
+
+      expect(result.ready).toBe(true);
+      expect(result.error).toBeUndefined();
+      expect(pythonEnvManager.initialize).toHaveBeenCalledWith('/fake/auto-build');
+    });
+
+    it('should return error when autoBuildSource is not found', async () => {
+      vi.mocked(pythonEnvManager.isEnvReady).mockReturnValue(false);
+
+      // Don't configure - autoBuildSource will be null
+      const result = await testProcessManager.ensurePythonEnvReady('TestContext');
+
+      expect(result.ready).toBe(false);
+      expect(result.error).toBe('auto-build source not found');
+      expect(pythonEnvManager.initialize).not.toHaveBeenCalled();
+    });
+
+    it('should return error when Python initialization fails', async () => {
+      vi.mocked(pythonEnvManager.isEnvReady).mockReturnValue(false);
+      vi.mocked(pythonEnvManager.initialize).mockResolvedValue({
+        ready: false,
+        pythonPath: null,
+        sitePackagesPath: null,
+        venvExists: false,
+        depsInstalled: false,
+        usingBundledPackages: false,
+        error: 'Failed to create venv: permission denied'
+      });
+
+      testProcessManager.configure(undefined, '/fake/auto-build');
+
+      const result = await testProcessManager.ensurePythonEnvReady('TestContext');
+
+      expect(result.ready).toBe(false);
+      expect(result.error).toBe('Failed to create venv: permission denied');
+    });
+
+    it('should return error when Python initialization fails without message', async () => {
+      vi.mocked(pythonEnvManager.isEnvReady).mockReturnValue(false);
+      vi.mocked(pythonEnvManager.initialize).mockResolvedValue({
+        ready: false,
+        pythonPath: null,
+        sitePackagesPath: null,
+        venvExists: false,
+        depsInstalled: false,
+        usingBundledPackages: false
+        // No error field
+      });
+
+      testProcessManager.configure(undefined, '/fake/auto-build');
+
+      const result = await testProcessManager.ensurePythonEnvReady('TestContext');
+
+      expect(result.ready).toBe(false);
+      expect(result.error).toBe('initialization failed');
+      expect(pythonEnvManager.initialize).toHaveBeenCalledWith('/fake/auto-build');
+    });
+  });
 });
@@ -301,6 +301,38 @@ export class AgentProcessManager {
    return null;
  }

+  /**
+   * Ensure Python environment is ready before spawning processes.
+   * This is a shared method used by AgentManager and AgentQueueManager
+   * to prevent race conditions where tasks start before venv initialization completes.
+   *
+   * @param context - Context identifier for logging (e.g., 'AgentManager', 'AgentQueue')
+   * @returns Object with ready status and optional error message
+   */
+  async ensurePythonEnvReady(context: string): Promise<{ ready: boolean; error?: string }> {
+    if (pythonEnvManager.isEnvReady()) {
+      return { ready: true };
+    }
+
+    console.log(`[${context}] Python environment not ready, waiting for initialization...`);
+
+    const autoBuildSource = this.getAutoBuildSourcePath();
+    if (!autoBuildSource) {
+      const error = 'auto-build source not found';
+      console.error(`[${context}] Cannot initialize Python - ${error}`);
+      return { ready: false, error };
+    }
+
+    const status = await pythonEnvManager.initialize(autoBuildSource);
+    if (!status.ready) {
+      console.error(`[${context}] Python environment initialization failed:`, status.error);
+      return { ready: false, error: status.error || 'initialization failed' };
+    }
+
+    console.log(`[${context}] Python environment now ready`);
+    return { ready: true };
+  }
+
  /**
   * Get project-specific environment variables based on project settings
   */
@@ -59,6 +59,8 @@ export class AgentQueueManager {
   * Prevents the race condition where generation starts before dependencies are installed,
   * which would cause it to fall back to system Python and fail with ModuleNotFoundError.
   *
+   * Delegates to AgentProcessManager.ensurePythonEnvReady() for the actual initialization.
+   *
   * @param projectId - The project ID for error event emission
   * @param eventType - The error event type to emit on failure
   * @returns true if environment is ready, false if initialization failed (error already emitted)
@@ -67,23 +69,10 @@ export class AgentQueueManager {
    projectId: string,
    eventType: 'ideation-error' | 'roadmap-error'
  ): Promise<boolean> {
-    const autoBuildSource = this.processManager.getAutoBuildSourcePath();
-
-    if (!pythonEnvManager.isEnvReady()) {
-      debugLog('[Agent Queue] Python environment not ready, waiting for initialization...');
-      if (autoBuildSource) {
-        const status = await pythonEnvManager.initialize(autoBuildSource);
-        if (!status.ready) {
-          debugError('[Agent Queue] Python environment initialization failed:', status.error);
-          this.emitter.emit(eventType, projectId, `Python environment not ready: ${status.error || 'initialization failed'}`);
-          return false;
-        }
-        debugLog('[Agent Queue] Python environment now ready');
-      } else {
-        debugError('[Agent Queue] Cannot initialize Python - auto-build source not found');
-        this.emitter.emit(eventType, projectId, 'Python environment not ready: auto-build source not found');
-        return false;
-      }
+    const status = await this.processManager.ensurePythonEnvReady('AgentQueue');
+    if (!status.ready) {
+      this.emitter.emit(eventType, projectId, `Python environment not ready: ${status.error || 'initialization failed'}`);
+      return false;
    }
    return true;
  }