From 337933509d623cb68f2b3fbb9ebeb3641901dab7 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 16 Aug 2026 09:43:12 -0700 Subject: [PATCH] fix(android): stop wrapper subprocess trees on cancellation (#124686) * fix(android): reap Gradle wrapper subprocesses * fix(android): preserve wrapper spawn errors --- scripts/run-android-gradle.mts | 30 ++---- test/scripts/run-android-gradle.test.ts | 132 +++++++++++++++++++++++- 2 files changed, 142 insertions(+), 20 deletions(-) diff --git a/scripts/run-android-gradle.mts b/scripts/run-android-gradle.mts index a6550427e89e..0ff60b713df5 100644 --- a/scripts/run-android-gradle.mts +++ b/scripts/run-android-gradle.mts @@ -1,9 +1,9 @@ #!/usr/bin/env node -import { spawn } from "node:child_process"; import fs from "node:fs"; import os from "node:os"; import path from "node:path"; import { fileURLToPath } from "node:url"; +import { runManagedCommand } from "./lib/managed-child-process.mts"; import { resolveRepoRoot } from "./lib/repo-root.mjs"; const repoRoot = resolveRepoRoot(import.meta.url); @@ -74,32 +74,24 @@ export function resolveAndroidSdkEnv(options: SdkOptions = {}) { return { ...env, ANDROID_HOME: defaultSdkDir }; } -export function run( +export async function run( command: string, args: readonly string[], cwd: string, env: NodeJS.ProcessEnv = process.env, ) { - return new Promise((resolve) => { - const child = spawn(command, args, { + try { + return await runManagedCommand({ + args: [...args], + bin: command, cwd, env, - stdio: "inherit", + shell: false, }); - child.on("close", (status, signal) => { - if (typeof status === "number") { - resolve(status); - } else if (signal) { - resolve(128); - } else { - resolve(1); - } - }); - child.on("error", (error) => { - console.error(error instanceof Error ? error.message : String(error)); - resolve(1); - }); - }); + } catch (error) { + console.error(error instanceof Error ? error.message : String(error)); + return 1; + } } export async function main(argv: string[] = process.argv.slice(2)) { diff --git a/test/scripts/run-android-gradle.test.ts b/test/scripts/run-android-gradle.test.ts index 8ae32bb05783..a554dfa1c5f2 100644 --- a/test/scripts/run-android-gradle.test.ts +++ b/test/scripts/run-android-gradle.test.ts @@ -1,11 +1,21 @@ +import { spawn } from "node:child_process"; +import fs from "node:fs"; +import os from "node:os"; import path from "node:path"; -import { describe, expect, it } from "vitest"; +import { setTimeout as delay } from "node:timers/promises"; +import { pathToFileURL } from "node:url"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { linuxArmAndroidGradleSkipMessage, resolveAndroidSdkEnv, + run, shouldSkipLinuxArmAndroidGradle, splitAndroidGradleArgs, } from "../../scripts/run-android-gradle.mts"; +import { useAutoCleanupTempDirTracker } from "../helpers/temp-dir.js"; + +const posixIt = process.platform === "win32" ? it.skip : it; +const tempDirs = useAutoCleanupTempDirTracker(afterEach); describe("run-android-gradle", () => { it("splits Gradle args from an optional post command", () => { @@ -90,4 +100,124 @@ describe("run-android-gradle", () => { expect(result).toBe(env); }); }); + + posixIt("terminates the active command tree when the wrapper is terminated", async () => { + const dir = tempDirs.make("openclaw-android-gradle-process-"); + const processTreePath = path.join(dir, "process-tree.json"); + const moduleUrl = pathToFileURL(path.resolve("scripts/run-android-gradle.mts")).href; + const childSource = ` +const { spawn } = require("node:child_process"); +const fs = require("node:fs"); +const descendant = spawn(process.execPath, [ + "-e", + "process.on('SIGTERM', () => {}); setInterval(() => {}, 1_000);", +], { stdio: "ignore" }); +fs.writeFileSync( + process.argv[1], + JSON.stringify({ childPid: process.pid, descendantPid: descendant.pid }), +); +setInterval(() => {}, 1_000); +`; + const runnerSource = ` +import { run } from ${JSON.stringify(moduleUrl)}; +process.exitCode = await run( + process.execPath, + ["-e", ${JSON.stringify(childSource)}, ${JSON.stringify(processTreePath)}], + process.cwd(), +); +`; + const runner = spawn( + process.execPath, + ["--import", "tsx", "--input-type=module", "-e", runnerSource], + { stdio: "ignore" }, + ); + const runnerPid = expectPid(runner.pid); + let childPid = 0; + let descendantPid = 0; + + try { + await waitFor(() => fs.existsSync(processTreePath)); + const processTree = JSON.parse(fs.readFileSync(processTreePath, "utf8")) as { + childPid: number; + descendantPid: number; + }; + childPid = processTree.childPid; + descendantPid = processTree.descendantPid; + expect(Number.isInteger(childPid)).toBe(true); + expect(Number.isInteger(descendantPid)).toBe(true); + expect(isProcessAlive(childPid)).toBe(true); + expect(isProcessAlive(descendantPid)).toBe(true); + + process.kill(runnerPid, "SIGTERM"); + const result = await waitForClose(runner); + await waitFor(() => !isProcessAlive(childPid), 1_500); + await waitFor(() => !isProcessAlive(descendantPid), 1_500); + + expect(isProcessAlive(childPid)).toBe(false); + expect(isProcessAlive(descendantPid)).toBe(false); + expect(result).toEqual({ code: 143, signal: null }); + } finally { + if (isProcessAlive(runnerPid)) { + process.kill(runnerPid, "SIGKILL"); + } + if (childPid && isProcessAlive(childPid)) { + process.kill(childPid, "SIGKILL"); + } + if (descendantPid && isProcessAlive(descendantPid)) { + process.kill(descendantPid, "SIGKILL"); + } + } + }); + + it("reports spawn errors and returns a failure status", async () => { + const error = vi.spyOn(console, "error").mockImplementation(() => {}); + const missingCommand = path.join(os.tmpdir(), `openclaw-missing-command-${process.pid}`); + try { + await expect(run(missingCommand, [], process.cwd(), {})).resolves.toBe(1); + expect(error).toHaveBeenCalledOnce(); + expect(String(error.mock.calls[0]?.[0])).toContain("ENOENT"); + } finally { + error.mockRestore(); + } + }); }); + +function expectPid(pid: number | undefined): number { + if (pid === undefined) { + throw new Error("expected child process pid"); + } + return pid; +} + +async function waitFor(condition: () => boolean, timeoutMs = 3_000): Promise { + const startedAt = Date.now(); + while (!condition()) { + if (Date.now() - startedAt > timeoutMs) { + throw new Error("timed out waiting for condition"); + } + await delay(5); + } +} + +async function waitForClose(child: ReturnType) { + return await new Promise<{ code: number | null; signal: NodeJS.Signals | null }>((resolve) => { + child.once("close", (code, signal) => resolve({ code, signal })); + }); +} + +function isProcessAlive(pid: number): boolean { + try { + process.kill(pid, 0); + } catch { + return false; + } + if (process.platform !== "linux") { + return true; + } + try { + const stat = fs.readFileSync(`/proc/${pid}/stat`, "utf8"); + return stat.charAt(stat.lastIndexOf(")") + 2) !== "Z"; + } catch { + return false; + } +}