From 9737be46d42d330040aa37e6a175780f132fd8e5 Mon Sep 17 00:00:00 2001 From: priyagupta108 Date: Thu, 9 Jul 2026 09:20:30 +0530 Subject: [PATCH 1/2] validate and retry manifest fetch --- __tests__/install-python.test.ts | 66 ++++++++++++++++++++++ dist/setup/index.js | 66 +++++++++++++++++++--- src/install-python.ts | 94 +++++++++++++++++++++++++++----- 3 files changed, 204 insertions(+), 22 deletions(-) diff --git a/__tests__/install-python.test.ts b/__tests__/install-python.test.ts index 51f9fa77f..e1dbd060c 100644 --- a/__tests__/install-python.test.ts +++ b/__tests__/install-python.test.ts @@ -31,6 +31,10 @@ describe('getManifest', () => { jest.resetAllMocks(); }); + afterEach(() => { + jest.useRealTimers(); + }); + it('should return manifest from repo', async () => { (tc.getManifestFromRepo as jest.Mock).mockResolvedValue(mockManifest); const manifest = await getManifest(); @@ -38,14 +42,76 @@ describe('getManifest', () => { }); it('should return manifest from URL if repo fetch fails', async () => { + jest.useFakeTimers(); (tc.getManifestFromRepo as jest.Mock).mockRejectedValue( new Error('Fetch failed') ); (httpm.HttpClient.prototype.getJson as jest.Mock).mockResolvedValue({ result: mockManifest }); + const promise = getManifest(); + await jest.runAllTimersAsync(); + const manifest = await promise; + expect(manifest).toEqual(mockManifest); + }); + + it('should fall back to URL if repo returns a truncated/empty manifest', async () => { + jest.useFakeTimers(); + // Simulate a truncated response that parses to an empty array. + (tc.getManifestFromRepo as jest.Mock).mockResolvedValue([]); + (httpm.HttpClient.prototype.getJson as jest.Mock).mockResolvedValue({ + result: mockManifest + }); + const promise = getManifest(); + await jest.runAllTimersAsync(); + const manifest = await promise; + expect(manifest).toEqual(mockManifest); + }); + + it('should retry on a transient invalid manifest and then succeed', async () => { + jest.useFakeTimers(); + (tc.getManifestFromRepo as jest.Mock) + // First attempt returns a truncated/empty body. + .mockResolvedValueOnce([]) + // Retry returns a valid manifest. + .mockResolvedValueOnce(mockManifest); + const promise = getManifest(); + await jest.runAllTimersAsync(); + const manifest = await promise; + expect(manifest).toEqual(mockManifest); + expect(tc.getManifestFromRepo as jest.Mock).toHaveBeenCalledTimes(2); + }); + + it('should fail loudly when the manifest is truncated/empty on every source', async () => { + jest.useFakeTimers(); + // Both the API and the raw URL keep returning truncated/empty bodies. + (tc.getManifestFromRepo as jest.Mock).mockResolvedValue([]); + (httpm.HttpClient.prototype.getJson as jest.Mock).mockResolvedValue({ + result: [] + }); + const promise = getManifest(); + // Attach a rejection handler before advancing timers to avoid unhandled rejection warnings. + const catchPromise = promise.catch(() => {}); + await jest.runAllTimersAsync(); + await catchPromise; + await expect(promise).rejects.toThrow( + 'Failed to fetch the Python versions manifest' + ); + }); + + it('should not retry the API on a rate-limit error and fall back to URL immediately', async () => { + const rateLimitError = Object.assign(new Error('API rate limit exceeded'), { + httpStatusCode: 403 + }); + (tc.getManifestFromRepo as jest.Mock).mockRejectedValue(rateLimitError); + (httpm.HttpClient.prototype.getJson as jest.Mock).mockResolvedValue({ + result: mockManifest + }); + // No fake timers needed: a rate-limit error must short-circuit retries + // (and their backoff) and fall straight through to the URL fallback. const manifest = await getManifest(); expect(manifest).toEqual(mockManifest); + expect(tc.getManifestFromRepo as jest.Mock).toHaveBeenCalledTimes(1); }); }); diff --git a/dist/setup/index.js b/dist/setup/index.js index 6ac18cd5e..31cd4ff71 100644 --- a/dist/setup/index.js +++ b/dist/setup/index.js @@ -55685,6 +55685,8 @@ function findRhelRelease(semanticVersionSpec, architecture, manifest, osVersion) } return undefined; } +const MANIFEST_FETCH_MAX_ATTEMPTS = 3; +const MANIFEST_FETCH_RETRY_BASE_DELAY_MS = 1000; async function findReleaseFromManifest(semanticVersionSpec, architecture, manifest) { if (!manifest) { manifest = await getManifest(); @@ -55712,15 +55714,54 @@ function isIToolRelease(obj) { typeof file.arch === 'string' && typeof file.download_url === 'string')); } +// Rejects empty or truncated manifest responses. +function isValidManifest(manifest) { + return (Array.isArray(manifest) && + manifest.length > 0 && + manifest.every(isIToolRelease)); +} +function sleep(ms) { + return new Promise(resolve => setTimeout(resolve, ms)); +} +// HTTP 403/429 from http-client (`statusCode`) or tool-cache (`httpStatusCode`). +function isRateLimitError(err) { + const status = err.httpStatusCode ?? + err.statusCode; + return status === 403 || status === 429; +} +// Fetches and validates a manifest, retrying transient failures with backoff. +async function fetchValidManifest(source, fetcher) { + let lastError; + let attempts = 0; + for (let attempt = 1; attempt <= MANIFEST_FETCH_MAX_ATTEMPTS; attempt++) { + attempts = attempt; + try { + const manifest = await fetcher(); + if (isValidManifest(manifest)) { + return manifest; + } + throw new Error(`The manifest fetched from ${source} is empty, truncated, or does not contain any valid tool release entries.`); + } + catch (err) { + lastError = err instanceof Error ? err : new Error(String(err)); + core.debug(`Attempt ${attempt}/${MANIFEST_FETCH_MAX_ATTEMPTS} to fetch the manifest from ${source} failed: ${lastError.message}`); + // Rate limits won't clear within the backoff window; fall back instead. + if (isRateLimitError(err)) { + core.debug(`${source} is rate-limited; skipping retries for this source.`); + break; + } + if (attempt < MANIFEST_FETCH_MAX_ATTEMPTS) { + const delay = MANIFEST_FETCH_RETRY_BASE_DELAY_MS * 2 ** (attempt - 1); + core.debug(`Retrying in ${delay}ms...`); + await sleep(delay); + } + } + } + throw new Error(`Failed to fetch a valid manifest from ${source} after ${attempts} attempt(s): ${lastError?.message}`); +} async function getManifest() { try { - const repoManifest = await getManifestFromRepo(); - if (Array.isArray(repoManifest) && - repoManifest.length && - repoManifest.every(isIToolRelease)) { - return repoManifest; - } - throw new Error('The repository manifest is invalid or does not include any valid tool release (IToolRelease) entries.'); + return await fetchValidManifest('the GitHub API', getManifestFromRepo); } catch (err) { core.debug('Fetching the manifest via the API failed.'); @@ -55728,10 +55769,17 @@ async function getManifest() { core.debug(err.message); } else { - core.error('An unexpected error occurred while fetching the manifest.'); + core.debug('An unexpected error occurred while fetching the manifest.'); } } - return await getManifestFromURL(); + try { + return await fetchValidManifest('the raw URL', getManifestFromURL); + } + catch (err) { + const message = err instanceof Error ? err.message : String(err); + // Fail loudly so the action doesn't exit 0 without installing Python. + throw new Error(`Failed to fetch the Python versions manifest. The response was empty, truncated, or invalid, and all retries were exhausted. ${message}`); + } } function getManifestFromRepo() { core.debug(`Getting manifest from ${MANIFEST_REPO_OWNER}/${MANIFEST_REPO_NAME}@${MANIFEST_REPO_BRANCH}`); diff --git a/src/install-python.ts b/src/install-python.ts index 9e17c3346..d2dd79b5b 100644 --- a/src/install-python.ts +++ b/src/install-python.ts @@ -80,6 +80,9 @@ function findRhelRelease( return undefined; } +const MANIFEST_FETCH_MAX_ATTEMPTS = 3; +const MANIFEST_FETCH_RETRY_BASE_DELAY_MS = 1000; + export async function findReleaseFromManifest( semanticVersionSpec: string, architecture: string, @@ -133,28 +136,93 @@ function isIToolRelease(obj: any): obj is IToolRelease { ); } +// Rejects empty or truncated manifest responses. +function isValidManifest(manifest: unknown): manifest is tc.IToolRelease[] { + return ( + Array.isArray(manifest) && + manifest.length > 0 && + manifest.every(isIToolRelease) + ); +} + +function sleep(ms: number): Promise { + return new Promise(resolve => setTimeout(resolve, ms)); +} + +// HTTP 403/429 from http-client (`statusCode`) or tool-cache (`httpStatusCode`). +function isRateLimitError(err: unknown): boolean { + const status = + (err as {httpStatusCode?: number}).httpStatusCode ?? + (err as {statusCode?: number}).statusCode; + return status === 403 || status === 429; +} + +// Fetches and validates a manifest, retrying transient failures with backoff. +async function fetchValidManifest( + source: string, + fetcher: () => Promise +): Promise { + let lastError: Error | undefined; + let attempts = 0; + + for (let attempt = 1; attempt <= MANIFEST_FETCH_MAX_ATTEMPTS; attempt++) { + attempts = attempt; + try { + const manifest = await fetcher(); + if (isValidManifest(manifest)) { + return manifest; + } + throw new Error( + `The manifest fetched from ${source} is empty, truncated, or does not contain any valid tool release entries.` + ); + } catch (err) { + lastError = err instanceof Error ? err : new Error(String(err)); + core.debug( + `Attempt ${attempt}/${MANIFEST_FETCH_MAX_ATTEMPTS} to fetch the manifest from ${source} failed: ${lastError.message}` + ); + + // Rate limits won't clear within the backoff window; fall back instead. + if (isRateLimitError(err)) { + core.debug( + `${source} is rate-limited; skipping retries for this source.` + ); + break; + } + + if (attempt < MANIFEST_FETCH_MAX_ATTEMPTS) { + const delay = MANIFEST_FETCH_RETRY_BASE_DELAY_MS * 2 ** (attempt - 1); + core.debug(`Retrying in ${delay}ms...`); + await sleep(delay); + } + } + } + + throw new Error( + `Failed to fetch a valid manifest from ${source} after ${attempts} attempt(s): ${lastError?.message}` + ); +} + export async function getManifest(): Promise { try { - const repoManifest = await getManifestFromRepo(); - if ( - Array.isArray(repoManifest) && - repoManifest.length && - repoManifest.every(isIToolRelease) - ) { - return repoManifest; - } - throw new Error( - 'The repository manifest is invalid or does not include any valid tool release (IToolRelease) entries.' - ); + return await fetchValidManifest('the GitHub API', getManifestFromRepo); } catch (err) { core.debug('Fetching the manifest via the API failed.'); if (err instanceof Error) { core.debug(err.message); } else { - core.error('An unexpected error occurred while fetching the manifest.'); + core.debug('An unexpected error occurred while fetching the manifest.'); } } - return await getManifestFromURL(); + + try { + return await fetchValidManifest('the raw URL', getManifestFromURL); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + // Fail loudly so the action doesn't exit 0 without installing Python. + throw new Error( + `Failed to fetch the Python versions manifest. The response was empty, truncated, or invalid, and all retries were exhausted. ${message}` + ); + } } export function getManifestFromRepo(): Promise { From 0bc3767d344f5c4affa63b7e39411c5e2291a9c7 Mon Sep 17 00:00:00 2001 From: priyagupta108 Date: Thu, 9 Jul 2026 09:40:20 +0530 Subject: [PATCH 2/2] Refactor error handling in isRateLimitError function for improved clarity --- __tests__/install-python.test.ts | 8 +------- dist/setup/index.js | 4 ++-- src/install-python.ts | 8 +++++--- 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/__tests__/install-python.test.ts b/__tests__/install-python.test.ts index e1dbd060c..860f168cc 100644 --- a/__tests__/install-python.test.ts +++ b/__tests__/install-python.test.ts @@ -57,7 +57,6 @@ describe('getManifest', () => { it('should fall back to URL if repo returns a truncated/empty manifest', async () => { jest.useFakeTimers(); - // Simulate a truncated response that parses to an empty array. (tc.getManifestFromRepo as jest.Mock).mockResolvedValue([]); (httpm.HttpClient.prototype.getJson as jest.Mock).mockResolvedValue({ result: mockManifest @@ -71,9 +70,7 @@ describe('getManifest', () => { it('should retry on a transient invalid manifest and then succeed', async () => { jest.useFakeTimers(); (tc.getManifestFromRepo as jest.Mock) - // First attempt returns a truncated/empty body. .mockResolvedValueOnce([]) - // Retry returns a valid manifest. .mockResolvedValueOnce(mockManifest); const promise = getManifest(); await jest.runAllTimersAsync(); @@ -84,13 +81,12 @@ describe('getManifest', () => { it('should fail loudly when the manifest is truncated/empty on every source', async () => { jest.useFakeTimers(); - // Both the API and the raw URL keep returning truncated/empty bodies. (tc.getManifestFromRepo as jest.Mock).mockResolvedValue([]); (httpm.HttpClient.prototype.getJson as jest.Mock).mockResolvedValue({ result: [] }); const promise = getManifest(); - // Attach a rejection handler before advancing timers to avoid unhandled rejection warnings. + // Prevent unhandled rejection before timers advance. const catchPromise = promise.catch(() => {}); await jest.runAllTimersAsync(); await catchPromise; @@ -107,8 +103,6 @@ describe('getManifest', () => { (httpm.HttpClient.prototype.getJson as jest.Mock).mockResolvedValue({ result: mockManifest }); - // No fake timers needed: a rate-limit error must short-circuit retries - // (and their backoff) and fall straight through to the URL fallback. const manifest = await getManifest(); expect(manifest).toEqual(mockManifest); expect(tc.getManifestFromRepo as jest.Mock).toHaveBeenCalledTimes(1); diff --git a/dist/setup/index.js b/dist/setup/index.js index 31cd4ff71..e0982e713 100644 --- a/dist/setup/index.js +++ b/dist/setup/index.js @@ -55725,8 +55725,8 @@ function sleep(ms) { } // HTTP 403/429 from http-client (`statusCode`) or tool-cache (`httpStatusCode`). function isRateLimitError(err) { - const status = err.httpStatusCode ?? - err.statusCode; + const e = err; + const status = e?.httpStatusCode ?? e?.statusCode; return status === 403 || status === 429; } // Fetches and validates a manifest, retrying transient failures with backoff. diff --git a/src/install-python.ts b/src/install-python.ts index d2dd79b5b..35b3170e7 100644 --- a/src/install-python.ts +++ b/src/install-python.ts @@ -151,9 +151,11 @@ function sleep(ms: number): Promise { // HTTP 403/429 from http-client (`statusCode`) or tool-cache (`httpStatusCode`). function isRateLimitError(err: unknown): boolean { - const status = - (err as {httpStatusCode?: number}).httpStatusCode ?? - (err as {statusCode?: number}).statusCode; + const e = err as + | {httpStatusCode?: number; statusCode?: number} + | null + | undefined; + const status = e?.httpStatusCode ?? e?.statusCode; return status === 403 || status === 429; }