Give the propagation models and layers an owner

.github/workflows/sanitizers.yml

@@ -9,11 +9,12 @@ name: 🧼 Sanitizers (Linux)
 # overflow, no double free, on either path. There is no backlog to clear
 # first, and the point of turning it on now is to keep it that way. See #162.
 #
-# Leaks are a separate matter and are deliberately NOT blocking: nothing owns
-# a PropagationModel (#159), so ASan reports every one as leaked. Making that
-# fail the build would produce a permanently red job, which teaches everyone
-# to ignore it. The leak check runs anyway, as an informational step, so the
-# number stays visible and can be made blocking once #159 lands.
+# Leaks are blocking for the unit suite. They were not when this job landed —
+# nothing owned a PropagationModel, so ASan reported every one as leaked, about
+# 4.7 MB in all. #159 gave the models, the propagative layer and two orphaned
+# constant-layer arrays an owner, which took the unit suite to zero, so the
+# gate went on. runff still leaks ~200 kB on paths the unit suite does not
+# reach, so its leak check stays informational until those are found too.
 #
 # A separate workflow rather than a job in main.yml because it needs its own
 # build: install-forefire.sh builds with -O3 -flto -fomit-frame-pointer, which
@@ -55,27 +56,31 @@ jobs:
           cmake -S . -B build-asan -DFOREFIRE_SANITIZE=address
           cmake --build build-asan -j"$(nproc)"
 
-      # Errors are blocking. detect_leaks=0 is the whole reason this can be:
-      # see the note at the top about #159.
+      # Leaks are blocking here: the unit suite reports none since the model
+      # and layer ownership fix in #159, and this is what keeps it that way.
       - name: Unit tests under ASan
         env:
-          ASAN_OPTIONS: detect_leaks=0
+          ASAN_OPTIONS: detect_leaks=1
         run: ctest --test-dir build-asan --output-on-failure
 
+      # runff still leaks about 200 kB over 783 allocations, on paths a single
+      # simulation reaches and the unit suite does not, so its leak check stays
+      # informational below. Errors are blocking here as they always were.
       - name: runff under ASan
         env:
           ASAN_OPTIONS: detect_leaks=0
         run: |
           cd tests/runff
           bash ff-run.bash
 
-      # Informational only. Prints what is leaking so the figure stays visible
-      # without gating the build on a known ownership gap.
-      - name: Leak report (informational)
+      # Informational only, and only for runff now. Keeps the figure visible so
+      # the remaining ownership gaps can be found and this can join the gate.
+      - name: runff leak report (informational)
         continue-on-error: true
         env:
           ASAN_OPTIONS: detect_leaks=1
         run: |
-          echo "::group::Unit suite leaks"
-          ctest --test-dir build-asan --output-on-failure || true
+          echo "::group::runff leaks"
+          cd tests/runff
+          bash ff-run.bash || true
           echo "::endgroup::"

TESTING.md

@@ -133,13 +133,23 @@ ASAN_OPTIONS=detect_leaks=0 ctest --test-dir build-asan --output-on-failure
 check. Other values are passed straight through — `undefined`, or
 `address,undefined` for both — but only `address` is currently verified clean.
 
-**`detect_leaks=0` is deliberate, not a workaround.** ForeFire reports zero
-ASan *errors* — no use-after-free, no overflow, no double free — on either test
-path, which is what makes a blocking job possible. It does leak: nothing owns a
-`PropagationModel` (#159), so every one is reported. Leaving leak detection on
-would produce a permanently failing job that everyone learns to ignore. The CI
-workflow runs the leak check anyway as an informational step, so the number
-stays visible, and it can be made blocking once #159 lands.
+ForeFire reports zero ASan *errors* — no use-after-free, no overflow, no double
+free — on either test path, which is what makes a blocking job possible.
+
+**Leaks are blocking for the unit suite and informational for `runff`.** The
+unit suite reports none: #159 gave the propagation and flux models, the
+propagative layer and two orphaned constant-layer arrays an owner, which took
+it from about 4.7 MB to zero. Running the suite with leak detection on is
+therefore a check rather than a report:
+
+```bash
+ASAN_OPTIONS=detect_leaks=1 ctest --test-dir build-asan --output-on-failure
+```
+
+`runff` still leaks about 200 kB over 783 allocations, on paths a full
+simulation reaches and the unit suite does not, so its leak check runs without
+gating the build. Closing those is what would let it join the gate; until then
+the figure stays visible in the job log.
 
 Note that the sanitizer build writes `bin/forefire` and `lib/libforefireL.so`
 like any other build, so it replaces a release build in the source tree.

src/DataBroker.cpp

@@ -1062,7 +1062,9 @@ namespace libforefire
 			XYZTDataLayer<double> *layer = new XYZTDataLayer<double>(name, SWCorner, timeOrigin, spatialExtent, Lt,
 																	 nx, ny, nz, nt, data);
 
-			// delete[] data;
+			// FFArray copies the values into storage of its own, so this
+			// array is ours to free and nothing refers to it afterwards.
+			delete[] data;
 			registerLayer(name, layer);
 		}
 		else if (layertype == "flux")
@@ -1081,7 +1083,8 @@ namespace libforefire
 			FluxLayer<double> *layer = new FluxLayer<double>(name,
 															 atmoSWCorner, atmoNECorner, atmosphericNx, atmosphericNy, domain->getCells(),
 															 data, SWCorner, timeOrigin, spatialExtent, Lt, nx, ny, nz, nt);
-			// delete[] data;
+			// Copied by FFArray, same as above.
+			delete[] data;
 			registerFluxModel(newFluxmodel);
 
 			registerFluxLayer(name, layer);

src/FireDomain.cpp

@@ -326,13 +326,18 @@
 			infrontiers.pop_back();
 		}
 
+		// The models this domain registered go back before the broker does:
+		// they hold a pointer to it, and freeing the broker first would leave
+		// a destructor reaching through a dangling pointer.
+		releaseOwnedModels();
+
 		// Deleting data broker and layers
 		if (dataBroker) {
 			delete dataBroker;
 			dataBroker = nullptr;
 		}
 		if (propagativeLayer) {
-			//delete propagativeLayer;
+			delete propagativeLayer;
 			propagativeLayer = nullptr;
 		}
 
@@ -695,20 +700,44 @@
 		 params->setParameter(model->getName() + ".keys", properties);
 		 //cout<< "loading"<< model->getName()<<index<<endl;
 		 propModelsTable[index] = model;
+		 // The table is shared, so remember which entries are ours to release.
+		 ownedPropModelIndices.push_back((size_t) index);
 	 }
- 
+
 	 void FireDomain::registerFluxModel(const int& index, FluxModel* model){
-		  
+
 		 fluxModelsTable[index] = model;
+		 ownedFluxModelIndices.push_back((size_t) index);
+	 }
+
+	 /*! \brief frees the models this domain put in the shared tables */
+	 void FireDomain::releaseOwnedModels(){
+		 for ( size_t i = 0; i < ownedPropModelIndices.size(); i++ ){
+			 const size_t index = ownedPropModelIndices[i];
+			 delete propModelsTable[index];
+			 propModelsTable[index] = 0;
+		 }
+		 ownedPropModelIndices.clear();
+
+		 for ( size_t i = 0; i < ownedFluxModelIndices.size(); i++ ){
+			 const size_t index = ownedFluxModelIndices[i];
+			 delete fluxModelsTable[index];
+			 fluxModelsTable[index] = 0;
+		 }
+		 ownedFluxModelIndices.clear();
 	 }
 
 	 bool FireDomain::addPropagativeLayer(string mname){
 		 /* searching if there exists a propagation model with associated name */
 		 /* affecting it to free index */
 		 size_t mindex = getFreePropModelIndex();
+		 if ( mindex >= NUM_MAX_PROPMODELS ) return false;
 		 PropagationModel* model = propModelInstanciation(mindex, mname);
 		 if ( model == 0 ) return false;
 		 /* Instantiating a flux layer related to this model */
+		 // Replacing an earlier layer: the old one is this domain's to free,
+		 // and nothing else refers to it.
+		 delete propagativeLayer;
 		 propagativeLayer = new PropagativeLayer<double>("ROSlayer", mindex);
 		 return true;
 	 }
@@ -827,6 +856,7 @@
 
 		 // Otherwise, searching for the model in the available ones
 		 size_t mindex = getFreeFluxModelIndex();
+		 if ( mindex >= NUM_MAX_FLUXMODELS ) return false;
 		 string fmname = "";
 		 if ( lname == "heatFlux" ) fmname = "heatFluxBasic";
 		 if ( lname == "vaporFlux" ) fmname = "vaporFluxBasic";
@@ -856,16 +886,29 @@
 
 	 size_t FireDomain::getFreePropModelIndex(){
 		 size_t mindex = NUM_MAX_PROPMODELS - 1;
-		 while ( propModelsTable[mindex] != 0 ) mindex--;
+		 // Bounded: mindex is unsigned, so walking off the bottom of a full
+		 // table wrapped round to SIZE_MAX and read far out of bounds.
+		 while ( propModelsTable[mindex] != 0 ){
+			 if ( mindex == 0 ){
+				 cerr<<"ERROR: no free propagation model slot, max is "
+					 <<NUM_MAX_PROPMODELS<<endl;
+				 return NUM_MAX_PROPMODELS;
+			 }
+			 mindex--;
+		 }
 		 return mindex;
 	 }
 
 	 size_t FireDomain::getFreeFluxModelIndex(){
 		 size_t mindex = 0;
 		 while ( fluxModelsTable[mindex] != 0 ){
 			 if(mindex >= NUM_MAX_FLUXMODELS -1){
-				 cout<<"ERROR No mor flx models allowed, max:"<< NUM_MAX_FLUXMODELS<<endl;
-				 return mindex;
+				 // Returning an out-of-range index rather than an occupied
+				 // one: the caller used to get a live slot back and quietly
+				 // overwrite the model already in it.
+				 cerr<<"ERROR: no free flux model slot, max is "
+					 <<NUM_MAX_FLUXMODELS<<endl;
+				 return NUM_MAX_FLUXMODELS;
 			 }
 			 mindex++;
 		 }
@@ -2778,13 +2821,18 @@
 
 		 /* initializations for the propagation model */
 		 propagativeLayer = nullptr;
-		 for ( size_t i = 0; i < NUM_MAX_PROPMODELS; i++ ) propModelsTable[i] = 0;
-
+		 // The model tables are NOT wiped here. They are static, shared by
+		 // every domain in the process, and clearing them on construction
+		 // dropped whatever an existing domain had registered: a second
+		 // FireDomain (Command.cpp creates one for the coupled master) left
+		 // the first one's PropagativeLayer holding an index that was either
+		 // empty or, once the new domain registered, pointing at the new
+		 // domain's model. Both tables are static storage and so already
+		 // start out null. Each domain now releases its own entries in the
+		 // destructor instead.
+
 		 ostringstream infile;
 		 infile << params->GetPath(params->getParameter("NetCDFfile"));
-
-		/* initializations for the flux models */
-		 for ( size_t i = 0; i < NUM_MAX_FLUXMODELS; i++ ) fluxModelsTable[i] = NULL;
 
 
 		 /* loading the layers for atmospheric variables and coupling variables */

src/FireDomain.h

@@ -309,6 +309,16 @@ class FireDomain: public ForeFireAtom, Visitable {
 	bool addPropagativeLayer(string);
 	size_t getFreePropModelIndex();
 
+	/*! \brief indices this domain put into the shared model tables
+	 *
+	 * propModelsTable and fluxModelsTable are static, so an entry cannot be
+	 * freed just because some domain is going away — only the domain that
+	 * registered it may. These record which ones are ours. */
+	std::vector<size_t> ownedPropModelIndices;
+	std::vector<size_t> ownedFluxModelIndices;
+	/*! \brief deletes this domain's models and clears their table entries */
+	void releaseOwnedModels();
+
 	/* Mesh properties */
     /*-----------------*/
 	FFPoint SWLngLat; /*!< SouthWest Corner of the mesh */

tests/unit/CMakeLists.txt

@@ -9,6 +9,7 @@
 add_executable(forefire_unit_tests
     main.cpp
     model_sandbox.cpp
+    test_domain_ownership.cpp
     test_flux_models.cpp
     test_http_server.cpp
     test_model_registry.cpp
@@ -25,7 +26,8 @@ endif()
 
 # doctest groups its cases into suites; registering one CTest entry per suite
 # keeps `ctest` output useful without needing doctest's CMake integration.
-foreach(_suite "model registry" "propagation models" "flux models" "http server")
+foreach(_suite "model registry" "propagation models" "flux models" "http server"
+                "domain ownership")
     string(REPLACE " " "_" _suite_id "${_suite}")
     add_test(NAME "unit.${_suite_id}"
              COMMAND forefire_unit_tests --test-suite=${_suite} --no-skipped-summary)

tests/unit/test_domain_ownership.cpp

@@ -0,0 +1,111 @@
+/**
+ * @file test_domain_ownership.cpp
+ * @brief A FireDomain owns what it registers, and only what it registers.
+ * @copyright Copyright (C) 2025 ForeFire, Fire Team, SPE, CNRS/Universita di Corsica.
+ * @license This program is free software; See LICENSE file for details. (See LICENSE file).
+ *
+ * propModelsTable and fluxModelsTable are static, shared by every domain in
+ * the process. Two things used to go wrong with that, both covered here (#159):
+ *
+ *   - Nothing freed a domain's models or its propagative layer, so every
+ *     domain built and thrown away cost about 183 kB, linearly, forever.
+ *   - The constructor wiped the whole table, so building a second domain
+ *     dropped the first one's registrations. The first domain's layer then
+ *     held an index that was empty, and became the *second* domain's model
+ *     once that one registered.
+ *
+ * The second is the one worth keeping a test on: it is silent, it produces
+ * wrong spread rates rather than a crash, and Command.cpp builds exactly that
+ * second domain for coupled runs.
+ */
+
+#include "doctest/doctest.h"
+
+#include "model_sandbox.h"
+
+#include "FireDomain.h"
+
+#include <string>
+
+using libforefire::FireDomain;
+using libforefire::FFPoint;
+using ff_test::ModelSandbox;
+
+namespace {
+
+size_t occupiedPropSlots() {
+    size_t used = 0;
+    for (size_t i = 0; i < FireDomain::NUM_MAX_PROPMODELS; i++)
+        if (FireDomain::propModelsTable[i] != 0) used++;
+    return used;
+}
+
+FireDomain* makeDomain() {
+    FFPoint sw(0., 0., 0.);
+    FFPoint ne(1000., 1000., 0.);
+    return new FireDomain(0., sw, ne);
+}
+
+} /* anonymous namespace */
+
+TEST_SUITE("domain ownership") {
+
+TEST_CASE("a destroyed domain releases its propagation model") {
+    ModelSandbox sandbox; // holds the fuel table and restores parameters
+    const size_t before = occupiedPropSlots();
+
+    FireDomain* domain = makeDomain();
+    REQUIRE(domain->addPropagativeLayer("Rothermel"));
+    CHECK(occupiedPropSlots() == before + 1);
+
+    delete domain;
+    // Back to where we started: the slot is free and the model is gone. This
+    // failing means the table fills up and the memory is never returned.
+    CHECK(occupiedPropSlots() == before);
+}
+
+TEST_CASE("building a domain leaves another domain's models alone") {
+    ModelSandbox sandbox;
+
+    FireDomain* first = makeDomain();
+    REQUIRE(first->addPropagativeLayer("Rothermel"));
+
+    size_t firstIndex = FireDomain::NUM_MAX_PROPMODELS;
+    for (size_t i = 0; i < FireDomain::NUM_MAX_PROPMODELS; i++)
+        if (FireDomain::propModelsTable[i] != 0) firstIndex = i;
+    REQUIRE(firstIndex < FireDomain::NUM_MAX_PROPMODELS);
+    const libforefire::PropagationModel* firstModel =
+        FireDomain::propModelsTable[firstIndex];
+
+    // The constructor used to clear the table here.
+    FireDomain* second = makeDomain();
+    CHECK(FireDomain::propModelsTable[firstIndex] == firstModel);
+
+    // And the second domain must not be handed the slot the first is using.
+    REQUIRE(second->addPropagativeLayer("Rothermel"));
+    CHECK(FireDomain::propModelsTable[firstIndex] == firstModel);
+
+    delete second;
+    // Destroying the second domain must not take the first one's model with
+    // it: each releases only what it registered.
+    CHECK(FireDomain::propModelsTable[firstIndex] == firstModel);
+
+    delete first;
+}
+
+TEST_CASE("many domains do not accumulate table entries") {
+    // The leak was linear and unbounded, so a loop is the shape of the check.
+    // Slot occupancy standing still is the observable part of it.
+    ModelSandbox sandbox;
+    const size_t before = occupiedPropSlots();
+
+    for (int i = 0; i < 40; i++) {
+        FireDomain* domain = makeDomain();
+        REQUIRE(domain->addPropagativeLayer("Rothermel"));
+        delete domain;
+    }
+
+    CHECK(occupiedPropSlots() == before);
+}
+
+} /* TEST_SUITE */