Introduce process file descriptor (pidfd) based process monitoring for Linux #125

iCharlesHu · 2025-07-16T22:01:11Z

The current process monitoring code for Linux has a flaw that makes it susceptible to infinite hangs under specific conditions:

The parent process uses any other method (other than Subprocess itself) to spawn new processes in addition to spawning with Subprocess.
The parent process fails to properly reap the non-Subprocess-spawned process, leaving it as a zombie in the process table.

This is because currently, we rely on running waitid() with P_ALL and WNOWAIT in an infinite loop to detect possible child process state transitions. However, we don’t reap the child process (by specifying WNOWAIT) unless we (Subprocess) actually spawned the process.

Here’s a simplified pseudo-code to illustrate the issue:

while true {
    var siginfo = siginfo_t()
    // We’re not reaping the child process
    if waitid(P_ALL, id_t(0), &siginfo, WEXITED | WNOWAIT) == 0 {
        guard let c = savedContinuation else {
            // If there’s no saved continuation, we didn’t spawn the process
            // In this case, we don’t reap the child process
            continue
        }

        siginfo = siginfo_t()
        waitid(P_PID, numericCast(pid), &siginfo, WEXITED) // We’re actually reaping the child process
    }
}

With this setup, if there are zombie children in the process table without reaping, waitid(P_ALL) will repeatedly return the same (non-Subprocess-spawned) PID with every call, causing an infinite loop.

You can observe this behavior with the following sample code:

let arguments = "\"\""

let pid = arguments.withCString { args in
    var pid: pid_t = -1
    let status = posix_spawn(&pid, "/bin/echo", nil, nil, [strdup(args)] + [nil], environ)
    guard status == 0 else {
        fatalError("posix_spawn: \(status), errno: \(errno)")
    }
    return pid
}
print("echo pid: \(pid)")

let result = try await Subprocess.run(
    .path("/bin/cat"),
    arguments: ["Package.swift"],
    output: .string(limit: .max, encoding: UTF8.self),
    error: .discarded
)
print("cat finished: \(result.terminationStatus)")
print("cat output: \(result.standardOutput ?? "")")

After running this example, you’ll notice that the parent process seems to be stuck, and the “cat finished” message is never printed. This is because the parent process never calls waitid on the echo call, leaving it in the process table. Consequently, the monitor thread runs in an infinite loop.

While some may argue that this is not a bug in Subprocess, but rather an issue with the parent code, since the POSIX standard mandates that the process spawning child process must reap the child process via waitid. However, Subprocess should still not hang due to someone else’s bug.

To resolve this issue, switch to a Linux-specific process monitoring method by creating and observing the process file descriptor (pidfd) using epoll. This approach is similar to the epoll implementation introduced in #117, with the only difference being that we’re polling pidfd instead of a regular file descriptor.

