Skip to content

feat(core): upload the snapshot log to the logs endpoint, not as a resource (PPLT-6034) #2417

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

Merged
this-is-shivamsingh merged 7 commits into from
Sep 8, 2026
Merged

feat(core): upload the snapshot log to the logs endpoint, not as a resource (PPLT-6034) #2417

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6a164ef
chore: empty commit for percy build testing
this-is-shivamsingh May 20, 2026
b0a6575
chore: empty commit for percy build testing 2
this-is-shivamsingh May 20, 2026
f8fcfb1
chore: change Example Page url to bing.com for percy build test
this-is-shivamsingh May 20, 2026
dd06438
feat(core): upload the snapshot log to the logs endpoint, not as a re…
this-is-shivamsingh Sep 3, 2026
b947260
test(core): fix execute-snapshot resource indices; drop review-flagge…
this-is-shivamsingh Sep 3, 2026
427346f
test(core): exercise uploadSnapshotLog default meta param for 100% br…
this-is-shivamsingh Sep 3, 2026
62dbc5d
Merge master into ss/PPLT-6034-cli-snapshot-log-dedicated-endpoint
this-is-shivamsingh Sep 3, 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
15 changes: 15 additions & 0 deletions packages/client/src/client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -943,6 +943,21 @@ export class PercyClient {
}, { identifier: 'build.send_logs', ...meta });
}

async sendSnapshotLog(buildId, snapshotId, content, meta = {}) {
validateId('build', buildId);
validateId('snapshot', snapshotId);
this.log.debug('Sending Snapshot Log', meta);
return this.post('logs', {
data: {
content,
build_id: buildId,
reference_id: snapshotId,
service_name: 'snapshot',
base64encoded: true
}
}, { identifier: 'snapshot.send_log', ...meta });
}

async getErrorAnalysis(errors, meta = {}) {
const errorLogs = formatLogErrors(errors);
this.log.debug('Sending error logs for analysis', meta);
Expand Down
25 changes: 25 additions & 0 deletions packages/client/test/client.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2682,6 +2682,31 @@ describe('PercyClient', () => {
});
});

describe('#sendSnapshotLog', () => {
it('should send the snapshot log to the logs API with service_name snapshot', async () => {
await expectAsync(client.sendSnapshotLog(1234, 5678, 'abcd')).toBeResolved();

expect(api.requests['/logs']).toBeDefined();
expect(api.requests['/logs'][0].method).toBe('POST');
expect(api.requests['/logs'][0].body).toEqual({
data: {
content: 'abcd',
build_id: 1234,
reference_id: 5678,
service_name: 'snapshot',
base64encoded: true
}
});
});

it('validates the build and snapshot ids', async () => {
await expectAsync(client.sendSnapshotLog(null, 5678, 'abcd'))
.toBeRejectedWithError('Missing build ID');
await expectAsync(client.sendSnapshotLog(1234, null, 'abcd'))
.toBeRejectedWithError('Missing snapshot ID');
});
});

