From 06ba59068072e3bd67a1bf3a0efe573008331f5f Mon Sep 17 00:00:00 2001 From: Shivanshu07 Date: Tue, 23 Jun 2026 22:42:04 +0530 Subject: [PATCH 1/2] 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) --- packages/core/src/api.js | 37 ++++++----------- packages/core/test/api.test.js | 76 +++++++++++++++------------------- 2 files changed, 45 insertions(+), 68 deletions(-) diff --git a/packages/core/src/api.js b/packages/core/src/api.js index c3dd2d6a8..2d5000ce0 100644 --- a/packages/core/src/api.js +++ b/packages/core/src/api.js @@ -385,14 +385,12 @@ 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. - 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() 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. + const snapshotPromise = new Promise((resolve, reject) => percy.upload(req.body, { resolve, reject }, 'app')); data = await handleSyncJob(snapshotPromise, percy, 'comparison'); } else { let upload = percy.upload(req.body, null, 'app'); @@ -905,15 +903,9 @@ 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. - 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); } - })(); - }); + // 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. + const snapshotPromise = new Promise((resolve, reject) => percy.upload(payload, { resolve, reject }, 'app')); data = await handleSyncJob(snapshotPromise, percy, 'comparison'); return res.json(200, { success: true, data }); } @@ -946,14 +938,9 @@ 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. - 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); } - })(); - }); + // 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. + const snapshotPromise = new Promise((resolve, reject) => percy.upload(comparisonData, { resolve, reject }, 'automate')); data = await handleSyncJob(snapshotPromise, percy, 'comparison'); } else { percy.upload(comparisonData, null, 'automate'); diff --git a/packages/core/test/api.test.js b/packages/core/test/api.test.js index 88f2a6dc1..953c60503 100644 --- a/packages/core/test/api.test.js +++ b/packages/core/test/api.test.js @@ -336,31 +336,31 @@ 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'); }); it('includes links in the /comparison endpoint response', async () => { @@ -607,33 +607,30 @@ 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'); }); it('has a /events endpoint that calls #sendBuildEvents() async with provided options with clientInfo present', async () => { @@ -1568,11 +1565,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 +1582,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 +1601,8 @@ 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'); }); it('returns 404 when the screenshot file is missing', async () => { From 61cce14e4dca3459ce2011274b160e78bbecc3f3 Mon Sep 17 00:00:00 2001 From: Shivanshu07 Date: Tue, 23 Jun 2026 22:57:06 +0530 Subject: [PATCH 2/2] 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) --- packages/core/src/api.js | 19 +++++++++++++++---- packages/core/test/api.test.js | 23 +++++++++++++++++++++++ 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/packages/core/src/api.js b/packages/core/src/api.js index 2d5000ce0..f6c9bfa25 100644 --- a/packages/core/src/api.js +++ b/packages/core/src/api.js @@ -389,8 +389,13 @@ export function createPercyServer(percy, port) { // 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. - const snapshotPromise = new Promise((resolve, reject) => percy.upload(req.body, { resolve, reject }, 'app')); + // 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) => { + percy.upload(req.body, { resolve, reject }, 'app').catch(reject); + }); data = await handleSyncJob(snapshotPromise, percy, 'comparison'); } else { let upload = percy.upload(req.body, null, 'app'); @@ -905,7 +910,10 @@ export function createPercyServer(percy, port) { if (percy.syncMode(payload)) { // 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. - const snapshotPromise = new Promise((resolve, reject) => percy.upload(payload, { resolve, reject }, 'app')); + // The .catch(reject) surfaces generator errors that bypass that callback. + const snapshotPromise = new Promise((resolve, reject) => { + percy.upload(payload, { resolve, reject }, 'app').catch(reject); + }); data = await handleSyncJob(snapshotPromise, percy, 'comparison'); return res.json(200, { success: true, data }); } @@ -940,7 +948,10 @@ export function createPercyServer(percy, port) { if (percy.syncMode(comparisonData)) { // 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. - const snapshotPromise = new Promise((resolve, reject) => percy.upload(comparisonData, { resolve, reject }, 'automate')); + // The .catch(reject) surfaces generator errors that bypass that callback. + const snapshotPromise = new Promise((resolve, reject) => { + percy.upload(comparisonData, { resolve, reject }, 'automate').catch(reject); + }); data = await handleSyncJob(snapshotPromise, percy, 'comparison'); } else { percy.upload(comparisonData, null, 'automate'); diff --git a/packages/core/test/api.test.js b/packages/core/test/api.test.js index 953c60503..35b49aead 100644 --- a/packages/core/test/api.test.js +++ b/packages/core/test/api.test.js @@ -361,6 +361,25 @@ describe('API Server', () => { 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 () => { @@ -631,6 +650,8 @@ describe('API Server', () => { })).toBeResolvedTo(jasmine.objectContaining({ data: getSnapshotDetailsResponse })); 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 () => { @@ -1603,6 +1624,8 @@ describe('API Server', () => { 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 () => {