Reach the boundary-snap reduction on every rank, and scan for the shape (#627)

[lmoresi]

The defect

snap_level_boundaries guarded its inversion-count allreduce on
snapped_any.any() — a mask over the calling rank's local vertices. A rank
whose partition holds no vertex on any registered bounding surface skipped the
whole block and never arrived at the reduction, leaving its peers in it. The
fix initialises the count before the guard and reaches the reduction
unconditionally; the cell-orientation loop stays inside it, because that work
really is local. Fixes #627.

Why a scan

This is the fourth instance of one shape: a collective behind a predicate on
rank-local data. uw.mpi.selective_ranks() cannot see it. That machinery
catches deliberate rank selection, and it works — but these guards read as
ordinary control flow (module is None, undecided.size,
snapped_any.any()), so the author never reaches for the context manager and
the check never fires.

Nor is annotation the answer. Collectivity is transitive, and in a parallel FE
library that closes over a third of the source — 829 of 2523 functions from
the comm.* primitives outward. Stokes.solve is collective; saying so adds
nothing.

What is rare is the conditional collective. tests/test_0052 asks one
question of every comm.<op> call site: is it reached on all ranks? Guards the
whole communicator agrees on are recognised rather than reported —

• the communicator geometry (uw.mpi.size > 1, the commonest guard here),
• the mesh's rank-invariant description (self.dim, .degree, .simplex),
• a parameter of the function or of an enclosing one, uniform by calling
  convention,
• and a value that has itself been through a collective, which is what lets
  the library's own idiom come out clean:

# COLLECTIVE, and reached on every rank: one with nothing to flip still
# has to vote or its peers block waiting for it.
n = uw.mpi.comm.allreduce(len(new_edges), op=MPI.SUM)
if n == 0:
    break

Across 210 call sites that leaves two entries in ACCEPTED, each carrying the
reason it is rank-uniform, and one in IN_FLIGHT for #612, whose fix is in an
open PR. A stale-entry test removes either as soon as the site stops tripping
the scan, so neither list can quietly become a blanket exemption.

Evidence

The scan finds #612 and #627 unaided, and nothing else. Controls run both
ways: the classifier is tested on the predicates that actually deadlocked and
on the uniform ones it must not flag, and the walker is tested on an injected
defect together with the fixed form of the same code — the fix must come out
clean or the check is measuring nothing.

It is a name-level scan, not a proof. It cannot see through a helper, and a
predicate it calls rank-local may be uniform for reasons it cannot read; that
is what ACCEPTED is for, and an entry there is a claim with a stated reason.

Testing

• test_0052 — 5 passed (level_1, tier_a; already inside the
  test_005[1-9] CI batch, no scripts/test.sh change needed)
• serial adapt suites test_0830, test_0835, test_0836 — 33 passed
• tests/parallel/test_0873_adapt_collective_stop_mpi.py at np=4 — 4 passed
• pre-fix discretisation_mesh.py from git scans as one finding at line 8388;
  post-fix, none

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

The fix. Correct and minimal. flipped is initialised before the guard, so
every path reaches the reduction with a defined value, and the local
orientation loop stays conditional. We checked the failure is reachable rather
than theoretical: snapped_any is built from _pinned_mask(dm, (s.label,))
over local vertices, and sync_mask reconciles it only between ranks that
share a vertex — a rank owning none has an all-False mask and no peer to
receive one from. Partition-dependent, so it presents as an intermittent hang.

Weakest part of the scan: name-level matching. It has already bitten us
twice while building this. Seeding from @collective_operation names pulled
in h5py.write and sympy.solve — 366 call sites instead of 210, most of them
noise. Requiring the receiver to be a communicator fixes that, but the same
weakness remains in the other direction: a collective reached through a helper
(self._reduce(...)) is invisible. The scan proves nothing about coverage.
Its value is that the sites it does see are checked, and that number is now
small enough to read.

Second weakness: the guard classifier is textual. uw.mpi.size > 1 had to
be special-cased out of > 0, and n == 0 needed one step of dataflow before
the house idiom stopped tripping it. Both are heuristics on unparsed source,
not analysis. A predicate written unusually will land in local-data and cost
a reviewer a minute, or — worse — land in uniform and be missed. The
asymmetry is at least in the safe direction for the tokens we chose: the
uniform set is short and specific, the local set is broad.

Third: ACCEPTED is a hole by construction. Two entries today, each with a
stated reason we verified rather than assumed — outcropping traced back to a
comm.bcast and to _domain_boundary_facets, which is allgathered and
deduplicated to identical bytes. The stale-entry test is the only thing keeping
that list from growing quietly, and it only fires when a site stops tripping,
never when an entry's reason goes stale while the site remains. A wrong reason
here is undetectable.

What we did not do. No runtime check. A timed Ibarrier participation
probe inside @collective_operation would catch what the static scan cannot —
divergence through helpers, and through anything dynamic — but it costs a
collective per decorated call and needs its own opt-in. Worth doing only if the
static list stops being sufficient.

Controls. Both directions, because a scan that flags a fix as well as a bug
measures nothing: the classifier is asserted on the four predicates that
actually deadlocked and on the uniform ones it must not flag; the walker is
asserted on an injected defect and on the fixed form of the same code. The
test_source_tree_is_scannable premise test exists because every other
assertion here passes vacuously if the path or the matching breaks — it asserts

100 call sites are found.

Verified: pre-fix discretisation_mesh.py from git scans as one finding at
line 8388; post-fix, none.