Skip to content

Commit 6de4cba

Browse files
authored
fix(core): Only consider exception mechanism when updating session status from event with exceptions (#18137)
This PR fixes an oversight in our session update logic when we update the SDK's session status while processing an error event. Previously, we'd always set `status: 'crashed'` as soon as the top-level `event.level` field was set to `'fatal'` (added in #15072). However, this disregarded the case where the level was initially `'fatal'` but later on the SDK or users decided to set the `event.exceptions.values[i].mechanism.handled` field to `true`. Simplest reproduction: ``` Sentry.captureException(new Error('test'), { captureContext: { level: 'fatal' } }); ``` This PR changes the update logic to only look at the event mechanisms if the event contained exceptions. I _think_ this is still compatible with #15072 because the PR suggests we only care about `event.level === 'fatal'` if no exceptions are on the event. But I'd appreciate a second pair of eyes on this (cc @timfish).
1 parent 8b33254 commit 6de4cba

File tree

2 files changed

+107
-2
lines changed

2 files changed

+107
-2
lines changed

packages/core/src/client.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1037,16 +1037,18 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
10371037

10381038
/** Updates existing session based on the provided event */
10391039
protected _updateSessionFromEvent(session: Session, event: Event): void {
1040+
// initially, set `crashed` based on the event level and update from exceptions if there are any later on
10401041
let crashed = event.level === 'fatal';
10411042
let errored = false;
10421043
const exceptions = event.exception?.values;
10431044

10441045
if (exceptions) {
10451046
errored = true;
1047+
// reset crashed to false if there are exceptions, to ensure `mechanism.handled` is respected.
1048+
crashed = false;
10461049

10471050
for (const ex of exceptions) {
1048-
const mechanism = ex.mechanism;
1049-
if (mechanism?.handled === false) {
1051+
if (ex.mechanism?.handled === false) {
10501052
crashed = true;
10511053
break;
10521054
}

packages/core/test/lib/client.test.ts

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { afterEach, beforeEach, describe, expect, it, test, vi } from 'vitest';
2+
import type { SeverityLevel } from '../../src';
23
import {
34
addBreadcrumb,
45
dsnToString,
@@ -2308,6 +2309,108 @@ describe('Client', () => {
23082309
});
23092310
});
23102311

2312+
describe('_updateSessionFromEvent()', () => {
2313+
describe('event has no exceptions', () => {
2314+
it('sets status to crashed if level is fatal', () => {
2315+
const client = new TestClient(getDefaultTestClientOptions());
2316+
const session = makeSession();
2317+
getCurrentScope().setSession(session);
2318+
2319+
client.captureEvent({ message: 'test', level: 'fatal' });
2320+
2321+
const updatedSession = client.session;
2322+
2323+
expect(updatedSession).toMatchObject({
2324+
duration: expect.any(Number),
2325+
errors: 1,
2326+
init: false,
2327+
sid: expect.any(String),
2328+
started: expect.any(Number),
2329+
status: 'crashed',
2330+
timestamp: expect.any(Number),
2331+
});
2332+
});
2333+
2334+
it.each(['error', 'warning', 'log', 'info', 'debug'] as const)(
2335+
'sets status to ok if level is %s',
2336+
(level: SeverityLevel) => {
2337+
const client = new TestClient(getDefaultTestClientOptions());
2338+
const session = makeSession();
2339+
getCurrentScope().setSession(session);
2340+
2341+
client.captureEvent({ message: 'test', level });
2342+
2343+
const updatedSession = client.session;
2344+
2345+
expect(updatedSession?.status).toEqual('ok');
2346+
},
2347+
);
2348+
});
2349+
2350+
describe('event has exceptions', () => {
2351+
it.each(['fatal', 'error', 'warning', 'log', 'info', 'debug'] as const)(
2352+
'sets status ok for handled exceptions and ignores event level %s',
2353+
(level: SeverityLevel) => {
2354+
const client = new TestClient(getDefaultTestClientOptions());
2355+
const session = makeSession();
2356+
getCurrentScope().setSession(session);
2357+
2358+
client.captureException(new Error('test'), { captureContext: { level } });
2359+
2360+
const updatedSession = client.session;
2361+
2362+
expect(updatedSession?.status).toEqual('ok');
2363+
},
2364+
);
2365+
2366+
it.each(['fatal', 'error', 'warning', 'log', 'info', 'debug'] as const)(
2367+
'sets status crashed for unhandled exceptions and ignores event level %s',
2368+
(level: SeverityLevel) => {
2369+
const client = new TestClient(getDefaultTestClientOptions());
2370+
const session = makeSession();
2371+
getCurrentScope().setSession(session);
2372+
2373+
client.captureException(new Error('test'), { captureContext: { level }, mechanism: { handled: false } });
2374+
2375+
const updatedSession = client.session;
2376+
2377+
expect(updatedSession?.status).toEqual('crashed');
2378+
},
2379+
);
2380+
2381+
it('sets status crashed if at least one exception is unhandled', () => {
2382+
const client = new TestClient(getDefaultTestClientOptions());
2383+
const session = makeSession();
2384+
getCurrentScope().setSession(session);
2385+
2386+
const event: Event = {
2387+
exception: {
2388+
values: [
2389+
{
2390+
mechanism: { type: 'generic', handled: true },
2391+
},
2392+
{
2393+
mechanism: { type: 'generic', handled: false },
2394+
},
2395+
{
2396+
mechanism: { type: 'generic', handled: true },
2397+
},
2398+
],
2399+
},
2400+
};
2401+
2402+
client.captureEvent(event);
2403+
2404+
const updatedSession = client.session;
2405+
2406+
expect(updatedSession).toMatchObject({
2407+
status: 'crashed',
2408+
errors: 1, // an event with multiple exceptions still counts as one error in the session
2409+
});
2410+
});
2411+
});
2412+
});
2413+
23112414
describe('recordDroppedEvent()/_clearOutcomes()', () => {
23122415
test('records and returns outcomes', () => {
23132416
const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN });

0 commit comments

Comments
 (0)