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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
