Merge branch 'main' into dependabot-npm_and_yarn-packages-ui-extensions-server-kit-vite-6.3.6

Author: isaacroldan

.changeset/healthy-coins-walk.md

@@ -0,0 +1,5 @@
+---
+'@shopify/app': patch
+---
+
+Fix concurrency issue with downloading Functions binaries

package.json

@@ -70,7 +70,7 @@
     "liquidjs": "10.20.1",
     "node-fetch": "^3.3.2",
     "nx": "21.5.2",
-    "oclif": "4.20.1",
+    "oclif": "4.22.22",
     "octokit-plugin-create-pull-request": "^3.12.2",
     "pathe": "1.1.1",
     "pin-github-action": "^3.3.1",
@@ -100,7 +100,6 @@
   "resolutions": {
     "@types/react": "17.0.2",
     "vite": "6.3.6",
-    "@oclif/core": "4.4.0",
     "whatwg-url": "14.0.0",
     "supports-hyperlinks": "3.1.0",
     "@graphql-tools/utils": "10.7.2",

packages/app/package.json

@@ -49,7 +49,7 @@
   "dependencies": {
     "@graphql-typed-document-node/core": "3.2.0",
     "@luckycatfactory/esbuild-graphql-loader": "3.8.1",
-    "@oclif/core": "4.4.0",
+    "@oclif/core": "4.5.3",
     "@shopify/cli-kit": "3.84.0",
     "@shopify/function-runner": "4.1.1",
     "@shopify/plugin-cloudflare": "3.84.0",

packages/app/src/cli/services/function/binaries.test.ts

@@ -141,6 +141,26 @@ describe('javy', () => {
     expect(fetch).toHaveBeenCalledOnce()
     await expect(fileExists(javy.path)).resolves.toBeTruthy()
   })
+
+  test('handles concurrent downloads of Javy', async () => {
+    // Given
+    await removeFile(javy.path)
+    await expect(fileExists(javy.path)).resolves.toBeFalsy()
+    vi.mocked(fetch).mockResolvedValue(new Response(gzipSync('javy binary')))
+
+    // When
+    await Promise.all([
+      downloadBinary(javy),
+      downloadBinary(javy),
+      downloadBinary(javy),
+      downloadBinary(javy),
+      downloadBinary(javy),
+    ])
+
+    // Then
+    expect(fetch).toHaveBeenCalledOnce()
+    await expect(fileExists(javy.path)).resolves.toBeTruthy()
+  })
 })
 
 describe('javy-plugin', () => {

packages/app/src/cli/services/function/binaries.ts

@@ -203,12 +203,52 @@ export function trampolineBinary() {
   return _trampoline
 }
 
+const downloadsInProgress = new Map<string, Promise<void>>()
+
 export async function downloadBinary(bin: DownloadableBinary) {
   const isDownloaded = await fileExists(bin.path)
   if (isDownloaded) {
     return
   }
 
+  // Downloads cannot run concurrently with `exec` since the `moveFile`
+  // operation is not atomic. It will delete the destination file if it exists
+  // which will cause `exec` to break if it tries to execute the file before
+  // the source file has been moved.
+  // We prevent downloads from happening concurrently with `exec` by enforcing
+  // that only one download happens for an executable. If we get here, we check
+  // if a download is in progress, wait for it to finish, and return without
+  // downloading again. If it's not in progress, then we start the download.
+  const downloadPromise = downloadsInProgress.get(bin.path)
+  if (downloadPromise) {
+    await downloadPromise
+    // Return now since we can assume it's downloaded. If it's not, an exception should've
+    // been thrown before which will cause the user-level operation to fail anyway.
+    return
+  }
+
+  // Do not perform any `await`s until we've called `downloadsInProgress.set`.
+  // Calling `await` before that can cause a different JS task to become active
+  // and start a concurrent download for this binary.
+
+  // My mental model is `performDownload` without the `await` will run
+  // synchronously until the first `await` in the function and then
+  // immediately return the promise which we then immediately store. Since JS
+  // doesn't have preemptive concurrency, we should be able to safely assume a
+  // different task in the task queue will not run in between starting
+  // `downloadFn` and the `set` operation on the following line.
+  const downloadOp = performDownload(bin)
+  downloadsInProgress.set(bin.path, downloadOp)
+  // Ensure we clean the entry if there's a failure.
+  try {
+    // Wait for the download to finish
+    await downloadOp
+  } finally {
+    downloadsInProgress.delete(bin.path)
+  }
+}
+
+async function performDownload(bin: DownloadableBinary) {
   const url = bin.downloadUrl(process.platform, process.arch)
   outputDebug(`Downloading ${bin.name} ${bin.version} from ${url} to ${bin.path}`)
   const dir = dirname(bin.path)

packages/cli-kit/package.json

@@ -103,7 +103,7 @@
     "@bugsnag/js": "7.25.0",
     "@graphql-typed-document-node/core": "3.2.0",
     "@iarna/toml": "2.2.5",
-    "@oclif/core": "4.4.0",
+    "@oclif/core": "4.5.3",
     "@opentelemetry/api": "1.9.0",
     "@opentelemetry/core": "1.30.0",
     "@opentelemetry/exporter-metrics-otlp-http": "0.57.0",

packages/cli/package.json

@@ -106,9 +106,9 @@
     "global-agent": "3.0.0"
   },
   "devDependencies": {
-    "@oclif/core": "4.4.0",
-    "@oclif/plugin-commands": "4.1.26",
-    "@oclif/plugin-plugins": "5.4.43",
+    "@oclif/core": "4.5.3",
+    "@oclif/plugin-commands": "4.1.33",
+    "@oclif/plugin-plugins": "5.4.47",
     "@shopify/app": "3.84.0",
     "@shopify/cli-kit": "3.84.0",
     "@shopify/plugin-cloudflare": "3.84.0",

packages/cli/src/index.ts

@@ -33,7 +33,7 @@ export {hooks as PluginHook} from '@oclif/plugin-plugins'
 export {AppSensitiveMetadataHook, AppInitHook, AppPublicMetadataHook} from '@shopify/app'
 export {push, pull, fetchStoreThemes} from '@shopify/theme'
 
-export const HydrogenInitHook = HydrogenHooks.init
+export const HydrogenInitHook: unknown = HydrogenHooks.init
 
 // Setup global support for environment variable based proxy configuration.
 createGlobalProxyAgent({

packages/plugin-cloudflare/package.json

@@ -46,7 +46,7 @@
     ]
   },
   "dependencies": {
-    "@oclif/core": "4.4.0",
+    "@oclif/core": "4.5.3",
     "@shopify/cli-kit": "3.84.0"
   },
   "devDependencies": {

packages/plugin-did-you-mean/package.json

@@ -41,7 +41,7 @@
     ]
   },
   "dependencies": {
-    "@oclif/core": "4.4.0",
+    "@oclif/core": "4.5.3",
     "@shopify/cli-kit": "3.84.0",
     "n-gram": "2.0.2"
   },