From eb90b29422931b4479a731e9bfc2a75428366c2a Mon Sep 17 00:00:00 2001 From: ozymandiashh <234437643+ozymandiashh@users.noreply.github.com> Date: Mon, 3 Aug 2026 23:08:59 +0300 Subject: [PATCH] fix(menubar): retry transient release-asset download failures `codeburn menubar` aborted the install on the first bad response from GitHub release-asset delivery. A transient HTTP 500 on the checksum fetch (issue #876) killed an otherwise healthy install even though the asset was published correctly and the next request succeeded. Retry the zip and checksum downloads up to 3 times with a short exponential backoff (0.5s, 1s) on 5xx responses and network-level errors. 4xx is never retried: 404/410 still falls through to the release-API discovery path unchanged, and a 403/429 rate limit cannot clear inside the backoff window so it surfaces immediately with its retry-after hint. The checksum comparison stays outside the retry loop so a genuine digest mismatch still aborts on the first look. Thrown errors now name the requested URL so a failure is actionable. Retry parameters and the fetch/sleep/log seams are injectable, matching the options pattern in src/sync/push.ts and src/cache-refresh-lock.ts. --- src/menubar-installer.ts | 113 ++++++++++++++++--- tests/menubar-installer.test.ts | 193 +++++++++++++++++++++++++++++++- 2 files changed, 290 insertions(+), 16 deletions(-) diff --git a/src/menubar-installer.ts b/src/menubar-installer.ts index 7dbed16..d410120 100644 --- a/src/menubar-installer.ts +++ b/src/menubar-installer.ts @@ -37,6 +37,32 @@ type ProxyEnv = Partial[1] type HeaderGetter = { get(name: string): string | null } +/// Only the response surface the asset downloads actually touch, so tests can inject a +/// plain object instead of constructing a full undici Response. +type FetchLikeResponse = { + ok: boolean + status: number + headers: HeaderGetter + body: unknown + text(): Promise +} +type FetchImpl = (url: string, options?: FetchOptions) => Promise + +/// Release-asset delivery (github.com -> Azure blob) occasionally returns a transient 5xx or +/// drops the socket. Three attempts with a short exponential backoff (0.5s, then 1s) rides out +/// that class of blip while adding at most ~1.5s before a genuinely broken download reports +/// back — `codeburn menubar` is interactive, so failing fast still matters. +const ASSET_MAX_ATTEMPTS = 3 +const ASSET_BASE_DELAY_MS = 500 + +export type AssetFetchOptions = { + fetchImpl?: FetchImpl + sleep?: (ms: number) => Promise + log?: (message: string) => void + maxAttempts?: number + baseDelayMs?: number +} + class HttpStatusError extends Error { constructor(message: string, readonly status: number) { super(message) @@ -135,7 +161,7 @@ export function formatGitHubReleaseLookupError(status: number, headers?: HeaderG return `${base}. ${details.join(' ')}` } -function isMissingDirectAssetError(err: unknown): boolean { +export function isMissingDirectAssetError(err: unknown): boolean { return err instanceof HttpStatusError && shouldFallbackToReleaseApi(err.status) } @@ -195,19 +221,75 @@ async function fetchLatestReleaseAssets(): Promise { return resolveLatestMenubarReleaseAssets(body) } -async function verifyChecksum(archivePath: string, checksumUrl: string): Promise { - const response = await fetchWithProxy(checksumUrl, { - headers: { 'User-Agent': 'codeburn-menubar-installer' }, - redirect: 'follow', - }) - if (!response.ok) { - throw new HttpStatusError(`Checksum download failed: HTTP ${response.status}`, response.status) +/// 5xx means "GitHub/the CDN is unhappy right now" and is worth another attempt. 4xx is not: +/// 404/410 must keep falling through to the release-API path untouched, and a 403/429 rate limit +/// cannot clear inside a 1.5s backoff window — hammering it would only spend more of the budget, +/// so those surface immediately with the retry-after hint instead. +function isTransientStatus(status: number): boolean { + return status >= 500 && status <= 599 +} + +function formatAssetHttpError(label: string, url: string, response: FetchLikeResponse): string { + const base = `${label} failed: HTTP ${response.status} (${url})` + if (response.status !== 403 && response.status !== 429) return base + const retryAfter = response.headers.get('retry-after') + const hint = retryAfter + ? `GitHub may be rate limiting this download; retry-after=${retryAfter}.` + : 'GitHub may be rate limiting this download.' + return `${base}. ${hint}` +} + +/// Fetch a release asset, retrying only transient failures. Returns the successful response; +/// the caller consumes the body. Every thrown message carries the requested URL so the user can +/// retry it by hand. +async function fetchReleaseAsset(url: string, label: string, options: AssetFetchOptions): Promise { + const doFetch = options.fetchImpl ?? fetchWithProxy + const sleep = options.sleep ?? ((ms: number) => new Promise(r => setTimeout(r, ms))) + const log = options.log ?? console.log + const maxAttempts = options.maxAttempts ?? ASSET_MAX_ATTEMPTS + const baseDelayMs = options.baseDelayMs ?? ASSET_BASE_DELAY_MS + + for (let attempt = 1; ; attempt++) { + const isLastAttempt = attempt >= maxAttempts + const delayMs = baseDelayMs * 2 ** (attempt - 1) + let response: FetchLikeResponse + try { + response = await doFetch(url, { + headers: { 'User-Agent': 'codeburn-menubar-installer' }, + redirect: 'follow', + }) + } catch (err) { + // Network-level failure (ECONNRESET / ETIMEDOUT / socket hang up): no status to inspect, + // and always transient enough to be worth one more try. + const reason = err instanceof Error ? err.message : String(err) + if (isLastAttempt) throw new Error(`${label} failed after ${maxAttempts} attempts: ${reason} (${url})`) + log(`${label} hit a network error (${reason}), retrying in ${delayMs}ms (attempt ${attempt + 1} of ${maxAttempts})...`) + await sleep(delayMs) + continue + } + + if (response.ok) return response + if (!isTransientStatus(response.status) || isLastAttempt) { + throw new HttpStatusError(formatAssetHttpError(label, url, response), response.status) + } + log(`${label} failed with HTTP ${response.status}, retrying in ${delayMs}ms (attempt ${attempt + 1} of ${maxAttempts})...`) + await sleep(delayMs) } +} + +export async function verifyChecksum( + archivePath: string, + checksumUrl: string, + options: AssetFetchOptions = {}, +): Promise { + const response = await fetchReleaseAsset(checksumUrl, 'Checksum download', options) const text = await response.text() const expected = text.trim().split(/\s+/)[0]!.toLowerCase() const fileBytes = await readFile(archivePath) const actual = createHash('sha256').update(fileBytes).digest('hex') if (actual !== expected) { + // Deliberately outside the retry loop: retries cover transport failures only. A digest + // mismatch is an integrity failure and must abort immediately, never re-download. throw new Error( `Checksum mismatch for ${archivePath}.\n` + ` Expected: ${expected}\n` + @@ -217,13 +299,14 @@ async function verifyChecksum(archivePath: string, checksumUrl: string): Promise } } -async function downloadToFile(url: string, destPath: string): Promise { - const response = await fetchWithProxy(url, { - headers: { 'User-Agent': 'codeburn-menubar-installer' }, - redirect: 'follow', - }) - if (!response.ok || response.body === null) { - throw new HttpStatusError(`Download failed: HTTP ${response.status}`, response.status) +export async function downloadToFile( + url: string, + destPath: string, + options: AssetFetchOptions = {}, +): Promise { + const response = await fetchReleaseAsset(url, 'Download', options) + if (response.body === null) { + throw new HttpStatusError(`Download failed: HTTP ${response.status} with an empty body (${url})`, response.status) } // fetch's ReadableStream needs to be wrapped for Node streams. const nodeStream = Readable.fromWeb(response.body as never) diff --git a/tests/menubar-installer.test.ts b/tests/menubar-installer.test.ts index 129fbac..57ac8fd 100644 --- a/tests/menubar-installer.test.ts +++ b/tests/menubar-installer.test.ts @@ -1,13 +1,20 @@ -import { describe, expect, it } from 'vitest' +import { createHash } from 'node:crypto' +import { mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' import { buildPersistentCodeburnLookupPath, + downloadToFile, formatGitHubReleaseLookupError, + isMissingDirectAssetError, resolveLatestMenubarReleaseAssets, resolveMenubarReleaseAssets, resolvePersistentCodeburnPathFromWhichOutput, resolveProxyUrlForUrl, resolveVersionedMenubarReleaseAssets, shouldFallbackToReleaseApi, + verifyChecksum, type ReleaseResponse, } from '../src/menubar-installer.js' @@ -161,3 +168,187 @@ describe('resolveMenubarReleaseAssets', () => { expect(proxyUrl).toBeUndefined() }) }) + +const ZIP_URL = 'https://github.com/getagentseal/codeburn/releases/download/mac-v0.9.19/CodeBurnMenubar-v0.9.19.zip' +const CHECKSUM_URL = `${ZIP_URL}.sha256` + +/** Minimal stand-in for the fetch response surface the asset downloads touch. */ +function httpResponse(status: number, body?: string, headers: Record = {}) { + return { + ok: status >= 200 && status < 300, + status, + headers: { get: (name: string) => headers[name.toLowerCase()] ?? null }, + body: body === undefined ? null : new Response(body).body, + text: async () => body ?? '', + } +} + +function sha256(text: string): string { + return createHash('sha256').update(Buffer.from(text)).digest('hex') +} + +describe('release asset download retry', () => { + let sandbox: string + let sleeps: number[] + let logs: string[] + let recorder: { sleep: (ms: number) => Promise; log: (message: string) => void } + + beforeEach(async () => { + sandbox = await mkdtemp(join(tmpdir(), 'menubar-installer-')) + sleeps = [] + logs = [] + recorder = { + sleep: async (ms: number) => { sleeps.push(ms) }, + log: (message: string) => { logs.push(message) }, + } + }) + + afterEach(async () => { + await rm(sandbox, { recursive: true, force: true }) + }) + + it('retries a transient 500 zip download and completes on the next attempt', async () => { + const dest = join(sandbox, 'CodeBurnMenubar-v0.9.19.zip') + const statuses = [500, 200] + let calls = 0 + + await downloadToFile(ZIP_URL, dest, { + ...recorder, + fetchImpl: async () => { + const status = statuses[calls++]! + return httpResponse(status, status === 200 ? 'zip-bytes' : 'upstream error') + }, + }) + + expect(calls).toBe(2) + expect(await readFile(dest, 'utf8')).toBe('zip-bytes') + expect(sleeps).toEqual([500]) + expect(logs).toHaveLength(1) + expect(logs[0]).toContain('HTTP 500') + expect(logs[0]).toContain('attempt 2 of 3') + }) + + it('retries a transient 500 checksum download, the failure reported in the issue', async () => { + const archive = join(sandbox, 'CodeBurnMenubar-v0.9.19.zip') + await writeFile(archive, 'zip-bytes') + const statuses = [500, 200] + let calls = 0 + + await verifyChecksum(archive, CHECKSUM_URL, { + ...recorder, + fetchImpl: async () => { + const status = statuses[calls++]! + return httpResponse(status, status === 200 ? `${sha256('zip-bytes')} CodeBurnMenubar-v0.9.19.zip` : 'boom') + }, + }) + + expect(calls).toBe(2) + expect(sleeps).toEqual([500]) + expect(logs[0]).toContain('Checksum download failed with HTTP 500') + }) + + it('gives up on a persistent 500 and names the requested URL in the error', async () => { + let calls = 0 + + await expect(verifyChecksum(join(sandbox, 'unused.zip'), CHECKSUM_URL, { + ...recorder, + fetchImpl: async () => { calls++; return httpResponse(500) }, + })).rejects.toThrow(CHECKSUM_URL) + + expect(calls).toBe(3) + expect(sleeps).toEqual([500, 1000]) + }) + + it('does not retry a 404 and still routes to the missing-asset fallback', async () => { + let calls = 0 + let captured: unknown + + await downloadToFile(ZIP_URL, join(sandbox, 'out.zip'), { + ...recorder, + fetchImpl: async () => { calls++; return httpResponse(404) }, + }).catch((err: unknown) => { captured = err }) + + expect(calls).toBe(1) + expect(sleeps).toEqual([]) + expect(isMissingDirectAssetError(captured)).toBe(true) + expect(captured).toBeInstanceOf(Error) + expect((captured as Error).message).toContain(ZIP_URL) + }) + + it('does not retry a 429 and surfaces the retry-after hint instead', async () => { + let calls = 0 + + await expect(downloadToFile(ZIP_URL, join(sandbox, 'out.zip'), { + ...recorder, + fetchImpl: async () => { calls++; return httpResponse(429, undefined, { 'retry-after': '120' }) }, + })).rejects.toThrow(/retry-after=120/) + + expect(calls).toBe(1) + expect(sleeps).toEqual([]) + }) + + it('retries a network-level failure and reports it with the URL when it persists', async () => { + let calls = 0 + + await expect(downloadToFile(ZIP_URL, join(sandbox, 'out.zip'), { + ...recorder, + fetchImpl: async () => { + calls++ + throw Object.assign(new Error('socket hang up'), { code: 'ECONNRESET' }) + }, + })).rejects.toThrow(/socket hang up/) + + expect(calls).toBe(3) + expect(sleeps).toEqual([500, 1000]) + expect(logs).toHaveLength(2) + }) + + it('recovers when a network-level failure clears on the next attempt', async () => { + const dest = join(sandbox, 'CodeBurnMenubar-v0.9.19.zip') + let calls = 0 + + await downloadToFile(ZIP_URL, dest, { + ...recorder, + fetchImpl: async () => { + calls++ + if (calls === 1) throw Object.assign(new Error('ETIMEDOUT'), { code: 'ETIMEDOUT' }) + return httpResponse(200, 'zip-bytes') + }, + }) + + expect(calls).toBe(2) + expect(await readFile(dest, 'utf8')).toBe('zip-bytes') + }) + + it('fails a genuine checksum mismatch immediately instead of re-downloading', async () => { + const archive = join(sandbox, 'CodeBurnMenubar-v0.9.19.zip') + await writeFile(archive, 'tampered-bytes') + let calls = 0 + + await expect(verifyChecksum(archive, CHECKSUM_URL, { + ...recorder, + fetchImpl: async () => { + calls++ + return httpResponse(200, `${sha256('zip-bytes')} CodeBurnMenubar-v0.9.19.zip`) + }, + })).rejects.toThrow(/Checksum mismatch/) + + // The retry budget covers transport only. A digest mismatch must abort on the first look. + expect(calls).toBe(1) + expect(sleeps).toEqual([]) + }) + + it('honors an overridden attempt budget', async () => { + let calls = 0 + + await expect(downloadToFile(ZIP_URL, join(sandbox, 'out.zip'), { + ...recorder, + maxAttempts: 2, + baseDelayMs: 10, + fetchImpl: async () => { calls++; return httpResponse(503) }, + })).rejects.toThrow(/HTTP 503/) + + expect(calls).toBe(2) + expect(sleeps).toEqual([10]) + }) +})