fix(amber, operator, v1.2): keep error information on three failure paths - #7809
Merged
aglinxinyuan merged 2 commits intoAug 23, 2026
Conversation
…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? (backported from commit 226124b) Generated-by: Claude Code (Opus 5)
Contributor
Author
|
The cherry-pick conflicted and was committed with conflict markers. Resolve the conflicts on this branch, then mark this PR ready for review. Conflicting files:
|
The cherry-pick of #7804 hit modify/delete on all three of its spec files: each already existed on `main` (added by #7018, #7219 and #5451, none of which were backported) but is absent here, so git could not apply a diff and the automation committed `main`'s whole file. Two of the three compile and pass against this branch unchanged; `PekkoActorRefMappingServiceSpec` does not, because it is written against `main` names this branch predates: - `virtualidentity.util.COORDINATOR` is `CONTROLLER` here, so the import did not resolve at all; - the scaladoc pointed at `PekkoMessageTransferServiceSpec`, a sibling that does not exist on this branch, leaving a dangling `[[link]]`. The assertions are untouched: every method the suite drives is identical on this branch, and the production hunks of #7804 applied cleanly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/v1.2 #7809 +/- ##
===============================================
Coverage 52.98% 52.98%
Complexity 2546 2546
===============================================
Files 1078 1078
Lines 42366 42365 -1
Branches 4565 4565
===============================================
Hits 22446 22446
+ Misses 18604 18603 -1
Partials 1316 1316
*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:
|
xuang7
approved these changes
Aug 23, 2026
aglinxinyuan
deleted the
backport/7804-keep-error-information-on-three-failure-v1.2
branch
August 23, 2026 04:47
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?
Automated backport of #7804 to
release/v1.2.Source: 226124b · automation run
Any related issues, documentation, discussions?
Backport of #7804. Originally linked #7803.
How was this PR tested?
Release-branch CI runs on this branch once the conflicts are resolved and this PR is marked ready for review.
Was this PR authored or co-authored using generative AI tooling?
No.