Allow partial sync after loading AOF with preamble by arthurkiller · Pull Request #2366 · valkey-io/valkey

arthurkiller · 2025-07-17T14:48:40Z

Noted this was reverted in #2925.

=====================

The AOF preamble mechanism replaces the traditional AOF base file with an RDB snapshot during rewrite operations, which reduces I/O overhead and improves loading performance.
However, when valkey loads the RDB-formatted preamble from the base AOF file, it does not process the replication ID (replid) information within the RDB AUX fields. This omission has two limitations:

On a primary, it prevents the primary from accepting PSYNC continue requests after restarting with a preamble-enabled AOF file.
On a replica, it prevents the replica from successfully performing partial sync requests (avoiding full sync) after restarting with a preamble-enabled AOF file.

To resolve this, this commit aligns the AOF preamble handling with the logic used for standalone RDB files, by storing the replication ID and replication offset in the AOF preamble and restoring them when loading the AOF file.

Resolves #2677

secwall · 2025-07-25T09:59:59Z

./runtest-cluster fails on my machine on new assert with backlog (serverAssert(server.repl_backlog)).
Could we also write some tests on this? For example for positive scenario?

arthurkiller · 2025-07-26T04:47:54Z

./runtest-cluster fails on my machine on new assert with backlog (serverAssert(server.repl_backlog)). Could we also write some tests on this? For example for positive scenario?

thx for your interest. The main purpose of this PR is to clarify why we missed this in preamble-aof, and make sure adding this will not break the consistency of data and replication.

I will try add more test cases.

arthurkiller · 2025-07-28T03:22:48Z

./runtest-cluster fails on my machine on new assert with backlog (serverAssert(server.repl_backlog)). Could we also write some tests on this? For example for positive scenario?

@secwall I still can not get an error after a weekend of testing. Can you give some hints on reproducing?

secwall · 2025-07-28T08:07:58Z

Here is how I get an error:

git clone https://github.com/arthurkiller/valkey.git
cd valkey
git checkout fix/load-replid-from-preamble-base-aof
make -j
./runtest-cluster

It fails on failover loop test:

*** Crash report found in valkey_6/log.txt ***
=== VALKEY BUG REPORT START: Cut & paste starting from here ===
1555848:M 28 Jul 2025 11:02:33.690 # === ASSERTION FAILED ===
1555848:M 28 Jul 2025 11:02:33.690 # ==> aof.c:1488 'server.repl_backlog' is not true

------ STACK TRACE ------

1555848 valkey-server *
../../../src/valkey-server *:30006 [cluster](loadSingleAppendOnlyFile+0xf39)[0x5d6ce14d5af9]
../../../src/valkey-server *:30006 [cluster](loadAppendOnlyFiles+0x112)[0x5d6ce14d5cd2]
../../../src/valkey-server *:30006 [cluster](loadDataFromDisk+0x114)[0x5d6ce1443434]
../../../src/valkey-server *:30006 [cluster](main+0x378)[0x5d6ce1415778]
/lib/x86_64-linux-gnu/libc.so.6(+0x2a1ca)[0x73b7bf42a1ca]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x8b)[0x73b7bf42a28b]
../../../src/valkey-server *:30006 [cluster](_start+0x25)[0x5d6ce14171a5]

1555849 bio_close_file
/lib/x86_64-linux-gnu/libc.so.6(+0x98d71)[0x73b7bf498d71]
/lib/x86_64-linux-gnu/libc.so.6(pthread_cond_wait+0x20d)[0x73b7bf49b7ed]
../../../src/valkey-server *:30006 [cluster](bioProcessBackgroundJobs+0x1af)[0x5d6ce1516a9f]
/lib/x86_64-linux-gnu/libc.so.6(+0x9caa4)[0x73b7bf49caa4]
/lib/x86_64-linux-gnu/libc.so.6(+0x129c3c)[0x73b7bf529c3c]

