fix(cloudflare): Wait for span links to be set#21167
Conversation
JPeer264
requested review from
andreiborza and
mydea
and removed request for
a team
![cursor[bot]](https://ghub.bbstale.com/avatars/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ce76d4b. Configure here.
| }, | ||
| (e: unknown) => { | ||
| async (e: unknown) => { | ||
| await awaitLink(); |
There was a problem hiding this comment.
Sync handlers skip link await
Medium Severity
For startNewTrace handlers that return synchronously or throw synchronously, the stored trace link is never awaited and is no longer registered with waitUntil. The span can end before getStoredSpanContext sets sentry.previous_trace, so alarm-style linking can fail for sync alarm implementations.
Reviewed by Cursor Bugbot for commit ce76d4b. Configure here.
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
I think the clanker's objection is valid, unless it's impossible to have a link promise and a non-then-able result. Might be good to add a test showing that case, if possible.
If I'm misreading it, happy to update to 👍
| }, | ||
| ); | ||
| } else { | ||
| waitUntil?.(teardown()); |
There was a problem hiding this comment.
If I understand the clanker's objection, seems like there should be a waitIUntil here to handle the linkPromise if the result is not thenable?
2 participants
This came up in #21101 where the link was not set in time and the tests failed. With this PR we are waiting for the links to be actually set, before the rootspan ends. Retrieving the stored span shouldn't take too long - and the work was also before already executed during the response.