[PoisonGodv]

Deflake Dual-channel full sync with compression delivers writes made during load test

Fixes #5003

Problem

Dual-channel full sync with compression delivers writes made during load in
tests/integration/repl-compression.tcl failed on
test-almalinux8-tls-module with:

Expected '1' to be equal to '0' (context: type eval line 23 cmd {assert_equal 1 [s 0 master_sync_in_progress]} proc ::start_server)

The test uses rdb-key-save-delay 100 with 10000 keys to create a roughly
one-second RDB transfer window. It then performs 200 small SETs and one 2 MiB
SET, and asserts that the replica is still loading and the primary is still in
state=bg_transfer. These assertions ensure that the writes exercise the
buffered compressed main-channel path.

That fixed window could close before the large SET completed on a slow runner.
The CI log shows that the 200 small writes reached the replica, but the 2 MiB
SET was still being sent after the approximately 1.9-second full sync had
finished. Waiting for that command's reply therefore allowed the subsequent
master_sync_in_progress assertion to observe a completed sync. Pipelining the
commands would not remove that wait for the large SET's reply.

Fix

Hold the replica's RDB load open until the in-load assertions have completed:

• Set key-load-delay 10000 before starting replication.
• Set loading-process-events-interval-bytes 1024 so the replica continues to
  process events and answer INFO and CONFIG SET while loading.
• Reset key-load-delay to 0 immediately after the in-load assertions.

The primary remains in bg_transfer until the replica acknowledges after
loading, so both sides stay in the intended state regardless of client speed.
The original sequential SETs through $primary, including the original large
SET, are retained; no deferring client or pipelining machinery is needed.

Apply the same hold to the related
Dual-channel load converges after primary repl-compression ... tests. They
also make fixed-window assertions after changing compression during load. The
hold keeps the initial codec state stable until those assertions finish, then
releases the load so renegotiation and convergence can proceed.

This is a test-only change.

Verification

Run locally on macOS (arm64), using the final sequential-write version.

Complete integration file:

./runtest --single integration/repl-compression --clients 1

• 27 passed, 0 failed. The runner reported one skipped test because this
  build lacks ZSTD; the other conditionally registered ZSTD-only cases were
  not run.

Reviewer's delayed-large-SET reproduction, with a temporary after 3000
immediately before the 2 MiB SET:

./runtest --single integration/repl-compression \
  --only "Dual-channel full sync with compression delivers writes made during load" \
  --loops 6 --clients 1

• 6 passed, 0 failed. The temporary delay was removed afterward.

Affected codec-transition repetitions:

./runtest --single integration/repl-compression \
  --only "Dual-channel load converges after primary repl-compression no->lz4 with lz4 replica" \
  --loops 6 --clients 1

./runtest --single integration/repl-compression \
  --only "Dual-channel load converges after primary repl-compression lz4->no with lz4 replica" \
  --loops 6 --clients 1

• 6 passed, 0 failed for each case (12 passed, 0 failed total).

These were non-TLS local runs, so TLS coverage was not exercised.

[coderabbitai]

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: f4cc775a-d562-4ad2-934a-9b38db92c808

Commits

Reviewing files that changed from the base of the PR and between 3a47a30 and c851353.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• Trigger review

No actionable comments were generated in the recent review.

Walkthrough

The dual-channel replication integration tests delay replica key loading during full sync and compression-transition cases. They set the loading event-processing interval to 1,024 bytes and reset the key-load delay after in-load assertions.

Changes: Dual-channel replication load tests

Layer / File(s)	Summary
Full-sync load delay tests/integration/repl-compression.tcl	The full-sync test delays key loading and sets the loading event-processing interval to 1,024 bytes. It resets the delay after its in-load assertions.
Compression-transition load delay tests/integration/repl-compression.tcl	The compression-transition tests delay key loading and set the loading event-processing interval to 1,024 bytes. They reset the delay after their in-load assertions.

Estimated code review effort: 2 (Simple) | ~10 minutes

Severity of issue fixed: Medium

Suggested reviewers: sarthakaggarwal97

Merge Risk: Minimal · previous review 3c0ff

The test changes are properly scoped and preserve the intended load-window coverage, with no established merge-blocking risk.

Pre-merge checks: 5

