Revert "feat(cloudflare): Introduce lock instrumentation for context.waitUntil to prevent multiple instrumentation (#17539)"
#17666
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| return allDone; | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Bug: Instrumentation Overlap Causes Memory Leaks
The makeFlushLock function doesn't prevent multiple instrumentation of the same ExecutionContext. Calling it repeatedly on the same context creates nested wrappers around context.waitUntil, which leads to incorrect pending task counts. This can cause memory leaks and prevent the flush lock from resolving correctly.
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
| context.waitUntil = promise => { | ||
| pending++; | ||
| return originalWaitUntil( | ||
| promise.finally(() => { | ||
| if (--pending === 0) resolveAllDone(); | ||
| }), | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Potential bug: The makeFlushLock function overwrites the native context.waitUntil method. Other parts of the code attempting to bind to this new function will fail, causing an "Illegal invocation" error.
-
Description: The
makeFlushLockfunction directly mutates the originalExecutionContextby overwriting the nativewaitUntilmethod with a JavaScript wrapper. Elsewhere in the code,context.waitUntil.bind(context)is called. Attempting to bind the new JavaScript function, which wraps the original native method, breaks thethiscontext required by the Cloudflare Workers runtime. This results in aTypeError: Illegal invocationwhenwaitUntilis called, causing the worker to crash. The removal of thecopyExecutionContextutility exacerbates this by ensuring multiple handlers operate on the same mutated context object. -
Suggested fix: Instead of mutating the original
contextobject,makeFlushLockshould return a newwaitUntilfunction and other related properties. The caller can then use this new object without affecting the global context. Alternatively, restore a mechanism similar to the removedcopyExecutionContextto provide isolated, correctly-bound contexts to different parts of the application.
severity: 0.85, confidence: 0.95
Did we get this right? 👍 / 👎 to inform future reviews.
3 participants
This PR reverts #17539 because it causes e2e test fails on PRs as well as on
developand release branches.It seems like when our e2e tests expect errors in websocket, another unrelated error is thrown:
The SDK catches this error instead but the tests fail because they expect the other error. Not sure what in #17539 caused this but for now let's unblock ourselves first and follow up with a fix in a new attempt (cc @0xbad0c0d3 )