Ashfaqbs commented on code in PR #1071:
URL: https://github.com/apache/flink-agents/pull/1071#discussion_r3915948229
##########
plan/src/main/java/org/apache/flink/agents/plan/actions/ChatModelInvoker.java:
##########
@@ -182,6 +182,12 @@ public ChatMessage call() throws Exception {
}
return new ChatAttemptResult(
model, chatModel, response, actualRetryCount,
totalWaitTimeSec);
+ } catch (InterruptedException e) {
+ // A cancellation signal, not a model failure: restore the
interrupt status and
+ // propagate immediately so task shutdown isn't delayed by
retry backoff or an
+ // extra model call, regardless of the configured
error-handling strategy.
+ Thread.currentThread().interrupt();
+ throw e;
} catch (Exception e) {
Review Comment:
Added, in 5803bb1d. Set the interrupt flag from within the mocked
`durableExecute` call itself so `Thread.sleep` throws immediately at no
wall-clock cost, with RETRY / numRetries=1 / retryWaitIntervalSec=1 as you
suggested. Verified it's the flag assertion alone that distinguishes the fix —
temporarily restored the pre-c30ba894 sleep block and confirmed this new test
fails there (times(1)/InterruptedException-thrown still pass either way) while
the two existing tests stay green.
##########
runtime/src/main/java/org/apache/flink/agents/runtime/context/RunnerContextImpl.java:
##########
@@ -576,6 +576,12 @@ protected <T> T durableExecuteCompletionOnly(
Exception exception = null;
try {
result = executionCallable.call();
+ } catch (InterruptedException e) {
+ // A cancellation signal, not a genuine call failure: leave the
durable slot
+ // unfinished so recovery re-executes or reconciles the call
instead of replaying a
+ // stale interruption as a completed success or failure.
+ Thread.currentThread().interrupt();
+ throw e;
Review Comment:
That reasoning matches what I see in #1042:
`ModelRoutingResolver.isCancellation(...)` gets added and called from
`ChatModelAction.processChatRequest` right before the IGNORE check, and
`Strategies.llm(...)`'s judge chat call through `chatWithRetries` is what would
make this path reachable in the first place. Agreed on skipping a separate
follow-up issue, conditional on #1042 landing as-is — I'll keep an eye on it
and flag here (or file the issue myself) if it merges in a materially different
shape.
--
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]
