Browse Source

fix(companion): store child PID and guard onExit cleanup

Greptile review caught two bugs in the PID singleton guard:

1. Wrong PID: was writing process.pid (parent) instead of child.pid
   (companion binary). Companion crashes went undetected.

2. Unconditional onExit cleanup: every instance deleted the PID file
   on exit, even passengers that skipped spawning. A short-lived
   passenger could clear the guard and allow duplicate companions.

Fix: write child.pid after spawn, track wasSpawner flag, only delete
PID file in onExit when this manager was the spawner.
Michael Henke 1 month ago
parent
commit
c93374357f
2 changed files with 24 additions and 4 deletions
  1. 20 2
      src/companion/manager.test.ts
  2. 4 2
      src/companion/manager.ts

+ 20 - 2
src/companion/manager.test.ts

@@ -473,18 +473,36 @@ describe('CompanionManager', () => {
     expect(state.sessions[0].session_id).toBe('test-no-pid');
   });
 
-  it('cleans up PID file on exit', () => {
+  it('cleans up PID file on exit when this manager was the spawner', () => {
     mkdirSync(path.dirname(stateFilePath()), { recursive: true });
     const pidFile = path.join(path.dirname(stateFilePath()), 'companion.pid');
-    writeFileSync(pidFile, String(process.pid));
+    writeFileSync(pidFile, '999999999'); // stale PID so spawn proceeds
 
     const m = make('test-pid-cleanup');
+    // Simulate a spawner by writing a PID file as if spawn succeeded.
+    // In reality the binary doesn't exist so spawn fails before writing,
+    // but the cleanup logic only fires when wasSpawner is true.
+    writeFileSync(pidFile, String(process.pid));
+    (m as unknown as { wasSpawner: boolean }).wasSpawner = true;
     m.onLoad();
     m.onExit();
 
     expect(existsSync(pidFile)).toBe(false);
   });
 
+  it('does not delete PID file on exit when this manager was not the spawner', () => {
+    mkdirSync(path.dirname(stateFilePath()), { recursive: true });
+    const pidFile = path.join(path.dirname(stateFilePath()), 'companion.pid');
+    writeFileSync(pidFile, String(process.pid));
+
+    const m = make('test-pid-no-cleanup');
+    m.onLoad(); // skips spawn because PID is alive, wasSpawner stays false
+    m.onExit();
+
+    // Non-spawner must not delete the guard file
+    expect(existsSync(pidFile)).toBe(true);
+  });
+
   it('removes disabled session entries on load', () => {
     mkdirSync(path.dirname(stateFilePath()), { recursive: true });
     writeFileSync(

+ 4 - 2
src/companion/manager.ts

@@ -178,6 +178,7 @@ export class CompanionManager {
   private readonly busyAgentSessions = new Map<string, string>();
   private readonly config?: CompanionConfig;
   private companionProcess: ChildProcess | null = null;
+  private wasSpawner = false;
 
   constructor(sessionId: string, cwd: string, config?: CompanionConfig) {
     this.id = sessionId;
@@ -284,13 +285,13 @@ export class CompanionManager {
   }
 
   onExit(): void {
+    activeManagers.delete(this);
     if (this.wasSpawner) {
       try {
         const pf = pidFilePath();
         if (existsSync(pf)) rmSync(pf, { force: true });
       } catch {}
     }
-    activeManagers.delete(this);
     if (this.companionProcess) {
       try {
         this.companionProcess.kill();
@@ -404,6 +405,7 @@ export class CompanionManager {
       });
       this.companionProcess = child;
       child.unref();
+      this.wasSpawner = true;
       log(
         '[companion] spawned',
         JSON.stringify({
@@ -414,7 +416,7 @@ export class CompanionManager {
       );
       try {
         mkdirSync(path.dirname(pidFile), { recursive: true });
-        writeFileSync(pidFile, String(process.pid));
+        writeFileSync(pidFile, String(child.pid));
       } catch {}
     } catch (err) {
       log('[companion] spawn failed', String(err));