aglinxinyuan opened a new pull request, #8404:
URL: https://github.com/apache/texera/pull/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 qualifi
cation, 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 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 on
e 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 the sibling PR, 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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
