test(amber): restore the pgroonga global WorkflowResourceSpec clobbers

[aglinxinyuan]

What changes were proposed in this PR?

FulltextSearchQueryUtils.usePgroonga (amber/src/main/scala/org/apache/texera/web/resource/dashboard/FulltextSearchQueryUtils.scala:32) is a JVM-global var whose production default is true. It is read at exactly one site, FulltextSearchQueryUtils.scala:52, to choose between the pgroonga arm and the to_tsvector/to_tsquery fallback. WorkflowResourceSpec.beforeAll forces it false and never puts it back.

amber declares no Test / fork — the two Test / fork := true settings a grep finds in build.sbt belong to ComputingUnitManagingService (build.sbt:191) and FileService (build.sbt:227), while the amber project (WorkflowExecutionService, build.sbt:252) declares neither fork nor parallelExecution — and suites are serialized by Tags.limit(Tags.Test, 1) (amber/build.sbt:48). One WorkflowExecutionService/test run is therefore a single JVM sharing a single flag: 193 completed suites in the run measured here.

one WorkflowExecutionService/test JVM -- unforked, suites serialized

  ...  ->  WorkflowResourceSpec                            ->  ...
           beforeAll: usePgroonga = false
           afterAll : (before) nothing                         every later suite
                      (after)  usePgroonga = <captured>        renders the fallback
                                                               arm, not production's

This suite genuinely needs the false arm, so the write is restored rather than deleted: the embedded Postgres it runs against has no pgroonga extension, and with the flag left true 11 of its 78 tests fail with ERROR: function pgroonga_condition(unknown, fuzzy_max_distance_ratio => numeric) does not exist. The 11 are named under How was this PR tested?.

The capture is taken in beforeAll, immediately before the write, and written back as the first statement of afterAll, ahead of closeConnectionPool(). That ordering is hygiene, not a fix for a live hazard: MockTexeraDB.closeConnectionPool (common/dao/src/test/scala/org/apache/texera/dao/MockTexeraDB.scala:183-192) already swallows any Exception itself (catch { case e: Exception => e.printStackTrace() } finally { ... }), so only an Error could escape it today. Putting the restore first means the ordering does not depend on that staying true.

The alternative — capturing into a val at construction time — restores whatever the flag held when sbt happened to instantiate the class, which is a property of sbt's scheduling rather than of what the suite clobbered. Both forms were run against the same scenario: an earlier suite leaves the flag false, then this suite runs, then a third suite reads it.

capture site	value restored	scenario outcome
construction time (private val)	true — the value at instantiation	79/80, true was not equal to false
write time (private var, this PR)	false — the value it clobbered	80/80

Honest qualification, since it cuts against this change: that scenario is built with a nested org.scalatest.Suites, which evaluates its nested suites as constructor arguments and therefore instantiates all of them before any of their tests run. Measured separately, sbt/ScalaTest does not do that for discovered suites — it constructs each one immediately before running it (order log: ctor:Bb, beforeAll:Bb, test:Bb, afterAll:Bb, ctor:Aa, ...). Nothing touches the flag between this suite's construction and its beforeAll, and there are zero extends Suites classes in amber/src/test/scala, so the two forms restore the same value in a real test run today and are behaviourally indistinguishable there. The write-time form is preferred only because its correctness does not depend on that measurement continuing to hold: a future Test / fork, a OneInstancePerTest mixin, or any nesting suite would each change the answer. The in-code comment carries that same qualification, so the file does not overstate the case either.

The field is also initialised from the live value at its declaration. That is not decoration — afterAll was measured to run even when beforeAll throws, so the restore can execute on a path where the capture line never did, and the initialiser keeps it from writing back an invented default.

The restore is unpinned: nothing in the repo would go red if it were deleted again. Pinning the arm false for an entire module run — a conservative superset of the leak's effect — leaves the failing-test identities byte-identical to baseline, so no suite in WorkflowExecutionService currently reads the flag in an arm-sensitive way. This PR removes latent cross-suite state leakage; it does not fix a currently-failing test, and no fragile ordering-dependent guard suite was added to manufacture a pin.

