Don't kill processes when cleaning up PID files #6311

twz123 · 2025-08-20T15:54:07Z

Description

It's better to leave the decision what to do about a lingering process to the users. Try to terminate the process gracefully once and poll for process termination. If the process didn't terminate in time, bail out with an error.

Re-implement the Supervisor proc handle without the stdlib. The os/exec API is not a good fit for implementing the PID file cleanup. Instead, use a combination of dirfds to /proc/<pid> directories, along with pidfd_send_signal (if available) to implement a race-free process termination.

Don't misuse shouldKillProcess to check if a process has terminated or not. Rename it to isK0sManaged and add appropriate docs. Instead, introduce a purpose-built hasTerminated function.

Split up Linux and macOS process handling. There is no proc filesystem on macOS. Split things up and leave a minimal implementation in place to reflect this fact. This allows most of the unit tests to run on macOS while making it clear that proc stuff doesn't exist on macOS.

Also enable the bogus PID file test unconditionally. This is just about the PID file parsing, which doesn't require platform-specific APIs.

See:

Kill supervised process after stop timeout #6203 (comment)

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update

How Has This Been Tested?

Manual test
Auto test added

Checklist

juanluisvaladas

I'm fine with the first 5 commits in their current state but it's not clear to me why it's better to bail out instead of just terminating the process.

I can't think of a scenario where killing the process is a bad idea, I guess I'm missing something?

juanluisvaladas · 2025-08-22T14:17:05Z

pkg/supervisor/prochandle_linux.go

 }
+
+func isZombie(pid int) (bool, error) {
+	// https://man7.org/linux/man-pages/man5/proc_pid_stat.5.html


Nit: Maybe it's worth adding another comment with the relevant kernel source (I had to search it to understand the fields and all)

https://github.com/torvalds/linux/blob/v4.19/fs/proc/array.c#L152

I picked 4.19 so that it's not a very recent version and people reading it don't think it's a new feature but it doesn't really matter.

It's also worth noting that we already depend indirectly on github.com/prometheus/procfs, which handles this, but actually I prefer this.

Nit: Maybe it's worth adding another comment with the relevant kernel source (I had to search it to understand the fields and all)

TBH, I found the man pages to be very clear and approachable for this. They mention field names, their order, their sprintf format directive, and a comprehensive description. I tried to pick that up in the comments below: "pid" and "comm" are the first two fields, "state" is the third one. Maybe we can make those comments more clear?

we already depend indirectly on github.com/prometheus/procfs

Yeah, I think there's quite a few indirect dependencies dealing with procfs. I think containerd might have something into that direction, too. But I think they're all coming from a different angle to the problem: Prometheus is about monitoring, i.e. gathering data in a bunch, containerd, again, might be more concerned about security aspects when querying procfs data.

twz123 · 2025-08-22T19:38:47Z

I'm fine with the first 5 commits in their current state but it's not clear to me why it's better to bail out instead of just terminating the process.

I can't think of a scenario where killing the process is a bad idea, I guess I'm missing something?

During review of #6203 @ncopa raised concerns about killing processes. He would prefer to leave that decision to the users. There's always the chance to break something when killing processes, e.g., say, etcd while it's doing stuff on disk.

ncopa · 2025-08-27T12:38:28Z

pkg/supervisor/prochandle_unix.go

+		if errors.Is(err, os.ErrProcessDone) {
+			return nil, syscall.ESRCH
+		}
+	}


This test should be removed. The process can disappear right after the test is performed and before the return of function, so we need to handle the case of missing process late. No need to handle it twice.

ncopa · 2025-08-27T13:09:07Z

pkg/supervisor/prochandle_linux.go

+	}
+
+	// Do a last TOCTOU check: We now know the state of the process with the
+	// referenced PID. We go back to os.Process, once again, to ensure that


We actually don't know the state of the process at this point. The state may have changed after we did the test(s).
For that reason I think the entire hasTerminated function should be removed.

ncopa · 2025-08-27T13:38:36Z

Don't misuse shouldKillProcess to check if a process has terminated or not. Rename it to isK0sManaged and add appropriate docs. Instead, introduce a purpose-built hasTerminated function.

This does not work. We can not rely on a hasTerminated function because the process state can change at any time. The process can disappear while we check if it is k0s managed. I also think that the isZombie is meaningless, as the process state can change after the test is done.

ncopa · 2025-08-27T14:09:19Z

The os package uses ErrProcessDone ("os: process already finished"), not syscall.ESRCH ("no such process").

Good catch! Seems to be since go 1.4.

It models the /proc mount, particularly the /proc/<pid> directories and their contents. It focuses on the requirements of the supervisor. Signed-off-by: Tom Wieczorek <[email protected]>

The Dir newtype around os.File provides methods for opening, reading, statting and listing files or directories relative to a dirfd file descriptor. Provides robust handling of directory relative operations, maintaining directory access consistency across rename/move operations, potentially mitigating symbolic link attacks. Signed-off-by: Tom Wieczorek <[email protected]>

The os package uses ErrProcessDone ("os: process already finished"), not syscall.ESRCH ("no such process"). Reflect that usage in the procHandle implementation. Make the openPID function actually check for the existence of the process, and in that case, return an ESRCH. This fixes a bug in which errors returned from procHandle's requestGracefulShutdown and kill methods were checked against ESRCH, where they should have been checked against ErrProcessDone. Signed-off-by: Tom Wieczorek <[email protected]>

