da-daken commented on PR #926: URL: https://github.com/apache/flink-agents/pull/926#issuecomment-5141688476
hi @weiqingy ，Thank you for the very thorough review. I have addressed the timeout configuration type, naming, and the > 0 activation behavior; kept toolActionId consistent with the previous approach; and fixed the recovery issue when switching from parallel back to serial execution. There is still one open point regarding internal exception handling in durableExecuteAllAsync. For the exception boundary of `durableExecuteAllAsync`, I prefer a **two-layer model**: some failures become `Outcome`s (caller can decide how to handle them), while others must still propagate as thrown exceptions (infrastructure / invariants). ## Layer A: Should become `Outcome` (caller can decide) | Case | Rationale | |------|-----------| | Single callable execution failure | Already `Outcome.failure`; `ToolCallAction` can record per-tool `ToolResponse` | | Batch timeout for unfinished slots | Already `Outcome.failure(TimeoutException)`; partial success is meaningful | | Per-slot result/exception serialization failure during finalize | Execution already finished; only persistence failed → that slot becomes failure, **without** affecting other successful slots’ `Outcome`s | | Per-slot deserialization failure during replay | Treat as recovery failure for that slot; can fail and re-run without aborting the whole batch | | Reconciler execution failure | Same as serial async: a per-call failure | **Principle:** If the error is **per-call, expected, and the caller can turn it into a `ToolResponse` or log**, wrap it in `Outcome`. ## Layer B: Should still throw (caller must not swallow) | Case | Rationale | |------|-----------| | `persistActionState()` failure (Fluss / state-backend IO) | Journal is untrustworthy; continuing may break durable semantics → fail fast and let Flink retry/recover | | Illegal state machine in `finalizeCallAt` (no slot / not PENDING / identity mismatch) | Usually a bug or serious state corruption; `Outcome` would hide invariant violations | | `reservePendingBatch` argument mismatch (ids/digests size) | Programming error; runtime should not swallow it | | `Error` (e.g. OOM) | Must not be caught and converted to `Outcome` | | Mailbox thread check failure | Breaks the concurrency model → must throw | | Durable path taken without `durableExecutionContext` | Configuration / lifecycle bug | I'm not sure this approach is beneficial for durableExecuteAllAsync — in that case, if ToolCallAction handles the exception here, it would mark all tool calls as failed. WDYT? -- 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]
