From 87ba5f0e337f0ba15dec44f4784130c78b675b06 Mon Sep 17 00:00:00 2001 From: David Edey Date: Mon, 17 Aug 2026 17:43:59 +0000 Subject: [PATCH] fix: don't fail the check group on transient GitHub API errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A GitHub incident could fail `check-group` on a PR whose required checks were all green. In gridai/grid run 32048244470 the loop logged "All required checks were successful!" and then crashed, because the PATCH updating the PR comment came back 503 after octokit had exhausted its own retries. Three things went wrong, all fixed here: - `notifyProgress` was called without `await`, so the rethrow of a non-403 error became an unhandled rejection that took the process down. It is now awaited, and a failure to write the comment is only warned about — the comment is informational, and the check statuses it summarises are unaffected by GitHub refusing to store it. - Any error inside the poll loop went straight to `core.setFailed`, so a single blip ended a run that had 40 minutes of budget left. Transient errors now warn and poll again on the next interval; the timeout timer still bounds the run, and names the API error if one was the last thing seen. - The 403 branch tested `e instanceof RequestError`, which never matched: several copies of `@octokit/request-error` are installed side by side and the throwing one is a different class. Transient and 403 classification now duck-type on `status`. The one-shot calls made before the loop starts — listing the PR's files and reading checkgroup.yml — have no later poll to fall back on, so they retry with backoff. Co-Authored-By: Claude Opus 5 (1M context) --- dist/check-group/core/config_getter.js | 3 +- dist/check-group/core/index.js | 41 ++++-- dist/check-group/core/transient_error.js | 167 +++++++++++++++++++++++ src/check-group/core/config_getter.ts | 6 +- src/check-group/core/index.ts | 39 ++++-- src/check-group/core/transient_error.ts | 78 +++++++++++ 6 files changed, 312 insertions(+), 22 deletions(-) create mode 100644 dist/check-group/core/transient_error.js create mode 100644 src/check-group/core/transient_error.ts diff --git a/dist/check-group/core/config_getter.js b/dist/check-group/core/config_getter.js index ea576fb2c..48dea75e1 100644 --- a/dist/check-group/core/config_getter.js +++ b/dist/check-group/core/config_getter.js @@ -73,6 +73,7 @@ Object.defineProperty(exports, "__esModule", { value: true }); exports.fetchConfig = void 0; var user_config_parser_1 = require("./user_config_parser"); var core = __importStar(require("@actions/core")); +var transient_error_1 = require("./transient_error"); /** * Fetches the app configuration from the user's repository. * @@ -116,7 +117,7 @@ var readConfig = function (context, branch) { return __awaiter(void 0, void 0, v switch (_a.label) { case 0: params = context.repo({ path: '.github/checkgroup.yml' }); - return [4 /*yield*/, context.octokit.config.get(__assign(__assign({}, params), { branch: branch }))]; + return [4 /*yield*/, (0, transient_error_1.withTransientRetry)("Reading '.github/checkgroup.yml' from '".concat(branch, "'"), function () { return context.octokit.config.get(__assign(__assign({}, params), { branch: branch })); })]; case 1: config = (_a.sent()).config; return [2 /*return*/, config]; diff --git a/dist/check-group/core/index.js b/dist/check-group/core/index.js index 92f9ed0e1..35d67654d 100644 --- a/dist/check-group/core/index.js +++ b/dist/check-group/core/index.js @@ -69,7 +69,7 @@ var subproj_matching_1 = require("./subproj_matching"); var satisfy_expected_checks_1 = require("./satisfy_expected_checks"); var config_getter_1 = require("./config_getter"); Object.defineProperty(exports, "fetchConfig", { enumerable: true, get: function () { return config_getter_1.fetchConfig; } }); -var request_error_1 = require("@octokit/request-error"); +var transient_error_1 = require("./transient_error"); /** * The orchestration class. */ @@ -79,6 +79,7 @@ var CheckGroup = /** @class */ (function () { this.timeoutTimer = setTimeout(function () { return ''; }, 0); this.inputs = {}; this.canComment = true; + this.lastTransientError = ""; this.pullRequestNumber = pullRequestNumber; this.config = config; this.context = context; @@ -115,6 +116,9 @@ var CheckGroup = /** @class */ (function () { this.timeoutTimer = setTimeout(function () { clearTimeout(_this.intervalTimer); core.setFailed("The timeout of ".concat(timeout, " minutes has triggered but not all required jobs were passing.") + + (_this.lastTransientError + ? " The GitHub API was also returning errors, the last being: ".concat(_this.lastTransientError) + : "") + " This job will need to be re-run to merge your PR." + " If you do not have write access to the repository you can ask ".concat(maintainers, " to re-run it for you.") + " If you have any other questions, you can reach out to ".concat(owner, " for help.")); @@ -131,7 +135,7 @@ var CheckGroup = /** @class */ (function () { return __generator(this, function (_a) { switch (_a.label) { case 0: - _a.trys.push([0, 2, , 3]); + _a.trys.push([0, 3, , 4]); // print in a group to reduce verbosity core.startGroup("Check ".concat(tries)); return [4 /*yield*/, getPostedChecks(this.context, this.sha)]; @@ -139,7 +143,9 @@ var CheckGroup = /** @class */ (function () { postedChecks = _a.sent(); core.debug("postedChecks: ".concat(JSON.stringify(postedChecks))); result = (0, satisfy_expected_checks_1.getSubProjResult)(subprojs, postedChecks); - this.notifyProgress(subprojs, postedChecks, result); + return [4 /*yield*/, this.notifyProgress(subprojs, postedChecks, result)]; + case 2: + _a.sent(); core.endGroup(); if (result === "all_passing") { core.info("All required checks were successful!"); @@ -149,15 +155,25 @@ var CheckGroup = /** @class */ (function () { else { this.intervalTimer = setTimeout(function () { return _this.runCheck(subprojs, tries + 1, interval); }, interval); } - return [3 /*break*/, 3]; - case 2: + return [3 /*break*/, 4]; + case 3: error_1 = _a.sent(); + core.endGroup(); + if ((0, transient_error_1.isTransientError)(error_1)) { + // A GitHub incident says nothing about the PR, so keep polling until the + // timeout timer fires rather than failing a PR whose checks are green. + this.lastTransientError = (0, transient_error_1.describeError)(error_1); + core.warning("Check ".concat(tries, " hit a transient GitHub API error, retrying in ").concat(interval / 1000, "s:") + + " ".concat(this.lastTransientError)); + this.intervalTimer = setTimeout(function () { return _this.runCheck(subprojs, tries + 1, interval); }, interval); + return [2 /*return*/]; + } // bubble up the error to the job core.setFailed(error_1); clearTimeout(this.intervalTimer); clearTimeout(this.timeoutTimer); - return [3 /*break*/, 3]; - case 3: return [2 /*return*/]; + return [3 /*break*/, 4]; + case 4: return [2 /*return*/]; } }); }); @@ -179,16 +195,18 @@ var CheckGroup = /** @class */ (function () { return [3 /*break*/, 4]; case 3: e_1 = _a.sent(); - if (e_1 instanceof request_error_1.RequestError && e_1.status === 403) { + // The comment is informational, so a failure to write it must never fail + // the job — the check statuses it summarises are unaffected. + if ((0, transient_error_1.httpStatus)(e_1) === 403) { // Forbidden: Resource not accessible by integration if (this.canComment) { - core.info("Failed to comment on the PR: ".concat(JSON.stringify(e_1))); + core.info("Failed to comment on the PR: ".concat((0, transient_error_1.describeError)(e_1))); } // Use this boolean to only print the info message once this.canComment = false; } else { - throw e_1; + core.warning("Failed to update the PR comment: ".concat((0, transient_error_1.describeError)(e_1))); } return [3 /*break*/, 4]; case 4: return [2 /*return*/]; @@ -203,9 +221,10 @@ var CheckGroup = /** @class */ (function () { CheckGroup.prototype.files = function () { return __awaiter(this, void 0, void 0, function () { var pullRequestFiles, filenames; + var _this = this; return __generator(this, function (_a) { switch (_a.label) { - case 0: return [4 /*yield*/, this.context.octokit.paginate(this.context.octokit.pulls.listFiles, this.context.repo({ "pull_number": this.pullRequestNumber }), function (response) { return response.data; })]; + case 0: return [4 /*yield*/, (0, transient_error_1.withTransientRetry)("Listing the files changed in the PR", function () { return _this.context.octokit.paginate(_this.context.octokit.pulls.listFiles, _this.context.repo({ "pull_number": _this.pullRequestNumber }), function (response) { return response.data; }); })]; case 1: pullRequestFiles = _a.sent(); filenames = []; diff --git a/dist/check-group/core/transient_error.js b/dist/check-group/core/transient_error.js new file mode 100644 index 000000000..6af60cebe --- /dev/null +++ b/dist/check-group/core/transient_error.js @@ -0,0 +1,167 @@ +"use strict"; +var __createBinding = (this && this.__createBinding) || (Object.create ? (function(o, m, k, k2) { + if (k2 === undefined) k2 = k; + var desc = Object.getOwnPropertyDescriptor(m, k); + if (!desc || ("get" in desc ? !m.__esModule : desc.writable || desc.configurable)) { + desc = { enumerable: true, get: function() { return m[k]; } }; + } + Object.defineProperty(o, k2, desc); +}) : (function(o, m, k, k2) { + if (k2 === undefined) k2 = k; + o[k2] = m[k]; +})); +var __setModuleDefault = (this && this.__setModuleDefault) || (Object.create ? (function(o, v) { + Object.defineProperty(o, "default", { enumerable: true, value: v }); +}) : function(o, v) { + o["default"] = v; +}); +var __importStar = (this && this.__importStar) || function (mod) { + if (mod && mod.__esModule) return mod; + var result = {}; + if (mod != null) for (var k in mod) if (k !== "default" && Object.prototype.hasOwnProperty.call(mod, k)) __createBinding(result, mod, k); + __setModuleDefault(result, mod); + return result; +}; +var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, generator) { + function adopt(value) { return value instanceof P ? value : new P(function (resolve) { resolve(value); }); } + return new (P || (P = Promise))(function (resolve, reject) { + function fulfilled(value) { try { step(generator.next(value)); } catch (e) { reject(e); } } + function rejected(value) { try { step(generator["throw"](value)); } catch (e) { reject(e); } } + function step(result) { result.done ? resolve(result.value) : adopt(result.value).then(fulfilled, rejected); } + step((generator = generator.apply(thisArg, _arguments || [])).next()); + }); +}; +var __generator = (this && this.__generator) || function (thisArg, body) { + var _ = { label: 0, sent: function() { if (t[0] & 1) throw t[1]; return t[1]; }, trys: [], ops: [] }, f, y, t, g; + return g = { next: verb(0), "throw": verb(1), "return": verb(2) }, typeof Symbol === "function" && (g[Symbol.iterator] = function() { return this; }), g; + function verb(n) { return function (v) { return step([n, v]); }; } + function step(op) { + if (f) throw new TypeError("Generator is already executing."); + while (_) try { + if (f = 1, y && (t = op[0] & 2 ? y["return"] : op[0] ? y["throw"] || ((t = y["return"]) && t.call(y), 0) : y.next) && !(t = t.call(y, op[1])).done) return t; + if (y = 0, t) op = [op[0] & 2, t.value]; + switch (op[0]) { + case 0: case 1: t = op; break; + case 4: _.label++; return { value: op[1], done: false }; + case 5: _.label++; y = op[1]; op = [0]; continue; + case 7: op = _.ops.pop(); _.trys.pop(); continue; + default: + if (!(t = _.trys, t = t.length > 0 && t[t.length - 1]) && (op[0] === 6 || op[0] === 2)) { _ = 0; continue; } + if (op[0] === 3 && (!t || (op[1] > t[0] && op[1] < t[3]))) { _.label = op[1]; break; } + if (op[0] === 6 && _.label < t[1]) { _.label = t[1]; t = op; break; } + if (t && _.label < t[2]) { _.label = t[2]; _.ops.push(op); break; } + if (t[2]) _.ops.pop(); + _.trys.pop(); continue; + } + op = body.call(thisArg, _); + } catch (e) { op = [6, e]; y = 0; } finally { f = t = 0; } + if (op[0] & 5) throw op[1]; return { value: op[0] ? op[1] : void 0, done: true }; + } +}; +Object.defineProperty(exports, "__esModule", { value: true }); +exports.withTransientRetry = exports.describeError = exports.isTransientError = exports.httpStatus = void 0; +/** + * Helpers for surviving GitHub API errors that say nothing about the PR being + * checked — 5xx during an incident, rate limiting, or a dropped connection. + */ +var core = __importStar(require("@actions/core")); +/** + * Several copies of `@octokit/request-error` end up installed side by side, so + * the class an octokit error is an instance of depends on which copy threw it + * and `instanceof RequestError` can't be relied on. The `status` property is + * stable across all of them. + */ +var httpStatus = function (error) { + var status = error === null || error === void 0 ? void 0 : error.status; + return typeof status === 'number' ? status : undefined; +}; +exports.httpStatus = httpStatus; +var TRANSIENT_NETWORK_CODES = new Set([ + 'ECONNABORTED', + 'ECONNREFUSED', + 'ECONNRESET', + 'EAI_AGAIN', + 'EHOSTUNREACH', + 'ENETUNREACH', + 'ENOTFOUND', + 'EPIPE', + 'ETIMEDOUT', + 'UND_ERR_CONNECT_TIMEOUT', + 'UND_ERR_SOCKET' +]); +var isTransientError = function (error) { + var status = (0, exports.httpStatus)(error); + if (status !== undefined) { + // 408 request timeout, 429 rate/abuse limit, 5xx server or gateway failure + return status >= 500 || status === 408 || status === 429; + } + var code = error === null || error === void 0 ? void 0 : error.code; + if (typeof code === 'string' && TRANSIENT_NETWORK_CODES.has(code)) { + return true; + } + var message = error === null || error === void 0 ? void 0 : error.message; + return typeof message === 'string' && /socket hang up|network timeout|request to .* failed/i.test(message); +}; +exports.isTransientError = isTransientError; +var describeError = function (error) { + var _a; + var status = (0, exports.httpStatus)(error); + var message = (_a = error === null || error === void 0 ? void 0 : error.message) !== null && _a !== void 0 ? _a : String(error); + return status === undefined ? "".concat(message) : "[HTTP ".concat(status, "] ").concat(message); +}; +exports.describeError = describeError; +/** + * Runs `operation`, retrying it on transient failures. Used for the one-shot + * calls made before the check loop starts, which have no later poll to fall + * back on. + */ +var withTransientRetry = function (description, operation, attempts, delayMs) { + if (attempts === void 0) { attempts = 5; } + if (delayMs === void 0) { delayMs = 5000; } + return __awaiter(void 0, void 0, void 0, function () { + var _loop_1, attempt, state_1; + return __generator(this, function (_a) { + switch (_a.label) { + case 0: + _loop_1 = function (attempt) { + var _b, error_1, backoffMs_1; + return __generator(this, function (_c) { + switch (_c.label) { + case 0: + _c.trys.push([0, 2, , 4]); + _b = {}; + return [4 /*yield*/, operation()]; + case 1: return [2 /*return*/, (_b.value = _c.sent(), _b)]; + case 2: + error_1 = _c.sent(); + if (attempt >= attempts || !(0, exports.isTransientError)(error_1)) { + throw error_1; + } + backoffMs_1 = delayMs * attempt; + core.warning("".concat(description, " failed with a transient error, retrying in ").concat(backoffMs_1 / 1000, "s") + + " (attempt ".concat(attempt, "/").concat(attempts, "): ").concat((0, exports.describeError)(error_1))); + return [4 /*yield*/, new Promise(function (resolve) { return setTimeout(resolve, backoffMs_1); })]; + case 3: + _c.sent(); + return [3 /*break*/, 4]; + case 4: return [2 /*return*/]; + } + }); + }; + attempt = 1; + _a.label = 1; + case 1: return [5 /*yield**/, _loop_1(attempt)]; + case 2: + state_1 = _a.sent(); + if (typeof state_1 === "object") + return [2 /*return*/, state_1.value]; + _a.label = 3; + case 3: + attempt++; + return [3 /*break*/, 1]; + case 4: return [2 /*return*/]; + } + }); + }); +}; +exports.withTransientRetry = withTransientRetry; diff --git a/src/check-group/core/config_getter.ts b/src/check-group/core/config_getter.ts index dcc11e99b..fb19ffa53 100644 --- a/src/check-group/core/config_getter.ts +++ b/src/check-group/core/config_getter.ts @@ -3,6 +3,7 @@ import { Context } from "probot"; import { parseUserConfig } from "./user_config_parser"; import { PullRequestEvent } from '@octokit/webhooks-types'; import * as core from '@actions/core' +import { withTransientRetry } from "./transient_error"; /** * Fetches the app configuration from the user's repository. @@ -32,6 +33,9 @@ export const fetchConfig = async (context: Context): Promise = const readConfig = async (context: Context, branch: string): Promise> => { const params = context.repo({path: '.github/checkgroup.yml'}) // https://github.com/probot/octokit-plugin-config - const { config } = await context.octokit.config.get({...params, branch: branch}) + const { config } = await withTransientRetry( + `Reading '.github/checkgroup.yml' from '${branch}'`, + () => context.octokit.config.get({...params, branch: branch}), + ) return config } diff --git a/src/check-group/core/index.ts b/src/check-group/core/index.ts index 1422d84c1..3e5147dc1 100644 --- a/src/check-group/core/index.ts +++ b/src/check-group/core/index.ts @@ -12,7 +12,7 @@ import { getSubProjResult } from "./satisfy_expected_checks"; import { fetchConfig } from "./config_getter"; import type { CheckGroupConfig, CheckResult, SubProjConfig } from "../types"; import type { Context } from "probot"; -import { RequestError } from "@octokit/request-error"; +import { describeError, httpStatus, isTransientError, withTransientRetry } from "./transient_error"; /** * The orchestration class. @@ -28,6 +28,7 @@ export class CheckGroup { inputs: Record = {}; canComment: boolean = true; + lastTransientError: string = ""; constructor( pullRequestNumber: number, @@ -73,6 +74,9 @@ export class CheckGroup { clearTimeout(this.intervalTimer) core.setFailed( `The timeout of ${timeout} minutes has triggered but not all required jobs were passing.` + + (this.lastTransientError + ? ` The GitHub API was also returning errors, the last being: ${this.lastTransientError}` + : ``) + ` This job will need to be re-run to merge your PR.` + ` If you do not have write access to the repository you can ask ${maintainers} to re-run it for you.` + ` If you have any other questions, you can reach out to ${owner} for help.` @@ -88,7 +92,7 @@ export class CheckGroup { const postedChecks = await getPostedChecks(this.context, this.sha); core.debug(`postedChecks: ${JSON.stringify(postedChecks)}`); const result = getSubProjResult(subprojs, postedChecks); - this.notifyProgress(subprojs, postedChecks, result) + await this.notifyProgress(subprojs, postedChecks, result) core.endGroup(); if (result === "all_passing") { @@ -100,6 +104,18 @@ export class CheckGroup { } } catch (error) { + core.endGroup(); + if (isTransientError(error)) { + // A GitHub incident says nothing about the PR, so keep polling until the + // timeout timer fires rather than failing a PR whose checks are green. + this.lastTransientError = describeError(error) + core.warning( + `Check ${tries} hit a transient GitHub API error, retrying in ${interval / 1000}s:` + + ` ${this.lastTransientError}` + ) + this.intervalTimer = setTimeout(() => this.runCheck(subprojs, tries + 1, interval), interval); + return; + } // bubble up the error to the job core.setFailed(error); clearTimeout(this.intervalTimer) @@ -119,15 +135,17 @@ export class CheckGroup { try { await commentOnPr(this.context, result, this.inputs, subprojs, postedChecks) } catch (e) { - if (e instanceof RequestError && e.status === 403) { + // The comment is informational, so a failure to write it must never fail + // the job — the check statuses it summarises are unaffected. + if (httpStatus(e) === 403) { // Forbidden: Resource not accessible by integration if (this.canComment) { - core.info(`Failed to comment on the PR: ${JSON.stringify(e)}`) + core.info(`Failed to comment on the PR: ${describeError(e)}`) } // Use this boolean to only print the info message once this.canComment = false } else { - throw e + core.warning(`Failed to update the PR comment: ${describeError(e)}`) } } } @@ -137,10 +155,13 @@ export class CheckGroup { * a pull request. */ async files(): Promise { - const pullRequestFiles = await this.context.octokit.paginate( - this.context.octokit.pulls.listFiles, - this.context.repo({"pull_number": this.pullRequestNumber}), - (response) => response.data, + const pullRequestFiles = await withTransientRetry( + "Listing the files changed in the PR", + () => this.context.octokit.paginate( + this.context.octokit.pulls.listFiles, + this.context.repo({"pull_number": this.pullRequestNumber}), + (response) => response.data, + ), ); const filenames: string[] = []; pullRequestFiles.forEach((pullRequestFile: any) => { diff --git a/src/check-group/core/transient_error.ts b/src/check-group/core/transient_error.ts new file mode 100644 index 000000000..86381a85b --- /dev/null +++ b/src/check-group/core/transient_error.ts @@ -0,0 +1,78 @@ +/** + * Helpers for surviving GitHub API errors that say nothing about the PR being + * checked — 5xx during an incident, rate limiting, or a dropped connection. + */ +import * as core from '@actions/core'; + +/** + * Several copies of `@octokit/request-error` end up installed side by side, so + * the class an octokit error is an instance of depends on which copy threw it + * and `instanceof RequestError` can't be relied on. The `status` property is + * stable across all of them. + */ +export const httpStatus = (error: unknown): number | undefined => { + const status = (error as {status?: unknown})?.status; + return typeof status === 'number' ? status : undefined; +}; + +const TRANSIENT_NETWORK_CODES = new Set([ + 'ECONNABORTED', + 'ECONNREFUSED', + 'ECONNRESET', + 'EAI_AGAIN', + 'EHOSTUNREACH', + 'ENETUNREACH', + 'ENOTFOUND', + 'EPIPE', + 'ETIMEDOUT', + 'UND_ERR_CONNECT_TIMEOUT', + 'UND_ERR_SOCKET' +]); + +export const isTransientError = (error: unknown): boolean => { + const status = httpStatus(error); + if (status !== undefined) { + // 408 request timeout, 429 rate/abuse limit, 5xx server or gateway failure + return status >= 500 || status === 408 || status === 429; + } + const code = (error as {code?: unknown})?.code; + if (typeof code === 'string' && TRANSIENT_NETWORK_CODES.has(code)) { + return true; + } + const message = (error as {message?: unknown})?.message; + return typeof message === 'string' && /socket hang up|network timeout|request to .* failed/i.test(message); +}; + +export const describeError = (error: unknown): string => { + const status = httpStatus(error); + const message = (error as {message?: unknown})?.message ?? String(error); + return status === undefined ? `${message}` : `[HTTP ${status}] ${message}`; +}; + +/** + * Runs `operation`, retrying it on transient failures. Used for the one-shot + * calls made before the check loop starts, which have no later poll to fall + * back on. + */ +export const withTransientRetry = async ( + description: string, + operation: () => Promise, + attempts = 5, + delayMs = 5000 +): Promise => { + for (let attempt = 1; ; attempt++) { + try { + return await operation(); + } catch (error) { + if (attempt >= attempts || !isTransientError(error)) { + throw error; + } + const backoffMs = delayMs * attempt; + core.warning( + `${description} failed with a transient error, retrying in ${backoffMs / 1000}s` + + ` (attempt ${attempt}/${attempts}): ${describeError(error)}` + ); + await new Promise(resolve => setTimeout(resolve, backoffMs)); + } + } +};