diff --git a/.changeset/fix-dev-bundle-upload-status-check.md b/.changeset/fix-dev-bundle-upload-status-check.md new file mode 100644 index 00000000000..4c757b1fda0 --- /dev/null +++ b/.changeset/fix-dev-bundle-upload-status-check.md @@ -0,0 +1,5 @@ +--- +'@shopify/app': patch +--- + +Fix dev/deploy bundle uploads feigning success when the GCS upload fails diff --git a/packages/app/src/cli/services/bundle.test.ts b/packages/app/src/cli/services/bundle.test.ts index 35fc7679335..b6ac856d765 100644 --- a/packages/app/src/cli/services/bundle.test.ts +++ b/packages/app/src/cli/services/bundle.test.ts @@ -6,6 +6,7 @@ import {inTemporaryDirectory, mkdir, writeFile, readFile, fileSize} from '@shopi import {brotliCompress, zip} from '@shopify/cli-kit/node/archiver' import {AbortError} from '@shopify/cli-kit/node/error' import {fetch} from '@shopify/cli-kit/node/http' +import {sleep} from '@shopify/cli-kit/node/system' vi.mock('@shopify/cli-kit/node/archiver', () => { return { @@ -24,6 +25,11 @@ vi.mock('@shopify/cli-kit/node/fs', async (importActual) => { return {...actual, fileSize: vi.fn(actual.fileSize)} }) +vi.mock('@shopify/cli-kit/node/system', async (importActual) => { + const actual: any = await importActual() + return {...actual, sleep: vi.fn()} +}) + describe('writeManifestToBundle', () => { test('writes manifest.json to the specified directory', async () => { await inTemporaryDirectory(async (tmpDir) => { @@ -211,7 +217,7 @@ describe('uploadToGCS', () => { // Given const bundlePath = joinPath(tmpDir, 'bundle.zip') await writeFile(bundlePath, 'small contents') - vi.mocked(fetch).mockResolvedValue({} as never) + vi.mocked(fetch).mockResolvedValue({ok: true, status: 200} as never) // When await uploadToGCS('https://signed.example/upload', bundlePath) @@ -225,6 +231,60 @@ describe('uploadToGCS', () => { }) }) + test('throws when the upload returns a non-2xx status so a failed upload is not treated as a success', async () => { + await inTemporaryDirectory(async (tmpDir) => { + // Given + const bundlePath = joinPath(tmpDir, 'bundle.zip') + await writeFile(bundlePath, 'small contents') + vi.mocked(fetch).mockResolvedValue({ + ok: false, + status: 403, + text: () => Promise.resolve('SignatureExpired'), + } as never) + + // When / Then + await expect(uploadToGCS('https://signed.example/upload', bundlePath)).rejects.toThrow(AbortError) + await expect(uploadToGCS('https://signed.example/upload', bundlePath)).rejects.toThrow(/HTTP 403/) + // A 403 is not retryable, so it is attempted exactly once per call. + expect(fetch).toHaveBeenCalledTimes(2) + }) + }) + + test('retries transient upload failures and succeeds', async () => { + await inTemporaryDirectory(async (tmpDir) => { + // Given + const bundlePath = joinPath(tmpDir, 'bundle.zip') + await writeFile(bundlePath, 'small contents') + vi.mocked(fetch) + .mockResolvedValueOnce({ok: false, status: 503, text: () => Promise.resolve('unavailable')} as never) + .mockResolvedValueOnce({ok: true, status: 200} as never) + + // When + await uploadToGCS('https://signed.example/upload', bundlePath) + + // Then + expect(fetch).toHaveBeenCalledTimes(2) + expect(sleep).toHaveBeenCalledTimes(1) + }) + }) + + test('gives up after exhausting retries on persistent transient failures', async () => { + await inTemporaryDirectory(async (tmpDir) => { + // Given + const bundlePath = joinPath(tmpDir, 'bundle.zip') + await writeFile(bundlePath, 'small contents') + vi.mocked(fetch).mockResolvedValue({ + ok: false, + status: 503, + text: () => Promise.resolve('unavailable'), + } as never) + + // When / Then + await expect(uploadToGCS('https://signed.example/upload', bundlePath)).rejects.toThrow(/HTTP 503/) + expect(fetch).toHaveBeenCalledTimes(3) + }) + }) + test('aborts with a helpful error when the bundle exceeds the 100 MB upload limit', async () => { await inTemporaryDirectory(async (tmpDir) => { // Given — mock the reported size so CI doesn't have to allocate 101 MB on disk. diff --git a/packages/app/src/cli/services/bundle.ts b/packages/app/src/cli/services/bundle.ts index bd853c31753..ec0f0b9d20b 100644 --- a/packages/app/src/cli/services/bundle.ts +++ b/packages/app/src/cli/services/bundle.ts @@ -4,15 +4,21 @@ import {AssetUrlSchema, DeveloperPlatformClient} from '../utilities/developer-pl import {MinimalAppIdentifiers} from '../models/organization.js' import {joinPath} from '@shopify/cli-kit/node/path' import {brotliCompress, zip} from '@shopify/cli-kit/node/archiver' -import {formData, fetch} from '@shopify/cli-kit/node/http' +import {fetch, Response} from '@shopify/cli-kit/node/http' import {fileSize, readFileSync} from '@shopify/cli-kit/node/fs' import {AbortError} from '@shopify/cli-kit/node/error' +import {sleep} from '@shopify/cli-kit/node/system' import {writeFile} from 'fs/promises' const MEGABYTE = 1024 * 1024 const MAX_BUNDLE_SIZE_MB = 100 const MAX_BUNDLE_SIZE_BYTES = MAX_BUNDLE_SIZE_MB * MEGABYTE +// A signed PUT upload is idempotent (same object key, same bytes), so it is safe +// to retry. We only retry statuses that are plausibly transient. +const UPLOAD_MAX_ATTEMPTS = 3 +const RETRYABLE_UPLOAD_STATUS_CODES = new Set([408, 429, 500, 502, 503, 504]) + export async function writeManifestToBundle(appManifest: AppManifest, bundlePath: string) { const manifestPath = joinPath(bundlePath, 'manifest.json') await writeFile(manifestPath, JSON.stringify(appManifest, null, 2)) @@ -30,7 +36,16 @@ export async function compressBundle(inputDirectory: string, outputPath: string, } /** - * Upload a file to GCS + * Upload a file to GCS using a signed URL. + * + * GCS replies to the signed PUT with a non-2xx status code when the upload fails + * (for example an expired signature, a malformed request, or a transient server + * error). The status code MUST be checked: `fetch` does not throw on a non-2xx + * HTTP response, so if it is ignored a failed upload is silently treated as a + * success and the caller proceeds to reference an object that was never stored. + * That surfaces downstream as a confusing "Uploaded file not found" error when + * the bundle is consumed (e.g. during devSessionCreate). + * * @param signedURL - The signed URL to upload the file to * @param filePath - The path to the file */ @@ -44,10 +59,38 @@ export async function uploadToGCS(signedURL: string, filePath: string) { `Check the asset paths in your extension configuration — a misconfigured source can pull in much more than intended. Exclude large files or directories from your bundle, then try again.`, ) } - const form = formData() + const buffer = readFileSync(filePath) - form.append('my_upload', buffer) - await fetch(signedURL, {method: 'put', body: buffer, headers: form.getHeaders()}, 'slow-request') + + let response: Response | undefined + for (let attempt = 1; attempt <= UPLOAD_MAX_ATTEMPTS; attempt++) { + // The signed URL only signs the `host` header, so no extra headers are + // required; node-fetch derives Content-Length from the buffer body. + // eslint-disable-next-line no-await-in-loop + response = await fetch(signedURL, {method: 'put', body: buffer}, 'slow-request') + if (response.ok) return + const lastAttempt = attempt === UPLOAD_MAX_ATTEMPTS + const retryable = RETRYABLE_UPLOAD_STATUS_CODES.has(response.status) + // node-fetch keeps the socket open until the body is consumed. On the final + // attempt we read it below for the error message; otherwise drain it here so + // the connection can be reused or released before the next attempt. + if (retryable && !lastAttempt) { + // eslint-disable-next-line no-await-in-loop + await response.text().catch(() => {}) + // eslint-disable-next-line no-await-in-loop + await sleep(2 ** attempt * 0.1) + continue + } + break + } + + const status = response?.status + const responseBody = (await response?.text().catch(() => ''))?.trim() + throw new AbortError( + `Failed to upload your app bundle to storage${status ? ` (HTTP ${status})` : ''}.`, + 'This is usually transient. Please try again, and check your network connection if it persists.', + responseBody ? [`Storage responded with: ${responseBody.slice(0, 300)}`] : undefined, + ) } /** diff --git a/packages/app/src/cli/services/deploy/upload.test.ts b/packages/app/src/cli/services/deploy/upload.test.ts index 7b428edc9de..fab9ca04a84 100644 --- a/packages/app/src/cli/services/deploy/upload.test.ts +++ b/packages/app/src/cli/services/deploy/upload.test.ts @@ -1,9 +1,9 @@ import {deploymentErrorsToCustomSections, uploadExtensionsBundle} from './upload.js' import {testApp, testDeveloperPlatformClient} from '../../models/app/app.test-data.js' import {AppDeploySchema, AppDeployVariables} from '../../api/graphql/app_deploy.js' -import {describe, expect, test, vi} from 'vitest' +import {beforeEach, describe, expect, test, vi} from 'vitest' import {inTemporaryDirectory, writeFile} from '@shopify/cli-kit/node/fs' -import {formData} from '@shopify/cli-kit/node/http' +import {fetch, formData} from '@shopify/cli-kit/node/http' import {joinPath} from '@shopify/cli-kit/node/path' vi.mock('@shopify/cli-kit/node/http') @@ -13,6 +13,12 @@ const app = testApp() const appManifest = await app.manifest(undefined) describe('uploadExtensionsBundle', () => { + beforeEach(() => { + // uploadToGCS validates the upload response status, so the signed PUT must + // resolve to a successful response or the upload would (correctly) throw. + vi.mocked(fetch).mockResolvedValue({ok: true, status: 200} as any) + }) + test('calls a mutation on partners', async () => { await inTemporaryDirectory(async (tmpDir) => { // Given