The GitHub Actions job "Comment commands" on texera.git/main has succeeded. Run started by GitHub user tanishqgandhi1908 (triggered by tanishqgandhi1908).
Head commit for run: bf0e7779ecbe64a918beadf20181306105449677 / Xinyuan Lin <[email protected]> test(amber): pin the previous run's storage-registry clean-up (#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) --------- Signed-off-by: Xinyuan Lin <[email protected]> Co-authored-by: Copilot Autofix powered by AI <[email protected]> Report URL: https://github.com/apache/texera/actions/runs/32302478244 With regards, GitHub Actions via GitBox
