From e4c65c3442c3fab9e9facbb4fb9d6711e9623b90 Mon Sep 17 00:00:00 2001 From: Derek Lewis Date: Sat, 15 Aug 2026 03:31:21 +0000 Subject: [PATCH 1/2] =?UTF-8?q?=F0=9F=8F=97=EF=B8=8F=F0=9F=94=A7=EF=BC=9As?= =?UTF-8?q?urvive=20a=20failed=20call=20to=20the=20GitHub=20API?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Landing #1815 crashed once with a stack trace and worked on a second run, unchanged, which is what a transient 502 or a throttled request looks like from here. Reads are now tried twice; a merge is not, since repeating one that already reached GitHub would report the pull request as unmergeable when it had in fact just merged. Anything still unexpected reports a line rather than a stack trace. That needed the repository, the default branch and the actor to be worked out when first wanted instead of as the file loads, since nothing could catch them failing up there. Signed-off-by: Derek Lewis Assisted-by: Claude-Code:claude-opus-5 --- build/tasks/land-pull-request.mts | 227 ++++++++++++++++++------------ 1 file changed, 138 insertions(+), 89 deletions(-) diff --git a/build/tasks/land-pull-request.mts b/build/tasks/land-pull-request.mts index f2d7a83b8..ced77ab42 100644 --- a/build/tasks/land-pull-request.mts +++ b/build/tasks/land-pull-request.mts @@ -32,30 +32,69 @@ type PullRequest = { const [number, ...flags] = process.argv.slice(2); const dryRun = flags.includes('--dry-run'); -const gh = (...args: string[]) => - execFileSync('gh', args, { encoding: 'utf8', maxBuffer: 1 << 24 }).trim(); -const git = (...args: string[]) => - execFileSync('git', args, { encoding: 'utf8', maxBuffer: 1 << 24 }).trim(); +const run = (command: string, args: string[]) => + execFileSync(command, args, { encoding: 'utf8', maxBuffer: 1 << 24 }).trim(); + +/** + * Runs `gh`, and tries a second time before giving up. A read that fails + * because GitHub returned a 502 or throttled the request is not a reason to + * refuse to land, and it happens often enough to have happened here: an + * earlier version crashed on one and worked when run again unchanged. + * @param {...string} args What to pass to `gh`. + * @returns {string} Its output. + */ +const gh = (...args: string[]) => { + try { + return run('gh', args); + } catch (first) { + // A merge is not safe to repeat blindly: if the first attempt reached + // GitHub, the second would report an already-merged pull request as a + // failure. Reads are. + if (args.includes('PUT')) throw first; + + return run('gh', args); + } +}; + +const git = (...args: string[]) => run('git', args); + +/** + * Works something out once, the first time it is wanted. Asked for at the top + * of the file instead, these would run before anything could catch them + * failing, and a broken `gh` would print a stack trace rather than a reason. + * @param {() => T} work How to find the answer. + * @returns {() => T} A function returning it, computed at most once. + */ +const once = (work: () => T) => { + let answer: T | undefined; + + return () => { + answer ??= work(); + + return answer; + }; +}; // Nothing here names a repository. A workflow sets GITHUB_REPOSITORY, and a // terminal has a remote to read it from, so a copy of this file lands in // another repository without being edited first. -const REPOSITORY = - process.env.GITHUB_REPOSITORY ?? - git('remote', 'get-url', 'origin').match( - /github\.com[/:](?[^/]+\/[^/]+?)(?:\.git)?$/ - )?.groups?.repo ?? - ''; - -const DEFAULT_BRANCH = gh( - 'api', - `repos/${REPOSITORY}`, - '--jq', - '.default_branch' +const repository = once( + () => + process.env.GITHUB_REPOSITORY ?? + git('remote', 'get-url', 'origin').match( + /github\.com[/:](?[^/]+\/[^/]+?)(?:\.git)?$/ + )?.groups?.repo ?? + '' +); + +const defaultBranch = once(() => + gh('api', `repos/${repository()}`, '--jq', '.default_branch') ); /** Who is asking for this to land: the labeller, or whoever is at the keyboard. */ -const ACTOR = process.env.LAND_ACTOR ?? gh('api', 'user', '--jq', '.login'); +const actor = once( + () => process.env.LAND_ACTOR ?? gh('api', 'user', '--jq', '.login') +); /** * Reads the pull request, waiting for GitHub to work out whether it merges. @@ -69,7 +108,7 @@ const readPull = async (pull: string): Promise => { JSON.parse( gh( 'api', - `repos/${REPOSITORY}/pulls/${pull}`, + `repos/${repository()}/pulls/${pull}`, '--jq', '{title, base: .base.ref, head: .head.sha, state, draft, mergeableState: .mergeable_state}' ) @@ -105,7 +144,7 @@ const checksRefuse = (sha: string) => JSON.parse( gh( 'api', - `repos/${REPOSITORY}/commits/${sha}/check-runs`, + `repos/${repository()}/commits/${sha}/check-runs`, '--jq', '[.check_runs[] | {name, status, conclusion}]' ) @@ -113,7 +152,7 @@ const checksRefuse = (sha: string) => JSON.parse( gh( 'api', - `repos/${REPOSITORY}/commits/${sha}/status`, + `repos/${repository()}/commits/${sha}/status`, '--jq', '[.statuses[] | {context, state}]' ) @@ -129,14 +168,14 @@ const checksRefuse = (sha: string) => const actorRefuses = () => { const permission = gh( 'api', - `repos/${REPOSITORY}/collaborators/${ACTOR}/permission`, + `repos/${repository()}/collaborators/${actor()}/permission`, '--jq', '.permission' ); return ['admin', 'maintain', 'write'].includes(permission) ? '' - : `${ACTOR} has ${permission} access, which does not carry the right to push`; + : `${actor()} has ${permission} access, which does not carry the right to push`; }; /** @@ -150,10 +189,10 @@ const refuse = (pull: PullRequest) => { } if (pull.mergeableState === 'dirty') { - return `it conflicts with ${DEFAULT_BRANCH}; rebase the branch onto it and push first`; + return `it conflicts with ${defaultBranch()}; rebase the branch onto it and push first`; } - if (pull.base !== DEFAULT_BRANCH) { + if (pull.base !== defaultBranch()) { return `it targets ${pull.base}; retarget it, or land its base first`; } @@ -161,87 +200,97 @@ const refuse = (pull: PullRequest) => { return 'GitHub has not worked out whether it merges yet; try again shortly'; } - const actor = actorRefuses(); + const unentitled = actorRefuses(); - if (actor !== '') return actor; + if (unentitled !== '') return unentitled; const checks = checksRefuse(pull.head); return checks === '' ? '' : `${checks}`; }; -if (number === undefined || !/^\d+$/.test(number)) { - console.error('Usage: nps "land [--dry-run]"'); - process.exitCode = 1; -} else { - const pull = await readPull(number); - const reason = refuse(pull); - - if (reason !== '') { - console.error(`#${number} cannot be landed: ${reason}.`); +// Anything unexpected -- a network failure that outlived its retry, a +// command that is not installed -- should say so in a line rather than +// print a stack trace at whoever applied a label. +try { + if (number === undefined || !/^\d+$/.test(number)) { + console.error('Usage: nps "land [--dry-run]"'); process.exitCode = 1; } else { - // Oldest first, so the landed message reads in the order the work was - // done rather than the order git lists it. - const shas = git( - 'rev-list', - '--reverse', - '--no-merges', - `origin/${DEFAULT_BRANCH}..${pull.head}` - ) - .split('\n') - .filter(Boolean); - const parts = shas.map((sha) => - partsOfMessage(git('log', '-1', '--format=%B', sha)) - ); - const message = composeLandingMessage( - parts, - `https://github.com/${REPOSITORY}/pull/${number}` - ); - const problems = validateCommitMessage(`${pull.title}\n\n${message}`); - const rule = '='.repeat(72); + const pull = await readPull(number); + const reason = refuse(pull); - console.log(`${rule}\n${pull.title}\n\n${message}\n${rule}`); - console.log( - `#${number}: ${shas.length} commit${shas.length === 1 ? '' : 's'}, ${pull.mergeableState}` - ); - - if (shas.length === 0) { - console.error(`\n#${number} has no commits over ${DEFAULT_BRANCH}.`); - process.exitCode = 1; - } else if (problems.length > 0) { - console.error( - '\nThe message this would land does not pass its own rules:' - ); - for (const problem of problems) console.error(` ${problem}`); + if (reason !== '') { + console.error(`#${number} cannot be landed: ${reason}.`); process.exitCode = 1; - } else if (dryRun) { - console.log('\nDry run; nothing landed.'); } else { - const merged = JSON.parse( - gh( - 'api', - '-X', - 'PUT', - `repos/${REPOSITORY}/pulls/${number}/merge`, - '-f', - 'merge_method=squash', - '-f', - `commit_title=${pull.title}`, - '-f', - `commit_message=${message}`, - '-f', - `sha=${pull.head}`, - '--jq', - '{merged, sha}' - ) + // Oldest first, so the landed message reads in the order the work was + // done rather than the order git lists it. + const shas = git( + 'rev-list', + '--reverse', + '--no-merges', + `origin/${defaultBranch()}..${pull.head}` + ) + .split('\n') + .filter(Boolean); + const parts = shas.map((sha) => + partsOfMessage(git('log', '-1', '--format=%B', sha)) + ); + const message = composeLandingMessage( + parts, + `https://github.com/${repository()}/pull/${number}` + ); + const problems = validateCommitMessage(`${pull.title}\n\n${message}`); + const rule = '='.repeat(72); + + console.log(`${rule}\n${pull.title}\n\n${message}\n${rule}`); + console.log( + `#${number}: ${shas.length} commit${shas.length === 1 ? '' : 's'}, ${pull.mergeableState}` ); - if (merged.merged === true) console.log(`\nLanded as ${merged.sha}`); - else { - console.error(`\n#${number} did not merge.`); + if (shas.length === 0) { + console.error(`\n#${number} has no commits over ${defaultBranch()}.`); process.exitCode = 1; + } else if (problems.length > 0) { + console.error( + '\nThe message this would land does not pass its own rules:' + ); + for (const problem of problems) console.error(` ${problem}`); + process.exitCode = 1; + } else if (dryRun) { + console.log('\nDry run; nothing landed.'); + } else { + const merged = JSON.parse( + gh( + 'api', + '-X', + 'PUT', + `repos/${repository()}/pulls/${number}/merge`, + '-f', + 'merge_method=squash', + '-f', + `commit_title=${pull.title}`, + '-f', + `commit_message=${message}`, + '-f', + `sha=${pull.head}`, + '--jq', + '{merged, sha}' + ) + ); + + if (merged.merged === true) console.log(`\nLanded as ${merged.sha}`); + else { + console.error(`\n#${number} did not merge.`); + process.exitCode = 1; + } } } } +} catch (error) { + console.error( + `Could not land #${number}: ${error instanceof Error ? error.message.split(String.fromCharCode(10))[0] : String(error)}` + ); + process.exitCode = 1; } From 90a68f7de9714167bf732cfc8a27fa31781e71f9 Mon Sep 17 00:00:00 2001 From: Derek Lewis Date: Sat, 15 Aug 2026 03:36:56 +0000 Subject: [PATCH 2/2] =?UTF-8?q?=F0=9F=8F=97=EF=B8=8F=F0=9F=94=A7=EF=BC=9As?= =?UTF-8?q?top=20the=20queue=20waiting=20for=20itself?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The queue reports as a check like any other job, so requiring every check to have finished meant requiring itself to have finished, which it cannot do until it stops waiting. Labelling #1816 deadlocked on exactly that, which is what an end-to-end test is for. Its own run is identified by the run id in the check's details URL rather than by the job's name, so renaming the job cannot quietly reintroduce the deadlock. Every other run still counts, including another queue run on the same commit. Signed-off-by: Derek Lewis Assisted-by: Claude-Code:claude-opus-5 --- build/shared/landing.mts | 17 +++++++++++++++- build/shared/landing.test.mts | 33 +++++++++++++++++++++++++++++++ build/tasks/land-pull-request.mts | 5 +++-- 3 files changed, 52 insertions(+), 3 deletions(-) diff --git a/build/shared/landing.mts b/build/shared/landing.mts index 3f83f43bc..0de64f7c7 100644 --- a/build/shared/landing.mts +++ b/build/shared/landing.mts @@ -98,6 +98,7 @@ export type CheckRun = { name: string; status: string; conclusion: string | null; + detailsUrl?: string; }; /** What GitHub reports about one commit status, which is the older kind. */ @@ -108,11 +109,25 @@ export type CommitStatus = { context: string; state: string }; * running counts against it: a label applied while a check was in flight says * nothing about how that check turned out. Neutral and skipped do not count * against it, since neither is a complaint. + * The queue is itself a check, so its own run is left out. Waiting for it + * would be waiting for a job that cannot finish until it stops waiting. * @param {CheckRun[]} runs The check runs reported on the commit. * @param {CommitStatus[]} statuses The commit statuses reported on it. + * @param {string} ownRunId The workflow run doing the asking, if it is one. * @returns {string} The reason, or an empty string if there is none. */ -export function checksVerdict(runs: CheckRun[], statuses: CommitStatus[]) { +export function checksVerdict( + all: CheckRun[], + statuses: CommitStatus[], + ownRunId = '' +) { + const runs = + ownRunId === '' + ? all + : all.filter( + (run) => + !(run.detailsUrl ?? '').includes(`/actions/runs/${ownRunId}/`) + ); const pending = [ ...runs.filter((run) => run.status !== 'completed').map((run) => run.name), ...statuses diff --git a/build/shared/landing.test.mts b/build/shared/landing.test.mts index 4317d076c..0e3fcf5b1 100644 --- a/build/shared/landing.test.mts +++ b/build/shared/landing.test.mts @@ -241,3 +241,36 @@ describe('checksVerdict', () => { ); }); }); + +describe('checksVerdict and its own run', () => { + test('does not wait for the queue that is doing the asking', () => { + // The queue reports as a check itself, so counting it would mean waiting + // for a job that cannot finish until it stops waiting. + const own = { + name: 'Land', + status: 'in_progress', + conclusion: null, + detailsUrl: 'https://github.com/o/r/actions/runs/999/job/1', + }; + + deepStrictEqual( + checksVerdict( + [own, { name: 'Lint', status: 'completed', conclusion: 'success' }], + [], + '999' + ), + '' + ); + }); + + test('still waits for a different run', () => { + const other = { + name: 'Lint', + status: 'in_progress', + conclusion: null, + detailsUrl: 'https://github.com/o/r/actions/runs/1000/job/1', + }; + + match(checksVerdict([other], [], '999'), /have not finished: Lint/); + }); +}); diff --git a/build/tasks/land-pull-request.mts b/build/tasks/land-pull-request.mts index ced77ab42..23702ea0c 100644 --- a/build/tasks/land-pull-request.mts +++ b/build/tasks/land-pull-request.mts @@ -146,7 +146,7 @@ const checksRefuse = (sha: string) => 'api', `repos/${repository()}/commits/${sha}/check-runs`, '--jq', - '[.check_runs[] | {name, status, conclusion}]' + '[.check_runs[] | {name, status, conclusion, detailsUrl: .details_url}]' ) ), JSON.parse( @@ -156,7 +156,8 @@ const checksRefuse = (sha: string) => '--jq', '[.statuses[] | {context, state}]' ) - ) + ), + process.env.GITHUB_RUN_ID ?? '' ); /**