1555850 bio_aof
/lib/x86_64-linux-gnu/libc.so.6(+0x98d71)[0x73b7bf498d71]
/lib/x86_64-linux-gnu/libc.so.6(pthread_cond_wait+0x20d)[0x73b7bf49b7ed]
../../../src/valkey-server *:30006 [cluster](bioProcessBackgroundJobs+0x1af)[0x5d6ce1516a9f]
/lib/x86_64-linux-gnu/libc.so.6(+0x9caa4)[0x73b7bf49caa4]
/lib/x86_64-linux-gnu/libc.so.6(+0x129c3c)[0x73b7bf529c3c]

1555851 bio_lazy_free
/lib/x86_64-linux-gnu/libc.so.6(+0x98d71)[0x73b7bf498d71]
/lib/x86_64-linux-gnu/libc.so.6(pthread_cond_wait+0x20d)[0x73b7bf49b7ed]
../../../src/valkey-server *:30006 [cluster](bioProcessBackgroundJobs+0x1af)[0x5d6ce1516a9f]
/lib/x86_64-linux-gnu/libc.so.6(+0x9caa4)[0x73b7bf49caa4]
/lib/x86_64-linux-gnu/libc.so.6(+0x129c3c)[0x73b7bf529c3c]

4/4 expected stacktraces.

------ STACK TRACE DONE ------

codecov · 2025-08-06T06:58:06Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.37%. Comparing base (54da834) to head (1d03246).
⚠️ Report is 43 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #2366      +/-   ##
============================================
- Coverage     72.64%   72.37%   -0.27%     
============================================
  Files           128      128              
  Lines         71278    70284     -994     
============================================
- Hits          51777    50866     -911     
+ Misses        19501    19418      -83

Files with missing lines	Coverage Δ
src/aof.c	`81.19% <100.00%> (+0.01%)`	⬆️
src/rdb.c	`77.17% <100.00%> (-0.39%)`	⬇️
src/rdb.h	`100.00% <ø> (ø)`
src/server.c	`88.39% <100.00%> (-0.04%)`	⬇️

... and 101 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.

ranshid · 2025-08-06T13:00:00Z

@arthurkiller can you also signoff the commits?

arthurkiller · 2025-08-06T13:12:11Z

@arthurkiller can you also signoff the commits?

@ranshid done

@secwall I've fixed this. Cause repl_buffer is created when we get a replica attached. We'd create this on data load. I'll add a tcl case for this later.

Not so sure if we use this replid from the previous RDB, will it make any side effects

arthurkiller · 2025-08-06T13:14:57Z

Here is how I get an error:

git clone https://github.com/arthurkiller/valkey.git
cd valkey
git checkout fix/load-replid-from-preamble-base-aof
make -j
./runtest-cluster

It fails on failover loop test:

*** Crash report found in valkey_6/log.txt ***
=== VALKEY BUG REPORT START: Cut & paste starting from here ===
1555848:M 28 Jul 2025 11:02:33.690 # === ASSERTION FAILED ===
1555848:M 28 Jul 2025 11:02:33.690 # ==> aof.c:1488 'server.repl_backlog' is not true

------ STACK TRACE ------

1555848 valkey-server *
../../../src/valkey-server *:30006 [cluster](loadSingleAppendOnlyFile+0xf39)[0x5d6ce14d5af9]
../../../src/valkey-server *:30006 [cluster](loadAppendOnlyFiles+0x112)[0x5d6ce14d5cd2]
../../../src/valkey-server *:30006 [cluster](loadDataFromDisk+0x114)[0x5d6ce1443434]
../../../src/valkey-server *:30006 [cluster](main+0x378)[0x5d6ce1415778]
/lib/x86_64-linux-gnu/libc.so.6(+0x2a1ca)[0x73b7bf42a1ca]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x8b)[0x73b7bf42a28b]
../../../src/valkey-server *:30006 [cluster](_start+0x25)[0x5d6ce14171a5]

