diff --git a/Sources/CopyCat/Broadcast.swift b/Sources/CopyCat/Broadcast.swift index 3e22c89..77c44e9 100644 --- a/Sources/CopyCat/Broadcast.swift +++ b/Sources/CopyCat/Broadcast.swift @@ -41,6 +41,21 @@ enum Broadcast { let outPath = compressIfNeeded(src: pngPath, stamp: stamp, dir: cacheDir, log: Log.cmdOptV) let outName = outPath.lastPathComponent + // Type the moment the local file exists — before peer discovery. The + // typed string doesn't depend on which hosts are reachable, and the + // discovery call can hang for tens of seconds when tailscaled is + // starved (UDP-blocked networks); the user's paste must never wait on + // the network. Typing already races the uploads on the remote side, + // so this ordering widens no window that wasn't open before. + // + // Remote home dir is unknown, so always tilde-prefix. Trailing space + // matches the local-paste path so the user can keep typing. + let typed = "~/\(remoteCacheRel)/\(outName) " + DispatchQueue.main.async { + Typer.type(typed) + } + Log.cmdOptV.info("typed \(typed)") + let sizeBytes = (try? FileManager.default.attributesOfItem(atPath: outPath.path)[.size] as? Int) ?? 0 let sizeKB = sizeBytes / 1024 @@ -63,14 +78,6 @@ enum Broadcast { ) } - // Remote home dir is unknown, so always tilde-prefix. Trailing space - // matches the local-paste path so the user can keep typing. - let typed = "~/\(remoteCacheRel)/\(outName) " - DispatchQueue.main.async { - Typer.type(typed) - } - Log.cmdOptV.info("typed \(typed)") - BroadcastStatus.shared.recordRun(hosts: hosts) // Prune off the SSH-fanout path on a lower-priority queue so @@ -190,14 +197,49 @@ struct ShellResult: Sendable { let stdout: String let stderr: String let exitCode: Int32 + /// True when the watchdog killed the process for exceeding `timeout`. + /// `exitCode` is the signal number in that case, so `isSuccess` is + /// already false — this flag exists so callers can report "timed out" + /// instead of a misleading "exited non-zero". + let timedOut: Bool var isSuccess: Bool { exitCode == 0 } } +// Process isn't Sendable, but the watchdog only calls terminate(), which is +// safe from another thread: it forwards SIGTERM and is a no-op once the +// process has exited. +private final class ProcessTerminator: @unchecked Sendable { + private let process: Process + private let lock = NSLock() + private var fired = false + + init(_ process: Process) { self.process = process } + + func terminate() { + lock.lock() + fired = true + lock.unlock() + process.terminate() + } + + var didFire: Bool { + lock.lock() + defer { lock.unlock() } + return fired + } +} + /// Runs a process with stdin detached, capturing stdout and stderr separately /// so callers can distinguish "no output" from "failed silently". Returns /// `nil` only when the process couldn't be spawned at all (bad path, etc). -func runShell(_ exec: String, args: [String], env: [String: String]? = nil) -> ShellResult? { +/// +/// `timeout` bounds the total run: past it the process gets SIGTERM, the +/// pipe reads unblock on EOF, and the result comes back with `timedOut` set. +/// Without it a hung child blocks the calling thread indefinitely. +func runShell( + _ exec: String, args: [String], env: [String: String]? = nil, timeout: TimeInterval? = nil +) -> ShellResult? { let p = Process() p.executableURL = URL(fileURLWithPath: exec) p.arguments = args @@ -209,15 +251,33 @@ func runShell(_ exec: String, args: [String], env: [String: String]? = nil) -> S p.standardInput = FileHandle(forReadingAtPath: "/dev/null") do { try p.run() - let outData = outPipe.fileHandleForReading.readDataToEndOfFile() - let errData = errPipe.fileHandleForReading.readDataToEndOfFile() - p.waitUntilExit() - return ShellResult( - stdout: String(data: outData, encoding: .utf8) ?? "", - stderr: String(data: errData, encoding: .utf8) ?? "", - exitCode: p.terminationStatus - ) } catch { return nil } + + var watchdog: DispatchWorkItem? + var terminator: ProcessTerminator? + if let timeout { + let t = ProcessTerminator(p) + let item = DispatchWorkItem { t.terminate() } + DispatchQueue.global(qos: .utility).asyncAfter(deadline: .now() + timeout, execute: item) + terminator = t + watchdog = item + } + + let outData = outPipe.fileHandleForReading.readDataToEndOfFile() + let errData = errPipe.fileHandleForReading.readDataToEndOfFile() + p.waitUntilExit() + watchdog?.cancel() + + // didFire alone can mislabel a natural exit: the watchdog may fire in the + // gap between exit and cancel. Requiring uncaughtSignal confirms the kill + // actually took the process down. + let timedOut = (terminator?.didFire ?? false) && p.terminationReason == .uncaughtSignal + return ShellResult( + stdout: String(data: outData, encoding: .utf8) ?? "", + stderr: String(data: errData, encoding: .utf8) ?? "", + exitCode: p.terminationStatus, + timedOut: timedOut + ) } diff --git a/Sources/CopyCat/SettingsView.swift b/Sources/CopyCat/SettingsView.swift index eb23b46..b216f3f 100644 --- a/Sources/CopyCat/SettingsView.swift +++ b/Sources/CopyCat/SettingsView.swift @@ -368,9 +368,10 @@ private struct HostsSettingsView: View { private func refreshPeers() { // tailscale status --json takes ~300-500ms; keep it off the main thread - // so opening the Settings window doesn't stall. + // so opening the Settings window doesn't stall. Uncached on purpose: + // this backs an explicit "Refresh peers" action. Task.detached(priority: .userInitiated) { - let fresh = TailscaleDiscovery.allPeers() + let fresh = TailscaleDiscovery.refreshPeers() await MainActor.run { peers = fresh } } } diff --git a/Sources/CopyCat/TailscaleDiscovery.swift b/Sources/CopyCat/TailscaleDiscovery.swift index fd80003..1c9a022 100644 --- a/Sources/CopyCat/TailscaleDiscovery.swift +++ b/Sources/CopyCat/TailscaleDiscovery.swift @@ -7,6 +7,44 @@ struct TailscalePeer: Identifiable, Hashable, Sendable { let os: String? } +// Single-flight TTL cache for the peer list. Callers sit on latency-critical +// paths — the paste pipeline and menu rendering — and each uncached lookup +// spawns a subprocess that can hang for tens of seconds when tailscaled is +// starved (UDP-blocked networks wedge its internals). The serial queue means +// a burst of concurrent callers rides one fetch: the first spawns it, the +// rest block until it lands and reuse the result via the TTL. +final class TailscaleStatusCache: @unchecked Sendable { + private let queue = DispatchQueue(label: "com.copycat.macos.tailscale-status") + private let ttl: TimeInterval + private var cached: [TailscalePeer]? + private var fetchedAt = Date.distantPast + + init(ttl: TimeInterval) { + self.ttl = ttl + } + + /// `maxAge` overrides the TTL for one call; 0 forces a fresh fetch. + /// Failures ([] from a timed-out or not-logged-in daemon) are cached + /// too — retrying a wedged daemon every paste is exactly the stall this + /// cache exists to prevent, and an empty list fails open (callers skip + /// filtering and attempt every host). + func peers( + maxAge: TimeInterval? = nil, + now: Date = Date(), + fetch: () -> [TailscalePeer] + ) -> [TailscalePeer] { + queue.sync { + if let cached, now.timeIntervalSince(fetchedAt) < maxAge ?? ttl { + return cached + } + let fresh = fetch() + cached = fresh + fetchedAt = now + return fresh + } + } +} + enum TailscaleDiscovery { private static let candidatePaths = [ "/Applications/Tailscale.app/Contents/MacOS/Tailscale", @@ -20,19 +58,43 @@ enum TailscaleDiscovery { static var isAvailable: Bool { executablePath != nil } - // Full peer list, online or not. Used by the Settings UI to suggest - // hosts the user can pick from. + // Normal `status --json` answers from local daemon state in well under a + // second; only a wedged daemon blows past this. On timeout the peer list + // comes back empty, which fails open: callers attempt every configured + // host and SSH's ConnectTimeout does the filtering. + private static let statusTimeout: TimeInterval = 3 + + // TTL trades staleness for latency: a peer that changed state within the + // window is misjudged for at most 30s, and the SSH timeout already covers + // attempting a host that just went offline. + private static let cache = TailscaleStatusCache(ttl: 30) + + /// Cached peer list for latency-sensitive callers (paste path, menu). static func allPeers() -> [TailscalePeer] { + cache.peers(fetch: fetchPeers) + } + + /// Uncached fetch for the Settings "Refresh peers" button — an explicit + /// refresh returning 30s-old data would make the button a no-op. + static func refreshPeers() -> [TailscalePeer] { + cache.peers(maxAge: 0, fetch: fetchPeers) + } + + private static func fetchPeers() -> [TailscalePeer] { guard let bin = executablePath else { return [] } // TAILSCALE_BE_CLI=1: when the bundled Tailscale binary is invoked from // a notarized app's subprocess (no TTY), it relaunches the GUI instead // of running as CLI. The env var forces CLI mode. See // tailscale/tailscale#16063 and #7140. let env = ["TAILSCALE_BE_CLI": "1", "PATH": "/usr/bin:/bin"] - guard let result = runShell(bin, args: ["status", "--json"], env: env) else { + guard let result = runShell(bin, args: ["status", "--json"], env: env, timeout: statusTimeout) else { Log.app.error("tailscale status: spawn failed for \(bin)") return [] } + guard !result.timedOut else { + Log.app.error("tailscale status timed out after \(Int(statusTimeout))s — daemon unresponsive; treating peers as unknown") + return [] + } guard result.isSuccess else { // Common when the user has Tailscale installed but isn't logged in // — info, not error, so it doesn't spam the console on every peer diff --git a/Tests/CopyCatTests/ShellTests.swift b/Tests/CopyCatTests/ShellTests.swift index db80c42..971a5ff 100644 --- a/Tests/CopyCatTests/ShellTests.swift +++ b/Tests/CopyCatTests/ShellTests.swift @@ -39,4 +39,32 @@ final class ShellTests: XCTestCase { XCTAssertTrue(result.stdout.isEmpty) XCTAssertTrue(result.stderr.contains("only-stderr")) } + + // MARK: - Timeout + + func testTimeoutKillsHungProcess() throws { + let start = Date() + let result = try XCTUnwrap(runShell("/bin/sleep", args: ["30"], timeout: 0.3)) + XCTAssertTrue(result.timedOut) + XCTAssertFalse(result.isSuccess) + // Well under the sleep duration proves the watchdog, not the child, + // ended the run. Generous bound so a loaded CI box doesn't flake. + XCTAssertLessThan(Date().timeIntervalSince(start), 5) + } + + func testFastProcessDoesNotTimeOut() throws { + let result = try XCTUnwrap(runShell("/bin/echo", args: ["hi"], timeout: 5)) + XCTAssertFalse(result.timedOut) + XCTAssertTrue(result.isSuccess) + XCTAssertEqual(result.stdout.trimmingCharacters(in: .whitespacesAndNewlines), "hi") + } + + func testNoTimeoutParameterMeansUnbounded() throws { + // Pin the default: omitting timeout must not kill even a slow-ish + // child. 1s keeps the suite fast while still outliving any plausible + // accidental internal deadline. + let result = try XCTUnwrap(runShell("/bin/sleep", args: ["1"])) + XCTAssertFalse(result.timedOut) + XCTAssertTrue(result.isSuccess) + } } diff --git a/Tests/CopyCatTests/TailscaleStatusCacheTests.swift b/Tests/CopyCatTests/TailscaleStatusCacheTests.swift new file mode 100644 index 0000000..8ba8fe0 --- /dev/null +++ b/Tests/CopyCatTests/TailscaleStatusCacheTests.swift @@ -0,0 +1,90 @@ +import XCTest +@testable import CopyCat + +final class TailscaleStatusCacheTests: XCTestCase { + private func peer(_ name: String) -> TailscalePeer { + TailscalePeer(hostname: name, online: true, os: nil) + } + + // Thread-safe call counter for the concurrency test; a captured `var` + // can't be mutated from concurrently-executing code under Swift 6. + private final class Counter: @unchecked Sendable { + private let lock = NSLock() + private var value = 0 + func increment() { + lock.lock() + value += 1 + lock.unlock() + } + var count: Int { + lock.lock() + defer { lock.unlock() } + return value + } + } + + func testFirstCallFetches() { + let cache = TailscaleStatusCache(ttl: 30) + let result = cache.peers { [peer("a")] } + XCTAssertEqual(result.map(\.hostname), ["a"]) + } + + func testServesCachedWithinTTL() { + let cache = TailscaleStatusCache(ttl: 30) + let t0 = Date() + var calls = 0 + _ = cache.peers(now: t0) { calls += 1; return [self.peer("a")] } + let second = cache.peers(now: t0.addingTimeInterval(29)) { calls += 1; return [self.peer("b")] } + XCTAssertEqual(calls, 1) + XCTAssertEqual(second.map(\.hostname), ["a"]) + } + + func testRefetchesPastTTL() { + let cache = TailscaleStatusCache(ttl: 30) + let t0 = Date() + var calls = 0 + _ = cache.peers(now: t0) { calls += 1; return [self.peer("a")] } + let second = cache.peers(now: t0.addingTimeInterval(31)) { calls += 1; return [self.peer("b")] } + XCTAssertEqual(calls, 2) + XCTAssertEqual(second.map(\.hostname), ["b"]) + } + + func testMaxAgeZeroForcesRefetch() { + let cache = TailscaleStatusCache(ttl: 30) + let t0 = Date() + var calls = 0 + _ = cache.peers(now: t0) { calls += 1; return [self.peer("a")] } + let second = cache.peers(maxAge: 0, now: t0) { calls += 1; return [self.peer("b")] } + XCTAssertEqual(calls, 2) + XCTAssertEqual(second.map(\.hostname), ["b"]) + } + + // Failure results must be cached like any other: refetching a wedged + // daemon on every call is the stall the cache exists to prevent. + func testEmptyResultIsCached() { + let cache = TailscaleStatusCache(ttl: 30) + let t0 = Date() + var calls = 0 + _ = cache.peers(now: t0) { calls += 1; return [] } + let second = cache.peers(now: t0.addingTimeInterval(5)) { calls += 1; return [self.peer("b")] } + XCTAssertEqual(calls, 1) + XCTAssertEqual(second, []) + } + + // A burst of concurrent callers (one per rapid ⌘⌥V press) must ride a + // single fetch: the first spawns it, the rest block and reuse. Each + // caller's default `now` predates the fetch's completion, so the TTL + // check passes for all of them once the result lands. + func testBurstCallersShareOneFetch() { + let cache = TailscaleStatusCache(ttl: 30) + let counter = Counter() + DispatchQueue.concurrentPerform(iterations: 5) { _ in + _ = cache.peers { + counter.increment() + Thread.sleep(forTimeInterval: 0.1) + return [] + } + } + XCTAssertEqual(counter.count, 1) + } +}