This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-8404-873c91ce70f15266383e82c2e0f74050692c2b84 in repository https://gitbox.apache.org/repos/asf/texera.git
commit 39e17fd1d1d4eb608300be19a31fc99a8cdc2d46 Author: Xinyuan Lin <[email protected]> AuthorDate: Sat Sep 5 05:51:51 2026 +0000 test(amber): restore the pgroonga global WorkflowResourceSpec clobbers (#8404) ### 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) --- .../resource/dashboard/file/WorkflowResourceSpec.scala | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/file/WorkflowResourceSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/file/WorkflowResourceSpec.scala index 1816ce9cd8..88f429a3e6 100644 --- a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/file/WorkflowResourceSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/file/WorkflowResourceSpec.scala @@ -160,8 +160,22 @@ class WorkflowResourceSpec new DashboardResource() } + // `FulltextSearchQueryUtils.usePgroonga` is a JVM-global `var` and amber's tests are + // unforked, so every suite in one `test` invocation shares it. This spec must force it + // `false` — its embedded Postgres has no pgroonga extension, and the `true` arm fails 11 + // of the search tests with `function pgroonga_condition(...) does not exist`. Capture the + // live value in `beforeAll` right before the write, so what is put back is exactly the + // value this suite clobbered. Capturing at construction instead would put back whatever + // the flag held when sbt instantiated this class; under sbt today that is the same value, + // since a suite is constructed immediately before it runs and nothing touches the flag in + // between — but that is a property of sbt's scheduling, not of this suite, and it stops + // holding under eager construction (a nesting `Suites`, `OneInstancePerTest`, forking). + // The initialiser below is only so the field never holds an invented default. + private var pgroongaBeforeWrite: Boolean = FulltextSearchQueryUtils.usePgroonga + override protected def beforeAll(): Unit = { initializeDBAndReplaceDSLContext() + pgroongaBeforeWrite = FulltextSearchQueryUtils.usePgroonga FulltextSearchQueryUtils.usePgroonga = false // disable pgroonga // add test user directly val userDao = new UserDao(getDSLContext.configuration()) @@ -195,6 +209,9 @@ class WorkflowResourceSpec } override protected def afterAll(): Unit = { + // Restore before the teardown below, so this does not depend on `closeConnectionPool` + // staying non-throwing (today it swallows any `Exception` itself). + FulltextSearchQueryUtils.usePgroonga = pgroongaBeforeWrite closeConnectionPool() }
