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

[lmoresi]

Release, don't destroy: MeshVariable DM rebuild keeps captured handles valid

Fixes#492.

What was wrong

Creating a MeshVariable on a mesh that already has fields rebuilds mesh.dm
(a finalized PETSc Section cannot be extended in place through petsc4py — we
verified addField + clearDS + createDS leaves the stale Section
installed and setLocalSection(None) is not bindable). The rebuild itself is
fine; the defect was that _setup_ds eagerly destroy()-ed the old DM and
the old variable vectors. That produced two distinct crash classes:

1. The blinded wrapper (the reported SIGSEGV).mesh.dm is a plain
   attribute, so a user-captured handle is the same petsc4py wrapper object
   the rebuild called .destroy() on. petsc4py zeroes that wrapper's handle,
   and the next call on it is a NULL-handle dereference — an immediate
   segfault on optimized PETSc (validity macros compiled out). Reproduced
   deterministically as subprocess exit −11 in the design probes. There was
   no user-side defence: even a pre-emptive incRef() leaves the wrapper
   blinded (and leaks the DM).

2. Stale views over freed vector buffers (the delayed CI detonation).
   .data is a numpy view into _lvec.array. The rebuild destroyed every
   variable's vectors, so any view captured beforehand silently read and
   wrote freed pages. On glibc a stale write corrupts allocator metadata and
   the process dies much later at an unrelated allocation — the deterministic
   Linux-only segfault that hit PR Adapt-on-top: closure-free edge_split engine, reconnection repair, and interface-pinned relaxation #488's CI in test_0844, two files after
   the arming in test_0842 (macOS was allocator-lucky; Guard Malloc was
   clean because the pages were genuinely freed and legitimately reused).

What the holder inventory found (and exonerated)

