False positive `A managed+dirty entity can not be scheduled for insertion` exception due to `spl_object_id()` collision

[eXsio]

Bug Report

Q	A
Version	3.5.0

Summary

This one is going to be borderline impossible to create a replication for, but I'll try to explain what I think is happening:

Doctrine uses spl_object_id() PHP function to uniquely identify an Entity within the UnitOfWork. The problem with that function is that it provides unique values for all objects that are currently referenced anywhere and haven't been GC'd. So, in theory, it can produce duplicate values, if the Object was there, but was GC'd. This theory became reality for me recently.

I'm using PHP 8.4, Symfony 7.3 and Doctrine 3.5.0. When switching from dev to prod envs in Symfony I've began to experience a weird "A managed+dirty entity can not be scheduled for insertion" exceptions from within Doctrine.

Upon further investigation it was obvious that my code is correct and something weird is happening with Doctrine, because my code was extremely straightforward:

• Instantiate a new Entity
• fill it with data
• call persist() on it

It turned out that, in the process of handling a Request, I had another Entity that PHP assigned the same value of spl_object_id(), but that Entity was deleted and un-referenced by everything in the code, so PHP has GC'd it. Upon instantiating my new Entity, PHP reused the same value of spl_object_id() and the following UnitOfWork piece of code threw an Exception:

https://github.com/doctrine/orm/blob/200a505f36b5bbdb9dcb3099adc37315badf1318/src/UnitOfWork.php#L1390-L1392

So, to sum up, it is perfectly possible to stumble upon this exception, even using this simple code:

$oldEntity = $em->find(EntityX::class, $oldEntityId);
$em->remove($oldEntity);
$em->flush();
$newEntity = new EntityY();
$em->persist($newEntity); //if PHP, for any reason, decides to reuse spl_object_id() value, Doctrine will throw a false-positive managed+dirty exception

My Workaround

I've created a Doctrine Listener on a preRemove Event and created an array that I store all removed Entities into. I never read from it, I'm just making sure that the object is referenced somewhere, so that it doesn't get GC'd and it's spl_object_id() value isn't available for another object. This can potentially have negative memory consumption impact, but I have no other choice.

Proposed fixes

There is a number of things that can be done here, but the most important thing is to acknowledge that spl_object_id() can potentially produce collisions and never treat it as an absolutely unique value.

1. The simplest thing would be to just get rid of the managed+dirty exception altogether, acknowledging that the check can be invalid and lead to false positives. Alternatively, we could assume that, if this check returns true, we are in the current scenario, we clear up the $originalEntityData and assume that the later/newer Entity has priority over the older one (and log a Warning).
2. Another thing (would work, but would be potentially memory inefficient) would be to keep references to every Entity that was ever touched by the UnitOfWork so that we make sure that the spl_object_id() doesn't produce collisions, at least in the context of Doctrine's world.
3. The $originalEntityData could be cleaned as a part of Entity Deletion process. This would help if the deletion was done through EntityManager::remove(), but still could cause failures if a QueryBuilder::delete was used to bulk-remove Entities.
4. Add destructors to Proxy objects (don't know if technically feasible, don't know the Doctrine src that good) so that the UoW is cleaned up properly after an Entity is GC'd.

Current behavior

"A managed+dirty entity can not be scheduled for insertion" Exception can potentially be thrown for an Entity that was never touched by an Entity Manager, depending on what was already done by the Unit of Work:

$instance = new Entity();
$em->persist($instance);

Expected behavior

Entity is persisted without exceptions.

How to reproduce

Impossible to reliably reproduce, because this bug depends on a number of different factors (like PHP behavior, frameworks used etc) and occurrs randomly (at lest for me).

[pich]

+1

[greg0ire]

Regarding your claim that this cannot be reproduced, wouldn't gc_collect_cycles() help with that? Also, what if $oldEntity is not part of a cycle in the first place? Wouldn't its Id get immediately freed?

Regarding your claim that the simple code you mentioned would reproduce the issue, wouldn't you need to unset($oldEntity); for the id to be freed?

If this is as simple as you say it is, I'm a bit surprised that you are the first to experience this. Are there no existing bug reports about this? I'm on my phone. Do you think it could have been introduced recently?

Note that in the past, we used spl_object_hash(), switching back to that might be a solution.

@pich please use emoji reactions instead of +1

[eXsio]

I understand that my examples may not fully describe the issue. I've stumbled upon it while working on a pretty complex system and it occurred only in prod env for some reason. I don't understand the internal logic of PHP's oid generation and what it actually takes to reliably generate a collision.

The things I know for sure was that there was a oid collision and that keeping an object reference on the side fixed it. From what I understand both object hash and id suffer from collision possibility. (given that one od the objects gets GC'd), as per PHP documentation.

It may just as well be a rare edge case, but it is possible.

[greg0ire]

Maybe scheduleForDelete() should act on originalEntityData instead of waiting for executeDeletions() to do so 🤔

[stof]

Note that in the past, we used spl_object_hash(), switching back to that might be a solution.

This suffers exactly form the same issue, given that spl_object_hash() is derived from the object id: https://github.com/php/php-src/blob/336fbf09d77ecbf0dddf47c32e91989b6c3e3216/ext/spl/php_spl.c#L664-L680

[beberlei]

EntityManager::remove & flush should cleanup all spl_object_id references already, so either that does not work anymore or we have a more complicated scenario at hand here