Make the attack link symmetric, following spring-attachment semantics

[drbergman]

Summary

This builds directly on @vincent-noel's #397, which diagnosed this bug and got the shape of the fix right. His write-up names the three root causes precisely, and the reverse-index idea here is his — a target that knows who is attacking it can clean up in O(#attackers) instead of scanning every cell. I've kept that idea and changed where the list lives and how consistently it's maintained.

The underlying problem, restated: phenotype.cell_interactions.pAttackTarget is the only long-lived Cell* in the object model with no owner and no mirror. delete_cell scrubs attached_cells, spring_attachments and state.neighbors — the Dec 2024 audit in #342 — but pAttackTarget was introduced five months earlier and was never added to that list. Grep core/ for pAttackTarget: zero hits in PhysiCell_cell.cpp.

The approach here is to make the attack link look exactly like a spring attachment: two halves, maintained together, torn down wherever springs are torn down.

What this changes

• Cell_State gains std::vector<Cell*> attacked_by, the mirror of pAttackTarget.
• Four methods mirroring the spring API. add_attacker / remove_attacker maintain one half, exactly as attach_cell_as_spring / detach_cell_as_spring do. remove_all_attackers / remove_self_from_attacked tear down both.
• Free functions begin_attack / end_attack, mirroring attach_cells_as_spring / detach_cells_as_spring. They keep all three records — the attacker's pAttackTarget, the target's attacked_by, and the spring — in lockstep, so a caller cannot set one without the others.
• Teardown is hooked at every site that destroys a cell or changes its identity, and all of them are serial: ~Cell, divide, both delete_cell variants, and convert_to_cell_definition.
• ingest_cell, fuse_cell and lyse_cell no longer tear down inline. They already call flag_for_removal(), and the serial cells_ready_to_die -> die() -> delete_cell() pass does the same work off-thread. Those three run inside the mechanics parallel-for, and remove_all_spring_attachments() / remove_all_attached_cells() walk their vector unlocked while attach_cell_as_spring / detach_cell_as_spring edit it under the critical -- a race that predates this PR and is measurable on pristine development (3 of 6 seeds on stock interaction-sample at 8 threads; 0 of 6 once deferred, with bit-identical single-threaded output).
• standard_cell_cell_interactions now calls begin_attack / end_attack instead of writing pAttackTarget inline. The guard that stops a cell looking for a new target while already attacking is unchanged and still upstream of the call.
• The MultiCellDS resume path restores both halves of the link.

How it differs from #397

Stated as differences, not defects — several of these are cases #397 simply didn't set out to cover, and some only became visible from running it.

1. The reverse list lives in Cell_State, not Cell_Interactions.

This is the substantive difference, and it removes two failure modes structurally rather than with extra code:

