5199 process termination by aleks-f · Pull Request #5203 · pocoproject/poco

aleks-f · 2026-02-11T22:33:05Z

No description provided.

Note: Types.h change already present in upstream poco. Only TestResult.cpp needed porting.

…sed (Windows) #5198

… NamedEvent race #5199

…ect #5201 On Windows, TerminateProcess only kills the target process — grandchildren survive as orphans. PROCESS_KILL_TREE creates a Job Object with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE so the entire process tree is torn down when ProcessRunner is stopped or destroyed.

…s groups #5201 On fork, the child calls setpgid(0, 0) to create a new process group. On stop, kill(-pid, sig) signals the entire group instead of just the leader. Gated on PROCESS_KILL_TREE to preserve default behavior.

Copilot

Pull request overview

This PR improves process termination behavior across platforms, primarily by adding an option to terminate an entire spawned process tree and by addressing a Windows termination-event lifetime race.

Changes:

Add PROCESS_KILL_TREE process option and implement tree-termination support (Windows Job Objects, Unix process groups).
Adjust Windows ServerApplication::hasConsole() detection to avoid misclassifying interactive launches when stdout/stderr are closed.
Refine CppUnit cxxabi.h availability detection to avoid _MSC_VER toolchains (clang-cl in particular).

Reviewed changes

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

Show a summary per file

File	Description
Util/src/ServerApplication.cpp	Expands console detection (console window + stdin/out/err handles) to avoid false “no console” when std handles are closed.
Foundation/src/Process_UNIX.cpp	Adds `setpgid(0, 0)` for `PROCESS_KILL_TREE` to enable process-group based signaling.
Foundation/src/ProcessRunner.cpp	Implements Windows Job Object assignment for kill-tree, and Unix group-based polling/kill logic; adjusts Windows termination signaling to keep the event alive.
Foundation/include/Poco/ProcessRunner.h	Stores a Windows Job Object handle (`_hJob`) when `PROCESS_KILL_TREE` is enabled.
Foundation/include/Poco/ProcessOptions.h	Introduces `PROCESS_KILL_TREE` and documents intended platform behavior.
CppUnit/src/TestResult.cpp	Updates `HAVE_CXXABI_H` detection to exclude MSVC toolchains.

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

Foundation/include/Poco/ProcessOptions.h

Foundation/src/ProcessRunner.cpp

Foundation/src/Process_UNIX.cpp

Util/src/ServerApplication.cpp

doc update Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

…ject/poco into 5199-process-termination

matejk · 2026-02-12T08:10:37Z

Related ticket #5201 is actually a new feature that adds functionality and extends the API.

Shall we add new feature to a bugfix release?

aleks-f · 2026-02-12T08:16:26Z

Related ticket #5201 is actually a new feature that adds functionality and extends the API.

Shall we add new feature to a bugfix release?

It doesn't break anything, it is just one more bit in options, off by default. strictly speaking, we shouldn't, but we need it and I don't want to wait for next release

matejk · 2026-02-12T09:19:32Z

The name of the PR does not reflect the actual changes: it fixes multiple issies.

matejk

PR Review: Process Termination Fixes

CI is green (all 88 checks pass, including ASAN/TSAN/UBSAN). The clang-cl fix in TestResult.cpp and the QNX SPAWN_SETGROUP/SPAWN_NEWPGROUP path look correct and clean.

Critical

_hJob handle leaked on normal process exit — The Job Object is created in run() but only cleaned up in stop() inside the _pPH guard. If the child exits on its own, run() completes and nulls _pPH, so stop() skips the cleanup block. Handle leaks on every natural-exit cycle.
Stale errno in kill(-pid, 0) polling loop — Thread::sleep(10) between iterations can modify errno. If kill() returns -1/ESRCH (group gone) but errno was clobbered to EPERM by sleep internals, the || errno == EPERM condition passes and the loop spins until timeout.
Unchecked setpgid(0, 0) return — If it fails, the child continues in the parent's process group. Later kill(-pid, SIGINT) targets a nonexistent group (ESRCH), the polling loop exits immediately, and stop() thinks the tree is dead.

Important

