Release, don't destroy: held DM handles and data views survive the MeshVariable rebuild (#492)

docs/developer/subsystems/data-access.md

@@ -155,6 +155,21 @@ This makes the cache self-healing — no code path that replaces `_lvec` needs t
 
 ```{warning} Variable Creation After Data Access
 Creating new MeshVariables on a mesh triggers a DM rebuild that replaces all existing variables' PETSc vectors. Code that accesses `.data` before all variables are created will get a stale cache automatically healed on the next access — but the old NumPy array reference becomes invalid. Always re-read `.data` after creating new variables.
+
+**The rule (issue #492)**: a raw NumPy view of variable data — anything like
+`view = np.asarray(var.data)` or a kept reference to `var.data` / `var.array`
+— does **not** survive creating another variable on the same mesh. The rebuild
+releases the underlying PETSc vector, so a held view silently reads and writes
+freed memory (no error is raised; on glibc the write corrupts the heap and the
+process crashes much later, at an unrelated allocation). UW3 invalidates every
+cache it hands out (`_canonical_data`, `_data_cache`, `_array_cache`), so
+property access is always safe; raw views captured by user code cannot be
+reached and must be re-read after any variable creation.
+
+Captured PETSc *handles* (`mesh.dm`, `var.vec`) get the gentler contract: since
+the #492 fix the rebuild drops its reference instead of destroying the object,
+so a held handle stays valid — it is merely stale (it describes the pre-rebuild
+layout) and should also be re-read.
 ```
 
 ## Performance Considerations

src/underworld3/discretisation/discretisation_mesh_variables.py

@@ -1691,27 +1691,38 @@ def _setup_ds(self):
             # When we rebuild the DM, existing variables' vectors must be recreated
             # from the new DM, but we need to preserve their data
 
-            # Save old variable data before destroying vectors
+            # Save old variable data, then RELEASE (not destroy) the old
+            # vectors. petsc4py's ``destroy()`` zeroes the handle of the very
+            # wrapper object a user may still hold (``var.vec`` returns the
+            # same wrapper), turning a later call on it into a NULL-handle
+            # dereference — a hard SIGSEGV on an optimized PETSc (issue #492).
+            # Dropping our reference instead lets PETSc refcounting free the
+            # Vec with its last holder: same memory behaviour when nobody
+            # else holds it, a stale-but-valid handle when someone does.
             var_data_backup = {}
             for var in self.mesh.vars.values():
                 if var._lvec is not None:
-                    # Save the data
                     var_data_backup[var.clean_name] = var._lvec.array.copy()
-                    # Destroy old vectors
-                    var._lvec.destroy()
                     var._lvec = None
                 if var._gvec is not None:
-                    var._gvec.destroy()
                     var._gvec = None
 
-            # Also invalidate mesh's local vector if it exists
+            # Release the mesh's combined local vector the same way; it is
+            # rebuilt from the new DM on the next update_lvec().
             if self.mesh._lvec is not None:
-                self.mesh._lvec.destroy()
                 self.mesh._lvec = None
                 self.mesh._stale_lvec = True
 
-            # Replace old DM with new one
-            dm_old.destroy()
+            # Swap in the rebuilt DM. The old DM is deliberately NOT
+            # destroyed (issue #492): ``mesh.dm`` is a plain attribute, so a
+            # user-captured handle is the SAME wrapper object — an eager
+            # destroy blinds it (handle -> 0, SIGSEGV on next use) and frees
+            # the C object while numpy views of the old vectors still alias
+            # its pages (the delayed heap-corruption crash on Linux CI).
+            # Dropping the reference is leak-free: solver-side holders are
+            # clones, so the old DM's last reference is normally this one and
+            # it is collected immediately; measured RSS over repeated
+            # rebuild+solve cycles is identical with and without the destroy.
             self.mesh.dm = dm_new
             self.mesh.dm_hierarchy[-1] = dm_new
 
@@ -1724,11 +1735,14 @@ def _setup_ds(self):
                 if var.clean_name in var_data_backup:
                     # _set_vec will create new vectors from the new DM
                     var._set_vec(available=True)
-                    # Eagerly invalidate cached data array. The .data property also
-                    # self-validates via _lvec identity check, but clearing here avoids
-                    # unnecessary recreation on next access.
+                    # Eagerly invalidate cached data/array views. The .data
+                    # property also self-validates via _lvec identity check,
+                    # but clearing here guarantees UW3 never hands back a view
+                    # of the released vectors (matches _on_mesh_adapted).
                     if hasattr(var, '_canonical_data'):
                         var._canonical_data = None
