Swarm/mesh checkpoint quiescence, .msh coordinate-system default, rank-symmetric stats temporaries

[lmoresi]

Three fixes from the triage backlog, plus an adversarial-review round and its responses. Closes #330, closes #397, closes #384.

The fixes

#330 — save() races its own metadata append (fix(swarm) + review follow-up)
Swarm.save and SwarmVariable.save appended rank-0 metadata after the data write and returned immediately on the other ranks; an immediate reopen (read_timestep after write_timestep) hit HDF5 file locking (BlockingIOError errno 35) and the rank divergence hung the next collective. Both methods now end with a barrier so the file is quiescent on return. The review found the identical pattern in MeshVariable.save and mesh.write — both now carry the same barrier, and both metadata blocks use context managers so an exception cannot leak the HDF5 handle.

tests/test_0003_save_load.py — previously 1 failure + a hard hang at np2 (the swarm tests were never run under MPI by any gate) — now passes 10/10 at np2 and np4 in ~5 s. This unmasked a further pre-existing crash (empty-rank navigation kd-tree, tiny meshes at np4), filed as #399 with a live reproducer; the affected test now uses a 4x4 mesh so the whole file stays MPI-clean.

#397 — mesh loaded from a .msh file couldn't be written
CoordinateSystemType stayed None and mesh.write() crashed in its metadata block. The construction now defaults to CARTESIAN — matching every uw.meshing constructor, and matching what CoordinateSystem already did at runtime (a None-typed mesh was always functionally Cartesian; the review verified no consumer anywhere treats None specially). Per the review: loading an h5 checkpoint that has lost its coordinate metadata now warns before defaulting, so a wrong label cannot launder itself silently into re-written files (pre-fix, the crash was the tripwire).

#384 — rank-asymmetric temporary names in vector/tensor stats
The stats temporaries were named with id(self) — a rank-local address — while variable creation/destruction runs collective DM operations keyed by field name. The suffix is now the parent's clean_name (identical on every rank). Per the review: both sites now refuse a pre-existing temp name outright — a collision would silently alias, overwrite, and deregister a user's variable (demonstrated live in the review round).

Adversarial review (public, round 1)

Posted in full as the first comment below. Summary: all four primary fixes held against rank-asymmetric control-flow tracing, barrier/file-close ordering, zero-particle ranks, and live np4 probes of both I/O branches with back-to-back write/read cycles. It caught three things that are fixed in the response commit (temp-name aliasing, the wider #330 pattern, the silent-default loudness), and one pre-existing defect outside the diff, filed as #400 (_tensor_stats indexes the flat component count against the structured array axis — crashes for any true tensor variable).

Verification

• test_0003_save_load.py: 10/10 at serial, np2, np4 (first time this file is MPI-clean end to end).
• tests/parallel/test_0750_global_statistics.py: 12 passed at np2 and np4.
• Level_1 + tier_a gate: 442 passed, 0 failures (run after the review-response commit).
• New issues filed on the way: Mesh construction crashes on empty ranks: navigation kd-tree build passes a 1-D empty array (StructuredQuadBox(elementRes=(2,2)) at np4) #399 (empty-rank kd-tree construction crash, with baseline repro on unmodified development), _tensor_stats crashes for genuine (d,d) tensor variables: flat component index used on the structured array axis #400 (tensor-stats indexing).

Reminder for the merge: manual close needed for #330, #397, #384 (PRs target development; auto-close does not fire).

Underworld development team with AI support from Claude Code

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[lmoresi]

Adversarial review — round 1 (public record)

Semi-independent skeptical pass over the code changes, instructed to refute/rebut each fix. Live probes ran at np4 in the branch worktree environment (both the mpio and force_sequential I/O branches, zero-particle ranks, and back-to-back write/read cycles).

Attack 1 — #330 barriers: can any rank miss them (new deadlock)?

HELD. Every exit path in both save methods was traced: one return each, after the barrier; the only raise (the .h5 suffix check) is argument-driven and rank-symmetric for every in-repo caller. Branch selection (mpio vs sequential) depends only on a build constant and a same-valued argument, so no rank can diverge. Both paths were already unconditionally collective before this change (allgather / collective create / barrier loop), so the added barrier introduces no new collectivity requirement. Verified live at np4 with zero-particle ranks and three consecutive write/read cycles on both branches: no hang, no BlockingIOError.
Documented residual: if rank 0 raises inside its solo metadata block (disk full, external lock), other ranks now hang at the barrier rather than desynchronising later — both outcomes are fatal under MPI; noted, accepted.

Attack 2 — barrier vs h5py close ordering

HELD. The rank-0 metadata append is inside a context manager, closed strictly before the barrier; non-zero ranks cannot pass until rank 0 arrives, i.e. after close. uw.selective_ranks contains no collectives and does not interact.

Attack 3 — #384 clean_name collisions / cleanup lifecycle

HELD for the fix — but the review DEMONSTRATED a new corruption surface: a user variable literally named _temp_mag_<X> would be silently aliased by variable creation, its data overwritten by the magnitude (probe: 42.0 → 1.414), and then deregistered. Unreachable under the old id(self) names. Fixed in the response commit: both stats sites refuse a pre-existing temp name. Repeated stats calls, exception-path cleanup, and cross-variable uniqueness (mesh.vars keyed by clean_name) all verified live.

Attack 4 — #397 None→CARTESIAN semantic drift

HELD. All construction sites grep-verified: every uw.meshing constructor passes an explicit enum; adapt/submesh children inherit the parent's. Pre-fix, CoordinateSystem(mesh, None) already fell to the Cartesian-by-default branch — None-typed meshes were functionally Cartesian everywhere at runtime, and no consumer special-cases None. The one real behaviour change found: an annulus/spherical h5 that LOST its metadata used to crash write() (a loud tripwire) and would now silently re-write a CARTESIAN label into descendant files. Fixed in the response commit: a rank-0 warning fires when the default engages on a metadata-less h5.

Attack 5 — the 2x2→4x4 test mesh change

HELD. The guarded behaviour (missing-directory raise, before any collective) is resolution-independent. The #399 justification verified live: elementRes=(2,2) at np4 deadlocks mesh construction on unmodified development.

Findings outside the diff

1. _tensor_stats crashes for genuine (d,d) tensor variables — flat component count indexed against the structured array axis (pre-existing; the diff only renamed its temporary). Filed as _tensor_stats crashes for genuine (d,d) tensor variables: flat component index used on the structured array axis #400.
2. The Swarm.save: non-zero ranks return while rank 0 still appends metadata — immediate reopen hits HDF5 file locking (BlockingIOError) #330 pattern also existed in MeshVariable.save and mesh.write (no trailing barrier; metadata handles not exception-safe — mesh.write never closed its handle on the exception path). Fixed in the response commit.
3. Cosmetic, pre-existing: both swarm save() signatures annotate filename: int; left untouched (out of scope).

Response commit

a559e47 carries all three fixes; the level_1+tier_a gate (442 passed) and the np2/np4 test batteries were re-run after it.

Underworld development team with AI support from Claude Code