wenjin272 opened a new pull request, #1379: URL: https://github.com/apache/flink-agents/pull/1379
Linked issue: Fixes #1372 ### Purpose of change Completing a `ctx.chat` child Action must not make its unfinished caller's memory writes durable. Previously, the shared cache allowed a child to flush a parent counter increment before the parent completed; a checkpoint and replay could then apply that increment again. #### Runtime flow `ChatSubgraphDefinition.newInvocation()` now creates a read-through memory scope for each chat call. On the first read of a caller entry, `ReadThroughMemoryStore` copies it into the chat's local cache. Child Actions share that cache throughout the model/tool loop, and their `persistCache()` calls retain local values without flushing the caller. The caller keeps its existing Action completion and persistence path. #### Key decisions Keep access to caller memory for tool parameter injection, including uncommitted values, while isolating chat writes. Copy entries before returning them: copying only on write would still expose mutable payloads and object field lists. Reuse the existing ephemeral store rather than changing the root Action commit protocol. Internal subagents retain their fully isolated memory scope. ### Behavioral Semantics #### Interaction decisions For a checkpoint taken after chat finishes but while the caller is waiting on another asynchronous call: | State backend | Retained ActionState | Recovery behavior | | --- | --- | --- | | HashMap | No | Replay the caller and chat; the completed caller increments the counter once. | | HashMap | Yes | Replay the caller and reuse the chat result; the completed caller increments the counter once. | | RocksDB | No | Replay the caller and chat; the completed caller increments the counter once. | | RocksDB | Yes | Replay the caller and reuse the chat result; the completed caller increments the counter once. | These combinations are tested with both JDK 11 parallel execution and JDK 21 continuations. #### Behavioral contracts - Tools can still read the caller's sensory and short-term memory through parameter injection, including pending writes. - Chat writes and mutations of copied values do not modify or persist caller memory. - Checkpoint recovery does not repeat a parent memory update because a chat child committed early, with or without retained chat results. - The model/tool loop continues to work across child Actions and replay, including chat invoked inside an internal subagent. #### Failure behavior Caller-store read failures and value-copy failures propagate through existing Action error handling; there is no fallback to sharing caller values. Provider errors keep the existing chat error behavior. No new configuration, retry policy, or error suppression is introduced. ### Tests | Contract | Coverage | | --- | --- | | Read caller values without flushing pending writes | `ReadThroughMemoryStoreTest.readsPersistedAndUncommittedValuesWithoutFlushingOrChangingParent`; Java and Python chat tool-injection assertions | | Isolate writes, mutable payloads, and object field lists | `ReadThroughMemoryStoreTest`, including nested List/Map and byte-array mutations | | Keep checkpointed memory aligned with caller replay | `ChatMemoryIsolationTest.checkpointDoesNotIncludeUnfinishedCallerWrites`: HashMap/RocksDB × with/without ActionState, on JDK 11 and 21 | | Preserve child-loop progress and replay | Existing `ChatCallTest` recovery phases and low-JDK subgraph execution/failover tests; Python direct-chat and nested-subagent E2E tests | Local validation: JDK 11 focused suite **27 passed**; JDK 21 focused suite **48 passed, 1 skipped**; Python/PyFlink 2.3 E2E on JDK 21 **2 passed**. Spotless, Ruff, and diff checks passed. The final mailbox-loop simplification was also verified by rerunning all four new recovery cases on JDK 21. Not verified: live model providers, Python failure/recovery E2E, real Kafka/Fluss ActionState stores, and injected serializer-copy failures. The full repository suite was not run. <details> <summary>Implementation invariants and regression evidence</summary> - `clear()` hides the parent view and preserves subsequent local writes; covered by `clearHidesParentAndRetainsNewLocalWrites`. - Local values survive child `persistCache()` calls. Separate chat scopes do not see each other's writes. - The recovery test serializes retained ActionState before restoring, avoiding shared test objects across runs. - Restoring the old shared-cache path makes all four new checkpoint cases fail: persisted counter is `1` while the unfinished caller's checkpoint should still contain `0`. - JDK 21 validation used a clean build with continuation classes; Python E2E used rebuilt distribution JARs and branch sources. </details> ### API No public Java, Python, or YAML API changes, and no checkpoint state-format changes. Chat's internal writes no longer reach caller memory; memory-based tool injection remains available. Internal subagent isolation is unchanged. ### 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: Codex CLI 0.153.4 (GPT-6) -- 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]
