fix(core): [PER-9706] sync Automate snapshots fail with "upload is not async iterable" (#2314)

* fix(core): stop async-iterating percy.upload() in sync relay routes

percy.upload() is the generatePromise-wrapped method and returns a Promise,
not an async iterable. The sync branches of /percy/comparison,
/percy/maestro-screenshot and /percy/automateScreenshot were changed (while
fixing Maestro sync) to `for await (const _ of percy.upload(...))`, which
throws "upload is not async iterable". SDKs surface this as
{"error":"upload is not async iterable"} on every sync snapshot, regressing
Percy-on-Automate sync (sync: true) on @percy/cli 1.32.0-1.32.2.

generatePromise already drives the underlying async generator to completion,
so calling percy.upload() inside the Promise executor enqueues #snapshots and
lets the sync queue resolve/reject the attached callback. Revert all three
sync routes to that proven 1.31.8 pattern (the raw generator remains
available at percy.yield.upload() if direct iteration is ever needed).

The original drain-canary tests passed only because they mocked percy.upload
to return an async generator, contradicting its real shape. Replace them with
regression tests that model the real return shape (a Promise that resolves
the callback asynchronously), so async-iterating the return value would fail.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(core): guard sync upload against generator errors and strengthen tests

Address PR review on the sync relay fix:

- Add a trailing .catch(reject) to percy.upload() at all three sync routes
  (/percy/comparison, /percy/maestro-screenshot, /percy/automateScreenshot)
  so a generator error that bypasses the sync-queue callback (e.g. a throw
  before the queue task runs) is surfaced as data.error via handleSyncJob
  instead of leaking an unhandled rejection and hanging the request.

- Assert percy.client.getComparisonDetails was called in the three sync
  regression tests, proving handleSyncJob ran to completion rather than the
  request resolving early.

- Add a /percy/comparison test that a rejected upload Promise (no callback
  invoked) surfaces as data.error, directly covering the new .catch(reject).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: Shivanshu-07

packages/core/src/api.js

@@ -385,13 +385,16 @@ export function createPercyServer(percy, port) {
     .route('post', '/percy/comparison', async (req, res) => {
       let data;
       if (percy.syncMode(req.body)) {
-        // percy.upload returns an async generator that must be drained for #snapshots.push to run.
+        // percy.upload() is the generatePromise-wrapped method: calling it drives the
+        // underlying async generator to completion (enqueuing #snapshots) and the sync
+        // queue resolves/rejects the attached callback. Do NOT `for await` the return
+        // value — it is a Promise, not an async iterable. The raw generator lives at
+        // percy.yield.upload() if direct iteration is ever needed. The trailing
+        // .catch(reject) surfaces generator errors that bypass the sync-queue callback
+        // (e.g. a throw before the queue task runs) instead of leaking an unhandled
+        // rejection and hanging the request.
         const snapshotPromise = new Promise((resolve, reject) => {
-          const upload = percy.upload(req.body, { resolve, reject }, 'app');
-          (async () => {
-            // eslint-disable-next-line no-unused-vars
-            try { for await (const _ of upload) { /* drain */ } } catch (e) { reject(e); }
-          })();
+          percy.upload(req.body, { resolve, reject }, 'app').catch(reject);
         });
         data = await handleSyncJob(snapshotPromise, percy, 'comparison');
       } else {
@@ -905,14 +908,11 @@ export function createPercyServer(percy, port) {
 
       let data;
       if (percy.syncMode(payload)) {
-        // percy.upload returns an async generator that must be drained for #snapshots.push to run.
-        // See docs/solutions/best-practices/2026-05-20-maestro-sync-promise-bug-investigation.md.
+        // See the /percy/comparison route: percy.upload() is the Promise-wrapped method;
+        // calling it drives the generator and the sync queue resolves/rejects the callback.
+        // The .catch(reject) surfaces generator errors that bypass that callback.
         const snapshotPromise = new Promise((resolve, reject) => {
-          const upload = percy.upload(payload, { resolve, reject }, 'app');
-          (async () => {
-            // eslint-disable-next-line no-unused-vars
-            try { for await (const _ of upload) { /* drain */ } } catch (e) { reject(e); }
-          })();
+          percy.upload(payload, { resolve, reject }, 'app').catch(reject);
         });
         data = await handleSyncJob(snapshotPromise, percy, 'comparison');
         return res.json(200, { success: true, data });
@@ -946,13 +946,11 @@ export function createPercyServer(percy, port) {
       let comparisonData = await WebdriverUtils.captureScreenshot(req.body);
 
       if (percy.syncMode(comparisonData)) {
-        // percy.upload returns an async generator that must be drained for #snapshots.push to run.
+        // See the /percy/comparison route: percy.upload() is the Promise-wrapped method;
+        // calling it drives the generator and the sync queue resolves/rejects the callback.
+        // The .catch(reject) surfaces generator errors that bypass that callback.
         const snapshotPromise = new Promise((resolve, reject) => {
-          const upload = percy.upload(comparisonData, { resolve, reject }, 'automate');
-          (async () => {
-            // eslint-disable-next-line no-unused-vars
-            try { for await (const _ of upload) { /* drain */ } } catch (e) { reject(e); }
-          })();
+          percy.upload(comparisonData, { resolve, reject }, 'automate').catch(reject);
         });
         data = await handleSyncJob(snapshotPromise, percy, 'comparison');
       } else {

packages/core/test/api.test.js

@@ -336,31 +336,50 @@ describe('API Server', () => {
     expect(percy.client.getComparisonDetails).toHaveBeenCalled();
   });
 
-  // Cross-consumer drain canary for /percy/comparison. Mirrors the maestro-screenshot
-  // canary at the bottom of this file. See docs/solutions/best-practices/
-  // 2026-05-20-maestro-sync-promise-bug-investigation.md.
-  it('/comparison sync mode: drains the upload generator (real return shape, no mock)', async () => {
-    let iterCount = 0;
+  // Regression: percy.upload() is the generatePromise-wrapped method and returns a Promise,
+  // not an async iterable. The relay must drive it and let the sync queue resolve the
+  // attached callback — it must never `for await` the return value (that throws
+  // "upload is not async iterable"). Modelled with the real shape: a Promise return whose
+  // callback is resolved asynchronously, so a `for await` over it would reject first.
+  it('/comparison sync mode: resolves via the upload callback, not by iterating the return value', async () => {
     spyOn(percy.client, 'getComparisonDetails').and.returnValue(getSnapshotDetailsResponse);
     spyOn(percy, 'upload').and.callFake((_, callback) => {
-      return (async function*() {
-        iterCount++;
-        callback.resolve();
-        yield;
-      })();
+      let promise = Promise.resolve();
+      promise.then(() => callback.resolve());
+      return promise;
     });
     await percy.start();
 
     await expectAsync(request('/percy/comparison', {
       method: 'POST',
       body: {
-        name: 'Drain canary',
+        name: 'Sync regression',
         sync: true,
         tag: { name: 'Tag', osName: 'OS', osVersion: '1', width: 1, height: 1, orientation: 'portrait' }
       }
     })).toBeResolvedTo(jasmine.objectContaining({ data: getSnapshotDetailsResponse }));
 
-    expect(iterCount).toBeGreaterThan(0);
+    expect(percy.upload).toHaveBeenCalledOnceWith(
+      jasmine.objectContaining({ name: 'Sync regression' }), jasmine.objectContaining({}), 'app');
+    // Proves handleSyncJob ran to completion rather than the request resolving early.
+    expect(percy.client.getComparisonDetails).toHaveBeenCalled();
+  });
+
+  // A generator-level failure that bypasses the sync-queue callback (the upload Promise
+  // rejects without resolve/reject being invoked) must surface as data.error via the
+  // route's .catch(reject), not hang the request.
+  it('/comparison sync mode: surfaces a rejected upload Promise as data.error', async () => {
+    spyOn(percy, 'upload').and.callFake(() => Promise.reject(new Error('generator boom')));
+    await percy.start();
+
+    await expectAsync(request('/percy/comparison', {
+      method: 'POST',
+      body: {
+        name: 'Sync reject',
+        sync: true,
+        tag: { name: 'Tag', osName: 'OS', osVersion: '1', width: 1, height: 1, orientation: 'portrait' }
+      }
+    })).toBeResolvedTo(jasmine.objectContaining({ data: { error: 'generator boom' } }));
   });
 
   it('includes links in the /comparison endpoint response', async () => {
@@ -607,33 +626,32 @@ describe('API Server', () => {
     resolve();
   });
 
-  // Cross-consumer drain canary for /percy/automateScreenshot. Mirrors the
-  // maestro-screenshot and /comparison canaries elsewhere in this file. See
-  // docs/solutions/best-practices/2026-05-20-maestro-sync-promise-bug-investigation.md.
-  it('/automateScreenshot sync mode: drains the upload generator (real return shape, no mock)', async () => {
-    let iterCount = 0;
+  // Regression mirror of the /comparison case for the Percy-on-Automate sync route:
+  // percy.upload() returns a Promise, so the relay must resolve through the sync callback
+  // rather than `for await`-ing the return value ("upload is not async iterable").
+  it('/automateScreenshot sync mode: resolves via the upload callback, not by iterating the return value', async () => {
     spyOn(percy.client, 'getComparisonDetails').and.returnValue(getSnapshotDetailsResponse);
     spyOn(WebdriverUtils, 'captureScreenshot').and.returnValue({ sync: true });
     spyOn(percy, 'upload').and.callFake((_, callback) => {
-      return (async function*() {
-        iterCount++;
-        callback.resolve();
-        yield;
-      })();
+      let promise = Promise.resolve();
+      promise.then(() => callback.resolve());
+      return promise;
     });
     await percy.start();
 
     await expectAsync(request('/percy/automateScreenshot', {
       method: 'post',
       body: {
-        name: 'Drain canary',
+        name: 'Sync regression',
         client_info: 'client',
         environment_info: 'environment',
         options: { sync: true }
       }
     })).toBeResolvedTo(jasmine.objectContaining({ data: getSnapshotDetailsResponse }));
 
-    expect(iterCount).toBeGreaterThan(0);
+    expect(percy.upload).toHaveBeenCalledOnceWith({ sync: true }, jasmine.objectContaining({}), 'automate');
+    // Proves handleSyncJob ran to completion rather than the request resolving early.
+    expect(percy.client.getComparisonDetails).toHaveBeenCalled();
   });
 
   it('has a /events endpoint that calls #sendBuildEvents() async with provided options with clientInfo present', async () => {
@@ -1568,11 +1586,9 @@ describe('API Server', () => {
       expect(payload.sync).toBe(true);
     });
 
-    // Sync mode bug fix coverage — see docs/solutions/best-practices/
-    // 2026-05-20-maestro-sync-promise-bug-investigation.md.
-    // Before the fix, the relay's `new Promise(executor => percy.upload(...))`
-    // returned an async generator that was never iterated, so #snapshots.push
-    // never ran and the promise hung forever. The fix drains the generator.
+    // Sync mode: a rejected upload is surfaced as data.error in a 200 response. The relay
+    // wires the sync queue's reject to the snapshot promise, which handleSyncJob converts
+    // into { error } rather than failing the request.
     it('sync mode: surfaces upload reject error as data.error (200 with error field)', async () => {
       spyOn(percy, 'upload').and.callFake((_, callback) => callback.reject(new Error('boom')));
       await percy.start();
@@ -1587,19 +1603,15 @@ describe('API Server', () => {
       }));
     });
 
-    it('sync mode: drains the upload generator (real percy.upload return shape, no mock)', async () => {
-      // Canary for the structural bug: spy on percy.upload but have it return a real
-      // async generator-shaped object that records whether it gets iterated.
-      // Before the fix, this iteration count would stay at 0 and the test would time out.
-      let iterCount = 0;
+    // Regression mirror of the /comparison and /automateScreenshot cases: percy.upload()
+    // returns a Promise, so the relay must resolve through the sync callback rather than
+    // `for await`-ing the return value ("upload is not async iterable").
+    it('sync mode: resolves via the upload callback, not by iterating the return value', async () => {
       spyOn(percy.client, 'getComparisonDetails').and.returnValue(getSnapshotDetailsResponse);
       spyOn(percy, 'upload').and.callFake((options, callback) => {
-        return (async function*() {
-          iterCount++;
-          // Simulate the queue-task path: the syncQueue would invoke callback.resolve.
-          callback.resolve();
-          yield;
-        })();
+        let promise = Promise.resolve();
+        promise.then(() => callback.resolve());
+        return promise;
       });
       await percy.start();
 
@@ -1610,9 +1622,10 @@ describe('API Server', () => {
         sync: true
       })).toBeResolvedTo(jasmine.objectContaining({ data: getSnapshotDetailsResponse }));
 
-      // Iteration count > 0 proves the relay drained the generator (vs the old
-      // bug where the generator was discarded).
-      expect(iterCount).toBeGreaterThan(0);
+      expect(percy.upload).toHaveBeenCalledOnceWith(
+        jasmine.objectContaining({ sync: true }), jasmine.objectContaining({}), 'app');
+      // Proves handleSyncJob ran to completion rather than the request resolving early.
+      expect(percy.client.getComparisonDetails).toHaveBeenCalled();
     });
 
     it('returns 404 when the screenshot file is missing', async () => {