fix(frontend): unsubscribe connection-status subscription in WorkflowWebsocketService.closeWebsocket#4377
Merged
mengw15 merged 5 commits intoapache:mainfrom Apr 17, 2026
Conversation
Contributor
|
@mengw15 please review it. |
Contributor
|
@Ma77Ball Please review it as well. |
mengw15
requested changes
Apr 14, 2026
Contributor
mengw15
left a comment
mengw15
left a comment
There was a problem hiding this comment.
Thanks for the PR! Nice catch! Left some comments.
mengw15
reviewed
Apr 16, 2026
…WebsocketService.closeWebsocket `openWebsocket` subscribed to `websocketEvent()` to track cluster status and connection health, but did not store the returned `Subscription`. Because `closeWebsocket` never called `unsubscribe()` on it, each successive call to `openWebsocket` (e.g. when the user switches computing units or workflows) added another live subscriber to the shared `webSocketResponseSubject`, causing: * a growing number of duplicate `updateConnectionStatus(true)` calls per event, * repeated `numWorkers` assignments from stale handler closures, and * a memory leak proportional to the number of websocket reconnections in the session. Fix: store the subscription in the new `statusUpdateSubscription` field (typed as `Subscription | undefined`) and unsubscribe it at the start of `closeWebsocket`, alongside the existing `wsWithReconnectSubscription` cleanup. The `?.unsubscribe()` safe-navigation operator makes the call a no-op when the field is still `undefined` (e.g. `closeWebsocket` is called before the socket was ever opened). Two new unit tests in `workflow-websocket.service.spec.ts` verify that the subscription is closed after `closeWebsocket` and that repeated closes do not throw. Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
officialasishkumar
force-pushed
the
fix/websocket-status-subscription-leak
branch
from
320eef3 to
eeab1c6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this PR?
WorkflowWebsocketService.openWebsocketsubscribed to the internalwebSocketResponseSubjectobservable to track cluster status and mark theconnection as live, but did not store the returned
Subscription. As aresult,
closeWebsocketcould not unsubscribe it, causing the handler toaccumulate on every call to
openWebsocket(e.g. when the user switchesworkflows or computing units).
Root cause (before):
Fix (after):
The new
statusUpdateSubscriptionfield is declared asSubscription | undefined;the
?.unsubscribe()call is safely a no-op whencloseWebsocketis called beforeopenWebsocketwas ever invoked.Any related issues, documentation, discussions?
Closes #4376
How was this PR tested?
Two new unit tests were added to
workflow-websocket.service.spec.ts:"should close the previous status subscription when openWebsocket is called again" uses a lightweight WebSocket test double, calls openWebsocket() twice and closeWebsocket() once, and verifies that the previous statusUpdateSubscription is torn down on each reopen and on close
The existing service-creation test continues to pass.
Was this PR authored or co-authored using generative AI tooling?
No.