DanielLeens commented on PR #11727:
URL: https://github.com/apache/seatunnel/pull/11727#issuecomment-5385709929
Flagging for the record: a new commit landed after my last comment —
`51c6a46a` ("Merge branch 'dev' into fix/11679-blockingworker-start-latch",
2026-08-22T19:00), on top of the `72268def` head my Issues 1 and 2 above were
written against. That's technically a new-commit trigger for a fresh full
re-review, so I want to be explicit about that rather than quietly treating it
as covered.
That said, I diffed the two heads before writing this: `git diff
72268def..51c6a46a -- TaskExecutionService.java
TaskDeployStaleContextRaceTest.java` is empty — the merge pulls in unrelated
`dev` changes (docs, workflows, other connectors) with zero net change to
either file this fix touches. So the code my Issues 1 and 2 describe is
byte-identical at the new head, and both remain open:
1. **Cross-generation eviction race (High)** — `taskDone()`'s key-only
`executionContexts.remove(taskGroupLocation)` and `recycleClassLoader()`'s
independent re-`get()` can still let a stale generation's cleanup evict and
release a newer, live generation's context and class loaders, since
`TaskGroupLocation` is reused verbatim across restore generations by design.
2. **Residual NPE / silent-null path (Medium)** —
`getClassLoaders().get(t.getTaskID())` still isn't guarded against a concurrent
`setClassLoaders(null)` from the race in (1), and a missing per-task entry
installs a silent null context class loader instead of failing fast.
@SEZ9's from-scratch review at this same head (`51c6a46ae449`) independently
reconfirms both at the same locations (`TaskExecutionService.java:1430` /
`:1481-1494` for the eviction race, `:1096` for the NPE/silent-null path), with
the same suggested direction I'd already converged on: make cleanup
ownership-aware (pin the specific `TaskGroupContext` at deploy time, atomic
`remove(key, value)`, skip recycle on identity mismatch) rather than key-only.
So: no new findings from me this round, but also no change to my position —
these two are in the same method this PR touches and the same race family the
fix targets, and I'd still like to see them closed here rather than deferred.
Given the diff is a no-op on the reviewable code, I'm not re-running the full
review template again; happy to do a fresh pass once an actual fix commit lands
for either issue.
--
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]
