synapse icon indicating copy to clipboard operation
synapse copied to clipboard

Silently drop PDUs we fail to parse from federation, instead of failing entire transaction

Open morguldir opened this issue 1 year ago • 3 comments

Related https://github.com/matrix-org/synapse/issues/7543 / #7543

Complement test: https://github.com/matrix-org/complement/pull/743

Sytest changes: https://github.com/matrix-org/sytest/pull/1391

Pull Request Checklist

morguldir avatar Oct 31 '24 18:10 morguldir

My reasoning for the error was that synapse would yell at you loudly before with a 500 for the whole transaction before, but there's not really anything another server can do with that information (a loud error)

Returning an error for that specific pdu at least lets the sending server output a warning or similar

From the discussion in #7543, i think the main problem is that most of the time it's impossible to calculate the event id, but here it was already provided. I'm not sure if there's a drawback to informing about the event not being processed

In the end i'm mostly worried about returning a 200 response every time though, regardless if it includes the error or not

morguldir avatar Dec 17 '24 18:12 morguldir

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Mar 23 '25 05:03 CLAassistant

~Err sorry, I obviously need more coffee. The exact opposite is true of what I said in #17893 (comment)~

Actually, it's partially true. This PR does two things, the latter of which the title fails to mention:

1. Silently drop PDUs we fail to parse from federation

2. Report an error back to the homeserver if an `event_id` was provided in an incoming v3+ room event.

Discussion in #17893 (comment) is advocating to also silently drop events which contain an event_id field. So dropping these lines and removing the second change from this PR.

@morguldir curious for your thoughts on that change?

I agree with @anoadragon453's assessment and think that we should merge this PR with these lines removed. This also means the test will need to be updated not to check for the error in the response.

Dropping the bad events is an important change that makes Synapse better, without being controversial. Keeping the potential per-event error responses, for a very specific case of event failures, can be decided on at a later point once things such as https://github.com/element-hq/synapse/issues/7543 & https://github.com/matrix-org/matrix-spec/issues/2027 get sorted out.

devonh avatar Nov 13 '25 23:11 devonh