describe('#getErrorAnalysis', () => {
const sharedTest = (reqBody, expectedBody) => {
it('should send error logs to API', async () => {
Expand Down
8 changes: 0 additions & 8 deletions packages/core/src/discovery.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ import {
createResource,
createRootResource,
createPercyCSSResource,
createLogResource,
redactSecrets,
yieldAll,
snapshotLogName,
waitForTimeout,
Expand Down Expand Up @@ -221,12 +219,6 @@ function processSnapshotResources({ domSnapshot, resources, ...snapshot }) {
// For multi dom root resources are stored as array
resources = resources.flat();

// include associated snapshot logs matched by meta information. Redact
// secrets before egress — this per-snapshot log resource is a parallel
// egress path to sendBuildLogs and must scrub tokens/credentials too (CWE-532).
resources.push(createLogResource(redactSecrets(logger.snapshotLogs(snapshot.meta.snapshot))));
logger.evictSnapshot(snapshot.meta.snapshot);

if (process.env.PERCY_GZIP) {
const kept = [];
for (let index = 0; index < resources.length; index++) {
Expand Down
36 changes: 35 additions & 1 deletion packages/core/src/snapshot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import logger from '@percy/logger';
import PercyConfig from '@percy/config';
import micromatch from 'micromatch';
import Pako from 'pako';
import { configSchema } from './config.js';
import Queue from './queue.js';
import {
Expand All @@ -10,7 +11,9 @@ import {
snapshotLogName,
decodeAndEncodeURLWithLogging,
compareObjectTypes,
normalizeOptions
normalizeOptions,
base64encode,
redactSecrets
} from './utils.js';
import { JobData } from './wait-for-job.js';

Expand Down Expand Up @@ -393,6 +396,30 @@ async function runDoctorOnFailure(percy) {
}
}

// Uploads a single web snapshot's CLI log to the logs endpoint, keyed by the
// snapshot id, so it lands directly in the log bucket instead of travelling as a
// snapshot resource. Best-effort: a failure here must never fail the snapshot.
export async function uploadSnapshotLog(percy, buildId, snapshotId, meta = {}) {
try {
if (!buildId || !snapshotId) return;

// Redact secrets before egress (CWE-532) — this per-snapshot log is a
// parallel egress path to sendBuildLogs and must scrub tokens/credentials.
let logs = redactSecrets(logger.snapshotLogs(meta.snapshot));
if (!logs.length) return;

let content = base64encode(Pako.gzip(JSON.stringify(logs)));
await percy.client.sendSnapshotLog(buildId, snapshotId, content, meta);
percy.log.debug(`Snapshot log sent for ${meta.snapshot?.name}`, meta);
} catch (err) {
percy.log.debug(`Could not send the snapshot log for ${meta.snapshot?.name}`, meta);
percy.log.debug(err);
} finally {
// drop this snapshot's cached logs regardless of upload outcome
logger.evictSnapshot(meta.snapshot);
}
}

// Creates a snapshots queue that manages a Percy build and uploads snapshots.
export function createSnapshotsQueue(percy) {
let { concurrency } = percy.config.discovery;
Expand Down Expand Up @@ -506,6 +533,13 @@ export function createSnapshotsQueue(percy) {
let response = yield percy.client[send](build.id, snapshot);
if (percy.deferUploads) percy.log.info(`Snapshot uploaded: ${name}`, meta);

// upload the per-snapshot CLI log directly to the logs endpoint (web
// snapshots only; automate/app comparisons carry no snapshot log). For
// sendSnapshot, response.data.id is the snapshot id. Best-effort.
if (send === 'sendSnapshot') {
yield uploadSnapshotLog(percy, build.id, response.data.id, meta);
Comment on lines +539 to +540

@this-is-shivamsingh this-is-shivamsingh Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point, does the snapshot is ran, and all the logs are added in the logger ?

@this-is-shivamsingh this-is-shivamsingh Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This runs inside the snapshot upload task, right after sendSnapshot resolves — so asset discovery and snapshot processing are complete and every log entry for this snapshot (matched by meta.snapshot.name + testCase) is already in the logger; logger.query returns them. It's actually a more complete capture than the old approach, which built the log resource during discovery (earlier). Verified live: support:debug on a new-CLI snapshot downloaded the log with 47 real entries.

@this-is-shivamsingh this-is-shivamsingh Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] Snapshot log cache not evicted when the snapshot send fails

logger.evictSnapshot(meta.snapshot) now runs only inside uploadSnapshotLog's finally, which is reached only after let response = yield percy.client[send](build.id, snapshot) (line 533) succeeds. If that send throws, the generator jumps straight to the queue's 'error' handler and this line — and the finally — never run, so the cached log entries for that snapshot key are never evicted (and never sent). Previously evictSnapshot ran unconditionally during discovery, before any upload, so a failed send still evicted. This is a bounded-per-build resource leak on the failure path.

Suggestion: also call logger.evictSnapshot(meta.snapshot) in the queue's 'error' handler, or wrap the client[send] + evict in a try/finally in the task handler so eviction always happens. Add a test for the send-failure path.

Reviewer: stack-code-reviewer

}

// Pushing to syncQueue, that will check for
// snapshot processing status, and will resolve once done
if (snapshot.sync) {
Expand Down
6 changes: 0 additions & 6 deletions packages/core/src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,12 +491,6 @@ export function createPercyCSSResource(url, css) {
return createResource(href, css, 'text/css', { pathname });
}

// Creates a log resource object.
export function createLogResource(logs) {
let [url, content] = [`/percy.${Date.now()}.log`, JSON.stringify(logs)];
return createResource(url, content, 'text/plain', { log: true });
}

// Returns true or false if the provided object is a generator or not
export function isGenerator(subject) {
return typeof subject?.next === 'function' && (
Expand Down
106 changes: 25 additions & 81 deletions packages/core/test/discovery.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,28 @@ describe('Discovery', () => {
jasmine.DEFAULT_TIMEOUT_INTERVAL = originalTimeout;
});

it('uploads the snapshot log to the logs endpoint instead of as a resource', async () => {
await percy.snapshot({
name: 'test snapshot',
url: 'http://localhost:8000',
domSnapshot: testDOM
});

await percy.idle();

// the per-snapshot log is no longer one of the captured resources
expect(captured[0].map(r => r.attributes['resource-url']))
.not.toContain(jasmine.stringMatching(/\/percy\.\d+\.log$/));

// it is uploaded via POST /logs with service_name 'snapshot' keyed by snapshot id
let snapshotLogReq = (api.requests['/logs'] || [])
.find(r => r.body?.data?.service_name === 'snapshot');
expect(snapshotLogReq).toBeDefined();
expect(snapshotLogReq.body.data.reference_id).toEqual('4567');
expect(snapshotLogReq.body.data.build_id).toEqual('123');
expect(snapshotLogReq.body.data.base64encoded).toBe(true);
});

it('gathers resources for a snapshot in GZIP format', async () => {
process.env.PERCY_GZIP = true;

Expand All @@ -110,11 +132,6 @@ describe('Discovery', () => {
expect(paths).toContain('/img.gif');

expect(captured[0]).toEqual([
jasmine.objectContaining({
attributes: jasmine.objectContaining({
'resource-url': jasmine.stringMatching(/^\/percy\.\d+\.log$/)
})
}),
jasmine.objectContaining({
id: sha256hash(Pako.gzip(testDOM)),
attributes: jasmine.objectContaining({
Expand Down Expand Up @@ -152,11 +169,6 @@ describe('Discovery', () => {
expect(paths).toContain('/img.gif');

expect(captured[0]).toEqual([
jasmine.objectContaining({
attributes: jasmine.objectContaining({
'resource-url': jasmine.stringMatching(/^\/percy\.\d+\.log$/)
})
}),
jasmine.objectContaining({
id: sha256hash(testDOM),
attributes: jasmine.objectContaining({
Expand Down Expand Up @@ -417,11 +429,6 @@ describe('Discovery', () => {
expect(paths).toContain('/style.css');

expect(captured[0]).toEqual([
jasmine.objectContaining({
attributes: jasmine.objectContaining({
'resource-url': jasmine.stringMatching(/^\/percy\.\d+\.log$/)
})
}),
jasmine.objectContaining({
id: sha256hash(prefetchDOM),
attributes: jasmine.objectContaining({
Expand Down Expand Up @@ -671,11 +678,6 @@ describe('Discovery', () => {
await percy.idle();

expect(captured[0]).toEqual([
jasmine.objectContaining({
attributes: jasmine.objectContaining({
'resource-url': jasmine.stringMatching(/^\/percy\.\d+\.log$/)
})
}),
jasmine.objectContaining({
attributes: jasmine.objectContaining({
'resource-url': 'http://localhost:8000/'
Expand Down Expand Up @@ -706,11 +708,6 @@ describe('Discovery', () => {
await percy.idle();

expect(captured[0]).toEqual([
jasmine.objectContaining({
attributes: jasmine.objectContaining({
'resource-url': jasmine.stringMatching(/^\/percy\.\d+\.log$/)
})
}),
jasmine.objectContaining({
attributes: jasmine.objectContaining({
'resource-url': 'http://localhost:8000/'
Expand Down Expand Up @@ -740,11 +737,6 @@ describe('Discovery', () => {
await percy.idle();

expect(captured[0]).toEqual([
jasmine.objectContaining({
attributes: jasmine.objectContaining({
'resource-url': jasmine.stringMatching(/^\/percy\.\d+\.log$/)
})
}),
jasmine.objectContaining({
attributes: jasmine.objectContaining({
'resource-url': 'http://localhost:8000/'
Expand Down Expand Up @@ -775,11 +767,6 @@ describe('Discovery', () => {
await percy.idle();

expect(captured[0]).toEqual([
jasmine.objectContaining({
attributes: jasmine.objectContaining({
'resource-url': jasmine.stringMatching(/^\/percy\.\d+\.log$/)
})
}),
jasmine.objectContaining({
attributes: jasmine.objectContaining({
'resource-url': 'http://localhost:8000/'
Expand Down Expand Up @@ -817,11 +804,6 @@ describe('Discovery', () => {
await percy.idle();

expect(captured[0]).toEqual([
jasmine.objectContaining({
attributes: jasmine.objectContaining({
'resource-url': jasmine.stringMatching(/^\/percy\.\d+\.log$/)
})
}),
jasmine.objectContaining({
attributes: jasmine.objectContaining({
'resource-url': 'http://localhost:8000/'
Expand Down Expand Up @@ -881,11 +863,6 @@ describe('Discovery', () => {
await percy.idle();

expect(captured[0]).toEqual([
jasmine.objectContaining({
attributes: jasmine.objectContaining({
'resource-url': jasmine.stringMatching(/^\/percy\.\d+\.log$/)
})
}),
jasmine.objectContaining({
id: sha256hash(Pako.gzip(testDOM.replace('style.css', 'large.css'))),
attributes: jasmine.objectContaining({
Expand Down Expand Up @@ -926,11 +903,6 @@ describe('Discovery', () => {
await percy.idle();

expect(captured[0]).toEqual([
jasmine.objectContaining({
attributes: jasmine.objectContaining({
'resource-url': jasmine.stringMatching(/^\/percy\.\d+\.log$/)
})
}),
jasmine.objectContaining({
attributes: jasmine.objectContaining({
'resource-url': 'http://localhost:8000/'
Expand Down Expand Up @@ -968,11 +940,6 @@ describe('Discovery', () => {
await percy.idle();

expect(captured[0]).toEqual([
jasmine.objectContaining({
attributes: jasmine.objectContaining({
'resource-url': jasmine.stringMatching(/^\/percy\.\d+\.log$/)
})
}),
jasmine.objectContaining({
attributes: jasmine.objectContaining({
'resource-url': 'http://localhost:8000/'
Expand Down Expand Up @@ -1343,11 +1310,6 @@ describe('Discovery', () => {
await percy.idle();

expect(captured[0]).toEqual([
jasmine.objectContaining({
attributes: jasmine.objectContaining({
'resource-url': jasmine.stringMatching(/^\/percy\.\d+\.log$/)
})
}),
jasmine.objectContaining({
attributes: jasmine.objectContaining({
'resource-url': 'http://localhost:8000/',
Expand Down Expand Up @@ -1750,7 +1712,6 @@ describe('Discovery', () => {

expect(captured).toHaveSize(1);
expect(captured[0].map(r => r.attributes['resource-url'])).toEqual([
jasmine.stringMatching(/\/percy\.\d+\.log$/),
'http://localhost:8000/'
]);

Expand Down Expand Up @@ -2709,10 +2670,9 @@ describe('Discovery', () => {
expect(paths.sort()).toEqual(['/img.gif', '/style.css']);

// both snapshots' captured resources should match
// the first captured resource is the log file which is dynamic
expect(captured[0][0]).toEqual(captured[1][0]);
expect(captured[0][1]).toEqual(captured[1][1]);
expect(captured[0][2]).toEqual(captured[1][2]);
expect(captured[0][3]).toEqual(captured[1][3]);
});

it('does not cache resource requests when disabled', async () => {
Expand All @@ -2726,11 +2686,10 @@ describe('Discovery', () => {
let paths = server.requests.map(r => r[0]);
expect(paths.sort()).toEqual(['/img.gif', '/img.gif', '/style.css', '/style.css']);

// bot snapshots' captured resources should match
// the first captured resource is the log file which is dynamic
// both snapshots' captured resources should match
expect(captured[0][0]).toEqual(captured[1][0]);
expect(captured[0][1]).toEqual(captured[1][1]);
expect(captured[0][2]).toEqual(captured[1][2]);
expect(captured[0][3]).toEqual(captured[1][3]);
});

it('does not cache root resource', async () => {
Expand Down Expand Up @@ -3785,11 +3744,6 @@ describe('Discovery', () => {
expect(paths2).not.toContain('/img.gif');

expect(captured[0]).toEqual([
jasmine.objectContaining({
attributes: jasmine.objectContaining({
'resource-url': jasmine.stringMatching(/^\/percy\.\d+\.log$/)
})
}),
jasmine.objectContaining({
attributes: jasmine.objectContaining({
'resource-url': 'http://localhost:8000/'
Expand Down Expand Up @@ -3895,11 +3849,6 @@ describe('Discovery', () => {
]));

expect(captured[0]).toEqual([
jasmine.objectContaining({
attributes: jasmine.objectContaining({
'resource-url': jasmine.stringMatching(/^\/percy\.\d+\.log$/)
})
}),
jasmine.objectContaining({
attributes: jasmine.objectContaining({
'resource-url': 'http://localhost:8000/'
Expand Down Expand Up @@ -3936,11 +3885,6 @@ describe('Discovery', () => {
await percy.idle();

expect(captured[0]).toEqual([
jasmine.objectContaining({
attributes: jasmine.objectContaining({
'resource-url': jasmine.stringMatching(/^\/percy\.\d+\.log$/)
})
}),
jasmine.objectContaining({
attributes: jasmine.objectContaining({
'resource-url': 'http://localhost:8000/'
Expand Down
Loading
Loading