Conversation
The two XIDMPRECORD AOF rewrite tests enabled AOF but left aof-use-rdb-preamble at its default (yes), and reloaded the dataset with DEBUG RELOAD, which round-trips an RDB snapshot. They therefore passed without ever exercising the XIDMPRECORD lines emitted by a plain command AOF rewrite. Force a plain command AOF (aof-use-rdb-preamble no), reload with DEBUG LOADAOF, and verify after recovery that re-sending the same (pid, iid) returns the original stream ID without appending entries. The XIDMPRECORD-only test now performs its first XADD IDMP validation after recovery, so its dedup state is established solely by XIDMPRECORD before the rewrite. Sensitivity-checked: with XIDMPRECORD emission disabled in rewriteStreamObject, both tests fail as intended. Signed-off-by: kexy <77062309+kexyseeds@users.noreply.github.com>
Same defect pattern as the XIDMPRECORD AOF rewrite tests: the test used DEBUG RELOAD, which saves and reloads an RDB snapshot, so it never exercised AOF recovery. Keep the AOF in plain command format (set aof-use-rdb-preamble no before enabling AOF, so the enable-time rewrite produces a command base) and reload with DEBUG LOADAOF, which flushes the pending buffer, empties the dataset and loads the base and incr files from disk. The post-reload assertions are now carried by the AOF file contents. Signed-off-by: kexy <77062309+kexyseeds@users.noreply.github.com>
|
|
kexyseeds
changed the title
kexyseeds
changed the title
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
2 participants
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.
While investigating #15836, I noticed three stream IDMP tests in
tests/unit/type/stream.tclthat intend to verify recovery from AOF but actually recover from RDB:XIDMPRECORD AOF rewrite restores IDMPXIDMPRECORD AOF rewrite emits XIDMPRECORD for stream with IDMP from XIDMPRECORD onlyXADD IDMP set in AOFTwo problems:
BGREWRITEAOF(or enabling AOF), they reload withDEBUG RELOAD, which saves the current dataset to RDB and loads it back (src/debug.c). The generated AOF is never read, so the tests pass even if the AOF recovery path drops the IDMP state.aof-use-rdb-preambleis left at its default (yes), so the rewritten base file is an RDB preamble rather than plain commands, and theXIDMPRECORDrecords emitted byrewriteStreamObject()are never exercised.This PR:
aof-use-rdb-preamble nobefore enabling AOF (with a comment explaining why), so the rewritten AOF is plain commands and recovery goes throughXIDMPRECORD/ command replay.DEBUG RELOADwithDEBUG LOADAOF, which empties the dataset and loads the AOF files from disk — the same pattern already used intests/unit/other.tcl.XADD IDMP set in AOF, recovery rides the enable-time rewrite plus the incrementally propagatedXADDcommands, so its comment describes that mechanism rather than the explicit-rewrite one.Validation:
XIDMPRECORDemission temporarily disabled inrewriteStreamObject(), the two rewrite tests fail (the retry appends a new entry and returns a different ID). With the incr AOF emptied after the buffer is flushed to disk,DEBUG LOADAOFloses the stream and the dedup state — confirming the assertions are carried by the AOF file contents.unit/type/streampasses.One point for discussion:
aof-use-rdb-preamble nois not restored afterwards and leaks to later tests on the same server. This mirrors the pre-existingappendonly yesleak in this file (the tests keep AOF enabled). If maintainers prefer, I can move these tests to a dedicatedstart_server {overrides {appendonly yes aof-use-rdb-preamble no}}block or restore the config explicitly.Commits carry
Signed-off-by(DCO).Note
Low Risk
Test-only changes to stream IDMP persistence coverage; no production code paths modified.
Overview
Fixes stream IDMP AOF tests so they exercise real AOF recovery instead of accidentally passing via RDB.
Three tests in
stream.tcl(XADD IDMP set in AOF, and twoXIDMPRECORDrewrite cases) now setaof-use-rdb-preamble nobefore enabling AOF, forcing plain command AOF and rewrite paths that emitXIDMPRECORD/ replayXADDrather than loading IDMP state from an RDB preamble. Recovery usesDEBUG LOADAOFinstead ofDEBUG RELOAD, which previously round-tripped through RDB and never read the on-disk AOF.The rewrite tests also tighten checks after reload (e.g. dedup for a second idempotent ID, and moving duplicate assertions to post-
LOADAOFin the XIDMPRECORD-only scenario).Reviewed by Cursor Bugbot for commit a251abc. Bugbot is set up for automated code reviews on this repo. Configure here.