fix(cloudflare): Ensure every request is instruments the functions#20044
Open
fix(cloudflare): Ensure every request is instruments the functions#20044
Conversation
Contributor
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨Deps
Nuxt
Other
Bug Fixes 🐛Ci
Node
Other
Documentation 📚
Internal Changes 🔧Core
Deps
Deps Dev
Other
🤖 This preview updates automatically when you update the PR. |
JPeer264
force-pushed
the
jp/fix-instrumentation
branch
from
f306352 to
e99be8e
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
+25
to
+33
|
|
||
| /** | ||
| * Mark an object as instrumented, storing the instrumented version. | ||
| * @param original The original uninstrumented object | ||
| * @param instrumented The instrumented version (defaults to original if not provided) | ||
| */ | ||
| export function markAsInstrumented<T>(original: T, instrumented?: T): void { | ||
| try { | ||
| (obj as SentryInstrumented<T>).__SENTRY_INSTRUMENTED__ = true; | ||
| if (isWeakMapKey(original)) { |
There was a problem hiding this comment.
Bug: The construct trap reuses a cached instrumented proxy with a stale context when a Durable Object class is instantiated multiple times, corrupting telemetry.
Severity: CRITICAL
Suggested Fix
When wrapping methods within the construct trap, pass noMark = true to ensureInstrumented (or the equivalent in wrapMethodWithSentry). This will prevent caching the instrumented proxy and ensure a new wrapper with the correct instance-specific context is created for each new Durable Object instance.
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/instrument.ts#L25-L33
Potential issue: When a Durable Object class is instantiated multiple times, methods
instrumented in the `construct` trap (e.g., `alarm`, `webSocketMessage`) are wrapped
using `ensureInstrumented` without preventing caching. This causes the instrumented
proxy from the first instance, which has a closure over the initial `context` and
`options`, to be reused for all subsequent instances. As a result, errors and spans from
these later instances will be reported with the incorrect context of the first instance,
leading to corrupted monitoring data.
Did we get this right? 👍 / 👎 to inform future reviews.
![cursor[bot]](https://avatars.githubusercontent.com/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 prepared a fix for the issue found in the latest run.
- ✅ Fixed: DO lifecycle proxies cache stale context across instances
- Fixed by creating instance-specific proxies with noMark=true to prevent caching stale context and options in the global WeakMap, ensuring each DurableObject instance uses its own DurableObjectState.
Or push these changes by commenting:
@cursor push 44b2aefb35
Preview (44b2aefb35)
diff --git a/packages/cloudflare/src/durableobject.ts b/packages/cloudflare/src/durableobject.ts
--- a/packages/cloudflare/src/durableobject.ts
+++ b/packages/cloudflare/src/durableobject.ts
@@ -3,7 +3,7 @@
import type { DurableObject } from 'cloudflare:workers';
import { setAsyncLocalStorageAsyncContextStrategy } from './async';
import type { CloudflareOptions } from './client';
-import { ensureInstrumented, getInstrumented, markAsInstrumented } from './instrument';
+import { getInstrumented, markAsInstrumented } from './instrument';
import { getFinalOptions } from './options';
import { wrapRequestHandler } from './request';
import { instrumentContext } from './utils/instrumentContext';
@@ -68,38 +68,50 @@
// Any other public methods on the Durable Object instance are RPC calls.
if (obj.fetch && typeof obj.fetch === 'function') {
- obj.fetch = ensureInstrumented(
- obj.fetch,
- original =>
- new Proxy(original, {
- apply(target, thisArg, args) {
- return wrapRequestHandler({ options, request: args[0], context }, () => {
- return Reflect.apply(target, thisArg, args);
- });
- },
- }),
- );
+ const originalFetch = obj.fetch;
+ obj.fetch = new Proxy(originalFetch, {
+ apply(target, thisArg, args) {
+ return wrapRequestHandler({ options, request: args[0], context }, () => {
+ return Reflect.apply(target, thisArg, args);
+ });
+ },
+ });
+ markAsInstrumented(obj.fetch);
}
if (obj.alarm && typeof obj.alarm === 'function') {
- obj.alarm = wrapMethodWithSentry({ options, context, spanName: 'alarm' }, obj.alarm);
+ const originalAlarm = obj.alarm;
+ obj.alarm = wrapMethodWithSentry({ options, context, spanName: 'alarm' }, originalAlarm, undefined, true);
+ markAsInstrumented(obj.alarm);
}
if (obj.webSocketMessage && typeof obj.webSocketMessage === 'function') {
+ const originalWebSocketMessage = obj.webSocketMessage;
obj.webSocketMessage = wrapMethodWithSentry(
{ options, context, spanName: 'webSocketMessage' },
- obj.webSocketMessage,
+ originalWebSocketMessage,
+ undefined,
+ true,
);
+ markAsInstrumented(obj.webSocketMessage);
}
if (obj.webSocketClose && typeof obj.webSocketClose === 'function') {
- obj.webSocketClose = wrapMethodWithSentry({ options, context, spanName: 'webSocketClose' }, obj.webSocketClose);
+ const originalWebSocketClose = obj.webSocketClose;
+ obj.webSocketClose = wrapMethodWithSentry(
+ { options, context, spanName: 'webSocketClose' },
+ originalWebSocketClose,
+ undefined,
+ true,
+ );
+ markAsInstrumented(obj.webSocketClose);
}
if (obj.webSocketError && typeof obj.webSocketError === 'function') {
+ const originalWebSocketError = obj.webSocketError;
obj.webSocketError = wrapMethodWithSentry(
{ options, context, spanName: 'webSocketError' },
- obj.webSocketError,
+ originalWebSocketError,
(_, error) =>
captureException(error, {
mechanism: {
@@ -107,7 +119,9 @@
handled: false,
},
}),
+ true,
);
+ markAsInstrumented(obj.webSocketError);
}
for (const method of Object.getOwnPropertyNames(obj)) {This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
| }); | ||
| }, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
DO lifecycle proxies cache stale context across instances
Medium Severity
In the DurableObject construct handler, lifecycle methods like fetch, alarm, webSocketMessage, etc. that live on the prototype are passed to ensureInstrumented / wrapMethodWithSentry. The first construction creates a proxy that captures the instance-specific context (DurableObjectState) and options in its closure, then caches the mapping protoMethod → proxy in the global WeakMap. When a second instance of the same class is constructed, obj.fetch resolves to the same prototype function, ensureInstrumented finds the cached proxy, and returns it — with the first instance's stale context and options baked in. This means waitUntil and other context-dependent operations target the wrong DurableObjectState.
Additional Locations (1)
Contributor
size-limit report 📦
|
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
2 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.
closes #20030
closes JS-2020
Important: This issue is not reproducible locally, so it can only be done on production. I still added an integration test (which would also pass on
developnow), just in case this would be added to miniflare.The problem is that when we instrument the requests we mark them on the object with
__SENTRY_INSTRUMENTED__. This would work the very first time and for other requests. Sometimes, however, the worker kind of restarts its state, without resetting__SENTRY_INSTRUMENTED__- which means that at this point we do not instrument the new setfetchrequests (and other requests).To remove this issue we set a global
WeakMapand cache the instrumented function in there. So whenever this scenario happens, we do not check via the__SENTRY_INSTRUMENTED__on the object itself, but on the globalWeakMap. The benefit of this is, that even when theWeakMapwould removes its reference, we just instrument again. In the worst case if theWeakMaploses its reference, and for some reason in the futurefetchrequests keep its instrumentation in the given scenario, then we just overwritefetchwith a fresh instrumentation.This is being achieved by wrapping all functions in
ensureInstrumented:before:
after: