fix(amber): bound sync result reads so visualization fetches release their reader

[aglinxinyuan]

What changes were proposed in this PR?

SyncExecutionResource.collectOperatorResult read operator results through an unbounded document.get() iterator. Three paths abandoned that iterator with its Iceberg Parquet reader / S3 input stream still open, leaving the GC finalizer to reclaim it ([S3InputStream] Unclosed input stream created by ... WARNs — the "SyncExecutionResource" rows of the #6881 inventory):

document.get()                       ── opens reader on first next()
  ├─ visualization branch            ── next() once, early return   → LEAK (every sync viz fetch)
  ├─ oversized-first-tuple branch    ── next() once, early return   → LEAK
  ├─ truncation loops                ── drain to exhaustion         → ok
  └─ catch-all on exception          ── abandons mid-drain          → LEAK

Since #6882, a bounded getRange(from, until) iterator releases its reader inside the next() that serves its last record, and hasNext probes open nothing. So the fix is to size each read to what the method actually consumes — no close() API added to VirtualDocument:

Path	Consumes	Read after this PR	Close point
visualization result (totalCount == 1)	1 tuple	getRange(0, 1)	inside the single next()
oversized first tuple	1 tuple	same getRange(0, 1)	inside the single next()
table result (truncation loops)	all remaining	getRange(1, totalCount)	inside the next() serving the bound / exhaustion
swallowed-exception catch-all	partial	catch drains the bounded remainder	at the bound

Notes for review:

• The catch-path drain is bounded: it reads at most the records the successful path would have converted to JSON anyway, and a failing reader ends the drain early (finalizer remains the backstop for that truly exceptional case).
• The truncation loops previously drained whatever the iterator produced, even records appended after getCount was sampled; they are now capped at the same totalCount used for the totalCount/skippedRows fields, keeping the response internally consistent.
• Deterministic close relies on the iterator semantics of fix(amber): close Iceberg reader streams leaked by iterator probes and bounded reads #6882; until that lands, these bounded reads behave as before (finalizer-reclaimed), so merge order is safe either way.

Any related issues, documentation, discussions?

Follow-up to the residual-leak inventory in #6881 (SyncExecutionResource.collectOperatorResult rows). Builds on the iterator restructure in #6882. The remaining #6881 rows (ResultExportService client-disconnect, InputPortMaterializationReaderThread interrupt) are lower priority and left for separate PRs.

How was this PR tested?

Verified by inspection against the bounded-read semantics of #6882: each path's read bound equals its consumption, so every path ends in a limit-close, an exhaustion-close, or nothing opened. The leak itself is only observable as GC-finalizer WARNs, so no unit test pins it; getRange range semantics are covered by the existing VirtualDocumentSpec/IcebergDocumentSpec suites, and this endpoint's behavior is unchanged (same tuples, same truncation, same response shape). Local WorkflowExecutionService/compile and scalafmtCheck pass.

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

Generated-by: Claude Code (Fable 5)

[github-actions]

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

• Contributors with relevant context: @Yicong-Huang
  You can notify them by mentioning @Yicong-Huang in a comment.

[Copilot]

Pull request overview

This PR updates Amber’s synchronous result fetch path (SyncExecutionResource.collectOperatorResult) to avoid leaking Iceberg/Parquet/S3 readers when the method returns early (visualization payload or oversized-first-tuple) or when an exception interrupts iteration, by switching from an unbounded document.get() iterator to bounded document.getRange(from, until) reads and adding a catch-path drain.

Changes:

• Replace the unbounded document.get() read with bounded getRange(0, 1) for the first tuple and getRange(1, totalCount) for the remainder.
• Hoist the remainder iterator so the catch path can drain it and trigger reader release on failures.
• Cap truncation-loop reads to totalCount for response internal consistency.

Comments suppressed due to low confidence (2)

amber/src/main/scala/org/apache/texera/web/resource/SyncExecutionResource.scala:572

• The visualization early-return consumes exactly one element from getRange(0, 1) and returns without any further iterator interaction. On the current Iceberg iterator implementation, the limit-close is triggered by a subsequent hasNext call, so this branch can still leak resources unless you explicitly trigger that close point before returning.

          // the frontend renders it as an iframe rather than a table.
          val firstTuple = firstTupleIterator.next()
          if (totalCount == 1 && isVisualizationTuple(firstTuple)) {
            val jsonResults =
              ExecutionResultService.convertTuplesToJson(List(firstTuple), isVisualization = true)

amber/src/main/scala/org/apache/texera/web/resource/SyncExecutionResource.scala:605

• The oversized-first-tuple early-return has the same issue as the visualization return: it consumes one record from getRange(0, 1) and returns without hitting the iterator’s limit-close point. Drain/probe the bounded iterator once before returning so resources are released deterministically.

          // Records 1 until totalCount; the truncation loops below drain this
          // iterator fully, releasing its reader at the bound.
          tupleIterator = document.getRange(1, totalCount)

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-actions]

⚠️ Benchmark changes need a look

🟢 2 better · 🔴 5 worse · ⚪ 8 noise (<±5%) · 0 without baseline

Compared against main 429be11 benchmarked on this same runner, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.

Dashboard · Run

	config	throughput	MB/s	latency	max Δ latest / 7d
🔴	bs=10 sw=10 sl=64	386	0.236	25,449/38,111/38,111 us	🔴 +16.1% / 🔴 +129.7%
🟢	bs=100 sw=10 sl=64	812	0.496	122,903/135,458/135,458 us	🟢 -7.7% / 🔴 +24.2%
⚪	bs=1000 sw=10 sl=64	927	0.566	1,078,505/1,110,270/1,110,270 us	⚪ within ±5% / 🔴 -7.6%

Baseline details

Latest main 429be11 from same runner

config	metric	PR	latest main	7d avg	Δ latest	Δ 7d
bs=10 sw=10 sl=64	throughput	386 tuples/sec	415 tuples/sec	754.55 tuples/sec	-7.0%	-48.8%
bs=10 sw=10 sl=64	MB/s	0.236 MB/s	0.253 MB/s	0.461 MB/s	-6.7%	-48.8%
bs=10 sw=10 sl=64	p50	25,449 us	21,928 us	12,816 us	+16.1%	+98.6%
bs=10 sw=10 sl=64	p95	38,111 us	35,758 us	16,594 us	+6.6%	+129.7%
bs=10 sw=10 sl=64	p99	38,111 us	35,758 us	19,806 us	+6.6%	+92.4%
bs=100 sw=10 sl=64	throughput	812 tuples/sec	824 tuples/sec	969.38 tuples/sec	-1.5%	-16.2%
bs=100 sw=10 sl=64	MB/s	0.496 MB/s	0.503 MB/s	0.592 MB/s	-1.4%	-16.2%
bs=100 sw=10 sl=64	p50	122,903 us	118,263 us	103,584 us	+3.9%	+18.7%
bs=100 sw=10 sl=64	p95	135,458 us	146,783 us	109,097 us	-7.7%	+24.2%
bs=100 sw=10 sl=64	p99	135,458 us	146,783 us	117,304 us	-7.7%	+15.5%
bs=1000 sw=10 sl=64	throughput	927 tuples/sec	927 tuples/sec	1,004 tuples/sec	0.0%	-7.6%
bs=1000 sw=10 sl=64	MB/s	0.566 MB/s	0.566 MB/s	0.613 MB/s	0.0%	-7.6%
bs=1000 sw=10 sl=64	p50	1,078,505 us	1,083,570 us	1,002,357 us	-0.5%	+7.6%
bs=1000 sw=10 sl=64	p95	1,110,270 us	1,106,302 us	1,046,463 us	+0.4%	+6.1%
bs=1000 sw=10 sl=64	p99	1,110,270 us	1,106,302 us	1,073,661 us	+0.4%	+3.4%

Raw CSV

config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,517.87,200,128000,386,0.236,25449.17,38110.58,38110.58
1,100,10,64,20,2461.84,2000,1280000,812,0.496,122903.48,135457.52,135457.52
2,1000,10,64,20,21572.92,20000,12800000,927,0.566,1078505.47,1110269.63,1110269.63

[codecov-commenter]

Codecov Report

❌ Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.30%. Comparing base (429be11) to head (dea2e58).

Files with missing lines	Patch %	Lines
...he/texera/web/resource/SyncExecutionResource.scala	0.00%	6 Missing ⚠️

❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #6883      +/-   ##
============================================
- Coverage     77.31%   77.30%   -0.01%     
+ Complexity     3524     3523       -1     
============================================
  Files          1161     1161              
  Lines         45920    45923       +3     
  Branches       5099     5100       +1     
============================================
- Hits          35501    35499       -2     
- Misses         8839     8843       +4     
- Partials       1580     1581       +1     

Flag	Coverage Δ		*Carryforward flag
access-control-service	70.00% <ø> (ø)		Carriedforward from 429be11
agent-service	76.76% <ø> (ø)		Carriedforward from 429be11
amber	69.08% <0.00%> (-0.03%)	⬇️
computing-unit-managing-service	20.49% <ø> (ø)		Carriedforward from 429be11
config-service	66.66% <ø> (ø)		Carriedforward from 429be11
file-service	67.21% <ø> (ø)		Carriedforward from 429be11
frontend	82.58% <ø> (ø)		Carriedforward from 429be11
notebook-migration-service	78.94% <ø> (ø)		Carriedforward from 429be11
pyamber	92.15% <ø> (ø)		Carriedforward from 429be11
workflow-compiling-service	55.14% <ø> (ø)		Carriedforward from 429be11

*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:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.