The GitHub Actions job "Backport Approval Check" on texera.git/gh-readonly-queue/main/pr-7802-61cb36eab900b694bc00c45e5dd8eebb47266d82 has succeeded. Run started by GitHub user aglinxinyuan (triggered by aglinxinyuan).
Head commit for run: 53ad65021b875e025d23b643f5f02f560afc8492 / Xinyuan Lin <[email protected]> fix(amber): report an uninitialized execution instead of NPEing, and map unparseable frames (#7802) ### What changes were proposed in this PR? Three defects in `WorkflowWebsocketResource`, all previously characterized without being cemented (#7303, #7676) so a fix would not have to fight a test asserting the broken behaviour. **1 & 2. A workflow with no execution NPEs instead of reporting "not initialized".** `WorkflowService.executionService` is a `BehaviorSubject` with **no initial value** (`WorkflowService.scala:140`), so `getValue` is `null` until an execution is published. - `case other =>` used `workflowStateOpt.map(_.executionService.getValue)`, which wraps that null into `Some(null)` — walking past the `case None` arm that exists to report the friendly error, then NPEing on `value.wsInput`. Now uses the already-computed `executionStateOpt`, which is built with `Option(...)`, so `Some(null)` cannot form and the existing `None` arm actually fires. - The `ModifyLogicRequest` arm had the same gap in a different shape: its guard tested the *workflow* where it meant the *execution*. Now `executionStateOpt.getOrElse(throw new IllegalStateException("workflow execution is not initialized"))`. Line 89 of this file already used `Option(...)` correctly, as does `WorkflowService` at its lines 208 and 349 — the fix adopts the established in-tree idiom rather than inventing one. **A shape decision worth reviewing.** For the `ModifyLogicRequest` arm I did *not* simply swap the outer condition to `executionStateOpt.isDefined`. That variant makes a workflow-without-execution **silently do nothing** instead of reporting, which is not the intent — and it would collide with the pre-existing test "ignore a ModifyLogicRequest that arrives before any workflow is attached", which pins the no-workflow case as `noException` plus `sent shouldBe empty`. Keeping the workflow guard and reporting the absent execution satisfies both. **3. An unparseable frame no longer escapes the error mapper.** `objectMapper.readValue` moved from above the `try` to its first statement, so a frame the mapper cannot bind is reported like any handler failure. `sessionState` and `executionStateOpt` stay outside, because the `catch` arm needs them — both routing arms still work. All messages use the existing wording, `"workflow execution is not initialized"`. ### The fixes are pinned Four new tests. Verified in both directions, with the production file reverted and restored: | | production reverted | with fixes | |---|---|---| | `WorkflowWebsocketResourceSpec` | **15 passed, 4 failed** | **19 passed, 0 failed** | The before-state failures are the right ones, from the JUnit XML (sbt's only reporter here is `-u`, so the console shows no per-test lines): | new test | failure without the fix | |---|---| | runtime command with a workflow but no execution | `Expected java.lang.IllegalStateException … java.lang.NullPointerException was thrown` | | `ModifyLogicRequest` before any execution exists | `Expected java.lang.IllegalStateException … java.lang.NullPointerException was thrown` | | unparseable frame instead of escaping unmapped | `List() was not equal to List("WorkflowErrorEvent")` | | unparseable frame recorded in the metadata store | `List() was not equal to List(COMPILATION_ERROR)` | None of the 15 pre-existing tests regressed. ### Spec comments were updated, not just tests added The spec's header paragraph on malformed frames, its "deliberately not covered" entry for `ModifyLogicRequest`, and two in-test notes all documented these as known-and-unpinned. Leaving them would have left the spec asserting one thing and explaining the opposite, so they are rewritten to match. Trap avoidance, all previously encountered in this file: no assertions on `ClusterListener.numWorkerNodesInCluster` (its default is `0`, so such an assertion passes even against a hard-coded literal); `PrivilegeEnum.WRITE` is fed rather than the `NONE` default; and the new tests use `TestWorkflowService`, which overrides `disconnect()`, so `afterEach` never reaches the null `AmberRuntime._actorSystem` — the same pattern the existing tests use. ### Verification - `WorkflowWebsocketResourceSpec`: **19/19**. - Blast radius: `TexeraWebSocketRequestSpec`, `SessionStateSpec`, `ServletAwareConfiguratorSpec`, `WebsocketInputSpec` — 28/28 across 4 suites. `TexeraWebSocketRequestSpec` pins `InvalidTypeIdException` at the mapper level, which these changes leave untouched. - `scalafmtCheck`, `Test/scalafmtCheck`, `scalafixAll --check` all pass. - Production diff is 15 lines in one file. ### Any related issues, documentation, discussions? Closes #7801 Closes #7454 ### How was this PR tested? ``` STORAGE_ICEBERG_CATALOG_TYPE=postgres sbt "WorkflowExecutionService/testOnly org.apache.texera.web.resource.WorkflowWebsocketResourceSpec" ``` ``` [info] Suites: completed 1, aborted 0 [info] Tests: succeeded 19, failed 0, canceled 0, ignored 0, pending 0 ``` ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 5) Co-authored-by: Xuan Gu <[email protected]> Report URL: https://github.com/apache/texera/actions/runs/35954101852 With regards, GitHub Actions via GitBox
