fix(amber): report a malformed PVE websocket handshake - #7852
Merged
aglinxinyuan merged 1 commit intoAug 23, 2026
Conversation
onOpen read cuid and pveName in its prologue, outside the Future that owns the catch arm and the pump. A handshake with no cuid, an empty value list for it, or a non-numeric one therefore threw out of onOpen itself, ahead of both: no [ERR] line, no __DONE__ sentinel, just a socket that closes. The frontend clears isInstalling only on the sentinel in onmessage and has no onclose handler, so the pip modal stayed locked on "installing" with an empty log. The reads move inside the try, and a requiredParam helper rejects an absent key, an empty value list and a blank value alike: ?pveName= reaches the endpoint as [""], which used to resolve to a venv directory named " ". Four cases added to PveWebsocketResourceSpec, each failing against the old code with the escaping exception rather than an assertion mismatch. 4 mutations, 4 killed: dropping either arm of the presence guard, hardcoding cuid into the message instead of interpolating the parameter name, and swallowing a bad cuid as 0. Hoisting the reads back out of the Future fails all four tests.
Contributor
Automated Reviewer SuggestionsBased on the
|
Contributor
Backport auto-label reportThis
|
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 379 | 0.231 | 26,325/30,726/30,726 us | 🔴 +18.2% / 🔴 +121.9% |
| 🔴 | bs=100 sw=10 sl=64 | 780 | 0.476 | 125,132/170,955/170,955 us | 🔴 +16.8% / 🔴 +71.7% |
| 🟢 | bs=1000 sw=10 sl=64 | 927 | 0.566 | 1,074,635/1,135,577/1,135,577 us | 🟢 -5.8% / 🔴 +19.2% |
Baseline details
Latest main ce0510d from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 379 tuples/sec | 418 tuples/sec | 833.79 tuples/sec | -9.3% | -54.5% |
| bs=10 sw=10 sl=64 | MB/s | 0.231 MB/s | 0.255 MB/s | 0.509 MB/s | -9.4% | -54.6% |
| bs=10 sw=10 sl=64 | p50 | 26,325 us | 22,270 us | 11,864 us | +18.2% | +121.9% |
| bs=10 sw=10 sl=64 | p95 | 30,726 us | 30,349 us | 14,381 us | +1.2% | +113.7% |
| bs=10 sw=10 sl=64 | p99 | 30,726 us | 30,349 us | 18,035 us | +1.2% | +70.4% |
| bs=100 sw=10 sl=64 | throughput | 780 tuples/sec | 827 tuples/sec | 1,083 tuples/sec | -5.7% | -28.0% |
| bs=100 sw=10 sl=64 | MB/s | 0.476 MB/s | 0.505 MB/s | 0.661 MB/s | -5.7% | -28.0% |
| bs=100 sw=10 sl=64 | p50 | 125,132 us | 119,434 us | 93,077 us | +4.8% | +34.4% |
| bs=100 sw=10 sl=64 | p95 | 170,955 us | 146,336 us | 99,553 us | +16.8% | +71.7% |
| bs=100 sw=10 sl=64 | p99 | 170,955 us | 146,336 us | 108,604 us | +16.8% | +57.4% |
| bs=1000 sw=10 sl=64 | throughput | 927 tuples/sec | 917 tuples/sec | 1,119 tuples/sec | +1.1% | -17.1% |
| bs=1000 sw=10 sl=64 | MB/s | 0.566 MB/s | 0.56 MB/s | 0.683 MB/s | +1.1% | -17.1% |
| bs=1000 sw=10 sl=64 | p50 | 1,074,635 us | 1,078,873 us | 909,247 us | -0.4% | +18.2% |
| bs=1000 sw=10 sl=64 | p95 | 1,135,577 us | 1,205,047 us | 952,561 us | -5.8% | +19.2% |
| bs=1000 sw=10 sl=64 | p99 | 1,135,577 us | 1,205,047 us | 985,186 us | -5.8% | +15.3% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,527.56,200,128000,379,0.231,26325.39,30726.09,30726.09
1,100,10,64,20,2562.74,2000,1280000,780,0.476,125132.04,170955.37,170955.37
2,1000,10,64,20,21586.02,20000,12800000,927,0.566,1074635.41,1135576.99,1135576.99
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7852 +/- ##
============================================
- Coverage 91.98% 91.96% -0.02%
Complexity 4510 4510
============================================
Files 1173 1173
Lines 47346 47347 +1
Branches 5305 5306 +1
============================================
- Hits 43550 43544 -6
- Misses 2155 2159 +4
- Partials 1641 1644 +3
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
Backport PR opened: draft #7866 (#7866) to |
aglinxinyuan
added a commit
that referenced
this pull request
Aug 23, 2026
### 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)
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?
PveWebsocketResource.onOpenread the handshake parameters in its prologue, outside theFuturethat owns both thecatcharm that turns a failure into an[ERR]line and the pump that writes to the socket. A handshake missingcuid, carrying an empty value list for it, or carrying a non-numeric one threw out ofonOpenitself, ahead of both — so the endpoint's own error contract could not see it:The client side makes that silent close worse than a lost message:
computing-unit-selection.component.tsclearsisInstalling/isLockedonly whenonmessagesees__DONE__, and there is noonclosehandler, 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. ArequiredParamhelper 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.cuidonOpen, socket closes empty[ERR] Missing required parameter: cuidthen__DONE__cuidpresent, value list emptycuid=abc[ERR] Invalid cuid: abcthen__DONE__pveName=(blank)[ERR] Missing required parameter: pveNamethen__DONE__Well-formed handshakes are untouched: the parsed
cuid/pveNamereachPveManagerexactly as before.Any related issues, documentation, discussions?
Follows #7847, which added
PveWebsocketResourceSpecand 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—cuidabsent,cuidwith an empty value list, a non-numericcuid, and a blankpveName— each asserting the client receives both the[ERR]line and the sentinel, and that the pump stops rather than parking inqueue.take().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 theFuturefails all four again.Mutation testing on the new code — 4 mutants, 4 killed: dropping the
values == nullarm, dropping theisEmptyarm, hardcodingcuidinto the message instead of interpolating the parameter name, and swallowing a badcuidas0.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.sbt scalafmtCheckAll "scalafixAll --check"Clean.
Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)