Skip to content

fix(anthropic): preserve response IDs across streaming chunks - #3420

Open
CCCLL wants to merge 2 commits into
agentscope-ai:mainfrom
CCCLL:fix/anthropic-stream-response-id
Open

CCCLL wants to merge 2 commits into
agentscope-ai:mainfrom
CCCLL:fix/anthropic-stream-response-id

Conversation

@CCCLL

@CCCLL CCCLL commented Oct 5, 2026 •

Copy link
Copy Markdown

AgentScope-Java Version

2.1.0-SNAPSHOT, based on main@960d7e46c88ed8567398361b69be6fb8c24e81c6.

Description

Anthropic provides the message ID in message_start, but the parser only kept it in a local variable for that event. Later text, tool-call, and usage chunks received generated UUIDs from ChatResponse.Builder. As ReasoningContext adopts each chunk's ID, the final assistant message lost the provider ID.

Retain the ID in the existing per-subscription stream state so all chunks and the final accumulated message keep the same ID. Missing or blank IDs continue to work as in #3394: retain the builder-generated fallback within a subscription and generate a separate fallback for each new subscription.

Related to #2231; this fixes only its Anthropic streaming parser gap. The OpenTelemetry response attribute, model-call event payload, and HTTP request-id headers are outside this change. #2233 also changes the private stream state class, so that overlap may need coordination.

Scope discussion: #2231 (comment).

The patch adds six fixture-based regression cases through the public parser API and ReasoningContext, covering provider ID propagation, missing/empty/blank IDs, repeated subscriptions, and overlapping subscriptions. All six fail on the original parser and pass with the fix. English and Chinese Anthropic documentation now describe the response ID behavior.

Validation on JDK 21 (Java 17 compilation target):

  • mvn -T1 -pl :agentscope-extensions-model-anthropic -am test: Core 2,966 tests, 9 existing skips; Anthropic 238 tests, 1 existing skip. No failures or errors. This also runs the configured Spotless check.
  • npm test in docs: 13 passing tests.
  • npm run validate in docs: passed, including the 432-page/584-redirect check and Mintlify build validation.
  • npm run broken-links in docs: passed.
  • git diff --check: passed.

The full repository reactor and live Anthropic API calls were not run. Maven used an isolated local repository and public repository settings; no build files or dependency versions changed.

Checklist

  • Code has been formatted with mvn spotless:apply (limited to the changed Java files).
  • Core and Anthropic module tests are passing with the existing skips noted above.
  • Javadoc comments are complete and follow project conventions.
  • Related English and Chinese documentation has been updated.
  • Code is ready for review.

@oss-maintainer oss-maintainer left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Preserves the Anthropic msg_... id across streaming chunks by widening the per-subscription holder (StreamUsageState → StreamState) and carrying ChatResponse.getId() forward between events. The root cause is real and the fix is in the right place: previously messageId was a local variable reset to null on every event, so only the message_start-derived chunk carried an id and every text/tool/usage chunk fell back to a freshly generated UUID — which in turn made ReasoningContext accumulate chunks under mismatched ids. Structure, javadoc and the en/zh docs pages are all consistent, and the new AnthropicStreamingResponseIdTest covers provider id, absent/blank/whitespace id, per-subscription fallback isolation and overlapping subscriptions. CI: compile + all 6 new tests and the 221 module tests pass (BUILD SUCCESS); the build (ubuntu-latest) red X is the codecov/codecov-action@v4 upload failing with EPROTO ... ssl/tls alert handshake failure on a tokenless fork upload, and the windows-latest job was cancelled as a consequence — not caused by this change.

Two concurrency/scoping points below are worth addressing before merge, hence COMMENT rather than APPROVE (they are not correctness blockers for the common single-message_start path).

Findings

  • [Warning] AnthropicResponseParser.java:152-158 — cross-event mutable state is read/written through flatMap, which does not document per-subscription sequencing; handle (or concatMap) expresses the intended 1:0..1 sequential mapping.
  • [Warning] AnthropicResponseParser.java:332 — unconditional write-back makes the id last-writer-wins; a stream with more than one message_start (gateway resumption / pause_turn continuation) can have the new message tagged with the previous id.
  • [Info] AnthropicResponseParser.java:142-147 — rename and javadoc look good; consider private fields and/or scoping the id hand-off to message_start explicitly.

Suggestions

Replace the flatMap + filter pair with a single handle (see inline comment) so the state carry-over is unambiguously per-subscription and sequential, and add one test with two message_start events in a single stream to lock down the message-boundary behaviour.

