sangkyoonnam opened a new pull request, #1171: URL: https://github.com/apache/flink-agents/pull/1171
Linked issue: #1170 ### Purpose of change #### User-visible outcome Replaying a durable call whose recorded result is `null` now returns `null` without running the callable or the reconciler again. Previously the replay threw `NullPointerException` from `tryGetCachedResult` on every attempt, so an unfinished action that had recorded such a call before a failover failed again on each recovery attempt unless it caught the exception. The built-in async sub-agent submit is such a call: a `DurableCallable<Void>` with a reconciler. `ToolCallAction` caught the exception and reported the sub-agent as failed, so a recovered action stopped waiting for a remote run that had already started. With this change the replayed submit returns and the action moves on to the durable result wait. #### Intent `serializeDurableResult(null)` stores a success with no result payload, and `tryGetCachedResult` turned that slot into `Optional.of(null)`. An `Optional` cannot say "recorded, and the value is null", so the lookup now returns the recorded `Outcome`, the same type `readTerminalOutcomeAt` already returns for `gather`, and `null` when there is nothing to replay. #### Runtime flow `durableExecuteCompletionOnly` (reached from `durableExecute` and from `JavaRunnerContextImpl#durableExecuteAsync` without a reconciler) and the terminal-slot branch of `durableExecuteWithReconcile` (reached from both with a reconciler) call `tryGetCachedResult`. It matches the slot at the current index as before, rethrows a recorded failure as before, and otherwise returns `Outcome.success(value)`, where `value` may be `null`. Both callers return `outcome.getValue()` when the outcome is non-null. On a miss, the completion-only path executes the call; the reconcile branch only reaches the lookup for a terminal slot and still throws `IllegalStateException` if nothing matches. `RunnerContextImpl#gather` falls back to `durableExecute` in sequence, so it takes the same path. #### Key decisions - Return an outcome rather than `Optional.ofNullable(...)`. `ofNullable` would report a recorded `null` as a miss: on the completion-only path the callable would run again, repeating its side effect, and a second slot would be recorded because the call index has already advanced. - Reuse `Outcome` instead of adding a holder type. ### Behavioral Semantics #### Interaction decisions | Slot at the current index | Completion-only path | Reconcile path | |---|---|---| | Matching, success with a value | Returns the value (unchanged) | Returns the value (unchanged) | | Matching, success with `null` | Returns `null`, no execution (was `NullPointerException`) | Returns `null`, no execution or reconcile (was `NullPointerException`) | | Matching, failure | Rethrows the recorded exception (unchanged) | Rethrows the recorded exception (unchanged) | | Matching, pending | Re-executes and finalizes (unchanged) | Reconciles (unchanged) | | No slot | Executes and records (unchanged) | Appends and executes (unchanged) | | A different call | Clears later slots, executes and records (unchanged) | Clears, appends and executes (unchanged) | #### Behavioral contracts 1. On the completion-only path, a recorded success with a `null` value replays as `null` and the callable is not called again. 2. On the reconcile path, a terminal success with a `null` value replays as `null`; neither the callable nor the reconciler runs. 3. A recorded non-null success replays its value, as before. 4. A recorded failure replays by rethrowing the recorded exception, as before. #### Failure behavior No new failure path. The null-success replay `NullPointerException` is no longer reachable. A result payload that fails to deserialize still throws from `tryGetCachedResult`, as before. ### Tests #### Contracts to tests | Contract | Tests | |---|---| | 1 | `RunnerContextImplDurableExecuteTest.nullResultReplaysThroughRuntimeStateWithoutReExecution` | | 2 | `RunnerContextImplDurableExecuteTest.testDurableExecuteReconcilableReplayNullSuccess`, `JavaRunnerContextImplDurableExecuteAsyncTest.testDurableExecuteAsyncReconcilableReplayNullSuccess` | | 3 | `testDurableExecuteReconcilableReplaySuccess`, `failedChatOutcomeReplaysThroughRuntimeState` (pre-existing) | | 4 | `testDurableExecuteReconcilableReplayFailure` (pre-existing) | #### Coverage and what was not verified `mvn -pl runtime test`: 1069 run, 0 failures, 1 skipped (1066 before the three new tests). `tools/lint.sh -c` passes on JDK 11, where Spotless runs. Not verified: none of the new regression tests drives `BaseAsyncSubagentSetup#submit` itself; the async test uses the same call shape (`Void`, reconciler, `durableExecuteAsync`). No end-to-end failover of a running job was run; the first test round-trips the action state through `ActionStateSerde` instead. <details> <summary>Implementation invariants and supporting evidence</summary> - Without the `RunnerContextImpl` change exactly the three new tests fail, with `NullPointerException` from `tryGetCachedResult`; the async one through `JavaRunnerContextImpl#resolveDurableAsync` and `durableExecuteWithReconcile`. - The new tests also assert that replay records and persists nothing new: slot count 1 and no extra persist. - The four-argument `CallResult` constructor marks a slot SUCCEEDED when its exception payload is null. - `tryGetCachedResult` has no other callers or overrides in the repository. - The Python runtime returns the hit flag and the value separately, and stores `None` as pickled bytes, so it never reaches an equivalent branch. </details> ### API #### Compatibility impact No change to the `RunnerContext` interface. The `protected` helper `RunnerContextImpl#tryGetCachedResult` now returns `Outcome<T>` (or `null`) instead of `Optional<T>`; nothing in the repository overrides or calls it outside the class, but an external subclass that does would need to adapt. The persisted action-state format is unchanged. ### Documentation <!-- Do not remove this section. Check the proper box only. --> - [ ] `doc-needed` <!-- Your PR changes impact docs --> - [x] `doc-not-needed` <!-- Your PR changes do not impact docs --> - [ ] `doc-included` <!-- Your PR already contains the necessary documentation updates --> ### Was this patch authored or co-authored using generative AI tooling? <!-- Do not remove this section. Check the proper box only. --> - [x] Yes - [ ] No Generated-by: Claude Code 2.1.283 (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]