As part of this change, I also unified the “process handle” design to make it easier to expose process handles to clients later (after the 1.0 release, as requested by #101). We chose to use ProcessIdentifier to host platform-specific process file descriptors and process handles because it perfectly aligns with the original use case. To ensure flexibility, we opted for a concrete ProcessIdentifier type instead of just a number, allowing us to add more information if necessary.

…r Linux

iCharlesHu · 2025-07-16T22:01:38Z

Resolves #111

iCharlesHu · 2025-07-16T22:27:25Z

waitid with P_PIDFD was introduced in Linux kernel 5.4, which focal should have. I'm looking into what's missing

cthielen · 2025-07-16T22:10:56Z

Sources/Subprocess/Platforms/Subprocess+Linux.swift

+// MARK:  - ProcesIdentifier
+
+/// A platform independent identifier for a Subprocess.
+public struct ProcessIdentifier: Sendable, Hashable {


Is it worthwhile to make this a protocol given the repetition?

Could you elaborate on how would we use this protocol? The reason it's repeated is because on different platforms we have different sets of fields. I don't think having a protocol would solve this problem because we'd still need to offer different concrete types.

File this one under premature optimization but a protocol might help if the shared, non-platform-specific code started relying on the existence of methods or attributes of ProcessIdentifier. I took a look and don't see any at the moment beyond description.

If there are more expectations requiring the various ProcessIdentifier definitions to stay in sync, it might be helpful to introduce a protocol, not because any given platform needs more than one concrete type, but because the protocol will serve as a contract to keep the implementations in sync with the expectations of the shared code.

cthielen · 2025-07-16T22:12:25Z

Sources/Subprocess/Platforms/Subprocess+Linux.swift

+        _processMonitorState.withLock { state in
+            switch state {
+            case .notStarted:
+                continuation.resume(throwing: SubprocessError(


I've been told resuming a continuation from within a lock can be troublesome.

Could you return the continuation from withLock and then resume?

This is definitely the case for withTaskCancellationHandler (see https://developer.apple.com/documentation/swift/withtaskcancellationhandler(operation:oncancel:isolation:)#Cancellation-handlers-and-locks); I'm not sure if that's true for continuations.

I general one shouldn’t do any arbitrary outcalls while holding a lock. We have already seen continuation resumptions under locks lead to deadlocks since the runtime itself is holding locks and doing outcalls such as calling cancellation handlers.

I also don’t see a reason why we should do it here. It doesn’t prevent any potential races from what I can see.

cthielen · 2025-07-16T22:15:44Z

Sources/Subprocess/Platforms/Subprocess+Linux.swift

+                return
+            }
+            for continuation in storage.continuations.values {
+                continuation.resume(throwing: error)


Same note here about resuming a continuation from a lock

cthielen · 2025-07-16T22:18:07Z

Sources/Subprocess/Platforms/Subprocess+Linux.swift

+        }
+    }
+
+    guard let state = state else {


Suggested change

guard let state = state else {

guard let state else {

cthielen · 2025-07-16T22:18:45Z

Sources/Subprocess/Platforms/Subprocess+Linux.swift

+
+    var one: UInt64 = 1
+    // Wake up the thread for shutdown
+    _ = _SubprocessCShims.write(state.shutdownFileDescriptor, &one, MemoryLayout<UInt64>.stride)


Suggested change

_ = _SubprocessCShims.write(state.shutdownFileDescriptor, &one, MemoryLayout<UInt64>.stride)

_ = _SubprocessCShims.write(state.shutdownFileDescriptor, &one, MemoryLayout<UInt64>.size)

cthielen · 2025-07-16T22:24:32Z

Sources/Subprocess/Platforms/Subprocess+Linux.swift

+                    return
+                }
+                for continuation in storage.continuations.values {
+                    continuation.resume(throwing: error)


Same comment about locks + continuations

cthielen · 2025-07-16T22:24:50Z

Sources/Subprocess/Platforms/Subprocess+Linux.swift

+            }
+        }
+
+        let unmanaged = Unmanaged<MonitorThreadContext>.fromOpaque(args!)


Is unmanaged used again?

cthielen · 2025-07-16T22:25:52Z

Sources/Subprocess/Platforms/Subprocess+Linux.swift

+                    }
+                    switch terminationStatus {
+                    case .success(let value):
+                        continuation.resume(returning: value)


Lock + continuation

Can you clarify that comment?

It's a reference to this earlier comment: #125 (comment)

There are a few places in this PR where a continuation is resumed from within a lock.

cthielen · 2025-07-16T22:27:03Z

Sources/Subprocess/Platforms/Subprocess+Linux.swift

+        }
+
+        return nil
+    },threadContext.toOpaque())


Would it be helpful if the third argument to pthread_create() were its own function? The threadContext argument feels disconnected from the call site many lines up.

cthielen · 2025-07-16T22:28:17Z

Sources/Subprocess/Platforms/Subprocess+Windows.swift

@@ -681,12 +681,26 @@ extension Environment {
 // MARK: - ProcessIdentifier

 /// A platform independent identifier for a subprocess.
-public struct ProcessIdentifier: Sendable, Hashable, Codable {
+public struct ProcessIdentifier: Sendable, Hashable {


Same comment here about whether a protocol would help with ProcessIdentifier

jakepetroules · 2025-07-16T22:55:25Z

Sources/Subprocess/Platforms/Subprocess+Linux.swift

+        _processMonitorState.withLock { state in
+            switch state {
+            case .notStarted:
+                continuation.resume(throwing: SubprocessError(


This is definitely the case for withTaskCancellationHandler (see https://developer.apple.com/documentation/swift/withtaskcancellationhandler(operation:oncancel:isolation:)#Cancellation-handlers-and-locks); I'm not sure if that's true for continuations.

jakepetroules · 2025-07-16T23:11:11Z

Sources/Subprocess/Execution.swift

@@ -35,16 +35,13 @@ public struct Execution: Sendable {
    public let processIdentifier: ProcessIdentifier

    #if os(Windows)
-    internal nonisolated(unsafe) let processInformation: PROCESS_INFORMATION
    internal let consoleBehavior: PlatformOptions.ConsoleBehavior


nit: unrelated, but you could delete consoleBehavior as well as nothing actually uses it.

jakepetroules · 2025-07-16T23:12:21Z

Sources/Subprocess/Platforms/Subprocess+Linux.swift

@@ -189,6 +194,30 @@ extension Configuration {
    }
 }

+// MARK:  - ProcesIdentifier


nit: spelling: "Proces" -> "Process"

jakepetroules · 2025-07-16T23:15:22Z

Sources/Subprocess/Platforms/Subprocess+Linux.swift

+public struct ProcessIdentifier: Sendable, Hashable {
+    /// The platform specific process identifier value
+    public let value: pid_t
+    internal let processFileDescriptor: PlatformFileDescriptor


waitid with P_PIDFD was introduced in Linux kernel 5.4, which focal should have. I'm looking into what's missing

@iCharlesHu One thing I'm concerned about this with change is that unfortunately I don't think we can guarantee that the Linux kernel is version 5.4 or later on all Linux distributions officially supported by Swift.

(Also note that PIDFD_NONBLOCK, if we want to use it, is Linux kernel 5.10 or later)

Containers aren't particularly useful for testing this because they're not going to be using the same kernel as the actual OS distribution. Also, the Swift project is dropping support for Focal in Swift 6.2. Are you planning to keep support for Swift 6.1 in SwiftSubprocess?

Here's the minimum kernel versions associated with each Linux distribution currently officially supported by the Swift project and that I expect to be supported for the Swift 6.2 release:

Amazon Linux 2 (kernel 4.14): https://docs.aws.amazon.com/linux/al2/ug/aml2-kernel.html

Debian 12 (kernel 6.1): https://www.debian.org/News/2023/20230610.en.html

Fedora 39 (kernel 6.5): https://en.wikipedia.org/wiki/Fedora_Linux_release_history, also that version is EoL anyways

RHEL UBI9 (kernel 5.14): https://docs.redhat.com/en/documentation/red_hat_enterprise_linux/9/html/9.0_release_notes/new-features#enhancement_kernel

Ubuntu 20.04 (kernel 5.4): https://en.wikipedia.org/wiki/Ubuntu_version_history#Table_of_versions

Ubuntu 22.04 (kernel 5.15 or 5.17): https://en.wikipedia.org/wiki/Ubuntu_version_history#Table_of_versions

Ubuntu 24.04 (kernel 6.8): https://en.wikipedia.org/wiki/Ubuntu_version_history#Table_of_versions

Thus, until the Swift project moves from Amazon Linux 2 to Amazon Linux 2023 (6.1 kernel), I think we have to retain the original process termination path for compatibility with that distribution, as a fallback path. And we'll need that implementation for OpenBSD and other platforms anyways.

There's also Android. As you may know, the Android ecosystem is often running quite old OS versions compared to iOS, and I think only as of Android 13 (2022) does the OS guarantee kernel 5.4 or later. I'm not sure which minimum Android version we're planning to target in the Swift project, but that would be a good question for @finagolfin or @marcprux or someone else from the Android working group.

Perhaps you could do this by making processFileDescriptor an optional property and falling back to the pid based paths when it is nil?

@jakepetroules thanks for the analysis. I did not know about Ubuntu's kernel release schedule I thought since 20.04 has 5.4 the later ones must have newer ones...

I do have a backup implementation which is to use signalfd. I initially opted for pidfd because it's more modern and precise. I guess we'll have to keep both

Nice, I wasn't aware of signalfd (just read up on it). Still, it's Linux specific, so OpenBSD and some other platforms would probably have to continue to rely on the waitid-loop implementation.

I'll try natively compiling this repo and then this pull on Android and running the tests, will let you know what I find.

jakepetroules · 2025-07-16T23:18:37Z

Sources/Subprocess/Platforms/Subprocess+Linux.swift

+                if rc != 0 {
+                    let error = SubprocessError(
+                        code: .init(.failedToMonitorProcess),
+                        underlyingError: .init(rawValue: errno)


errno is not safe across Swift function calls, so technically you need to save it to a local variable immediately after calling epoll_ctl since the initializer you pass to code could change it. Worth an audit across the entire codebase as well.

jakepetroules · 2025-07-16T23:25:25Z

Sources/Subprocess/Platforms/Subprocess+Linux.swift

+                -1
+            )
+            if eventCount < 0 {
+                if errno == EINTR || errno == EAGAIN {


suggestion: it might be worth introducing a helper function to handle EINTR/EAGAIN since it's such a common pattern throughout this codebase; see https://github.com/apple/swift-system/blob/6ee9a58c36ad98f4bd917a64d153dd211512e65d/Sources/System/Util.swift#L27 for example.

jakepetroules · 2025-07-16T23:28:02Z

Sources/Subprocess/Platforms/Subprocess+Unix.swift

-                underlyingError: .init(rawValue: errno)
-            )
+
+        #if os(Linux)


I think you may want || os(Android) here too, I'm not sure if os(Linux) applies there.

grynspan · 2025-07-17T22:17:56Z

This is not a bug in the existing implementation. It is a bug in the POSIX specification (and a bug in the program.)

jakepetroules · 2025-07-17T23:15:43Z

Sources/_SubprocessCShims/process_shims.c

@@ -664,6 +504,10 @@ int _subprocess_fork_exec(
        // If we reached this point, something went wrong
        write_error_and_exit;
    } else {
+        int _pidfd = _pidfd_open(childPid);


Could we use clone + CLONE_PIDFD (Linux 5.2) instead of fork + pidfd_open? Like FreeBSD's pdfork, this avoids races since combining the latter two functions is not atomic.

jakepetroules · 2025-07-17T23:20:27Z

This is not a bug in the existing implementation. It is a bug in the POSIX specification (and a bug in the program.)

That may be true, but the implementation Charles is proposing here is more defensive against other parts of the program misbehaving, which seems like a good thing.

Including scenarios where zombies are being reaped correctly throughout the entire program, but maybe the body of one particular Subprocess.run call is stuck or otherwise taking an incredibly long time -- this implementation prevents that one process from holding up everything else.

grynspan · 2025-07-18T21:02:01Z

Sources/_SubprocessCShims/process_shims.c

-    //  - musl 1.1.24 (October 2019)
-    //  - FreeBSD 13.1 (May 2022)
-    //  - Android 14 (API level 34) (October 2023)
-    return posix_spawn_file_actions_addchdir_np(file_actions, path);


This will emit a deprecation warning as of *OS 26 since the standardized version has been added.

grynspan · 2025-07-18T21:03:36Z

Sources/Subprocess/Platforms/Subprocess+Darwin.swift

+// MARK: - ProcessIdentifier
+
+/// A platform independent identifier for a Subprocess.
+public struct ProcessIdentifier: Sendable, Hashable {


Could you make this type move-only and incorporate the close() operation into deinit?

Closing might involve closing FDs right? Which might be an asynchronous and throwing operation.

That close() can fail at all is an unfortunate weird corner of POSIX that I personally tend to ignore, because a failure in close() other than EINTR/EAGAIN is basically non-recoverable. What are you even supposed to do? What can a user do to fix the problem? Generally nothing.

So I just about always just drop a close() failure on the floor. </hottake>

(As for asynchronous, it's a blocking operation in userland but it can't fail to make forward progress in this case because there's no network I/O involved unless we're doing something really wonky.)

We've had this discussion in another place (here?). We ended up not calling close, but asserting that close has already been called in deinit.

But it does seem like a design people are going to reach for repeatedly. I wonder if we can put our thought process down somewhere.

Introduce process file descriptor (pidfd) based process monitoring fo…

9b99e41

…r Linux

iCharlesHu requested review from cthielen, parkera and itingliu July 16, 2025 22:01

iCharlesHu mentioned this pull request Jul 16, 2025

OpenBSD / FreeBSD Support for Subprocess #115

Open

cthielen reviewed Jul 16, 2025

View reviewed changes

jakepetroules reviewed Jul 16, 2025

View reviewed changes

jakepetroules reviewed Jul 17, 2025

View reviewed changes

grynspan reviewed Jul 18, 2025

View reviewed changes

	_ = _SubprocessCShims.write(state.shutdownFileDescriptor, &one, MemoryLayout<UInt64>.stride)
	_ = _SubprocessCShims.write(state.shutdownFileDescriptor, &one, MemoryLayout<UInt64>.size)

Introduce process file descriptor (pidfd) based process monitoring for Linux #125

Are you sure you want to change the base?

Introduce process file descriptor (pidfd) based process monitoring for Linux #125

Conversation

iCharlesHu commented Jul 16, 2025

Uh oh!

iCharlesHu commented Jul 16, 2025

Uh oh!

iCharlesHu commented Jul 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FranzBusch Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grynspan commented Jul 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakepetroules commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

FranzBusch Jul 17, 2025 •

edited

Loading

jakepetroules commented Jul 17, 2025 •

edited

Loading