From 99763da835fdb9efe727e757f86d695f6e5b6e1a Mon Sep 17 00:00:00 2001 From: GeneralD Date: Tue, 21 Jul 2026 22:07:45 +0900 Subject: [PATCH 1/9] =?UTF-8?q?refactor(#340):=20executeProcess=20?= =?UTF-8?q?=E3=81=AE=20timeout=20oracle=20=E3=82=92=20ProcessExecutor=20?= =?UTF-8?q?=E3=81=AB=E6=8A=BD=E8=B1=A1=E5=8C=96?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit フレークの構造原因(timeout 判定が実クロック/実プロセス/ブロッキング drain に 直結し、テスト oracle が CI スケジューリングに人質)を 2 段 seam で断つ。 - ProcessGateway.runProcess を OS プリミティブとして追加(Domain 契約 + DarwinGateway live)。非ブロッキング drain(readabilityHandler)でプール枯渇を根絶し、cancel 時は SIGTERM→+500ms→SIGKILL で子を殺して即時 resume — 孤児 grandchild が pipe を掴んでも timeout の返りを塞がない - ProcessExecutor(新 covered モジュール)を新設。@Dependency(\.continuousClock) で timeout レースを駆動し、nil timeout は yt-dlp/ffmpeg 向けに無効化 - CustomScript/YouTube の重複 executeProcess static を撤去し、live processRunner を @Dependency(\.processExecutor) へ委譲(init 捕捉で構築時 override が効く)。YouTube 側に 残っていた waitUntilExit()(#308 が lyrics から除去した call)もこれで消える - 実時間 oracle テスト(50 echo <30s / timer <5s)を削除し、fake 停止プロセス + TestClock の 決定的テストへ置換。実サブプロセス検証は timing oracle なしの薄いスモークに縮小 - ProcessExecutor は DataSource 階層の collaborator で adjacent-layer 則は維持 (DataSource → ProcessExecutor → ProcessGateway) docs/ARCHITECTURE.md(module graph / Layer Summary / Key Design Decisions)を更新。 --- Package.swift | 18 ++ .../DarwinGateway+RunProcess.swift | 179 ++++++++++++++++++ .../ProcessExecutorRegistration.swift | 7 + Sources/Domain/Misc/ProcessExecutor.swift | 45 +++++ Sources/Domain/Misc/ProcessGateway.swift | 15 ++ .../CustomScriptLyricsDataSourceImpl.swift | 117 +----------- .../ProcessExecutor/ProcessExecutorImpl.swift | 57 ++++++ .../YouTubeWallpaperDataSourceImpl.swift | 80 ++------ .../BenchmarkHandlerTests.swift | 3 + .../ConfigHandlerImplTests.swift | 3 + .../DarwinGatewayRunProcessTests.swift | 102 ++++++++++ ...ustomScriptLyricsDataSourceImplTests.swift | 155 +++++---------- .../MediaRemoteDataSourceImplTests.swift | 3 + .../FakeProcessGateway.swift | 72 +++++++ .../ProcessExecutorImplTests.swift | 106 +++++++++++ .../ProcessHandlerImplTests.swift | 9 + .../ServiceHandlerImplTests.swift | 3 + .../YouTubeToolDetectionTests.swift | 3 + .../YouTubeWallpaperResolveTests.swift | 46 ++--- docs/ARCHITECTURE.md | 9 +- 20 files changed, 725 insertions(+), 307 deletions(-) create mode 100644 Sources/DarwinGateway/DarwinGateway+RunProcess.swift create mode 100644 Sources/DependencyInjection/ProcessExecutorRegistration.swift create mode 100644 Sources/Domain/Misc/ProcessExecutor.swift create mode 100644 Sources/ProcessExecutor/ProcessExecutorImpl.swift create mode 100644 Tests/DarwinGatewayTests/DarwinGatewayRunProcessTests.swift create mode 100644 Tests/ProcessExecutorTests/FakeProcessGateway.swift create mode 100644 Tests/ProcessExecutorTests/ProcessExecutorImplTests.swift diff --git a/Package.swift b/Package.swift index ee7e6539..7adf9c60 100644 --- a/Package.swift +++ b/Package.swift @@ -273,6 +273,7 @@ let package = Package( "MediaRemoteDataSource", "WallpaperDataSource", "AudioTapDataSource", + "ProcessExecutor", "SQLiteDataStore", "DarwinGateway", "CoreAudioTapGateway", @@ -480,6 +481,15 @@ let package = Package( ] ), + // ── ProcessExecutor (clock-driven timeout over ProcessGateway, #340) ── + .target( + name: "ProcessExecutor", + dependencies: [ + "Domain", + .product(name: "Dependencies", package: "swift-dependencies"), + ] + ), + // ── DataStore ── .target( name: "SQLiteDataStore", @@ -616,6 +626,14 @@ let package = Package( ), .testTarget(name: "AppTests", dependencies: ["App"]), .testTarget(name: "DarwinGatewayTests", dependencies: ["DarwinGateway"]), + .testTarget( + name: "ProcessExecutorTests", + dependencies: [ + "ProcessExecutor", + "Domain", + .product(name: "Dependencies", package: "swift-dependencies"), + ] + ), .testTarget(name: "AppKitScreenProviderTests", dependencies: ["AppKitScreenProvider", "Domain"]), .testTarget(name: "RandomSourceTests", dependencies: ["RandomSource", "Domain"]), .testTarget( diff --git a/Sources/DarwinGateway/DarwinGateway+RunProcess.swift b/Sources/DarwinGateway/DarwinGateway+RunProcess.swift new file mode 100644 index 00000000..5c4de457 --- /dev/null +++ b/Sources/DarwinGateway/DarwinGateway+RunProcess.swift @@ -0,0 +1,179 @@ +import Darwin +import Domain +import Foundation +import os + +// Async subprocess primitive for `ProcessExecutor` (#340). Split from DarwinGateway.swift +// to keep that file focused and under the line budget; the conformance is completed here. +extension DarwinGateway { + public func runProcess( + executable: String, arguments: [String], environment: [String: String] + ) async throws -> (status: Int32, stdout: String, stderr: String) { + let process = Process() + process.executableURL = URL(fileURLWithPath: executable) + process.arguments = arguments + process.environment = environment + process.standardInput = FileHandle.nullDevice + let stdoutPipe = Pipe() + let stderrPipe = Pipe() + process.standardOutput = stdoutPipe + process.standardError = stderrPipe + + let state = RunProcessState() + + return try await withTaskCancellationHandler { + try await withCheckedThrowingContinuation { + (continuation: CheckedContinuation<(status: Int32, stdout: String, stderr: String), Error>) in + // If cancellation already fired, bail before spawning anything. + guard state.register(continuation) else { + continuation.resume(throwing: CancellationError()) + return + } + + let group = DispatchGroup() + + // Registered before run() so an already-exited process still delivers its + // termination (Foundation only guarantees delivery when set ahead of exit) — + // paired with group.leave() instead of a post-hoc waitUntilExit(), which was + // observed to hang after repeated short-lived invocations (#308). + group.enter() + process.terminationHandler = { _ in group.leave() } + + // Non-blocking drain: readabilityHandler fires on a dispatch source as bytes + // arrive, so no thread-pool thread is parked on readDataToEndOfFile for the + // child's whole lifetime — that parking was the #340 pool-exhaustion cause. + group.enter() + drainPipe(stdoutPipe.fileHandleForReading, into: { state.appendStdout($0) }, onEOF: { group.leave() }) + group.enter() + drainPipe(stderrPipe.fileHandleForReading, into: { state.appendStderr($0) }, onEOF: { group.leave() }) + + do { + try process.run() + } catch { + stdoutPipe.fileHandleForReading.readabilityHandler = nil + stderrPipe.fileHandleForReading.readabilityHandler = nil + process.terminationHandler = nil + state.resume(with: .failure(error)) + return + } + + // Close the launch/cancel race: if cancellation requested termination before + // we had a pid, kill the child now that it is running. + if let pid = state.markLaunched(process.processIdentifier) { + terminateProcess(pid) + } + + group.notify(queue: .global()) { + state.resume( + with: .success((process.terminationStatus, state.stdoutTrimmed, state.stderrTrimmed))) + } + } + } onCancel: { + // Resume the continuation with CancellationError *immediately* — do NOT wait for + // the pipes to drain. A SIGTERM-ignoring script's orphaned grandchild can hold + // the pipe open long past the kill, and the executor's timeout must return + // promptly regardless (#340). The child is signalled here; the leaked drain + // finishes and is ignored (resume-once) whenever the grandchild finally exits. + let (killPid, continuation) = state.requestCancel() + if let killPid { terminateProcess(killPid) } + continuation?.resume(throwing: CancellationError()) + } + } +} + +/// Installs a non-blocking readabilityHandler that accumulates bytes and signals EOF +/// (an empty read) exactly once, detaching itself. +private func drainPipe( + _ handle: FileHandle, into append: @escaping @Sendable (Data) -> Void, onEOF: @escaping @Sendable () -> Void +) { + handle.readabilityHandler = { handle in + let data = handle.availableData + guard !data.isEmpty else { + handle.readabilityHandler = nil + onEOF() + return + } + append(data) + } +} + +/// SIGTERM, then SIGKILL after a short grace if the child ignores the polite signal — +/// so a script trapping SIGTERM cannot outlive the executor's timeout. Targets the +/// specific pid (never the process group) so lyra itself is never at risk; a shell +/// script's own grandchildren are outside this guarantee. +private func terminateProcess(_ pid: Int32) { + kill(pid, SIGTERM) + DispatchQueue.global().asyncAfter(deadline: .now() + .milliseconds(500)) { + if kill(pid, 0) == 0 { kill(pid, SIGKILL) } + } +} + +/// Coordinates the checked continuation, byte accumulation, and the launch/cancel races +/// for a single `runProcess` call. `@unchecked Sendable`: every field is touched only +/// inside `lock`. +private final class RunProcessState: @unchecked Sendable { + typealias Continuation = CheckedContinuation<(status: Int32, stdout: String, stderr: String), Error> + + private let lock = OSAllocatedUnfairLock() + private var continuation: Continuation? + private var resumed = false + private var launched = false + private var terminateRequested = false + private var pid: Int32? + private var stdout = Data() + private var stderr = Data() + + /// Registers the continuation. Returns `false` if cancellation already fired (the + /// caller must resume-throw itself), `true` to proceed with spawning. + func register(_ newContinuation: Continuation) -> Bool { + lock.withLock { + continuation = newContinuation + return !terminateRequested + } + } + + func appendStdout(_ data: Data) { lock.withLock { stdout.append(data) } } + func appendStderr(_ data: Data) { lock.withLock { stderr.append(data) } } + var stdoutTrimmed: String { lock.withLock { Self.trimmed(stdout) } } + var stderrTrimmed: String { lock.withLock { Self.trimmed(stderr) } } + + /// Marks the child launched, recording its pid. Returns the pid iff a termination was + /// already requested (cancel raced ahead of the pid) so the caller kills it now. + func markLaunched(_ newPid: Int32) -> Int32? { + lock.withLock { + launched = true + pid = newPid + return terminateRequested ? newPid : nil + } + } + + /// Handles cancellation atomically: records the request, and returns the pid to kill + /// (if the child is already launched) plus the continuation to resume-throw (if it is + /// set and not yet resumed). The caller performs the kill and resume outside the lock. + func requestCancel() -> (killPid: Int32?, continuation: Continuation?) { + lock.withLock { + terminateRequested = true + let killPid = launched ? pid : nil + guard !resumed else { return (killPid, nil) } + resumed = true + defer { continuation = nil } + return (killPid, continuation) + } + } + + /// Resumes the continuation exactly once for normal completion / launch failure; the + /// losing side of a completion-vs-cancel race is ignored. + func resume(with result: Result<(status: Int32, stdout: String, stderr: String), Error>) { + let pending = lock.withLock { () -> Continuation? in + guard !resumed else { return nil } + resumed = true + defer { continuation = nil } + return continuation + } + pending?.resume(with: result) + } + + private static func trimmed(_ data: Data) -> String { + String(data: data, encoding: .utf8)?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" + } +} diff --git a/Sources/DependencyInjection/ProcessExecutorRegistration.swift b/Sources/DependencyInjection/ProcessExecutorRegistration.swift new file mode 100644 index 00000000..4d632ede --- /dev/null +++ b/Sources/DependencyInjection/ProcessExecutorRegistration.swift @@ -0,0 +1,7 @@ +import Dependencies +import Domain +import ProcessExecutor + +extension ProcessExecutorKey: DependencyKey { + public static let liveValue: any ProcessExecutor = ProcessExecutorImpl() +} diff --git a/Sources/Domain/Misc/ProcessExecutor.swift b/Sources/Domain/Misc/ProcessExecutor.swift new file mode 100644 index 00000000..bc74435c --- /dev/null +++ b/Sources/Domain/Misc/ProcessExecutor.swift @@ -0,0 +1,45 @@ +import Dependencies + +/// Runs a subprocess with an optional timeout, capturing stdout/stderr. +/// +/// This is the DataSource-tier collaborator that both `CustomScriptLyricsDataSourceImpl` +/// and `YouTubeWallpaperDataSourceImpl` share instead of each carrying its own +/// `executeProcess` static (#340). It sits directly above `ProcessGateway`: +/// +/// - the **executor** owns the clock-driven timeout race (`@Dependency(\.continuousClock)`), +/// so a hung child is detected deterministically in tests — no real waiting; +/// - the **gateway** (`runProcess`) owns spawning + non-blocking drain + cancellation. +/// +/// Separating the two is what makes the timeout logic testable: the flaky +/// pre-#340 tests could only assert timeout by running a real subprocess and +/// measuring wall-clock, whose oracle was hostage to CI scheduling. +public protocol ProcessExecutor: Sendable { + /// - Parameters: + /// - timeoutMs: milliseconds before the child is killed and a + /// `(-1, "", "timed out after …ms")` result returned. `nil` (or a + /// non-finite / ≤ 0 value) disables the timeout — used for long-lived, + /// low-frequency tools like yt-dlp / ffmpeg, which must run to completion. + /// - Returns: the child's exit status plus trimmed stdout/stderr. + func run( + executable: String, arguments: [String], environment: [String: String], timeoutMs: Double? + ) async throws -> (status: Int32, stdout: String, stderr: String) +} + +public enum ProcessExecutorKey: TestDependencyKey { + public static let testValue: any ProcessExecutor = UnimplementedProcessExecutor() +} + +extension DependencyValues { + public var processExecutor: any ProcessExecutor { + get { self[ProcessExecutorKey.self] } + set { self[ProcessExecutorKey.self] = newValue } + } +} + +private struct UnimplementedProcessExecutor: ProcessExecutor { + func run( + executable: String, arguments: [String], environment: [String: String], timeoutMs: Double? + ) async throws -> (status: Int32, stdout: String, stderr: String) { + fatalError("ProcessExecutor.run not implemented") + } +} diff --git a/Sources/Domain/Misc/ProcessGateway.swift b/Sources/Domain/Misc/ProcessGateway.swift index f18adfb4..46ad5763 100644 --- a/Sources/Domain/Misc/ProcessGateway.swift +++ b/Sources/Domain/Misc/ProcessGateway.swift @@ -34,6 +34,16 @@ public protocol ProcessGateway: Sendable { func runInteractiveShell(_ command: String) -> Int32 func runCapturingOutput(executable: String, arguments: [String]) -> String? func runStreaming(executable: String, arguments: [String]) -> AsyncStream + + /// Async subprocess primitive for `ProcessExecutor` (#340): spawns the child, + /// drains stdout/stderr *without blocking a thread pool*, and runs to completion. + /// Timeout is deliberately NOT a parameter here — the clock-driven timeout lives + /// in `ProcessExecutor` so it stays testable. On task **cancellation** (the + /// executor's timeout path), the child is terminated (SIGTERM, then SIGKILL after + /// a short grace) and `CancellationError` is thrown. + func runProcess( + executable: String, arguments: [String], environment: [String: String] + ) async throws -> (status: Int32, stdout: String, stderr: String) } public enum ProcessGatewayKey: TestDependencyKey { @@ -68,4 +78,9 @@ private struct UnimplementedProcessGateway: ProcessGateway { func runStreaming(executable: String, arguments: [String]) -> AsyncStream { fatalError("ProcessGateway.runStreaming not implemented") } + func runProcess( + executable: String, arguments: [String], environment: [String: String] + ) async throws -> (status: Int32, stdout: String, stderr: String) { + fatalError("ProcessGateway.runProcess not implemented") + } } diff --git a/Sources/LyricsDataSource/CustomScriptLyricsDataSourceImpl.swift b/Sources/LyricsDataSource/CustomScriptLyricsDataSourceImpl.swift index cea371cd..2bbffd0e 100644 --- a/Sources/LyricsDataSource/CustomScriptLyricsDataSourceImpl.swift +++ b/Sources/LyricsDataSource/CustomScriptLyricsDataSourceImpl.swift @@ -1,7 +1,6 @@ import Dependencies import Domain import Foundation -import os public struct CustomScriptLyricsDataSourceImpl: Sendable { // Config values (fallback_command, timeout_ms, configDir) are deliberately NOT @@ -15,8 +14,12 @@ public struct CustomScriptLyricsDataSourceImpl: Sendable { ) public init() { + // Resolve the executor from the context active at init (as the daemon's live DI + // graph provides it) and capture it — a construction-time `withDependencies` + // override then sticks, mirroring how `configDataSource` is captured. + @Dependency(\.processExecutor) var processExecutor self.init(processRunner: { executable, arguments, environment, timeoutMs in - try await Self.executeProcess( + try await processExecutor.run( executable: executable, arguments: arguments, environment: environment, timeoutMs: timeoutMs) }) } @@ -113,108 +116,8 @@ private struct ScriptOutput: Decodable { } } -// MARK: - Async Process - -extension CustomScriptLyricsDataSourceImpl { - static func executeProcess( - executable: String, arguments: [String], environment: [String: String], timeoutMs: Double - ) async throws -> (status: Int32, stdout: String, stderr: String) { - // timeout_ms comes straight from user config: clamp to a finite sane window - // (1 ms … 1 h) before Int conversion — Int(Double) traps on NaN/±inf/ - // out-of-range, which would let a pathological config value crash the daemon. - let timeoutMs = timeoutMs.isFinite ? min(max(timeoutMs, 1), 3_600_000) : 5000 - return try await withCheckedThrowingContinuation { continuation in - let process = Process() - process.executableURL = URL(fileURLWithPath: executable) - process.arguments = arguments - process.environment = environment - process.standardInput = FileHandle.nullDevice - let stdoutPipe = Pipe() - let stderrPipe = Pipe() - process.standardOutput = stdoutPipe - process.standardError = stderrPipe - - let buffer = ScriptProcessBuffer() - let group = DispatchGroup() - let hasResumed = OSAllocatedUnfairLock(initialState: false) - - // Registered before `run()` so there is no race with an already-exited - // process (Foundation only guarantees delivery when the handler is set - // ahead of termination). Paired with `group.leave()` here rather than a - // post-hoc `waitUntilExit()`, which has been empirically observed to hang - // indefinitely after repeated short-lived invocations (#308 review). - group.enter() - process.terminationHandler = { _ in group.leave() } - - do { - try process.run() - } catch { - continuation.resume(throwing: error) - return - } - - group.enter() - DispatchQueue.global().async { - buffer.stdout = stdoutPipe.fileHandleForReading.readDataToEndOfFile() - group.leave() - } - group.enter() - DispatchQueue.global().async { - buffer.stderr = stderrPipe.fileHandleForReading.readDataToEndOfFile() - group.leave() - } - - let timeoutWorkItem = DispatchWorkItem { - let shouldResume = hasResumed.withLock { done -> Bool in - guard !done else { return false } - done = true - return true - } - guard shouldResume else { return } - // SIGTERM first, then escalate to SIGKILL on the direct child pid if it - // ignores the polite signal — otherwise a custom script that traps SIGTERM - // keeps running after lyra has moved on, defeating the configured timeout. - // Target the specific pid (never the process group) so lyra itself is never - // at risk; a shell script's own grandchildren are outside this guarantee. - process.terminate() - let pid = process.processIdentifier - DispatchQueue.global().asyncAfter(deadline: .now() + .milliseconds(500)) { - if process.isRunning { kill(pid, SIGKILL) } - } - continuation.resume(returning: (-1, "", "timed out after \(Int(timeoutMs))ms")) - } - DispatchQueue.global().asyncAfter( - deadline: .now() + .milliseconds(Int(timeoutMs)), execute: timeoutWorkItem) - - group.notify(queue: .global()) { - let shouldResume = hasResumed.withLock { done -> Bool in - guard !done else { return false } - done = true - return true - } - guard shouldResume else { return } - timeoutWorkItem.cancel() - // All three members (stdout drain, stderr drain, terminationHandler) - // have completed, so terminationStatus is already valid to read — - // no blocking waitUntilExit() call needed on the success path. - continuation.resume( - returning: (process.terminationStatus, buffer.stdoutTrimmed, buffer.stderrTrimmed)) - } - } - } -} - -/// Accumulates stdout/stderr from concurrent pipe-drain tasks. `@unchecked Sendable` -/// because each property is written by exactly one DispatchQueue task and read only -/// after the DispatchGroup barrier — no lock needed (mirrors YouTubeWallpaperDataSourceImpl's PipeBuffer). -private final class ScriptProcessBuffer: @unchecked Sendable { - var stdout = Data() - var stderr = Data() - - var stdoutTrimmed: String { trimmed(stdout) } - var stderrTrimmed: String { trimmed(stderr) } - - private func trimmed(_ data: Data) -> String { - String(data: data, encoding: .utf8)?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" - } -} +// The subprocess plumbing that used to live here (an `executeProcess` static with a +// real clock, blocking pipe drain, and SIGTERM→SIGKILL timeout) moved to +// `ProcessExecutor` + `DarwinGateway.runProcess` (#340) so the timeout oracle is +// abstracted and testable. The no-arg init's live `processRunner` now delegates to +// `@Dependency(\.processExecutor)`. diff --git a/Sources/ProcessExecutor/ProcessExecutorImpl.swift b/Sources/ProcessExecutor/ProcessExecutorImpl.swift new file mode 100644 index 00000000..e45df1ec --- /dev/null +++ b/Sources/ProcessExecutor/ProcessExecutorImpl.swift @@ -0,0 +1,57 @@ +import Dependencies +import Domain + +public struct ProcessExecutorImpl: Sendable { + @Dependency(\.continuousClock) private var clock + @Dependency(\.processGateway) private var gateway + + public init() {} +} + +extension ProcessExecutorImpl: ProcessExecutor { + public func run( + executable: String, arguments: [String], environment: [String: String], timeoutMs: Double? + ) async throws -> (status: Int32, stdout: String, stderr: String) { + let gateway = self.gateway + + // No timeout: run to completion. `nil`, non-finite, and ≤ 0 all mean "don't + // race a timer" — used for long-lived tools (yt-dlp/ffmpeg) that must finish. + guard let timeoutMs, timeoutMs.isFinite, timeoutMs > 0 else { + return try await gateway.runProcess( + executable: executable, arguments: arguments, environment: environment) + } + + // Clamp to a finite sane window (1 ms … 1 h) before `Int(Double)`, which traps + // on NaN/±inf/out-of-range — a pathological config value must not crash the daemon. + let clampedMs = Int(min(max(timeoutMs, 1), 3_600_000)) + let clock = self.clock + + return try await withThrowingTaskGroup(of: Outcome.self) { group in + group.addTask { + .completed( + try await gateway.runProcess( + executable: executable, arguments: arguments, environment: environment)) + } + group.addTask { + try await clock.sleep(for: .milliseconds(clampedMs)) + return .timedOut + } + // The first child to finish (or throw) decides the outcome. A launch error + // surfaces from the gateway task here and propagates. When the timeout wins, + // `cancelAll()` cancels the still-running gateway task — `runProcess` then + // terminates the child and throws `CancellationError`, discarded on scope exit. + defer { group.cancelAll() } + switch try await group.next()! { + case .completed(let result): + return result + case .timedOut: + return (status: -1, stdout: "", stderr: "timed out after \(clampedMs)ms") + } + } + } +} + +private enum Outcome: Sendable { + case completed((status: Int32, stdout: String, stderr: String)) + case timedOut +} diff --git a/Sources/WallpaperDataSource/YouTubeWallpaperDataSourceImpl.swift b/Sources/WallpaperDataSource/YouTubeWallpaperDataSourceImpl.swift index a5b7e895..505a0bfb 100644 --- a/Sources/WallpaperDataSource/YouTubeWallpaperDataSourceImpl.swift +++ b/Sources/WallpaperDataSource/YouTubeWallpaperDataSourceImpl.swift @@ -11,12 +11,20 @@ public struct YouTubeWallpaperDataSourceImpl: Sendable { let replaceItemAtPath: @Sendable (String, String) -> Void public init() { + // Resolve+capture the executor from the init-time context (the daemon's live DI + // graph), so a construction-time override sticks in tests. + @Dependency(\.processExecutor) var processExecutor self.init( tempPathFor: { url, ext in try WallpaperCache().tempPath(for: url, ext: ext) }, processRunner: { executablePath, arguments in - try await Self.executeProcess(executablePath: executablePath, arguments: arguments) + // timeout nil: yt-dlp/ffmpeg are long-lived and must run to completion. + // Pass the daemon's inherited environment (as the old executeProcess did by + // never setting process.environment) so the tools keep their PATH etc. + try await processExecutor.run( + executable: executablePath, arguments: arguments, + environment: ProcessInfo.processInfo.environment, timeoutMs: nil) }, fileExistsAtPath: { path in FileManager.default.fileExists(atPath: path) @@ -222,72 +230,10 @@ extension YouTubeWallpaperDataSourceImpl { } } -// MARK: - Async Process - -extension YouTubeWallpaperDataSourceImpl { - static func executeProcess(executablePath: String, arguments: [String]) async throws -> (status: Int32, stdout: String, stderr: String) { - try await withCheckedThrowingContinuation { continuation in - let process = Process() - process.executableURL = URL(fileURLWithPath: executablePath) - process.arguments = arguments - process.standardInput = FileHandle.nullDevice - let stdoutPipe = Pipe() - let stderrPipe = Pipe() - process.standardOutput = stdoutPipe - process.standardError = stderrPipe - - do { - try process.run() - } catch { - continuation.resume(throwing: error) - return - } - - // Drain both pipes concurrently on background threads so that neither fills - // its OS pipe buffer and deadlocks the child process. ffmpeg in particular - // streams extensive progress output to stderr throughout a transcode, which - // can easily exceed the ~64 KB pipe buffer before the process exits. - let buffer = PipeBuffer() - let group = DispatchGroup() - - group.enter() - DispatchQueue.global().async { - buffer.stdout = stdoutPipe.fileHandleForReading.readDataToEndOfFile() - group.leave() - } - - group.enter() - DispatchQueue.global().async { - buffer.stderr = stderrPipe.fileHandleForReading.readDataToEndOfFile() - group.leave() - } - - group.notify(queue: .global()) { - // Both pipes have drained (EOF received → child has exited), but NSTask's - // internal SIGCHLD processing may not have run yet. waitUntilExit() ensures - // terminationStatus is valid before we read it. - process.waitUntilExit() - continuation.resume( - returning: (process.terminationStatus, buffer.stdoutTrimmed, buffer.stderrTrimmed)) - } - } - } -} - -/// Accumulates stdout and stderr bytes from concurrent pipe-drain tasks. -/// Marked `@unchecked Sendable` because each stored property is written by exactly -/// one DispatchQueue task and read only after the DispatchGroup barrier — no lock needed. -private final class PipeBuffer: @unchecked Sendable { - var stdout = Data() - var stderr = Data() - - var stdoutTrimmed: String { trimmed(stdout) } - var stderrTrimmed: String { trimmed(stderr) } - - private func trimmed(_ data: Data) -> String { - String(data: data, encoding: .utf8)?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" - } -} +// The subprocess plumbing that used to live here (an `executeProcess` static with a +// blocking pipe drain and a residual `waitUntilExit()` — the very call #308 removed from +// the lyrics side) moved to `ProcessExecutor` + `DarwinGateway.runProcess` (#340). The +// no-arg init's live `processRunner` now delegates to `@Dependency(\.processExecutor)`. // MARK: - Errors diff --git a/Tests/BenchmarkHandlerTests/BenchmarkHandlerTests.swift b/Tests/BenchmarkHandlerTests/BenchmarkHandlerTests.swift index ab021dad..aac3b849 100644 --- a/Tests/BenchmarkHandlerTests/BenchmarkHandlerTests.swift +++ b/Tests/BenchmarkHandlerTests/BenchmarkHandlerTests.swift @@ -37,6 +37,9 @@ private final class MockGateway: ProcessGateway, @unchecked Sendable { func run(executable: String, arguments: [String]) -> Int32 { 0 } func runInteractiveShell(_ command: String) -> Int32 { 0 } func runCapturingOutput(executable: String, arguments: [String]) -> String? { nil } + func runProcess(executable: String, arguments: [String], environment: [String: String]) async throws -> ( + status: Int32, stdout: String, stderr: String + ) { fatalError("unused") } func runStreaming(executable: String, arguments: [String]) -> AsyncStream { AsyncStream { $0.finish() } } diff --git a/Tests/ConfigHandlerTests/ConfigHandlerImplTests.swift b/Tests/ConfigHandlerTests/ConfigHandlerImplTests.swift index 0fb57ed1..0e249fb7 100644 --- a/Tests/ConfigHandlerTests/ConfigHandlerImplTests.swift +++ b/Tests/ConfigHandlerTests/ConfigHandlerImplTests.swift @@ -437,6 +437,9 @@ private final class ProcessGatewaySpy: ProcessGateway, @unchecked Sendable { return runStatus } func runCapturingOutput(executable: String, arguments: [String]) -> String? { nil } + func runProcess(executable: String, arguments: [String], environment: [String: String]) async throws -> ( + status: Int32, stdout: String, stderr: String + ) { fatalError("unused") } func runStreaming(executable: String, arguments: [String]) -> AsyncStream { AsyncStream { $0.finish() } } diff --git a/Tests/DarwinGatewayTests/DarwinGatewayRunProcessTests.swift b/Tests/DarwinGatewayTests/DarwinGatewayRunProcessTests.swift new file mode 100644 index 00000000..654bc37f --- /dev/null +++ b/Tests/DarwinGatewayTests/DarwinGatewayRunProcessTests.swift @@ -0,0 +1,102 @@ +import Darwin +import Foundation +import Testing + +@testable import DarwinGateway + +/// Thin real-subprocess smokes for `runProcess` (#340). These carry **no timing +/// oracle** — the deterministic timeout logic lives in `ProcessExecutor` and is +/// tested there with a fake clock. Here we only assert the OS primitive's contract: +/// status, captured stdout/stderr, environment pass-through, launch failure, and +/// that repeated short-lived spawns don't hang (the #308 regression class). +/// `.serialized` because real processes are global OS state (cf. #315). +@Suite("DarwinGateway.runProcess", .serialized) +struct DarwinGatewayRunProcessTests { + private let gateway = DarwinGateway() + + @Test("captures stdout and a zero status from a successful command") + func capturesStdout() async throws { + let result = try await gateway.runProcess( + executable: "/bin/echo", arguments: ["hi there"], environment: [:]) + #expect(result.status == 0) + #expect(result.stdout == "hi there") + } + + @Test("captures stderr and a non-zero status") + func capturesStderrAndExitCode() async throws { + let result = try await gateway.runProcess( + executable: "/bin/sh", arguments: ["-c", "echo oops 1>&2; exit 3"], environment: [:]) + #expect(result.status == 3) + #expect(result.stderr.contains("oops")) + } + + @Test("passes the environment through to the child") + func passesEnvironment() async throws { + let result = try await gateway.runProcess( + executable: "/bin/sh", arguments: ["-c", "printf %s \"$FOO\""], environment: ["FOO": "bar"]) + #expect(result.status == 0) + #expect(result.stdout == "bar") + } + + @Test("throws when the executable cannot be launched") + func throwsOnLaunchFailure() async { + await #expect(throws: (any Error).self) { + try await gateway.runProcess( + executable: "/nonexistent/definitely-not-real-xyz", arguments: [], environment: [:]) + } + } + + @Test("repeated short-lived spawns complete without hanging (#308 regression guard)") + func repeatedSpawnsDoNotHang() async throws { + for i in 0..<50 { + let result = try await gateway.runProcess( + executable: "/bin/echo", arguments: ["n-\(i)"], environment: [:]) + #expect(result.status == 0) + #expect(result.stdout == "n-\(i)") + } + } + + @Test("cancelling the task terminates the child and throws instead of waiting it out") + func cancellationTerminatesChild() async throws { + let task = Task { + try await gateway.runProcess( + executable: "/bin/sh", arguments: ["-c", "sleep 5"], environment: [:]) + } + // Let the child actually spawn before cancelling (real-process smoke, not a + // state-polling assertion — a short spawn delay is acceptable here). + try await Task.sleep(for: .milliseconds(50)) + task.cancel() + await #expect(throws: CancellationError.self) { try await task.value } + } + + @Test("cancellation SIGKILLs a child that ignores SIGTERM") + func cancellationForceKillsStubbornChild() async throws { + let pidFile = NSTemporaryDirectory() + "lyra-runproc-\(UUID().uuidString).pid" + defer { try? FileManager.default.removeItem(atPath: pidFile) } + + // The child records its own pid, traps (ignores) SIGTERM, then sleeps far past any + // reasonable window — only the SIGTERM→SIGKILL escalation can stop it. + let task = Task { + try await gateway.runProcess( + executable: "/bin/sh", + arguments: ["-c", "echo $$ > '\(pidFile)'; trap '' TERM; sleep 3"], + environment: [:]) + } + try await Task.sleep(for: .milliseconds(100)) // let it record its pid + install the trap + task.cancel() + await #expect(throws: CancellationError.self) { try await task.value } + + let pidString = + (try? String(contentsOfFile: pidFile, encoding: .utf8))? + .trimmingCharacters(in: .whitespacesAndNewlines) ?? "" + let pid = Int32(pidString) ?? 0 + #expect(pid > 0, "the child should have recorded its pid before ignoring SIGTERM") + + // The reap is asynchronous — poll rather than sleep a fixed interval. + let deadline = ContinuousClock.now + .seconds(3) + while kill(pid, 0) == 0, ContinuousClock.now < deadline { + try? await Task.sleep(for: .milliseconds(20)) + } + #expect(kill(pid, 0) != 0, "a SIGTERM-ignoring child must be SIGKILLed, not left running") + } +} diff --git a/Tests/LyricsDataSourceTests/CustomScriptLyricsDataSourceImplTests.swift b/Tests/LyricsDataSourceTests/CustomScriptLyricsDataSourceImplTests.swift index fe66ff23..daee1d1d 100644 --- a/Tests/LyricsDataSourceTests/CustomScriptLyricsDataSourceImplTests.swift +++ b/Tests/LyricsDataSourceTests/CustomScriptLyricsDataSourceImplTests.swift @@ -176,20 +176,6 @@ struct CustomScriptLyricsDataSourceImplTests { #expect(await captured.environment?["LYRA_CONFIG_DIR"] == "/retained") } - @Test("executeProcess survives pathological timeout values — huge/NaN/infinite clamp instead of trapping") - func executeProcessClampsPathologicalTimeouts() async throws { - for pathological in [1e300, .nan, .infinity] as [Double] { - let result = try await CustomScriptLyricsDataSourceImpl.executeProcess( - executable: "/bin/echo", arguments: ["ok"], environment: [:], timeoutMs: pathological) - #expect(result.status == 0) - #expect(result.stdout == "ok") - } - // A negative value clamps to the 1 ms floor — the call must not trap; whether the - // subprocess beats the 1 ms deadline is timing-dependent, so only no-crash is asserted. - _ = try await CustomScriptLyricsDataSourceImpl.executeProcess( - executable: "/bin/echo", arguments: ["ok"], environment: [:], timeoutMs: -5) - } - @Test("$LYRA_CONFIG_DIR / ${LYRA_CACHE_DIR} placeholders expand in fallback_command elements") func placeholdersExpandInFallbackCommand() async { let captured = CapturedInvocation() @@ -237,111 +223,33 @@ struct CustomScriptLyricsDataSourceImplTests { #expect(result == nil) } - // MARK: - Real executeProcess (regression guard for the b09c1da waitUntilExit() hang fix) + // MARK: - Live processRunner wiring // - // Every test above stubs `processRunner`, so none of them ever runs the real - // `executeProcess` static function — the exact code whose `waitUntilExit()` was - // replaced with `terminationHandler` + a 3rd DispatchGroup pair after repeated - // short-lived invocations were empirically observed to hang (#308 review). These - // two tests spawn real subprocesses through `executeProcess` directly so a future - // refactor that reintroduces a blocking wait is caught by the suite. - - @Test("executeProcess spawns a real subprocess repeatedly without hanging") - func executeProcessRealSubprocessDoesNotHang() async throws { - let iterations = 50 - let start = ContinuousClock.now - - for i in 0.. '\(pidFile)'; trap '' TERM; sleep 30"], - environment: [:], - timeoutMs: 200 - ) - #expect(result.status == -1) - - let pidString = - (try? String(contentsOfFile: pidFile, encoding: .utf8))? - .trimmingCharacters(in: .whitespacesAndNewlines) ?? "" - let pid = Int32(pidString) ?? 0 - #expect(pid > 0, "the script should have recorded its pid before ignoring SIGTERM") - - // After the escalation reaps it, kill(pid, 0) fails (ESRCH). Poll rather than - // sleep a fixed interval — the reap is asynchronous. - let deadline = ContinuousClock.now + .seconds(3) - while kill(pid, 0) == 0, ContinuousClock.now < deadline { - try? await Task.sleep(for: .milliseconds(20)) - } - #expect(kill(pid, 0) != 0, "a SIGTERM-ignoring script must be SIGKILLed, not left running") + // echo's non-JSON output fails validation, so get() returns nil … + #expect(result == nil) + // … but the spy proves init()/resolvedCacheDir()/the closure resolved the command, + // appended title+artist, and forwarded the configured timeout. + #expect(await spy.lastCall?.executable == "/bin/echo") + #expect(await spy.lastCall?.arguments == ["unused", "Song", "Artist"]) + #expect(await spy.lastCall?.timeoutMs == 5000) } // MARK: - Config hot reload (#41) @@ -495,3 +403,30 @@ private actor CapturedInvocation { self.timeoutMs = timeoutMs } } + +/// Records the single call the no-arg init's live `processRunner` makes into the +/// injected `ProcessExecutor`, and returns a canned result. Used to prove config → +/// executor wiring without spawning a real subprocess. +private actor SpyProcessExecutor: ProcessExecutor { + struct Call { + let executable: String + let arguments: [String] + let environment: [String: String] + let timeoutMs: Double? + } + + private(set) var lastCall: Call? + private let result: (status: Int32, stdout: String, stderr: String) + + init(result: (status: Int32, stdout: String, stderr: String)) { + self.result = result + } + + func run( + executable: String, arguments: [String], environment: [String: String], timeoutMs: Double? + ) async throws -> (status: Int32, stdout: String, stderr: String) { + lastCall = Call( + executable: executable, arguments: arguments, environment: environment, timeoutMs: timeoutMs) + return result + } +} diff --git a/Tests/MediaRemoteDataSourceTests/MediaRemoteDataSourceImplTests.swift b/Tests/MediaRemoteDataSourceTests/MediaRemoteDataSourceImplTests.swift index f3c17ab9..05e4ad50 100644 --- a/Tests/MediaRemoteDataSourceTests/MediaRemoteDataSourceImplTests.swift +++ b/Tests/MediaRemoteDataSourceTests/MediaRemoteDataSourceImplTests.swift @@ -461,6 +461,9 @@ private final class StreamingGateway: ProcessGateway, @unchecked Sendable { func runInteractiveShell(_ command: String) -> Int32 { 0 } func runCapturingOutput(executable: String, arguments: [String]) -> String? { nil } + func runProcess(executable: String, arguments: [String], environment: [String: String]) async throws -> ( + status: Int32, stdout: String, stderr: String + ) { fatalError("unused") } func runStreaming(executable: String, arguments: [String]) -> AsyncStream { let lines: [String] = lock.withLock { runStreamingCallCount += 1 diff --git a/Tests/ProcessExecutorTests/FakeProcessGateway.swift b/Tests/ProcessExecutorTests/FakeProcessGateway.swift new file mode 100644 index 00000000..b252b0e4 --- /dev/null +++ b/Tests/ProcessExecutorTests/FakeProcessGateway.swift @@ -0,0 +1,72 @@ +import Domain + +struct FakeLaunchError: Error {} + +/// Fake `ProcessGateway` for exercising `ProcessExecutorImpl` in isolation. Only +/// `runProcess` carries behavior; the rest of the wide gateway surface is unused +/// here and fatal-errors if ever touched. +final class FakeProcessGateway: ProcessGateway, @unchecked Sendable { + enum Behavior { + /// Never completes until the surrounding task is cancelled, then propagates + /// `CancellationError` — mirrors the live gateway killing a hung child when + /// the executor's timeout fires. + case hang + /// Completes immediately with a fixed result. + case returns((status: Int32, stdout: String, stderr: String)) + /// Throws immediately, as the live gateway does when the executable can't launch. + case throwsLaunchError + } + + let behavior: Behavior + private let recorded = RecordedCall() + init(_ behavior: Behavior) { self.behavior = behavior } + + /// The arguments the most recent `runProcess` was called with (for pass-through assertions). + var lastCall: (executable: String, arguments: [String], environment: [String: String])? { + recorded.value + } + + func runProcess( + executable: String, arguments: [String], environment: [String: String] + ) async throws -> (status: Int32, stdout: String, stderr: String) { + recorded.value = (executable, arguments, environment) + switch behavior { + case .returns(let result): + return result + case .throwsLaunchError: + throw FakeLaunchError() + case .hang: + try await withTaskCancellationHandler { + while true { + try Task.checkCancellation() + await Task.yield() + } + } onCancel: { + } + // Unreachable — the loop only exits by throwing on cancellation. + return (status: 0, stdout: "", stderr: "") + } + } + + // MARK: Unused gateway surface + + var resourceSnapshot: ResourceSnapshot { .init(cpuUser: 0, cpuSystem: 0, peakRSS: 0, currentRSS: 0) } + var overlayPIDs: [Int32] { [] } + func spawnDaemon(executablePath: String) -> Int32? { fatalError("unused") } + func sendSignal(_ pid: Int32, signal: Int32) -> Bool { fatalError("unused") } + func isRunning(_ pid: Int32) -> Bool { fatalError("unused") } + func acquireLock() -> Bool { fatalError("unused") } + var isLocked: Bool { fatalError("unused") } + func releaseLock() { fatalError("unused") } + func runLaunchctl(_ arguments: [String]) -> Int32 { fatalError("unused") } + func findExecutable(_ name: String) -> String? { fatalError("unused") } + func run(executable: String, arguments: [String]) -> Int32 { fatalError("unused") } + func runInteractiveShell(_ command: String) -> Int32 { fatalError("unused") } + func runCapturingOutput(executable: String, arguments: [String]) -> String? { fatalError("unused") } + func runStreaming(executable: String, arguments: [String]) -> AsyncStream { fatalError("unused") } +} + +/// Lock-free single-writer/single-reader box (`@unchecked` on the enclosing class). +private final class RecordedCall: @unchecked Sendable { + var value: (executable: String, arguments: [String], environment: [String: String])? +} diff --git a/Tests/ProcessExecutorTests/ProcessExecutorImplTests.swift b/Tests/ProcessExecutorTests/ProcessExecutorImplTests.swift new file mode 100644 index 00000000..862d20e8 --- /dev/null +++ b/Tests/ProcessExecutorTests/ProcessExecutorImplTests.swift @@ -0,0 +1,106 @@ +import Dependencies +import Domain +import Testing + +@testable import ProcessExecutor + +@Suite("ProcessExecutorImpl") +struct ProcessExecutorImplTests { + // MARK: - Timeout (the #340 raison d'être) + + @Test("times out deterministically when the child never completes — no real waiting") + func timesOutOnHungChild() async throws { + let gateway = FakeProcessGateway(.hang) + let executor = withDependencies { + // ImmediateClock resolves the timeout sleep instantly, so the timeout + // branch fires with zero wall-clock — the deterministic replacement for + // the pre-#340 "run a real subprocess and measure elapsed" oracle. + $0.continuousClock = ImmediateClock() + $0.processGateway = gateway + } operation: { + ProcessExecutorImpl() + } + + let result = try await executor.run( + executable: "/bin/sh", arguments: ["-c", "sleep 10"], environment: [:], timeoutMs: 100) + + #expect(result.status == -1) + #expect(result.stdout.isEmpty) + #expect(result.stderr.contains("timed out")) + } + + @Test("returns the child's result when it completes before the timeout elapses") + func returnsResultWhenChildCompletes() async throws { + let gateway = FakeProcessGateway(.returns((status: 0, stdout: "hello", stderr: ""))) + let executor = withDependencies { + // TestClock is never advanced, so the timeout sleep stays pending forever — + // the completed child always wins the race. Deterministic, zero wall-clock. + $0.continuousClock = TestClock() + $0.processGateway = gateway + } operation: { + ProcessExecutorImpl() + } + + let result = try await executor.run( + executable: "/bin/echo", arguments: ["hello"], environment: [:], timeoutMs: 5000) + + #expect(result.status == 0) + #expect(result.stdout == "hello") + } + + // MARK: - No timeout (long-lived tools) + + @Test("nil timeout runs the child to completion and passes arguments straight through") + func nilTimeoutPassesThrough() async throws { + let gateway = FakeProcessGateway(.returns((status: 0, stdout: "done", stderr: ""))) + let executor = withDependencies { + $0.processGateway = gateway + } operation: { + ProcessExecutorImpl() + } + + let result = try await executor.run( + executable: "/usr/bin/yt-dlp", arguments: ["-o", "out.mp4"], environment: ["A": "b"], + timeoutMs: nil) + + #expect(result.status == 0) + #expect(result.stdout == "done") + #expect(gateway.lastCall?.executable == "/usr/bin/yt-dlp") + #expect(gateway.lastCall?.arguments == ["-o", "out.mp4"]) + #expect(gateway.lastCall?.environment == ["A": "b"]) + } + + @Test("a non-finite timeout is treated as no timeout — never traps on Int(Double)") + func nonFiniteTimeoutDoesNotTrap() async throws { + let gateway = FakeProcessGateway(.returns((status: 0, stdout: "ok", stderr: ""))) + let executor = withDependencies { + $0.processGateway = gateway + } operation: { + ProcessExecutorImpl() + } + + let result = try await executor.run( + executable: "/bin/echo", arguments: [], environment: [:], timeoutMs: .nan) + + #expect(result.status == 0) + #expect(result.stdout == "ok") + } + + // MARK: - Launch failure + + @Test("propagates the gateway's launch error instead of swallowing it") + func propagatesLaunchError() async { + let gateway = FakeProcessGateway(.throwsLaunchError) + let executor = withDependencies { + $0.continuousClock = TestClock() + $0.processGateway = gateway + } operation: { + ProcessExecutorImpl() + } + + await #expect(throws: FakeLaunchError.self) { + try await executor.run( + executable: "/nope", arguments: [], environment: [:], timeoutMs: 5000) + } + } +} diff --git a/Tests/ProcessHandlerTests/ProcessHandlerImplTests.swift b/Tests/ProcessHandlerTests/ProcessHandlerImplTests.swift index 8a317392..c8c9cacf 100644 --- a/Tests/ProcessHandlerTests/ProcessHandlerImplTests.swift +++ b/Tests/ProcessHandlerTests/ProcessHandlerImplTests.swift @@ -273,6 +273,9 @@ private struct StubProcessGateway: ProcessGateway { func run(executable: String, arguments: [String]) -> Int32 { 0 } func runInteractiveShell(_ command: String) -> Int32 { 0 } func runCapturingOutput(executable: String, arguments: [String]) -> String? { nil } + func runProcess(executable: String, arguments: [String], environment: [String: String]) async throws -> ( + status: Int32, stdout: String, stderr: String + ) { fatalError("unused") } func runStreaming(executable: String, arguments: [String]) -> AsyncStream { AsyncStream { $0.finish() } } @@ -329,6 +332,9 @@ private final class SpyProcessGateway: ProcessGateway, @unchecked Sendable { func run(executable: String, arguments: [String]) -> Int32 { 0 } func runInteractiveShell(_ command: String) -> Int32 { 0 } func runCapturingOutput(executable: String, arguments: [String]) -> String? { nil } + func runProcess(executable: String, arguments: [String], environment: [String: String]) async throws -> ( + status: Int32, stdout: String, stderr: String + ) { fatalError("unused") } func runStreaming(executable: String, arguments: [String]) -> AsyncStream { AsyncStream { $0.finish() } } @@ -392,6 +398,9 @@ private final class RestartSuccessProcessGateway: ProcessGateway, @unchecked Sen func run(executable: String, arguments: [String]) -> Int32 { 0 } func runInteractiveShell(_ command: String) -> Int32 { 0 } func runCapturingOutput(executable: String, arguments: [String]) -> String? { nil } + func runProcess(executable: String, arguments: [String], environment: [String: String]) async throws -> ( + status: Int32, stdout: String, stderr: String + ) { fatalError("unused") } func runStreaming(executable: String, arguments: [String]) -> AsyncStream { AsyncStream { $0.finish() } } diff --git a/Tests/ServiceHandlerTests/ServiceHandlerImplTests.swift b/Tests/ServiceHandlerTests/ServiceHandlerImplTests.swift index 3e6ce5b6..b8ab5a05 100644 --- a/Tests/ServiceHandlerTests/ServiceHandlerImplTests.swift +++ b/Tests/ServiceHandlerTests/ServiceHandlerImplTests.swift @@ -20,6 +20,9 @@ private struct StubGateway: ProcessGateway { func run(executable: String, arguments: [String]) -> Int32 { 0 } func runInteractiveShell(_ command: String) -> Int32 { 0 } func runCapturingOutput(executable: String, arguments: [String]) -> String? { nil } + func runProcess(executable: String, arguments: [String], environment: [String: String]) async throws -> ( + status: Int32, stdout: String, stderr: String + ) { fatalError("unused") } func runStreaming(executable: String, arguments: [String]) -> AsyncStream { AsyncStream { $0.finish() } } diff --git a/Tests/WallpaperDataSourceTests/YouTubeToolDetectionTests.swift b/Tests/WallpaperDataSourceTests/YouTubeToolDetectionTests.swift index b314113e..5b73a7fb 100644 --- a/Tests/WallpaperDataSourceTests/YouTubeToolDetectionTests.swift +++ b/Tests/WallpaperDataSourceTests/YouTubeToolDetectionTests.swift @@ -44,6 +44,9 @@ private struct LiveGateway: ProcessGateway { return String(data: pipe.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8)? .trimmingCharacters(in: .whitespacesAndNewlines) } + func runProcess(executable: String, arguments: [String], environment: [String: String]) async throws -> ( + status: Int32, stdout: String, stderr: String + ) { fatalError("unused") } func runStreaming(executable: String, arguments: [String]) -> AsyncStream { AsyncStream { $0.finish() } } diff --git a/Tests/WallpaperDataSourceTests/YouTubeWallpaperResolveTests.swift b/Tests/WallpaperDataSourceTests/YouTubeWallpaperResolveTests.swift index 32aae9e4..e35aa880 100644 --- a/Tests/WallpaperDataSourceTests/YouTubeWallpaperResolveTests.swift +++ b/Tests/WallpaperDataSourceTests/YouTubeWallpaperResolveTests.swift @@ -239,7 +239,14 @@ struct YouTubeWallpaperResolveTests { @Test("public init helper closures execute successfully") func publicInitHelpers() async throws { - let dataSource = YouTubeWallpaperDataSourceImpl() + // The live processRunner closure now delegates to a ProcessExecutor captured at + // init (#340), so construct the DataSource inside the override to prove the no-arg + // init wired the closure — without spawning a real subprocess. + let dataSource = withDependencies { + $0.processExecutor = StubProcessExecutor(result: (status: 0, stdout: "", stderr: "")) + } operation: { + YouTubeWallpaperDataSourceImpl() + } let fm = FileManager.default let tempDir = fm.temporaryDirectory.appendingPathComponent(UUID().uuidString) try fm.createDirectory(at: tempDir, withIntermediateDirectories: true) @@ -267,27 +274,10 @@ struct YouTubeWallpaperResolveTests { #expect(fm.fileExists(atPath: originalPath)) } - @Test("executeProcess captures status, stdout, and stderr") - func executeProcessCapturesStreams() async throws { - let result = try await YouTubeWallpaperDataSourceImpl.executeProcess( - executablePath: "/bin/sh", - arguments: ["-c", "echo hello; echo boom 1>&2; exit 7"] - ) - - #expect(result.status == 7) - #expect(result.stdout == "hello") - #expect(result.stderr == "boom") - } - - @Test("executeProcess throws when executable cannot be launched") - func executeProcessThrows() async { - await #expect(throws: (any Error).self) { - _ = try await YouTubeWallpaperDataSourceImpl.executeProcess( - executablePath: "/definitely/missing/executable", - arguments: [] - ) - } - } + // The real-subprocess coverage (status/stdout/stderr capture, launch failure) that + // used to exercise YouTube's own `executeProcess` static now lives on the shared + // primitive `DarwinGateway.runProcess` (#340); the resolve tests below still stub the + // `processRunner` seam, so they are unaffected by the consolidation. @Test("error descriptions include contextual details") func errorDescriptions() { @@ -408,11 +398,23 @@ private struct StubGateway: ProcessGateway { func run(executable: String, arguments: [String]) -> Int32 { 0 } func runInteractiveShell(_ command: String) -> Int32 { 0 } func runCapturingOutput(executable: String, arguments: [String]) -> String? { nil } + func runProcess(executable: String, arguments: [String], environment: [String: String]) async throws -> ( + status: Int32, stdout: String, stderr: String + ) { fatalError("unused") } func runStreaming(executable: String, arguments: [String]) -> AsyncStream { AsyncStream { continuation in continuation.finish() } } } +private struct StubProcessExecutor: ProcessExecutor { + let result: (status: Int32, stdout: String, stderr: String) + func run( + executable: String, arguments: [String], environment: [String: String], timeoutMs: Double? + ) async throws -> (status: Int32, stdout: String, stderr: String) { + result + } +} + private actor ProcessRunner { typealias ResultTuple = (status: Int32, stdout: String, stderr: String) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 4b15307d..55739b98 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -148,6 +148,7 @@ graph LR FrequencyAnalyzer[FrequencyAnalyzer] FileWatchGateway[FileWatchGateway] DeveloperLog[DeveloperLog] + ProcessExecutor[ProcessExecutor] end subgraph DataStore @@ -179,6 +180,9 @@ graph LR WallpaperRepository -.-> WallpaperDataSource MediaRemoteDataSource -.-> DarwinGateway WallpaperDataSource -.-> DarwinGateway + LyricsDataSource -.-> ProcessExecutor + WallpaperDataSource -.-> ProcessExecutor + ProcessExecutor -.-> DarwinGateway ConfigDataSource -.-> FileWatchGateway AudioTapDataSource -.-> CoreAudioTapGateway @@ -212,6 +216,7 @@ graph LR style FrequencyAnalyzer fill:#d57,stroke:#333,color:#fff style FileWatchGateway fill:#d57,stroke:#333,color:#fff style DeveloperLog fill:#d57,stroke:#333,color:#fff + style ProcessExecutor fill:#d57,stroke:#333,color:#fff style SQLiteDataStore fill:#a75,stroke:#333,color:#fff ``` @@ -244,7 +249,7 @@ Presenters subscribe to Interactors via Combine. Interactors access UseCases via | View | `Views` | SwiftUI views + `AppWindow` (NSWindow subclass). Feature dirs: `Header/`, `Lyrics/`, `Ripple/`, `Overlay/`, `Shared/` | | Presenter | `Presenters` | `Track/` (Header, Lyrics), `Wallpaper/` (Wallpaper, Ripple), `App/` (AppPresenter), `Config/` (`ConfigStatusPresenter`, #41). DecodeEffect engine, RippleState | | Handler | `ProcessHandler`, `VersionHandler`, `ServiceHandler`, `HealthHandler`, `TrackHandler`, `ConfigHandler`, `BenchmarkHandler` | CLI command logic. ProcessHandler: process lifecycle. VersionHandler: version string. ServiceHandler: LaunchAgent install/uninstall. HealthHandler: connectivity checks. TrackHandler: now-playing info with metadata/lyrics resolution. ConfigHandler: config template/init/path resolution. BenchmarkHandler: CPU/memory measurement via `ProcessGateway`. Protocols in Domain, injected via `@Dependency`. All handlers return `Result` — never throw | -| Provider / Support | `AppKitScreenProvider`, `StandardOutput`, `DarwinGateway`, `CoreAudioTapGateway`, `FrequencyAnalyzer`, `FileWatchGateway`, `DeveloperLog` | Platform/provider implementations that do not fit the core Clean Architecture layers directly. `AppKitScreenProvider` adapts `NSScreen` into `ScreenProvider`; `StandardOutput` owns CLI output rendering; `DarwinGateway` owns macOS process/system calls; `CoreAudioTapGateway` owns the live CoreAudio process-tap calls (`AudioTapGateway` implementation); `FrequencyAnalyzer` owns the vDSP FFT to per-bar magnitude conversion (pure computation behind Domain's `FrequencyAnalyzing` / `FrequencyAnalyzerFactory`); `FileWatchGateway` owns the DispatchSource-based config watch — directory + file two-tier (`ConfigWatchGateway` implementation, consumed by `ConfigDataSource`, #41); `DeveloperLog` (`FileDeveloperLog`) is a general write-only config-gated diagnostic sink (StandardOutput family, not a DataStore), injected as a cross-cutting service — the lyrics-resolution trace (#331) is its first instance | +| Provider / Support | `AppKitScreenProvider`, `StandardOutput`, `DarwinGateway`, `CoreAudioTapGateway`, `FrequencyAnalyzer`, `FileWatchGateway`, `DeveloperLog` | Platform/provider implementations that do not fit the core Clean Architecture layers directly. `AppKitScreenProvider` adapts `NSScreen` into `ScreenProvider`; `StandardOutput` owns CLI output rendering; `DarwinGateway` owns macOS process/system calls; `CoreAudioTapGateway` owns the live CoreAudio process-tap calls (`AudioTapGateway` implementation); `FrequencyAnalyzer` owns the vDSP FFT to per-bar magnitude conversion (pure computation behind Domain's `FrequencyAnalyzing` / `FrequencyAnalyzerFactory`); `FileWatchGateway` owns the DispatchSource-based config watch — directory + file two-tier (`ConfigWatchGateway` implementation, consumed by `ConfigDataSource`, #41); `DeveloperLog` (`FileDeveloperLog`) is a general write-only config-gated diagnostic sink (StandardOutput family, not a DataStore), injected as a cross-cutting service — the lyrics-resolution trace (#331) is its first instance; `ProcessExecutor` (`ProcessExecutorImpl`) is the DataSource-tier collaborator that adds a clock-driven timeout + capture over `ProcessGateway.runProcess`, shared by the lyrics and YouTube DataSources so neither carries its own subprocess plumbing (#340) | | Interactor | `TrackInteractor`, `ScreenInteractor`, `WallpaperInteractor`, `SpectrumInteractor`, `ConfigInteractor` | Combine-based reactive pipelines over UseCases (GUI) | | DI Wiring | `DependencyInjection` | All liveValue registrations, FontMetrics, HealthCheck | | Entity | `Entity` | Pure data types, zero external dependencies | @@ -295,6 +300,8 @@ graph TD **ProcessGateway OS boundary**: `ProcessGateway` centralizes OS-bound work in Domain (resource sampling, process management, lock files, launchctl, executable discovery, streaming subprocesses). `DarwinGateway` is the live macOS implementation, and `DependencyInjection` wires it into handlers and data sources so application logic no longer reaches directly into `Process`, `flock`, `getrusage`, or `which`. +**ProcessExecutor two-seam split for a testable timeout (#340)**: Running a user script with a timeout has two responsibilities that must be split so the timeout is testable. The **OS primitive** is `ProcessGateway.runProcess(executable:arguments:environment:)` (live in `DarwinGateway`): it spawns the child, drains stdout/stderr *without* blocking a thread-pool thread (`readabilityHandler`, not `readDataToEndOfFile` — the old blocking drain parked two pool threads per call for the child's whole lifetime and was the pool-exhaustion cause), and on task **cancellation** terminates the child (SIGTERM → SIGKILL after a 500 ms grace, targeting the direct pid) and resumes *immediately* with `CancellationError` — it does **not** wait for the pipes to EOF, so a SIGTERM-ignoring script whose orphaned grandchild holds the pipe open cannot stall the return. The **timeout race** lives one layer up in `ProcessExecutor` (`ProcessExecutorImpl`, its own covered module): it races `@Dependency(\.continuousClock).sleep(timeoutMs)` against `gateway.runProcess(...)` and, on timeout, cancels the run task (which kills the child) and returns `(-1, "", "timed out …")`; `timeoutMs == nil` disables the race for long-lived tools (yt-dlp/ffmpeg). This split is what makes the oracle deterministic: the pre-#340 tests could only assert a timeout by spawning a real subprocess and measuring wall-clock, so the pass/fail hinged on CI scheduling and flaked. Now `ProcessExecutorImplTests` drives the race with a fake hung gateway + `ImmediateClock`/`TestClock` (zero real time), and `DarwinGatewayRunProcessTests` keeps only thin real-subprocess smokes with **no** timing oracle. Both `CustomScriptLyricsDataSourceImpl` and `YouTubeWallpaperDataSourceImpl` now delegate their live `processRunner` to `@Dependency(\.processExecutor)` (captured at init so a construction-time override sticks), removing two duplicated `executeProcess` statics — including the residual `waitUntilExit()` on the YouTube side that #308 had removed from the lyrics side. `ProcessExecutor` is a DataSource-tier collaborator over the gateway, not an upper layer reaching down, so the adjacent-layer rule (`architecture-boundaries.md`) holds: DataSource → ProcessExecutor → ProcessGateway. + **AudioTapGateway CoreAudio boundary (#313)**: `AudioTapGateway` (Domain) wraps the imperative CoreAudio calls of the process-tap capture chain (tap → aggregate device → IOProc), and `CoreAudioTapGateway` is the live 1:1 pass-through implementation — fully symmetric to `ProcessGateway`/`DarwinGateway`. The protocol signature is deliberately CoreAudio-shaped (`CATapDescription`, `AudioStreamBasicDescription`, `AudioDeviceIOBlock`): type-erasing those would force per-callback conversion — allocation on the RT-safe IOProc path. Domain's `import CoreAudio` follows the same contract-layer exception as the Interactor protocols' Combine import. A GitHub survey (2026-07) found no maintained SPM library wrapping the macOS 14.4+ process-tap API (`SimplyCoreAudio` is stale and pre-dates it; `AudioCap` is sample code), so absorbing CoreAudio behind a third-party wrapper was rejected; if one matures later, this Gateway protocol is the single swap point. Rule of thumb for future gateways: protocol in Domain (platform-typed signatures allowed when the boundary's shape is the contract), live implementation in its own Support module, `liveValue` in `DependencyInjection/GatewayRegistration.swift`. The same shape covers non-gateway Support modules too: `FrequencyAnalyzer` is consumed through Domain's `FrequencyAnalyzing` protocol, built via the injected `FrequencyAnalyzerFactory` (a factory because the analyzer is rebuilt at runtime — bar count follows the overlay width, sample rate follows the tap — and the UseCase memoizes across rebuilds). **Analyzer memoization is UseCase-private state, not a DataStore (#313)**: `SpectrumUseCaseImpl` keeps the built analyzer in a private var keyed on `(bars, sampleRate)`. Modeling this as an in-memory DataStore was considered and rejected: the DataStore layer caches *domain data* — Entity values a Repository can read back (`MetadataDataStore.read(title:artist:)`) — whereas this memo reuses a *computational resource* (vDSP FFT setup) that is single-slot, instance-scoped, and main-thread-confined (the documented reason the class is `@unchecked Sendable`). Moving it behind a shared injected store would weaken the thread-confinement story and add indirection with no testability gain (rebuild-on-change is already covered by tests). Boundary for future work: if spectrum *results* (per-track bar data, resolved values) ever need caching across consumers, that IS domain data — put it in a DataStore behind the Repository, per the MetadataRepository precedent. From fb4006d331a6362c6f50427137f10f22c2a672c3 Mon Sep 17 00:00:00 2001 From: GeneralD Date: Tue, 21 Jul 2026 22:09:33 +0900 Subject: [PATCH 2/9] chore(#340): bump version to 2.28.1 --- Sources/VersionHandler/Resources/version.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/VersionHandler/Resources/version.txt b/Sources/VersionHandler/Resources/version.txt index 90efbd4e..9738a24f 100644 --- a/Sources/VersionHandler/Resources/version.txt +++ b/Sources/VersionHandler/Resources/version.txt @@ -1 +1 @@ -2.28.0 +2.28.1 From e30ae140b0b8c1f9e62701c74069fe2a2c920cde Mon Sep 17 00:00:00 2001 From: YUMENOSUKE Date: Wed, 22 Jul 2026 13:40:40 +0900 Subject: [PATCH 3/9] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../DarwinGatewayRunProcessTests.swift | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/Tests/DarwinGatewayTests/DarwinGatewayRunProcessTests.swift b/Tests/DarwinGatewayTests/DarwinGatewayRunProcessTests.swift index 654bc37f..86bd9894 100644 --- a/Tests/DarwinGatewayTests/DarwinGatewayRunProcessTests.swift +++ b/Tests/DarwinGatewayTests/DarwinGatewayRunProcessTests.swift @@ -58,13 +58,21 @@ struct DarwinGatewayRunProcessTests { @Test("cancelling the task terminates the child and throws instead of waiting it out") func cancellationTerminatesChild() async throws { + let pidFile = NSTemporaryDirectory() + "lyra-runproc-\(UUID().uuidString).pid" + defer { try? FileManager.default.removeItem(atPath: pidFile) } + let task = Task { try await gateway.runProcess( - executable: "/bin/sh", arguments: ["-c", "sleep 5"], environment: [:]) + executable: "/bin/sh", + arguments: ["-c", "echo $$ > '\(pidFile)'; sleep 5"], + environment: [:]) + } + + let deadline = ContinuousClock.now + .seconds(1) + while !FileManager.default.fileExists(atPath: pidFile), ContinuousClock.now < deadline { + try? await Task.sleep(for: .milliseconds(10)) } - // Let the child actually spawn before cancelling (real-process smoke, not a - // state-polling assertion — a short spawn delay is acceptable here). - try await Task.sleep(for: .milliseconds(50)) + task.cancel() await #expect(throws: CancellationError.self) { try await task.value } } From 84c9c723c72f32f84c85329c1bf8e9a317681f23 Mon Sep 17 00:00:00 2001 From: YUMENOSUKE Date: Wed, 22 Jul 2026 13:41:24 +0900 Subject: [PATCH 4/9] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../DarwinGatewayRunProcessTests.swift | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/Tests/DarwinGatewayTests/DarwinGatewayRunProcessTests.swift b/Tests/DarwinGatewayTests/DarwinGatewayRunProcessTests.swift index 86bd9894..dd0592e9 100644 --- a/Tests/DarwinGatewayTests/DarwinGatewayRunProcessTests.swift +++ b/Tests/DarwinGatewayTests/DarwinGatewayRunProcessTests.swift @@ -80,17 +80,24 @@ struct DarwinGatewayRunProcessTests { @Test("cancellation SIGKILLs a child that ignores SIGTERM") func cancellationForceKillsStubbornChild() async throws { let pidFile = NSTemporaryDirectory() + "lyra-runproc-\(UUID().uuidString).pid" - defer { try? FileManager.default.removeItem(atPath: pidFile) } + let readyFile = NSTemporaryDirectory() + "lyra-runproc-\(UUID().uuidString).ready" + defer { + try? FileManager.default.removeItem(atPath: pidFile) + try? FileManager.default.removeItem(atPath: readyFile) + } - // The child records its own pid, traps (ignores) SIGTERM, then sleeps far past any - // reasonable window — only the SIGTERM→SIGKILL escalation can stop it. + // The child records its own pid, traps (ignores) SIGTERM, then signals readiness. let task = Task { try await gateway.runProcess( executable: "/bin/sh", - arguments: ["-c", "echo $$ > '\(pidFile)'; trap '' TERM; sleep 3"], + arguments: ["-c", "echo $$ > '\(pidFile)'; trap '' TERM; echo ready > '\(readyFile)'; sleep 3"], environment: [:]) } - try await Task.sleep(for: .milliseconds(100)) // let it record its pid + install the trap + + let deadline = ContinuousClock.now + .seconds(1) + while !FileManager.default.fileExists(atPath: readyFile), ContinuousClock.now < deadline { + try? await Task.sleep(for: .milliseconds(10)) + } task.cancel() await #expect(throws: CancellationError.self) { try await task.value } From c3449e5bf3e820a81559fbb1ffe866ad93b7603f Mon Sep 17 00:00:00 2001 From: GeneralD Date: Wed, 22 Jul 2026 13:45:45 +0900 Subject: [PATCH 5/9] =?UTF-8?q?test(#340):=20suggestion=20=E3=81=AE?= =?UTF-8?q?=E3=83=9D=E3=83=BC=E3=83=AA=E3=83=B3=E3=82=B0=E3=82=92=E8=A3=9C?= =?UTF-8?q?=E4=BF=AE=20=E2=80=94=20deadline=20=E5=86=8D=E5=AE=A3=E8=A8=80?= =?UTF-8?q?=E3=82=92=E8=A7=A3=E6=B6=88=E3=81=97=E7=8C=B6=E4=BA=88=E3=82=92?= =?UTF-8?q?=203s=20=E3=81=AB=E7=B5=B1=E4=B8=80?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot suggestion (84c9c72/e30ae14) が同一関数内で deadline を再宣言して コンパイルエラーになっていたため readyDeadline に改名。あわせて spawn 待ち の猶予をリポジトリ内の他ポーリングと同じ 3s に揃え、低速 CI での誤カットを防ぐ。 --- Tests/DarwinGatewayTests/DarwinGatewayRunProcessTests.swift | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Tests/DarwinGatewayTests/DarwinGatewayRunProcessTests.swift b/Tests/DarwinGatewayTests/DarwinGatewayRunProcessTests.swift index dd0592e9..dd0d123e 100644 --- a/Tests/DarwinGatewayTests/DarwinGatewayRunProcessTests.swift +++ b/Tests/DarwinGatewayTests/DarwinGatewayRunProcessTests.swift @@ -68,7 +68,7 @@ struct DarwinGatewayRunProcessTests { environment: [:]) } - let deadline = ContinuousClock.now + .seconds(1) + let deadline = ContinuousClock.now + .seconds(3) while !FileManager.default.fileExists(atPath: pidFile), ContinuousClock.now < deadline { try? await Task.sleep(for: .milliseconds(10)) } @@ -94,8 +94,8 @@ struct DarwinGatewayRunProcessTests { environment: [:]) } - let deadline = ContinuousClock.now + .seconds(1) - while !FileManager.default.fileExists(atPath: readyFile), ContinuousClock.now < deadline { + let readyDeadline = ContinuousClock.now + .seconds(3) + while !FileManager.default.fileExists(atPath: readyFile), ContinuousClock.now < readyDeadline { try? await Task.sleep(for: .milliseconds(10)) } task.cancel() From 16218afcdda2f51915a75e15775c3ad88a01c786 Mon Sep 17 00:00:00 2001 From: GeneralD Date: Wed, 22 Jul 2026 13:45:45 +0900 Subject: [PATCH 6/9] =?UTF-8?q?fix(#340):=20=E9=9D=9E=20nil=20=E3=81=AE?= =?UTF-8?q?=E4=B8=8D=E6=AD=A3=20timeout=20=E3=82=92=E7=84=A1=E5=8A=B9?= =?UTF-8?q?=E5=8C=96=E3=81=A7=E3=81=AF=E3=81=AA=E3=81=8F=E3=82=AF=E3=83=A9?= =?UTF-8?q?=E3=83=B3=E3=83=97=20(PR#341=20Codex=20P2)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit timeout_ms = 0 / 負値 / NaN / inf が「timeout 無効」に落ちる回帰を修正。 旧 executeProcess の契約(有限値は 1ms…1h にクランプ、非有限は 5s デフォルト) を復元し、timeout 無効化は nil のみに限定。設定ミスや壊れた設定値で 歌詞解決が永久にブロックされることはなくなる。 TDD: ImmediateClock + hang fake で 4 値をパラメタライズし、現行実装が 永久待ちになる RED を確認してからクランプを実装。 --- Sources/Domain/Misc/ProcessExecutor.swift | 8 +++--- .../ProcessExecutor/ProcessExecutorImpl.swift | 14 +++++----- .../ProcessExecutorImplTests.swift | 26 ++++++++++++++----- 3 files changed, 33 insertions(+), 15 deletions(-) diff --git a/Sources/Domain/Misc/ProcessExecutor.swift b/Sources/Domain/Misc/ProcessExecutor.swift index bc74435c..f2737545 100644 --- a/Sources/Domain/Misc/ProcessExecutor.swift +++ b/Sources/Domain/Misc/ProcessExecutor.swift @@ -16,9 +16,11 @@ import Dependencies public protocol ProcessExecutor: Sendable { /// - Parameters: /// - timeoutMs: milliseconds before the child is killed and a - /// `(-1, "", "timed out after …ms")` result returned. `nil` (or a - /// non-finite / ≤ 0 value) disables the timeout — used for long-lived, - /// low-frequency tools like yt-dlp / ffmpeg, which must run to completion. + /// `(-1, "", "timed out after …ms")` result returned. Only `nil` disables + /// the timeout — used for long-lived, low-frequency tools like yt-dlp / + /// ffmpeg, which must run to completion. Non-nil values are normalized: + /// finite ones clamp to 1 ms … 1 h, non-finite ones fall back to a 5 s + /// default — a configured timeout can never disable itself by accident. /// - Returns: the child's exit status plus trimmed stdout/stderr. func run( executable: String, arguments: [String], environment: [String: String], timeoutMs: Double? diff --git a/Sources/ProcessExecutor/ProcessExecutorImpl.swift b/Sources/ProcessExecutor/ProcessExecutorImpl.swift index e45df1ec..23b66fe6 100644 --- a/Sources/ProcessExecutor/ProcessExecutorImpl.swift +++ b/Sources/ProcessExecutor/ProcessExecutorImpl.swift @@ -14,16 +14,18 @@ extension ProcessExecutorImpl: ProcessExecutor { ) async throws -> (status: Int32, stdout: String, stderr: String) { let gateway = self.gateway - // No timeout: run to completion. `nil`, non-finite, and ≤ 0 all mean "don't - // race a timer" — used for long-lived tools (yt-dlp/ffmpeg) that must finish. - guard let timeoutMs, timeoutMs.isFinite, timeoutMs > 0 else { + // No timeout: run to completion. Only `nil` means "don't race a timer" — used + // for long-lived tools (yt-dlp/ffmpeg) that must finish. + guard let timeoutMs else { return try await gateway.runProcess( executable: executable, arguments: arguments, environment: environment) } - // Clamp to a finite sane window (1 ms … 1 h) before `Int(Double)`, which traps - // on NaN/±inf/out-of-range — a pathological config value must not crash the daemon. - let clampedMs = Int(min(max(timeoutMs, 1), 3_600_000)) + // A non-nil but invalid value clamps instead of disabling: a config typo like + // `timeout_ms = 0` (or NaN/±inf) must still bound the child — the pre-#340 + // contract. Finite values clamp to a sane window (1 ms … 1 h), non-finite ones + // fall back to the 5 s default; both keep `Int(Double)` from trapping. + let clampedMs = timeoutMs.isFinite ? Int(min(max(timeoutMs, 1), 3_600_000)) : 5000 let clock = self.clock return try await withThrowingTaskGroup(of: Outcome.self) { group in diff --git a/Tests/ProcessExecutorTests/ProcessExecutorImplTests.swift b/Tests/ProcessExecutorTests/ProcessExecutorImplTests.swift index 862d20e8..9c9d2ea6 100644 --- a/Tests/ProcessExecutorTests/ProcessExecutorImplTests.swift +++ b/Tests/ProcessExecutorTests/ProcessExecutorImplTests.swift @@ -70,20 +70,34 @@ struct ProcessExecutorImplTests { #expect(gateway.lastCall?.environment == ["A": "b"]) } - @Test("a non-finite timeout is treated as no timeout — never traps on Int(Double)") - func nonFiniteTimeoutDoesNotTrap() async throws { - let gateway = FakeProcessGateway(.returns((status: 0, stdout: "ok", stderr: ""))) + // MARK: - Invalid non-nil timeouts + + @Test( + "a non-nil but invalid timeout clamps instead of disabling — a bad config can't hang forever", + arguments: [ + (timeoutMs: 0.0, expected: "timed out after 1ms"), + (timeoutMs: -5.0, expected: "timed out after 1ms"), + (timeoutMs: Double.nan, expected: "timed out after 5000ms"), + (timeoutMs: Double.infinity, expected: "timed out after 5000ms"), + ]) + func invalidTimeoutClamps(timeoutMs: Double, expected: String) async throws { + let gateway = FakeProcessGateway(.hang) let executor = withDependencies { + $0.continuousClock = ImmediateClock() $0.processGateway = gateway } operation: { ProcessExecutorImpl() } + // Only `nil` may disable the timeout: a configured `timeout_ms` of 0, a negative + // value, or NaN/±inf must still bound the child (pre-#340 contract, #341 review). + // The clamp also keeps `Int(Double)` from trapping on the non-finite values. let result = try await executor.run( - executable: "/bin/echo", arguments: [], environment: [:], timeoutMs: .nan) + executable: "/bin/sh", arguments: ["-c", "sleep 10"], environment: [:], + timeoutMs: timeoutMs) - #expect(result.status == 0) - #expect(result.stdout == "ok") + #expect(result.status == -1) + #expect(result.stderr.contains(expected)) } // MARK: - Launch failure From d74949a5c52b7521eda061eed1e126e86defdeee Mon Sep 17 00:00:00 2001 From: GeneralD Date: Wed, 22 Jul 2026 13:45:45 +0900 Subject: [PATCH 7/9] =?UTF-8?q?fix(#340):=20SIGKILL=20=E3=82=A8=E3=82=B9?= =?UTF-8?q?=E3=82=AB=E3=83=AC=E3=83=BC=E3=82=B7=E3=83=A7=E3=83=B3=E3=82=92?= =?UTF-8?q?=20Process=20=E3=81=AE=20identity=20=E3=81=A7=E3=82=B2=E3=83=BC?= =?UTF-8?q?=E3=83=88=20(PR#341=20Codex=20P1)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 500ms 猶予後の kill(pid, 0) 判定は、子が即終了して pid が別プロセスに 再利用された場合に無関係なプロセスを SIGKILL しうる。この Process インスタンス自身の isRunning(true の間は未 reap で pid は必ずこの子)で SIGTERM/SIGKILL 双方をゲートし、猶予中はインスタンスを retain して Foundation の reap 監視も維持する。RunProcessState は pid でなく Process を保持する形に変更。 --- .../DarwinGateway+RunProcess.swift | 57 ++++++++++--------- 1 file changed, 31 insertions(+), 26 deletions(-) diff --git a/Sources/DarwinGateway/DarwinGateway+RunProcess.swift b/Sources/DarwinGateway/DarwinGateway+RunProcess.swift index 5c4de457..607e1440 100644 --- a/Sources/DarwinGateway/DarwinGateway+RunProcess.swift +++ b/Sources/DarwinGateway/DarwinGateway+RunProcess.swift @@ -58,9 +58,9 @@ extension DarwinGateway { } // Close the launch/cancel race: if cancellation requested termination before - // we had a pid, kill the child now that it is running. - if let pid = state.markLaunched(process.processIdentifier) { - terminateProcess(pid) + // the child existed, kill it now that it is running. + if let killTarget = state.markLaunched(process) { + terminateProcess(killTarget) } group.notify(queue: .global()) { @@ -74,8 +74,8 @@ extension DarwinGateway { // the pipe open long past the kill, and the executor's timeout must return // promptly regardless (#340). The child is signalled here; the leaked drain // finishes and is ignored (resume-once) whenever the grandchild finally exits. - let (killPid, continuation) = state.requestCancel() - if let killPid { terminateProcess(killPid) } + let (killTarget, continuation) = state.requestCancel() + if let killTarget { terminateProcess(killTarget) } continuation?.resume(throwing: CancellationError()) } } @@ -98,13 +98,19 @@ private func drainPipe( } /// SIGTERM, then SIGKILL after a short grace if the child ignores the polite signal — -/// so a script trapping SIGTERM cannot outlive the executor's timeout. Targets the -/// specific pid (never the process group) so lyra itself is never at risk; a shell -/// script's own grandchildren are outside this guarantee. -private func terminateProcess(_ pid: Int32) { - kill(pid, SIGTERM) +/// so a script trapping SIGTERM cannot outlive the executor's timeout. Both signals are +/// gated on this `Process` instance's own `isRunning` rather than `kill(pid, 0)`: while +/// it is true the child is unreaped and the pid is still ours, so a pid recycled to an +/// unrelated process after a prompt exit can never be targeted. The captured instance +/// also keeps Foundation's reaping alive through the grace window. Targets the specific +/// pid (never the process group) so lyra itself is never at risk; a shell script's own +/// grandchildren are outside this guarantee. +private func terminateProcess(_ process: Process) { + guard process.isRunning else { return } + kill(process.processIdentifier, SIGTERM) DispatchQueue.global().asyncAfter(deadline: .now() + .milliseconds(500)) { - if kill(pid, 0) == 0 { kill(pid, SIGKILL) } + guard process.isRunning else { return } + kill(process.processIdentifier, SIGKILL) } } @@ -117,9 +123,8 @@ private final class RunProcessState: @unchecked Sendable { private let lock = OSAllocatedUnfairLock() private var continuation: Continuation? private var resumed = false - private var launched = false private var terminateRequested = false - private var pid: Int32? + private var launched: Process? private var stdout = Data() private var stderr = Data() @@ -137,27 +142,27 @@ private final class RunProcessState: @unchecked Sendable { var stdoutTrimmed: String { lock.withLock { Self.trimmed(stdout) } } var stderrTrimmed: String { lock.withLock { Self.trimmed(stderr) } } - /// Marks the child launched, recording its pid. Returns the pid iff a termination was - /// already requested (cancel raced ahead of the pid) so the caller kills it now. - func markLaunched(_ newPid: Int32) -> Int32? { + /// Marks the child launched, recording the `Process` instance (not just its pid, so + /// a later kill can verify identity via `isRunning`). Returns it iff a termination + /// was already requested (cancel raced ahead of the launch) so the caller kills now. + func markLaunched(_ newProcess: Process) -> Process? { lock.withLock { - launched = true - pid = newPid - return terminateRequested ? newPid : nil + launched = newProcess + return terminateRequested ? newProcess : nil } } - /// Handles cancellation atomically: records the request, and returns the pid to kill - /// (if the child is already launched) plus the continuation to resume-throw (if it is - /// set and not yet resumed). The caller performs the kill and resume outside the lock. - func requestCancel() -> (killPid: Int32?, continuation: Continuation?) { + /// Handles cancellation atomically: records the request, and returns the `Process` + /// to kill (if the child is already launched) plus the continuation to resume-throw + /// (if it is set and not yet resumed). The caller performs both outside the lock. + func requestCancel() -> (killTarget: Process?, continuation: Continuation?) { lock.withLock { terminateRequested = true - let killPid = launched ? pid : nil - guard !resumed else { return (killPid, nil) } + let killTarget = launched + guard !resumed else { return (killTarget, nil) } resumed = true defer { continuation = nil } - return (killPid, continuation) + return (killTarget, continuation) } } From 55b03c1a5e849fe1175a5813293808556b21ad97 Mon Sep 17 00:00:00 2001 From: GeneralD Date: Wed, 22 Jul 2026 14:04:06 +0900 Subject: [PATCH 8/9] =?UTF-8?q?docs(#340):=20ARCHITECTURE.md=20=E3=81=AE?= =?UTF-8?q?=E5=9B=B3=E3=81=8B=E3=82=89=E6=BC=8F=E3=82=8C=E3=81=A6=E3=81=84?= =?UTF-8?q?=E3=81=9F=20Domain=20=E8=A6=81=E7=B4=A0=E3=82=92=E8=A3=9C?= =?UTF-8?q?=E5=AE=8C?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Domain の全 protocol と図・表を突き合わせて発覚した欠けを解消: - RandomSource: Support subgraph にノード追加(SystemRandomSource、Presenters が DecodeEffect / wallpaper shuffle で消費)。Layer Summary の Provider/Support 行にも ProcessExecutor とあわせて Modules 列へ追記 - WallpaperRepository -.-> SQLiteDataStore エッジ追加(WallpaperCacheStore / GRDBWallpaperCacheStore の消費が図に現れていなかった) - HealthHandler の孤立を解消: HealthCheckable 実装群(Lyrics/Metadata/Wallpaper DataSource + ConfigRepository)への点線エッジを追加 - FontMetricsProvider: 文書全体で言及ゼロだったため、DI Wiring 行に AppKitFontMetrics(Views の SwiftUIResolver が消費)と healthCheckers 集約を明記 - Layer Overview に Presenters -.-> Support 点線を追加(RandomSource 消費の反映) --- docs/ARCHITECTURE.md | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 55739b98..429ca6b9 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -62,7 +62,7 @@ graph TD Domain --> Entity CLI -.-> CommandHandler - Presenters -.-> Interactor + Presenters -.-> Interactor & Support CommandHandler -.-> UseCase & Support Interactor -.-> UseCase UseCase -.-> Repository & Support @@ -149,6 +149,7 @@ graph LR FileWatchGateway[FileWatchGateway] DeveloperLog[DeveloperLog] ProcessExecutor[ProcessExecutor] + RandomSource[RandomSource] end subgraph DataStore @@ -160,6 +161,7 @@ graph LR ProcessHandler -.-> DarwinGateway ServiceHandler -.-> DarwinGateway BenchmarkHandler -.-> DarwinGateway + HealthHandler -.-> LyricsDataSource & MetadataDataSource & WallpaperDataSource & ConfigRepository TrackInteractor -.-> PlaybackUseCase & MetadataUseCase & LyricsUseCase & ConfigUseCase ScreenInteractor -.-> ConfigUseCase WallpaperInteractor -.-> WallpaperUseCase & ConfigUseCase @@ -177,7 +179,7 @@ graph LR MetadataUseCase -.-> MetadataRepository MetadataRepository -.-> MetadataDataSource & SQLiteDataStore WallpaperUseCase -.-> WallpaperRepository - WallpaperRepository -.-> WallpaperDataSource + WallpaperRepository -.-> WallpaperDataSource & SQLiteDataStore MediaRemoteDataSource -.-> DarwinGateway WallpaperDataSource -.-> DarwinGateway LyricsDataSource -.-> ProcessExecutor @@ -217,6 +219,7 @@ graph LR style FileWatchGateway fill:#d57,stroke:#333,color:#fff style DeveloperLog fill:#d57,stroke:#333,color:#fff style ProcessExecutor fill:#d57,stroke:#333,color:#fff + style RandomSource fill:#d57,stroke:#333,color:#fff style SQLiteDataStore fill:#a75,stroke:#333,color:#fff ``` @@ -249,9 +252,9 @@ Presenters subscribe to Interactors via Combine. Interactors access UseCases via | View | `Views` | SwiftUI views + `AppWindow` (NSWindow subclass). Feature dirs: `Header/`, `Lyrics/`, `Ripple/`, `Overlay/`, `Shared/` | | Presenter | `Presenters` | `Track/` (Header, Lyrics), `Wallpaper/` (Wallpaper, Ripple), `App/` (AppPresenter), `Config/` (`ConfigStatusPresenter`, #41). DecodeEffect engine, RippleState | | Handler | `ProcessHandler`, `VersionHandler`, `ServiceHandler`, `HealthHandler`, `TrackHandler`, `ConfigHandler`, `BenchmarkHandler` | CLI command logic. ProcessHandler: process lifecycle. VersionHandler: version string. ServiceHandler: LaunchAgent install/uninstall. HealthHandler: connectivity checks. TrackHandler: now-playing info with metadata/lyrics resolution. ConfigHandler: config template/init/path resolution. BenchmarkHandler: CPU/memory measurement via `ProcessGateway`. Protocols in Domain, injected via `@Dependency`. All handlers return `Result` — never throw | -| Provider / Support | `AppKitScreenProvider`, `StandardOutput`, `DarwinGateway`, `CoreAudioTapGateway`, `FrequencyAnalyzer`, `FileWatchGateway`, `DeveloperLog` | Platform/provider implementations that do not fit the core Clean Architecture layers directly. `AppKitScreenProvider` adapts `NSScreen` into `ScreenProvider`; `StandardOutput` owns CLI output rendering; `DarwinGateway` owns macOS process/system calls; `CoreAudioTapGateway` owns the live CoreAudio process-tap calls (`AudioTapGateway` implementation); `FrequencyAnalyzer` owns the vDSP FFT to per-bar magnitude conversion (pure computation behind Domain's `FrequencyAnalyzing` / `FrequencyAnalyzerFactory`); `FileWatchGateway` owns the DispatchSource-based config watch — directory + file two-tier (`ConfigWatchGateway` implementation, consumed by `ConfigDataSource`, #41); `DeveloperLog` (`FileDeveloperLog`) is a general write-only config-gated diagnostic sink (StandardOutput family, not a DataStore), injected as a cross-cutting service — the lyrics-resolution trace (#331) is its first instance; `ProcessExecutor` (`ProcessExecutorImpl`) is the DataSource-tier collaborator that adds a clock-driven timeout + capture over `ProcessGateway.runProcess`, shared by the lyrics and YouTube DataSources so neither carries its own subprocess plumbing (#340) | +| Provider / Support | `AppKitScreenProvider`, `StandardOutput`, `DarwinGateway`, `CoreAudioTapGateway`, `FrequencyAnalyzer`, `FileWatchGateway`, `DeveloperLog`, `ProcessExecutor`, `RandomSource` | Platform/provider implementations that do not fit the core Clean Architecture layers directly. `AppKitScreenProvider` adapts `NSScreen` into `ScreenProvider`; `StandardOutput` owns CLI output rendering; `DarwinGateway` owns macOS process/system calls; `CoreAudioTapGateway` owns the live CoreAudio process-tap calls (`AudioTapGateway` implementation); `FrequencyAnalyzer` owns the vDSP FFT to per-bar magnitude conversion (pure computation behind Domain's `FrequencyAnalyzing` / `FrequencyAnalyzerFactory`); `FileWatchGateway` owns the DispatchSource-based config watch — directory + file two-tier (`ConfigWatchGateway` implementation, consumed by `ConfigDataSource`, #41); `DeveloperLog` (`FileDeveloperLog`) is a general write-only config-gated diagnostic sink (StandardOutput family, not a DataStore), injected as a cross-cutting service — the lyrics-resolution trace (#331) is its first instance; `ProcessExecutor` (`ProcessExecutorImpl`) is the DataSource-tier collaborator that adds a clock-driven timeout + capture over `ProcessGateway.runProcess`, shared by the lyrics and YouTube DataSources so neither carries its own subprocess plumbing (#340); `RandomSource` (`SystemRandomSource`) provides injectable randomness consumed by Presenters (`DecodeEffect`, wallpaper shuffle) so random behavior is deterministic under test | | Interactor | `TrackInteractor`, `ScreenInteractor`, `WallpaperInteractor`, `SpectrumInteractor`, `ConfigInteractor` | Combine-based reactive pipelines over UseCases (GUI) | -| DI Wiring | `DependencyInjection` | All liveValue registrations, FontMetrics, HealthCheck | +| DI Wiring | `DependencyInjection` | All liveValue registrations; also hosts two thin boundary pieces with no module of their own — `AppKitFontMetrics` (`FontMetricsProvider` live, consumed by Views' `SwiftUIResolver`) and the `healthCheckers` aggregation (the `HealthCheckable` implementations live beside their APIs in the DataSource/Repository modules) | | Entity | `Entity` | Pure data types, zero external dependencies | | Domain | `Domain` | Protocols, DependencyKeys (`@_exported import Entity`) | | UseCase | `ConfigUseCase`, `PlaybackUseCase`, `LyricsUseCase`, `MetadataUseCase`, `WallpaperUseCase`, `SpectrumUseCase` | Business logic only, no cross-UseCase deps | From b60f64d4643763c665a3d061fff3a61c46adcc8f Mon Sep 17 00:00:00 2001 From: GeneralD Date: Wed, 22 Jul 2026 14:27:17 +0900 Subject: [PATCH 9/9] =?UTF-8?q?test(#340):=20=E4=BA=8B=E5=89=8D=E3=82=AD?= =?UTF-8?q?=E3=83=A3=E3=83=B3=E3=82=BB=E3=83=AB=E6=B8=88=E3=81=BF=E5=91=BC?= =?UTF-8?q?=E3=81=B3=E5=87=BA=E3=81=97=E3=81=AE=20register=20=E3=82=AC?= =?UTF-8?q?=E3=83=BC=E3=83=89=E5=88=86=E5=B2=90=E3=82=92=E3=82=AB=E3=83=90?= =?UTF-8?q?=E3=83=BC?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit codecov patch レポートの未カバー 3 行のうち、決定的にテスト可能な 「呼び出し時点でキャンセル済み → spawn せず即 CancellationError」の ガード分岐(L29-30)にスモークを追加。キャンセル観測を spin で確定させて から runProcess に入るため、タイミング oracle なしで必ず同じ経路を踏む。 残る未カバーは launch と markLaunched の間にキャンセルが割り込む 数マイクロ秒のレース防御(L63-64)のみ。外部から決定的に踏める窓では ないため、テスト用フックを増やしてまでカバーせず未カバーのまま許容する。 --- .../DarwinGatewayRunProcessTests.swift | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/Tests/DarwinGatewayTests/DarwinGatewayRunProcessTests.swift b/Tests/DarwinGatewayTests/DarwinGatewayRunProcessTests.swift index dd0d123e..f8bdb6f0 100644 --- a/Tests/DarwinGatewayTests/DarwinGatewayRunProcessTests.swift +++ b/Tests/DarwinGatewayTests/DarwinGatewayRunProcessTests.swift @@ -77,6 +77,23 @@ struct DarwinGatewayRunProcessTests { await #expect(throws: CancellationError.self) { try await task.value } } + @Test("a call from an already-cancelled task throws immediately without spawning") + func preCancelledCallThrowsWithoutSpawning() async { + let marker = NSTemporaryDirectory() + "lyra-runproc-\(UUID().uuidString).marker" + defer { try? FileManager.default.removeItem(atPath: marker) } + + let task = Task { [gateway] in + // Hold until cancellation is definitely observed, so runProcess enters with the + // flag already set and takes the register-guard bail-out — no child is spawned. + while !Task.isCancelled { await Task.yield() } + return try await gateway.runProcess( + executable: "/bin/sh", arguments: ["-c", "echo x > '\(marker)'"], environment: [:]) + } + task.cancel() + await #expect(throws: CancellationError.self) { try await task.value } + #expect(!FileManager.default.fileExists(atPath: marker), "a pre-cancelled call must not spawn") + } + @Test("cancellation SIGKILLs a child that ignores SIGTERM") func cancellationForceKillsStubbornChild() async throws { let pidFile = NSTemporaryDirectory() + "lyra-runproc-\(UUID().uuidString).pid"