fix: honor session cookies across HTTP client request paths - #2104
fix: honor session cookies across HTTP client request paths#2104Ayush7614 wants to merge 1 commit into
Conversation
Httpx send_request/stream now send outbound session cookies like crawl. Impit respects persist_cookies_per_session, keys the client cache by jar identity, and closes cached clients on cleanup. Httpx fingerprint headers are generated from a single profile so Accept and User-Agent stay consistent.
There was a problem hiding this comment.
Pull request overview
This PR fixes inconsistent cookie handling across HTTP client request paths so that session cookies are reliably sent (and optionally persisted) whether requests go through crawler navigation (crawl) or handler-level calls (send_request/stream). It also makes Httpx’s fingerprint-derived headers internally consistent by sourcing Accept, Accept-Language, and User-Agent from a single generated fingerprint profile.
Changes:
- Httpx:
send_request/streamnow attach outbound session cookies via the shared request-building path (matchingcrawlbehavior). - Impit: implements
persist_cookies_per_sessionsemantics, caches clients by(proxy, cookie-jar identity), and adds client closing on cleanup/eviction. - Tests: adds coverage for cookie sending/persistence toggles, single-fingerprint headers, and Impit cleanup cache reset.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/unit/http_clients/test_http_clients.py | Adds unit coverage for session cookie sending in send_request/stream, persistence on/off behavior, single-fingerprint headers, and Impit cache cleanup. |
| src/crawlee/http_clients/_impit.py | Honors cookie persistence flag by using a resolved jar (shared vs copy), introduces client caching keyed by proxy + cookie jar identity, and closes cached clients on cleanup/eviction. |
| src/crawlee/http_clients/_httpx.py | Ensures handler-level requests attach session cookies and derives Accept/Accept-Language/User-Agent from one fingerprint profile. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Ephemeral jars (persist_cookies_per_session=False) must not pollute / thrash the LRU cache. | ||
| cacheable = cookie_jar is None or self._persist_cookies_per_session | ||
| cache_key = self._make_cache_key(proxy_url, cookie_jar) if cacheable else None |
Mantisus
left a comment
Mantisus
left a comment
There was a problem hiding this comment.
Thank you for your contributions!
When persist_cookies_per_session is False, _resolve_cookie_jar builds a fresh jar for every request, and since the cache key includes the jar identity, that means a new AsyncClient per request. Consider building the Cookie header directly instead of passing a CookieJar to AsyncClient, then the client can stay cached and shared.
For example:
import urllib.request
from http.cookiejar import CookieJar
def get_cookie_header(jar: CookieJar, url: str, headers: HttpHeaders | None = None) -> str:
request = urllib.request.Request(url, headers=dict(headers) if headers else {})
jar.add_cookie_header(request)
return request.get_header('Cookie', '')| for cookie in session.cookies.jar: | ||
| jar.set_cookie(deepcopy(cookie)) | ||
| return jar |
There was a problem hiding this comment.
SessionCookies already does exactly this, so we can use it here.
from crawlee.sessions import SessionCookies
return SessionCookies(session.cookies).jarThe deepcopy isn't needed. CookieJar.set_cookie replaces the entry in its own dict rather than mutating the stored Cookie, and extract_cookies always builds new objects. A fresh jar is enough to isolate the session.
| if cache_key is not None: | ||
| # Close the client being evicted when the LRU is full, to avoid leaking connections. | ||
| if len(self._client_cache) >= self._client_cache.maxsize: | ||
| _evicted_key, evicted_entry = next(iter(self._client_cache.items())) |
There was a problem hiding this comment.
If I'm not mistaken, you need to use popitem with cachetools.LRUCache
| # Close the client being evicted when the LRU is full, to avoid leaking connections. | ||
| if len(self._client_cache) >= self._client_cache.maxsize: | ||
| _evicted_key, evicted_entry = next(iter(self._client_cache.items())) | ||
| asyncio.get_running_loop().create_task(self._close_client(evicted_entry['client'])) |
There was a problem hiding this comment.
The task may be garbage collected before it completes.
| async def _close_client(self, client: AsyncClient) -> None: | ||
| # Impit exposes cleanup via the async context manager protocol. | ||
| result = client.__aexit__(None, None, None) | ||
| if hasattr(result, '__await__'): | ||
| await result # type: ignore[misc] |
There was a problem hiding this comment.
It's a good guard, but overall, impit should handle potential leaks well thanks to its Rust implementation
Summary
send_request/streamnow send outbound session cookies the same waycrawlalready did (via_build_request).persist_cookies_per_session(was accepted but ignored), caches clients by(proxy, cookie-jar identity), and closes cached clients on cleanup/eviction.Accept/Accept-Language/User-Agentare generated from a single fingerprint profile instead of two independentgenerate()calls that could mix browser profiles.Why
context.send_request()silently dropped session cookies under Httpx, breaking auth that worked for navigation. Impit's documentedpersist_cookies_per_session=Falsenever took effect because the session jar was always attached and mutated in place.Test plan
tests/unit/http_clients/test_http_clients.py— full file (82 passed)