aglinxinyuan opened a new pull request, #7872: URL: https://github.com/apache/texera/pull/7872
### What changes were proposed in this PR? `WorkflowLifecycleManagerSpec` goes from 3 tests to 11, covering the deferred clean-up of a finished workflow: refreshing the deadline when a later terminal state arrives, postponing while a user is connected, and cancelling the pending deadline when a user reconnects before it fires. Measured with one fresh sbt JVM per run, `rm -rf` on the jacoco dir first, and an identical suite-name filter. The "before" figure was re-measured here from HEAD's 3-test spec rather than taken from the build report. | Metric | Before | After | |---|---|---| | Codecov (fully-covered lines) | 14/28 = 50.0% | **25/28 = 89.3%** | | Missed lines | 3 | **0** | | JaCoCo line-hit | 25/28 | **28/28** | | Branch arms | 19 covered / 21 missed | **33 covered / 7 missed** | | Instructions | 194/340 | **332/340** | ### Verification 28 mutations, **27 killed, 1 survivor.** The first draft reported no survivors. That was false: **fourteen semantic survivors existed** against its 10-test version, found across two adversarial passes. The tests that failed to discriminate did so for an instructive reason — several asserted exact log messages, which only proves the interpolants are not *interchangeable*, not that the values are right. **The survivor, with its price tag:** hardcoding line 66's user-count interpolant to the literal `2` survives 11/11. The declined-clean-up message is read at exactly one user count, and the only path into `cleanUp` with a user present requires holding the manager's monitor. Killing it would cost more than it is worth; stated rather than papered over. ### Where review was wrong, and it is worth recording Two reviewer findings did not survive checking: - One reported a mutation as passing 10/10. **Not reproducible on this tree** — it fails two tests, and one of them was already in the pre-review bundle. - Another described a kill mechanism that does not hold: the mutation neuters only one line, while a *different* line in `refreshDeadline` still produces the asserted effect. The consequence claim attached to it was also too strong — the leak it described needs the reconnecting user to stay connected past a further condition. A third suggestion was implemented but with tolerance added: an exact-boundary comparison between a fired-at instant and a logged deadline crosses pekko's `nanoTime` wheel and the wall clock, so it carries 200ms of slack. And one of my own test names asserted an accident as a requirement — "does not schedule a second clean-up once the first has completed" is a consequence of `cleanUp` never resetting its flag plus pekko's post-fire `Cancellable` semantics, not a designed behaviour. Renamed. ### Cost, stated plainly Suite runtime goes from 13s to **37s**. Three fixtures had to move to longer timeouts (3s and 5s) to break a collision the reviewers found — with a short timeout and a user count of 1, a timeout mutation and a count mutation produce the same observable outcome — plus one new 3.5s test. There is no injectable clock: `cleanUpTimeout` is an `Int` in seconds and the scheduler comes through the `AmberRuntime` singleton, so the wall-clock cost is the price of testing this file without a production seam. ### Deliberately not included Lines 33, 40 and 86 stay partial, so the file cannot exceed 25/28 without a production change — line 33 is the Scala 2.13 lazy-val bitmap re-check inside `logger$lzycompute`, and the others are similar guards. Line 97 (`setCleanUpDeadline(newState.state)`, inside a subscribe lambda) is absent from JaCoCo's tracked set entirely: the report lists exactly 28 lines and 97 is not among them, because `SyntheticFilter` drops the enclosing lambda. Nothing outside `WorkflowLifecycleManager.scala` was pinned — in particular not `ExecutionStateStore`'s replay semantics or `AmberRuntime`'s scheduler. No production file is touched. Both `git diff -- '*/src/main/*'` and `git diff --cached -- '*/src/main/*'` are empty, and the production sha256 is byte-identical to the pre-mutation snapshot. ### Any related issues, documentation, discussions? Closes #7869 ### How was this PR tested? ``` sbt "WorkflowExecutionService/testOnly org.apache.texera.web.WorkflowLifecycleManagerSpec" ``` ``` [info] Total number of tests run: 11 [info] Tests: succeeded 11, failed 0, canceled 0, ignored 0, pending 0 ``` `Test/scalafmtCheck` passes. ### 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]
