Skip to content

fix(spiders): preserve cookies for browser-engine responses in cache - #379

Merged
D4Vinci merged 2 commits into
D4Vinci:devfrom
amitvijapur:fix/cache-drops-browser-cookies
Jul 25, 2026
Merged

fix(spiders): preserve cookies for browser-engine responses in cache#379
D4Vinci merged 2 commits into
D4Vinci:devfrom
amitvijapur:fix/cache-drops-browser-cookies

Conversation

@amitvijapur

Copy link
Copy Markdown
Contributor

Fixes #376

Root cause

Response.cookies has two possible shapes (scrapling/engines/toolbelt/custom.py:48):

  • Static engine: a flat dict (convertor.py:317)
  • Browser engines, sync and async (Playwright): a tuple of full cookie dicts (convertor.py:139, convertor.py:287)

ResponseCacheManager.put() only handled the dict case:

"cookies": dict(response.cookies) if isinstance(response.cookies, dict) else {},

Any browser-engine response has tuple cookies, so isinstance(..., dict) is False and the else {} branch discards every cookie before it's ever written to the cache file. get() then rebuilds the Response with cookies={}, so a replayed browser-engine response has no cookies at all — this silently breaks anything downstream relying on cookies (sessions, auth, CSRF tokens).

Fix

Preserve whichever shape the cookies are in, instead of collapsing non-dict cookies to {}:

  • put(): serialize a tuple as a JSON array (list(response.cookies)) and a dict as a JSON object (dict(response.cookies)).
  • get(): JSON arrays deserialize back as list, so restore the tuple shape Response.__init__ expects; JSON objects already come back as dict.

This keeps the round-trip shape-preserving rather than normalizing everything to a flat dict, so code that depends on the tuple-of-full-cookie-dicts shape for browser-engine responses (domain, path, expiry, etc.) still gets that shape back after a cache replay.

Tests

Added two regression tests to tests/spiders/test_cache.py:

  • test_put_get_roundtrip_preserves_browser_engine_cookies — tuple-of-dicts cookies (the browser-engine shape) survive put()/get() unchanged. This fails on main/dev before the fix (assert {} == (...)) and passes after.
  • test_put_get_roundtrip_preserves_static_engine_cookies — flat-dict cookies (the static-engine shape) still round-trip correctly, confirming no regression for the existing working case.
tests/spiders/test_cache.py::TestResponseCacheManager::test_put_get_roundtrip_preserves_browser_engine_cookies PASSED
tests/spiders/test_cache.py::TestResponseCacheManager::test_put_get_roundtrip_preserves_static_engine_cookies PASSED

Full tests/spiders/test_cache.py suite (12 tests) passes. ruff check, ruff format --check, mypy, pyright, and bandit all pass clean on the changed file. Ran the broader non-browser test suite (pytest tests/ -k "not (DynamicFetcher or StealthyFetcher)", 738 passed) to confirm no regressions elsewhere — the only failures were pre-existing, unrelated tests failing due to missing local Playwright browser binaries.

Credit to @truongsontung for the initial diagnosis in the issue thread — I independently confirmed the same root cause and landed on a slightly different fix (shape-preserving round-trip rather than always flattening to a dict on read) to avoid changing the cookie shape browser-engine consumers see after a cache replay.

…4Vinci#376)

ResponseCacheManager.put() only recognized the flat `dict` shape that
static-engine responses use for `Response.cookies`. Browser engines
(Playwright) populate cookies as a `tuple` of full cookie dicts, so the
isinstance(dict) guard fell through to the `else {}` branch and silently
discarded every cookie before it ever reached the cache file. Replaying a
cached browser-engine response then rebuilt it with cookies={}.

Preserve whichever shape the cookies are in instead of collapsing anything
non-dict to {}: serialize a tuple as a JSON array and a dict as a JSON
object, then restore the tuple shape on read since JSON arrays deserialize
back as `list`.

Add regression tests covering both the browser-engine (tuple) and
static-engine (dict) cookie round-trip through put()/get().
@yetval

yetval commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Reviewed and verified this locally against dev. LGTM.

The root cause is as described, the new browser cookie test fails before the fix and passes after, and the full test suites, lint, and type checks are all clean. Edge cases the tests don't cover (empty tuple cookies, old cache files, float expiries) also behave.

Minor notes, none blocking: passing response.cookies straight to orjson would work too, but the explicit conversion makes the two shapes obvious, so keep it. A tagged schema or helper functions would be overengineering for a fix this narrow. The cookie type annotation doesn't match what browser cookies actually contain, but that's inherited from Response and belongs in a separate cleanup.

Shape-preserving is the right call over flattening, which would collide keys and lose data.

@amitvijapur

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough verification — especially exercising the edge cases beyond the tests (empty tuple cookies, old cache files, float expiries). Agreed on all three notes: keeping the explicit conversion for readability, no schema machinery for a fix this narrow, and the Response.cookies annotation being a separate cleanup. If it'd be useful, happy to pick that annotation cleanup up in a follow-up PR.

@D4Vinci
D4Vinci merged commit b5c8963 into D4Vinci:dev Jul 25, 2026
5 checks passed
@D4Vinci

D4Vinci commented Jul 25, 2026

Copy link
Copy Markdown
Owner

LGTM, thanks for solving this!

@D4Vinci D4Vinci mentioned this pull request Jul 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ResponseCacheManager drops all cookies for browser-engine responses (cache replay loses cookies)

3 participants