We instrumented every candidate captor of the old DM. All PETSc-side holders
are properly reference-counted and none of them hold mesh.dm at all:
solvers clone the hierarchy (clone_dm_hierarchy), so SNES/KSP/PCMG level
DMs are clones; the adapt-child coarse chain (_custom_mg_coarse_meshes)
holds Python Mesh objects whose coarse/fine links live on the solver
clones — the suspected dangling coarse-chain links never dangled. The
DMInterpolation cache stores no DM (it is passed per call). Pre-solve the old
DM's refcount is exactly 1 (the mesh's own wrapper); after a solve it is 2
(mesh._lvec). The eager destroy therefore genuinely freed the C object
while Python-side aliases (the user's wrapper, numpy views) still pointed at
it.

The fix

• _setup_ds now drops its references instead of destroying: the old DM,
  the old variable _lvec/_gvec, and mesh._lvec die with their last
  holder via PETSc refcounting. A captured handle stays valid — stale, but
  safe to query — which is the contract the issue asked for ("at worst a
  wrong answer from an out-of-date object, never a crash").
• The rebuild's restore loop now clears _data_cache/_array_cache
  alongside _canonical_data (matching the mesh.adapt path), so UW3 can
  never hand back a view of the released vectors.
• mesh._lvec release composes with the existing lazy rebuild:
  update_lvec() recreates it from the new DM on next access.

Why this does not leak

With the destroy gone, the old wrapper loses its last Python reference at the
end of _setup_ds and petsc4py's dealloc frees the C object — unless
someone still holds it, which is the point. Measured RSS over 12
rebuild+solve cycles: baseline 305.6→327.8 MB, no-destroy 303.7→325.9 MB —
identical +22.2 MB growth (solver/JIT machinery, in both modes). A
rebuild-only loop grows 7.1 MB over 20 cycles (the 20 real variables' FE
bookkeeping), bounded at 50 MB in the regression test.

Contract change (documented in data-access.md)

• Raw numpy views (np.asarray(var.data), kept var.data/var.array
  references) do not survive creating another variable on the same mesh —
  UW3 cannot reach them; re-read after any variable creation. Property access
  is always safe (self-validating cache, now backed by eager invalidation).
• Captured PETSc handles (mesh.dm, var.vec) get the gentler contract:
  they stay valid but stale, and should also be re-read.

Tests

• tests/test_0858_dm_rebuild_lifecycle.py (level_1/tier_a; adapt-child case
  level_2): the Creating a MeshVariable destroys the previous mesh.dm; a held handle segfaults (use-after-free) #492 reproducer (held handle survives + solve works on the
  rebuilt DM), refcount hygiene + 20-cycle RSS bound, view refresh, and the
  test_0842-shaped adapt-child arm/solve/teardown sequence. Negative
  control: all three lifecycle tests fail cleanly on the unfixed build
  (held handle zeroed — assert 0 != 0), pass post-fix. The pre-fix SIGSEGV
  itself (probe exit −11) is cited in the docstring rather than executed in
  CI.
• tests/parallel/ptest_0011_dm_rebuild_held_handles.py (+ mpi_runner.sh
  entries): the reproducer contract on every rank; verified np2 and np4.
• Verification runs: 0842+0844 pair 53 passed; fault set 0845–0848 47 passed;
  full level_1 and tier_a gate 0 failed.

Underworld development team with AI support from Claude Code

[Copilot]

Pull request overview

This PR fixes a PETSc DM/Vec lifecycle bug triggered by MeshVariable-driven DM rebuilds: previously _setup_ds() explicitly destroy()-ed the old DM and vectors, which could blind user-held petsc4py wrappers (segfault) and leave NumPy views pointing at freed buffers (heap corruption). The change shifts to releasing references and strengthens cache invalidation so held handles remain stale-but-valid and UW3 won’t re-serve views of released buffers.

Changes:

• Update DM-rebuild path to release (not destroy()) the old DM and variable vectors, preserving safety for captured petsc4py handles.
• Ensure MeshVariable rebuild restores preserved data while eagerly invalidating all data/array caches to prevent stale views.
• Add serial + MPI regression tests for held-handle survival and view refresh; document the updated contract in data-access.md.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/underworld3/discretisation/discretisation_mesh_variables.py	Stop destroying old DM/Vecs during DM rebuild; restore data and invalidate caches to avoid stale views/use-after-free.
tests/test_0858_dm_rebuild_lifecycle.py	New lifecycle/regression tests for held DM handles, memory behavior across rebuild loops, and view refresh semantics.
tests/parallel/ptest_0011_dm_rebuild_held_handles.py	New MPI reproducer asserting the held-handle contract holds on every rank and solve remains correct.
tests/parallel/mpi_runner.sh	Add the new MPI test to the parallel runner at np=2 and np=4.
docs/developer/subsystems/data-access.md	Document contract for raw NumPy views vs captured PETSc handles across MeshVariable-triggered DM rebuilds.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[lmoresi]

Adversarial review — PR #536 (release, don't destroy: DM rebuild lifecycle)

Branch bugfix/492-dm-rebuild-lifecycle, commits a0d5d74 / 3c96f41 / 0c1201c, fixes #492.
Reviewed independently on a clean worktree (r536-review, env amr-dev) reset to
pull/536/head, ./uw build from scratch. The "unfixed" comparator is the same env
with the installed (pure-Python) discretisation_mesh_variables.py swapped for the
merge-base version — byte-equivalent to building a0d5d748^, since that is the only
file the src commit touches.

Verdict

Approve. No merge-blockers. The one-line-shaped core change does exactly what the
design note claims, and every sharp probe we aimed at it came back clean: PETSc object
creation/destruction counts are byte-identical with and without the eager destroy over
a realistic 50-cycle solve+rebuild loop, numerical outputs are byte-identical, the
stale-lvec window is closed by the pre-existing _stale_lvec guard, and interpreter
exit with a superseded DM held in a global is silent. Three non-blocking findings below
(one test-robustness risk on Linux, one coverage gap, one inert-code nit).

MERGE-BLOCKERS

None.

Evidence

1. Leak reality — object counts, not just RSS (clean)

The PR's 20-cycle/50 MB RSS bound is loose, so we measured the sharp thing: a
realistic #417-shaped loop (per cycle: create MeshVariable → build Poisson → solve →
drop solver → gc), run under -log_view so PetscFinalize reports per-class
creations/destructions, on both builds at N=20 and N=50.

