Spring attachments: pair attach/detach is not atomic, and .size() is read outside the lock

[drbergman]

Two data races remain in the spring-attachment code after #428. Neither can crash — #428 removed the crashing dereference — but both are reachable in any model with a non-zero detachment_rate, and one of them produces physically wrong results silently.

Line numbers are against development at 39be5bd3.

---

1. Pair attach/detach is not atomic, producing one-way springs

attach_cells_as_spring() and detach_cells_as_spring() each take the unnamed critical twice — once per cell — rather than once for the pair (core/PhysiCell_cell.cpp:3579 and :3593):

void attach_cells_as_spring( Cell* pCell_1, Cell* pCell_2 )
{
	pCell_1->attach_cell_as_spring( pCell_2 );   // critical #1
	pCell_2->attach_cell_as_spring( pCell_1 );   // critical #2
	return; 
}

The lock lives inside Cell::attach_cell_as_spring() (:3376) and Cell::detach_cell_as_spring() (:3417), so another thread can run to completion between the two halves. Both functions are called from dynamic_spring_attachments(), which runs inside the mechanics parallel-for (core/PhysiCell_cell_container.cpp:226), so two threads can be attaching and detaching the same pair concurrently:

T1: A.attach(B)        -> A has B
T2: A.detach(B)        -> A loses B
T2: B.detach(A)        -> not found, no-op
T1: B.attach(A)        -> B has A, A does not have B

The result is a one-way spring: B lists A, A does not list B.

Why this is not cosmetic

standard_elastic_contact_function() applies force to pC1 only (core/PhysiCell_standard_models.cpp:997):

axpy( &(pC1->velocity) , effective_attachment_elastic_constant , displacement ); 

and the force loop iterates each cell's own list (PhysiCell_cell_container.cpp:237). Symmetry of the spring force therefore comes entirely from both cells holding each other. A one-way spring means one cell accelerates toward the other while the other feels nothing — Newton's third law is violated and momentum is not conserved for that pair.

Observed

I added a symmetry audit after the spring pass, counting attachments where B ∈ A.spring_attachments but A ∉ B.spring_attachments, and ran it against a build with #428 applied:

• A model with detachment_rate = 0 for all cell types: 0 one-way springs across full runs.
• The same model with detachment_rate = 0.5: 964 and 933 one-way springs in two runs, present in ~12% of mechanics steps, up to 3 at once, each persisting ~10 consecutive steps before self-healing (they clear when the holding cell next detaches).

The detector was validated with a positive control — deliberately removing the second half of attach_cells_as_spring() made it fire on essentially every step (7201 hits), confirming it can see asymmetry when asymmetry exists.

So the failure is real but bounded: transient, low-count, and impossible in models that never detach.

Possible fix

Make the pair operation atomic — one critical section covering both halves, rather than one per cell. That requires care: Cell::attach_cell_as_spring() / detach_cell_as_spring() take the critical themselves and OpenMP criticals are not reentrant, so the lock cannot simply be hoisted around the existing calls. Splitting each into a locked public entry point and an unlocked internal helper would work.

---

2. .size() is read outside the lock in the attachment half

dynamic_spring_attachments() reads spring-attachment sizes with no lock in three places, while other threads may be inside push_back() / pop_back() on those same vectors:

• core/PhysiCell_standard_models.cpp:1440 — pCell->state.spring_attachments.size()
• :1450 — pTest->state.spring_attachments.size(), i.e. another cell's vector
• :1461 — pCell->state.spring_attachments.size() again

    // check if I have max number of attachments 
    if( pCell->state.spring_attachments.size() >= phenotype.mechanics.maximum_number_of_attachments )
    { return; }

This is a data race in the formal sense. On libstdc++/libc++ vector::size() is end_ - begin_, two pointer loads that a concurrent reallocation updates non-atomically, so a torn read can yield a nonsense length. It cannot crash here — the values are only compared numerically, never used to index — but the comparison can be made against a garbage size. #428 made this function crash-free; it did not make it race-free.

Related: maximum_number_of_attachments is advisory

Line 1450 checks the target's remaining capacity, then attach_cells_as_spring() is called at :1460. Cell::attach_cell_as_spring() re-checks only for duplicates inside the critical (PhysiCell_cell.cpp:3380-3387), never the cap. The check and the mutation are therefore not atomic, and a cell can be pushed past maximum_number_of_attachments by concurrent attachers.

I looked for this specifically and saw zero cap violations in the runs above, so it is unconfirmed in practice — the test model may simply never approach its cap. Flagging it as a latent consequence of the same non-atomicity rather than an observed bug.

Possible fix

Move the capacity test inside Cell::attach_cell_as_spring(), alongside the existing duplicate check, so the decision and the push_back happen under one lock. That subsumes the racy reads at :1450 and :1461, and makes the cap an invariant rather than a hint.

---

Notes

• Both issues predate Take spring attachments under the lock before iterating them #428 and are unchanged by it; Take spring attachments under the lock before iterating them #428 fixed a separate crashing race in the detachment loop.
• Both are unreachable in models where no cell ever detaches (detachment_rate = 0 everywhere), which is a common configuration.
• I have not run ThreadSanitizer against the post-Take spring attachments under the lock before iterating them #428 code; the analysis above is from the source plus the symmetry audit described. TSan would be the natural next step and should flag the .size() reads directly.
• Reproduction materials for the audit can be shared if useful.

[drbergman]

Another option that feels a little crazy, but hear me out: a separate global std::unordered_set spring_attachments that holds all spring attachments.

• Detachments just iterates this list, using the sum of the detachment rates as the effective rate (equivalent to what we do now up to first order)
• attachments then iterates over all cells and their neighbors (as before, but only after detachments are done)
  • if the current pair is already in the spring_attachments, skip
  • otherwise, flip the coin (again weighting by the sum of their attachment rates, equivalent to what we do now up to first order)
  • attaching is just adding this to the set; which is race-safe since it is a hash map
• In each of these operations, the current number of attachments on a single cell, still held by the individual cells, is updated through adding/removing from the set

I'm still working out the teardown operations so that we don't need to iterate over all the springs.