Jason/update psycopg3#10
Merged
Merged
Conversation
This comes from https://github.com/erikjohnston/rust-signed-json/blob/main/src/json.rs. We need to be able to serialise canonical JSON in Rust to be able to calculate event IDs once we port the event class to Rust. We could instead make the above a properly published crate, but feels easier to pull it into Synapse utils.
…" (element-hq#19742) The original commit should only have changed the lockfile. This reverts commit bdb1cf7 (from element-hq#19703). --------- Co-authored-by: Olivier 'reivilibre <oliverw@matrix.org>
…le. (element-hq#19743) See: - element-hq#19742 - element-hq#19686 (etc) Documentation https://docs.github.com/en/code-security/reference/supply-chain-security/dependabot-options-reference#versioning-strategy-- We were considering `lockfile-only` but it sounds like `increase-if-necessary` would increase the upper bound for us, if we had one. Let's try it. --------- Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org>
Exposes `tombstoned` and `replacement_room` in room details on admin API endpoint `GET /_synapse/admin/v1/rooms/<room_id>`. Resolves element-hq#18347
…lement-hq#19736) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
The juicy details and explanation are in the diff itself. Split out from element-hq#18873 in order to fix paginating from [MSC3871](matrix-org/matrix-spec-proposals#3871) gap tokens actually backfilling history. To be clear, this is a good change to make outside of the [MSC3871](matrix-org/matrix-spec-proposals#3871) use case. For example (as the new Complement test shows), fixes a problem where if you try to paginate `/messages` from tokens returned by `/context`, we could fail to backfill anything new and hide away history. Also fixes matrix-org/complement#853
Fixes the symptoms of element-hq#19315 / element-hq#19588 but not the underlying reason causing the number to grow so large in the first place. ``` ValueError: Exceeds the limit (4300 digits) for integer string conversion; use sys.set_int_max_str_digits() to increase the limit ``` Copied from the original pull request on [Famedly's Synapse repo](famedly/synapse#221) (with some edits): Basing the time interval around a 5 seconds leaves a big window of waiting especially as this window is doubled each retry, when another worker could be making progress but can not. Right now, the retry interval in seconds looks like `[0.2, 5, 10, 20, 40, 80, 160, 320, (continues to double)]` after which logging should start about excessive times and (relatively quickly) end up with an extremely large retry interval with an unrealistic expectation past the heat death of the universe. 1 year in seconds = 31,536,000. With this change, retry intervals in seconds should look more like: ``` [ 0.2, 0.4, 0.8, 1.6, 3.2, 6.4, 12.8, 25.6, 51.2, 60, < never goes higher than this ] ``` Logging about excessive wait times will start at 10 minutes. <details> <summary>Previous breakdown when we were using 15 minutes</summary> ``` [ 0.2, 0.4, 0.8, 1.6, 3.2, 6.4, 12.8, 25.6, 51.2, 102.4, # 1.7 minutes 204.8, # 3.41 minutes 409.6, # 6.83 minutes 819.2, # 13.65 minutes < logging about excessive times will start here, 13th iteration 900, # 15 minutes < never goes higher than this ] ``` </details> Further suggested work in this area could be to define the cap, the retry interval starting point and the multiplier depending on how frequently this lock should be checked. See data below for reasons why. Increasing the jitter range may also be a good idea --------- Co-authored-by: Eric Eastwood <madlittlemods@gmail.com>
This is another stepping stone in porting the event class fully to Rust. The new `Signatures` class is relatively simple, as we actually don't interact with it that much in the code. It does *not* implement `Mapping` or `MutableMapping` as that takes quite a lot of effort that we don't need, even though it would be more ergonomic.
Similar to element-hq#19706, let's port the `unsigned` field into a Rust class. This does change things a bit in that we now define exactly what unsigned fields that are allowed to be added to an event, and what actually gets persisted. This should be a noop though, as we carefully filter out what unsigned fields we allow in from federation, for example As a side effect of this cleanup, I think this fixes handling `unsigned.age` on events received over federation.
Fixes the symptoms of element-hq#19315 / element-hq#19588 but not the underlying reason causing the number to grow so large in the first place. ``` ValueError: Exceeds the limit (4300 digits) for integer string conversion; use sys.set_int_max_str_digits() to increase the limit ``` Copied from the original pull request on [Famedly's Synapse repo](famedly/synapse#221) (with some edits): Basing the time interval around a 5 seconds leaves a big window of waiting especially as this window is doubled each retry, when another worker could be making progress but can not. Right now, the retry interval in seconds looks like `[0.2, 5, 10, 20, 40, 80, 160, 320, (continues to double)]` after which logging should start about excessive times and (relatively quickly) end up with an extremely large retry interval with an unrealistic expectation past the heat death of the universe. 1 year in seconds = 31,536,000. With this change, retry intervals in seconds should look more like: ``` [ 0.2, 0.4, 0.8, 1.6, 3.2, 6.4, 12.8, 25.6, 51.2, 60, < never goes higher than this ] ``` Logging about excessive wait times will start at 10 minutes. <details> <summary>Previous breakdown when we were using 15 minutes</summary> ``` [ 0.2, 0.4, 0.8, 1.6, 3.2, 6.4, 12.8, 25.6, 51.2, 102.4, # 1.7 minutes 204.8, # 3.41 minutes 409.6, # 6.83 minutes 819.2, # 13.65 minutes < logging about excessive times will start here, 13th iteration 900, # 15 minutes < never goes higher than this ] ``` </details> Further suggested work in this area could be to define the cap, the retry interval starting point and the multiplier depending on how frequently this lock should be checked. See data below for reasons why. Increasing the jitter range may also be a good idea --------- Co-authored-by: Eric Eastwood <madlittlemods@gmail.com> (cherry picked from commit 3f58bc5)
…MENTSEC-2025-1636) Fixes: GHSA-6qf2-7x63-mm6v Reviewed-on: element-hq/synapse-private#117
Better to retry more quickly than have workers wait around. 5 seconds is still a reasonable gap in time to not overwhelm anything. This matters most in cross-worker scenarios. When locks are on the same worker, when the lock holder releases, we signal to other locks (with the same name/key) that they should try reacquiring the lock immediately. But locks on other workers only re-check based on their retry `_timeout_interval`. Updating to 5 seconds to match the previous intentions based on the [flawed code](https://github.com/element-hq/synapse/blob/6100f6e4f7fb0c72f1ae2802683ebc811c0e3a77/synapse/handlers/worker_lock.py#L278). We can assume they were trying to have 5 seconds as the max value to retry. Spawning from element-hq#19394 (comment)
…19756) So people have to specify which time unit they want to use. Spawning from element-hq#19394 (comment)
…son.rs`) (element-hq#19763) (introduced in element-hq#19739) Seems like some automatic change from `poetry run ./scripts-dev/lint.sh`
Based on element-hq#19708. This is on the path to porting the entire event class to Rust, as `event.content` will then return the new Rust class `JsonObject`. This PR adds a pure Rust `JsonObject` class that is a `Mapping` representing a json-style object. It uses `serde_json::Value` as its in-memory representation and `pythonize` for conversion when a field is looked up on the object. I'm not thrilled with the name, but couldn't think of a better one. This also adds `JsonObject` handling to the JSON serialisation functions we use, as well as to the `freeze(..)` function. Reviewable commit-by-commit.
…_INTERVAL` (element-hq#19772) There is no behavioral change, only a change to the tests. See element-hq#19772 (comment) for an explanation of why the tests needed changing (and diff comments). Follow-up to element-hq#19394. The test discussion originally happened in element-hq#19394 (comment) This is spawning from thinking about the problem again.
Handle arbitrary sized integers in `unsigned` (and other Rust objects that use `serde_json::Value`)
…neously used by psycopg
…=1 for a potential performance micro-enhancement
Follow on from element-hq#19701. Unfortunately serde has a bug when using `#[serde(flatten)]` with `arbitrary-precision` feature when handling integers that fit in a i128 when doing `serde_json::from_value`. See serde-rs/serde#2230. The `depythonize` hits the same issue. To fix this we make it so we only parse events from strings and not values.
Follow on from element-hq#19701 This (more or less) matches what we had before. Otherwise we just get a default `<builtins.Event at 0x...>` --------- Co-authored-by: Eric Eastwood <erice@element.io>
…nt accidents with database engines
…all` (element-hq#19818) Knowledge from @reivilibre in [`#element-backend-internal:matrix.org`](https://matrix.to/#/!SGNQGPGUwtcPBUotTL:matrix.org/$2WUfCdA02wZw-6-jhw3QbLQ44BmKJrqLuZ6wjz2r7hk?via=jki.re&via=element.io&via=matrix.org) on 2025-12-15. Spawning from me making Rust changes but nothing useful was printed until I added `-v`, ```shell $ poetry install --extras all Installing dependencies from lock file Package operations: 0 installs, 1 update, 0 removals - Updating pyjwt (2.11.0 -> 2.12.0) Installing the current project: matrix-synapse (1.154.0rc1) Failed to install /home/eric/Documents/github/element/synapse ``` I also see `poetry run maturin develop` suggested but I'd prefer not to need to install `maturin` as yet another system tool to manage myself.
…ent-hq#19741) Fix element-hq#2860 Also cleans up comments around plans to define `ALLOWED_SCHEMES` as we can rely on Bleach's `ALLOWED_PROTOCOLS` defaults (`http`, `https` and `mailto`).
Followup to element-hq#19801: I only meant for this logging to happen on the instance that is doing the persisting.
…SM-only versions, is included in our support policy. (element-hq#19823) The reason for querying this support was wanting support for SQLite's JSON operators, which are currently not present in the SQLite version found in Ubuntu's oldest supported LTS. The JSON operators were used in some of the sticky events work (related: element-hq#19452). Our ruling was that we should support Ubuntu oldest LTS equally to Debian oldstable, so support the oldest of the two versions from those. That makes some kind of sense as it would be difficult to do otherwise without dropping support for that version of Ubuntu altogether, given if we kept publishing packages intended for use with Postgres, there's a risk that an innocent sysadmin would update their SQLite deployment without realising that it is no longer supported. This was [discussed months ago (private)](https://docs.google.com/document/d/12RZKPk3a4__JUSH9wYHODo9rRyKzsHg6BSCAcmqmbOU/edit?tab=t.0#bookmark=id.fcdvoc88dy5s) and at [private](https://docs.google.com/document/d/12RZKPk3a4__JUSH9wYHODo9rRyKzsHg6BSCAcmqmbOU/edit?tab=t.0#bookmark=id.u48ivjge4qpt). --------- Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org>
… config has changed (element-hq#19792) Reintroduces element-hq#19714, after being reverted in element-hq#19784. Fix element-hq#18844 Fix element-hq#19783 Fix element-hq#18880 This PR also adds a fix so that we don't always return immediately when using the e2ee extension. --------- Co-authored-by: Benjamin Bouvier <benjamin@bouvier.cc> Co-authored-by: Eric Eastwood <madlittlemods@gmail.com> Co-authored-by: Eric Eastwood <erice@element.io>
This is in prep for converting the event serialization to Rust. This is a fairly mechanical port, except that we store the appservice ID rather than the appservice object. This avoids us having to store a `Py<..>` (or port the appservice object over).
…orkers when MSC4452: Preview URL capabilities API is enabled. (element-hq#19839) Fixes: element-hq#19825 Introduced in: element-hq#19715 Always populate `url_preview_enabled` so `/capabilities` can expose it Needed so this line can be happy: https://github.com/element-hq/synapse/blob/106ed3623d434891fe1ac50aacc851e9804404fe/synapse/rest/client/capabilities.py#L82 --------- Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org>
…-hq#19832) This was all done through claude. ## Fix flaky `test_edu_large_messages_not_splitting_one_user` (`TooLong` under `trial -jN`) ### Problem The "Build .deb packages" CI step intermittently failed with: ``` twisted.protocols.amp.TooLong ...in tests.rest.client.test_sendtodevice.SendToDeviceTestCase.test_edu_large_messages_not_splitting_one_user ``` The deb build runs the suite with `twisted.trial -j2`. In that mode, worker log events are shipped to the manager process over Twisted's AMP protocol, which encodes each value with a 2-byte length prefix — so any single log line of **64 KiB or more** raises `TooLong`. ### Root cause Not a bug in the to-device/EDU logic, and **not** debug logging enabled by the build (it runs at the default `ERROR` level). It's a **log-level leak between tests sharing a `-j2` worker**: - `tests/logging/test_loggers.py::ExplicitlyConfiguredLoggerTestCase` calls `root_logger.setLevel(logging.DEBUG)` directly and never restores it (no `setUp`/`tearDown`/`addCleanup`). - When that test runs before `test_edu_large_messages_not_splitting_one_user` **in the same worker process**, the root logger is left at `DEBUG`. - That test deliberately builds an EDU of exactly `SOFT_MAX_EDU_SIZE - 1` (65 535) bytes. Storing it triggers `synapse/storage/database.py`'s `[SQL values]` DEBUG log, which dumps the full query params — producing a **65 708-byte** line that overflows AMP's cap. It looks "flaky" purely because of `-j2` scheduling: whether the two tests land on the same worker, and in what order. ### Fix Three commits: 1. **Restore the root logger level in `ExplicitlyConfiguredLoggerTestCase`** — `addCleanup` to put the level back. Fixes the root cause. 2. **Truncate oversized log lines in the test log handler** (`ToTwistedHandler.emit`) — caps lines at 1000 chars so no debug line can break `trial -jN`, regardless of which query is logged (defense in depth). 3. **Truncate values in `[SQL values]` debug logging** — caps the logged param repr at 1000 chars (guarded by `isEnabledFor(DEBUG)` to keep the hot path lazy). Keeps production debug logs sane too. ### Testing - The reproduction (`trial -j2 tests.logging.test_loggers <the EDU test>`) went from **~3/8 failing** to **10/10 passing**. - Confirmed the root level is restored after the logging tests, and that the `[SQL values]` line is now capped (~50 KB with a `[truncated]` marker, was 65 708). - `ruff` + `mypy` clean; `tests.logging.test_loggers` and `tests.rest.client.test_sendtodevice` pass (14/14). --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Eric Eastwood <erice@element.io>
…ns (element-hq#19847) I noticed that the documentation doesn't match reality. The values from the database are cast to bools at: - https://github.com/element-hq/synapse/blob/ff034c1f623bc769f1f3bca801592dd42697ea66/synapse/storage/databases/main/registration.py#L424-L439 - https://github.com/element-hq/synapse/blob/ff034c1f623bc769f1f3bca801592dd42697ea66/synapse/storage/databases/main/__init__.py#L330-L344
…sycopg preference
jason-famedly
commented
Jun 12, 2026
Comment on lines
-479
to
+533
| # for COPY. This assumes all inner sequences are the same length. | ||
| # for COPY. These inner sequences must be the same length. | ||
| assertion_length = 0 | ||
| for _inner_value in values: | ||
| # Check for the Sized class here, to verify that this particular | ||
| # iterable can use len(). In the future, switch the `values` argument to | ||
| # use this as a Collection instead of an Iterable, which allows all the | ||
| # types wanted while excluding Generators and this assertion can be | ||
| # removed. | ||
| assert isinstance(_inner_value, Sized) | ||
| if not assertion_length: | ||
| assertion_length = len(_inner_value) | ||
| assert assertion_length == len(_inner_value) | ||
|
|
||
| # To avoid having to port several psycopg2 utilities that are built into its | ||
| # Cursor class(mogrify, for example) and import execute_values() from it's | ||
| # 'extras' module, use a different mechanism that facilitates performance. | ||
| # | ||
| # The COPY Postgres-only verb allows for a bulk import and export of data. | ||
| # However building this query for use with VALUES is somewhat convoluted. | ||
| # | ||
| # For exporting of data, which would be the equivalent of a SELECT query, | ||
| # and given a simple query of the sort: | ||
| # SELECT * FROM table, (VALUES ?) AS ld(id) WHERE table.id = ld.id | ||
| # and VALUES being an example of sequential numbers, 1-5 representing the 5 | ||
| # rows to retrieve, the "VALUES ?" clause needs to be expanded to | ||
| # VALUES (?), (?), (?), (?), (?) | ||
| # Then, the values themselves have to be flattened in a similar fashion | ||
| # [(1, 2, 3, 4, 5)] | ||
| # Similarly, disregarding that the WHERE clause will no longer hold true, if | ||
| # the count VALUES to be passed were 3, then VALUES clause would expand to: | ||
| # VALUES (?, ?, ?), (?, ?, ?), (?, ?, ?), (?, ?, ?), (?, ?, ?) | ||
| # and for berevity assume that all passed values were actually the same, the | ||
| # values would be flattened to look like: | ||
| # [(1, 1, 1, 2, 2, 2, 3, 3, 3, 4, 4, 4, 5, 5, 5)] | ||
|
|
Author
There was a problem hiding this comment.
Definitely make sure this is right
jason-famedly
commented
Jun 12, 2026
Comment on lines
+130
to
+136
| # The "pure python" implementation of psycopg(v3+) does not play nicely with the | ||
| # complement docker image because of differences in the required debian package `libpq`. | ||
| # Using the "binary" extra to inject that correct lib prevented the debian packaging of | ||
| # Synapse to fail(see test workflow 'poetry_lockfile' where all packages must have | ||
| # sdists and not just wheels). The "c" extra is a source distribution, but requires a | ||
| # compilation step during build. | ||
| # |
Author
There was a problem hiding this comment.
Probably make sure this makes sense to someone other than me
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
18 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merge in current upstream
developTry and address some feedback