diff --git a/.agent/work-plans/issue-111/plan.md b/.agent/work-plans/issue-111/plan.md new file mode 100644 index 0000000..39b2adc --- /dev/null +++ b/.agent/work-plans/issue-111/plan.md @@ -0,0 +1,151 @@ +# Plan: Weather radar overlay shows stale precipitation (does not advance to latest mosaic) + +## Issue + +https://github.com/rolker/camp/issues/111 + +## Context + +The NEXRAD radar overlay (#99) shows stale precipitation: it never advances to +the latest IEM mosaic across the 5-minute auto-refresh, including across sessions. +Reading the current code (post-#98) rules out two of the three candidate causes +the issue lists and confirms the third: + +- **Cause #1 — CDN/HTTP caching of the static tile URL (ROOT CAUSE).** The IEM + request URL `.../nexrad-n0q-900913/{z}/{x}/{y}.png` carries no cache-buster. + `CachedTileLoader::invalidateCache()` (`cached_tile_loader.cpp:50`) correctly + wipes the per-layer disk subdir each cycle, so the next `load()` does a real + network GET — but to the *same* URL, which a CDN/proxy between CAMP and IEM can + satisfy from its own cache with an older tile. The `onRefreshTimer` comment + (`map_tiles.cpp:260`) did not account for caching *between* client and origin — + that is the gap. (Endpoint check, 2026-06-22: the IEM tile URL returns HTTP 200 + + identical bytes with and without an unknown `?t=` param, so the cache-buster + is safe and won't backfire to blank tiles.) +- **Cause #2 — global `CachedFileLoader` re-serving (DISPROVEN).** + `CachedFileLoader::load` (`cached_file_loader.cpp:55`) gates re-use solely on + `file_path.exists()`; there is no in-memory URL cache, no in-flight dedup, and + no `QNetworkDiskCache` on the QNAM (ctor lines 18-24). Once the disk subdir is + wiped it cannot re-serve. +- **Cause #3 — timer cadence / visibility pausing (DISPROVEN).** + `onRefreshTimer()` runs unconditionally; `setRefreshInterval` only stops the + timer for `msec<=0`; nothing in the visibility path pauses it. + +Fix: attach a per-refresh cache-buster query param to the radar layer's request +URL so each cycle's GET is a URL the CDN cannot answer from cache. + +## Approach + +1. **Add an opt-in cache-buster to `CachedTileLoader`** (the per-layer network + choke point, owned per-`MapTiles`). New state `quint64 cache_bust_ = 0;` + (0 = disabled). Methods: + - `void enableCacheBusting()` — seed `cache_bust_` from wall clock + (`QDateTime::currentMSecsSinceEpoch()`); marks the layer as busting. + - `void bumpCacheBust()` — advance with a monotonic guard + `cache_bust_ = std::max(cache_bust_ + 1, currentMSecsSinceEpoch())`. + Time-seeding gives cross-session freshness (the "across sessions" symptom); + the `+1` guard guarantees a *strictly distinct* value even on back-to-back + calls, which is the testable invariant. + - `quint64 cacheBustToken() const` — getter for tests. + - `static std::string withCacheBust(const std::string& url, quint64 token)` — + pure helper: appends `?t=` (or `&t=` if the URL already has a `?`). +2. **Apply it in `CachedTileLoader::load`** (`cached_tile_loader.cpp:36`): when + `cache_bust_ != 0`, set `url_str = withCacheBust(url_str, cache_bust_)`. The + busted URL is used *only* as the network `url_str`; the disk path stays + `z/x/y.png` (from `operator std::string`), so the buster does not bloat disk + cache or change the within-cycle disk-hit behavior. +3. **Drive it from `MapTiles`:** + - `setRefreshInterval(msec>0)` (`map_tiles.cpp:229`) calls + `tile_loader_->enableCacheBusting()` — ties busting to "this is a refreshing + layer". Static OSM/WMTS layers never call this, so their URLs are unchanged. + - `onRefreshTimer()` (`map_tiles.cpp:248`) calls `tile_loader_->bumpCacheBust()` + *before* `setLayout()`/`update()`, so the loads triggered by the next + `paint()` carry the new token. +4. **Correct the stale freshness comment** at `map_tiles.cpp:260-266`: disk + invalidation alone does NOT force a fresh frame across a CDN; the cache-buster + is what defeats intermediary caching. Keep the timestamp-pinning caveat and + point the comment at the WMS/nowCOAST end state (#118). +5. **Remove ADR-0004** (operator decision, 2026-06-22). ADR-0004 recorded the IEM + provider as a stopgap and asserted "refresh yields a genuinely fresh frame" — + a claim this bug disproves. Rather than edit an accepted ADR's Consequences + (which workspace ADR-0012 says requires a new/superseding ADR, not an in-place + edit), the radar-provider decision is **retired from the ADR set** and tracked + forward as an issue: the intended end state is NOAA nowCOAST via WMS, filed as + **camp#118** (which carries the IEM-stopgap rationale so discoverability is not + lost). Fold the ADR removal into this PR — #111 is what invalidated it. +6. **Tests** (extend `test/test_map_tiles_refresh.cpp`; already CMake-registered, + links `camp_map` where `CachedTileLoader` lives — no CMake change): + - `withCacheBust` pure-logic: distinct tokens → distinct URLs; correct `?` + vs `&` joining for URLs with/without an existing query. + - Refresh path: reach the loader via + `layer->findChild()` (it is parented to + `MapTiles`, like the existing `findChild()` pattern); after + `enableCacheBusting()`, two `invokeRefresh()` cycles strictly increase + `cacheBustToken()`. + - Opt-in: a layer that never calls `setRefreshInterval` keeps + `cacheBustToken() == 0` (static layers unaffected). No live-network test. +7. **Invalidate-on-show** (operator requirement, 2026-06-23 — "never show yesterday's + radar, not even briefly"). The 5-min refresh only invalidates the disk cache on + the timer; enabling radar shortly after launch would serve a previous session's + on-disk tiles until the next refresh. Override `MapTiles::itemChange` so that on + the `ItemVisibleHasChanged → true` transition a *refreshing* layer (gated on + `cacheBustToken() != 0`) drops its disk cache + bumps the buster, making the + first paint fetch fresh (brief blank, never stale). Static layers keep their + cache on show. Tests: token advances on show for a refreshing layer; a static + layer's visibility toggle does not bust. The broader mid-session "displayed + frame ages when fetches stop" safeguard is filed separately as **camp#119**. + +## Files to Change + +| File | Change | +|------|--------| +| `src/camp2/map_tiles/cached_tile_loader.h` | Add `cache_bust_`, `enableCacheBusting()`, `bumpCacheBust()`, `cacheBustToken()`, static `withCacheBust()` | +| `src/camp2/map_tiles/cached_tile_loader.cpp` | Implement the above; apply buster in `load()`; `enableCacheBusting()` idempotent (no token regression on re-enable, review #1) | +| `src/camp2/map_tiles/map_tiles.h` | Declare the `itemChange` override + shared `refreshTiles()` helper | +| `src/camp2/map_tiles/map_tiles.cpp` | `setRefreshInterval` → `enableCacheBusting`; extract `refreshTiles()` (bump + invalidate + **setLayout rebuild**); `onRefreshTimer` and `itemChange`-on-show both call it (the on-show path must rebuild, not just bump — Round-2 must-fix); freshness comment points at #118 | +| `docs/decisions/0004-weather-radar-tile-provider.md` | **Delete** — radar-provider decision retired from the ADR set; tracked forward as camp#118 (operator decision) | +| `test/test_map_tiles_refresh.cpp` | Add `withCacheBust` + token-bump + opt-in + invalidate-on-show tests | + +## Principles Self-Check + +| Principle | Consideration | +|---|---| +| Enforcement over documentation | Regression test asserts per-refresh URL/token distinctness — the enforceable invariant — without a live-network test | +| Capture decisions, not just implementations | The corrected freshness reasoning lives in the `onRefreshTimer` comment + camp#118 (the WMS/nowCOAST end state). ADR-0004's now-false "fresh frame" claim is retired rather than edited in place (ADR-0012) | +| A change includes its consequences | Same PR updates code, the `onRefreshTimer` comment, removes ADR-0004, and the test | +| Only what's needed | Minimal per-layer cache-buster, not a general `Cache-Control`/HTTP-cache layer. Opt-in via `setRefreshInterval`, so non-radar layers are untouched | +| Test what breaks | Invariant = "a refresh produces a request the CDN cannot satisfy from cache"; asserted via strict per-cycle token monotonicity | +| Safety / operational awareness (project, inferred) | Stale radar can mislead more than blank radar; a visible-staleness safeguard is noted as possible follow-up (out of scope here) | + +## ADR Compliance + +| ADR | Triggered | How addressed | +|---|---|---| +| camp ADR-0004 (radar tile provider) | Yes (directly) | **Removed** — its "fresh frame" claim is disproven and the provider choice is a stopgap, not settled architecture. Retired from the ADR set and tracked forward as camp#118 (operator decision, 2026-06-22) | +| camp ADR-0002 (Web-Mercator scene/layer) | No | Radar stays in pure-Qt `libcamp_map`; no scene/layer-model change | +| workspace ADR-0008 (ROS 2 conventions) | No | camp is a Qt/C++ app; no ROS package/launch/`package.xml` change | +| workspace ADR-0012 (cross-reference addendums) | Yes | ADR-0012 says correcting/softening Consequences needs a new/superseding ADR, not an in-place edit; we remove ADR-0004 entirely (operator call) rather than amend it, so no in-place substantive edit is made | + +## Consequences + +| If we change... | Also update... | Included in plan? | +|---|---|---| +| Radar request URL (add cache-buster) | Remove ADR-0004; track end state as camp#118 | Yes (step 5) | +| Refresh freshness semantics | `onRefreshTimer` comment (`map_tiles.cpp:260`) | Yes (step 4) | +| `CachedTileLoader` / refresh path | `test/test_map_tiles_refresh.cpp` | Yes (step 6) | + +## Open Questions (resolved) + +- **Empirical confirmation of cause #1 — RESOLVED (operator decision, 2026-06-22):** + the cache-buster is adopted as a low-risk *defensive* fix that is correct + regardless of whether CDN caching is the sole mechanism (a no-op query param if + no intermediary caches exist). No live two-cycle sidecar diff is required first; + causes #2/#3 are already disproven by code-reading. +- **Does IEM `tile.py` tolerate an unknown `?t=` param — RESOLVED:** verified live + 2026-06-22 — `…/nexrad-n0q-900913/4/4/6.png` and the same URL with + `?t=1750000000000` both return HTTP 200, `image/png`, identical 10287 bytes. The + buster will not 4xx or blank the radar. + +## Estimated Scope + +Single PR. One subsystem (`map_tiles`), one mechanism (per-refresh cache-buster), +plus the ADR/comment/test consequences. ~80-120 lines incl. tests. diff --git a/.agent/work-plans/issue-111/progress.md b/.agent/work-plans/issue-111/progress.md new file mode 100644 index 0000000..578df62 --- /dev/null +++ b/.agent/work-plans/issue-111/progress.md @@ -0,0 +1,268 @@ +--- +issue: 111 +--- + +# Issue #111 — Weather radar overlay shows stale precipitation (does not advance to latest mosaic) + +## Issue Review +**Status**: complete +**When**: 2026-06-22 22:35 +00:00 +**By**: Claude Code Agent (Claude Opus) + +**Issue**: #111 +**Comment**: (best-effort post follows this entry; not recorded inline) +**Scope verdict**: well-scoped + +### Summary + +The NEXRAD radar overlay (added in #99, ADR-0004) displays stale precipitation: it +does not advance to the latest IEM mosaic across the #99 Phase-2 5-minute auto-refresh. +The issue lists three candidate root causes. Reading the current code (lines below are +post-#98; the issue's line numbers are stale) lets two of them be largely ruled out and +points strongly at the third: + +- **Cause #1 — CDN / HTTP-level caching of the static tile URL (MOST LIKELY).** The IEM + `nexrad-n0q-900913/{z}/{x}/{y}.png` URL carries no timestamp/cache-buster + (ADR-0004:67-69). `CachedTileLoader::invalidateCache()` + (`cached_tile_loader.cpp:50-85`) does correctly delete and recreate the per-layer disk + subdir, so the next `load()` misses the disk cache and issues a real network GET — but + that GET hits the *same* URL, which a CDN / proxy edge between CAMP and the IEM origin + can answer from its own cache with an older tile. ADR-0004's freshness rationale + (`Consequences`, lines 100-105) and the `onRefreshTimer` comment + (`map_tiles.cpp:260-266`) both reason only about the *disk* cache and the origin alias + "always serving the latest mosaic"; **neither accounts for HTTP/CDN caching between + client and origin.** That is the gap. Likely fix: a per-refresh cache-buster + (`?t=`) on the request URL — note this changes the URL contract recorded + in ADR-0004 and therefore needs an ADR addendum (see ADR Applicability). + +- **Cause #2 — global `CachedFileLoader` re-serving by URL/path (LARGELY DISPROVEN BY + CODE).** `CachedFileLoader::load()` (`cached_file_loader.cpp:55-74`) gates re-use solely + on on-disk `file_path.exists()`; if the file exists it rewrites the request to a + `file://` URL, otherwise it does a network GET. There is **no in-memory URL-keyed cache, + no in-flight dedup, and no `QNetworkDiskCache` installed on the `QNetworkAccessManager`** + (constructor lines 18-24). So once `invalidateCache()` removes the per-layer subdir, the + global loader cannot independently re-serve the previous content. This cause can be + largely set aside — worth one confirming glance during implementation, not a primary + investigation line. + +- **Cause #3 — refresh-timer cadence / visibility pausing (LARGELY DISPROVEN BY CODE).** + `onRefreshTimer()` (`map_tiles.cpp:248-271`) runs unconditionally once the timer is + started; `setRefreshInterval()` (`:229-246`) only *stops* the timer for `msec <= 0`, and + nothing in the visibility toggle path pauses it. So a default-OFF / toggled layer does + not pause refresh once an interval is set. The one residual sub-question worth a quick + check is whether `setRefreshInterval(5min)` is actually invoked on the radar layer in the + deployed registration path (`background_manager.cpp`) — but the *staleness* is observed + with the layer ON, so cadence is unlikely to be the mechanism. + +Net: this is a well-scoped diagnostic bug whose most probable fix is a cache-buster on the +radar request URL. Recommend the implementer confirm cause #1 empirically (compare the +served tile bytes/headers across two refresh cycles, e.g. the `.json` reply-header sidecar +`CachedFileLoader` already writes at `cached_file_loader.cpp:102-112`) before coding, so +the fix targets the real layer. + +### Principle Alignment + +| Principle | Status | Notes | +|---|---|---| +| Human control and transparency | OK | Restores expected behavior (fresh radar). A cache-buster is a transparent, mechanical URL change with no operator-visible config impact | +| Enforcement over documentation | Action needed | The fix must ship a regression test. CDN behavior can't be unit-tested directly, but if the fix is a cache-buster the test can assert the request URL carries a *distinct* per-refresh query param across two `onRefreshTimer` cycles — that is the enforceable invariant | +| Capture decisions, not just implementations | Action needed | If cause #1 is confirmed and a cache-buster is added, ADR-0004's "always-latest / static URL + disk-invalidation suffices" reasoning is now known to be incomplete. Record the correction as an ADR-0004 cross-reference addendum (permitted by workspace ADR-0012) or a Consequences update — do not leave the corrected rationale only in a commit message | +| A change includes its consequences | Action needed | Same PR must update: (a) ADR-0004 Consequences (lines 100-105); (b) the `onRefreshTimer` freshness comment (`map_tiles.cpp:250-266`) which currently asserts disk invalidation is sufficient; (c) likely `test/test_map_tiles_refresh.cpp` | +| Only what's needed | Watch | Prefer a minimal cache-buster over building a general HTTP cache-control / `Cache-Control: no-cache` request-header layer. If a request header (e.g. no-cache) proves sufficient and avoids changing the URL/disk-path contract, that may be even smaller — weigh in plan-task | +| Improve incrementally | OK | Single subsystem, single mechanism; completable in one reviewable PR | +| Test what breaks | Action needed | The regression target is "a refresh produces a request the CDN cannot satisfy from cache" — assert per-cycle URL/param distinctness (see Enforcement row). Avoid a live-network test in CI | +| Workspace vs. project separation | OK | Entirely within the `camp` project repo (`ui_ws/src/camp/`); no workspace-infra change | +| Safety / operational awareness (project) | Watch | Inferred (no `camp/PRINCIPLES.md`; consistent with the #98 review's project-safety row). Stale radar is arguably worse than blank radar: a plausible-but-old precipitation frame can mislead the operator's live weather picture during a survey. Raises the priority of getting freshness genuinely correct, and argues for a visible-staleness safeguard as possible follow-up | + +### ADR Applicability + +| ADR | Triggered | Notes | +|---|---|---| +| camp ADR-0004 — Weather-radar tile provider (IEM NEXRAD N0Q) | **Yes (directly)** | The issue challenges ADR-0004's core freshness premise. The likely fix amends the URL/refresh contract this ADR records, so ADR-0004 needs a cross-reference addendum or Consequences update in the same PR | +| camp ADR-0002 — Web-Mercator scene & layer model | No (tangential) | Radar lives in the pure-Qt `libcamp_map` layer per this ADR, but no scene/layer-model change is implied | +| workspace ADR-0008 — Follow ROS 2 conventions | No | camp is a Qt/C++ app; no ROS package interface / launch / `package.xml` change | +| workspace ADR-0001 — Adopt ADRs | Yes (via 0004) | The corrected freshness reasoning should be captured, not buried in a commit — satisfied by the ADR-0004 addendum above | + +### Consequences + +- **ADR-0004** Consequences section (the "Refresh yields a genuinely fresh frame" bullet, + lines 100-105) — update/addend to acknowledge intermediary HTTP/CDN caching and the + cache-buster mitigation. +- **`src/camp2/map_tiles/map_tiles.cpp:250-266`** — the `onRefreshTimer` comment that + asserts disk invalidation alone forces a fresh network frame; correct it to reflect the + CDN-caching reality and the cache-buster. +- **`test/test_map_tiles_refresh.cpp`** — extend (or add a sibling) to assert the + per-refresh request-distinctness invariant. + +### Actions +- [ ] Enforcement: ship a regression test asserting the per-refresh request carries a + distinct cache-buster (or otherwise can't be CDN-cached), without a live-network test in CI. +- [ ] Capture decisions: add an ADR-0004 cross-reference addendum / Consequences update + recording that the static URL + disk-invalidation reasoning was incomplete (CDN caching). +- [ ] Consequences: in the same PR update ADR-0004 (lines 100-105), the `onRefreshTimer` + freshness comment (`map_tiles.cpp:250-266`), and `test/test_map_tiles_refresh.cpp`. +- [ ] Test what breaks: define the regression invariant (per-cycle URL/param distinctness), + not a live-network assertion. +- [ ] Recommendation (plan-task): empirically confirm cause #1 (compare served tile + bytes/reply-headers across two refresh cycles via the `.json` sidecar) before coding, and + weigh a minimal cache-buster vs. a `no-cache` request header for the smallest fix. +- [ ] Recommendation (project safety): consider a visible radar-staleness safeguard + (e.g. surface frame age / dim on suspected-stale) as possible follow-up, given stale + radar can mislead more than blank radar. + +## Plan Authored +**Status**: complete +**When**: 2026-06-22 23:26 +00:00 +**By**: Claude Code Agent (Claude Opus) + +**Plan**: `.agent/work-plans/issue-111/plan.md` at `5b45a72` +**Branch**: feature/issue-111 at `5b45a72` +**Phases**: single + +Root cause confirmed by code reading: CDN/HTTP caching of the static IEM tile +URL (cause #1); causes #2 and #3 disproven. Fix = opt-in per-refresh cache-buster +on `CachedTileLoader` (`?t=`), seeded from wall clock (cross-session +freshness) with a monotonic guard (strict per-cycle distinctness), driven by +`MapTiles::setRefreshInterval`/`onRefreshTimer`. Same PR corrects the +`onRefreshTimer` freshness comment, adds an ADR-0004 Consequences addendum, and +extends `test/test_map_tiles_refresh.cpp` (pure `withCacheBust` + token-bump + +opt-in invariants; no live-network test). + +### Open questions +- [ ] Empirical pre-coding confirmation of cause #1: diff served tile bytes / + reply-headers across two refresh cycles via the `.json` sidecar + (`cached_file_loader.cpp:102`) before implementing. +- [ ] Verify IEM `tile.py` accepts and ignores an unknown `?t=` query param + (returns the tile, not 4xx/blank) against the live endpoint. + +## Plan Review +**Status**: complete +**When**: 2026-06-22 23:52 +00:00 +**By**: Claude Code Agent (Claude Opus) + + +**Plan**: `.agent/work-plans/issue-111/plan.md` at `5b45a72` +**PR**: PR-less (`--issue` / file-path mode; no draft PR) +**Verdict**: changes-requested + +### Findings +- [ ] (must-fix) Plan cites workspace ADR-0012 as permitting an in-place + **Consequences** edit to camp ADR-0004, but ADR-0012 lists "Adding or removing + Consequences that weren't previously recorded" and "Reversing or softening the + position" as substantive changes that **still require a new/superseding ADR**. + Correcting the now-false "Refresh yields a genuinely fresh frame" bullet + (ADR-0004:100-105) and adding the CDN-caching consequence is exactly that. Fix: + record the corrected reasoning + cache-buster decision in a NEW camp ADR (e.g. + ADR-0006) and give ADR-0004 only a permitted cross-reference addendum (Status + note / References pointer) — or get explicit sign-off to edit ADR-0004 in place. + — `plan.md:64-66`, `plan.md:104`, `plan.md:113` +- [ ] (suggestion) Empirical confirmation of cause #1 remains an open question, yet + the plan commits to coding the cache-buster before confirming. Causes #2/#3 are + disproven by code-reading (solid), but recommend either running the `.json` + sidecar header/byte diff across two cycles, or explicitly framing the cache-buster + as a low-risk defensive fix that is correct regardless of whether CDN caching is + the sole mechanism. — `plan.md:119-126` +- [ ] (suggestion) Open question #2 (does IEM `tile.py` accept an unknown `?t=` + param, or 4xx/blank?) is load-bearing: if the endpoint rejects unknown params the + fix backfires into blank radar. Recommend a one-shot live `curl` check against the + endpoint during implementation. — `plan.md:124-126` +- [ ] (suggestion) Minor citation: the Context section references + `cached_file_loader.cpp:55` / `:102` without a path; the file lives at + `src/camp2/util/cached_file_loader.cpp`, not under `map_tiles/`. Diagnostic-only + (not a file-to-change), but fix for accuracy. — `plan.md:22-26` + +### Verified (no action) +- File targeting: all 5 files-to-change exist; line refs (`cached_tile_loader.cpp:36` + for `url_str`, `:38` for the disk path; `map_tiles.cpp:229`/`:248`/`:260-266`; + ADR-0004:100-105) are accurate. +- "No CMake change" holds: `cached_tile_loader.cpp` is already in `CAMP_MAP_SOURCES` + (CMakeLists.txt:245) → compiled into `camp_map`, which `test_map_tiles_refresh` + already links (CMakeLists.txt:519-523) along with `Qt5::Test`. +- Test approach is feasible: `tile_loader_` is parented to `MapTiles` + (`map_tiles.cpp:43`), so `findChild()` works like the existing + `findChild()` pattern; the `+1` monotonic guard makes the per-cycle token + assertion deterministic (no wall-clock flakiness). +- Opt-in design is sound: only the radar layer calls `setRefreshInterval` + (`background_manager.cpp:62`; confirmed by `map_tiles.h:64`), so tying + `enableCacheBusting()` to it leaves every static OSM/WMTS layer's URL unchanged. +- Scope (single PR, one subsystem, ~80-120 lines), consequences table (complete), + and ROS conventions (N/A — pure Qt/C++) all check out. + +### Summary +Diagnosis and fix are well-reasoned and the plan is implementation-ready on the code +side — file targeting, the test strategy, and the opt-in design are all verified +against source. The one must-fix is governance, not code: the ADR-0004 update must go +through a new/superseding ADR (ADR-0012 does **not** sanction the in-place Consequences +edit the plan describes). Resolve that and address the cache-buster empirical/endpoint +caveats and the plan is good to implement. + +### Recommended Actions +- [ ] Re-route the ADR-0004 update: new camp ADR for the corrected freshness decision + + cache-buster, with ADR-0004 receiving only a cross-reference addendum (per + ADR-0012). Amend `plan.md` step 5 and the ADR Compliance / Consequences tables. +- [ ] Decide on empirical confirmation of cause #1 (run the sidecar diff, or accept + the cache-buster as a defensive fix) and note the decision in the plan. +- [ ] Verify the live IEM endpoint tolerates `?t=` before relying on it. +- [ ] Fix the `cached_file_loader.cpp` path citation in the Context section. + +## Local Review (Pre-Push) +**Status**: complete +**When**: 2026-06-23 10:14 +00:00 +**By**: Claude Code Agent (Claude Opus) +**Verdict**: approved + +**Branch**: feature/issue-111 at `123d3ef` +**Mode**: pre-push +**Depth**: Deep (reason: ADR removal under docs/decisions/ — Deep promotion trigger; >200 lines) +**Must-fix**: 0 | **Suggestions**: 4 +**Round**: 1 | **Ship**: recommended — no must-fix; cache-buster fix is correct and self-contained, remaining items are advisory. + +### Findings +- [ ] (suggestion) `enableCacheBusting()` re-seeds to wall clock unconditionally — can regress below an already-bumped token if `setRefreshInterval(>0)` is called twice (unreachable today); harden to `cache_bust_==0`-only or `max(cache_bust_+1, now)` — `src/camp2/map_tiles/cached_tile_loader.cpp:98` +- [ ] (suggestion) No end-to-end test that `load()` emits a `?t=`-busted network URL when busting is enabled; the `invalidateCache()`-before-`load()` ordering is unguarded — `test/test_map_tiles_refresh.cpp` +- [ ] (suggestion) ADR-0004 hard-deleted rather than retained as `Status: Superseded (camp#118)` per supersede-don't-delete convention; camp#118 unverifiable from this host (gh unauthenticated) — `docs/decisions/0004-weather-radar-tile-provider.md` +- [ ] (suggestion) Blank-on-`?t=`-rejection is a silent failure mode if IEM ever stops ignoring unknown query params — `src/camp2/map_tiles/cached_tile_loader.cpp:119` + +## Local Review (Pre-Push) +**Status**: complete +**When**: 2026-06-23 10:40 +00:00 +**By**: Claude Code Agent (Claude Opus) +**Verdict**: changes-requested + +**Branch**: feature/issue-111 at `7c4233e` +**Mode**: pre-push +**Depth**: Deep (reason: ADR removal under docs/decisions/ — Deep promotion trigger; >200 lines) +**Must-fix**: 1 | **Suggestions**: 4 +**Round**: 2 | **Ship**: continue — a genuine correctness gap in the new on-show feature (`itemChange`, added since Round 1) warrants a fix + one more read; the core 5-min refresh fix is sound. + +### Findings +- [ ] (must-fix) `itemChange` invalidate-on-show wipes the disk cache + bumps the token but never rebuilds the tile set, so tiles already in `tiles_` (prior show, or the ctor-seeded zoom-0 tile loaded at token 0) are not re-fetched; toggling radar off→on re-shows the prior stale frame until the next 5-min refresh, missing the plan-step-7 "first paint fetches fresh, never stale" guarantee. Fix: call `setLayout(tile_layout_)` after bump+invalidate (mirror `onRefreshTimer`). Core `onRefreshTimer` path unaffected. — `src/camp2/map_tiles/map_tiles.cpp:304` +- [ ] (suggestion) Test gap (ties to must-fix): no test exercises `load()`'s `cache_bust_!=0` branch (busted network URL) or that becoming visible re-fetches; on-show tests assert only token advance, so they pass even with the must-fix bug present (false confidence). — `test/test_map_tiles_refresh.cpp:245` +- [ ] (suggestion) Operational (post-fix): forcing reload on show makes rapid visibility toggling re-download the whole layer (`removeRecursively` + per-tile guaranteed-miss GET); consider a freshness guard/debounce for marginal field connectivity. — `src/camp2/map_tiles/map_tiles.cpp:304` +- [ ] (suggestion) ADR-0004 hard-deleted rather than `Status: Superseded`; decision still live in code; recorded operator decision (plan step 5, camp#118) — confirm camp#118/#119 exist (unverifiable offline). Cross-confirmed with Round-1 review. — `docs/decisions/0004-weather-radar-tile-provider.md` +- [ ] (suggestion) `StaticLayerVisibilityDoesNotBust` comment claims it guards disk-cache-on-show but only asserts token==0; align comment or add a disk-state check. — `test/test_map_tiles_refresh.cpp:266` + +## Local Review (Pre-Push) +**Status**: complete +**When**: 2026-06-23 10:59 +00:00 +**By**: Claude Code Agent (Claude Opus) +**Verdict**: approved + +**Branch**: feature/issue-111 at `f518508` +**Mode**: pre-push +**Depth**: Deep (reason: ADR deletion under docs/decisions/ — Deep promotion trigger; >200 lines) +**Must-fix**: 0 | **Suggestions**: 5 +**Round**: 3 | **Ship**: recommended — Round-2 must-fix resolved and guarded by a targeted regression test (`BecomingVisibleRebuildsTilesNotJustToken`); 0 new must-fix; remaining items advisory/deferred. + +### Findings +- [ ] (suggestion) No end-to-end test exercises `load()`'s `cache_bust_!=0` branch — that the wiring actually emits a `?t=`-busted *network* URL; pure-logic + token tests cover the rest. (Recurring R1/R2.) — `test/test_map_tiles_refresh.cpp` +- [ ] (suggestion) `tile_loader_` is an uninitialized raw pointer read by the `itemChange` guard; currently safe (virtual dispatch + ctor assigns it before `setLayout`), but initialize to `nullptr` to remove the latent risk. — `src/camp2/map_tiles/map_tiles.h:87` +- [ ] (suggestion) Rapid radar off→on toggling forces a full re-download (`removeRecursively` + guaranteed-miss GETs); documented tradeoff, deferred to camp#119; a debounce would help marginal field links. (Recurring R2.) — `src/camp2/map_tiles/map_tiles.cpp:298` +- [ ] (suggestion) ADR-0004 hard-deleted rather than `Status: Superseded`; operator-decided (plan step 5), tracked to camp#118/#119 — confirm those issues exist before push (unverifiable offline). (Recurring R1/R2.) — `docs/decisions/0004-weather-radar-tile-provider.md` +- [ ] (suggestion) Silent blank-on-reject if a future provider 4xx's the unknown `?t=` param (`cached_file_loader.cpp:120` drops the failed reply, never stale — correct posture); surface fetch failures in field UI via camp#119. (Recurring R2.) — `src/camp2/map_tiles/cached_tile_loader.cpp:48` + + diff --git a/docs/decisions/0004-weather-radar-tile-provider.md b/docs/decisions/0004-weather-radar-tile-provider.md deleted file mode 100644 index 0f2b160..0000000 --- a/docs/decisions/0004-weather-radar-tile-provider.md +++ /dev/null @@ -1,150 +0,0 @@ -# ADR-0004: Weather-radar tile provider (IEM NEXRAD N0Q) - -## Status - -Accepted - -## Context - -Issue [#99](https://github.com/rolker/camp/issues/99) adds an auto-refreshing -weather-radar overlay to the camp2 map system. The radar overlay is a network -tile source: it introduces a **new long-lived external dependency** on a -service that the deployed CAMP fetches imagery from at runtime, in the field. -That is an architectural decision worth recording — both for the choice itself -and for its field-operations consequences (what happens when the service is -slow, restructured, or unreachable on a boat with marginal connectivity). - -The map system already has all the tile plumbing (`MapTiles` z/x/y + WMTS -layers, `CachedTileLoader`/`CachedFileLoader` HTTP-plus-disk-cache, -`BackgroundManager::createDefaultLayers()` registration). The only open question -for #99 was *which* radar provider to wire in, how its imagery is delivered, and -how its failure modes interact with field use. - -**Key constraint discovered during endpoint verification (2026-06-18):** the -`MapTiles` layer consumes *tiled* sources (slippy/XYZ or WMTS — a `{z}/{x}/{y}` -address scheme). NOAA's own radar distribution does **not** provide tiled -radar: - -- The legacy ArcGIS service - (`nowcoast.noaa.gov/arcgis/.../radar_meteo_imagery_nexrad_time/MapServer/WMTS`) - has been decommissioned — nowCOAST migrated to GeoServer, and the new host - (`new.nowcoast.noaa.gov`) does not answer. -- The current nowCOAST GeoServer serves radar (`base_reflectivity_mosaic`, - time-enabled MRMS, EPSG:3857) only via **WMS** (dynamic `GetMap` with an - arbitrary bbox). Its GeoWebCache WMTS endpoint advertises only non-radar - layers (bluetopo, stofs3d); the time-varying radar is deliberately not - tile-cached. - -Consuming nowCOAST radar would therefore require building **WMS support** into -CAMP (per-tile EPSG:3857 bbox `GetMap` + time-dimension handling, or a new -dynamic-WMS layer type) — materially more than the tile-plumbing the map system -already has. - -Candidate providers considered: - -- **NOAA nowCOAST (WMS).** Authoritative MRMS radar direct from NOAA, but - WMS-only — does not fit the `MapTiles` tile path without new WMS code. -- **Iowa Environmental Mesonet (IEM) NEXRAD N0Q.** NOAA NEXRAD base-reflectivity - data, redistributed by Iowa State University as **XYZ Web-Mercator - (EPSG:3857) tiles** — a drop-in for the existing `MapTiles` slippy path. No API - key, long-standing academic service, ~5-min cadence, CONUS coverage (includes - the target operating area, Lake Massabesic, NH). The `n0q` product alias - always serves the latest mosaic. -- **RainViewer / commercial tile APIs.** Global XYZ tiles, but the latest-frame - path changes each cycle (requires fetching a JSON index before each refresh), - and the data is third-party rather than NOAA-origin. - -## Decision - -Use **IEM NEXRAD N0Q** as the radar tile provider for the #99 overlay, wired as -an XYZ (slippy) `MapTiles` layer via `osm::generateTileLayout()` — the same path -as the OSM/OpenSeaMap basemap layers, *not* the WMTS path used for the NOAA -charts layer. - -Endpoint (verified live 2026-06-18 — returns 256×256 `image/png` tiles in -EPSG:3857, NH-area tile confirmed): - -``` -https://mesonet.agron.iastate.edu/cache/tile.py/1.0.0/nexrad-n0q-900913/{z}/{x}/{y}.png -``` - -Rationale: - -- **NOAA-origin data.** The imagery is NOAA NEXRAD base reflectivity; IEM is the - redistributor, not the data author. Authoritative for operational use. -- **Fits the existing tile path.** XYZ Web-Mercator tiles drop straight into - `MapTiles`/`generateTileLayout()` — no new protocol code, unlike nowCOAST WMS. -- **No API key.** Nothing to provision, rotate, or risk committing — consistent - with the keyless OSM/NOAA-charts layers and the no-secrets workspace rule. -- **Always-latest frame.** The `nexrad-n0q` alias serves the most recent mosaic - (no timestamp pinning), so the 5-minute refresh genuinely fetches fresh - imagery (see Consequences). -- **US coverage** includes the target operating area. -- **Graceful offline degradation.** On any fetch error (DNS failure, timeout, - 404, offline), `CachedFileLoader::downloadFinished()` logs and drops the reply - *without* emitting `dataLoaded`, so the tile's pixmap is never set — the tile - stays blank. No crash, no UI block. The radar layer also ships **default-OFF** - at ~0.65 opacity, so a missing or stale radar layer cannot degrade the - operator's view of the basemap or chart. - -## Consequences - -- A new runtime network dependency exists (IEM, `mesonet.agron.iastate.edu`). If - IEM changes URLs or goes down, the radar layer shows blank tiles; everything - else is unaffected (default-OFF, independent layer). This ADR is the traceable - record if the provider must be swapped (e.g. back to a NOAA-direct WMS feed if - CAMP ever grows WMS support). -- The radar layer refreshes every ~5 minutes (`setRefreshInterval`, #99 Phase 2), - re-fetching from IEM each cycle. Cadence is hardcoded for now; exposing it as a - user setting is a possible follow-up. -- **Refresh yields a genuinely fresh frame** because `nexrad-n0q` always resolves - to the latest mosaic. The disk-cache invalidation in `onRefreshTimer` - (`map_tiles.cpp`) is what forces the network re-fetch — without it the on-disk - PNGs would re-serve the previous frame for the same `z/x/y`. A future - timestamp-pinned provider would break this assumption (static overlay); that - caveat is noted at `onRefreshTimer`. -- **Enabling radar reintroduces #98-class within-cycle tile accumulation — on a - second layer.** The refresh path bounds memory only AT each refresh boundary - (`setLayout` resets the tile set); within a cycle, `paint()` still only hides - (never deletes) tiles as the operator pans/zooms, exactly as the existing - layers do. Default-OFF makes this *conditional* — it does not eliminate the #98 - leak, it just keeps the radar layer from contributing unless an operator turns - it on. With radar enabled, an operator who pans/zooms heavily before a refresh - accumulates tiles on the radar layer in addition to the basemap/chart layers. - The bounded-memory test guards the refresh-boundary reset, not the within-cycle - growth. -- Disk caching applies per-layer under - `~/.CCOMAutonomousMissionPlanner/map_tiles/nexrad_radar/`; the refresh path - invalidates that subdir each cycle so stale radar PNGs are not re-served. - -## Alternatives considered - -- **NOAA nowCOAST (WMS), direct.** Rejected for #99: radar is WMS-only, which - does not fit the `MapTiles` tile path. Adopting it would mean building a WMS - `GetMap` layer type (per-tile bbox + time dimension) — a larger feature than - this overlay warrants. Revisit if CAMP grows general WMS support or if - direct-from-NOAA provenance becomes a hard requirement. -- **RainViewer / commercial radar tiles.** Rejected as the default: the - latest-frame path changes each cycle (extra JSON-index fetch before every - refresh) and the data is third-party rather than NOAA-origin. Can be revisited - if non-US coverage is ever needed. -- **No radar overlay (status quo).** Rejected: situational weather awareness on - open water is operationally valuable, and the map system already had every - piece needed except a periodic refresh. - -## References - -- Issue [#99](https://github.com/rolker/camp/issues/99) — the radar overlay -- [ADR-0002](0002-web-mercator-scene-and-layer-model.md) — Web-Mercator scene + - library split (the radar layer lives in pure-Qt `libcamp_map`) -- [ADR-0003](0003-backgrounds-as-layers-and-depth-tree.md) — backgrounds as - stackable layers (the radar is one such stacked, toggleable layer) -- [#98](https://github.com/rolker/camp/issues/98) — tile-lifecycle OOM; the - refresh path's bounded-memory test (`test/test_map_tiles_refresh.cpp`) is the - coordination gate (the radar is a QPixmap `MapTiles` layer, NOT the #96 GDAL - raster path) -- IEM NEXRAD tile service: - `https://mesonet.agron.iastate.edu/cache/tile.py/1.0.0/nexrad-n0q-900913/{z}/{x}/{y}.png` - (NOAA NEXRAD base reflectivity, EPSG:3857, ~5-min cadence; verified 2026-06-18) -- `src/camp2/background/background_manager.cpp` — registration point -- `src/camp2/util/cached_file_loader.cpp` — the graceful-degradation fetch path diff --git a/src/camp2/map_tiles/cached_tile_loader.cpp b/src/camp2/map_tiles/cached_tile_loader.cpp index 7eab057..ae82c21 100644 --- a/src/camp2/map_tiles/cached_tile_loader.cpp +++ b/src/camp2/map_tiles/cached_tile_loader.cpp @@ -5,11 +5,14 @@ #include #include #include +#include //#include "main/main_window.h" #include "util/cached_file_loader.h" #include +#include + namespace camp { @@ -35,6 +38,17 @@ void CachedTileLoader::load(TileAddress address) } QString url_str = address.url().c_str(); + // [#111] Defeat intermediary (CDN/proxy) HTTP caching of the cache-buster-less + // tile URL: when enabled, append a per-refresh token to the *network* URL only. + // The disk path below is built from `address` (z/x/y.png), so the buster never + // touches the local cache layout — it just forces the GET to be a URL the CDN + // cannot answer from its own cache. 0 == disabled (static layers append nothing). + // Safe degradation: a tile server that 4xx'd on the unknown ?t= param would just + // yield a blank tile (CachedFileLoader drops a failed reply without setting the + // pixmap) — never a stale frame. IEM tolerates ?t= today (verified, HTTP 200). + if(cache_bust_ != 0) + url_str = QString::fromStdString(withCacheBust(url_str.toStdString(), cache_bust_)); + QFileInfo file_path(local_cache_path_, address); CachedFileClient* client = new CachedFileClient(this); @@ -84,6 +98,43 @@ void CachedTileLoader::invalidateCache() QDir::root().mkpath(normalized); } +void CachedTileLoader::enableCacheBusting() +{ + // Idempotent: if busting is already on, leave the token alone rather than + // re-seeding to the current clock — a fresh seed could regress BELOW a token + // already advanced by bumpCacheBust() (which can sit above wall-clock via its + // +1 guard), briefly re-exposing a CDN-cached URL. Unreachable today (one + // setRefreshInterval call per layer) but cheap to make robust. + if(cache_bust_ != 0) + return; + // Seed from the wall clock so the token is distinct across sessions too (the + // disk cache survives a restart, so a session-local counter starting at 0 each + // launch could collide with a previously-cached busted URL). std::max with 1 + // keeps it non-zero (0 means disabled) even in the impossible clock-at-epoch case. + cache_bust_ = std::max(1, static_cast(QDateTime::currentMSecsSinceEpoch())); +} + +void CachedTileLoader::bumpCacheBust() +{ + if(cache_bust_ == 0) // not a refreshing layer — nothing to bump. + return; + // Advance to the current wall clock, but never to an equal-or-lower value than + // last time: the +1 guard guarantees a STRICTLY greater token even on two + // refreshes within the same millisecond. Strict monotonicity is the invariant + // the regression test asserts (and it means each cycle's URL is genuinely new). + cache_bust_ = std::max(cache_bust_ + 1, + static_cast(QDateTime::currentMSecsSinceEpoch())); +} + +std::string CachedTileLoader::withCacheBust(const std::string& url, quint64 token) +{ + // Join with '?' if the URL has no query yet, else '&'. The XYZ tile URLs used + // by refreshing layers (e.g. IEM radar) carry no query, so this is the '?' path; + // the '&' branch keeps the helper correct for any URL that already has one. + const char separator = (url.find('?') == std::string::npos) ? '?' : '&'; + return url + separator + "t=" + std::to_string(token); +} + void CachedTileLoader::dataLoaded(QByteArray &data, CachedFileClient* client) { auto address = client->property("address").value(); diff --git a/src/camp2/map_tiles/cached_tile_loader.h b/src/camp2/map_tiles/cached_tile_loader.h index 498ac23..42312bd 100644 --- a/src/camp2/map_tiles/cached_tile_loader.h +++ b/src/camp2/map_tiles/cached_tile_loader.h @@ -5,6 +5,7 @@ #include "tile_address.h" #include #include +#include namespace camp { @@ -37,6 +38,26 @@ class CachedTileLoader: public QObject // not the expected map_tiles subdir (never touches the global cache root). void invalidateCache(); + // [#111] Per-refresh URL cache-busting. invalidateCache() clears only the + // *local* disk cache; a CDN/proxy between CAMP and the tile origin can still + // re-serve a stale tile for the same cache-buster-less z/x/y URL. Appending a + // distinct query token to the request URL each refresh makes the GET a URL the + // intermediary cannot satisfy from cache. Opt-in (off until enabled), so static + // OSM/WMTS layers are unaffected; only refreshing layers (radar) enable it. + // + // enableCacheBusting() seeds the token from the wall clock so freshness holds + // across sessions too (the on-disk cache survives a restart). bumpCacheBust() + // advances it with a strictly-monotonic guard so back-to-back refreshes always + // produce a distinct token — the testable invariant. The busted token is + // applied to the network URL only; the on-disk path stays z/x/y.png. + void enableCacheBusting(); + void bumpCacheBust(); + quint64 cacheBustToken() const { return cache_bust_; } + + // Append a "?t=" (or "&t=" if the URL already has a query) cache- + // buster to a tile URL. Pure/static so it is unit-testable without a network. + static std::string withCacheBust(const std::string& url, quint64 token); + signals: void pixmapLoaded(QPixmap pixmap, TileAddress tile_address); @@ -47,6 +68,11 @@ public slots: // Base relative file location where map tiles are stored locally QString local_cache_path_; + // [#111] Per-refresh URL cache-buster token. 0 == disabled (the default, so + // non-refreshing layers append nothing). Non-zero on a refreshing layer once + // enableCacheBusting() has run; advanced by bumpCacheBust() each cycle. + quint64 cache_bust_ = 0; + private slots: void dataLoaded(QByteArray &data, CachedFileClient* client); diff --git a/src/camp2/map_tiles/map_tiles.cpp b/src/camp2/map_tiles/map_tiles.cpp index d6f9ce2..aef3a24 100644 --- a/src/camp2/map_tiles/map_tiles.cpp +++ b/src/camp2/map_tiles/map_tiles.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -243,33 +244,82 @@ void MapTiles::setRefreshInterval(int msec) connect(refresh_timer_, &QTimer::timeout, this, &MapTiles::onRefreshTimer); } refresh_timer_->start(msec); + + // [#111] A refreshing layer (radar) is exactly where per-refresh URL cache- + // busting belongs: it re-fetches the same z/x/y URLs every cycle, so without a + // changing query token a CDN/proxy can re-serve a stale frame. Tying this to + // setRefreshInterval (and nothing else calls it for static layers) keeps the + // OSM/WMTS basemap URLs untouched. + if(tile_loader_) + tile_loader_->enableCacheBusting(); } -void MapTiles::onRefreshTimer() +void MapTiles::refreshTiles() { - // Drop disk-cached PNGs first so the re-fetch hits the network rather than - // re-serving stale tiles, then reset the layout. setLayout() deletes every - // current Tile* (each holds a QGraphicsPixmapItem child) and rebuilds the - // zoom-0 tiles, so memory is reset to the seed set AT each refresh boundary. - // Within a cycle, paint() no longer grows tiles_ without bound: the [#98] LRU - // eviction (see evictIfNeeded) caps off-screen tile accumulation as the - // viewport pans/zooms, which is what actually bounds the never-refreshing - // OSM/WMTS basemap. This refresh reset and the eviction cap are complementary — - // the refresh exists for radar freshness, not as the memory bound. + // Force a genuinely fresh re-fetch of every tile. Three steps, all required: + // 1. bumpCacheBust() advances the per-refresh URL token, so the network GET + // is a URL a *CDN/proxy* between CAMP and the origin cannot answer from + // its own edge cache. Disk invalidation alone (#99) does NOT defeat that + // intermediary cache — that gap is exactly the #111 stale-radar bug. + // 2. invalidateCache() drops this layer's *local* disk PNGs, so the next + // load() goes to the network instead of re-serving the prior frame for + // the same z/x/y. + // 3. setLayout() deletes every current Tile* (each holds a QGraphicsPixmapItem + // child) and rebuilds the zoom-0 tiles, which is what actually RE-ISSUES + // the load() requests — without it, steps 1-2 only affect tiles fetched + // *later*, leaving the already-built (stale) Tile objects on screen. + // setLayout() also resets memory to the seed set AT each refresh boundary; + // within a cycle, paint() growth is bounded separately by the [#98] LRU + // eviction (see evictIfNeeded), so this reset and the eviction cap are + // complementary — the refresh exists for radar freshness, not the memory bound. // - // FRESHNESS (#99): re-fetching each cycle yields a genuinely *fresh* radar frame - // because the configured IEM "nexrad-n0q" tile product always serves the latest - // mosaic (no timestamp pinning) — verified live 2026-06-18 (see ADR-0004). The - // disk-cache invalidation below is what forces the network re-fetch; without it - // the on-disk PNGs would re-serve the previous frame for this same z/x/y. A - // future timestamp-pinned source would break this assumption and re-serve a - // static frame — keep that in mind if the radar provider is ever changed. + // The IEM "nexrad-n0q" product resolves to the latest mosaic (no timestamp + // pinning), so once both caches are bypassed the re-fetch is genuinely current. + // The cache-buster is a defensive fix: it is correct regardless of whether CDN + // caching is the sole mechanism (a no-op query param if no intermediary caches). + // IEM is an interim stopgap — the intended end state is NOAA nowCOAST via WMS + // (camp#118), at which point the provider's freshness contract is revisited. if(tile_loader_) + { + tile_loader_->bumpCacheBust(); tile_loader_->invalidateCache(); + } setLayout(tile_layout_); update(); } +void MapTiles::onRefreshTimer() +{ + // The 5-minute cadence (#99 Phase 2) for time-varying overlays (radar): each + // tick re-fetches a fresh frame via the shared refresh path. + refreshTiles(); +} + +QVariant MapTiles::itemChange(GraphicsItemChange change, const QVariant& value) +{ + // [#111] Make the FIRST paint after the operator turns a refreshing layer on + // fetch fresh, not a frame cached in a previous session. The 5-minute timer is + // the only OTHER thing that refreshes, so enabling radar shortly after launch + // (or toggling it off→on) would otherwise re-show the previously-built tiles — + // yesterday's on-disk frame — until the next tick. On the visible transition we + // run the same full refresh (bump + invalidate + rebuild), so the ensuing paint + // re-fetches with a CDN-distinct URL. Gated on cache-busting being enabled, + // which is true only for refreshing (radar) layers — static OSM/WMTS basemaps + // keep their cache (and built tiles) on show. + // + // Tradeoff: rapidly toggling radar off→on re-downloads the layer each time (the + // "always fresh on show" guarantee is deliberate — never show a stale frame). + // A freshness guard (skip the reload if the last fetch is recent) would need + // per-fetch timestamps, which is the broader staleness-tracking work in camp#119; + // deferred there rather than added here. + if(change == ItemVisibleHasChanged && value.toBool() && tile_loader_ && + tile_loader_->cacheBustToken() != 0) + { + refreshTiles(); + } + return Layer::itemChange(change, value); +} + void MapTiles::updateViewScale(double view_scale) { //updateViewport(view_context_.viewport); diff --git a/src/camp2/map_tiles/map_tiles.h b/src/camp2/map_tiles/map_tiles.h index 60320a6..8af1c9f 100644 --- a/src/camp2/map_tiles/map_tiles.h +++ b/src/camp2/map_tiles/map_tiles.h @@ -55,6 +55,15 @@ public slots: void updateViewScale(double view_scale); void wmtsCapabilitiesReady(); +protected: + // [#111] When a refreshing layer (radar) becomes visible, drop its disk cache + // and advance the cache-buster so the FIRST paint after the operator enables it + // fetches a fresh frame instead of serving a tile cached in a previous session + // (e.g. yesterday's radar). Static layers (cache-busting off) are untouched, so + // their disk cache is never dropped on show. The brief gap shows blank, never a + // stale frame. + QVariant itemChange(GraphicsItemChange change, const QVariant& value) override; + private: TileLayout tile_layout_; std::map tiles_; @@ -75,7 +84,7 @@ public slots: std::map tile_last_visible_gen_; bool eviction_pending_ = false; - CachedTileLoader* tile_loader_; + CachedTileLoader* tile_loader_ = nullptr; const wmts::Capabilities* wmts_capabilites_ = nullptr; QString wmts_layer_id_; @@ -91,6 +100,14 @@ public slots: // pre-refresh pixmap would then satisfy the tileLoaded guard on the rebuilt // same-position tile and paint a stale radar frame for up to one cycle. quint64 layout_epoch_ = 0; + + // [#111] Shared refresh path: bump the cache-buster, drop the disk cache, and + // rebuild the tile set (re-issuing the network loads). Called by the periodic + // onRefreshTimer() and by itemChange() when a refreshing layer becomes visible, + // so the two cannot drift — both deliver a genuinely fresh frame, never a stale + // one carried over in the already-built tiles. + void refreshTiles(); + private slots: void tileLoaded(QPixmap pixmap, TileAddress tile); diff --git a/test/test_map_tiles_refresh.cpp b/test/test_map_tiles_refresh.cpp index 957f2a3..c0ffc5b 100644 --- a/test/test_map_tiles_refresh.cpp +++ b/test/test_map_tiles_refresh.cpp @@ -12,6 +12,13 @@ // (isActive/interval/disabled) and verify one DETERMINISTIC refresh by invoking // the refresh slot directly. // +// [#111] This file also covers the per-refresh URL cache-buster that defeats +// CDN/proxy caching of the static radar tile URL: withCacheBust() pure-logic, the +// strict per-cycle token advance on refresh, and that static layers never bust. +// The invariant is URL/token DISTINCTNESS per cycle — asserted without any live +// network (the IEM endpoint's tolerance of an unknown ?t= param was confirmed +// out-of-band, HTTP 200 + identical bytes). +// // Access mechanism (named per plan-review finding #2): MapTiles::tiles_, // setLayout, and onRefreshTimer are all private. Tests therefore: // - count Tile children via the public QGraphicsItem::childItems() + @@ -33,11 +40,13 @@ #include "map_tiles/map_tiles.h" #include "map_tiles/tile.h" #include "map_tiles/tile_layout.h" +#include "map_tiles/cached_tile_loader.h" using camp::map::Map; using camp::map_tiles::MapTiles; using camp::map_tiles::Tile; using camp::map_tiles::TileLayout; +using camp::map_tiles::CachedTileLoader; namespace { @@ -173,6 +182,137 @@ TEST(MapTilesRefresh, RefreshBoundaryResetsTileSet) } } +// [#111] withCacheBust is pure/static: a distinct token must yield a distinct URL, +// joined with '?' when the URL has no query yet and '&' when it already does. +TEST(MapTilesRefresh, CacheBustAppendsDistinctQueryToken) +{ + // No existing query → '?t='. XYZ tile URLs (e.g. IEM radar) take this path. + EXPECT_EQ(CachedTileLoader::withCacheBust("https://h/9/1/2.png", 42), + "https://h/9/1/2.png?t=42"); + // Existing query → '&t=' so the URL stays well-formed. + EXPECT_EQ(CachedTileLoader::withCacheBust("https://h/wms?layer=x", 42), + "https://h/wms?layer=x&t=42"); + // The whole point: different tokens produce different URLs (CDN-distinct). + EXPECT_NE(CachedTileLoader::withCacheBust("https://h/9/1/2.png", 1), + CachedTileLoader::withCacheBust("https://h/9/1/2.png", 2)); +} + +// Enabling refresh (setRefreshInterval > 0) turns cache-busting on with a non-zero +// seed, and every refresh cycle advances the token STRICTLY — so each cycle's +// request URL is one a CDN/proxy cannot satisfy from its edge cache (#111). +TEST(MapTilesRefresh, RefreshAdvancesCacheBustTokenStrictly) +{ + Map map; + auto* layer = new MapTiles(map.topLevelLayers(), "test_radar_bust", makeLayout(1, 1)); + + layer->setRefreshInterval(300000); // enables busting (radar cadence) + CachedTileLoader* loader = layer->findChild(); + ASSERT_NE(loader, nullptr) << "MapTiles owns a CachedTileLoader child"; + + const quint64 t0 = loader->cacheBustToken(); + EXPECT_NE(t0, quint64(0)) + << "enableCacheBusting (via setRefreshInterval) should seed a non-zero token"; + + ASSERT_TRUE(invokeRefresh(layer)); + const quint64 t1 = loader->cacheBustToken(); + ASSERT_TRUE(invokeRefresh(layer)); + const quint64 t2 = loader->cacheBustToken(); + + EXPECT_GT(t1, t0) << "a refresh must advance the cache-bust token"; + EXPECT_GT(t2, t1) + << "each refresh must yield a STRICTLY greater token (a CDN-distinct URL), " + "even for back-to-back cycles within the same millisecond"; +} + +// Refresh (and therefore cache-busting) is strictly opt-in: a layer that never +// calls setRefreshInterval — every existing static OSM/WMTS layer — keeps token 0, +// so withCacheBust is never applied and its request URLs are unchanged. +TEST(MapTilesRefresh, StaticLayerNeverBustsCache) +{ + Map map; + auto* layer = new MapTiles(map.topLevelLayers(), "test_static_bust", makeLayout(1, 1)); + + CachedTileLoader* loader = layer->findChild(); + ASSERT_NE(loader, nullptr); + EXPECT_EQ(loader->cacheBustToken(), quint64(0)) + << "a layer that never enables refresh must not cache-bust its URLs"; +} + +// [#111] A refreshing (radar) layer becoming visible must run the full refresh, +// which advances the cache-bust token (the network URL becomes CDN-distinct). +TEST(MapTilesRefresh, BecomingVisibleAdvancesCacheBustToken) +{ + Map map; + auto* layer = new MapTiles(map.topLevelLayers(), "test_radar_show", makeLayout(1, 1)); + + layer->setRefreshInterval(300000); // enable cache-busting (radar) + layer->setVisible(false); + CachedTileLoader* loader = layer->findChild(); + ASSERT_NE(loader, nullptr); + + const quint64 hidden = loader->cacheBustToken(); + ASSERT_NE(hidden, quint64(0)) << "refreshing layer should have busting enabled"; + + layer->setVisible(true); // show transition → full refresh (bump + invalidate + rebuild) + EXPECT_GT(loader->cacheBustToken(), hidden) + << "becoming visible must advance the token so the first paint fetches fresh, " + "never a previous session's cached frame"; +} + +// [#111] Token advance alone is NOT enough — the already-built tiles must be +// REBUILT on show, else they re-show the stale (e.g. yesterday's) frame until the +// next 5-min refresh. This guards the must-fix: itemChange must run the full +// refresh (setLayout rebuild), not just bump+invalidate. We count destruction of +// the pre-show Tile objects (robust to allocator pointer reuse): a rebuild deletes +// and re-creates them; a token-only path would leave them alive. +TEST(MapTilesRefresh, BecomingVisibleRebuildsTilesNotJustToken) +{ + Map map; + auto* layer = new MapTiles(map.topLevelLayers(), "test_radar_show_rebuild", makeLayout(2, 2)); + + layer->setRefreshInterval(300000); // refreshing (radar) layer + layer->setVisible(false); + ASSERT_EQ(tileChildCount(layer), 4) << "ctor seeds 2x2 = 4 tiles"; + + int destroyed = 0; + for(QGraphicsItem* child : layer->childItems()) + if(Tile* t = qgraphicsitem_cast(child)) + QObject::connect(t, &QObject::destroyed, [&destroyed]{ ++destroyed; }); + + layer->setVisible(true); // show → full refresh must DELETE + rebuild the tiles + + EXPECT_EQ(destroyed, 4) + << "becoming visible must REBUILD the tile set (re-fetch), not merely advance " + "the token — otherwise the already-built stale tiles persist on screen"; + EXPECT_EQ(tileChildCount(layer), 4) << "rebuilt back to the 2x2 layout"; +} + +// A static (non-refreshing) layer toggling visibility never enters the +// invalidate-on-show branch: itemChange gates the whole bump/invalidate/rebuild on +// cacheBustToken() != 0, and a static layer's token stays 0 — so its disk cache and +// built tiles are preserved on show. token == 0 is the proxy for "branch not taken". +TEST(MapTilesRefresh, StaticLayerVisibilityDoesNotRefresh) +{ + Map map; + auto* layer = new MapTiles(map.topLevelLayers(), "test_static_show", makeLayout(2, 2)); + + int destroyed = 0; + for(QGraphicsItem* child : layer->childItems()) + if(Tile* t = qgraphicsitem_cast(child)) + QObject::connect(t, &QObject::destroyed, [&destroyed]{ ++destroyed; }); + + layer->setVisible(false); + layer->setVisible(true); + + CachedTileLoader* loader = layer->findChild(); + ASSERT_NE(loader, nullptr); + EXPECT_EQ(loader->cacheBustToken(), quint64(0)) + << "a static layer must never enable cache-busting (token stays 0)"; + EXPECT_EQ(destroyed, 0) + << "a static layer must NOT rebuild its tiles on show — the invalidate-on-show " + "branch is gated on a non-zero token, which a static layer never has"; +} + int main(int argc, char** argv) { // MapTiles is a QGraphicsObject; Map builds a QGraphicsScene and MapItem ctors