The GitHub Actions job "Required Checks" on texera.git/main has succeeded. Run started by GitHub user github-merge-queue[bot] (triggered by github-merge-queue[bot]).
Head commit for run: 226124bb66491b2abccb02269cf23ebaf151bb84 / Xinyuan Lin <[email protected]> fix(amber, operator): keep error information on three failure paths (#7804) ### What changes were proposed in this PR? Three error paths that silently lost the information they exist to surface. All were characterized during earlier coverage work and deliberately left unpinned so a fix would not have to fight a test asserting the broken behaviour. **1. `OPVersion.getVersion` no longer returns `null` when `git log` fails.** The `GitAPIException` catch now stores the same `"N/A"` sentinel its `NullPointerException` sibling already did, so the result is non-null *and* memoized — previously the trailing `opMap.get(operatorName)` handed back `null`, which propagated into operator metadata (`LogicalOp.scala:462-466` → `OperatorMetadataGenerator.scala:171`), and every later call re-ran the failing `git log`. `printStackTrace()` is kept. That `"N/A"` is the intended sentinel is corroborated downstream: `agent-service/src/agent/util/workflow-utils.ts:193` already defends with `?? "N/A"`. **2. `InputManager.startInputPortReaderThreads` now passes the cause.** It wrapped a start failure as `new RuntimeException(s"…: ${e.getMessage}")` without `e`, discarding the original stack trace. The reader thread itself already does this correctly (`InputPortMaterializationReaderThread.scala:147`). **3. `PekkoActorRefMappingService.retrieveActorRef`'s catch no longer re-reads the failing value.** It bound `e` but never used it, instead reading `actorService.parent` again to name the parent in its warning — so a **persistently** unreachable parent threw straight out of the handler meant to contain it. The message now omits the parent and passes `e` as the log throwable, matching house style (`WorkflowActor.scala:144`, `SyncExecutionResource.scala:705`). **Why not hoist the read instead:** capturing the parent ref before the `try` was considered and rejected — the read *is* what throws, so hoisting it would let the failure escape `retrieveActorRef` entirely rather than fixing anything. ### All three are pinned, verified in both directions The production files were reverted and restored to confirm each test actually fails without its fix: | Suite | production reverted | with fixes | |---|---|---| | `OPVersionSpec` | **9 passed, 2 failed** | **11 passed** | | `InputManagerSpec` + `PekkoActorRefMappingServiceSpec` | **20 passed, 2 failed** | **22 passed** | The before-state failures are the right ones: - `null was not equal to "N/A"` (both new OPVersion cases) - `null equaled null the original failure must be attached as the cause` - `java.lang.IllegalStateException: parent is unreachable at … PekkoActorRefMappingService.retrieveActorRef` — i.e. thrown *out of* the handler, exactly the predicted failure mode ### On the tests - **OPVersion**: two cases using the existing unborn-HEAD throwaway-repo technique — the `"N/A"` fallback, and that it is *memoized* so a failing `git log` is not retried. - **InputManager**: a `NoOpReaderThread` subclass (no-op `run()`, so no storage is touched) is started and joined, then installed into the private map by reflection — the same technique as the existing `OutputPortStorageWriterThreadSpec.installWriterThread`. `Thread.start()` on a TERMINATED thread then throws `IllegalThreadStateException` deterministically, and the test asserts both the wrapping message and that `getCause` is that exception. - **PekkoActorRefMappingService**: the new case sets the failure counter to `Int.MaxValue` so *every* read throws, not just the first — the existing single-failure test could not have caught this. It also asserts the id remains askable afterwards, i.e. was not wrongly marked queried. Three stale comments were updated rather than left contradicting the new assertions: `OPVersionSpec`'s "Deliberately NOT asserted" paragraph about the null, and the defect notes on the two amber specs. ### Verification - `OPVersionSpec` 11/11; `InputManagerSpec` + `PekkoActorRefMappingServiceSpec` + `InputPortMaterializationReaderThreadSpec` 32/32 (the reader-thread spec included as the nearest neighbour to the `InputManager` change). - Specs were filtered deliberately rather than running the whole `WorkflowOperator` module, because `FileScanSourceOpExecSpec` currently aborts on Windows over a leaked handle — that is being fixed separately in #7800. - `scalafmtCheck`, `Test/scalafmtCheck` and `scalafixAll --check` pass for both modules. One pre-existing scalafix *warning* in an untouched file (`OutputManagerSpec.scala:59`, unused suppression) is unrelated. ### Any related issues, documentation, discussions? Closes #7803 ### How was this PR tested? ``` sbt "WorkflowOperator/testOnly org.apache.texera.amber.operator.metadata.OPVersionSpec" "WorkflowExecutionService/testOnly org.apache.texera.amber.engine.architecture.messaginglayer.InputManagerSpec org.apache.texera.amber.engine.architecture.common.PekkoActorRefMappingServiceSpec" ``` ``` [info] Tests: succeeded 11, failed 0, canceled 0, ignored 0, pending 0 [info] Tests: succeeded 22, 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) Report URL: https://github.com/apache/texera/actions/runs/32411776428 With regards, GitHub Actions via GitBox