Notes for the author

  • No CLA bot status is reported for this repo (cla_status=unknown), so nothing to sign on our side — thanks for opening this, and for the well-scoped tests.
  • Please consider re-running the pipeline (/rerun or a maintainer-approved re-run) for the codecov step; the coverage upload failure is unrelated to your change and usually clears on retry.

Automated review by github-manager-bot

Comment on lines 158 to 164
.flatMap(
event -> {
try {
return Flux.just(
parseStreamEvent(event, startTime, usageState));
parseStreamEvent(
event, startTime, streamState));
} catch (Exception e) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Warning] StreamState is now read and written from every event of a subscription, but the events still flow through flatMap (default concurrency 256). flatMap is not guaranteed to be sequential the way map is, so the cross-event carry-over (streamState.messageId written at L332, read at L186) relies on a happens-before edge that flatMap does not document; the same applies to the existing token counters.

Since each event maps to at most one ChatResponse (Flux.just(...) / Flux.empty()), the whole thing can be expressed with handle, which is strictly per-subscription and sequential:

return eventFlux.handle(
        (event, sink) -> {
            try {
                ChatResponse response = parseStreamEvent(event, startTime, streamState);
                if (response != null
                        && (!response.getContent().isEmpty() || response.getUsage() != null)) {
                    sink.next(response);
                }
            } catch (Exception e) {
                log.warn("Error parsing stream event: {}", e.getMessage());
            }
        });

That also folds the filter(...) into one operator. If you prefer to keep flatMap, please note the per-subscription/single-thread confinement assumption in the javadoc of StreamState.

ChatResponse response =
ChatResponse.builder().id(messageId).content(contentBlocks).usage(usage).build();
// Retain the generated fallback as well when the provider omits the message id.
streamState.messageId = response.getId();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Warning] Writing the resolved id back on every event makes StreamState.messageId last-writer-wins across message boundaries. In a stream that carries more than one message_start (gateway stream_resumption / pause_turn continuations, or a replayed/concatenated stream), a content event emitted before the second message_start has been processed writes its old id back and the new message can end up tagged with the previous msg_... id.

Two cheap hardening options:

  1. keep the write here, but re-scope the state on message_stop so a new message generation never inherits the previous id; or
  2. only let resolveMessageId(...) on message_start establish the id, and never write back an id derived from a filtered-out event.

The new tests cover re-subscription and overlapping subscriptions nicely — a case with two message_start events in one stream would lock down this boundary as well.

@CCCLL

CCCLL commented Oct 5, 2026

Copy link
Copy Markdown
Author

Thanks for the careful review!

flatMap → handle: Adopted. For the record, the previous code was already sequential: the flatMap mapper runs inside onNext, which Reactive Streams §1.3 requires to be signalled serially with a happens-before relationship. The concurrency of 256 bounds inner subscriptions, not mapper invocations, and Flux.just(x) is a scalar emitted synchronously. So this change doesn't alter behaviour, but handle states the 1:0..1 sequential intent more clearly and folds in the filter.

Multiple message_start events: The id is not last-writer-wins across message boundaries. On message_start the id is always re-established via resolveMessageId(...) instead of being inherited from StreamState, and when the provider omits it, ChatResponse.Builder generates a fresh UUID. Since events are processed serially, any content event before the second message_start really does belong to the first message. I added two tests that lock this down:

  • switchesToTheNextProviderIdAtEachMessageStart
  • generatesAFreshFallbackIdAtEachMessageStartWithoutProviderId

Both pass on the original implementation as well. I didn't take option 2 (never writing back an id that came from a filtered-out event). When the provider omits the id, the fallback is generated on the message_start response, which is then filtered out. Without the write-back, every later chunk would get its own UUID, which is exactly the bug this PR fixes. A pause_turn continuation is a new request and therefore a new subscription with a fresh StreamState, which generatesANewFallbackIdForEachSubscription already covers.

Field visibility: I kept the fields as they were so the diff stays focused on the id fix. The outer class can access them either way.

@CLAassistant

CLAassistant commented Oct 5, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@codecov

codecov Bot commented Oct 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@CCCLL
CCCLL force-pushed the fix/anthropic-stream-response-id branch from 5d40df4 to dc7d704 Compare November 2, 2026 13:28
@mintlify

mintlify Bot commented Nov 2, 2026

Copy link
Copy Markdown

Preview approval required

This pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit dc7d704.

Approve preview

@jujn jujn added the P2 重要不紧急 label Nov 4, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 重要不紧急

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants