refactor: deepen process termination intent by yohamta0 · Pull Request #2210 · dagucloud/dagu

yohamta0 · 2026-05-26T01:31:52Z

Summary

Refactor process termination around lifecycle intent so runtime code requests graceful or forceful stops without assuming Unix signal semantics.

Changes

Add a cmdutil.TerminationIntent Module with graceful and force modes plus compatibility wrappers for legacy signal-based callers.
Route Node, Runner, Agent cleanup, command, harness, sub-DAG, bash, and terminal stop paths through intent-based termination.
Keep Unix signal delivery and Windows process-tree termination behind platform Adapters.
Add regression coverage for force intent and signal_on_stop override behavior.

Related Issues

N/A

Checklist

Code follows the project style guidelines
Self-review of the code has been performed
Tests have been added or updated as needed
Documentation has been updated as needed (not applicable; internal process lifecycle refactor)
Changes have been tested locally

Testing

go test ./internal/cmn/cmdutil ./internal/runtime/... ./internal/engine ./internal/agent ./internal/service/frontend/terminal -count=1
GOOS=windows GOARCH=amd64 go test -c -o /tmp/dagu-cmdutil.test.exe ./internal/cmn/cmdutil
GOOS=windows GOARCH=amd64 go test -c -o /tmp/dagu-runtime.test.exe ./internal/runtime
GOOS=windows GOARCH=amd64 go test -c -o /tmp/dagu-command.test.exe ./internal/runtime/builtin/command
GOOS=windows GOARCH=amd64 go test -c -o /tmp/dagu-runtime-agent.test.exe ./internal/runtime/agent
GOOS=windows GOARCH=amd64 go test -c -o /tmp/dagu-agent.test.exe ./internal/agent
GOOS=windows GOARCH=amd64 go test -c -o /tmp/dagu-terminal.test.exe ./internal/service/frontend/terminal

Summary by CodeRabbit

Refactor
- Introduced an intent-based stop model for unified graceful/force termination and updated process-group termination across platforms and components.
Tests
- Added and expanded tests validating termination intents, stop behavior, and force-vs-graceful shutdown handling.

coderabbitai · 2026-05-26T01:32:05Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 82026fbe-fd54-4ca0-9174-a131aaf0c378

📥 Commits

Reviewing files that changed from the base of the PR and between c8cd1c4 and 2c19f0f.

📒 Files selected for processing (4)

internal/cmn/cmdutil/termination.go
internal/cmn/cmdutil/termination_test.go
internal/intg/distr/zombie_recovery_test.go
internal/runtime/builtin/command/command_unix_test.go

📝 Walkthrough

Walkthrough

This PR refactors process termination from signal-based to intent-based semantics. It introduces TerminationIntent carrying mode and signal, adds intent-based TerminateProcessGroup APIs (Unix/Windows), defines an executor Stopper interface, updates executors to implement Stop(intent), and propagates intent-driven stop requests through Node, Runner, and Agent with timeout-based escalation.

Changes

Process Termination Intent Refactoring

Layer / File(s)	Summary
Core Termination Intent Model `internal/cmn/cmdutil/termination.go`, `internal/cmn/cmdutil/termination_test.go`	Introduces `TerminationMode` (graceful/force) and `TerminationIntent` with constructors (`TerminationFromSignal`, `GracefulTermination`, `ForceTermination`) and helpers (`IsForce`, `IsTermination`, `SignalName`, `WithSignal`); unit tests validate mapping and overrides.
Platform Process Termination APIs `internal/cmn/cmdutil/cmd_unix.go`, `internal/cmn/cmdutil/cmd_windows.go`, `internal/cmn/cmdutil/parent_exit_watcher_unix.go`, `internal/cmn/cmdutil/parent_exit_watcher_windows.go`, `internal/service/frontend/terminal/process_windows.go`	Unix and Windows now export intent-based `TerminateProcessGroup` and `TerminateMultipleProcessGroups`; deprecated signal-based `KillProcessGroup` and `KillMultipleProcessGroups` delegate to new functions via `TerminationFromSignal`; Windows `setupCommand` ensures new process groups; parent-exit watcher and terminal utilities use forced intent termination.
Executor Stopper Interface and Implementations `internal/runtime/executor/executor.go`, `internal/runtime/builtin/command/command.go`, `internal/runtime/builtin/harness/harness.go`, `internal/runtime/executor/dag_runner.go`	Adds `Stopper` interface with `Stop(TerminationIntent) error`; command, harness, and DAG-runner executors implement `Stopper`, with `Kill(sig)` delegating to intent-based `Stop` that calls `TerminateProcessGroup`.
Node and Runner Stop Propagation `internal/runtime/node.go`, `internal/runtime/node_test.go`, `internal/runtime/runner.go`	Adds `Node.Stop(ctx, intent, allowOverride)` and `Runner.Stop(ctx, plan, intent, ...)`; `Signal` methods delegate to `Stop`; node applies `SignalOnStop` overrides and prefers `executor.Stopper.Stop(intent)` with fallback to `cmd.Kill(intent.Signal)`; tests verify graceful and forced intents are observed.
Agent Orchestration with Timeout Escalation `internal/runtime/agent/agent.go`, `internal/agent/bash.go`, `internal/intg/distr/zombie_recovery_test.go`, `internal/runtime/builtin/command/command_unix_test.go`	Replaces signal-based `signal` helper with intent-driven `stopChildren` that coordinates graceful termination with `MaxCleanUpTime`, periodic retry, and forced-termination escalation using `ForceTermination()` when cleanup expires.