A comment elsewhere that this change falsifies. DatasetSearchQueryBuilderSpec's header comment (:114-123) explains why that suite's keyword assertions are deliberately arm-independent, and its parenthetical names both writers of the global: "DatasetResourceSpec or WorkflowResourceSpec ran earlier in this JVM and left the global false (both set it and neither restores it...)". After this commit that sentence is false — this suite does restore it. This PR deliberately does not edit that comment. Any narrowing written here ("only DatasetResourceSpec leaks") would itself become false the moment the sibling PR lands, so the correction belongs in exactly one place: the sibling (#8403) rewrites that paragraph into a form that names no suite at all and is therefore true whichever of the two merges first. If this PR merges first, the comment is stale until the sibling lands — and the guardrail it states, "anything added here must keep that property", stays valid in every one of the four states, so nothing that reads it is misled about what to do.

Writers of the global today:

writer	restores?
FulltextSearchQueryUtilsSpec:64,71,87	yes — after { ... }, per test
WorkflowResourceSpec:179	this PR
DatasetResourceSpec:99	no — deleted outright by #8403, since that suite never reaches the flag read

Once both land, this spec is the only writer of the global outside FulltextSearchQueryUtilsSpec. With only one of the two applied the flag's end state differs — with only this PR, DatasetResourceSpec still writes false without restoring; with only the sibling, this suite still leaks false — and whichever of the two sbt schedules last decides it (sbt's order was measured to be neither alphabetical nor command-line order). Inference, not a measurement: since nothing in the module was found to be arm-sensitive, the failing-test identity set should not move in either partial state. The sibling-only tree was never built or run, so that half is reasoning from the arm-sensitivity measurement rather than an observation.

Scope. One test file, +17 lines: the capture, the restore, and the comment explaining why the capture sits where it does. No src/main file is touched and no other test file is touched. No test is added, renamed or deleted; the suite's own 78 tests are unchanged and so are their outcomes.

Any related issues, documentation, discussions?

Closes #8400

How was this PR tested?

Everything below is read out of amber/target/test-reports/TEST-*.xml, not the sbt console summary. All probe suites were deleted before committing; the branch is one commit.

1. The suite really does need the false write. With beforeAll changed to leave the flag true, WorkflowResourceSpec alone reports 78 tests and 11 failures, every one an org.jooq.exception.DataAccessException caused by ERROR: function pgroonga_condition(unknown, fuzzy_max_distance_ratio => numeric) does not exist. The suite passes keywords through a getKeywordsArray helper at 16 call sites (:331-751), one of them with the reserved-character string "+-@()<>~*\"", so it reaches the flag read for real instead of taking the empty-keywords early return at FulltextSearchQueryUtils.scala:45-47.

/search API should be able to search for resources by keyword
/search API should be able to search for workflows in different columns in Workflow table
/search API should be able to search text phrases
/search API should be able to search with arbitrary number of keywords in different combinations
/search API should filter results by different resourceType
/search API should handle multiple keywords correctly
/search API should handle reserved characters in the keywords
/search API should not be able to search workflows from different user accounts
/search API should not return resources that belong to a different user
/search API should return multiple matching resources from a single resource type
/search API should return resources that match any of all provided keywords

So the right fix here is a restore, not the deletion the sibling PR makes.

2. Red before / green after, with an explicitly-ordered throwaway probe — Suites(new WorkflowResourceSpec, new PgroongaProbeTailSpec), where the tail asserts the global still holds production's default. The nesting pins the ordering because a plain testOnly of two classes does not order them. Both suites in the same invocation, since separate invocations get fresh classloaders and reset the static:

tree	result	non-passing identity in the XML
baseline 1cbe857007	78 passed, 1 failed	the JVM-global usePgroonga, after the preceding suite finished should still hold production's default
with this PR	79 passed, 0 failed	none
control: probe alone, baseline	1 passed	none

The control matters: the tail assertion is not unconditionally red, so its red above is caused by the leak.

3. Write-time vs construction-time capture — the table in the first section. Same probe shape, with an added first nested suite that leaves the flag false. Both arms were measured on the committed content: the write-time arm as committed, the construction-time arm by changing only var to val and dropping the capture line from beforeAll. The construction-time form restores true and clobbers it (XML: true was not equal to false on usePgroonga after WorkflowResourceSpec should still be the false the earlier suite left); the write-time form restores false and the run is 80/80. As stated above, this reflects the eager instantiation that Suites nesting creates rather than sbt's own (measured lazy) behaviour, so it is a design argument, not a live bug.

4. Instantiation and lifecycle semantics, measured with an append-only order log from two suites plus one whose beforeAll throws:

ctor:Bb       flag=true    <- Bb fully constructed, run and torn down
beforeAll:Bb  flag=true
test:Bb       flag=true
afterAll:Bb
ctor:Aa       flag=true    <- only now is Aa constructed
beforeAll:Aa  flag=true
test:Aa       sets flag=false
afterAll:Aa
throwspec:beforeAll entered, about to throw
throwspec:afterAll RAN     <- afterAll runs even when beforeAll throws

