Skip to content

fix(amber): prevent region termination retry hang - #7122

Open
VuMartin wants to merge 5 commits into
apache:mainfrom
VuMartin:fix/6920-region-termination-retry
Open

fix(amber): prevent region termination retry hang#7122
VuMartin wants to merge 5 commits into
apache:mainfrom
VuMartin:fix/6920-region-termination-retry

Conversation

@VuMartin

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

This PR fixes a region termination hang caused by unresponsive EndWorker RPC calls.

Previously, Future.collect(endWorkerRequests) could wait forever if a worker never responded. This prevented the termination retry mechanism from running because the failure was never propagated, causing the workflow to silently stall.

This change adds timeout handling around the termination futures so stuck EndWorker and gracefulStop operations fail and trigger the existing retry budget. Cleanup of actor references and control channels is delayed until graceful termination succeeds, preventing retries from using stale worker state.

Before: gracefulStop timeout → silent hang forever.
Screenshot 2026-07-29 at 1 26 05 AM

After: gracefulStop timeout → retries 150 times → error.
Screenshot 2026-07-29 at 11 21 16 PM

Any related issues, documentation, discussions?

Fixes #6920

How was this PR tested?

Manual: Simulated slow worker shutdown (> 5s delay in postStop). Confirmed retry loop executes all 150 attempts and error fires with stuck workers listed. No silent hang.

Ran the Amber test suite locally.

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

Generated-by: ChatGPT (5.5-mini)

@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: @aglinxinyuan
    You can notify them by mentioning @aglinxinyuan in a comment.

@VuMartin VuMartin changed the title Add timeout handling for region termination retries fix(amber): prevent region termination retry hang Jul 30, 2026
@codecov-commenter

codecov-commenter commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.56%. Comparing base (301e3a8) to head (010367b).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7122      +/-   ##
============================================
- Coverage     79.57%   79.56%   -0.01%     
+ Complexity     3837     3833       -4     
============================================
  Files          1159     1159              
  Lines         46145    46149       +4     
  Branches       5127     5127              
============================================
  Hits          36718    36718              
  Misses         7799     7799              
- Partials       1628     1632       +4     
Flag Coverage Δ *Carryforward flag
access-control-service 70.00% <ø> (ø) Carriedforward from 301e3a8
agent-service 77.42% <ø> (ø) Carriedforward from 301e3a8
amber 73.06% <100.00%> (-0.02%) ⬇️
computing-unit-managing-service 20.49% <ø> (ø) Carriedforward from 301e3a8
config-service 66.66% <ø> (ø) Carriedforward from 301e3a8
file-service 67.21% <ø> (ø) Carriedforward from 301e3a8
frontend 83.05% <ø> (ø) Carriedforward from 301e3a8
notebook-migration-service 78.94% <ø> (ø) Carriedforward from 301e3a8
pyamber 97.36% <ø> (ø) Carriedforward from 301e3a8
workflow-compiling-service 26.31% <ø> (ø) Carriedforward from 301e3a8

*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

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 Not labeled automatically — none of the files this PR modifies exist on this branch (amber/src/main/scala/org/apache/texera/amber/engine/architecture/scheduling/RegionExecutionManager.scala). The fix may target code that isn't on this release, or the files were moved/renamed after the branch was cut. Please check and add release/v1.2 by hand if this fix should be backported here.

Auto-label run.

@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

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

Compared against main 301e3a8 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 379 0.231 25,771/38,184/38,184 us 🔴 +15.0% / 🔴 +142.1%
🔴 bs=100 sw=10 sl=64 785 0.479 124,836/155,611/155,611 us 🔴 -6.6% / 🔴 +44.9%
bs=1000 sw=10 sl=64 922 0.563 1,090,401/1,119,066/1,119,066 us ⚪ within ±5% / 🔴 +11.2%
Baseline details

Latest main 301e3a8 from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 379 tuples/sec 430 tuples/sec 786.12 tuples/sec -11.9% -51.8%
bs=10 sw=10 sl=64 MB/s 0.231 MB/s 0.263 MB/s 0.48 MB/s -12.2% -51.9%
bs=10 sw=10 sl=64 p50 25,771 us 22,416 us 12,305 us +15.0% +109.4%
bs=10 sw=10 sl=64 p95 38,184 us 34,339 us 15,774 us +11.2% +142.1%
bs=10 sw=10 sl=64 p99 38,184 us 34,339 us 18,978 us +11.2% +101.2%
bs=100 sw=10 sl=64 throughput 785 tuples/sec 840 tuples/sec 999.71 tuples/sec -6.5% -21.5%
bs=100 sw=10 sl=64 MB/s 0.479 MB/s 0.513 MB/s 0.61 MB/s -6.6% -21.5%
bs=100 sw=10 sl=64 p50 124,836 us 117,650 us 100,616 us +6.1% +24.1%
bs=100 sw=10 sl=64 p95 155,611 us 151,823 us 107,356 us +2.5% +44.9%
bs=100 sw=10 sl=64 p99 155,611 us 151,823 us 113,255 us +2.5% +37.4%
bs=1000 sw=10 sl=64 throughput 922 tuples/sec 917 tuples/sec 1,031 tuples/sec +0.5% -10.6%
bs=1000 sw=10 sl=64 MB/s 0.563 MB/s 0.56 MB/s 0.63 MB/s +0.5% -10.6%
bs=1000 sw=10 sl=64 p50 1,090,401 us 1,093,712 us 980,328 us -0.3% +11.2%
bs=1000 sw=10 sl=64 p95 1,119,066 us 1,138,831 us 1,027,528 us -1.7% +8.9%
bs=1000 sw=10 sl=64 p99 1,119,066 us 1,138,831 us 1,054,298 us -1.7% +6.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,527.31,200,128000,379,0.231,25771.09,38183.50,38183.50
1,100,10,64,20,2548.53,2000,1280000,785,0.479,124835.91,155611.33,155611.33
2,1000,10,64,20,21687.20,20000,12800000,922,0.563,1090400.83,1119065.99,1119065.99

@Yicong-Huang

Copy link
Copy Markdown
Contributor

Thanks for the fix, I will check details tomorrow. It is related to @aglinxinyuan's recent efforts in #6891 #6918.

two high level comments for this quick fix:

  1. please remove a actor ref once receive return from terminating it. we want to prevent other actors from reaching it.
  2. please keep the time out shorter. 30s is quite long. those messages should happen in ms level.

We need to figure out the root cause of the hanging, the timeout can only be a patch for short term.

@VuMartin

Copy link
Copy Markdown
Contributor Author

Thanks. I updated the termination timeout to 5s. For the actor ref cleanup, I kept the removal after successful graceful termination because gracefulStop still requires the actor ref to be available. The actor ref is removed immediately after termination succeeds to prevent further access.

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.

Region-termination retry budget is bypassed by non-delivery, wedging the workflow silently

3 participants