Skip to content

fix(camp): close GDAL/OGR handle leaks in chart/vector loading (#152)#159

Merged
rolker merged 6 commits into
jazzyfrom
feature/issue-152
Jun 30, 2026
Merged

fix(camp): close GDAL/OGR handle leaks in chart/vector loading (#152)#159
rolker merged 6 commits into
jazzyfrom
feature/issue-152

Conversation

@rolker

@rolker rolker commented Jun 30, 2026

Copy link
Copy Markdown
Owner

Summary

Closes #152. Every chart load leaked GDAL/OGR handles, valgrind-confirmed at ~180 KB of OGR/PROJ state per Georeferenced instance, growing monotonically across a session.

Two leak sites fixed:

  1. Georeferenced had no destructor — its two OGRCoordinateTransformation members (created in extractGeoreference) leaked per DepthRaster / VectorDataset. Added a virtual ~Georeferenced() that DestroyCTs both.
    • Why virtual: defensive future-proofing — today neither subclass is deleted through a Georeferenced* (DepthRaster as DepthRaster*, VectorDataset via the QObject parent chain), so the dtor runs regardless; virtual keeps delete-through-base correct if that ever changes.
    • Rule-of-three: = delete copy ctor/assignment now that the class owns handles (both subclasses are heap-only, so no live call site changes).
  2. VectorDataset::open never GDALClosed the dataset, never destroyed the per-layer transformation, and never destroyed any OGRPointIterator. Fixes: RAII-close the dataset; per-layer transformation freed by RAII (unique_ptr + DestroyCT deleter) at end-of-layer (previously all but the last layer leaked); every iterator destroyed at its site, including the interior-ring iterator inside the ring loop.

Testable seam (refactor)

open() interleaved the GDAL/OGR resource lifecycle with building the project item graph (Point/LineString/Polygon + connect(autonomousVehicleProject(), …)), so any test driving open() would link ~all 52 app TUs and stand up a QApplication-backed AutonomousVehicleProject. The pure parse is split into a MissionItem-free TU:

  • vector/vector_parse.{h,cpp}camp::vector::parseVectorLayers(GDALDataset*) returns plain WGS84 geometry (ParsedLayer/ParsedGeometry) and owns the transform + iterator lifecycle.
  • open() → RAII-open dataset, extractGeoreference, then buildItems(parseVectorLayers(...)).
  • buildItems holds the unchanged item-graph construction; per-geometry coordinate handling preserved verbatim.

Tests

  • test_vector_dataset_cleanup (new) — links the standalone vector_parse TU (no item graph); synthetic 2-layer GeoPackage (point + linestring + polygon-with-hole per layer) exercises every parse/iterator site; asserts the parse is non-vacuous and the GDAL open-dataset count returns to baseline.
  • test_depth_raster — added a baseline-delta test guarding the pre-existing DepthRaster GDALClose.

The OGRCoordinateTransformation handles (the primary ~180 KB leak) are not visible to GetOpenDatasets(), so they are proven closed under valgrind:

test_vector_dataset_cleanup:  definitely lost: 0 bytes in 0 blocks
                              indirectly lost: 0 bytes in 0 blocks
test_depth_raster:            definitely lost: 0 bytes in 0 blocks
                              indirectly lost: 0 bytes in 0 blocks

Full package suite: 154 tests, 0 failures.

Review

Local pre-push review (/run-issue): approved, Ship: recommended (zero must-fix; two adversarial passes confirmed behavior-preserving + leak-free). Two review suggestions folded (RAII exception-safety on the per-layer transform; corrected the ~Georeferenced comment); the third (attach valgrind output) is the block above.


Authored-By: Claude Code Agent
Model: Claude Opus 4.8

Claude Code Agent added 6 commits June 30, 2026 11:31
Virtual destructor for Georeferenced + RAII fixes in VectorDataset::open
Every chart load leaked GDAL/OGR handles (valgrind-confirmed ~180 KB of
OGR/PROJ state per Georeferenced instance, growing across a session):

- Georeferenced had no destructor, so its two OGRCoordinateTransformation
  members (created in extractGeoreference) leaked per DepthRaster /
  VectorDataset. Add a virtual ~Georeferenced() that DestroyCT's both; the
  dtor is virtual because both subclasses are deleted through a base
  subobject. Rule-of-three: =delete copy ctor/assignment now that the class
  owns handles (both subclasses are heap-only, so no live call site changes).
- VectorDataset::open never GDALClose'd the dataset, never destroyed the
  per-layer transformation, and never destroyed any OGRPointIterator. Fixes:
  RAII-close the dataset; destroy the transformation at end-of-layer (else
  all but the last layer leaked); destroy every iterator at its site,
  including the interior-ring iterator inside the ring loop.

To make the leak path testable without standing up the whole application
(open() built the project item graph + connected to AutonomousVehicleProject,
which would drag ~all app TUs into a test), split the pure parse into a new
MissionItem-free TU: camp::vector::parseVectorLayers(GDALDataset*) returns
plain WGS84 geometry and owns the transform + iterator lifecycle. open() now
RAII-opens the dataset, calls extractGeoreference, then
buildItems(parseVectorLayers(...)); buildItems holds the unchanged item-graph
construction. Per-geometry coordinate handling preserved verbatim.

Tests:
- test_vector_dataset_cleanup (new): links the standalone vector_parse TU;
  synthetic 2-layer GeoPackage (point + linestring + polygon-with-hole per
  layer) exercises every parse/iterator site; asserts the parse is non-vacuous
  and the GDAL open-dataset count returns to baseline.
- test_depth_raster: add a baseline-delta test guarding the pre-existing
  DepthRaster GDALClose.

The OGRCoordinateTransformation handles (the primary ~180 KB leak) are not
visible to GetOpenDatasets(), so they are proven closed under valgrind:
valgrind --leak-check=full on both test binaries reports
"definitely lost: 0 bytes, indirectly lost: 0 bytes".

Closes #152
Copilot AI review requested due to automatic review settings June 30, 2026 17:17

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@rolker rolker merged commit df4c4c6 into jazzy Jun 30, 2026
1 check failed
@rolker rolker deleted the feature/issue-152 branch June 30, 2026 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OGR/GDAL handle leaks in chart/vector loading (Georeferenced + VectorDataset)

2 participants