test(amber): cover AdvanceRegionExecutionsHandler

[eugenegujing]

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 IllegalStateExceptions 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)

[github-actions]

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

• No candidates found from git blame history.

[github-actions]

⚠️ Benchmark changes need a look

🟢 2 better · 🔴 5 worse · ⚪ 8 noise (<±5%) · 0 without baseline

Compared against main 4013ca7 benchmarked on this same runner, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.

Dashboard · Run

	config	throughput	MB/s	latency	max Δ latest / 7d
🔴	bs=10 sw=10 sl=64	390	0.238	25,798/29,136/29,136 us	🔴 +18.7% / 🔴 +102.5%
⚪	bs=100 sw=10 sl=64	810	0.494	121,556/156,252/156,252 us	⚪ within ±5% / 🔴 +45.2%
🔴	bs=1000 sw=10 sl=64	913	0.557	1,083,808/1,169,067/1,169,067 us	🔴 +7.0% / 🔴 +12.8%

Baseline details

Latest main 4013ca7 from same runner

config	metric	PR	latest main	7d avg	Δ latest	Δ 7d
bs=10 sw=10 sl=64	throughput	390 tuples/sec	434 tuples/sec	774.89 tuples/sec	-10.1%	-49.7%
bs=10 sw=10 sl=64	MB/s	0.238 MB/s	0.265 MB/s	0.473 MB/s	-10.2%	-49.7%
bs=10 sw=10 sl=64	p50	25,798 us	21,740 us	12,738 us	+18.7%	+102.5%
bs=10 sw=10 sl=64	p95	29,136 us	34,544 us	16,348 us	-15.7%	+78.2%
bs=10 sw=10 sl=64	p99	29,136 us	34,544 us	18,848 us	-15.7%	+54.6%
bs=100 sw=10 sl=64	throughput	810 tuples/sec	816 tuples/sec	1,005 tuples/sec	-0.7%	-19.4%
bs=100 sw=10 sl=64	MB/s	0.494 MB/s	0.498 MB/s	0.613 MB/s	-0.8%	-19.4%
bs=100 sw=10 sl=64	p50	121,556 us	119,174 us	100,970 us	+2.0%	+20.4%
bs=100 sw=10 sl=64	p95	156,252 us	158,112 us	107,605 us	-1.2%	+45.2%
bs=100 sw=10 sl=64	p99	156,252 us	158,112 us	116,429 us	-1.2%	+34.2%
bs=1000 sw=10 sl=64	throughput	913 tuples/sec	945 tuples/sec	1,030 tuples/sec	-3.4%	-11.4%
bs=1000 sw=10 sl=64	MB/s	0.557 MB/s	0.577 MB/s	0.629 MB/s	-3.5%	-11.4%
bs=1000 sw=10 sl=64	p50	1,083,808 us	1,054,845 us	991,433 us	+2.7%	+9.3%
bs=1000 sw=10 sl=64	p95	1,169,067 us	1,092,578 us	1,036,668 us	+7.0%	+12.8%
bs=1000 sw=10 sl=64	p99	1,169,067 us	1,092,578 us	1,070,470 us	+7.0%	+9.2%

Raw CSV

config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,512.32,200,128000,390,0.238,25798.02,29135.68,29135.68
1,100,10,64,20,2469.25,2000,1280000,810,0.494,121555.90,156252.35,156252.35
2,1000,10,64,20,21898.62,20000,12800000,913,0.557,1083807.65,1169066.52,1169066.52

[Copilot]

Pull request overview

Adds the first direct unit test coverage for the coordinator-side AdvanceRegionExecutionsHandler in Amber, asserting its key behavioral contracts (delegation target, non-blocking reply semantics, and client notification behavior on failure).

Changes:

• Introduces AdvanceRegionExecutionsHandlerSpec with a controllable WorkflowExecutionManager stub to observe delegation and async failure paths deterministically.
• Verifies the handler replies immediately (does not await the advance) and is dispatchable via the real AsyncRPCServer under METHOD_COORDINATOR_INITIATE_ADVANCE_REGION_EXECUTIONS.
• Verifies FatalError client notification behavior for WorkflowRuntimeException (with related worker id) vs other failures (no worker id), and no notification on success.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.67%. Comparing base (4013ca7) to head (4271686).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #7648      +/-   ##
============================================
- Coverage     89.68%   89.67%   -0.02%     
  Complexity     4399     4399              
============================================
  Files          1175     1176       +1     
  Lines         46979    46992      +13     
  Branches       5265     5268       +3     
============================================
+ Hits          42133    42138       +5     
- Misses         3082     3089       +7     
- Partials       1764     1765       +1     

Flag	Coverage Δ		*Carryforward flag
access-control-service	70.00% <ø> (ø)		Carriedforward from 12169c2
agent-service	98.62% <ø> (ø)		Carriedforward from 12169c2
amber	86.38% <ø> (ø)
computing-unit-managing-service	72.46% <ø> (ø)		Carriedforward from 12169c2
config-service	77.31% <ø> (ø)		Carriedforward from 12169c2
file-service	68.90% <ø> (ø)		Carriedforward from 12169c2
frontend	90.67% <ø> (-0.04%)	⬇️	Carriedforward from 12169c2
notebook-migration-service	78.89% <ø> (ø)		Carriedforward from 12169c2
pyamber	97.57% <ø> (ø)		Carriedforward from 12169c2
workflow-compiling-service	57.89% <ø> (ø)		Carriedforward from 12169c2

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.