fix: FrameProcessorExecutor cancellation fixes

[clintropolis]

Description

FrameProcessorExecutor.runFully registers the processor's return future and the processor itself in two separate synchronized blocks. A cancel(cancellationId) that occurs between them ends up canceling cancelableProcessors without ever seeing the processor, but registerCancelableProcessor registered it anyway, leaving an entry that no cancel will ever remove.

To fix, this PR modifies registerCancelableProcessor to return false if the cancellationId is not present (similar to what registerCancelableFuture does already), and cancels the processor like it would have done if it had been registered properly and been 'seen' by the cancel.

[kdelay]

In the exec.execute catch block, the result of cancelableProcessors.remove(cancellationId, processor) is dropped and cancel(Collections.singleton(processor)) runs unconditionally. If a cancel(cancellationId) lands between registerCancelableProcessor returning true and that remove, its removeAll already took this processor and it will fail the outputs and call cleanup() itself, so cleanup runs twice. The finished listener and doProcessorCleanup both gate on the remove result; the same if (cancellationId == null || removed) guard would fit here.

[FrankChen021]

🟡 Changes recommended

The new registration check closes the one-shot cancel-versus-registration gap, but reusing a cancellation ID can still let an old run register after its return future was canceled. That stale processor can execute under the new registration and remain unowned, so the lifecycle race should be fixed before merging.

Reviewed 2 of 2 changed files; findings in 1 file. Coverage included the complete merge-base diff plus surrounding executor, cancellation, run-all, call-site, processor-contract, and test code.

Validation: git diff --check a3f0c577f0d985f8b69abf9d3a096b9305b461de..6b21d37b94f845a69c2a7f00ff723cc25234df21 -- processing/src/main/java/org/apache/druid/frame/processor/FrameProcessorExecutor.java processing/src/test/java/org/apache/druid/frame/processor/FrameProcessorExecutorTest.java passed. No builds, tests, installs, or formatters were run per the requested static-review scope.

Severity	Findings
P0	0
P1	0
P2	1
P3	0
Total	1

---

This is an automated review by Codex GPT-5.6-Luna(max)

After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.

[FrankChen021]

[P2] Do not admit a canceled run into a reused cancellation ID

Finding: This check only tests whether the string is currently active, so a run paused after its return future was registered can be canceled, followed by registerCancellationId(cancellationId), before it reaches this block. The old run then sees the new registration, inserts its already-canceled processor, and executes it; because its canceled future listener already ran, no later cancellation is guaranteed to remove or clean that processor, allowing canceled work and resources to survive under the new generation.

Suggestion: Bind processor registration to the same cancellation generation as the return future (or reject it when that future is already canceled), and add a cancel-then-re-register race test for the same ID.

[capistrant]

are these cancellationIDs ever actually re-used?

[capistrant]

took some time to look at this and I just don't see what could actually run into this in the existing implementation. Maybe it is a proactive suggestion? If that is the case are we better off updating the javadoc for registerCancellationId to prevent future callers from re-registering with an id they are re-using?

[clintropolis]

yea, this cannot happen right now because nothing re-uses cancel id. I guess it could if something ever did try to do this, however I also can't think of a great case for doing such a thing, it would have to be something like a retry that re-uses the executor I guess? I think i'll just add javadoc as @capistrant suggests to mention that re-using cancellationId is not supported and we can fix the problem if something ever does need to do that.

[capistrant]

I think the change looks good and that @FrankChen021 comment has a legitimate angle but I don't think we are currently exposed to running into it. I think we could maybe update some javadocs to try and prevent future callers from hitting it? Either way, Approving this fix in its current iteration

[FrankChen021]

Follow-up assessment

🟢 Approval recommended

The latest reply correctly identifies that the prior P2 is not reachable through the current in-tree callers: production registration uses a per-work-order cancellation ID or a newly created result-reader executor, and this review found no current-head defect requiring an inline response. Reusing an ID across generations remains a future contract concern, so documenting that unsupported usage in registerCancellationId is sufficient follow-up; no inline reply was useful because the author’s response addresses the reported scenario.

Reviewed 2 of 2 changed files.

---

This is an automated review by Codex GPT-5.6-Luna(max)

[FrankChen021]

🟢 Approval recommended

No actionable issues found. The cancellation-registration checks preserve cleanup ownership when cancellation races registration or initial executor submission fails. The new Javadoc explicitly disallows re-registering a canceled ID; rechecking the production callers confirms that the previously discussed ID-reuse scenario is outside their supported lifecycle, so the prior concern remains withdrawn.

Reviewed 2 of 2 changed files, starting with the incremental Javadoc change and then reviewing the complete current PR diff, surrounding executor cancellation paths, run-all cleanup, processor contracts, test helpers, and production registration callers.

Validation: scoped git diff --check against merge base 3cedc8ad05cc173905a08555ff4e3d371244e45a passed. Static review only; no builds or tests were run.

---

This is an automated review by Codex GPT-5.6-Luna(max)