Repository navigation
feat(browser): one browser context per job, wiped between variants (v1.28.0) - #170
Conversation
…1.28.0 Each device variant of a job has always rendered in a browser context of its own, so the second variant re-fetches every script, stylesheet and API response the first already pulled. `variantContext.shared` (off by default) renders a job's variants in ONE context instead, so Chrome serves the second from the cache the first filled. Measured on a production storefront, second variant, with the on-disk resource cache already on and warm: same-origin network responses fell from 102 to 22 on a product page and 99 to 16 on a catalog page, and the on-disk cache was not cannibalised — the two caches hold different things, since one shared between unrelated renders can only keep cookieless GET script/stylesheet responses. Only the cache is shared. Between variants every cookie is deleted but `documentReuse.cookies.pin` and the navigation origin's storage is cleared, so what a variant inherits is exactly what it inherits today. That wipe is the feature: measured, an unwiped shared context handed the second variant 125 cookies instead of the 6 pinned ones and put both devices on one session id, one visitor id and one bot-manager token. A wipe that cannot be applied takes the sharing with it — that variant renders in a context of its own. Sample jobs never share, since their later variants exist to fetch cold. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a 'One context per job' feature (config.variantContext.shared) that allows device variants of a single job to share a browser context, optimizing performance by sharing Chrome's HTTP cache while wiping cookies and origin storage between variants to prevent state leakage. The changes include updates to browser management, worker rendering loops, configuration validation, and new tests. The review feedback suggests improving robustness by wrapping Puppeteer API calls (createCDPSession and cdp.detach) in try...catch blocks instead of relying solely on promise .catch() handlers, which prevents synchronous exceptions from escaping and potentially disrupting the render loop.
| const cdp = await page.createCDPSession().catch((err: unknown) => { | ||
| logger.warn({ err, url }, 'could not open a CDP session to clear storage between variants'); | ||
| return null; | ||
| }); | ||
| if (!cdp) return false; |
There was a problem hiding this comment.
When invoking Puppeteer APIs (such as createCDPSession), wrap the calls in a try...catch block rather than relying solely on promise .catch() handlers. Puppeteer can throw synchronous exceptions (e.g., if the page or session is already disposed), which a promise .catch() will not intercept.
let cdp;
try {
cdp = await page.createCDPSession();
} catch (err) {
logger.warn({ err, url }, 'could not open a CDP session to clear storage between variants');
return false;
}References
- When invoking Puppeteer APIs, wrap the calls in a try...catch block rather than relying solely on promise .catch() handlers to intercept synchronous exceptions.
There was a problem hiding this comment.
Applied in bb8e816, though not for the stated reason — worth recording so a later reader isn't misled.
page.createCDPSession() and CDPSession.detach() are both declared async in puppeteer 24 (cdp/Page.js:766, cdp/CDPSession.js:102), so neither can throw synchronously; an async function's throw is always a rejection, which .catch() does intercept. The original form was not leaking anything.
The change is still worth making for a different reason: the cookie wipe immediately above already uses try/catch, so the function now reads the same way throughout, and the detach catch carries a comment explaining why a failure there is safe to swallow (the session dies with its page, which is closed right after either way, and swallowing keeps it from masking the boolean verdict that decides whether the variant renders in this context).
| } finally { | ||
| await cdp.detach().catch(noop); | ||
| } |
There was a problem hiding this comment.
When invoking Puppeteer/CDP APIs (such as cdp.detach), wrap the calls in a try...catch block rather than relying solely on promise .catch() handlers. If cdp.detach() throws a synchronous exception (e.g., if the session is already closed), it will escape the finally block and propagate, potentially crashing the render loop.
} finally {
try {
await cdp.detach();
} catch {}
}References
- When invoking Puppeteer APIs, wrap the calls in a try...catch block rather than relying solely on promise .catch() handlers to intercept synchronous exceptions.
There was a problem hiding this comment.
Applied in bb8e816, though not for the stated reason — worth recording so a later reader isn't misled.
page.createCDPSession() and CDPSession.detach() are both declared async in puppeteer 24 (cdp/Page.js:766, cdp/CDPSession.js:102), so neither can throw synchronously; an async function's throw is always a rejection, which .catch() does intercept. The original form was not leaking anything.
The change is still worth making for a different reason: the cookie wipe immediately above already uses try/catch, so the function now reads the same way throughout, and the detach catch carries a comment explaining why a failure there is safe to swallow (the session dies with its page, which is closed right after either way, and swallowing keeps it from masking the boolean verdict that decides whether the variant renders in this context).
…ie block Review feedback on #170. The stated premise does not hold — `createCDPSession` and `detach` are both `async`, so neither can throw synchronously and the `.catch()` form was already sufficient — but the try/catch reads consistently with the cookie wipe directly above it, and the detach comment now says why a failure there is safe to swallow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
What
A job's device variants have always rendered in a browser context each — own cookie jar, own origin storage, own copy of Chrome's HTTP cache. The last of those is waste: both variants load the same page seconds apart, so the second re-fetches everything the first already pulled.
variantContext.shared(new, off by default) renders a job's variants in one context, so Chrome serves the second from the cache the first filled. Only the cache is shared — between variants every cookie is deleted butdocumentReuse.cookies.pin, and the navigation origin'slocal_storage,indexeddb,service_workersandcache_storageare cleared. What a variant inherits is exactly what it inherits today.Why the wipe is the feature
Sharing a context without wiping was the obvious version, and it is the wrong one. Measured on a live product page, the second variant inherited 125 cookies instead of the 6 pinned, and the two devices ended on one
X-SESSIONID, one visitor id and one bot-manager token — where rendering them apart gives each its own. Two devices sharing an identity is precisely whatcookies.pinexists to prevent, and nothing downstream would notice.Measured live, through the shipped code path
All three page types were driven through
ManagedBrowser.createContext/getPage(context)/resetForNextVariantexactly asWorker.renderVariantdoes, against production with the bypass token — not a reimplementation. Second variant, production config, on-disk resource cache already on and warm:The first variant is unchanged throughout (product 102 → 103, catalog 99 → 98, home 69 → 70 — run-to-run noise). The on-disk resource cache is not cannibalised (70 hits before, 66 after): the two caches hold different things, since one shared between unrelated renders can only keep cookieless GET script/stylesheet responses while a per-job one is thrown away with its job.
Fidelity, mobile (the replayed variant), judged against each page's own churn floor. Offers, outcome, status and indexability identical in every arm:
Render time did not move reliably in either direction, which is expected — settle is CPU-bound. The win is fetches, not latency, and not throughput. Same-origin XHR went 22 → 21, so dynamic API calls are essentially unchanged: what this removes is repeat static-asset fetches at the edge. Also measured on this site: no service workers and no IndexedDB on any page or device.
Does it cost CPU? (asked because fixing the resource-cache replay bug did)
That fix made resources that were being silently dropped load again — new work, and the tell was output growing ~49 KB. This is a different mechanism: the same resources load in both arms, only their source changes. Measured anyway, mobile variant, two samples per arm:
No increase — task time trends flat-to-down (~8-10% on product and catalog), layout and style time consistently slightly lower. Two direct checks on the "requests finally run" mechanism: failed-request counts are unchanged, and output did not grow (bytes and divergence unchanged), which is the signal that was present in the replay incident and is absent here. A cache hit also skips transfer, decompression and — since V8 stores its code cache with the HTTP cache entry — script recompilation, which plausibly explains the small decrease.
Two things the data does not settle, worth watching after enabling: DOM node counts on the home page ran ~10% higher in the shared arm (within churn on divergence, but only two samples), and memory is reasoned rather than measured — an incognito context's HTTP cache is in-memory, so variant 1's assets stay resident until the context closes instead of being freed at its page close. Peak should be comparable (one copy held across two renders vs two copies held serially), but on a memory-pressured fleet that is the first number to watch.
Safety properties
variantContextResetFailures. The saving is only worth having while it costs nothing.documentReuse.enabled— an adaptive site cannot reuse a document but still fetches the same assets twice.variantContextis unscopable in overrides, for the same reasondocumentReuseis.Review notes
npm ci,npm run lint,npm run format:check,node --test→ 262/262 pass (11 new).🤖 Generated with Claude Code