From c0dc781f472fdd20866e8d4998a9b3c719310dde Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Sat, 5 Sep 2026 16:10:16 -0700 Subject: [PATCH] fix: make execGh's gh-failure backoff cap configurable per caller updateChecker's 30-min scheduler interval always exceeds the 15-min backoff cap #6345 hardcoded, so the cooldown from a failed release check expires before every subsequent tick and never actually suppresses a retry -- a silent no-op for its only production caller. execGh now takes an optional backoffMaxMs override; branch-reconcile and the other pollers keep the default (appropriate for their much faster tick rate), and updateChecker passes 3x its own interval so repeated failures actually widen the gap between real attempts. --- server/services/github.js | 15 ++++++++---- server/services/github.test.js | 34 +++++++++++++++++++++++++++ server/services/updateChecker.js | 8 +++++-- server/services/updateChecker.test.js | 7 ++++-- 4 files changed, 55 insertions(+), 9 deletions(-) diff --git a/server/services/github.js b/server/services/github.js index bc375a268f..edd9bc597c 100644 --- a/server/services/github.js +++ b/server/services/github.js @@ -96,13 +96,13 @@ function ghBackoffActive(key, now = Date.now()) { return Boolean(entry && now < entry.retryAfter); } -function recordGhCallOutcome(key, ok, now = Date.now()) { +function recordGhCallOutcome(key, ok, maxMs = GH_BACKOFF_MAX_MS, now = Date.now()) { if (ok) { ghCallBackoff.delete(key); return; } const failures = (ghCallBackoff.get(key)?.failures || 0) + 1; - const delay = Math.min(GH_BACKOFF_BASE_MS * 2 ** (failures - 1), GH_BACKOFF_MAX_MS); + const delay = Math.min(GH_BACKOFF_BASE_MS * 2 ** (failures - 1), maxMs); ghCallBackoff.set(key, { failures, retryAfter: now + delay }); } @@ -120,9 +120,14 @@ export function __resetGhCallBackoff() { * cleared on normal exit so it never fires for a completed run. `input`, when * supplied, is written to stdin (used by structured `gh api --input -` calls). * `backoffKey`, when supplied, opts this call into the consecutive-failure - * backoff above — see the comment there for why it's opt-in. + * backoff above — see the comment there for why it's opt-in. `backoffMaxMs` + * overrides the default cap: it MUST exceed the calling scheduler's own tick + * interval, or the cooldown always expires before the next tick and never + * actually suppresses a retry — a real attempt fires every tick regardless of + * `backoffKey` (caught in review on the updateChecker caller: its 30-min + * interval exceeds the 15-min default cap, so the backoff there was a no-op). */ -export function execGh(args, timeoutMs = DEFAULT_EXEC_GH_TIMEOUT_MS, { cwd = null, env = null, input = null, backoffKey = null } = {}) { +export function execGh(args, timeoutMs = DEFAULT_EXEC_GH_TIMEOUT_MS, { cwd = null, env = null, input = null, backoffKey = null, backoffMaxMs = GH_BACKOFF_MAX_MS } = {}) { if (backoffKey !== null && ghBackoffActive(backoffKey)) { return Promise.reject(new Error(`gh command backing off for ${backoffKey} after repeated failures`)); } @@ -140,7 +145,7 @@ export function execGh(args, timeoutMs = DEFAULT_EXEC_GH_TIMEOUT_MS, { cwd = nul const settle = (ok) => { if (settled) return; settled = true; - if (backoffKey !== null) recordGhCallOutcome(backoffKey, ok); + if (backoffKey !== null) recordGhCallOutcome(backoffKey, ok, backoffMaxMs); }; const timer = setTimeout(() => { timedOut = true; diff --git a/server/services/github.test.js b/server/services/github.test.js index 8911590652..e2199167f8 100644 --- a/server/services/github.test.js +++ b/server/services/github.test.js @@ -253,6 +253,40 @@ describe('execGh backoffKey', () => { await expect(p2).rejects.toThrow(/exited with code 1/); // real second attempt, not a backoff rejection expect(spawn).toHaveBeenCalledTimes(2); }); + + it('honors a caller-supplied backoffMaxMs instead of the default cap', async () => { + // Regression for a real defect: a caller whose own retry interval exceeds + // the default 15-min cap (e.g. updateChecker's 30-min scheduler) would see + // the cooldown always expire before its next attempt, making the backoff a + // silent no-op — a real gh call fires every tick regardless of consecutive + // failures. A longer backoffMaxMs must actually widen the window past what + // the default cap would allow. + // + // 6 consecutive failures push the uncapped exponential delay (30s * 2^5 = + // 960s = 16min) past the DEFAULT 15-min cap but under a 60-min custom cap — + // the exact boundary where the two caps disagree. Each iteration advances + // time past its own delay first, so the prior failure's cooldown never + // blocks the next attempt from actually spawning. + for (let i = 0; i < 6; i++) { + const uncappedDelay = 30_000 * 2 ** i; + if (i > 0) vi.advanceTimersByTime(uncappedDelay + 1000); + const failing = makeChild(); + spawn.mockReturnValueOnce(failing); + const attempt = execGh(['api', 'releases/latest'], 5000, { backoffKey: 'k', backoffMaxMs: 60 * 60 * 1000 }); + attempt.catch(() => {}); + failing.emit('close', 1); + await expect(attempt).rejects.toThrow(); + } + expect(spawn).toHaveBeenCalledTimes(6); + + // Past the 15-min DEFAULT cap (which would have already expired and let a + // real 7th attempt through) but short of the 16-min delay the custom + // 60-min cap actually applies. + vi.advanceTimersByTime(15 * 60 * 1000 + 1000); + await expect(execGh(['api', 'releases/latest'], 5000, { backoffKey: 'k', backoffMaxMs: 60 * 60 * 1000 })) + .rejects.toThrow(/backing off/); + expect(spawn).toHaveBeenCalledTimes(6); // still no 7th spawn — the custom cap, not the default, governed this + }); }); // The merge-follow-up reaper turns "the forge says this PR is OPEN" into a diff --git a/server/services/updateChecker.js b/server/services/updateChecker.js index c4732e19df..f598b33030 100644 --- a/server/services/updateChecker.js +++ b/server/services/updateChecker.js @@ -213,11 +213,15 @@ export async function checkForUpdate({ manual = false } = {}) { // consecutive-failure backoff — the 30-min scheduler retries a failure // on its very next tick with no cooldown of its own, which piled up // alongside branch-reconcile's identical gap during a real `gh` blip - // (both logged the same incident). + // (both logged the same incident). `backoffMaxMs` must exceed + // CHECK_INTERVAL_MS (30 min) — execGh's default 15-min cap always expires + // before this scheduler's own next tick, so it would never actually skip + // a scheduled attempt; a comfortable margin above the interval is what + // lets repeated failures widen the gap between real attempts. const raw = await execGh( ['api', `repos/${UPSTREAM_OWNER}/${UPSTREAM_REPO}/releases/latest`], undefined, - manual ? {} : { backoffKey: 'update-check' } + manual ? {} : { backoffKey: 'update-check', backoffMaxMs: CHECK_INTERVAL_MS * 3 } ); let data; try { data = JSON.parse(raw); } catch { throw new Error(`Failed to parse GitHub release response: ${raw.slice(0, 200)}`); } diff --git a/server/services/updateChecker.test.js b/server/services/updateChecker.test.js index 19284ead1d..31bcfc6d6f 100644 --- a/server/services/updateChecker.test.js +++ b/server/services/updateChecker.test.js @@ -304,11 +304,14 @@ describe('checkForUpdate', () => { await checkForUpdate(); // The scheduler retries a failed check on its next 30-min tick with no // cooldown of its own — the actual backoff mechanics live in execGh - // itself (github.js, mocked wholesale here); see github.test.js. + // itself (github.js, mocked wholesale here); see github.test.js. The cap + // must exceed the 30-min scheduler interval (90 min = 3x here) or the + // cooldown always expires before the next tick and never actually + // suppresses a retry — a real gh call fires every tick regardless. expect(execGh).toHaveBeenCalledWith( ['api', 'repos/atomantic/PortOS/releases/latest'], undefined, - { backoffKey: 'update-check' } + { backoffKey: 'update-check', backoffMaxMs: 90 * 60 * 1000 } ); });