diff --git a/node-src/lib/upload.ts b/node-src/lib/upload.ts index 25ad46341..dc42d20c8 100644 --- a/node-src/lib/upload.ts +++ b/node-src/lib/upload.ts @@ -138,7 +138,12 @@ export async function uploadBuild( return { ...file, ...target }; }) ); - zipTarget = uploadBuild.info.zipTarget; + + // Use the last received zipTarget, as it will have the largest allowed size. + // If all files in the batch are copied rather than uploaded, we won't receive a zipTarget. + if (uploadBuild.info.zipTarget) { + zipTarget = uploadBuild.info.zipTarget; + } } if (!targets.length) { diff --git a/node-src/lib/waitForSentinel.ts b/node-src/lib/waitForSentinel.ts index 2c7df6ca1..ef2d5f2d6 100644 --- a/node-src/lib/waitForSentinel.ts +++ b/node-src/lib/waitForSentinel.ts @@ -7,10 +7,10 @@ import { Context } from '../types'; // completed successfully and 'ERROR' if an error occurred. const SENTINEL_SUCCESS_VALUE = 'OK'; -export async function waitForSentinel(ctx: Context, url: string) { +export async function waitForSentinel(ctx: Context, { name, url }: { name: string; url: string }) { const { experimental_abortSignal: signal } = ctx.options; - ctx.log.debug(`Waiting for sentinel file to appear at ${url}`); + ctx.log.debug(`Waiting for '${name}' sentinel file to appear at ${url}`); return retry( async (bail) => { @@ -26,15 +26,15 @@ export async function waitForSentinel(ctx: Context, url: string) { if (response.status === 403) { return bail(new Error('Provided signature expired.')); } - throw new Error('Sentinel file not present.'); + throw new Error(`Sentinel file '${name}' not present.`); } const result = await res.text(); if (result !== SENTINEL_SUCCESS_VALUE) { - ctx.log.debug(`Sentinel file not OK, got ${result}`); - return bail(new Error('Sentinel file error.')); + ctx.log.debug(`Sentinel file '${name}' not OK, got '${result}'.`); + return bail(new Error(`Sentinel file '${name}' not OK.`)); } - ctx.log.debug(`Sentinel file OK.`); + ctx.log.debug(`Sentinel file '${name}' OK.`); }, { retries: 185, // 3 minutes and some change (matches the lambda timeout with some extra buffer) diff --git a/node-src/tasks/upload.test.ts b/node-src/tasks/upload.test.ts index e03698cef..5916c5cdd 100644 --- a/node-src/tasks/upload.test.ts +++ b/node-src/tasks/upload.test.ts @@ -6,7 +6,13 @@ import { default as compress } from '../lib/compress'; import { getDependentStoryFiles as getDepStoryFiles } from '../lib/getDependentStoryFiles'; import { findChangedDependencies as findChangedDep } from '../lib/findChangedDependencies'; import { findChangedPackageFiles as findChangedPkg } from '../lib/findChangedPackageFiles'; -import { calculateFileHashes, validateFiles, traceChangedFiles, uploadStorybook } from './upload'; +import { + calculateFileHashes, + validateFiles, + traceChangedFiles, + uploadStorybook, + waitForSentinels, +} from './upload'; vi.mock('form-data'); vi.mock('fs'); @@ -335,92 +341,6 @@ describe('uploadStorybook', () => { expect(ctx.uploadedFiles).toBe(2); }); - it('batches calls to uploadBuild mutation', async () => { - const client = { runQuery: vi.fn() }; - client.runQuery.mockReturnValueOnce({ - uploadBuild: { - info: { - sentinelUrls: [], - targets: Array.from({ length: 1000 }, (_, i) => ({ - contentType: 'application/javascript', - filePath: `${i}.js`, - formAction: `https://s3.amazonaws.com/presigned?${i}.js`, - formFields: {}, - })), - }, - userErrors: [], - }, - }); - client.runQuery.mockReturnValueOnce({ - uploadBuild: { - info: { - sentinelUrls: [], - targets: [ - { - contentType: 'application/javascript', - filePath: `1000.js`, - formAction: `https://s3.amazonaws.com/presigned?1000.js`, - formFields: {}, - }, - ], - }, - userErrors: [], - }, - }); - - createReadStreamMock.mockReturnValue({ pipe: vi.fn() } as any); - http.fetch.mockReturnValue({ ok: true }); - - const fileInfo = { - lengths: Array.from({ length: 1001 }, (_, i) => ({ knownAs: `${i}.js`, contentLength: i })), - paths: Array.from({ length: 1001 }, (_, i) => `${i}.js`), - total: Array.from({ length: 1001 }, (_, i) => i).reduce((a, v) => a + v), - }; - const ctx = { - client, - env, - log, - http, - sourceDir: '/static/', - options: {}, - fileInfo, - announcedBuild: { id: '1' }, - } as any; - await uploadStorybook(ctx, {} as any); - - expect(client.runQuery).toHaveBeenCalledTimes(2); - expect(client.runQuery).toHaveBeenCalledWith(expect.stringMatching(/UploadBuildMutation/), { - buildId: '1', - files: Array.from({ length: 1000 }, (_, i) => ({ - contentLength: i, - filePath: `${i}.js`, - })), - }); - expect(client.runQuery).toHaveBeenCalledWith(expect.stringMatching(/UploadBuildMutation/), { - buildId: '1', - files: [{ contentLength: 1000, filePath: `1000.js` }], // 0-based index makes this file #1001 - }); - - // Empty files are not uploaded - expect(http.fetch).not.toHaveBeenCalledWith( - 'https://s3.amazonaws.com/presigned?0.js', - expect.objectContaining({ body: expect.any(FormData), method: 'POST' }), - expect.objectContaining({ retries: 0 }) - ); - expect(http.fetch).toHaveBeenCalledWith( - 'https://s3.amazonaws.com/presigned?999.js', - expect.objectContaining({ body: expect.any(FormData), method: 'POST' }), - expect.objectContaining({ retries: 0 }) - ); - expect(http.fetch).toHaveBeenCalledWith( - 'https://s3.amazonaws.com/presigned?1000.js', - expect.objectContaining({ body: expect.any(FormData), method: 'POST' }), - expect.objectContaining({ retries: 0 }) - ); - expect(ctx.uploadedBytes).toBe(500500); - expect(ctx.uploadedFiles).toBe(1000); - }); - it('calls experimental_onTaskProgress with progress', async () => { const client = { runQuery: vi.fn() }; client.runQuery.mockReturnValue({ @@ -733,5 +653,128 @@ describe('uploadStorybook', () => { expect(ctx.uploadedBytes).toBe(80); expect(ctx.uploadedFiles).toBe(2); }); + + it('handles zipTarget being undefined', async () => { + const client = { runQuery: vi.fn() }; + client.runQuery.mockReturnValueOnce({ + uploadBuild: { + info: { + sentinelUrls: [], + targets: Array.from({ length: 1000 }, (_, i) => ({ + contentType: 'application/javascript', + filePath: `${i}.js`, + formAction: `https://s3.amazonaws.com/presigned?${i}.js`, + formFields: {}, + })), + zipTarget: { + contentType: 'application/zip', + filePath: 'storybook.zip', + formAction: 'https://s3.amazonaws.com/presigned?storybook.zip', + formFields: {}, + }, + }, + userErrors: [], + }, + }); + client.runQuery.mockReturnValueOnce({ + uploadBuild: { + info: { + sentinelUrls: [], + targets: [ + { + contentType: 'application/javascript', + filePath: `1000.js`, + formAction: `https://s3.amazonaws.com/presigned?1000.js`, + formFields: {}, + }, + ], + zipTarget: undefined, + }, + userErrors: [], + }, + }); + + makeZipFile.mockReturnValue(Promise.resolve({ path: 'storybook.zip', size: 80 })); + createReadStreamMock.mockReturnValue({ pipe: vi.fn() } as any); + http.fetch.mockReturnValue({ ok: true, text: () => Promise.resolve('OK') }); + + const fileInfo = { + lengths: Array.from({ length: 1001 }, (_, i) => ({ knownAs: `${i}.js`, contentLength: i })), + paths: Array.from({ length: 1001 }, (_, i) => `${i}.js`), + total: Array.from({ length: 1001 }, (_, i) => i).reduce((a, v) => a + v), + }; + const ctx = { + client, + env, + log, + http, + sourceDir: '/static/', + options: { zip: true }, + fileInfo, + announcedBuild: { id: '1' }, + } as any; + await uploadStorybook(ctx, {} as any); + + expect(http.fetch).toHaveBeenCalledWith( + 'https://s3.amazonaws.com/presigned?storybook.zip', + expect.objectContaining({ body: expect.any(FormData), method: 'POST' }), + { retries: 0 } + ); + expect(http.fetch).not.toHaveBeenCalledWith( + 'https://s3.amazonaws.com/presigned?0.js', + expect.anything(), + expect.anything() + ); + expect(http.fetch).not.toHaveBeenCalledWith( + 'https://s3.amazonaws.com/presigned?1000.js', + expect.anything(), + expect.anything() + ); + }); + }); +}); + +describe('waitForSentinels', () => { + it('dedupes sentinel URLs before awaiting them', async () => { + const client = { runQuery: vi.fn() }; + http.fetch.mockReturnValue({ ok: true, text: () => Promise.resolve('OK') }); + + const sentinelUrls = [ + 'https://chromatic-builds.s3.us-west-2.amazonaws.com/59c59bd0183bd100364e1d57-pbxunskvpo/.chromatic/files-copied.txt?foo', + 'https://chromatic-builds.s3.us-west-2.amazonaws.com/59c59bd0183bd100364e1d57-pbxunskvpo/.chromatic/zip-unpacked.txt?bar', + 'https://chromatic-builds.s3.us-west-2.amazonaws.com/59c59bd0183bd100364e1d57-pbxunskvpo/.chromatic/zip-unpacked.txt?baz', + 'https://chromatic-builds.s3.us-west-2.amazonaws.com/59c59bd0183bd100364e1d57-pbxunskvpo/.chromatic/files-copied.txt?baz', + ]; + const ctx = { + client, + env, + log, + http, + options: {}, + sentinelUrls, + } as any; + await waitForSentinels(ctx, {} as any); + + // Last one wins + expect(http.fetch).not.toHaveBeenCalledWith( + sentinelUrls[0], + expect.any(Object), + expect.any(Object) + ); + expect(http.fetch).not.toHaveBeenCalledWith( + sentinelUrls[1], + expect.any(Object), + expect.any(Object) + ); + expect(http.fetch).toHaveBeenCalledWith( + sentinelUrls[2], + expect.any(Object), + expect.any(Object) + ); + expect(http.fetch).toHaveBeenCalledWith( + sentinelUrls[3], + expect.any(Object), + expect.any(Object) + ); }); }); diff --git a/node-src/tasks/upload.ts b/node-src/tasks/upload.ts index 254fba90f..50c49df18 100644 --- a/node-src/tasks/upload.ts +++ b/node-src/tasks/upload.ts @@ -232,7 +232,15 @@ export const waitForSentinels = async (ctx: Context, task: Task) => { if (ctx.skip || !ctx.sentinelUrls?.length) return; transitionTo(finalizing)(ctx, task); - await Promise.all(ctx.sentinelUrls.map((url) => waitForSentinel(ctx, url))); + // Dedupe sentinels, ignoring query params + const sentinels = Object.fromEntries( + ctx.sentinelUrls.map((url) => { + const { host, pathname } = new URL(url); + return [host + pathname, { name: pathname.split('/').at(-1), url }]; + }) + ); + + await Promise.all(Object.values(sentinels).map((sentinel) => waitForSentinel(ctx, sentinel))); }; export default createTask({