do not crash when cannot upload asset

lib/s3UploadFile.js

@@ -1,6 +1,10 @@
 import fs from 'fs';
+import path from 'path';
 import mime from 'mime';
 
+const uploadError = status =>
+  Object.assign(new Error(`Upload failed with status ${status}`), { statusCode: status });
+
 const uploadFile = async (fileName, s3Url) => {
   const stats = fs.statSync(fileName);
   const fileBuffer = fs.readFileSync(fileName);
@@ -16,7 +20,7 @@ const uploadFile = async (fileName, s3Url) => {
   });
 
   if (!response.ok) {
-    throw new Error(`Upload failed with status ${response.status}`);
+    throw uploadError(response.status);
   }
 
   return s3Url;
@@ -36,7 +40,9 @@ const uploadFileFormData = async (filePath, data) => {
     formData.append('Content-Type', contentType);
   }
 
-  const fileName = filePath.split('/').pop();
+  // S3 expands this into the ${filename} placeholder in the presigned key, so it
+  // must be the basename even when the caller passes an OS-native path.
+  const fileName = path.basename(filePath);
   formData.append('file', new File([fileBuffer], fileName, { type: contentType }));
 
   const response = await fetch(data.url, {
@@ -45,10 +51,10 @@ const uploadFileFormData = async (filePath, data) => {
   });
 
   if (!response.ok) {
-    throw new Error(`Upload failed with status ${response.status}`);
+    throw uploadError(response.status);
   }
 
   return true;
 };
 
-export { uploadFile, uploadFileFormData };
+export { uploadError, uploadFile, uploadFileFormData };

lib/watch.js

@@ -2,7 +2,6 @@ import fs from 'fs';
 import path from 'path';
 import chokidar from 'chokidar';
 import async from 'async';
-import cloneDeep from 'lodash.clonedeep';
 import debounce from 'lodash.debounce';
 import ServerError from './ServerError.js';
 
@@ -28,6 +27,13 @@ class AlreadyLoggedError extends Error {
   }
 }
 
+// Report a sync failure without killing the process, then throw so callers know
+// it was already logged — watch mode's queue continues, sync -f exits non-zero.
+const failSync = async (logMessage, errorMessage = logMessage) => {
+  await logger.Error(logMessage, { exit: false, notify: false });
+  throw new AlreadyLoggedError(errorMessage);
+};
+
 const filePathUnixified = filePath =>
   filePath
     .replace(/\\/g, '/')
@@ -115,14 +121,12 @@ const pushFile = async (gateway, syncedFilePath) => {
       const body = e.response.body;
       const error = body.error || (body.errors && body.errors.join(', '));
       if (error) {
-        await logger.Error(`[Sync] Failed to sync: ${filePath}\n${error}`, { exit: false, notify: false });
-        throw new AlreadyLoggedError(error);
+        await failSync(`[Sync] Failed to sync: ${filePath}\n${error}`, error);
       }
     }
     // Network connection errors should not kill sync — it may be a transient failure
     if (e.name === 'RequestError') {
-      await logger.Error(`[Sync] Failed to sync: ${filePath}`, { exit: false, notify: false });
-      throw new AlreadyLoggedError(e.message);
+      await failSync(`[Sync] Failed to sync: ${filePath}`, e.message);
     }
     // For HTTP status code errors, use the centralized handler
     await ServerError.handler(e);
@@ -146,14 +150,12 @@ const deleteFile = async (gateway, syncedFilePath) => {
       const body = e.response.body;
       const error = body.error || (body.errors && body.errors.join(', '));
       if (error) {
-        await logger.Error(`[Sync] Failed to delete: ${filePath}\n${error}`, { exit: false, notify: false });
-        throw new AlreadyLoggedError(error);
+        await failSync(`[Sync] Failed to delete: ${filePath}\n${error}`, error);
       }
     }
     // Network connection errors should not kill sync — it may be a transient failure
     if (e.name === 'RequestError') {
-      await logger.Error(`[Sync] Failed to delete: ${filePath}`, { exit: false, notify: false });
-      throw new AlreadyLoggedError(e.message);
+      await failSync(`[Sync] Failed to delete: ${filePath}`, e.message);
     }
     await ServerError.handler(e);
   }
@@ -168,42 +170,91 @@ const pushFileDirectAssets = async (gateway, syncedFilePath) => {
   }
 };
 
