Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Workflow] State Contamination in Marking Stores due to Class-based Getter Cache#62199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
carsonbot commentedOct 29, 2025
Hey! Thanks for your PR. You are targeting branch "7.4" but it seems your PR description refers to branch "7.4 bug fixes". Cheers! Carsonbot |
| privatestaticfunctiongetSubjectIdentity(object$subject):string | ||
| { | ||
| returnsprintf('%s_%s',$subject::class,spl_object_id($subject)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
even this is not safe, as object ids can be reused after an object is destructed (which is likely to happen in the case of a messenger consumer, otherwise you would be leaking memory)
siganushkaOct 29, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
even this is not safe, as object ids can be reused after an object is destructed (which is likely to happen in the case of a messenger consumer, otherwise you would be leaking memory)
In my use case, when dealing with multiple instances of the same object, the value ofgetMarking is still influenced by the first object, even if it is the same process.
$foo1 =newPost();$foo1->setStatus('draft');$foo2 =newPost();$foo2->setStatus('published');$testStateMachine->getMarking($foo1)->getPlaces();// actual: ['draft' => 1]$testStateMachine->getMarking($foo2)->getPlaces();// actual: ['draft' => 1] expected: ['published' => 1]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Are you talking about the existing code or your patched code ?
I'm not saying that the existing code is correct. But your patch does not ensure correctness either AFAICT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Are you talking about the existing code or your patched code ?
I'm not saying that the existing code is correct. But your patch does not ensure correctness either AFAICT.
Existing code! I fixed this issue in the patch code.
$foo1 =newPost();$foo1->setStatus('draft');$foo2 =newPost();$foo2->setStatus('published');$testStateMachine->getMarking($foo1)->getPlaces();// actual: ['draft' => 1]$testStateMachine->getMarking($foo2)->getPlaces();// actual: ['draft' => 1] expected: ['published' => 1]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I fixed the implementation.
Thank you@siganushka. |
99bbae9 intosymfony:7.4Uh oh!
There was an error while loading.Please reload this page.
Caching the getter was implemented in 6.4 in#50974 So this fix needs to be merged in 6.4 rather than in 7.4 only. |
IIUC, we only cached the type of getter, not the actual getter closure, so that's fine there (and in 7.3 also) |
Indeed.#60114 changed the cached information in a broken way, introducing that bug. And btw, the type declaration for properties storing the cache is wrong now as it has not been updated when changing the cached info. |
…ache (nicolas-grekas)This PR was merged into the 7.4 branch.Discussion----------[Workflow] State contamination due to class-based setter cache| Q | A| ------------- | ---| Branch? | 7.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Issues | -| License | MITForgotten in#62199Commits-------2bbc09b [Workflow] State contamination due to class-based setter cache
Uh oh!
There was an error while loading.Please reload this page.
State Contamination in Marking Stores due to Class-based Getter Cache
The issue manifests in Marking Store configurations (e.g.,
MethodMarkingStore), tracing back to a logic flaw in the core Workflow component's state-accessing mechanism.The
getMarking()method caches the state-accessing Getter Closure based solely on the class name (Subject::class).In a concurrent, multi-subject environment (e.g., Messenger Workers), this leads to state contamination:
A Worker processes Subject 1, successfully transitioning its state from first_place to second_place.
The cached Getter Closure is created or updated, capturing a reference or memory state related to second_place.
When the same Worker processes Subject 2 (a different instance, whose actual state is first_place in the database), the cached Getter is used.
This Getter incorrectly returns the state related to Subject 1's final marking (second_place), despite Subject 2's data being correct.
Consequently,
getMarking()reports a marking of ["second_place":1], causing valid transitions from first_place to be incorrectly blocked.