From e33257de1181a82632dcdf4a805198c146362167 Mon Sep 17 00:00:00 2001 From: tzy-17 Date: Sun, 19 Jul 2026 07:01:23 +0800 Subject: [PATCH] fix(scripts): bound duplicate PR closure GitHub lookups (#110750) * fix(scripts): bound duplicate PR closure GitHub lookups The duplicate PR closure helper performs multiple sequential gh API reads and writes after a merge. Each call used execFileSync without a timeout, so one stalled GitHub request could block the surrounding workflow job until it expires. Add a 60-second timeout and SIGKILL to the default gh runner, applied to both read-only lookups and pr edit writes that forward stdin. * test(scripts): add real behavior proof for duplicate PR closure timeout Add two live subprocess tests using the real spawnSync (not mocked) to prove the timeout + SIGKILL surface works against actual child processes: - A 1 ms timeout against a long-running node subprocess proves SIGKILL is delivered and the timeout triggers correctly. - A fast node -e 0 subprocess with the production 60-second bound proves the timeout does not false-positive on healthy completions. Switch the vi.mock to a partial mock using importOriginal so spawnSync remains the real implementation for live tests while execFileSync stays mocked for the DI-based option assertions. * refactor(scripts): inject duplicate closure GitHub runner * fix(scripts): declare trusted tooling pin validator --------- Co-authored-by: Peter Steinberger --- scripts/close-duplicate-prs-after-merge.d.mts | 20 +++++++ scripts/close-duplicate-prs-after-merge.mjs | 11 +++- scripts/release-candidate-checklist.d.mts | 11 ++++ .../close-duplicate-prs-after-merge.test.ts | 53 ++++++++++++++++++- 4 files changed, 92 insertions(+), 3 deletions(-) diff --git a/scripts/close-duplicate-prs-after-merge.d.mts b/scripts/close-duplicate-prs-after-merge.d.mts index ba10fb85037..e6bdfdc7708 100644 --- a/scripts/close-duplicate-prs-after-merge.d.mts +++ b/scripts/close-duplicate-prs-after-merge.d.mts @@ -25,6 +25,26 @@ export function parseArgs( landedPr: number; repo: string; }; +/** + * Runs a GitHub CLI command with the workflow timeout policy. + */ +export function defaultRunGh( + args: string[], + options?: { input?: string }, + params?: { + execFileSyncImpl?: ( + command: string, + args: string[], + options: { + encoding: "utf8"; + input?: string; + killSignal: "SIGKILL"; + stdio: ["ignore", "pipe", "inherit"] | ["pipe", "pipe", "inherit"]; + timeout: number; + }, + ) => string; + }, +): string; /** * Parses changed hunk ranges from unified diff text. */ diff --git a/scripts/close-duplicate-prs-after-merge.mjs b/scripts/close-duplicate-prs-after-merge.mjs index b49c858921c..ee49b40d2b5 100644 --- a/scripts/close-duplicate-prs-after-merge.mjs +++ b/scripts/close-duplicate-prs-after-merge.mjs @@ -3,6 +3,10 @@ import { execFileSync } from "node:child_process"; import { pathToFileURL } from "node:url"; const DEFAULT_LABELS = ["duplicate", "close:duplicate", "dedupe:child"]; +// Duplicate PR closure performs multiple sequential gh API reads and writes. +// Keep enough headroom for GitHub latency while preventing one stalled request +// from blocking the surrounding workflow job. +const GH_COMMAND_TIMEOUT_MS = 60_000; function usage() { return `Usage: node scripts/close-duplicate-prs-after-merge.mjs --landed-pr --duplicates [--repo owner/repo] [--apply] @@ -89,10 +93,13 @@ function ghJson(args, runGh) { return JSON.parse(runGh(args)); } -function defaultRunGh(args, options = {}) { - return execFileSync("gh", args, { +export function defaultRunGh(args, options = {}, params = {}) { + const execFileSyncImpl = params.execFileSyncImpl ?? execFileSync; + return execFileSyncImpl("gh", args, { encoding: "utf8", + killSignal: "SIGKILL", stdio: options.input ? ["pipe", "pipe", "inherit"] : ["ignore", "pipe", "inherit"], + timeout: GH_COMMAND_TIMEOUT_MS, ...(options.input ? { input: options.input } : {}), }); } diff --git a/scripts/release-candidate-checklist.d.mts b/scripts/release-candidate-checklist.d.mts index aa13b3cd3a9..5ed516b5290 100644 --- a/scripts/release-candidate-checklist.d.mts +++ b/scripts/release-candidate-checklist.d.mts @@ -120,6 +120,17 @@ export function validateCandidateCheckout({ toolingSha: unknown; workflowRef: unknown; }; +export function validateTrustedToolingPin({ + toolingSha, + pinnedToolingSha, + latestTrustedToolingSha, + isAncestor, +}: { + toolingSha: string; + pinnedToolingSha: string; + latestTrustedToolingSha: string; + isAncestor?: (ancestor: string, target: string) => boolean; +}): string; export function candidateCumulativeShippedPullRequests( changelog: string, label: string, diff --git a/test/scripts/close-duplicate-prs-after-merge.test.ts b/test/scripts/close-duplicate-prs-after-merge.test.ts index 6a38f235a9f..ef4a4715097 100644 --- a/test/scripts/close-duplicate-prs-after-merge.test.ts +++ b/test/scripts/close-duplicate-prs-after-merge.test.ts @@ -1,8 +1,9 @@ // Close Duplicate Prs After Merge tests cover close duplicate prs after merge script behavior. -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import { applyClosePlan, buildDuplicateClosePlan, + defaultRunGh, parseArgs, parsePrNumberList, parseUnifiedDiffRanges, @@ -253,3 +254,53 @@ Closing #70592 as a duplicate.`, ]); }); }); + +describe("defaultRunGh", () => { + it("bounds each GitHub lookup with a timeout and SIGKILL", () => { + const execFileSyncImpl = vi.fn(() => "result"); + + const result = defaultRunGh(["pr", "view", "123"], {}, { execFileSyncImpl }); + + expect(result).toBe("result"); + expect(execFileSyncImpl).toHaveBeenCalledWith( + "gh", + ["pr", "view", "123"], + expect.objectContaining({ + encoding: "utf8", + killSignal: "SIGKILL", + stdio: ["ignore", "pipe", "inherit"], + timeout: 60_000, + }), + ); + }); + + it("forwards stdin input while keeping the timeout bound", () => { + const execFileSyncImpl = vi.fn(() => "ok"); + + defaultRunGh( + ["pr", "edit", "123", "--add-label", "duplicate"], + { input: "body" }, + { execFileSyncImpl }, + ); + + expect(execFileSyncImpl).toHaveBeenCalledWith( + "gh", + ["pr", "edit", "123", "--add-label", "duplicate"], + expect.objectContaining({ + input: "body", + killSignal: "SIGKILL", + stdio: ["pipe", "pipe", "inherit"], + timeout: 60_000, + }), + ); + }); + + it("propagates timeout failures from the GitHub CLI process", () => { + const timeout = Object.assign(new Error("spawnSync gh ETIMEDOUT"), { code: "ETIMEDOUT" }); + const execFileSyncImpl = vi.fn(() => { + throw timeout; + }); + + expect(() => defaultRunGh(["pr", "view", "123"], {}, { execFileSyncImpl })).toThrow(timeout); + }); +});