test(express): Error handler tests - #23724
Merged
Merged
Conversation
s1gr1d
requested review from
a team,
isaacs,
msonnb,
mydea and
stephanie-anderson
and removed request for
a team
August 28, 2026 09:59
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
+254
to
+262
| it('defers to the integration once the request is marked', () => { | ||
| const request = makeHandledRequest(); | ||
| captureExceptionSpy.mockClear(); | ||
| const error = Object.assign(new Error('boom'), { statusCode: 500 }); | ||
|
|
||
| expressErrorHandler()(error, request, makeResponse(), vi.fn()); | ||
|
|
||
| expect(captureExceptionSpy).not.toHaveBeenCalled(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Bug: A request is marked as handled before checking if an error should be captured, causing subsequent, distinct errors on the same request to be silently dropped.
Severity: MEDIUM
Suggested Fix
Move the markExpressErrorHandled(request) call to after the shouldHandleError check. This ensures a request is only marked as handled if the error is actually captured and processed.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location:
packages/server-utils/test/integrations/express-error-handler.test.ts#L254-L262
Potential issue: The request is marked as handled by calling
`markExpressErrorHandled(request)` before the `shouldHandleError` check is performed. If
an initial error is skipped (e.g., a 400 error that is configured to be ignored), the
request is still marked. Consequently, if a second, distinct error occurs on the same
request (e.g., a 500 error), the `isExpressErrorHandled(request)` check will return
`true`, causing this subsequent error to be silently dropped instead of being captured.
Did we get this right? 👍 / 👎 to inform future reviews.
Comment on lines
+273
to
+282
| // TODO: `res.sentry` carries the captured event id, but only this middleware sets it. | ||
| // Once the integration captures first, apps reading `res.sentry` get undefined instead of the id. | ||
| it.fails('exposes the event id on the response when the integration captured the error', () => { | ||
| const res = makeResponse(); | ||
| const error = Object.assign(new Error('boom'), { statusCode: 500 }); | ||
|
|
||
| expressErrorHandler()(error, makeHandledRequest(), res, vi.fn()); | ||
|
|
||
| expect(res.sentry).toBe('event-id'); | ||
| }); |
Contributor
There was a problem hiding this comment.
Bug: The res.sentry property is not set on the response when an error is captured before the expressErrorHandler middleware runs, as the handler exits early.
Severity: LOW
Suggested Fix
Ensure that res.sentry is set on the response object whenever an error is captured by the Express integration, regardless of whether it's captured in the main error handler or another part of the instrumentation.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location:
packages/server-utils/test/integrations/express-error-handler.test.ts#L273-L282
Potential issue: When an error is captured by the integration before the main
`expressErrorHandler` middleware runs, the request is marked as handled. The
`expressErrorHandler` then sees this mark via `isExpressErrorHandled(request)` and exits
early by calling `next(error)`. As a result, the code that sets the Sentry event ID on
the response, `(res as { sentry?: string }).sentry = eventId;`, is never reached. This
leaves `res.sentry` as `undefined` for any downstream middleware that might rely on it.
Did we get this right? 👍 / 👎 to inform future reviews.
Member
|
good stuff! isaacs review came in after I merged the PR, we may need/want to adjust some things based on it 😓 General thoughts:
|
Contributor
size-limit report 📦
|
isaacs
approved these changes
Aug 28, 2026
isaacs
left a comment
isaacs
left a comment
Member
There was a problem hiding this comment.
Awesome work, I was going to create an issue to circle back on this, but you beat me to it ;)
It's hilarious that the bots are upset about the tests showing failures, when that's exactly the point. 🤖🤪
Landing this, and then we can make them pass in a follow-up.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
shouldHandleErroroption is going to be removed fromsetupExpressErrorHandler. This PR just adds some tests to split up the PR a bit.Two tests are marked
it.fails. Both cover behaviour @isaacs raised on #23464.The first shows the dedup marker is keyed on the request, not the error, so only the first error per request is captured. A 4xx that
shouldHandleErrorskips still marks the request, so a later 500 is lost.The second shows
res.sentryis undefined whenever the integration captures first, because only the deprecated middleware sets it.Reference:
expressIntegration#23464