Skip to content

Conversation

@tusharpandey13
Copy link
Contributor

@tusharpandey13 tusharpandey13 commented Sep 21, 2025

  • All new/changed/fixed functionality is covered by tests (or N/A)
  • I have added documentation for all new/changed functionality (or N/A)

📋 Changes

This PR implements three complementary fixes to resolve updateSession reliability issues:

  • Fixed race condition in timestamp calculations - Eliminated inconsistent maxAge values by using the same updatedAt timestamp throughout calculateMaxAge() instead of calling this.epoch() twice, preventing Max-Age=0 cookies from timing inconsistencies

    • MODIFIED src/server/session/abstract-session-store.ts
  • Implemented conditional legacy cookie cleanup - Added existence check before deleting legacy cookies to prevent unnecessary Max-Age=0 cookies appearing in response headers during updateSession() calls

    • MODIFIED src/server/session/stateless-session-store.ts
    • MODIFIED src/server/session/stateless-session-store.test.ts (updated test mocks)
  • Fixed header overwrite in Pages Router - Resolved session chunk loss by collecting all set-cookie headers into an array before setting them, preventing res.setHeader() from overwriting multiple cookies

    • MODIFIED src/server/client.ts
    • ADDED src/server/updateSession-header-fix.test.ts (comprehensive test suite)

📎 References

Fixes: #2316

🎯 Testing

Automated:

  • Updated existing StatelessSessionStore unit tests to mock getChunkedCookie() properly for legacy cookie scenarios
  • Added comprehensive test suite updateSession-header-fix.test.ts covering multiple set-cookie header scenarios, legacy cookie deletion, and edge cases
  • All 308+ unit tests pass

Manual:

  1. Create a Pages Router API route that calls updateSession(req, res, session) with chunked session data
  2. Add console.log(res.getHeaders()) immediately after the updateSession() call
  3. Before fix: Shows either 'set-cookie': ['appSession=; Path=/; Max-Age=0'] (unnecessary deletion) or missing session chunks (header overwrite)
  4. After fix: Shows all session chunks properly preserved without unnecessary Max-Age=0 deletion cookies

…t; perviously this was setting a SetCookie header with maxAge=0 for each call to updateSession()
@tusharpandey13 tusharpandey13 requested a review from a team as a code owner September 21, 2025 19:42
@codecov-commenter
Copy link

codecov-commenter commented Sep 21, 2025

Codecov Report

❌ Patch coverage is 93.10345% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.61%. Comparing base (76e635d) to head (594e0e3).

Files with missing lines Patch % Lines
src/server/client.ts 83.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2330      +/-   ##
==========================================
+ Coverage   86.90%   87.61%   +0.71%     
==========================================
  Files          37       37              
  Lines        4070     4086      +16     
  Branches      799      809      +10     
==========================================
+ Hits         3537     3580      +43     
+ Misses        530      503      -27     
  Partials        3        3              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tusharpandey13 tusharpandey13 marked this pull request as draft September 21, 2025 19:43
@tusharpandey13 tusharpandey13 changed the title fix: only set maxAge=0 for legcy cookie if cookies is actually present; perviously this was setting a SetCookie header with maxAge=0 for each call to updateSession() bugfix: Fix updateSession setting unnecessary Max-Age=0 cookies Sep 21, 2025
@tusharpandey13 tusharpandey13 changed the title bugfix: Fix updateSession setting unnecessary Max-Age=0 cookies bugfix: Fix updateSession setting unnecessary Max-Age=0 cookies Sep 21, 2025
…er() was called with multiple entries of set-ccokie header key, leading to the latest entries overwriting old entries, leading to missing cookies
@tusharpandey13 tusharpandey13 changed the title bugfix: Fix updateSession setting unnecessary Max-Age=0 cookies Fix updateSession setting empty cookies and header overwrite issues Sep 30, 2025
@tusharpandey13 tusharpandey13 marked this pull request as ready for review September 30, 2025 06:59
@tusharpandey13 tusharpandey13 changed the title Fix updateSession setting empty cookies and header overwrite issues Fix updateSession and header overwrite issues Sep 30, 2025
@tusharpandey13 tusharpandey13 changed the title Fix updateSession and header overwrite issues Fix updateSession and header overwrite issues Sep 30, 2025
@tusharpandey13
Copy link
Contributor Author

@claude please review this PR

@github-actions
Copy link
Contributor

github-actions bot commented Oct 29, 2025

Claude finished @tusharpandey13's task —— View job


Code Review Complete

  • Read PR diff and understand changes
  • Read project context
  • Review changed files for context
  • Perform code quality review
  • Perform security review
  • Perform performance review
  • Provide inline feedback on specific issues
  • Post overall assessment

Overall Assessment

⚠️ This PR requires minor test improvements before merging. The core fixes for the race condition, legacy cookie cleanup, and header overwrite issues are sound and well-implemented. However, the test suite contains non-deterministic behavior (using Math.random()) and type safety issues that should be addressed to ensure reliable test coverage.


Job Run | Branch: debug/updateSession-empty-cookie

@tusharpandey13 tusharpandey13 merged commit df1725e into main Oct 29, 2025
13 checks passed
@tusharpandey13 tusharpandey13 deleted the debug/updateSession-empty-cookie branch October 29, 2025 07:17
This was referenced Oct 30, 2025
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.

updateSession sets an empty cookie

4 participants