test(amber): cover WorkflowExecutionService state events and teardown

[aglinxinyuan]

What changes were proposed in this PR?

WorkflowExecutionServiceSpec goes from 3 tests to 11. The three existing tests stop at construction; the new ones drive the metadata-store handler and the teardown path.

Metric	Before	After
Codecov (fully-covered lines)	26/72 = 36.1%	36/72 = 50.0%
JaCoCo line-hit	30/72	39/72
Branch arms	5/18	10/18

+10 fully-covered lines and +5 branch arms. The two metrics differ by one because line 179 was already line-hit and flips only by completing its second arm — they are not interchangeable, so both are given.

Of the +10, six are logic and four are not. Lines 95, 179, 181–184 are the recovery-banner arm and the four unsubscribeAll calls. Lines 101, 107, 108 and 109 are scalac-generated public accessor pairs (confirmed with javap -p -l) that move because the teardown test assigns those vars from outside. Those four are a real consequence of driving teardown rather than coverage farming, but they are not logic and I would rather split them out than present all ten as equivalent.

What the reviewers found

Two independent adversarial reviewers ran against the first draft. Six of eight fresh mutants survived it. The three worth naming:

• workflowContext.workflowSettings = request.workflowSettings (line 90) could be deleted outright and all 8 tests still passed. Every test executes that line, so it was fully covered and entirely unconstrained. It is not cosmetic: outputPortsNeedingStorage is what CostBasedScheduleGenerator reads to decide which output ports get materialized, and dataTransferBatchSize / executionMode feed the resource allocator. Dropping it silently reverts every execution to the default settings while the request's are ignored.
• The recovering-state guard was pinned one-sidedly. The test only ever drove isRecovering false → true, so weakening newState.isRecovering != oldState.isRecovering to a bare newState.isRecovering survived. That mutant is the mirror of the failure the test's own comment claimed to guard: an update that clears the flag emits no event, so the frontend's Recovering banner never comes down.
• Both the state guard and the fatalErrors guard could be deleted wholesale and nothing failed.

All are now killed by a named test. The published mutation table was also re-run from scratch, one mutant at a time, because one row's failure message had been copy-pasted from another row.

Verification

Measured with an identical suite-name filter on both sides, one fresh sbt JVM per measurement, rm -rf of the jacoco dir between runs, counters read per-line out of jacoco.xml.

An independent measurer re-derived the figure a second way — over the whole amber unit module, the actual CI scope, rather than the six-suite filter — and got byte-identical per-line data on both sides. That independently confirms the six-suite list is complete and that 26 → 36 is what Codecov will show.

Two corrections that measurement forced, both worth stating:

• The repair round added 3 tests after the original measurement, and the expectation was that the figure would rise above 36. It did not. Those 3 tests bought zero additional fully-covered lines — they add branch arms on line 94 and mutation-kill strength only. 36 is the number.
• Line 185 already counted as a Codecov hit before this PR despite mi=4, ci=1, because the method's return instruction is attributed to it. It is genuinely executed only now.

Full amber unit scope, both sides: 190 suites, the same 7 pre-existing Windows-only failures by name, zero new. The only per-suite change anywhere is this spec going 3 → 11 tests, so the new beforeAll inserts do not leak — MockTexeraDB gives each suite its own database, and ids 9207–9210 are unique across the repo because the Iceberg statistics URI is machine-global.

Deliberately not included

executeWorkflow's live-runtime half — the 44-line hole — has no seam. Line 124 calls ComputingUnitMaster.createAmberRuntime, which builds an AmberClient over AmberRuntime.actorSystem, a JVM-global private var that nothing in unit scope initialises. Injecting a seam would be a production change.

Lines 113/114/115 are refused for a sharper reason: JaCoCo probes the try block only at its exit, so they flip only if compile and Workflow.fromCompilationResult both succeed — which falls straight into createAmberRuntime. Whether that is survivable depends on ClientEventSpec having restored the global to null in its afterAll. WorkflowServiceSpec's own header documents this cross-suite hazard as its reason for deliberately steering into a compile failure instead. Cementing an accident is worse than leaving three lines.

Three mutants are reported as live rather than dropped: reordering client.shutdown() against the four unsubscribeAll calls (order is not a stated contract), and two whose only kill would be to assert current behaviour that is arguably wrong — adding FAILED to the stuck-banner guard, and pinning which field the teardown guard reads, where the only discriminating state currently NPEs.

No production file is touched.

Any related issues, documentation, discussions?

Closes #8033

How was this PR tested?

sbt "WorkflowExecutionService/testOnly org.apache.texera.web.service.WorkflowExecutionServiceSpec"