1555849 bio_close_file
/lib/x86_64-linux-gnu/libc.so.6(+0x98d71)[0x73b7bf498d71]
/lib/x86_64-linux-gnu/libc.so.6(pthread_cond_wait+0x20d)[0x73b7bf49b7ed]
../../../src/valkey-server *:30006 [cluster](bioProcessBackgroundJobs+0x1af)[0x5d6ce1516a9f]
/lib/x86_64-linux-gnu/libc.so.6(+0x9caa4)[0x73b7bf49caa4]
/lib/x86_64-linux-gnu/libc.so.6(+0x129c3c)[0x73b7bf529c3c]

1555850 bio_aof
/lib/x86_64-linux-gnu/libc.so.6(+0x98d71)[0x73b7bf498d71]
/lib/x86_64-linux-gnu/libc.so.6(pthread_cond_wait+0x20d)[0x73b7bf49b7ed]
../../../src/valkey-server *:30006 [cluster](bioProcessBackgroundJobs+0x1af)[0x5d6ce1516a9f]
/lib/x86_64-linux-gnu/libc.so.6(+0x9caa4)[0x73b7bf49caa4]
/lib/x86_64-linux-gnu/libc.so.6(+0x129c3c)[0x73b7bf529c3c]

1555851 bio_lazy_free
/lib/x86_64-linux-gnu/libc.so.6(+0x98d71)[0x73b7bf498d71]
/lib/x86_64-linux-gnu/libc.so.6(pthread_cond_wait+0x20d)[0x73b7bf49b7ed]
../../../src/valkey-server *:30006 [cluster](bioProcessBackgroundJobs+0x1af)[0x5d6ce1516a9f]
/lib/x86_64-linux-gnu/libc.so.6(+0x9caa4)[0x73b7bf49caa4]
/lib/x86_64-linux-gnu/libc.so.6(+0x129c3c)[0x73b7bf529c3c]

4/4 expected stacktraces.

------ STACK TRACE DONE ------

Currently, our cluster cases can not cover this situation

I still failed get this by run cluster test

21:10:51> Wait cluster to be stable: OK
21:10:51> Master #0 still should have its replicas: OK
21:10:51> Each master should have at least two replicas attached: OK
Testing unit: 28-cluster-shards.tcl
21:10:51> (init) Restart killed instances: OK
21:10:51> Cluster nodes are reachable: OK
21:10:51> Cluster nodes hard reset: OK
21:10:51> Cluster Join and auto-discovery test: OK
21:10:55> Before slots allocation, all nodes report cluster failure: OK
21:10:55> Create a 8 nodes cluster with 4 shards: OK
21:10:55> Cluster should start ok: OK
21:10:59> Set cluster hostnames and verify they are propagated: OK
21:11:01> Verify information about the shards: OK
21:11:01> Verify no slot shard: OK
21:11:01> Kill a node and tell the replica to immediately takeover: OK
21:11:02> Verify health as fail for killed node: OK
21:11:06> Restarting primary node: OK
21:11:06> Instance #0 gets converted into a replica: OK
21:11:06> Test the replica reports a loading state while it's loading: OK
21:11:09> Regression test for a crash when calling SHARDS during handshake: OK
21:11:09> Cluster is up: OK
21:11:12> Shard ids are unique: OK
21:11:12> CLUSTER MYSHARDID reports same id for both primary and replica: OK
21:11:12> New replica receives primary's shard id: OK
21:11:12> CLUSTER MYSHARDID reports same shard id after shard restart: OK
21:11:14> CLUSTER MYSHARDID reports same shard id after cluster restart: OK
Cleaning up...
killing stale instance 2625352
killing stale instance 2625364
killing stale instance 2625395
killing stale instance 2625405
killing stale instance 2625416
killing stale instance 2625432
killing stale instance 2625443
killing stale instance 2625452
killing stale instance 2625462
killing stale instance 2625473
killing stale instance 2626003
killing stale instance 2626009
killing stale instance 2628745
killing stale instance 2628751
killing stale instance 2628757
killing stale instance 2628763
killing stale instance 2628769
killing stale instance 2628775
killing stale instance 2628782
killing stale instance 2628789
GOOD! No errors.

