aglinxinyuan opened a new pull request, #7712: URL: https://github.com/apache/texera/pull/7712
### What changes were proposed in this PR? `clearExecutionResources` -- the method that wipes the previous run's storage registry before a new execution starts -- was entered by no test. It was unreachable only by accident: `SyncExecutionResourceSpec:145-151` documents parking its fixtures under a *second* computing unit precisely to avoid this path, so the recipe existed and merely needed inverting. Tests **8 -> 10**. Lines **76/96 (79.2%) -> 84/96 (87.5%)**, branches 9/14, measured with only this spec running. The new test drives the real `initExecutionService` path under `MockTexeraDB` (zonky EmbeddedPostgres, no Docker), steering `executeWorkflow()` into a compile failure so it returns before any runtime is created, and asserts the registry rows for the previous execution are deleted -- scoped to that workflow *and* that computing unit, for the latest execution only. **This PR is small on coverage and I would rather say so than dress it up: 8 lines.** What makes it worth landing is the contract, which is currently untested and easy to break silently, and the two defects the verification turned up. ### Verification Review proposed 20 mutations. **Every one survived the baseline suite** (one exception noted below). 19 are now killed, each verified red on the named test; one is recorded as unpinnable. The recurring cause was a **degenerate fixture** -- every id was the same number: | Surviving mutation | Why nothing noticed | |---|---| | `WID.eq(cuid).and(CUID.eq(wid))` transposed | `wid == uid == cuid == 9411` made the mutated SQL byte-identical | | drop the `WID` leg | only one workflow ran on the unit | | `Some(executions.max)` -> `.min` | the unit owned exactly one execution | | delete `.eq` -> `.le` on execution id | no execution existed below the one under test | | `req.computingUnitId` -> the service's own field | request and service carried the same id | | `executionName` / `engineVersion` at the insert site | the inserted row was never read back | | drop `registerCleanUpOnStateChange` | the stubbed lifecycle manager made it unobservable | Fixed by giving every domain its own literal (wid 9411, otherWid 9412, uid 9413, cuid 9414, otherCuid 9415), seeding a second workflow on the same unit, an older execution below the one under test, and reading the newly inserted row back. ### Two defects the verification exposed **The test was cementing a bug.** Guarding the clean-up on the previous execution being terminal -- a defensible production fix -- **killed** the pre-repair test, because the fixture persisted a non-terminal previous execution and asserted it was wiped. That test would have blocked the fix. The fixture now uses a terminal previous execution, so the guard can be added without this spec fighting it. **The suite was doing a real S3 delete.** `clearExecutionResources` calls `LargeBinaryManager.deleteByExecution`, which reaches `S3StorageClient` against `http://localhost:9000`. With `SERIAL` ids the fixture's execution got **eid 1 -- `LargeBinaryManager`'s `DEFAULT_EXECUTION_ID` sentinel** -- so on a dev box running `bin/local-dev.sh up` the test recursively deleted `objects/1/` in MinIO. The baseline log shows it. Explicit non-1 eids fix the blast radius; the call itself still attempts a connection (now to a nonexistent prefix), which cannot be avoided without a production seam. ### One mutation is left alive Deleting line 406 (`LargeBinaryManager.deleteByExecution`) survives, and is recorded in the spec header rather than counted as covered. The injectable overload is `private[util]`, so the call site cannot use it, and asserting the S3 effect would mean asserting the ambient environment. ### Deliberately not included The fault-tolerance block: gated on `ApplicationConfig.faultToleranceLogRootFolder`, a `val` in a Scala `object` read at object-init with no seam, which `ApplicationConfigSpec:90` asserts is `None`. The replay block: its values are consumed only past `createAmberRuntime`, which the compile failure guarantees is never reached, so it is pinnable only by reflecting into a private field. Also reported, not tested: **`lastCompletedLogicalPlan` (line 163) is a write-only var** -- a repo-wide grep finds only its declaration and its single write, no readers. Worth recording for future coverage work on this file: JaCoCo's `SyntheticFilter` drops Scala's `$anonfun$` methods, so every lambda body here -- the lifecycle cleanup callback, the completion diff-handler, the errorHandler, both cleanup loops, the replay `foreach` -- is invisible to the coverage number. Roughly 30 lines where a test moves it by exactly zero. No production file is touched. ### Any related issues, documentation, discussions? Closes #7711 ### How was this PR tested? ``` STORAGE_ICEBERG_CATALOG_TYPE=postgres sbt "WorkflowExecutionService/testOnly org.apache.texera.web.service.WorkflowServiceSpec" ``` ``` [info] Total number of tests run: 10 [info] Tests: succeeded 10, failed 0, canceled 0, ignored 0, pending 0 ``` `Test/scalafmtCheck` and `Test/scalafix --check` both pass. ### 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]
