From bbe5e1ef4e8ff61a23536063c2d93cb23c0c48fe Mon Sep 17 00:00:00 2001 From: jack <212554440+jackjackbits@users.noreply.github.com> Date: Tue, 7 Jul 2026 18:09:30 +0200 Subject: [PATCH] Fix reachability-gate races flagged by Codex on #1389 (#1394) P1: TorManager.shutdownCompletely() resets didStart asynchronously (after Arti has actually stopped, up to ~5s later). A brief offline->online flap could call startIfNeeded() inside that window; the guard on didStart dropped it, and nothing reevaluated afterwards, so Tor stayed down while activationAllowed was true. Track shutdowns in flight and record a deferred start, honored when the last shutdown finishes (still gated on allowAutoStart/foreground at that point). P2: NWPathReachabilityMonitor.ingest() cancelled and rescheduled the flush a full debounce interval from "now" on every observation, even duplicates (e.g. interface detail changes while still unsatisfied). ReachabilityDebounce already preserves the original pending.since, so schedule the flush for the remaining time to the true deadline instead of restarting the window. Tests: debounce deadline preservation (pure) + a monitor-level timing test that a mid-window duplicate does not postpone the offline commit. Co-authored-by: jack Co-authored-by: Claude Fable 5 --- .../Services/NetworkReachabilityMonitor.swift | 14 +++++++- .../NetworkReachabilityGateTests.swift | 33 +++++++++++++++++++ localPackages/Arti/Sources/TorManager.swift | 18 ++++++++++ 3 files changed, 64 insertions(+), 1 deletion(-) diff --git a/bitchat/Services/NetworkReachabilityMonitor.swift b/bitchat/Services/NetworkReachabilityMonitor.swift index bc2b77cf..53bb0677 100644 --- a/bitchat/Services/NetworkReachabilityMonitor.swift +++ b/bitchat/Services/NetworkReachabilityMonitor.swift @@ -48,6 +48,15 @@ struct ReachabilityDebounce { /// Whether a change is currently waiting out the debounce window. var hasPendingChange: Bool { pending != nil } + /// Time left before the pending change may commit, or `nil` when nothing + /// is pending. Lets callers schedule a flush at the true deadline instead + /// of a full interval from "now" (duplicate observations must not push + /// the deadline out). + func pendingRemaining(at now: Date) -> TimeInterval? { + guard let pending else { return nil } + return max(0, interval - now.timeIntervalSince(pending.since)) + } + /// Feed a raw observation. Returns the new committed value if it changed, /// otherwise `nil`. mutating func observe(reachable: Bool, at now: Date) -> Bool? { @@ -157,7 +166,10 @@ final class NWPathReachabilityMonitor: NetworkReachabilityMonitoring { } } flushWorkItem = work - DispatchQueue.main.asyncAfter(deadline: .now() + debounce.interval, execute: work) + // Fire at the pending change's real deadline (pending.since + interval): + // duplicate path updates re-enter here and must not restart the window. + let delay = debounce.pendingRemaining(at: now()) ?? debounce.interval + DispatchQueue.main.asyncAfter(deadline: .now() + delay, execute: work) } private func publish(_ reachable: Bool) { diff --git a/bitchatTests/Services/NetworkReachabilityGateTests.swift b/bitchatTests/Services/NetworkReachabilityGateTests.swift index 420a54db..fee973aa 100644 --- a/bitchatTests/Services/NetworkReachabilityGateTests.swift +++ b/bitchatTests/Services/NetworkReachabilityGateTests.swift @@ -57,6 +57,39 @@ final class NetworkReachabilityGateTests: XCTestCase { XCTAssertTrue(d.committed) } + func test_debounce_duplicateObservationsPreservePendingDeadline() { + var d = ReachabilityDebounce(interval: 2.5, initial: true) + let t0 = Date() + XCTAssertNil(d.observe(reachable: false, at: t0)) + // Duplicate unsatisfied updates mid-window keep the original deadline. + XCTAssertNil(d.observe(reachable: false, at: t0.addingTimeInterval(1.0))) + XCTAssertEqual(d.pendingRemaining(at: t0.addingTimeInterval(1.0)), 1.5) + // A duplicate arriving past the deadline commits immediately. + XCTAssertEqual(d.observe(reachable: false, at: t0.addingTimeInterval(2.5)), false) + XCTAssertNil(d.pendingRemaining(at: t0.addingTimeInterval(2.5))) + } + + func test_monitor_duplicateUpdatesDoNotPostponeOfflineCommit() async { + let monitor = NWPathReachabilityMonitor(debounceInterval: 1.0) + var received: [Bool] = [] + let cancellable = monitor.reachabilityPublisher.sink { received.append($0) } + defer { cancellable.cancel() } + + let start = Date() + monitor.ingest(reachable: false) + try? await Task.sleep(nanoseconds: 500_000_000) + // Duplicate unsatisfied update mid-window (e.g. interface detail change + // while still offline) must not restart the debounce window. + monitor.ingest(reachable: false) + + let committed = await waitUntil(timeout: 2.0) { !received.isEmpty } + XCTAssertTrue(committed) + XCTAssertEqual(received, [false]) + // The flush must fire at the original ~1.0s deadline, not ~1.5s + // (a full interval after the duplicate). + XCTAssertLessThan(Date().timeIntervalSince(start), 1.4) + } + // MARK: - Service gating func test_start_whenUnreachable_suppressesTorAndRelays() { diff --git a/localPackages/Arti/Sources/TorManager.swift b/localPackages/Arti/Sources/TorManager.swift index cd831ba5..2e252fad 100644 --- a/localPackages/Arti/Sources/TorManager.swift +++ b/localPackages/Arti/Sources/TorManager.swift @@ -75,6 +75,11 @@ public final class TorManager: ObservableObject { } private var didStart = false + // shutdownCompletely() resets `didStart` asynchronously (after Arti has + // actually stopped). A startIfNeeded() arriving in that window must not be + // dropped — it is recorded here and honored when the shutdown finishes. + private var shutdownsInFlight = 0 + private var startPendingAfterShutdown = false private var bootstrapMonitorStarted = false private var pathMonitor: NWPathMonitor? private var isAppForeground: Bool = true @@ -90,6 +95,11 @@ public final class TorManager: ObservableObject { public func startIfNeeded() { guard allowAutoStart else { return } guard isAppForeground else { return } + if shutdownsInFlight > 0 { + SecureLogger.debug("TorManager: startIfNeeded() deferred - shutdown in flight", category: .session) + startPendingAfterShutdown = true + return + } guard !didStart else { return } didStart = true isDormant = false @@ -329,6 +339,8 @@ public final class TorManager: ObservableObject { public func shutdownCompletely() { SecureLogger.debug("TorManager: shutdownCompletely() called", category: .session) + startPendingAfterShutdown = false + shutdownsInFlight += 1 Task.detached { [weak self] in guard let self = self else { return } _ = arti_stop() @@ -352,6 +364,12 @@ public final class TorManager: ObservableObject { self.bootstrapMonitorStarted = false // Note: Don't clear startedAt here - it will be set fresh on next startIfNeeded() // Clearing it here races with startup and defeats the grace period + self.shutdownsInFlight -= 1 + if self.shutdownsInFlight == 0 && self.startPendingAfterShutdown { + self.startPendingAfterShutdown = false + SecureLogger.debug("TorManager: honoring start deferred during shutdown", category: .session) + self.startIfNeeded() + } } } }