diff --git a/jest.config.js b/jest.config.js index 101666d6..f25dbdc9 100644 --- a/jest.config.js +++ b/jest.config.js @@ -8,4 +8,4 @@ module.exports = { '^.+\\.ts$': 'ts-jest' }, verbose: true -} \ No newline at end of file +} diff --git a/package-lock.json b/package-lock.json index e72dedbd..1d43fbf3 100644 --- a/package-lock.json +++ b/package-lock.json @@ -5781,9 +5781,9 @@ } }, "node_modules/follow-redirects": { - "version": "1.15.2", - "resolved": "https://registry.npmjs.org/follow-redirects/-/follow-redirects-1.15.2.tgz", - "integrity": "sha512-VQLG33o04KaQ8uYi2tVNbdrWp1QWxNNea+nmIB4EVM28v0hmP17z7aG1+wAkNzVq4KeXTq3221ye5qTJP91JwA==", + "version": "1.15.4", + "resolved": "https://registry.npmjs.org/follow-redirects/-/follow-redirects-1.15.4.tgz", + "integrity": "sha512-Cr4D/5wlrb0z9dgERpUL3LrmPKVDsETIJhaCMeDfuFYcqa5bldGV6wBsAN6X/vxlXQtFBMrXdXxdL8CbDTGniw==", "dev": true, "funding": [ { diff --git a/package.json b/package.json index 73a7093c..ea3ac2ed 100644 --- a/package.json +++ b/package.json @@ -13,7 +13,7 @@ "lint": "eslint packages/**/*.ts", "lint-fix": "eslint packages/**/*.ts --fix", "new-package": "scripts/create-package", - "test": "jest --testTimeout 10000" + "test": "jest --testTimeout 60000" }, "devDependencies": { "@types/jest": "^29.5.4", diff --git a/packages/artifact/__tests__/download-artifact.test.ts b/packages/artifact/__tests__/download-artifact.test.ts index 815a760c..1c0c9b0d 100644 --- a/packages/artifact/__tests__/download-artifact.test.ts +++ b/packages/artifact/__tests__/download-artifact.test.ts @@ -9,7 +9,8 @@ import archiver from 'archiver' import { downloadArtifactInternal, - downloadArtifactPublic + downloadArtifactPublic, + streamExtractExternal } from '../src/internal/download/download-artifact' import {getUserAgentString} from '../src/internal/shared/user-agent' import {noopLogs} from './common' @@ -248,7 +249,44 @@ describe('download-artifact', () => { }) }) - it('should fail if blob storage response is non-200', async () => { + it('should fail if blob storage storage chunk does not respond within 30s', async () => { + // mock http client to delay response data by 30s + const msg = new http.IncomingMessage(new net.Socket()) + msg.statusCode = 200 + + const mockGet = jest.fn(async () => { + return new Promise((resolve, reject) => { + // Resolve with a 200 status code immediately + resolve({ + message: msg, + readBody: async () => { + return Promise.resolve(`{"ok": true}`) + } + }) + + // Reject with an error after 31 seconds + setTimeout(() => { + reject(new Error('Request timeout')) + }, 31000) // Timeout after 31 seconds + }) + }) + + const mockHttpClient = (HttpClient as jest.Mock).mockImplementation( + () => { + return { + get: mockGet + } + } + ) + + await expect( + streamExtractExternal(fixtures.blobStorageUrl, fixtures.workspaceDir) + ).rejects.toBeInstanceOf(Error) + + expect(mockHttpClient).toHaveBeenCalledWith(getUserAgentString()) + }, 35000) // add longer timeout to allow for timer to run out + + it('should fail if blob storage response is non-200 after 5 retries', async () => { const downloadArtifactMock = github.getOctokit(fixtures.token).rest .actions.downloadArtifact as MockedDownloadArtifact downloadArtifactMock.mockResolvedValueOnce({ @@ -290,7 +328,60 @@ describe('download-artifact', () => { expect(mockGetArtifactFailure).toHaveBeenCalledWith( fixtures.blobStorageUrl ) - }) + expect(mockGetArtifactFailure).toHaveBeenCalledTimes(5) + }, 38000) + + it('should retry if blob storage response is non-200 and then succeed with a 200', async () => { + const downloadArtifactMock = github.getOctokit(fixtures.token).rest + .actions.downloadArtifact as MockedDownloadArtifact + downloadArtifactMock.mockResolvedValueOnce({ + headers: { + location: fixtures.blobStorageUrl + }, + status: 302, + url: '', + data: Buffer.from('') + }) + + const mockGetArtifact = jest + .fn(mockGetArtifactSuccess) + .mockImplementationOnce(mockGetArtifactFailure) + + const mockHttpClient = (HttpClient as jest.Mock).mockImplementation( + () => { + return { + get: mockGetArtifact + } + } + ) + + const response = await downloadArtifactPublic( + fixtures.artifactID, + fixtures.repositoryOwner, + fixtures.repositoryName, + fixtures.token + ) + + expect(downloadArtifactMock).toHaveBeenCalledWith({ + owner: fixtures.repositoryOwner, + repo: fixtures.repositoryName, + artifact_id: fixtures.artifactID, + archive_format: 'zip', + request: { + redirect: 'manual' + } + }) + expect(mockHttpClient).toHaveBeenCalledWith(getUserAgentString()) + expect(mockGetArtifactFailure).toHaveBeenCalledWith( + fixtures.blobStorageUrl + ) + expect(mockGetArtifactFailure).toHaveBeenCalledTimes(1) + expect(mockGetArtifactSuccess).toHaveBeenCalledWith( + fixtures.blobStorageUrl + ) + expect(mockGetArtifactSuccess).toHaveBeenCalledTimes(1) + expect(response.downloadPath).toBe(fixtures.workspaceDir) + }, 28000) }) describe('internal', () => { diff --git a/packages/artifact/src/internal/download/download-artifact.ts b/packages/artifact/src/internal/download/download-artifact.ts index a61e2937..dc54f6fe 100644 --- a/packages/artifact/src/internal/download/download-artifact.ts +++ b/packages/artifact/src/internal/download/download-artifact.ts @@ -38,20 +38,65 @@ async function exists(path: string): Promise { } async function streamExtract(url: string, directory: string): Promise { + let retryCount = 0 + while (retryCount < 5) { + try { + await streamExtractExternal(url, directory) + return + } catch (error) { + retryCount++ + core.debug( + `Failed to download artifact after ${retryCount} retries due to ${error.message}. Retrying in 5 seconds...` + ) + // wait 5 seconds before retrying + await new Promise(resolve => setTimeout(resolve, 5000)) + } + } + + throw new Error(`Artifact download failed after ${retryCount} retries.`) +} + +export async function streamExtractExternal( + url: string, + directory: string +): Promise { const client = new httpClient.HttpClient(getUserAgentString()) const response = await client.get(url) - if (response.message.statusCode !== 200) { throw new Error( `Unexpected HTTP response from blob storage: ${response.message.statusCode} ${response.message.statusMessage}` ) } + const timeout = 30 * 1000 // 30 seconds + return new Promise((resolve, reject) => { + const timerFn = (): void => { + response.message.destroy( + new Error(`Blob storage chunk did not respond in ${timeout}ms`) + ) + } + const timer = setTimeout(timerFn, timeout) + response.message + .on('data', () => { + timer.refresh() + }) + .on('error', (error: Error) => { + core.debug( + `response.message: Artifact download failed: ${error.message}` + ) + clearTimeout(timer) + reject(error) + }) .pipe(unzip.Extract({path: directory})) - .on('close', resolve) - .on('error', reject) + .on('close', () => { + clearTimeout(timer) + resolve() + }) + .on('error', (error: Error) => { + reject(error) + }) }) }