Fix uninitialized and leaked libRoadrunner instance handle

[drbergman]

Fix uninitialized and leaked libRoadrunner instance handle

RoadRunnerIntracellular::rrHandle is declared with no initializer, clone() returns a model with no RoadRunner instance, and nothing ever frees one.

A cell that changes type gets a handle that was never created. Only start() creates the instance, and it is called from exactly two places — Cell::divide() and create_cell( Cell_Definition& ). Cell::convert_to_cell_definition() assigns a phenotype and does not, so every cell transformation leaves the cell passing indeterminate memory to the RoadRunner C API.

Nothing is ever freed. freeRRInstance appears nowhere in the tree and the class declares no destructor, so every cell death and every conversion permanently orphans one instance.

The fix

• clone() calls start(), so it returns a usable model. This matches MaBoSS and dFBA, which both initialize in their own copy constructors — dFBAIntracellular::start() is empty for exactly that reason.
• A destructor frees rrHandle and result.
• rrHandle gets an in-class initializer, which covers the pointer-taking constructor too.
• Copy construction and assignment are deleted; with an owning destructor the implicit ones would double-free.

Because clone() is now self-sufficient, both existing start() calls in core/PhysiCell_cell.cpp are redundant and removed.

Verification

The stock ode_energy sample exercises neither bug — it has no cell death and no type conversion. Extended with a second cell definition carrying the same <intracellular> block, transformations in both directions, and matched birth/death holding ~144 agents:

scenario	before	after
no conversion, no death	127.9 MB, exit 0	121.4 MB, exit 0
conversions enabled	crash 8/8	clean 6/6
death only, 2880 min	530.7 / 578.6 MB	109.1 / 108.8 MB

Also included: unknown SBML species

get_parameter_value() / set_parameter_value() indexed species_result_column_index with operator[], so an unknown species silently resolved to column 0 — reading, or writing, a different species than the caller named. Both now use find() and exit, as validate_SBML_species() already does at setup. See this comment.

[Copilot]

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

[drbergman]

@rheiland I added these correctness checks in this commit onto PR #425. The rest of this PR should make this obsolete, but before a wrong species name would be given the default value (0) which meant it would do stuff to/with index 0 rather than throw.

[vincent-noel]

It looks good, and I had also noticed this lack of destructor.
However, looking at the test results, it does change something in PhysiBoSS results. I would keep this on hold until I have some time to understand the difference.

[drbergman]

Thanks, @vincent-noel ! Claude did find something along these lines:
https://drbergman.github.io/physicell_pr_prep_2608/#:~:text=under%20cell%20turnover.-,CONCERNS%20FOUND%20IN%20THE%20DIFF,-CI%20is%20red

Happy to continue working on this with you

[vincent-noel]

After having a quick look, and trying to remember my thought process when I initially wrote this:
There are two classes which have intracellular objects: Cell and Cell_Definition.

In Cell_Definition, we won't do actual simulation, so we can have a minimal instantiation (no MaBoSS or RoadRunner object basically).
So I created two functions:

• clone() do the minimum
• start() actually finish the instantiation to be ready to simulate

But now that I think about it, no cell definition is created by clone(), only cells. Maybe this was the case back when we had cell definition inheritance ? So maybe it is ok to merge them.

Also, one thing I saw when testing now: Intracellular is cloned twice at cell creation, because phenotype is copied twice

• First in the Cell constructor, from the default cell definition
• Then in create_cell(Cell_Definition), from the cell definition in argument

It is already suboptimal, and we could think of fixing this by removing the initialization as default. But in the specific context of the clone() and start() discussion, if start() and clone() are merged now, we would end up calling it twice.

[drbergman]

So then as I see it, here's the simplest path forward:

1. fixing double-clone: pre-existing behavior; suboptimal as it does unnecessary work; fix in separate PR targeted to that. Action: for now, open Issue on it to document
2. clone(): It is only called in the = operator of Phenotype, meaning we need to work hard to find where it is called; if we call start() within clone(), then worst case, we have cell defs that hold more in memory than we strictly need of them. Action: call start() within dFBA and MaBoSS clone() calls.
3. overloading =: I'm not a fan of this. It makes it hard to track down where the overload is being used and to identify the side effects baked in to an = call. I'd rather have a constructor built for this or, if we need an instance first before setting values, having a dedicated method. If we make this a function pointer, we could even have the Cell_Definition class override this to a state where it does the limited clone() and not the full start(). Action: None. Feel free to critique this idea.