SORT and SORT_RO should be read-only: stop converting listpack sorted sets to btree

[rainsupreme]

SORT and SORT_RO destructively converted a listpack-encoded sorted set to btree encoding before sorting. This dates back to the original Redis "SORT by nosort" change and was an implementation convenience, not a feature.

With forkless save (#4460) a background iterator may be reading an object's value memory while the main thread executes commands. Writes are gated by bgIteration_blockClientIfRequired(), which only blocks commands classified as writes. SORT_RO is READONLY, so the conversion freed the listpack from under a background reader.

Rather than special-casing SORT_RO as a write (the PFCOUNT approach), make SORT genuinely read-only with respect to the key being sorted: handle the listpack encoding directly in the two places that consumed the btree (the BY nosort ordered walk, and the collect-all path), using the same lpSeek/zzlNext/zzlPrev idiom as ZRANGE.

User-visible change: SORT / SORT_RO no longer changes the encoding of a small sorted set, as observable via OBJECT ENCODING.

Tests: SORT and SORT_RO leave a listpack zset as listpack across ALPHA, BY nosort, LIMIT, BY pattern and GET; integer members (the listpack integer path) in both encodings; and a 50-member parity test asserting identical results between listpack and btree for the nosort/LIMIT edge cases.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 81c3c34c-71c6-4285-af61-fbb6a912386f

📥 Commits

Reviewing files that changed from the base of the PR and between 144ab36 and 4020b2b.

📒 Files selected for processing (1)

• src/sort.c

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.

---

📝 Walkthrough

Walkthrough

sortCommandGeneric now sorts LISTPACK and BTREE sorted sets in place. Tests cover encoding retention, integer-member sorting, LIMIT behavior, cluster-compatible BY/GET patterns, and equivalent results across both encodings.

Changes

Sorted set sorting

Layer / File(s)	Summary
Encoding-aware SORT paths src/sort.c	The sort path uses zsetLength and keeps the original sorted-set encoding. A helper converts LISTPACK entries to objects. LISTPACK and BTREE ranges use separate iteration logic.
Encoding and ordering validation tests/unit/sort.tcl	Tests verify encoding retention, integer-member sorting, LIMIT behavior, cluster-compatible BY/GET patterns, and matching results for LISTPACK and BTREE sorted sets.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant SortCommand as SORT/SORT_RO
  participant SortGeneric as sortCommandGeneric
  participant Zset as Sorted set
  SortCommand->>SortGeneric: Sort sorted set
  SortGeneric->>Zset: Read length and encoding
  SortGeneric->>Zset: Iterate LISTPACK or BTREE in place
  Zset-->>SortGeneric: Return sorted members

Loading

Merge Risk: ⚪ Minimal · up to 4020b

The change preserves sorted-set behavior and encoding across the reviewed paths, with no material merge-blocking risk identified.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically summarizes the main change: SORT and SORT_RO preserve listpack encoding instead of converting sorted sets to btree.
Description check	✅ Passed	The description directly explains the implementation, motivation, user-visible behavior, and test coverage for the sorted-set encoding change.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

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

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

❌ Patch coverage is 98.30508% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 80.56%. Comparing base (6225ee7) to head (4020b2b).
⚠️ Report is 11 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/sort.c	98.30%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #4698      +/-   ##
============================================
+ Coverage     80.41%   80.56%   +0.15%     
============================================
  Files           191      192       +1     
  Lines         99454   100839    +1385     
============================================
+ Hits          79975    81242    +1267     
- Misses        19479    19597     +118     

Files with missing lines	Coverage Δ
src/sort.c	96.57% <98.30%> (+0.07%)	⬆️

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

[madolson]

Madelyn approves of this message. I didn't look at the PR, but this seems fine. I am OK if only write commands change the encoding.

[rainsupreme]

The test-sanitizer-address failure is TTL expiration during forkless bgsave in tests/integration/rdb.tcl, tracked as #4611 with a fix open in #4673