fix(amber, v1.2): report a malformed PVE websocket handshake - #7866
Open
github-actions[bot] wants to merge 1 commit into
Open
fix(amber, v1.2): report a malformed PVE websocket handshake#7866github-actions[bot] wants to merge 1 commit into
github-actions[bot] wants to merge 1 commit into
Conversation
Contributor
Author
|
The cherry-pick conflicted and was committed with conflict markers. Resolve the conflicts on this branch, then mark this PR ready for review. Conflicting files:
|
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
### What changes were proposed in this PR? `PveWebsocketResource.onOpen` read the handshake parameters in its prologue, outside the `Future` that owns both the `catch` arm that turns a failure into an `[ERR]` line and the pump that writes to the socket. A handshake missing `cuid`, carrying an empty value list for it, or carrying a non-numeric one threw out of `onOpen` itself, ahead of both — so the endpoint's own error contract could not see it: ``` Before: ?pveName=env&action=install -> NumberFormatException out of onOpen -> socket closes, nothing sent After: ?pveName=env&action=install -> [ERR] Missing required parameter: cuid -> __DONE__ ``` The client side makes that silent close worse than a lost message: `computing-unit-selection.component.ts` clears `isInstalling`/`isLocked` only when `onmessage` sees `__DONE__`, and there is no `onclose` handler, so the pip modal stayed locked on "installing" with an empty log until the user navigated away. The three reads move inside the `try`, so a malformed handshake now travels the same path as any other failure — an `[ERR]` line followed by the sentinel. A `requiredParam` helper rejects an absent key, an empty value list, and a blank value alike; the last one is reachable from the wire, since `?pveName=` arrives as `[""]` and used to resolve to a venv directory named `" "` rather than failing. | Handshake | Before | After | | --- | --- | --- | | no `cuid` | NPE out of `onOpen`, socket closes empty | `[ERR] Missing required parameter: cuid` then `__DONE__` | | `cuid` present, value list empty | IndexOutOfBoundsException, same | same as above | | `cuid=abc` | NumberFormatException, same | `[ERR] Invalid cuid: abc` then `__DONE__` | | `pveName=` (blank) | resolved a venv path with a whitespace name | `[ERR] Missing required parameter: pveName` then `__DONE__` | Well-formed handshakes are untouched: the parsed `cuid`/`pveName` reach `PveManager` exactly as before. ### Any related issues, documentation, discussions? Follows #7847, which added `PveWebsocketResourceSpec` and deliberately left this path unpinned so that fixing it would not have to fight a test that had cemented it. ### How was this PR tested? Four cases added to `PveWebsocketResourceSpec` — `cuid` absent, `cuid` with an empty value list, a non-numeric `cuid`, and a blank `pveName` — each asserting the client receives both the `[ERR]` line and the sentinel, and that the pump stops rather than parking in `queue.take()`. ```bash sbt "WorkflowExecutionService/testOnly org.apache.texera.web.resource.pythonvirtualenvironment.PveWebsocketResourceSpec" ``` `Tests: succeeded 7, failed 0`. Written before the fix: all four failed against the old source with the escaping exception itself (`NullPointerException`, `IndexOutOfBoundsException`, `NumberFormatException`) rather than an assertion mismatch, which is the defect stated as a test. Hoisting the reads back out of the `Future` fails all four again. Mutation testing on the new code — 4 mutants, 4 killed: dropping the `values == null` arm, dropping the `isEmpty` arm, hardcoding `cuid` into the message instead of interpolating the parameter name, and swallowing a bad `cuid` as `0`. `PveResourceSpec`, the other suite in the package, is unaffected — locally it reports 6 pre-existing failures that all come from the Windows interpreter path (`Scripts\python.exe`), unrelated to the endpoint. ```bash sbt scalafmtCheckAll "scalafixAll --check" ``` Clean. Backport notes -- two places where `release/v1.2` differs from the source commit's branch, so the cherry-pick did not apply as-is: - `PveManager.createNewPve`/`installUserPackages` still take an `isLocal` flag here, so `val isLocal = !KubernetesConfig.kubernetesComputingUnitEnabled` stays in `onOpen`'s prologue rather than moving into the `try` with the other three reads. It reads configuration, not the handshake, so none of the failure modes this commit fixes can originate from it, and the endpoint's error contract is unchanged by where it sits. - The spec's `pythonBinFor` helper drops the source commit's Windows `Scripts/python.exe` arm. This branch's `PveManager.pythonBinPath` resolves `bin/python` unconditionally -- the platform split is a later change to `PveManager` that this backport does not carry -- so keeping the arm would assert an interpreter path the endpoint under test cannot produce. ### Was this PR authored or co-authored using generative AI tooling? (backported from commit c9a0ddf) Generated-by: Claude Code (Opus 5)
aglinxinyuan
force-pushed
the
backport/7852-report-a-malformed-pve-websocket-handsha-v1.2
branch
from
August 23, 2026 05:26
f5bd718 to
11b6c3d
Compare
aglinxinyuan
marked this pull request as ready for review
August 23, 2026 05:26
Contributor
Author
Automated Reviewer SuggestionsBased on the
|
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
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?
Automated backport of #7852 to
release/v1.2.Source: c9a0ddf · automation run
Any related issues, documentation, discussions?
Backport of #7852.
How was this PR tested?
Release-branch CI runs on this branch once the conflicts are resolved and this PR is marked ready for review.
Was this PR authored or co-authored using generative AI tooling?
No.