Skip to content

fix(store): close sub-iter when MultiListIter closed early - #3437

Merged
imbajin merged 1 commit into
apache:masterfrom
vaidikgarg-bit:fix/multilist-iterator-close-leak
Oct 30, 2026
Merged

imbajin merged 1 commit into
apache:masterfrom
vaidikgarg-bit:fix/multilist-iterator-close-leak

Conversation

@vaidikgarg-bit

@vaidikgarg-bit vaidikgarg-bit commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Purpose of the PR

MultiListIterator.close() closes innerIterator and whatever remains in innerListIterator. Both fields are set on the first hasNext() or isValid() call, so an iterator closed before that has null in both. In that case close() skips both and clears the list, and none of the sub-iterators gets closed. AggregativeQueryObserver runs into this when a query is cancelled after getIterator() returns but before its read loop calls hasNext(). The scan iterators BusinessHandlerImpl already opened for the query are then left open.

Main Changes

  • close() starts innerListIterator from the full list when iteration never began, so the existing loop closes every sub-iterator. When iteration has started, it keeps its current behavior: sub-iterators that were exhausted and closed are not closed again.
  • FilterIteratorContractTest gains three tests. One closes a bare MultiListIterator before iteration and then closes it again, one closes it through the limit-dedup wrapper MapLimitIterator, and one closes it partway through. In each, every sub-iterator must be closed exactly once.

Verifying these changes

  • Trivial rework / code cleanup without any test coverage. (No Need)
  • Already covered by existing tests, such as (please modify tests here).
  • Need tests and can be verified as follows:
    • on master the two close-before-iteration tests fail, since the sub-iterators are never closed; the partial-iteration test passes on master too and guards against double closing
    • store-server-test (192), store-core-test and store-rocksdb-test (19) pass locally, and mvn editorconfig:check reports nothing in tracked files

Does this PR potentially affect the following parts?

Documentation Status

  • Doc - TODO: required documentation is pending; complete it before merging.
  • Doc - Done: documentation is included here or linked below.
  • Doc - No Need: no user-visible documentation is affected.

Visual overview

MultiListIterator closing before the first read

Closing before the first read now closes the registered child iterators instead of skipping cleanup. Scope: normal close path only; this illustration does not assert test/CI completion, exception safety or concurrency safety.

close() reached the sub-iterators only through innerIterator and
innerListIterator, which are set on the first hasNext() or isValid().
Closing before that cleared the list without closing anything, so a
query cancelled before its first read left its store scans open.

Start innerListIterator from the full list when iteration never began.
Sub-iterators already exhausted and closed are still not closed twice.

close apache#3436
@codecov

codecov Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.73%. Comparing base (608cb1d) to head (f9a62d1).
⚠️ Report is 22 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3437      +/-   ##
============================================
+ Coverage     57.27%   57.73%   +0.46%     
+ Complexity     6577     2589    -3988     
============================================
  Files           776      779       +3     
  Lines         68881    69103     +222     
  Branches       9431     9489      +58     
============================================
+ Hits          39449    39895     +446     
+ Misses        25804    25547     -257     
- Partials       3628     3661      +33     
Flag Coverage Δ *Carryforward flag
commons 96.37% <ø> (ø) Carriedforward from 608cb1d
hstore 36.11% <ø> (+0.02%) ⬆️
pd 52.80% <ø> (+0.64%) ⬆️ Carriedforward from 608cb1d
server_hbase 42.18% <ø> (+<0.01%) ⬆️ Carriedforward from 608cb1d
server_memory 55.99% <ø> (+<0.01%) ⬆️ Carriedforward from 608cb1d
server_rocksdb 42.66% <ø> (+<0.01%) ⬆️ Carriedforward from 608cb1d
store 35.93% <ø> (+2.78%) ⬆️

*This pull request uses carry forward flags. Click here to find out more.

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

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: no. Score: 9/10. Summary: close() now walks the full iterators snapshot when iteration never started, so cancelling before the first hasNext() closes every sub-iterator, and the started path still closes each sub-iterator once; the three new FilterIteratorContractTest cases pin both paths.

@imbajin imbajin changed the title fix(store): close sub-iterators when MultiListIterator is closed early fix(store): close sub-iter when MultiListIter closed early Oct 30, 2026
@imbajin
imbajin merged commit 4641958 into apache:master Oct 30, 2026
16 checks passed
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.

[Bug] Store MultiListIterator leaks its sub-iterators when closed before the first hasNext()

3 participants