[info] Tests: succeeded 11, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.

WorkflowExecutionService/Test/scalafmtCheck and WorkflowExecutionService/Test/scalafix --check both pass.

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Opus 5)

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[github-actions]

Automated Reviewer Suggestions

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

• Contributors with relevant context: @mengw15
  You can notify them by mentioning @mengw15 in a comment.

[github-actions]

⚠️ Benchmark changes need a look

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

Compared against main 90d0404 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	439	0.268	21,236/31,929/31,929 us	🟢 -14.1% / 🔴 +124.6%
⚪	bs=100 sw=10 sl=64	940	0.574	102,320/118,909/118,909 us	⚪ within ±5% / 🔴 +25.1%
⚪	bs=1000 sw=10 sl=64	1,094	0.668	907,321/965,690/965,690 us	⚪ within ±5% / 🔴 -7.9%

Baseline details

Latest main 90d0404 from same runner

config	metric	PR	latest main	7d avg	Δ latest	Δ 7d
bs=10 sw=10 sl=64	throughput	439 tuples/sec	410 tuples/sec	894.27 tuples/sec	+7.1%	-50.9%
bs=10 sw=10 sl=64	MB/s	0.268 MB/s	0.25 MB/s	0.546 MB/s	+7.2%	-50.9%
bs=10 sw=10 sl=64	p50	21,236 us	24,713 us	11,164 us	-14.1%	+90.2%
bs=10 sw=10 sl=64	p95	31,929 us	28,397 us	14,219 us	+12.4%	+124.6%
bs=10 sw=10 sl=64	p99	31,929 us	28,397 us	16,863 us	+12.4%	+89.3%
bs=100 sw=10 sl=64	throughput	940 tuples/sec	948 tuples/sec	1,158 tuples/sec	-0.8%	-18.8%
bs=100 sw=10 sl=64	MB/s	0.574 MB/s	0.578 MB/s	0.707 MB/s	-0.7%	-18.8%
bs=100 sw=10 sl=64	p50	102,320 us	106,191 us	88,711 us	-3.6%	+15.3%
bs=100 sw=10 sl=64	p95	118,909 us	116,238 us	95,062 us	+2.3%	+25.1%
bs=100 sw=10 sl=64	p99	118,909 us	116,238 us	102,275 us	+2.3%	+16.3%
bs=1000 sw=10 sl=64	throughput	1,094 tuples/sec	1,104 tuples/sec	1,188 tuples/sec	-0.9%	-7.9%
bs=1000 sw=10 sl=64	MB/s	0.668 MB/s	0.674 MB/s	0.725 MB/s	-0.9%	-7.9%
bs=1000 sw=10 sl=64	p50	907,321 us	901,998 us	874,204 us	+0.6%	+3.8%
bs=1000 sw=10 sl=64	p95	965,690 us	955,725 us	916,845 us	+1.0%	+5.3%
bs=1000 sw=10 sl=64	p99	965,690 us	955,725 us	947,186 us	+1.0%	+2.0%

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,455.78,200,128000,439,0.268,21235.53,31928.90,31928.90
1,100,10,64,20,2127.21,2000,1280000,940,0.574,102319.50,118908.85,118908.85
2,1000,10,64,20,18282.38,20000,12800000,1094,0.668,907321.06,965690.43,965690.43

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.53%. Comparing base (90d0404) to head (fd7b7f5).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #8035      +/-   ##
============================================
+ Coverage     93.52%   93.53%   +0.01%     
- Complexity     4730     4734       +4     
============================================
  Files          1195     1195              
  Lines         48511    48493      -18     
  Branches       5379     5379              
============================================
- Hits          45370    45359      -11     
+ Misses         1693     1684       -9     
- Partials       1448     1450       +2     

Flag	Coverage Δ		*Carryforward flag
access-control-service	81.00% <ø> (ø)		Carriedforward from 66cb727
agent-service	99.32% <ø> (ø)		Carriedforward from 66cb727
amber	89.65% <ø> (+0.04%)	⬆️
computing-unit-managing-service	73.67% <ø> (ø)		Carriedforward from 66cb727
config-service	86.86% <ø> (ø)		Carriedforward from 66cb727
file-service	87.91% <ø> (ø)		Carriedforward from 66cb727
frontend	95.86% <ø> (-0.01%)	⬇️	Carriedforward from 66cb727
notebook-migration-service	79.31% <ø> (ø)		Carriedforward from 66cb727
pyamber	98.54% <ø> (-0.01%)	⬇️	Carriedforward from 66cb727
workflow-compiling-service	77.19% <ø> (ø)		Carriedforward from 66cb727

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

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

[mengw15]

LGTM