zuiderkwast · 2025-09-22T13:43:43Z

Great finding @arthurkiller! One of the main points of AOF is to be able to avoid full sync after a restart, so it's pretty sad that it doesn't work and isn't tested. Maybe this is why our documentation about prsistence says this:

There are many users using AOF alone, but we discourage it since to have an RDB snapshot from time to time is a great idea for doing database backups, for faster restarts, and in the event of bugs in the AOF engine.

It's too late to get this in 9.0 but I hope we can have it ready for 9.1. Will you add a test of the scenario (AOF rewrite, restart, psync)?

arthurkiller · 2025-09-22T13:48:46Z

Great finding @arthurkiller! One of the main points of AOF is to be able to avoid full sync after a restart, so it's pretty sad that it doesn't work and isn't tested. Maybe this is why our documentation about prsistence says this:

There are many users using AOF alone, but we discourage it since to have an RDB snapshot from time to time is a great idea for doing database backups, for faster restarts, and in the event of bugs in the AOF engine.

It's too late to get this in 9.0 but I hope we can have it ready for 9.1. Will you add a test of the scenario (AOF rewrite, restart, psync)?

@zuiderkwast Thank you for your attention. I almost forgot about this Pull Request :) .

I'll try to add more comprehensive tests as soon as possible. Hectic these days.

zuiderkwast · 2025-10-07T15:08:54Z

When you have time, will you add these two in some file under tests/unit/cluster/:

Replica restart with AOF load, then PSYNC.
Primary loads AOF, then replica doing partial sync from the primary.

We should use an AOF file with preamble and multiple AOF commands after the preamble to check that the offset is calculated correctly.

Signed-off-by: arthur.lee <liziang.arthur@bytedance.com>

src/aof.c

…eInfo Signed-off-by: arthur.lee <liziang.arthur@bytedance.com>

src/rdb.c

Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech> Signed-off-by: Arthur Lee <arthurkiller@users.noreply.github.com> Signed-off-by: arthur.lee <liziang.arthur@bytedance.com>

zuiderkwast

Very good, thank you for your patience!

I have only very minor comments about comments. (I can apply them before merging, if you don't.)

src/rdb.c

src/aof.c

Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech> Signed-off-by: Arthur Lee <arthurkiller@users.noreply.github.com>

arthurkiller · 2025-11-07T03:11:42Z

Thanks a lot :)

The AOF preamble mechanism replaces the traditional AOF base file with an RDB snapshot during rewrite operations, which reduces I/O overhead and improves loading performance. However, when valkey loads the RDB-formatted preamble from the base AOF file, it does not process the replication ID (replid) information within the RDB AUX fields. This omission has two limitations: * On a primary, it prevents the primary from accepting PSYNC continue requests after restarting with a preamble-enabled AOF file. * On a replica, it prevents the replica from successfully performing partial sync requests (avoiding full sync) after restarting with a preamble-enabled AOF file. To resolve this, this commit aligns the AOF preamble handling with the logic used for standalone RDB files, by storing the replication ID and replication offset in the AOF preamble and restoring them when loading the AOF file. Resolves valkey-io#2677 --------- Signed-off-by: arthur.lee <liziang.arthur@bytedance.com> Signed-off-by: Arthur Lee <arthurkiller@users.noreply.github.com> Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

arthurkiller · 2025-12-04T12:50:18Z

@zuiderkwast I think this patch did not work correctly for the replica.
We loaded replication info from rdb and set it into repl_offset. But the incr aof loading did not increase this offset.

If the master has some non-idempotent command such as incrby replicated to replica and write into the Incr aof after the Base Aof, this can cause inconsistency between the replica and the master.

