Skip to content

feat(fcm): Improve HTTP/2 session error handling.#3126

Open
jonathanedey wants to merge 5 commits intoerror-revampfrom
je-error-re
Open

feat(fcm): Improve HTTP/2 session error handling.#3126
jonathanedey wants to merge 5 commits intoerror-revampfrom
je-error-re

Conversation

@jonathanedey
Copy link
Copy Markdown
Collaborator

@jonathanedey jonathanedey commented Apr 29, 2026

This PR refactors how FCM http2 session errors are handled making use of the new cause attribute on FirebaseErrors to allow failing of request at the stream level with context on session error cause.

  • Defer session error reporting by instead of failing fast then waiting for the BatchResponse attached to the error to resolve, we now let streams fail naturally and attach session errors as cause to failed messages in BatchResponse.
  • Add AggregateError support when multiple session errors are captured during a batch.
  • Simplify Http2SessionHandler by removing the promise-based failure tracking (invoke()), since we no longer need to catch rejections in sendEach().
  • Remove internal FirebaseMessagingSessionError

@jonathanedey jonathanedey added the release:stage Stage a release candidate label Apr 29, 2026
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the messaging service's HTTP/2 session management, replacing the specialized FirebaseMessagingSessionError with a more standard error aggregation approach. By utilizing AggregateError, the system now captures multiple session-level failures and includes them in the BatchResponse. The Http2SessionHandler was also updated to collect errors throughout the session lifecycle. Feedback focuses on a logic error in createSession where the internal session state is not correctly updated, a recommendation for null-safety when checking session status, and an improvement to error messaging to ensure session failure details are not lost during serialization.

Comment thread src/utils/api-request.ts Outdated
Comment thread src/utils/api-request.ts Outdated
Comment thread src/messaging/messaging.ts
@jonathanedey
Copy link
Copy Markdown
Collaborator Author

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the HTTP/2 session error handling in the messaging module, replacing the promise-rejection model in Http2SessionHandler with an error-collection mechanism. This change allows session errors to be aggregated into the BatchResponse using AggregateError, leading to the removal of the FirebaseMessagingSessionError class. The PR also includes various formatting and semicolon fixes for consistency. Feedback includes suggestions to simplify redundant logic in the error parsing function and to maintain consistency in the use of optional chaining when accessing error properties.

Comment thread src/messaging/messaging.ts Outdated
Comment thread src/utils/api-request.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release:stage Stage a release candidate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants