DanielLeens commented on PR #11757: URL: https://github.com/apache/seatunnel/pull/11757#issuecomment-5549248730
## Re-review of the FAILED-fallthrough fix (`4336d46`) Traced this from scratch against the actual source rather than taking the commit message at face value. **The gap, confirmed:** `finishOwnedResources()` (the `completionLatch==0` path) was already correctly guarded — it only touches `taskAsyncFunctionFuture`/`timerFlushFutures` after `finishExecutionContext()` confirms this tracker still owns the active context for its `TaskGroupLocation`. But `cancelAllTask()` had no such guard, and it's reachable two ways that skip that check entirely: - `taskDone()`'s bottom fallthrough (`if (!isCancel.get() && ex != null)`), which fires on *any* task failure regardless of whether `completionLatch` has reached zero — so a sibling task in a stale, already-superseded generation can still fail and reach this line. - The `cancellationFuture.whenComplete()` handler in the constructor. Both call sites operate on `taskAsyncFunctionFuture`/`timerFlushFutures`, which are keyed by `TaskGroupLocation` alone — reused verbatim across restore generations, exactly like `executionContexts`. A stale tracker reaching either path would cancel/remove whatever the *current* generation has registered under that same key. Same corruption class this whole PR exists to fix. **The fix:** `cancelAllTask()` now takes the identical `synchronized (TaskExecutionService.this)` + `executionContexts.get(taskGroupLocation) != ownedContext` guard `finishOwnedResources()` already uses, wrapping only the two shared-map calls (`cancelAsyncFunction`, `cancelTimerFlushForTaskGroup`). I checked both of those for anything that could turn this new critical section into a contention/deadlock risk: `cancelAsyncFunction` only calls `CompletableFuture#cancel(true)` (non-blocking state transition) and `cancelTimerFlushForTaskGroup` only calls `ScheduledFuture#cancel(false)` (non-blocking, doesn't wait for the task to stop) — same non-blocking calls `finishOwnedResources` already makes under this same lock today, so this doesn't introduce a new blocking-under-lock risk, just reuses the existing coarse instance-wide monitor a third time. The per-tracker-private `blockingFutures`/`currRunningTaskFuture` cancellation stays outside the guard, correctly, since those aren't shar ed across generations. Centralizing the guard inside `cancelAllTask()` rather than only at the `taskDone()` call site is the right call — it also closes the same gap in the constructor's cancellation handler, which has an identical staleness exposure that wasn't part of the original report but is the same bug. **The test:** `testStaleFailedTaskDoneDoesNotCleanupNewerGenerationResources` uses a two-task old-generation group and fails only the first task, so `completionLatch` stays at 1 and `finishOwnedResources()` is never invoked in that call — isolating the fallthrough path cleanly from the already-covered `completionLatch==0` branch. That's exactly the gap in the existing `testStaleTaskDoneDoesNotCleanupNewerGenerationResources` I flagged (single always-successful task, never exercises `ex != null`). I confirmed by inspection that without the guard this new test would fail (`cancelAllTask` would unconditionally cancel `asyncFuture`/`timerFlushFuture`), and with it, it passes for the reason claimed. No new issues from this pass. This closes the last item outstanding from my review. Waiting on the fresh `Build` run on this head before treating CI as a satisfied gate. -- 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]
