fix(expr): give every evaluator its own ExprTk parser (#257)

CHANGELOG.md

@@ -293,6 +293,72 @@ in `CMakeLists.txt`) is derived from it.
   Ships as vendored-NFsim carry `bngsim/carry-symmetry-factor-all-rate-laws`;
   candidate to push upstream, where the defect is ~14 years old and untouched
   on `RuleWorld/nfsim` master.
+- **A parallel fan-out could still die with no diagnosis: clones shared one
+  ExprTk parser, and a parser keeps a strong handle on the last symbol table
+  compiled through it (issue #257).** #201 found two threads compiling through
+  one `exprtk::parser` and serialized `compile()`. The regression test it added
+  then failed about **10% of the time on `main`** — 2 of 20 consecutive runs,
+  and it took a `git push` down through the pre-push hook once. The residue was
+  not the `compile()` race: the macOS crash report puts the faulting thread in
+  `NetworkModel::clone()` and another, concurrently, in
+  `ExprTkEvaluator::evaluate()` under `CVode` with the GIL released — an
+  evaluator being *read* against an evaluator being *constructed*, which a mutex
+  around `compile()` cannot cover.
+
+  The mechanism is one line of ExprTk. `parser::compile()` ends with
+
+      symtab_store_.symtab_list_ = expr.get_symbol_table_list();
+
+  and never clears it, so a parser retains a *strong handle* on the symbol table
+  of the last expression compiled through it — and ExprTk refcounts symbol
+  tables with a plain `std::size_t`. One parser behind two evaluators therefore
+  means thread B's compile dropping thread A's symbol table while thread A is
+  churning that same counter with no lock at all: in `register_symbol_table`,
+  in the growth of its expression vector, in its own destructor. A lost update
+  runs `clear()` on a symbol table whose variable addresses are already baked
+  into A's live compiled nodes. The failure is a corrupt heap, so it surfaces
+  wherever the *next* allocation looks — the reported signature is `SIGTRAP`
+  from the macOS malloc freelist check, which is none of the three signals #201
+  was calibrated against.
+
+  No lock could have closed it, because the counter is touched on paths no
+  evaluator API sees. So the sharing is gone instead: **an `ExprTkEvaluator` now
+  owns all of its ExprTk state and shares none of it** — parser, symbol table,
+  expression list, function adapters — and `clone_empty()` hands back an
+  evaluator that inherits nothing. The mutex #201 added is gone with it.
+
+  The performance argument for sharing turned out to be illusory. It existed to
+  avoid re-constructing the ~100 KB `exprtk::parser<double>` (51 µs, measured)
+  per model clone — but `NetworkModel copy;` default-constructs an evaluator that
+  `clone()` replaces one line later, so that construction was being paid and
+  thrown away on every clone anyway. The parser is now built lazily on first
+  compile, which makes the discarded evaluator free, and the clone that needs a
+  parser builds exactly the one it uses. `NetworkModel::clone()`, before → after:
+
+  | model | before | after |
+  |---|---:|---:|
+  | `BIOMD0000000701.xml` (71 params, events) | 521 µs | 527 µs |
+  | `func_composition.net` | 92 µs | 94 µs |
+  | `simple_decay.net` (no expressions) | 67 µs | **34 µs** |
+
+  Unchanged where the clone compiles anything, and halved where it compiles
+  nothing — that model used to construct a parser only to discard it.
+
+  Every `run_batch` / `parameter_scan` / `compute_all_sensitivities` fan-out over
+  a model with events was on this path, and the failure was a silent process
+  death rather than an exception — a long job lost a worker with nothing to read.
+  Two new cases in `tests/test_bngsim.cpp` cover it, both run against the pre-fix
+  design before being trusted: the invariant directly (a clone's parser is not
+  its source's, via the new `ExprTkEvaluator::parser_identity()`), and the race
+  it prevents (8 threads cloning, compiling and evaluating). Rebuilt on the old
+  shared-parser evaluator, the test binary died in **8 of 8** runs, always inside
+  that second case, always on the `SIGTRAP`; a standalone harness at the same
+  parameters put the rate at 19 of 20 over a wider sample and turned up `SIGSEGV`
+  and `SIGBUS` as well. On the fix, 20 of 20 clean.
+
+  `python/tests/test_expression_parser_thread_safety.py` — the #201 test that was
+  flaking, 18 of 20 on `main` — is **25 of 25** clean on the fix, and its failure
+  message now says which of the two defects a given signal would mean.
 - **A parameter that a same-named function shadows was still listed as a knob
   (issues #256 and #266 — the same defect reported from the SBML side and the
   `.net` side).** Every function gets a parameter slot to hold its evaluated

include/bngsim/expression.hpp

@@ -62,6 +62,34 @@ class ExpressionEvaluator {
 //   if(cond, true_val, false_val), mratio(a, b, z), time()
 // Note: `t` is intentionally NOT registered (BNG2.pl uses time() only),
 // leaving `t` free as a model identifier.
+//
+// ─── THREAD SAFETY (issues #201, #257) ───────────────────────────────────────
+//
+// An ExprTkEvaluator owns ALL of its ExprTk state and shares NONE of it with any
+// other evaluator — parser, symbol table, expression list, function adapters.
+// That is a deliberate design decision, not an accident of implementation, and
+// `parser_identity()` exists so it can be asserted rather than merely believed.
+//
+// The evaluator itself is *not* internally synchronized: it is per-model-instance
+// state, and a NetworkModel is not thread-safe (every fan-out clones — see the
+// THREAD SAFETY note in model.cpp and
+// python/tests/test_parallel_workers_clone_the_model.py). One thread per
+// evaluator is the contract; concurrent compile()/evaluate() on ONE evaluator is
+// a caller bug that no lock in here would make correct anyway, because
+// compile() grows `expressions` under an evaluate() that is reading it.
+//
+// What made the shared parser unsalvageable rather than merely lock-hungry:
+// exprtk::parser::compile() ends with
+//     symtab_store_.symtab_list_ = expr.get_symbol_table_list();
+// and never clears it, so a parser keeps a *strong handle* on the symbol table of
+// the last expression compiled through it — and exprtk's symbol_table refcount is
+// a plain std::size_t. With one parser behind two evaluators, thread B's next
+// compile drops thread A's symbol table while thread A is still incrementing and
+// decrementing that same counter outside any lock (register_symbol_table, growth
+// of the expression vector, destruction). A lost update frees the symbol table's
+// data underneath the variable pointers already baked into A's compiled nodes.
+// #201's mutex serialized compile() and could not have covered that, which is why
+// the regression test it added stayed ~10% flaky until #257.
 class ExprTkEvaluator : public ExpressionEvaluator {
   public:
     ExprTkEvaluator();
@@ -89,15 +117,28 @@ class ExprTkEvaluator : public ExpressionEvaluator {
     // Must be called after construction, before compiling time-dependent expressions.
     void set_time_ptr(double *time_addr);
 
-    // ─── Efficient clone support ─────────────────────────────────────────
+    // ─── Clone support ───────────────────────────────────────────────────
     //
-    // clone_empty() creates a new evaluator that shares the heavyweight
-    // ExprTk parser with this evaluator, but has its own symbol table and
-    // expression list. The caller must register variables and compile
-    // expressions on the returned evaluator.
+    // clone_empty() creates a new evaluator that shares NOTHING with this one:
+    // its own parser, symbol table, expression list and function adapters. The
+    // caller must register variables and compile expressions on the returned
+    // evaluator (NetworkModel::clone() does, from the cached preprocessed
+    // strings this one exposes via preprocessed_expr()).
     //
-    // This avoids re-constructing the ~100KB exprtk::parser<double>
-    // template object on every model clone.
+    // It used to hand the new evaluator this one's exprtk::parser<double>, to
+    // save re-constructing the ~100KB template object per model clone. Issue
+    // #257 retired that: the parser retains a strong handle on the last symbol
+    // table compiled through it (see the class comment), so sharing it couples
+    // the lifetime of one evaluator's symbol table to another evaluator's next
+    // compile, across threads, through a non-atomic refcount.
+    //
+    // The saving was illusory in any case. `NetworkModel copy;` default-
+    // constructs an evaluator before clone() overwrites it, so the "avoided"
+    // parser construction was being paid and thrown away on every clone. The
+    // parser is now built lazily on first compile, which means that discarded
+    // evaluator never builds one: measured, a clone pays the same single ~51 µs
+    // parser construction it always did, and a model with no expressions at all
+    // now pays none.
     std::unique_ptr<ExprTkEvaluator> clone_empty() const;
 
     // compile_preprocessed() compiles an already-preprocessed expression
@@ -113,12 +154,17 @@ class ExprTkEvaluator : public ExpressionEvaluator {
     // Number of compiled expressions.
     int n_expressions() const;
 
+    // Identity of this evaluator's exprtk::parser, or nullptr if it has not
+    // compiled anything yet (the parser is constructed lazily on first
+    // compile). Two distinct evaluators must never report the same non-null
+    // value — that is the whole of the issue #257 invariant, and this accessor
+    // is what makes it testable instead of merely documented. Not for
+    // dereferencing: the value is an identity, and the type says so.
+    const void *parser_identity() const;
+
   private:
     struct Impl;
     std::unique_ptr<Impl> impl_;
-
-    // Private constructor for clone_empty() — shares parser
-    explicit ExprTkEvaluator(std::shared_ptr<void> shared_parser);
 };
 
 // ─── Reserved names introspection ────────────────────────────────────────────

python/tests/test_expression_parser_thread_safety.py

@@ -1,17 +1,33 @@
-"""Issue #201 — the ExprTk parser is shared across clones and must be serialized.
+"""Issues #201 and #257 — no two model clones may share ExprTk state.
 
-``ExprTkEvaluator::Impl`` shares one ``exprtk::parser`` across every clone of a
-model, to avoid re-constructing the ~100KB template object per clone. The comment
-justifying that said the parser "is stateless between compile() calls", which is
-true *between* sequential calls and false *during* one: ``compile()`` drives a
-lexer, a token scanner and an error list that all live in the parser object.
+``ExprTkEvaluator::Impl`` used to share one ``exprtk::parser`` across every clone
+of a model, to avoid re-constructing the ~100KB template object per clone. The
+comment justifying that said the parser "is stateless between compile() calls",
+which is true *between* sequential calls and false *during* one: ``compile()``
+drives a lexer, a token scanner and an error list that all live in the parser
+object.
 
 Two threads compiling through one parser therefore corrupt each other. The
 observed failures on ``compute_all_sensitivities``'s default fan-out were 0 of 8
 runs surviving — SIGSEGV, SIGABRT and SIGBUS, plus an ``ERR244 - Expected ')'``
 which is ExprTk's way of reporting an *unregistered symbol* when one compile's
 symbol resolution is clobbered by another.
 
+**#201's fix — a mutex around compile() — was necessary and not sufficient, and
+this test measured the difference.** It went on failing about 10% of the time
+(2 of 20 consecutive runs on ``main``), with a *fourth* signature: SIGTRAP, the
+macOS malloc freelist check, raised in ``NetworkModel::clone()`` while another
+thread sat in ``ExprTkEvaluator::evaluate()`` under ``CVode``. That thread was
+not compiling, so no lock on ``compile()`` was ever going to cover it. The cause
+(issue #257) is that ``exprtk::parser::compile()`` ends by copying the compiled
+expression's symbol tables into the parser and never clears them, so a shared
+parser holds a strong handle on one clone's symbol table and drops it from
+whichever thread compiles next — through a refcount that is a plain
+``std::size_t``. Clones now share no ExprTk state at all, and this test went from
+18 of 20 to 25 of 25. The C++ side of the coverage is
+``test_evaluator_clone_shares_no_exprtk_state`` and
+``test_concurrent_clone_compile_evaluate`` in ``tests/test_bngsim.cpp``.
+
 **Why it needs an integration running, not just concurrent clones.** Concurrent
 ``clone()`` alone cannot trip it — the GIL serializes it (1200 concurrent clones of
 this very model are clean). The second compiler has to be running with the GIL
@@ -21,20 +37,23 @@
 another compiles inside ``clone()`` — which is why this reproduces on an *events*
 model under a parallel sensitivity job and nowhere else.
 
-**The subprocess, and why there is no in-process companion.** Half the failure
+**The subprocess, and why the in-process companion is in C++.** Half the failure
 modes are a signal, not an exception, so an in-process assertion cannot observe
 them — a SIGSEGV takes the whole pytest session down and reports nothing useful.
 Each trial therefore runs in its own process and the assertion is on the exit
-status. A faster in-process test of the same mechanism (clone on one thread while
-another integrates) was written and then **deleted**: rebuilt without the lock it
-still passed at 400 iterations, so it could not fail, and a test that cannot fail
-is worse than no test — it reads as coverage. What survives is calibrated: 6 of 6
-trials die on the unlocked build (SIGSEGV, 3x SIGABRT, SIGBUS, one ERR247).
+status. A faster in-process *Python* test of the same mechanism (clone on one
+thread while another integrates) was written and then **deleted**: rebuilt without
+the lock it still passed at 400 iterations, so it could not fail, and a test that
+cannot fail is worse than no test — it reads as coverage. The GIL is why: it
+serializes the clone-side compiles, leaving only the handful of GIL-free lazy
+memos per run to collide. The #257 companion is in C++ for exactly that reason —
+no GIL, so 8 threads collide freely and the pre-fix design dies in 8 of 8 runs
+rather than 1 in 10.
 
 **The repeat count.** A race that has been fixed and a race that merely did not
 fire look identical in one run. Six trials is not proof either; it is calibrated
 against the measured pre-fix rate of 0 survivors in 8, where P(6 false passes) is
-negligible. The real guarantee is that the parser now travels with its mutex.
+negligible. The real guarantee is that no ExprTk state crosses a clone boundary.
 """
 
 from __future__ import annotations
@@ -88,7 +107,9 @@ def test_parallel_compute_all_sensitivities_survives_its_default_fanout():
             how = f"signal {-p.returncode}" if p.returncode < 0 else f"exit {p.returncode}"
             failures.append(f"trial {trial}: {how}\n{p.stderr[-700:]}")
     assert not failures, (
-        f"{len(failures)} of {_TRIALS} parallel sensitivity runs died — the shared "
-        f"ExprTk parser is being compiled through by more than one thread (issue "
-        f"#201).\n\n" + "\n\n".join(failures)
+        f"{len(failures)} of {_TRIALS} parallel sensitivity runs died — ExprTk state "
+        f"is crossing a model-clone boundary. A signal here means memory, not logic: "
+        f"SIGSEGV/SIGABRT/SIGBUS is a parser compiled through by more than one thread "
+        f"(issue #201), SIGTRAP is the macOS malloc freelist check firing on a heap "
+        f"already corrupted by a shared symbol table (issue #257).\n\n" + "\n\n".join(failures)
     )

python/tests/test_parallel_workers_clone_the_model.py

@@ -1,7 +1,13 @@
 """Every parallel worker integrates its OWN model clone — the invariant #201 rests on.
 
-Issue #201 fixed a race on the one object clones genuinely share, the ExprTk parser.
-The audit that accompanied it found no second race, but the reason was
+Issue #201 fixed a race on the one object clones then shared, the ExprTk parser, by
+serializing ``compile()``. Issue #257 found the rest of that race — a parser also
+retains a handle on the last symbol table compiled through it, which no lock on
+``compile()`` could cover — and removed the sharing entirely, so clones now share no
+ExprTk state at all. The invariant below is what makes *that* sufficient rather than
+merely necessary, and it is the one this test holds.
+
+The audit that accompanied #201 found no second race, but the reason was
 circumstantial: ``NetworkModel::state_switch`` and ``event_trigger_residual_expr``
 are lazy memos — ``const`` methods that compile into ``impl_->evaluator`` on first
 use — and they are safe *only* because every thread fan-out in the library hands