NfsimSimulator::run() leaks the whole NFcore::System whenever anything between create_system() and the stepping loop throws

Summary

run() owns the parsed system through a bare pointer: auto *system = impl_->create_system(); (line 1067). The only delete system; on the failure path is inside the try/catch that wraps the stepping loop (line 1132); everything before it is unguarded. That unguarded region includes impl_->prepare_system(system, seed); (line 1068) — which deliberately throws std::runtime_error("NFsim setup failed: " + ...) for post-parse setup failures (the issue-#63 path) — plus times.output_times() (line 1089), result.allocate(...), and resolve_output_functions(...). Any throw there leaks the entire NFcore::System (all MoleculeTypes, molecules, reaction lists). The session path is not affected because initialize() stores into impl_->session_system, which the Impl destructor frees.

Location: src/nfsim_simulator.cpp:1067

Reproduction

Use the scaling form — it proves the leaked block is the NFcore::System rather than fragmentation, and it pairs the leak against a successful-run control. Note in the issue that n_points=-1 is only an instrument to force a throw in the unguarded region (the public Python API rejects it at python/bngsim/_simulator.py:3034); the user-reachable trigger is prepare_system()'s "NFsim setup failed" throw at src/nfsim_simulator.cpp:1068.

import resource, gc
from bngsim import _bngsim_core as C
def rss(): return resource.getrusage(resource.RUSAGE_SELF).ru_maxrss/1e6
xml = "/Users/hlavacek/Code/bngsim/tests/data/nfsim/simple_system.xml"

# Control: successful runs free the System at nfsim_simulator.cpp:1137.
ts_ok = C.TimeSpec(); ts_ok.t_start=0.0; ts_ok.t_end=0.01; ts_ok.n_points=2
s = C.NfsimSimulator(xml); s.run(ts_ok, 42, 0.0)
before = rss()
for _ in range(100): s.run(ts_ok, 42, 0.0)
print("100 successful runs:", before, "->", rss(), "MB")   # 51.2 -> 62.5

# Leak: a throw from the unguarded region (here output_times(), line 1089)
# skips the only delete, which lives in the stepping loop's catch (line 1132).
ts_bad = C.TimeSpec(); ts_bad.t_start=0.0; ts_bad.t_end=10.0; ts_bad.n_points=-1
for lim in (1000, 100000):
    s = C.NfsimSimulator(xml); s.set_molecule_limit(lim)
    try: s.run(ts_bad, 42, 0.0)
    except Exception: pass
    gc.collect(); before = rss()
    for _ in range(50):
        try: s.run(ts_bad, 42, 0.0)
        except Exception: pass
    gc.collect()
    print(f"molecule_limit={lim}: {(rss()-before)/50*1000:.0f} KB leaked per throwing run")
# molecule_limit=1000: 3 KB ; molecule_limit=100000: 5122 KB  -> the leak IS the System

Run with: perl -e 'alarm shift; exec @ARGV' 200 /Users/hlavacek/Code/bngsim/.venv/bin/python <script>

import resource, gc
from bngsim import _bngsim_core as C
xml = "/Users/hlavacek/Code/bngsim/tests/data/nfsim/simple_system.xml"
def rss(): return resource.getrusage(resource.RUSAGE_SELF).ru_maxrss/1e6
# n_points = -1 makes TimeSpec::output_times() throw from the vector ctor at
# nfsim_simulator.cpp:1089 -- i.e. AFTER create_system() has built the System.
ts = C.TimeSpec(); ts.t_start=0.0; ts.t_end=10.0; ts.n_points=-1
s = C.NfsimSimulator(xml)
try:
    s.run(ts, 42, 0.0)
except Exception as e:
    print("first throw:", type(e).__name__, e)
print("rss after 1:", rss(), "MB")
for i in range(300):
    try: s.run(ts, 42, 0.0)
    except Exception: pass
gc.collect()
print("rss after 301:", rss(), "MB")

Observed

first throw: ValueError vector
rss after 1: 51.265536 MB
rss after 301: 1576.124416 MB
rc=0

~5 MB leaked per throwing run() on a 6-observable toy model (tests/data/nfsim/simple_system.xml); the simulator object itself is reused, so nothing else accounts for the 1.5 GB growth.

Expected

run() should own the System with a std::unique_ptrNFcore::System (or extend the try/catch to cover create_system() onward), so that a throw from prepare_system(), output_times(), allocate() or resolve_output_functions() frees it. Today a caller that repeatedly hits the documented "NFsim setup failed" path — e.g. a parameter scan or a PyBNF fit over a model with an unsupported functional rate — grows RSS without bound.

Verification notes

The repro was independently re-run and the analysis re-checked against existing tests, git blame/git log -S and CHANGELOG.md (confidence: high).

The claim survives refutation. I re-ran the repro and reproduced it exactly (51.2 MB -> 1572.1 MB over 301 throwing run() calls, rc=0), then ran two additional tests the original agent did not, both of which strengthen it:

(1) CONTROL — 101 successful run() calls on the same simulator and model grew RSS only 51.2 -> 62.5 MB (~0.1 MB/run, allocator noise). Successful runs do the identical create_system() + prepare_system() work; the only difference is that delete system; executes. This rules out the mundane alternative causes: NFsim global-state accumulation, per-call Result objects, and heap fragmentation.

(2) SCALING — the leak tracks molecule_limit linearly: molecule_limit=1000 leaks 3 KB per throwing run, molecule_limit=100000 leaks 5122 KB per throwing run (~100x for 100x the limit). That identifies the leaked block positively as NFcore::System's preallocated per-MoleculeType molecule arrays, i.e. the System itself. Nothing else on that path scales with molecule_limit.

CODE READING CONFIRMS THE PATH. src/nfsim_simulator.cpp:1067 auto *system = impl_->create_system(); takes ownership in a bare pointer. The try block does not open until the stepping loop; its catch (line ~1132) is commented "Free the parsed System before propagating (no other cleanup hook on this path)" — the author knew the System must be freed on a throw and covered only the loop. Everything between 1067 and the try is unguarded: prepare_system() (1068), times.output_times() (1089), result.allocate(), resolve_output_functions(). The session path is genuinely unaffected (initialize() at 1153 stores into impl_->session_system, freed by the Impl dtor at 780-782 and reset()).

NOT INTENDED. git log -S "delete system;" -- src/nfsim_simulator.cpp returns only 764f4b1 "Initial public release" — no commit deliberately placed the delete there. No test in python/tests/ or tests/ and nothing in CHANGELOG.md asserts this behavior; there is no docstring sanctioning it.

ONE CORRECTION TO THE CLAIM'S FRAMING, which is why I set severity low rather than the claimed medium. The n_points=-1 trigger in the repro is not reachable from the public Python API — python/bngsim/_simulator.py guards it at
… (truncated)

Severity assessed as low. Reviewed against main at commit c3ab2ee.

Start in src/nfsim_simulator.cpp:1067 and trace ownership from create_system() through prepare_system(), output_times(), result.allocate(), and resolve_output_functions(). Run the supplied Python reproduction with tests/data/nfsim/simple_system.xml, noting that python/bngsim/_simulator.py:3034 rejects n_points=-1. Done means throwing setup paths release NFcore::System while successful runs retain their existing behavior.