Unfreeze event dict after check_event_allowed module callbacks by MatMaul · Pull Request #19547 · element-hq/synapse

MatMaul · 2026-03-11T12:48:52Z

Fix initially contributed in this PR: #18103

Fixes #18101

Pydantic validation was also failing if m.mentions is present (cf test).

Related bug: #18117

Discussed in #synapse-dev:matrix.org

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file.
Code style is correct (run the linters)

MatMaul · 2026-03-11T13:24:51Z

+        # Unfreeze the event dict to avoid potential issues with frozen dicts further
+        # down the code. Pydantic for example is not happy with frozen dicts.
+        # cf https://github.com/element-hq/synapse/issues/18117
+        event._dict = unfreeze(event._dict)


Initial implementation was adding an unfreeze method to EventBase. I changed that since we really don't want this to be called inside the modules.

MadLittleMods · 2026-03-17T20:15:14Z

@@ -288,6 +289,9 @@ async def check_event_allowed(
        # the hashes and signatures.
        event.freeze()


We should also update the freeze() docstring to callout the specific use cases that it's meant for and that you should remember to unfreeze before going back to Synapse code, etc

MadLittleMods · 2026-03-17T20:17:51Z

+        # Unfreeze the event dict to avoid potential issues with frozen dicts further
+        # down the code. Pydantic for example is not happy with frozen dicts.
+        # cf https://github.com/element-hq/synapse/issues/18117
+        event._dict = unfreeze(event._dict)


Perhaps we should create new freeze_event(...) and unfreeze_event(...) functions in this file that handles this logic. That way the pair can better be associated together as event._dict is a pretty arbitrary detail.

The location in this file vs being event.freeze() will also prevent people from accidentally mis-using it elsewhere (leaking into other parts of the code that it shouldn't).

Even one step further, we could use a context manager pattern for this with freeze_event(event) as frozen_event: ... so we never forget to unfreeze

MadLittleMods · 2026-03-17T20:19:08Z

@@ -0,0 +1 @@
+Unfreeze event dict after check_event_allowed module callbacks. Frozen dicts can cause issues with Pydantic validation among other things. Contributed by @c-cal.


Currently, the PR description just says "Related to #18117" but I think this fixes the issue. Is that the case?

Well it depends of how you look at it: we are fixing the problem for the module callback here, but we are not fixing the fact that a frozen event can be problematic in synapse code globally.

hmmm, I see. I was jumping the gun since this is the only place we call event.freeze().

But we still have the USE_FROZEN_DICTS stuff which appears to do the same thing. At-least in this case, it's a self-inflicted configuration option and happens all the time, not just when a Synapse module is configured and some check_event_allowed callbacks are configured.

It's ironic that this config options says that it "prevents bugs" but introduces a whole new category of potential bugs.

MadLittleMods · 2026-03-17T20:20:23Z

@@ -0,0 +1 @@
+Unfreeze event dict after check_event_allowed module callbacks. Frozen dicts can cause issues with Pydantic validation among other things. Contributed by @c-cal.


Feel free to add your own handle here as well

MadLittleMods · 2026-03-17T20:21:37Z

@@ -0,0 +1 @@
+Unfreeze event dict after check_event_allowed module callbacks. Frozen dicts can cause issues with Pydantic validation among other things. Contributed by @c-cal.


Perhaps better 🤷 :

Suggested change

Unfreeze event dict after check_event_allowed module callbacks. Frozen dicts can cause issues with Pydantic validation among other things. Contributed by @c-cal.

Unfreeze event dict after module callbacks have run. Frozen dicts can cause issues with Pydantic validation and other type comparisons. Contributed by @c-cal.

MadLittleMods · 2026-03-17T20:25:15Z

        event.freeze()

+        allow = True
+        event_dict = None


Suggested change

event_dict = None

replacement_event_dict = None

MadLittleMods · 2026-03-17T20:31:28Z

+
+        room_id = channel.json_body["room_id"]
+
+        event_id = self.create_and_send_event(


👍 Tested that this test fails when we comment out the unfreeze(...) fix

MadLittleMods · 2026-03-17T20:34:04Z

+        event._dict = unfreeze(event._dict)

-        return True, None
+        return allow, event_dict


As an aside, I wonder if we have to be careful about someone passing back a frozen replacement_event_dict 🤔

Not something we necessarily have to concern ourselves with this PR but may be another problem. Perhaps we just need to tighten up the docstring details on what we expect passed back.

And hopefully, this doesn't naturally happen from someone taking the event, making a copy, mutating it, and passing it back.

MadLittleMods · 2026-03-17T20:35:34Z

+            content={
+                "body": "test",
+                "msgtype": "m.text",
+                "m.mentions": {},


We should call out that this is the secret sauce that tests xxx and is important because xxx

MadLittleMods · 2026-03-17T20:35:52Z

        user: UserID,
        soft_failed: bool = False,
        prev_event_ids: list[str] | None = None,
+        content: dict[str, Any] | None = None,


From the new usage, it looks like this was done in order to be able to define m.mentions

c-cal and others added 4 commits March 11, 2026 12:47

unfreeze event after executing all check_event_allowed callbacks

08d1279

add a changelog entry

4804e04

Add test

6087a5a

changelog

73f0faf

MatMaul changed the title ~~Unfreeze event after module callbacks~~ Unfreeze event dict after check_event_allowed module callbacks Mar 11, 2026

MatMaul commented Mar 11, 2026

View reviewed changes

fix changelog

4c86b67

MatMaul force-pushed the unfreeze-after-module-callbacks branch from c9b87a9 to 4c86b67 Compare March 11, 2026 13:26

MatMaul mentioned this pull request Mar 11, 2026

fix: unfreeze notification_levels for PushRuleEvaluator #18103

Closed

3 tasks

MatMaul marked this pull request as ready for review March 11, 2026 13:37

MatMaul requested a review from a team as a code owner March 11, 2026 13:37

MadLittleMods added the A-Modules label Mar 17, 2026

MadLittleMods reviewed Mar 17, 2026

View reviewed changes

Use context manager

d8426ee

MadLittleMods mentioned this pull request Apr 10, 2026

Fix validation of frozen message event with mentions. #19634

Open

3 tasks

MadLittleMods mentioned this pull request Apr 20, 2026

event.freeze() causes subtle downstream bugs that makes type comparisons brittle #18117

Open

		@@ -288,6 +289,9 @@ async def check_event_allowed(
		# the hashes and signatures.
		event.freeze()

		@@ -0,0 +1 @@
		Unfreeze event dict after check_event_allowed module callbacks. Frozen dicts can cause issues with Pydantic validation among other things. Contributed by @c-cal.

	Unfreeze event dict after check_event_allowed module callbacks. Frozen dicts can cause issues with Pydantic validation among other things. Contributed by @c-cal.
	Unfreeze event dict after module callbacks have run. Frozen dicts can cause issues with Pydantic validation and other type comparisons. Contributed by @c-cal.


		room_id = channel.json_body["room_id"]

		event_id = self.create_and_send_event(

Conversation

MatMaul commented Mar 11, 2026 • edited by MadLittleMods Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MadLittleMods Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MatMaul commented Mar 11, 2026 •

edited by MadLittleMods

Loading

MadLittleMods Mar 17, 2026 •

edited

Loading