Skip to content

Update session status to 'unhandled' and extend SessionStatus type #17849

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

Update session status to 'unhandled' and extend SessionStatus type #17849

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d4c29f4
fix(session): Update session status to 'unhandled' and extend Session…
JealousGx Oct 2, 2025
b2c5600
fix(tests): Update expected session status from 'crashed' to 'unhandl…
JealousGx Oct 2, 2025
87f9ab7
fix(session): Extend requestSession status to include 'unhandled' and…
JealousGx Oct 2, 2025
540fcf8
fix(session): Update requestSession status handling for exceptions
JealousGx Oct 2, 2025
8ae61ed
fix(session): Combine 'crashed' and 'unhandled' counts in session agg…
JealousGx Oct 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion dev-packages/browser-integration-tests/suites/sessions/update-session/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ sentryTest('should update session when an error is thrown.', async ({ getLocalTe
expect(updatedSession).toBeDefined();
expect(updatedSession.init).toBe(false);
expect(updatedSession.errors).toBe(1);
expect(updatedSession.status).toBe('crashed');
expect(updatedSession.status).toBe('unhandled');
expect(pageloadSession.sid).toBe(updatedSession.sid);
});

Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -963,7 +963,7 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {

if (shouldUpdateAndSend) {
updateSession(session, {
...(crashed && { status: 'crashed' }),
...(crashed && { status: 'unhandled' }),
errors: session.errors || Number(errored || crashed),
});
this.captureSession(session);
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/types-hoist/session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export interface Session {

export type SessionContext = Partial<Session>;

export type SessionStatus = 'ok' | 'exited' | 'crashed' | 'abnormal';
export type SessionStatus = 'ok' | 'exited' | 'crashed' | 'abnormal' | 'unhandled';

/** JSDoc */
export interface SessionAggregates {
Comment on lines 30 to 36
Copy link

Choose a reason for hiding this comment

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

Potential bug: The new 'unhandled' session status is not handled by all parts of the system, leading to data corruption in session aggregation and inconsistent status reporting.

  • Description: The introduction of the 'unhandled' session status is incomplete. The session aggregation logic in packages/node-core/src/integrations/http/httpServerIntegration.ts contains a hardcoded mapping of statuses that was not updated to include 'unhandled'. When a session with this new status is processed, the logic attempts to access a non-existent key, which results in bucket['undefined'] = NaN. This corrupts the session aggregate data used for health monitoring. Additionally, packages/core/src/server-runtime-client.ts was not updated and still sets the session status to 'crashed' for unhandled exceptions, creating an inconsistency with other parts of the client that now use 'unhandled'.

  • Suggested fix: Update the session aggregation mapping in httpServerIntegration.ts to include the 'unhandled' status. Also, update server-runtime-client.ts and any other remaining locations, including tests, to use the 'unhandled' status instead of 'crashed' for consistency.
    severity: 0.75, confidence: 0.98

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Author

@JealousGx JealousGx Oct 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@Lms24 , we dont need to update the test cases in httpServerIntegration.test.ts file right? I noticed crashed represents unhandled cases, so, it would be pointless to handle unhandled separately, if I got that right.

Expand Down