Fix TTL compaction filter value parsing - #539
Conversation
WalkthroughThe TTL compaction filter now validates encoded values, safely decodes timestamps, uses millisecond system time, and removes expired TTL values. Catch2 tests cover expired, unexpired, and non-TTL values for RocksDB datastore variants. ChangesTTL compaction filtering
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
store_handler/eloq_data_store_service/rocksdb_data_store_common.cpp (1)
17-44: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winFix is correct; consider extracting a shared decode helper to prevent the encode/decode logic from drifting apart again.
The logic correctly reads the TTL marker from the first word's MSB and only reads the expiration timestamp from the second word when the marker is set, with size guards that retain truncated values instead of reading past
existing_value's bounds. This matches the PR's stated fix.This decode logic duplicates the parsing already implemented in
DecodeHasTTLFromTs/DeserializeValueToRecord(Lines 1374-1412) and the encoding inEncodeHasTTLIntoTs/TransformRecordToValueSlices(Lines 1329-1372). The bug this PR fixes exists because the compaction filter and the record encoder/decoder disagreed about the on-disk layout. Keeping two independent implementations of the same wire format increases the risk that a future change to one path silently breaks the other, since RocksDB compaction runs on live data with no schema check.Extract a single length-checked decode helper that both
FilterandDeserializeValueToRecordcall, so the TTL marker and offset logic exists in one place.As per coding guidelines, "Document non-obvious invariants and operational constraints... explain why rather than restating syntax" for
**/*; consolidating the decode logic also documents the shared invariant in one place instead of two comments that can drift apart.♻️ Proposed shared decode helper
// Decodes the version timestamp and, if present, the TTL expiration // timestamp from a value encoded by TransformRecordToValueSlices. // Returns false if `data`/`size` is too short to hold the fields that // the TTL marker claims are present; callers must treat that as // "keep the value unchanged" rather than reading past `size`. bool TryDecodeVersionAndTtl(const char *data, size_t size, uint64_t &ts, bool &has_ttl, uint64_t &ttl, size_t &record_offset) { if (size < sizeof(uint64_t)) { return false; } std::memcpy(&ts, data, sizeof(ts)); has_ttl = (ts & MSB) != 0; ts &= MSB_MASK; record_offset = sizeof(uint64_t); if (has_ttl) { if (size < sizeof(uint64_t) * 2) { return false; } std::memcpy(&ttl, data + sizeof(uint64_t), sizeof(ttl)); record_offset += sizeof(uint64_t); } else { ttl = 0; } return true; }
FilterandDeserializeValueToRecordthen both call this helper instead of re-implementing the offsets.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@store_handler/eloq_data_store_service/rocksdb_data_store_common.cpp` around lines 17 - 44, Extract a shared length-checked decode helper for the version timestamp, TTL marker, TTL expiration, and record offset, preserving truncated-value handling. Update both Filter and DeserializeValueToRecord to call this helper instead of duplicating the wire-format parsing, and keep the helper’s invariant documented in one place.Source: Coding guidelines
tx_service/tests/TTLCompactionFilter-Test.cpp (1)
46-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for truncated/malformed values.
The PR objective states the filter retains malformed or truncated values without reading past their bounds, and
rocksdb_data_store_common.cpp(Lines 17-20 and 31-34) implements two distinct bound checks for this. None of the threeSECTIONs here exercise a value shorter thansizeof(uint64_t), or a value with the TTL marker set but shorter thansizeof(uint64_t) * 2. Add sections for both cases so the safety behavior this PR introduces is actually verified by the test suite.✅ Proposed additional sections
SECTION("value shorter than the encoded timestamp is retained") { const std::string value(sizeof(uint64_t) - 1, '\0'); REQUIRE_FALSE(ShouldFilter(value)); } SECTION("TTL-flagged value truncated before the expiration word is retained") { std::string value(sizeof(uint64_t) + 1, '\0'); uint64_t encoded_ts = EloqDS::MSB | 42; std::memcpy(value.data(), &encoded_ts, sizeof(encoded_ts)); REQUIRE_FALSE(ShouldFilter(value)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tx_service/tests/TTLCompactionFilter-Test.cpp` around lines 46 - 69, Add two sections to the TTL compaction filter test covering malformed inputs: verify a value shorter than sizeof(uint64_t) is retained, and verify a TTL-marked value shorter than sizeof(uint64_t) * 2 is also retained after encoding the marker into its prefix. Place them alongside the existing sections in the test case and assert both through ShouldFilter.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tx_service/tests/CMakeLists.txt`:
- Around line 69-74: Update the TTLCompactionFilter-Test setup in the CMake
configuration to add the store_handler/eloq_data_store_service include directory
and link the target containing rocksdb_data_store_common.cpp, while preserving
its existing Catch test registration and data-store condition.
---
Nitpick comments:
In `@store_handler/eloq_data_store_service/rocksdb_data_store_common.cpp`:
- Around line 17-44: Extract a shared length-checked decode helper for the
version timestamp, TTL marker, TTL expiration, and record offset, preserving
truncated-value handling. Update both Filter and DeserializeValueToRecord to
call this helper instead of duplicating the wire-format parsing, and keep the
helper’s invariant documented in one place.
In `@tx_service/tests/TTLCompactionFilter-Test.cpp`:
- Around line 46-69: Add two sections to the TTL compaction filter test covering
malformed inputs: verify a value shorter than sizeof(uint64_t) is retained, and
verify a TTL-marked value shorter than sizeof(uint64_t) * 2 is also retained
after encoding the marker into its prefix. Place them alongside the existing
sections in the test case and assert both through ShouldFilter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 20e31901-722a-4d06-acfe-007f7ea687cf
📒 Files selected for processing (3)
store_handler/eloq_data_store_service/rocksdb_data_store_common.cpptx_service/tests/CMakeLists.txttx_service/tests/TTLCompactionFilter-Test.cpp
2 participants
What
memcpyfor word decoding while preserving the existing value-size invariants.ELOQDSS_ROCKSDB_CLOUD_S3and build/run the full suite in the same amd64/arm64 CI jobs.Testsworkflow runs when a newer commit is pushed to the same PR or ref.Why / root cause
EloqDSS RocksDB values are stored as:
TTLCompactionFilterincorrectly checked offset 8 for the TTL MSB. Offset 8 is the expiration timestamp, whose high bit is clear for ordinary epoch-millisecond values, so expired records were retained during both automatic and manual compaction.Observable impact
Expired records can accumulate physically in RocksDB even though reads treat them as deleted. Scans must traverse and deserialize those records before filtering them, which can make sparse ranges progressively slower. After this fix is deployed, subsequent compactions can reclaim expired records; cleanup is still governed by normal compaction scheduling and backlog and is not immediate at expiration time.
Design notes
CompactionFilterFactorycapturessystem_clockepoch milliseconds when it creates each compaction filter; all records processed by that filter use the same timestamp.Checks
git diff --check origin/main...HEAD— passed..github/workflows/tests.ymlYAML parse — passed.Risk and rollback
The main risk is deleting a record whose first-word TTL marker and second-word expiration are corrupt but happen to look valid. Correctly encoded non-TTL and unexpired values are retained by regression coverage. Rollback is a revert of these commits; already compacted expired records do not need restoration.
Reviewer focus
TransformRecordToValueSlicesandDeserializeValueToRecordmatches offsets 0 and 8 used here.