diff --git a/docs/developer/subsystems/data-access.md b/docs/developer/subsystems/data-access.md index 764d57315..d35e15c8f 100644 --- a/docs/developer/subsystems/data-access.md +++ b/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 diff --git a/src/underworld3/discretisation/discretisation_mesh_variables.py b/src/underworld3/discretisation/discretisation_mesh_variables.py index 09d4a1968..8c95671f0 100644 --- a/src/underworld3/discretisation/discretisation_mesh_variables.py +++ b/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] diff --git a/tests/parallel/mpi_runner.sh b/tests/parallel/mpi_runner.sh index 9ac17118c..c46923666 100755 --- a/tests/parallel/mpi_runner.sh +++ b/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)" diff --git a/tests/parallel/ptest_0011_dm_rebuild_held_handles.py b/tests/parallel/ptest_0011_dm_rebuild_held_handles.py new file mode 100644 index 000000000..63bb800be --- /dev/null +++ b/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) diff --git a/tests/test_0858_dm_rebuild_lifecycle.py b/tests/test_0858_dm_rebuild_lifecycle.py new file mode 100644 index 000000000..fc6c37e75 --- /dev/null +++ b/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()