Skip to content

fix(cloudflare): Try/catch on non-configurable prototypes #23132

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JPeer264 merged 1 commit into from
Aug 7, 2026
+42 −1
Merged

fix(cloudflare): Try/catch on non-configurable prototypes #23132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion packages/cloudflare/src/durableobject.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,11 @@ function instrumentPrototypeRpcMethods(obj: object, excludedMethods?: ReadonlySe
}

const wrapped = createRpcPrototypeWrapper(methodName, descriptor.value as UncheckedMethod);
Object.defineProperty(prototype, methodName, { ...descriptor, value: wrapped });

try {
Object.defineProperty(prototype, methodName, { ...descriptor, value: wrapped });
} catch {}

// Only the wrapper is marked, not the original method: `wrapMethodWithSentry` resolves
// through the same global map and must not resolve the original to this wrapper,
// which would recurse.
Comment on lines +331 to 337

@sentry sentry Bot Aug 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The code repeatedly tries to instrument non-configurable methods on each new class instance because it doesn't mark them as "failed to wrap" after the first attempt.
Severity: LOW

Suggested Fix

To prevent redundant work, the original method should be marked as having had an instrumentation attempt, even if it fails. This could be achieved by marking the original descriptor.value within the catch block after Object.defineProperty throws, preventing getRpcMethodDescriptor from re-selecting it for wrapping on subsequent constructions.

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/durableobject.ts#L331-L337

Potential issue: When a class has a non-configurable method, the instrumentation logic
attempts to wrap it using `Object.defineProperty`. This fails as expected, but the
original method is not marked to prevent future wrapping attempts. Consequently, for
every new instance of the class, the system re-attempts the same failed wrapping
operation. This results in minor performance overhead due to the creation of a new
closure and a doomed `defineProperty` call on each construction. The application's
functionality is not affected, but it introduces redundant work.

Did we get this right? 👍 / 👎 to inform future reviews.

Expand Down
37 changes: 37 additions & 0 deletions packages/cloudflare/test/durableobject.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,43 @@ describe('instrumentDurableObjectWithSentry', () => {
expect(obj.rpcMethod()).toBe('result');
});

it('skips non-configurable prototype methods instead of failing construction', () => {
const testClass = class {
sealedMethod() {
return 'sealed-result';
}

rpcMethod() {
return 'rpc-result';
}
};
Object.defineProperty(testClass.prototype, 'sealedMethod', {
value: testClass.prototype.sealedMethod,
writable: false,
enumerable: false,
configurable: false,
});
const originalSealedMethod = testClass.prototype.sealedMethod;

const instrumented = instrumentDurableObjectWithSentry(
vi.fn().mockReturnValue({ enableRpcTracePropagation: true }),
testClass as any,
);

let obj: any;
expect(() => {
obj = Reflect.construct(instrumented, []);
}).not.toThrow();

// The non-configurable method keeps its original (unwrapped) implementation
expect(testClass.prototype.sealedMethod).toBe(originalSealedMethod);
expect(obj.sealedMethod()).toBe('sealed-result');

// Other methods on the same prototype are still wrapped
expect(getInstrumented(obj.rpcMethod)).toBeTruthy();
expect(obj.rpcMethod()).toBe('rpc-result');
});

it('does not wrap Object.prototype methods as RPC methods', () => {
const testClass = class {
rpcMethod() {
Expand Down
Loading