From 95391d7345911746f2e60f3b3bbff9a501d8346c Mon Sep 17 00:00:00 2001 From: andyhtran <76441965+andyhtran@users.noreply.github.com> Date: Wed, 22 Jul 2026 20:28:14 -0400 Subject: [PATCH] Make broadcast paste immune to a wedged Tailscale daemon MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Typing the path now happens the moment the compressed file exists, before peer discovery. The typed string never depended on which hosts are reachable, yet it waited on tailscale status --json — which hangs for tens of seconds when tailscaled is starved (UDP-blocked networks), so a burst of pastes flushed all at once when the daemon recovered. Three layers, so no single one is load-bearing: - Broadcast.handle types before discovery. Uploads already raced the typed path, so the ordering exposes no window that wasn't open before. - runShell gains a watchdog timeout (3s for status): a hung child gets SIGTERM and the result reports timedOut, failing open to "attempt all hosts, let SSH's ConnectTimeout filter". - TailscaleStatusCache (30s TTL, single-flight): a paste burst and every menu render ride one status subprocess instead of spawning their own. Settings' "Refresh peers" bypasses the TTL so the button stays honest. Co-Authored-By: Claude Fable 5 --- Sources/CopyCat/Broadcast.swift | 94 +++++++++++++++---- Sources/CopyCat/SettingsView.swift | 5 +- Sources/CopyCat/TailscaleDiscovery.swift | 68 +++++++++++++- Tests/CopyCatTests/ShellTests.swift | 28 ++++++ .../TailscaleStatusCacheTests.swift | 90 ++++++++++++++++++ 5 files changed, 263 insertions(+), 22 deletions(-) create mode 100644 Tests/CopyCatTests/TailscaleStatusCacheTests.swift 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) + } +}