Make server teardown safe against pid reuse and refuse to erase unreadable stores

.changeset/process-safety-teardown.md

@@ -0,0 +1,9 @@
+---
+"devctl": patch
+---
+
+Server teardown is now safe against pid reuse. Every signal devctl sends while stopping or cleaning up after a server is checked against the identity it recorded for that process while it was alive, so a pid the kernel has since handed to an unrelated process is never signaled. The deliberate-stop path and the crash-cleanup path now share one revalidated sweep, and the crash path, whose process is already gone, no longer directs a signal at its former process group at all.
+
+`devctl stop` now also cleans up a descendant that escaped into the background. A server that spawns a helper which itself exits, leaving a grandchild reparented away, used to leave that grandchild running after a stop; the stop now sweeps the server's whole session, not only the processes still directly parented to it.
+
+The daemon refuses to start rather than erase its own records. If a saved store (the registry, run state, or resource locks) exists but cannot be read, from an I/O error or too many open file descriptors, devctld now exits and lets launchd retry instead of treating the data as absent and overwriting it on the next write. A missing file still starts clean, and an unparseable one is still quarantined and rebuilt.

Sources/DevCtlDaemonCore/Control/ControlServer.swift

@@ -571,12 +571,12 @@ public actor Router {
             return
         }
         let pid = root.pid
-        let sweep = ProcessTree.descendants(of: pid)
-        if case .failed(let code) = sweep {
-            DevCtlLog.daemon.error(
-                "orphan bounce descendant sweep failed (errno \(code)); group-only")
-        }
-        let descendants = sweep.identities
+        /** A server is spawned as a session leader (createSession), so its
+            session id is its own pid; sweeping the session as well as the parent
+            chain catches an orphan descendant that setpgid'd or setsid'd out of
+            the group, the same union stop() and the crash path use. */
+        let descendants = ProcessTree.liveDescendants(
+            rootPid: pid, sessionID: pid, snapshot: [])
         ProcessTree.signalTree(
             descendants: descendants, revalidate: true, rootIdentity: root, rootPid: pid,
             signal: SIGTERM)

Sources/DevCtlDaemonCore/Supervisor/ProcessTree.swift

@@ -151,20 +151,36 @@ public enum ProcessTree {
                 let live = self.identity(of: identity.pid)
                 guard shouldSignal(snapshotted: identity, live: live) else { continue }
             }
-            if getpgid(identity.pid) != rootPid {
+            /** Skip a group member only when the group itself was signaled;
+                otherwise (the root is gone or recycled, so `kill(-rootPid)` was
+                withheld) a member still in that group would be missed, and it
+                must be signaled individually instead. */
+            if !rootStillOurs || getpgid(identity.pid) != rootPid {
                 kill(identity.pid, signal)
             }
         }
     }
 