• convert_to_cell_definition does phenotype = cd.phenotype. A phenotype-resident attacked_by is wiped by that assignment while the attackers keep live pAttackTargets, reopening the original dangling pointer. state is explicitly left untouched by transformation (there's a comment to that effect at the end of the function), so a state-resident list survives.

• Cell::divide does child->phenotype = phenotype. That is a value copy, so a phenotype-resident attacked_by gives the daughter its own vector holding the same Cell* values — valid pointers to the cells attacking the parent. It bites because remove_self_from_attackers() clears unconditionally:

  for (Cell* pCell : phenotype.cell_interactions.attacked_by)
  { pCell->phenotype.cell_interactions.pAttackTarget = NULL; }

with no pCell->...pAttackTarget == this test, so deleting the daughter aborts every attack aimed at the parent. Adding that test would defuse it (the version here has one). Keeping the list in state avoids the situation entirely: copy_data doesn't touch state and nothing else assigns child->state, so the daughter's list is default-constructed empty.

2. Both directions are maintained.

#397 has remove_self_from_attackers() — target deleted, clear the attackers. This adds the inverse, remove_self_from_attacked() — attacker deleted, deregister from its target. Without it, deleting an attacker leaves a stale pointer in the victim's attacked_by, and the later remove_self_from_attackers() executes pCell->...pAttackTarget = NULL through that freed pointer — a write, where the current bug is a read.

There's a guaranteed trigger rather than a hypothetical one: an attacker that enters a death model can never run its own end-of-attack erase, because standard_cell_cell_interactions returns early for dead callers. That's #395.

3. Hooked at every teardown site, not only delete_cell(int).

~Cell()'s salvage path, divide, delete_cell_original and convert_to_cell_definition all need it too. #342 added the neighbour scrub to both delete_cell and ~Cell(); this follows the same convention. ingest_cell, fuse_cell and lyse_cell are covered through the removal queue rather than inline, for the thread-safety reason above.

4. Paired begin/end functions rather than inline maintenance.

#397 maintains attacked_by inline at the two call sites in standard_cell_cell_interactions. Bundling the three records into begin_attack / end_attack means a future call site can't set the pointer and forget the list, and it matches how springs are already handled.

5. Mutual attacks share one spring.

Found by running it, not by reading. attach_cell_as_spring dedupes, so when A and B attack each other there is a single spring between them. Ending either attack removes it, stranding the other. end_attack and remove_self_from_attacked now only drop the spring once neither direction still needs it. Current development has this too — it unconditionally calls detach_cells_as_spring at attack end.

6. Side effects on adjacent issues.

• attack-induced spring removed on attacker division, but attack continues #396 (daughter keeps attacking after division) is fixed by the state placement plus the divide teardown.
• detach attacker on transformation #341's concern is handled by the convert_to_cell_definition hook. That PR is still the clearer statement of the transformation problem and I'd rather it land on its own merits; this just makes the teardown unconditional.
• lyse_cell set flag_for_removal() before dead = true, unlike ingest_cell and fuse_cell. Brought into line. Its missing spring teardown is now moot, since all three defer to the removal queue.

What this deliberately does not do

• Nothing about neighbour asymmetry. That's Fix(cell-neighbors): update max_cell_interactive_distance_in_voxel on position change #409 / Include neighbor lists and pressure computation in non-movable cells. #410 territory and I've stayed out of it. Worth noting the neighbour predicate in add_potentials is symmetric by construction — the sum of both cells' reaches — so asymmetry comes from bookkeeping (the is_movable / is_out_of_domain gate on the refresh, and stale max_cell_interactive_distance_in_voxel), not from geometry.
• No guard on the attack_cell write. attack_cell writes damage and total_attack_time through the target pointer with no liveness check. That's safe once the pointer can't dangle, but it's still an unguarded write.
• A mutual attack can still lose its spring. If A is attacking B and, in the same step, A ends its attack while B starts attacking A, B's begin_attack can land between A's is_attacked_by check and its detach_cells_as_spring — B finds the pair already spring-linked so adds nothing, then A tears the spring down and B is left attacking A without one. Closing it needs the check and the detach under a single lock, which the non-reentrant unnamed critical makes awkward; note the unconditional detach on development loses that spring in every mutual attack, threads or not.
• Direct assignment to pAttackTarget still bypasses the invariant. Custom modules that write the field directly get no protection. A dedicated Attack type owning both endpoints would close that off properly, and would also remove the special case in dynamic_spring_attachments that has to sniff pAttackTarget to avoid randomly detaching an attack spring. That's a bigger change and a separate conversation.

Related

• Fix use after free in cell attack  #397 — the diagnosis and the reverse-index approach this builds on
• attack-induced spring not removed on attacker death #395 — dead attacker never releases; the reason the inverse teardown is needed
• attack-induced spring removed on attacker division, but attack continues #396 — daughter inherits the attack
• detach attacker on transformation #341 — detach attacker on transformation
• Fix(cell-neighbors): update max_cell_interactive_distance_in_voxel on position change #409, Include neighbor lists and pressure computation in non-movable cells. #410 — the neighbour side, not touched here
• rare nullptr in get_single_signal #320 / bugfix: if cell dies, must remove self from neighbors (to avoid interactions with deleted cells).  #342 — the Dec 2024 precedent for scrubbing back-references at deletion
• Cell definitions share fixed_duration boolean for apoptosis death model #199 — shared Cycle_Model fixed_duration; independent bug, but it is what makes the zero-mechanics-step removal reachable from configs that asked for a fixed duration

Happy to fold this into #397 as commits there, or co-author, or close this and let Vincent take whichever parts are useful — the diagnosis was his and I don't want to fragment the work.

[Copilot]

Pull request overview

This PR makes cell “attack” relationships symmetric and teardown-safe by introducing a reverse index (Cell_State::attacked_by) that mirrors phenotype.cell_interactions.pAttackTarget, and by centralizing attack start/stop logic to keep pointer, reverse list, and spring attachment consistent.

Changes:

• Add Cell_State::attacked_by plus new APIs (add_attacker, remove_attacker, remove_all_attackers, remove_self_from_attacked) and free functions (begin_attack, end_attack) to manage the bidirectional attack link.
• Update standard_cell_cell_interactions to use begin_attack / end_attack instead of directly mutating pAttackTarget and spring attachments.
• Hook attack teardown into major lifecycle/teardown paths (destructor, divide, delete variants, ingest/fuse/lyse, transformation) and restore attack links on MultiCellDS resume.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
modules/PhysiCell_MultiCellDS.cpp	Restores attack relationships when recreating simulation state.
core/PhysiCell_standard_models.cpp	Switches attack start/end to centralized begin/end helpers.
core/PhysiCell_cell.h	Adds new state (attacked_by) and declares new attack management APIs.
core/PhysiCell_cell.cpp	Implements bidirectional attack bookkeeping and adds teardown calls across cell lifecycle events.

Suppressed comments (2)

core/PhysiCell_cell.cpp:3537

• remove_self_from_attacked() reads pTarget->phenotype.cell_interactions.pAttackTarget to decide whether to detach the shared spring. During standard_cell_cell_interactions(), pTarget's pAttackTarget can be modified concurrently by another thread, so this cross-thread read introduces a data race.

	pTarget->remove_attacker( this );
	// mutual attackers share one spring, so only drop it once both are done
	if( pTarget->phenotype.cell_interactions.pAttackTarget != this )
	{ detach_cells_as_spring( this , pTarget ); }

core/PhysiCell_cell.cpp:3599

• end_attack() reads pTarget->phenotype.cell_interactions.pAttackTarget to decide whether to detach the shared spring. Since standard_cell_cell_interactions() runs in a parallel loop and pTarget can concurrently start/end an attack, this read can race with a write on another thread.

	pTarget->remove_attacker( pAttacker );
	// two cells can attack each other, and attach_cell_as_spring dedupes, so the
	// two attacks share ONE spring. Only drop it once nobody still needs it.
	if( pTarget->phenotype.cell_interactions.pAttackTarget != pAttacker )
	{ detach_cells_as_spring( pAttacker , pTarget ); }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

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

Suppressed comments (1)

core/PhysiCell_cell.cpp:3552

• remove_self_from_attacked() clears this cell’s pAttackTarget and then calls into the target (remove_attacker / detach) without any synchronization on pAttackTarget. This function is invoked on cells other than the current OpenMP iteration owner (e.g., ingest_cell()/fuse_cell() call pCell_to_eat->remove_self_from_attacked() inside the parallel standard_cell_cell_interactions loop), so this becomes a cross-thread write to pAttackTarget and can race with that cell’s own reads of pAttackTarget during its update.

To avoid undefined behavior, pAttackTarget needs consistent synchronization for both readers and writers (e.g., a dedicated named omp critical guarding pAttackTarget access, or an atomic/handle-based design).

	Cell* pTarget = phenotype.cell_interactions.pAttackTarget;
	if( pTarget == NULL )
	{ return; }
	phenotype.cell_interactions.pAttackTarget = NULL;
	pTarget->remove_attacker( this );