Check name	Status	Explanation
Title check		The title names the main change: a deflake fix for the dual-channel full sync compression test that checks writes made during load. It matches the changeset, which holds the replica RDB load open with…
Description check		The description states the failing test, the CI error, the cause (a fixed RDB transfer window that can close before the large SET completes), and the fix (holding replica load open with key-load-delay…
Linked Issues check		Issue #5003 requires the test Dual-channel full sync with compression delivers writes made during load to stop failing at assert_equal 1 [s 0 master_sync_in_progress]. The whole-PR diff (f2ca837 t…
Out of Scope Changes check		All changes are in tests/integration/repl-compression.tcl. No product code changed. The first hunk applies the replica load hold to the target test, which connects directly to #5003. The second hunk…
Docstring Coverage		No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…

Full details: Title check

Explanation

The title names the main change: a deflake fix for the dual-channel full sync compression test that checks writes made during load. It matches the changeset, which holds the replica RDB load open with key-load-delay, and the PR objectives, which describe the same test fix for issue #5003. The title is specific and concise.

Full details: Description check

Explanation

The description states the failing test, the CI error, the cause (a fixed RDB transfer window that can close before the large SET completes), and the fix (holding replica load open with key-load-delay and loading-process-events-interval-bytes, then resetting the delay). It also lists local verification runs. This matches the test-only changes in tests/integration/repl-compression.tcl and the linked objectives.

Full details: Linked Issues check

Explanation

Issue #5003 requires the test Dual-channel full sync with compression delivers writes made during load to stop failing at assert_equal 1 [s 0 master_sync_in_progress]. The whole-PR diff (f2ca837 to 3c0fffa) changes only tests/integration/repl-compression.tcl. For this test, the replica now sets key-load-delay 10000 and loading-process-events-interval-bytes 1024 before replicaof. It sets key-load-delay 0 after the in-load assertions. The RDB load therefore stays open for the in-load checks, so the master_sync_in_progress and state=bg_transfer assertions no longer depend on client speed. The head contains the sequential primary SETs and no deferring client or pipelining. This matches the current PR description. The previous assessment described a deferring client, which is absent at this head, so that part of the earlier conclusion is superseded. Residual uncertainty: the author's runs were non-TLS only. No TLS run and no re-execution of the test was performed in this review. The key-load-delay semantics were not re-verified in source here.

Full details: Out of Scope Changes check

Explanation

All changes are in tests/integration/repl-compression.tcl. No product code changed. The first hunk applies the replica load hold to the target test, which connects directly to #5003. The second hunk applies the same hold, with the same reset, to the Dual-channel load converges after primary repl-compression ... codec-transition tests. The PR description and objectives state this extension and its reason: those tests also make fixed-window assertions during load, which share the same timing race. This is test-only hardening within the same file. It does not alter product behavior. The bounded-frame ZSTD test is unchanged, as the PR objectives state. The extension is a scope expansion beyond the issue text, but it is not an unrelated change.

Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.)

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

• X
• Mastodon
• Reddit
• LinkedIn

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: ed513512-bbb5-4a56-b68c-0223abec3e8a

Commits

Reviewing files that changed from the base of the PR and between 570fd68 and 3c0fffa.

Files selected for processing: 1

• tests/integration/repl-compression.tcl

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

[valkey-review-bot]

Review state

Ready for human review.
The latest commit drops the pipelining and puts back the original sequential SETs. What remains is the replica load hold: key-load-delay 10000 plus loading-process-events-interval-bytes 1024, reset after the in-load assertions. That keeps both sides in the sync window, so the test no longer depends on how fast the client runs. The PR description now describes this fix, so the earlier description finding is resolved.

Risk: Low. Only Tcl integration tests in repl-compression.tcl change. If the change is wrong, the worst case is a test that stays flaky or runs slower.

Open findings: none.

Resolved (1)

• (minor) The PR description still described the pipelining-only fix and did not mention the key-load-delay hold that is now the actual fix.

Backport: Not needed. Test-only change that deflakes an integration test. There is no server code fix to backport.

_{Updated for 3c0fffa at 2026-10-29 07:17 UTC. The bot rewrites this comment on every review.}

[Baraa-Hasheesh]

@PoisonGodv I was recently checking, I'm attaching my findings on this issue

---

Note this was generated with the help of AI

Analysis

