Callback-array parallel safety: central view/copy guard, honest exceptions, no dead snapshots (#379 items 3+4b)

[lmoresi]

First of the #379 series (see the issue for the full map). Draft until #378 merges — the two canonical callbacks this PR migrates are the same closures #378 patches inline, so this PR merges second and absorbs a small, mechanical conflict (the inline guard block is deleted here because the guard moves into the class).

What this does

A central guard for canonical-storage callbacks. Callbacks written for a variable's canonical storage are inherited by every array derived from it, so they also fire on views and on temporary fancy-index copies. A copy's contents differ from rank to rank, so a PETSc sync run from inside the callback executes its collective operations on some ranks only — that is the #376 parallel hang. The new add_canonical_callback() applies the fix once, on the class: views hand the full canonical array to the callback, copies are skipped (numpy's write-back re-fires through the parent), and view-vs-copy is decided by identity in numpy's base chain — never np.may_share_memory, which is false for any zero-size array and would re-create the rank asymmetry on ranks with no local data. Any future callback registered this way gets the protection for free instead of re-inheriting the trap.

The mesh-variable and swarm-variable canonical callbacks now register through it. The mesh callback body moves to MeshVariable._flush_canonical_to_petsc(), which becomes the single write-back path (and is reused by the planned synchronised_array_update rework, the next PR in the series).

Callback exceptions now propagate. The immediate callback path used to swallow every exception into a logger.warning. A swallowed failure leaves PETSc out of sync with the canonical array on one rank only — the silent desynchronisation that hid #376. Failures now surface where they happen.

Dead weight removed. _create_variable_array (mesh and swarm) and _create_flat_data_array had no callers and embodied the unguarded pattern — deleted. Every write also used to snapshot prior values into change_info['old_value'] (a full-array copy per in-place operation); nothing ever read it, so the snapshot is gone and the key stays as None for compatibility.

Not touched here: the swarm.points legacy callback (marked TODO(BUG); removed by the #379 item-1 PR) and the delayed-replay machinery (rebuilt in the item-2 PR).

Verification

• New tests/test_0059_canonical_callback_guard.py (level_1 / tier_a, written first and shown to fail): view/copy classification including the zero-size-view empty-rank case, exactly-once firing for masked += writes, and a no-reference-cycle check.
• Quick gate level_1 and tier_a: 405 passed, 1 pre-existing xfail.
• Parallel smoke at np=2: swarm write/timestep and mesh-deform kd-tree tests pass.

Underworld development team with AI support from Claude Code

[lmoresi]

Adversarial review — round 1

Two independent reviewers (numpy-subclass semantics; codebase regressions) attacked this branch. The core guard survived everything thrown at it, with a structural reason worth recording: numpy's view base-collapse only walks past bases of matching type, and the canonical array's own base is always a plain ndarray — so the base chain always stops exactly at the canonical. Verified empirically for slice/nested/reshape/transpose/newaxis/dtype-changed views, zero-size slices on empty selections, masked and integer fancy writes (exactly one firing via parent write-back, including when the parent is itself a view), and detached copies (correctly silent — previously these packed the copy's wrong-sized contents, so skipping is strictly better). The deleted factories are confirmed dead (no caller in src/ or tests/), and old_value is confirmed unread everywhere.

Findings to fix (this branch)

1. Raise-after-mutation poisons mesh.X.coords (should-fix). __setitem__ writes before notifying, so now that callback exceptions propagate, the coordinate-lock rejection ("an in-place write to mesh.X.coords is rejected") raises AFTER the values landed in mesh._coords — the error claims rejection while the mesh's coordinate cache now disagrees with the DM. tests/test_0540_coordinate_change_locking.py does exactly the catch-and-continue this invites. Fix: the rejection path restores _coords from the DM before raising, making the message honest.

2. The delayed path still swallows exceptions (should-fix). The commit message says "exceptions propagate", but only the immediate path was fixed — the delayed executors (which mesh.access() still routes through) keep the logger.warning swallow. The stacked synchronised_array_update: collective dirty-variable flush replaces rank-local event replay (#379 item 2) #383 rewrites this machinery and removes the swallow, but this branch should not ship a claim it doesn't hold on its own. Fix: remove the two delayed-path swallows here too.

3. sync_data (different-size branch) orphans canonical callbacks (should-fix, latent). The new object copies the callback list, but the copied dispatch weakrefs the OLD canonical — every future write to the returned array is classified as a copy and silently skipped. No in-tree caller pairs the two today (the only sync_data caller is the deprecated swarm.points wrapper, removed by Retire the swarm.points writable stack: read-only snapshot, coords is the write path (#379 item 1) #382), but sync_data is exactly the resize-after-migration idiom canonical arrays would reach for. Fix: re-register canonical callbacks against the new object (and make remove_callback able to remove a canonical callback by its original function, closing the related API wart).

4. Governing docs drifted (should-fix, docs). docs/developer/subsystems/data-access.md (the authoritative document) still points at the deleted _create_variable_array(); UW3_Developers_NDArrays.md shows the removed old_value snapshot as current internals; UW3_Style_and_Patterns_Guide.md documents old_value as carrying previous values.

5. Swarm canonical callback contradicts the new no-cycle comment (note→fix). It closes over the variable strongly (pre-existing, gc-collectable), while the mesh side uses array.owner. Aligning it on array.owner makes the comment's guarantee true.

6. Test gap (should-fix). No test pins the new propagate behaviour or the old_value is None contract — the fix half of the third commit ships without the test that would catch its reversion.

Recorded, no action

• _is_canonical/_canonical_ref tags are unused within this branch alone (they become load-bearing in synchronised_array_update: collective dirty-variable flush replaces rank-local event replay (#379 item 2) #383) — dissolves when the stack lands together.
• Pickling of arrays carrying closures was equally impossible before this branch.

Fixes follow on this branch; the stacked branches will be rebased over them.

Underworld development team with AI support from Claude Code

[lmoresi]

Round 1 — response (fixed in 1f42128)

All six findings addressed:

1. Poisoned coords on rejection — the coordinate callback now restores the canonical from the DM before the rejection surfaces, so mesh.X.coords always agrees with the DM after a refused write. Pinned by a new asserting test in test_0540 (that module previously only printed).
2. Delayed-path swallow — removed; both executors now propagate, so the exceptions-propagate contract holds on this branch alone, including via mesh.access().
3. sync_data orphaning — the different-size branch re-registers canonical callbacks against the new object (the dispatch wrapper records its original function as _wrapped). Pinned by test_sync_data_resize_rehomes_canonical_callbacks.
4. remove_callback wart — now removes a canonical callback by its original function.
5. Swarm callback cycle — resolves the variable via array.owner like the mesh side; the no-cycle rationale in the guard's docstring is now honoured by all three registration sites.
6. Docs — data-access.md, UW3_Developers_NDArrays.md, and the patterns guide updated; new tests pin propagation and the old_value = None contract.

Quick gate: 409 passed, 1 pre-existing xfail. The stacked branches (#382, #383, #385) will be rebased over this; their own round-1 fixes follow on their threads.

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]

Round 2 — all round-1 fixes verified closed (plus one diagnostics guard, ca406f1)

The verifier confirmed: the coord-rejection restore is genuine (the cache never aliases the live DM buffer, so the rollback is real, shapes verified on bare and variable-carrying meshes, test run on two builds); the delayed-path swallow removal, sync_data re-homing (order-preserving), remove_callback-by-original, and the swarm array.owner rewrite all hold. One small addition from a round-2 note: the restore now runs only when cache and DM shapes agree, so a failure that replaced the coordinate Vec at a different size (mid-rebuild) cannot mask the original error with a broadcast exception.

Underworld development team with AI support from Claude Code