Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle duplicate OTK uploads racing #17241

Merged
merged 2 commits into from
May 29, 2024
Merged

Conversation

erikjohnston
Copy link
Member

Currently this causes one of then to 500.

Stack trace
UniqueViolation: duplicate key value violates unique constraint "e2e_one_time_keys_json_uniqueness"
DETAIL:  Key (user_id, device_id, algorithm, key_id)=(xxx, xxx, xxx, xxx) already exists.

  File "synapse/http/server.py", line 332, in _async_render_wrapper
    callback_return = await self._async_render(request)
  File "synapse/http/server.py", line 544, in _async_render
    callback_return = await raw_callback_return
  File "synapse/replication/http/_base.py", line 433, in _check_auth_and_handle
    code, response = await self._handle_request(request, content, **kwargs)
  File "synapse/replication/http/devices.py", line 161, in _handle_request
    results = await self.e2e_keys_handler.upload_keys_for_user(
  File "synapse/logging/opentracing.py", line 921, in _wrapper
    return await func(*args, **kwargs)
  File "synapse/handlers/e2e_keys.py", line 817, in upload_keys_for_user
    await self._upload_one_time_keys_for_user(
  File "synapse/handlers/e2e_keys.py", line 896, in _upload_one_time_keys_for_user
    await self.store.add_e2e_one_time_keys(user_id, device_id, time_now, new_keys)
  File "synapse/storage/databases/main/end_to_end_keys.py", line 538, in add_e2e_one_time_keys
    await self.db_pool.runInteraction(
  File "synapse/storage/database.py", line 950, in runInteraction
    return await delay_cancellation(_runInteraction())
  File "twisted/internet/defer.py", line 1999, in _inlineCallbacks
    result = context.run(
  File "twisted/python/failure.py", line 519, in throwExceptionIntoGenerator
    return g.throw(self.value.with_traceback(self.tb))
  File "synapse/storage/database.py", line 916, in _runInteraction
    result: R = await self.runWithConnection(
  File "synapse/storage/database.py", line 1045, in runWithConnection
    return await make_deferred_yieldable(
  File "twisted/python/threadpool.py", line 269, in inContext
    result = inContext.theWork()  # type: ignore[attr-defined]
  File "twisted/python/threadpool.py", line 285, in <lambda>
    inContext.theWork = lambda: context.call(  # type: ignore[attr-defined]
  File "twisted/python/context.py", line 117, in callWithContext
    return self.currentContext().callWithContext(ctx, func, *args, **kw)
  File "twisted/python/context.py", line 82, in callWithContext
    return func(*args, **kw)
  File "twisted/enterprise/adbapi.py", line 282, in _runWithConnection
    result = func(conn, *args, **kw)
  File "synapse/storage/database.py", line 1038, in inner_func
    return func(db_conn, *args, **kwargs)
  File "synapse/storage/database.py", line 778, in new_transaction
    r = func(cursor, *args, **kwargs)
  File "synapse/storage/databases/main/end_to_end_keys.py", line 571, in _add_e2e_one_time_keys_txn
    self.db_pool.simple_insert_many_txn(
  File "synapse/storage/database.py", line 1150, in simple_insert_many_txn
    txn.execute_values(sql, values, fetch=False)
  File "synapse/storage/database.py", line 413, in execute_values
    return self._do_execute(
  File "synapse/storage/database.py", line 486, in _do_execute
    return func(sql, *args, **kwargs)
  File "synapse/storage/database.py", line 416, in <lambda>
    lambda the_sql, the_values: execute_values(
  File "psycopg2/extras.py", line 1299, in execute_values
    cur.execute(b''.join(parts))

Currently this casues one of then to 500.
@erikjohnston erikjohnston marked this pull request as ready for review May 28, 2024 16:23
@erikjohnston erikjohnston requested a review from a team as a code owner May 28, 2024 16:23
Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of me wonders if we'd rather INSERT OR IGNORE instead, but maybe this is just a plain saner approach and I don't think we should be afraid of linearising these requests for a single device, so fine.

@erikjohnston
Copy link
Member Author

Part of me wonders if we'd rather INSERT OR IGNORE instead, but maybe this is just a plain saner approach and I don't think we should be afraid of linearising these requests for a single device, so fine.

Yeah, though we'd then need to be careful that we are only ignoring where the keys are actually the same, which is a bit harder for JSON

@erikjohnston erikjohnston merged commit 94ef2f4 into develop May 29, 2024
38 checks passed
@erikjohnston erikjohnston deleted the erikj/one_time_key_errors branch May 29, 2024 10:16
H-Shay pushed a commit to H-Shay/hq_synapse that referenced this pull request May 31, 2024
Currently this causes one of then to 500.
Mic92 pushed a commit to Mic92/synapse that referenced this pull request Jun 14, 2024
Currently this causes one of then to 500.
yingziwu added a commit to yingziwu/synapse that referenced this pull request Jun 26, 2024
- Fix the building of binary wheels for macOS by switching to macOS 12 CI runners. ([\#17319](element-hq/synapse#17319))

- When rolling back to a previous Synapse version and then forwards again to this release, don't require server operators to manually run SQL. ([\#17305](element-hq/synapse#17305), [\#17309](element-hq/synapse#17309))

- Use the release branch for sytest in release-branch PRs. ([\#17306](element-hq/synapse#17306))

- Fix bug where one-time-keys were not always included in `/sync` response when using workers. Introduced in v1.109.0rc1. ([\#17275](element-hq/synapse#17275))
- Fix bug where `/sync` could get stuck due to edge case in device lists handling. Introduced in v1.109.0rc1. ([\#17292](element-hq/synapse#17292))

- Add the ability to auto-accept invites on the behalf of users. See the [`auto_accept_invites`](https://element-hq.github.io/synapse/latest/usage/configuration/config_documentation.html#auto-accept-invites) config option for details. ([\#17147](element-hq/synapse#17147))
- Add experimental [MSC3575](matrix-org/matrix-spec-proposals#3575) Sliding Sync `/sync/e2ee` endpoint for to-device messages and device encryption info. ([\#17167](element-hq/synapse#17167))
- Support [MSC3916](matrix-org/matrix-spec-proposals#3916) by adding unstable media endpoints to `/_matrix/client`. ([\#17213](element-hq/synapse#17213))
- Add logging to tasks managed by the task scheduler, showing CPU and database usage. ([\#17219](element-hq/synapse#17219))

- Fix deduplicating of membership events to not create unused state groups. ([\#17164](element-hq/synapse#17164))
- Fix bug where duplicate events could be sent down sync when using workers that are overloaded. ([\#17215](element-hq/synapse#17215))
- Ignore attempts to send to-device messages to bad users, to avoid log spam when we try to connect to the bad server. ([\#17240](element-hq/synapse#17240))
- Fix handling of duplicate concurrent uploading of device one-time-keys. ([\#17241](element-hq/synapse#17241))
- Fix reporting of default tags to Sentry, such as worker name. Broke in v1.108.0. ([\#17251](element-hq/synapse#17251))
- Fix bug where typing updates would not be sent when using workers after a restart. ([\#17252](element-hq/synapse#17252))

- Update the LemonLDAP documentation to say that claims should be explicitly included in the returned `id_token`, as Synapse won't request them. ([\#17204](element-hq/synapse#17204))

- Improve DB usage when fetching related events. ([\#17083](element-hq/synapse#17083))
- Log exceptions when failing to auto-join new user according to the `auto_join_rooms` option. ([\#17176](element-hq/synapse#17176))
- Reduce work of calculating outbound device lists updates. ([\#17211](element-hq/synapse#17211))
- Improve performance of calculating device lists changes in `/sync`. ([\#17216](element-hq/synapse#17216))
- Move towards using `MultiWriterIdGenerator` everywhere. ([\#17226](element-hq/synapse#17226))
- Replaces all usages of `StreamIdGenerator` with `MultiWriterIdGenerator`. ([\#17229](element-hq/synapse#17229))
- Change the `allow_unsafe_locale` config option to also apply when setting up new databases. ([\#17238](element-hq/synapse#17238))
- Fix errors in logs about closing incorrect logging contexts when media gets rejected by a module. ([\#17239](element-hq/synapse#17239), [\#17246](element-hq/synapse#17246))
- Clean out invalid destinations from `device_federation_outbox` table. ([\#17242](element-hq/synapse#17242))
- Stop logging errors when receiving invalid User IDs in key querys requests. ([\#17250](element-hq/synapse#17250))

* Bump anyhow from 1.0.83 to 1.0.86. ([\#17220](element-hq/synapse#17220))
* Bump bcrypt from 4.1.2 to 4.1.3. ([\#17224](element-hq/synapse#17224))
* Bump lxml from 5.2.1 to 5.2.2. ([\#17261](element-hq/synapse#17261))
* Bump mypy-zope from 1.0.3 to 1.0.4. ([\#17262](element-hq/synapse#17262))
* Bump phonenumbers from 8.13.35 to 8.13.37. ([\#17235](element-hq/synapse#17235))
* Bump prometheus-client from 0.19.0 to 0.20.0. ([\#17233](element-hq/synapse#17233))
* Bump pyasn1 from 0.5.1 to 0.6.0. ([\#17223](element-hq/synapse#17223))
* Bump pyicu from 2.13 to 2.13.1. ([\#17236](element-hq/synapse#17236))
* Bump pyopenssl from 24.0.0 to 24.1.0. ([\#17234](element-hq/synapse#17234))
* Bump serde from 1.0.201 to 1.0.202. ([\#17221](element-hq/synapse#17221))
* Bump serde from 1.0.202 to 1.0.203. ([\#17232](element-hq/synapse#17232))
* Bump twine from 5.0.0 to 5.1.0. ([\#17225](element-hq/synapse#17225))
* Bump types-psycopg2 from 2.9.21.20240311 to 2.9.21.20240417. ([\#17222](element-hq/synapse#17222))
* Bump types-pyopenssl from 24.0.0.20240311 to 24.1.0.20240425. ([\#17260](element-hq/synapse#17260))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants