diff --git a/.changeset/process-safety-teardown.md b/.changeset/process-safety-teardown.md new file mode 100644 index 0000000..7a1b7e5 --- /dev/null +++ b/.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. diff --git a/AGENTS.md b/AGENTS.md index 4c2c5f0..f12e02d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -9,8 +9,8 @@ Identity and stack - Three products, one daemon: devctld owns all server processes; devctl (CLI) and devctl.app (SwiftUI MenuBarExtra) are thin clients over a unix socket (default ~/Library/Application Support/devctl/daemon.sock; DEVCTL_SOCKET overrides; /tmp fallback near the sun_path limit), NDJSON protocol. devctl daemon install/uninstall/start/stop/restart manage the LaunchAgent (dev.quantizor.devctl); tests and the smoke gate run devctld --foreground. Codebase map -- Sources/DevCtlKit: shared core, the unit-test target of record. Models.swift (specs, phases, ServerStatus, whose displayPort is the one home for which of the three port fields a human is shown; ServerSpec.validationErrors is the per-spec check the `register` seam runs so a directly-registered spec is screened like a committed one), Wire.swift (JSONCoding, typed request/response/event frames, NDJSON framing, stable error codes), Client/DaemonClient.swift (blocking-POSIX socket actor used unchanged by CLI and app; a SO_RCVTIMEO response deadline, raised for a command carrying its own timeout, so a wedged daemon fails a request instead of hanging the client forever), Paths/Paths.swift (path constants, canonical project path, atomic write + defensive load with per-call unique temp names so two writers in one process cannot rename each other's temp away, SHA-256 over CryptoKit with a chunked file-digest entry point so hashing a file costs a chunk of memory rather than the file; agent.path holds login-shell PATH for the daemon), Setup/ (SetupPlanner: first-run / upgrade decisions, harness offers, stage-and-rename binary install, and CLIOwner: whether devctl or Homebrew owns the CLI, decided by realpath-matching the running bundle against the Caskroom backlink rather than any `/Caskroom/` substring, which drives skipping the binary install and the PATH warning under brew; AppInstancePolicy decides which of two copies of one bundle quits at launch, scoped to the bundle path so the DMG-to-Applications handoff, the one case where two copies are correct, is left alone), Agent/ (AgentContext: the pure session-context renderer the hook injects, bad-state servers first with a devctl why recommendation and devctl's own stderr count, never raw child output; DiscoveryStanza), Net/LoopbackProbe.swift (dual-stack loopback listen probe shared by the daemon port pre-check and CLI doctor), Net/PortClaim.swift + PortMaterializer.swift (effectivePort claim: portSpan and named ports; env injection and URL rewrite, including resolving a root-relative head or healthcheck url against the server's own base), Config/ (ProjectConfig loader and validator; ConfigProjection projects merged specs back down to devservers.json, dropping everything the machine derived, for `config init`; EffectiveHost is the one home for the host a spawn will use, read by both prepareSpawn and config check; LocalOverlay; LockResource reads locks declarations and resolves a resource's state path; WatchPolicy is the pure settle/quiet/burst decision behind auto-restart and WatchPaths resolves the entries config check warns about), Resource/ResourceIdentity.swift (bounded fingerprint of a lock resource's state so `lock` can report a change made under a live holder), Launchd/ (LaunchdAdmin: dual install path; SMAppService via app deep link when /Applications/devctl.app exists, else legacy home LaunchAgent + Application Support bin/devctld; --legacy forces the home path; DaemonRecoveryPolicy decides whether an unreachable daemon is auto-restarted; AgentRebindPolicy + agent.rebind settle the ad-hoc CDHash window on DMG replace), DeepLink/ (parse/serialize + DeepLinkRunner + notification action map), Update/ (UpdateCheck: GitHub releases/latest against DevCtlVersion, one on-disk cache with an ETag shared by the app poll and `devctl doctor`, every failure silent, and never fed into AgentContext.render; DevCtlDistribution: the one home for the tap token, releases URL, and brew upgrade/uninstall commands), Log/DevCtlLog.swift (OSLog facade with a recording backend for tests). -- Sources/DevCtlDaemonCore: daemon logic as a library. Supervisor/ (ServerSupervisor actor per server: spawn, spool capture, health-gated phase machine, ensure/wait, group + descendant teardown with ProcessIdentity start-time revalidation; the ProcessLauncher seam; ProcessTree QA1123 sysctl sweep, plus narrowed/isAlive, the one home for turning a pid read off disk or the wire into one the kernel calls take, since a trapping conversion there is a crash loop under KeepAlive), Health/HealthProber.swift (EffectiveHealthcheck resolution, the HealthProber seam with ephemeral URLSession HTTP probes + BSD TCP, and PortGuard's lsof diagnostics, which live in that same file), Registry/ (owner of registry.json and state.json), Control/ (Router method dispatch + port pre-check + persisted resource locks with daemon-owned pause/resume and dead-holder auto-release + the boot-restore gate that answers daemon.info and refuses everything else with daemon-starting + NWListener ControlServer whose startAccepting awaits the listener's ready state and throws rather than suspending forever). prepareSpawn is the one funnel every start-shaped path takes and the one home for the trust gate: its `userInitiated` flag records trust for an explicit command acting on a committed server and refuses an autonomous restore/sweep of an unapproved project. register validates the spec and writeConfig refuses a project the daemon does not track; every project-scoped arm canonicalizes the path at the decode seam. +- Sources/DevCtlKit: shared core, the unit-test target of record. Models.swift (specs, phases, ServerStatus, whose displayPort is the one home for which of the three port fields a human is shown; ServerSpec.validationErrors is the per-spec check the `register` seam runs so a directly-registered spec is screened like a committed one), Wire.swift (JSONCoding, typed request/response/event frames, NDJSON framing, stable error codes), Client/DaemonClient.swift (blocking-POSIX socket actor used unchanged by CLI and app; a SO_RCVTIMEO response deadline, raised for a command carrying its own timeout, so a wedged daemon fails a request instead of hanging the client forever), Paths/Paths.swift (path constants, canonical project path, atomic write + load which distinguishes a missing file from an unreadable one from a corrupt one, with loadDefensively collapsing all three to nil for caches only, and per-call unique temp names so two writers in one process cannot rename each other's temp away, SHA-256 over CryptoKit with a chunked file-digest entry point so hashing a file costs a chunk of memory rather than the file; agent.path holds login-shell PATH for the daemon), Setup/ (SetupPlanner: first-run / upgrade decisions, harness offers, stage-and-rename binary install, and CLIOwner: whether devctl or Homebrew owns the CLI, decided by realpath-matching the running bundle against the Caskroom backlink rather than any `/Caskroom/` substring, which drives skipping the binary install and the PATH warning under brew; AppInstancePolicy decides which of two copies of one bundle quits at launch, scoped to the bundle path so the DMG-to-Applications handoff, the one case where two copies are correct, is left alone), Agent/ (AgentContext: the pure session-context renderer the hook injects, bad-state servers first with a devctl why recommendation and devctl's own stderr count, never raw child output; DiscoveryStanza), Net/LoopbackProbe.swift (dual-stack loopback listen probe shared by the daemon port pre-check and CLI doctor), Net/PortClaim.swift + PortMaterializer.swift (effectivePort claim: portSpan and named ports; env injection and URL rewrite, including resolving a root-relative head or healthcheck url against the server's own base), Config/ (ProjectConfig loader and validator; ConfigProjection projects merged specs back down to devservers.json, dropping everything the machine derived, for `config init`; EffectiveHost is the one home for the host a spawn will use, read by both prepareSpawn and config check; LocalOverlay; LockResource reads locks declarations and resolves a resource's state path; WatchPolicy is the pure settle/quiet/burst decision behind auto-restart and WatchPaths resolves the entries config check warns about), Resource/ResourceIdentity.swift (bounded fingerprint of a lock resource's state so `lock` can report a change made under a live holder), Launchd/ (LaunchdAdmin: dual install path; SMAppService via app deep link when /Applications/devctl.app exists, else legacy home LaunchAgent + Application Support bin/devctld; --legacy forces the home path; DaemonRecoveryPolicy decides whether an unreachable daemon is auto-restarted; AgentRebindPolicy + agent.rebind settle the ad-hoc CDHash window on DMG replace), DeepLink/ (parse/serialize + DeepLinkRunner + notification action map), Update/ (UpdateCheck: GitHub releases/latest against DevCtlVersion, one on-disk cache with an ETag shared by the app poll and `devctl doctor`, every failure silent, and never fed into AgentContext.render; DevCtlDistribution: the one home for the tap token, releases URL, and brew upgrade/uninstall commands), Log/DevCtlLog.swift (OSLog facade with a recording backend for tests). +- Sources/DevCtlDaemonCore: daemon logic as a library. Supervisor/ (ServerSupervisor actor per server: spawn, spool capture, health-gated phase machine, ensure/wait, group + descendant teardown through signalRun, the one signalling path, over ProcessTree.liveDescendants, the one home for the snapshot + parent-chain + session union, revalidated against ProcessIdentity start time so a recycled pid is never signaled; the ProcessLauncher seam; ProcessTree QA1123 sysctl sweep, plus narrowed/isAlive, the one home for turning a pid read off disk or the wire into one the kernel calls take, since a trapping conversion there is a crash loop under KeepAlive), Health/HealthProber.swift (EffectiveHealthcheck resolution, the HealthProber seam with ephemeral URLSession HTTP probes + BSD TCP, and PortGuard's lsof diagnostics, which live in that same file), Registry/ (owner of registry.json and state.json), Control/ (Router method dispatch + port pre-check + persisted resource locks with daemon-owned pause/resume and dead-holder auto-release + the boot-restore gate that answers daemon.info and refuses everything else with daemon-starting + NWListener ControlServer whose startAccepting awaits the listener's ready state and throws rather than suspending forever). prepareSpawn is the one funnel every start-shaped path takes and the one home for the trust gate: its `userInitiated` flag records trust for an explicit command acting on a committed server and refuses an autonomous restore/sweep of an unapproved project. register validates the spec and writeConfig refuses a project the daemon does not track; every project-scoped arm canonicalizes the path at the decode seam. - Sources/devctld: thin main; identical behavior under launchd and --foreground (tests and the smoke gate use foreground). Applies agent.path into process env before spawn, accepts on the socket before boot restore and marks the router restoring across it so a client can tell a busy daemon from a dead one, and runs the watch sweep on its own timer once restore has finished, so a boot spawn is never read as a config change. - Tests: DevCtlKitTests is the unit-test target of record and holds the schema goldens; DevCtlDaemonCoreTests drives a real Router over temp paths; DevCtlCLITests covers CLI behavior with a contract and no other way to exercise it (argument parsing, the lock notices and identity verdict), importing the executable target with @testable. TestSupport.swift is the one home for the fixture-server lookup and reserves ports 45000 to 45500 for the unit suites; touching it reaps fixtures orphaned by an interrupted run, but only those whose parent is gone and whose port is in that block, so a concurrent test run and smoke.sh (which orphans a fixture on purpose, outside the block) are both left alone. - Sources/devctl: CLI (swift-argument-parser). Two files only: HookSupport.swift (HookContext, the thin socket fetch over DevCtlKit's AgentContext renderer, + HarnessAdapter registry, each adapter with install/uninstall/hookState over a settings file devctl does not own and never edits without being asked; adding a harness: CONTRIBUTING.md) and CLI.swift, which holds every command as a struct, including Switch (branch switching + lifecycle playbooks), Lock (run-under-resource-lock), Doctor (health report; owns the cross-project port-collision and squatter findings, plus report-only harness-hook and update findings), Uninstall (the one uninstall verb: agent, hooks, and CLI, with --agent-only for the cask and --purge for data; `daemon uninstall` is a deprecated alias warning on stderr), HookInstall/HookUninstall, and Link / x-url (deep links). CLI.swift is past the size where splitting is worth asking about. @@ -30,14 +30,14 @@ Commands Hard rules - Output capture is spool-file fds, never pipes: children must survive daemon death without SIGPIPE. Do not introduce pipe-based capture anywhere. -- Teardown signals the process group AND every live descendant, found three ways because no one of them is sufficient: a sysctl parent-chain sweep snapshotted while the root still parents them (children that setpgid/setsid escape the group; orphans reparent to launchd and fall out of the parent-pid chain), a refresh of that snapshot every 200ms while the server is still starting (a worker forked a beat after spawn is otherwise in no snapshot, and with no healthcheck the first probe that would refresh it is a whole stabilization window away), and a live session sweep keyed on the run's session id, which is the only handle that survives the root exiting since createSession makes the root a session leader and an escaped child keeps the session even after reparenting. The session sweep refuses any session that is not led by the root pid and refuses the daemon's own session; without those guards it would signal the daemon and every server it supervises. Deliberate stop escalates after grace (snapshot union fresh sweep). Unexpected root exit applies SIGTERM to the group plus the union of snapshot and session sweep. Keep every half when touching stop() or the crash exit path. +- Teardown signals the process group AND every live descendant, found three ways because no one of them is sufficient: a sysctl parent-chain sweep snapshotted while the root still parents them (children that setpgid/setsid escape the group; orphans reparent to launchd and fall out of the parent-pid chain), a refresh of that snapshot every 200ms while the server is still starting (a worker forked a beat after spawn is otherwise in no snapshot, and with no healthcheck the first probe that would refresh it is a whole stabilization window away), and a live session sweep keyed on the run's session id, which is the only handle that survives the root exiting since createSession makes the root a session leader and an escaped child keeps the session even after reparenting. The session sweep refuses any session that is not led by the root pid and refuses the daemon's own session; without those guards it would signal the daemon and every server it supervises. The deliberate-stop and crash paths run one revalidated pass, signalRun over ProcessTree.liveDescendants, which unions all three sources and signals the root's process group only while the pid still names the process whose identity was captured while it was alive: a recycled pid is never hit, and the crash path, whose root is already reaped, passes rootIdentity nil so the group is never signaled at all. recordOutcome captures the run's pid, session, and snapshot at entry, since a concurrent start can replace them mid-teardown. Keep the three-source union and the revalidation when touching stop() or the crash exit path. - All JSON goes through JSONCoding: sorted keys, ISO-8601 UTC with milliseconds, no interior newlines. Never construct a raw JSONEncoder or JSONDecoder; the golden tests and NDJSON line framing depend on this determinism. - Wire methods are typed end to end: the daemon sniffs the {id, method} head, then re-decodes the full typed frame. A new method extends WireMethod plus Codable params/result types in Wire.swift; no untyped dictionaries on the wire. - Every CLI command supports --json with a stable schema generated from the shared Codable types; failures emit {ok:false, error:{code,message,hint}} on stdout, hint being the literal remediation command. Error codes grow append-only. Golden tests in Tests/DevCtlKitTests assert exact schema strings; a changed field is an API change: update docs/cli-contract.md in the same commit, then the golden. - Structured log files keep per-file monotonic timestamps (clamp on append); the since-query binary search depends on it. - The daemon never acts on a project's committed config before trust is recorded, enforced in prepareSpawn: an explicit command records trust, an autonomous restore or watch sweep refuses an unapproved project. The SessionStart hook never emits raw log lines or command strings into agent context (child output is attacker-influenceable). A spec reaching the daemon through `register` is validated like a committed one, and writeConfig only writes for a project the daemon already tracks. - A user-supplied regex (`logs --grep`) is screened before it runs: a nested unbounded quantifier is refused, because Swift's backtracking engine turns `(a+)+` into minutes of CPU on a single line and the match runs on the log actor. -- State files load defensively: parse failure quarantines to .corrupt- and continues; never fatal (a startup parse crash under launchd KeepAlive loops forever). Corollary: new fields on persisted types (registry, state) stay optional so existing files keep parsing. +- State files load through AtomicFile.load, which separates three outcomes: a missing file starts empty, a parse failure quarantines to .corrupt- and continues (never fatal, since a startup parse crash under launchd KeepAlive loops forever), and a file that exists but cannot be READ (EMFILE, an I/O error) throws so the daemon refuses to start rather than erasing it on the next write. The daemon's main probes registry, state, and locks this way before serving. loadDefensively collapses all three to nil and is only for a rebuildable cache or a secondary hint. Corollary: new fields on persisted types (registry, state) stay optional so existing files keep parsing. - Registry/state writes are temp + fsync + rename. - Binary upgrades stage and rename(2); never overwrite a running signed Mach-O. diff --git a/Sources/DevCtlDaemonCore/Control/ControlServer.swift b/Sources/DevCtlDaemonCore/Control/ControlServer.swift index 14049f7..48d824a 100644 --- a/Sources/DevCtlDaemonCore/Control/ControlServer.swift +++ b/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) diff --git a/Sources/DevCtlDaemonCore/Supervisor/ProcessTree.swift b/Sources/DevCtlDaemonCore/Supervisor/ProcessTree.swift index 561c072..e79f59c 100644 --- a/Sources/DevCtlDaemonCore/Supervisor/ProcessTree.swift +++ b/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 { diff --git a/Sources/DevCtlDaemonCore/Supervisor/ServerSupervisor.swift b/Sources/DevCtlDaemonCore/Supervisor/ServerSupervisor.swift index 23f7269..19166e8 100644 --- a/Sources/DevCtlDaemonCore/Supervisor/ServerSupervisor.swift +++ b/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 diff --git a/Sources/DevCtlKit/Paths/Paths.swift b/Sources/DevCtlKit/Paths/Paths.swift index 172036f..21a9b86 100644 --- a/Sources/DevCtlKit/Paths/Paths.swift +++ b/Sources/DevCtlKit/Paths/Paths.swift @@ -175,8 +175,24 @@ public enum AtomicFile { _ = try FileManager.default.replaceItemAt(url, withItemAt: tmp) } - public static func loadDefensively(_ 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-` 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(_ 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(_ 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 diff --git a/Sources/devctld/main.swift b/Sources/devctld/main.swift index a689fbe..9e12844 100644 --- a/Sources/devctld/main.swift +++ b/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) diff --git a/Sources/fixture-server/main.swift b/Sources/fixture-server/main.swift index bc31a93..1219801 100644 --- a/Sources/fixture-server/main.swift +++ b/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)) diff --git a/Tests/DevCtlDaemonCoreTests/SupervisorTests.swift b/Tests/DevCtlDaemonCoreTests/SupervisorTests.swift index 0984e2f..1c8482d 100644 --- a/Tests/DevCtlDaemonCoreTests/SupervisorTests.swift +++ b/Tests/DevCtlDaemonCoreTests/SupervisorTests.swift @@ -174,6 +174,92 @@ private func makeEnv() throws -> TestEnv { if !reaped { kill(child, SIGKILL) } } + /** Deliberate stop must sweep the session, not only the parent chain. The + fixture backgrounds a sleep through a shell that then exits, so by stop + time the sleep has reparented away and a `descendants(of: root)` walk can + no longer reach it: only the session sweep can. Before stop() unioned in + the session members, this sleep outlived `devctl stop`. */ + @Test func deliberateStopKillsAnOrphanedSessionGrandchild() async throws { + let fixture = try #require(fixtureServerExecutable()) + let env = try makeEnv() + let paths = env.paths + let registry = Registry(paths: paths) + let spec = ServerSpec(command: [fixture, "--orphan-grandchild"], name: "web") + let supervisor = ServerSupervisor( + launcher: SubprocessLauncher(), paths: paths, projectPath: env.projectPath, + registry: registry, spec: spec) + let started = await supervisor.start() + let root = pid_t(exactly: try #require(started.pid)) + var grandchild: pid_t? + for _ in 0..<40 { + let spool = + (try? String( + contentsOf: paths.structuredLogFile(project: env.projectPath, server: "web"), + encoding: .utf8)) ?? "" + if let match = spool.range(of: #"grandchild pid (\d+)"#, options: .regularExpression) { + grandchild = String(spool[match]).split(separator: " ").last.flatMap { pid_t($0) } + break + } + try await Task.sleep(for: .milliseconds(50)) + } + let child = try #require(grandchild) + #expect(kill(child, 0) == 0) + /** The precondition that makes this a session-only case: the sleep is no + longer a parent-chain descendant of the root, so only a session sweep + finds it. */ + if let root { + #expect(!ProcessTree.descendants(of: root).identities.contains { $0.pid == child }) + } + let stopped = await supervisor.stop(graceSeconds: 2) + #expect(stopped.phase == .stopped) + var reaped = false + for _ in 0..<100 where !reaped { + if kill(child, 0) != 0 { + reaped = true + break + } + try await Task.sleep(for: .milliseconds(50)) + } + #expect( + reaped, + "orphaned session grandchild \(child) survived devctl stop (state: \(processState(of: child)))") + if !reaped { kill(child, SIGKILL) } + } + + /** A stop racing concurrent starts must signal only the run being torn + down. The race is pid churn: a start can replace `pid` while a stop for + the previous run is mid-teardown, and recordOutcome for the old exit can + run while a new run is live. Every teardown signal now revalidates the pid + against the identity captured while that process was alive and reads the + run's fields captured at entry, so a recycled or replaced pid is never + hit. The supervisor's host process (this test) is therefore never signaled + out from under itself. Reaching the assertion at all is the guarantee the + SIGKILL bug removed; the rounds force the churn that surfaced it. */ + @Test func concurrentStopAndStartNeverSignalTheWrongProcess() async throws { + let env = try makeEnv() + let paths = env.paths + let registry = Registry(paths: paths) + /** A short-lived child bounds the test: even an interleaving that leaves a + teardown waiting on the run task resolves when the child exits on its + own, so a regression cannot hang the suite, only slow this case. */ + let spec = ServerSpec(command: ["/bin/sh", "-c", "sleep 2"], name: "web") + let supervisor = ServerSupervisor( + launcher: SubprocessLauncher(), paths: paths, projectPath: env.projectPath, + registry: registry, spec: spec) + for _ in 0..<4 { + await withTaskGroup(of: Void.self) { group in + group.addTask { _ = await supervisor.stop(graceSeconds: 1) } + group.addTask { _ = await supervisor.start() } + group.addTask { _ = await supervisor.start() } + for await _ in group {} + } + } + let phase = await supervisor.status().phase + #expect([.stopped, .starting, .running, .crashed].contains(phase)) + _ = await supervisor.stop(graceSeconds: 2) + #expect(getpid() > 0) // the test process survived the race + } + /** Reads a live process's parent from ps, for failure evidence only. */ private func parentPid(of pid: pid_t) -> String { shell(["/bin/ps", "-o", "ppid=", "-p", String(pid)]) diff --git a/Tests/DevCtlKitTests/AtomicFileTests.swift b/Tests/DevCtlKitTests/AtomicFileTests.swift new file mode 100644 index 0000000..9f37902 --- /dev/null +++ b/Tests/DevCtlKitTests/AtomicFileTests.swift @@ -0,0 +1,56 @@ +import Foundation +import Testing + +@testable import DevCtlKit + +private struct Box: Codable, Equatable { + var value: Int +} + +@Suite struct AtomicFileTests { + private func tempURL() -> URL { + FileManager.default.temporaryDirectory + .appending(path: "atomicfile-\(UUID().uuidString)") + .appending(path: "store.json") + } + + @Test func loadReturnsNilForAMissingFile() throws { + #expect(try AtomicFile.load(Box.self, from: tempURL()) == nil) + } + + @Test func loadDecodesAValidFile() throws { + let url = tempURL() + try AtomicFile.write(Data(#"{"value":7}"#.utf8), to: url) + #expect(try AtomicFile.load(Box.self, from: url) == Box(value: 7)) + } + + @Test func loadQuarantinesCorruptBytesAndReturnsNil() throws { + let url = tempURL() + try AtomicFile.write(Data("not json at all".utf8), to: url) + #expect(try AtomicFile.load(Box.self, from: url) == nil) + #expect(!FileManager.default.fileExists(atPath: url.path)) + let siblings = try FileManager.default.contentsOfDirectory( + atPath: url.deletingLastPathComponent().path) + #expect(siblings.contains { $0.contains("corrupt") }) + } + + /** A file that exists but cannot be read as data (here a directory at the + store path) must throw, so the daemon refuses to start rather than + treating real data as absent and erasing it on the next write. */ + @Test func loadThrowsWhenTheStoreExistsButCannotBeRead() throws { + let url = tempURL() + try FileManager.default.createDirectory(at: url, withIntermediateDirectories: true) + #expect(throws: (any Error).self) { + _ = try AtomicFile.load(Box.self, from: url) + } + } + + /** loadDefensively is the opposite contract: any failure, including the read + failure above, collapses to nil. It is only safe where losing the value + does no harm. */ + @Test func loadDefensivelySwallowsAReadFailure() throws { + let url = tempURL() + try FileManager.default.createDirectory(at: url, withIntermediateDirectories: true) + #expect(AtomicFile.loadDefensively(Box.self, from: url) == nil) + } +}