Skip to content

fix(cookie_manager): prevent duplicate cookies on reused request options#2572

Merged
AlexV525 merged 2 commits into
mainfrom
fix/2442-cookie-retry-dedup
Jul 21, 2026
Merged

fix(cookie_manager): prevent duplicate cookies on reused request options#2572
AlexV525 merged 2 commits into
mainfrom
fix/2442-cookie-retry-dedup

Conversation

@AlexV525

@AlexV525 AlexV525 commented Jul 20, 2026

Copy link
Copy Markdown
Member

Closes #2442.
Supersedes #2498.

Summary

Reusing the same RequestOptions instance sends it through CookieManager again with the Cookie header produced by the previous pass. Merging that generated header with the cookie jar again makes jar cookies accumulate on every pass.

This change keeps weak per-RequestOptions state containing the original source header and the last header generated by the manager. When the current header still exactly matches that generated value, loadCookies rebuilds it from the original caller input and the latest jar state.

Unlike the name-only filtering proposed in #2498, this preserves the established behavior where caller-provided and jar cookies with the same name can coexist. It also removes stale jar values after updates, deletion, or an origin change.

The state is intentionally scoped to the same CookieManager and RequestOptions instance reported in #2442. A copied request or a different manager instance is a new identity and is outside this fix.

Implementation details

Why an Expando

Expando<T> associates private data with an object identity without adding a field to that object's class. The association does not keep its property value alive after the key object becomes inaccessible. It has been part of dart:core since before Dart 1.0, so this use does not raise the package's Dart 2.18 lower bound.

CookieManager owns one instance:

final Expando<_CookieHeaderState> _cookieHeaderStates = Expando();

Each key is a RequestOptions object, and each value contains two immutable strings: the header supplied as the source of the merge and the header last generated by this manager. Keeping this bookkeeping outside RequestOptions.extra avoids reserving a user-visible key, exposing internal state to loggers, or copying an internal marker through RequestOptions.copyWith.

State transitions

Interceptor pass Current Cookie header Source used for the merge Result
First pass Caller header or null Current header Source + cookies currently loaded from the jar
Reused options, unchanged header Exactly equals this manager's last output Stored original source Original source + freshly loaded jar cookies
Header replaced between passes Does not equal the last output New current header New source + freshly loaded jar cookies

For example:

first pass:  source=provided=value, jar=session=old
             -> provided=value; session=old

second pass after the jar changes to session=new:
             source=provided=value, jar=session=new
             -> provided=value; session=new

The old jar contribution is not treated as caller input, so it is neither duplicated nor retained after an update, deletion, or origin change.

Interceptor ordering

Request interceptors run in registration order. CookieManager reads both options.uri and the current Cookie header at its position in that chain, so interceptors that mutate either value must run before it:

dio.interceptors
  ..add(uriOrCookieMutatingInterceptor)
  ..add(CookieManager(cookieJar))
  ..add(interceptorThatDoesNotChangeUriOrCookies);

Changes made by a later interceptor have these consequences:

Later mutation Effect when the same options are reused
Authorization, body, method, extra, or unrelated headers Does not affect Cookie state matching
Replace the complete Cookie header The replacement becomes the source for the next merge
Append, reorder, or reformat the generated Cookie header It no longer exactly matches the stored output; the embedded old jar cookies can be treated as source cookies and merged again
Change baseUrl or path The current request may carry cookies selected for the URI observed before that change

The URI case is an ordering requirement for the current request, not only for retries. Moving a request from one origin or path to another after CookieManager has selected cookies can send cookies outside the scope for which they were loaded.

Why not deduplicate cookie names or values

A serialized Cookie request header contains name=value pairs but not the domain and path metadata that identified the cookies in the jar. The same request can legitimately contain multiple cookies with the same name, and dio also deliberately preserves a caller-provided cookie alongside a same-name jar cookie.

Filtering by name, or by a reconstructed cookie identity, therefore cannot distinguish a cookie generated by the previous interceptor pass from one explicitly supplied by the caller. Tracking the exact previous output preserves that provenance without changing first-pass merge behavior.

API and identity boundaries

  • No public API, request header precedence, or cookie ordering is changed.
  • No state is added to RequestOptions.extra.
  • Entries are scoped to one CookieManager and one RequestOptions object identity.
  • A copyWith result or a different manager instance is a new identity and intentionally starts with its current header as a new source.

Verification

Added six regression tests covering three sequential Dio.fetch calls with the same options, caller-provided same-name cookies, jar update and deletion, origin changes, caller header replacement, and state isolation between options. All six fail against origin/main and pass with this change.

The dio_cookie_manager package passes 21 tests. Scoped Melos format and fatal-info analysis checks are clean. Local coverage records 74 of 75 executable lines in cookie_mgr.dart (98.67%), including all new state logic.

Implementation and tests by Codex; independent authenticity and adversarial review passes by two Codex subagents.

