fix(persistence): a restart no longer flattens every compact encoding - #840
Merged
Conversation
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
|
Warning Review limit reachedNext included review available in 40 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (6)
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. Comment |
RDB decode rebuilt every container in its full form, so a listpack hash, a listpack list, an intset and a set listpack all came back as hashtable/linkedlist/hashtable on the first reload. Redis preserves all of them across DEBUG RELOAD. The memory cost is not incidental. The SADD-builds-a-listpack win (#787, set 404.5 B/key) reverted to 978.2 B/key -- 1.85x back to 4.48x vs redis 7.4.2 -- the moment the server came back up, which makes every small-container encoding win in this campaign conditional on this fix. ## The trap: two decoders, and the obvious one is not the one that runs `src/persistence/rdb.rs` has TWO decode paths. `read_entry_zero_copy` is a second, hand-rolled decoder that builds each container inline and never calls `value_codec::decode_value_body`. Hooking `decode_value_body` alone therefore changed nothing observable: the restart path -- including `load_from_bytes`, the AOF RDB preamble -- goes through `read_entry_zero_copy`, and the guard test still failed with all four types flattened. Both funnels are now covered. ## Change - `value_codec::compact_after_decode` re-derives the compact encoding against thresholds already in the tree (LISTPACK_MAX_ENTRIES, LISTPACK_MAX_ELEMENT_SIZE, INTSET_MAX_ENTRIES). - Applied at `read_entry_zero_copy`'s single Entry funnel and, via `decode_value_body_compacting`, at the `value_codec` path. - No wire-format change: every listpack variant already maps to the same `ValueType` tag as its full form (`value_type_of`), so files stay readable in both directions. They are not byte-identical -- a listpack preserves insertion order where a HashMap does not. - `HashWithTtl` is deliberately not compacted: a listpack carries no TTL sidecar. ## Deliberately NOT the cold/spill path `ValueKind::classify_cold` accepts only the canonical full forms, so a cold-decoded `SetListpack` falls through its `_ => Err(WrongType)` arm and answers WRONGTYPE for a perfectly valid set. The first attempt compacted the shared `decode_value_body` and turned 11 cold-tier tests red for exactly that reason. Compaction is therefore opt-in, RDB-path only. Widening `classify_cold` so the cold tier compacts too is a named follow-up, as is the `redis_rdb.rs` read side used by DUMP/RESTORE and replica full sync. ## Tests `tests/restart_preserves_compact_encoding.rs` writes one key of each compact type plus a 200-field hash as a negative control -- without it, a bug that compacted EVERYTHING would pass -- restarts a real server on the same --dir, and asserts the target encoding of all five. It asserts the post-restart encoding absolutely rather than before == after, so it is meaningful on main, where a small string set legitimately gains a listpack here. Three existing tests asserted the OLD behaviour and are corrected: persistence::rdb::tests::test_round_trip_{hash,list,set} matched RedisValueRef::Hash/List/Set and panicked "Expected hash" the moment decode started returning the compact form. They now assert the listpack variant AND keep every data assertion, so they check strictly more than before -- the encoding as well as the contents. Lib suite 5137 passed / 0 failed. Refs #787 author: Tin Dang
…re; pin the zset exclusion Fix-forward of #794 after the rebase onto v0.8.9 (6251429), per the adversarial review. Compile. The branch merged textually clean and did not compile: #803 made `SetValue = indexmap::IndexSet<Bytes>` and deleted `Listpack::to_hash_set`, and the new unit tests built a `HashSet<Bytes>` for `RedisValueRef::Set`. Four errors, all in test code. They now use `SetValue` and `to_set_value()`. Both runtimes check clean with `--all-targets`. Gate. `tests/restart_preserves_compact_encoding.rs` was `#[ignore]`d, and every `--ignored` invocation in `.github/workflows/` names a specific `--test` target, so the headline regression test ran nowhere. The `#[ignore]` is dropped: the test spawns one server twice on a reserved port in a unique `--dir`, exactly like ~60 other un-ignored suites under `tests/`, so it now runs in every leg that runs `cargo nextest run` / `cargo test` (hosted tokio Check, self-hosted monoio, both VM suites of `scripts/ci-local.sh`). The fixed 3 s sleep after `BGREWRITEAOF` is replaced with a wait for the AOF manifest `seq` to advance and the new `moon.aof.<seq>.base.rdb` to exist -- the command acks at enqueue, so `aof_rewrite_in_progress:0` can be observed before the rewrite starts, and `aof_base_size` in `INFO persistence` does not move when it finishes (measured 66 before and after a rewrite that wrote a 3 KB base; that INFO field is its own small bug, not fixed here). The reply is asserted too, so a refused rewrite fails loudly instead of "passing" on a command-log replay. Proven red against a pre-fix binary and green with the fix (outputs in tmp/perf-campaign/FIXFWD-819-794.md). Probe. The test asserts through `OBJECT ENCODING`, which reads `entry.value` via `Database::get` / `get_if_alive_any_plane` on both dispatch paths -- neither routes through `get_promoted`, so the probe cannot itself flatten the key (moon#832). Stated in the test header so nobody "improves" it into a promoting accessor. Zsets. The review asked whether the exclusion makes #793's 20.5x win restart-transient. It does -- and the arm cannot land here: `SortedSetKind::project_mut` / `project_ref` accept only `SortedSetBPTree`, and `SortedSetKind::upgrade` deliberately leaves `SortedSetListpack` alone, so a zset compacted on reload would answer WRONGTYPE to every zset command on the mutable path, `ZADD` included. `SortedSetListpack` is unreachable from every load path today (`value_codec`, `redis_rdb`, DUMP/RESTORE all rebuild the full form), so the arm would create that hazard rather than inherit it. A unit test pins the exclusion (`small_zset_is_left_in_full_form_until_the_write_ path_accepts_a_listpack`) so that lifting it is a decision taken by the change that adds the write-side upgrade arm. `INTSET_MAX_ENTRIES` (512) had two private definitions and a comment promising they agree; it is now one `pub const` in `storage::db`, read by both the `SADD` path and the decode-side re-derivation. CHANGELOG: the rebase carried the entry into the [0.8.9] section; it is moved back under [Unreleased] and updated to describe the above. Refs #787, #832 author: Tin Dang
…layout The gate un-ignored by this branch hung for its full 30s timeout on the hosted tokio Check leg — TRY 3 FAIL, deterministic, not a flake — while passing on monoio. It waited on the `appendonlydir` manifest `seq`, which the tokio TopLevel writer never creates: it appends to one flat `<dir>/appendonly.aof` instead (src/persistence/aof/auto_rewrite.rs:59-62, "they never coexist for one server"). `manifest_seq` therefore returned None forever and the wait could only time out. Confirmed on disk: a tokio server with --appendonly yes writes `<dir>/appendonly.aof` and no `appendonlydir` at all. Replaces the manifest-only read with a layout-independent `Base` fingerprint — manifest `seq` where that layout exists, otherwise the flat file's (len, mtime), which a rewrite replaces wholesale. The manifest arm keeps its existing requirement that the published `moon.aof.<seq>.base.rdb` also exists, since the seq line lands before the file is fsynced into place. The gate still discriminates. Against an unfixed origin/main binary it fails in 0.56s naming every flattened encoding — "h: was listpack, after restart hashtable ... si: was intset, after restart hashtable" — rather than timing out, so a future regression is reported as itself and not as a hang. tokio 4.42s pass, monoio 5.09s pass, unfixed main FAILED as designed.
TinDang97
force-pushed
the
fixfwd/794
branch
from
September 7, 2026 16:29
2f63167 to
dac7a7f
Compare
This was referenced Sep 7, 2026
TinDang97
added a commit
that referenced
this pull request
Sep 7, 2026
…ywhere (#850) `cargo clippy` without `--all-targets` never looks at `tests/`, `benches/` or `examples/`. Every clippy invocation in ci.yml is lib-only, and so are ci-local's four clippy legs, so nothing in this repo has ever linted that code. What that cost, in order: moon#835 two errors sat unnoticed: tests/busy_poll_idle.rs:83 (collapsible_if) and src/io/fd_table.rs:153 (manual_contains). The second is cfg(target_os = "linux"), so it is invisible to a macOS run as well — three local runs agreed it was clean because all three shared the same blind spot. moon#849 fixed both errors. It did not add a gate. moon#840 added a third the same day it merged: tests/restart_preserves_compact_encoding.rs:97, collapsible_if. Three separate agents reported main red on `clippy --all-targets` while a lib-only run said clean. Fixing errors without a gate just resets the clock. This adds `Clippy (all targets)` to the Check job and fixes #840's lint. The leg runs on ubuntu-latest on purpose: fd_table.rs is Linux-gated and no macOS lint can see it, so this is the only place that covers it. Verified the gate can report its own failure — reverting the collapsed `if` gives `RC=101` and 2 errors; restored, `RC=0` and 0 errors. A gate that has not been made to fail is not evidence. Refs: #835, #840, #849
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
1 participant
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.
Reworked from PR #794 after its base moved to
6251429f.A restart flattened every compact encoding: a hash written as
listpackcameback as
hashtable, a list aslinkedlist, a set ashashtable, an intset ashashtable. The memory won by the compact encodings was surrendered on thefirst reload.
Why it needed rework
It merged textually clean and did not compile:
#803(7678156f) madeSetValue = indexmap::IndexSet<Bytes>and deletedListpack::to_hash_set(),so the branch's
HashSetsignatures were stale. Four compile errors, all fixed.A clean text rebase is not a compile.
The gate now actually runs, and can fail
It was
#[ignore]d, and every--ignoredinvocation in.github/workflows/names a specific
--testtarget — so nothing picked it up. It is nowun-ignored and runs in hosted Check, the self-hosted monoio leg, and both
ci-local.shVM suites.Proven RED against
origin/main:Green with the fix. The
hbignegative control holds on both sides, so the testdiscriminates rather than being stuck-red.
The first replacement for its 3 s sleep polled
aof_base_sizeand timed out onboth binaries — that INFO field does not move across a rewrite (measured 66
before and after). Corrected to wait on the AOF manifest
seq; the stale fieldis noted separately and not fixed here.
Sorted sets are deliberately excluded
SortedSetKind::project_mut/project_refaccept onlySortedSetBPTree, andupgradedeliberately skipsSortedSetListpack. A reload-compacted zset wouldanswer
WRONGTYPEtoZADDafter the very restart that compacted it — addingthe arm here would create the hazard, not fix it. A tripwire unit test pins
the exclusion; #793 lifts it when it adds the write-side upgrade arm.
Verification
cargo check --all-targets(monoio) and--no-default-features --features runtime-tokio,jemalloc --all-targets: both clean.restart_preserves_compact_encoding: 1 passed.Database::get/get_if_alive_any_plane,neither of which promotes — so they are not confounded by A read on the mutable path permanently flattens a compact encoding: get_promoted upgrades unconditionally, nothing downgrades #832.
Related: #787, #793, #791, #832.