feat(workflow): Automatically cleanup AsyncLocalStorage on workflow context disposal

[mjameswh]

What was changed and why

• Track instances of AsyncLocalStorage created inside the workflow sandbox and automatically clean them up on workflow context disposal, after execution of the dispose hook on internal interceptors.

  • Proper use of AsyncLocalStorage was difficult due to the necessity of cleaning them up from an internal interceptor's dispose hook (fixes [Feature Request] Simplify proper usage of AsyncLocalStorage in Workflow context #1432).

• The AsyncLocalStorage instances created by the SDK to track cancellation and update scopes are now automatically cleaned up at the sandbox vm destruction time.

  • PR 1834 added a workflowModule.dispose() call during cleanup of the reusable VM workflow, but that resulted in a regression in 1.14.0 where a workflow might end up getting unexpectedly cancelled if evicted from cache while blocked on a condition(fn, timeout) (see investigation details below). Fixes [Bug] Signal caused condition to fail with CancelledFailure on 1.14.0 #1866.

    • condition with timeout creates a CancellationScope to perform the race between the sleep and the blocking condition src
    • On completion, the current CancellationScope is canceled
    • Current CancellationScope is stored in a AsyncLocalStorage which falls back to the workflow cancellation scope if not in a context where there is an active store. src
    • 1834 added a workflowModule.dispose() call to the dispose method for reusable VM workflow
    • This in turn disables the AsyncLocalStorage that stores the cancellation tokens (src). Any subsequent getStore calls that happen before a new run will return undefined.
    • If a workflow ends up getting disposed after the condition scope is created, but before the scope is cancelled, then the workflow ends up get cancelled instead of the condition scope.
    • This lines up from the trace provided by the user on 1866 which is a CancelledFailure: Workflow cancelled, but coming from the cancel inside of the condition scope on this line

• Properly dispose a workflow execution context on early failure (e.g. if requested workflow type doesn't resolve to an exported workflow function).

  • We do not add the workflow execution context to the lang-side workflow cache if it failed during early initialization. It is therefore not possible to clean it up at a later time, i.e. on cache eviction. This could previously cause a memory leak if some some non-garbage-collectible resources (e.g. AsyncLocalStorage) had been allocated during early initialization. We now do ensure that dispose is properly called in that scenario.

Checklist

1. How was this tested:

• Added failing integration test in first commit, second commit fixes the test.
• Actual disposal of AsyncLocalStorage lifecycle can't really be asserted through automated testing. We temporarily added console.log calls inside our mocked ALS class to confirm that dispose() is called at the correct moment in various scenarios.

2. Any docs updates needed?

• We should consider documenting the use of AsyncLocalStorage in workflow context.

[chris-olszewski]

Overall makes sense to me.

Just a few small questions.

[chris-olszewski]

Are there any additional assertions that would be valuable here aside from "it doesn't throw"?

[mjameswh]

I played around with the possibility of observing the destruction of ALS by escaping the sandbox to create a FinalizationRegistry, which worked. The problem, though, is that I couldn't get that test to fail with the previous code, so that's not proving anything.

With that in mind, and given the complexity and fragility of that test, I decided not to commit it.

And I really can't think of any other side effect to look for.

[chris-olszewski]

If both of these actually disable the ALS, do we need both?

[mjameswh]

Yeah, I was hesitant about whether we want the "normal" disable() function to work properly, or if it should be a noop. I'm now leading for the former, so I'll delete the __temporal one.

[chris-olszewski]

👍 on having it function properly

[chris-olszewski]

Looking into how the lint violation got through CI and why it isn't showing up locally, but fix is in #1875

[mjameswh]

Looking into how the lint violation got through CI and why it isn't showing up locally, but fix is in #1875

Ah, yeah, I was also looking at that just now. Let's merge your dep upgrade pr first.