diff --git a/.agent/work-plans/issue-96/plan.md b/.agent/work-plans/issue-96/plan.md new file mode 100644 index 0000000..0188497 --- /dev/null +++ b/.agent/work-plans/issue-96/plan.md @@ -0,0 +1,102 @@ +# Plan: GDAL dataset leak in `loadAndReprojectFile` + +## Issue + +https://github.com/rolker/camp/issues/96 + +## Context + +`loadAndReprojectFile` in `src/camp2/raster/raster_layer.cpp` (lines 125–265) +opens two GDAL dataset handles and never closes them: + +- `dataset` via `GDALOpen` (line 129) +- `reprojected_dataset` via `GDALAutoCreateWarpedVRT` (line 137) + +The sibling `initExtent()` (lines 77–101) already uses the correct pattern — +`GDALClose(reprojected)` (line 98) then `GDALClose(dataset)` (line 100). Every +return path in `loadAndReprojectFile` leaks one or both handles: + +- Normal return (line 264): leaks both. +- `if(!reprojected_dataset) return result` (line 140): leaks `dataset`. +- Abort-flag `return {}` paths inside the scan loops: leak both. + +Callers that trigger the leak on every invocation: `RasterLayer` constructor +(via `loadFile`), `setColormap()`, `readSettings()`. A colormap ramp flip leaks +two handles per flip. + +## Approach + +1. **Wrap both handles in RAII `unique_ptr`s** in `loadAndReprojectFile`. + Use a lambda deleter `[](GDALDataset* d){ if(d) GDALClose(d); }` so all + return paths (normal, early, abort) are covered automatically — no manual + `GDALClose` call needed. **(Plan Review #3 — close order)** Keep `dataset` + declared *before* `reprojected_dataset` (as the current code does): local + `unique_ptr`s destruct in reverse declaration order, so the warped VRT closes + first, then the source — matching `initExtent()`'s deliberate + `GDALClose(reprojected)` then `GDALClose(dataset)` (the VRT references the + source). A comment on the declarations records why the order matters so a + future refactor doesn't reorder them. + +2. **Add a regression test** `test/test_raster_layer_gdal_cleanup.cpp`: + Create a minimal WGS84 single-band Float32 GeoTIFF in a `QTemporaryDir`, + construct `RasterLayer` with it, trigger `setColormap()` with two additional + ramp types (each call re-invokes `loadAndReprojectFile`), destroy the layer + (dtor calls `future_watcher_.waitForFinished()`), then assert the + process-wide open-dataset count is unchanged. + Follow the `test_gggs_render.cpp` pattern: `QApplication` + `camp::map::Map` + + `map.topLevelLayers()` as the `MapItem*` parent. + **(Plan Review #2 — baseline-delta)** `GDALDataset::GetOpenDatasets()` counts + *all* process-wide handles, so capture the count *before* constructing the + layer and assert it returns to that baseline after destruction (delta == 0), + rather than assuming zero. + **(Plan Review #1 — no vacuous pass)** Assert each load actually reached the + warp path and succeeded — `RasterLayer::status() == ""` (the `imageReady()` + slot only clears the status after the warp produced non-empty mipmaps; a + null/unwarpable load leaves `"(load failed)"`). A load that never warps never + opens the handles, so without this guard a silently-failing load could pass + the leak check vacuously. + +3. **Wire the test in `CMakeLists.txt`** — `ament_add_gtest` linking `camp_map`, + `Qt5::Core`, `Qt5::Widgets`, `Qt5::Concurrent`, and `${GDAL_LIBRARY}`. + +## Files to Change + +| File | Change | +|------|--------| +| `src/camp2/raster/raster_layer.cpp` | Add `#include `; replace bare `auto dataset` / `auto reprojected_dataset` pointers with `unique_ptr` + lambda-`GDALClose` deleter (declaration order preserved + commented per #3); change `GDALAutoCreateWarpedVRT(dataset, ...)` to `GDALAutoCreateWarpedVRT(dataset.get(), ...)` | +| `test/test_raster_layer_gdal_cleanup.cpp` | New: regression test — synthetic WGS84 GeoTIFF, `setColormap` stress, baseline-delta `GetOpenDatasets` assertion (#2) + `status()`-based load-success guard (#1) | +| `CMakeLists.txt` | Add `ament_add_gtest(test_raster_layer_gdal_cleanup ...)` after the existing GDAL-linked tests | + +## Principles Self-Check + +| Principle | Consideration | +|---|---| +| Fix it completely | RAII covers all early-return and abort-flag paths. The `if(!reprojected_dataset)` early return that previously leaked `dataset` is automatically covered. No partial fix. | +| Include the test | Regression guard via `GDALDataset::GetOpenDatasets()` — makes the leak observable if RAII is ever removed or regressed. | +| No scope creep | Only `loadAndReprojectFile` body changes. No interface, no header, no callers, no other functions. | + +## ADR Compliance + +| ADR | Triggered | How addressed | +|---|---|---| +| ADR-0002 — Worktree isolation | Yes | Working in worktree `issue-camp-96` on branch `feature/issue-96`. | +| ADR-0008 — ROS 2 conventions | Yes | `unique_ptr` with custom deleter is idiomatic modern C++; matches ROS 2 C++ style guide. No C-style resource management needed. | +| ADR-0013 — progress.md vocabulary | Yes | This plan is recorded under `## Plan Authored`. | + +## Consequences + +| If we change... | Also update... | Included in plan? | +|---|---|---| +| `dataset` → `unique_ptr` | `GDALAutoCreateWarpedVRT(dataset, ...)` → `(...dataset.get(), ...)` | Yes — same edit pass in `loadAndReprojectFile` | +| `reprojected_dataset` → `unique_ptr` | All `reprojected_dataset->...` accesses unchanged (`operator->` on `unique_ptr` works identically); `for(auto&& band: reprojected_dataset->GetBands())` unchanged | Yes — `operator->` is transparent | +| New test file | `CMakeLists.txt` must register it and link `camp_map` + GDAL | Yes — step 3 | + +## Open Questions + +- [x] No open questions. The three Plan Review suggestions (close order, + baseline-delta, no-vacuous-pass) were folded into the approach above and + implemented as-built. + +## Estimated Scope + +Single PR. diff --git a/.agent/work-plans/issue-96/progress.md b/.agent/work-plans/issue-96/progress.md new file mode 100644 index 0000000..3e42698 --- /dev/null +++ b/.agent/work-plans/issue-96/progress.md @@ -0,0 +1,193 @@ +--- +issue: 96 +--- + +# Issue #96 — GDAL dataset leak in `loadAndReprojectFile` + +## Issue Review +**Status**: complete +**When**: 2026-06-22 00:00 +00:00 +**By**: Claude Code Agent (Claude Sonnet) + +**Issue**: #96 +**Comment**: (best-effort post follows this entry; not recorded inline) +**Scope verdict**: well-scoped + +### Scope Assessment + +The issue identifies a specific resource leak in `loadAndReprojectFile` in +`src/camp2/raster/raster_layer.cpp`: two GDAL dataset handles (`GDALOpen` result +and `GDALAutoCreateWarpedVRT` result) are opened but never closed, on every return +path including early-exit paths. The sibling `initExtent()` shows the correct +pattern and already uses `GDALClose`. The suggested fix — RAII `unique_ptr` with a +custom deleter — is idiomatic C++ and covers all return paths automatically. The +HOST NOTE confirms the leak is still present at HEAD (post-#102/#104 single-band +colormap work). Scope is minimal: fix the function, add a test. + +The issue correctly distinguishes this from the zoom/pan OOM (#98) — these are +separate phenomena with separate causes. + +### Principle Alignment + +| Principle | Status | Notes | +|---|---|---| +| A change includes its consequences | Watch | Issue explicitly calls for a test. Implementation must include one — the pattern of GDAL resource mis-management could silently recur without a regression guard. | +| Only what's needed | OK | Fix is narrowly scoped: RAII wrappers in one function. No interface changes, no `.msg`/`.srv` changes. | +| Improve incrementally | OK | Small, self-contained bug fix. Does not require a companion redesign. | +| Test what breaks | Watch | GDAL dataset lifecycle is hard to assert at the unit-test level (no mock). A functional/integration test (load a raster N times, verify memory does not grow unboundedly, or verify `GDALOpen` call count with a GDAL callback) may be more practical than a pure unit test. Implementation should choose a feasible test strategy and document it if a simpler option is unavailable. | +| Human control and transparency | OK | No behavioral change to the user-visible layer — same pixels, same colormap. Fix is transparent to users. | + +### ADR Applicability + +| ADR | Triggered | Notes | +|---|---|---| +| ADR-0002 — Worktree isolation | Yes | Worktree `issue-camp-96` (branch `feature/issue-96`) already exists — OK. | +| ADR-0008 — ROS 2 conventions | Yes | C++ change in a ROS 2 package. RAII is consistent with ROS 2 / modern C++ style. No convention concerns. | +| ADR-0013 — progress.md vocabulary | Yes | This entry uses `## Issue Review` — correct. | + +### Consequences + +- Only `raster_layer.cpp` changes; no package interface changes, no `.msg`/`.srv` files, no callers affected. +- The early-return path `if(!reprojected_dataset) return result;` also leaks `dataset` — the RAII approach covers this automatically; the implementation must not miss it. +- Callers (`RasterLayer` ctor, `setColormap`, `readSettings`) do not need changes — the fix is internal to `loadAndReprojectFile`. + +### Actions +- [ ] Implement RAII fix for both GDAL handles in `loadAndReprojectFile`, covering all return paths (including the `if(!reprojected_dataset)` early return that also leaks `dataset`). +- [ ] Add a regression test (automated or documented manual) for the resource leak. + +## Plan Authored +**Status**: complete +**When**: 2026-06-22 00:00 +00:00 +**By**: Claude Code Agent (Claude Sonnet) + +**Plan**: `.agent/work-plans/issue-96/plan.md` at `c327fe2` +**Branch**: feature/issue-96 at `c327fe2` +**Phases**: single + +### Open questions +- [ ] No open questions — plan is review-plan-ready. + +## Plan Review +**Status**: complete +**When**: 2026-06-22 02:45 +00:00 +**By**: Claude Code Agent (Claude Opus) + +**Plan**: `.agent/work-plans/issue-96/plan.md` at `c327fe2` +**PR**: PR-less (reviewed via worktree; `gh` unauthenticated in this environment) +**Verdict**: approve-with-suggestions + +Independence note: the `## Plan Authored` entry shares the workspace agent name +("Claude Code Agent") but was a separate Sonnet sub-agent invocation; this review +is a fresh-context Opus sub-agent, so it is genuinely independent — the +`(in-context — author self-review)` annotation is omitted as it would be inaccurate. + +### Findings +- [ ] (suggestion) Assert load success, not just zero open handles — if the synthetic raster fails to load/reproject, every early-return path leaks nothing, so a `GetOpenDatasets()==0` assertion passes vacuously even if `loadAndReprojectFile` never reached the warp path. Add an assertion that the load produced a non-empty image/mipmaps (e.g. via `imageReady`/`boundingRect`) so the test is a meaningful regression guard. — `plan.md:34` +- [ ] (suggestion) Prefer a baseline-delta over `EXPECT_EQ(n, 0)` — `GDALDataset::GetOpenDatasets()` counts *all* process-wide open handles, not just the layer's. Capture the count before constructing `RasterLayer` and assert equality after destruction, so the test won't flake if `Map`/GDAL internals hold an unrelated handle. — `plan.md:39` +- [ ] (suggestion) Preserve declaration order to keep close order = reprojected-then-source — `initExtent()` deliberately `GDALClose(reprojected)` before `GDALClose(dataset)` (the warped VRT references the source). Local `unique_ptr`s destruct in reverse declaration order, so keep `dataset` declared before `reprojected_dataset` (as the current code already does); a note in the implementation avoids an accidental reorder. — `plan.md:29` + +## Implementation +**Status**: complete +**When**: 2026-06-22 03:30 +00:00 +**By**: Claude Code Agent (Claude Opus) + +**Plan**: `.agent/work-plans/issue-96/plan.md` (synced as-built) +**Branch**: feature/issue-96 +**Commits**: `bb2dd9d` (fix), `8552267` (test + CMake + plan sync) + +### What was done + +**RAII fix** (`src/camp2/raster/raster_layer.cpp`, `bb2dd9d`) — wrapped both GDAL +handles in `loadAndReprojectFile` in `std::unique_ptr` +with a `[](GDALDataset* d){ if(d) GDALClose(d); }` deleter, so every return path +(normal, the `!reprojected_dataset` early return that previously leaked the source, +and the abort-flag `return {}` paths) frees both. `GDALAutoCreateWarpedVRT(dataset, …)` +→ `(dataset.get(), …)`; the `reprojected_dataset->…` accesses are unchanged +(`operator->` is transparent). Added `#include `. + +- **Close order (Plan Review #3):** `dataset` is declared *before* + `reprojected_dataset`, so reverse-declaration destruction closes the warped VRT + first, then the source — matching `initExtent()`'s deliberate + `GDALClose(reprojected)` then `GDALClose(dataset)` (the VRT references the source). + A multi-line comment on the declarations records the constraint so a refactor + won't reorder them. + +**Regression test** (`test/test_raster_layer_gdal_cleanup.cpp` + `CMakeLists.txt`, +`8552267`) — follows the `test_gggs_render.cpp` pattern (`QApplication`, +`camp::map::Map`, `map.topLevelLayers()` as the `MapItem*` parent; links `camp_map`, +`Qt5::Core/Gui/Widgets/Concurrent/Positioning`, `${GDAL_LIBRARY}`). Writes a +synthetic 8×8 WGS84 single-band Float32 GeoTIFF with real georeferencing (so the +warp path runs), constructs a `RasterLayer`, flips the colormap twice +(Grayscale → Turbo; default is Viridis — each flip re-invokes `loadAndReprojectFile`), +destroys the layer, and asserts the open-dataset count. + +- **Baseline-delta (Plan Review #2):** captures `GDALDataset::GetOpenDatasets(&n)` + *before* constructing the layer and asserts the count returns to that baseline + after destruction (delta == 0) — not `== 0` — since the count is process-wide. +- **No vacuous pass (Plan Review #1):** asserts each load actually reached the warp + path and succeeded. `RasterLayer` exposes no "load succeeded" accessor, so the + test uses the existing public `MapItem::status()`: `imageReady()` sets it to `""` + only after the warp produced non-empty mipmaps, and `"(load failed)"` otherwise. + A `waitForLoad()` helper spins the event loop until status leaves `"(loading...)"` + and returns true iff it became empty. Also asserts `valid()` and a non-empty + `boundingRect()`. No new accessor was added — `status()` already existed. + +### Build / test + +- Built the camp deps first (a fresh container had empty `core_ws/install`): + `colcon build --packages-up-to marine_ais_msgs marine_interfaces marine_autonomy` + in `core_ws` (the other deps — `marine_sensor_msgs`, `grid_map_ros`, `nav2_msgs` + — resolve from `/opt/ros/jazzy`). Then `./ui_ws/build.sh camp` — clean. +- `./ui_ws/test.sh camp`: **102 tests, 0 failures, 2 skipped** (the 2 skips are the + pre-existing offscreen-GL-gated `test_gggs_render` cases, unrelated). The new + `test_raster_layer_gdal_cleanup` passes. +- **Non-vacuous-pass verified:** temporarily neutered the deleter to a no-op, + rebuilt, and confirmed the test FAILS at the final `EXPECT_EQ(openDatasetCount(), + baseline)` (delta == 6 = three loads × two handles) while the load-success guards + still passed — proving the leaked handles register in `GetOpenDatasets()` and the + test is a real regression guard. Restored the deleter and re-confirmed green. + +### Deviations +None. All three folded Plan Review resolutions implemented as specified. Did not push. + +## Local Review (Pre-Push) +**Status**: complete +**When**: 2026-06-22 03:52 +00:00 +**By**: Claude Code Agent (Claude Opus) +**Verdict**: approved + +**Branch**: feature/issue-96 at `7016b91` +**Mode**: pre-push +**Depth**: Deep (reason: 429 total diff lines ≥ 200; GDAL resource lifecycle + async worker-thread load) +**Must-fix**: 0 | **Suggestions**: 2 +**Round**: 1 | **Ship**: recommended — no must-fix findings; clean, well-tested, plan-adherent fix + +### Findings +- [x] (suggestion) `LoadResult` double members lack default initializers — pre-existing, not touched by this diff, never read on the failure path (`imageReady` returns on empty mipmaps); optional `= 0.0` hardening — `src/camp2/raster/raster_layer.h:60` +- [x] (suggestion) Add a comment noting the deliberate raw `delete layer` (synchronous dtor join before the open-dataset count; `removeFromMap()` would defer and break the guarantee) — `test/test_raster_layer_gdal_cleanup.cpp:129` + +### Notes +- Static analysis clean (cppcheck: no findings on the test; `raster_layer.cpp` hit a Qt `slots`-macro parser limitation, not a code issue). cpplint not installed on this host. +- Two fresh-context Claude Adversarial passes (Lens A logic, Lens B systemic, Deep prompt) independently confirmed: reverse-declaration destruction closes the warped VRT before its source per `[stmt.dcl]/3`; all five return paths (normal + two early + two abort) now free both handles; test is non-vacuous (asserts each load reached the warp path via `status()`) and race-free (dtor abort+`waitForFinished()` joins the worker before the process-wide `GetOpenDatasets()` baseline-delta check). +- Plan adherence: full — exactly the three planned files changed; all three Plan Review resolutions present in code. No scope creep. +- Build/test not re-run here (offline container); implementation entry records `./ui_ws/test.sh camp` green (102 tests, 0 failures, 2 pre-existing GL-gated skips) with a verified non-vacuous-pass check (neutered deleter → delta == 6 failure). + +## Implementation +**Status**: complete +**When**: 2026-06-22 04:03 +00:00 +**By**: Claude Code Agent (Claude Opus) + +**Branch**: feature/issue-96 at `11452ab` +**Addressed**: `## Local Review (Pre-Push)` (When 2026-06-22 03:52 +00:00, at `7016b91`) +**Commits**: `8121162` (finding 1), `11452ab` (finding 2) + +### Actions +- [x] (suggestion) Default-init `LoadResult` double members — added `= 0.0` to `world_x`/`world_y`/`scale_x`/`scale_y` so a failure-path `LoadResult` carries deterministic values (matches the existing `is_scalar = false`) — `src/camp2/raster/raster_layer.h:60` (`8121162`) +- [x] (suggestion) Documented the deliberate raw `delete layer` — added a comment explaining that `removeFromMap()` defers via `deleteLater()` (verified at `src/camp2/map/layer.cpp:67`), which would run the open-dataset count before the dtor's join completes and flake; the direct `delete` keeps the join synchronous — `test/test_raster_layer_gdal_cleanup.cpp:129` (`11452ab`) + +### Build / test +- Built core deps (`colcon build --packages-up-to marine_ais_msgs marine_interfaces marine_autonomy` in `core_ws` — empty `install` on this container) then `./ui_ws/build.sh camp` — clean. +- `./ui_ws/test.sh camp`: **102 tests, 0 failures, 2 skipped** (the pre-existing offscreen-GL-gated `test_gggs_render` cases). `test_raster_layer_gdal_cleanup` green. + +### Deferred +None — both pre-push suggestions actioned. diff --git a/CMakeLists.txt b/CMakeLists.txt index 311c54d..a1c2334 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -557,6 +557,25 @@ if(BUILD_TESTING) ${GDAL_LIBRARY} ) + # [camp#96] RasterLayer GDAL handle-leak regression: a load + two colormap + # re-renders must return the process-wide open-dataset count to its baseline, + # proving loadAndReprojectFile RAII-closes its source dataset + warped VRT. + ament_add_gtest(test_raster_layer_gdal_cleanup + test/test_raster_layer_gdal_cleanup.cpp + ) + target_include_directories(test_raster_layer_gdal_cleanup PRIVATE + ${CMAKE_CURRENT_SOURCE_DIR}/src/camp2 + ) + target_link_libraries(test_raster_layer_gdal_cleanup + camp_map + Qt5::Core + Qt5::Gui + Qt5::Widgets + Qt5::Concurrent + Qt5::Positioning + ${GDAL_LIBRARY} + ) + # [camp#102] GGGS tile-set default-off visibility + persistence round-trip # (non-GL state machine — the highest-risk logic of the lazy/async change). ament_add_gtest(test_gggs_visibility diff --git a/src/camp2/raster/raster_layer.cpp b/src/camp2/raster/raster_layer.cpp index 8a35657..2a73205 100644 --- a/src/camp2/raster/raster_layer.cpp +++ b/src/camp2/raster/raster_layer.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -126,7 +127,18 @@ RasterLayer::LoadResult RasterLayer::loadAndReprojectFile(const QString& filenam { LoadResult result; - auto dataset = GDALDataset::FromHandle(GDALOpen(filename.toLatin1(), GA_ReadOnly)); + // [#96] RAII-close both GDAL handles so every return path — normal, the + // !reprojected_dataset early return, and the abort-flag `return {}` paths in + // the scan loops below — frees them; previously they leaked on every call + // (ctor / setColormap / readSettings). Declaration order matters: local + // unique_ptrs destruct in reverse declaration order, so `dataset` MUST stay + // declared before `reprojected_dataset` — the warped VRT references the source + // dataset, so it has to close first, then the source. This mirrors + // initExtent()'s deliberate GDALClose(reprojected) then GDALClose(dataset). + // Do not reorder. + const auto gdal_closer = [](GDALDataset* d){ if(d) GDALClose(d); }; + std::unique_ptr dataset( + GDALDataset::FromHandle(GDALOpen(filename.toLatin1(), GA_ReadOnly)), gdal_closer); if(!dataset) { @@ -134,7 +146,9 @@ RasterLayer::LoadResult RasterLayer::loadAndReprojectFile(const QString& filenam return result; } - auto reprojected_dataset = GDALDataset::FromHandle(GDALAutoCreateWarpedVRT(dataset, nullptr, web_mercator::wkt, GRA_Bilinear, 0.0, nullptr)); + std::unique_ptr reprojected_dataset( + GDALDataset::FromHandle(GDALAutoCreateWarpedVRT(dataset.get(), nullptr, web_mercator::wkt, GRA_Bilinear, 0.0, nullptr)), + gdal_closer); if(!reprojected_dataset) { diff --git a/src/camp2/raster/raster_layer.h b/src/camp2/raster/raster_layer.h index 36aa177..878386a 100644 --- a/src/camp2/raster/raster_layer.h +++ b/src/camp2/raster/raster_layer.h @@ -57,10 +57,10 @@ class RasterLayer: public map::Layer struct LoadResult { Mipmaps mipmaps; - double world_x; - double world_y; - double scale_x; - double scale_y; + double world_x = 0.0; + double world_y = 0.0; + double scale_x = 0.0; + double scale_y = 0.0; bool is_scalar = false; // single-band Float32 (depth) -> ColorMap-shaded }; diff --git a/test/test_raster_layer_gdal_cleanup.cpp b/test/test_raster_layer_gdal_cleanup.cpp new file mode 100644 index 0000000..b0a8c23 --- /dev/null +++ b/test/test_raster_layer_gdal_cleanup.cpp @@ -0,0 +1,145 @@ +// [camp#96] Regression test for the GDAL dataset leak in +// RasterLayer::loadAndReprojectFile. That function opens a source dataset +// (GDALOpen) and a warped VRT (GDALAutoCreateWarpedVRT) on every invocation +// (ctor, setColormap, readSettings) and historically closed neither — leaking +// two handles per load. The fix wraps both in RAII unique_ptrs; this test +// proves no handle survives a load + colormap re-render cycle. +// +// Strategy (Plan Review resolutions): +// - #2 baseline-delta: GDALDataset::GetOpenDatasets() counts ALL process-wide +// open handles, so capture the count BEFORE constructing the layer and +// assert it returns to that baseline after destruction (delta == 0), rather +// than assuming zero. +// - #1 no vacuous pass: assert each load actually SUCCEEDED and reached the +// warp path (status() == "" — imageReady() only clears the status after the +// warp produced non-empty mipmaps; a null/unwarpable load leaves +// "(load failed)"). A silently-failing load can't make the leak check pass +// vacuously, because a load that never warps also never opens the handles. + +#include + +#include + +#include +#include + +#include +#include +#include + +#include "map/map.h" +#include "map/layer_list.h" +#include "raster/raster_layer.h" + +namespace +{ + +// Minimal valid WGS84 single-band Float32 GeoTIFF with real georeferencing, so +// GDALAutoCreateWarpedVRT(... web_mercator ...) actually produces a reprojected +// dataset and loadAndReprojectFile runs its full warp + scalar-shade path. +QString writeRaster(const QTemporaryDir& dir) +{ + if(GDALGetDriverCount() == 0) + GDALAllRegister(); + const QString path = dir.filePath("raster.tif"); + GDALDriver* driver = GetGDALDriverManager()->GetDriverByName("GTiff"); + if(!driver) + return QString(); + + const int w = 8, h = 8; + GDALDataset* ds = driver->Create(path.toUtf8().constData(), w, h, 1, GDT_Float32, nullptr); + if(!ds) + return QString(); + + // Small north-up patch near (-71.4, 43.0). + double geo[6] = {-71.40, 0.0001, 0.0, 43.00, 0.0, -0.0001}; + ds->SetGeoTransform(geo); + + OGRSpatialReference srs; + srs.SetWellKnownGeogCS("WGS84"); + char* wkt = nullptr; + srs.exportToWkt(&wkt); + ds->SetProjection(wkt); + CPLFree(wkt); + + // Varying values so the ColorMap spans a real (non-degenerate) range. + std::vector samples(static_cast(w) * h); + for(size_t i = 0; i < samples.size(); ++i) + samples[i] = static_cast(i); + + GDALRasterBand* band = ds->GetRasterBand(1); + const CPLErr err = band->RasterIO(GF_Write, 0, 0, w, h, samples.data(), w, h, GDT_Float32, 0, 0); + GDALClose(ds); + return err == CE_None ? path : QString(); +} + +int openDatasetCount() +{ + int count = 0; + GDALDataset::GetOpenDatasets(&count); + return count; +} + +// The pixel load is async (QtConcurrent + QFutureWatcher); imageReady() runs on +// the event loop and sets the status to "" (success) or "(load failed)". Spin +// the loop until the status leaves "(loading...)"; returns true on success. +bool waitForLoad(const camp::raster::RasterLayer* layer, int timeout_ms = 5000) +{ + QElapsedTimer timer; + timer.start(); + while(layer->status() == QStringLiteral("(loading...)") && timer.elapsed() < timeout_ms) + QCoreApplication::processEvents(QEventLoop::AllEvents, 50); + return layer->status().isEmpty(); +} + +} // namespace + +TEST(RasterLayerGdalCleanupTest, LoadAndColormapFlipsLeakNoDatasets) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + const QString path = writeRaster(dir); + ASSERT_FALSE(path.isEmpty()); + + camp::map::Map map; + camp::map::LayerList* layers = map.topLevelLayers(); + ASSERT_NE(layers, nullptr); + + // [#2] Baseline AFTER writeRaster (which opened+closed its own handle) and + // BEFORE the layer exists — Map/GDAL internals may hold unrelated handles. + const int baseline = openDatasetCount(); + + auto* layer = new camp::raster::RasterLayer(layers, path); + + // [#1] The synchronous initExtent() warp must have succeeded... + ASSERT_TRUE(layer->valid()); + EXPECT_FALSE(layer->boundingRect().isEmpty()); + // ...and so must the async load that opens the handles under test. + ASSERT_TRUE(waitForLoad(layer)) << "initial load did not reach the warp path"; + + // Two additional ramp types (default is Viridis); each re-invokes + // loadAndReprojectFile, opening and — with the fix — closing both handles. + layer->setColormap(camp::map::ColorMap::Grayscale); + ASSERT_TRUE(waitForLoad(layer)) << "Grayscale re-render did not reach the warp path"; + layer->setColormap(camp::map::ColorMap::Turbo); + ASSERT_TRUE(waitForLoad(layer)) << "Turbo re-render did not reach the warp path"; + + // Dtor aborts + joins the in-flight load (waitForFinished), so every + // loadAndReprojectFile invocation has returned and closed its handles. + // Deliberately a raw `delete` rather than removeFromMap(): removeFromMap() + // defers destruction (deleteLater) to a later event-loop turn, so the + // open-dataset count below could run before the dtor's join completes and + // flake. The direct delete makes the join synchronous and the guarantee firm. + delete layer; + + // [#2] Every load + colormap flip closed its source dataset and warped VRT. + EXPECT_EQ(openDatasetCount(), baseline); +} + +int main(int argc, char** argv) +{ + qputenv("QT_QPA_PLATFORM", "offscreen"); + QApplication app(argc, argv); + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +}