fix(engine) #5635: an index cursor never hands out a null entry, and a restarted index scan releases its cursors

[lvca]

Closes #5635.

Both follow-ups of #5601 / #5609, in one change because the second half of the first one (removing the private null guards consumers had grown) only makes sense once the contract holds.

1. LSMTreeIndexCursor.next() could return null after hasNext() returned true

hasNext() answered on how many underlying page cursors were still live, not on whether any of them still held a surviving RID. A scan that ended on a run of tombstoned keys therefore said "yes" and then handed the caller nothing. IndexCursor extends Iterator<Identifiable>, so for (final Identifiable r : cursor) yielded that null.

hasNext() now prefetches: it runs the merge until it holds an entry it can actually emit, so it is exact, and next() is a pure drain. The work it does is the work next() used to do - the prefetched entry is cached until drained, so nothing happens twice.

next() throws NoSuchElementException when exhausted, rather than keeping the null for one release. That is the option the issue left open, and the reason for picking it is that this class was the odd one out, not the norm: EmptyIndexCursor, TempIndexCursor, IndexCursorCollection, MultiIndexCursor and GeoIndexCursor all throw already, and every range() / iterator() caller in the tree drives the cursor with hasNext(). A caller that already handled the null still works - it just never sees one.

Two consequences were user-visible, beyond the countEntries() residual the issue mentions:

• SELECT min(...) / SELECT max(...) could answer with a deleted key. MaxMinFromIndexStep reads the key off the cursor after next(). The trailing null still moved the cursor, so on a type whose lowest (or highest) keys had all been deleted, the key left behind was a tombstoned one.
• countEntries() counted the null as an entry - Index misc: countEntries() counts tombstones as live entries, geospatial get() materialises the whole candidate set #5601's residual, and the reason it survived a full compaction that had already dropped every tombstone.

getRecord() and getKeys() are settled at the same time: they describe the entry next() last returned. The old implementation peeked at the not-yet-consumed value at currentValueIndex, which read as "the current entry" only by accident - a caller reading them right after next() saw the following row. This is what TempIndexCursor, IndexCursorCollection, MultiIndexCursor and GeoIndexCursor already do, so it is an alignment rather than a new rule.

