DanielLeens commented on PR #11458: URL: https://github.com/apache/seatunnel/pull/11458#issuecomment-5437712751
Thanks for digging into this, @nzw921rx — this is a real gap and it's on me for not catching it in my August 25 review. I re-traced the specific path you're describing against the current head (`bbaf984dc`) and can independently confirm it: - `getCoordinatorVertexList()`/`getPhysicalVertexList()` (`SubPlan.java:609-614`) are plain unfiltered getters — they return every vertex in the pipeline regardless of status. So during a whole-job `preApplyResources(null)` (the only call issued for active-master-failover restore, via `pendingJobSchedule` -> `CoordinatorService.java:422`), `getReusableSlot` is invoked for an already-FINISHED task group's `TaskGroupLocation` exactly the same as for a still-running one. - `releaseTaskGroupResource` (`JobMaster.java:941-983`) releases the resource for a finished task group but never removes its entry from `ownedSlotProfilesIMap` — it only records the release in the in-memory `releasedSlotWhenTaskGroupFinished` map, which guards `releasePipelineResource` against double-releasing it, but only within the same `JobMaster` instance's lifetime (`JobMaster.java:993-995`). - After an active-master failover, the restored `JobMaster` is a brand-new object with an empty `releasedSlotWhenTaskGroupFinished` (`JobMaster.java:231`), so that guard is gone, while the persisted `ownedSlotProfilesIMap` entry for the finished task group survives (it's only ever fully wiped by `releasePipelineResource` for the whole pipeline, `JobMaster.java:1011`). - `slotActiveCheck` (`AbstractResourceManager.java:277-296`) and the Worker-side `DefaultSlotService#releaseSlot` (`DefaultSlotService.java:203-231`) both key off worker address + slot ID + sequence + owner job ID only — none of which encode task-group identity. So if that freed physical slot gets handed to a different task group of the same job before the failover, both checks pass even though the persisted mapping now points at the wrong task group. One nuance I'd add to the framing: `releaseTaskGroupResource`, `releasePipelineResource`, and `ownedSlotProfilesIMap`'s lifecycle are all pre-existing code this PR doesn't touch — the staleness itself isn't new. But this PR is what makes the scenario reachable in practice: before this fix, a fixed-slot job under an active-master failover would just stay deadlocked in PENDING (#11437) once slots were exhausted, so the whole-job restore never succeeded, and a pipeline in that state could never progress far enough afterward to reach the corrupting `releasePipelineResource` call. `getReusableSlot` is exactly the mechanism that now lets that restore succeed via a same-job/different-task-group coincidence, so I agree this needs to be closed in this PR rather than filed as a separate pre-existing-defect ticket — it sits squarely inside the normal target scenario (fixed slots + same job + active-master failover), not an obscure corner case. I'm revising my August 25 "Ready to merge" conclusion: this is a blocker. `getReusableSlot`'s reuse decision needs to validate the exact retained allocation, not just worker/slot liveness and job ownership — a generation/token minted on each successful `requestSlot` and persisted/compared during recovery, or a direct `TaskGroupLocation` check on the Worker side, both sound like reasonable directions. I'd also fold in a regression test for the release-path half of this (the `releasePipelineResource` double-release from your second focused test), not just the reuse-acceptance half, since both need to hold for the fix to be complete. @zhangshenghang — there's no new commit yet, so I'm not re-posting a full review record for this round, but this is now the primary blocker to resolve. Happy to do a full re-review once a fix is pushed. -- 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]