New Pull Request Checklist

  • I have read the Documentation
  • I have read the Agent Contribution Guidelines (required if any part of the change was produced with AI assistance)
  • I have searched for a similar pull request in the project and found none (not applicable: this replaces fix(cookie_manager): deduplicate cookies when request is retried #2498)
  • I have updated this branch with the latest main branch to avoid conflicts (via merge from master or rebase)
  • I have added the required tests to prove the fix/feature I am adding
  • I have updated the documentation (not applicable: no public API or usage changes)
  • I have run the affected package tests without failures
  • I have updated the CHANGELOG.md in the corresponding package

Additional context and info

The previous implementation in #2498 used RFC 6265 cookie-store identity rules to justify filtering request-header cookies. Cookie-store replacement and request-header merging are different operations; this PR avoids changing first-pass merge semantics.

Track the source and generated Cookie headers per RequestOptions.

Repeated interceptor passes now merge caller input with the latest cookie jar state.

Preserve existing same-name caller cookies.

Cover jar updates, deletion, origin changes, and isolated request state.

Co-Authored-By: Codex <noreply@openai.com>
Document the URI and Cookie header ordering requirements for reused request options.

Co-Authored-By: Codex <noreply@openai.com>
@github-actions

Copy link
Copy Markdown
Contributor

Code Coverage Report: Only Changed Files listed

Package Base Coverage New Coverage Difference
plugins/cookie_manager/lib/src/cookie_mgr.dart 🟢 98.57% 🟢 98.67% 🟢 0.1%
Overall Coverage 🟢 85.95% 🟢 85.98% 🟢 0.03%

Minimum allowed coverage is 0%, this run produced 85.98%

@CaiJingLong CaiJingLong left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Conclusion: Approve

Summary of Changes

This PR fixes duplicate cookies accumulating when the same RequestOptions instance is reused across repeated interceptor passes (the retry scenario from #2442). CookieManager now keeps weak per-RequestOptions state (via Expando) recording the original caller-supplied Cookie header (source) and the last header generated by the manager (merged). On a reused pass whose current header exactly matches the stored merged value, loadCookies rebuilds from the stored original source plus freshly loaded jar cookies, instead of treating the previous output as new caller input.

Review Details

Dimension Conclusion Notes
Correctness Pass State transition logic is sound. Traced first-pass, reused-pass, jar-update, caller-header-replacement, and origin-change paths; all produce correct non-duplicating output. The merged == previousCookies guard correctly distinguishes the manager's own previous output from a caller-modified header. Expando does not retain the key after GC, so no memory leak. Empty-jar / empty-header edge cases are handled (empty merged vs null header falls through to the new-source branch, which is correct).
Tests Pass 6 new regression tests under reusing request options cover: repeated Dio.fetch with the same options, same-name caller + jar cookies, jar update + deleteAll, caller header replacement, origin change, and per-options state isolation. Verified locally: all 6 fail against origin/main source (e.g. provided=first; saved=value; saved=value) and pass with this PR. The end-to-end test via _CookieRecordingAdapter asserts the header observed at the adapter level, not just after onRequest, which is the right place to catch duplication. Full suite: 21/21 pass.
Style Pass Follows existing file conventions. _CookieHeaderState is a private immutable value class with a const constructor. Expando is a standard dart:core API available well below the package's 2.18 lower bound — no SDK bump. No debug residue, no commented-out code.
Risk Pass No public API, signature, default behavior, or header precedence change. State is scoped to CookieManager + RequestOptions identity; copyWith and different manager instances correctly start fresh (verified by the isolation test). The documented limitation — a later interceptor that appends/reformats the generated header will cause jar cookies to be re-merged on reuse — is pre-existing on main (same duplicate occurs) and is honestly disclosed in both the PR body and the loadCookies/class doc comments. Concurrent reuse of one RequestOptions across in-flight requests is outside the fix scope and is not a regression.
Docs Pass CHANGELOG.md updated under ## Unreleased. Class and loadCookies doc comments document the new ordering requirement (register after URI/cookie-mutating interceptors) and the exact-match reuse semantics. No public API or usage changes, so README/API docs need no update.
Repo constraints Pass Branch fix/2442-cookie-retry-dedup follows category/ticket-id-description. Commits use Conventional Commits (fix(...), docs(...)) with Co-Authored-By: Codex attribution. PR description discloses AI usage. PR template items are checked honestly; unchecked items carry (not applicable — reason) notes. Motivation traces to #2442; supersedes #2498 with a narrower, semantics-preserving approach.

Confirmed Key Points

  • Correctness: The source/merged state machine correctly reconstructs the original caller input on reuse and reloads jar cookies fresh, eliminating the accumulation reported in #2442 while preserving the existing same-name caller + jar coexistence behavior. No off-by-one, null-deref, or unhandled branch found.
  • Tests: 6 regression tests reproduce the issue against main and pass with the fix; full dio_cookie_manager suite (21 tests) green; dart analyze clean; dart format clean.
  • Risk: Additive, identity-scoped, no API/behavioral break. The only behavioral subtlety (later-interceptor header mutation defeating the exact-match guard) is pre-existing and explicitly documented.

Suggestions (non-blocking)

  • The PR checklist item "I have read the Documentation" is unchecked without a note. Consider checking it or adding a brief (not applicable — no public API change) note for consistency with the other unchecked items, though this is purely cosmetic.

本评论由 AI agent omp(模型: glm-5-2)生成

@AlexV525
AlexV525 merged commit 3b5cd7d into main Jul 21, 2026
5 checks passed
@AlexV525
AlexV525 deleted the fix/2442-cookie-retry-dedup branch July 21, 2026 01:30
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.

[Question][CookieManager] About override the old cookies by new cookies when they have the same keys?

2 participants