One Character Was Hiding Every Healthy Process

This is a bug you can see in a single line, once you know where to look. It had been quietly corrupting a metric people use to decide whether a release is safe.

What the code is trying to do

Sentry has a feature called release health. The idea is simple: out of all the times your code ran, what fraction finished normally and what fraction blew up? If version 2.1 crashes twice as often as 2.0, you want to know before your users tell you.

Each run gets a session. In the Node SDK, one session means one run of your process.

A session has a status. ok means “still running, nothing has gone wrong.” Anything else — exited, crashed, abnormal, unhandled — means it’s over. Those are called terminal statuses, and the spec has a rule about them:

When a session is moved away from ok it must not be updated anymore.

Once a session is finished, leave it alone. Don’t touch it again.

The line

Node hooks into beforeExit, which fires when the process is about to shut down cleanly. That’s the moment to say “this run finished fine.”

The comment above the code described exactly the right rule: only end the session if it exists and hasn’t already reached a terminal status.

The code underneath did the opposite:

if (session?.status !== 'ok') {
  endSession();
}

Read it slowly. ok means still healthy. So this says: only end the session if it is NOT still healthy.

That’s backwards. And it’s backwards in both directions at once:

• Your process ran fine. Status is ok. The condition is false. Nothing is sent. The run is never reported at all.

• Your process already crashed. Status is terminal. The condition is true, so it sends another update to a session that was already finished — with a recalculated duration. Precisely what the quoted spec rule forbids.

The comment and the code sat two lines apart and disagreed with each other.

Why it mattered more than it looks

Three things stack up here.

This integration is on by default. You don’t opt in. Every Node app using Sentry had it.

It’s the only place Node ends a session. Not one of several — the only one. Starting a session just creates the record; the one and only route to actually sending it is endSession(). The browser SDK sends an initial session right after starting one, so it has a second chance. Node has no such fallback.

So for any process that exited normally — a script, a CLI tool, a background worker, a serverless function — release health received nothing at all.

And the half that was dropped was the healthy half. Crashes still got through by other routes. Clean runs didn’t. A metric built on “what fraction of runs were fine” was being fed almost exclusively the bad ones. Your crash rate looked worse than reality, which is arguably more harmful than no data — you’d trust a number that was wrong in a specific direction.

The fix

-        if (session?.status !== 'ok') {
+        if (session?.status === 'ok') {

Two characters.

I wrote the guard as === 'ok' rather than listing out the terminal statuses to exclude, for a reason: ok is the only non-terminal status. Checking for the one good state is a shorter and more durable rule than denylisting four bad ones — and if a new terminal status is ever added, this line still behaves correctly. It also matches how the rest of the SDK already decides whether a session is finished.

I also trimmed the comment. It had said terminal means “i.e. Exited or Crashed,” which lists only two of the four.

The part that explains the bug’s lifespan

There was no test file for this integration at all. Not a thin one. None.

So I wrote one: nine cases. Six of them fail against the old guard. The other three pin down behaviour that was already correct, so the next person changing this file finds out immediately if they break it.

A one-character inversion survived in a default-on integration because nothing was watching that line.

What I’d take from it

• When a comment and the code disagree, that’s a finding. The comment was right. Someone understood the rule, wrote it down clearly, and then wrote the opposite in code. A comment is evidence of intent — when it contradicts the implementation, don’t assume the comment is stale.

• “No tests exist for this file” is a strong signal. Not proof of a bug, but it tells you where to look. Untested code isn’t necessarily wrong; it just has nothing stopping it from being wrong.

• Prefer checking for the one good state. === 'ok' can’t drift. A list of bad states has to be maintained forever, and it silently rots the day someone adds a fifth one.

• The most dangerous bug isn’t missing data — it’s biased data. Nobody noticed because the dashboards still had numbers in them. They were just numbers built from the crashes only.