You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@texera.apache.org by "aglinxinyuan (via GitHub)" <gi...@apache.org> on 2026/08/22 23:53:22 UTC
[PR] fix(amber): report a malformed PVE websocket handshake [texera]
aglinxinyuan opened a new pull request, #7852:
URL: https://github.com/apache/texera/pull/7852
### 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.
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(amber): report a malformed PVE websocket handshake [texera]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #7852:
URL: https://github.com/apache/texera/pull/7852#issuecomment-5383267818
## [Codecov](https://app.codecov.io/gh/apache/texera/pull/7852?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 91.96%. Comparing base ([`ce0510d`](https://app.codecov.io/gh/apache/texera/commit/ce0510d32cdf654184a6d3d27ce9da11e2bf4bc6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`63016f4`](https://app.codecov.io/gh/apache/texera/commit/63016f418501366be8b65f6e5b045c9b61c6416d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ 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
```
| [Flag](https://app.codecov.io/gh/apache/texera/pull/7852/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | *Carryforward flag |
|---|---|---|---|
| [access-control-service](https://app.codecov.io/gh/apache/texera/pull/7852/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `81.00% <ø> (ø)` | | Carriedforward from [ce0510d](https://app.codecov.io/gh/apache/texera/commit/ce0510d32cdf654184a6d3d27ce9da11e2bf4bc6?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [agent-service](https://app.codecov.io/gh/apache/texera/pull/7852/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `98.62% <ø> (ø)` | | Carriedforward from [ce0510d](https://app.codecov.io/gh/apache/texera/commit/ce0510d32cdf654184a6d3d27ce9da11e2bf4bc6?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [amber](https://app.codecov.io/gh/apache/texera/pull/7852/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `88.41% <100.00%> (-0.04%)` | :arrow_down: | |
| [computing-unit-managing-service](https://app.codecov.io/gh/apache/texera/pull/7852/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `73.67% <ø> (ø)` | | Carriedforward from [ce0510d](https://app.codecov.io/gh/apache/texera/commit/ce0510d32cdf654184a6d3d27ce9da11e2bf4bc6?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [config-service](https://app.codecov.io/gh/apache/texera/pull/7852/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `86.73% <ø> (ø)` | | Carriedforward from [ce0510d](https://app.codecov.io/gh/apache/texera/commit/ce0510d32cdf654184a6d3d27ce9da11e2bf4bc6?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [file-service](https://app.codecov.io/gh/apache/texera/pull/7852/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `75.74% <ø> (ø)` | | Carriedforward from [ce0510d](https://app.codecov.io/gh/apache/texera/commit/ce0510d32cdf654184a6d3d27ce9da11e2bf4bc6?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [frontend](https://app.codecov.io/gh/apache/texera/pull/7852/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `93.99% <ø> (ø)` | | Carriedforward from [ce0510d](https://app.codecov.io/gh/apache/texera/commit/ce0510d32cdf654184a6d3d27ce9da11e2bf4bc6?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [notebook-migration-service](https://app.codecov.io/gh/apache/texera/pull/7852/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `79.13% <ø> (ø)` | | Carriedforward from [ce0510d](https://app.codecov.io/gh/apache/texera/commit/ce0510d32cdf654184a6d3d27ce9da11e2bf4bc6?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [pyamber](https://app.codecov.io/gh/apache/texera/pull/7852/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `97.52% <ø> (ø)` | | Carriedforward from [ce0510d](https://app.codecov.io/gh/apache/texera/commit/ce0510d32cdf654184a6d3d27ce9da11e2bf4bc6?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [workflow-compiling-service](https://app.codecov.io/gh/apache/texera/pull/7852/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `77.19% <ø> (ø)` | | Carriedforward from [ce0510d](https://app.codecov.io/gh/apache/texera/commit/ce0510d32cdf654184a6d3d27ce9da11e2bf4bc6?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
*This pull request uses carry forward flags. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/texera/pull/7852?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(amber): report a malformed PVE websocket handshake [texera]
Posted by "aglinxinyuan (via GitHub)" <gi...@apache.org>.
aglinxinyuan merged PR #7852:
URL: https://github.com/apache/texera/pull/7852
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(amber): report a malformed PVE websocket handshake [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7852:
URL: https://github.com/apache/texera/pull/7852#issuecomment-5384284283
Backport PR opened: draft #7866 (https://github.com/apache/texera/pull/7866) to `release/v1.2`, assigned to @aglinxinyuan — needs manual work because the cherry-pick conflicts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(amber): report a malformed PVE websocket handshake [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7852:
URL: https://github.com/apache/texera/pull/7852#issuecomment-5383236260
<!-- texera-reviewer-suggestion -->
### Automated Reviewer Suggestions
Based on the `git blame` history of the changed files, we recommend the following reviewers:
- **Contributors with relevant context:** `@SarahAsad23`
You can notify them by mentioning `@SarahAsad23` in a comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(amber): report a malformed PVE websocket handshake [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7852:
URL: https://github.com/apache/texera/pull/7852#issuecomment-5383258579
<!-- texera-benchmarks-comment -->
## ⚠️ Benchmark changes need a look
🟢 2 better · 🔴 7 worse · ⚪ 6 noise (<±5%) · 0 without baseline
> Compared against main `ce0510d` benchmarked on **this same runner**, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.
[Dashboard](https://apache.github.io/texera/dev/bench/) · [Run](https://github.com/apache/texera/actions/runs/32606429097)
| | 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% |
<details><summary>Baseline details</summary>
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% |
</details>
<details><summary>Raw CSV</summary>
```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
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(amber): report a malformed PVE websocket handshake [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7852:
URL: https://github.com/apache/texera/pull/7852#issuecomment-5383236369
<!-- backport-auto-label-report -->
### Backport auto-label report
This `fix:` PR was checked against each actively-supported release branch. `release/*` labels drive the post-merge backport, so add or remove one to change where this fix lands.
| Release branch | Analysis |
| --- | --- |
| ✅ `release/v1.2` | Change detected on this branch — label added; this fix is queued to backport here. Requested review from @xuang7. |
_[Auto-label run](https://github.com/apache/texera/actions/runs/32606419272)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org