Repository navigation
Add Unix process-group cleanup alongside process trees - #288
David Negstad (danegsta) wants to merge 24 commits into
Conversation
Implement native Windows, Linux, and Darwin process snapshots in pkg/process. Validate ancestry and process identities, propagate shutdown contexts, and roll back failed starts through owned process references. Fail closed when startup cleanup is uncertain, migrate callers and fixtures, and remove the unused dependency graph and notices.\n\nAddresses #274. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use explicit process inspection and cleanup-job rights, and open suspended threads with the thread-specific resume right. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use a named VEOF character constant and annotate each write that terminates child input. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 43ddd59f-7775-46a1-8fd7-ebab5b17f2f6
Recognize native process-gone errors after leader exit and retain detached cleanup on monitor cancellation. Avoid concurrent automatic cleanup in group regression tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 43ddd59f-7775-46a1-8fd7-ebab5b17f2f6
Preserve finalized upstream process handling and delay-based tests while retaining Unix process-group and process-tree cleanup. Adapt group inspection to the current ProcessHandle APIs. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 43ddd59f-7775-46a1-8fd7-ebab5b17f2f6
Keep owner monitoring active after child inspection errors and avoid group enumeration while the verified leader is alive. Use existing PID conversion helpers throughout group cleanup and its tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 43ddd59f-7775-46a1-8fd7-ebab5b17f2f6
There was a problem hiding this comment.
🟡 Changes recommended
Group inspection errors can abort shutdown before forced termination and verified-descendant cleanup, leaving processes running.
1 open issue
What changed in this PR
Adds Unix process-group cleanup alongside the identity-aware process-tree cleanup from #278, targeting children created during shutdown while retaining cleanup for discovered descendants outside the group.
Changes:
- Isolates ordinary Unix launches and adds group monitoring and bounded termination.
- Keeps
monitor-processactive after the group leader exits. - Adds lifecycle tests and documents group cleanup and root-only behavior.
| File | Description |
|---|---|
| README.md | Documents group monitoring, cleanup, and limitations. |
| pkg/process/process_types.go | Adds captured-group stop options and API documentation. |
| pkg/process/process_group.go | Defines group identity and cancellable waiting. |
| pkg/process/process_group_windows.go | Provides Windows compatibility stubs. |
| pkg/process/process_group_unix.go | Implements group discovery, validation, signaling, and enumeration. |
| pkg/process/process_group_unix_test.go | Tests isolation, cleanup, cancellation, and identity validation. |
| pkg/process/process_group_linux_test.go | Tests enumeration behavior before and after leader exit. |
| pkg/process/os_executor.go | Integrates group stopping into shared tree-cleanup phases. |
| pkg/process/os_executor_unix.go | Adds group termination and default launch isolation. |
| internal/dcpproc/process_group_unix_test.go | Tests watchdog behavior after leader exit and inspection failures. |
| internal/dcpproc/commands/stop_process_tree.go | Documents group cleanup in command help. |
| internal/dcpproc/commands/process.go | Extends watchdog monitoring and cleanup to captured groups. |
Review effort: Balanced
| if options.group != nil { | ||
| stopResult, stopErr = e.stopProcessGroup(stopCtx, options.group, rootOpts|optWaitForStdio) |
Karol Zadora-Przylecki (karolz-ms)
left a comment
There was a problem hiding this comment.
🤖 Copilot Code Review
Reviewed Unix process-group isolation, monitoring, and tree/group cleanup in PR #288. Left inline comments on process-group member enumeration, executor group retention across leader exit, mid-snapshot leader exit handling, monitor-process PID-reuse error handling, and concurrent stopProcessGroup ownership/state.
| func (g *ProcessGroup) members(ctx context.Context) ([]processInfo, error) { | ||
| targetGroupID, targetGroupIDErr := PidT_ToInt(g.leader.Pid) | ||
| if targetGroupIDErr != nil { | ||
| return nil, targetGroupIDErr | ||
| } | ||
| processes, snapshotErr := snapshotProcesses(ctx) | ||
| if snapshotErr != nil { | ||
| return nil, fmt.Errorf("could not enumerate process group %d: %w", g.leader.Pid, snapshotErr) | ||
| } | ||
| members := make([]processInfo, 0) | ||
| for _, info := range processes { | ||
| if contextErr := ctx.Err(); contextErr != nil { | ||
| return nil, contextErr | ||
| } | ||
| memberPID, memberPIDErr := PidT_ToInt(info.handle.Pid) | ||
| if memberPIDErr != nil { | ||
| return nil, fmt.Errorf("could not get process group for process %d: %w", info.handle.Pid, memberPIDErr) | ||
| } | ||
| groupID, groupErr := unix.Getpgid(memberPID) | ||
| if errors.Is(groupErr, syscall.ESRCH) { | ||
| continue | ||
| } | ||
| if groupErr != nil { | ||
| return nil, fmt.Errorf("could not get process group for process %d: %w", info.handle.Pid, groupErr) | ||
| } | ||
| if groupID == targetGroupID { | ||
| members = append(members, info) | ||
| } | ||
| } | ||
| return members, nil |
There was a problem hiding this comment.
❌ error: ProcessGroup.members discards readable process records on partial snapshot errors, fails to wrap ErrIncompleteProcessTree, and uses post-snapshot unix.Getpgid syscalls across all system processes.
- Discarding
processeson partialsnapshotErr(lines 157–160):snapshotProcesses(ctx)returns both all successfully readprocessesand a summarizedsnapshotErrfor unreadable entries. Becauseg.membersimmediately returnsnil, fmt.Errorf(...)whensnapshotErr != nil,g.isRunningfails with an error even when a live member of the target group (!member.exited) is already present inprocesses(which also causeschildGroup.Waitindcp monitor-processto exit its wait loop on a single unrelated/procinspection error after leader exit). Additionally, when no live group member is observed andsnapshotErr != nil, the returned error does not wrapErrIncompleteProcessTree, breaking callers that checkerrors.Is(err, process.ErrIncompleteProcessTree)(internal/dcpproc/commands/stop_process_tree.go:88,controllers/physical_process_controller.go:631). - Post-snapshot
unix.Getpgid(memberPID)loop (lines 166–179):- TOCTOU & Darwin zombie gap: If a group member
M1is alive (!info.exited) whensnapshotProcessesreads it, forksM2into a lower PID already passed by the scan, and exits beforeunix.Getpgid(memberPID)runs (or becomes a zombieSZOMBon Darwin, where XNU'sgetpgidusesproc_findand returnsESRCHfor zombies),if errors.Is(groupErr, syscall.ESRCH) { continue }silently skipsM1, defeating the double-snapshot check ing.isRunning. - Cross-session / LSM errors:
processescontains all OS processes across all users and sessions. POSIX permitsgetpgidto returnEPERMfor processes in another session, and Linux LSMs (e.g., SELinuxprocess:getpgid) can returnEACCES/EPERMon confined system processes; any non-ESRCHerror on an unrelated PID abortsg.members.
- TOCTOU & Darwin zombie gap: If a group member
Suggested fix: Populate groupID directly in processInfo when decoding /proc/<pid>/stat (fields[2] in parseLinuxProcessStat) and Darwin's KinfoProc (native.Eproc.Pgid in decodeDarwinProcessInfo). In g.members / g.isRunning, inspect the returned processes even when snapshotErr != nil: if a live group member is present, return true, nil; only return an error wrapping ErrIncompleteProcessTree when no live group member is observed and snapshotErr != nil.
| if (options.opts & optSkipDescendants) != 0 { | ||
| options.group = nil | ||
| } else if options.group == nil { | ||
| var groupErr error | ||
| options.group, groupErr = FindProcessGroup(handle) | ||
| if groupErr != nil && !IsProcessGoneErr(groupErr) { | ||
| return fmt.Errorf("could not identify process group for process %d: %w", handle.Pid, groupErr) | ||
| } | ||
| } |
There was a problem hiding this comment.
❌ error: OSExecutor does not retain the ProcessGroup for processes it starts, and FindProcessGroup fails once the leader exits or becomes a zombie.
OSExecutor.prepareProcessStartplaces Unix child processes into an isolated process group (Setpgid: true), andOSExecutor.doWaitreaps the leader as soon as it exits. However,OSExecutordoes not capture or store*ProcessGrouponwaitStateduringstartProcess(unlikewinConsoleAvailabilityon Windows), instead callingFindProcessGroup(handle)lazily instopProcessInternal.- Furthermore,
FindProcessGroup(pkg/process/process_group_unix.go:30, 47) callshandle.findProcessInfo(), which returnsErrProcessNotFoundwheninfo.exitedis true (pkg/process/process_handle.go:133), even when the leader still exists in the process table as an unreaped zombie with matching identity. - As a result, for any process started via
OSExecutor.StartProcessorStartAndForget(including V1Executableand V2PhysicalProcess), if the leader spawns group members and exits beforeStopProcessis called (e.g., right asctxexpires inStartProcess, duringDispose(), or before an explicitStopProcesscall),FindProcessGroup(handle)returnsErrProcessNotFound,options.groupremainsnil, andStopProcessfails to clean up the surviving process group. Similarly, if a short-lived leader exits before the asynchronously spawneddcp monitor-processreachesFindProcessGroup(childHandle)(internal/dcpproc/commands/process.go:84),monitor-processtreats the zombie/reaped leader as gone and exits without monitoring or cleaning up the surviving group.
Suggested fix: Capture *ProcessGroup in OSExecutor.startProcess (before doWait can reap the leader) and store it on waitState so stopProcessInternal can populate options.group when options.group == nil and (options.opts & optSkipDescendants) == 0. In FindProcessGroup, use readProcessInfo(handle.Pid) + validateIdentity(handle, info.handle) (matching hasLiveLeader in pkg/process/process_group_unix.go:64-72) so an unreaped zombie leader with matching identity still resolves to its ProcessGroup.
| tree, treeErr := resolveProcessTree(graceCtx, handle) | ||
| if treeErr != nil && !errors.Is(treeErr, ErrIncompleteProcessTree) { | ||
| if errors.Is(treeErr, context.DeadlineExceeded) && ctx.Err() == nil { | ||
| if options.group != nil && IsProcessGoneErr(treeErr) && !errors.Is(treeErr, ErrProcessIdentityMismatch) { | ||
| // Captured group members remain addressable after the leader's ancestry is gone. | ||
| tree = []ProcessHandle{handle} | ||
| treeErr = nil | ||
| } else if errors.Is(treeErr, context.DeadlineExceeded) && ctx.Err() == nil { | ||
| procTreeLog.Error(treeErr, "Process tree enumeration exceeded the graceful-stop deadline; force-stopping only the root") | ||
| return forceRootAfterIncompleteEnumeration(treeErr) | ||
| } else { | ||
| return fmt.Errorf("could not get process tree for process %d: %w", handle.Pid, treeErr) | ||
| } | ||
| return fmt.Errorf("could not get process tree for process %d: %w", handle.Pid, treeErr) | ||
| } |
There was a problem hiding this comment.
GetProcessTree causes StopProcess to fail with ErrIncompleteProcessTree even when options.group != nil.
If the group leader exits before resolveProcessTree (GetProcessTree) starts, treeErr is ErrProcessNotFound (without ErrIncompleteProcessTree), which lines 675–678 clear (tree = []ProcessHandle{handle}, treeErr = nil) when options.group != nil.
However, if the group leader exits during GetProcessTree (between before and after in getProcessTree, pkg/process/process_tree.go:61-75), GetProcessTree returns tree along with fmt.Errorf("%w: root process %d exited or changed identity after the process snapshot: %w", ErrIncompleteProcessTree, root.Pid, afterErr). Because errors.Is(treeErr, ErrIncompleteProcessTree) is true, stopProcessTreeInternal retains treeErr even when afterErr is ErrProcessNotFound (and not ErrProcessIdentityMismatch), causing StopProcess and dcp monitor-process to fail with ErrIncompleteProcessTree based solely on whether the leader exited before or during the snapshot.
Suggested fix: Handle root exit during GetProcessTree consistently with root exit before GetProcessTree when options.group != nil and !errors.Is(treeErr, ErrProcessIdentityMismatch) (provided snapshotAll and buildProcessTree did not record other incomplete-tree errors).
| if stopErr != nil { | ||
| if process.IsProcessGoneErr(stopErr) { | ||
| if childGroup == nil && process.IsProcessGoneErr(stopErr) { | ||
| log.V(1).Info("Child process exited before cleanup completed", "Error", stopErr) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
childGroup == nil && process.IsProcessGoneErr(stopErr) (here at line 110 and at line 184) causes monitor-process to fail when the child group has already exited and its leader PID was reused.
On Unix, childGroup is non-nil for ordinary processes. When StopViaConsole(..., StopWithProcessGroup(childGroup)) runs after the child group has already exited, StopProcess returns nil if the leader PID is unused, and returns ErrProcessIdentityMismatch (which satisfies IsProcessGoneErr) if the entire group exited and the kernel reused the PID for an unrelated process (a PID cannot be reused on Unix while a process group with that PGID still exists).
Because childGroup != nil, monitor-process logs "Failed to stop child [service] process" and returns a non-zero exit error on that PID-reuse race, contradicting the comment at lines 97–101.
Suggested fix: Replace if childGroup == nil && process.IsProcessGoneErr(stopErr) with if process.IsProcessGoneErr(stopErr) at both line 110 and line 184.
There was a problem hiding this comment.
Related path that the guard change suggested here won't fix: the same PID-reuse race also surfaces in ProcessGroup.isRunning / Wait.
hasLiveLeader (process_group_unix.go:72) returns ErrProcessIdentityMismatch when the leader PID now belongs to a different process, and isRunning (process_group_unix.go:120) propagates it. So childGroup.Wait returns an error instead of "group is empty". In monitor-process that lands in the childWaitErr != nil branch: it logs "Error waiting for child process cleanup target; continuing to monitor owner", stops watching the group, and lingers until DCP exits. Then, per this thread, the cleanup fails with the same mismatch error and the process exits non-zero.
Given the kernel guarantee quoted above (a PID can't be reallocated while a group with that PGID has members), a leader-PID mismatch means the original group is already empty. isRunning can therefore return (false, nil) for that case, and signal can treat it as nothing to signal, instead of returning an error.
The probability is low (PID recycling within one poll interval). The cost is a leaked helper process plus an error exit, and it is a small fix to make together with the guard change suggested here.
| ws, shouldStopProcess := e.tryStartWaiting(group.leader, waitable, waitReasonStopping) | ||
| waitEndedCh := ws.waitEndedCh | ||
| if !shouldStopProcess && (opts&optIsResponsibleForStopping) == 0 { | ||
| select { | ||
| case <-ws.waitEndedCh: | ||
| // A completed wait for the leader does not mean its group has exited. | ||
| default: | ||
| return singleProcessStopResult{waitEndedCh: waitEndedCh}, group.wait(ctx, stopWaitPollInterval) | ||
| } | ||
| } | ||
| defer e.finishStopAttempt(ws) | ||
| if (opts & optWaitForStdio) == 0 { | ||
| waitEndedCh = makeClosedChan() | ||
| } |
There was a problem hiding this comment.
stopProcessGroup drops forceKillUsed, checks optWaitForStdio after the non-owner return, and loses stop-ownership serialization once the leader exits.
- In
stopProcessGroup(lines 218–226), thedefault:branch for a non-owning caller (!shouldStopProcess) returnssingleProcessStopResult{waitEndedCh: waitEndedCh}beforeif (opts & optWaitForStdio) == 0 { waitEndedCh = makeClosedChan() }(lines 228–230) and without settingforceKillUsed: e.forceKillWasUsed(ws)(unlikestopSingleProcessat lines 88–96). If the owning caller escalated toSIGKILL, a joining caller receivesforceKillUsed: falseand treats the root stop as graceful. - In
tryStartWaiting(pkg/process/os_executor.go:281-285), once the leader exits (!ws.waitEnded.IsZero()),tryStartWaitingreturnsws, falsewithout checking or settingwaitReasonStopping. InstopProcessGroup,case <-ws.waitEndedCh:then falls through for every concurrent caller, anddefer e.finishStopAttempt(ws)clearsws.reason &^= waitReasonStoppingwhen any caller finishes—even while another caller is still actively stopping the group.
Suggested fix: Move the optWaitForStdio check before the !shouldStopProcess branch, populate forceKillUsed: e.forceKillWasUsed(ws) after group.wait completes in the non-owning branch, and update tryStartWaiting so waitReasonStopping ownership is still tracked on ws even after ws.waitEnded is non-zero (removing the case <-ws.waitEndedCh: bypass).
Karol Zadora-Przylecki (karolz-ms)
left a comment
There was a problem hiding this comment.
🤖 Copilot Code Review
Two findings that are not covered by the existing threads (graceful-stage ordering for group stops, and the test-fixture convention). A third, related finding is posted as a reply on the existing PID-reuse thread. Both inline findings were verified on Linux (container, -race) and compared against the merge-base.
| if signalErr := group.signal(ctx, signal); signalErr != nil { | ||
| return result, signalErr | ||
| } | ||
| return result, group.wait(ctx, stopWaitPollInterval) |
There was a problem hiding this comment.
stopRootProcess (os_executor.go:579) now blocks in this group.wait until every group member exits or graceCtx expires. Descendants in other groups or sessions (e.g. setsid/daemonized ones) are only signalled afterwards by stopChildren. If the budget has expired, forceDescendants (os_executor.go:752) SIGKILLs them without ever sending SIGTERM. The doc comment on stopProcessTreeInternal (os_executor.go:540-546) still says descendants get the remainder of the graceful budget once the root exits. That is no longer true when a group is involved.
Repro (Linux container, -race): the root sh exits on SIGTERM, a same-group member ignores SIGTERM, and a setsid descendant traps SIGTERM and records it.
- Merge-base (
0fa5d16d): about 1 ms after the root exits the descendant receives SIGTERM and stops gracefully. The member is SIGKILLed when the 15 s budget expires. - This PR: SIGTERM to the group at t0, SIGKILL to the group at t0+15 s. The descendant is only ever sent SIGKILL, never SIGTERM.
No fault injection is needed. Any group member that is slow to exit, or ignores SIGTERM, triggers it.
Fix direction (keeps the "group first, then tree survivors" ordering): in the graceful step wait only for the leader's exit, as before. Then run the descendants' graceful stage while waiting for the group to empty under the same graceCtx. Escalate the group and the survivors only at the deadline. Please also add a regression test with a same-group member that ignores SIGTERM and an out-of-group descendant that must observe SIGTERM.
| inputRead, inputWrite, inputErr := os.Pipe() | ||
| require.NoError(t, inputErr) | ||
| t.Cleanup(func() { _ = inputRead.Close(); _ = inputWrite.Close() }) | ||
| cmd := exec.Command("sh", "-c", script) |
There was a problem hiding this comment.
startProcessGroupShell here (and TestStartProcessIsolatesUnixProcessGroup at line 44) drive the process-lifecycle fixtures with sh -c scripts (traps, &, $!, read) instead of the shared delay test tool. 7 of the 9 tests in this file depend on them.
I found no other host-side sh -c process fixtures in the repo's tests (the other sh uses run inside containers), and the contributor rules say to use delay for process-lifecycle subprocesses. Trap and read semantics also differ between dash (Linux) and macOS's bash 3.2, so these tests are shell-dependent.
Most of this is covered by existing delay flags:
TestStartProcessIsolatesUnixProcessGroupandTestProcessGroupValidatesIdentity:delay --delay=....TestStopRootOnlySkipsUnixGroupCleanupandTestProcessGroupOutlivesLeader:--child-spec=1 --couple-childrenplusStopRootOnly()to make the leader exit while a member survives, asinternal/dcpproc/process_group_unix_test.goalready does.
The remaining three (TestStopProcessKillsLateGroupMember, TestStopProcessKillsIgnoringGroupMemberAfterLeaderExit, TestStopProcessGroupRespectsCallerCancellation) need two small additions to test/delay. First, children that ignore SIGTERM while the leader does not (--ignore-sigterm is not propagated to children today). Second, spawning a child from the SIGTERM handler.
Unix process-tree cleanup snapshots descendants before teardown. Children created while shutdown handlers run can escape that snapshot and survive cleanup. This change also signals an isolated process group while retaining tree cleanup for descendants that moved into another group or session.
Ordinary Unix executor launches receive their own group unless explicit group/session setup is already present.
monitor-processfollows live group membership after the leader exits and cleans up on owner exit or watchdog interruption. Group cleanup uses the native process snapshots and identity validation from #278, sharing its bounded graceful and forced stop phases. Root-only stopping and Windows behavior remain unchanged.Dependency: This PR targets
dev/karolz/local-process-tree-impldirectly and depends on #278. It intentionally remains outside that PR's existing native stack; the existing stack is unchanged.Remaining limitation: Descendants that both leave the group and lose their ancestry before discovery remain outside best-effort cleanup coverage.