fix(internal): avoid panic and order-dependent concat for nil map values - #1182
Open
Lcos-000 wants to merge 1 commit into
Open
fix(internal): avoid panic and order-dependent concat for nil map values#1182Lcos-000 wants to merge 1 commit into
Lcos-000 wants to merge 1 commit into
Conversation
ConcatItems panics with SIGSEGV when merging map[string]any stream chunks in which the same key is nil in an earlier chunk and set in a later chunk. The reverse ordering returns a misleading type-mismatch error instead. Both make concat order-dependent for the same stream content. concatMaps collects every value (including nil) into rms[key] and hands the whole slice to toSliceValue. reflect.TypeOf(nil) yields a nil reflect.Type, which makes reflect.SliceOf(nil) panic; a trailing nil instead triggers the type-mismatch branch. Filter nil values in concatMaps before toSliceValue: nil means the key is absent in that chunk and does not contribute. All-nil keeps the key with a zero value (SetMapIndex with nil would delete the key); one non-nil takes that value; multiple non-nil concat as before. This makes concat order- independent for the same content and removes the unreachable panic path. Closes cloudwego#1181
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.
ConcatItems panics with SIGSEGV when merging map[string]any stream chunks in which the same key is nil in an earlier chunk and set in a later chunk. The reverse ordering returns a misleading type-mismatch error instead. Both make concat order-dependent for the same stream content.
concatMaps collects every value (including nil) into rms[key] and hands the whole slice to toSliceValue. reflect.TypeOf(nil) yields a nil reflect.Type, which makes reflect.SliceOf(nil) panic; a trailing nil instead triggers the type-mismatch branch.
Filter nil values in concatMaps before toSliceValue: nil means the key is absent in that chunk and does not contribute. All-nil keeps the key with a zero value (SetMapIndex with nil would delete the key); one non-nil takes that value; multiple non-nil concat as before. This makes concat order-independent for the same content and removes the unreachable panic path.
What type of PR is this?
/kind fix
Check the PR title.
concatMaps collects every value (including nil) into rms[key] and hands the whole slice to toSliceValue. reflect.TypeOf(nil) yields a nil reflect.Type, which makes reflect.SliceOf(nil) panic; a trailing nil instead triggers the type-mismatch branch.
Filter nil values in concatMaps before toSliceValue: nil means the key is absent in that chunk and does not contribute. All-nil keeps the key with a zero value (SetMapIndex with nil would delete the key); one non-nil takes that value; multiple non-nil concat as before. This makes concat order- independent for the same content and removes the unreachable panic path.
Closes #1181
What type of PR is this?
Check the PR title.
(Optional) Translate the PR title into Chinese.
(Optional) More detailed description for this PR(en: English/zh: Chinese).
en:
zh(optional):
(Optional) Which issue(s) this PR fixes:
(optional) The PR that updates user documentation:
PR title:
fix(internal): avoid panic and order-dependent concat for nil map values(Optional) Translate the PR title into Chinese.
修复合并
map[string]any流式分片时因同一 key 出现 nil 值导致的 panic 与顺序相关行为。(Optional) More detailed description for this PR(en: English/zh: Chinese).
en:
internal.ConcatItemsis the unified stream-chunk concat entry shared byschema.ConcatMessages,schema.ConcatAgenticMessages(content-blockExtra), and graph state merging. It panics with SIGSEGV (unrecoverable, no recover up the call stack) when mergingmap[string]anychunks where the same key isnilin an earlier chunk and a non-nil value in a later chunk; the reverse ordering returns the misleading errorunexpected slice element type. Got string, expected <nil>and rejects a legitimately concatable stream. So merging the same content yields different results depending purely on chunk ordering.Root cause:
concatMapscollects every value (includingnil) per key intorms[key]and hands the whole[]anytotoSliceValue.reflect.TypeOf(nil)yields anilreflect.Type;reflect.SliceOf(nil)then SIGSEGVs (line 210). A trailingnilinstead hitstyp != vt(line 216) and returns the type-mismatch error.Fix: filter
nilinconcatMapsbefore callingtoSliceValue—nilmeans the key is absent in that chunk and does not contribute. Then:SetMapIndexwith anilinterface would delete the key, as the original single-nil branch already handled);This makes concat order-independent for identical content and removes the unreachable panic path.
toSliceValue's "type mismatch" path now only fires for genuine heterogeneous conflicts (e.g.stringvsint), not fornil.Behavior after fix:
[{"a":nil},{"a":"str"}]{"a":"str"}[{"a":"str"},{"a":nil}]Got string, expected <nil>{"a":"str"}[{"a":nil},{"a":nil}]{"a":nil}(key preserved)[{"a":nil},{"a":"foo"},{"a":"bar"},{"a":nil}]{"a":"foobar"}[{"a":{"x":nil}},{"a":{"x":"v"}}]{"a":{"x":"v"}}All existing
internal/concat_test.gocases still pass; 6 new sub-tests cover both orderings plus nested-map and all-nil variants.zh(optional):
internal.ConcatItems是流式分片合并的统一入口,被schema.ConcatMessages、schema.ConcatAgenticMessages(内容块Extra)以及图状态合并共同调用。当合并map[string]any分片时,若同一 key 在较早分片中为nil、在较晚分片中为非 nil 值,会触发 SIGSEGV(栈上无 recover,整个进程崩溃);反向顺序则返回误导性错误unexpected slice element type. Got string, expected <nil>,拒绝一个本可正常合并的流。即:对同一内容合并,结果纯粹取决于分片顺序。根因:
concatMaps会把每个 key 的全部值(含nil)累积进rms[key],整体交给toSliceValue。reflect.TypeOf(nil)返回nilreflect.Type,reflect.SliceOf(nil)随即 SIGSEGV;末位nil则命中typ != vt报类型不匹配错误。修复: 在
concatMaps调用toSliceValue之前过滤掉nil(语义: nil 表示该 key 在该分片中缺席、不参与合并)。随后:SetMapIndex会删 key,原单 nil 分支已如此处理);由此合并对相同内容与顺序无关,并消除不可达的 panic 路径。
toSliceValue的"类型不匹配"错误只对真正的异型冲突(如stringvsint)触发,不再因nil误报。internal/concat_test.go既有用例全部通过,新增 6 个子测试覆盖两种顺序与嵌套 map、全 nil 变体。(Optional) Which issue(s) this PR fixes:
Fixes #1181
(optional) The PR that updates user documentation:
N/A: internal bug fix, no public API change.