Skip to content

fix(list): bound LTRIM work by list length - #3775

Open
PoisonGodv wants to merge 4 commits into
apache:unstablefrom
PoisonGodv:fix/ltrim-out-of-range-start
Open

PoisonGodv wants to merge 4 commits into
apache:unstablefrom
PoisonGodv:fix/ltrim-out-of-range-start

Conversation

@PoisonGodv

Copy link
Copy Markdown
Contributor

Title: fix(list): bound LTRIM work by list length

Problem

For a string of indices where start == stop and start is beyond the list end, List::Trim does not take the existing empty-range fast path. Its left-delete loop instead runs from metadata.head to metadata.head + start, without being bounded by metadata.tail. On a one-element list this creates point deletes for nonexistent list subkeys.

Minimal example:

RPUSH k a
LTRIM k 2147483647 2147483647

The expected result is a prompt OK with k removed. On the unpatched base, the regression client exceeded a one-second deadline.

A temporary diagnostic counter measured the behavior without allowing an uncontrolled write batch:

  • LTRIM k 1000000 1000000 on a one-element list completed after exactly 1,000,000 left-side Delete calls (0.48 seconds in the local ASAN run).
  • The maximum-index example reached exactly 1,000,000 left-side Delete calls in 0.27 seconds before the diagnostic deliberately stopped it. The maximum-input run was not allowed to continue, so this is not a claim that billions of completions were observed.

Valkey source aa7545348b688b469d073a01ce4836ed637577f9 returns OK immediately and deletes the key. Its implementation explicitly treats start >= length as an empty range.

Fix

After negative-index normalization, use the existing metadata-deletion path when start >= metadata.size. This is the same result as trimming to an empty list, but avoids constructing deletes for indices that cannot exist. The existing start > stop behavior and retained-range loops are unchanged.

Tests

Add TestLTRIM/LTRIM_with_maximum_start_index, using math.MaxInt32 for both indices and a one-second client deadline. The regression verifies that the resulting list length is zero, the key no longer exists, its TTL is removed (TTL == -2), and the key can be recreated with the expected contents.

Verification on macOS arm64 with Xcode Clang 21 and ASAN:

  • New regression on unpatched 84e883225ec2a2de4ed59f86a0f431377face2f2: FAIL (context deadline exceeded). An external watchdog only terminated the test server during cleanup after the client deadline.
  • Strengthened regression on the patch: PASS in both txn-context-enabled configurations.
  • Complete tests/gocase/unit/type/list package: PASS (TEST_EXIT=0).
  • go vet ./unit/type/list: PASS.
  • ./x.py check format: PASS.

Not run

  • Full C++ unit tests and the complete all-package Go suite.
  • clang-tidy and golangci-lint.
  • Tests on platforms other than macOS arm64.
  • Replication and cluster integration tests.

@github-actions

Copy link
Copy Markdown

Hi @PoisonGodv,

Thank you for your pull request. Please review our Contributing Guide.

Please make sure you understand your changes and explain your reasoning in this pull request. Low-quality pull requests may be closed.

@PoisonGodv

Copy link
Copy Markdown
Contributor Author

Could a maintainer please approve the two pending workflows for the current head 7dd48c2? CI (https://github.com/apache/kvrocks/actions/runs/43001672485) and Compatibility Tests (https://github.com/apache/kvrocks/actions/runs/43001672745) are both action_required, so the test suites have not run yet. Thank you!

@aleksraiden
aleksraiden self-requested a review November 12, 2026 09:24
@PoisonGodv

Copy link
Copy Markdown
Contributor Author

@aleksraiden, could you please approve the two pending workflows for the refreshed head b9690334e65de77de9780ec3d98451b5a9e1c1ff? CI (https://github.com/apache/kvrocks/actions/runs/43204693846) and Compatibility Tests (https://github.com/apache/kvrocks/actions/runs/43204693775) are both action_required with zero jobs started, so the test suites have not run yet. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants