fix(#8817, #8814, #8813): tx index range after compaction, UPDATE Halloween problem, TS oldest sample after COMPACT - #8825

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

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 help to get the list of available commands.}

This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again.

Up to standards ✅

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

Review

Three well-diagnosed fixes, each with a regression test that reproduces the report. Compared with the PR description, the root causes look right. Notes below, roughly by importance.

#8814 MaterializeRecordsStep / UpdateExecutionPlanner

1. Cost on every index-backed UPDATE. readsIndex(sourcePlan) is true for any index read, including the common UPDATE T SET x = ? WHERE id = ?. Those statements now go through a barrier, a sort and a second getRecord() by address, even though they cannot hit the Halloween problem. The problem only exists when the SET targets a property of the index being walked. Gating on that would keep the hot point-update path as it was. At minimum, a short comment or a benchmark number showing the overhead is negligible would help. CLAUDE.md asks us to protect performance.

2. LIMIT can under-deliver. drain counts raw buffered addresses (buffer.size()), but syncPull later drops duplicates (multi-value index returning one RID per key) and deleted records. UPDATE ... LIMIT 5 can then update fewer than 5 records even when more match. This is an edge case, but the step comment says "up to N", so I would either dedupe while draining or document it.

3. Limit evaluated twice. limit.getValue(context) is called once for UpdateExecutionPlan and again for handleTarget. Compute it once.

4. Semantics change. The records are now re-read after the address pass, so a row changed by an earlier row of the same statement is re-evaluated against its current state, not the WHERE snapshot. The Javadoc documents this, which is good. Update order also changes to physical order. Both are probably fine, but worth a line in the PR description.

5. Timeout. drain pulls the whole upstream with no timeout check of its own. It relies on upstream steps honoring it, which is probably OK. Please confirm UPDATE ... TIMEOUT still fires during the drain.

6. Tests. The Halloween test only covers a non-unique LSM index with plain range spellings. Missing cases:

   • compound index
   • several buckets, since the code has per-bucket slices and a dedup reset
   • a multi-value (BY ITEM) index for the dedup path
   • a deleted-since-read record
   • an UPSERT regression check
   • UPDATE ... RETURN AFTER

   Also, limitStillStopsEarly asserts the count only, not that the plan stopped reading early.

7. Minor: the comment in handleTarget is missing a period after "addresses are read first". The error-path comment in syncPull is in ALL CAPS, unlike the rest of the code.

#8817 LSMTreeIndexCursor

The fix is minimal and consistent with get()/addIndexKeyLock(). Good. getMainIndex() is null-guarded. Please check whether getClosestEntryInTx has sibling lookups by index.getName() elsewhere in the cursor (for example a descending path or a TxChanges helper) that need the same change. The test compacts only after loading, so it does not cover a second compaction, where the mutable file name changes again.

#8813 TimeSeriesSealedStore

• The running-max prefix is the right fix for overlapping blocks. The version-plus-length validation is sound: directoryVersion is written under the write lock and read under the read lock, and the racy publication of the volatile cache only costs a redundant rebuild.
• restoreMinTimestampOrder() in appendBlock is a full scan of the directory on every append, so appends become O(blocks). The directory is probably small, but a cheap check on the last element (entry.minTimestamp >= previous last) before scanning would keep the common case O(1).
• directoryVersion is incremented inside the loadDirectory loop once per block. One bump after the loop would do.
• truncateBefore and the other rewrite paths bump the version twice (clear, then addAll). That is harmless, but a single bump after the final state is clearer.
• checkIntegrity and truncateToBlockCount allocate and sort a copy by blockStartOffset. That is fine for rare operations, but the sort could be skipped when the directory is already in file order.
• Line ~3483 blockDirectory.getFirst().minTimestamp stays valid because the directory is sorted by minTimestamp. Worth confirming that all other places that assume "last entry = last written block" (for example anything deriving a block count or next offset from the directory tail) were audited. I found none in a quick grep of TimeSeriesSealedStore.
• Issue8813CompactedOldestSampleTest inserts 200k rows with an SQL command per row. It is tagged slow, so it is acceptable. It is also seeded and deterministic, which is good. The checks are strong: oldest sample plus window counts, both before and after reopen.
• Style: the new Comparator.comparingLong(...) calls need java.util.Comparator imported (check it is). Several new comments are long multi-sentence blocks. CLAUDE.md prefers short comments.

Style and conventions

• Tests use assertThat(...) style, extend TestHelper, and carry license headers. Good.
• The @author Luca Garulli Javadoc on new files follows the repo habit, but please confirm that is intended, given the "don't add Claude as author" rule.
• Issue8813...Test fields are misaligned (COMPACT_EVERY). That is cosmetic.

Verdict

No blocking correctness problems found. The main thing I would change before merging is point 1 for #8814: restrict the barrier to UPDATEs that modify a property of the index being read, or show the overhead is negligible. Points 2 and 6 are worth addressing too.

Review of PR 8825 (tx index range after compaction, UPDATE Halloween, TS oldest sample after COMPACT)

Overall this is a well-reasoned set of three fixes. The root causes are correctly identified and the comments explain the "why". I did not run the tests locally; this is a read-through of the diff plus a few greps.

8817 - LSMTreeIndexCursor

• The fix is correct and consistent with LSMTreeIndex.get() (LSMTreeIndex.java:570 uses the logical name). The mainIndex != null ? ... : index.getName() fallback matches the existing describeIndexForRebuild() pattern. The remaining getName() uses in the cursor are only messages.
• Test is good: unique/non-unique, below/above/inside range, tx delete + update, min/max, ORDER BY, openCypher, and the raw Index.range() API.