From [test-almalinux8-tls-module](https://github.com/valkey-io/valkey/actions/runs/40044203787/job/120722038514), using server log timestamps:

Time (UTC)	Side	Event
00:35:57.781	replica	REPLICAOF enabled, PRIMARY <-> REPLICA sync started
00:35:57.787	primary	Starting BGSAVE for SYNC ... dual-channel, compression: lz4
00:35:59.455	replica	Done downloading RDB
00:35:59.692	replica	PRIMARY <-> REPLICA sync: Finished with success. The window is over after ~1.9 s
00:36:02.704	replica	DB 9: 10200 keys: 10000 RDB keys + the 200 small during_load:* SETs. during_load:large (a new key) has not arrived yet
00:36:07.724	replica	DB 9: 10200 keys: the 2 MiB SET is still being sent to the primary
00:36:12.061	test	The 2 MiB SET returns, then assert_equal 1 [s 0 master_sync_in_progress] fails with Expected '1' to be equal to '0'

Why pipelining wouldn't have helped here

• The 200 small SETs had reached the replica by the first 5 s key-count sample. Nearly all of the ~12 s between the end of the sync and the assertion went into the single 2 MiB SET.
• Pipelining removes the per-command round trips, but the same 2 MiB still goes over the same slow link. The [$rd read] loop waits for the large SET's reply before assert_equal 1 [s 0 master_sync_in_progress], so the assertion would still run ~12 s after the ~1.9 s window closed.

Suggested fix: hold the window open instead of racing it

# Hold the replica's RDB load open (up to ~100 s) until the assertions below.
$replica config set key-load-delay 10000
# Process events often enough to answer INFO / CONFIG SET while loading.
$replica config set loading-process-events-interval-bytes 1024

After assertions are done you can release it

$replica config set key-load-delay 0

• The primary keeps the replica in bg_transfer until the replica's first ACK after loading, so both the replica-side and the primary-side assertions stay valid while the hold is on.
• The writes still take the buffered main-channel path that the test is meant to cover, however long the client takes.
• The Dual-channel load converges after primary repl-compression ... tests use the same fixed window followed by assertions, and need the same hold.

[pirate329]

Tested locally by delaying only the 2MB SET by 3s (after 3000 before it), which matches the CI log:

• unstable: fails with the CI error (Expected '1' to be equal to '0')
• this PR: fails 4/4
• unstable + @Baraa-Hasheesh's key-load-delay hold: passes 6/6, and 6/6 without the delay

So the window closing before the large SET is the cause, and holding the load open fixes it.

[PoisonGodv]

@Baraa-Hasheesh @pirate329 Thanks for the delayed-large-SET experiment—it showed why pipelining alone was insufficient: waiting for the large SET's reply still let the roughly 1-second load window close before the in-load assertions.

I've switched to holding the replica load open with key-load-delay 10000 and loading-process-events-interval-bytes 1024, allowing INFO/CONFIG SET processing while the primary remains in bg_transfer. The delay is reset to 0 immediately after the in-load assertions. I applied the same hold to both supported codec-transition tests; the bounded-frame ZSTD test is unchanged because it has no equivalent fixed-window post-write assertion.

Final local verification:

• ./runtest --single integration/repl-compression --clients 1: 27 passed, 0 failed; ZSTD-only coverage was skipped because this build lacks ZSTD.
• With a temporary after 3000 before the 2 MiB SET, the full-sync test with --loops 6 --clients 1: 6 passed, 0 failed.
• With a temporary after 3000 before each transition's compression flip, both supported transition tests with --loops 6 --clients 1: 12 passed, 0 failed.

The temporary delays were removed after verification. These were non-TLS local runs.

[Baraa-Hasheesh]

@PoisonGodv can you update the PR description to fit the new fix

Also please revert the pipeline changes as they are not needed

[PoisonGodv]

@Baraa-Hasheesh Done: I reverted all pipelining/deferring-client changes, restored the original sequential primary SETs and large SET, and updated the PR description to describe the replica load-hold fix.

Final verification (non-TLS local runs):

• Full integration/repl-compression: 27 passed, 0 failed; one explicit skip because this build lacks ZSTD, and conditionally registered ZSTD-only cases were not run.
• Delayed-large-SET reproduction (after 3000 immediately before the large SET), 6 loops: 6 passed, 0 failed; the temporary delay was removed afterward.
• Each supported codec-transition test, 6 loops: 6 passed, 0 failed (12 passed, 0 failed total).

[PoisonGodv]

@JimB123 This one is approved by Baraa-Hasheesh, but the CI workflows on the current head (3a47a30) are still "awaiting approval". Could you approve them when you have a moment? Thanks!

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.29%. Comparing base (58a6ddc) to head (c851353).
⚠️ Report is 2 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #5076      +/-   ##
============================================
- Coverage     81.29%   81.29%   -0.01%     
============================================
  Files           199      199              
  Lines        104095   104099       +4     
============================================
+ Hits          84625    84628       +3     
- Misses        19470    19471       +1     

see 21 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.