From 7649925f0b8fc08dbdd262b763d973170fcbd2c8 Mon Sep 17 00:00:00 2001 From: Tim Morgan Date: Fri, 31 Jul 2026 14:11:48 -0700 Subject: [PATCH] Stop reporting Aviation Weather outages as app defects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SF50-TOLD-2F reported a gzip failure with dataSize 0 on the TAF cache: aviationweather.gov serves an empty cache file while regenerating it, so the app tried to gunzip nothing. SF50-TOLD-2G is the same shape with a 502 from the CDN on the winds-aloft endpoint. Neither is a defect here, yet both reached Sentry as warnings. An upstream 500 is not automatically retryable. That the AWC answers one at all is a flaw on their end, and working around a third party's flawed behavior with a backoff loop is not this app's job. So the app reports nothing to Sentry for third-party failures and leaves the retry decision to the pilot. - Reject an empty body at the fetch site as Errors.emptyResponse rather than letting it reach the decompressor, so "the server sent nothing" stays distinguishable from "the body arrived corrupt". A corrupt non-empty body is still reported: that would be a real contract break. - Add isUpstreamServerFailure (5xx and empty bodies) and fold it together with the existing cancellation and URLError sets into shouldReport, the single gate all three feeds and WeatherViewModel's conditions stream now consult. The conditions stream re-reports whatever the loader surfaces, so widening only the loader would have left the duplicate report that PR #9 fixed for URLErrors. - Collapse the three duplicated catch-block classifications into recordLoadFailure, preserving the existing Sentry tags and fingerprints so grouping is unaffected. The pilot's recourse needed no new code. The weather row stays a navigation link in the error state, and WeatherSource already titles its button "Try Again" whenever an error is set, calling load(force: true) to bypass the 15 minute reload interval. That makes the existing recoverySuggestion — re-download later or enter weather manually — true. shouldReport is left untested: it is internal and only observable through Sentry, so pinning it would mean reaching through @testable, the white-box shape rejected in review in ef1fe35. Verified: swift format --strict and swiftlint --strict clean on changed files; SF50 TOLD builds for iOS 26 iPhone 17 Pro; SF50 Shared Unit Tests 304/304 pass; the Try Again control confirmed on an iOS 26 simulator in the weather error state. Fixes SF50-TOLD-2F Fixes SF50-TOLD-2G Co-Authored-By: Claude Opus 5 (1M context) --- SF50 Shared/Errors.swift | 2 + .../Weather/WeatherLoader+Loading.swift | 114 ++++++++++-------- SF50 Shared/Weather/WeatherLoader+Types.swift | 6 + SF50 Shared/Weather/WeatherViewModel.swift | 4 +- 4 files changed, 71 insertions(+), 55 deletions(-) diff --git a/SF50 Shared/Errors.swift b/SF50 Shared/Errors.swift index 6f5c405..06897c6 100644 --- a/SF50 Shared/Errors.swift +++ b/SF50 Shared/Errors.swift @@ -9,6 +9,8 @@ extension WeatherLoader.Errors: LocalizedError { switch self { case .badResponse(let response): String(localized: "Received HTTP response \(response.statusCode).") + case .emptyResponse: + String(localized: "The weather service sent an empty response.") case .gzipDecompressionFailed: String(localized: "Downloaded weather data was corrupted or incomplete.") case .invalidTextEncoding: diff --git a/SF50 Shared/Weather/WeatherLoader+Loading.swift b/SF50 Shared/Weather/WeatherLoader+Loading.swift index ba595d5..053dbfe 100644 --- a/SF50 Shared/Weather/WeatherLoader+Loading.swift +++ b/SF50 Shared/Weather/WeatherLoader+Loading.swift @@ -27,6 +27,51 @@ extension WeatherLoader { guard let urlError = error as? URLError else { return false } return transientURLErrorCodes.contains(urlError.code) } + + /// Whether `error` is Aviation Weather failing on its own end rather than a defect here. + /// + /// Its CDN intermittently answers `5xx`, and a cache file being regenerated is served + /// as an empty body. A body that arrives corrupt is excluded: that is a real contract + /// break worth knowing about. + static func isUpstreamServerFailure(_ error: some Swift.Error) -> Bool { + switch error as? Errors { + case .badResponse(let response): (500..<600).contains(response.statusCode) + case .emptyResponse: true + default: false + } + } + + /// Whether `error` points at a defect in this app, and so belongs in Sentry. + /// + /// A cancelled load, a network the pilot has no coverage on, and Aviation Weather + /// failing on its own end are all outside this app's control. Those are logged and + /// surfaced to the pilot, who decides whether to retry. + static func shouldReport(_ error: some Swift.Error) -> Bool { + !isNetworkCancellation(error) && !isTransientNetworkError(error) + && !isUpstreamServerFailure(error) + } + + /// Logs a weather-load failure, reporting only genuine defects to Sentry. + private static func recordLoadFailure(_ error: some Swift.Error, dataType: String) { + guard shouldReport(error) else { + logger.info( + "Not reporting weather load failure; cause is outside this app", + metadata: ["error": "\(error)", "dataType": "\(dataType)"] + ) + return + } + + SentrySDK.capture(error: error) { scope in + scope.setLevel(.warning) + scope.setTag(value: dataType, key: "weather.dataType") + scope.setFingerprint(["weather-loading", dataType]) + } + logger.error( + "Failed to load weather data", + metadata: ["error": "\(error)", "dataType": "\(dataType)"] + ) + } + func loadMETARs() async { observations = .loading await notifySubscribers() @@ -98,22 +143,10 @@ extension WeatherLoader { observations = .value(newMETARs) } catch { - if Self.isNetworkCancellation(error) { - // Don't update observations if cancelled - } else if Self.isTransientNetworkError(error) { - Self.logger.info( - "Transient network error loading METARs", - metadata: ["error": "\(error)"] - ) - observations = .error(error) - } else { - SentrySDK.capture(error: error) { scope in - scope.setLevel(.warning) - scope.setTag(value: "metar", key: "weather.dataType") - scope.setFingerprint(["weather-loading", "metar"]) - } - observations = .error(error) - } + // Don't update observations if cancelled + guard !Self.isNetworkCancellation(error) else { return } + Self.recordLoadFailure(error, dataType: "metar") + observations = .error(error) } } @@ -187,22 +220,10 @@ extension WeatherLoader { forecasts = .value(newTAFs) } catch { - if Self.isNetworkCancellation(error) { - // Don't update forecasts if cancelled - } else if Self.isTransientNetworkError(error) { - Self.logger.info( - "Transient network error loading TAFs", - metadata: ["error": "\(error)"] - ) - forecasts = .error(error) - } else { - SentrySDK.capture(error: error) { scope in - scope.setLevel(.warning) - scope.setTag(value: "taf", key: "weather.dataType") - scope.setFingerprint(["weather-loading", "taf"]) - } - forecasts = .error(error) - } + // Don't update forecasts if cancelled + guard !Self.isNetworkCancellation(error) else { return } + Self.recordLoadFailure(error, dataType: "taf") + forecasts = .error(error) } } @@ -235,26 +256,10 @@ extension WeatherLoader { windsAloft = .value(stationData) } catch { - if Self.isNetworkCancellation(error) { - // Don't update windsAloft if cancelled - } else if Self.isTransientNetworkError(error) { - Self.logger.info( - "Transient network error loading winds aloft", - metadata: ["error": "\(error)"] - ) - windsAloft = .error(error) - } else { - SentrySDK.capture(error: error) { scope in - scope.setLevel(.warning) - scope.setTag(value: "windsAloft", key: "weather.dataType") - scope.setFingerprint(["weather-loading", "windsAloft"]) - } - Self.logger.error( - "Failed to load winds aloft", - metadata: ["error": "\(error)"] - ) - windsAloft = .error(error) - } + // Don't update windsAloft if cancelled + guard !Self.isNetworkCancellation(error) else { return } + Self.recordLoadFailure(error, dataType: "windsAloft") + windsAloft = .error(error) } } @@ -275,6 +280,11 @@ extension WeatherLoader { } } + guard !data.isEmpty else { + Self.logger.error("Empty weather response", metadata: ["url": "\(url)"]) + throw Errors.emptyResponse(url: url) + } + Self.logger.info( "Downloaded weather data", metadata: [ diff --git a/SF50 Shared/Weather/WeatherLoader+Types.swift b/SF50 Shared/Weather/WeatherLoader+Types.swift index e6a1682..393ffd0 100644 --- a/SF50 Shared/Weather/WeatherLoader+Types.swift +++ b/SF50 Shared/Weather/WeatherLoader+Types.swift @@ -10,6 +10,12 @@ extension WeatherLoader { /// HTTP response was not successful. case badResponse(_ response: HTTPURLResponse) + /// The server answered successfully but sent no body. + /// + /// Aviation Weather serves an empty cache file while it is being + /// regenerated, so this is distinct from a body that arrived corrupt. + case emptyResponse(url: URL) + /// Failed to decompress GZIP data. case gzipDecompressionFailed( url: URL, diff --git a/SF50 Shared/Weather/WeatherViewModel.swift b/SF50 Shared/Weather/WeatherViewModel.swift index 5181a26..455e9a8 100644 --- a/SF50 Shared/Weather/WeatherViewModel.swift +++ b/SF50 Shared/Weather/WeatherViewModel.swift @@ -202,9 +202,7 @@ public final class WeatherViewModel: WithIdentifiableError { } case .error(let error): if !self.isManualMode { - if !WeatherLoader.isTransientNetworkError(error), - !WeatherLoader.isNetworkCancellation(error) - { + if WeatherLoader.shouldReport(error) { SentrySDK.capture(error: error) { scope in scope.setLevel(.warning) scope.setTag(value: "conditions", key: "weather.dataType")