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 <jackjackbits@users.noreply.github.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
jack
2026-07-07 18:09:30 +02:00
committed by GitHub
co-authored by jack Claude Fable 5
parent 28ffc53f3f
commit bbe5e1ef4e
3 changed files with 64 additions and 1 deletions
@@ -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) {
@@ -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() {
@@ -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()
}
}
}
}