Sequence Diagram(s)

sequenceDiagram
  participant Agent
  participant Runner as Runner.Stop
  participant Node as Node.Stop
  participant Executor as Executor.Stop
  participant Terminator as TerminateProcessGroup
  Agent->>Agent: stopChildren(sig, allowOverride)
  Note over Agent: intent = TerminationFromSignal(sig)
  Agent->>Runner: Stop(intent, maxCleanUpTime)
  loop Graceful phase
    Runner->>Node: Stop(intent, allowOverride)
    activate Node
    Note over Node: Apply SignalOnStop override
    Node->>Executor: Stop(intent)
    Executor->>Terminator: TerminateProcessGroup(cmd, intent)
    deactivate Node
    Note over Agent: Poll, retry every N ms
  end
  alt Timeout
    Agent->>Runner: Stop(ForceTermination())
    Runner->>Node: Stop(ForceTermination(), allowOverride)
    Node->>Executor: Stop(ForceTermination())
    Executor->>Terminator: TerminateProcessGroup(cmd, ForceTermination())
  else Success
    Note over Agent: Done channel signals completion
  end

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

dagucloud/dagu#2125: Overlapping parent-exit watcher cleanup changes replacing signal-based escalation with intent-based TerminateProcessGroup(..., ForceTermination()).
dagucloud/dagu#2034: Related changes to node stop/signal flow and SignalOnStop/allowOverride logic.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'refactor: deepen process termination intent' clearly and concisely summarizes the main change—refactoring process termination to use explicit lifecycle intent instead of signal semantics.
Description check	✅ Passed	The PR description comprehensively covers all required template sections: Summary describes the refactoring objective, Changes lists key modifications, Related Issues confirms N/A, and Checklist confirms all items completed including testing across platforms.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch refactor/process-termination-intent

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/cmn/cmdutil/termination.go`:
- Around line 41-46: GracefulTermination currently returns
Mode=TerminationModeGraceful even for force-class signals (e.g., os.Kill), which
violates the "force signal => force mode" invariant enforced by
TerminationFromSignal and WithSignal; update GracefulTermination(sig os.Signal)
so it normalizes force signals by detecting the same force-class signals those
functions use and returning TerminationIntent{Mode: TerminationModeForce,
Signal: sig} when sig is a force signal, otherwise return the existing graceful
intent—follow the same detection logic used in TerminationFromSignal/WithSignal
so behavior is consistent across these helpers.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 41880d11-443f-4f54-a176-0cf92645c625

📥 Commits

Reviewing files that changed from the base of the PR and between d1cbf38 and c8cd1c4.

📒 Files selected for processing (16)

internal/agent/bash.go
internal/cmn/cmdutil/cmd_unix.go
internal/cmn/cmdutil/cmd_windows.go
internal/cmn/cmdutil/parent_exit_watcher_unix.go
internal/cmn/cmdutil/parent_exit_watcher_windows.go
internal/cmn/cmdutil/termination.go
internal/cmn/cmdutil/termination_test.go
internal/runtime/agent/agent.go
internal/runtime/builtin/command/command.go
internal/runtime/builtin/harness/harness.go
internal/runtime/executor/dag_runner.go
internal/runtime/executor/executor.go
internal/runtime/node.go
internal/runtime/node_test.go
internal/runtime/runner.go
internal/service/frontend/terminal/process_windows.go

yohamta0 · 2026-05-26T01:49:52Z

@coderabbitai review

coderabbitai · 2026-05-26T01:49:58Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

refactor: deepen process termination intent

c8cd1c4

coderabbitai Bot reviewed May 26, 2026

View reviewed changes

Comment thread internal/cmn/cmdutil/termination.go

yohamta0 added 2 commits May 26, 2026 10:45

test: use termination intent in process cleanup

601456e

fix: normalize force signal termination intent

2c19f0f

yohamta0 merged commit 1f41fc5 into main May 26, 2026
10 checks passed

yohamta0 deleted the refactor/process-termination-intent branch May 26, 2026 02:17

coderabbitai Bot mentioned this pull request May 27, 2026

refactor: introduce managed process lifecycle #2216

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: deepen process termination intent#2210

refactor: deepen process termination intent#2210
yohamta0 merged 3 commits into
mainfrom
refactor/process-termination-intent

yohamta0 commented May 26, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented May 26, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

yohamta0 commented May 26, 2026

Uh oh!

coderabbitai Bot commented May 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

yohamta0 commented May 26, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Related Issues

Checklist

Testing

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yohamta0 commented May 26, 2026

Uh oh!

coderabbitai Bot commented May 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

yohamta0 commented May 26, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 26, 2026 •

edited

Loading