fix(hooks): stop existing Gmail watcher before re-entry to prevent leaks

renewInterval is not cleared on re-entry to startGmailWatcher,
leaking the previous timer. Each config reload adds another
interval that fires independently.

Clear existing watcher state before starting a new one.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>

src/hooks/gmail-watcher.test.ts

@@ -197,4 +197,164 @@ describe("startGmailWatcher", () => {
     });
     expect(mocks.spawn).not.toHaveBeenCalled();
   });
+
+  it("kills existing watcher process on re-entry before spawning new one", async () => {
+    mocks.runCommandWithTimeout.mockResolvedValue({ code: 0, stdout: "", stderr: "" });
+    const spawnedChildren: Array<
+      EventEmitter & { kill: ReturnType<typeof vi.fn>; killed: boolean }
+    > = [];
+    mocks.spawn.mockImplementation(() => {
+      const child = new EventEmitter();
+      const mockedChild = Object.assign(child, {
+        kill: vi.fn(() => {
+          queueMicrotask(() => child.emit("exit", null, "SIGTERM"));
+          return true;
+        }),
+        killed: false,
+      });
+      spawnedChildren.push(mockedChild);
+      return mockedChild;
+    });
+
+    // First start
+    await startGmailWatcher(createGmailConfig());
+    expect(spawnedChildren).toHaveLength(1);
+    expect(spawnedChildren[0].kill).not.toHaveBeenCalled();
+
+    // Second start (re-entry) should kill the first process
+    await startGmailWatcher(createGmailConfig());
+    expect(spawnedChildren).toHaveLength(2);
+    expect(spawnedChildren[0].kill).toHaveBeenCalledWith("SIGTERM");
+  });
+
+  it("clears existing renewInterval on re-entry to prevent interval leak", async () => {
+    vi.useFakeTimers();
+    try {
+      mocks.runCommandWithTimeout.mockResolvedValue({ code: 0, stdout: "", stderr: "" });
+
+      // First start - creates a renewal interval
+      await startGmailWatcher(createGmailConfig());
+      const timersAfterFirstStart = vi.getTimerCount();
+      expect(timersAfterFirstStart).toBeGreaterThanOrEqual(1);
+
+      // Second start (re-entry without stop) - the guard should clear the old
+      // interval before creating a new one, keeping the timer count stable.
+      await startGmailWatcher(createGmailConfig());
+      expect(vi.getTimerCount()).toBe(timersAfterFirstStart);
+    } finally {
+      vi.useRealTimers();
+    }
+  });
+
+  it("only one renewal fires per tick after multiple starts", async () => {
+    vi.useFakeTimers();
+    try {
+      // Resolve watch-start immediately on every call
+      mocks.runCommandWithTimeout.mockResolvedValue({ code: 0, stdout: "", stderr: "" });
+
+      // Start twice without stopping
+      await startGmailWatcher(createGmailConfig());
+      await startGmailWatcher(createGmailConfig());
+
+      // runCommandWithTimeout is called once per start (the gog watch start
+      // call).  After two successful starts it has been called twice.
+      expect(mocks.runCommandWithTimeout).toHaveBeenCalledTimes(2);
+
+      // Advance by one full renewal cycle.
+      // Default renewEveryMinutes = 720 (12 h) = 43_200_000 ms.
+      // If the old interval leaked, the callback would fire twice per cycle.
+      await vi.advanceTimersByTimeAsync(720 * 60_000);
+
+      // Only ONE renewal should have fired (the latest interval).
+      expect(mocks.runCommandWithTimeout).toHaveBeenCalledTimes(3);
+    } finally {
+      vi.useRealTimers();
+    }
+  });
+
+  it("escalates to SIGKILL and resolves on final timeout when process ignores signals", async () => {
+    vi.useFakeTimers();
+    try {
+      mocks.runCommandWithTimeout.mockResolvedValue({ code: 0, stdout: "", stderr: "" });
+
+      // Spawn a process that never emits exit/close/error
+      const stubbornChild = new EventEmitter();
+      const killCalls: string[] = [];
+      Object.assign(stubbornChild, {
+        kill: vi.fn((sig: string) => {
+          killCalls.push(sig);
+          return true;
+        }),
+        killed: false,
+      });
+      mocks.spawn.mockReturnValueOnce(stubbornChild);
+
+      await startGmailWatcher(createGmailConfig());
+      expect(mocks.spawn).toHaveBeenCalledTimes(1);
+
+      // Now spawn a normal child for the second start so re-entry triggers settle
+      mocks.spawn.mockImplementation(() => {
+        const child = new EventEmitter();
+        return Object.assign(child, {
+          kill: vi.fn(() => {
+            queueMicrotask(() => child.emit("exit", null, "SIGTERM"));
+            return true;
+          }),
+          killed: false,
+        });
+      });
+
+      // Re-entry starts settle on stubbornChild
+      const startPromise = startGmailWatcher(createGmailConfig());
+
+      // After 3s the escalation fires SIGKILL
+      await vi.advanceTimersByTimeAsync(3_000);
+      expect(killCalls).toContain("SIGTERM");
+      expect(killCalls).toContain("SIGKILL");
+
+      // After 8s total the final timeout resolves even though exit never fired
+      await vi.advanceTimersByTimeAsync(5_000);
+      await expect(startPromise).resolves.toEqual({ started: true });
+    } finally {
+      vi.useRealTimers();
+    }
+  });
+
+  it("cancels stale respawn timeout when re-entry happens during 5s window", async () => {
+    vi.useFakeTimers();
+    try {
+      mocks.runCommandWithTimeout.mockResolvedValue({ code: 0, stdout: "", stderr: "" });
+      const spawnedChildren: Array<EventEmitter & { kill: ReturnType<typeof vi.fn> }> = [];
+      mocks.spawn.mockImplementation(() => {
+        const child = new EventEmitter();
+        const mockedChild = Object.assign(child, {
+          kill: vi.fn(() => {
+            queueMicrotask(() => child.emit("exit", null, "SIGTERM"));
+            return true;
+          }),
+        });
+        spawnedChildren.push(mockedChild);
+        return mockedChild;
+      });
+
+      // First start
+      await startGmailWatcher(createGmailConfig());
+      expect(spawnedChildren).toHaveLength(1);
+
+      // Process crashes (exit code 1). This queues a 5s respawn timeout.
+      spawnedChildren[0].emit("exit", 1, null);
+
+      // Before the 5s timer fires, a config reload triggers re-entry.
+      // The re-entry guard should cancel the stale respawn timeout.
+      await startGmailWatcher(createGmailConfig());
+      expect(spawnedChildren).toHaveLength(2);
+
+      // Advance past the 5s respawn window. If the stale timeout was NOT
+      // cancelled, it would spawn a 3rd process (duplicate).
+      await vi.advanceTimersByTimeAsync(6000);
+      expect(spawnedChildren).toHaveLength(2); // No duplicate spawned
+    } finally {
+      vi.useRealTimers();
+    }
+  });
 });

