From d1422b1ceeba61150aec5254de322aed9f3a5c65 Mon Sep 17 00:00:00 2001 From: Weekend Superhero <4048475+WeekendSuperhero@users.noreply.github.com> Date: Fri, 11 Sep 2026 03:19:28 -0700 Subject: [PATCH] swift: let watchIPNBus outlive a quiet tailnet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit watchIPNBus opens a long poll, but builds its request through basicAuthURLRequest, which returns URLSessionConfiguration.default. That config's timeoutIntervalForRequest is 60s, and it is measured from the LAST byte received rather than from the start of the request — so a watch over an idle tailnet is failed with NSURLErrorTimedOut roughly once a minute, forever. The consumer sees an error, re-arms, and the cycle repeats. doSimpleAPIRequest and doJSONAPIRequest already take `timeoutInterval: TimeInterval = 60` and set it on the request. The long poll — the one endpoint that genuinely has to outlast its own silence — is the one that never got the parameter. This adds it, defaulting to `LocalAPIClient.noTimeout` so the documented behaviour ("a single application-wide watcher will often suffice") is what you actually get. Worth noting that the doc comment's own advice works against it: it recommends always passing rateLimitNetmaps, whose job is to make the stream quieter, and therefore more likely to sit silent for 60s. URLSession has no sentinel for "never": a non-finite timeoutIntervalForRequest is not honoured and a non-positive one restores the 60s default, so noTimeout is a year — a span no watch will reach. Both the request and the session config are set, since URLRequest's own timeoutInterval overrides the session's for that task. Measured on an iOS 27 device build against a real tailnet, watching the loopback connection: every watch closed at 78.3s carrying ~24KB, or at 60.1s carrying 606 bytes once the initial netmap was dropped from the mask — in both shapes, exactly 60s after the last byte. With this change and no other, a four-minute soak produced one watch and zero timeouts. The new test asks for a 2s timeout and asserts NSURLErrorTimedOut reaches the consumer, which pins the plumbing end to end: the value has to survive basicAuthURLRequest, MessageProcessor.start and the URLSession that MessageReader actually builds. It deliberately does not test the default, because doing so would mean outsitting 60s of silence — which is exactly why this went unnoticed. Updates tailscale/tailscale#21220 Signed-off-by: Weekend Superhero <4048475+WeekendSuperhero@users.noreply.github.com> --- .../LocalAPI/LocalAPIClient.swift | 32 +++++++++++-- .../TailscaleKitTests.swift | 45 +++++++++++++++++++ 2 files changed, 73 insertions(+), 4 deletions(-) diff --git a/swift/TailscaleKit/LocalAPI/LocalAPIClient.swift b/swift/TailscaleKit/LocalAPI/LocalAPIClient.swift index 7133505..3d9f2cf 100644 --- a/swift/TailscaleKit/LocalAPI/LocalAPIClient.swift +++ b/swift/TailscaleKit/LocalAPI/LocalAPIClient.swift @@ -46,6 +46,12 @@ public actor LocalAPIClient { self.logger = logger } + /// Pass as `watchIPNBus(timeoutInterval:)` to keep a long poll open across an idle tailnet. + /// URLSession has no sentinel for "never": a non-finite timeoutIntervalForRequest is not + /// honoured, and a non-positive one restores the 60s default. So this is simply a span no + /// watch will reach. + public static let noTimeout: TimeInterval = 365 * 24 * 60 * 60 + // MARK: - IPN Bus @@ -60,13 +66,31 @@ public actor LocalAPIClient { /// - Parameters: /// - mask: a mask indicating the events we wish to observe /// - consumer: an actor implementing MessageConsumer to which incoming events will be sent + /// - timeoutInterval: how long the poll may go WITHOUT receiving data before URLSession fails it + /// with NSURLErrorTimedOut. This is not a budget for the whole watch — the + /// timer restarts on every byte. Defaults to `noTimeout`, because a watch + /// should outlive a quiet tailnet; pass a finite value to have the consumer's + /// error callback run on a schedule instead. /// - Returns: The MessageProcessor handling the incoming event stream. This should be destroyed/stopped when the caller /// wishes to unsubscribe from the event stream. - public func watchIPNBus(mask: Ipn.NotifyWatchOpt, consumer: MessageConsumer) async throws -> MessageProcessor { + public func watchIPNBus(mask: Ipn.NotifyWatchOpt, + consumer: MessageConsumer, + timeoutInterval: TimeInterval = LocalAPIClient.noTimeout) async throws -> MessageProcessor { let params = [URLQueryItem(name: "mask", value: String(mask.rawValue))] - let (request, sessionConfig) = try await self.basicAuthURLRequest(endpoint: .watchIPNBus, - method: .GET, - params: params) + var request: URLRequest + let sessionConfig: URLSessionConfiguration + (request, sessionConfig) = try await self.basicAuthURLRequest(endpoint: .watchIPNBus, + method: .GET, + params: params) + + // basicAuthURLRequest hands back URLSessionConfiguration.default, whose + // timeoutIntervalForRequest is 60s measured from the LAST byte received. The bus sends + // nothing while the tailnet is idle, so leaving that default in place fails a quiet watch + // with -1001 about once a minute, forever. doSimpleAPIRequest and doJSONAPIRequest below + // already take a timeoutInterval and set it on the request; the long poll — the one + // endpoint that genuinely needs to outlast its own silence — is the one that never did. + request.timeoutInterval = timeoutInterval + sessionConfig.timeoutIntervalForRequest = timeoutInterval let messageProcessor = await MessageProcessor(consumer: consumer, logger: logger) messageProcessor.start(request, config: sessionConfig) diff --git a/swift/TailscaleKitXCTests/TailscaleKitTests.swift b/swift/TailscaleKitXCTests/TailscaleKitTests.swift index 8f6333e..508c57f 100644 --- a/swift/TailscaleKitXCTests/TailscaleKitTests.swift +++ b/swift/TailscaleKitXCTests/TailscaleKitTests.swift @@ -182,6 +182,51 @@ final class TailscaleKitTests: XCTestCase { XCTFail(error.localizedDescription) } } + + /// The bus watch's request timeout is the interval it may go WITHOUT data, not a budget for + /// the whole watch, so the default has to outlive an idle tailnet. Asking for a short one + /// pins the plumbing: the timeout has to reach the request that MessageReader actually runs, + /// and land on the consumer as NSURLErrorTimedOut. A test for the DEFAULT would have to + /// outsit 60s of silence, which is why the regression went unnoticed. + func testWatchIPNBusHonoursItsTimeoutInterval() async throws { + let config = mockConfig() + let logger = BlackholeLogger() + + let ts1 = try TailscaleNode(config: config, logger: logger) + try await ts1.up() + + let timedOut = expectation(description: "the bus watch timed out") + let consumer = TimeoutObservingConsumer { + let err = $0 as NSError + if err.domain == NSURLErrorDomain && err.code == NSURLErrorTimedOut { + timedOut.fulfill() + } + } + + let api = LocalAPIClient(localNode: ts1, logger: logger) + let processor = try await api.watchIPNBus(mask: [.initialState, .noPrivateKeys], + consumer: consumer, + timeoutInterval: 2) + + await fulfillment(of: [timedOut], timeout: 20.0) + processor.cancel() + } +} + +/// Watches only for the bus's error callback. The notifications themselves are beside the +/// point here: the initial state lands immediately, and the test is about the silence after it. +actor TimeoutObservingConsumer: MessageConsumer { + private let onError: @Sendable (any Error) -> Void + + init(onError: @escaping @Sendable (any Error) -> Void) { + self.onError = onError + } + + func notify(_ notify: Ipn.Notify) {} + + func error(_ error: any Error) { + onError(error) + } }