+// Register the assets uploaded since the last flush. Building the manifest reads
+// each file from disk, so it can fail just like the request itself; either way the
+// batch is put back so the next flush retries it — the assets are already
+// uploaded, they are just not registered yet.
+const sendManifestBatch = async gateway => {
+  if (manifestFilesToAdd.length === 0) return;
+
+  const batch = manifestFilesToAdd;
+  manifestFilesToAdd = [];
+  try {
+    const manifest = manifestGenerateForAssets(batch);
+    logger.Debug(manifest);
+    await gateway.sendManifest(manifest);
+  } catch (e) {
+    manifestFilesToAdd.push(...batch);
+    throw e;
+  }
+};
+
 const manifestSend = debounce(
   gateway => {
-    const manifest = manifestGenerateForAssets(manifestFilesToAdd.slice());
-    logger.Debug(manifest);
-    gateway.sendManifest(manifest);
-    manifestFilesToAdd = [];
+    // Fires from a debounce timer, outside any request's try/catch — an error here
+    // would be unhandled and kill the process. sendManifestBatch is async, so a
+    // throw while building the manifest arrives as a rejection too.
+    sendManifestBatch(gateway).catch(e =>
+      logger.Error(`[Sync] Failed to update assets manifest: ${e.message || e}`, { exit: false, notify: false })
+    );
   },
   1000,
   { maxWait: 1000 * 10 }
 );
 
 const manifestAddAsset = path => manifestFilesToAdd.push(path);
 
