Skip to content

fix(workflow-operator): set alreadyClosed before onClose#5678

Merged
Yicong-Huang merged 1 commit into
apache:mainfrom
justinsiek:fix/autoclosing-iterator-close-once
Jun 13, 2026
Merged

fix(workflow-operator): set alreadyClosed before onClose#5678
Yicong-Huang merged 1 commit into
apache:mainfrom
justinsiek:fix/autoclosing-iterator-close-once

Conversation

@justinsiek

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

AutoClosingIterator.hasNext only set alreadyClosed = true after calling onClose(), and so if onClose() throws, alreadyClosed would stay false, and so a subsequent hasNext would reinvoke onClose(), running cleanup a second time on a resource whose close already failed.

The change makes alreadyClosed = true run before onClose().

Any related issues, documentation, discussions?

Closes #5660

How was this PR tested?

Updated AutoClosingIteratorSpec — replaced the existing characterization test ("re-invoke onClose on a retry when the previous onClose threw") with a positive assertion that a second hasNext after a throwing close does NOT re-invoke onClose (closeCount stays at 1) and returns false.

sbt "WorkflowOperator/testOnly org.apache.texera.amber.operator.source.scan.AutoClosingIteratorSpec"

  • 10/10 pass. sbt scalafmtCheckAll passes.

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

Generated-by: Claude Code (Claude Opus 4.8)

@Yicong-Huang Yicong-Huang 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.

LGTM, thanks

@codecov-commenter

codecov-commenter commented Jun 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 53.07%. Comparing base (7ae9b35) to head (ad1b3b9).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5678      +/-   ##
============================================
- Coverage     53.23%   53.07%   -0.16%     
+ Complexity     2561     2541      -20     
============================================
  Files          1075     1075              
  Lines         42388    42191     -197     
  Branches       4640     4597      -43     
============================================
- Hits          22566    22394     -172     
+ Misses        18502    18480      -22     
+ Partials       1320     1317       -3     
Flag Coverage Δ *Carryforward flag
access-control-service 71.42% <ø> (-3.36%) ⬇️
agent-service 34.36% <ø> (ø) Carriedforward from 7ae9b35
amber 53.89% <100.00%> (-0.32%) ⬇️
computing-unit-managing-service 1.65% <ø> (ø)
config-service 56.71% <ø> (-1.86%) ⬇️
file-service 57.06% <ø> (ø)
frontend 47.50% <ø> (ø) Carriedforward from 7ae9b35
pyamber 90.79% <ø> (ø) Carriedforward from 7ae9b35
python 90.74% <ø> (ø) Carriedforward from 7ae9b35
workflow-compiling-service 58.69% <ø> (ø)

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

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

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

CI benchmark results are noisy; treat <±5% as noise unless repeated.

Dashboard · Run

config throughput MB/s latency max Δ latest / 7d
🔴 bs=10 sw=10 sl=64 350 0.214 24,309/60,687/60,687 us 🔴 +58.3% / 🔴 +68.5%
🟢 bs=100 sw=10 sl=64 821 0.501 120,269/152,529/152,529 us 🟢 +12.6% / 🔴 +7.2%
bs=1000 sw=10 sl=64 946 0.577 1,053,355/1,103,324/1,103,324 us ⚪ within ±5% / 🔴 -6.4%
Baseline details

Latest main 7ae9b35 from 2026-06-13T00:44:28.840Z

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 350 tuples/sec 394.18 tuples/sec 400.34 tuples/sec -11.2% -12.6%
bs=10 sw=10 sl=64 MB/s 0.214 MB/s 0.241 MB/s 0.244 MB/s -11.1% -12.4%
bs=10 sw=10 sl=64 p50 24,309 us 24,805 us 24,191 us -2.0% +0.5%
bs=10 sw=10 sl=64 p95 60,687 us 38,345 us 36,018 us +58.3% +68.5%
bs=10 sw=10 sl=64 p99 60,687 us 38,345 us 36,018 us +58.3% +68.5%
bs=100 sw=10 sl=64 throughput 821 tuples/sec 729.17 tuples/sec 867.19 tuples/sec +12.6% -5.3%
bs=100 sw=10 sl=64 MB/s 0.501 MB/s 0.445 MB/s 0.529 MB/s +12.6% -5.3%
bs=100 sw=10 sl=64 p50 120,269 us 133,219 us 114,874 us -9.7% +4.7%
bs=100 sw=10 sl=64 p95 152,529 us 173,178 us 142,264 us -11.9% +7.2%
bs=100 sw=10 sl=64 p99 152,529 us 173,178 us 142,264 us -11.9% +7.2%
bs=1000 sw=10 sl=64 throughput 946 tuples/sec 946.3 tuples/sec 1,010 tuples/sec -0.0% -6.3%
bs=1000 sw=10 sl=64 MB/s 0.577 MB/s 0.578 MB/s 0.616 MB/s -0.1% -6.4%
bs=1000 sw=10 sl=64 p50 1,053,355 us 1,059,718 us 995,071 us -0.6% +5.9%
bs=1000 sw=10 sl=64 p95 1,103,324 us 1,104,221 us 1,046,228 us -0.1% +5.5%
bs=1000 sw=10 sl=64 p99 1,103,324 us 1,104,221 us 1,046,228 us -0.1% +5.5%
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,571.32,200,128000,350,0.214,24309.34,60687.35,60687.35
1,100,10,64,20,2437.23,2000,1280000,821,0.501,120269.28,152529.43,152529.43
2,1000,10,64,20,21150.51,20000,12800000,946,0.577,1053355.04,1103323.90,1103323.90

@Yicong-Huang Yicong-Huang added this pull request to the merge queue Jun 13, 2026
Merged via the queue into apache:main with commit 1a65a31 Jun 13, 2026
22 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.

Set AutoClosingIterator.alreadyClosed BEFORE invoking onClose so a throwing close is not retried

3 participants