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 · 🔴 3 worse · ⚪ 10 noise (<±5%) · 0 without baseline

Compared against main f5017f7 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	395	0.241	26,163/31,928/31,928 us	🔴 +9.3% / 🔴 +105.8%
🟢	bs=100 sw=10 sl=64	813	0.496	118,927/152,463/152,463 us	🟢 -7.7% / 🔴 +43.2%
⚪	bs=1000 sw=10 sl=64	901	0.55	1,113,986/1,167,106/1,167,106 us	⚪ within ±5% / 🔴 +14.2%

Baseline details

Latest main f5017f7 from same runner

config	metric	PR	latest main	7d avg	Δ latest	Δ 7d
bs=10 sw=10 sl=64	throughput	395 tuples/sec	434 tuples/sec	779.28 tuples/sec	-9.0%	-49.3%
bs=10 sw=10 sl=64	MB/s	0.241 MB/s	0.265 MB/s	0.476 MB/s	-9.1%	-49.3%
bs=10 sw=10 sl=64	p50	26,163 us	23,943 us	12,712 us	+9.3%	+105.8%
bs=10 sw=10 sl=64	p95	31,928 us	30,986 us	16,345 us	+3.0%	+95.3%
bs=10 sw=10 sl=64	p99	31,928 us	30,986 us	19,050 us	+3.0%	+67.6%
bs=100 sw=10 sl=64	throughput	813 tuples/sec	815 tuples/sec	1,017 tuples/sec	-0.2%	-20.1%
bs=100 sw=10 sl=64	MB/s	0.496 MB/s	0.498 MB/s	0.621 MB/s	-0.4%	-20.1%
bs=100 sw=10 sl=64	p50	118,927 us	119,084 us	100,048 us	-0.1%	+18.9%
bs=100 sw=10 sl=64	p95	152,463 us	165,242 us	106,477 us	-7.7%	+43.2%
bs=100 sw=10 sl=64	p99	152,463 us	165,242 us	114,739 us	-7.7%	+32.9%
bs=1000 sw=10 sl=64	throughput	901 tuples/sec	912 tuples/sec	1,048 tuples/sec	-1.2%	-14.0%
bs=1000 sw=10 sl=64	MB/s	0.55 MB/s	0.556 MB/s	0.639 MB/s	-1.1%	-14.0%
bs=1000 sw=10 sl=64	p50	1,113,986 us	1,093,168 us	976,350 us	+1.9%	+14.1%
bs=1000 sw=10 sl=64	p95	1,167,106 us	1,151,383 us	1,022,084 us	+1.4%	+14.2%
bs=1000 sw=10 sl=64	p99	1,167,106 us	1,151,383 us	1,053,520 us	+1.4%	+10.8%

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,506.38,200,128000,395,0.241,26162.55,31928.07,31928.07
1,100,10,64,20,2459.97,2000,1280000,813,0.496,118927.20,152462.66,152462.66
2,1000,10,64,20,22208.18,20000,12800000,901,0.550,1113986.45,1167106.06,1167106.06

[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.78%. Comparing base (f5017f7) to head (1d81408).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #7648      +/-   ##
============================================
- Coverage     90.14%   89.78%   -0.37%     
- Complexity     4415     4416       +1     
============================================
  Files          1176     1176              
  Lines         47133    46924     -209     
  Branches       5285     5265      -20     
============================================
- Hits          42487    42129     -358     
- Misses         2879     3023     +144     
- Partials       1767     1772       +5     

Flag	Coverage Δ		*Carryforward flag
access-control-service	70.00% <ø> (ø)		Carriedforward from e4500eb
agent-service	98.62% <ø> (ø)		Carriedforward from e4500eb
amber	86.68% <ø> (+<0.01%)	⬆️
computing-unit-managing-service	72.81% <ø> (+0.35%)	⬆️	Carriedforward from e4500eb
config-service	77.08% <ø> (-0.47%)	⬇️	Carriedforward from e4500eb
file-service	68.90% <ø> (ø)		Carriedforward from e4500eb
frontend	90.68% <ø> (-0.83%)	⬇️	Carriedforward from e4500eb
notebook-migration-service	78.35% <ø> (-0.96%)	⬇️	Carriedforward from e4500eb
pyamber	97.57% <ø> (-0.01%)	⬇️	Carriedforward from e4500eb
workflow-compiling-service	57.89% <ø> (ø)		Carriedforward from e4500eb

*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.