GH-5971: Add failing regression test for streaming observation stop order#6317
Open
BK202503 wants to merge 1 commit into
Open
GH-5971: Add failing regression test for streaming observation stop order#6317BK202503 wants to merge 1 commit into
BK202503 wants to merge 1 commit into
Conversation
…ervation stop order) Encodes the expected lifecycle described in spring-projects#5971 as an assertion that under streaming `spring.ai.advisor` must stop before its parent `spring.ai.chat.client` (LIFO). The test captures observation start/stop events through a custom `ObservationHandler` registered on a `TestObservationRegistry` and is currently `@Disabled` because the bug in spring-projects#5971 makes it fail with the inverse order: [spring.ai.advisor must stop before spring.ai.chat.client] Expecting actual: 1 to be less than: 0 `ChatClient.prompt(...).stream()` produces: stop spring.ai.chat.client <- outer fires first under the bug stop spring.ai.advisor <- inner fires second while the inverse order is expected (`.doFinally` callbacks fire from inner to outer along the operator chain). Once the lifecycle is fixed the `@Disabled` can be removed and the test serves as a regression guard. Signed-off-by: BK202503 <199436087+BK202503@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a
@Disabledregression test that encodes the expected observation lifecycle for #5971.Background
Per #5971,
ChatClient.prompt(...).stream()stops its observations in the wrong order:spring.ai.chat.clientfires its.doFinally(... .stop())before the innerspring.ai.advisor, instead of the LIFO order one would expect for nested observations.What this PR adds
A new test in
DefaultChatClientTests,streamingChatClientStopsObservationsInLifoOrder, that:ObservationHandleron aTestObservationRegistrycapturing everyonStartandonStopevent by name.ChatClient.prompt("hello").stream().chatResponse().blockLast()against a mockedChatModel.spring.ai.chat.clientandspring.ai.advisorwere started, and thatspring.ai.advisor's stop event appears beforespring.ai.chat.client's in the captured sequence.Without the fix the test fails as expected:
i.e. the observed
stopEventsis["spring.ai.chat.client", "spring.ai.advisor", ...]—chat.clientstops at index 0 (first),advisorat index 1 (second).Why
@DisabledThe bug in #5971 has not yet been root-caused on
main, so the test is kept@Disabledfor now to avoid red CI. The@Disabledmessage references the issue and asks the next contributor who fixes the lifecycle to flip the annotation off.Scope
spring-ai-client-chat).main(commit at HEAD when the test was written) using theChatModelmock pattern already established in this file.Follow-ups
The other half of #5971 —
spring.ai.toolobservations gettingnullparent in streaming — is not covered by this PR. The tool-call path involves aSchedulers.boundedElastic()boundary insideToolCallingAdvisor.handleToolCallRecursion, and the Reactor context thatToolCallReactiveContextHolderreads from there appears not to carry the parent observation that the call (blocking) path inherits viaObservation.observe(...). Happy to fold a second@Disabledtest for that on top of this PR if maintainers prefer one PR over two.