The GitHub Actions job "Required Checks" on texera.git/gh-readonly-queue/main/pr-7648-363537e0251182472f540079d262599f6cbb5240 has failed. Run started by GitHub user aglinxinyuan (triggered by aglinxinyuan).
Head commit for run: 9f6d373ad5e6e7eaef3a15d0337b23d16796bd71 / Eugene Gu <[email protected]> test(amber): cover AdvanceRegionExecutionsHandler (#7648) ### What changes were proposed in this PR? This adds `AdvanceRegionExecutionsHandlerSpec`, the first direct coverage of `AdvanceRegionExecutionsHandler` (added in #6960). The handler is four lines long but carries three distinct contracts, none of which was asserted anywhere: it delegates the advance to the workflow execution manager with the coordinator's own actor service, it replies to the requesting round *without* awaiting that advance (the whole point of routing the advance through its own control round), and on failure it notifies the client with a `FatalError` that carries the related worker id when the cause is a `WorkflowRuntimeException` and `None` otherwise. Six tests, test-only — no production file is touched: 1. `should advance the region executions with the coordinator's own actor service` — asserts the forwarded `PekkoActorService` is the coordinator's own instance (reference equality, plus a non-null guard so a vacuous `null == null` cannot pass). 2. `should reply immediately instead of awaiting the advance` — the reply is defined while the advance's promise is still unsatisfied. Deterministic, not timing-based: the handler's `EmptyReturn()` becomes an already-satisfied future through `returnAsFuture`, while the stubbed advance never completes. No sleeps or polling anywhere in the spec. 3. `should reply to the requesting round while the advance is still running` — delivers the request through `processDCM` and the real `AsyncRPCServer`, so it also pins that the handler is dispatchable under `METHOD_COORDINATOR_INITIATE_ADVANCE_REGION_EXECUTIONS`, the wire name `PortCompletedHandler` actually sends. 4. `should notify the client of a failed advance with the related worker id` 5. `should notify the client of a failed advance without a worker id for other failures` 6. `should not notify the client when the advance succeeds` The fixture pattern is reused from the sibling handler specs (`TestKit` + `RegionExecutionManagerTestSupport.createCoordinatorHarness()` + a real `CoordinatorProcessor` and `CoordinatorAsyncRPCHandlerInitializer`, as in `PortCompletedHandlerSpec` and `EmbeddedControlMessageHandlerSpec`). The only new piece is a `WorkflowExecutionManager` subclass that records advance calls and hands back a promise the test controls: driving the real manager into an async failure requires staging a full region launch/termination, and the causes it produces there are `IllegalStateException`s or the bare `Throwable` from `ErrorUtils.reconstructThrowable` — never a `WorkflowRuntimeException` carrying a worker id, which is the case test 4 exists to pin. Only the advance is stubbed; the coordinator processor, RPC server dispatch, RPC client and output gateway are all real. Note on test 4: the `WorkflowRuntimeException` arm currently pins *intent* rather than today-observable behavior. The two production subclasses that do carry a worker id, `StateManager.InvalidStateException` and `InvalidTransitionException` (`StateManager.scala:31-35`), are thrown worker-side, and a failed worker RPC reaches the coordinator through `ErrorUtils.reconstructThrowable`, which always rebuilds a plain `Throwable` and so drops the type; every other production site constructs a `WorkflowRuntimeException` with a message only, leaving `relatedWorkerId` empty. Both arms would therefore behave identically at present. The branch is in the handler and documented as intended, so the test guards it against regression. ### Any related issues, documentation, discussions? Closes #7646 The handler was introduced by #6960 (backported as #7096). ### How was this PR tested? `sbt "WorkflowExecutionService/testOnly *AdvanceRegionExecutionsHandlerSpec"` — 6 tests, all passing. No regressions in the neighbourhood: `*coordinator.promisehandlers.* *scheduling.*` runs 20 suites / 166 tests / 0 failures. `WorkflowExecutionService/Test/scalafmtCheck` is clean. Each test was mutation-checked: the production handler was temporarily broken, the suite re-run, and the handler reverted. Every behavior-changing mutant is caught, and each test is the sole killer of at least one: | Mutant | Caught by | |---|---| | `FatalError(err, err.relatedWorkerId)` → `FatalError(err, None)` | 4 | | `FatalError(err, Some(COORDINATOR))` — wrong but non-empty worker id | 4 | | drop the `.onFailure` block | 4, 5 | | `.onFailure { case _ => () }` (swallow the failure) | 4, 5 | | collapse both arms into a single catch-all | 4 | | notify on `.onSuccess` instead | 4, 5, 6 | | send the `FatalError` twice | 4 | | await the advance (`.map(_ => EmptyReturn())`) | 2, 3 | | never delegate (`Future.Unit` in place of the call) | 1, 2, 3, 4, 5 | | `advanceRegionExecutions(null)` instead of `cp.actorService` | 1 | | call `advanceRegionExecutions` twice | 3 | The only surviving mutant, `EmptyReturn()` → `Future.value(EmptyReturn())`, is semantically equivalent (`returnAsFuture` is an eager `Future.apply`), so there is nothing to catch. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 5) Co-authored-by: Meng Wang <[email protected]> Co-authored-by: Xinyuan Lin <[email protected]> Report URL: https://github.com/apache/texera/actions/runs/31935942284 With regards, GitHub Actions via GitBox
