-
Notifications
You must be signed in to change notification settings - Fork 113
fix(gui): reopen workflow websocket when switching workflows #3773
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a bug where workflow execution history was incorrectly attributed to the previous workflow when users switched workflows without changing computing units. The websocket connection was not being reopened when only the workflow ID changed.
- Added tracking of workflow ID (
wid) in addition to computing unit ID (cuid) to detect when websocket reconnection is needed - Modified the reconnection logic to trigger when either
widorcuidchanges - Added cleanup of cached identifiers when computing units are deselected or terminated
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
core/gui/src/app/workspace/service/computing-unit-status/computing-unit-status.service.ts
Outdated
Show resolved
Hide resolved
|
@seongjinyoon Thanks for fixing the issue. Can you recommend 1-2 reviewers? |
I have asked @kunwp1 to review the code. |
|
I also assigned @Xiao-zhen-Liu as a reviewer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed the PR and left a few clarification questions. If you can address them, please add comments to the code. Also, can you update the comments of the registerWorkflowMetadataSubscription and the PR description to replace onComputingUnitChange with selectComputingUnit?
core/gui/src/app/workspace/service/computing-unit-status/computing-unit-status.service.ts
Outdated
Show resolved
Hide resolved
core/gui/src/app/workspace/service/computing-unit-status/computing-unit-status.service.ts
Outdated
Show resolved
Hide resolved
core/gui/src/app/workspace/service/computing-unit-status/computing-unit-status.service.ts
Outdated
Show resolved
Hide resolved
|
It looks good to me now. Well done! @Xiao-zhen-Liu Can you review the PR? |
Xiao-zhen-Liu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Description:
Ensure workflow executions are always attributed to the correct workflow even when reusing a computing unit across multiple workflows.
Closes #3759
Problem:
When users switch workflows without changing the computing unit, the frontend keeps the existing WebSocket session so the backend continues to use the previous wid and execution history is midfiled.
In Texera, a WebSocket connection requires
widandcuid. When we switch to a different workflow, we callregisterWorkflowMetadataSubscription(). This function callsonComputingUnitChange()whenwidchanges, butonComputingUnitChange()only checks for changes incuidwithout checkingwidchanges.Solution:
Have the Angular computing-unit status service track the
widandcuid, forcing the websocket to reconnect whenever either of them change and clear the cached identifiers when the selection disappears or is terminated.Additionally, fixed typo in
core/gui/src/app/workspace/component/power-button/computing-unit-selection.component.ts