Skip to content

Give the propagation models and layers an owner - #182

Open
HugoFara wants to merge 1 commit into
devfrom
fix/propagation-layer-ownership
Open

Give the propagation models and layers an owner#182
HugoFara wants to merge 1 commit into
devfrom
fix/propagation-layer-ownership

Conversation

@HugoFara

Copy link
Copy Markdown
Collaborator

Closes#159. Building and destroying a FireDomain with a propagation layer cost about 183 kB every time, linearly and without limit.

Measured over 500 domains, each attaching a Rothermel layer:

RSS growthoccupied model slots
before91,504 kBnever released
after180 kBback to zero after each domain

The part that is not a leak

While reproducing this I found the reason nobody could simply add the missing delete: the FireDomain constructor wiped both static model tables.

for ( size_t i = 0; i < NUM_MAX_PROPMODELS; i++ ) propModelsTable[i] = 0;
for ( size_t i = 0; i < NUM_MAX_FLUXMODELS; i++ ) fluxModelsTable[i] = NULL;

propModelsTable is static and shared by every domain in the process, so building a second domain dropped the first one's registrations. Demonstrated:

steppropModelsTable
A created + layer49 = 0x3389c890
B created, no layer yetempty — A's model is gone
B created + layer49 = 0x33c80b30

A's PropagativeLayer still holds index 49 throughout. Between the second and third rows that index is null, and FireDomain.cpp:1349 dereferences it without checking. After the third row it resolves to B's model, so A propagates using another domain's parameters — silently, with wrong spread rates rather than a crash.

Command.cpp:205 builds exactly that second domain for coupled runs.

The wipe is gone. Both tables are static storage and already start out null, and each domain now records the entries it registered and releases exactly those — before the data broker, since the models hold a pointer to it.

Also on this path

  • getFreePropModelIndex counted down through an unsigned index with no lower bound. A full table wrapped to SIZE_MAX and read far out of bounds. getFreeFluxModelIndex bound-checked but then returned an occupied index, silently overwriting a live model. Both now report and return an out-of-range value their callers refuse. This was reachable only once the tables could fill up, which is to say: only after this change.
  • DataBroker::addConstantLayer allocated an array, handed it to a layer that copies it, and dropped it.FFArray's matrix constructor does data = new T[size] and copies element by element, so the caller's array is never adopted. Both delete[] data; lines were sitting there commented out, one per branch.

The leak gate is now on

This is what #162 was waiting for. Under ASan the unit suite goes from 4.7 MB leaked to none, so sanitizers.yml now runs it with detect_leaks=1 as a blocking check.

runff still leaks ~200 kB over 783 allocations, on paths a full simulation reaches and the unit suite does not, so its leak check stays informational. Closing those is what would let it join the gate.

Verification

  • Unit suite 5/5, including tests/unit/test_domain_ownership.cpp (new): the release, the cross-domain clobbering, and a 40-domain loop.
  • Negative controls, one per half of the fix. Restoring the constructor wipe fails the clobbering case; removing the release call fails the slot case. Neither passes with the bug present.
  • runff: KML and NetCDF both match within tolerance.
  • ASan in ubuntu:24.04: zero errors, unit suite leak-free, runff passing.

One test changed shape

every model can be destroyed used to delete model by hand. That is a double free now that the domain owns them, so each model takes its own sandbox and destroying the sandbox is the destruction under test — which is how destruction actually happens in a run. Its comment claiming "nothing deletes a model in a normal run" was true when written and is not any more.


This pull request, including its code changes and this description, was generated by Claude Opus 5, and reviewed manually before submitting.

Building and destroying a FireDomain with a propagation layer cost about
183 kB every time, linearly and without limit. Two things were missing an
owner, and a third made ownership impossible.
The propagative layer was never freed: the delete in ~FireDomain was
commented out and the pointer nulled, which also removed any chance of a
later cleanup finding it.
The models were never freed either. propModelsTable and fluxModelsTable
are static, shared by every domain in the process, and nothing cleared
them. Each domain now records the entries it registered and releases
exactly those, before the data broker goes, since the models hold a
pointer to it.
The reason that could not simply be added: the FireDomain constructor
wiped both tables. Building a second domain therefore dropped the first
one's registrations, and its PropagativeLayer was left holding an index
that was empty until the new domain registered — at which point it
resolved to the *second* domain's model. Command.cpp builds exactly that
second domain for coupled runs. The wipe is gone; both tables are static
storage and already start out null.
Two smaller things on the same path:
- getFreePropModelIndex counted down through an unsigned index with no
lower bound, so a full table wrapped to SIZE_MAX and read far out of
bounds. getFreeFluxModelIndex bound-checked but then returned an
occupied index, silently overwriting a live model. Both now report
and return an out-of-range value that their callers refuse.
- DataBroker::addConstantLayer allocated an array, handed it to a layer
that copies it, and dropped it. Both delete[] lines were
sitting there commented out, one per branch.
Measured over 500 domains, each with a Rothermel layer: RSS growth falls
from 91,504 kB to 180 kB, and occupied model slots from perpetually
climbing to zero after each domain. Under ASan the unit suite goes from
4.7 MB leaked to none, so the leak check is now blocking for it; runff
still leaks ~200 kB on paths the suite does not reach and stays
informational.
tests/unit/test_domain_ownership.cpp covers the release, the
cross-domain clobbering and the loop. The model registry tests no longer
delete models by hand, which is a double free now that the domain owns
them; each takes its own sandbox instead, so destruction is exercised
the way it actually happens.
Closes#159
@HugoFara
HugoFara requested a review from filippiAugust 12, 2026 20:35
Sign up for freeto 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.

1 participant

@HugoFara