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 } ); });