src/hooks/gmail-watcher.ts

@@ -28,6 +28,7 @@ let watcherProcess: ChildProcess | null = null;
 let renewInterval: ReturnType<typeof setInterval> | null = null;
 let shuttingDown = false;
 let currentConfig: GmailHookRuntimeConfig | null = null;
+let respawnTimeout: ReturnType<typeof setTimeout> | null = null;
 
 /**
  * Check if gog binary is available
@@ -114,7 +115,8 @@ function spawnGogServe(cfg: GmailHookRuntimeConfig): ChildProcess {
     }
     log.warn(`gog exited (code=${code}, signal=${signal}); restarting in 5s`);
     watcherProcess = null;
-    setTimeout(() => {
+    respawnTimeout = setTimeout(() => {
+      respawnTimeout = null;
       if (shuttingDown || !currentConfig) {
         return;
       }
@@ -125,6 +127,46 @@ function spawnGogServe(cfg: GmailHookRuntimeConfig): ChildProcess {
   return child;
 }
 
+/**
+ * Send SIGTERM, escalate to SIGKILL after 3 s, and resolve on exit/close/error
+ * or a final 5 s timeout after SIGKILL so the caller never hangs.
+ */
+function settleProcess(proc: ChildProcess): Promise<void> {
+  return new Promise<void>((resolve) => {
+    let settled = false;
+    const settle = () => {
+      if (settled) {
+        return;
+      }
+      settled = true;
+      clearTimeout(escalation);
+      clearTimeout(finalTimeout);
+      resolve();
+    };
+
+    proc.on("exit", settle);
+    proc.on("close", settle);
+    proc.on("error", settle);
+
+    proc.kill("SIGTERM");
+
+    const escalation = setTimeout(() => {
+      try {
+        proc.kill("SIGKILL");
+      } catch {
+        // already dead
+      }
+    }, 3_000);
+
+    const finalTimeout = setTimeout(() => {
+      if (!settled) {
+        log.warn("gog process did not exit after SIGKILL; giving up");
+        settle();
+      }
+    }, 8_000);
+  });
+}
+
 export type GmailWatcherStartResult = {
   started: boolean;
   reason?: string;
@@ -235,6 +277,28 @@ export async function startGmailWatcher(
   }
   currentConfig = runtimeConfig;
 
+  // Stop any existing watcher before doing async setup so a re-entry
+  // does not orphan the old serve process or leave a dangling timer.
+  // This must run before Tailscale/watch-start to prevent the old
+  // process from exiting and queuing a respawn during async work.
+  if (watcherProcess || renewInterval || respawnTimeout) {
+    shuttingDown = true;
+    if (respawnTimeout) {
+      clearTimeout(respawnTimeout);
+      respawnTimeout = null;
+    }
+    if (renewInterval) {
+      clearInterval(renewInterval);
+      renewInterval = null;
+    }
+    if (watcherProcess) {
+      const oldProcess = watcherProcess;
+      watcherProcess = null;
+      await settleProcess(oldProcess);
+    }
+    shuttingDown = false;
+  }
+
   // Set up Tailscale endpoint if needed
   if (runtimeConfig.tailscale.mode !== "off") {
     const cancellation = createGmailWatcherCancellation(options);
@@ -283,8 +347,6 @@ export async function startGmailWatcher(
   }
   shuttingDown = false;
   watcherProcess = spawnGogServe(runtimeConfig);
-
-  // Set up renewal interval
   const renewMs = runtimeConfig.renewEveryMinutes * 60_000;
   renewInterval = setInterval(() => {
     if (shuttingDown) {
@@ -306,6 +368,10 @@ export async function startGmailWatcher(
 export async function stopGmailWatcher(): Promise<void> {
   shuttingDown = true;
 
+  if (respawnTimeout) {
+    clearTimeout(respawnTimeout);
+    respawnTimeout = null;
+  }
   if (renewInterval) {
     clearInterval(renewInterval);
     renewInterval = null;
@@ -313,24 +379,9 @@ export async function stopGmailWatcher(): Promise<void> {
 
   if (watcherProcess) {
     log.info("stopping gmail watcher");
-    watcherProcess.kill("SIGTERM");
-
-    // Wait a bit for graceful shutdown
-    await new Promise<void>((resolve) => {
-      const timeout = setTimeout(() => {
-        if (watcherProcess) {
-          watcherProcess.kill("SIGKILL");
-        }
-        resolve();
-      }, 3000);
-
-      watcherProcess?.on("exit", () => {
-        clearTimeout(timeout);
-        resolve();
-      });
-    });
-
+    const proc = watcherProcess;
     watcherProcess = null;
+    await settleProcess(proc);
   }
 
   currentConfig = null;