element-hq / element-hq/synapse

Fix unique constraint exceptions on remote media redownload

Open
#12,661 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
4.6k
Forks
600
Avg merge
5d 22h
Merged PRs (30d)
51

Description

This issue has been migrated from [#12661](https://github.com/matrix-org/synapse/pull/12661).

---

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

This fixes a very specific problem, with the following conditions;
- The custom storage provider does not `store_remote`
- The storage provider has not stored the media
- The media was previously downloaded (and the file info registered in the database)
- The local `remote_content` folder was wiped

Synapse then tries to redownload the file, generating a new `file_info`, but the previous file info was already stored in the database under a unique key constraint, creating the following error;

Error

```
2022-05-06 20:10:53,419 - synapse.rest.media.v1.media_repository - 361 - WARNING - GET-1070 - Ignoring _download_remote_file exception
Traceback (most recent call last):
File "/usr/local/lib/python3.9/site-packages/synapse/rest/media/v1/media_repository.py", line 348, in _get_remote_media_impl
media_info = await self._download_remote_file(
File "/usr/local/lib/python3.9/site-packages/synapse/rest/media/v1/media_repository.py", line 474, in _download_remote_file
await self.store.store_cached_remote_media(
File "/usr/local/lib/python3.9/site-packages/synapse/storage/databases/main/media_repository.py", line 471, in store_cached_remote_media
await self.db_pool.simple_insert(
File "/usr/local/lib/python3.9/site-packages/synapse/storage/database.py", line 966, in simple_insert
await self.runInteraction(desc, self.simple_insert_txn, table, values)
File "/usr/local/lib/python3.9/site-packages/synapse/storage/database.py", line 797, in runInteraction
return await delay_cancellation(defer.ensureDeferred(_runInteraction()))
File "/usr/local/lib/python3.9/site-packages/twisted/internet/defer.py", line 1656, in _inlineCallbacks
result = current_context.run(
File "/usr/local/lib/python3.9/site-packages/twisted/python/failure.py", line 489, in throwExceptionIntoGenerator
return g.throw(self.type, self.value, self.tb)
File "/usr/local/lib/python3.9/site-packages/synapse/storage/database.py", line 769, in _runInteraction
result = await self.runWithConnection(
File "/usr/local/lib/python3.9/site-packages/synapse/storage/database.py", line 892, in runWithConnection
return await make_deferred_yieldable(
File "/usr/local/lib/python3.9/site-packages/twisted/python/threadpool.py", line 244, in inContext
result = inContext.theWork() # type: ignore[attr-defined]
File "/usr/local/lib/python3.9/site-packages/twisted/python/threadpool.py", line 260, in
inContext.theWork = lambda: context.call( # type: ignore[attr-defined]
File "/usr/local/lib/python3.9/site-packages/twisted/python/context.py", line 117, in callWithContext
return self.currentContext().callWithContext(ctx, func, *args, **kw)
File "/usr/local/lib/python3.9/site-packages/twisted/python/context.py", line 82, in callWithContext
return func(*args, **kw)
File "/usr/local/lib/python3.9/site-packages/twisted/enterprise/adbapi.py", line 282, in _runWithConnection
result = func(conn, *args, **kw)
File "/usr/local/lib/python3.9/site-packages/synapse/storage/database.py", line 885, in inner_func
return func(db_conn, *args, **kwargs)
File "/usr/local/lib/python3.9/site-packages/synapse/storage/database.py", line 633, in new_transaction
r = func(cursor, *args, **kwargs)
File "/usr/local/lib/python3.9/site-packages/synapse/storage/database.py", line 980, in simple_insert_txn
txn.execute(sql, vals)
File "/usr/local/lib/python3.9/site-packages/synapse/storage/database.py", line 334, in execute
self._do_execute(self.txn.execute, sql, *args)
File "/usr/local/lib/python3.9/site-packages/synapse/storage/database.py", line 367, in _do_execute
return func(sql, *args)
psycopg2.errors.UniqueViolation: duplicate key value violates unique constraint "remote_media_cache_media_origin_media_id_key"
DETAIL: Key (media_origin, media_id)=(neko.dev, wLFwLqbnyvrstuomVXdKMqyJ) already exists.
```

This error gets surpressed due to a logic error described in #12660, where synapse only tests if file_info is available before moving on and ignoring the error, before running into a `fetch_media` call later on, which returns no responder, which then results in a 404.

This PR attempts to fix that issue, and also add a warning log whenever such an error is ignored. (If I recall correctly, this means it'll also be captured by sentry)

### Pull Request Checklist

* [x] Pull request is based on the develop branch
* [x] Pull request includes a [changelog file](https://matrix-org.github.io/synapse/latest/development/contributing_guide.html#changelog). 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.
* [x] Pull request includes a [sign off](https://matrix-org.github.io/synapse/latest/development/contributing_guide.html#sign-off)
* [x] [Code style](https://matrix-org.github.io/synapse/latest/code_style.html) is correct
(run the [linters](https://matrix-org.github.io/synapse/latest/development/contributing_guide.html#run-the-linters))

`Signed-off-by: Jonathan de Jong `

Contributor guide

Open the contributing guide

Research direction

Start by reading synapse/rest/media/v1/media_repository.py around _get_remote_media_impl and _download_remote_file, then synapse/storage/databases/main/media_repository.py around store_cached_remote_media. Reproduce the listed custom-provider and wiped-folder conditions, and verify that a remote redownload no longer fails with a unique-constraint error or ends in a 404.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
backend, databases
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.