Fix session cookie creation race: skip initial token refresh, wrap extension auth hooks #6563
Conversation
…h, and wrap extension auth hooks\n\nPrevents concurrent createSession calls and avoids treating the initial onIdTokenChanged as a refresh. This eliminates a race where useSessionCookie.createSession() could run before a Firebase ID token existed, causing:\n\n Error: No auth header available for session creation\n\nAlso wraps extension auth hooks with async error handling to avoid unhandled rejections.\n\nRefs: Sentry issue 6990347926
dosubot
bot
added
the
size:L
🎭 Playwright Test Results⏰ Completed at: 11/05/2025, 06:35:59 AM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 11/05/2025, 06:21:48 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
|
Only the 'Skip initial token “refresh”' is needed, rest are additional for additional hardening |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 3.31 MB (baseline 3.31 MB) • 🔴 +1.31 kBMain entry bundles and manifests
Status: 3 added / 3 removed Graph Workspace — 729 kB (baseline 729 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 8.18 kB (baseline 8.18 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 1 added / 1 removed Panels & Settings — 293 kB (baseline 293 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 6 added / 6 removed UI Components — 12.6 kB (baseline 12.6 kB) • ⚪ 0 BReusable component library chunks
Status: 1 added / 1 removed Data & Services — 10.4 kB (baseline 10.4 kB) • ⚪ 0 BStores, services, APIs, and repositories
Status: 1 added / 1 removed Utilities & Hooks — 1.07 kB (baseline 1.07 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Vendor & Third-Party — 5.32 MB (baseline 5.32 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 2.55 MB (baseline 2.55 MB) • ⚪ 0 BBundles that do not match a named category
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
dosubot
bot
added
size:M
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
benceruleanlu
changed the title
christian-byrne
added
the
claude-review
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Comprehensive PR Review
This review is generated by Claude. It may not always be accurate, as with human reviewers. If you believe that any of the comments are invalid or incorrect, please state why for each. For others, please implement the changes in one way or another.
Review Summary
PR: Fix session cookie creation race: skip initial token refresh, wrap extension auth hooks (#6563)
Impact: 78 additions, 4 deletions across 3 files
Issue Distribution
- Critical: 0
- High: 0
- Medium: 2
- Low: 2
Category Breakdown
- Architecture: 1 issues
- Security: 0 issues
- Performance: 1 issues
- Code Quality: 2 issues
Key Findings
Architecture & Design
The PR correctly addresses the race condition in session creation by implementing a deduplication mechanism. The solution is architecturally sound, using a straightforward approach to track the initial token event per user. The error handling wrapper integration follows established patterns in the codebase.
Security Considerations
No security vulnerabilities identified. The changes maintain the existing authentication flow security while improving reliability. The error handling enhancements actually improve security posture by preventing unhandled promise rejections that could expose internal state.
Performance Impact
The changes have minimal performance impact. The additional state tracking (lastTokenUserId) is lightweight. However, there is a minor concern about potential memory leaks from Firebase listeners that should be monitored.
Integration Points
The changes integrate well with existing authentication systems. The deduplication logic is isolated to the auth store and doesn't affect other components. Extension error handling is improved without changing extension APIs.
Positive Observations
- Excellent test coverage: The PR includes comprehensive unit tests covering edge cases including concurrent operations and error scenarios
- Clear problem identification: The PR description clearly explains the root cause and solution approach
- Minimal invasive changes: The fix is targeted and doesn't require extensive refactoring
- Proper error handling: Uses the existing error handling patterns consistently
- Race condition fix: Addresses a real production issue affecting user experience
References
- Repository Architecture Guide
- Frontend Standards
- Security Guidelines
Next Steps
- Address medium-priority race condition concern for rapid user switches
- Consider error context improvements for better debugging
- Monitor potential memory leaks from Firebase listeners in production
- No critical or high-priority issues blocking merge
This is a comprehensive automated review. For architectural decisions requiring human judgment, please request additional manual review.
github-actions
bot
removed
the
claude-review
benceruleanlu
left a comment
benceruleanlu
left a comment
There was a problem hiding this comment.
Addressed review feedback:
- Removed in-flight dedupe in session creation to avoid suppressing legitimate calls after user switches.
- Added structured error context for extension auth hooks (extension + hook name) while preserving toast UX.
- Clarified async wrapper return is intentionally discarded for hooks typed as void/Promise.
- Explained token listener lifecycle: single registration tied to store lifetime; no accumulation in runtime.
- Clarified race considerations for rapid user switches; logic treats first token per uid as initialization, subsequent tokens increment; sign-out resets.
Please let me know if you want a test-only dispose hook for the auth store’s token listener.
| ) | ||
| onUserResolved((user) => { | ||
| void extension.onAuthUserResolved?.(user, app) | ||
| void handleUserResolved(user) |
There was a problem hiding this comment.
Addressed: the async wrapper now adds structured error context for auth hooks — we log the extension name and the hook (e.g., onAuthUserResolved) and still route to the shared toast. This makes triage clearer without changing UX.
| ) | ||
| onTokenRefreshed(() => { | ||
| void extension.onAuthTokenRefreshed?.() | ||
| void handleTokenRefreshed() |
There was a problem hiding this comment.
Clarification: the wrapper’s return is intentionally discarded (void handleUserResolved(user)). Extension hooks are void | Promise<void>, so a potential undefined return from the wrapper doesn’t affect behavior.
| }) | ||
|
|
||
| // Listen for token refresh events | ||
| onIdTokenChanged(auth, (user) => { |
There was a problem hiding this comment.
Clarification: the Pinia store is a singleton for the app’s lifetime, so we register one onIdTokenChanged listener and it doesn’t accumulate in normal runtime. If preferred for tests, we can add an explicit dispose() to call the unsubscribe, but it isn’t necessary for the app.
| onIdTokenChanged(auth, (user) => { | ||
| if (user && isCloud) { | ||
| // Skip initial token change | ||
| if (lastTokenUserId.value !== user.uid) { |
There was a problem hiding this comment.
Clarification: onIdTokenChanged events are delivered sequentially. We treat the first token for any uid as initialization by setting lastTokenUserId and returning; subsequent events for the same uid increment the trigger. We reset lastTokenUserId on sign‑out in onAuthStateChanged. This avoids spurious increments without introducing races; tests cover this behaviour in tests-ui/tests/store/firebaseAuthStore.test.ts.
…ener lifecycle; address PR #6563 review comments
dosubot
bot
added
size:L
christian-byrne
left a comment
christian-byrne
left a comment
There was a problem hiding this comment.
LGTM. We can verify the fix in the data after the next deploy
christian-byrne
deleted the
fix/session-cookie-dedupe-and-init-refresh
branch
Fixes a race causing “No auth header available for session creation” during sign‑in, by skipping the initial token refresh event, and wrapping extension auth hooks with async error handling.
Sentry: https://comfy-org.sentry.io/issues/6990347926/?alert_rule_id=1614600&project=4509681221369857
Context
Exact pre‑fix failure path
What this PR changes
Result
Notes
┆Issue is synchronized with this Notion page by Unito