+const assetUploadData = normalizedPath => {
+  const fileSubdir = normalizedPath.startsWith('app/assets')
+    ? path.dirname(normalizedPath).replace('app/assets', '')
+    : '/' + path.dirname(normalizedPath).replace('/public/assets', '');
+  const key = directUploadData.fields.key.replace('assets/${filename}', `assets${fileSubdir}/\${filename}`);
+  const data = { ...directUploadData, fields: { ...directUploadData.fields, key } };
+  logger.Debug(data);
+  return data;
+};
+
+const uploadAsset = async (gateway, filePath, normalizedPath) => {
+  const authorizationUsed = directUploadData;
+  try {
+    await uploadFileFormData(filePath, assetUploadData(normalizedPath));
+  } catch (e) {
+    // The presigned upload authorization is fetched once at sync start and
+    // expires server-side; from then on every asset upload gets a 403 until
+    // it is refreshed. Refresh and retry once instead of failing.
+    if (e.statusCode !== 403) throw e;
+    logger.Debug('[Sync] Asset upload authorization expired, refreshing it and retrying...');
+    // A concurrent upload may have refreshed it already while this one was in
+    // flight — then just retry with what it fetched.
+    if (directUploadData === authorizationUsed) await refreshDirectUploadData(gateway);
+    await uploadFileFormData(filePath, assetUploadData(normalizedPath));
+  }
+};
+
 const sendAsset = async (gateway, filePath) => {
+  const normalizedPath = filePath.replace(/\\/g, '/');
   try {
-    const data = cloneDeep(directUploadData);
-    const normalizedPath = filePath.replace(/\\/g, '/');
-    const fileSubdir = normalizedPath.startsWith('app/assets')
-      ? path.dirname(normalizedPath).replace('app/assets', '')
-      : '/' + path.dirname(normalizedPath).replace('/public/assets', '');
-    const key = data.fields.key.replace('assets/${filename}', `assets${fileSubdir}/\${filename}`);
-    data.fields.key = key;
-    logger.Debug(data);
-    await uploadFileFormData(filePath, data);
+    await uploadAsset(gateway, filePath, normalizedPath);
     manifestAddAsset(filePath);
     manifestSend(gateway);
     logger.Success(`[Sync] Synced asset: ${normalizedPath}`);
   } catch (e) {
     logger.Debug(e.message);
     logger.Debug(e.stack);
-    if (ServerError.isNetworkError(e)) {
-      await logger.Error(`[Sync] Failed to sync: ${filePath}`);
+    // Network connection errors should not kill sync — it may be a transient failure
+    if (e.name === 'RequestError') {
+      await failSync(`[Sync] Failed to sync: ${normalizedPath}`, e.message);
+    }
+    // Refreshing the upload authorization talks to the API, so its failures are
+    // HTTP status code errors — the centralized handler explains those (a 401
+    // tells the user to refresh their token).
+    if (e.name === 'StatusCodeError') {
       await ServerError.handler(e);
-    } else {
-      await logger.Error(`[Sync] Failed to sync ${filePath}: ${e.message || e}`);
     }
+    const message = e.message || String(e);
+    // Asset upload failures must never kill watch mode — log and let the queue
+    // continue; the throw lets single-file mode (sync -f) exit non-zero.
+    await failSync(`[Sync] Failed to sync ${normalizedPath}: ${message}`, message);
   }
 };
 
@@ -214,6 +265,16 @@ const fetchDirectUploadData = async gateway => {
   directUploadData = data;
 };
 
+// The queue uploads several assets concurrently, so when the authorization
+// expires they all get 403 at once — share one in-flight refresh between them.
+let directUploadDataRefresh = null;
+const refreshDirectUploadData = gateway => {
+  directUploadDataRefresh ||= fetchDirectUploadData(gateway).finally(() => {
+    directUploadDataRefresh = null;
+  });
+  return directUploadDataRefresh;
+};
+
 const start = async (env, directAssetsUpload, liveReload) => {
   const program = {
     email: env.MARKETPLACE_EMAIL,
@@ -330,12 +391,10 @@ const sendFile = async (gateway, filePath) => {
   await pushFileDirectAssets(gateway, filePath);
 
   // If it was an asset file, we need to flush the manifest immediately
-  // since we're not in watch mode with debouncing
-  if (isAssetsPath(filePath) && manifestFilesToAdd.length > 0) {
-    const manifest = manifestGenerateForAssets(manifestFilesToAdd.slice());
-    logger.Debug(manifest);
-    await gateway.sendManifest(manifest);
-    manifestFilesToAdd = [];
+  // since we're not in watch mode with debouncing. Unlike the debounced flush this
+  // one lets the error through, so a single-file sync exits non-zero.
+  if (isAssetsPath(filePath)) {
+    await sendManifestBatch(gateway);
   }
 };
 

package.json

@@ -60,7 +60,6 @@
     "inquirer": "^13.3.0",
     "is-stream": "^4.0.1",
     "livereload": "^0.10.3",
-    "lodash.clonedeep": "^4.5.0",
     "lodash.compact": "^3.0.1",
     "lodash.debounce": "^4.0.8",
     "lodash.flatten": "^4.4.0",

test/unit/s3UploadFile.test.js

@@ -4,6 +4,7 @@
  */
 import { describe, test, expect, vi, beforeEach, afterEach } from 'vitest';
 import fs from 'fs';
+import path from 'path';
 import mime from 'mime';
 
 // Mock fs module
@@ -114,7 +115,11 @@ describe('s3UploadFile', () => {
         status: 403
       });
 
-      await expect(uploadFile(fileName, s3Url)).rejects.toThrow('Upload failed with status 403');
+      // statusCode lets callers (sendAsset) detect an expired upload authorization and retry
+      await expect(uploadFile(fileName, s3Url)).rejects.toMatchObject({
+        message: 'Upload failed with status 403',
+        statusCode: 403
+      });
     });
 
     test('throws error when upload fails with 500 server error', async () => {
@@ -260,8 +265,10 @@ describe('s3UploadFile', () => {
       expect(result).toBe(true);
     });
 
-    test('correctly extracts filename from Unix path', async () => {
-      const filePath = '/home/user/projects/app/assets/images/logo.png';
+    // S3 expands the uploaded file's name into the ${filename} placeholder in the
+    // presigned key, so anything but the basename puts the object at the wrong key
+    // while the upload still reports success.
+    const uploadedFileName = async filePath => {
       const data = {
         url: 'https://s3.amazonaws.com/bucket',
         fields: { key: 'assets/${filename}' }
@@ -273,26 +280,21 @@ describe('s3UploadFile', () => {
 
       await uploadFileFormData(filePath, data);
 
-      // Verify FormData was created (we can't easily inspect FormData contents)
-      expect(global.fetch).toHaveBeenCalled();
-      const fetchCall = global.fetch.mock.calls[0];
-      expect(fetchCall[1].body).toBeInstanceOf(FormData);
-    });
-
-    test('correctly extracts filename from Windows path', async () => {
-      const filePath = 'C:\\Users\\user\\projects\\app\\assets\\images\\logo.png';
-      const data = {
-        url: 'https://s3.amazonaws.com/bucket',
-        fields: { key: 'assets/${filename}' }
-      };
+      const body = global.fetch.mock.calls[0][1].body;
+      expect(body).toBeInstanceOf(FormData);
+      return body.get('file').name;
+    };
 
-      fs.readFileSync.mockReturnValue(Buffer.from('image'));
-      mime.getType.mockReturnValue('image/png');
-      global.fetch.mockResolvedValue({ ok: true, status: 200 });
+    test('correctly extracts filename from Unix path', async () => {
+      expect(await uploadedFileName('/home/user/projects/app/assets/images/logo.png')).toBe('logo.png');
+    });
 
-      await uploadFileFormData(filePath, data);
+    test('correctly extracts filename from a native OS path', async () => {
+      // Built with path.join so it uses backslashes on Windows, where a path split
+      // on '/' alone would yield the whole path as the filename.
+      const filePath = path.join('modules', 'community', 'public', 'assets', 'images', 'logo.png');
 
-      expect(global.fetch).toHaveBeenCalled();
+      expect(await uploadedFileName(filePath)).toBe('logo.png');
     });
 
     test('handles large file near 50MB limit with FormData', async () => {
@@ -328,7 +330,10 @@ describe('s3UploadFile', () => {
         status: 403
       });
 
-      await expect(uploadFileFormData(filePath, data)).rejects.toThrow('Upload failed with status 403');
+      await expect(uploadFileFormData(filePath, data)).rejects.toMatchObject({
+        message: 'Upload failed with status 403',
+        statusCode: 403
+      });
     });
 
     test('throws error when FormData upload fails with network error', async () => {
@@ -584,7 +589,10 @@ describe('s3UploadFile', () => {
         status: 403
       });
 
-      await expect(uploadFile(fileName, s3Url)).rejects.toThrow('Upload failed with status 403');
+      await expect(uploadFile(fileName, s3Url)).rejects.toMatchObject({
+        message: 'Upload failed with status 403',
+        statusCode: 403
+      });
     });
 
     test('uploadFileFormData handles invalid presigned data', async () => {