From b32d2c4f42fb861db0864450efbf4e1623a9b275 Mon Sep 17 00:00:00 2001 From: KeviM Date: Fri, 4 Sep 2026 02:03:57 -0700 Subject: [PATCH 01/11] fix(error): scope the 404 no-active-outlook reading to the SPC static feeds Error::from_response mapped every HTTP 404 to FeedUnavailable, so ErrorCode::NotFound was unreachable and a retired ArcGIS layer, a renamed MapServer path or a dead IEM endpoint reached consumers as SPC's normal "no active outlook" state. The documented consumer pattern then clears rows for what is actually a broken integration. from_response now takes a Feed404 trust-boundary argument. Only StaticFeedClient passes NoActiveOutlook; ArcGISClient and ArchiveClient take the NotFound default. inspect_arcgis_envelope no longer reuses the HTTP status mapper for ArcGIS logical errors, which arrive over HTTP 200: Error::from_arcgis maps code 404 to NotFound (verified live -- a renamed MapServer path answers HTTP 200 with {"error":{"code":404}}) and keeps a non-HTTP code such as 1000 out of Error::http_status. --- CHANGELOG.md | 23 +++++++++++ include/spc/error.hpp | 41 ++++++++++++++++++-- src/api/client.cpp | 33 ++++++++++------ src/core/error.cpp | 90 ++++++++++++++++++++++++++++++++----------- tests/test_client.cpp | 79 +++++++++++++++++++++++++++++++++++++ 5 files changed, 229 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a9dfd1..c38e487 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,29 @@ uses [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Changed + +- **HTTP 404 now says which kind of 404 it was.** `ErrorCode::FeedUnavailable` + ("no active outlook — clear the rows") is produced only by + `StaticFeedClient`, the one feed where SPC uses 404 that way. A 404 from the + ArcGIS MapServer or from IEM — a retired product, a renamed service path, a + wrong base URL — is now `ErrorCode::NotFound`, which was previously + unreachable. A logical ArcGIS `{"error":{"code":404}}` envelope, which is how + a renamed MapServer path is actually reported (over HTTP 200), maps to + `NotFound` as well. Consumers that branch on `is_feed_unavailable()` for + `ArcGISClient` or `ArchiveClient` results should treat `NotFound` as a fault + and alert on it instead of clearing rows. +- `Error::from_response` takes a trailing `Feed404` argument stating what a 404 + means for the feed that answered. It defaults to `Feed404::NotFound`, so + existing calls keep compiling and get the safe reading. + +### Added + +- `Error::from_arcgis`, for ArcGIS logical failure envelopes. It keeps the + ArcGIS code in `Error::http_status` only while that code is HTTP-shaped + (100..599) and records it in `Error::detail`, so a code such as 1000 can no + longer masquerade as an HTTP status. + ## [0.2.0] - 2026-09-03 ### Added diff --git a/include/spc/error.hpp b/include/spc/error.hpp index e693e46..949004a 100644 --- a/include/spc/error.hpp +++ b/include/spc/error.hpp @@ -13,11 +13,16 @@ enum class ErrorCode { NetworkError, RateLimited, ServerError, + /// A genuine fault: a bad URL, a retired product, a renamed MapServer + /// path, or a wrong ArcGIS layer id. Every 404 that is not an SPC static + /// feed lands here — including a logical ArcGIS `{"error":{"code":404}}` + /// envelope, which is how a retired MapServer path is reported. NotFound, /// SPC returns HTTP 404 for "no active outlook" (e.g. overnight day-1 /// probabilistic). This is a normal, expected state — distinct from a /// genuine `NotFound` (bad URL / retired product). Consumers treat it as - /// "clear the rows for this day/hazard", not as an error. + /// "clear the rows for this day/hazard", not as an error. Only + /// `StaticFeedClient` produces it: see `Feed404`. FeedUnavailable, InvalidRequest, ParseError, @@ -49,10 +54,28 @@ enum class ErrorCode { return "Unknown"; } +/// How a transport-level HTTP 404 is to be read for the feed that answered. +/// +/// `Error::from_response` cannot tell a "no active outlook" 404 from a +/// retired endpoint by looking at the status alone, so the caller — which +/// knows which host it addressed — states the semantics explicitly. +enum class Feed404 : std::uint8_t { + /// A 404 is a fault (bad URL / retired product) -> `ErrorCode::NotFound`. + /// The safe default: every feed except the SPC static products. + NotFound, + /// SPC's static `.nolyr.geojson` products answer 404 with an HTML page + /// when nothing is issued -> `ErrorCode::FeedUnavailable`. + NoActiveOutlook, +}; + /// Error information returned by SDK operations. struct Error { ErrorCode code; std::string message; + /// Transport HTTP status. For a logical ArcGIS failure (reported over + /// HTTP 200) this carries the ArcGIS error code when that code is in the + /// 100..599 HTTP range, and 0 otherwise — ArcGIS also uses codes such as + /// 1000 that are not HTTP statuses. See `from_arcgis`. int http_status{0}; std::string detail; @@ -94,9 +117,19 @@ struct Error { return {ErrorCode::InvalidRequest, std::move(msg), 0, ""}; } - /// Create an Error from an HTTP response status code and body. SPC's 404 - /// "no active outlook" maps to `FeedUnavailable` (not `NotFound`). - [[nodiscard]] static Error from_response(int status, const std::string& body); + /// Create an Error from an HTTP response status code and body. + /// + /// `semantics` decides what a 404 means for the feed that answered: + /// `Feed404::NoActiveOutlook` (SPC static products only) yields + /// `FeedUnavailable`; the default `Feed404::NotFound` yields `NotFound`. + [[nodiscard]] static Error from_response(int status, const std::string& body, + Feed404 semantics = Feed404::NotFound); + + /// Create an Error from an ArcGIS logical failure envelope + /// (`{"error":{"code":...,"message":...}}`), which the MapServer reports + /// over HTTP 200. A code of 404 — how a renamed or retired service path + /// is reported — is a genuine `NotFound`, never `FeedUnavailable`. + [[nodiscard]] static Error from_arcgis(int arcgis_code, const std::string& body); }; /// Result type for SDK operations. diff --git a/src/api/client.cpp b/src/api/client.cpp index d5155b5..7cab387 100644 --- a/src/api/client.cpp +++ b/src/api/client.cpp @@ -147,22 +147,28 @@ Result inspect_arcgis_envelope(const std::string& body) { if (error != nullptr && error->is_object()) { const double raw_code = detail::json_number_or_numeric_string(*error, "code"); const int code = raw_code > 0.0 ? static_cast(raw_code) : 400; - return std::unexpected(Error::from_response(code, body)); + // A logical ArcGIS failure travels over HTTP 200, so it must not go + // through the HTTP status mapper: an ArcGIS code 404 (renamed or + // retired service path) is a genuine NotFound, and a code like 1000 + // is not an HTTP status at all. + return std::unexpected(Error::from_arcgis(code, body)); } const Json* exceeded = detail::lookup(*root, "exceededTransferLimit"); return exceeded != nullptr && exceeded->is_boolean() && exceeded->get(); } -/// SPC 404 == "no active outlook" (FeedUnavailable). Map HTTP status to the -/// right error; only a real body is handed to the parser. -Result body_or_error(Result r) { +/// Map HTTP status to the right error; only a real body is handed to the +/// parser. `semantics` is the trust boundary: only the SPC static feeds may +/// read a 404 as "no active outlook" (FeedUnavailable). For every other host +/// a 404 is a retired or wrong URL, i.e. NotFound. +Result body_or_error(Result r, Feed404 semantics) { if (!r) { return std::unexpected(r.error()); } if (r->status_code == 200) { return std::move(r->body); } - return std::unexpected(Error::from_response(r->status_code, r->body)); + return std::unexpected(Error::from_response(r->status_code, r->body, semantics)); } } // namespace @@ -192,7 +198,8 @@ Result StaticFeedClient::day_categorical(std::int32_t } const std::string url = std::format("{}day{}otlk_cat.nolyr.geojson", kStaticBase, day); Result body = - body_or_error(with_retry([&] { return impl_->http->get(url); }, impl_->retry)); + body_or_error(with_retry([&] { return impl_->http->get(url); }, impl_->retry), + Feed404::NoActiveOutlook); if (!body) { return std::unexpected(body.error()); } @@ -220,7 +227,8 @@ Result StaticFeedClient::day_probabilistic(std::int32_t day, : std::format("day{}otlk_{}.nolyr.geojson", day, tag); const std::string url = std::string{kStaticBase} + filename; Result body = - body_or_error(with_retry([&] { return impl_->http->get(url); }, impl_->retry)); + body_or_error(with_retry([&] { return impl_->http->get(url); }, impl_->retry), + Feed404::NoActiveOutlook); if (!body) { return std::unexpected(body.error()); } @@ -238,7 +246,8 @@ Result StaticFeedClient::day4_8(std::int32_t day) { } const std::string url = std::format("{}day{}prob.nolyr.geojson", kStaticDay48Base, day); Result body = - body_or_error(with_retry([&] { return impl_->http->get(url); }, impl_->retry)); + body_or_error(with_retry([&] { return impl_->http->get(url); }, impl_->retry), + Feed404::NoActiveOutlook); if (!body) { return std::unexpected(body.error()); } @@ -281,7 +290,7 @@ struct ArcGISClient::Impl { url += "&outSR=" + percent_encode(out_spatial_reference); } Result body = - body_or_error(with_retry([&] { return http->get(url); }, retry)); + body_or_error(with_retry([&] { return http->get(url); }, retry), Feed404::NotFound); if (!body) { return std::unexpected(body.error()); } @@ -545,7 +554,8 @@ Result ArchiveClient::watches(const std::string& ts) { url += std::format("?ts={}", ts); } Result body = - body_or_error(with_retry([&] { return impl_->http->get(url); }, impl_->retry)); + body_or_error(with_retry([&] { return impl_->http->get(url); }, impl_->retry), + Feed404::NotFound); if (!body) { return std::unexpected(body.error()); } @@ -569,7 +579,8 @@ Result ArchiveClient::storm_reports(const std::string& start url += std::format("&wfo={}", wfo); } Result body = - body_or_error(with_retry([&] { return impl_->http->get(url); }, impl_->retry)); + body_or_error(with_retry([&] { return impl_->http->get(url); }, impl_->retry), + Feed404::NotFound); if (!body) { return std::unexpected(body.error()); } diff --git a/src/core/error.cpp b/src/core/error.cpp index 9bc0c0a..94c50c3 100644 --- a/src/core/error.cpp +++ b/src/core/error.cpp @@ -1,34 +1,18 @@ #include "spc/error.hpp" +#include #include #include #include namespace spc { -Error Error::from_response(int status, const std::string& body) { - Error err; - err.http_status = status; - - // Determine error code from HTTP status. SPC serves a static HTML 404 - // page when a product has no active outlook (the documented normal - // overnight state for e.g. day-1 probabilistic); we surface that as - // FeedUnavailable so callers clear rows rather than treat it as a fault. - if (status == 404) { - err.code = ErrorCode::FeedUnavailable; - } else if (status == 400) { - err.code = ErrorCode::InvalidRequest; - } else if (status == 429 || status == 503) { - err.code = ErrorCode::RateLimited; - } else if (status >= 500) { - err.code = ErrorCode::ServerError; - } else { - err.code = ErrorCode::Unknown; - } +namespace { - // Best-effort: ArcGIS returns JSON `{"error":{"code":...,"message":...}}` - // on logical failures even with HTTP 200. Parse via glz::generic (no - // schema to reject on); fall back to the raw body for SPC's HTML pages. +/// Best-effort: ArcGIS returns JSON `{"error":{"code":...,"message":...}}` +/// on logical failures even with HTTP 200. Parse via glz::generic (no schema +/// to reject on); fall back to the raw body for SPC's HTML pages. +void fill_message_from_body(Error& err, const std::string& body) { glz::generic root{}; glz::error_ctx ec = glz::read_json(root, body); if (!ec && root.is_object()) { @@ -49,6 +33,34 @@ Error Error::from_response(int status, const std::string& body) { // Not valid JSON (SPC's HTML 404) — keep a short raw snippet. err.message = body.substr(0, 256); } +} + +} // namespace + +Error Error::from_response(int status, const std::string& body, Feed404 semantics) { + Error err; + err.http_status = status; + + // Determine error code from HTTP status. SPC serves a static HTML 404 + // page when a product has no active outlook (the documented normal + // overnight state for e.g. day-1 probabilistic); only a caller that + // addressed those feeds may ask for that reading. Every other 404 — a + // renamed MapServer path, a retired IEM endpoint, a typo in a base URL — + // is a genuine fault, so the default is NotFound. + if (status == 404) { + err.code = semantics == Feed404::NoActiveOutlook ? ErrorCode::FeedUnavailable + : ErrorCode::NotFound; + } else if (status == 400) { + err.code = ErrorCode::InvalidRequest; + } else if (status == 429 || status == 503) { + err.code = ErrorCode::RateLimited; + } else if (status >= 500) { + err.code = ErrorCode::ServerError; + } else { + err.code = ErrorCode::Unknown; + } + + fill_message_from_body(err, body); if (err.message.empty()) { err.message = "HTTP " + std::to_string(status); @@ -57,4 +69,38 @@ Error Error::from_response(int status, const std::string& body) { return err; } +Error Error::from_arcgis(int arcgis_code, const std::string& body) { + Error err; + + // ArcGIS codes overlap the HTTP status space but are not HTTP statuses: + // the transport answered 200. Keep the code in http_status only while it + // is HTTP-shaped, so a code such as 1000 cannot masquerade as a status. + err.http_status = arcgis_code >= 100 && arcgis_code <= 599 ? arcgis_code : 0; + + if (arcgis_code == 404) { + // A renamed or retired MapServer path. Never "no active outlook": + // ArcGIS signals an empty product as HTTP 200 with no features. + err.code = ErrorCode::NotFound; + } else if (arcgis_code == 429) { + err.code = ErrorCode::RateLimited; + } else if (arcgis_code >= 500 && arcgis_code <= 599) { + err.code = ErrorCode::ServerError; + } else { + // 400, 403, 498/499 (token), 1000+ (operation failures): the request + // as posed cannot be served. + err.code = ErrorCode::InvalidRequest; + } + + fill_message_from_body(err, body); + + if (err.detail.empty()) { + err.detail = std::format("arcgisCode={}", arcgis_code); + } + if (err.message.empty()) { + err.message = std::format("ArcGIS error {}", arcgis_code); + } + + return err; +} + } // namespace spc diff --git a/tests/test_client.cpp b/tests/test_client.cpp index e7242b9..409db0a 100644 --- a/tests/test_client.cpp +++ b/tests/test_client.cpp @@ -249,6 +249,85 @@ TEST(ArcGISClientPaging, ReturnsLogicalArcGISErrorsReportedWithHttp200) { EXPECT_EQ(result.error().message, "Invalid or missing input parameters."); } +// ===== HTTP 404 trust boundary ===== +// +// SPC's static products answer 404 with an HTML page when there is no active +// outlook (verified live: a retired www.spc.noaa.gov outlook path returns +// HTTP 404 text/html). Every other feed's 404 is a genuine fault. The ArcGIS +// MapServer reports a retired service path as HTTP 200 with a logical +// `{"error":{"code":404}}` envelope (verified live against a renamed service). + +TEST(Feed404Semantics, StaticFeedReadsSpcHtml404AsNoActiveOutlook) { + std::shared_ptr transport = std::make_shared(); + transport->responses = {{404, "404 Not Found", {}}}; + StaticFeedClient client{transport}; + + const Result result = client.day_categorical(1); + + ASSERT_FALSE(result); + EXPECT_EQ(result.error().code, ErrorCode::FeedUnavailable); + EXPECT_TRUE(result.error().is_feed_unavailable()); + EXPECT_EQ(result.error().http_status, 404); +} + +TEST(Feed404Semantics, ArcGisTransport404IsAGenuineNotFound) { + std::shared_ptr transport = std::make_shared(); + transport->responses = {{404, "not found", {}}}; + ArcGISClient client{transport}; + + const Result result = client.query_categorical(1); + + ASSERT_FALSE(result); + EXPECT_EQ(result.error().code, ErrorCode::NotFound); + EXPECT_FALSE(result.error().is_feed_unavailable()); +} + +TEST(Feed404Semantics, ArcGisLogicalNotFoundIsAGenuineNotFound) { + std::shared_ptr transport = std::make_shared(); + // Live shape for a renamed / retired MapServer path. + transport->responses = { + {200, + R"({"error":{"code":404,"message":"Service outlooks/SPC_wx_outlks_RETIRED/MapServer not found ","details":[]}})", + {}}, + }; + ArcGISClient client{transport}; + + const Result result = client.query_categorical(1); + + ASSERT_FALSE(result); + EXPECT_EQ(result.error().code, ErrorCode::NotFound); + EXPECT_FALSE(result.error().is_feed_unavailable()); + EXPECT_NE(result.error().message.find("not found"), std::string::npos); +} + +TEST(Feed404Semantics, ArcGisCodeThatIsNotAnHttpStatusStaysOutOfHttpStatus) { + std::shared_ptr transport = std::make_shared(); + transport->responses = { + {200, R"({"error":{"code":1000,"message":"Unable to complete operation.","details":[]}})", {}}, + }; + ArcGISClient client{transport}; + + const Result> result = + client.query_layer(ArcGISService::Outlooks, 1, {}); + + ASSERT_FALSE(result); + EXPECT_EQ(result.error().code, ErrorCode::InvalidRequest); + EXPECT_EQ(result.error().http_status, 0); + EXPECT_NE(result.error().detail.find("1000"), std::string::npos); +} + +TEST(Feed404Semantics, ArchiveTransport404IsAGenuineNotFound) { + std::shared_ptr transport = std::make_shared(); + transport->responses = {{404, "gone", {}}}; + ArchiveClient client{transport}; + + const Result result = client.watches(); + + ASSERT_FALSE(result); + EXPECT_EQ(result.error().code, ErrorCode::NotFound); + EXPECT_FALSE(result.error().is_feed_unavailable()); +} + TEST(ArcGISClientRouting, ActiveWatchesDirectCallersToTheIemClient) { std::shared_ptr transport = std::make_shared(); ArcGISClient client{transport}; From 0b8482bef6790c7d022ca10bf57dc7fd75b05e5d Mon Sep 17 00:00:00 2001 From: KeviM Date: Fri, 4 Sep 2026 02:06:54 -0700 Subject: [PATCH 02/11] fix(pagination): advance ArcGIS paging by the records returned and bound the loop ArcGISPager::advance() added the requested resultRecordCount to the offset. ArcGIS clamps that request to the layer's own maxRecordCount, so a short page still flagged exceededTransferLimit made the next resultOffset skip every record in the gap -- a successful result with a silent hole. inspect_arcgis_envelope already had the parsed root, so it now reports the features-array length too and advance() consumes it. The loop also had no ceiling: a server or caching proxy that keeps reporting truncation looped forever, accumulating one raw page body per iteration, and the int32 offset would eventually overflow. Paging now fails with ServerError when a truncated page carries no records, and after ArcGISPager::max_pages() (100) requests while the server is still truncating. offset() is int64. Verified live against SPC_wx_outlks layer 1 with resultRecordCount=1: f=json reports exceededTransferLimit at the root, f=geojson at the root and under properties. Both placements are now read. --- CHANGELOG.md | 15 +++++++ include/spc/pagination.hpp | 48 +++++++++++++++++------ src/api/client.cpp | 49 +++++++++++++++++++---- tests/test_client.cpp | 80 +++++++++++++++++++++++++++++++++++++- 4 files changed, 173 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c38e487..5e66b6f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,21 @@ uses [Semantic Versioning](https://semver.org/spec/v2.0.0.html). means for the feed that answered. It defaults to `Feed404::NotFound`, so existing calls keep compiling and get the safe reading. +### Fixed + +- **ArcGIS paging advanced `resultOffset` by the requested page size, not by + the records the server returned.** ArcGIS clamps `resultRecordCount` to the + layer's own `maxRecordCount`, so a truncated page can be shorter than the + 2000 requested; the offset then skipped the gap and the caller got a + successful result with a silent hole. `ArcGISPager::advance()` now takes the + returned record count. +- ArcGIS paging is bounded. A page that reports truncation while carrying no + records, and a server that never stops reporting truncation, now fail with + `ErrorCode::ServerError` after at most `ArcGISPager::max_pages()` (100) + requests instead of looping forever and growing memory without bound. + `ArcGISPager::offset()` is a `std::int64_t`, so the arithmetic cannot + overflow. + ### Added - `Error::from_arcgis`, for ArcGIS logical failure envelopes. It keeps the diff --git a/include/spc/pagination.hpp b/include/spc/pagination.hpp index 75df3c2..46c704e 100644 --- a/include/spc/pagination.hpp +++ b/include/spc/pagination.hpp @@ -9,7 +9,13 @@ namespace spc { /// An ArcGIS layer query caps the result set (commonly 1000-2000 records) /// and signals truncation with `"exceededTransferLimit": true` in the /// response envelope. To page, re-issue with `resultOffset` advanced by the -/// page size until the server stops reporting truncation. +/// number of records the server actually returned until it stops reporting +/// truncation. +/// +/// The requested `resultRecordCount` is only a hint: ArcGIS clamps it to the +/// layer's own `maxRecordCount`, so a truncated page can be shorter than the +/// page size. Advancing by the request size would then skip every record in +/// the gap, which is why `advance()` requires the returned record count. /// /// SPC outlook layers are tiny (single-digit feature counts), so paging /// rarely triggers — but storm-report / LSR layers can exceed the cap, and @@ -18,36 +24,56 @@ namespace spc { class ArcGISPager { public: /// `page_size` is the `resultRecordCount` passed per request. ArcGIS's - /// hard server max is typically 2000; that is the default. - explicit ArcGISPager(std::int32_t page_size = 2000) - : page_size_(page_size > 0 ? page_size : 2000) {} + /// hard server max is typically 2000; that is the default. `max_pages` + /// bounds a server (or caching proxy) that keeps reporting truncation + /// without advancing — see `page_limit_reached()`. + explicit ArcGISPager(std::int32_t page_size = 2000, std::int32_t max_pages = 100) + : page_size_(page_size > 0 ? page_size : 2000), + max_pages_(max_pages > 0 ? max_pages : 100) {} [[nodiscard]] std::int32_t page_size() const noexcept { return page_size_; } + /// Hard ceiling on the number of pages fetched for one query. + [[nodiscard]] std::int32_t max_pages() const noexcept { return max_pages_; } + /// Current `resultOffset` to send on the next request. - [[nodiscard]] std::int32_t offset() const noexcept { return offset_; } + [[nodiscard]] std::int64_t offset() const noexcept { return offset_; } /// Whether another page should be fetched. Seeded `true`; the caller /// passes the response's `exceededTransferLimit` after each page. [[nodiscard]] bool has_more() const noexcept { return has_more_; } - /// Record the just-fetched page: advance the offset by `page_size` and - /// keep going only while the server still reports truncation. - void advance(bool exceeded_transfer_limit) noexcept { - offset_ += page_size_; - has_more_ = exceeded_transfer_limit; + /// True when paging stopped at `max_pages` while the server was still + /// reporting truncation — a non-converging server, not a complete result. + [[nodiscard]] bool page_limit_reached() const noexcept { + return server_truncated_ && pages_fetched_ >= max_pages_; + } + + /// Record the just-fetched page: advance the offset by the records the + /// server returned, and keep going only while it still reports truncation + /// and the page ceiling has not been reached. + void advance(std::int32_t records_returned, bool exceeded_transfer_limit) noexcept { + offset_ += records_returned > 0 ? records_returned : 0; + ++pages_fetched_; + server_truncated_ = exceeded_transfer_limit; + has_more_ = exceeded_transfer_limit && pages_fetched_ < max_pages_; } /// Reset to the first page. void reset() noexcept { offset_ = 0; + pages_fetched_ = 0; has_more_ = true; + server_truncated_ = false; } private: std::int32_t page_size_; - std::int32_t offset_{0}; + std::int32_t max_pages_; + std::int64_t offset_{0}; + std::int32_t pages_fetched_{0}; bool has_more_{true}; + bool server_truncated_{false}; }; } // namespace spc diff --git a/src/api/client.cpp b/src/api/client.cpp index 7cab387..b03b78d 100644 --- a/src/api/client.cpp +++ b/src/api/client.cpp @@ -138,7 +138,19 @@ const char* service_base(ArcGISService service) { return kArcGisOutlks; } -Result inspect_arcgis_envelope(const std::string& body) { +/// What `paged()` needs from one ArcGIS response envelope: whether the server +/// truncated the result, and how many records it actually returned. +struct ArcGISEnvelope { + std::int32_t feature_count{0}; + bool exceeded_transfer_limit{false}; +}; + +bool envelope_flag(const Json& root, const char* key) { + const Json* flag = detail::lookup(root, key); + return flag != nullptr && flag->is_boolean() && flag->get(); +} + +Result inspect_arcgis_envelope(const std::string& body) { const glz::expected root = detail::parse_root(body); if (!root) { return std::unexpected(Error::parse(root.error())); @@ -153,8 +165,22 @@ Result inspect_arcgis_envelope(const std::string& body) { // is not an HTTP status at all. return std::unexpected(Error::from_arcgis(code, body)); } - const Json* exceeded = detail::lookup(*root, "exceededTransferLimit"); - return exceeded != nullptr && exceeded->is_boolean() && exceeded->get(); + ArcGISEnvelope envelope; + // Verified live against SPC_wx_outlks layer 1 with resultRecordCount=1: + // `f=json` carries the flag at the root, `f=geojson` carries it at the + // root AND under `properties`. Read both so either shape is honoured. + envelope.exceeded_transfer_limit = envelope_flag(*root, "exceededTransferLimit"); + if (!envelope.exceeded_transfer_limit) { + const Json* properties = detail::lookup(*root, "properties"); + envelope.exceeded_transfer_limit = + properties != nullptr && envelope_flag(*properties, "exceededTransferLimit"); + } + // Esri (`f=json`) and GeoJSON (`f=geojson`) both name the array `features`. + const Json* features = detail::lookup(*root, "features"); + if (features != nullptr && features->is_array()) { + envelope.feature_count = static_cast(features->get_array().size()); + } + return envelope; } /// Map HTTP status to the right error; only a real body is handed to the @@ -294,12 +320,21 @@ struct ArcGISClient::Impl { if (!body) { return std::unexpected(body.error()); } - const Result exceeded = inspect_arcgis_envelope(*body); - if (!exceeded) { - return std::unexpected(exceeded.error()); + const Result envelope = inspect_arcgis_envelope(*body); + if (!envelope) { + return std::unexpected(envelope.error()); + } + if (envelope->exceeded_transfer_limit && envelope->feature_count == 0) { + // The offset would never move: paging cannot converge. + return std::unexpected(Error::server( + "ArcGIS reported a truncated page containing no records")); } pages.push_back(std::move(*body)); - pager.advance(*exceeded); + pager.advance(envelope->feature_count, envelope->exceeded_transfer_limit); + } + if (pager.page_limit_reached()) { + return std::unexpected(Error::server(std::format( + "ArcGIS paging did not converge within {} pages", pager.max_pages()))); } return pages; } diff --git a/tests/test_client.cpp b/tests/test_client.cpp index 409db0a..8fdb0cd 100644 --- a/tests/test_client.cpp +++ b/tests/test_client.cpp @@ -1,5 +1,7 @@ #include "spc/api.hpp" +#include "spc/pagination.hpp" +#include #include #include #include @@ -30,6 +32,32 @@ HttpResponse empty_feature_collection() { return {200, R"({"type":"FeatureCollection","features":[],"exceededTransferLimit":false})", {}}; } +/// A FeatureCollection of `count` placeholder features that still reports +/// truncation — the shape ArcGIS returns when a layer's own maxRecordCount is +/// below the requested resultRecordCount. +std::string truncated_page(std::int32_t count) { + std::string body = R"({"type":"FeatureCollection","features":[)"; + for (std::int32_t i = 0; i < count; ++i) { + if (i > 0) { + body += ","; + } + body += R"({"type":"Feature","properties":{},"geometry":null})"; + } + body += R"(],"exceededTransferLimit":true})"; + return body; +} + +/// Answers every request with the same truncated page, forever. +class AlwaysTruncatingTransport final : public HttpTransport { +public: + mutable std::int32_t calls = 0; + + [[nodiscard]] Result get(std::string_view /*path*/) const override { + ++calls; + return HttpResponse{200, truncated_page(1), {}}; + } +}; + #if defined(__GNUC__) #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wdeprecated-declarations" @@ -207,7 +235,7 @@ TEST(ArcGISClientRouting, RejectsUnsupportedProductsBeforeNetworkAccess) { TEST(ArcGISClientPaging, EncodesParametersAndFetchesEveryPage) { std::shared_ptr transport = std::make_shared(); transport->responses = { - {200, R"({"features":[],"exceededTransferLimit":true})", {}}, + {200, truncated_page(2000), {}}, {200, R"({"features":[],"exceededTransferLimit":false})", {}}, }; ArcGISClient client{transport}; @@ -231,6 +259,56 @@ TEST(ArcGISClientPaging, EncodesParametersAndFetchesEveryPage) { EXPECT_NE(transport->requests[1].find("resultOffset=2000"), std::string::npos); } +TEST(ArcGISClientPaging, AdvancesTheOffsetByTheRecordsTheServerActuallyReturned) { + // ArcGIS clamps resultRecordCount to the layer's own maxRecordCount, so a + // truncated page can be shorter than the 2000 requested. Advancing by the + // request size would skip the records in between. + std::shared_ptr transport = std::make_shared(); + transport->responses = { + {200, truncated_page(3), {}}, + {200, truncated_page(2), {}}, + {200, R"({"features":[],"exceededTransferLimit":false})", {}}, + }; + ArcGISClient client{transport}; + + const Result> result = + client.query_layer(ArcGISService::Outlooks, 1, {}); + + ASSERT_TRUE(result); + ASSERT_EQ(transport->requests.size(), 3u); + EXPECT_NE(transport->requests[0].find("resultOffset=0"), std::string::npos); + EXPECT_NE(transport->requests[1].find("resultOffset=3"), std::string::npos); + EXPECT_NE(transport->requests[2].find("resultOffset=5"), std::string::npos); +} + +TEST(ArcGISClientPaging, FailsWhenATruncatedPageCarriesNoRecords) { + // Zero records plus exceededTransferLimit:true cannot converge — the + // offset would never move. Fail instead of looping. + std::shared_ptr transport = std::make_shared(); + transport->responses = {{200, R"({"features":[],"exceededTransferLimit":true})", {}}}; + ArcGISClient client{transport}; + + const Result> result = + client.query_layer(ArcGISService::Outlooks, 1, {}); + + ASSERT_FALSE(result); + EXPECT_EQ(result.error().code, ErrorCode::ServerError); + EXPECT_EQ(transport->requests.size(), 1u); +} + +TEST(ArcGISClientPaging, StopsAndFailsWhenTheServerNeverStopsReportingTruncation) { + std::shared_ptr transport = + std::make_shared(); + ArcGISClient client{transport}; + + const Result> result = + client.query_layer(ArcGISService::Outlooks, 1, {}); + + ASSERT_FALSE(result); + EXPECT_EQ(result.error().code, ErrorCode::ServerError); + EXPECT_EQ(transport->calls, ArcGISPager{}.max_pages()); +} + TEST(ArcGISClientPaging, ReturnsLogicalArcGISErrorsReportedWithHttp200) { std::shared_ptr transport = std::make_shared(); transport->responses = { From d79d3ed5fa7247bd0022aa7fc2840bb0c3411683 Mon Sep 17 00:00:00 2001 From: KeviM Date: Fri, 4 Sep 2026 02:08:08 -0700 Subject: [PATCH 03/11] fix(fire-weather)!: require an explicit FireWeatherLayer when parsing The two-argument parse_fire_weather(body, day) defaulted to FireWeatherLayer::Outlook, so a dry-thunderstorm body decoded dn=5 as "ELEV" (severity 1) rather than "IDRT" (severity 0) -- the dn label confusion 0.2.0 fixed, still reachable through the public API. The captured day-1 and day-2 fire-weather payloads carry no LABEL key at all, only the numeric dn band index that both layers use with different meanings, so nothing in a body identifies its layer and there is no safe default. The overload is removed; the layer is now a required argument. A regression test asserts the ambiguity directly: both captured payloads contain no LABEL, and the same body yields ELEV/CRIT/EXTM through the outlook layer and IDRT/SDRT through the dry-thunderstorm layer. It also gives arcgis_day2_fire_weather.esri.json its first reader. --- CHANGELOG.md | 11 +++++++++ include/spc/models/fire_weather.hpp | 13 +++++----- src/models/fire_weather.cpp | 4 --- tests/test_arcgis.cpp | 38 ++++++++++++++++++++++++++++- 4 files changed, 55 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e66b6f..c3b612f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,17 @@ uses [Semantic Versioning](https://semver.org/spec/v2.0.0.html). `ArcGISPager::offset()` is a `std::int64_t`, so the arithmetic cannot overflow. +### Removed + +- The two-argument `parse_fire_weather(body, day)` overload. It silently + assumed `FireWeatherLayer::Outlook`, so a dry-thunderstorm body decoded + `dn=5` as `"ELEV"` (severity 1) instead of `"IDRT"` (severity 0) — the label + confusion 0.2.0 fixed, still reachable through the public API. The captured + day-1 and day-2 payloads carry no LABEL at all, only the shared numeric + `dn`, so nothing in a body says which layer produced it. Pass the layer + explicitly: `parse_fire_weather(body, day, FireWeatherLayer::Outlook)` + restores the old behaviour where that was in fact the right layer. + ### Added - `Error::from_arcgis`, for ArcGIS logical failure envelopes. It keeps the diff --git a/include/spc/models/fire_weather.hpp b/include/spc/models/fire_weather.hpp index 45b49d6..96bf495 100644 --- a/include/spc/models/fire_weather.hpp +++ b/include/spc/models/fire_weather.hpp @@ -44,13 +44,14 @@ struct FireWeatherPayload { /// (IDRT/SDRT) and anything else map to 0 (kept by label, no severity). [[nodiscard]] std::uint8_t fire_severity_from_label(std::string_view label) noexcept; -/// Parse a fire-weather outlook (ArcGIS Esri or static GeoJSON). Throws +/// Parse one known ArcGIS fire-weather layer (Esri or GeoJSON body). Throws /// std::runtime_error on malformed JSON. -[[nodiscard]] FireWeatherPayload parse_fire_weather(std::string_view body, std::int32_t day); - -/// Parse one known ArcGIS fire-weather layer. Day 1 and 2 use the layer kind -/// to disambiguate numeric `dn` codes shared by categorical and dry-thunder -/// products. Day 3 through 8 decode LABEL/label/dn as probabilities. +/// +/// `layer` is required and is not inferable from the body: NOAA's day-1 and +/// day-2 payloads carry no LABEL at all, only a numeric `dn` band index that +/// the outlook (5=ELEV, 8=CRIT, 10=EXTM) and dry-thunderstorm (5=IDRT, +/// 8=SDRT) products both use with different meanings. Day 3 through 8 decode +/// LABEL/label/dn as probabilities. [[nodiscard]] FireWeatherPayload parse_fire_weather(std::string_view body, std::int32_t day, FireWeatherLayer layer); diff --git a/src/models/fire_weather.cpp b/src/models/fire_weather.cpp index df05fc9..89aeff5 100644 --- a/src/models/fire_weather.cpp +++ b/src/models/fire_weather.cpp @@ -112,10 +112,6 @@ std::uint8_t fire_severity_from_label(std::string_view label) noexcept { return 0; // dry-thunderstorm bands (IDRT/SDRT) and unknowns: label-only } -FireWeatherPayload parse_fire_weather(std::string_view body, std::int32_t day) { - return parse_fire_weather(body, day, FireWeatherLayer::Outlook); -} - FireWeatherPayload parse_fire_weather(std::string_view body, std::int32_t day, FireWeatherLayer layer) { const Json root = parse_root_or_throw(body); diff --git a/tests/test_arcgis.cpp b/tests/test_arcgis.cpp index db9490f..e11e9c6 100644 --- a/tests/test_arcgis.cpp +++ b/tests/test_arcgis.cpp @@ -19,6 +19,7 @@ #include "spc/models/storm_report.hpp" #include "spc/models/watch.hpp" +#include #include #include #include @@ -230,7 +231,8 @@ TEST(NetNewModels, FireWeatherOwnSeverityMapper) { EXPECT_EQ(fire_severity_from_label("CRIT"), 2); EXPECT_EQ(fire_severity_from_label("EXTM"), 3); EXPECT_EQ(fire_severity_from_label("SLGT"), 0); // not categorical - const FireWeatherPayload p = parse_fire_weather(slurp("arcgis_day1_fire_weather.esri.json"), 1); + const FireWeatherPayload p = + parse_fire_weather(slurp("arcgis_day1_fire_weather.esri.json"), 1, FireWeatherLayer::Outlook); EXPECT_EQ(p.day, 1); ASSERT_EQ(p.features.size(), 3u); EXPECT_EQ(p.features[0].label, "ELEV"); @@ -244,6 +246,40 @@ TEST(NetNewModels, FireWeatherOwnSeverityMapper) { } } +TEST(NetNewModels, FireWeatherLayerIsTheOnlyThingThatDisambiguatesDayOneAndTwoDn) { + // The captured day-1 and day-2 payloads carry no LABEL at all, only the + // numeric dn band index (5/8/10) that the Outlook and DryThunderstorm + // layers both use with different meanings. Nothing in the body says which + // layer it came from, so the caller must say — there is no safe default. + for (const std::string& name : + {std::string{"arcgis_day1_fire_weather.esri.json"}, + std::string{"arcgis_day2_fire_weather.esri.json"}}) { + const std::string body = slurp(name); + EXPECT_EQ(body.find("LABEL"), std::string::npos) << name; + EXPECT_EQ(body.find("\"label\""), std::string::npos) << name; + + const std::int32_t day = name.find("day1") != std::string::npos ? 1 : 2; + const FireWeatherPayload outlook = parse_fire_weather(body, day, FireWeatherLayer::Outlook); + const FireWeatherPayload dry = + parse_fire_weather(body, day, FireWeatherLayer::DryThunderstorm); + + ASSERT_EQ(outlook.features.size(), 3u) << name; + ASSERT_EQ(dry.features.size(), 3u) << name; + EXPECT_EQ(outlook.features[0].label, "ELEV") << name; + EXPECT_EQ(outlook.features[1].label, "CRIT") << name; + EXPECT_EQ(outlook.features[2].label, "EXTM") << name; + EXPECT_EQ(dry.features[0].label, "IDRT") << name; + EXPECT_EQ(dry.features[1].label, "SDRT") << name; + for (const FireWeatherFeature& f : dry.features) { + EXPECT_EQ(f.layer, FireWeatherLayer::DryThunderstorm) << name; + EXPECT_EQ(f.severity, 0) << name; + EXPECT_NE(f.label, "ELEV") << name; + EXPECT_NE(f.label, "CRIT") << name; + EXPECT_NE(f.label, "EXTM") << name; + } + } +} + TEST(NetNewModels, FireWeatherDryThunderstormCodesUseTheirOwnLabels) { const std::string body = R"({"features":[ {"attributes":{"dn":5},"geometry":{"rings":[[[0,1],[1,1],[1,0],[0,0],[0,1]]]}}, From adaaf485ee41da4d4d879a3bc6df0d2f04a34c6a Mon Sep 17 00:00:00 2001 From: KeviM Date: Fri, 4 Sep 2026 04:36:01 -0700 Subject: [PATCH 04/11] fix(archive): encode IEM query values and stop the limiter crashing or hanging Three defects on the ArchiveClient path. Percent-encoding: ts, sts, ets and wfo went into IEM query URLs raw while the ArcGIS path in the same file encoded every value. api.hpp documents the timestamps as ISO 8601, which permits a +HH:MM offset, and a raw + decodes server-side as a space, so an offset-bearing timestamp silently queried a different window; an & in any value injected extra parameters. All four now go through the existing percent_encode. SIGFPE: RateLimiter::Config is a public aggregate with no validation, so a refill_interval of zero reached 'elapsed / refill_interval' in refill() and crashed on the first try_acquire(). The constructor clamps it, and clamps initial_tokens to max_tokens. Hang: acquire() with no max_wait polls try_acquire() every 10 ms, and try_acquire() returns false permanently once a configured daily_limit is spent -- only a UTC-midnight reset clears it -- so the caller's thread spun until midnight. Waiting cannot help, so acquire() and acquire_for() now fail immediately in that state. ArchiveClient additionally gives its limiter a 5 s bound, which makes the documented RateLimited result reachable, and acquires a token inside the retry lambda so every attempt is paid for. --- CHANGELOG.md | 23 ++++++++++++++++ include/spc/rate_limit.hpp | 16 +++++++++-- src/api/client.cpp | 56 ++++++++++++++++++++++++++------------ src/core/rate_limit.cpp | 33 +++++++++++++++++++++- tests/CMakeLists.txt | 1 + tests/test_client.cpp | 34 +++++++++++++++++++++++ tests/test_rate_limit.cpp | 52 +++++++++++++++++++++++++++++++++++ 7 files changed, 193 insertions(+), 22 deletions(-) create mode 100644 tests/test_rate_limit.cpp diff --git a/CHANGELOG.md b/CHANGELOG.md index c3b612f..a29f2ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,29 @@ uses [Semantic Versioning](https://semver.org/spec/v2.0.0.html). 2000 requested; the offset then skipped the gap and the caller got a successful result with a silent hole. `ArcGISPager::advance()` now takes the returned record count. +- **`RateLimiter` crashed on a zero `refill_interval` (SIGFPE).** + `RateLimiter::Config` is a public aggregate with no validation, so + `RateLimiter{{.refill_interval = 0ms}}` reached an integer division by zero + in `refill()` on the first `try_acquire()`. The constructor now clamps a + non-positive interval to the 1000 ms default, and clamps `initial_tokens` to + `max_tokens`. +- **`RateLimiter::acquire()` hung forever once a `daily_limit` was spent.** + With no `Config::max_wait` it polls `try_acquire()` every 10 ms, and + `try_acquire()` returns false permanently until the next UTC-midnight reset, + so the caller's thread spun until then. It now returns false immediately + when the daily quota is exhausted, since waiting cannot help. +- `ArchiveClient` interpolated `ts`, `sts`, `ets` and `wfo` into IEM query URLs + with no percent-encoding, while the ArcGIS path in the same file encoded + every value. `api.hpp` documents the timestamps as ISO 8601, which permits a + `+HH:MM` offset, and a raw `+` decodes server-side as a space — so an + offset-bearing timestamp silently queried a different window. An `&` in any + of the four injected extra query parameters. All four are now encoded. +- `ArchiveClient` now bounds its rate-limit wait (5 s) instead of blocking the + caller's thread indefinitely, which also makes the documented + `ErrorCode::RateLimited` result reachable, and acquires a token per retry + attempt rather than per call — `with_retry` re-issues up to 4 requests, and + retries precisely on 429/503, so one token was buying up to four requests + exactly when IEM was asking for less traffic. - ArcGIS paging is bounded. A page that reports truncation while carrying no records, and a server that never stops reporting truncation, now fail with `ErrorCode::ServerError` after at most `ArcGISPager::max_pages()` (100) diff --git a/include/spc/rate_limit.hpp b/include/spc/rate_limit.hpp index ded2213..6227051 100644 --- a/include/spc/rate_limit.hpp +++ b/include/spc/rate_limit.hpp @@ -18,9 +18,11 @@ namespace spc { class RateLimiter { public: struct Config { - std::uint16_t max_tokens = 2; ///< default: gentle on IEM - std::chrono::milliseconds refill_interval{1000}; ///< 1 token / sec - std::uint16_t initial_tokens = 2; + std::uint16_t max_tokens = 2; ///< default: gentle on IEM + /// Time per token. Clamped to 1000 ms if set to zero or less. + std::chrono::milliseconds refill_interval{1000}; + std::uint16_t initial_tokens = 2; ///< clamped to `max_tokens` + /// Longest `acquire()` may block. Unset means block indefinitely. std::optional max_wait; std::int32_t daily_limit{0}; ///< 0 = no daily cap }; @@ -28,6 +30,13 @@ class RateLimiter { explicit RateLimiter(Config config); [[nodiscard]] bool try_acquire() noexcept; + + /// Block until a token is available. With `Config::max_wait` set this is + /// `acquire_for(*max_wait)`; without it the wait is unbounded, so a + /// synchronous caller can stall for as long as the bucket stays empty. + /// Returns false only on a bounded wait that expired, or when a + /// configured `daily_limit` is spent — no amount of waiting clears that + /// before the next UTC day, so it fails immediately rather than spinning. [[nodiscard]] bool acquire(); [[nodiscard]] bool acquire_for(std::chrono::milliseconds max_wait); [[nodiscard]] std::uint16_t available_tokens() const noexcept; @@ -38,6 +47,7 @@ class RateLimiter { private: void refill() noexcept; void check_daily_reset() noexcept; + [[nodiscard]] bool daily_quota_exhausted() noexcept; Config config_; mutable std::mutex mutex_; diff --git a/src/api/client.cpp b/src/api/client.cpp index b03b78d..32498c1 100644 --- a/src/api/client.cpp +++ b/src/api/client.cpp @@ -9,6 +9,7 @@ #include #include +#include #include #include #include @@ -126,6 +127,15 @@ std::string percent_encode(std::string_view value) { return encoded; } +/// IEM is a courtesy third party, so the bucket stays small — but the wait +/// must be bounded, or `RateLimiter::acquire()` blocks the caller's thread +/// forever and the documented `ErrorCode::RateLimited` result is unreachable. +RateLimiter::Config archive_rate_limit() { + RateLimiter::Config config; + config.max_wait = std::chrono::seconds{5}; + return config; +} + const char* service_base(ArcGISService service) { switch (service) { case ArcGISService::Outlooks: @@ -560,7 +570,7 @@ struct ArchiveClient::Impl { r.initial_delay = std::chrono::milliseconds{500}; return r; }()), - limiter(RateLimiter::Config{}) {} + limiter(archive_rate_limit()) {} explicit Impl(std::shared_ptr transport) : http(usable_transport(std::move(transport))), retry([] { @@ -569,7 +579,22 @@ struct ArchiveClient::Impl { policy.initial_delay = std::chrono::milliseconds{500}; return policy; }()), - limiter(RateLimiter::Config{}) {} + limiter(archive_rate_limit()) {} + + /// Pay a token per *attempt*, not per call: `with_retry` re-issues the + /// request up to `max_attempts` times, and it retries precisely on + /// 429/503 — the responses in which IEM is asking for less traffic. + /// Acquiring once outside the retry loop undercounted the budget by 4x. + Result rate_limited_get(const std::string& url) { + return with_retry( + [&]() -> Result { + if (!limiter.acquire()) { + return std::unexpected(Error::rate_limited("IEM rate limit")); + } + return http->get(url); + }, + retry); + } }; ArchiveClient::ArchiveClient(ClientConfig config) @@ -581,16 +606,14 @@ ArchiveClient::ArchiveClient(ArchiveClient&&) noexcept = default; ArchiveClient& ArchiveClient::operator=(ArchiveClient&&) noexcept = default; Result ArchiveClient::watches(const std::string& ts) { - if (!impl_->limiter.acquire()) { - return std::unexpected(Error::rate_limited("IEM rate limit")); - } + // `ts` is caller-supplied and reaches a query string, so it is encoded + // like every ArcGIS query value: an unencoded '&' would inject a + // parameter and an unencoded '+' would decode server-side as a space. std::string url = std::format("{}json/spcwatch.py", kIemBase); if (!ts.empty()) { - url += std::format("?ts={}", ts); + url += "?ts=" + percent_encode(ts); } - Result body = - body_or_error(with_retry([&] { return impl_->http->get(url); }, impl_->retry), - Feed404::NotFound); + Result body = body_or_error(impl_->rate_limited_get(url), Feed404::NotFound); if (!body) { return std::unexpected(body.error()); } @@ -605,17 +628,14 @@ Result ArchiveClient::watches(const std::string& ts) { Result ArchiveClient::storm_reports(const std::string& start_iso, const std::string& end_iso, const std::string& wfo) { - if (!impl_->limiter.acquire()) { - return std::unexpected(Error::rate_limited("IEM rate limit")); - } - std::string url = - std::format("{}geojson/lsr.geojson?sts={}&ets={}", kIemBase, start_iso, end_iso); + // api.hpp documents these as ISO 8601, which permits a "+HH:MM" offset; a + // raw '+' decodes server-side as a space and silently shifts the window. + std::string url = std::format("{}geojson/lsr.geojson?sts={}&ets={}", kIemBase, + percent_encode(start_iso), percent_encode(end_iso)); if (!wfo.empty()) { - url += std::format("&wfo={}", wfo); + url += "&wfo=" + percent_encode(wfo); } - Result body = - body_or_error(with_retry([&] { return impl_->http->get(url); }, impl_->retry), - Feed404::NotFound); + Result body = body_or_error(impl_->rate_limited_get(url), Feed404::NotFound); if (!body) { return std::unexpected(body.error()); } diff --git a/src/core/rate_limit.cpp b/src/core/rate_limit.cpp index a4bff49..4784441 100644 --- a/src/core/rate_limit.cpp +++ b/src/core/rate_limit.cpp @@ -4,14 +4,37 @@ #include "spc/rate_limit.hpp" #include +#include namespace spc { +namespace { + +/// Config is a public aggregate with no validation, so clamp the two fields +/// that can otherwise make the limiter misbehave: a non-positive refill +/// interval divides by zero in refill(), and initial_tokens above max_tokens +/// hands out a burst the bucket was never sized for. +RateLimiter::Config sanitized(RateLimiter::Config config) { + if (config.refill_interval <= std::chrono::milliseconds::zero()) { + config.refill_interval = std::chrono::milliseconds{1000}; + } + config.initial_tokens = std::min(config.initial_tokens, config.max_tokens); + return config; +} + +} // namespace + RateLimiter::RateLimiter(Config config) - : config_(config), tokens_(config_.initial_tokens), + : config_(sanitized(config)), tokens_(config_.initial_tokens), last_refill_(std::chrono::steady_clock::now()), day_start_(std::chrono::floor(std::chrono::system_clock::now())) {} +bool RateLimiter::daily_quota_exhausted() noexcept { + std::lock_guard lock(mutex_); + check_daily_reset(); + return config_.daily_limit > 0 && daily_requests_used_ >= config_.daily_limit; +} + void RateLimiter::check_daily_reset() noexcept { std::chrono::system_clock::time_point now = std::chrono::system_clock::now(); std::chrono::system_clock::time_point current_day = std::chrono::floor(now); @@ -65,6 +88,11 @@ bool RateLimiter::acquire() { if (try_acquire()) { return true; } + // Waiting cannot help: only a UTC-day rollover clears the quota, and + // spinning toward midnight is never what the caller wanted. + if (daily_quota_exhausted()) { + return false; + } std::this_thread::sleep_for(std::chrono::milliseconds(10)); } } @@ -76,6 +104,9 @@ bool RateLimiter::acquire_for(std::chrono::milliseconds max_wait) { if (try_acquire()) { return true; } + if (daily_quota_exhausted()) { + return false; + } std::this_thread::sleep_for(std::chrono::milliseconds(10)); } return false; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index db765f1..de8b983 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -4,6 +4,7 @@ add_executable(spc_tests test_parser.cpp test_corpus.cpp test_arcgis.cpp + test_rate_limit.cpp ) target_link_libraries(spc_tests PRIVATE spc_api diff --git a/tests/test_client.cpp b/tests/test_client.cpp index 8fdb0cd..4eac19a 100644 --- a/tests/test_client.cpp +++ b/tests/test_client.cpp @@ -418,6 +418,40 @@ TEST(ArcGISClientRouting, ActiveWatchesDirectCallersToTheIemClient) { EXPECT_TRUE(transport->requests.empty()); } +// ===== ArchiveClient (IEM) query construction ===== +// +// api.hpp documents start_iso/end_iso as ISO 8601, which permits a "+HH:MM" +// UTC offset. A raw '+' in a query string decodes server-side as a space, so +// an unencoded offset silently queries a different window; a raw '&' in any +// value injects extra parameters. + +TEST(ArchiveClientRouting, PercentEncodesTheWatchTimestamp) { + std::shared_ptr transport = std::make_shared(); + transport->responses = {{200, R"({"features":[]})", {}}}; + ArchiveClient client{transport}; + + ASSERT_TRUE(client.watches("202605191200&x=1")); + + ASSERT_EQ(transport->requests.size(), 1u); + EXPECT_EQ(transport->requests[0], + "https://mesonet.agron.iastate.edu/json/spcwatch.py?ts=202605191200%26x%3D1"); +} + +TEST(ArchiveClientRouting, PercentEncodesIsoOffsetsAndTheWfoFilter) { + std::shared_ptr transport = std::make_shared(); + transport->responses = {{200, R"({"features":[]})", {}}}; + ArchiveClient client{transport}; + + ASSERT_TRUE(client.storm_reports("2026-05-19T12:00:00+00:00", "2026-05-20T12:00:00+00:00", + "ICT&sts=1900-01-01")); + + ASSERT_EQ(transport->requests.size(), 1u); + EXPECT_EQ(transport->requests[0], + "https://mesonet.agron.iastate.edu/geojson/lsr.geojson" + "?sts=2026-05-19T12%3A00%3A00%2B00%3A00&ets=2026-05-20T12%3A00%3A00%2B00%3A00" + "&wfo=ICT%26sts%3D1900-01-01"); +} + TEST(HttpClientLifecycle, ConcurrentClientsShareProcessWideCurlState) { std::vector workers; workers.reserve(16); diff --git a/tests/test_rate_limit.cpp b/tests/test_rate_limit.cpp new file mode 100644 index 0000000..34911f8 --- /dev/null +++ b/tests/test_rate_limit.cpp @@ -0,0 +1,52 @@ +/// @file test_rate_limit.cpp +/// @brief RateLimiter contract: a caller-settable Config must not be able to +/// crash or hang the limiter. + +#include "spc/rate_limit.hpp" + +#include +#include + +namespace { + +using namespace spc; + +TEST(RateLimit, AZeroRefillIntervalIsClampedInsteadOfDividingByZero) { + // Config is a public aggregate with no validation, so a zero interval + // reached `elapsed / refill_interval` and raised SIGFPE on first use. + RateLimiter::Config config; + config.refill_interval = std::chrono::milliseconds{0}; + RateLimiter limiter{config}; + + EXPECT_GT(limiter.config().refill_interval.count(), 0); + EXPECT_TRUE(limiter.try_acquire()); +} + +TEST(RateLimit, InitialTokensAreClampedToTheBucketSize) { + RateLimiter::Config config; + config.max_tokens = 2; + config.initial_tokens = 9; + RateLimiter limiter{config}; + + EXPECT_EQ(limiter.available_tokens(), 2); +} + +TEST(RateLimit, AnExhaustedDailyQuotaFailsFastInsteadOfWaitingForMidnight) { + // try_acquire() returns false permanently once the daily quota is spent, + // and only a UTC-midnight rollover clears it. acquire() with no max_wait + // used to busy-wait toward that rollover; it must give up at once. + RateLimiter::Config config; + config.daily_limit = 1; + RateLimiter limiter{config}; + + ASSERT_TRUE(limiter.acquire()); + + const std::chrono::steady_clock::time_point start = std::chrono::steady_clock::now(); + EXPECT_FALSE(limiter.acquire()); + EXPECT_FALSE(limiter.acquire_for(std::chrono::milliseconds{5000})); + const std::chrono::milliseconds waited = std::chrono::duration_cast( + std::chrono::steady_clock::now() - start); + EXPECT_LT(waited.count(), 1000); +} + +} // namespace From abba1647fb4f2815185acc2b65e6c570263e41fe Mon Sep 17 00:00:00 2001 From: KeviM Date: Fri, 4 Sep 2026 04:39:51 -0700 Subject: [PATCH 05/11] fix(parser): decode numeric-as-string probabilities without a locale dependency std::stod delegates to strtod, which honours the process LC_NUMERIC. On a comma-decimal host -- any app calling setlocale(LC_ALL, "") on a de_DE/fr_FR/pt_BR desktop -- strtod("0.15") consumes only "0" and returns 0 without throwing. Verified locally under LC_NUMERIC=de_DE.UTF-8. The live Day 4-8 static feed carries its probability only as the string LABEL "0.15", so normalized_probability returned 0, parse_day4_8's probability > 0.0 gate dropped the feature, and day4_8() returned a successful but silently empty payload. has_zero_dn had the mirror image: a string dn of "0.0" failed its full-consume check, so the fire-weather no-risk sentinel shipped as a real band. Both are pinned by new tests that skip when de_DE.UTF-8 is unavailable. spc::detail::parse_double is locale-independent: std::from_chars where the standard library implements it for double, and a classic-locale stream otherwise. libc++ only ships floating-point from_chars from version 20 and this project's clang-tidy job builds against libc++ 18, so the fallback is not hypothetical; both branches were compiled and the full suite run against each. Output is byte-identical to a C-locale stod for every value in the fixture corpus, which is what the spc-data byte-identity gate covers. The parse is narrower than stod only in rejecting leading whitespace and a leading '+' -- neither appears in any SPC payload. --- CHANGELOG.md | 17 ++++++ include/spc/models/common.hpp | 14 +++++ src/models/common.cpp | 81 ++++++++++++++++++++++++-- src/models/fire_weather.cpp | 9 ++- tests/CMakeLists.txt | 1 + tests/test_locale.cpp | 103 ++++++++++++++++++++++++++++++++++ 6 files changed, 215 insertions(+), 10 deletions(-) create mode 100644 tests/test_locale.cpp diff --git a/CHANGELOG.md b/CHANGELOG.md index a29f2ee..66c0900 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,23 @@ uses [Semantic Versioning](https://semver.org/spec/v2.0.0.html). 2000 requested; the offset then skipped the gap and the caller got a successful result with a silent hole. `ArcGISPager::advance()` now takes the returned record count. +- **Numeric-as-string probabilities were locale-dependent.** `std::stod` + delegates to `strtod`, which honours the process `LC_NUMERIC`. On a + comma-decimal host — any application that calls `setlocale(LC_ALL, "")` on a + de_DE / fr_FR / pt_BR desktop, as Qt and GTK apps do — `strtod("0.15")` + consumed only `"0"` and returned 0 without throwing. The live Day 4-8 static + feed carries its probability only as the string `"LABEL": "0.15"`, so the + feature was dropped by the `probability > 0.0` gate and + `StaticFeedClient::day4_8()` returned a successful, silently empty payload. + The same parse gated fire weather's no-risk sentinel filter, so a + `"Probability Too Low"` polygon shipped as a real band. Both now go through + a locale-independent `spc::detail::parse_double`, which uses + `std::from_chars` where the standard library provides it for `double` and a + classic-locale stream elsewhere (libc++ only implements floating-point + `from_chars` from version 20; the project's clang-tidy job builds against + libc++ 18). Output is unchanged in the C locale for every value in the + fixture corpus; the parse is narrower than `std::stod` only in rejecting + leading whitespace and a leading `+`, neither of which any SPC payload uses. - **`RateLimiter` crashed on a zero `refill_interval` (SIGFPE).** `RateLimiter::Config` is a public aggregate with no validation, so `RateLimiter{{.refill_interval = 0ms}}` reached an integer division by zero diff --git a/include/spc/models/common.hpp b/include/spc/models/common.hpp index 58b5034..b35de3b 100644 --- a/include/spc/models/common.hpp +++ b/include/spc/models/common.hpp @@ -15,6 +15,7 @@ #include "spc/types.hpp" +#include #include #include #include @@ -34,6 +35,19 @@ const Json* lookup(const Json& obj, const char* key); /// String value of `obj[key]`, or "" if absent / null / non-string. std::string json_string(const Json& obj, const char* key); +/// Parse the leading decimal number of `text` **locale-independently**, and +/// report how many characters it consumed. Returns false when nothing parses. +/// +/// `std::stod` delegates to `strtod`, which honours the process `LC_NUMERIC`: +/// on a comma-decimal host (any app that calls `setlocale(LC_ALL, "")` on a +/// de_DE / fr_FR / pt_BR desktop) `strtod("0.15")` stops at the '.' and +/// returns 0. SPC publishes probabilities as numeric strings, so that turned +/// a live outlook into a successful, silently empty payload. +/// +/// Narrower than `std::stod` by design: leading whitespace and a leading '+' +/// are rejected. No SPC payload uses either form. +bool parse_double(std::string_view text, double& value, std::size_t& consumed); + /// SPC ships `LABEL` as either a string ("SLGT", "5") or a number (5). /// Always returns a numeric view; non-numeric / missing yields 0. double json_number_or_numeric_string(const Json& obj, const char* key); diff --git a/src/models/common.cpp b/src/models/common.cpp index 5776418..cde8d64 100644 --- a/src/models/common.cpp +++ b/src/models/common.cpp @@ -10,6 +10,31 @@ #include +// Floating-point std::from_chars is the locale-independent parse the standard +// intends, but libc++ only implements it from version 20 (the project's own +// clang-tidy job builds against libc++ 18, where the overload is deleted). +// libstdc++ and MSVC advertise it through __cpp_lib_to_chars; libc++ does not +// define that macro at all, so fall back to its version. +// Definable on the command line to exercise the fallback on a toolchain that +// has from_chars. +#ifndef SPC_HAS_FP_FROM_CHARS +#if defined(__cpp_lib_to_chars) && __cpp_lib_to_chars >= 201611L +#define SPC_HAS_FP_FROM_CHARS 1 +#elif defined(_LIBCPP_VERSION) && _LIBCPP_VERSION >= 200000 +#define SPC_HAS_FP_FROM_CHARS 1 +#else +#define SPC_HAS_FP_FROM_CHARS 0 +#endif +#endif + +#if SPC_HAS_FP_FROM_CHARS +#include +#include +#else +#include +#include +#endif + namespace spc { namespace detail { @@ -38,6 +63,51 @@ std::string json_string(const Json& obj, const char* key) { return {}; } +bool parse_double(std::string_view text, double& value, std::size_t& consumed) { + if (text.empty()) { + return false; + } + // Gate the leading character so both implementations below agree on what + // they accept: from_chars takes '-', a digit, '.', or inf/nan, and never + // leading whitespace or '+'. + const char first = text.front(); + const bool leading_ok = first == '-' || first == '.' || (first >= '0' && first <= '9') || + first == 'i' || first == 'I' || first == 'n' || first == 'N'; + if (!leading_ok) { + return false; + } + +#if SPC_HAS_FP_FROM_CHARS + double parsed = 0.0; + const std::from_chars_result result = + std::from_chars(text.data(), text.data() + text.size(), parsed); + if (result.ec != std::errc{}) { + return false; + } + value = parsed; + consumed = static_cast(result.ptr - text.data()); + return true; +#else + std::istringstream stream{std::string{text}}; + stream.imbue(std::locale::classic()); + double parsed = 0.0; + stream >> parsed; + if (stream.fail()) { + return false; + } + value = parsed; + // tellg() reports -1 once the whole buffer was consumed. + consumed = text.size(); + if (!stream.eof()) { + const std::streamoff position = stream.tellg(); + if (position >= 0) { + consumed = static_cast(position); + } + } + return true; +#endif +} + /// SPC ships `LABEL` as either a string ("SLGT", "5") or a number (5). Always /// returns a numeric view; non-numeric / missing yields 0. double json_number_or_numeric_string(const Json& obj, const char* key) { @@ -49,12 +119,13 @@ double json_number_or_numeric_string(const Json& obj, const char* key) { return v->get(); } if (v->is_string()) { + // Was std::stod, whose strtod honours LC_NUMERIC; see parse_double. + // Byte-identical to a C-locale stod for every value in the fixture + // corpus, which is what the spc-data byte-identity gate covers. const std::string s = v->get(); - try { - return std::stod(s); - } catch (...) { - return 0.0; - } + double value = 0.0; + std::size_t consumed = 0; + return parse_double(s, value, consumed) ? value : 0.0; } return 0.0; } diff --git a/src/models/fire_weather.cpp b/src/models/fire_weather.cpp index 89aeff5..e997833 100644 --- a/src/models/fire_weather.cpp +++ b/src/models/fire_weather.cpp @@ -63,13 +63,12 @@ bool has_zero_dn(const Json& props) { if (!dn->is_string()) { return false; } + // Locale-independent: a comma-decimal strtod stops at the '.' of "0.0", + // the full-consume check fails, and the no-risk sentinel ships as a band. const std::string text = dn->get(); + double value = 0.0; std::size_t consumed = 0; - try { - return std::stod(text, &consumed) == 0.0 && consumed == text.size(); - } catch (...) { - return false; - } + return detail::parse_double(text, value, consumed) && value == 0.0 && consumed == text.size(); } std::string label_from_dn(const Json& props, FireWeatherLayer layer) { diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index de8b983..1d674e7 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -5,6 +5,7 @@ add_executable(spc_tests test_corpus.cpp test_arcgis.cpp test_rate_limit.cpp + test_locale.cpp ) target_link_libraries(spc_tests PRIVATE spc_api diff --git a/tests/test_locale.cpp b/tests/test_locale.cpp new file mode 100644 index 0000000..df50a42 --- /dev/null +++ b/tests/test_locale.cpp @@ -0,0 +1,103 @@ +/// @file test_locale.cpp +/// @brief SPC publishes probabilities as numeric strings ("0.15"). Decoding +/// them must not depend on the host application's LC_NUMERIC: a host that +/// calls setlocale(LC_ALL, "") on a comma-decimal desktop (as Qt/GTK apps do) +/// would otherwise get a successful but silently empty outlook. + +#include "spc/models/convective.hpp" +#include "spc/models/fire_weather.hpp" +#include "spc/models/outlook.hpp" + +#include +#include +#include +#include +#include +#include + +namespace { + +using namespace spc; + +std::string slurp(const std::string& name) { + std::ifstream f(std::filesystem::path(SPC_FIXTURES_DIR) / name, std::ios::binary); + EXPECT_TRUE(f.is_open()) << "missing fixture: " << name; + std::stringstream buf; + buf << f.rdbuf(); + return buf.str(); +} + +/// setlocale is process-global, so restore it on every exit path. +class ScopedNumericLocale { +public: + explicit ScopedNumericLocale(const char* name) { + const char* previous = std::setlocale(LC_NUMERIC, nullptr); + saved_ = previous != nullptr ? previous : "C"; + applied_ = std::setlocale(LC_NUMERIC, name) != nullptr; + } + ~ScopedNumericLocale() { (void)std::setlocale(LC_NUMERIC, saved_.c_str()); } + ScopedNumericLocale(const ScopedNumericLocale&) = delete; + ScopedNumericLocale& operator=(const ScopedNumericLocale&) = delete; + ScopedNumericLocale(ScopedNumericLocale&&) = delete; + ScopedNumericLocale& operator=(ScopedNumericLocale&&) = delete; + + [[nodiscard]] bool applied() const noexcept { return applied_; } + +private: + std::string saved_; + bool applied_{false}; +}; + +TEST(LocaleIndependence, Day48StaticFeedKeepsItsProbabilityUnderACommaDecimalLocale) { + // The live Day 4-8 feed carries exactly one probability field and it is a + // string: {"DN": 15, "LABEL": "0.15"}. Under a comma-decimal locale + // strtod("0.15") consumes only "0", so the feature was dropped by the + // probability > 0.0 gate and the whole product came back empty. + const ScopedNumericLocale locale{"de_DE.UTF-8"}; + if (!locale.applied()) { + GTEST_SKIP() << "de_DE.UTF-8 is not installed on this host"; + } + + const Day48OutlookPayload payload = parse_day4_8(slurp("day4prob.nolyr.geojson"), 4); + + ASSERT_EQ(payload.features.size(), 1u); + EXPECT_DOUBLE_EQ(payload.features[0].probability, 0.15); +} + +TEST(LocaleIndependence, ProbabilisticOutlookKeepsItsIsoplethsUnderACommaDecimalLocale) { + const ScopedNumericLocale locale{"de_DE.UTF-8"}; + if (!locale.applied()) { + GTEST_SKIP() << "de_DE.UTF-8 is not installed on this host"; + } + + const ProbOutlookPayload payload = + parse_probabilistic(slurp("arcgis_day1_prob_tornado.geojson"), 1, "tornado"); + + ASSERT_GT(payload.features.size(), 0u); + for (const ProbOutlookFeature& f : payload.features) { + EXPECT_GE(f.probability, 0.01); + EXPECT_LE(f.probability, 1.0); + } +} + +TEST(LocaleIndependence, FireWeatherNoRiskSentinelIsStillDroppedUnderACommaDecimalLocale) { + // has_zero_dn requires the whole string to parse; a comma-decimal strtod + // stops at the '.' of "0.0", the full-consume check fails, and the no-risk + // sentinel polygon ships as a real band. + const ScopedNumericLocale locale{"de_DE.UTF-8"}; + if (!locale.applied()) { + GTEST_SKIP() << "de_DE.UTF-8 is not installed on this host"; + } + const std::string body = R"({"features":[ + {"attributes":{"dn":"0.0"},"geometry":{"rings":[[[0,1],[1,1],[1,0],[0,0],[0,1]]]}}, + {"attributes":{"dn":"5"},"geometry":{"rings":[[[2,1],[3,1],[3,0],[2,0],[2,1]]]}} + ]})"; + + const FireWeatherPayload payload = + parse_fire_weather(body, 1, FireWeatherLayer::DryThunderstorm); + + ASSERT_EQ(payload.features.size(), 1u); + EXPECT_EQ(payload.features[0].label, "IDRT"); +} + +} // namespace From 80c55c2722c31f90888984188d23009f7554ddac Mon Sep 17 00:00:00 2001 From: KeviM Date: Fri, 4 Sep 2026 04:42:55 -0700 Subject: [PATCH 06/11] fix(http): restrict curl to http/https, bound responses, and honour Retry-After HttpClient::get("file:///etc/hosts") returned the file's contents: is_absolute_url() recognises only http:// and https://, so a file:// URL was treated as relative, concatenated onto the empty default base_url, and passed to libcurl unchanged. CURLOPT_PROTOCOLS_STR and CURLOPT_REDIR_PROTOCOLS_STR now pin the transport to http and https, with MAXREDIRS at 10. ClientConfig gains max_response_bytes (64 MB) so an unbounded IEM window cannot buffer an arbitrary body, a full JSON AST of it and the payload at once. Note: this ceiling has no automated test, since exercising it needs a real server and unit tests must not touch the network. with_retry now reads the Retry-After it was already capturing (the delta-seconds form, clamped to max_delay) instead of retrying a server that asked for 60 s after 200 ms. The jitter multiply moved before the max_delay clamp so a delay cannot exceed the documented ceiling, and max_attempts == 0 performs the request once rather than reporting a network error for a request never made. The review also claimed max_attempts == 255 loops forever. It does not: the 'attempt < max_attempts' guard returns at 255 before the counter can wrap. A test pins that. The default User-Agent is generated from PROJECT_VERSION, with a test that fails when the two drift. --- CHANGELOG.md | 18 ++++++ include/spc/http_client.hpp | 21 ++++++- include/spc/retry.hpp | 67 +++++++++++++++++--- src/CMakeLists.txt | 3 + src/http/client.cpp | 35 +++++++++-- tests/CMakeLists.txt | 2 + tests/test_client.cpp | 24 +++++++ tests/test_retry.cpp | 121 ++++++++++++++++++++++++++++++++++++ 8 files changed, 276 insertions(+), 15 deletions(-) create mode 100644 tests/test_retry.cpp diff --git a/CHANGELOG.md b/CHANGELOG.md index 66c0900..9fe9814 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,24 @@ uses [Semantic Versioning](https://semver.org/spec/v2.0.0.html). 2000 requested; the offset then skipped the gap and the caller got a successful result with a silent hole. `ArcGISPager::advance()` now takes the returned record count. +- **`HttpClient` reached the local filesystem.** `is_absolute_url()` only + recognises `http://` and `https://`, so `file:///etc/hosts` was classified as + relative, appended to the empty default `base_url`, and handed to libcurl, + which read the file and returned it as the response body. The transport now + restricts libcurl to `http` and `https` on the request and on redirects, and + caps redirects at 10. +- `ClientConfig::max_response_bytes` (64 MB default) bounds a single response + body. An IEM archive window is caller-chosen and unbounded, and the body was + buffered whole, then parsed into a full JSON AST, then into the payload. +- Retry honours a `Retry-After` header on 429/503 (delta-seconds form, clamped + to `max_delay`) instead of retrying a server that asked for 60 s after + 200 ms. The header was already captured and then discarded. +- Retry jitter is applied before the `max_delay` clamp, not after, so a delay + can no longer exceed the documented ceiling by `jitter_factor`. A + `max_attempts` of 0 now performs the request once instead of returning a + fabricated "Max retry attempts exceeded" for a request never made. +- The default `User-Agent` is generated from `PROJECT_VERSION` instead of a + hard-coded literal, and a test fails if the two ever disagree. - **Numeric-as-string probabilities were locale-dependent.** `std::stod` delegates to `strtod`, which honours the process `LC_NUMERIC`. On a comma-decimal host — any application that calls `setlocale(LC_ALL, "")` on a diff --git a/include/spc/http_client.hpp b/include/spc/http_client.hpp index 3e07998..55c677d 100644 --- a/include/spc/http_client.hpp +++ b/include/spc/http_client.hpp @@ -2,7 +2,14 @@ #include "spc/error.hpp" +/// Set from `PROJECT_VERSION` by the build. The fallback keeps the header +/// usable when it is read outside the project's own CMake targets. +#ifndef SPC_VERSION_STRING +#define SPC_VERSION_STRING "0.2.0" +#endif + #include +#include #include #include #include @@ -26,9 +33,14 @@ struct ClientConfig { /// URLs — one client then serves spc.noaa.gov, the ArcGIS MapServer, /// and the IEM archive interchangeably (spc-data's fetcher behavior). std::string base_url; - std::string user_agent{"spc-cpp/0.2.0 (contact@predictioncast.ai)"}; + std::string user_agent{"spc-cpp/" SPC_VERSION_STRING " (contact@predictioncast.ai)"}; std::chrono::seconds timeout{15}; bool verify_ssl{true}; + /// Hard ceiling on a single response body. A wide IEM archive window is + /// unbounded by construction — the caller chooses the date range — and the + /// body is buffered whole, then parsed into a full JSON AST, then into the + /// payload. Exceeding this aborts the transfer with a network error. + std::size_t max_response_bytes{64UL * 1024UL * 1024UL}; }; /// GET transport boundary used by the high-level clients. @@ -58,8 +70,11 @@ class HttpClient final : public HttpTransport { HttpClient(const HttpClient&) = delete; HttpClient& operator=(const HttpClient&) = delete; - /// GET `path`. If `path` is an absolute URL (starts with http) it is - /// used verbatim; otherwise it is appended to `config().base_url`. + /// GET `path`. If `path` is an absolute `http://` or `https://` URL it is + /// used verbatim; otherwise it is appended to `config().base_url`. Only + /// those two schemes are accepted — every other scheme (`file://`, + /// `dict://`, `scp://`, ...) is refused by the transport, on the request + /// and on any redirect. [[nodiscard]] Result get(std::string_view path) const override; [[nodiscard]] const ClientConfig& config() const noexcept; diff --git a/include/spc/retry.hpp b/include/spc/retry.hpp index 6ecc881..915c862 100644 --- a/include/spc/retry.hpp +++ b/include/spc/retry.hpp @@ -4,10 +4,15 @@ #include "spc/http_client.hpp" #include +#include #include #include #include +#include +#include +#include #include +#include namespace spc { @@ -58,8 +63,6 @@ struct RetryResult { delay_ms *= policy.backoff_multiplier; } - delay_ms = std::min(delay_ms, static_cast(policy.max_delay.count())); - if (policy.jitter_factor > 0) { static thread_local std::mt19937 rng{std::random_device{}()}; std::uniform_real_distribution dist(1.0 - policy.jitter_factor, @@ -67,20 +70,70 @@ struct RetryResult { delay_ms *= dist(rng); } + // Clamp AFTER the jitter: clamping first let jitter carry the delay back + // above the documented ceiling by up to jitter_factor. + delay_ms = std::min(delay_ms, static_cast(policy.max_delay.count())); + delay_ms = std::max(delay_ms, 0.0); + return std::chrono::milliseconds{static_cast(delay_ms)}; } +/// The `Retry-After` delay a 429/503 asked for, or zero. +/// +/// Only the delta-seconds form is parsed; the HTTP-date form falls back to the +/// computed backoff. 429 and 503 are exactly the responses `should_retry` +/// fires on and exactly the ones that carry this header, and `HttpResponse` +/// already captures every header. +[[nodiscard]] inline std::chrono::milliseconds retry_after(const HttpResponse& response) { + for (const std::pair& header : response.headers) { + const std::string& name = header.first; + if (name.size() != 11) { + continue; + } + bool matches = true; + const std::string_view target{"retry-after"}; + for (std::size_t i = 0; i < name.size(); ++i) { + const char lowered = + name[i] >= 'A' && name[i] <= 'Z' ? static_cast(name[i] - 'A' + 'a') : name[i]; + if (lowered != target[i]) { + matches = false; + break; + } + } + if (!matches) { + continue; + } + std::int64_t seconds = 0; + const std::from_chars_result parsed = std::from_chars( + header.second.data(), header.second.data() + header.second.size(), seconds); + if (parsed.ec == std::errc{} && seconds > 0) { + return std::chrono::milliseconds{seconds * 1000}; + } + return std::chrono::milliseconds{0}; + } + return std::chrono::milliseconds{0}; +} + /// Execute an HTTP operation with exponential-backoff retry. template [[nodiscard]] Result with_retry(Operation&& operation, const RetryPolicy& policy) { std::chrono::milliseconds total_delay{0}; + // max_attempts == 0 would otherwise skip the operation entirely and report + // a fabricated network error for a request that was never made. Iterate a + // wider counter so the top of the uint8 range cannot wrap. + const std::uint16_t attempts = std::max(1, policy.max_attempts); - for (std::uint8_t attempt = 1; attempt <= policy.max_attempts; ++attempt) { + for (std::uint16_t attempt = 1; attempt <= attempts; ++attempt) { Result result = operation(); + const std::uint8_t attempt_number = static_cast(attempt); if (result.has_value()) { - if (should_retry(*result, policy) && attempt < policy.max_attempts) { - std::chrono::milliseconds delay = calculate_retry_delay(attempt, policy); + if (should_retry(*result, policy) && attempt < attempts) { + // Respect a server that told us how long to wait. + const std::chrono::milliseconds requested = retry_after(*result); + const std::chrono::milliseconds delay = + std::min(std::max(calculate_retry_delay(attempt_number, policy), requested), + policy.max_delay); total_delay += delay; std::this_thread::sleep_for(delay); continue; @@ -88,8 +141,8 @@ template return result; } - if (should_retry(result.error(), policy) && attempt < policy.max_attempts) { - std::chrono::milliseconds delay = calculate_retry_delay(attempt, policy); + if (should_retry(result.error(), policy) && attempt < attempts) { + std::chrono::milliseconds delay = calculate_retry_delay(attempt_number, policy); total_delay += delay; std::this_thread::sleep_for(delay); continue; diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index a33412c..dc24afe 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -22,6 +22,9 @@ add_library(spc_http STATIC ) spc_configure_target(spc_http) target_link_libraries(spc_http PUBLIC spc_core CURL::libcurl) +# The default User-Agent lives in an installed public header, so the version +# has to reach consumers too. PUBLIC exports it through spcTargets.cmake. +target_compile_definitions(spc_http PUBLIC SPC_VERSION_STRING="${PROJECT_VERSION}") target_include_directories(spc_http PUBLIC $ $ diff --git a/src/http/client.cpp b/src/http/client.cpp index aea9513..53f38f3 100644 --- a/src/http/client.cpp +++ b/src/http/client.cpp @@ -8,9 +8,23 @@ namespace spc { namespace { +/// Accumulates the body, refusing to grow past a ceiling. Returning a short +/// count makes libcurl abort the transfer with CURLE_WRITE_ERROR. +struct BodySink { + std::string body; + std::size_t limit{0}; + bool overflowed{false}; +}; + std::size_t write_cb(char* ptr, std::size_t size, std::size_t nmemb, void* user) { - static_cast(user)->append(ptr, size * nmemb); - return size * nmemb; + BodySink* sink = static_cast(user); + const std::size_t chunk = size * nmemb; + if (sink->limit > 0 && sink->body.size() + chunk > sink->limit) { + sink->overflowed = true; + return 0; + } + sink->body.append(ptr, chunk); + return chunk; } std::size_t header_cb(char* buffer, std::size_t size, std::size_t nitems, void* userdata) { @@ -101,16 +115,23 @@ Result HttpClient::get(std::string_view path) const { CURL* curl = impl_->curl; const std::string url = is_absolute_url(path) ? std::string{path} : impl_->config.base_url + std::string{path}; - std::string body; + BodySink sink; + sink.limit = impl_->config.max_response_bytes; std::vector> response_headers; curl_easy_reset(curl); curl_easy_setopt(curl, CURLOPT_URL, url.c_str()); curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, &write_cb); - curl_easy_setopt(curl, CURLOPT_WRITEDATA, &body); + curl_easy_setopt(curl, CURLOPT_WRITEDATA, &sink); curl_easy_setopt(curl, CURLOPT_HEADERFUNCTION, &header_cb); curl_easy_setopt(curl, CURLOPT_HEADERDATA, &response_headers); curl_easy_setopt(curl, CURLOPT_TIMEOUT, static_cast(impl_->config.timeout.count())); + // This SDK speaks to public HTTP(S) endpoints only. Without this libcurl + // happily honours file://, dict://, scp:// and friends, and a path built + // from user input becomes a local-file read. + curl_easy_setopt(curl, CURLOPT_PROTOCOLS_STR, "http,https"); + curl_easy_setopt(curl, CURLOPT_REDIR_PROTOCOLS_STR, "http,https"); + curl_easy_setopt(curl, CURLOPT_MAXREDIRS, 10L); // Parity with spc-data/src/fetcher.cpp: curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1L); curl_easy_setopt(curl, CURLOPT_NOSIGNAL, 1L); @@ -121,6 +142,10 @@ Result HttpClient::get(std::string_view path) const { CURLcode rc = curl_easy_perform(curl); if (rc != CURLE_OK) { + if (sink.overflowed) { + return std::unexpected(Error::network( + "response exceeded ClientConfig::max_response_bytes")); + } return std::unexpected(Error::network(curl_easy_strerror(rc))); } @@ -129,7 +154,7 @@ Result HttpClient::get(std::string_view path) const { return HttpResponse{ static_cast(http_code), - std::move(body), + std::move(sink.body), std::move(response_headers), }; } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 1d674e7..ab1a891 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -6,6 +6,7 @@ add_executable(spc_tests test_arcgis.cpp test_rate_limit.cpp test_locale.cpp + test_retry.cpp ) target_link_libraries(spc_tests PRIVATE spc_api @@ -13,6 +14,7 @@ target_link_libraries(spc_tests PRIVATE ) target_compile_definitions(spc_tests PRIVATE SPC_FIXTURES_DIR="${CMAKE_CURRENT_SOURCE_DIR}/fixtures" + SPC_PROJECT_VERSION="${PROJECT_VERSION}" ) target_compile_options(spc_tests PRIVATE -Wall -Wextra -Wpedantic) include(GoogleTest) diff --git a/tests/test_client.cpp b/tests/test_client.cpp index 4eac19a..7097e2f 100644 --- a/tests/test_client.cpp +++ b/tests/test_client.cpp @@ -452,6 +452,30 @@ TEST(ArchiveClientRouting, PercentEncodesIsoOffsetsAndTheWfoFilter) { "&wfo=ICT%26sts%3D1900-01-01"); } +TEST(HttpClientLifecycle, DefaultUserAgentCarriesTheProjectVersion) { + // The UA string is a literal in an installed header; nothing tied it to + // project(spc-cpp VERSION ...), so a release bump left every outbound + // request to NOAA and IEM announcing the previous version. + const ClientConfig config; + + EXPECT_NE(config.user_agent.find(SPC_PROJECT_VERSION), std::string::npos) + << "user_agent \"" << config.user_agent << "\" does not carry version " + << SPC_PROJECT_VERSION; +} + +TEST(HttpClientLifecycle, RefusesEverySchemeOtherThanHttpAndHttps) { + // is_absolute_url() only recognises http:// and https://, so a file:// URL + // was treated as relative, concatenated onto the empty default base_url, + // and handed to libcurl — which read the local file and returned it as the + // response body. Nothing in this SDK's scope should reach the filesystem. + const HttpClient client; + + const Result result = client.get("file:///etc/hosts"); + + ASSERT_FALSE(result) << "file:// must not be fetched"; + EXPECT_EQ(result.error().code, ErrorCode::NetworkError); +} + TEST(HttpClientLifecycle, ConcurrentClientsShareProcessWideCurlState) { std::vector workers; workers.reserve(16); diff --git a/tests/test_retry.cpp b/tests/test_retry.cpp new file mode 100644 index 0000000..ce7892a --- /dev/null +++ b/tests/test_retry.cpp @@ -0,0 +1,121 @@ +/// @file test_retry.cpp +/// @brief Retry arithmetic and the Retry-After contract. + +#include "spc/http_client.hpp" +#include "spc/retry.hpp" + +#include +#include +#include +#include + +namespace { + +using namespace spc; + +TEST(Retry, JitterCannotPushTheDelayPastMaxDelay) { + // The clamp used to run before the jitter multiply, so a delay pinned at + // max_delay came back up to jitter_factor above the documented ceiling. + RetryPolicy policy; + policy.initial_delay = std::chrono::milliseconds{10000}; + policy.max_delay = std::chrono::milliseconds{30000}; + policy.jitter_factor = 0.5; + + for (std::uint8_t attempt = 1; attempt <= 8; ++attempt) { + for (int sample = 0; sample < 64; ++sample) { + const std::chrono::milliseconds delay = calculate_retry_delay(attempt, policy); + EXPECT_LE(delay.count(), policy.max_delay.count()); + EXPECT_GE(delay.count(), 0); + } + } +} + +TEST(Retry, ZeroMaxAttemptsStillPerformsTheRequestOnce) { + // The loop `for (attempt = 1; attempt <= max_attempts; ...)` never ran, so + // with_retry reported a network error for a request never made. + RetryPolicy policy; + policy.max_attempts = 0; + int calls = 0; + + const Result result = with_retry( + [&]() -> Result { + ++calls; + return HttpResponse{200, "{}", {}}; + }, + policy); + + EXPECT_EQ(calls, 1); + ASSERT_TRUE(result); + EXPECT_EQ(result->status_code, 200); +} + +TEST(Retry, MaxAttemptsAtTheTopOfTheRangeTerminates) { + // The review claimed `++attempt` wrapping 255 -> 0 made this loop run + // forever. It does not: the `attempt < max_attempts` guard returns the + // result at attempt 255 before the counter can wrap. Pinned so the wider + // loop counter keeps that true. + RetryPolicy policy; + policy.max_attempts = 255; + policy.initial_delay = std::chrono::milliseconds{0}; + policy.max_delay = std::chrono::milliseconds{0}; + policy.jitter_factor = 0.0; + int calls = 0; + + const Result result = with_retry( + [&]() -> Result { + ++calls; + return std::unexpected(Error::network("boom")); + }, + policy); + + EXPECT_EQ(calls, 255); + ASSERT_FALSE(result); +} + +TEST(Retry, HonoursRetryAfterInsteadOfTheComputedBackoff) { + // 429 and 503 are exactly the responses that carry Retry-After, and the + // SDK already captures every header. A server asking for 60 s was being + // retried after 200 ms. + RetryPolicy policy; + policy.max_attempts = 2; + policy.initial_delay = std::chrono::milliseconds{1}; + policy.max_delay = std::chrono::milliseconds{150}; + policy.jitter_factor = 0.0; + + const std::chrono::steady_clock::time_point start = std::chrono::steady_clock::now(); + const Result result = with_retry( + []() -> Result { + return HttpResponse{429, "slow down", {{"Retry-After", "60"}}}; + }, + policy); + const std::chrono::milliseconds waited = std::chrono::duration_cast( + std::chrono::steady_clock::now() - start); + + ASSERT_TRUE(result); + EXPECT_EQ(result->status_code, 429); + // 60 s clamped to max_delay, not the 1 ms the backoff would have chosen. + EXPECT_GE(waited.count(), 100); +} + +TEST(Retry, IgnoresAnUnparseableRetryAfterAndFallsBackToTheBackoff) { + RetryPolicy policy; + policy.max_attempts = 2; + policy.initial_delay = std::chrono::milliseconds{1}; + policy.max_delay = std::chrono::milliseconds{5000}; + policy.jitter_factor = 0.0; + + const std::chrono::steady_clock::time_point start = std::chrono::steady_clock::now(); + const Result result = with_retry( + []() -> Result { + // The HTTP-date form, which the SDK does not parse. + return HttpResponse{503, "", {{"retry-after", "Wed, 21 Oct 2026 07:28:00 GMT"}}}; + }, + policy); + const std::chrono::milliseconds waited = std::chrono::duration_cast( + std::chrono::steady_clock::now() - start); + + ASSERT_TRUE(result); + EXPECT_LT(waited.count(), 1000); +} + +} // namespace From fec03d6d116e7df2b119036cdffb81debae35f70 Mon Sep 17 00:00:00 2001 From: KeviM Date: Fri, 4 Sep 2026 04:46:41 -0700 Subject: [PATCH 07/11] chore: make the ISA baseline opt-in, tighten CI, and close the doc gaps Default Release builds passed -march=x86-64-v3 whenever the compiler accepted it. check_cxx_compiler_flag says nothing about the CPU the built library will run on, and this SDK installs an exported package, so the build host and the run host are routinely different -- 'make build' produced an archive that SIGILLs on pre-Haswell/pre-Zen hardware. It is now behind SPC_TUNE_X86_64_V3, defaulting off, with -mtune=generic. ci.yml gains a top-level 'permissions: contents: read' (it runs on pull_request and executes third-party build scripts) and pins both actions to full commit SHAs. ArcGISClient::query_storm_reports() is a permanently failing stub; it now carries the deprecation attribute, doc comment, README line and CHANGELOG entry its sibling query_active_watches() already had. query_fire_weather's all-or-nothing contract across its two merged layers is documented and pinned by a test. The Esri/GeoJSON parity gate now reads all seven captured fixture pairs instead of one; the test named for probabilistic parity never opened the Esri side at all. CLAUDE.md and CONTRIBUTING.md list the fixtures-check and lint-md gates CI enforces, CONTRIBUTING names all seven jobs, make help lists every target, and the README gains src/core/, query_layer, and the heading the CHANGELOG cross-reference pointed at. --- .github/workflows/ci.yml | 23 ++++--- CHANGELOG.md | 26 ++++++++ CLAUDE.md | 6 ++ CMakeLists.txt | 14 +++- CONTRIBUTING.md | 15 +++-- Makefile | 6 ++ README.md | 11 +++ include/spc/api.hpp | 11 ++- tests/test_arcgis.cpp | 140 ++++++++++++++++++++++++++++++++++++--- tests/test_client.cpp | 21 ++++++ 10 files changed, 250 insertions(+), 23 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9f16e9a..586a5bf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -6,11 +6,18 @@ on: pull_request: workflow_dispatch: +# Least privilege by default. The workflow runs on pull_request, and +# consumer-smoke plus the FetchContent builds clone and execute third-party +# code, so no job should inherit a writable token. A job that needs more grants +# itself a narrower block. +permissions: + contents: read + jobs: build-linux: runs-on: ubuntu-24.04 steps: - - uses: actions/checkout@v7.0.1 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Install dependencies run: | @@ -34,7 +41,7 @@ jobs: build-macos: runs-on: macos-latest steps: - - uses: actions/checkout@v7.0.1 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Install dependencies run: | @@ -53,13 +60,13 @@ jobs: markdown-lint: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v7.0.1 - - uses: DavidAnson/markdownlint-cli2-action@v23 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - uses: DavidAnson/markdownlint-cli2-action@ded1f9488f68a970bc66ea5619e13e9b52e601cd # v23 sanitizer: runs-on: ubuntu-24.04 steps: - - uses: actions/checkout@v7.0.1 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Install dependencies run: | sudo apt-get update @@ -78,7 +85,7 @@ jobs: thread-sanitizer: runs-on: ubuntu-24.04 steps: - - uses: actions/checkout@v7.0.1 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Install dependencies run: | sudo apt-get update @@ -97,7 +104,7 @@ jobs: clang-tidy: runs-on: ubuntu-24.04 steps: - - uses: actions/checkout@v7.0.1 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Install dependencies run: | sudo apt-get update @@ -120,7 +127,7 @@ jobs: consumer-smoke: runs-on: ubuntu-24.04 steps: - - uses: actions/checkout@v7.0.1 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Install dependencies run: | sudo apt-get update diff --git a/CHANGELOG.md b/CHANGELOG.md index 9fe9814..83bfff8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -106,8 +106,34 @@ uses [Semantic Versioning](https://semver.org/spec/v2.0.0.html). explicitly: `parse_fire_weather(body, day, FireWeatherLayer::Outlook)` restores the old behaviour where that was in fact the right layer. +- Release builds no longer default to `-march=x86-64-v3`. The probe only + proved the *compiler* accepted the flag, never that the run host has + AVX2/BMI2/FMA — and this is an installable SDK, so the build host and the run + host are routinely different. `-mtune=generic` is the default; set + `SPC_TUNE_X86_64_V3=ON` to opt in to the non-portable artifact. +- The Esri-vs-GeoJSON parity gate now reads every captured fixture pair (three + categorical, four probabilistic). The test named for probabilistic parity + only ever opened the GeoJSON side, so `parse_esri_rings` was pinned by one + categorical layer. +- `ci.yml` declares `permissions: contents: read` at the top level — it runs on + `pull_request` and executes third-party build scripts — and pins both actions + to full commit SHAs instead of mutable tags. +- `CLAUDE.md` and `CONTRIBUTING.md` list the `fixtures-check` and `lint-md` + gates that CI enforces, `CONTRIBUTING.md` names all seven CI jobs, `make help` + lists every target, and the README documents `src/core/`, `query_layer`, and + the `JSON library: Glaze (divergence note)` heading the CHANGELOG points at. + +### Deprecated + +- `ArcGISClient::query_storm_reports()`. The SPC MapServer has no Local Storm + Report layer, so the method always failed without touching the network — it + now carries the attribute and doc comment its sibling + `query_active_watches()` already had. Use `ArchiveClient::storm_reports()`. + ### Added +- `ArcGISClient::query_fire_weather()` documents its all-or-nothing contract: + it merges two layers, and a failure on either discards both. - `Error::from_arcgis`, for ArcGIS logical failure envelopes. It keeps the ArcGIS code in `Error::http_status` only while that code is HTTP-shaped (100..599) and records it in `Error::detail`, so a code such as 1000 can no diff --git a/CLAUDE.md b/CLAUDE.md index dee2185..a66d702 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -6,6 +6,8 @@ make build make test make lint +make fixtures-check # fixture-provenance gate; CI fails on a stale SHA256SUMS +make lint-md # markdown-lint gate over **/*.md make test-consumers python3 tools/verify_arcgis_metadata.py # live, opt-in ``` @@ -47,3 +49,7 @@ Glaze 8.3 parses loose SPC JSON. GoogleTest 1.18 runs the unit suite. The tag must match `project(spc-cpp VERSION ...)`. A `vX.Y.Z` tag triggers the release workflow, which reads the matching `CHANGELOG.md` section. + +The default `ClientConfig::user_agent` is generated from `PROJECT_VERSION`, so +a bump carries automatically. `HttpClientLifecycle.DefaultUserAgentCarriesTheProjectVersion` +fails if that ever stops being true. diff --git a/CMakeLists.txt b/CMakeLists.txt index 821bab5..c39f590 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -14,6 +14,8 @@ option(SPC_BUILD_TESTS "Build tests" ON) option(SPC_BUILD_EXAMPLES "Build examples" ON) option(SPC_ENABLE_LTO "Enable Link Time Optimization" ON) option(SPC_NATIVE_ARCH "Use -march=native for CPU-specific tuning" OFF) +option(SPC_TUNE_X86_64_V3 + "Build Release with -march=x86-64-v3 (AVX2/BMI2/FMA). Makes the artifact non-portable." OFF) option(SPC_ENABLE_CLANG_TIDY "Run clang-tidy while compiling project targets" OFF) option(SPC_WARNINGS_AS_ERRORS "Treat project warnings as errors" OFF) @@ -62,14 +64,24 @@ if(NOT MSVC) if(SPC_NATIVE_ARCH) set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -march=native") message(STATUS "Using -march=native for CPU-specific optimizations") - else() + elseif(SPC_TUNE_X86_64_V3) + # Opt-in only. check_cxx_compiler_flag proves the *compiler* accepts + # the flag, never that the *run* host has AVX2/BMI2/FMA -- and this is + # an installable SDK (install(EXPORT) + find_package(spc)), so the + # build host and the run host are routinely different. Defaulting this + # on made every stock Release build SIGILL on pre-Haswell / pre-Zen + # hardware and inside VMs that do not expose AVX2. include(CheckCXXCompilerFlag) check_cxx_compiler_flag("-march=x86-64-v3" COMPILER_SUPPORTS_X86_64_V3) if(COMPILER_SUPPORTS_X86_64_V3) set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -march=x86-64-v3") + message(STATUS "Release builds target x86-64-v3; the artifact is not portable") else() + message(WARNING "SPC_TUNE_X86_64_V3 requested but the compiler rejects the flag") set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -mtune=generic") endif() + else() + set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -mtune=generic") endif() endif() diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 600903d..a3db55c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -15,11 +15,15 @@ cd spc-cpp sudo apt install -y build-essential cmake clang-format \ libcurl4-openssl-dev -make build # CMake configure + Release build -make test # Run unit tests (ctest) -make lint # clang-format --dry-run + cpp_auto_audit +make build # CMake configure + Release build +make test # Run unit tests (ctest) +make lint # clang-format --dry-run + cpp_auto_audit +make fixtures-check # fixture-provenance gate (stale SHA256SUMS fails CI) +make lint-md # markdown-lint gate over **/*.md ``` +`make help` lists every target. + ## Code style - `.clang-format` (LLVM base, **tabs**, width 4, 100-col). `make @@ -45,8 +49,9 @@ their own product-specific severity mappers. ## PRs -- Branch, push, open a PR against `main`. CI (linux + macos + - markdown-lint) must be green. +- Branch, push, open a PR against `main`. Every CI job must be green: + `build-linux`, `build-macos`, `markdown-lint`, `sanitizer`, + `thread-sanitizer`, `clang-tidy` and `consumer-smoke`. - Conventional-commit subject lines. - Update `CHANGELOG.md` under `[Unreleased]`. - A maintainer merges with a **merge commit** (history reachability diff --git a/Makefile b/Makefile index 877d86b..5741ba8 100644 --- a/Makefile +++ b/Makefile @@ -108,9 +108,15 @@ help: @echo " make build - Configure and build the SDK (Release)" @echo " make debug - Configure and build the SDK (Debug)" @echo " make test - Run tests" + @echo " make test-consumers - Installed-package + FetchContent consumer checks" @echo " make lint - Check formatting + cpp_auto_audit" + @echo " make lint-md - Markdown lint (CI gate)" @echo " make format - Format code in place" + @echo " make format-md - Fix markdown lint findings in place" + @echo " make fixtures-check - Verify fixture provenance (CI gate)" @echo " make coverage - Generate code coverage report (requires lcov)" + @echo " make install-hooks - Install the pre-commit hook" + @echo " make pre-commit - Format, then lint" @echo " make clean - Remove build artifacts" @echo " make help - Show this help" @echo "" diff --git a/README.md b/README.md index 49dec2b..1c074c0 100644 --- a/README.md +++ b/README.md @@ -57,6 +57,14 @@ publishes CAP and WFO polygons, but it omits the SPC parameters represented by `ArcGISClient::query_active_watches()` method is deprecated and returns `InvalidRequest` instead of fabricating incomplete watches. +Storm reports are the same story: the SPC MapServer publishes no Local Storm +Report layer, so `ArcGISClient::query_storm_reports()` is deprecated and always +fails. Use `ArchiveClient::storm_reports()`. + +For anything the typed methods do not cover, +`ArcGISClient::query_layer(service, layer_id, params)` runs a raw paged query +against any of the three NOAA MapServers and returns the page bodies. + ### Custom networking `HttpClient` is the default GET transport. Implement `HttpTransport` and pass @@ -94,6 +102,7 @@ target_link_libraries(myapp PRIVATE spc::spc) | --- | --- | | `include/spc/` | Public clients, models, errors, and geometry helpers | | `src/api/` | Static feed, ArcGIS, and IEM routing | +| `src/core/` | Errors, geometry, and the rate limiter | | `src/models/` | Glaze-backed GeoJSON and Esri parsers | | `src/http/` | libcurl transport | | `tests/` | Public client tests and captured parser fixtures | @@ -117,6 +126,8 @@ The normal unit suite does not depend on NOAA availability. The metadata command compares the live ArcGIS layer IDs and names with the checked 2026-09-03 contract, then queries all 39 feature layers. +### JSON library: Glaze (divergence note) + SPC payloads vary in key case, numeric representation, and geometry type. The parsers use Glaze 8.3's generic JSON tree to handle those shapes. The original convective parser stays aligned with the downstream `spc-data` service; new diff --git a/include/spc/api.hpp b/include/spc/api.hpp index 6f67f28..45e4945 100644 --- a/include/spc/api.hpp +++ b/include/spc/api.hpp @@ -93,6 +93,11 @@ class ArcGISClient { [[nodiscard]] Result query_conditional_intensity(std::int32_t day, const std::string& hazard); [[nodiscard]] Result query_day4_8(std::int32_t day); + /// Merges both published fire-weather layers for `day`. All-or-nothing: if + /// either layer fails, nothing is returned, including the features already + /// parsed from the other. `FireWeatherPayload` cannot represent a partial + /// result, so a caller that needs one layer independently should use + /// `query_layer(ArcGISService::FireWeather, ...)`. [[nodiscard]] Result query_fire_weather(std::int32_t day); /// NOAA's WWA polygons do not contain the SPC watch parameters represented /// by WatchPayload. Use ArchiveClient::watches() for active SPC watches. @@ -100,7 +105,11 @@ class ArcGISClient { "use ArchiveClient::watches() for SPC watch data")]] [[nodiscard]] Result query_active_watches(); [[nodiscard]] Result query_active_md(); - [[nodiscard]] Result query_storm_reports(); + /// The SPC MapServer publishes no Local Storm Report layer, so this always + /// fails without touching the network. Use `ArchiveClient::storm_reports()`. + [[deprecated("use ArchiveClient::storm_reports(); the SPC MapServer has no LSR " + "layer")]] [[nodiscard]] Result + query_storm_reports(); /// Raw paged query against one of the documented NOAA MapServers. [[nodiscard]] Result> diff --git a/tests/test_arcgis.cpp b/tests/test_arcgis.cpp index e11e9c6..a09261d 100644 --- a/tests/test_arcgis.cpp +++ b/tests/test_arcgis.cpp @@ -19,12 +19,16 @@ #include "spc/models/storm_report.hpp" #include "spc/models/watch.hpp" +#include #include +#include #include #include #include #include #include +#include +#include namespace { @@ -181,14 +185,134 @@ TEST(ArcGISParity, EsriRingsMatchGeoJsonForDay1Categorical) { "genuinely diverged from the verbatim GeoJSON path (not source quantization)"; } -TEST(ArcGISParity, EsriProbTornadoMatchesGeoJsonProb) { - const ProbOutlookPayload gj = - parse_probabilistic(slurp("arcgis_day1_prob_tornado.geojson"), 1, "tornado"); - ASSERT_GT(gj.features.size(), 0u); - for (const ProbOutlookFeature& f : gj.features) { - EXPECT_GT(f.probability, 0.0); - EXPECT_LT(f.probability, 1.0); - EXPECT_FALSE(f.rings.empty()); +// Grid-probe membership agreement between two ring sets, on the same terms as +// the day-1 categorical gate above: probes within ~5 m of either boundary are +// skipped, everything else must classify identically. +struct MembershipProbe { + std::size_t compared = 0; + std::size_t agreed = 0; +}; + +void probe_membership(const std::vector& reference, const std::vector& candidate, + MembershipProbe& probe) { + constexpr double kSkip = 5.0e-5; + constexpr int kN = 40; + + double minx = 1e18; + double miny = 1e18; + double maxx = -1e18; + double maxy = -1e18; + for (const Polygon& r : reference) { + for (const LonLat& p : r) { + minx = std::min(minx, p.lon); + maxx = std::max(maxx, p.lon); + miny = std::min(miny, p.lat); + maxy = std::max(maxy, p.lat); + } + } + const double pad = 0.5; + minx -= pad; + maxx += pad; + miny -= pad; + maxy += pad; + + for (int ix = 0; ix <= kN; ++ix) { + for (int iy = 0; iy <= kN; ++iy) { + const double px = minx + (maxx - minx) * (static_cast(ix) / kN); + const double py = miny + (maxy - miny) * (static_cast(iy) / kN); + if (dist_to_boundary(px, py, reference) < kSkip || + dist_to_boundary(px, py, candidate) < kSkip) { + continue; + } + ++probe.compared; + if (inside_any(px, py, reference) == inside_any(px, py, candidate)) { + ++probe.agreed; + } + } + } +} + +// Every captured Esri/GeoJSON fixture pair, not just the day-1 categorical +// one. The probabilistic pairs previously had no reader at all: the test named +// for Esri-vs-GeoJSON probabilistic parity only ever opened the GeoJSON side, +// so parse_esri_rings' hole-dropping rule was pinned by one categorical layer. +TEST(ArcGISParity, EveryCapturedEsriFixtureMatchesItsGeoJsonTwin) { + struct Pair { + std::string stem; + std::int32_t day; + std::string hazard; ///< empty for the categorical layers + }; + const std::vector pairs = { + {"arcgis_day1_categorical", 1, ""}, {"arcgis_day2_categorical", 2, ""}, + {"arcgis_day3_categorical", 3, ""}, {"arcgis_day1_prob_tornado", 1, "tornado"}, + {"arcgis_day1_prob_hail", 1, "hail"}, {"arcgis_day1_prob_wind", 1, "wind"}, + {"arcgis_day2_prob_wind", 2, "wind"}, + }; + + for (const Pair& pair : pairs) { + // Label -> rings, from the verbatim GeoJSON walker. + std::vector>> reference; + if (pair.hazard.empty()) { + const CategoricalOutlookPayload gj = + parse_categorical(slurp(pair.stem + ".geojson"), pair.day); + for (const OutlookFeature& f : gj.features) { + reference.emplace_back(f.label, f.rings); + } + } else { + const ProbOutlookPayload gj = + parse_probabilistic(slurp(pair.stem + ".geojson"), pair.day, pair.hazard); + for (const ProbOutlookFeature& f : gj.features) { + EXPECT_GT(f.probability, 0.0) << pair.stem; + EXPECT_LT(f.probability, 1.0) << pair.stem; + // ProbOutlookFeature keeps no label; the isopleth value is the + // band identity, and both sides derive it from the same string. + reference.emplace_back(std::format("{:.6f}", f.probability), f.rings); + } + } + ASSERT_GT(reference.size(), 0u) << pair.stem; + + const glz::expected root = + detail::parse_root(slurp(pair.stem + ".esri.json")); + ASSERT_TRUE(root.has_value()) << pair.stem; + const Json* feats = detail::lookup(*root, "features"); + ASSERT_NE(feats, nullptr) << pair.stem; + ASSERT_TRUE(feats->is_array()) << pair.stem; + + MembershipProbe probe; + std::size_t matched = 0; + for (const glz::generic& feat : feats->get_array()) { + const Json* attrs = detail::lookup(feat, "attributes"); + const Json* geom = detail::lookup(feat, "geometry"); + if (attrs == nullptr || geom == nullptr) { + continue; + } + const std::string label = + pair.hazard.empty() + ? detail::json_string(*attrs, "label") + : std::format("{:.6f}", detail::normalized_probability(*attrs)); + const std::vector* expected = nullptr; + for (const std::pair>& band : reference) { + if (band.first == label) { + expected = &band.second; + break; + } + } + if (expected == nullptr) { + continue; + } + const std::vector esri_rings = detail::parse_esri_rings(*geom); + ASSERT_FALSE(esri_rings.empty()) << pair.stem << " band " << label; + ++matched; + probe_membership(*expected, esri_rings, probe); + } + + EXPECT_EQ(matched, reference.size()) + << pair.stem << ": Esri and GeoJSON disagree on the band set"; + EXPECT_GT(probe.compared, 100u) << pair.stem << ": too few clear-of-boundary probes"; + EXPECT_EQ(probe.agreed, probe.compared) + << pair.stem << ": Esri vs GeoJSON disagreed on " + << (probe.compared - probe.agreed) << "/" << probe.compared + << " unambiguous interior/exterior probes"; } } diff --git a/tests/test_client.cpp b/tests/test_client.cpp index 7097e2f..6343caa 100644 --- a/tests/test_client.cpp +++ b/tests/test_client.cpp @@ -215,6 +215,27 @@ TEST(ArcGISClientRouting, CoversEveryPublishedFireWeatherFeatureLayer) { } } +TEST(ArcGISClientRouting, FireWeatherIsAllOrNothingAcrossItsTwoMergedLayers) { + // query_fire_weather merges two layers per day. If the second fails, the + // features already parsed from the first are discarded and the caller gets + // an error with no indication that half the product was retrieved. + // FireWeatherPayload cannot represent the partial state, so the contract + // is all-or-nothing; this pins it. + std::shared_ptr transport = std::make_shared(); + const std::string body = R"({"features":[ + {"attributes":{"dn":5},"geometry":{"rings":[[[0,1],[1,1],[1,0],[0,0],[0,1]]]}} + ],"exceededTransferLimit":false})"; + // A 400 is not retryable, so the second layer fails on its first request. + transport->responses = {{200, body, {}}, {400, "bad request", {}}}; + ArcGISClient client{transport}; + + const Result result = client.query_fire_weather(1); + + ASSERT_FALSE(result); + EXPECT_EQ(result.error().code, ErrorCode::InvalidRequest); + EXPECT_EQ(transport->requests.size(), 2u); +} + TEST(ArcGISClientRouting, RejectsUnsupportedProductsBeforeNetworkAccess) { std::shared_ptr transport = std::make_shared(); ArcGISClient client{transport}; From be61e20a341d489fb1fa016dcb890488a6f775b5 Mon Sep 17 00:00:00 2001 From: KeviM Date: Fri, 4 Sep 2026 04:48:16 -0700 Subject: [PATCH 08/11] style: apply clang-format --- include/spc/error.hpp | 2 +- src/api/client.cpp | 23 ++++++++++------------- src/http/client.cpp | 4 ++-- tests/test_arcgis.cpp | 27 ++++++++++++--------------- tests/test_client.cpp | 4 +++- 5 files changed, 28 insertions(+), 32 deletions(-) diff --git a/include/spc/error.hpp b/include/spc/error.hpp index 949004a..5d9058a 100644 --- a/include/spc/error.hpp +++ b/include/spc/error.hpp @@ -123,7 +123,7 @@ struct Error { /// `Feed404::NoActiveOutlook` (SPC static products only) yields /// `FeedUnavailable`; the default `Feed404::NotFound` yields `NotFound`. [[nodiscard]] static Error from_response(int status, const std::string& body, - Feed404 semantics = Feed404::NotFound); + Feed404 semantics = Feed404::NotFound); /// Create an Error from an ArcGIS logical failure envelope /// (`{"error":{"code":...,"message":...}}`), which the MapServer reports diff --git a/src/api/client.cpp b/src/api/client.cpp index 32498c1..95139f0 100644 --- a/src/api/client.cpp +++ b/src/api/client.cpp @@ -233,9 +233,8 @@ Result StaticFeedClient::day_categorical(std::int32_t Error::invalid_request("categorical outlook day must be 1, 2, or 3")); } const std::string url = std::format("{}day{}otlk_cat.nolyr.geojson", kStaticBase, day); - Result body = - body_or_error(with_retry([&] { return impl_->http->get(url); }, impl_->retry), - Feed404::NoActiveOutlook); + Result body = body_or_error( + with_retry([&] { return impl_->http->get(url); }, impl_->retry), Feed404::NoActiveOutlook); if (!body) { return std::unexpected(body.error()); } @@ -262,9 +261,8 @@ Result StaticFeedClient::day_probabilistic(std::int32_t day, const std::string filename = day == 3 ? "day3otlk_prob.nolyr.geojson" : std::format("day{}otlk_{}.nolyr.geojson", day, tag); const std::string url = std::string{kStaticBase} + filename; - Result body = - body_or_error(with_retry([&] { return impl_->http->get(url); }, impl_->retry), - Feed404::NoActiveOutlook); + Result body = body_or_error( + with_retry([&] { return impl_->http->get(url); }, impl_->retry), Feed404::NoActiveOutlook); if (!body) { return std::unexpected(body.error()); } @@ -281,9 +279,8 @@ Result StaticFeedClient::day4_8(std::int32_t day) { Error::invalid_request("extended outlook day must be between 4 and 8")); } const std::string url = std::format("{}day{}prob.nolyr.geojson", kStaticDay48Base, day); - Result body = - body_or_error(with_retry([&] { return impl_->http->get(url); }, impl_->retry), - Feed404::NoActiveOutlook); + Result body = body_or_error( + with_retry([&] { return impl_->http->get(url); }, impl_->retry), Feed404::NoActiveOutlook); if (!body) { return std::unexpected(body.error()); } @@ -336,15 +333,15 @@ struct ArcGISClient::Impl { } if (envelope->exceeded_transfer_limit && envelope->feature_count == 0) { // The offset would never move: paging cannot converge. - return std::unexpected(Error::server( - "ArcGIS reported a truncated page containing no records")); + return std::unexpected( + Error::server("ArcGIS reported a truncated page containing no records")); } pages.push_back(std::move(*body)); pager.advance(envelope->feature_count, envelope->exceeded_transfer_limit); } if (pager.page_limit_reached()) { - return std::unexpected(Error::server(std::format( - "ArcGIS paging did not converge within {} pages", pager.max_pages()))); + return std::unexpected(Error::server( + std::format("ArcGIS paging did not converge within {} pages", pager.max_pages()))); } return pages; } diff --git a/src/http/client.cpp b/src/http/client.cpp index 53f38f3..fa8c967 100644 --- a/src/http/client.cpp +++ b/src/http/client.cpp @@ -143,8 +143,8 @@ Result HttpClient::get(std::string_view path) const { CURLcode rc = curl_easy_perform(curl); if (rc != CURLE_OK) { if (sink.overflowed) { - return std::unexpected(Error::network( - "response exceeded ClientConfig::max_response_bytes")); + return std::unexpected( + Error::network("response exceeded ClientConfig::max_response_bytes")); } return std::unexpected(Error::network(curl_easy_strerror(rc))); } diff --git a/tests/test_arcgis.cpp b/tests/test_arcgis.cpp index a09261d..4fba947 100644 --- a/tests/test_arcgis.cpp +++ b/tests/test_arcgis.cpp @@ -21,8 +21,8 @@ #include #include -#include #include +#include #include #include #include @@ -243,9 +243,9 @@ TEST(ArcGISParity, EveryCapturedEsriFixtureMatchesItsGeoJsonTwin) { std::string hazard; ///< empty for the categorical layers }; const std::vector pairs = { - {"arcgis_day1_categorical", 1, ""}, {"arcgis_day2_categorical", 2, ""}, - {"arcgis_day3_categorical", 3, ""}, {"arcgis_day1_prob_tornado", 1, "tornado"}, - {"arcgis_day1_prob_hail", 1, "hail"}, {"arcgis_day1_prob_wind", 1, "wind"}, + {"arcgis_day1_categorical", 1, ""}, {"arcgis_day2_categorical", 2, ""}, + {"arcgis_day3_categorical", 3, ""}, {"arcgis_day1_prob_tornado", 1, "tornado"}, + {"arcgis_day1_prob_hail", 1, "hail"}, {"arcgis_day1_prob_wind", 1, "wind"}, {"arcgis_day2_prob_wind", 2, "wind"}, }; @@ -287,9 +287,8 @@ TEST(ArcGISParity, EveryCapturedEsriFixtureMatchesItsGeoJsonTwin) { continue; } const std::string label = - pair.hazard.empty() - ? detail::json_string(*attrs, "label") - : std::format("{:.6f}", detail::normalized_probability(*attrs)); + pair.hazard.empty() ? detail::json_string(*attrs, "label") + : std::format("{:.6f}", detail::normalized_probability(*attrs)); const std::vector* expected = nullptr; for (const std::pair>& band : reference) { if (band.first == label) { @@ -310,9 +309,8 @@ TEST(ArcGISParity, EveryCapturedEsriFixtureMatchesItsGeoJsonTwin) { << pair.stem << ": Esri and GeoJSON disagree on the band set"; EXPECT_GT(probe.compared, 100u) << pair.stem << ": too few clear-of-boundary probes"; EXPECT_EQ(probe.agreed, probe.compared) - << pair.stem << ": Esri vs GeoJSON disagreed on " - << (probe.compared - probe.agreed) << "/" << probe.compared - << " unambiguous interior/exterior probes"; + << pair.stem << ": Esri vs GeoJSON disagreed on " << (probe.compared - probe.agreed) + << "/" << probe.compared << " unambiguous interior/exterior probes"; } } @@ -355,8 +353,8 @@ TEST(NetNewModels, FireWeatherOwnSeverityMapper) { EXPECT_EQ(fire_severity_from_label("CRIT"), 2); EXPECT_EQ(fire_severity_from_label("EXTM"), 3); EXPECT_EQ(fire_severity_from_label("SLGT"), 0); // not categorical - const FireWeatherPayload p = - parse_fire_weather(slurp("arcgis_day1_fire_weather.esri.json"), 1, FireWeatherLayer::Outlook); + const FireWeatherPayload p = parse_fire_weather(slurp("arcgis_day1_fire_weather.esri.json"), 1, + FireWeatherLayer::Outlook); EXPECT_EQ(p.day, 1); ASSERT_EQ(p.features.size(), 3u); EXPECT_EQ(p.features[0].label, "ELEV"); @@ -375,9 +373,8 @@ TEST(NetNewModels, FireWeatherLayerIsTheOnlyThingThatDisambiguatesDayOneAndTwoDn // numeric dn band index (5/8/10) that the Outlook and DryThunderstorm // layers both use with different meanings. Nothing in the body says which // layer it came from, so the caller must say — there is no safe default. - for (const std::string& name : - {std::string{"arcgis_day1_fire_weather.esri.json"}, - std::string{"arcgis_day2_fire_weather.esri.json"}}) { + for (const std::string& name : {std::string{"arcgis_day1_fire_weather.esri.json"}, + std::string{"arcgis_day2_fire_weather.esri.json"}}) { const std::string body = slurp(name); EXPECT_EQ(body.find("LABEL"), std::string::npos) << name; EXPECT_EQ(body.find("\"label\""), std::string::npos) << name; diff --git a/tests/test_client.cpp b/tests/test_client.cpp index 6343caa..a09b798 100644 --- a/tests/test_client.cpp +++ b/tests/test_client.cpp @@ -402,7 +402,9 @@ TEST(Feed404Semantics, ArcGisLogicalNotFoundIsAGenuineNotFound) { TEST(Feed404Semantics, ArcGisCodeThatIsNotAnHttpStatusStaysOutOfHttpStatus) { std::shared_ptr transport = std::make_shared(); transport->responses = { - {200, R"({"error":{"code":1000,"message":"Unable to complete operation.","details":[]}})", {}}, + {200, + R"({"error":{"code":1000,"message":"Unable to complete operation.","details":[]}})", + {}}, }; ArcGISClient client{transport}; From e9e16503bb95e628a6a31205030811b26a9a75e1 Mon Sep 17 00:00:00 2001 From: KeviM Date: Fri, 4 Sep 2026 04:52:00 -0700 Subject: [PATCH 09/11] chore(tidy): silence easily-swappable-parameters on the ArcGISPager ctor bugprone-* is enabled with WarningsAsErrors: '*', and the two int32 constructor parameters are adjacent and same-typed. Matches the suppression the repo already uses on ArchiveClient::storm_reports. Also names the SPC_VERSION_STRING fallback literal in the CLAUDE.md release checklist: the generated definition covers anyone linking the targets, but the header's #ifndef fallback is not test-covered. --- CLAUDE.md | 10 +++++++--- include/spc/pagination.hpp | 1 + 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index a66d702..c0b3c0f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -50,6 +50,10 @@ Glaze 8.3 parses loose SPC JSON. GoogleTest 1.18 runs the unit suite. The tag must match `project(spc-cpp VERSION ...)`. A `vX.Y.Z` tag triggers the release workflow, which reads the matching `CHANGELOG.md` section. -The default `ClientConfig::user_agent` is generated from `PROJECT_VERSION`, so -a bump carries automatically. `HttpClientLifecycle.DefaultUserAgentCarriesTheProjectVersion` -fails if that ever stops being true. +The default `ClientConfig::user_agent` is generated from `PROJECT_VERSION` via +`SPC_VERSION_STRING`, so a bump carries automatically for anything linking the +targets, and `HttpClientLifecycle.DefaultUserAgentCarriesTheProjectVersion` +fails if that stops being true. Also bump the `#ifndef SPC_VERSION_STRING` +fallback literal in `include/spc/http_client.hpp`: it only applies to a +consumer that includes the header without linking the target, so no test +covers it. diff --git a/include/spc/pagination.hpp b/include/spc/pagination.hpp index 46c704e..c8bdc13 100644 --- a/include/spc/pagination.hpp +++ b/include/spc/pagination.hpp @@ -27,6 +27,7 @@ class ArcGISPager { /// hard server max is typically 2000; that is the default. `max_pages` /// bounds a server (or caching proxy) that keeps reporting truncation /// without advancing — see `page_limit_reached()`. + // NOLINTNEXTLINE(bugprone-easily-swappable-parameters) explicit ArcGISPager(std::int32_t page_size = 2000, std::int32_t max_pages = 100) : page_size_(page_size > 0 ? page_size : 2000), max_pages_(max_pages > 0 ? max_pages : 100) {} From 75d20f3602af662a0679548c28117eb379dbd4ab Mon Sep 17 00:00:00 2001 From: KeviM Date: Fri, 4 Sep 2026 04:56:29 -0700 Subject: [PATCH 10/11] refactor(parser): return parse_double's outcome as a struct clang-tidy's bugprone-easily-swappable-parameters flagged the adjacent 'double& value, std::size_t& consumed' out-parameters as convertible and swappable. They are: transposing them at a call site compiles. Returning a ParsedNumber removes the hazard instead of suppressing the warning, and reads better at both call sites. Both parse paths (from_chars and the classic-locale stream fallback) rebuilt and the full suite run against each. --- include/spc/models/common.hpp | 11 +++++++++-- src/models/common.cpp | 37 ++++++++++++++++------------------- src/models/fire_weather.cpp | 5 ++--- 3 files changed, 28 insertions(+), 25 deletions(-) diff --git a/include/spc/models/common.hpp b/include/spc/models/common.hpp index b35de3b..99649c4 100644 --- a/include/spc/models/common.hpp +++ b/include/spc/models/common.hpp @@ -35,8 +35,15 @@ const Json* lookup(const Json& obj, const char* key); /// String value of `obj[key]`, or "" if absent / null / non-string. std::string json_string(const Json& obj, const char* key); +/// Outcome of `parse_double`. +struct ParsedNumber { + double value = 0.0; ///< meaningful only when `ok` + std::size_t consumed = 0; ///< characters the number occupied + bool ok = false; +}; + /// Parse the leading decimal number of `text` **locale-independently**, and -/// report how many characters it consumed. Returns false when nothing parses. +/// report how many characters it consumed. /// /// `std::stod` delegates to `strtod`, which honours the process `LC_NUMERIC`: /// on a comma-decimal host (any app that calls `setlocale(LC_ALL, "")` on a @@ -46,7 +53,7 @@ std::string json_string(const Json& obj, const char* key); /// /// Narrower than `std::stod` by design: leading whitespace and a leading '+' /// are rejected. No SPC payload uses either form. -bool parse_double(std::string_view text, double& value, std::size_t& consumed); +ParsedNumber parse_double(std::string_view text); /// SPC ships `LABEL` as either a string ("SLGT", "5") or a number (5). /// Always returns a numeric view; non-numeric / missing yields 0. diff --git a/src/models/common.cpp b/src/models/common.cpp index cde8d64..af1368b 100644 --- a/src/models/common.cpp +++ b/src/models/common.cpp @@ -63,9 +63,10 @@ std::string json_string(const Json& obj, const char* key) { return {}; } -bool parse_double(std::string_view text, double& value, std::size_t& consumed) { +ParsedNumber parse_double(std::string_view text) { + ParsedNumber parsed; if (text.empty()) { - return false; + return parsed; } // Gate the leading character so both implementations below agree on what // they accept: from_chars takes '-', a digit, '.', or inf/nan, and never @@ -74,37 +75,35 @@ bool parse_double(std::string_view text, double& value, std::size_t& consumed) { const bool leading_ok = first == '-' || first == '.' || (first >= '0' && first <= '9') || first == 'i' || first == 'I' || first == 'n' || first == 'N'; if (!leading_ok) { - return false; + return parsed; } #if SPC_HAS_FP_FROM_CHARS - double parsed = 0.0; const std::from_chars_result result = - std::from_chars(text.data(), text.data() + text.size(), parsed); + std::from_chars(text.data(), text.data() + text.size(), parsed.value); if (result.ec != std::errc{}) { - return false; + return ParsedNumber{}; } - value = parsed; - consumed = static_cast(result.ptr - text.data()); - return true; + parsed.consumed = static_cast(result.ptr - text.data()); + parsed.ok = true; + return parsed; #else std::istringstream stream{std::string{text}}; stream.imbue(std::locale::classic()); - double parsed = 0.0; - stream >> parsed; + stream >> parsed.value; if (stream.fail()) { - return false; + return ParsedNumber{}; } - value = parsed; // tellg() reports -1 once the whole buffer was consumed. - consumed = text.size(); + parsed.consumed = text.size(); if (!stream.eof()) { const std::streamoff position = stream.tellg(); if (position >= 0) { - consumed = static_cast(position); + parsed.consumed = static_cast(position); } } - return true; + parsed.ok = true; + return parsed; #endif } @@ -122,10 +121,8 @@ double json_number_or_numeric_string(const Json& obj, const char* key) { // Was std::stod, whose strtod honours LC_NUMERIC; see parse_double. // Byte-identical to a C-locale stod for every value in the fixture // corpus, which is what the spc-data byte-identity gate covers. - const std::string s = v->get(); - double value = 0.0; - std::size_t consumed = 0; - return parse_double(s, value, consumed) ? value : 0.0; + const ParsedNumber parsed = parse_double(v->get()); + return parsed.ok ? parsed.value : 0.0; } return 0.0; } diff --git a/src/models/fire_weather.cpp b/src/models/fire_weather.cpp index e997833..52b5d85 100644 --- a/src/models/fire_weather.cpp +++ b/src/models/fire_weather.cpp @@ -66,9 +66,8 @@ bool has_zero_dn(const Json& props) { // Locale-independent: a comma-decimal strtod stops at the '.' of "0.0", // the full-consume check fails, and the no-risk sentinel ships as a band. const std::string text = dn->get(); - double value = 0.0; - std::size_t consumed = 0; - return detail::parse_double(text, value, consumed) && value == 0.0 && consumed == text.size(); + const detail::ParsedNumber parsed = detail::parse_double(text); + return parsed.ok && parsed.value == 0.0 && parsed.consumed == text.size(); } std::string label_from_dn(const Json& props, FireWeatherLayer layer) { From dcf6025004dc4ac114767c87982263eeb0d126bb Mon Sep 17 00:00:00 2001 From: KeviM Date: Fri, 4 Sep 2026 04:59:39 -0700 Subject: [PATCH 11/11] ci: generate de_DE.UTF-8 so the locale regression tests run on Linux The LocaleIndependence tests GTEST_SKIP when a comma-decimal locale is unavailable, which on the Ubuntu runners is always -- so the locale defect they guard was only ever checked on a developer's macOS box. --- .github/workflows/ci.yml | 14 ++++++++++---- CHANGELOG.md | 2 ++ 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 586a5bf..14b2196 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -24,7 +24,10 @@ jobs: sudo apt-get update sudo apt-get install -y --no-install-recommends \ build-essential cmake pkg-config clang-format \ - libcurl4-openssl-dev + libcurl4-openssl-dev locales + # The LocaleIndependence tests need a comma-decimal locale; without + # one they GTEST_SKIP and the regression they guard goes unchecked. + sudo locale-gen de_DE.UTF-8 - name: Build run: make build CMAKE_ARGS=-DSPC_WARNINGS_AS_ERRORS=ON @@ -71,7 +74,8 @@ jobs: run: | sudo apt-get update sudo apt-get install -y --no-install-recommends \ - build-essential cmake libcurl4-openssl-dev + build-essential cmake libcurl4-openssl-dev locales + sudo locale-gen de_DE.UTF-8 - name: Build with address and undefined behavior sanitizers run: >- cmake -S . -B build-sanitized @@ -90,7 +94,8 @@ jobs: run: | sudo apt-get update sudo apt-get install -y --no-install-recommends \ - build-essential cmake libcurl4-openssl-dev + build-essential cmake libcurl4-openssl-dev locales + sudo locale-gen de_DE.UTF-8 - name: Build with ThreadSanitizer run: >- cmake -S . -B build-thread-sanitized @@ -132,5 +137,6 @@ jobs: run: | sudo apt-get update sudo apt-get install -y --no-install-recommends \ - build-essential cmake libcurl4-openssl-dev + build-essential cmake libcurl4-openssl-dev locales + sudo locale-gen de_DE.UTF-8 - run: ./tools/test_consumers.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index 83bfff8..f65fb7a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -115,6 +115,8 @@ uses [Semantic Versioning](https://semver.org/spec/v2.0.0.html). categorical, four probabilistic). The test named for probabilistic parity only ever opened the GeoJSON side, so `parse_esri_rings` was pinned by one categorical layer. +- The Linux CI jobs generate `de_DE.UTF-8`, so the locale regression tests run + there instead of skipping. - `ci.yml` declares `permissions: contents: read` at the top level — it runs on `pull_request` and executes third-party build scripts — and pins both actions to full commit SHAs instead of mutable tags.