Skip to content

Commit c4d58d9

Browse files
authored
fix(browser-pool): preserve caller's AbortContext across p-limit queue (#3673)
`BrowserPool.newPage()` serializes calls through `pLimit(1)`. `p-limit` resumes queued callbacks in the previous task's AsyncLocalStorage context, so a queued `newPage` call can inherit a sibling's aborted `cancelTask` and its first `tryCancel()` throws `InternalTimeoutError` pre-emptively. This fix explicitly passes the correct `AsyncLocalStorage` context into the `pLimit` arrow function to ensure task isolation. Closes #3670
1 parent eef94d4 commit c4d58d9

3 files changed

Lines changed: 85 additions & 7 deletions

File tree

packages/browser-pool/src/browser-pool.ts

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import { AsyncResource } from 'node:async_hooks';
2+
13
import type { TieredProxy } from '@crawlee/core';
24
import type { BrowserFingerprintWithHeaders } from 'fingerprint-generator';
35
import { FingerprintGenerator } from 'fingerprint-generator';
@@ -445,16 +447,25 @@ export class BrowserPool<
445447
throw new Error('Provided browserPlugin is not one of the plugins used by BrowserPool.');
446448
}
447449

450+
// Bind the limiter callback to the current async-hooks context. p-limit
451+
// otherwise resumes queued callbacks in the previous task's
452+
// AsyncLocalStorage context, leaking aborted cancelTasks across unrelated
453+
// requests (https://github.com/apify/crawlee/issues/3670). Mirrors the
454+
// fix p-limit landed upstream in v5 (sindresorhus/p-limit#71); v5 is an
455+
// ESM-only rewrite, so we can't bump it in Crawlee v3.
456+
// TODO(crawlee@v4): bump p-limit to v5 and drop this AsyncResource.bind wrapper.
448457
// Limiter is necessary - https://github.com/apify/crawlee/issues/1126
449-
return this.limiter(async () => {
450-
let browserController = this._pickBrowserWithFreeCapacity(browserPlugin, { proxyTier, proxyUrl });
458+
return this.limiter(
459+
AsyncResource.bind(async () => {
460+
let browserController = this._pickBrowserWithFreeCapacity(browserPlugin, { proxyTier, proxyUrl });
451461

452-
if (!browserController)
453-
browserController = await this._launchBrowser(id, { browserPlugin, proxyTier, proxyUrl });
454-
tryCancel();
462+
if (!browserController)
463+
browserController = await this._launchBrowser(id, { browserPlugin, proxyTier, proxyUrl });
464+
tryCancel();
455465

456-
return await this._createPageForBrowser(id, browserController, pageOptions, proxyUrl);
457-
});
466+
return await this._createPageForBrowser(id, browserController, pageOptions, proxyUrl);
467+
}),
468+
);
458469
}
459470

460471
/**

test/browser-pool/browser-pool.test.ts

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,38 @@ describe.each([
123123
expect(page.close).toBeDefined();
124124
});
125125

126+
// https://github.com/apify/crawlee/issues/3670
127+
test('should not leak aborted cancelTask between concurrent newPage calls', async () => {
128+
const previousTimeout = browserPool.operationTimeoutMillis;
129+
browserPool.operationTimeoutMillis = 1;
130+
131+
// Each newPage call is wrapped in its own outer addTimeoutToPromise,
132+
// matching how BasicCrawler wraps _runRequestHandler. Without the fix,
133+
// queued limiter callbacks inherit the previous task's aborted
134+
// cancelTask via AsyncLocalStorage propagation through p-limit, causing
135+
// their first tryCancel() to throw InternalTimeoutError pre-emptively;
136+
// that error then gets silently swallowed by the outer wrap.
137+
const results = await Promise.allSettled(
138+
Array.from({ length: 5 }, () =>
139+
addTimeoutToPromise(async () => browserPool.newPage(), 60_000, 'outer timed out'),
140+
),
141+
);
142+
143+
browserPool.operationTimeoutMillis = previousTimeout;
144+
browserPool.retireAllBrowsers();
145+
146+
// All calls must reject — none should silently resolve with undefined.
147+
expect(results.every((r) => r.status === 'rejected')).toBe(true);
148+
149+
// Each rejection should reflect this call's own newPage timeout, not
150+
// a leaked "canceled due to a timeout" from a sibling's aborted context.
151+
for (const r of results) {
152+
expect(r.status === 'rejected' && (r.reason as Error).message).toMatch(
153+
/browserController\.newPage\(\) (failed|timed out)/,
154+
);
155+
}
156+
});
157+
126158
// TODO: this test is very flaky in the CI
127159
test.skip('should allow early aborting in case of outer timeout', async () => {
128160
const timeout = browserPool.operationTimeoutMillis;

test/core/crawlers/playwright_crawler.test.ts

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,41 @@ describe('PlaywrightCrawler', () => {
141141
});
142142
});
143143

144+
// https://github.com/apify/crawlee/issues/3670
145+
test('should not silently drop requests when BrowserPool.newPage() times out', async () => {
146+
const success: string[] = [];
147+
const failure: string[] = [];
148+
149+
const crawler = new PlaywrightCrawler({
150+
maxRequestRetries: 0,
151+
browserPoolOptions: {
152+
operationTimeoutSecs: 0.001,
153+
},
154+
requestHandler: async ({ request }) => {
155+
success.push(request.url);
156+
},
157+
failedRequestHandler: async ({ request }) => {
158+
failure.push(request.url);
159+
},
160+
});
161+
162+
const urls = [
163+
`http://${HOSTNAME}:${port}/?q=1`,
164+
`http://${HOSTNAME}:${port}/?q=2`,
165+
`http://${HOSTNAME}:${port}/?q=3`,
166+
`http://${HOSTNAME}:${port}/?q=4`,
167+
`http://${HOSTNAME}:${port}/?q=5`,
168+
];
169+
170+
const stats = await crawler.run(urls);
171+
172+
// Every request must be accounted for by either requestHandler or failedRequestHandler.
173+
expect(success.length + failure.length).toBe(urls.length);
174+
// With operationTimeoutSecs=0.001, no request can actually succeed, so every one must fail.
175+
expect(stats.requestsFinished).toBe(0);
176+
expect(stats.requestsFailed).toBe(urls.length);
177+
});
178+
144179
test('should override goto timeout with navigationTimeoutSecs', async () => {
145180
const timeoutSecs = 10;
146181
let options: PlaywrightGotoOptions;

0 commit comments

Comments
 (0)