+                    var._data_cache = None
+                    var._array_cache = None
                     # Restore the data
                     var._lvec.array[...] = var_data_backup[var.clean_name]
 

tests/parallel/mpi_runner.sh

@@ -39,6 +39,11 @@ mpirun -np 3 $PYTHON ./ptest_0010_snapshot_disk.py
 echo "ptest 0010 snapshot on-disk -np 4"
 mpirun -np 4 $PYTHON ./ptest_0010_snapshot_disk.py
 
+echo "ptest 0011 DM rebuild held handles -np 2"
+mpirun -np 2 $PYTHON ./ptest_0011_dm_rebuild_held_handles.py
+echo "ptest 0011 DM rebuild held handles -np 4"
+mpirun -np 4 $PYTHON ./ptest_0011_dm_rebuild_held_handles.py
+
 echo "ptest 0004 checkpoint FMG hierarchy -np 2"
 mpirun -np 2 $PYTHON ./ptest_0004_checkpoint_fmg_hierarchy.py
 echo "ptest 0004 checkpoint FMG hierarchy -np 3 (uneven partition)"

tests/parallel/ptest_0011_dm_rebuild_held_handles.py

@@ -0,0 +1,53 @@
+"""MPI test for the issue #492 DM-rebuild contract.
+
+Run via mpi_runner.sh (mpirun -np N python ptest_0011_*.py).
+
+Creating a second MeshVariable rebuilds ``mesh.dm`` collectively. Asserts on
+EVERY rank:
+  - a handle captured before the rebuild stays valid (stale, not blinded);
+  - the captured wrapper is the old DM's last holder (refcount 1);
+  - the rebuilt DM carries both fields and supports a solve.
+
+Pre-fix, the eager ``dm_old.destroy()`` zeroed the captured wrapper's handle
+on every rank (SIGSEGV on next use — design probe exit -11, issue #492).
+"""
+import numpy as np
+import underworld3 as uw
+
+mesh = uw.meshing.UnstructuredSimplexBox(
+    minCoords=(0.0, 0.0), maxCoords=(1.0, 1.0), cellSize=0.3,
+    regular=False, qdegree=2)
+
+held = mesh.dm
+dim0 = held.getDimension()
+
+v = uw.discretisation.MeshVariable("v1", mesh, mesh.dim, degree=2)
+assert mesh.dm is held, f"rank {uw.mpi.rank}: first variable must not rebuild"
+
+p = uw.discretisation.MeshVariable("p1", mesh, 1, degree=1)
+assert mesh.dm is not held, f"rank {uw.mpi.rank}: second variable must rebuild"
+assert held.handle != 0, f"rank {uw.mpi.rank}: held DM handle was blinded (#492)"
+assert held.getDimension() == dim0
+assert held.getRefCount() == 1, (
+    f"rank {uw.mpi.rank}: expected the captured wrapper to be the last "
+    f"holder, refcount {held.getRefCount()}")
+assert mesh.dm.getNumFields() == 2
+
+# the rebuilt DM must be collectively functional
+poisson = uw.systems.Poisson(mesh, u_Field=p)
+poisson.constitutive_model = uw.constitutive_models.DiffusionModel
+poisson.constitutive_model.Parameters.diffusivity = 1.0
+poisson.f = 0.0
+poisson.add_dirichlet_bc(0.0, "Bottom")
+poisson.add_dirichlet_bc(1.0, "Top")
+poisson.petsc_options["ksp_rtol"] = 1e-8
+poisson.solve()
+
+err = float(np.linalg.norm(p.data[:, 0] - p.coords[:, 1]))
+nrm = float(np.linalg.norm(p.coords[:, 1])) + 1e-30
+# rank-local relative error on an exact linear solution
+assert err / nrm < 1e-8, f"rank {uw.mpi.rank}: solve wrong on rebuilt DM"
+
+if uw.mpi.rank == 0:
+    print(f"OK: held DM handle valid after rebuild on {uw.mpi.size} ranks; "
+          f"solve on rebuilt DM exact", flush=True)

tests/test_0858_dm_rebuild_lifecycle.py

