Skip to content

Terminate app process group on exit#1602

Merged
JoshVanL merged 7 commits intodapr:masterfrom
acroca:kill-app-process-tree
Mar 4, 2026
Merged

Terminate app process group on exit#1602
JoshVanL merged 7 commits intodapr:masterfrom
acroca:kill-app-process-tree

Conversation

@acroca
Copy link
Member

@acroca acroca commented Mar 4, 2026

When running an app that spawns other child processes (like go run does) we need to make sure we kill the whole group and not just the parent.

The current implementation leaves the child process orphan, so in some cases like a go run it means the app doesn't really stop when using a dapr stop command. This PR fixes it by killing the whole process tree.

Signed-off-by: Albert Callarisa <albert@diagrid.io>
@acroca acroca requested review from a team as code owners March 4, 2026 10:37
JoshVanL
JoshVanL previously approved these changes Mar 4, 2026
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates dapr run shutdown behavior to terminate the entire application process group (so grandchild processes like those spawned by go run don’t get orphaned), improving reliability of dapr stop/shutdown flows.

Changes:

  • Introduces an OS-specific killProcessGroup(*os.Process) helper (Unix: process-group signaling; Windows: delegates to Process.Kill).
  • Replaces direct Process.Kill() calls with killProcessGroup() when stopping the app in cmd/run.go.
  • Adds Unix implementation that targets the app’s process group to terminate grandchildren.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
cmd/run_unix.go Adds Unix killProcessGroup implementation using Getpgid and group signaling.
cmd/run_windows.go Adds Windows killProcessGroup stub delegating to Process.Kill.
cmd/run.go Switches app termination logic to use killProcessGroup in two shutdown paths.

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

You can also share your feedback on Copilot code review. Take the survey.

Signed-off-by: Albert Callarisa <albert@diagrid.io>
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.


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

Signed-off-by: Albert Callarisa <albert@diagrid.io>
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.


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

Signed-off-by: Albert Callarisa <albert@diagrid.io>
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.


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

Signed-off-by: Albert Callarisa <albert@diagrid.io>
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.


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

Signed-off-by: Albert Callarisa <albert@diagrid.io>
@codecov
Copy link

codecov bot commented Mar 4, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 15.43%. Comparing base (fd34ce5) to head (4ef451e).
⚠️ Report is 24 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1602      +/-   ##
==========================================
- Coverage   15.45%   15.43%   -0.03%     
==========================================
  Files          64       64              
  Lines        7220     7230      +10     
==========================================
  Hits         1116     1116              
- Misses       6018     6028      +10     
  Partials       86       86              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.


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

Signed-off-by: Albert Callarisa <albert@diagrid.io>
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.


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

@JoshVanL
Copy link
Contributor

JoshVanL commented Mar 4, 2026

Not an interesting network failure, looks good

@JoshVanL JoshVanL merged commit 35cba49 into dapr:master Mar 4, 2026
30 of 31 checks passed
@acroca acroca deleted the kill-app-process-tree branch March 4, 2026 13:28
dapr-bot pushed a commit that referenced this pull request Mar 6, 2026
* Kill app process group when stopping

Signed-off-by: Albert Callarisa <albert@diagrid.io>

* Address copilot comments

Signed-off-by: Albert Callarisa <albert@diagrid.io>

* Addressed copilot comments

Signed-off-by: Albert Callarisa <albert@diagrid.io>

* Addressed copilot comments

Signed-off-by: Albert Callarisa <albert@diagrid.io>

* Addressed copilot comments

Signed-off-by: Albert Callarisa <albert@diagrid.io>

* Addressed copilot comments

Signed-off-by: Albert Callarisa <albert@diagrid.io>

* Addressed copilot comments

Signed-off-by: Albert Callarisa <albert@diagrid.io>

---------

Signed-off-by: Albert Callarisa <albert@diagrid.io>
(cherry picked from commit 35cba49)
JoshVanL pushed a commit that referenced this pull request Mar 6, 2026
* Kill app process group when stopping



* Address copilot comments



* Addressed copilot comments



* Addressed copilot comments



* Addressed copilot comments



* Addressed copilot comments



* Addressed copilot comments



---------


(cherry picked from commit 35cba49)

Signed-off-by: Albert Callarisa <albert@diagrid.io>
Co-authored-by: Albert Callarisa <albert@diagrid.io>
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.

3 participants