fix(universaldb): bound rocksdb range scans [SLOPFIX]

[NathanFlurry]

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[NathanFlurry]

• SPLIT UP: agentic loop test #4932
• chore(rivetkit): doc skip ready wait #4927
• fix(guard): parse gateway bypass query booleans #4926
• feat(depot-client): add vfs staging cache ttl #4930
• feat(depot-client): wire sqlite optimization flags #4923
• fix(universaldb): bound rocksdb range scans [SLOPFIX] #4929 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[claude]

Code Review: fix(universaldb): bound rocksdb range scans

Overview

This PR fixes two related bugs in universaldb:

1. Primary bug (in tx_ops.rs): Local (uncommitted) writes were being merged into range results using only the raw anchor key of the selector (key >= begin && key < end), completely ignoring the or_equal and offset parameters. This meant, for example, a first_greater_than(k) begin selector was incorrectly including the key k itself.

2. Rocksdb scan refactor (in transaction_task.rs): Removes resolve_key_selector_for_range, which had incomplete handling for offset=0 cases (fell through to _ => Ok(key.to_vec())), and replaces it with client-side filtering using the new shared helpers.

---

What's Good

• The core fix for tx_ops.rs is correct. The old key >= begin && key < end ignored selector semantics; the new range_contains properly handles all four standard selector forms.
• Extracting range_begin_contains / range_end_contains as shared pub(crate) helpers avoids duplicating the logic between tx_ops.rs and transaction_task.rs.
• Removing the DB-querying resolve_key_selector_for_range path simplifies the code significantly and eliminates extra reads.
• Test 5 directly exercises the bug that was fixed.

---

Issues

1. Begin selector correctness for last_less_than / last_less_or_equal (offset=0) when used as range start

The iterator is seeded at the raw anchor key (begin) in handle_get_range:

rocksdb::IteratorMode::From(&begin, rocksdb::Direction::Forward)

For last_less_than(k) ((false, 0)), the resolved key is the last DB key < k — something that may be entirely before k. But because iteration starts at k and range_begin_contains returns key > k, those earlier keys are never visited. The pre-PR code at least returned the raw key as the scan start, giving key >= k; the new code now additionally skips k itself.

In practice offset=0 selectors are rarely used as range-begin, but the rocksdb test at tests/rocksdb.rs:152 uses last_less_or_equal as an end selector. That case behaves as key <= end (approximation of "last key <= end"), which is typically fine for prefix ranges. A brief comment on both functions noting the approximation would help future readers.

2. range_begin_contains (false, 0) — different behavior from old code

Old path (via _ => Ok(key.to_vec())): scan starts at begin, keeps keys >= begin.
New path: range_begin_contains (false, 0) returns key > begin.

This is a behavioral change (drops begin from the result) for anyone calling get_range with a last_less_than begin selector against RocksDB directly, even though both old and new behavior were already incorrect for that case.

3. Missing test: first_greater_than boundary in local-write merge

The real fix is in tx_ops.rs, but Test 5 only covers first_greater_or_equal. The original bug was that first_greater_than(key_b) would incorrectly include a local write to exactly key_b. That case is not covered by a test:

// Missing: a local set to key_b with begin = first_greater_than(key_b)
// should NOT appear in the range result

4. Conflict-range registration uses raw anchor keys (pre-existing, worth noting)

// tx_ops.rs
self.add_conflict_range(opt.begin.key(), opt.end.key(), ConflictRangeType::Read);

This also ignores or_equal/offset, so the recorded conflict range is slightly wider than the actual read range. This is a conservative over-approximation (causes false serialization conflicts, not missed ones) so it's safe, but it's a related issue to the one being fixed.

---

Minor

• The _ => key >= begin / _ => key < end fallback arms are fine per the style guide (integers are an open value space). A short comment explaining they cover offset values beyond {0, 1} would help future readers.
• The test assertion test_subspace.pack(&("range_z",)) in the outer scope compiles because test_subspace is re-bound inside the run closure (not captured from the outer scope). Not a bug, but slightly confusing — consider a direct byte comparison instead.

---

Summary

The tx_ops.rs fix is correct and resolves a real bug. The RocksDB scan refactor is a reasonable simplification given the pre-existing incompleteness. Main asks before merging:

1. Add a test for the first_greater_than begin-boundary case in the local-write merge path (a local write to exactly key_b should be excluded when begin = first_greater_than(key_b)).
2. Consider adding a short comment on range_begin_contains / range_end_contains noting that offset=0 cases are approximations that would require an actual DB lookup for full correctness.