[sdk/nodejs] Account for outstanding async work done in prepareResource
#7704
Conversation
| @@ -563,6 +569,9 @@ async function prepareResource(label: string, res: Resource, custom: boolean, re | |||
| } | |||
| } | |||
|
|
|||
| // free the RPC queue | |||
| done(); | |||
There was a problem hiding this comment.
Don't we need to put this in a finally to ensure it runs even if there are errors? It looks like all other places we use rpcKeepAlive do that?
Also - this pattern seems fundamentally somewhat flimsy. This may be a necessary patch to take - but I expect we'll need to revisit whether this approach overall is the right foundation for ensuring we track all async work correctly.
There was a problem hiding this comment.
Good catch. Follow up here: #7707
this pattern seems fundamentally somewhat flimsy
Agree, I called this out in slack. Unfortunately our current architecture forces us to account for async work as we allow reusing the process across multiple automation api updates. Open to ideas or brainstorming here. Perhaps we could move some of this accounting into the debuggablePromise implementation, but that just sort of shifts the problem around.
Fixes #6998
Turns out we weren't adding
prepareResourcework the the keep-alive queue. We were waiting untilrunAsyncResourceOpwhich happens in a.thenchained off ofprepareResource. The automation api shutdown routine,runtime.disconnect, was unaware of this outstanding work as a result.