Four findings: construction is lazy, per suite, immediately before that suite runs; suites do not interleave; afterAll still runs when beforeAll throws (sbt reported Suites: completed 2, aborted 1 and the throwing suite's test never logged a line, so the restore can execute on a path where the capture never did); and Bb ran before Aa although Aa was listed first on the testOnly command line and sorts first alphabetically.

5. Arm sensitivity, i.e. why the restore is unpinned. Production default mutated true -> false in src/main for one whole module run, then reverted: 195 report files and 86 non-passing identities, byte-identical to the baseline list (diff empty). That is a superset of the leak's effect — every one of the 193 suites ran on the fallback arm, not just the ones scheduled after this spec — so no suite outcome in this module depends on the arm. The mutation was reverted and verified: git diff --name-only 1cbe857007 -- '*/src/main/*' is empty and line 32 reads var usePgroonga: Boolean = true again.

6. Regression, widest scope that runs locally — AMBER_TEST_FILTER=skip-integration sbt WorkflowExecutionService/test, baseline (both touched files at their 1cbe857007 content) measured first. Counting rule: a non-passing identity is a <testcase> element in amber/target/test-reports/TEST-*.xml carrying a <failure>, <error> or <skipped> child, printed as KIND \t suite \t test name and sorted; the same rule is applied to both runs.

run	report files	sbt summary	non-passing identities
baseline 1cbe857007	195	2297 tests: 2214 succeeded, 83 failed, 1 canceled, 1 pending; 193 suites completed, 1 aborted	86
with this PR	195	identical, line for line	86

diff of the two sorted identity lists is empty. The 86 break down as 84 <failure> + 1 <error> (the aborted suite's SuiteSelector pseudo-testcase) + 1 <skipped>, and the run's own reporter tally agrees: Total 2301, Failed 84, Errors 1, Passed 2216, Canceled 1, Pending 1. Excluding <skipped> the same runs read as 85 rows — the absolute number is method-dependent, which is why the rule is stated; the load-bearing fact is that the two lists are byte-identical, not the count.

All 86 are pre-existing on this box, in 13 suites, none of them touched by this PR:

suite	rows	why it is red here
ResultExportServiceSpec	17	org.apache.iceberg.exceptions.RESTException — REST catalog GET to localhost:8181, no Docker on this box
DataProcessingSpec	16	same catalog GET, wrapped in java.lang.Throwable
ExecutionStatsServiceSpec	12	same
ExecutionResultServiceSpec	11	same
SyncExecutionResourceSpec	8	same
InputPortMaterializationReaderThreadSpec	8	same (an engine worker-manager suite, not a dashboard one)
PveResourceSpec	6	python virtual environment: Python executable not found for PVE
ReconfigurationSpec, PauseSpec	2 + 2	same catalog GET
WorkflowExecutionServiceSpec	1	same
DefaultCostEstimatorSpec	1	the aborted suite: RESTException from the same catalog GET at construction
GitVersionControlLocalFileStorageSpec	1	local file-tree assertion, testFileTreeRetrieval
NetworkOutputBufferSpec	1	the <skipped> row — a pendingUntilFixed test, not a failure

WorkflowResourceSpec, the only file this PR touches, is green in both wide runs at tests="78" errors="0" failures="0"; so is DatasetSearchQueryBuilderSpec (tests="24"), the spec whose header comment describes this leak. The change alters what the suite leaves behind, not what it asserts.

7. Lint (CI gates): WorkflowExecutionService/scalafmtCheck, WorkflowExecutionService/Test/scalafmtCheck and WorkflowExecutionService/scalafixAll --check all report [success] on the committed tree.

Corrections made after review. Four claims in an earlier draft of this description were wrong or unstated, and are corrected above rather than quietly dropped: (a) the restore-before-teardown ordering was justified as protecting against a teardown throw, but closeConnectionPool catches Exception itself, so the justification is hygiene only; (b) the non-passing identity count is counting-method dependent (the same runs read as 85 rows if <skipped> is excluded), so the rule is now stated; (c) the pre-existing local failures were described as Iceberg/Docker dashboard suites, which under-describes them — see the table above; and (d) this PR falsifies a sentence in DatasetSearchQueryBuilderSpec's header comment, which the earlier draft cited as supporting evidence without disclosing that it goes stale; An earlier revision of this branch corrected that comment here; the edit has been reverted, because the narrowing it wrote would go false as soon as the sibling landed. The correction now lives only in the sibling PR, in an order-neutral form, and this PR discloses the staleness instead.

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

Generated-by: Claude Code (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.

[Copilot]

🟢 Approval recommended

The change is narrowly scoped to test hygiene, is consistent with Amber’s single-JVM test execution model, and cleanly restores the mutated global state.

Pull request overview

This PR prevents cross-suite state leakage in Amber’s test JVM by restoring the JVM-global FulltextSearchQueryUtils.usePgroonga flag after WorkflowResourceSpec runs. This keeps later suites from silently running against the fallback full-text-search path when they expect the production default.

Changes:

• Capture the pre-test value of FulltextSearchQueryUtils.usePgroonga in beforeAll immediately before forcing it to false.
• Restore the captured value at the start of afterAll (before DB teardown) to avoid leaving mutated global state behind.
• Add an in-file comment documenting why the capture/restore is structured this way for unforked, single-JVM test runs.

File summaries

File	Description
amber/src/test/scala/org/apache/texera/web/resource/dashboard/file/WorkflowResourceSpec.scala	Captures and restores the global usePgroonga flag around the suite to prevent cross-suite contamination in unforked Amber tests.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 0
• Review effort level: Lite

---

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

[github-actions]

⚠️ Benchmark changes need a look

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

Compared against main 1cbe857 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	367	0.224	27,118/31,562/31,562 us	🔴 +30.8% / 🔴 +109.6%
🔴	bs=100 sw=10 sl=64	777	0.474	126,612/159,242/159,242 us	🔴 +12.6% / 🔴 +45.7%
🔴	bs=1000 sw=10 sl=64	901	0.55	1,106,051/1,235,438/1,235,438 us	🔴 +8.5% / 🔴 +18.5%

Baseline details

Latest main 1cbe857 from same runner

config	metric	PR	latest main	7d avg	Δ latest	Δ 7d
bs=10 sw=10 sl=64	throughput	367 tuples/sec	425 tuples/sec	755.36 tuples/sec	-13.6%	-51.4%
bs=10 sw=10 sl=64	MB/s	0.224 MB/s	0.26 MB/s	0.461 MB/s	-13.8%	-51.4%
bs=10 sw=10 sl=64	p50	27,118 us	20,727 us	12,938 us	+30.8%	+109.6%
bs=10 sw=10 sl=64	p95	31,562 us	36,522 us	15,980 us	-13.6%	+97.5%
bs=10 sw=10 sl=64	p99	31,562 us	36,522 us	19,233 us	-13.6%	+64.1%
bs=100 sw=10 sl=64	throughput	777 tuples/sec	845 tuples/sec	976.3 tuples/sec	-8.0%	-20.4%
bs=100 sw=10 sl=64	MB/s	0.474 MB/s	0.516 MB/s	0.596 MB/s	-8.1%	-20.5%
bs=100 sw=10 sl=64	p50	126,612 us	117,808 us	102,340 us	+7.5%	+23.7%
bs=100 sw=10 sl=64	p95	159,242 us	141,433 us	109,262 us	+12.6%	+45.7%
bs=100 sw=10 sl=64	p99	159,242 us	141,433 us	118,827 us	+12.6%	+34.0%
bs=1000 sw=10 sl=64	throughput	901 tuples/sec	930 tuples/sec	1,006 tuples/sec	-3.1%	-10.5%
bs=1000 sw=10 sl=64	MB/s	0.55 MB/s	0.568 MB/s	0.614 MB/s	-3.2%	-10.5%
bs=1000 sw=10 sl=64	p50	1,106,051 us	1,071,579 us	999,855 us	+3.2%	+10.6%
bs=1000 sw=10 sl=64	p95	1,235,438 us	1,139,119 us	1,042,833 us	+8.5%	+18.5%
bs=1000 sw=10 sl=64	p99	1,235,438 us	1,139,119 us	1,070,722 us	+8.5%	+15.4%

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,545.17,200,128000,367,0.224,27118.12,31562.34,31562.34
1,100,10,64,20,2574.76,2000,1280000,777,0.474,126612.23,159242.31,159242.31
2,1000,10,64,20,22199.08,20000,12800000,901,0.550,1106050.63,1235438.41,1235438.41

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.02%. Comparing base (1cbe857) to head (4034358).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #8404      +/-   ##
============================================
- Coverage     94.03%   94.02%   -0.01%     
+ Complexity     4821     4818       -3     
============================================
  Files          1204     1204              
  Lines         48991    48991              
  Branches       5956     5956              
============================================
- Hits          46067    46063       -4     
- Misses         1458     1460       +2     
- Partials       1466     1468       +2     

Flag	Coverage Δ		*Carryforward flag
access-control-service	81.00% <ø> (ø)		Carriedforward from 1cbe857
agent-service	99.32% <ø> (ø)		Carriedforward from 1cbe857
amber	89.89% <ø> (-0.03%)	⬇️
computing-unit-managing-service	73.67% <ø> (ø)		Carriedforward from 1cbe857
config-service	87.12% <ø> (ø)		Carriedforward from 1cbe857
file-service	87.91% <ø> (ø)		Carriedforward from 1cbe857
frontend	96.79% <ø> (ø)		Carriedforward from 1cbe857
notebook-migration-service	83.57% <ø> (ø)		Carriedforward from 1cbe857
pyamber	98.47% <ø> (ø)		Carriedforward from 1cbe857
workflow-compiling-service	77.19% <ø> (ø)		Carriedforward from 1cbe857

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

[mengw15]

LGTM