Update get_pdu to return the original, pristine EventBase

[MadLittleMods]

Update get_pdu to return the untouched, pristine EventBase as it was originally seen over federation (no metadata added). Previously, we returned the same event reference that we stored in the cache which downstream code modified in place and added metadata like setting it as an outlier and essentially poisoned our cache. Now we always return a copy of the event so the original can stay pristine in our cache and re-used for the next cache call.

Split out from #13205

As discussed at:

• Backfill remote event fetched by MSC3030 so we can paginate from it later #13205 (comment)
• Backfill remote event fetched by MSC3030 so we can paginate from it later #13205 (comment)

Related to #12584. This PR doesn't fix that issue because it hits get_event which exists from the local database before it tries to get_pdu.

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
  • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
• Pull request includes a sign off
• Code style is correct
  (run the linters)

[richvdh]

thanks for your continued diligence here!

[richvdh]

wait... does this loop always try all the destinations, even if the first one works? that would be a substantial bug, if it was ever called with more than one destination, which I don't think it is...

[MadLittleMods]

Seems like it is bugged. I can add a break or refactor to do the return inside the for-loop in a separate PR after this merges ⏩

[MadLittleMods]

I've added a FIXME comment in the code for now so we don't forget or at least will know to tackle it at some point. I plan to tackle in a fast-follow-up after this PR since it's a separate bug. Feel free to push to include it here.

[MadLittleMods]

Followed up in #13346

[richvdh]

if we're going to build a new EventBase on each call, why not just cache the raw json?

[MadLittleMods]

It would work. But we would have to convert the EventBase back to JSON from get_pdu_from_destination_raw since it needs one for _check_sigs_and_hash(room_version, pdu) anyway (whether that be in get_pdu_from_destination_raw or in get_pdu). I'm inclined to leave it as-is. Feel free to push again

[richvdh]

I have a suggestion for a somewhat different approach. It's arguably cleaner, but also probably a bunch more work; I'd be interested in your thoughts.

In my ideal world, _EventInternalMetadata.outlier should be a private, immutable property. Which is to say, you have to know whether you are dealing with an outlier at the time you construct an _EventInternalMetadata and hence when you call make_event_from_dict. We're some way off that goal in the codebase as a whole, but that doesn't stop us thinking about how to move in that direction for this part of the code.

It would mean passing an outlier flag into get_pdu telling it whether we're going to persist the result as an outlier or not. (That's fairly easy, because get_pdu is called in two places: FederationEventHandler.get_event, which only deals in outliers, and FederationClient._check_sigs_and_hash_and_fetch_one, where we can infer the outlieryness from the input pdu).

That then raises a few possibilities: we could continue to construct a new EventBase on each call to get_pdu as proposed here. Or we could use a different cache key depending on the outlier flag and have (up to) two cache entries for each event.

[MadLittleMods]

I have a suggestion for a somewhat different approach. It's arguably cleaner, but also probably a bunch more work; I'd be interested in your thoughts.

In my ideal world, _EventInternalMetadata.outlier should be a private, immutable property. Which is to say, you have to know whether you are dealing with an outlier at the time you construct an _EventInternalMetadata and hence when you call make_event_from_dict. We're some way off that goal in the codebase as a whole, but that doesn't stop us thinking about how to move in that direction for this part of the code.

-- #13320 (comment)

I like the idea of having it be immutable 👍

It would mean passing an outlier flag into get_pdu telling it whether we're going to persist the result as an outlier or not. (That's fairly easy, because get_pdu is called in two places: FederationEventHandler.get_event, which only deals in outliers, and FederationClient._check_sigs_and_hash_and_fetch_one, where we can infer the outlieryness from the input pdu).

That then raises a few possibilities: we could continue to construct a new EventBase on each call to get_pdu as proposed here. Or we could use a different cache key depending on the outlier flag and have (up to) two cache entries for each event.

But needing to pass in outlier or the metadata (when we need to add more than just outlier in the future) makes the function signature surface a bit icky. I'd rather just force downstream consumers to create a copy with the metadata they need/expect, or want to add.

We can tackle this in another potential PR though.

[richvdh]

yes, there's definitely a balance of two evils here. I'm happy to punt this for now at least.

[richvdh]

lgtm otherwise!

[MadLittleMods]

Thanks for the review @richvdh 🦧