Skip to content

Commit

Permalink
Merge pull request #892 from chromaui/ghengeveld/ap-4055-zip-upload-c…
Browse files Browse the repository at this point in the history 
…an-fail-when-uploading-multiple-batches-and

Fix potential zip upload error when deduping files on a very large Storybook
  • Loading branch information
@ghengeveld
ghengeveld authored Jan 17, 2024
2 parents 10c2ca0 + 4d8a0b9 commit 4b05f7e
Show file tree
Hide file tree
Showing 4 changed files with 151 additions and 95 deletions.
7 changes: 6 additions & 1 deletion node-src/lib/upload.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
12 changes: 6 additions & 6 deletions node-src/lib/waitForSentinel.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand All @@ -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)
Expand Down
217 changes: 130 additions & 87 deletions node-src/tasks/upload.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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({
Expand Down Expand Up @@ -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)
);
});
});
10 changes: 9 additions & 1 deletion node-src/tasks/upload.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down

0 comments on commit 4b05f7e

Please sign in to comment.