fix(runner): build a new message when saving input blobs, instead of writing into the caller's Content - #1378
Conversation
|
Hi @svetanis, thank you for your contribution! We appreciate you taking the time to submit this pull request. Currently this PR is under review by our team, we will keep you posted if any additional information is required. thank you. |
4f140c7 to
7abdc1c
Compare
MiloszSobczyk
left a comment
There was a problem hiding this comment.
Hi @svetanis, thank you for your contribution. Please look into the comments I left within my review.
7abdc1c to
d43aea5
Compare
|
Thanks for the review @MiloszSobczyk.
The branch has also been rebased against the latest |
|
Thank you for addressing my comments @svetanis. I have two more suggestions regarding the test suite. Please consider following them.
|
d43aea5 to
512047c
Compare
|
Thanks for the review, @MiloszSobczyk. Both applied. Changes in the
|
…riting into the caller's Content
512047c to
d5f28e6
Compare
…writing into the caller's Content Merge #1378 **Please ensure you have read the [contribution guide](./CONTRIBUTING.md) before creating a pull request.** ### Link to Issue or Description of Change **1. Link to an existing issue (if applicable):** - Closes: [#1377](#1377) **2. Or, if no issue exists, describe the change:** **Problem:** With `saveInputBlobsAsArtifacts(true)`, `Runner.appendNewMessageToSession` replaces each inline blob with a placeholder by writing into the parts list of the `Content` the caller passed to `runAsync` ([`Runner.java:376-382`](https://github.com/google/adk-java/blob/main/core/src/main/java/com/google/adk/runner/Runner.java#L376)). That list is never copied on the way in, so for an immutable one the `set` throws `UnsupportedOperationException` — before the model is called, and before anything is appended: | Construction | Backing list | Result | |---|---|---| | `builder().parts(List.of(text, blob))` | caller's `List.of`, uncopied | **throws** | | `builder().parts(textBuilder, blobBuilder)` | genai's own `ImmutableList` | **throws** | | `Content.fromParts(text, blob)` | `Arrays.asList` | works | **Solution:** Rewrite a **copy** of the parts list and build a new `Content` for the event. The loop moves into a helper returning both the prepared message and the saves it scheduled: ```java private record OffloadedMessage(Content message, Completable artifactSaves) { ... } private OffloadedMessage offloadInputBlobs(Session session, Content message, String invocationId) { // The runner directly saves the artifacts (if applicable) in the user message and replaces // the artifact data with a file name placeholder. List<Part> parts = new ArrayList<>(message.parts().get()); ... return new OffloadedMessage( message.toBuilder().parts(ImmutableList.copyOf(parts)).build(), saveArtifactsFlow); } ``` so `appendNewMessageToSession` becomes one decision: ```java OffloadedMessage offloaded = this.artifactService != null && saveInputBlobsAsArtifacts ? offloadInputBlobs(session, newMessage, invocationContext.invocationId()) : OffloadedMessage.unchanged(newMessage); ``` The functional change is one line — `new ArrayList<>(...)`, which always accepts `set`. Documented behaviour is unchanged: the persisted message still carries the placeholder, the blob is still saved, and the model still never sees it. ### Testing Plan **Unit Tests:** - [x] I have added or updated unit tests for my change. - [x] All unit tests pass locally. ```text Tests run: 94, Failures: 0, Errors: 0, Skipped: 0 # with the fix Tests run: 94, Failures: 5 # reverting only Runner.java ``` Nine tests added. Five fail without the fix (regression tests); four pass in both states (parity tests pinning behaviour that must not change): | Test | Without fix | |---|---| | `..._immutablePartsList_savesArtifactAndCompletes` | **fails** | | `..._partBuilderPartsList_savesArtifactAndCompletes` | **fails** | | `..._appendedEventReplacesBlobWithPlaceholder` | **fails** | | `..._doesNotModifyCallerMessage` | **fails** | | `..._storesBlobVerbatim` | **fails** | | `..._fromPartsConstruction_savesArtifactAndCompletes` | passes | | `..._disabled_keepsBlobAndSavesNothing` | passes | | `..._textOnlyMessage_passesThroughUnchanged` | passes | | `..._disabledWithTextOnlyMessage_passesThroughUnchanged` | passes | The last two cover ordinary traffic: a text-only prompt with the option on (the runner still walks the parts and, after the fix, copies them), and the default path with the option off (the rewrite is skipped entirely and the caller's message passes through uncopied). **Manual End-to-End (E2E) Tests:** A demo drives one real agent turn per row through `InMemoryRunner` (no custom wiring) against `gemini-2.5-flash`, varying only the construction and the flag, and reads back both the artifact store and the session afterwards. _Before:_ ```text Run A (immutable List.of) threw UnsupportedOperationException model calls 0 nothing appended Run B (Part.Builder varargs) threw UnsupportedOperationException model calls 0 nothing appended Run C (Content.fromParts) completed, blob offloaded, placeholder appended Run D (immutable, flag off) completed, blob reaches the model, no artifact BUG REPRODUCED ``` _After (same demo):_ ```text Run A (immutable List.of) completed model calls 1 payload back verbatim placeholder appended Run B (Part.Builder varargs) completed model calls 1 payload back verbatim placeholder appended Run C (Content.fromParts) unchanged Run D (immutable, flag off) unchanged Run E (text only, flag on) completed, nothing stored, text through untouched Run F (text only, flag off) completed, nothing stored, text through untouched FIXED ``` On every row where the offload ran, the payload came back out of storage verbatim, the model was never shown the blob, and the model **was** shown the placeholder — so this is not merely "stopped crashing". ### Checklist - [x] I have read the [CONTRIBUTING.md](./CONTRIBUTING.md) document. - [x] My pull request contains a single commit. - [x] I have performed a self-review of my own code. - [x] I have commented my code, particularly in hard-to-understand areas. - [x] I have added tests that prove my fix is effective or that my feature works. - [x] New and existing unit tests pass locally with my changes. - [x] I have manually tested my changes end-to-end. - [ ] Any dependent changes have been merged and published in downstream modules. COPYBARA_INTEGRATE_REVIEW=#1378 from svetanis:fix/runner-content-mutation d5f28e6 PiperOrigin-RevId: 959996970
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
2. Or, if no issue exists, describe the change:
Problem:
With
saveInputBlobsAsArtifacts(true),Runner.appendNewMessageToSessionreplaces each inline blob witha placeholder by writing into the parts list of the
Contentthe caller passed torunAsync(
Runner.java:376-382).That list is never copied on the way in, so for an immutable one the
setthrowsUnsupportedOperationException— before the model is called, and before anything is appended:builder().parts(List.of(text, blob))List.of, uncopiedbuilder().parts(textBuilder, blobBuilder)ImmutableListContent.fromParts(text, blob)Arrays.asListSolution:
Rewrite a copy of the parts list and build a new
Contentfor the event. The loop moves into ahelper returning both the prepared message and the saves it scheduled:
so
appendNewMessageToSessionbecomes one decision:The functional change is one line —
new ArrayList<>(...), which always acceptsset. Documentedbehaviour is unchanged: the persisted message still carries the placeholder, the blob is still saved, and
the model still never sees it.
Testing Plan
Unit Tests:
Nine tests added. Five fail without the fix (regression tests); four pass in both states (parity tests
pinning behaviour that must not change):
..._immutablePartsList_savesArtifactAndCompletes..._partBuilderPartsList_savesArtifactAndCompletes..._appendedEventReplacesBlobWithPlaceholder..._doesNotModifyCallerMessage..._storesBlobVerbatim..._fromPartsConstruction_savesArtifactAndCompletes..._disabled_keepsBlobAndSavesNothing..._textOnlyMessage_passesThroughUnchanged..._disabledWithTextOnlyMessage_passesThroughUnchangedThe last two cover ordinary traffic: a text-only prompt with the option on (the runner still walks the
parts and, after the fix, copies them), and the default path with the option off (the rewrite is skipped
entirely and the caller's message passes through uncopied).
Manual End-to-End (E2E) Tests:
A demo drives one real agent turn per row through
InMemoryRunner(no custom wiring) againstgemini-2.5-flash, varying only the construction and the flag, and reads back both the artifact storeand the session afterwards.
Before:
After (same demo):
On every row where the offload ran, the payload came back out of storage verbatim, the model was never
shown the blob, and the model was shown the placeholder — so this is not merely "stopped crashing".
Checklist