The guards are removed in the same change, so there is one contract rather than a mix: FullTextQueryExecutor (four sites, from #5118), GeoIndexCursor.fetchNext(), SQLFunctionGeoPredicate, MultiIndexCursor (which propagated the null upward), FullTextSearch and LSMTreeIndex.countEntries(). LSMVectorIndex's inline cursor also throws now instead of answering null. The contract is documented once, on IndexCursor.

2. FetchFromIndexStep.reset() dropped its cursors without closing them

A reset restarts the step - inited goes back to false and init() rebuilds every cursor from scratch, which is what UpdateExecutionPlan.reset() relies on (it re-runs the plan straight after). So the previous run's cursors have to be released, exactly as close() already documents: a LSMTreeIndexUnderlyingCompactedSeriesCursor stays registered with its file, and dropRetiredCompactedIndexes skips a retired file that still has one - for the lifetime of the database, since nothing else will ever close it.

It was worse than a leak. nextCursors was not even cleared, so the pending cursors of the previous run survived into the new one and init() appended to them: the restarted scan replayed the old, partly consumed cursors before reaching the ones it had just opened.

close() and reset() now share one release path. It also covers the per-value cursors of a key IN [...] lookup, which processInCondition() hands to customIterator - nothing had ever closed those, not even close().

reset() is still not propagated to prev: SelectExecutionPlan.reset() walks every step itself, so a step that reset its predecessor would reset it twice. That is now stated in the javadoc rather than left as a question.

The same abandoned-cursor leak is fixed where the contract change took me: MaxMinFromIndexStep (stops on the first entry by design), the full-text term walks in FullTextQueryExecutor (a prefix walk stops on the prefix boundary, so early exit is the common path), and the Cypher NodeIndexSeek / NodeIndexRangeScan operators, whose close() carried a // IndexCursor doesn't need explicit closing comment that has been wrong since #5601. NodeIndexSeek also rotates one cursor per IN value and dropped each one on the way.

Tests

LSMTreeIndexCursorContractTest - 8 tests over a fixture with a contiguous tombstone run at both ends of the key space (3,900 of 4,000 records deleted), driving the bucket-level index so the cursor under test is a bare LSMTreeIndexCursor rather than the MultiIndexCursor a TypeIndex wraps it in. The wrapper happens to absorb the null, which is a large part of why this stayed invisible. 6 of the 8 fail on main: the for-each yielding null (mutable pages and after a full compaction), the naive count being one too high, next() not throwing, a tombstones-only range not being empty, and getRecord()/getKeys() describing the wrong entry.

FetchFromIndexStepResetTest - 4 tests, all failing on main. Tracking cursors pulled out of a real execution plan prove reset() reaches and closes the current cursor, the pending nextCursors, and the IN-list cursors; a fourth asserts the behavioural consequence, that a restarted step replays the scan from the beginning instead of the tail of the previous run. Each asserts its precondition first (the step is mid-scan, holding an open cursor), so none of them can hold vacuously.

LSMTreeIndexCursorTombstoneRunTest loses its local countEntries workaround - it skipped nulls, and now asserts they never appear.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Puky78GhudfgTHPtYSuFPm

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics -3 complexity

Metric	Results
Complexity	-3

View in Codacy

🟢 Coverage 90.82% diff coverage · -5.99% coverage variation

Metric	Results
Coverage variation	✅ -5.99% coverage variation
Diff coverage	✅ 90.82% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (7081057)	149675	112546	75.19%
Head commit (ff29bfa)	181684 (+32009)	125737 (+13191)	69.21% (-5.99%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#5641)	98	89	90.82%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

_{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.}

[claude]

Review: PR #5641 - index cursor null contract + cursor release on restart

I read through all 15 source files and both new tests against main. This is a careful, well-scoped fix and the reasoning in the PR description matches the code. Summary: looks good to merge, with one minor consistency nit and a couple of notes.

Strengths

• The core fix is correct. hasNext() prefetching via fetchNext() and next() draining nextValue gives an exact iterator; the prefetched entry is cached so nothing is done twice, and close() resets nextValue/merge state so hasNext() cannot resurrect a drained cursor. The termination argument in fetchNext()/advanceCursor() is preserved.
• getRecord()/getKeys() now describe the last-returned entry. This is not just an alignment, it fixes a latent mismatch: FetchFromIndexStep.fetchNextEntry() builds new Pair(cursor.getKeys(), value) right after cursor.next(); under the old peek-ahead semantics the key belonged to the following row, so key/rid pairs could be misaligned. Same for MaxMinFromIndexStep reading the key after next().
• Cursor-release coverage is comprehensive: releaseCursors() shared by close()/reset(), the previously-unclosed customCursors for IN lists, MaxMinFromIndexStep, and the Cypher NodeIndexSeek/NodeIndexRangeScan operators (whose "doesnt need explicit closing" comments were indeed wrong since PR 5601), plus NodeIndexSeek rotating one cursor per IN value.
• The reset() bug is real and nasty: nextCursors was never cleared, so a restarted step replayed the previous runs partially-consumed cursors. Good catch, and the behavioural test (replay-from-start) pins it rather than just the leak.
• Tests are strong. Driving the bucket-level index so the cursor under test is a bare LSMTreeIndexCursor (not the MultiIndexCursor wrapper that absorbed the null) is the key insight for why this stayed invisible; tombstone runs at both ends; each reset test asserts its precondition first so it cannot pass vacuously. I confirmed the main-source consumers of IndexCursor.next() (gremlin ArcadeFilterByIndexStep, RedisQueryEngine) all guard with hasNext(), so the throw-on-exhaustion change is safe for them.

Minor: two FullTextQueryExecutor sites left without try/finally close

collectAllIndexedRids (503) and the prefix walk (692) were wrapped in try { ... } finally { cursor.close(); }, but the two document-frequency loops still are not: line 533 (getPostings(searchKey)) and line 565 (getPostings(term.text())). Both drain the cursor to exhaustion with no break, so on the normal path the underlying cursors close themselves and there is no retired-file leak - this is not a correctness bug. But it is inconsistent with the two siblings you just fixed in the same file, and it leaves a cursor unreleased if computeIfAbsent/put ever throws mid-loop. Worth wrapping them the same way for uniformity.

Notes (no action needed)

• The getRecord()/getKeys() semantic flip (peek-ahead to last-returned) is a behavioural change on the public IndexCursor interface. In-tree consumers are all updated and the contract is now documented on the interface, so this is fine; just flagging it for any out-of-tree cursor consumer.
• MultiIndexCursor.getComparator()/getBinaryKeyTypes() call cursor.hasNext(), which now triggers a (cached) prefetch merge. Harmless, but these are no longer allocation-free probes.

Nice work - the fix is well-reasoned and the test coverage directly targets the failure modes.

Reviewed with Claude Code.

[claude]

Review: PR #5641 - index cursor never hands out a null; restarted scan releases its cursors

I reviewed the full diff (engine cursor contract, the consumer clean-ups, the two leak fixes, and both new test classes). This is a careful, well-motivated change and the writeup and release notes make the intent easy to verify. Overall it looks correct and ready, with a couple of minor notes.

What is strong

• Right root cause. Making hasNext() prefetch until it holds an emittable RID and turning next() into a pure drain is the correct fix, and centralizing the contract on the IndexCursor Javadoc (instead of a scatter of private null guards) is the right call. The prefetch does exactly the work the old next() did and caches it, so there is no double work.
• Fixes latent bugs beyond the reported one. Realigning getRecord()/getKeys() to describe the entry next() last returned (rather than peeking at currentValueIndex, i.e. the following row) is a genuine correctness fix. FetchFromIndexStep, DeleteFromIndexStep and MaxMinFromIndexStep all pair next() with getKeys() and were silently relying on the accidental alignment. I traced every getKeys()/getRecord() consumer in main; none reads them before the first next(), so the new "null before first next()" semantics are safe.
• The guard removals are safe and even fix nulls the old code swallowed. e.g. TypeIndex.get() did result.add(cursor.next()) and Set.of(cursor.next()), and ArcadeFilterByIndexStep did resultSet.add(cursor.next()) - all of which could have inserted a null under the old optimistic behavior. The exact hasNext() makes those paths correct too, not just the ones that had explicit guards.
• MultiIndexCursor interaction verified. Its lookahead pattern (buffer child.next(), compare with child.getKeys(), only advance the selected child) stays correct under the new last-returned getKeys() semantics.
• Leak fixes are real and consistent. reset() sharing releaseCursors() (and the observation that nextCursors was not even cleared, so a restart replayed partly-consumed cursors) is the important one; extending the same release to the IN-list customCursors, MIN/MAX, the full-text term walks, and the Cypher NodeIndexSeek/NodeIndexRangeScan operators (whose "IndexCursor does not need explicit closing" comment had been wrong since 5601) is a thorough sweep. NodeIndexSeek per-IN-value cursor rotation correctly closes each before opening the next.
• Tests are excellent. The contract test deliberately drives the bucket-level index so the cursor under test is a bare LSMTreeIndexCursor (not the MultiIndexCursor that absorbed the null), with tombstone runs at both ends. The reset test uses tracking cursors pulled from a real plan and asserts each precondition (mid-scan, holding an open cursor) so none can pass vacuously. Both classes fail on main per the description.

Minor notes (non-blocking)

1. Public-API behavioral change worth flagging for users. IndexCursor is a public type; next() now throws NoSuchElementException instead of returning null when exhausted. Any downstream/user code driving the low-level index API with while ((rid = cursor.next()) != null) will now see an exception instead of a terminating null. This is the correct Iterator contract and is documented in the release notes, but since it is a behavioral break on a public surface it may deserve an explicit upgrade-note callout for anyone using the index API directly.

2. Release-notes markdown nit. docs/release-26.8.1.md lines 909-910: the new section last line runs directly into the next "Server and cluster status endpoints..." heading with no blank line in between. Add a blank line so the heading renders.

3. Pre-existing (not introduced here), for awareness: in MaxMinFromIndexStep the else { resultValue = key; } branch is effectively dead because getKeys() always returns Object[], so key instanceof Object[] is always true when non-null. Harmless and out of scope for this PR - just noting it since the file was touched.

Nice work - the contract consolidation plus the leak sweep is exactly the kind of change that pays for itself.

Note: I reviewed statically; I was unable to run mvn in this environment to independently confirm the build/tests, so I am relying on the PR stated test results for the compile/green status.

[codecov]

Codecov Report

❌ Patch coverage is 78.57143% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.26%. Comparing base (7081057) to head (ff29bfa).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...arcadedb/index/fulltext/FullTextQueryExecutor.java	75.00%	1 Missing and 5 partials ⚠️
...cadedb/query/sql/executor/MaxMinFromIndexStep.java	60.00%	1 Missing and 3 partials ⚠️
...ava/com/arcadedb/index/lsm/LSMTreeIndexCursor.java	90.62%	1 Missing and 2 partials ⚠️
...ncypher/executor/operators/NodeIndexRangeScan.java	0.00%	3 Missing ⚠️
...y/opencypher/executor/operators/NodeIndexSeek.java	50.00%	2 Missing and 1 partial ⚠️
...va/com/arcadedb/index/fulltext/FullTextSearch.java	50.00%	0 Missing and 1 partial ⚠️
...java/com/arcadedb/index/vector/LSMVectorIndex.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5641      +/-   ##
============================================
+ Coverage     66.48%   67.26%   +0.78%     
============================================
  Files          1771     1771              
  Lines        149675   149692      +17     
  Branches      31743    31736       -7     
============================================
+ Hits          99504   100695    +1191     
+ Misses        37173    35869    -1304     
- Partials      12998    13128     +130     

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

🚀 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.