Validate GCS bundle upload status in uploadToGCS

.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

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('<Error>SignatureExpired</Error>'),
+      } 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.

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,
+  )
 }
 
 /**

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