Skip to content

fix(amber): classify wrapped sync timeouts - #7640

Open
anthonychengit wants to merge 1 commit into
apache:mainfrom
anthonychengit:fix/sync-timeout-classification
Open

fix(amber): classify wrapped sync timeouts#7640
anthonychengit wants to merge 1 commit into
apache:mainfrom
anthonychengit:fix/sync-timeout-classification

Conversation

@anthonychengit

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

RxJava's blockingGet can wrap a checked TimeoutException. The previous catch matched only a direct timeout, so a wrapped timeout fell through to the generic error handler and returned Error instead of Killed.

Before: timeout -> wrapper exception -> generic handler -> Error
After:  timeout -> cause-chain check -> timeout handler -> Killed

This change classifies an exception as a timeout when any safe, finite link in its cause chain is a TimeoutException. Tests cover direct and nested timeouts as positive cases, plus unrelated exceptions and null as negative cases.

Any related issues, documentation, discussions?

Closes #6198

How was this PR tested?

sbt "WorkflowExecutionService / Test / testOnly org.apache.texera.web.resource.SyncExecutionResourceSpec -- -z isCausedByTimeout"
sbt scalafmtCheckAll
sbt "scalafixAll --check"

The focused timeout-classification tests passed. Scala formatting and Scalafix checks also passed.

I also ran the complete SyncExecutionResourceSpec. The new timeout cases passed; 8 pre-existing storage-result cases could not complete because the local Iceberg REST catalog expected at localhost:8181 was not running. Those cases are outside this change and will be exercised by the repository CI environment.

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

Generated-by: OpenAI Codex (GPT-5)

@Yicong-Huang Yicong-Huang added the release/v1.2 back porting to release/v1.2 label Aug 13, 2026
@github-actions
github-actions Bot requested a review from xuang7 August 13, 2026 23:13
@github-actions

Copy link
Copy Markdown
Contributor

Backport auto-label report

This fix: PR was checked against each actively-supported release branch. release/* labels drive the post-merge backport, so add or remove one to change where this fix lands.

Release branch Analysis
release/v1.2 Change detected on this branch — label added; this fix is queued to backport here. Requested review from @xuang7.

Auto-label run.

@github-actions

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

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

  • Contributors with relevant context: @mengw15, @aglinxinyuan
    You can notify them by mentioning @mengw15, @aglinxinyuan in a comment.

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

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

Compared against main 12169c2 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 402 0.245 24,069/31,788/31,788 us 🟢 -9.2% / 🔴 +94.4%
bs=100 sw=10 sl=64 804 0.491 121,813/150,022/150,022 us ⚪ within ±5% / 🔴 +39.4%
bs=1000 sw=10 sl=64 905 0.552 1,102,980/1,146,747/1,146,747 us ⚪ within ±5% / 🔴 -12.2%
Baseline details

Latest main 12169c2 from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 402 tuples/sec 429 tuples/sec 774.89 tuples/sec -6.3% -48.1%
bs=10 sw=10 sl=64 MB/s 0.245 MB/s 0.262 MB/s 0.473 MB/s -6.5% -48.2%
bs=10 sw=10 sl=64 p50 24,069 us 22,740 us 12,738 us +5.8% +88.9%
bs=10 sw=10 sl=64 p95 31,788 us 35,012 us 16,348 us -9.2% +94.4%
bs=10 sw=10 sl=64 p99 31,788 us 35,012 us 18,848 us -9.2% +68.7%
bs=100 sw=10 sl=64 throughput 804 tuples/sec 814 tuples/sec 1,005 tuples/sec -1.2% -20.0%
bs=100 sw=10 sl=64 MB/s 0.491 MB/s 0.497 MB/s 0.613 MB/s -1.2% -19.9%
bs=100 sw=10 sl=64 p50 121,813 us 121,776 us 100,970 us +0.0% +20.6%
bs=100 sw=10 sl=64 p95 150,022 us 153,249 us 107,605 us -2.1% +39.4%
bs=100 sw=10 sl=64 p99 150,022 us 153,249 us 116,429 us -2.1% +28.9%
bs=1000 sw=10 sl=64 throughput 905 tuples/sec 919 tuples/sec 1,030 tuples/sec -1.5% -12.2%
bs=1000 sw=10 sl=64 MB/s 0.552 MB/s 0.561 MB/s 0.629 MB/s -1.6% -12.2%
bs=1000 sw=10 sl=64 p50 1,102,980 us 1,082,721 us 991,433 us +1.9% +11.3%
bs=1000 sw=10 sl=64 p95 1,146,747 us 1,152,160 us 1,036,668 us -0.5% +10.6%
bs=1000 sw=10 sl=64 p99 1,146,747 us 1,152,160 us 1,070,470 us -0.5% +7.1%
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,497.44,200,128000,402,0.245,24068.70,31788.23,31788.23
1,100,10,64,20,2486.97,2000,1280000,804,0.491,121813.37,150021.83,150021.83
2,1000,10,64,20,22101.37,20000,12800000,905,0.552,1102979.75,1146747.43,1146747.43

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.65%. Comparing base (12169c2) to head (0c4367e).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...he/texera/web/resource/SyncExecutionResource.scala 71.42% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7640      +/-   ##
============================================
- Coverage     89.65%   89.65%   -0.01%     
- Complexity     4397     4401       +4     
============================================
  Files          1177     1177              
  Lines         46996    47004       +8     
  Branches       5268     5272       +4     
============================================
+ Hits          42136    42140       +4     
- Misses         3094     3097       +3     
- Partials       1766     1767       +1     
Flag Coverage Δ *Carryforward flag
access-control-service 70.00% <ø> (ø) Carriedforward from 12169c2
agent-service 98.62% <ø> (ø) Carriedforward from 12169c2
amber 86.33% <71.42%> (-0.02%) ⬇️
computing-unit-managing-service 72.46% <ø> (ø) Carriedforward from 12169c2
config-service 77.31% <ø> (ø) Carriedforward from 12169c2
file-service 68.90% <ø> (ø) Carriedforward from 12169c2
frontend 90.67% <ø> (ø) Carriedforward from 12169c2
notebook-migration-service 78.89% <ø> (ø) Carriedforward from 12169c2
pyamber 97.57% <ø> (ø) Carriedforward from 12169c2
workflow-compiling-service 57.89% <ø> (ø) Carriedforward from 12169c2

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

@anthonychengit
anthonychengit marked this pull request as ready for review August 15, 2026 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

engine fix release/v1.2 back porting to release/v1.2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sync-execution timeout misclassified as "Error" instead of "Killed" (wrapped TimeoutException)

3 participants