aglinxinyuan opened a new pull request, #7823: URL: https://github.com/apache/texera/pull/7823
### What changes were proposed in this PR? `ExecutionReconfigurationService.registerWorkerCompletionCallback` discarded the `Disposable` returned by `client.registerCallback`, so its engine callback outlived the service. It now hands it to `addSubscription(...)`, matching `registerCompletionDiffHandler` directly below it. It was the only `registerCallback` site in `org.apache.texera.web.service` not doing this: | File | sites | wrapped | |---|---|---| | `ExecutionStatsService.scala` | 5 | yes | | `ExecutionResultService.scala` | 3 | yes | | `ExecutionConsoleService.scala` | 2 | yes | | `ExecutionRuntimeService.scala` | 1 | yes | | `ExecutionReconfigurationService.scala` | 1 | **no** | ### Why this actually leaks, rather than being tidied up by something else The teardown path is live: `WorkflowExecutionService.unsubscribeAll` (`WorkflowExecutionService.scala:185`) calls `executionReconfigurationService.unsubscribeAll()`, which released every other subscription but not this one. And `client.shutdown()` is not an alternative release — it flips `isActive` and poison-pills the `ClientActor`, but never touches `registeredObservables`, which is written only in `registerCallback` and never cleared. The `PublishSubject` chain therefore keeps the subscriber closure, and through it the service, its `ExecutionStateStore` and its `Workflow`, for as long as the `AmberClient` is reachable — which `WorkflowExecutionService.client` guarantees. ### The fix is pinned New test: `"the worker completion callback" should "release the engine subscription once the service is unsubscribed"`. Verified with the production file reverted and restored: | | production reverted | with fix | |---|---|---| | `ExecutionReconfigurationServiceSpec` | **13 passed, 1 failed** | **14 passed** | The before-state failure is the right one — `ArrayBuffer() did not contain element class …UpdateExecutorCompleted`, i.e. the disposable was never disposed because it was never registered. Two additions to the existing `TestAmberClient` double were needed, and the reasons are worth stating: an explicit `disposedCallbacks` record, because disposal-removal from the `callbacks` map alone is indistinguishable from "never registered" or "cleared by `reset`"; and a `fireIfRegistered` helper, because the strict `fire` calls `fail(...)` when nothing is registered and so cannot express "a late event is inert". ### Two comments were rewritten, not just tests added Both would otherwise have contradicted the code: 1. `TestAmberClient.registerCallback`'s scaladoc said production "currently DISCARDS it" and that "no test below fires an engine event after `unsubscribeAll`, so the suite neither depends on the leak nor breaks when it is fixed." Both clauses are now false. 2. The routing comment in `"stop announcing completions once the service is unsubscribed"` called the engine callback an "unrelated, currently broken, seam". The routing decision — drive that test through `onWorkerReconfigured` rather than the engine event — is still right and unchanged, but re-justified: the two are separate subscriptions, so routing through the engine event would leave an empty batch explainable by either one being released. That test's assertion is untouched, so it stays agnostic exactly as #7692 intended. ### Verification - `ExecutionReconfigurationServiceSpec` 14/14. - Dependents and neighbours: `ExecutionRuntimeServiceSpec`, `WorkflowExecutionServiceSpec`, `ExecutionStatsServiceSpec`, `ExecutionConsoleServiceSpec`, `WorkflowWebsocketResourceSpec`, `TexeraWebSocketEventSpec` — **59/59 across 6 suites, 0 aborted**. - `scalafmtCheck`, `Test/scalafmtCheck`, `scalafixAll --check` all pass. (The one scalafix warning is a pre-existing `// scalafix:ok` in `OutputManagerSpec`, unrelated.) ### One thing deliberately left alone The suite still carries an assertion recorded as *observed, not endorsed*: an N-worker operator fires N `ModifyLogicCompletedEvent`s, which contradicts `registerCompletionDiffHandler`'s own comment claiming the frontend is notified once **all** workers finish. That is a separate defect with its own decision to make, so this PR does not touch it. ### Any related issues, documentation, discussions? Closes #7822 ### How was this PR tested? ``` STORAGE_ICEBERG_CATALOG_TYPE=postgres sbt "WorkflowExecutionService/testOnly org.apache.texera.web.service.ExecutionReconfigurationServiceSpec" ``` ``` [info] Total number of tests run: 14 [info] Tests: succeeded 14, failed 0, canceled 0, ignored 0, pending 0 ``` ### 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]
