diff --git a/.agent/work-plans/issue-98/plan.md b/.agent/work-plans/issue-98/plan.md new file mode 100644 index 0000000..b11b68a --- /dev/null +++ b/.agent/work-plans/issue-98/plan.md @@ -0,0 +1,147 @@ +# Plan: CAMP silently crashes on map zoom/pan during live ops (recurring) — likely OOM + +## Issue + +https://github.com/rolker/camp/issues/98 + +## Context + +`MapTiles::paint()` (`src/camp2/map_tiles/map_tiles.cpp`) creates a new `Tile*` for +each newly-visible tile address and inserts it into `tiles_` (a +`std::map`). Off-screen tiles are only hidden via +`setVisible(false)`. The **only** deletion of accumulated tiles is in `setLayout()`, +which is called at construction and on each `onRefreshTimer` fire. + +The radar overlay calls `setRefreshInterval()`, so `setLayout()` runs every ~5 min and +self-bounds. The OSM/WMTS basemap never calls `setRefreshInterval()`, so `setLayout()` +is never called after initial construction. Every new tile area visited in a pan/zoom +session adds a permanent entry to `tiles_` → RSS grows monotonically → OOM-kill (silent +crash with no stacktrace). This matches the field evidence (crash while zooming/panning, +auto-relaunch healthy). + +The `test_map_tiles_refresh.cpp` comment (lines 149–156) explicitly notes this gap: +"this guards ONLY the refresh-boundary reset … does NOT cover the within-cycle growth +that the #98 OOM is about." + +## Approach + +1. **Add LRU eviction state to `MapTiles`** — a paint-call generation counter + (`paint_generation_`) and a per-tile last-visible generation map + (`tile_last_visible_gen_`). These track the most recent paint call in which each + tile was in the visible set. Both live in `MapTiles` (no change to `Tile`). + +2. **Schedule eviction from `paint()`, run it DEFERRED (as built)** — after the + existing visibility loop, `paint()` does NOT delete any tile (deleting a `Tile`, + a `QGraphicsObject` scene child, mid-paint is a use-after-free risk per the Plan + Review must-fix). Instead it: + - Records `tile_last_visible_gen_[addr] = paint_generation_` for each visible tile. + - Increments `paint_generation_`. + - Computes the cap `max(kTileEvictionMinCap, kTileEvictionMultiplier * int(visible_tiles.size()))`. + - If `tiles_.size() > cap` AND `!eviction_pending_`, sets `eviction_pending_ = true` + and queues `evictIfNeeded()` onto the event loop via + `QMetaObject::invokeMethod(this, "evictIfNeeded", Qt::QueuedConnection)`. The + `eviction_pending_` flag debounces this so paint() never queues more than one + pending eviction. + + **`evictIfNeeded()` (private slot, the primary deletion path)** runs in the event + loop where deleting scene items is safe: + - Clears `eviction_pending_`. + - Recomputes the cap from the CURRENT visible count (counts `tile->isVisible()`). + - Collects candidates = tiles NOT visible right now (live `isVisible()` guard, so a + tile re-shown between scheduling and now is never evicted). + - Sorts candidates ascending by `tile_last_visible_gen_` (missing entry → 0 = + oldest) and deletes oldest-first (`delete` + `tiles_.erase` + erase its + `tile_last_visible_gen_` entry) until `tiles_.size() <= cap`. Because + `cap >= visible_count`, there are always enough non-visible candidates to reach + the cap without touching a visible tile. + +3. **Clear eviction state in `setLayout()`** — add `tile_last_visible_gen_.clear()` + and `eviction_pending_ = false` alongside the existing `tiles_.clear()`. + `paint_generation_` is not reset (monotonic counter; cleared entries for the new + tile set start fresh). + +4. **Update `onRefreshTimer` comment** — lines 150–155 in `map_tiles.cpp` say + "paint() still only hides (not deletes) tiles … between refreshes." This will no + longer be accurate once step 2 lands; update the comment to reflect that eviction + now also bounds within-cycle accumulation. + +5. **Add `test_map_tiles_eviction.cpp` (as built)** — a MULTI-zoom-level layout is + required: `setLayout()` pre-seeds the whole `zoom_levels.front()` grid at + construction, so a single-zoom layout mints nothing in `paint()` and would not + reproduce the bug (Plan Review suggestion). The test builds a 1×1 front level plus + a 20×20 deeper level; `paint()` renders and mints the deeper level on demand. + `paint()` is driven directly with a hand-built `QPainter` world transform + (`QTransform(8,0,0,-8,tx,ty)` — uniform scale 8 with a web-mercator y-flip), chosen + so all index arithmetic in `MapTiles::paint()` lands on exact integers; this is + deterministic and avoids `QGraphicsScene::render()` QPA fragility entirely (the + plan's fallback proved unnecessary). A small (one-tile) viewport is panned across + every position of the 20×20 grid; because eviction is DEFERRED, + `QApplication::processEvents()` is called after each `paint()` so the queued + `evictIfNeeded()` runs. The test asserts `tileChildCount()` stays ≤ the 256 floor + throughout and after the full sweep. A second test (`DeepLevelTilesAreMintedByPaint`) + asserts the deeper tiles really are minted by `paint()` (not pre-seeded), so the + pan genuinely exercises the accumulation path. Non-vacuity was verified empirically: + with `evictIfNeeded()` stubbed to a no-op, `tiles_` reaches 401 (1 front + 400 deep) + and the test fails at the 256 cap. Follows `test_map_tiles_refresh.cpp` patterns + (offscreen QApplication, `tileChildCount` helper). + +6. **Register test in `CMakeLists.txt`** — mirror the `test_map_tiles_refresh` block: + `ament_add_gtest`, include `${CMAKE_CURRENT_SOURCE_DIR}/src/camp2`, link + `camp_map Qt5::Widgets Qt5::Test`. + +## Files to Change + +| File | Change | +|------|--------| +| `src/camp2/map_tiles/map_tiles.h` | Add `paint_generation_`, `tile_last_visible_gen_`, `eviction_pending_`, and the `evictIfNeeded()` private slot | +| `src/camp2/map_tiles/map_tiles.cpp` | Eviction constants (file-private); schedule deferred eviction in `paint()`; `evictIfNeeded()` slot (primary deletion path); clear eviction state in `setLayout()`; update `onRefreshTimer` comment | +| `test/test_map_tiles_refresh.cpp` | Update SCOPE comment to point at the new eviction test | +| `test/test_map_tiles_eviction.cpp` | New test asserting `tileChildCount()` bounded after pan | +| `CMakeLists.txt` | Register `test_map_tiles_eviction` target | + +## Principles Self-Check + +| Principle | Consideration | +|---|---| +| Safety first | Eviction guard ensures currently-visible tiles are never evicted — the operator display never shows a blank tile due to eviction | +| Only what's needed | Change is confined to `MapTiles`; no new classes, no change to `Tile` or `TileAddress` | +| Test what breaks | New test asserts the exact invariant that caused the OOM — `tiles_.size()` bounded during pan | +| A change includes its consequences | `onRefreshTimer` comment update included; `test_map_tiles_refresh.cpp` scope comment updated to reflect that the within-cycle gap is now closed | +| Capture decisions, not just implementations | Eviction constants documented with a comment at the definition site explaining the N× rationale | +| Improve incrementally | Single PR, single mechanism, no layout or API changes | + +## ADR Compliance + +| ADR | Triggered | How addressed | +|---|---|---| +| ADR-0002 (worktree isolation) | Yes | Work is in `issue-camp-98` worktree, camp repo on `feature/issue-98` | +| ADR-0008 (ROS 2 conventions) | Partial | C++ Qt code in a ROS 2 package; follows C++ naming conventions; no new package interfaces | +| ADR-0013 (progress.md vocabulary) | Yes | `## Plan Authored` entry written per vocabulary | + +## Consequences + +| If we change... | Also update... | Included in plan? | +|---|---|---| +| Eviction in `paint()` | `onRefreshTimer` comment (lines 150–155) that says "paint() still only hides" | Yes — step 4 | +| `tiles_` map now has eviction | `tile_last_visible_gen_` must be cleared in `setLayout()` to avoid stale entries after layout change | Yes — step 3 | +| Within-cycle gap closed | `test_map_tiles_refresh.cpp` SCOPE comment (lines 149–156) acknowledging the gap — update to note the gap is fixed in `test_map_tiles_eviction.cpp` | Yes — step 4 scope | + +## Open Questions (resolved) + +- **Constant values — RESOLVED (operator-confirmed).** `kTileEvictionMinCap = 256`, + `kTileEvictionMultiplier = 4`, giving `cap = max(256, 4 × visible)`. OSM tiles decode + to ~256×256 ARGB (~256 KB each), so the 256-tile floor caps the basemap tile buffer + at ~64 MB — a generous pan buffer sized for the salmon operator workstation, bounded + so an all-afternoon survey can't OOM-kill CAMP. The multiplier keeps the cap above the + working set at high zoom. Rationale is documented at the constants' definition site in + `map_tiles.cpp`. +- **Test driving mechanism — RESOLVED.** `QGraphicsScene::render()` was not needed. + `paint()` is called directly with a hand-built `QPainter` world transform, which is + fully deterministic under offscreen QPA and lets the test step the viewport exactly + tile-by-tile. The planned `evictIfNeeded`-via-`invokeMethod` fallback became the + PRIMARY production mechanism (deferred eviction, per the Plan Review must-fix) rather + than a test-only path. + +## Estimated Scope + +Single PR. diff --git a/.agent/work-plans/issue-98/progress.md b/.agent/work-plans/issue-98/progress.md new file mode 100644 index 0000000..9946958 --- /dev/null +++ b/.agent/work-plans/issue-98/progress.md @@ -0,0 +1,185 @@ +--- +issue: 98 +--- + +# Issue #98 — CAMP silently crashes on map zoom/pan during live ops (recurring) — likely OOM + +## Issue Review +**Status**: complete +**When**: 2026-06-22 12:00 +00:00 +**By**: Claude Code Agent (Claude Sonnet) + +**Issue**: #98 +**Comment**: (best-effort post follows this entry; not recorded inline) +**Scope verdict**: well-scoped + +### Summary + +The root cause (per the verified host note, 2026-06-22) is `MapTiles::paint()` accumulating +`Tile*` entries in `tiles_` without eviction: off-screen tiles are only hidden +(`setVisible(false)`) — deletion only occurs in `setLayout()`. The radar overlay's +`setRefreshInterval` path calls `setLayout()` periodically and self-bounds; the OSM/WMTS +basemap has no refresh interval and never calls `setLayout()` after initial load, so its +`tiles_` map grows monotonically with each newly-visited tile area → OOM-kill → the silent +crash. The fix is an LRU eviction cap on `tiles_` in `paint()`, scoped to the basemap +accumulation path, preserving the radar's refresh semantics and never evicting +currently-visible tiles. + +### Principle Alignment + +| Principle | Status | Notes | +|---|---|---| +| Human control and transparency | OK | Fix is mechanical memory bounding; no user-visible behavior change except elimination of silent OOM crash | +| Enforcement over documentation | OK | Fix is in code + backed by a test | +| Capture decisions, not just implementations | Watch | Eviction policy parameters (N × visible-tile cap, LRU strategy) are design choices; should be recorded in a code comment or follow-up ADR note so the bound is traceable | +| A change includes its consequences | Action needed | Issue requires a test asserting `tiles_` stays bounded across simulated pan; the `onRefreshTimer` comment in `map_tiles.cpp:150–155` calls out "paint() still only hides tiles" — this comment must be updated when the fix lands | +| Only what's needed | OK | Targeted: add eviction to paint() basemap path; preserve radar refresh semantics; no other scope | +| Improve incrementally | OK | Single class, single mechanism; completable in one PR | +| Test what breaks | Action needed | A `test_map_tiles_eviction.cpp` (or extension of `test_map_tiles_refresh.cpp`) must assert that `tiles_` size stays bounded after a simulated pan across many new tile addresses | +| Workspace vs. project separation | OK | Change is entirely within the `camp` project repo (`ui_ws/src/camp/`) | +| Safety First (project) | Watch | CAMP is the operator display; OOM mid-survey is a vigilance/safety hit. Eviction must never discard currently-visible tiles — the issue states this, but it must be enforced by the test | + +### ADR Applicability + +| ADR | Triggered | Notes | +|---|---|---| +| ADR-0002 — Worktree isolation | Yes | Already satisfied: work is in the `issue-camp-98` worktree | +| ADR-0008 — ROS 2 conventions | Partial | Qt/C++ code in a ROS 2 package; follow C++ conventions; no new package interfaces | +| ADR-0013 — progress.md vocabulary | Yes | This entry uses `## Issue Review` per the vocabulary | + +### Consequences + +- `map_tiles.cpp` comment at lines 150–155 (`onRefreshTimer` note about "paint() still only hides tiles") will need updating after the eviction fix lands — it will no longer be true that paint() never deletes tiles +- Test suite: add `test_map_tiles_eviction.cpp` (or extend `test_map_tiles_refresh.cpp`) to `ui_ws/src/camp/test/` + +### Actions +- [ ] Add LRU eviction cap to `MapTiles::paint()` (basemap path) so `tiles_` cannot grow unbounded; N cap should be configurable or at least documented in a comment +- [ ] Ensure eviction never removes currently-visible tiles (guard in eviction logic) +- [ ] Add test asserting `tiles_` size stays bounded across a simulated multi-area pan (follow `test_map_tiles_refresh.cpp` pattern) +- [ ] Update `onRefreshTimer` comment in `map_tiles.cpp:150–155` to reflect that eviction now also bounds within-cycle accumulation +- [ ] Document the eviction bound choice (N × visible tile count, or similar) in a comment near the eviction logic so future maintainers can trace the rationale + +## Plan Authored +**Status**: complete +**When**: 2026-06-22 14:30 +00:00 +**By**: Claude Code Agent (Claude Sonnet) + +**Plan**: `.agent/work-plans/issue-98/plan.md` at `618c2f6` +**Branch**: feature/issue-98 at `618c2f6` +**Phases**: single + +### Open questions +- [ ] Confirm `kTileEvictionMinCap` and `kTileEvictionMultiplier` constants are appropriate for deployment hardware (salmon) memory constraints — lower cap may be needed if RSS budget is tight. +- [ ] Confirm that `QGraphicsScene::render()` with varying source rect reliably drives distinct `painter->worldTransform()` values inside `MapTiles::paint()` in offscreen CI; fall back to protected slot if not. + +## Plan Review +**Status**: complete +**When**: 2026-06-22 05:10 +00:00 +**By**: Claude Code Agent (Claude Opus) + +**Plan**: `.agent/work-plans/issue-98/plan.md` at `618c2f6` +**PR**: PR-less (`--issue` dispatch) +**Verdict**: approve-with-suggestions + +### Findings +- [ ] (must-fix) Step 2 deletes `Tile` (a `QGraphicsObject` child of `MapTiles`) inside `paint()` — removing scene items mid-paint risks use-after-free per Qt; defer eviction to the event loop and promote the planned `evictIfNeeded` slot from test-fallback to the primary deletion path (`QMetaObject::invokeMethod(..., Qt::QueuedConnection)` / `QTimer::singleShot(0,...)`) — `plan.md:33` +- [ ] (suggestion) Step 5 test reuses single-zoom-level `makeLayout`, but `setLayout()` pre-seeds the whole `zoom_levels.front()` grid at construction, so `paint()` mints no new tiles — the test won't reproduce the field mechanism (deeper-zoom tiles minted by `paint()`); use a multi-zoom-level layout and `processEvents()` after `render()` if eviction is deferred — `plan.md:51` +- [ ] (suggestion) Confirm `tile_last_visible_gen_` keys identically to `tiles_` — `TileAddress::operator<` ignores `layout_epoch_` (`map_tiles.cpp:183`), which is the desired behavior; add a one-line comment so the shared comparator semantics aren't silently broken — `plan.md:30` +- [ ] (noted, no action) Evicting a tile with an in-flight pixmap is already safe — `tileLoaded()`'s `tiles_.find` guard drops late pixmaps for evicted addresses (`map_tiles.cpp:182`) + +### Summary +Diagnosis is correct; LRU-eviction approach is sound, minimal, and well-scoped with consequences and tests accounted for. The one structural concern — synchronous `Tile` deletion inside `paint()` — should be resolved by deferring eviction (the plan's own `evictIfNeeded` slot supports this) before implementation. With that addressed, the plan is ready. + +### Recommended Actions +- [ ] Defer eviction out of `paint()`; make `evictIfNeeded` the primary deletion path. +- [ ] Use a multi-zoom-level test layout; `processEvents()` after `render()` when eviction is deferred. +- [ ] Comment the shared `TileAddress` comparator semantics on `tile_last_visible_gen_`. + +## Implementation +**Status**: complete +**When**: 2026-06-22 05:30 +00:00 +**By**: Claude Opus + +**Branch**: feature/issue-98 +**Commits**: `0b99bc8` (eviction logic), `9ba8ded` (test + CMake + comment/plan sync) +**Build/test**: clean build; `./ui_ws/test.sh camp` → 105 tests, 0 failures, 2 skipped (the new test included) + +### What was built +Deferred LRU eviction that bounds `MapTiles::tiles_`, fixing the unbounded pan/zoom +growth behind the #98 OOM (the OSM/WMTS basemap never refreshes, so `setLayout()` — the +only prior deletion path — was never reached after construction). + +- **Cap** = `max(kTileEvictionMinCap, kTileEvictionMultiplier * visible_count)` = + `max(256, 4 * visible)`. Constants are file-private in `map_tiles.cpp` with the + ~256 KB/tile → ~64 MB floor rationale documented at the definition site (operator-confirmed). +- **Deferred, never synchronous (Plan Review must-fix).** `paint()` does NOT delete any + `Tile`. After the existing visibility loop it records `tile_last_visible_gen_[addr] = + paint_generation_` for each visible tile, bumps `paint_generation_`, and — if + `tiles_.size() > cap` and no eviction is already pending — sets `eviction_pending_ = + true` and queues `evictIfNeeded()` via `QMetaObject::invokeMethod(..., Qt::QueuedConnection)`. + The `eviction_pending_` flag debounces so at most one eviction is queued at a time. +- **`evictIfNeeded()` private slot** (the primary deletion path, runs in the event loop + where deleting scene children is safe): clears `eviction_pending_`; recomputes the cap + from the live visible count; collects candidates guarded on `tile->isVisible()` (a tile + visible *now* is never evicted, even if it was off-screen when scheduled); sorts ascending + by `tile_last_visible_gen_` (missing → 0 = oldest); deletes oldest-first + (`delete` + `tiles_.erase` + erase the gen entry) until `tiles_.size() <= cap`. Since + `cap >= visible_count`, there are always enough non-visible candidates to reach the cap. +- **`setLayout()`** clears `tile_last_visible_gen_` and resets `eviction_pending_`; + `paint_generation_` stays monotonic (not reset). +- `tile_last_visible_gen_` is keyed by `TileAddress` so it shares `tiles_`' comparator + (`operator<` ignores the refresh epoch); a header comment records this so the shared + semantics aren't silently broken. + +### Test (`test_map_tiles_eviction.cpp`) +Multi-zoom layout (1×1 front + 20×20 deep) — required because `setLayout()` pre-seeds the +whole front grid at construction, so a single-zoom layout would mint nothing in `paint()` +and not reproduce the bug. `paint()` is driven directly with a hand-built `QPainter` world +transform (`QTransform(8,0,0,-8,tx,ty)`: uniform scale 8 with a web-mercator y-flip), chosen +so all index arithmetic lands on exact integers — deterministic under offscreen QPA, so the +plan's `QGraphicsScene::render()` fallback was unnecessary. A one-tile viewport is panned +across every position of the 20×20 grid with `QApplication::processEvents()` after each +paint (eviction is deferred); the test asserts `tileChildCount()` stays ≤ the 256 cap +throughout and after the sweep. A second test (`DeepLevelTilesAreMintedByPaint`) confirms +the deep tiles really are minted by `paint()`. Registered in `CMakeLists.txt` mirroring the +`test_map_tiles_refresh` block; the latter's SCOPE comment now points here. + +### Non-vacuity check (without-fix-fails) +Verified empirically: with `evictIfNeeded()` stubbed to an early `return` (eviction +disabled) and camp rebuilt, the pan drives `tiles_` to **401** (1 front + 400 deep) and the +test fails (`actual: 401 vs 256`). The stub was reverted, camp rebuilt, and both eviction +tests pass. So the test is a genuine regression gate, not a tautology. + +### Consequence updates +- `onRefreshTimer` comment in `map_tiles.cpp` updated: it no longer claims paint() "only + hides (not deletes) tiles"; it now notes the [#98] eviction bounds within-cycle accumulation. +- `test_map_tiles_refresh.cpp` SCOPE comment updated to point at the new eviction test. +- `plan.md` synced to the as-built deferred design, chosen constants, multi-zoom test, and + resolved Open Questions. + +### Notes for the host +- A fresh container had empty `core_ws`/underlay installs. I built camp's deps first + (`colcon build --packages-up-to marine_ais_msgs marine_interfaces marine_autonomy` in + `core_ws`, ROS Jazzy supplying the message deps) before `build.sh camp` succeeded. +- Not pushed, per the handoff contract. + +## Local Review (Pre-Push) +**Status**: complete +**When**: 2026-06-22 05:43 +00:00 +**By**: Claude Code Agent (Claude Opus) +**Verdict**: approved + +**Branch**: feature/issue-98 at `75f0035` +**Mode**: pre-push +**Depth**: Deep (reason: 661 changed lines ≥ 200, plus Qt object-lifecycle / deferred-deletion dimension) +**Must-fix**: 0 | **Suggestions**: 1 +**Round**: 1 | **Ship**: recommended — no Must-fix; only an optional eviction-determinism suggestion, diff is shippable + +### Findings +- [ ] (suggestion) `evictIfNeeded()` sort has no tie-breaker for same-generation tiles — harmless to the memory bound & visible-tile safety; affects only eviction determinism — `src/camp2/map_tiles/map_tiles.cpp:164` + +### Notes +- Static analysis (ament_cpplint + cppcheck): no PR-introduced defects. cpplint output was all whole-file ament-style conventions this Qt codebase deliberately doesn't adopt (copyright headers, header-guard style, namespace indentation, C-style `int(...)` casts, for-colon spacing); new code matches surrounding idiom. cppcheck `slots`-macro errors are Qt parse noise, not bugs. +- Both Claude Adversarial passes concur the deferred-eviction logic and Qt lifecycle are sound: in-flight pixmap for an evicted tile is dropped by `tileLoaded`'s `tiles_.find` guard; a queued `evictIfNeeded()` surviving a `setLayout()` is benign (rebuilds candidates from current state); no repaint busy-loop; visible tiles never evicted (`cap >= visible_count`). +- Plan adherence: matches `plan.md` step-for-step (deferred slot as primary deletion path per Plan Review must-fix; multi-zoom test; consequence comment updates). No scope creep. +- Build/test not re-run this round; relied on the Implementation entry's documented clean build + 105 tests (0 failures) and the empirically-verified non-vacuity check (401 without fix → test fails). diff --git a/CMakeLists.txt b/CMakeLists.txt index a1c2334..3c140ce 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -522,6 +522,23 @@ if(BUILD_TESTING) Qt5::Test ) + # MapTiles bounded-memory LRU eviction (#98). The within-cycle regression gate: + # panning a small viewport across a large deeper zoom level mints tiles via + # paint(); deferred evictIfNeeded() must keep tiles_ bounded by the cap rather + # than growing until OOM. Links the shared camp_map library where MapTiles/Tile + # live. + ament_add_gtest(test_map_tiles_eviction + test/test_map_tiles_eviction.cpp + ) + target_include_directories(test_map_tiles_eviction PRIVATE + ${CMAKE_CURRENT_SOURCE_DIR}/src/camp2 + ) + target_link_libraries(test_map_tiles_eviction + camp_map + Qt5::Widgets + Qt5::Test + ) + # GGGS GPU-warp tile layer (camp#90 / I4): GggsTile extent-from-geotransform + # NoData/range handling, and the shader-warp == web_mercator::geoToMap parity # anchor. Links camp_map (GggsTile + web_mercator) and GDAL (synthetic tiles). diff --git a/src/camp2/map_tiles/map_tiles.cpp b/src/camp2/map_tiles/map_tiles.cpp index 6b60a58..d6f9ce2 100644 --- a/src/camp2/map_tiles/map_tiles.cpp +++ b/src/camp2/map_tiles/map_tiles.cpp @@ -8,7 +8,10 @@ #include #include #include +#include #include +#include +#include #include "wmts/capabilities.h" namespace camp @@ -17,6 +20,23 @@ namespace camp namespace map_tiles { +namespace +{ + +// [#98] Eviction cap for MapTiles::tiles_, computed as +// max(kTileEvictionMinCap, kTileEvictionMultiplier * visible_tile_count). +// +// OSM tiles decode to ~256x256 ARGB (~256 KB each), so the 256-tile floor caps +// the basemap tile buffer at roughly 64 MB — a generous pan buffer sized for the +// salmon operator workstation, large enough that normal pan/zoom never blanks a +// tile that's about to be revisited, yet bounded so an all-afternoon survey can't +// OOM-kill CAMP (issue #98). The multiplier keeps the cap comfortably above the +// working set at high zoom, where the viewport can show many small tiles at once. +constexpr int kTileEvictionMinCap = 256; +constexpr int kTileEvictionMultiplier = 4; + +} // namespace + MapTiles::MapTiles(map::MapItem* parentItem, const QString& label, const TileLayout& tile_layout): map::Layer(parentItem, label), tile_layout_(tile_layout) { @@ -86,6 +106,80 @@ void MapTiles::paint(QPainter *painter, const QStyleOptionGraphicsItem *option, for(auto tile: tiles_) if(visible_tiles.find(tile.first) == visible_tiles.end()) tile.second->setVisible(false); + + // [#98] Record the paint generation in which each visible tile was seen, then + // advance the generation. This is the LRU bookkeeping consumed by + // evictIfNeeded(): tiles not visited recently have the smallest generation and + // are evicted first. + for(const auto& address: visible_tiles) + tile_last_visible_gen_[address] = paint_generation_; + ++paint_generation_; + + // [#98] If tiles_ has outgrown the cap, schedule a DEFERRED eviction. Eviction + // must not happen here: deleting a Tile (a QGraphicsObject scene child) inside + // paint() removes a scene item mid-paint, a use-after-free risk. Instead queue + // evictIfNeeded() onto the event loop, where deleting scene items is safe. The + // eviction_pending_ flag debounces this so we don't post a fresh invocation on + // every paint while one is already queued. + const int cap = std::max(kTileEvictionMinCap, + kTileEvictionMultiplier * int(visible_tiles.size())); + if(int(tiles_.size()) > cap && !eviction_pending_) + { + eviction_pending_ = true; + QMetaObject::invokeMethod(this, "evictIfNeeded", Qt::QueuedConnection); + } +} + +void MapTiles::evictIfNeeded() +{ + eviction_pending_ = false; + + // Recompute the cap from the CURRENT visible set and collect eviction + // candidates: tiles that are not visible right now. A tile that became visible + // again between scheduling and now must never be evicted, so we guard on the + // live isVisible() rather than the visible set captured at schedule time. + int visible_count = 0; + std::vector> candidates; + candidates.reserve(tiles_.size()); + for(const auto& entry: tiles_) + { + if(entry.second && entry.second->isVisible()) + { + ++visible_count; + continue; + } + auto gen_it = tile_last_visible_gen_.find(entry.first); + quint64 gen = (gen_it != tile_last_visible_gen_.end()) ? gen_it->second : 0; + candidates.emplace_back(gen, entry.first); + } + + const int cap = std::max(kTileEvictionMinCap, + kTileEvictionMultiplier * visible_count); + if(int(tiles_.size()) <= cap) + return; + + // Oldest last-visible generation first (a missing entry sorts as 0 == oldest). + // cap >= visible_count, so there are always enough non-visible candidates to + // bring tiles_ down to the cap without touching a visible tile. + std::sort(candidates.begin(), candidates.end(), + [](const std::pair& a, + const std::pair& b) + { return a.first < b.first; }); + + size_t to_evict = tiles_.size() - cap; + for(const auto& candidate: candidates) + { + if(to_evict == 0) + break; + auto tile_it = tiles_.find(candidate.second); + if(tile_it != tiles_.end()) + { + delete tile_it->second; + tiles_.erase(tile_it); + } + tile_last_visible_gen_.erase(candidate.second); + --to_evict; + } } void MapTiles::setLayout(const TileLayout& tile_layout) @@ -94,6 +188,13 @@ void MapTiles::setLayout(const TileLayout& tile_layout) if(tile.second) delete tile.second; tiles_.clear(); + // [#98] Drop the LRU eviction bookkeeping for the now-deleted tile set, and + // cancel any queued eviction (its candidate addresses are gone). Note + // paint_generation_ is deliberately NOT reset — it stays monotonic; entries for + // the freshly seeded tiles simply start accumulating again from the current + // generation on the next paint. + tile_last_visible_gen_.clear(); + eviction_pending_ = false; // [#99] Bump the layout generation so any in-flight pixmap requested under the // previous layout (same tile_layout_ pointer across a refresh) is rejected by // tileLoaded once it lands on the rebuilt tile set. @@ -149,10 +250,12 @@ void MapTiles::onRefreshTimer() // 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 bounded AT each refresh boundary. NOTE: this does - // not change within-cycle accumulation — paint() still only hides (not - // deletes) tiles as the viewport pans/zooms between refreshes, exactly as - // before. The refresh resets periodically; it is not an eviction policy. + // 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. // // FRESHNESS (#99): re-fetching each cycle yields a genuinely *fresh* radar frame // because the configured IEM "nexrad-n0q" tile product always serves the latest diff --git a/src/camp2/map_tiles/map_tiles.h b/src/camp2/map_tiles/map_tiles.h index 332ee51..60320a6 100644 --- a/src/camp2/map_tiles/map_tiles.h +++ b/src/camp2/map_tiles/map_tiles.h @@ -59,6 +59,22 @@ public slots: TileLayout tile_layout_; std::map tiles_; + // [#98] LRU eviction bookkeeping that bounds tiles_ against the unbounded + // pan/zoom growth behind the #98 OOM. The OSM/WMTS basemap never refreshes + // (only the radar overlay calls setRefreshInterval -> setLayout), so without a + // cap tiles_ grows for every newly-visited tile area until the process is + // OOM-killed. paint_generation_ is a monotonically increasing paint-call + // counter; tile_last_visible_gen_ records, per tile, the most recent paint + // generation in which it was visible — the LRU key used to pick eviction + // victims (oldest first). tile_last_visible_gen_ is keyed by TileAddress and + // therefore shares tiles_' ordering: TileAddress::operator< ignores the + // refresh epoch (tile_address.cpp), which is exactly what we want so the two + // maps stay in lock-step. eviction_pending_ debounces the deferred eviction so + // paint() schedules at most one evictIfNeeded() at a time. + quint64 paint_generation_ = 0; + std::map tile_last_visible_gen_; + bool eviction_pending_ = false; + CachedTileLoader* tile_loader_; const wmts::Capabilities* wmts_capabilites_ = nullptr; @@ -78,6 +94,15 @@ public slots: private slots: void tileLoaded(QPixmap pixmap, TileAddress tile); + // [#98] Deferred LRU eviction of off-screen tiles. paint() only SCHEDULES this + // (via a queued invocation) — it never deletes Tile* itself, because deleting a + // scene child mid-paint is a use-after-free risk. Running in the event loop, this + // slot recomputes the cap from the CURRENT visible count, then deletes the + // least-recently-visible non-visible tiles until tiles_.size() <= cap. A tile + // visible at eviction time is never evicted, even if it was off-screen when the + // eviction was scheduled. + void evictIfNeeded(); + // [#99 Phase 2] Refresh slot — invoked by refresh_timer_ on timeout, and // directly (via QMetaObject::invokeMethod) by test_map_tiles_refresh for a // deterministic single-fire check. Invalidates the disk cache then resets the diff --git a/test/test_map_tiles_eviction.cpp b/test/test_map_tiles_eviction.cpp new file mode 100644 index 0000000..f369679 --- /dev/null +++ b/test/test_map_tiles_eviction.cpp @@ -0,0 +1,195 @@ +// [#98] Coverage for MapTiles' bounded-memory LRU eviction — the fix for the +// CAMP-silently-crashes-on-map-zoom/pan OOM. +// +// Root cause: MapTiles::paint() mints a new Tile* for every newly-visible tile +// address and only hides (setVisible(false)) off-screen ones. The sole deletion +// path was setLayout(), reached only by the radar overlay's ~5-min refresh; the +// OSM/WMTS basemap never refreshes, so tiles_ grew for every tile area visited +// during a pan/zoom session until the process was OOM-killed. The fix caps tiles_ +// at max(256, 4 * visible) and evicts the least-recently-visible off-screen tiles +// (deferred onto the event loop, since deleting a scene child inside paint() is a +// use-after-free risk). +// +// Why this test reproduces the field mechanism where test_map_tiles_refresh.cpp +// could not: setLayout() pre-seeds the ENTIRE zoom_levels.front() grid at +// construction, so paint() mints nothing if the layout has a single zoom level. +// We therefore build a layout with a tiny front level and a large DEEPER level; +// paint() renders the deeper level and mints its tiles on demand as we pan, which +// is exactly the basemap accumulation path. We drive paint() directly with a +// hand-built QPainter world transform (deterministic, no QPA rendering) and pan a +// small viewport across every position of the deeper grid. Because eviction is +// deferred, we processEvents() after each paint so the queued evictIfNeeded() +// runs, then assert tiles_ never exceeds the cap. +// +// Non-vacuity: the deeper grid is 20x20 = 400 tiles plus the 1 seeded front tile +// = up to 401 minted. The cap with a small viewport is the 256 floor. With the +// eviction logic removed, tiles_ reaches 401 and the final assertion (<= 256) +// fails — confirmed empirically by building once with evictIfNeeded()'s body +// stubbed out. So this is a genuine regression gate, not a tautology. +// +// Access mechanism (private tiles_): count Tile children via the public +// QGraphicsItem::childItems() + qgraphicsitem_cast, mirroring +// test_map_tiles_refresh.cpp. + +#include + +#include +#include +#include +#include +#include + +#include "map/map.h" +#include "map/layer_list.h" +#include "map_tiles/map_tiles.h" +#include "map_tiles/tile.h" +#include "map_tiles/tile_layout.h" + +using camp::map::Map; +using camp::map_tiles::MapTiles; +using camp::map_tiles::Tile; +using camp::map_tiles::TileLayout; + +namespace +{ + +// The eviction floor (kTileEvictionMinCap in map_tiles.cpp). Kept in sync by hand +// — the constant is file-private. A small panning viewport keeps the visible +// count tiny, so 4 * visible never beats this floor and the cap stays here. +constexpr int kEvictionFloor = 256; + +// Geometry constants shared by the layout and the pan transform. See the +// transform derivation in panTo() below; chosen so all index arithmetic in +// MapTiles::paint() lands on exact integers (no floating-point boundary flutter). +constexpr double kFrontScale = 1.0; // front level: coarse, never rendered here +constexpr double kDeepScale = 0.0625; // deep level: 1/16, exactly representable +constexpr int kTilePixels = 256; // tile_width == tile_height +constexpr double kDeepTileWorld = kTilePixels * kDeepScale; // = 16.0, exact +constexpr int kViewScale = 8; // world transform uniform scale (m11/|m22|) +constexpr int kDeepGrid = 20; // deep level is 20x20 = 400 tiles + +// A two-zoom-level layout: a 1x1 front level (all that setLayout() seeds) and a +// gridxgrid deeper level that paint() mints on demand. Field-realistic shape: the +// basemap that never refreshes is exactly such a multi-level OSM/WMTS pyramid. +TileLayout makeMultiZoomLayout(int grid) +{ + TileLayout layout; + + TileLayout::ZoomLevel front; + front.id = "0"; + front.scale = kFrontScale; + front.top_left_corner = QPointF(0, 0); + front.tile_width = kTilePixels; + front.tile_height = kTilePixels; + front.matrix_width = 1; + front.matrix_height = 1; + layout.zoom_levels.push_back(front); + + TileLayout::ZoomLevel deep; + deep.id = "1"; + deep.scale = kDeepScale; + deep.top_left_corner = QPointF(0, 0); + deep.tile_width = kTilePixels; + deep.tile_height = kTilePixels; + deep.matrix_width = grid; + deep.matrix_height = grid; + layout.zoom_levels.push_back(deep); + + layout.url_static_parts = {"file:///nonexistent/"}; + layout.url_variable_keys = {}; + return layout; +} + +int tileChildCount(const MapTiles* layer) +{ + int n = 0; + for(QGraphicsItem* child : layer->childItems()) + if(qgraphicsitem_cast(child) != nullptr) + ++n; + return n; +} + +// Drive one paint() of the deep level with the viewport positioned over deep-tile +// (col, row). The world transform is a uniform scale kViewScale with a y-flip +// (m22 negative, matching web-mercator north-up), translated so the painter +// window's world rect is exactly the one deep tile (col, row): +// +// m11 = S, m22 = -S, m31 = -S*col*T, m32 = -S*row*T (S = kViewScale, T = 16) +// +// Then in MapTiles::paint(): lod = S/2 = 4, so the front level (scale*lod = 4 >= +// 2) is skipped and the deep level (scale*lod = 0.25 < 2) is rendered; the window +// maps to world [col*T,(col+1)*T] x (north-up) row [row..row+1], so paint() mints +// the up-to-2x2 deep tiles around (col, row). A WxH = (T*S)x(T*S) = 128x128 image +// gives window().width()/height() == T*S, the one-tile world span. +void panTo(MapTiles* layer, int col, int row) +{ + const int wh = static_cast(kDeepTileWorld) * kViewScale; // 16 * 8 = 128 + QImage image(wh, wh, QImage::Format_ARGB32); + QPainter painter(&image); + const double tx = -kViewScale * col * kDeepTileWorld; + const double ty = -kViewScale * row * kDeepTileWorld; + painter.setWorldTransform(QTransform(kViewScale, 0, 0, -kViewScale, tx, ty)); + QStyleOptionGraphicsItem option; + layer->paint(&painter, &option, nullptr); +} + +} // namespace + +// Sanity: the deeper level really is minted by paint() (not pre-seeded). After a +// single paint at the origin, more than the 1 seeded front tile must exist. +TEST(MapTilesEviction, DeepLevelTilesAreMintedByPaint) +{ + Map map; + auto* layer = + new MapTiles(map.topLevelLayers(), "test_evict_mint", makeMultiZoomLayout(kDeepGrid)); + + ASSERT_EQ(tileChildCount(layer), 1) << "ctor should seed only the 1x1 front level"; + + panTo(layer, 0, 0); + QApplication::processEvents(); + + EXPECT_GT(tileChildCount(layer), 1) + << "paint() must mint deep-level tiles on demand — otherwise the pan below " + "would never reproduce the #98 accumulation path"; +} + +// The load-bearing regression test: pan a small viewport across the entire 20x20 +// deeper grid (>> the 256 cap of distinct tiles). With deferred eviction running +// after each paint, tiles_ must stay at or below the cap. Without the fix it would +// climb to ~401 and this assertion would fail. +TEST(MapTilesEviction, PanAcrossLargeGridStaysBounded) +{ + Map map; + auto* layer = + new MapTiles(map.topLevelLayers(), "test_evict_bounded", makeMultiZoomLayout(kDeepGrid)); + + // Step the viewport so its 2x2 footprint sweeps cols/rows 0..kDeepGrid-1, + // visiting every one of the 400 deep tiles. processEvents() after each paint + // lets the queued evictIfNeeded() run (eviction is deferred off paint()). + for(int row = 0; row < kDeepGrid - 1; ++row) + for(int col = 0; col < kDeepGrid - 1; ++col) + { + panTo(layer, col, row); + QApplication::processEvents(); + EXPECT_LE(tileChildCount(layer), kEvictionFloor) + << "tiles_ exceeded the eviction cap mid-pan at (" << col << "," << row + << ") — within-cycle accumulation is unbounded (the #98 OOM)"; + } + + // Flush any final queued eviction and assert the steady-state bound. A small + // viewport keeps visible small, so the cap is the 256 floor. + QApplication::processEvents(); + EXPECT_LE(tileChildCount(layer), kEvictionFloor) + << "after panning the whole grid, tiles_ must be bounded by the eviction cap"; +} + +int main(int argc, char** argv) +{ + // MapTiles is a QGraphicsObject; Map builds a QGraphicsScene and MapItem ctors + // touch qApp, so a QApplication is required. Force offscreen for headless CI, + // mirroring test/test_map_tiles_refresh.cpp. + qputenv("QT_QPA_PLATFORM", "offscreen"); + QApplication app(argc, argv); + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} diff --git a/test/test_map_tiles_refresh.cpp b/test/test_map_tiles_refresh.cpp index 190423d..957f2a3 100644 --- a/test/test_map_tiles_refresh.cpp +++ b/test/test_map_tiles_refresh.cpp @@ -148,12 +148,12 @@ TEST(MapTilesRefresh, RefreshRepopulatesTiles) // the per-layout count, never N*count. // // SCOPE (honest): this guards ONLY the refresh-boundary reset — that a refresh -// does not leave the previous cycle's tiles behind. It does NOT cover the -// within-cycle growth that the #98 OOM is about: between refreshes, paint() still -// only hides (never deletes) tiles as the viewport pans/zooms, so tiles_ can still -// grow within a single cycle exactly as in the existing layers. This test is the -// #98-coordination artifact (the refresh path doesn't make the boundary leak), -// not a fix for the within-cycle accumulation. +// does not leave the previous cycle's tiles behind. The within-cycle growth that +// the #98 OOM is actually about — between refreshes, paint() minting new tiles as +// the viewport pans/zooms — is now bounded by the [#98] LRU eviction and is +// covered by test_map_tiles_eviction.cpp (which pans across a deeper zoom level +// and asserts tiles_ stays under the cap). This test remains the refresh-boundary +// regression gate; the two together close the #98 tile-lifecycle risk. TEST(MapTilesRefresh, RefreshBoundaryResetsTileSet) { Map map;