8814 - MaterializeRecordsStep / UpdateExecutionPlanner

• Nice use of PhysicalOrderRidBuffer (one long per record). The targeted trigger (readsIndex + modifiedProperties, conservative null for non-SET ops and paths) keeps the barrier off the common path, and the EXPLAIN-based test pinning "barrier only when needed" is a good touch.
• Behaviour change: rows now reach the UPDATE in physical order rather than index order. Visible for RETURN AFTER/RETURN BEFORE over an index range. With LIMIT n the set is still the first n in index order, but output order changes. Probably fine, worth a line in the PR description.
• Timeout: drain() has no explicit timeout check and relies on the upstream select timeout. handleTimeout is chained after this step, so UPDATE ... TIMEOUT over a very large range may only be enforced after the barrier drained. Consider checking the context timeout inside the drain loop.
• Retry after failure: if drain() throws, drained is already true and slices stays null, so a later syncPull NPEs. Minor: set drained = true only after success, or initialise slices to an empty list.
• Tests: limitStillStopsEarly only checks the count; it could also assert each updated row has b == 1. A test for a non-SET op (e.g. INCREMENT a) hitting the barrier, and one confirming UPSERT is left untouched, would cover the remaining modifiedProperties() branches.

8813 - TimeSeriesSealedStore

• The running-max prefix for the lower-bound search is the right fix for overlapping blocks. The directoryVersion cache guard is sound (written under the write lock, validated under the read lock, volatile publish; concurrent duplicate rebuilds are benign). rewriteWithBlocks and truncateBefore already produce a minTimestamp-ordered directory, so no re-sort is missed there.
• appendBlock sorts the whole directory under the write lock on each out-of-order append. TimSort on nearly sorted data is close to O(n), but inserting the single entry at its binary-searched position would avoid the comparator sort. Optional.
• directoryVersion is bumped twice around every clear/addAll (and again in restoreMinTimestampOrder). Harmless, but one bump after the final state is easier to reason about.
• truncateToBlockCount and checkIntegrity now follow file order via blockStartOffset, which is correct.
• Coverage gap: Issue8813CompactedOldestSampleTest is end-to-end (200k rows, @Tag("slow"), appropriate). There is no focused store-level test for truncateToBlockCount / checkIntegrity with an out-of-order directory, which are behaviour changes in this PR. A small test (append blocks with decreasing minTs, truncate/check, reopen) would pin those paths cheaply.

Style (CLAUDE.md)
Conventions look followed (finals, imports, braceless single-statement ifs, assertj style). Nit: constants in Issue8813CompactedOldestSampleTest are misaligned.

No blocking issues found. Before merge I would address the drain timeout and NPE-on-retry robustness, and add a focused test for the truncateToBlockCount/checkIntegrity ordering changes.

Pre-verified at the head e5a4ec28cf (engine built from it) against 3d5e1a514c, with each issue's repro. Temurin 25 with compact object headers and Temurin 21 give the same answers:

• Inside a transaction, an index range or index-end read misses the records the transaction inserted once the LSM index has a compacted sub-index: WHERE a BETWEEN 500 AND 509 returns 0 of 10 (26.8.1 to main) #8817: TxRange at 200,000 committed vertices, NOTUNIQUE and UNIQUE: inside the transaction a BETWEEN 500 AND 509 counts 10 of 10 (was 0) and TypeIndex.range([500], [1009]) 20 of 20 (was 10). TxFastPath: every index read inside the transaction now equals the scan (14 checks were wrong on 3d5e1a514c: MIN/MAX FROM INDEX, the openCypher index-end min/max, the range counts, the BETWEEN rows, the openCypher index-ordered read, and the placement checks). The GEOSPATIAL check inside a transaction counts 15,877 of 15,877 (was 14,877).
• SQL UPDATE that moves an indexed key forward, filtered by a range on that key, updates the same record again and again (Halloween problem): UPDATE V SET a = a + 10 WHERE a < 1001 makes 50,601 updates for 1,001 records (26.8.1 to main) #8814: UPDATE V SET a = a + 10, b = b + 1 WHERE a BETWEEN 0 AND 1000 makes 1,001 updates, each record once (was 50,601), and the same with WHERE a < 1001; 20,000 records with a 10,000-record range make 10,001 updates in 0.8 s (was 5,006,001 in 208 s).
• TIMESERIES: after COMPACT, ORDER BY ts ASC LIMIT 1 returns a sample that is not the oldest when samples arrived out of order (26.9.1 is right) #8813: after COMPACT TIMESERIES TYPE, ORDER BY ts ASC LIMIT 1, a bare LIMIT 1, and the first row of LIMIT 2 all return 1700000480463, the oldest sample (was 1700000630715). TsDiff's 34 checks before and after COMPACT, plus LIMIT 1 to 50 per host, are all right.

Codecov Report

❌ Patch coverage is 84.14634% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.28%. Comparing base (7f2c770) to head (e5a4ec2).
⚠️ Report is 10 commits behind head on main.

@@             Coverage Diff              @@
##               main    #8825      +/-   ##
============================================
+ Coverage     76.27%   76.28%   +0.01%     
+ Complexity     3743     3742       -1     
============================================
  Files          2097     2098       +1     
  Lines        204408   204544     +136     
  Branches      43014    43047      +33     
============================================
+ Hits         155911   156036     +125     
  Misses        31679    31679              
- Partials      16818    16829      +11     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.