@@ -0,0 +1,172 @@
+"""DM rebuild lifecycle across MeshVariable creation (issue #492).
+
+Creating a MeshVariable on a mesh that already has fields rebuilds ``mesh.dm``
+(the finalized PETSc Section cannot be extended in place through petsc4py).
+Before the fix, ``_setup_ds`` eagerly ``destroy()``-ed the old DM and the old
+variable vectors. petsc4py's ``destroy()`` zeroes the handle of the wrapper
+object itself — and ``mesh.dm`` is a plain attribute, so a user-captured
+handle IS that wrapper. The next call on it was a NULL-handle dereference:
+a hard SIGSEGV on optimized PETSc, verified as subprocess exit code -11 by
+the issue-492 design probes (probe3_stale_holders.py). That crash cannot be
+asserted in CI without segfaulting the test process, so these tests assert
+the observable precondition instead: the held wrapper must keep a non-zero
+handle and keep answering queries. On the unfixed build the handle is zeroed,
+so the asserts below fail cleanly.
+
+The fix drops the reference instead of destroying: the old DM/Vecs die with
+their last holder (PETSc refcounting), so a held handle is stale-but-valid.
+Measured RSS over repeated rebuild+solve cycles is identical with and without
+the eager destroy (design note, scratchpad i492), which the accumulation test
+bounds here.
+"""
+import gc
+import resource
+
+import numpy as np
+import pytest
+import underworld3 as uw
+
+pytestmark = [pytest.mark.level_1, pytest.mark.tier_a]
+
+
+def _box(cellSize=0.3, **kwargs):
+    return uw.meshing.UnstructuredSimplexBox(
+        minCoords=(0.0, 0.0), maxCoords=(1.0, 1.0), cellSize=cellSize,
+        regular=False, qdegree=2, **kwargs)
+
+
+def _solve_poisson(mesh, u, preconditioner=None):
+    poisson = uw.systems.Poisson(mesh, u_Field=u)
+    poisson.constitutive_model = uw.constitutive_models.DiffusionModel
+    poisson.constitutive_model.Parameters.diffusivity = 1.0
+    poisson.f = 0.0
+    poisson.add_dirichlet_bc(0.0, "Bottom")
+    poisson.add_dirichlet_bc(1.0, "Top")
+    poisson.petsc_options["ksp_rtol"] = 1e-8
+    if preconditioner is not None:
+        poisson.preconditioner = preconditioner
+    poisson.solve()
+    return poisson
+
+
+def test_held_dm_handle_survives_variable_creation():
+    """The issue #492 reproducer: a captured ``mesh.dm`` handle must remain
+    valid (stale, but safe to query) after a second variable rebuilds the DM,
+    and the mesh must work on the new DM."""
+    mesh = _box()
+    held = mesh.dm
+    cells = held.getHeightStratum(0)[1]
+
+    v = uw.discretisation.MeshVariable("v1", mesh, mesh.dim, degree=2)
+    # first variable takes the addField fast path — no rebuild
+    assert mesh.dm is held
+
+    p = uw.discretisation.MeshVariable("p1", mesh, 1, degree=1)
+    # second variable rebuilds: mesh.dm is a NEW wrapper ...
+    assert mesh.dm is not held
+    assert mesh.dm.getNumFields() == 2
+    # ... and the held handle is ALIVE (pre-fix: handle == 0, then SIGSEGV
+    # on any call — see module docstring)
+    assert held.handle != 0
+    assert held.getDimension() == 2
+    assert held.getHeightStratum(0)[1] == cells
+
+    # the mesh is fully functional on the rebuilt DM
+    poisson = _solve_poisson(mesh, p)
+    err = np.linalg.norm(p.data[:, 0] - p.coords[:, 1])
+    assert err / (np.linalg.norm(p.coords[:, 1]) + 1e-30) < 1e-8
+
+
+def test_old_dm_released_to_last_holder_no_accumulation():
+    """The mesh must hand the old DM to its remaining holders (refcount 1 =
+    the captured wrapper is the last one), and unheld rebuild cycles must not
+    accumulate memory: dropping the eager destroy is leak-free because the
+    wrapper's own dealloc frees the object when nobody else kept it."""
+    mesh = _box()
+    u0 = uw.discretisation.MeshVariable("u0", mesh, 1, degree=1)
+
+    held = mesh.dm
+    u1 = uw.discretisation.MeshVariable("u1", mesh, 1, degree=1)
+    assert held.handle != 0
+    # the mesh released its reference: the captured wrapper is the last holder
+    assert held.getRefCount() == 1
+    del held
+    gc.collect()
+
+    # unheld cycles: each creation rebuilds the DM; nothing may accumulate.
+    # Bound set from measurement on this loop (see test docstring note below);
+    # a leaked DM per cycle would blow through it immediately.
+    rss0 = resource.getrusage(resource.RUSAGE_SELF).ru_maxrss / 1024**2
+    for i in range(20):
+        uw.discretisation.MeshVariable(f"w{i}", mesh, 1, degree=1)
+        gc.collect()
+    rss1 = resource.getrusage(resource.RUSAGE_SELF).ru_maxrss / 1024**2
+    assert rss1 - rss0 < 50.0, f"RSS grew {rss1 - rss0:.1f} MB over 20 rebuilds"
+
+
+def test_data_views_are_refreshed_after_rebuild():
+    """UW3 must never hand back a view of the released vectors: after a
+    rebuild, ``.data`` returns a fresh buffer carrying the preserved values.
+    (A RAW numpy view captured by user code before the rebuild cannot be
+    reached — data-access.md documents that it does not survive variable
+    creation and must be re-read.)"""
+    mesh = _box()
+    u1 = uw.discretisation.MeshVariable("u1", mesh, 1, degree=1)
+    u1.data[:, 0] = 42.0
+    stale_view = np.asarray(u1.data)
+
+    uw.discretisation.MeshVariable("u2", mesh, 1, degree=1)
+
+    fresh = u1.data
+    # Object identity only — never np.shares_memory here: the released buffer
+    # is freed before the replacement allocates, so the allocator may recycle
+    # the same block and make an address comparison fail spuriously on
+    # exactly the platform this guards (#536 review). Dereferencing
+    # stale_view is likewise out: it dangles by the documented contract.
+    assert fresh is not stale_view
+    assert np.all(np.asarray(fresh)[:, 0] == 42.0)  # data preserved across rebuild
+    # round-trip on the new buffer
+    u1.data[:, 0] = 7.0
+    assert np.all(np.asarray(u1.data)[:, 0] == 7.0)
+
+
+@pytest.mark.level_2
+def test_adapt_child_second_variable_after_solve():
+    """The PR #488 CI detonation shape (test_0842-shaped, 2-D for speed):
+    on an adapt child, variable -> solve -> SECOND variable -> second solve,
+    then full teardown. Pre-fix this armed a use-after-free that crashed
+    Linux CI two test files later; post-fix the old DM survives as long as
+    anything holds it and teardown is clean."""
+    pytest.importorskip(
+        "underworld3.utilities._nvb_transform",
+        reason="native uwnvb transform not built (needs the custom-PETSc/amr env)")
+
+    mesh = _box(cellSize=0.3, refinement=1)
+
+    def metric(centroids):
+        r = np.linalg.norm(np.asarray(centroids) - 0.5, axis=1)
+        h = np.where(r < 0.18, 0.04,
+                     np.minimum(0.04 + (0.3 - 0.04) * (r - 0.18) / 0.25, 0.3))
+        return 1.0 / h**2
+
+    child = mesh.adapt(metric, max_levels=1)
+
+    u1 = uw.discretisation.MeshVariable("u1", child, 1, degree=1)
+    # fmg builds the custom-P coarse chain — the original detonation had the
+    # FMG hierarchy live on the child when the rebuild fired (#536 review:
+    # under "auto" a single-field solver declines to GAMG and the chain this
+    # test exists to exercise is never constructed).
+    _solve_poisson(child, u1, preconditioner="fmg")
+
+    held = child.dm  # captured post-solve, pre-rebuild (the arming step)
+    u2 = uw.discretisation.MeshVariable("u2", child, 1, degree=1)
+    assert child.dm is not held
+    assert held.handle != 0 and held.getDimension() == 2
+
+    poisson2 = _solve_poisson(child, u2)
+    err = np.linalg.norm(u2.data[:, 0] - u2.coords[:, 1])
+    assert err / (np.linalg.norm(u2.coords[:, 1]) + 1e-30) < 1e-8
+
+    # teardown ordering from the CI story: solver, then meshes, then gc
+    del poisson2, held, u1, u2, child, mesh
+    gc.collect()