sangkyoonnam opened a new pull request, #1298: URL: https://github.com/apache/flink-agents/pull/1298
Linked issue: #1294 ### Purpose of change #### User-visible outcome On the JDK < 21 parallel engine, an async call that leaves its worker interrupted keeps its result or original exception if re-acquisition succeeds, and subsequent Action code uses its own runner context. Cancellation no longer produces an uncaught `IllegalStateException: Current thread does not own the lock` from releasing an unowned lock. The intent is to preserve the call's outcome while keeping shutdown waits interruptible. #### Runtime flow `executeAsync` and nonempty `executeAllAsync` batches release the parallel lock and re-acquire it in `finally` through `reacquireAndRestore`. The helper records and clears the existing flag, calls `acquireByWorker`, restores an initially set flag in `finally`, and restores runner context only if acquisition succeeds. `Work.execute()` records action failures instead of throwing them. On return, `runOneTask` skips `markDone`, mailbox completion and `releaseToMain` only when the executor is shut down and the worker lacks ownership. Otherwise scheduling continues. #### Key decisions - Clear the existing flag once instead of retrying acquisition. A later interrupt can still abort acquisition, so the interrupts that `close()` repeats once per second during shutdown can end the wait. - Limit the ownership guard to shutdown. A worker returning without ownership while the coordinator is running still reaches the existing ownership checks and fails visibly. ### Behavioral Semantics #### Interaction decisions The table covers lock-backed calls. An interrupted acquisition means `acquireByWorker` throws `InterruptedException`. | Flag at re-acquisition entry | Acquisition | Coordinator | Outcome | |---|---|---|---| | Clear | Succeeds | Either | Original outcome kept; context restored | | Set | Succeeds | Either | Original outcome kept; context restored; flag set again | | Either | Interrupted | Shut down | Original outcome replaced; task abandoned without completion or lock release | | Either | Interrupted | Running | Original outcome replaced; existing ownership checks fail | #### Behavioral contracts 1. With the flag set at re-acquisition entry, successful re-acquisition preserves a returned result. 2. Under the same conditions, the call's original exception is propagated. 3. A flag already set at re-acquisition entry remains set on return or exception. 4. Before an Action resumes after successful re-acquisition, its runner context is restored. 5. Shutdown interruption of re-acquisition leaves the task uncompleted, without a mailbox failure or uncaught lock-ownership exception. #### Failure behavior Interrupted re-acquisition raises `InterruptedException`, replacing the call's outcome; context restoration is skipped. `Work.execute()` records the failure. During shutdown, `runOneTask` abandons the unowned task. Outside shutdown, the existing ownership checks still fail. Call failures otherwise propagate after successful re-acquisition and restoration; restoration failures still propagate. ### Tests | Contract | Coverage | |---|---| | 1. Result kept | `ContinuationActionExecutorTest.executeAsyncKeepsResultWhenCallLeavesInterruptFlagSet` | | 2. Exception kept | `ContinuationActionExecutorTest.executeAsyncKeepsFailureWhenCallLeavesInterruptFlagSet` | | 3. Flag preserved | Both executor tests above assert the flag on exit | | 4. Context restored | Both executor tests check the restoration callback; actual runner-context values are not asserted | | 5. Shutdown abandons cleanly | `ParallelExecutionCoordinatorTest.shutdownDuringAsyncReacquireAbandonsTaskWithoutLockErrors` | Coverage targets outcome preservation, interrupt visibility and clean cancellation. A Java chat-call cancellation check on Flink 2.3.0 also showed no ownership error. Not verified: - Interrupt-flag handling through `executeAllAsync` specifically. - A fresh interrupt during executor re-acquisition outside shutdown. - Actual runner-context values after resumption. - Deterministic shutdown interruption through the real executor and coordinator together. <details> <summary>Ownership invariants and supporting test evidence</summary> - The executor tests assert that the restoration callback runs under worker ownership. An unowned shutdown task skips completion and `releaseToMain`. - The coordinator test uses `FakeWork` that returns without ownership after interrupted re-acquisition; it asserts no commit, mailbox failure or uncaught error. - Focused suites: `ContinuationActionExecutorTest`, `ParallelExecutionCoordinatorTest` and `ParallelExecutionLockTest`. JDK 11/17: 33 tests pass; JDK 21: 27 pass and all 6 executor tests are skipped because they require the JDK < 21 implementation. - `mvn -pl runtime test` passes on JDK 17 (1355 tests) and JDK 21 (1365). - Flink 2.3.0, JDK 17-built distribution, Java `WorkflowSingleAgentExample`, cancellation with a pending chat request: `CANCELED`, no lock-ownership or uncaught error. </details> ### API Public signatures are unchanged. Existing JDK < 21 parallel callers retain outcomes when an existing interrupt flag previously caused re-acquisition to fail. JDK 21+ and execution without the parallel lock are unchanged. `ParallelExecutionLock.isHeldByCurrentThread()` is package-private. ### Documentation - [ ] `doc-needed` - [x] `doc-not-needed` - [ ] `doc-included` ### Was this patch authored or co-authored using generative AI tooling? - [x] Yes - [ ] No Generated-by: Claude Code 2.1.303 (Claude Opus 5.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]