This is in preparation for disabling the code paths on macOS that rely on Linux-only proc filesystem parts. Signed-off-by: Tom Wieczorek <[email protected]>

There is no proc filesystem on macOS. Split things up and leave a minimal implementation in place to reflect this fact. This allows most of the unit tests to run on macOS while making it clear that proc stuff doesn't exist on macOS. Also enable the bogus PID file test unconditionally. This is just about the PID file parsing, which doesn't require platform-specific APIs. Signed-off-by: Tom Wieczorek <[email protected]>

Also adjust logging a bit. Signed-off-by: Tom Wieczorek <[email protected]>

Don't misuse shouldKillProcess to check if a process has terminated or not. Rename it to isK0sManaged and add appropriate docs. Instead, introduce a purpose-built hasTerminated function. This better reflects what this is about. Signed-off-by: Tom Wieczorek <[email protected]>

This allows for using the pidfd_send_signal syscall. Signed-off-by: Tom Wieczorek <[email protected]>

It's better to leave the decision what to do about a lingering process to the users. Try to terminate the process gracefully once and poll for process termination. If the process didn't terminate in time, bail out with an error. Re-implement the Supervisor proc handle without the stdlib. The os/exec API is not a good fit for implementing the PID file cleanup. Instead, use a combination of dirfds to /proc/<pid> directories, along with pidfd_send_signal (if available) to implement a race-free process termination. Signed-off-by: Tom Wieczorek <[email protected]>

ncopa

LGTM

twz123 added the enhancement New feature or request label Aug 20, 2025

This was referenced Aug 20, 2025

Kill supervised process after stop timeout #6203

Closed

Bail out after stop timeout when stopping supervised processes #6312

Draft

twz123 marked this pull request as ready for review August 20, 2025 17:45

twz123 requested review from a team as code owners August 20, 2025 17:45

twz123 requested review from kke and makhov August 20, 2025 17:45

twz123 force-pushed the no-kill-on-pid-file-cleanup branch from c983be6 to 8d2dd2b Compare August 21, 2025 07:03

juanluisvaladas reviewed Aug 22, 2025

View reviewed changes

ncopa reviewed Aug 27, 2025

View reviewed changes

twz123 force-pushed the no-kill-on-pid-file-cleanup branch from 8d2dd2b to 24422a4 Compare September 19, 2025 16:12

twz123 added this to the 1.34 milestone Sep 19, 2025

twz123 force-pushed the no-kill-on-pid-file-cleanup branch 2 times, most recently from 3b370d3 to 6e5d7a0 Compare September 19, 2025 16:21

twz123 added 9 commits September 19, 2025 18:43

Introduce the procfs package

e286115

It models the /proc mount, particularly the /proc/<pid> directories and their contents. It focuses on the requirements of the supervisor. Signed-off-by: Tom Wieczorek <[email protected]>

Rename prochandle_unix.go to prochandle_linux.go

8b0d00c

This is in preparation for disabling the code paths on macOS that rely on Linux-only proc filesystem parts. Signed-off-by: Tom Wieczorek <[email protected]>

Rename graceful shutdown to graceful termination

36f9051

Also adjust logging a bit. Signed-off-by: Tom Wieczorek <[email protected]>

Add support for opening PID directories backed by dirfds

fb67a64

This allows for using the pidfd_send_signal syscall. Signed-off-by: Tom Wieczorek <[email protected]>

twz123 force-pushed the no-kill-on-pid-file-cleanup branch from 6e5d7a0 to 4b1861c Compare September 19, 2025 16:44

ncopa approved these changes Sep 22, 2025

View reviewed changes

twz123 merged commit 5e87c30 into k0sproject:main Sep 22, 2025
189 of 190 checks passed

twz123 deleted the no-kill-on-pid-file-cleanup branch September 22, 2025 15:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Don't kill processes when cleaning up PID files #6311

Don't kill processes when cleaning up PID files #6311

Uh oh!

twz123 commented Aug 20, 2025 •

edited

Loading

Uh oh!

juanluisvaladas left a comment

Uh oh!

juanluisvaladas Aug 22, 2025

Uh oh!

juanluisvaladas Aug 22, 2025

Uh oh!

twz123 Aug 27, 2025

Uh oh!

twz123 Aug 27, 2025

Uh oh!

twz123 commented Aug 22, 2025

Uh oh!

ncopa Aug 27, 2025

Uh oh!

ncopa Aug 27, 2025

Uh oh!

ncopa commented Aug 27, 2025

Uh oh!

ncopa commented Aug 27, 2025

Uh oh!

ncopa left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Don't kill processes when cleaning up PID files #6311

Don't kill processes when cleaning up PID files #6311

Uh oh!

Conversation

twz123 commented Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Checklist

Uh oh!

juanluisvaladas left a comment

Choose a reason for hiding this comment

Uh oh!

juanluisvaladas Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

juanluisvaladas Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

twz123 Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

twz123 Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

twz123 commented Aug 22, 2025

Uh oh!

ncopa Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

ncopa Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

ncopa commented Aug 27, 2025

Uh oh!

ncopa commented Aug 27, 2025

Uh oh!

ncopa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

twz123 commented Aug 20, 2025 •

edited

Loading