arthurkiller · 2025-12-04T13:07:33Z

One possible approach is replica should try to connect to the master while loading the data, only load the rdb data and try to do psync-continue with master if the master is alive. Otherwise, we should load the whole data.

…#2366)" This reverts commit 2da21d9. Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

…2925) This reverts commit 2da21d9. The implementation in #2366 made it possible to perform a partial resynchronization after loading an AOF file, by storing the replication offset in the AOF preamble and counting the bytes in the AOF command stream in the same way as we count byte offset in a replication stream. However, this approach isn't safe because some commands are replicated but not stored in the AOF file. This includes the commands REPLCONF, PING, PUBLISH and module-implemented commands where a module can control to propagate a command to the replication stream only, to AOF only or to both. This oversight led to data inconsistency, where the wrong replication offset is used for partial resynchronization as explained in issue #2904. The revert caused small merge conflicts with 3c3a196 which are solved. Fixes #2904. Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

ranshid · 2025-12-11T06:42:30Z

removing the release-notes label for now to avoid accidentally adding this to the release note as this was currently reverted in #2925

…#2366)" (valkey-io#2925) This reverts commit 2da21d9. The implementation in valkey-io#2366 made it possible to perform a partial resynchronization after loading an AOF file, by storing the replication offset in the AOF preamble and counting the bytes in the AOF command stream in the same way as we count byte offset in a replication stream. However, this approach isn't safe because some commands are replicated but not stored in the AOF file. This includes the commands REPLCONF, PING, PUBLISH and module-implemented commands where a module can control to propagate a command to the replication stream only, to AOF only or to both. This oversight led to data inconsistency, where the wrong replication offset is used for partial resynchronization as explained in issue valkey-io#2904. The revert caused small merge conflicts with 3c3a196 which are solved. Fixes valkey-io#2904. Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

arthurkiller force-pushed the fix/load-replid-from-preamble-base-aof branch from e594366 to 462e1f5 Compare July 17, 2025 14:50

arthurkiller changed the title ~~feat: add replid for base aof file with preamble~~ [Q] add replid for base aof file with preamble Jul 17, 2025

arthurkiller force-pushed the fix/load-replid-from-preamble-base-aof branch from a52ec12 to 98ad37b Compare August 6, 2025 13:03

zuiderkwast added this to Valkey 9.1 Sep 22, 2025

zuiderkwast mentioned this pull request Oct 2, 2025

Stop discouraging AOF valkey-io/valkey-doc#360

Open

zuiderkwast moved this to Todo in Valkey 9.1 Oct 2, 2025

zuiderkwast mentioned this pull request Oct 4, 2025

[NEW] Support partial sync of a replica after a restart when aof is enabled #2677

Closed

arthurkiller force-pushed the fix/load-replid-from-preamble-base-aof branch 2 times, most recently from 3e9a4fd to bf34cd1 Compare October 18, 2025 08:16

arthurkiller changed the title ~~[Q] add replid for base aof file with preamble~~ [Feat] add replid for base aof file with preamble Oct 18, 2025

arthurkiller force-pushed the fix/load-replid-from-preamble-base-aof branch 5 times, most recently from b1d8058 to 1644ee2 Compare October 20, 2025 08:55

arthurkiller added 4 commits October 20, 2025 16:56

feat: add replid for base aof file with preamble

3bd50f4

Signed-off-by: arthur.lee <liziang.arthur@bytedance.com>

fix: create replbuffer before we start feed

b05bb46

Signed-off-by: arthur.lee <liziang.arthur@bytedance.com>

fix: replication feed log and debug loadaof check cached primary first

75008d4

Signed-off-by: arthur.lee <liziang.arthur@bytedance.com>

fix: only load and change cached master on start time

0e33b42

Signed-off-by: arthur.lee <liziang.arthur@bytedance.com>

arthurkiller commented Oct 30, 2025

