Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions server/services/github.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
}

Expand All @@ -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`));
}
Expand All @@ -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;
Expand Down
34 changes: 34 additions & 0 deletions server/services/github.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 6 additions & 2 deletions server/services/updateChecker.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)}`); }
Expand Down
7 changes: 5 additions & 2 deletions server/services/updateChecker.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
);
});

Expand Down