perf(cookie): avoid quadratic chunked cookie parsing and header rebuilds#1368
Open
trivikr wants to merge 2 commits into
Open
perf(cookie): avoid quadratic chunked cookie parsing and header rebuilds#1368trivikr wants to merge 2 commits into
trivikr wants to merge 2 commits into
Conversation
Assisted-by: OpenAI:gpt-5.4 Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com>
📝 WalkthroughWalkthroughIntroduces WeakMap-backed caching for parsed request cookies and response Set-Cookie state in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/utils/cookie.ts (1)
313-321: Malformed cookie handling is reasonable but worth documenting.When
parseSetCookiereturnsundefinedfor a malformed cookie (Line 316), the cookie is preserved instate.cookiesbut its key isundefined, meaning it won't match for deduplication. This is a reasonable fallback—malformed cookies won't be lost, but repeated writes of the same malformed cookie would accumulate. Consider adding a brief comment explaining this behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/cookie.ts` around lines 313 - 321, The code intentionally preserves malformed Set-Cookie strings in state.cookies while pushing undefined into state.keys when parseSetCookie returns undefined, which prevents them from being deduplicated via state.distinctKeys; add a concise inline comment next to the loop (or above the parseSetCookie usage) explaining that malformed cookies are retained unchanged, that _getDistinctCookieKey is only called for parsed cookies, and that resulting undefined keys mean malformed cookies will not be deduplicated (so repeated malformed cookies may accumulate), to make this fallback behavior explicit for future readers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/utils/cookie.ts`:
- Around line 313-321: The code intentionally preserves malformed Set-Cookie
strings in state.cookies while pushing undefined into state.keys when
parseSetCookie returns undefined, which prevents them from being deduplicated
via state.distinctKeys; add a concise inline comment next to the loop (or above
the parseSetCookie usage) explaining that malformed cookies are retained
unchanged, that _getDistinctCookieKey is only called for parsed cookies, and
that resulting undefined keys mean malformed cookies will not be deduplicated
(so repeated malformed cookies may accumulate), to make this fallback behavior
explicit for future readers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3dcf89a2-5780-4843-9557-295def9802b7
📒 Files selected for processing (2)
src/utils/cookie.tstest/unit/cookie-performance.test.ts
commit: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/utils/cookie.ts (1)
297-345: Consider extracting_getSetCookieState/_writeSetCookieStateto an internal module.This file is now well beyond the short-file target; moving this helper block to
src/utils/internal/would keepcookie.tseasier to maintain.As per coding guidelines, "Keep files short — aim for less than 200 lines of code per file, split when larger."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/cookie.ts` around lines 297 - 345, Extract the helper functions _getSetCookieState and _writeSetCookieState into a new internal module under src/utils/internal (e.g., src/utils/internal/setCookieState.ts), move any related types/consts they need (SetCookieState, responseCookiesCache) or re-export them as needed, and update all callers in cookie.ts to import these functions from the new module; ensure references to parseSetCookie, _getDistinctCookieKey, and Headers remain accessible (either by importing them into the new module or keeping them in cookie.ts and passing them in), run/fix tests and exports so behavior is identical after the move.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/cookie.ts`:
- Around line 34-44: The parseCookies function currently returns the
cached.cookies object instance directly from requestCookiesCache, allowing
callers to mutate shared state; change parseCookies (and the cache set) to
always return a shallow cloned cookies object instead of the original
reference—when reading cached (cached.source and cached.cookies) or after
computing cookies via parseCookie(source), return a new object (e.g.,
{...cached.cookies} or a new object copy of parseCookie result) so callers
cannot mutate the cache stored in requestCookiesCache; keep using the same keys
(headers, source) and function names (parseCookies, parseCookie,
requestCookiesCache) but ensure clones are stored/returned rather than the
original object references.
---
Nitpick comments:
In `@src/utils/cookie.ts`:
- Around line 297-345: Extract the helper functions _getSetCookieState and
_writeSetCookieState into a new internal module under src/utils/internal (e.g.,
src/utils/internal/setCookieState.ts), move any related types/consts they need
(SetCookieState, responseCookiesCache) or re-export them as needed, and update
all callers in cookie.ts to import these functions from the new module; ensure
references to parseSetCookie, _getDistinctCookieKey, and Headers remain
accessible (either by importing them into the new module or keeping them in
cookie.ts and passing them in), run/fix tests and exports so behavior is
identical after the move.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4fc46945-ade0-45f8-8171-adbbe74d3ede
📒 Files selected for processing (1)
src/utils/cookie.ts
Comment on lines
34
to
+44
| export function parseCookies(event: HTTPEvent): Record<string, string | undefined> { | ||
| return parseCookie(event.req.headers.get("cookie") || ""); | ||
| const headers = event.req.headers; | ||
| const source = headers.get("cookie") || ""; | ||
| const cached = requestCookiesCache.get(headers); | ||
| if (cached && cached.source === source) { | ||
| return cached.cookies; | ||
| } | ||
|
|
||
| const cookies = parseCookie(source); | ||
| requestCookiesCache.set(headers, { source, cookies }); | ||
| return cookies; |
There was a problem hiding this comment.
Prevent shared mutable state from leaking through parseCookies.
At Line 39 and Line 44, the same cached object instance is returned. If any caller mutates that object, later reads for the same Headers instance can return corrupted cookie values.
💡 Proposed fix
const requestCookiesCache = new WeakMap<
Headers,
- { source: string; cookies: Record<string, string | undefined> }
+ { source: string; cookies: Readonly<Record<string, string | undefined>> }
>();
export function parseCookies(event: HTTPEvent): Record<string, string | undefined> {
const headers = event.req.headers;
const source = headers.get("cookie") || "";
const cached = requestCookiesCache.get(headers);
if (cached && cached.source === source) {
- return cached.cookies;
+ return cached.cookies as Record<string, string | undefined>;
}
- const cookies = parseCookie(source);
+ const cookies = Object.freeze(
+ parseCookie(source),
+ ) as Readonly<Record<string, string | undefined>>;
requestCookiesCache.set(headers, { source, cookies });
- return cookies;
+ return cookies as Record<string, string | undefined>;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/cookie.ts` around lines 34 - 44, The parseCookies function
currently returns the cached.cookies object instance directly from
requestCookiesCache, allowing callers to mutate shared state; change
parseCookies (and the cache set) to always return a shallow cloned cookies
object instead of the original reference—when reading cached (cached.source and
cached.cookies) or after computing cookies via parseCookie(source), return a new
object (e.g., {...cached.cookies} or a new object copy of parseCookie result) so
callers cannot mutate the cache stored in requestCookiesCache; keep using the
same keys (headers, source) and function names (parseCookies, parseCookie,
requestCookiesCache) but ensure clones are stored/returned rather than the
original object references.
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
2 participants
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.
Summary
This PR improves cookie performance in the chunked-cookie path used by sessions.
Previously,
setCookie()rebuilt and reparsed the fullset-cookieheader list on every write. SincesetChunkedCookie()callssetCookie()for the main cookie and for each chunk, large chunked session writes became quadratic in chunk count. The read path had a similar issue:getChunkedCookie()repeatedly calledgetCookie(), which reparsed the full request cookie header each time.What Changed
Headersinstance insrc/utils/cookie.tsgetChunkedCookie()to read from a single parsed cookie map instead of reparsing per chunk.set-cookiestate in memory so unique cookie writes can append without reparsing existing headers.Summary by CodeRabbit
Refactor
Tests