fix: replace domain-specific exit handler with generic signal handler registry (#299)

* fix: replace domain-specific exit handler with generic signal handler registry

Rewrite exitHandler.ts as a generic, domain-agnostic exit signal module
that exports onExitSignal(handler) returning a dispose function.

- subprocess.ts now registers via onExitSignal instead of direct
  process.on(SIGINT/SIGTERM) calls
- resolveTokens registers a signal handler that captures tokens by
  closure, fixing the race condition where the exit handler would
  read the wrong token after disposal
- Remove setupExitHandler and runCleanup — domain cleanup is handled
  by post.ts + await using

Co-authored-by: Cursor <cursoragent@cursor.com>

* tweaks

* simplify handler installation

* extract to util

* fix race in dispose

* wrap dispose body in try/finally to ensure disposingRef always settles

---------

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com>

utils/exitHandler.ts

@@ -1,119 +1,35 @@
-import { LEAPING_INTO_ACTION_PREFIX } from "../mcp/comment.ts";
-import type { ToolState } from "../mcp/server.ts";
-import { buildPullfrogFooter } from "./buildPullfrogFooter.ts";
-import { log } from "./cli.ts";
-import { createOctokit, parseRepoContext } from "./github.ts";
-import { revokeGitHubInstallationToken } from "./token.ts";
+import os from "node:os";
+
+type ExitSignalHandler = (signal: "SIGINT" | "SIGTERM") => void | Promise<void>;
+
+const handlers = new Set<ExitSignalHandler>();
+let installed = false;
 
 /**
- * Build error comment body with error message and footer
+ * Register a handler to run when the process receives SIGINT or SIGTERM.
+ * Returns a dispose function that removes the handler.
  */
-export function buildErrorCommentBody(params: {
-  owner: string;
-  repo: string;
-  runId: string | undefined;
-  isCancellation: boolean;
-}): string {
-  const workflowRunLink = params.runId
-    ? `[workflow run logs](https://github.com/${params.owner}/${params.repo}/actions/runs/${params.runId})`
-    : "workflow run logs";
-  const errorMessage = params.isCancellation
-    ? `This run was cancelled 🛑\n\nThe workflow was cancelled before completion. Please check the ${workflowRunLink} for details.`
-    : `This run croaked 😵\n\nThe workflow encountered an error before any progress could be reported. Please check the ${workflowRunLink} for details.`;
-  const footer = buildPullfrogFooter({
-    triggeredBy: true,
-    workflowRun: params.runId
-      ? { owner: params.owner, repo: params.repo, runId: params.runId }
-      : undefined,
-  });
-  return `${errorMessage}${footer}`;
+export function onExitSignal(handler: ExitSignalHandler): () => void {
+  installSignalHandlers();
+  handlers.add(handler);
+  return () => {
+    handlers.delete(handler);
+  };
 }
 
-let cleanupFn: ((isCancellation: boolean) => Promise<void>) | undefined;
+function installSignalHandlers(): void {
+  if (installed) return;
+  installed = true;
 
-export function setupExitHandler(toolState: ToolState): void {
-  let hasCleanedUp = false;
-
-  async function cleanup(isCancellation: boolean): Promise<void> {
-    if (hasCleanedUp) {
-      return;
-    }
-    hasCleanedUp = true;
-
-    const token = process.env.GITHUB_TOKEN;
-    const commentId = toolState.progressCommentId;
-    const wasUpdated = toolState.wasUpdated === true;
-
-    // update progress comment if it was never updated (still shows "leaping into action")
-    if (token && commentId && !wasUpdated) {
-      try {
-        const repoContext = parseRepoContext();
-        const octokit = createOctokit(token);
-
-        const existingComment = await octokit.rest.issues.getComment({
-          owner: repoContext.owner,
-          repo: repoContext.name,
-          comment_id: commentId,
-        });
-
-        const commentBody = existingComment.data.body || "";
-
-        // only update if comment still shows the initial "leaping into action" message
-        if (commentBody.startsWith(LEAPING_INTO_ACTION_PREFIX)) {
-          const runId = process.env.GITHUB_RUN_ID;
-
-          const body = buildErrorCommentBody({
-            owner: repoContext.owner,
-            repo: repoContext.name,
-            runId,
-            isCancellation,
-          });
-
-          await octokit.rest.issues.updateComment({
-            owner: repoContext.owner,
-            repo: repoContext.name,
-            comment_id: commentId,
-            body,
-          });
-
-          log.info("» updated progress comment with error message");
-        }
-      } catch {
-        // ignore errors during cleanup
-      }
-    }
-
-    // revoke token
-    if (token) {
-      try {
-        await revokeGitHubInstallationToken(token);
-        log.debug("» installation token revoked");
-      } catch {
-        // ignore errors during cleanup
-      }
-    }
-  }
-
-  // store cleanup function for runCleanup()
-  cleanupFn = cleanup;
-
-  // handle cancellation signals
-  function handleSignal(): void {
-    log.info("» workflow cancelled, cleaning up...");
-    cleanup(true).finally(() => process.exit(1));
+  async function handleSignal(signal: "SIGINT" | "SIGTERM") {
+    await Promise.allSettled([...handlers].map((h) => Promise.try(h, signal)));
+    exitWithSignal(signal);
   }
 
   process.on("SIGINT", handleSignal);
   process.on("SIGTERM", handleSignal);
 }
 
-/**
- * Run cleanup explicitly. Called from entry.ts in finally block.
- */
-export async function runCleanup(): Promise<void> {
-  try {
-    await cleanupFn?.(false);
-  } catch {
-    // ignore errors during cleanup
-  }
+export function exitWithSignal(signal: "SIGINT" | "SIGTERM") {
+  process.exit(128 + os.constants.signals[signal]);
 }

utils/postCleanup.ts

@@ -1,6 +1,6 @@
 import { LEAPING_INTO_ACTION_PREFIX } from "../mcp/comment.ts";
+import { buildPullfrogFooter } from "./buildPullfrogFooter.ts";
 import { log } from "./cli.ts";
-import { buildErrorCommentBody } from "./exitHandler.ts";
 import { createOctokit, parseRepoContext } from "./github.ts";
 import { type ResolvedPromptInput, resolvePromptInput } from "./payload.ts";
 import { getJobToken } from "./token.ts";
@@ -14,6 +14,30 @@ type JsonPromptInput = Extract<ResolvedPromptInput, object>; // not string
  * */
 const SHOULD_CHECK_REASON = true;
 
+/**
+ * Build error comment body with error message and footer
+ */
+function buildErrorCommentBody(params: {
+  owner: string;
+  repo: string;
+  runId: string | undefined;
+  isCancellation: boolean;
+}): string {
+  const workflowRunLink = params.runId
+    ? `[workflow run logs](https://github.com/${params.owner}/${params.repo}/actions/runs/${params.runId})`
+    : "workflow run logs";
+  const errorMessage = params.isCancellation
+    ? `This run was cancelled 🛑\n\nThe workflow was cancelled before completion. Please check the ${workflowRunLink} for details.`
+    : `This run croaked 😵\n\nThe workflow encountered an error before any progress could be reported. Please check the ${workflowRunLink} for details.`;
+  const footer = buildPullfrogFooter({
+    triggeredBy: true,
+    workflowRun: params.runId
+      ? { owner: params.owner, repo: params.repo, runId: params.runId }
+      : undefined,
+  });
+  return `${errorMessage}${footer}`;
+}
+
 /**
  * Validate that the progress comment is stuck on "Leaping into action"
  * Fetches the comment by ID and checks if it starts with LEAPING_INTO_ACTION_PREFIX

utils/subprocess.ts

@@ -2,6 +2,7 @@ import { type ChildProcess, spawn as nodeSpawn } from "node:child_process";
 import { performance } from "node:perf_hooks";
 import { DEFAULT_ACTIVITY_CHECK_INTERVAL_MS, DEFAULT_ACTIVITY_TIMEOUT_MS } from "./activity.ts";
 import { log } from "./cli.ts";
+import { onExitSignal } from "./exitHandler.ts";
 
 export type TrackChildOptions = {
   child: ChildProcess;
@@ -18,6 +19,9 @@ let externalSignalHandler: SignalHandler | null = null;
 
 // track a child process for cleanup on Ctrl+C
 export function trackChild(options: TrackChildOptions): void {
+  // the signal handler cleans up all tracked children
+  // so we only have to install it once some child gets tracked
+  installSignalHandler();
   activeChildren.set(options.child, options.killGroup ?? false);
 }
 
@@ -32,8 +36,7 @@ export function setSignalHandler(handler: SignalHandler | null): void {
 }
 
 // kill all tracked children without exiting
-export function killTrackedChildren(): number {
-  const count = activeChildren.size;
+export function killTrackedChildren() {
   for (const entry of activeChildren) {
     const child = entry[0];
     const killGroup = entry[1];
@@ -47,35 +50,24 @@ export function killTrackedChildren(): number {
     }
     child.kill("SIGKILL");
   }
-  return count;
-}
-
-// cleanup handler for SIGINT/SIGTERM - kills all tracked children
-function cleanupAndExit(signal: string): void {
-  const count = killTrackedChildren();
-  if (count > 0) {
-    log.info(`» received ${signal}, killing ${count} subprocess(es)...`);
-  }
-  // force exit after a short delay if process is stuck
-  setTimeout(() => process.exit(1), 500).unref();
-  process.exit(1);
-}
-
-function handleSignal(signal: NodeJS.Signals): void {
-  if (externalSignalHandler) {
-    externalSignalHandler(signal);
-    return;
-  }
-  cleanupAndExit(signal);
 }
 
 // install signal handlers once (call early in process lifecycle)
 let handlersInstalled = false;
-export function installSignalHandlers(): void {
+function installSignalHandler(): void {
   if (handlersInstalled) return;
   handlersInstalled = true;
-  process.on("SIGINT", () => handleSignal("SIGINT"));
-  process.on("SIGTERM", () => handleSignal("SIGTERM"));
+  onExitSignal((signal) => {
+    if (externalSignalHandler) {
+      externalSignalHandler(signal);
+      return;
+    }
+    const count = activeChildren.size;
+    if (count > 0) {
+      log.info(`» received ${signal}, killing ${count} subprocess(es)...`);
+    }
+    killTrackedChildren();
+  });
 }
 
 export interface SpawnOptions {
@@ -106,7 +98,7 @@ export async function spawn(options: SpawnOptions): Promise<SpawnResult> {
   const { cmd, args, env, input, timeout, cwd, stdio, onStdout, onStderr } = options;
   const activityTimeoutMs = options.activityTimeout ?? DEFAULT_ACTIVITY_TIMEOUT_MS;
 
-  installSignalHandlers();
+  installSignalHandler();
 
   const startTime = performance.now();
   let stdoutBuffer = "";

utils/token.ts

@@ -2,6 +2,7 @@ import assert from "node:assert/strict";
 import * as core from "@actions/core";
 import type { PushPermission } from "../external.ts";
 import { log } from "./cli.ts";
+import { onExitSignal } from "./exitHandler.ts";
 import { acquireNewToken } from "./github.ts";
 import { isGitHubActions } from "./globals.ts";
 
@@ -131,18 +132,35 @@ export async function resolveTokens(params: ResolveTokensParams): Promise<TokenR
   const revertGithubToken = setEnvironmentVariable("GITHUB_TOKEN", mcpToken);
   mcpTokenValue = mcpToken;
 
-  return {
-    gitToken,
-    mcpToken,
-    async [Symbol.asyncDispose]() {
+  let disposingRef: PromiseWithResolvers<void> | undefined;
+
+  const dispose = async () => {
+    if (disposingRef) {
+      // this can happen if the signal arrives when disposing tokens
+      // we make sure to wait for the current dispose to complete
+      return disposingRef.promise;
+    }
+    disposingRef = Promise.withResolvers();
+    try {
       mcpTokenValue = undefined;
       revertGithubToken();
-      // revoke both tokens
       await Promise.all([
         revokeGitHubInstallationToken(gitToken),
         revokeGitHubInstallationToken(mcpToken),
       ]);
-    },
+    } finally {
+      removeSignalHandler();
+      disposingRef.resolve();
+      disposingRef = undefined;
+    }
+  };
+
+  const removeSignalHandler = onExitSignal(dispose);
+
+  return {
+    gitToken,
+    mcpToken,
+    [Symbol.asyncDispose]: dispose,
   };
 }