rolker · rolker · Jun 30, 2026 · Jun 30, 2026 · Jun 30, 2026 · Jun 30, 2026
diff --git a/.agent/work-plans/issue-152/plan.md b/.agent/work-plans/issue-152/plan.md
@@ -0,0 +1,76 @@
+# Plan: Fix GDAL/OGR handle leaks in Georeferenced / VectorDataset chart loading
+
+## Issue
+
+https://github.com/rolker/camp/issues/152
+
+## Context
+
+Two classes leak GDAL/OGR handles on every chart load, confirmed by valgrind (~180 KB of OGR/PROJ state per `DepthRaster` / `VectorDataset` instance, growing monotonically across a session):
+
+1. **`Georeferenced`** (`georeferenced.{h,cpp}`) — `extractGeoreference()` allocates `m_projectTransformation` and `m_unprojectTransformation` via `OGRCreateCoordinateTransformation` (lines 48–49). There is no destructor to free them. This base class is inherited by both `DepthRaster` and `VectorDataset`, so every instance leaks both handles.
+
+2. **`VectorDataset::open`** (`vector/vectordataset.cpp`) — four leaks:
+   - `GDALDataset*` from `GDALOpenEx` (line 22) is never `GDALClose`d.
+   - Per-layer `OGRCoordinateTransformation* unprojectTransformation` (line 35) is never destroyed.
+   - `OGRPointIterator* pi` is never destroyed at three sites: linestring (line 72), polygon exterior ring (line 100), polygon interior rings (line 119).
+   - (`OGRFeature` is correctly freed via `DestroyFeature` — no change needed there.)
+
+`DepthRaster` already calls `GDALClose(dataset)` (line 30 of `depth_raster.cpp`) — no dataset handle leak there. The `gdal_closer` RAII pattern is already established in `raster_layer.cpp:176–178`.
+
+## Approach
+
+1. **Add `virtual ~Georeferenced()`** — destroy both OGR coordinate transformations. Also `= delete` the copy ctor/assignment (rule-of-three: the class now owns handles; both subclasses are heap-only so no live call site changes). *(review-plan suggestion folded)*
+2. **Fix `VectorDataset::open`** — RAII-close the GDAL dataset; destroy the per-layer transformation **at end-of-layer** (else all but the last layer leak); destroy every `OGRPointIterator` **at its site**, including the interior-ring iterator **inside the ring loop** (`:119`, reassigned per ring). *(review-plan must-fix folded)*
+3. **Extract a testable seam (operator decision, post-review).** The original `open()` interleaved the GDAL/OGR resource lifecycle with construction of the project item graph (`Point`/`LineString`/`Polygon` + `connect(autonomousVehicleProject(), …)`), so any test driving `open()` would have to link ~all 52 app TUs and stand up a `QApplication`-backed `AutonomousVehicleProject` — the "bounded source list" balloons. Instead split the pure parse into a new MissionItem-free TU:
+   - `src/camp/vector/vector_parse.{h,cpp}` — `camp::vector::parseVectorLayers(GDALDataset*)` returns plain WGS84 geometry (`ParsedLayer`/`ParsedGeometry`) and owns the per-layer transform + per-ring iterator lifecycle (the #152 leak sites). No Qt-item / project coupling.
+   - `VectorDataset::open` RAII-opens the dataset, calls `extractGeoreference`, then `buildItems(parseVectorLayers(...))`.
+   - `VectorDataset::buildItems(layers)` — the unchanged item-graph construction, now fed plain data. Coordinate handling preserved verbatim per geometry type.
+4. **Extend `test_depth_raster.cpp`** — add a GDAL open-dataset-count baseline-delta test (`LoadAndDestroyLeavesNoOpenDataset`). NOTE: it guards the **pre-existing** `GDALClose`, not the new dtor — `GetOpenDatasets()` can't see the `OGRCoordinateTransformation` handles the dtor frees (valgrind-only). Stated in the test comment. *(review-plan suggestion folded)*
+5. **Add `test_vector_dataset_cleanup.cpp`** — links the standalone `vector_parse` TU (no item graph). Synthetic 2-layer GeoPackage (point + linestring + polygon-with-hole per layer); calls `parseVectorLayers`; asserts every parse branch ran (non-vacuous) and the GDAL open-dataset count returns to baseline. The transform/iterator leaks are **valgrind-only** (run the binary with `--leak-check=full`; definitely-lost must be 0) — documented in the test + CMake comments.
+
+## Files to Change
+
+| File | Change |
+|------|--------|
+| `src/camp/georeferenced.h` | Add `virtual ~Georeferenced();` + `= delete` copy ctor/assignment |
+| `src/camp/georeferenced.cpp` | Implement destructor: `OGRCoordinateTransformation::DestroyCT` on both members |
+| `src/camp/vector/vector_parse.h` | **New** — `ParsedGeometry`/`ParsedLayer` + `parseVectorLayers(GDALDataset*)` (MissionItem-free) |
+| `src/camp/vector/vector_parse.cpp` | **New** — pure parse; per-layer transform + per-ring iterator created and destroyed; `OGRFeature::DestroyFeature` retained |
+| `src/camp/vector/vectordataset.h` | Forward-declare `camp::vector::ParsedLayer`; declare private `buildItems` |
+| `src/camp/vector/vectordataset.cpp` | `open()` → RAII dataset + `extractGeoreference` + `buildItems(parseVectorLayers(...))`; new `buildItems` holds the item-graph construction |
+| `test/test_depth_raster.cpp` | Add GDAL open-dataset baseline-delta test (guards pre-existing `GDALClose`; dtor leak is valgrind-only) |
+| `test/test_vector_dataset_cleanup.cpp` | **New** — links `vector_parse` TU; synthetic 2-layer GPKG; asserts parse branches + handle-count baseline; transform/iterator leaks valgrind-only |
+| `CMakeLists.txt` | Add `vector_parse.cpp` to executable sources; register `test_vector_dataset_cleanup` |
+
+## Principles Self-Check
+
+| Principle | Consideration |
+|---|---|
+| Only what's needed | Fix targets exactly the confirmed leak sites; no refactor beyond RAII at those sites |
+| A change includes its consequences | Tests added for both fix paths; valgrind re-run required in PR |
+| Test what breaks | New tests catch regressions in the GDAL dataset leak path (handle count delta); OGR transform leak covered by valgrind output in PR |
+| Improve incrementally | Single PR; no structural refactor of `Georeferenced` beyond the dtor |
+
+## ADR Compliance
+
+| ADR | Triggered | How addressed |
+|---|---|---|
+| ADR-0002 — Worktree isolation | Yes | Worktree `issue-camp-152` / branch `feature/issue-152` already in use |
+| ADR-0008 — ROS 2 conventions | Marginal | C++ source only; follow existing RAII and naming patterns in this package |
+| ADR-0013 — progress.md vocabulary | Yes | progress.md maintained |
+
+## Consequences
+
+| If we change... | Also update... | Included in plan? |
+|---|---|---|
+| `Georeferenced` gains `virtual ~Georeferenced()` | `DepthRaster` and `VectorDataset` inherit — they will call the base dtor automatically; no extra change needed | Yes — confirmed both subclasses hold no additional OGR handles beyond those `Georeferenced` owns |
+| `VectorDataset::open` RAII-closes `GDALDataset*` | Existing behaviour (close on scope exit of `open()`) is unchanged — dataset was only needed for the duration of that call | Yes |
+
+## Open Questions
+
+- None — fix approach is unambiguous; valgrind harness is the implementer's responsibility (run before PR).
+
+## Estimated Scope
+
+Single PR.
diff --git a/.agent/work-plans/issue-152/progress.md b/.agent/work-plans/issue-152/progress.md
@@ -0,0 +1,110 @@
+---
+issue: 152
+---
+
+# Issue #152 — Fix GDAL/OGR handle leaks in Georeferenced / VectorDataset chart loading
+
+## Issue Review
+**Status**: complete
+**When**: 2026-06-30 00:00 +00:00
+**By**: Claude Code Agent (Claude Sonnet)
+
+**Issue**: #152
+**Comment**: (best-effort post follows this entry; not recorded inline)
+**Scope verdict**: well-scoped
+
+### Summary
+
+Confirmed GDAL/OGR memory leaks in `camp` project repo, specifically:
+1. `Georeferenced` — missing destructor; two `OGRCoordinateTransformation*` members allocated in `extractGeoreference()` (lines 48–49 of `georeferenced.cpp`) are never freed. Every `DepthRaster` and `VectorDataset` instance leaks ~180 KB of OGR/PROJ state.
+2. `VectorDataset::open` — `GDALDataset*` from `GDALOpenEx` is never `GDALClose`d; per-layer `OGRCoordinateTransformation` is never destroyed; `OGRPointIterator*` leaked at all three geometry branches (linestring line 72, polygon exterior ring line 100, polygon interior ring line 119).
+
+The leaks are well-evidenced (valgrind confirmed). The fix is clearly scoped to two files (`georeferenced.{h,cpp}` and `vector/vectordataset.cpp`) and is completable in a single PR.
+
+### Principle Alignment
+
+| Principle | Status | Notes |
+|---|---|---|
+| Human control and transparency | OK | Issue is well-documented with valgrind stacks; proposed fix is unambiguous |
+| Enforcement over documentation | Watch | No automated memory-leak CI check exists; valgrind re-run is manual. Acceptable for this codebase, but worth noting |
+| Capture decisions, not just implementations | Watch | Adding a `virtual` destructor to `Georeferenced` is a minor design decision (vtable impact); the rationale should appear in the PR description |
+| A change includes its consequences | Action needed | Re-run valgrind after the fix and include output in the PR to prove the leak is closed; explicitly confirm `DepthRaster` has no direct GDAL handle members beyond those `Georeferenced` holds |
+| Only what's needed | OK | Fix is minimal and targeted; no scope creep |
+| Improve incrementally | OK | Small, reviewable single-PR change |
+| Test what breaks | Watch | No automated regression for this leak; valgrind harness is manual. Acceptable given the project's test maturity |
+| Workspace vs. project separation | OK | Correctly scoped to the `camp` project repo |
+
+### ADR Applicability
+
+| ADR | Triggered | Notes |
+|---|---|---|
+| ADR-0002 — Worktree isolation | Yes | Worktree already exists (`issue-camp-152`) |
+| ADR-0008 — ROS 2 conventions | Marginal | C++ source only; no package interface changes; follow existing RAII patterns in this package |
+| ADR-0013 — progress.md vocabulary | Yes | This entry |
+
+### Consequences
+
+- `DepthRaster` inherits `Georeferenced` and will gain the virtual destructor. Confirmed it holds no direct GDAL handle members beyond those `Georeferenced` owns — no additional fix needed there.
+- The `gdal_closer` lambda pattern (unique_ptr + custom deleter) is already established in `camp_map/raster/raster_layer.cpp:176–178`. The implementer should reuse or reference that pattern for `VectorDataset::open`'s `GDALDataset*` to keep the codebase consistent.
+
+### Actions
+- [ ] Re-run valgrind after the fix and include "definitely lost" output in the PR (proves the leak is closed from `Georeferenced::extractGeoreference` frames)
+- [ ] Add virtual destructor to `Georeferenced`; include "why virtual" rationale in PR description (base class with polymorphic subclasses — deleting through base pointer would be UB without it)
+- [ ] In `VectorDataset::open`: use the `gdal_closer` unique_ptr pattern (already in `raster_layer.cpp:176`) for the `GDALDataset*`; destroy the per-layer `OGRCoordinateTransformation`; call `OGRPointIterator::destroy(pi)` at all three iterator sites
+- [ ] Confirm `DepthRaster` has no direct GDAL handle members that also need cleanup (quick grep shows it does not, but verify during implementation)
+
+## Plan Authored
+**Status**: complete
+**When**: 2026-06-30 14:00 +00:00
+**By**: Claude Code Agent (Claude Sonnet)
+
+**Plan**: `.agent/work-plans/issue-152/plan.md` at `40d4749`
+**Branch**: feature/issue-152 at `40d4749`
+**Phases**: single
+
+### Open questions
+- [ ] No open questions — plan is review-plan-ready.
+
+## Plan Review
+**Status**: complete
+**When**: 2026-06-30 15:15 +00:00
+**By**: Claude Code Agent (Claude Opus)
+<!-- Independent: the plan was authored by a separate fresh-context Sonnet dispatch;
+this review is a separate Opus dispatch. The shared workspace identity "Claude Code Agent"
+matches by name for every agent, so the mechanical self-review check does not apply here. -->
+
+**Plan**: `.agent/work-plans/issue-152/plan.md` at `40d4749`
+**PR**: PR-less (`gh` unauthenticated in this environment; reviewed local plan file)
+**Verdict**: approve-with-suggestions
+
+### Findings
+- [ ] (must-fix) `test_vector_dataset_cleanup` has no library to link — `vectordataset.cpp`/`group.cpp`/`autonomousvehicleproject.cpp` compile only into the `CCOMAutonomousMissionPlanner` executable, and `open()` drags the full QGraphics item graph + `AutonomousVehicleProject` (mission_manager, platform_manager, map/raster). Settle the build strategy (bounded source list vs. valgrind-only descope) before implementing — `plan.md:26,36,37`
+- [ ] (must-fix) In `VectorDataset::open`, destroy the per-layer `unprojectTransformation` at end-of-layer (else all but the last layer leak), and destroy the interior-ring `OGRPointIterator` inside the ring loop (`:119`, reassigned per ring) — `plan.md:34`
+- [ ] (suggestion) Automated tests cover only the GDAL-dataset leak; `GetOpenDatasets()` does not see the `OGRCoordinateTransformation` handles that are the primary ~180 KB `Georeferenced` leak (valgrind-only). The `DepthRaster` test guards the pre-existing `GDALClose`, not this PR's dtor. State this in test comments so the names don't mislead — `plan.md:25,26,45`
+- [ ] (suggestion) Rule of three: `= delete` copy ctor/assignment on `Georeferenced` when adding the owning dtor (latent double-free if ever copied; currently heap-only so no live bug) — `plan.md:23,32`
+
+### Notes
+- Source fixes verified against actual code: all five leak sites confirmed real and correctly located (`georeferenced.cpp:48-49`; `vectordataset.cpp:22,35,72,100,119`). Approach correctly reuses the `gdal_closer` RAII pattern (`raster_layer.cpp:176`) and the `GetOpenDatasets()` baseline-delta test pattern (`test_raster_layer_gdal_cleanup.cpp`).
+- `virtual ~Georeferenced()` is not strictly required (no call site deletes through `Georeferenced*`) but is a sound defensive choice; review-issue already tracks the PR rationale.
+
+## Local Review (Pre-Push)
+**Status**: complete
+**When**: 2026-06-30 16:39 +00:00
+**By**: Claude Code Agent (Claude Opus)
+**Verdict**: approved
+
+**Branch**: feature/issue-152 at `550164a`
+**Mode**: pre-push
+**Depth**: Deep (reason: memory/resource-lifecycle fix; polymorphic base-class change affecting DepthRaster + VectorDataset)
+**Must-fix**: 0 | **Suggestions**: 3
+**Round**: 1 | **Ship**: recommended — zero must-fix; refactor verified behavior-preserving and leak-free on every path by two adversarial passes
+
+### Findings
+- [ ] (suggestion) Attach valgrind `--leak-check=full` output (`definitely lost: 0`) to the PR — the automated tests only see the GDAL dataset handle via `GetOpenDatasets()`, not the primary ~180 KB `OGRCoordinateTransformation` leak the new dtor frees — `georeferenced.cpp:14`, `test/test_vector_dataset_cleanup.cpp`
+- [ ] (suggestion) Soften the `~Georeferenced` comment: `DepthRaster` is deleted as `DepthRaster*` and `VectorDataset` via the QObject chain — neither is delete-through-`Georeferenced*`, so the stated risk is overstated (the `virtual` is fine as defensive future-proofing) — `georeferenced.h:13`
+- [ ] (suggestion) Optional RAII parity: the per-layer `unprojectTransformation` raw pointer is held across the feature loop and its end-of-layer `DestroyCT` is not exception-safe (OOM-only); wrap it in `unique_ptr<OGRCoordinateTransformation, &DestroyCT>` like the dataset closer — `src/camp/vector/vector_parse.cpp:49`
+
+### Notes
+- Static analysis: cppcheck clean (one const-pointer style nit only); ament_cpplint findings (header-guard style, trailing whitespace) are all pre-existing package style confirmed via `git blame` (2017–2021), not introduced here.
+- Two fresh-context Claude adversarial passes (Lens A logic / Lens B systemic-lifecycle) independently traced every GDAL/OGR handle and confirmed: all coordinate handling preserved verbatim (incl. the Point `(Y,X)` vs Line/Polygon `(X,Y)` axis quirk), every handle freed on every path, no double-free, no new leak, `=delete` copy ops break no call site (both subclasses heap-only via `new`), and the constructor zero-inits both transformation pointers so the dtor null-checks are safe.
+- Full plan adherence: both review-plan must-fixes (end-of-layer transform destroy; per-ring iterator destroy) and both suggestions (rule-of-three `=delete`; honest test comments) are folded into the implementation. The `vector_parse` testable-seam extraction resolves the plan-review must-fix about the test having no linkable library.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -97,6 +97,7 @@ set(SOURCES
     src/camp/waypointdetails.cpp
 
     src/camp/vector/vectordataset.cpp
+    src/camp/vector/vector_parse.cpp
     src/camp/group.cpp
     src/camp/vector/point.cpp
     src/camp/vector/linestring.cpp
@@ -497,6 +498,28 @@ if(BUILD_TESTING)
     ${GDAL_LIBRARY}
   )
 
+  # [camp#152] VectorDataset GDAL/OGR handle-leak regression. parseVectorLayers
+  # creates and destroys each layer's OGRCoordinateTransformation and every
+  # OGRPointIterator and must leave the process-wide open-dataset count at
+  # baseline. NOTE: GetOpenDatasets() can see the GDAL *dataset* handle but NOT
+  # the OGRCoordinateTransformation pipelines that are the primary ~180 KB leak —
+  # those are proven only under valgrind (run this binary with
+  # `valgrind --leak-check=full`; "definitely lost" must be 0). Linked against
+  # the standalone vector_parse TU so the leak path is exercised without the
+  # MissionItem / AutonomousVehicleProject item graph.
+  ament_add_gtest(test_vector_dataset_cleanup
+    test/test_vector_dataset_cleanup.cpp
+    src/camp/vector/vector_parse.cpp
+  )
+  target_include_directories(test_vector_dataset_cleanup PRIVATE
+    ${CMAKE_CURRENT_SOURCE_DIR}/src/camp
+  )
+  target_link_libraries(test_vector_dataset_cleanup
+    Qt5::Core
+    Qt5::Positioning
+    ${GDAL_LIBRARY}
+  )
+
   # A* shoal avoidance + unknown-depth-as-obstacle (#59 PR6 — depth/A* decoupled
   # from BackgroundRaster). Pure std C++; no Qt/ROS link needed.
   ament_add_gtest(test_astar

diff --git a/src/camp/georeferenced.cpp b/src/camp/georeferenced.cpp
@@ -11,6 +11,17 @@ Georeferenced::Georeferenced(): m_geoTransform{0.0,1.0,0.0,0.0,0.0,1.0}, m_inver
 
 }
 
+Georeferenced::~Georeferenced()
+{
+    // [#152] Free the PROJ-backed transformation pipelines allocated by
+    // extractGeoreference. Each is ~tens of KB of PROJ state; without this they
+    // leaked once per chart load (DepthRaster + VectorDataset both derive here).
+    if(m_projectTransformation)
+        OGRCoordinateTransformation::DestroyCT(m_projectTransformation);
+    if(m_unprojectTransformation)
+        OGRCoordinateTransformation::DestroyCT(m_unprojectTransformation);
+}
+
 QPointF Georeferenced::pixelToProjectedPoint(const QPointF &point) const
 {
     return QPointF(m_geoTransform[0]+point.x()*m_geoTransform[1]+point.y()*m_geoTransform[2],

diff --git a/src/camp/georeferenced.h b/src/camp/georeferenced.h
@@ -10,6 +10,18 @@ class Georeferenced
 {
 public:
     Georeferenced();
+    // [#152] Owns m_project/unprojectTransformation (created in
+    // extractGeoreference) and frees them here. virtual is defensive
+    // future-proofing: today neither subclass is deleted through a
+    // Georeferenced* (DepthRaster is deleted as DepthRaster*, VectorDataset via
+    // the QObject parent chain), so the dtor runs regardless — but making it
+    // virtual keeps delete-through-base correct if that ever changes. The
+    // =delete rule-of-three guards against a copy silently double-freeing those
+    // owned handles; both subclasses are heap-only today, so this removes a
+    // latent foot-gun without affecting any live call site.
+    virtual ~Georeferenced();
+    Georeferenced(const Georeferenced &) = delete;
+    Georeferenced &operator=(const Georeferenced &) = delete;
     QPointF pixelToProjectedPoint(QPointF const &point) const;
     QPointF projectedPointToPixel(QPointF const &point) const;
     QPointF project(QGeoCoordinate const &point) const;