Skip to content

test: fix flaky DisaggReadSnapshot — skip background tasks during write+mergeDeltaAll - #10898

Merged
ti-chi-bot[bot] merged 2 commits into
masterfrom
fix-flaky-test-disagg-read-snapshot
Jun 11, 2026
Merged

test: fix flaky DisaggReadSnapshot — skip background tasks during write+mergeDeltaAll#10898
ti-chi-bot[bot] merged 2 commits into
masterfrom
fix-flaky-test-disagg-read-snapshot

Conversation

@JaySon-Huang

@JaySon-Huang JaySon-Huang commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: close #10897

Problem Summary:
DMStoreForSegmentReadTaskTest.DisaggReadSnapshot is flaky — the initial write(4096) schedules a background flush task
whose completion handler (placeDeltaIndex) and follow-up background merge delta both acquire is_updating on
DeltaValueSpace. When mergeDeltaAll() runs before these background tasks release the lock, it fails silently,
leaving 5 persisted column files instead of the expected 4.

What is changed and how it works?

Enable skip_check_segment_update failpoint before the initial write(4096) + mergeDeltaAll() block in the test,
with a scoped guard that disables it on exit. This prevents checkSegmentUpdate from scheduling any background
flush/merge tasks during the setup phase, eliminating the race entirely.

This is consistent with other tests in the same file (fetchPagesNoTinyNoInMem, fetchPagesTinyNoInMem,
fetchPagesTinyInMem) which already enable skip_check_segment_update via disableFlushCache() before their
write+mergeDeltaAll blocks.

Check List

Tests

  • Unit test

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

None

Summary by CodeRabbit

  • Tests
    • Improved reliability of segment snapshot comparison tests by adding explicit control around the stable write/merge sequence used during setup, and ensuring that control is cleaned up afterward. This reduces flaky behavior during snapshot validation and makes test outcomes more consistent.

…g setup

skip_check_segment_update prevents write() from scheduling background
flush/merge tasks, avoiding a race where background placeDeltaIndex or
merge delta holds is_updating and causes mergeDeltaAll() to fail
silently, resulting in 5 persisted column files instead of the expected
4.

Close #10897.
@ti-chi-bot ti-chi-bot Bot added the release-note-none Denotes a PR that doesn't merit a release note. label Jun 10, 2026
@pantheon-ai

pantheon-ai Bot commented Jun 10, 2026

Copy link
Copy Markdown

@JaySon-Huang I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details.

⏳ This process typically takes 10-30 minutes depending on the complexity of the changes.

ℹ️ Learn more details on Pantheon AI.

@ti-chi-bot ti-chi-bot Bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 10, 2026
@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 7f8e6bdf-7873-4f86-a918-64595d538b85

📥 Commits

Reviewing files that changed from the base of the PR and between fa539e7 and cdd75c2.

📒 Files selected for processing (1)
  • dbms/src/Storages/DeltaMerge/tests/gtest_segment_read_task.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
  • dbms/src/Storages/DeltaMerge/tests/gtest_segment_read_task.cpp

📝 Walkthrough

Walkthrough

The test DisaggReadSnapshot now enables the skip_check_segment_update failpoint before the stable write()/mergeDeltaAll() sequence and disables it via a scope guard afterward to prevent races with background flush during test setup.

Changes

Test Stabilization via Failpoint Control

Layer / File(s) Summary
Failpoint control for stable write/merge sequence
dbms/src/Storages/DeltaMerge/tests/gtest_segment_read_task.cpp
Enable skip_check_segment_update before write()/mergeDeltaAll() and disable it on scope exit via a scope guard, preventing background flush from racing with merge in the stable setup.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

A rabbit paused where tests would race,
It set a gentle failpoint in place,
Before the writes and merge took flight,
A scope-guard kept the timing right,
Now tests hop steady through the night 🐰

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The PR title clearly and concisely describes the main change: fixing a flaky test by skipping background tasks during write+mergeDeltaAll operations.
Description check ✅ Passed The PR description follows the repository template, includes issue number (#10897), explains the problem, describes the solution, and completes all required checklist items.
Linked Issues check ✅ Passed The PR successfully addresses issue #10897 by enabling the skip_check_segment_update failpoint to prevent background tasks from racing with mergeDeltaAll, eliminating the flaky test failure.
Out of Scope Changes check ✅ Passed All changes in the PR are directly scoped to fixing the flaky test by adding failpoint control; no unrelated modifications are present.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-flaky-test-disagg-read-snapshot

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 Infer (1.2.0)
dbms/src/Storages/DeltaMerge/tests/gtest_segment_read_task.cpp

dbms/src/Storages/DeltaMerge/tests/gtest_segment_read_task.cpp:15:10: fatal error: 'Common/Logger.h' file not found
15 | #include <Common/Logger.h>
| ^~~~~~~~~~~~~~~~~
1 error generated.
dbms/src/Storages/DeltaMerge/tests/gtest_segment_read_task.cpp:56:5-138:5: ERROR translating statement 'CompoundStmt'
Aborting translation of method 'DB::DM::tests::SegmentReadTaskTest::initInputStream' in file 'dbms/src/Storages/DeltaMerge/tests/gtest_segment_read_task.cpp': "Assert_failure src/clang/cAst_utils.ml:249:53"
Uncaught Internal Error: "Assert_failure src/clang/cAst_utils.ml:249:53"
Error backtrace:
Raised at ClangFrontend__CAst_utils.get_decl_from_typ_ptr in file "src/clang/cAst_utils.ml", line 249, characters 53-65
Called from ClangFrontend__CTrans.CTrans_funct.get_destructor_decl_ref in file "src/clang/cTrans.ml", line 658, characters 12-59
Called from ClangFrontend__CTrans.CTrans_funct.destructor_calls.(fun) in file "src/clang/cTrans.ml", line 2048, characters 12-69
Ca

... [truncated 2200 characters] ...

characters 6-141
Called from ClangFrontend__CFrontend_decl.CFrontend_decl_funct.add_method in file "src/clang/cFrontend_decl.ml" (inlined), line 54, characters 4-52
Called from ClangFrontend__CFrontend_decl.CFrontend_decl_funct.process_method_decl.add_method_if_create_procdesc in file "src/clang/cFrontend_decl.ml" (inlined), line 123, characters 16-158
Called from ClangFrontend__CFrontend_decl.CFrontend_decl_funct.process_method_decl in file "src/clang/cFrontend_decl.ml", line 126, characters 17-97
Called from ClangFrontend__CFrontend_decl.CFrontend_decl_funct.process_methods in file "src/clang/cFrontend_decl.ml" (inlined), line 270, characters 8-122
Called from Stdlib__List.iter in file "list.ml" (inlined), line 110, characters 12-15
Called from Stdlib__List.iter in file "list.ml" (inlin


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Signed-off-by: JaySon-Huang <tshent@qq.com>
@JaySon-Huang

Copy link
Copy Markdown
Contributor Author

/test pull-integration-test

@ti-chi-bot ti-chi-bot Bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jun 11, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JinheLin, Lloyd-Pottiger

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [JinheLin,Lloyd-Pottiger]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot Bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jun 11, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

[LGTM Timeline notifier]

Timeline:

  • 2026-06-11 05:26:25.683261882 +0000 UTC m=+1024086.753579262: ☑️ agreed by Lloyd-Pottiger.
  • 2026-06-11 08:47:20.809940879 +0000 UTC m=+1036141.880258269: ☑️ agreed by JinheLin.

@ti-chi-bot
ti-chi-bot Bot merged commit f78d49b into master Jun 11, 2026
11 checks passed
@ti-chi-bot
ti-chi-bot Bot deleted the fix-flaky-test-disagg-read-snapshot branch June 11, 2026 08:53
@JaySon-Huang

Copy link
Copy Markdown
Contributor Author

/cherry-pick release-nextgen-202603

@ti-chi-bot

Copy link
Copy Markdown
Member

@JaySon-Huang: new pull request created to branch release-nextgen-202603: #10932.

Details

In response to this:

/cherry-pick release-nextgen-202603

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot Bot pushed a commit that referenced this pull request Jun 26, 2026
…te+mergeDeltaAll (#10898) (#10932)

close #10897\n\nSigned-off-by: JaySon-Huang <tshent@qq.com>\n\nCo-authored-by: JaySon-Huang <tshent@qq.com>
JaySon-Huang added a commit to JaySon-Huang/tiflash that referenced this pull request Jun 30, 2026
…te+mergeDeltaAll (pingcap#10898)

close pingcap#10897\n\nSigned-off-by: JaySon-Huang <tshent@qq.com>
windtalker pushed a commit to windtalker/tiflash that referenced this pull request Jul 9, 2026
…te+mergeDeltaAll (pingcap#10898)

close pingcap#10897\n\nSigned-off-by: JaySon-Huang <tshent@qq.com>
JaySon-Huang added a commit to yongman/tiflash that referenced this pull request Aug 11, 2026
…te+mergeDeltaAll (pingcap#10898)

close pingcap#10897\n\nSigned-off-by: JaySon-Huang <tshent@qq.com>
@ti-chi-bot ti-chi-bot Bot added the needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. label Aug 11, 2026
@ti-chi-bot

Copy link
Copy Markdown
Member

In response to a cherrypick label: new pull request created to branch release-8.5: #11035.

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

Labels

approved lgtm needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky test: DMStoreForSegmentReadTaskTest.DisaggReadSnapshot — mergeDeltaAll races with background flush

4 participants