fix(kotlin): keep the SSE flow alive when a deep payload overflows the parser stack - #2502
fix(kotlin): keep the SSE flow alive when a deep payload overflows the parser stack#2502NathanTarbert wants to merge 3 commits into
Conversation
Python Preview PackagesVersion
Install with uvAdd the TestPyPI index to your [[tool.uv.index]]
name = "testpypi"
url = "https://test.pypi.org/simple/"
explicit = trueThen install the packages you need: # Core SDK
uv add 'ag-ui-protocol==0.0.0.dev1787767693' --index testpypi
# Integrations (each already depends on the matching ag-ui-protocol preview)
uv add 'ag-ui-langgraph==0.0.0.dev1787767693' --index testpypi
uv add 'ag-ui-crewai==0.0.0.dev1787767693' --index testpypi
# NOTE: ag-ui-agent-spec depends on pyagentspec (git-only, not on PyPI).
# You will need to install pyagentspec separately from its git repo.
uv add 'ag-ui-agent-spec==0.0.0.dev1787767693' --index testpypi
uv add 'ag_ui_adk==0.0.0.dev1787767693' --index testpypi
uv add 'ag_ui_strands==0.0.0.dev1787767693' --index testpypiInstall with pippip install \
--index-url https://test.pypi.org/simple/ \
--extra-index-url https://pypi.org/simple/ \
ag-ui-protocol==0.0.0.dev1787767693
Commit: 87a140a |
@ag-ui/a2a-middleware
@ag-ui/a2ui-middleware
@ag-ui/event-throttle-middleware
@ag-ui/mcp-apps-middleware
@ag-ui/mcp-middleware
@ag-ui/a2a
@ag-ui/adk
@ag-ui/ag2
@ag-ui/agno
@ag-ui/aws-strands
@ag-ui/claude-agent-sdk
@ag-ui/claude-managed-agents
@ag-ui/crewai
@ag-ui/langchain
@ag-ui/langgraph
@ag-ui/llamaindex
@ag-ui/mastra
@ag-ui/pydantic-ai
@ag-ui/vercel-ai-sdk
@ag-ui/watsonx
@ag-ui/a2ui-toolkit
create-ag-ui-app
@ag-ui/client
@ag-ui/core
@ag-ui/encoder
@ag-ui/proto
commit: |
contextablemark
left a comment
contextablemark
left a comment
There was a problem hiding this comment.
There is one portability and safety blocker here.
The new catch (e: Error) is in commonMain, but it solves a JVM-specific failure mode by swallowing the entire platform Error hierarchy. On the JVM that includes fatal conditions such as OutOfMemoryError, linkage failures, and VM/internal errors, so the SSE parser could silently drop a payload and continue after the process is already compromised.
The regression test has the same mismatch: it lives in commonTest, but assumes 100,000 levels of recursion produce a recoverable StackOverflowError. Kotlin/Native does not expose that JVM exception; overflowing its native stack can terminate the test process instead, and the release allTests path includes iOS targets.
Please avoid catching all Errors in shared code. Either reject excessive nesting in a platform-neutral way and test that from commonTest, or isolate narrowly caught JVM overflow handling and its regression test in the JVM source sets.
catch (e: Error) is gone from commonMain. On the JVM it took the whole platform Error hierarchy, so an OutOfMemoryError or a linkage failure would be logged as a parse failure and the stream would carry on from a process that is already compromised. And it did nothing for the targets that need it most: Kotlin/Native has no JVM StackOverflowError, and overflowing its stack can end the process with nothing to catch. The nesting is counted before the deserializer is handed the payload, which is the only form of this check that means the same thing on every target. The scan is iterative, because a recursive depth check fails the way the parser it protects fails, and it tracks string state with escapes so that brackets inside a string are data rather than structure. MAX_JSON_DEPTH is 512: far above the few levels the protocol emits and above the arbitrary customer JSON that rides in state snapshots, far below what threatens any target's stack. Tests moved off the JVM assumption and now assert behaviour that holds everywhere: an over-nested payload is dropped and the stream keeps delivering, a 64-level payload is accepted, and brackets inside a string do not count. That middle test is deliberately not written against MAX_JSON_DEPTH. Derived from the constant it scales with it and passes for any value, including one too low to carry real payloads; mutation-testing caught that, since setting the limit to 2 failed nothing until the depth was made absolute. jvmTest 271 across the three modules, 0 failures, on OpenJDK 21.0.12.1 matching the workflow's java-version. compileKotlinIosSimulatorArm64 succeeds, so the common code builds for a Native target. iOS tests could not be run here: the test link step needs full Xcode and only CommandLineTools is installed, so CI's allTests is the first place those execute. Refs #2442
|
Hey @contextablemark, You were right on both halves, and the portability one is the more serious of the two: the patch solved a JVM failure mode in shared code and did nothing for the targets where the same payload is worst. Kotlin/Native has no JVM I took your first option. The nesting is counted before the deserializer is handed the payload, which is the only shape of this check that means the same thing on every target, and it lives in Two details in the implementation that are there deliberately. The scan is iterative, because a recursive depth check fails exactly the way the parser it protects fails. And it tracks string state with escape handling, so brackets inside a string are data rather than structure — without that, a message whose text happens to contain a run of
Worth reporting one thing mutation-testing caught, because it is the kind of test that looks like coverage and is not. My "accepts nesting up to the limit" case was written as On verification, and being precise about the boundary rather than claiming more than I ran. Mutations, each killed: disabling the guard fails the over-nesting test, ignoring string state fails the brackets-in-strings test, and setting the limit to 2 fails the 64-level test. |
2 participants
The Kotlin SDK's SSE parser now survives a payload deep enough to overflow the parser's stack, instead of the flow being torn down under the collector.
Thanks @ez-lbz. The report was precise about why the existing guard misses this, which is the whole bug.
Sequence
SseParser.parseFlowwrapsjson.decodeFromStringincatch (e: Exception). A deeply nested payload raisesStackOverflowError, which is anError, not anException, so it goes straight past the handler and out of the flow.Change
A second
catch (e: Error)arm alongside the existing one.This started as
catch (e: Throwable)with aCancellationExceptionrethrow, and mutation testing is what changed it: deleting that rethrow failed no test, so it was an untested guard sitting in the patch.catch (e: Error)is narrower, lives in the Kotlin common stdlib so it stays multiplatform-safe (unlikeStackOverflowError, which is JVM-only), and structurally cannot swallowCancellationException, since that is anException. Nothing in the final patch is uncovered.Tests
New
SseParserTest.parseFlow_survivesDeeplyNestedJsonsends a 100,000-deep nested array followed by a validTextMessageStartEvent. Before the change it fails withjava.lang.StackOverflowError at StringJsonLexer.kt:43, matching the reported trace.Full
jvmTestacross all three modules after the change: core 125, client 133, tools 11, so 269 tests, 0 failures. Baseline:kotlin-client:jvmTestwas 132, 0 failures. OpenJDK 21.0.12.1, matching thejava-version: "21"in.github/workflows/publish-kotlin-sdk.yml; Gradle 8.14.2 via the committed wrapper.Mutation-checked: narrowing to
Exceptionfails the new test, deleting thecatch (e: Error)block fails it, and narrowingErrortoNotImplementedErrorfails it.Verified on the JVM target only. The change is in
commonMainandErroris common stdlib, so it compiles everywhere, but the Native, JS and iOS targets were not built or run locally.On the rest of the issue
This covers the Kotlin item only. The Java, Dart and Rust items in #2442 sit with different owners and different toolchains, and I have suggested on the issue that it be split per SDK so none of them blocks the others.
Checklist
Refs #2442