Skip to content

fix: close CloseableIterable owners in Iceberg read paths#5149

Merged
Yicong-Huang merged 1 commit into
apache:mainfrom
mengw15:fix/5143-iceberg-closeable-leak
May 23, 2026
Merged

fix: close CloseableIterable owners in Iceberg read paths#5149
Yicong-Huang merged 1 commit into
apache:mainfrom
mengw15:fix/5143-iceberg-closeable-leak

Conversation

@mengw15
Copy link
Copy Markdown
Contributor

@mengw15 mengw15 commented May 22, 2026

What changes were proposed in this PR?

Fixes a resource leak in IcebergUtil.readDataFileAsIterator and five sibling sites in IcebergDocument that share the same anti-pattern:

closeableIterable.iterator().asScala

The bare Iterator returned to callers held no reference to its parent CloseableIterable, so the parent could never be closed. Under S3FileIO:

  1. Every read leaked one S3InputStream (kept open until GC because nothing in the call graph could close it).
  2. The leaked stream had already borrowed one slot from the AWS SDK's ApacheHttpClient connection pool (default 50; texera did not override).
  3. After ~50 leaked reads the pool may saturate; new S3 reads then block on acquireConnection until JVM restart.

This PR:

  • Introduces CloseableScalaIterator[T] (Iterator[T] with AutoCloseable, idempotent close()) in IcebergUtil, which wraps a CloseableIterable[T] and propagates close() to the parent.
  • Changes IcebergUtil.readDataFileAsIterator to return CloseableScalaIterator[Record] instead of bare Iterator[Record]. Callers must now close it (e.g. via Using.resource).
  • Updates the single caller in IcebergDocument's read iterator to track the close handle in a sibling AutoCloseable field (currentRecordIteratorCloser) and close it on file-switch, on exhaustion, and on caller-imposed until cap. The sibling field is necessary because Iterator.drop(n) returns a bare iterator that loses the wrapper type.
  • Wraps the four eagerly-consumed planFiles() call sites — getCount, seekToUsableFile, getTableStatistics, asInputStream — in Using.resource so the metadata-side CloseableIterable<FileScanTask> is closed promptly.

Known limitation (out of scope here): if a caller of IcebergDocument.get() / getRange() / getAfter() stops iterating before hasNext returns false (e.g. throws mid-loop, or calls .take(n) and then drops the result), the LAST file's CloseableScalaIterator will leak until JVM GC. Fixing this requires changing the public Iterator[T] return type on VirtualDocument to Iterator[T] with AutoCloseable and updating all callers — best done as a separate refactor.

Any related issues, documentation, discussions?

Closes #5143.

How was this PR tested?

  • Added IcebergUtilLeakSpec (2 cases): validates that CloseableScalaIterator (a) closes its parent CloseableIterable when used inside Using.resource, and (b) is idempotent under repeated close() calls.
  • All existing iceberg specs still pass:
    • IcebergUtilSpec: 14/14
    • IcebergUtilLeakSpec: 2/2 (new)
    • IcebergDocumentSpec: 18/18 (exercises the modified read iterator's close-on-reassign / close-on-exhaustion paths against real Iceberg infrastructure)
    • IcebergTableStatsSpec: 12/12 (exercises getTableStatistics with the new Using.resource wrap)
    • IcebergDocumentConsoleMessagesSpec: 1/1

Run locally:

sbt "WorkflowCore/testOnly org.apache.texera.amber.util.IcebergUtilSpec org.apache.texera.amber.util.IcebergUtilLeakSpec org.apache.texera.amber.storage.result.iceberg.*"

Result: 47 succeeded, 0 failed.

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code (claude-opus-4-7)

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 22, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 43.39%. Comparing base (bf2f92c) to head (31cbdff).

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5149      +/-   ##
============================================
+ Coverage     43.36%   43.39%   +0.02%     
+ Complexity     2217     2214       -3     
============================================
  Files          1049     1049              
  Lines         40560    40569       +9     
  Branches       4322     4323       +1     
============================================
+ Hits          17589    17605      +16     
+ Misses        21883    21871      -12     
- Partials       1088     1093       +5     
Flag Coverage Δ *Carryforward flag
access-control-service 39.53% <ø> (ø)
agent-service 33.76% <ø> (ø) Carriedforward from bf2f92c
amber 43.92% <100.00%> (+0.07%) ⬆️
computing-unit-managing-service 0.00% <ø> (ø)
config-service 0.00% <ø> (ø)
file-service 32.18% <ø> (ø)
frontend 34.61% <ø> (ø) Carriedforward from bf2f92c
python 90.50% <ø> (ø) Carriedforward from bf2f92c
workflow-compiling-service 56.81% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 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.

@mengw15 mengw15 force-pushed the fix/5143-iceberg-closeable-leak branch 2 times, most recently from f3e4b9b to a6911da Compare May 22, 2026 08:01
Copy link
Copy Markdown
Contributor

@Yicong-Huang Yicong-Huang left a comment

Choose a reason for hiding this comment

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

Left comments in line. I think the direction is correct, I like the idea of using ClosableIterator! but the implementation can be improved.

Also please make sure coverage is filled.

@mengw15 mengw15 force-pushed the fix/5143-iceberg-closeable-leak branch 2 times, most recently from 3b474c2 to 03612c8 Compare May 22, 2026 19:38
@mengw15 mengw15 requested a review from Yicong-Huang May 22, 2026 19:40
@mengw15 mengw15 force-pushed the fix/5143-iceberg-closeable-leak branch from 03612c8 to 8c51c3a Compare May 22, 2026 20:21
`IcebergUtil.readDataFileAsIterator` and five sibling sites in
`IcebergDocument` shared the same anti-pattern:
`closeableIterable.iterator().asScala` returned a bare Scala iterator
with no reference to the parent `CloseableIterable`. Under `S3FileIO`
each call leaked one `S3InputStream` (kept open until GC) plus one
slot of the AWS SDK's default 50-slot Apache HTTP connection pool;
after ~50 reads any new S3 read blocked indefinitely on
`acquireConnection` until JVM restart.

Introduce `CloseableScalaIterator[T]` (`Iterator[T] with AutoCloseable`,
idempotent `close()`) that wraps a `CloseableIterable[T]` and
propagates `close()` to the parent. Change `readDataFileAsIterator`
to return this wrapper. Update the `IcebergDocument` read iterator
to track the close handle in a sibling `AutoCloseable` field (needed
because `Iterator.drop(n)` returns a bare iterator that loses the
wrapper type) and close it on file-switch, on exhaustion, and on
caller-imposed `until` cap. Wrap the four eagerly-consumed
`planFiles()` call sites (`getCount`, `seekToUsableFile`,
`getTableStatistics`, `asInputStream`) in `Using.resource` so the
metadata-side `CloseableIterable<FileScanTask>` is released promptly.

Known limitation out of scope here: if a caller of
`IcebergDocument.get()` / `getRange()` / `getAfter()` stops iterating
before `hasNext` returns `false`, the LAST file's wrapper still
leaks until GC. Fixing that requires changing the public `Iterator[T]`
return type on `VirtualDocument` to `Iterator[T] with AutoCloseable`
and updating every caller — best done as a separate refactor.

Closes apache#5143.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@mengw15 mengw15 force-pushed the fix/5143-iceberg-closeable-leak branch from 8c51c3a to 31cbdff Compare May 22, 2026 20:25
@Yicong-Huang Yicong-Huang enabled auto-merge May 23, 2026 07:04
Copy link
Copy Markdown
Contributor

@Yicong-Huang Yicong-Huang left a comment

Choose a reason for hiding this comment

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

LGTM!

@Yicong-Huang Yicong-Huang added this pull request to the merge queue May 23, 2026
Merged via the queue into apache:main with commit 5e56956 May 23, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

IcebergUtil.readDataFileAsIterator leaks the parent CloseableIterable / S3InputStream per data-file read

3 participants