start() catch block doesn't close _hJob — Handle leak on startup failure.
Windows stop() doesn't wait for child tree — Only waits for the main PID; closes the Job Object but returns immediately. Asymmetric with Unix behavior.
kill(-pid, SIGINT) return unchecked — EPERM on the initial signal gets masked as a timeout.
fork/setpgid race — Parent could call kill(-pid, ...) before child reaches setpgid(0, 0). Standard fix: also call setpgid(pid, pid) in parent after fork.

Minor

No tests for PROCESS_KILL_TREE — The existing ProcessRunnerTest doesn't exercise the new option.
Silent degradation on Windows — If Job Object creation fails, PROCESS_KILL_TREE is silently ignored with no log/warning.
hasConsole() may over-detect — Piped handles (e.g. from a service manager) would cause a false positive. Consider GetFileType(h) == FILE_TYPE_CHAR to confirm it's a console device.

See inline comments for details.

Foundation/src/ProcessRunner.cpp

Foundation/src/Process_UNIX.cpp

Util/src/ServerApplication.cpp

matejk

PR Review — Round 2 (post `3dc180d2`)

All 5 issues from the first review have been addressed ✅ — HandleGuard RAII, stale errno, kill return check, setpgid failure handling, and hasConsole GetFileType. Good work.

Remaining / new issues below. See inline comments for details.

Critical

Job Object setup fails silently — KILL_TREE degrades to no-op — The entire CreateJobObjectW → SetInformationJobObject → OpenProcess → AssignProcessToJobObject chain can fail at any stage (most commonly AssignProcessToJobObject returns ERROR_ACCESS_DENIED when the child is already in a non-breakaway job — common in CI, containers, and service managers). Every failure path just calls _hJob.reset() and continues. No log, no warning, no exception. The user explicitly opts in to tree-kill but the feature silently becomes a no-op.

Important

start() catch block: Process::kill() can throw, corrupting state — In the catch-all at lines 421-433, Process::kill(p) is called unguarded. It can throw NotFoundException/SystemException (see Process_WIN32U.cpp/Process_UNIX.cpp). If it does, _t.join() and _started.store(false) are never reached. The ProcessRunner is stuck in a state where _started==true but the thread is orphaned — subsequent start() always throws InvalidAccessException.
Windows stop() sends NamedEvent unconditionally when KILL_TREE is active — asymmetric with Unix — On Unix, stop() branches on PROCESS_KILL_TREE to signal the group vs individual process. On Windows, the NamedEvent graceful signal is always sent (regardless of KILL_TREE), and tree termination relies on closing _hJob later. This means: (a) the NamedEvent triggers graceful shutdown in the leader, which may start cleanup — then _hJob close forcefully terminates the tree mid-cleanup; (b) the NamedEvent only works for Poco ServerApplication-based processes — non-Poco children get no graceful signal at all.
_exit(73) magic number is opaque to the parent — If setpgid(0,0) fails in the child, exit code 73 is returned. The parent sees return=73 in the error string with no indication this was an infrastructure failure vs the application itself returning 73. The actual errno from setpgid is lost.
Documentation/code mismatch in ProcessOptions.h — The doc says the termination request is sent to the main process, but the code sends kill(-pid, SIGINT) to the entire process group. The doc should match the actual group-wide SIGINT behavior.

Other

No tests for PROCESS_KILL_TREE — ProcessRunnerTest has zero test cases exercising the new option. No coverage for Job Object creation, process group setup, or tree termination on stop.
HandleGuard design concerns — Public handle member bypasses the RAII invariant (code already accesses _hJob.handle directly). reset() has no self-assignment guard (reset(handle) would close then lose the resource). Inconsistent with ProcessHandleImpl in the same codebase which uses a private _hProcess + process() accessor.

Foundation/src/ProcessRunner.cpp

Foundation/src/Process_UNIX.cpp

Foundation/include/Poco/ProcessOptions.h

Foundation/include/Poco/ProcessRunner.h

Copilot

Pull request overview

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

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

Copilot · 2026-02-13T08:44:17Z

Foundation/src/ProcessRunner.cpp

