feat(node): Use process.on('SIGTERM') for flushing in Vercel functions
#17583
Conversation
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
| if (process.env.VERCEL) { | ||
| process.on('SIGTERM', async () => { | ||
| // We have 500ms for processing here, so we try to make sure to have enough time to send the events | ||
| await client.flush(200); |
There was a problem hiding this comment.
Did you test this on a vercel function? Generally makes sense but we could bump this more up I'd say?
There was a problem hiding this comment.
did not test this so far at all, maybe you can take this PR over when you have some time and properly test it. The reason I went with 200 here is because this is just the timeout used for finishing events, we still need time to actually make the http request so there needs to be some wiggle room -to the max. 500ms - if we spend 500ms on processing events, we have no more time to actually send the events 😅
There was a problem hiding this comment.
Ah yeah that makes sense I thought this time is included already, I'll take it over
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| export function vercelWaitUntil(task: Promise<unknown>): void { | ||
| // We only flush manually in Vercel Edge runtime | ||
| // In Node runtime, we use process.on('SIGTERM') instead |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
no, that's the whole point of this PR my friend
| if (process.env.VERCEL) { | ||
| process.on('SIGTERM', async () => { | ||
| // We have 500ms for processing here, so we try to make sure to have enough time to send the events | ||
| await client.flush(200); | ||
| }); | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| // We have 500ms for processing here, so we try to make sure to have enough time to send the events | ||
| await client.flush(200); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Multiple SIGTERM handlers accumulate on repeated init calls
Each call to init() on Vercel registers a new SIGTERM handler using process.on('SIGTERM', ...) without checking if one was already registered. Since calling init() multiple times is a supported scenario (as confirmed by the existing multiple-init integration test), this can lead to multiple handlers being registered. When SIGTERM is received, each handler will call client.flush(200) on its respective client, potentially causing multiple concurrent flush operations within the 500ms Vercel shutdown window, which could lead to race conditions or unexpected behavior.
There was a problem hiding this comment.
I think we can safely ignore this case.
| // Note: vercelWaitUntil only does something in Vercel Edge runtime | ||
| // In Node runtime, we use process.on('SIGTERM') instead | ||
| // @ts-expect-error This is not typed | ||
| if (GLOBAL_OBJ[Symbol.for('@vercel/request-context')]) { | ||
| // Vercel has a waitUntil equivalent that works without execution context |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
we instead moved the flushing this to node-core
|
|
||
| beforeEach(() => { | ||
| globalWithEdgeRuntime.EdgeRuntime = 'edge-runtime'; | ||
| }); |
There was a problem hiding this comment.
Tests don't verify new non-Edge early return behavior (Bugbot Rules)
Per the review rules requiring "tests actually test the newly added behaviour": The beforeEach sets EdgeRuntime = 'edge-runtime' for all tests, so none of them exercise the newly added early return path when EdgeRuntime is not a string (the Node runtime case). The core behavior change of this PR — vercelWaitUntil becoming a no-op in Node runtime — has no test coverage. A test verifying that waitUntil is NOT called when EdgeRuntime is undefined would validate this new behavior.
| process.on('SIGTERM', async () => { | ||
| // We have 500ms for processing here, so we try to make sure to have enough time to send the events | ||
| await client.flush(200); | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Bug: The async SIGTERM handler for process.on() is not awaited, creating a race condition where the process may exit before client.flush() completes, leading to data loss.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The SIGTERM handler is an async function, but Node.js's process.on() does not wait for the returned Promise to resolve. When a SIGTERM signal is received, the client.flush(200) operation is initiated, but the process may terminate before this asynchronous operation completes. This creates a race condition that can lead to the loss of buffered events that were supposed to be sent to Sentry before shutdown. The integration tests mask this issue by using setInterval to artificially keep the process alive, which is not a reliable mechanism for production environments.
💡 Suggested Fix
To ensure the flush completes, the process exit should be explicitly managed. After the await client.flush(200) call, add process.exit(0) to gracefully terminate the process only after the flush has finished. This prevents the race condition and ensures events are not lost.
🤖 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/node-core/src/sdk/index.ts#L149-L154
Potential issue: The `SIGTERM` handler is an `async` function, but Node.js's
`process.on()` does not wait for the returned Promise to resolve. When a `SIGTERM`
signal is received, the `client.flush(200)` operation is initiated, but the process may
terminate before this asynchronous operation completes. This creates a race condition
that can lead to the loss of buffered events that were supposed to be sent to Sentry
before shutdown. The integration tests mask this issue by using `setInterval` to
artificially keep the process alive, which is not a reliable mechanism for production
environments.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8029537
| } | ||
|
|
||
| // Note: vercelWaitUntil only does something in Vercel Edge runtime | ||
| // In Node runtime, we use process.on('SIGTERM') instead |
There was a problem hiding this comment.
flushIfServerless returns early without flushing in Vercel Node
In Vercel Node runtime, when @vercel/request-context exists, flushIfServerless calls vercelWaitUntil and immediately returns. However, vercelWaitUntil now returns early when EdgeRuntime is not defined (which is the case in Node). This means flushIfServerless returns without awaiting the flush and without registering with waitUntil. The code never reaches the serverless fallback that would await flushWithTimeout(timeout). While the SIGTERM handler provides a safety net at function termination, explicit calls to await flushIfServerless() (e.g., in framework integrations like astro, nuxt, react-router) won't actually wait for the flush to complete in Vercel Node runtime.
Additional Locations (1)
| return; | ||
| } | ||
|
|
||
| // Note: vercelWaitUntil only does something in Vercel Edge runtime |
There was a problem hiding this comment.
shouldn't this comment be in the other file vercelWaitUntil?
There was a problem hiding this comment.
there's a comment in both places 👍
This adds a better way to ensure we flush in node-based vercel functions.
Note that this does not work in edge functions, so we need to keep the
vercelWaitUntilcode around for this - but we can noop it in node and instead rely on the better way to handle this, I believe.@chargome when you're back, can you verify if this makes sense? 😅
Closes #17567