Skip to content

fix(WorkflowExecutionService): shutdown console writer thread on unsubscribe - #7914

Open
VuMartin wants to merge 3 commits into
apache:mainfrom
VuMartin:fix/console-writer-thread-leak
Open

fix(WorkflowExecutionService): shutdown console writer thread on unsubscribe#7914
VuMartin wants to merge 3 commits into
apache:mainfrom
VuMartin:fix/console-writer-thread-leak

Conversation

@VuMartin

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Fixes a console writer thread leak in ExecutionConsoleService.

The console writer executor was not being shut down when an execution service was unsubscribed. This could leave texera-console-writer threads alive after workflow execution finished.

Before:
Screenshot 2026-08-23 at 11 40 04 PM

After:
Screenshot 2026-08-23 at 6 40 25 PM

Screenshot 2026-08-23 at 6 41 53 PM Screenshot 2026-08-23 at 6 42 18 PM Screenshot 2026-08-23 at 6 42 42 PM Screenshot 2026-08-23 at 6 42 55 PM

This PR:

  • Shuts down the console writer executor during unsubscribeAll().
  • Waits for termination and falls back to shutdownNow() if necessary.
  • Closes active console message writers and clears the writer map.
  • Adds a test verifying the console writer executor is shut down and terminated.

Any related issues, documentation, discussions?

Fixes #7455

How was this PR tested?

Ran:

sbt "project WorkflowExecutionService" "testOnly *ExecutionConsoleServiceSpec -- -z unsubscribeAll"

Manual testing:

Ran workflows multiple times and checked the console writer threads with:

jcmd 57436 Thread.print | grep "texera-console-writer"
jcmd 67548 Thread.print | grep "texera-console-writer"

Verified that the console writer threads are terminated after workflow execution completes and unsubscribeAll() is called.

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

@Yicong-Huang Yicong-Huang added the release/v1.2 back porting to release/v1.2 label Aug 24, 2026
@github-actions
github-actions Bot requested a review from xuang7 August 24, 2026 09:15
@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

⚠️ Benchmark changes need a look

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

Compared against main 84e2d7d 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 367 0.224 25,757/35,421/35,421 us 🟢 -8.3% / 🔴 +147.1%
🔴 bs=100 sw=10 sl=64 798 0.487 122,402/161,127/161,127 us 🔴 +18.4% / 🔴 +62.0%
bs=1000 sw=10 sl=64 910 0.556 1,097,954/1,174,433/1,174,433 us ⚪ within ±5% / 🔴 +22.5%
Baseline details

Latest main 84e2d7d from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 367 tuples/sec 391 tuples/sec 833.1 tuples/sec -6.1% -55.9%
bs=10 sw=10 sl=64 MB/s 0.224 MB/s 0.239 MB/s 0.508 MB/s -6.3% -55.9%
bs=10 sw=10 sl=64 p50 25,757 us 24,389 us 11,860 us +5.6% +117.2%
bs=10 sw=10 sl=64 p95 35,421 us 38,643 us 14,334 us -8.3% +147.1%
bs=10 sw=10 sl=64 p99 35,421 us 38,643 us 18,194 us -8.3% +94.7%
bs=100 sw=10 sl=64 throughput 798 tuples/sec 832 tuples/sec 1,085 tuples/sec -4.1% -26.5%
bs=100 sw=10 sl=64 MB/s 0.487 MB/s 0.508 MB/s 0.662 MB/s -4.1% -26.5%
bs=100 sw=10 sl=64 p50 122,402 us 120,405 us 92,973 us +1.7% +31.7%
bs=100 sw=10 sl=64 p95 161,127 us 136,095 us 99,460 us +18.4% +62.0%
bs=100 sw=10 sl=64 p99 161,127 us 136,095 us 107,737 us +18.4% +49.6%
bs=1000 sw=10 sl=64 throughput 910 tuples/sec 924 tuples/sec 1,111 tuples/sec -1.5% -18.1%
bs=1000 sw=10 sl=64 MB/s 0.556 MB/s 0.564 MB/s 0.678 MB/s -1.4% -18.0%
bs=1000 sw=10 sl=64 p50 1,097,954 us 1,079,444 us 915,816 us +1.7% +19.9%
bs=1000 sw=10 sl=64 p95 1,174,433 us 1,129,274 us 959,066 us +4.0% +22.5%
bs=1000 sw=10 sl=64 p99 1,174,433 us 1,129,274 us 993,644 us +4.0% +18.2%
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,545.57,200,128000,367,0.224,25757.10,35420.89,35420.89
1,100,10,64,20,2506.24,2000,1280000,798,0.487,122401.78,161126.70,161126.70
2,1000,10,64,20,21972.91,20000,12800000,910,0.556,1097953.94,1174433.04,1174433.04

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.92%. Comparing base (bd54a85) to head (e49b30d).
⚠️ Report is 38 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...e/texera/web/service/ExecutionConsoleService.scala 75.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7914      +/-   ##
============================================
+ Coverage     91.53%   91.92%   +0.39%     
- Complexity     4471     4569      +98     
============================================
  Files          1172     1170       -2     
  Lines         47311    47339      +28     
  Branches       5305     5302       -3     
============================================
+ Hits          43305    43516     +211     
+ Misses         2348     2200     -148     
+ Partials       1658     1623      -35     
Flag Coverage Δ *Carryforward flag
access-control-service 81.00% <ø> (ø) Carriedforward from bd54a85
agent-service 98.62% <ø> (ø) Carriedforward from bd54a85
amber 89.11% <75.00%> (+1.07%) ⬆️
computing-unit-managing-service 73.67% <ø> (ø) Carriedforward from bd54a85
config-service 86.73% <ø> (ø) Carriedforward from bd54a85
file-service 73.95% <ø> (ø) Carriedforward from bd54a85
frontend 93.28% <ø> (ø) Carriedforward from bd54a85
notebook-migration-service 79.13% <ø> (ø) Carriedforward from bd54a85
pyamber 97.57% <ø> (ø) Carriedforward from bd54a85
workflow-compiling-service 77.19% <ø> (ø) Carriedforward from bd54a85

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

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.

ExecutionConsoleService leaks a non-daemon thread per execution

3 participants