Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

KeyError: 'approved' when register_user in replication module is called #14131

Closed
TheTimeWalker opened this issue Oct 11, 2022 · 7 comments · Fixed by #14135
Closed

KeyError: 'approved' when register_user in replication module is called #14131

TheTimeWalker opened this issue Oct 11, 2022 · 7 comments · Fixed by #14135
Assignees
Labels
A-Registration Creating an account T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. X-Regression Something broke which worked on a previous release X-Release-Blocker Must be resolved before making a release

Comments

@TheTimeWalker
Copy link

Description

Currently with the latest RC for 1.69.0, an error appears on the replication module when an app service wants to register an user with register_user. The server returns a 500 Internal Server Error because it fails with a KeyError for approved.

The stacktrace points to approved=content["approved"] which was recently added in preparation for MSC3866. However, the experimental feature has not been enabled.

Steps to reproduce

  • Enable application service replication with a worker
  • Let an app service (e.g. Mautrix Telegram Bridge) send a register_user command
    • In the bridge example, the admin command clean-rooms

Expectation: POST request returns with 200
Reality: POST request returns with 500 Internal Server Error

Homeserver

Own homeserver

Synapse Version

1.69.0rc2

Installation Method

Docker (matrixdotorg/synapse)

Platform

OS: Debian 11
Docker version 20.10.10

Relevant log output

2022-10-11 00:23:33,870 - synapse.http.server - 123 - ERROR - POST-43 - Failed handle request via 'ReplicationRegisterServlet': <SynapseRequest at 0x7f007f16fb80 method='POST' uri='/_synapse/replication/register_user/%40telegrambot%3Afoxden.party/ODHvDpnGFc' clientproto='HTTP/1.1' site='9093'>
Traceback (most recent call last):
  File "/usr/local/lib/python3.9/site-packages/synapse/http/server.py", line 306, in _async_render_wrapper
    callback_return = await self._async_render(request)
  File "/usr/local/lib/python3.9/site-packages/synapse/http/server.py", line 512, in _async_render
    callback_return = await raw_callback_return
  File "/usr/local/lib/python3.9/site-packages/synapse/replication/http/_base.py", line 356, in _check_auth_and_handle
    return await self.response_cache.wrap(
  File "/usr/local/lib/python3.9/site-packages/synapse/util/caches/response_cache.py", line 250, in wrap
    return await make_deferred_yieldable(entry.result.observe())
  File "/usr/local/lib/python3.9/site-packages/twisted/internet/defer.py", line 1660, in _inlineCallbacks
    result = current_context.run(gen.send, result)
  File "/usr/local/lib/python3.9/site-packages/synapse/util/caches/response_cache.py", line 246, in cb
    return await callback(*args, **kwargs)
  File "/usr/local/lib/python3.9/site-packages/synapse/replication/http/register.py", line 106, in _handle_request
    approved=content["approved"],
KeyError: 'approved'


### Anything else that would be useful to know?

_No response_
@DMRobertson
Copy link
Contributor

Presumably due to #13556

@DMRobertson DMRobertson added T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. X-Regression Something broke which worked on a previous release A-Registration Creating an account labels Oct 11, 2022
@DMRobertson
Copy link
Contributor

I would guess the fix is to change

https:/matrix-org/synapse/pull/13556/files#diff-1a94c74b88ea0968731da4af72b1180b9ec0faf0f22c6961cc80a4cfb7af4fa0R106

to content.get("approved", ...) with an appropriate fallback? But @babolivier has all the context here.

@DMRobertson DMRobertson added the X-Release-Blocker Must be resolved before making a release label Oct 11, 2022
@babolivier
Copy link
Contributor

babolivier commented Oct 11, 2022

It looks to me like approved should always be in the replication content regardless of whether the experimental feature is enabled or not. @TheTimeWalker is the worker handling the registration of the app service user also running 1.69.0rc2 (or even rc1)?

@babolivier
Copy link
Contributor

to content.get("approved", ...) with an appropriate fallback?

That would fix the symptom of approved not being in the payload, but not the actual issue of it missing. Workers registering a user are, as far as I can tell, all supposed to go through this block which systematically sets the approved property in the payload.

@babolivier
Copy link
Contributor

Right, after some internal discussion it looks like my understanding that workers shouldn't be running a version older than the main process was wrong. I've opened #14135 which should ensure things go smoothly even in this case.

@TheTimeWalker
Copy link
Author

Thanks for the tip! I have noticed that I somehow didn't bring up the worker to the latest version.
After updating the worker to 1.69.0rc2 I'm not getting this error anymore. I can confirm that this issue only happens when the worker is on an older version than the main instance

@babolivier
Copy link
Contributor

The PR has merged - closing the issue manually so it doesn't count towards release blockers.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Registration Creating an account T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. X-Regression Something broke which worked on a previous release X-Release-Blocker Must be resolved before making a release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants