[BugFix] Fix LoadChunkSpiller init race that crashes BE during load spill by sevev · Pull Request #76098 · StarRocks/starrocks

sevev · 2026-07-09T07:58:14Z

Why I'm doing:

LoadChunkSpiller::_prepare() lazily creates the spiller and its serde, but used
_spiller == nullptr as its "already initialized" flag without any lock. Multiple
memtable-flush threads share a single LoadChunkSpiller and call spill() concurrently
on the first spill (see the parallel-flush comment on LoadChunkSpiller::spill).

Because _spiller is assigned before the serde's encode context is created, a racing
thread could observe a non-null _spiller, skip initialization, and call
ColumnarSerde::serialize() while the encode context was still null. serialize() then
dereferenced the null context in _get_encode_levels() and crashed the BE/CN with
SIGSEGV. Observed on a shared-data (CN) production cluster during load spill:

PC: starrocks::spill::ColumnarSerde::serialize(...)
*** SIGSEGV (@0x50) received ... ***
    ColumnarSerde::serialize
    LoadChunkSpiller::_do_spill
    LoadChunkSpiller::spill
    lake::SpillMemTableSink::flush_chunk
    MemTable::flush
    FlushToken::_flush_memtable
    MemtableFlushTask::run

What I'm doing:

Serialize the one-time initialization in _prepare() with a std::mutex, and gate
readiness on a separate std::atomic<bool> _prepared that is stored with release
ordering only after the spiller and its serde are fully constructed; readers load
it with acquire ordering. Readiness is no longer inferred from _spiller != nullptr.
Build the spiller on a local and publish it via std::move, so a half-initialized
spiller is never observable through _spiller.
Defense in depth: ColumnarSerde::serialize() now returns an error instead of
dereferencing a null encode context, mirroring the existing null guard in
_max_serialized_size().
Add a regression test that drives many threads through the first spill of a fresh
LoadChunkSpiller and asserts all concurrent spills succeed.

What type of PR is this:

Does this PR entail a change in behavior?

Yes, this PR will result in a change in behavior.
No, this PR will not result in a change in behavior.

If yes, please specify the type of change:

Interface/UI changes: syntax, type conversion, expression evaluation, display information
Parameter changes: default values, similar parameters but with different default values
Policy changes: use new policy to replace old one, functionality automatically enabled
Feature removed
Miscellaneous: upgrade & downgrade compatibility, etc.

Checklist:

I have added test cases for my bug fix or my new feature
This pr needs user documentation (for new or modified features or behaviors)
- I have added documentation for my new feature or new function
- This pr needs auto generate documentation
This is a backport pr

Bugfix cherry-pick branch check:

I have checked the version labels which the pr will be auto-backported to the target branch
- 4.1
- 4.0
- 3.5

…pill LoadChunkSpiller::_prepare() lazily creates the spiller and its serde but used `_spiller == nullptr` as the readiness flag with no lock. Multiple memtable-flush threads share one LoadChunkSpiller and call spill() concurrently on the first spill (see the parallel-flush comment on spill()). One thread could publish `_spiller` (making it non-null) before creating the serde's encode context, while a racing thread observed the non-null spiller, skipped initialization, and called ColumnarSerde::serialize() with a null encode context. serialize() then dereferenced it in _get_encode_levels() and crashed the BE/CN with SIGSEGV. Serialize the one-time initialization with a mutex and gate readiness on a separate `_prepared` atomic that is stored with release ordering only after the spiller and its serde are fully constructed; readers check it with acquire ordering. The spiller is built on a local and published via std::move, so a half-initialized spiller is never observable through `_spiller`. As defense in depth, ColumnarSerde::serialize() now returns an error instead of dereferencing a null encode context, mirroring the existing null guard in _max_serialized_size(). Signed-off-by: sevev <qiangzh95@gmail.com>

CelerData-Reviewer · 2026-07-09T08:17:32Z

@codex review

chatgpt-codex-connector · 2026-07-09T08:21:48Z

Codex Review: Didn't find any major issues. 🎉

Reviewed commit: 0404bacfff

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

github-actions · 2026-07-09T09:54:04Z

[BE Incremental Coverage Report]

✅ pass : 18 / 19 (94.74%)

file detail

	path	covered_line	new_line	coverage	not_covered_line_detail
🔵	be/src/compute_env/spill/serde.cpp	1	2	50.00%	[111]
🔵	be/src/compute_env/load_spill/load_chunk_spiller.cpp	17	17	100.00%	[]

Address review feedback: replace the hand-rolled double-checked locking (std::mutex + std::atomic<bool>) in _prepare() with std::call_once, the idiomatic one-time-init primitive used elsewhere in the codebase. It establishes the happens-before edge for the fully-constructed spiller without any manual acquire/release reasoning. The init body still builds the spiller on a local and publishes it via std::move only after its serde's encode context is prepared, so a half-initialized spiller is never observable. The outcome is cached in _prepare_status and returned to every caller, so a failed init is reported to all concurrent flush threads instead of leaving a null/half-built spiller reachable (the crash this fix addresses). LoadChunkSpiller is per-load (a fresh unique_ptr in each SpillMemTableSink), so a cached failure never leaks into a later load. Signed-off-by: sevev <qiangzh95@gmail.com>

CelerData-Reviewer · 2026-07-13T03:24:36Z

@codex review

github-actions · 2026-07-13T03:25:18Z

Module-risk briefing for Codex review

This is review focus, not a finding list. Validate against the diff.

High-confidence risks

Module: compute_env/spill
Changed files: be/src/compute_env/spill/serde.cpp
Historical failure pattern: Recurring SEGVs/null-derefs in this module from concurrency races that break an invariant a downstream call trusts, on paths outside normal steady-state data flow (prior fixes: a cancel-path use-after-free in the sort sink, and an OOB read from a stale caller-supplied bound under concurrent mutation).
Review focus: Confirm the new std::call_once/_prepare_status gating in LoadChunkSpiller::_prepare() fully closes the window where _spiller becomes observable before its serde's encode context is built, and that ColumnarSerde::serialize()'s new null-_encode_context check is genuine defense-in-depth rather than the only safeguard — verify no other caller can reach serialize()/_get_encode_levels() before prepare() has run.
Evidence: [BugFix] Fix spill partition sort sink cancel crash #75140, [BugFix] Add some assert on PartitionedSpillerWriter #74081 (from 2 merged bugfixes)

chatgpt-codex-connector · 2026-07-13T03:28:34Z

Codex Review: Didn't find any major issues. Keep them coming!

Reviewed commit: 7308d39ded

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

github-actions · 2026-07-13T05:44:58Z

[Java-Extensions Incremental Coverage Report]

✅ pass : 0 / 0 (0%)

github-actions · 2026-07-13T05:45:01Z

[FE Incremental Coverage Report]

✅ pass : 0 / 0 (0%)

github-actions Bot added 4.1 4.0 labels Jul 9, 2026

mergify Bot assigned sevev Jul 9, 2026

github-actions Bot requested review from luohaha and stdpain July 9, 2026 08:01

sevev force-pushed the fix-load-chunk-spiller-init-race branch from bc43745 to 0404bac Compare July 9, 2026 08:15

luohaha reviewed Jul 13, 2026

View reviewed changes

Comment thread be/src/compute_env/load_spill/load_chunk_spiller.cpp Outdated

luohaha approved these changes Jul 13, 2026

View reviewed changes

sevev enabled auto-merge (squash) July 13, 2026 04:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[BugFix] Fix LoadChunkSpiller init race that crashes BE during load spill#76098

[BugFix] Fix LoadChunkSpiller init race that crashes BE during load spill#76098
sevev wants to merge 2 commits into
StarRocks:mainfrom
sevev:fix-load-chunk-spiller-init-race

sevev commented Jul 9, 2026

Uh oh!

CelerData-Reviewer commented Jul 9, 2026

Uh oh!

chatgpt-codex-connector Bot commented Jul 9, 2026

Uh oh!

github-actions Bot commented Jul 9, 2026

Uh oh!

Uh oh!

CelerData-Reviewer commented Jul 13, 2026

Uh oh!

github-actions Bot commented Jul 13, 2026

Uh oh!

chatgpt-codex-connector Bot commented Jul 13, 2026

Uh oh!

github-actions Bot commented Jul 13, 2026

Uh oh!

github-actions Bot commented Jul 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

sevev commented Jul 9, 2026

Why I'm doing:

What I'm doing:

What type of PR is this:

Checklist:

Bugfix cherry-pick branch check:

Uh oh!

CelerData-Reviewer commented Jul 9, 2026

Uh oh!

chatgpt-codex-connector Bot commented Jul 9, 2026

Uh oh!

github-actions Bot commented Jul 9, 2026

[BE Incremental Coverage Report]

file detail

Uh oh!

Uh oh!

CelerData-Reviewer commented Jul 13, 2026

Uh oh!

github-actions Bot commented Jul 13, 2026

Module-risk briefing for Codex review

High-confidence risks

Uh oh!

chatgpt-codex-connector Bot commented Jul 13, 2026

Uh oh!

github-actions Bot commented Jul 13, 2026

[Java-Extensions Incremental Coverage Report]

Uh oh!

github-actions Bot commented Jul 13, 2026

[FE Incremental Coverage Report]

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants