Conversation
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
Collaborator
|
Looks good: fixes the leak and the cross-domain clobbering, tests cover both. Minor, non-blocking: same missing "table full" check in |
Collaborator
Author
|
Noted for |
This was referenced Sep 26, 2026
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #159.
A
FireDomainwith a propagation layer leaked ~183 kB each time, with no upper limit (500 domains: +91.5 MB RSS before, +180 kB after).The fix is not just the missing
delete. The constructor also wiped the static model tables, so building a second domain (Command.cpp:205does this in coupled runs) left the first domain with a null slot, or with the second domain's model, which silently gives wrong spread rates.getFreePropModelIndex/getFreeFluxModelIndex, and a leaked array inDataBroker::addConstantLayer.Tested: new
tests/unit/test_domain_ownership.cpp(each half of the fix has a negative control). Unit tests 5/5.runffmatches. ASan leaks drop from 4.7 MB to 0, sosanitizers.ymlnow runs withdetect_leaks=1and blocks on leaks (unblocks #162).Code and description generated with Claude Opus 5, reviewed before submitting.