Object (N=50)	fixed C/D	unfixed C/D
Distributed Mesh	6635 / 5586	6635 / 5586
Vector	8848 / 6426	8848 / 6426
Section	16709 / 11470	16709 / 11470
Index Set	30125 / 28230	30125 / 28230

Identical to the object on both builds, at both N. The release path destroys
exactly as many C objects as the eager destroy did; the live-at-exit population grows
only because the loop itself accumulates variables (same growth both builds — a
pre-existing structural property, not a PR regression). RSS traces agree within noise
(cycle 50: 414.2 MB fixed vs 414.0 MB unfixed; identical at every 10-cycle checkpoint).
Who frees the old DM when the user's wrapper is dropped without destroy(): verified
by the equality above — petsc4py __dealloc__ decrefs and the C object dies with its
last holder, or the counts would diverge by N.

2. The mesh._lvec release window (clean)

_stale_lvec = True on this path is pre-existing (present at a0d5d748^); the PR only
removed the destroy(). The mesh.lvec property raises RuntimeError while stale,
and every consumer we found (petsc_maths.pyx ×3, petsc_generic_snes_solvers.pyx
aux-vec sites) calls update_lvec() first, which rebuilds from the new DM
(_lvec is None → createLocalVec() on self.dm). Probe (post-solve rebuild, then
immediate hits): mesh.lvec raises in the window; uw.function.evaluate on a
pre-existing variable returns exact values immediately after the rebuild; an immediate
second solve on the new DM is exact (rel err 6e-9); variable data preserved across two
rebuilds. The solver-clone DM's auxiliary vec keeps its own counted ref to the old
combined vec either way (destroy only ever dropped one ref), and is replaced at the
next solve's setAuxiliaryVec — no pre/post difference.

3. Cache audit (one inert-code nit)

Enumerated everything cached against DM/Vec identity:

• _canonical_data + _canonical_data_lvec_id id-check — the real self-heal; cleared
  eagerly (pre-existing).
• _dminterpolation_cache — stores no DM, invalidated on this path (pre-existing).
• mesh._evaluation_hash fast path is disabled (if False and …, _function.pyx:1242)
  — no stale-result surface.
• kd-trees, _internal_boundary_cache, _owned_cells_mask_cache, coord arrays — keyed
  on topology/geometry, which clone() shares; semantically unaffected.
• Nit (Minor): the newly added var._data_cache = None / var._array_cache = None
  clears are inert for mesh variables — those attributes are initialized
  (discretisation_mesh_variables.py:464-465) and cleared (1744-45, and 1851-52 on the
  adapt path) but never populated or read anywhere in the mesh-variable class; they are
  live caches only on SwarmVariable (swarm.py). Harmless and symmetric with the
  adapt path, but the PR body's claim that these clears "guarantee UW3 never hands back
  a view of the released vectors" over-credits them — the guarantee is _canonical_data
  • the lvec-id check. Worth a one-word comment fix at most.

4. Semantic drift (none measured)

Identity probe — Stokes (P2/P1, sinusoidal body force, tolerance 1e-8) → third
variable created after the solve (rebuild with a live solver) → Poisson solve →
Stokes re-solve — printed to 15 significant digits: diff of fixed vs unfixed outputs
is empty (|v|, |p|, SNES its/reason, |q|, re-solve norms all identical).
Exit teardown: a script holding the superseded DM in a module global to interpreter
exit returns 0 with empty stderr on the fixed build — no PetscFinalize-ordering
warnings. (On the unfixed build the same script can't even reach exit: the handle is
already blinded — the bug being fixed.)

5. Tests

• test_0858_dm_rebuild_lifecycle.py: 4/4 pass on the fixed build (8.3 s).
• Negative control verified first-hand: with the installed file reverted to the
  merge-base version, 3/4 fail cleanly (held.handle == 0 asserts; no segfault) and
  the view-refresh test passes, exactly as the PR body states.
