From fa9a3cc52f87d9d43c62ef33e60fbc3769d3744f Mon Sep 17 00:00:00 2001
From: gsxdsm <gsxdsm@users.noreply.github.com>
Date: Sun, 21 Jun 2026 03:32:53 -0700
Subject: [PATCH] Address PR review feedback (#1704): clear stale external
 marker on acquire

Move externalEngines.delete(projectId) to immediately after acquiring the
singleton lock instead of after engine.start() succeeds. If a project was
marked external, the holder exits, acquire succeeds, but start() then throws,
the success-path delete never ran and hasRunningEngine() reported a phantom
engine forever. Added a regression test for the failed-takeover path.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---
 .../__tests__/project-engine-manager.test.ts  | 34 +++++++++++++++++++
 packages/engine/src/project-engine-manager.ts |  7 ++--
 2 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/packages/engine/src/__tests__/project-engine-manager.test.ts b/packages/engine/src/__tests__/project-engine-manager.test.ts
index 4dbdea6991..b533a90eea 100644
--- a/packages/engine/src/__tests__/project-engine-manager.test.ts
+++ b/packages/engine/src/__tests__/project-engine-manager.test.ts
@@ -830,5 +830,39 @@ describe("ProjectEngineManager", () => {
       warnSpy.mockRestore();
       vi.useRealTimers();
     });
+
+    it("clears the external marker even if the takeover start fails", async () => {
+      // External owner first, then it exits so acquire succeeds — but
+      // engine.start() throws. The marker must be cleared (at acquire time) so
+      // hasRunningEngine() does not report a phantom engine that never started.
+      const manager = new ProjectEngineManager(centralCore);
+      const acquire = acquireEngineSingleton as ReturnType<typeof vi.fn>;
+
+      acquire.mockRejectedValueOnce(
+        new EngineAlreadyRunningError("proj_aaa", "socket"),
+      );
+      await expect(manager.ensureEngine("proj_aaa")).rejects.toBeInstanceOf(
+        EngineAlreadyRunningError,
+      );
+      expect(manager.getExternalEngineIds().has("proj_aaa")).toBe(true);
+
+      // Owner exits: acquire succeeds (default mock), but the engine fails to start.
+      (ProjectEngine as unknown as ReturnType<typeof vi.fn>).mockImplementationOnce(
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        function (config: any) {
+          return {
+            start: vi.fn().mockRejectedValue(new Error("boom")),
+            stop: vi.fn().mockResolvedValue(undefined),
+            getTaskStore: vi.fn().mockReturnValue({ projectId: config.projectId }),
+            _config: config,
+          };
+        },
+      );
+      await expect(manager.ensureEngine("proj_aaa")).rejects.toThrow("boom");
+
+      expect(manager.getExternalEngineIds().has("proj_aaa")).toBe(false);
+      expect(manager.getEngine("proj_aaa")).toBeUndefined();
+      expect(manager.hasRunningEngine()).toBe(false);
+    });
   });
 });
diff --git a/packages/engine/src/project-engine-manager.ts b/packages/engine/src/project-engine-manager.ts
index 9c51c6580e..f862a46403 100644
--- a/packages/engine/src/project-engine-manager.ts
+++ b/packages/engine/src/project-engine-manager.ts
@@ -479,6 +479,11 @@ export class ProjectEngineManager {
       throw err;
     });
     this.singletonLocks.set(projectId, singleton);
+    // Acquiring the singleton proves no other process owns the engine, so clear
+    // any prior "owned by another process" marker now — before engine.start().
+    // If start fails below we release the lock and a later tick retries; leaving
+    // the marker set here would make hasRunningEngine() report a phantom engine.
+    this.externalEngines.delete(projectId);
 
     const engine = new ProjectEngine(
       runtimeConfig,
@@ -497,8 +502,6 @@ export class ProjectEngineManager {
 
     this.engines.set(projectId, engine);
     this.starting.delete(projectId);
-    // We now own the engine — clear any prior "owned by another process" marker.
-    this.externalEngines.delete(projectId);
     runtimeLog.log(
       `Started engine for ${project.name ?? projectId} (${projectId})`,
     );