Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged
nicolas-grekas merged 1 commit intosymfony:7.4fromsiganushka:7.4
Oct 29, 2025

Conversation

@siganushka
Copy link
Contributor

@siganushkasiganushka commentedOct 29, 2025
edited
Loading

State Contamination in Marking Stores due to Class-based Getter Cache

QA
Branch?7.4
Bug fix?yes
New feature?no
Deprecations?no
LicenseMIT

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.

ThegetMarking() 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.

$subject1 =newSubject('first_place');$subject2 =newSubject('second_place');$markingStore =newMethodMarkingStore(true);$marking1 =$markingStore->getMarking($subject1);$marking2 =$markingStore->getMarking($subject2);// Expected: ["first_place" => 1]// Actual: ["first_place" => 1]$marking1 ->getPlaces();// Expected: ["second_place" => 1]// Actual: ["first_place" => 1]$marking2 ->getPlaces();

@carsonbot
Copy link

Hey!

Thanks for your PR. You are targeting branch "7.4" but it seems your PR description refers to branch "7.4 bug fixes".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot


privatestaticfunctiongetSubjectIdentity(object$subject):string
{
returnsprintf('%s_%s',$subject::class,spl_object_id($subject));
Copy link
Member

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)

Copy link
ContributorAuthor

@siganushkasiganushkaOct 29, 2025
edited
Loading

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]

Copy link
Member

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.

Copy link
ContributorAuthor

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]

Copy link
Member

@nicolas-grekasnicolas-grekas left a 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.

@nicolas-grekas
Copy link
Member

Thank you@siganushka.

@nicolas-grekasnicolas-grekas merged commit99bbae9 intosymfony:7.4Oct 29, 2025
12 checks passed
@stof
Copy link
Member

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.

@nicolas-grekas
Copy link
Member

Caching the getter was implemented in 6.4 in#50974

IIUC, we only cached the type of getter, not the actual getter closure, so that's fine there (and in 7.3 also)

@stof
Copy link
Member

Indeed.#60114 changed the cached information in a broken way, introducing that bug.
However, the fix is incomplete. setters are also affected by the cache contamination.

And btw, the type declaration for properties storing the cache is wrong now as it has not been updated when changing the cached info.

loic425 reacted with thumbs up emoji

nicolas-grekas added a commit that referenced this pull requestOct 29, 2025
…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
This was referencedNov 2, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@stofstofstof left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@lyrixxlyrixxAwaiting requested review from lyrixxlyrixx is a code owner

Assignees

No one assigned

Projects

None yet

Milestone

7.4

Development

Successfully merging this pull request may close these issues.

4 participants

@siganushka@carsonbot@nicolas-grekas@stof

[8]ページ先頭

©2009-2025 Movatter.jp