• ptest_0011_dm_rebuild_held_handles.py: pass at np2 and np4 ("held DM handle valid
  after rebuild … solve on rebuilt DM exact"). mpi_runner.sh entries present.
• Full gate level_1 and tier_a (excl. test_0050): 594 passed, 0 failed,
  17 skipped (parallel tests without --with-mpi), 1 xfailed (pre-existing
  1-manifold evaluate), 7:18.
• Docs commit: the data-access.md contract is accurate — we verified empirically that a
  raw numpy view does NOT pin the released Vec (the view's base chain bottoms out in a
  bare memoryview; a weakref on the old _lvec wrapper dies during the rebuild even
  while the view is held), so "aliases freed memory, re-read after variable creation"
  is the correct statement of the post-fix world.

Findings (non-blocking)

1. (Moderate, test robustness) np.shares_memory in
   test_data_views_are_refreshed_after_rebuild is allocator-dependent. The old Vec
   buffer is genuinely freed during the rebuild (measured above) and the replacement
   Vec has the identical size; if the allocator recycles the block (glibc tcache makes
   this likely for small test meshes; macOS happened not to), shares_memory returns
   True and the test fails spuriously — on exactly the platform (Linux CI) this test is
   meant to guard. Suggest replacing the assertion with identity + value checks
   (u1._lvec is not old_lvec; values preserved; round-trip write) or tolerating the
   reuse case. The ordering is confirmed adverse: a weakref probe shows the old wrapper
   dies at the var._lvec = None release step (nothing else — not _canonical_data,
   not the numpy view — holds it), i.e. the buffer is freed before the restore loop's
   _set_vec allocates the identically-sized replacement.

2. (Moderate, coverage) The adapt-child arm does not run the original detonation's
   solver path.test_adapt_child_second_variable_after_solve uses a single-field
   Poisson under preconditioner="auto", which declines FMG for single-field solvers
   and keeps GAMG — so the _custom_mg_coarse_meshes / custom-P chain from the
   test_0842 FMG arm (the holders originally suspected in Creating a MeshVariable destroys the previous mesh.dm; a held handle segfaults (use-after-free) #492) is never built in this
   regression test. The holder inventory exonerated those links by measurement, and the
   0842 pair passes, but nothing in the suite now re-arms the exact original sequence
   (FMG child solve → post-solve variable creation). Cheap hardening: set
   preconditioner="fmg" on the first child solve in this test. The design note's own
   outstanding item — a one-off Linux ASan run of 0842+0844 — also remains the
   definitive close-out for the class-2 story and should ride the first Linux CI run of
   this branch.

3. (Minor) Inert cache clears + stray docstring pointer. Finding §3 above
   (_data_cache/_array_cache are swarm-only caches); and
   test_old_dm_released_to_last_holder_no_accumulation's comment says "see test
   docstring note below" — there is no note below. The getRefCount() == 1 asserts
   will fail if a future legitimate holder of mesh.dm appears; that is arguably the
   point (hygiene pin), but be aware they encode "no PETSc-side holder exists" as an
   invariant. One contract boundary worth a sentence in data-access.md eventually:
   MeshVariable.__del__ (and Mesh.__del__) still eagerly destroy() their vectors,
   so the "handles stay valid but stale" promise holds across rebuilds, not across
   variable/mesh destruction — fine in practice (variables live as long as their mesh)
   and out of Creating a MeshVariable destroys the previous mesh.dm; a held handle segfaults (use-after-free) #492's scope, but it is the same wrapper-blinding shape.

Files

• Probes + outputs: scratchpad/r536/ (probe_leak.py, probe_window.py,
  probe_identity.py, probe_exit.py, leak_{fixed,unfixed}{20,50}.out,
  identity{fixed,unfixed}.out, exit_fixed.{out,err})
• Review worktree: .claude/worktrees/r536-review (PR head 0c1201c)

Underworld development team with AI support from Claude Code

[lmoresi]

Response commit 5afc44d takes the review's two moderate hardenings: the view test asserts object identity + preserved values instead of np.shares_memory (the released buffer is freed before its identically-sized replacement allocates, so allocator recycling could fail the address comparison spuriously on exactly the platform it guards — and the dangling view is never dereferenced); and the adapt-child arm's first solve runs under preconditioner="fmg" so the custom-P coarse chain from the original detonation sequence is genuinely constructed. 4/4 locally. The remaining review notes stand as recorded: the inert swarm-cache clears (harmless), the Linux ASan close-out (tracked on #492 at close), and the __del__ eager destroy (same shape, out of scope).

Underworld development team with AI support from Claude Code