refactor: introduce managed process lifecycle#2216
Conversation
📝 WalkthroughWalkthroughThis pull request refactors process lifecycle management by introducing ChangesProcess Lifecycle Management Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/lifecycle.go`:
- Around line 94-118: Release() currently calls p.platform.release() without
synchronization while Stop() holds p.mu, risking concurrent access; change
Release() to acquire p.mu before invoking p.stopWatch() and p.platform.release()
(while still using p.releaseOnce.Do to ensure one-time semantics) and store the
result into p.releaseErr under the same lock to avoid races with Stop(); keep
the early nil check for p and ensure the lock is released before returning.
- Around line 54-57: When proc.platform.afterStart(cmd) fails, the started
process isn't reaped; call cmd.Wait() (on the same *exec.Cmd referenced as cmd)
before/while releasing platform resources to avoid leaving a zombie. Update the
failure branch that currently calls proc.platform.release() and returns
fmt.Errorf("failed to contain process: %w", err) to first invoke cmd.Wait(),
handle/collect any Wait error (append or wrap it with the original afterStart
error) and then call proc.platform.release(); ensure the final returned error
includes both the afterStart error and any cmd.Wait() error for visibility.
🪄 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: 852223b0-3b26-4d04-9094-c3d2a42daa0b
📒 Files selected for processing (12)
internal/cmn/cmdutil/cmd_unix.gointernal/cmn/cmdutil/cmd_windows.gointernal/cmn/cmdutil/lifecycle.gointernal/cmn/cmdutil/lifecycle_test.gointernal/cmn/cmdutil/lifecycle_unix.gointernal/cmn/cmdutil/lifecycle_windows.gointernal/cmn/cmdutil/termination.gointernal/runtime/builtin/command/command.gointernal/runtime/builtin/command/command_test.gointernal/runtime/builtin/harness/harness.gointernal/runtime/executor/dag_runner.gointernal/runtime/executor/dag_runner_test.go
💤 Files with no reviewable changes (1)
- internal/cmn/cmdutil/cmd_windows.go
Summary
Testing
Summary by cubic
Introduced
cmdutil.ManagedProcessto manage process lifecycles with clear stop requests and outcomes, and migrated command, harness, and local DAG execution to use it. Cleanup is hardened on containment or parent-watcher failures; Windows uses Job Objects with a process-tree fallback, Unix uses process groups.Refactors
cmdutil.ManagedProcesswithStopRequest/StopOutcome, parent-exit watching, and idempotentRelease.SetupCommandpreserves existingSysProcAttrand ensures PGID is set.PID(); release on exit.TerminateProcessGrouphelpers, now backed by the lifecycle layer.Migration
cmdutil.StartManagedProcess(cmd), thenproc.Stop(...),proc.Wait(), andproc.Release().GracefulTermination/ForceTermination) viaStopRequest(optionally include a reason) over raw signals.Written for commit 0f7979d. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Refactor
Tests