Merge pull request #1194 from morluto/fix/darwin-helper-compile-tmpdir

fix(process): stop cancelled Swift helper builds before removing their files

src/native/NativeUiHelperRuntime.ts

@@ -1,7 +1,8 @@
-import { mkdtemp, rm, symlink } from "node:fs/promises";
+import { mkdir, mkdtemp, rm, symlink } from "node:fs/promises";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
 import { fileURLToPath } from "node:url";
+import { SWIFTC_INTERRUPT } from "../process/DarwinProcessRunTokenReader.js";
 import { execFileOutput } from "../process/ExecFileOutput.js";
 import { safeParseJson } from "../domain/safeJson.js";
 import type { NativeUiHelper } from "./NativeUiObservation.js";
@@ -22,6 +23,9 @@ export const createNativeUiHelperRuntime = () => {
         ),
         main,
       );
+      // Keep swiftc's intermediate objects inside the removable root.
+      const compilerTemporary = join(root, "tmp");
+      await mkdir(compilerTemporary);
       await execFileOutput(
         "/usr/bin/xcrun",
         [
@@ -41,6 +45,8 @@ export const createNativeUiHelperRuntime = () => {
         {
           timeout: 60_000,
           maxBuffer: 1024 * 1024,
+          stopSignal: SWIFTC_INTERRUPT,
+          env: { ...process.env, TMPDIR: compilerTemporary },
           ...(signal === undefined ? {} : { signal }),
         },
       );

src/process/DarwinProcessRunTokenReader.ts

@@ -1,4 +1,4 @@
-import { mkdtemp, rm } from "node:fs/promises";
+import { mkdir, mkdtemp, rm } from "node:fs/promises";
 import { rmSync } from "node:fs";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
@@ -65,6 +65,13 @@ const sourceFiles = [
   ),
 ];
 
+/**
+ * swift-driver runs each frontend job in its own process group. On SIGINT it
+ * stops those jobs and deletes their objects before exiting; on SIGTERM it
+ * exits while a job can still write objects under TMPDIR.
+ */
+export const SWIFTC_INTERRUPT = "SIGINT";
+
 /** The native Darwin process-ownership reader could not be prepared. */
 export class DarwinProcessOwnershipInspectionError extends Error {
   constructor(message: string, options?: ErrorOptions) {
@@ -149,7 +156,14 @@ export const createDarwinProcessRunTokenReader = (
       exitCleanupInstalled = true;
       if (signal.aborted || closed) throw abortReason(signal);
       const output = join(operationRoot, "reader");
-      const compilerEnvironment = { ...process.env };
+      // swiftc keeps intermediate objects under TMPDIR, so keep them inside
+      // the root that cleanup removes.
+      const compilerTemporary = join(operationRoot, "tmp");
+      await mkdir(compilerTemporary);
+      const compilerEnvironment: NodeJS.ProcessEnv = {
+        ...process.env,
+        TMPDIR: compilerTemporary,
+      };
       // Use Swift's reusable default cache for REA's internal helper compilation.
       delete compilerEnvironment.CLANG_MODULE_CACHE_PATH;
       try {
@@ -160,6 +174,7 @@ export const createDarwinProcessRunTokenReader = (
             timeout: 60_000,
             maxBuffer: 1024 * 1024,
             signal,
+            stopSignal: SWIFTC_INTERRUPT,
             env: compilerEnvironment,
           },
         );

src/process/ExecFileOutput.test.ts

@@ -1,5 +1,10 @@
+import { access, readFile } from "node:fs/promises";
+import { join } from "node:path";
+
 import { describe, expect, it } from "vitest";
 
+import { createTestTempDirectory } from "../../tests/fixtures/temporaryDirectory.js";
+
 import { execFileOutput } from "./ExecFileOutput.js";
 
 describe("execFileOutput", () => {
@@ -32,4 +37,61 @@ describe("execFileOutput", () => {
       ]),
     ).rejects.toMatchObject({ code: 7, stdout: "out", stderr: "err" });
   });
+
+  it.skipIf(process.platform === "win32")(
+    "stops an aborted process with the caller's signal and settles after it exits",
+    async () => {
+      const directory = await createTestTempDirectory("rea-exec-stop-");
+      const ready = join(directory, "ready");
+      const stopped = join(directory, "stopped");
+      // The child records the signal it received only after a cleanup delay.
+      const script = [
+        'const fs = require("node:fs");',
+        'for (const name of ["SIGINT", "SIGTERM"])',
+        `  process.on(name, () => setTimeout(() => { fs.writeFileSync(${JSON.stringify(stopped)}, name); process.exit(0); }, 200));`,
+        `fs.writeFileSync(${JSON.stringify(ready)}, "");`,
+        "setInterval(() => {}, 1000);",
+      ].join("\n");
+      const controller = new AbortController();
+      const execution = execFileOutput(process.execPath, ["-e", script], {
+        signal: controller.signal,
+        stopSignal: "SIGINT",
+      }).then(
+        () => undefined,
+        async (cause: unknown) => ({
+          cause,
+          stoppedBeforeSettling: await readFile(stopped, "utf8").catch(
+            () => null,
+          ),
+        }),
+      );
+      for (let attempt = 0; attempt < 500; attempt += 1) {
+        if (
+          await access(ready).then(
+            () => true,
+            () => false,
+          )
+        )
+          break;
+        await new Promise((resolve) => setTimeout(resolve, 10));
+      }
+
+      controller.abort();
+      const outcome = await execution;
+
+      expect(outcome?.cause).toMatchObject({ name: "AbortError" });
+      expect(outcome?.stoppedBeforeSettling).toBe("SIGINT");
+    },
+  );
+
+  it("rejects an already aborted stoppable command without starting it", async () => {
+    const controller = new AbortController();
+    controller.abort();
+    await expect(
+      execFileOutput(process.execPath, ["-e", ""], {
+        signal: controller.signal,
+        stopSignal: "SIGINT",
+      }),
+    ).rejects.toMatchObject({ name: "AbortError" });
+  });
 });

src/process/ExecFileOutput.ts

@@ -6,7 +6,29 @@ import {
 type ExecFileOutputOptions = Omit<
   ExecFileOptionsWithStringEncoding,
   "encoding" | "maxBuffer"
-> & { readonly maxBuffer?: number };
+> & {
+  readonly maxBuffer?: number;
+  /**
+   * Signal that stops the process on abort or timeout. The promise then
+   * settles only after the process exits, so the caller can remove files it
+   * writes; a process still running after a grace period is killed. Without
+   * it, Node's execFile sends SIGTERM on abort and rejects before the
+   * process exits.
+   */
+  readonly stopSignal?: NodeJS.Signals;
+};
+
+/** How long a stopped process may take to exit before it is killed. */
+const STOP_GRACE_MS = 5_000;
+
+const abortError = (signal: AbortSignal): Error => {
+  const error = new Error("The operation was aborted", {
+    cause: signal.reason,
+  });
+  error.name = "AbortError";
+  Reflect.set(error, "code", "ABORT_ERR");
+  return error;
+};
 
 /** Captured subprocess output read from an execFileOutput rejection. */
 export interface ExecFileOutputFailure {
@@ -47,22 +69,49 @@ export const execFileOutput = (
   options: ExecFileOutputOptions = {},
 ): Promise<{ readonly stdout: string; readonly stderr: string }> =>
   new Promise((resolve, reject) => {
-    execFile(
+    const { stopSignal, signal, ...execOptions } = options;
+    const stopped =
+      stopSignal === undefined || signal === undefined
+        ? undefined
+        : { signal, stopSignal };
+    if (stopped?.signal.aborted === true) {
+      reject(abortError(stopped.signal));
+      return;
+    }
+    let aborted = false;
+    let grace: NodeJS.Timeout | undefined;
+    const child = execFile(
       command,
       [...arguments_],
       {
-        ...options,
+        ...execOptions,
+        ...(stopped === undefined
+          ? signal === undefined
+            ? {}
+            : { signal }
+          : { killSignal: stopped.stopSignal }),
         encoding: "utf8",
         maxBuffer: options.maxBuffer ?? Number.POSITIVE_INFINITY,
       },
       (error, stdout, stderr) => {
-        if (error !== null) {
-          Reflect.set(error, "stdout", stdout);
-          Reflect.set(error, "stderr", stderr);
-          reject(error);
+        stopped?.signal.removeEventListener("abort", onAbort);
+        clearTimeout(grace);
+        const failure =
+          aborted && stopped !== undefined ? abortError(stopped.signal) : error;
+        if (failure !== null) {
+          Reflect.set(failure, "stdout", stdout);
+          Reflect.set(failure, "stderr", stderr);
+          reject(failure);
           return;
         }
         resolve({ stdout, stderr });
       },
     );
+    const onAbort = (): void => {
+      if (stopped === undefined) return;
+      aborted = true;
+      child.kill(stopped.stopSignal);
+      grace = setTimeout(() => child.kill("SIGKILL"), STOP_GRACE_MS);
+    };
+    stopped?.signal.addEventListener("abort", onAbort, { once: true });
   });

tests/boundary/process/darwinProcessRunTokenReader.test.ts

@@ -15,6 +15,7 @@ import { fileURLToPath } from "node:url";
 import { promisify } from "node:util";
 import { _electron as electron } from "playwright-core";
 import { expect, it, vi } from "vitest";
+import { z } from "zod";
 import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js";
 
 import {
@@ -294,6 +295,94 @@ it.skipIf(process.platform === "win32")(
   },
 );
 
+const temporaryStateSchema = z.object({
+  root: z.string(),
+  temporary: z.string(),
+});
+
+/**
+ * A compiler that behaves like swift-driver: a job in its own process group
+ * writes an object under TMPDIR, SIGINT stops the job and deletes the object,
+ * and any other signal leaves the job writing after the driver exits.
+ */
+const writeTemporaryObjectCompiler = async (directory: string) => {
+  const executable = join(directory, "fake-temporary-xcrun");
+  const statePath = join(directory, "temporary-state.json");
+  const source = `#!/usr/bin/env node
+const fs = require("node:fs");
+const os = require("node:os");
+const path = require("node:path");
+const { spawn } = require("node:child_process");
+const temporary = fs.mkdtempSync(path.join(os.tmpdir(), "TemporaryDirectory."));
+const object = path.join(temporary, "ProcessRunTokenReader-1.o");
+const job = spawn(process.execPath, ["-e",
+  "setTimeout(() => { const fs = require('node:fs'); fs.mkdirSync(" + JSON.stringify(temporary) +
+  ", { recursive: true }); fs.writeFileSync(" + JSON.stringify(object) + ", 'object'); }, 300)"],
+  { detached: true, stdio: "ignore" });
+job.unref();
+process.on("SIGINT", () => {
+  job.kill("SIGKILL");
+  fs.rmSync(object, { force: true });
+  process.exit(130);
+});
+const output = process.argv[process.argv.indexOf("-o") + 1];
+fs.writeFileSync(${JSON.stringify(statePath)}, JSON.stringify({ root: path.dirname(output), temporary }));
+setInterval(() => {}, 1000);
+`;
+  await writeFile(executable, source);
+  await chmod(executable, 0o755);
+  return { executable, statePath };
+};
+
+it.skipIf(process.platform === "win32")(
+  "removes cancelled compiler temporaries with the helper root",
+  async () => {
+    const directory = await createTestTempDirectory(
+      "rea-process-token-temporary-test-",
+    );
+    const compiler = await writeTemporaryObjectCompiler(directory);
+    const reader = createDarwinProcessRunTokenReader({
+      xcrun: compiler.executable,
+    });
+    const controller = new AbortController();
+    const preparation = reader.prepare(controller.signal).then(
+      () => undefined,
+      (cause: unknown) => cause,
+    );
+    let temporary: string | undefined;
+    try {
+      const deadline = Date.now() + 5_000;
+      let state: z.infer<typeof temporaryStateSchema> | undefined;
+      while (state === undefined && Date.now() < deadline) {
+        const parsed = temporaryStateSchema.safeParse(
+          await readFile(compiler.statePath, "utf8").then(
+            (text): unknown => JSON.parse(text),
+            () => undefined,
+          ),
+        );
+        if (parsed.success) state = parsed.data;
+        else await new Promise((resolve) => setTimeout(resolve, 10));
+      }
+      if (state === undefined) throw new Error("fake compiler did not start");
+      temporary = state.temporary;
+      expect(state.temporary.startsWith(`${state.root}/`)).toBe(true);
+
+      controller.abort();
+      expect(await preparation).toMatchObject({ name: "AbortError" });
+      await reader.close();
+      // A job that outlived the compiler would recreate the root by now.
+      await new Promise((resolve) => setTimeout(resolve, 600));
+      await expect(access(state.root)).rejects.toThrow();
+    } finally {
+      if (!controller.signal.aborted) controller.abort();
+      await preparation;
+      await reader.close();
+      if (temporary !== undefined)
+        await rm(temporary, { recursive: true, force: true });
+    }
+  },
+);
+
 it.skipIf(process.platform === "win32").each(["first", "second"] as const)(
   "keeps shared compilation alive when the %s waiter cancels",
   async (cancelledWaiter) => {