-    /** Individually SIGKILL snapshotted PIDs that still match, used when the
-        root is already gone so group-directed kill no longer applies. */
-    public static func escalateIndividuals(_ identities: [ProcessIdentity]) {
-        for identity in identities {
-            let live = self.identity(of: identity.pid)
-            guard shouldSignal(snapshotted: identity, live: live) else { continue }
-            kill(identity.pid, SIGKILL)
+    /** Every way a live descendant of a run can be found, deduped by pid: the
+        snapshot taken while the root still parented them, a fresh parent-chain
+        sweep, and the session members that kept the root's session after
+        setpgid/setsid took them out of the group. No single source is enough
+        (see the note on ServerSupervisor.startDescendantWatch), so both the
+        deliberate-stop and crash paths union all three and revalidate each pid
+        at signal time. A failed sweep contributes nothing rather than throwing:
+        teardown proceeds with whatever the other sources found. */
+    public static func liveDescendants(
+        rootPid: pid_t, sessionID: pid_t?, snapshot: [ProcessIdentity]
+    ) -> [ProcessIdentity] {
+        var byPid: [pid_t: ProcessIdentity] = [:]
+        for identity in snapshot { byPid[identity.pid] = identity }
+        for identity in descendants(of: rootPid).identities { byPid[identity.pid] = identity }
+        if let sessionID {
+            for identity in sessionMembers(of: sessionID, sessionLeaderPid: rootPid).identities {
+                byPid[identity.pid] = identity
+            }
         }
+        return Array(byPid.values)
     }
 
     private struct TableRow: Sendable {

Sources/DevCtlDaemonCore/Supervisor/ServerSupervisor.swift

@@ -352,49 +352,53 @@ public actor ServerSupervisor {
         stopRequested = true
         stopWasDeliberate = deliberate
         phase = .stopping
-        /** Capture before any signal: after the grace window the pid number may
-            name a different process, and SIGKILL must not follow a recycled id. */
+        /** Capture the run's identity and its session before any signal and
+            before any await: after the grace window the pid number may name a
+            different process, recordOutcome for this same exit can run during the
+            awaits below and clear the live fields, and signalRun revalidates
+            against the captured identity so a recycled pid is never hit. */
         let rootIdentity = ProcessTree.identity(of: target)
-        let snapshotResult = ProcessTree.descendants(of: target)
-        if case .failed(let code) = snapshotResult {
-            DevCtlLog.supervisor.error(
-                "descendant sweep failed before SIGTERM (errno \(code)); group-only teardown")
-        }
-        let snapshot = snapshotResult.identities
-        ProcessTree.signalTree(
-            descendants: snapshot, rootPid: target, signal: SIGTERM)
+        let sessionID = rootSessionID
+        let snapshot = lastDescendantSnapshot
+        signalRun(
+            target: target, rootIdentity: rootIdentity, sessionID: sessionID,
+            snapshot: snapshot, signal: SIGTERM)
         let deadline = ContinuousClock.now.advanced(by: .seconds(graceSeconds))
         while ContinuousClock.now < deadline {
             if runTask == nil { break }
             if kill(target, 0) != 0 { break }
             try? await Task.sleep(for: .milliseconds(100))
         }
-        /** Escalate: the pre-signal snapshot plus a fresh sweep (new children may
-            have appeared during the grace window while the parent lived). */
-        let fresh = ProcessTree.descendants(of: target)
-        if case .failed(let code) = fresh {
-            DevCtlLog.supervisor.error(
-                "descendant sweep failed before SIGKILL (errno \(code)); using pre-signal snapshot")
-        }
-        var byPid: [pid_t: ProcessIdentity] = [:]
-        for identity in snapshot + fresh.identities {
-            byPid[identity.pid] = identity
-        }
-        let escalation = Array(byPid.values)
-        if let rootIdentity,
-            ProcessTree.shouldSignal(
-                snapshotted: rootIdentity, live: ProcessTree.identity(of: target))
-        {
-            ProcessTree.signalTree(
-                descendants: escalation, revalidate: true, rootIdentity: rootIdentity,
-                rootPid: target, signal: SIGKILL)
-        } else {
-            ProcessTree.escalateIndividuals(escalation)
-        }
+        /** Escalate over a freshly re-derived union (new children may have
+            appeared during the grace window); signalRun SIGKILLs the group only
+            while the root still lives, and otherwise the survivors individually. */
+        signalRun(
+            target: target, rootIdentity: rootIdentity, sessionID: sessionID,
+            snapshot: snapshot, signal: SIGKILL)
         await waitForRunTaskCompletion()
         return status()
     }
 
+    /** One revalidated teardown pass. Descendants come from every source at once
+        (the startup snapshot, a fresh parent-chain sweep, and the root's session
+        members), so a child that escaped the group by setpgid or setsid is still
+        found. The root's process group is signaled only while `rootPid` still
+        names the process `rootIdentity` recorded; once it has exited (or been
+        recycled) the group is never touched and only the descendants that still
+        match their recorded identity are signaled individually. Pass
+        `rootIdentity: nil` from the crash path, where the root is already reaped,
+        so `kill(-pid)` can never follow a recycled id. This is the one home for
+        turning a run's descendants into kernel signals. */
+    private func signalRun(
+        target: pid_t, rootIdentity: ProcessIdentity?, sessionID: pid_t?,
+        snapshot: [ProcessIdentity], signal: Int32
+    ) {
+        ProcessTree.signalTree(
+            descendants: ProcessTree.liveDescendants(
+                rootPid: target, sessionID: sessionID, snapshot: snapshot),
+            revalidate: true, rootIdentity: rootIdentity, rootPid: target, signal: signal)
+    }
+
     public func status() -> ServerStatus {
         let check = EffectiveHealthcheck.resolve(spec: spec)
         let terminal = phase == .crashed || phase == .failed
@@ -850,6 +854,13 @@ public actor ServerSupervisor {
 
     private func recordOutcome(_ outcome: ProcessOutcome, id: String) async {
         runTask = nil
+        /** Capture this run's teardown inputs before the awaits below: a
+            concurrent start() can replace `pid`, `rootSessionID`, and the
+            snapshot while recordOutcome is suspended, and the crash sweep must
+            act on the run that just exited, never on a newly started one. */
+        let capturedPid = pid
+        let capturedSessionID = rootSessionID
+        let capturedSnapshot = lastDescendantSnapshot
         healthTask?.cancel()
         healthTask = nil
         switch outcome {
@@ -873,17 +884,17 @@ public actor ServerSupervisor {
         errorSummary = captureErrorSummary(since: windowStart)
         descendantTask?.cancel()
         descendantTask = nil
-        if !stopRequested, let rootPid = pid {
-            /** Union of the snapshot and a live session sweep. The snapshot can
-                be stale (a worker forked moments before the crash may never have
-                been sampled, and under load the sampler may not even have been
-                scheduled), while the session sweep cannot see a descendant that
-                called setsid for itself. Neither covers the other, so both run. */
-            let escaped = ProcessTree.sessionMembers(
-                of: rootSessionID ?? rootPid, sessionLeaderPid: rootPid
-            ).identities
-            let union = Array(Set(lastDescendantSnapshot).union(escaped))
-            ProcessTree.signalTree(descendants: union, rootPid: rootPid, signal: SIGTERM)
+        if !stopRequested, let rootPid = capturedPid {
+            /** The root is already reaped here, so signalRun gets rootIdentity:
+                nil and never signals its process group: kill(-pid) on a reaped id
+                could land on a recycled group. Only the escaped descendants that
+                still match their recorded identity are swept, drawn from the
+                snapshot, a parent-chain sweep, and the session at once, since no
+                one source sees a child that setpgid'd, setsid'd, or forked after
+                the last sample. */
+            signalRun(
+                target: rootPid, rootIdentity: nil, sessionID: capturedSessionID,
+                snapshot: capturedSnapshot, signal: SIGTERM)
         }
         lastDescendantSnapshot = []
         rootSessionID = nil

Sources/DevCtlKit/Paths/Paths.swift

@@ -175,8 +175,24 @@ public enum AtomicFile {
         _ = try FileManager.default.replaceItemAt(url, withItemAt: tmp)
     }
 
-    public static func loadDefensively<T: Decodable>(_ type: T.Type, from url: URL) -> T? {
-        guard let data = try? Data(contentsOf: url) else { return nil }
+    /** Loads a persisted store, distinguishing three outcomes a single nil used
+        to blur together. A missing file returns nil: there is no prior state, so
+        starting empty is correct. A file that exists but cannot be READ (EMFILE
+        as the daemon nears its fd limit, an I/O error, a permission change)
+        THROWS, so the caller refuses to start rather than treating real data as
+        absent and erasing it on the next write. A file that reads but will not
+        PARSE is quarantined to `.corrupt-<timestamp>` and returns nil, because
+        the bytes are unusable and starting empty is the only recovery (a parse
+        crash under launchd KeepAlive would loop forever). */
+    public static func load<T: Decodable>(_ type: T.Type, from url: URL) throws -> T? {
+        let data: Data
+        do {
+            data = try Data(contentsOf: url)
+        } catch let error as CocoaError
+            where error.code == .fileReadNoSuchFile || error.code == .fileNoSuchFile
+        {
+            return nil
+        }
         do {
             return try JSONCoding.decoder().decode(type, from: data)
         } catch {
@@ -186,6 +202,15 @@ public enum AtomicFile {
             return nil
         }
     }
+
+    /** Non-throwing convenience: a missing, unreadable, or corrupt file all yield
+        nil. Use only where losing the value is safe (a rebuildable cache, a
+        secondary hint read after the primary store already loaded), never for a
+        store whose next write would overwrite real data. Reach for `load` there,
+        and refuse to start on a read failure. */
+    public static func loadDefensively<T: Decodable>(_ type: T.Type, from url: URL) -> T? {
+        try? load(type, from: url)
+    }
 }
 
 /** SHA-256 over CryptoKit, which is a system framework here rather than a

Sources/devctld/main.swift

@@ -87,6 +87,24 @@ try? FileManager.default.removeItem(at: paths.stoppedIntentFile)
     stay findable under a sealed in-bundle LaunchAgent. */
 LaunchdAdmin.applyAgentPathToProcess(paths: paths)
 
+/** Refuse to start on a persisted store that exists but cannot be READ (EMFILE
+    as the daemon nears its fd limit, an I/O error, a permission change): treating
+    it as absent would let the next write erase real registry, state, or lock
+    data. A missing file is fine (first run), and a corrupt one is quarantined by
+    the load. Exiting non-zero here leaves the data intact and lets launchd retry
+    after the throttle window, when a transient failure has likely cleared. */
+do {
+    _ = try AtomicFile.load(RegistryFile.self, from: paths.registryFile)
+    _ = try AtomicFile.load(StateFile.self, from: paths.stateFile)
+    _ = try AtomicFile.load(LocksFile.self, from: paths.locksFile)
+} catch {
+    FileHandle.standardError.write(
+        Data(
+            "devctld: a saved store exists but could not be read (\(error)); refusing to start so it is not overwritten. Free file descriptors or fix the file's permissions, then retry.\n"
+                .utf8))
+    exit(1)
+}
+
 let registry = Registry(paths: paths)
 let router = Router(launcher: SubprocessLauncher(), paths: paths, registry: registry)
 

Sources/fixture-server/main.swift

@@ -11,6 +11,11 @@ import Foundation
     --grandchild-after S   delay that spawn, which is what puts it past the
                            supervisor's early snapshot and makes the teardown
                            race deterministic instead of load-dependent
+    --orphan-grandchild    background a `sleep 1000` through a shell that then
+                           exits, so the sleep reparents away from this process
+                           but keeps its session. A parent-chain sweep can no
+                           longer find it; only a session sweep can. Prints
+                           `grandchild pid N` so a teardown test can verify it
     --ignore-sigterm       install SIG_IGN for SIGTERM (escalation verification)
     --emit-binary          write raw non-UTF8 bytes into stdout once
     --err-lines N          write N lines to stderr at startup (error-tally fixture)
@@ -26,6 +31,7 @@ var exitAfter: Double?
 var exitCode: Int32 = 0
 var spawnGrandchild = false
 var grandchildAfter: Double?
+var orphanGrandchild = false
 var ignoreSigterm = false
 var emitBinary = false
 var errLines = 0
@@ -46,6 +52,8 @@ while let arg = argIterator.next() {
         spawnGrandchild = true
     case "--grandchild-after":
         grandchildAfter = argIterator.next().flatMap { Double($0) }
+    case "--orphan-grandchild":
+        orphanGrandchild = true
     case "--ignore-sigterm":
         ignoreSigterm = true
     case "--emit-binary":
@@ -78,6 +86,19 @@ func launchGrandchild() {
     print("grandchild pid \(child.processIdentifier)")
 }
 
+/** Backgrounds a sleep through a shell that exits immediately, so the sleep
+    reparents to launchd while keeping this process's session (no setsid). Only a
+    session sweep can find it afterward, which is what a deliberate-stop teardown
+    test needs to distinguish from a parent-chain sweep. `$!` is the backgrounded
+    pid, echoed to the inherited stdout so the test can read it from the spool. */
+func launchOrphanGrandchild() {
+    let shell = Process()
+    shell.executableURL = URL(fileURLWithPath: "/bin/sh")
+    shell.arguments = ["-c", "sleep 1000 & echo grandchild pid $!"]
+    try? shell.run()
+    shell.waitUntilExit()
+}
+
 if spawnGrandchild {
     if let grandchildAfter {
         /** On a background queue so the heartbeat loop below still runs and the
@@ -88,6 +109,10 @@ if spawnGrandchild {
     }
 }
 
+if orphanGrandchild {
+    launchOrphanGrandchild()
+}
+
 if emitBinary {
     let junk: [UInt8] = [0xFF, 0xFE, 0x00, 0x80, 0x0A]
     FileHandle.standardOutput.write(Data(junk))