diff --git a/packages/cli-kit/src/public/node/http.test.ts b/packages/cli-kit/src/public/node/http.test.ts index f848c4e76a..2440233987 100644 --- a/packages/cli-kit/src/public/node/http.test.ts +++ b/packages/cli-kit/src/public/node/http.test.ts @@ -292,6 +292,24 @@ describe('downloadFile', () => { await expect(fileExists(to)).resolves.toBe(false) }) }) + + test('Fails and cleans up if server returns 500', async () => { + await inTemporaryDirectory(async (tmpDir) => { + // Given + const url = 'https://shopify.example/500.txt' + const filename = '/bin/500.txt' + const to = joinPath(tmpDir, filename) + + // When + const result = downloadFile(url, to) + + // Then + await expect(result).rejects.toThrow( + 'Failed to download file from https://shopify.example/500.txt: 500 Internal Server Error', + ) + await expect(fileExists(to)).resolves.toBe(false) + }) + }) }) describe('requestMode', () => { diff --git a/packages/cli-kit/src/public/node/http.ts b/packages/cli-kit/src/public/node/http.ts index 8cbc8556a5..8121f31a2f 100644 --- a/packages/cli-kit/src/public/node/http.ts +++ b/packages/cli-kit/src/public/node/http.ts @@ -226,38 +226,52 @@ export function downloadFile(url: string, to: string): Promise { return runWithTimer('cmd_all_timing_network_ms')(() => { return new Promise((resolve, reject) => { - if (!fileExistsSync(dirname(to))) { - mkdirSync(dirname(to)) - } + nodeFetch(url, {redirect: 'follow'}) + .then((res) => { + if (!res.ok) { + return reject(new Error(`Failed to download file from ${sanitizedUrl}: ${res.status} ${res.statusText}`)) + } + if (!res.body) { + return reject(new Error(`Failed to download file from ${sanitizedUrl}: Empty response body`)) + } - const file = createFileWriteStream(to) + if (!fileExistsSync(dirname(to))) { + mkdirSync(dirname(to)) + } - // if we can't remove the file for some reason (seen on windows), that's ok -- it's in a temporary directory - const tryToRemoveFile = () => { - try { - unlinkFileSync(to) - // eslint-disable-next-line no-catch-all/no-catch-all - } catch (err: unknown) { - outputDebug(outputContent`Failed to remove file ${outputToken.path(to)}: ${outputToken.raw(String(err))}`) - } - } + const file = createFileWriteStream(to) - file.on('finish', () => { - file.close() - resolve(to) - }) + // if we can't remove the file for some reason (seen on windows), that's ok -- it's in a temporary directory + const tryToRemoveFile = () => { + try { + if (!file.destroyed) { + file.destroy() + } + unlinkFileSync(to) + // eslint-disable-next-line no-catch-all/no-catch-all + } catch (err: unknown) { + outputDebug(outputContent`Failed to remove file ${outputToken.path(to)}: ${outputToken.raw(String(err))}`) + } + } - file.on('error', (err) => { - tryToRemoveFile() - reject(err) - }) + file.on('finish', () => { + file.close() + resolve(to) + }) - nodeFetch(url, {redirect: 'follow'}) - .then((res) => { - res.body?.pipe(file) + file.on('error', (err) => { + tryToRemoveFile() + reject(err instanceof Error ? err : new Error(String(err))) + }) + + res.body.on('error', (err) => { + tryToRemoveFile() + reject(err instanceof Error ? err : new Error(String(err))) + }) + + res.body.pipe(file) }) .catch((err) => { - tryToRemoveFile() reject(err instanceof Error ? err : new Error(String(err))) }) })