+			}
+			else
+			{
+				Poco::format(jobErr, "CreateJobObjectW: %s", Error::getMessage(Error::last()));


Same as the previous comment - verify the correct signature for Poco::format. The typical pattern is either format(output, formatString, args...) or output = format(formatString, args...).

Copilot · 2026-02-13T08:44:18Z

Foundation/src/ProcessRunner.cpp

+				{
+					if (!AssignProcessToJobObject(_hJob.handle(), hProc))
+					{
+						Poco::format(jobErr, "AssignProcessToJobObject: %s", Error::getMessage(Error::last()));


Same Poco::format signature issue as above. Ensure consistent usage throughout this code block.

Foundation/src/ProcessRunner.cpp

aleks-f and others added 5 commits February 11, 2026 16:29

fix(build): clang-cl support #5195

af76d1a

Note: Types.h change already present in upstream poco. Only TestResult.cpp needed porting.

fix(ServerApplication): hasConsole() returns false when stdout is clo…

436622b

…sed (Windows) #5198

fix(ProcesssRunner): stop() termination signal lost on Windows due to…

20806d9

… NamedEvent race #5199

aleks-f requested a review from Copilot February 11, 2026 22:33

Copilot started reviewing on behalf of aleks-f February 11, 2026 22:56 View session

Copilot AI reviewed Feb 11, 2026

View reviewed changes

aleks-f and others added 7 commits February 12, 2026 00:05

Update Foundation/include/Poco/ProcessOptions.h

2c62b7f

doc update Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

fix(ProcesRunner): terminate group, not only parent #5199

9fb9103

Merge branch '5199-process-termination' of https://github.com/pocopro…

601a130

…ject/poco into 5199-process-termination

fix(ProcesRunner): account for EPERM on kill #5199

a2e0b7e

fix(ProcesRunner): handle Job Object return values #5199

c6dea11

fix(Proces_UNIX): fix QNX kill #5201

1fbefd5

fix(ServerApplication): NULL -> nullptr #5198

858ca0c

aleks-f requested a review from matejk February 12, 2026 06:52

matejk reviewed Feb 12, 2026

View reviewed changes

aleks-f added 5 commits February 12, 2026 06:42

fix(ProcessRunenr): RAII wrapper for JobObject #5201

2ec96b6

fix(ProcessRunenr): stale errno #5201

37b1300

fix(ProcessRunenr): check kill return value #5201

b28e448

fix(ProcessRunenr): exit after fork on setpgi failure #5201

27e05ee

fix(ServerApplication): check console type #198

3dc180d

matejk reviewed Feb 12, 2026

View reviewed changes

aleks-f added 6 commits February 12, 2026 08:38

feat(ProcessRunenr): add kill tests #5201

85907b4

doc(ProcessRunner): PROCESS_KILL_TREE limitations #5201

3097513

fix(ProcessRunner): child exit codes #5201

2d59024

fix(ProcessRunner): HandleGuard encapsulation #5201

6a65459

fix(ci): mongodb action

7091bac

fix(ProcessRunner): capture job error #5201

be9b9f1

aleks-f requested a review from Copilot February 13, 2026 08:39

Copilot AI reviewed Feb 13, 2026

View reviewed changes

Copilot started reviewing on behalf of aleks-f February 13, 2026 08:47 View session

chore(test): add check for no error [skip_ci] #5201

ecb573e

matejk merged commit e377bda into main Feb 13, 2026
100 checks passed

matejk deleted the 5199-process-termination branch February 13, 2026 13:47

matejk pushed a commit that referenced this pull request Feb 14, 2026

5199 process termination (#5203)

7071b2d

Conversation

aleks-f commented Feb 11, 2026

Uh oh!

Copilot AI left a comment • edited by aleks-f Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matejk commented Feb 12, 2026

Uh oh!

aleks-f commented Feb 12, 2026

Uh oh!

matejk commented Feb 12, 2026

Uh oh!

matejk left a comment

Choose a reason for hiding this comment

PR Review: Process Termination Fixes

Critical

Important

Minor

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matejk left a comment

Choose a reason for hiding this comment

PR Review — Round 2 (post 3dc180d2)

Critical

Important

Other

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Copilot AI left a comment •

edited by aleks-f

Loading

PR Review — Round 2 (post `3dc180d2`)