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://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 👍
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?
There was a problem hiding this comment.
whoopsie. That was actually a good catch. I actually wanted to go away from waitUntil, as with that it is a little unreliable if a trace link is created or not.
I'll change the logic a little so we don't have duplicated code for sync and async returns.
There was a problem hiding this comment.
Ok I had to change it to a sync KV update to make it work for sync alarms. I also didn't want to return a promise for sync alarms, as this would change the users method to be a promise.
I now added also a sync alarm test as proof.
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 3d9cf8c. Configure here.
| return undefined; | ||
| } | ||
| ): StoredSpanContext | undefined { | ||
| return originalStorage.kv.get<StoredSpanContext>(getTraceLinkKey(methodName)); |
There was a problem hiding this comment.
getStoredSpanContext lost error handling, may crash user code
Medium Severity
getStoredSpanContext had its try-catch removed in this refactor, but storeSpanContext still has one. If originalStorage.kv is undefined (e.g., on legacy KV-backed Durable Objects where the sync KV API isn't available), calling originalStorage.kv.get(...) throws a TypeError. The caller in wrapMethodWithSentry.ts invokes getStoredSpanContext inside the startSpan callback but outside the try-catch block that wraps the user's handler, so the error propagates through handleCallbackErrors which re-throws, crashing the user's alarm handler with a Sentry-internal error.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 3d9cf8c. Configure here.
size-limit report 📦
|
| return res; | ||
| }; | ||
|
|
||
| const onRejected = (e: unknown) => { |
There was a problem hiding this comment.
note: I outsourced that since it was used 4 times
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.