-
Notifications
You must be signed in to change notification settings - Fork 156
fix: reusable-vm runs dispose interceptors #1834
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.
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
fix: reusable-vm runs dispose interceptors #1834
Conversation
call the workflow interface dispose method, same as in github.com/temporalio/sdk-typescript/blob/main/packages/worker/src/workflow/vm.ts#L127-L130 otherwise memory leaks occur when `AsyncLocalStorage` is used. from my testing we re-evaluate modules on every workflow run, even when in the `ReusableVMWorkflow` see also: temporalio#1432
99b413e to
3c20e67
Compare
|
Yeah, that definitely seems right. I'm totally baffled this bug didn’t get exposed earlier. Thanks a lot @mnahkies for your work in investigating this. |
|
Thanks for adding the test @THardy98 and getting this merged 🎉 Thought I'd just drop some runtime metrics showing the impact of this change for one of our workers that is under a roughly constant load of workflows, tldr; it is much more stable
Previously it was OOMing frequently and restarting, with performance degrading up until the point it restarted. This has also decreased the end-to-end duration of the workflows processed by this worker, roughly 2-3x improvement. |

What was changed
call the workflow interface dispose method, same as in github.com/temporalio/sdk-typescript/blob/main/packages/worker/src/workflow/vm.ts#L127-L130
see also: #1432
Why?
otherwise memory leaks occur when
AsyncLocalStorageis used, as interceptors likesdk-typescript/packages/interceptors-opentelemetry/src/workflow/index.ts
Lines 230 to 236 in 34ff54b
from my testing we re-evaluate modules on every workflow run, even when in the
ReusableVMWorkflowis used. meaning eg: module level constAsyncLocalStorageinstances are re-created for each workflow, and so it's important they are disposed of.Checklist
Tested manually with a minimal example of our real workload. Pending testing with our production workflow code.