From 049341866e6e4f780aa68dfb697d48aefb327b36 Mon Sep 17 00:00:00 2001 From: Daniel Bergman Date: Thu, 6 Aug 2026 15:34:03 -0400 Subject: [PATCH 1/6] Make the attack link symmetric, following spring-attachment semantics 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, but pAttackTarget was introduced after that audit and was never added to it. standard_cell_cell_interactions does release an attack when it sees pTarget->phenotype.death.dead, so an attacker normally detaches long before its target is removed. That test cannot fire when the target is freed with no intervening mechanics step, which happens whenever a death phase exits on its first advance_cycle: Cell::advance_bundled_phenotype_functions calls check_for_death and then advance_cycle in the same invocation, so a cell can go alive -> dead -> flagged_for_removal -> freed inside one phenotype update. The attacker then reads, and writes through, freed memory on its next step. ingest_cell and fuse_cell of a live cell, and direct delete_cell/die() from user code, reach the same state by other routes. Give Cell_State an attacked_by list mirroring pAttackTarget, maintained the way the two halves of a spring attachment are: - add_attacker / remove_attacker maintain one half - remove_all_attackers / remove_self_from_attacked tear down both - begin_attack / end_attack keep pAttackTarget, attacked_by and the spring in lockstep, mirroring attach_cells_as_spring / detach_cells_as_spring The list lives in state rather than phenotype so that it survives convert_to_cell_definition, which replaces phenotype wholesale, and is not copied to daughters by divide. Teardown is hooked at every site that already calls remove_all_spring_attachments: ~Cell, divide, both delete_cell variants, ingest_cell, fuse_cell and lyse_cell. lyse_cell was additionally missing remove_all_spring_attachments and set flag_for_removal before dead = true, unlike ingest_cell and fuse_cell; both brought into line. Two cells can attack each other and attach_cell_as_spring dedupes, so a mutual attack shares one spring. end_attack now drops it only once neither direction needs it. The MultiCellDS resume path restores both halves of the link. Adding a member to Cell_State changes sizeof(Cell), and the Makefiles declare no header dependencies, so a clean build is required after taking this change. Co-Authored-By: Claude Opus 5 --- core/PhysiCell_cell.cpp | 120 ++++++++++++++++++++++++++++- core/PhysiCell_cell.h | 15 ++++ core/PhysiCell_standard_models.cpp | 10 +-- modules/PhysiCell_MultiCellDS.cpp | 3 + 4 files changed, 139 insertions(+), 9 deletions(-) diff --git a/core/PhysiCell_cell.cpp b/core/PhysiCell_cell.cpp index 1f3e2646b..0eea9312f 100644 --- a/core/PhysiCell_cell.cpp +++ b/core/PhysiCell_cell.cpp @@ -238,12 +238,15 @@ Cell_State::Cell_State() neighbors.resize(0); spring_attachments.resize(0); + attacked_by.resize(0); + orientation.resize( 3 , 0.0 ); simple_pressure = 0.0; attached_cells.clear(); spring_attachments.clear(); + attacked_by.clear(); number_of_nuclei = 1; @@ -457,6 +460,8 @@ Cell::~Cell() { // release any attached cells (as of 1.7.2 release) this->remove_all_attached_cells(); + this->remove_all_attackers(); + this->remove_self_from_attacked(); // 1.11.0 this->remove_all_spring_attachments(); @@ -551,6 +556,10 @@ Cell* Cell::divide( ) // make sure ot remove adhesions remove_all_attached_cells(); + // the springs below include the one holding an attack together, so end the + // attack rather than leave it spring-less + remove_all_attackers(); + remove_self_from_attacked(); remove_all_spring_attachments(); // version 1.10.3: @@ -651,6 +660,7 @@ Cell* Cell::divide( ) set_total_volume(phenotype.volume.total); // child->set_phenotype( phenotype ); + // pAttackTarget was cleared above, so the daughter inherits a NULL one child->phenotype = phenotype; if (child->phenotype.intracellular){ @@ -1144,6 +1154,14 @@ void Cell::convert_to_cell_definition( Cell_Definition& cd ) Geometry cell_geometry = phenotype.geometry; Molecular cell_molecular = phenotype.molecular; Custom_Cell_Data cell_custom_data = custom_data; + + // pAttackTarget is phenotype state, and the assignment below replaces it, so + // end this cell's own attack. attacked_by and the spring are in state, which + // transformation preserves, so attacks against this cell continue. + remove_self_from_attacked(); + // should we also remove all attackers? That would be a change in behavior, so for now we don't. + // remove_all_attackers(); + // use the cell defaults; type = cd.type; type_name = cd.name; @@ -1210,6 +1228,8 @@ void delete_cell( int index ) // release any attached cells (as of 1.7.2 release) pDeleteMe->remove_all_attached_cells(); + pDeleteMe->remove_all_attackers(); + pDeleteMe->remove_self_from_attacked(); // 1.11.0 pDeleteMe->remove_all_spring_attachments(); @@ -1244,6 +1264,8 @@ void delete_cell_original( int index ) // before June 11, 2020 // release any attached cells (as of 1.7.2 release) (*all_cells)[index]->remove_all_attached_cells(); + (*all_cells)[index]->remove_all_attackers(); + (*all_cells)[index]->remove_self_from_attacked(); // 1.11.0 (*all_cells)[index]->remove_all_spring_attachments(); @@ -1473,6 +1495,8 @@ void Cell::ingest_cell( Cell* pCell_to_eat ) // things that have their own thread safety pCell_to_eat->flag_for_removal(); pCell_to_eat->remove_all_attached_cells(); + pCell_to_eat->remove_all_attackers(); + pCell_to_eat->remove_self_from_attacked(); pCell_to_eat->remove_all_spring_attachments(); return; @@ -1641,6 +1665,8 @@ void Cell::fuse_cell( Cell* pCell_to_fuse ) // things that have their own thread safety pCell_to_fuse->flag_for_removal(); pCell_to_fuse->remove_all_attached_cells(); + pCell_to_fuse->remove_all_attackers(); + pCell_to_fuse->remove_self_from_attacked(); pCell_to_fuse->remove_all_spring_attachments(); return; @@ -1652,12 +1678,12 @@ void Cell::lyse_cell( void ) if( phenotype.volume.total < 1e-15 ) { return; } + // mark it as dead (before flagging, to match ingest_cell and fuse_cell) + phenotype.death.dead = true; + // flag for removal flag_for_removal(); // should be safe now - // mark it as dead - phenotype.death.dead = true; - // set secretion and uptake to zero phenotype.secretion.set_all_secretion_to_zero( ); phenotype.secretion.set_all_uptake_to_zero( ); @@ -1672,6 +1698,9 @@ void Cell::lyse_cell( void ) // remove all adhesions remove_all_attached_cells(); + remove_all_attackers(); + remove_self_from_attacked(); + remove_all_spring_attachments(); // set volume to zero set_total_volume( 0.0 ); @@ -3457,6 +3486,58 @@ void Cell::remove_all_spring_attachments( void ) return; } +void Cell::add_attacker( Cell* pAddMe ) +{ + #pragma omp critical + { + if( std::find( state.attacked_by.begin() , state.attacked_by.end() , pAddMe ) + == state.attacked_by.end() ) + { state.attacked_by.push_back( pAddMe ); } + } + return; +} + +void Cell::remove_attacker( Cell* pRemoveMe ) +{ + #pragma omp critical + { + auto search = std::find( state.attacked_by.begin() , state.attacked_by.end() , pRemoveMe ); + if( search != state.attacked_by.end() ) + { + // copy last entry over it, then shrink by one + *search = state.attacked_by.back(); + state.attacked_by.pop_back(); + } + } + return; +} + +void Cell::remove_all_attackers( void ) +{ + for( int i = 0; i < state.attacked_by.size() ; i++ ) + { + Cell* pAttacker = state.attacked_by[i]; + if( pAttacker->phenotype.cell_interactions.pAttackTarget == this ) + { pAttacker->phenotype.cell_interactions.pAttackTarget = NULL; } + detach_cells_as_spring( pAttacker , this ); + } + state.attacked_by.clear(); + return; +} + +void Cell::remove_self_from_attacked( void ) +{ + Cell* pTarget = phenotype.cell_interactions.pAttackTarget; + if( pTarget == NULL ) + { return; } + phenotype.cell_interactions.pAttackTarget = NULL; + 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 ); } + return; +} + void attach_cells( Cell* pCell_1, Cell* pCell_2 ) { @@ -3483,6 +3564,39 @@ void detach_cells_as_spring( Cell* pCell_1 , Cell* pCell_2 ) { pCell_1->detach_cell_as_spring( pCell_2 ); pCell_2->detach_cell_as_spring( pCell_1 ); + return; +} + +void begin_attack( Cell* pAttacker , Cell* pTarget ) +{ + // no self-attack, and nothing to do without both ends + if( pAttacker == NULL || pTarget == NULL || pAttacker == pTarget ) + { return; } + + // a cell attacks at most one target at a time. if it is already attacking + // something, close that attack out cleanly rather than orphaning the link. + if( pAttacker->phenotype.cell_interactions.pAttackTarget != NULL ) + { pAttacker->remove_self_from_attacked(); } + + pAttacker->phenotype.cell_interactions.pAttackTarget = pTarget; + pTarget->add_attacker( pAttacker ); + // spring-link these cells + attach_cells_as_spring( pAttacker , pTarget ); + return; +} + +void end_attack( Cell* pAttacker , Cell* pTarget ) +{ + if( pAttacker == NULL || pTarget == NULL ) + { return; } + + if( pAttacker->phenotype.cell_interactions.pAttackTarget == pTarget ) + { pAttacker->phenotype.cell_interactions.pAttackTarget = NULL; } + 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 ); } return; } diff --git a/core/PhysiCell_cell.h b/core/PhysiCell_cell.h index 5b7c52d96..278ddf3b7 100644 --- a/core/PhysiCell_cell.h +++ b/core/PhysiCell_cell.h @@ -145,6 +145,10 @@ class Cell_State std::vector attached_cells; std::vector spring_attachments; + // reverse of phenotype.cell_interactions.pAttackTarget. in state, not + // phenotype, so it survives convert_to_cell_definition() + std::vector attacked_by; + std::vector neighbors; std::vector orientation; @@ -239,6 +243,12 @@ class Cell : public Basic_Agent void detach_cell_as_spring( Cell* pRemoveMe ); // done void remove_all_spring_attachments( void ); // done + void add_attacker( Cell* pAddMe ); // pAddMe has started attacking me + void remove_attacker( Cell* pRemoveMe ); // pRemoveMe has stopped attacking me + // call these wherever remove_all_spring_attachments() is called + void remove_all_attackers( void ); // everyone attacking me stops + void remove_self_from_attacked( void ); // I stop attacking whomever I attack + // I want to eventually deprecate this, by ensuring that // critical BioFVM and PhysiCell data elements are synced when they are needed @@ -296,6 +306,11 @@ void detach_cells( Cell* pCell_1 , Cell* pCell_2 ); void attach_cells_as_spring( Cell* pCell_1, Cell* pCell_2 ); void detach_cells_as_spring( Cell* pCell_1 , Cell* pCell_2 ); +// maintain pAttackTarget, attacked_by, and the spring together. use these +// rather than writing pAttackTarget directly. +void begin_attack( Cell* pAttacker , Cell* pTarget ); +void end_attack( Cell* pAttacker , Cell* pTarget ); + std::vector find_nearby_cells( Cell* pCell ); // new in 1.8.0 std::vector find_nearby_interacting_cells( Cell* pCell ); // new in 1.8.0 diff --git a/core/PhysiCell_standard_models.cpp b/core/PhysiCell_standard_models.cpp index 4d170a406..396158585 100644 --- a/core/PhysiCell_standard_models.cpp +++ b/core/PhysiCell_standard_models.cpp @@ -1265,7 +1265,9 @@ void standard_cell_cell_interactions( Cell* pCell, Phenotype& phenotype, double { if( UniformRandom() < probability ) { - pCell->phenotype.cell_interactions.pAttackTarget = pTarget; + // sets pAttackTarget, registers pCell in pTarget's + // attacked_by, and spring-links the two + begin_attack( pCell , pTarget ); attacked = true; /* std::cout << "********* ********* ******** start atack **** " << PhysiCell_globals.current_time << std::endl; @@ -1273,8 +1275,6 @@ void standard_cell_cell_interactions( Cell* pCell, Phenotype& phenotype, double << "attack duration: " << pCell->phenotype.cell_interactions.attack_duration << " " << "attack damage rate: " << pCell->phenotype.cell_interactions.attack_damage_rate << std::endl; */ - // spring-link these cells - attach_cells_as_spring(pCell,pTarget); } } @@ -1341,9 +1341,7 @@ void standard_cell_cell_interactions( Cell* pCell, Phenotype& phenotype, double << "damage delivered: " << pCell->phenotype.cell_interactions.total_damage_delivered << std::endl; */ - detach_cells_as_spring(pCell,pTarget); - - pCell->phenotype.cell_interactions.pAttackTarget = NULL; + end_attack( pCell , pTarget ); } } diff --git a/modules/PhysiCell_MultiCellDS.cpp b/modules/PhysiCell_MultiCellDS.cpp index 40aac05e9..006455dea 100644 --- a/modules/PhysiCell_MultiCellDS.cpp +++ b/modules/PhysiCell_MultiCellDS.cpp @@ -2196,7 +2196,10 @@ int recreate_sim_state(std::string filename, Microenvironment& M, { if (cell->ID == pair.second) { + // restore both halves, or removing the target later will + // not clear this pointer (pair.first)->phenotype.cell_interactions.pAttackTarget = cell; + cell->add_attacker( pair.first ); if (debug_print) { std::cout << " cell ID=" << (pair.first)->ID << " attacking cell ID=" << cell->ID << std::endl; } break; From 7a81b97e102e48bf6312a12027d884f13b6ff3ec Mon Sep 17 00:00:00 2001 From: Daniel Bergman Date: Thu, 6 Aug 2026 20:05:38 -0400 Subject: [PATCH 2/6] Address review: attacked_by races, and restore the spring on resume Two issues raised by Copilot on #422. remove_all_attackers() walked state.attacked_by with no synchronisation while add_attacker / remove_attacker mutate it under an omp critical. It is reachable from ingest_cell / fuse_cell inside the parallel interactions loop, so a concurrent push_back could reallocate the vector mid-iteration. It cannot simply take the critical for the duration of the loop: detach_cells_as_spring() takes the same unnamed critical, and OpenMP criticals are not reentrant, so that would deadlock. Instead swap the list out under the lock and walk the local copy. The same review flagged the mutual-attack check in end_attack and remove_self_from_attacked, which read another cell's pAttackTarget with no synchronisation. Ask the equivalent question of our own attacked_by list instead, under the lock, via the new Cell::is_attacked_by(). "pTarget attacks me" and "pTarget is in my attacked_by" are the same statement given the invariant this change maintains. The MultiCellDS resume path restored pAttackTarget and attacked_by but not the attack spring, leaving a resumed run with an attack and no spring behind it -- which is exactly the inconsistency the rest of this change exists to prevent. Use begin_attack() so all three records are restored together. Co-Authored-By: Claude Opus 5 --- core/PhysiCell_cell.cpp | 32 +++++++++++++++++++++++++------ core/PhysiCell_cell.h | 1 + modules/PhysiCell_MultiCellDS.cpp | 8 ++++---- 3 files changed, 31 insertions(+), 10 deletions(-) diff --git a/core/PhysiCell_cell.cpp b/core/PhysiCell_cell.cpp index 0eea9312f..dc7346f8c 100644 --- a/core/PhysiCell_cell.cpp +++ b/core/PhysiCell_cell.cpp @@ -3514,17 +3514,35 @@ void Cell::remove_attacker( Cell* pRemoveMe ) void Cell::remove_all_attackers( void ) { - for( int i = 0; i < state.attacked_by.size() ; i++ ) + // Swap the list out under the same lock add_attacker / remove_attacker use, then + // walk the local copy. Holding the lock across the loop is not an option: + // detach_cells_as_spring() takes the same unnamed critical and OpenMP criticals + // are not reentrant, so that would deadlock. + std::vector attackers; + #pragma omp critical + { attackers.swap( state.attacked_by ); } + + for( int i = 0; i < attackers.size() ; i++ ) { - Cell* pAttacker = state.attacked_by[i]; + Cell* pAttacker = attackers[i]; if( pAttacker->phenotype.cell_interactions.pAttackTarget == this ) { pAttacker->phenotype.cell_interactions.pAttackTarget = NULL; } detach_cells_as_spring( pAttacker , this ); } - state.attacked_by.clear(); return; } +bool Cell::is_attacked_by( Cell* pCell ) +{ + bool found = false; + #pragma omp critical + { + found = std::find( state.attacked_by.begin() , state.attacked_by.end() , pCell ) + != state.attacked_by.end(); + } + return found; +} + void Cell::remove_self_from_attacked( void ) { Cell* pTarget = phenotype.cell_interactions.pAttackTarget; @@ -3532,8 +3550,10 @@ void Cell::remove_self_from_attacked( void ) { return; } phenotype.cell_interactions.pAttackTarget = NULL; pTarget->remove_attacker( this ); - // mutual attackers share one spring, so only drop it once both are done - if( pTarget->phenotype.cell_interactions.pAttackTarget != this ) + // mutual attackers share one spring, so only drop it once both are done. + // "does pTarget attack me?" is asked of our own attacked_by, which we can lock, + // rather than by reading pTarget's pAttackTarget unsynchronised. + if( !is_attacked_by( pTarget ) ) { detach_cells_as_spring( this , pTarget ); } return; } @@ -3595,7 +3615,7 @@ void end_attack( Cell* pAttacker , Cell* pTarget ) 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 ) + if( !pAttacker->is_attacked_by( pTarget ) ) { detach_cells_as_spring( pAttacker , pTarget ); } return; } diff --git a/core/PhysiCell_cell.h b/core/PhysiCell_cell.h index 278ddf3b7..4c5e17f6e 100644 --- a/core/PhysiCell_cell.h +++ b/core/PhysiCell_cell.h @@ -248,6 +248,7 @@ class Cell : public Basic_Agent // call these wherever remove_all_spring_attachments() is called void remove_all_attackers( void ); // everyone attacking me stops void remove_self_from_attacked( void ); // I stop attacking whomever I attack + bool is_attacked_by( Cell* pCell ); // is pCell currently attacking me? // I want to eventually deprecate this, by ensuring that // critical BioFVM and PhysiCell data elements are synced when they are needed diff --git a/modules/PhysiCell_MultiCellDS.cpp b/modules/PhysiCell_MultiCellDS.cpp index 006455dea..6ac94df5f 100644 --- a/modules/PhysiCell_MultiCellDS.cpp +++ b/modules/PhysiCell_MultiCellDS.cpp @@ -2196,10 +2196,10 @@ int recreate_sim_state(std::string filename, Microenvironment& M, { if (cell->ID == pair.second) { - // restore both halves, or removing the target later will - // not clear this pointer - (pair.first)->phenotype.cell_interactions.pAttackTarget = cell; - cell->add_attacker( pair.first ); + // restore all three records, not just the pointer: the spring + // is not serialised, so without this a resumed run starts with + // an attack that has no spring behind it + begin_attack( pair.first , cell ); if (debug_print) { std::cout << " cell ID=" << (pair.first)->ID << " attacking cell ID=" << cell->ID << std::endl; } break; From 12720aa91a8c1aae2189a84bcc2b7542023d7eba Mon Sep 17 00:00:00 2001 From: Daniel Bergman Date: Fri, 7 Aug 2026 07:54:48 -0400 Subject: [PATCH 3/6] Fix a typo in an adjacent comment "make sure ot remove adhesions" -> "to". Pre-existing, but it sits in the hunk this branch touches. Co-Authored-By: Claude Opus 5 --- core/PhysiCell_cell.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/PhysiCell_cell.cpp b/core/PhysiCell_cell.cpp index dc7346f8c..777bc20ba 100644 --- a/core/PhysiCell_cell.cpp +++ b/core/PhysiCell_cell.cpp @@ -554,7 +554,7 @@ Cell* Cell::divide( ) // phenotype.flagged_for_division = false; // phenotype.flagged_for_removal = false; - // make sure ot remove adhesions + // make sure to remove adhesions remove_all_attached_cells(); // the springs below include the one holding an attack together, so end the // attack rather than leave it spring-less From 6c79881aa345cafc9f4dc7a64f74360013cc0545 Mon Sep 17 00:00:00 2001 From: Daniel Bergman Date: Fri, 7 Aug 2026 10:12:04 -0400 Subject: [PATCH 4/6] Read pAttackTarget once in the effector-attack block The block tested phenotype.cell_interactions.pAttackTarget for NULL and then loaded it again into a local. Those are two separate loads of a field another thread can clear: remove_all_attackers() nulls an attacker's pAttackTarget when its target is eaten, fused or lysed, and that runs from standard_cell_cell_interactions inside the mechanics parallel-for. If the clear lands between the test and the reload, the local is NULL and attack_cell() dereferences it -- it only guards against attacking itself. Take one snapshot and use it throughout, so the block either sees a target it can safely act on or sees NULL and does nothing. This narrows rather than removes the underlying data race: the field is a plain Cell* read and written across threads. Making that formally race-free needs either the read under the same critical as the write, which serialises the mechanics hot path, or std::atomic, which changes the type of a public field. Co-Authored-By: Claude Opus 5 --- core/PhysiCell_standard_models.cpp | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/core/PhysiCell_standard_models.cpp b/core/PhysiCell_standard_models.cpp index 396158585..9cc7589bc 100644 --- a/core/PhysiCell_standard_models.cpp +++ b/core/PhysiCell_standard_models.cpp @@ -1316,11 +1316,14 @@ void standard_cell_cell_interactions( Cell* pCell, Phenotype& phenotype, double // move effector attack here. - if( pCell->phenotype.cell_interactions.pAttackTarget != NULL ) + // read the target once. another thread can clear this field while we are in + // here -- remove_all_attackers() does exactly that when our target is eaten, + // fused or lysed -- and attack_cell() dereferences without a null check, so + // testing the field and then reloading it can hand it a NULL. + Cell* pAttackTarget = pCell->phenotype.cell_interactions.pAttackTarget; + if( pAttackTarget != NULL ) { - Cell* pTarget = pCell->phenotype.cell_interactions.pAttackTarget; - - pCell->attack_cell(pTarget,dt); + pCell->attack_cell(pAttackTarget,dt); attacked = true; // attacked at least one cell in this time step // attack_cell @@ -1330,18 +1333,18 @@ void standard_cell_cell_interactions( Cell* pCell, Phenotype& phenotype, double probability = dt / (1e-15 + pCell->phenotype.cell_interactions.attack_duration); - if( UniformRandom() < probability || pTarget->phenotype.death.dead ) + if( UniformRandom() < probability || pAttackTarget->phenotype.death.dead ) { /* std::cout << "********* ********* ******** attack done **** " << PhysiCell_globals.current_time << " " << probability << " " - << "attack time: " << pTarget->state.total_attack_time << " " - << "damage: " << pTarget->phenotype.cell_integrity.damage << " " - << "dead? " << (int) pTarget->phenotype.death.dead << " " + << "attack time: " << pAttackTarget->state.total_attack_time << " " + << "damage: " << pAttackTarget->phenotype.cell_integrity.damage << " " + << "dead? " << (int) pAttackTarget->phenotype.death.dead << " " << "damage delivered: " << pCell->phenotype.cell_interactions.total_damage_delivered << std::endl; */ - end_attack( pCell , pTarget ); + end_attack( pCell , pAttackTarget ); } } From 9fbd031bc6523ab545ba46f7932581d933cb3947 Mon Sep 17 00:00:00 2001 From: Daniel Bergman Date: Fri, 7 Aug 2026 18:04:39 -0400 Subject: [PATCH 5/6] Defer teardown out of the mechanics parallel-for ingest_cell, fuse_cell and lyse_cell each flag their victim for removal and then immediately tear down its attachments, springs and attack links. All three run from standard_cell_cell_interactions, inside the mechanics parallel-for, so that teardown mutates other cells' state from a thread that does not own them. That is a real race, not a theoretical one. remove_all_spring_attachments walks state.spring_attachments by index with no lock, while attach_cell_as_spring and detach_cell_as_spring edit that same vector under the unnamed critical. detach is swap-and-pop: if the writer removes an entry below the walker's cursor, the tail element is moved into a slot the walker has already passed and never gets detached, leaving a neighbour holding a pointer to a cell that is about to be freed. A push_back that reallocates is worse. Instrumenting the walk to publish which cell each thread is tearing down, and checking for a collision in the locked mutators, finds this on stock interaction-sample at 8 threads in 3 of 6 seeds -- and on pristine development, so it predates the attack link entirely. The work was already redundant: flag_for_removal() queues the cell, and the serial cells_ready_to_die -> die() -> delete_cell() pass does the same four steps. Dropping the in-loop copies removes every unlocked walk from a parallel region; what remains there are locked mutations, which are mutually serialised. Teardowns walked halves, from ~1500 to ~900. Verified: 0 collisions in 6 of 6 seeds, and single-threaded output is bit-identical -- no cells.mat or microenvironment file differs. Co-Authored-By: Claude Opus 5 --- core/PhysiCell_cell.cpp | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/core/PhysiCell_cell.cpp b/core/PhysiCell_cell.cpp index 777bc20ba..76d90ab85 100644 --- a/core/PhysiCell_cell.cpp +++ b/core/PhysiCell_cell.cpp @@ -1492,12 +1492,13 @@ void Cell::ingest_cell( Cell* pCell_to_eat ) } - // things that have their own thread safety + // Teardown is deferred rather than done here. flag_for_removal() queues this + // cell, and the serial cells_ready_to_die -> die() -> delete_cell() pass does + // the same four steps off-thread. Doing them here means mutating other cells' + // lists from inside the mechanics parallel-for, where remove_all_* walks a + // vector unlocked while attach/detach_cell_as_spring edits it under the + // critical -- a real race, not a theoretical one. pCell_to_eat->flag_for_removal(); - pCell_to_eat->remove_all_attached_cells(); - pCell_to_eat->remove_all_attackers(); - pCell_to_eat->remove_self_from_attacked(); - pCell_to_eat->remove_all_spring_attachments(); return; } @@ -1663,11 +1664,13 @@ void Cell::fuse_cell( Cell* pCell_to_fuse ) } // things that have their own thread safety + // Teardown is deferred rather than done here. flag_for_removal() queues this + // cell, and the serial cells_ready_to_die -> die() -> delete_cell() pass does + // the same four steps off-thread. Doing them here means mutating other cells' + // lists from inside the mechanics parallel-for, where remove_all_* walks a + // vector unlocked while attach/detach_cell_as_spring edits it under the + // critical -- a real race, not a theoretical one. pCell_to_fuse->flag_for_removal(); - pCell_to_fuse->remove_all_attached_cells(); - pCell_to_fuse->remove_all_attackers(); - pCell_to_fuse->remove_self_from_attacked(); - pCell_to_fuse->remove_all_spring_attachments(); return; } @@ -1697,10 +1700,12 @@ void Cell::lyse_cell( void ) // remove all adhesions - remove_all_attached_cells(); - remove_all_attackers(); - remove_self_from_attacked(); - remove_all_spring_attachments(); + // Teardown is deferred rather than done here. flag_for_removal() queues this + // cell, and the serial cells_ready_to_die -> die() -> delete_cell() pass does + // the same four steps off-thread. Doing them here means mutating other cells' + // lists from inside the mechanics parallel-for, where remove_all_* walks a + // vector unlocked while attach/detach_cell_as_spring edits it under the + // critical -- a real race, not a theoretical one. // set volume to zero set_total_volume( 0.0 ); From 649dc13a9bfdab829c2504b3c549d4ad4006791c Mon Sep 17 00:00:00 2001 From: Daniel Bergman Date: Sat, 8 Aug 2026 14:55:40 -0400 Subject: [PATCH 6/6] Correct a comment the teardown deferral invalidated The snapshot comment named remove_all_attackers() running from ingest_cell/fuse_cell/lyse_cell as the thing that could clear the field mid-block. Deferring teardown to the serial removal pass removed that path, so the comment described a mechanism no longer in the tree. State what is actually true, and why the single load is kept anyway. Co-Authored-By: Claude Opus 5 --- core/PhysiCell_standard_models.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/PhysiCell_standard_models.cpp b/core/PhysiCell_standard_models.cpp index 9cc7589bc..2a57d44a7 100644 --- a/core/PhysiCell_standard_models.cpp +++ b/core/PhysiCell_standard_models.cpp @@ -1316,10 +1316,10 @@ void standard_cell_cell_interactions( Cell* pCell, Phenotype& phenotype, double // move effector attack here. - // read the target once. another thread can clear this field while we are in - // here -- remove_all_attackers() does exactly that when our target is eaten, - // fused or lysed -- and attack_cell() dereferences without a null check, so - // testing the field and then reloading it can hand it a NULL. + // read the target once. attack_cell() dereferences without a null check, so + // testing this field and then reloading it would let any concurrent clear + // hand it a NULL. One load is free and does not depend on which teardown + // paths currently run inside this loop. Cell* pAttackTarget = pCell->phenotype.cell_interactions.pAttackTarget; if( pAttackTarget != NULL ) {