fix(docker): serve a hooked request from its own browser, not the shared pool - #2249
Draft
mvletter wants to merge 1 commit into
Draft
fix(docker): serve a hooked request from its own browser, not the shared pool#2249mvletter wants to merge 1 commit into
mvletter wants to merge 1 commit into
Conversation
…red pool
A declarative hook changes the browser context it runs in, and the change
outlives the request. `add_cookies` installs cookies into the context; the
context belongs to a crawler handed out by `crawler_pool.get_crawler()`, which
is shared -- the permanent one for the default browser config, in practice
almost every request. `release_crawler()` only decrements `active_requests`, so
the next request inherits whatever the last one left behind.
Measured on unmodified 0.9.3 with `CRAWL4AI_HOOKS_ENABLED=true`, four
sequential requests to a URL that echoes the cookies it received:
1. no hooks -> no cookie (correct)
2. add_cookies hook -> cookie present (correct)
3. no hooks -> cookie from 2 (wrong)
4. no hooks -> cookie from 2 (wrong)
Sequential, so it does not need concurrency to happen.
Scoping the hook per request does not fix it, which is worth stating because
it is the obvious first attempt: with the hook resolved from a ContextVar set
only on the requesting task, request 2's hook set was verifiably empty and it
still came back carrying request 1's cookie. The cookie is in the context, not
in the hook.
So a request that brings hooks now gets a browser of its own, disposed when it
finishes. This is the same shape the PDF branch already used, and
`_dispose_crawler()` now reads an explicit `pooled` marker instead of inferring
"not pooled" from the strategy type -- which only ever recognised PDF. The
non-streaming handler's inline close/release branch is replaced by that same
helper, so both handlers dispose crawlers through one path.
Requests without hooks are untouched: same pool, same counters.
Tests import the real `crawler_pool` rather than a copy of its logic, so they
notice if the module changes underneath them.
mvletter
marked this pull request as draft
September 11, 2026 10:53
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A declarative hook changes the browser context it runs in, and the change outlives the request.
add_cookiesinstalls cookies into the context; the context belongs to a crawler handed out bycrawler_pool.get_crawler(), which is shared — the permanent one for the default browser config, in practice almost every request.release_crawler()only decrementsactive_requests, so the next request inherits whatever the last one left behind.Reproduction
Unmodified
unclecode/crawl4ai:0.9.3,CRAWL4AI_HOOKS_ENABLED=true, four sequential POSTs to/crawlagainst a URL that echoes the cookies it received (https://httpbin.org/cookies):add_cookieshookSequential, so it does not need concurrent requests to happen.
The same is visible without hooks at all: crawl
https://httpbin.org/cookies/set?probe=x, then crawlhttps://httpbin.org/cookiesas a separate request, and the cookie the site set during the first crawl is still there. Cookies stay scoped to their own domain, so this does not cross sites — but it does cross requests, and on a shared deployment that means it crosses callers.Why scoping the hook does not fix it
Worth stating, because it is the obvious first attempt and it looks like it works. I built it: one dispatcher per hook point on the strategy, resolving the actual hook from a
ContextVarset only on the requesting task. Instrumented, the scoping was correct — request 2's hook set was verifiably empty:And request 2 still came back carrying request 1's cookie. The cookie is in the context, not in the hook.
The change
A request that brings hooks gets a browser of its own, disposed when it finishes. That is the shape the PDF branch already used, so this mostly makes an existing idea explicit:
crawler_pool.get_unpooled_crawler()starts a crawler outside the pool, behind the same memory guardget_crawler()applies, and marks itpooled = False._dispose_crawler()reads that marker instead of inferring "not pooled" from the strategy type, which only ever recognisedPDFCrawlerStrategy.Requests without hooks are untouched: same pool, same counters, same lifetimes.
Cost is one browser start per hooked request. Hooks are opt-in and off by default, and callers that use them tend to batch many URLs into one request, so this is per batch rather than per page.
Tests
tests/docker/test_pool_release.pygains two cases. They import the realcrawler_poolrather than a copy of its logic, so they fail if the module changes underneath them — the existing cases in that file re-implementrelease_crawlerlocally ("mirrors the logic that will be added to ..."), which cannot catch a change in the code it is meant to cover.The four-request sequence above was re-run against a build of this branch: #3 and #4 come back clean, #2 still gets its cookie.