View reviewed changes

src/aof.c Outdated Show resolved Hide resolved

misc: de-duplicate the code by adding replicationRestoreOffsetFromSav…

abbc6a2

…eInfo Signed-off-by: arthur.lee <liziang.arthur@bytedance.com>

arthurkiller force-pushed the fix/load-replid-from-preamble-base-aof branch from 9a31911 to abbc6a2 Compare November 5, 2025 10:02

zuiderkwast reviewed Nov 5, 2025

View reviewed changes

src/rdb.c Outdated Show resolved Hide resolved

src/rdb.c Outdated Show resolved Hide resolved

Update src/rdb.c

4842aa7

Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech> Signed-off-by: Arthur Lee <arthurkiller@users.noreply.github.com> Signed-off-by: arthur.lee <liziang.arthur@bytedance.com>

arthurkiller force-pushed the fix/load-replid-from-preamble-base-aof branch from 6ff5646 to 4842aa7 Compare November 6, 2025 12:57

zuiderkwast approved these changes Nov 6, 2025

View reviewed changes

src/rdb.c Outdated Show resolved Hide resolved

src/aof.c Show resolved Hide resolved

zuiderkwast added the release-notes This issue should get a line item in the release notes label Nov 6, 2025

arthurkiller and others added 2 commits November 7, 2025 11:10

Update src/rdb.c

e3bf363

Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech> Signed-off-by: Arthur Lee <arthurkiller@users.noreply.github.com>

Update src/aof.c

1d03246

Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech> Signed-off-by: Arthur Lee <arthurkiller@users.noreply.github.com>

zuiderkwast changed the title ~~[Feat] add replid for base aof file with preamble~~ Allow partial sync after loading AOF with preamble Nov 11, 2025

zuiderkwast merged commit 2da21d9 into valkey-io:unstable Nov 11, 2025
55 checks passed

github-project-automation bot moved this from Todo to Done in Valkey 9.1 Nov 11, 2025

arthurkiller mentioned this pull request Dec 4, 2025

[BUG] Fix for psync After Restarting AOF-Enabled Instances May Introduce New Data Inconsistency #2904

Closed

zuiderkwast added a commit to zuiderkwast/placeholderkv that referenced this pull request Dec 10, 2025

Revert "Allow partial sync after loading AOF with preamble (valkey-io…

60b6411

…#2366)" This reverts commit 2da21d9. Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

zuiderkwast mentioned this pull request Dec 10, 2025

Revert "Allow partial sync after loading AOF with preamble (#2366)" #2925

Merged

ranshid removed the release-notes This issue should get a line item in the release notes label Dec 11, 2025

zuiderkwast removed this from Valkey 9.1 Feb 3, 2026

Comments

Conversation

arthurkiller commented Jul 17, 2025 • edited by enjoy-binbin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

secwall commented Jul 25, 2025

Uh oh!

arthurkiller commented Jul 26, 2025

Uh oh!

arthurkiller commented Jul 28, 2025

Uh oh!

secwall commented Jul 28, 2025

Uh oh!

codecov bot commented Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ranshid commented Aug 6, 2025

Uh oh!

arthurkiller commented Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arthurkiller commented Aug 6, 2025

Uh oh!

zuiderkwast commented Sep 22, 2025

Uh oh!

arthurkiller commented Sep 22, 2025

Uh oh!

zuiderkwast commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zuiderkwast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

arthurkiller commented Nov 7, 2025

Uh oh!

Uh oh!

arthurkiller commented Dec 4, 2025

Uh oh!

arthurkiller commented Dec 4, 2025

Uh oh!

ranshid commented Dec 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

arthurkiller commented Jul 17, 2025 •

edited by enjoy-binbin

Loading

codecov bot commented Aug 6, 2025 •

edited

Loading

arthurkiller commented Aug 6, 2025 •

edited

Loading

zuiderkwast commented Oct 7, 2025 •

edited

Loading