Uh oh!
There was an error while loading. Please reload this page.
fix(cloudflare): Wait for span links to be set - #21167
Conversation
Uh oh!
There was an error while loading. Please reload this page.
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.
Uh oh!
There was an error while loading. Please reload this page.
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
👋 @mydea, @andreiborza — Please review this PR when you get a chance! |
👋 @isaacs — Please review this PR when you get a chance! |
isaacs
left a comment
There was a problem hiding this comment.
Looks good to me! If you think the clanker's objection is valid, could address it with a try/catch, but that's only important if we care to support versions that don't have the API in use here.
| originalStorage.kv.put(getTraceLinkKey(methodName), storedContext); | ||
| } | ||
| } catch (error) { | ||
| // Silently ignore storage errors to prevent internal failures from affecting user code |
There was a problem hiding this comment.
Bug: The code assumes originalStorage.kv exists, but it is undefined for legacy KV-backed Durable Objects. This causes trace linking to silently fail for those users.
Severity: MEDIUM
Suggested Fix
Add a guard to check for the existence of originalStorage.kv before attempting to use its methods. This will prevent silent errors and the loss of tracing functionality for users on legacy KV-backed Durable Objects. If backward compatibility is required, consider adding a fallback to the previous asynchronous storage mechanism.
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/cloudflare/src/utils/traceLinks.ts#L39-L43
Potential issue: The functions `storeSpanContext` and `getStoredSpanContext` were
updated to use the synchronous KV API via `originalStorage.kv`. However, for legacy
KV-backed Durable Objects, the `originalStorage.kv` property is `undefined`. This leads
to a `TypeError` when `put` or `get` is called. Although the error is caught by a
`try-catch` block, preventing a crash, it results in a silent failure. Consequently,
span context is not stored or retrieved for these legacy durable objects, which breaks
distributed tracing continuity for that specific, supported configuration.
Did we get this right? 👍 / 👎 to inform future reviews.
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.