Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Shivanshu-07 wants to merge 2 commits into
base: master
Choose a base branch
from
+73 −62
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
06ba590
fix(core): stop async-iterating percy.upload() in sync relay routes
Shivanshu-07 Jun 23, 2026
61cce14
fix(core): guard sync upload against generator errors and strengthen …
Shivanshu-07 Jun 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 17 additions & 19 deletions packages/core/src/api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 });
Expand Down Expand Up @@ -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 {
Expand Down
99 changes: 56 additions & 43 deletions packages/core/test/api.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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();
Expand All @@ -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();

Expand All @@ -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 () => {
Expand Down
Loading