- Only build the Rust sender from a real `RustFederationClient`. A test
that mocks the federation client no longer fails with a `TypeError`.
- Log the `TX` and response lines under Python's logger names, so log
configs that name those loggers still apply.
- Move the PDUs and EDUs into the request body, so a transaction holds
one copy of them while it is retried, besides the canonical JSON.
- Drop the unused `time_now` from `Event::to_pdu_value`.
- Set the compare mode per `TransactionManager` in the tests that use
it, not on the class.
- Test that the tracing headers reach the request.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R5b7Ruy4cgt27Mjq6Ptxbb
At the new `transactions` level, `TransactionManager` hands each
transaction to the Rust sender. Python keeps the tracing span, the
module callback and the `last_sent_pdu_time` gauge.
In tests, every transaction body Rust builds is checked against the one
Python builds (`SYNAPSE_TEST_RUST_FED_STRICT`). The federation sender
and catch-up test cases also run at the new level, with Rust building
the transactions that their mocked `send_transaction` receives.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R5b7Ruy4cgt27Mjq6Ptxbb
- Create the sleeper's `Notified` under the map lock, so a wake that lands
just before the sleep starts is not lost. Count sleeps per destination
and forget the destination in a drop guard, which also runs on cancel.
- Map an SRV `.` target to `ConnectError`, as Python's resolver does.
- Call a custom parser's `finish` when its last write fails, and when the
request is cancelled mid-read.
- Check the path is ASCII after the backoff check, as Python does.
- `put_json` takes `rewrite_age_ts`, which `send_transaction` sets. Any
other `json_data_callback` sends through the Python client.
- Count attempts in the no-retry test, and test the request counters,
the proxy-worker fallback, shutdown and the server-up wake.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R5b7Ruy4cgt27Mjq6Ptxbb
Add the `experimental_features.rust_federation_sending` level, `off` or
`client` for now. At `client`, `MatrixFederationHttpClient` sends its
JSON requests (`put_json`, `post_json`, `get_json`,
`get_json_with_headers`, `delete_json`, `send_direct_request`) through
the Rust client. Media downloads, and an instance that sends through a
proxy worker, stay on the Python path.
The facade keeps the tracing span around the call and injects its
headers. The request and response counters are bumped from the GIL hop
that returns the result. The limiter's background write is factored out
of `RetryDestinationLimiter` so both paths share it.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R5b7Ruy4cgt27Mjq6Ptxbb
The Rust replacement for `MatrixFederationAgent` and its connection pool,
as a library: nothing in Synapse sends through it yet. The request layer
(signing, retries, backoff) comes next and will be its first caller.
`Agent::request` takes a `matrix-federation://` request, applies
`.well-known` delegation to the URI and the `Host` header, and sends it
through hyper-util's pooled client (5 idle connections per host, 120s
idle timeout, HTTP/1.1, no automatic retries), as Python's pool does.
The pool calls our connector, derived from matrix-hyper-federation-
client's `MatrixConnector` (Apache-2.0). It resolves the server name
(SRV), tries each server in order and returns the first error if all
fail. For each server it resolves addresses through the IP range policy,
connects TCP with Twisted's staggered attempts and 30s timeout, or
tunnels through `https_proxy` with `CONNECT` unless `no_proxy` exempts
the host, then does TLS verified against the delegated server name,
never the SRV target. The `.well-known` fetch shares the pool and, as in
Python, blocks IP literals by the blocklist alone.
Failures are a typed `TransportError` (DNS, connect, TLS, and so on), so
the request layer can tell a DNS failure from the rest. The agent
implements `FederationTransport`.
`RustFederationAgentForTest` exposes a blocking `request` to Python. The
trial tests drive it against a real TLS server on a thread, signed by
the test CA, covering the delegation, SRV, SNI, IP literal, blocklist,
certificate verification and connection reuse cases.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R5b7Ruy4cgt27Mjq6Ptxbb
Fixes#20309.
For endpoints that validate the request body with Pydantic, a validation
failure put Pydantic's raw verbose error text into the `error` field of
the standard error response, e.g.:
```json
{
"errcode": "M_MISSING_PARAM",
"error": "1 validation error for PutBody\\nroom_id\\n Field required [type=missing, input_value={}, input_type=dict]\\n For further information visit https://errors.pydantic.dev/2.12/v/missing"
}
```
This leaks internal details clients cannot act upon (model class names,
raw input values, pydantic.dev URLs). This PR adds a
`_format_pydantic_error` helper in `synapse/http/servlet.py` that builds
a short client-friendly message from the structured
`ValidationError.errors()` data instead of `str(e)`:
- `{}` against a model requiring `room_id` -> `Missing required field
'room_id'.`
- `{"room_id": 123}` -> `'room_id': Input should be a valid string.`
- nested models use dotted paths, e.g. `Missing required field
'put.room_id'.`
- multiple errors are joined: `Missing required field 'name'; Missing
required field 'put'.`
The `errcode` selection logic (`M_MISSING_PARAM` / `M_INVALID_PARAM` /
`M_BAD_JSON`) is unchanged.
We sometimes see a lot of `ERROR` logs like the following:
```
Closing scope Scope<... master.write_bytes_to_request> which is not the currently-active one None
```
This error is generated by `opentracing.Scope` checking that it is the
"active" one. Synapse tracks "active" spans via logcontexts, so this
indirectly asserts that the scope is closed in the context it was opened
in. However, the producer methods are often called from the reactor and
therefore withing the sentinel logcontext, which produces the error
above.
This specifically happens when the producer tries to write large
responses but gets paused, and then later resumes.
The fix is to simply use `Span` directly, rather than scopes. `Span`
does not perform the checks.
I noticed this when deploying #19979, though it is unrelated.
I.e. `default_config("test")` and `default_config("test", False)` are so
opaque and hard to connect the dots with.
Spawning from trying to figure out what the `server_name` is set as for
our `HomeserverTestCase` in order to reference it in
https://github.com/element-hq/synapse/pull/19848#discussion_r3397455309
This fixes one of the 2 blockers to using pytest instead of Trial (which
is not formally-motivated, but sometimes seems like an interesting idea
because
pytest has seen a lot of developer experience features that Trial
hasn't. It would also removes one more coupling to the Twisted
framework.)
---
The `test_` prefix to this test helper makes it appear as a test to
pytest.
We *can* set a `__test__ = False` attribute on the test, but it felt
cleaner to just rename it (as I also thought it would be a test from
that name!).
This was previously reported as:
https://github.com/element-hq/synapse/issues/18665
---------
Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org>
Related to https://github.com/element-hq/synapse/issues/17035, when
Synapse receives a request that is larger than the maximum size allowed,
it aborts the connection without ever sending back a HTTP response.
I dug into our usage of twisted and how best to try and report such an
error and this is what I came up with.
It would be ideal to be able to report the status from within
`handleContentChunk` but that is called too early on in the twisted http
handling code, before things have been setup enough to be able to
properly write a response.
I tested this change out locally (both with C-S and S-S apis) and they
do receive a 413 response now in addition to the connection being
closed.
Hopefully this will aid in being able to quickly detect when
https://github.com/element-hq/synapse/issues/17035 is occurring as the
current situation makes it very hard to narrow things down to that
specific issue without making a lot of assumptions.
This PR also responds with more meaningful error codes now in the case
of:
- multiple `Content-Length` headers
- invalid `Content-Length` header value
- request content size being larger than the `Content-Length` value
### Pull Request Checklist
<!-- Please read
https://element-hq.github.io/synapse/latest/development/contributing_guide.html
before submitting your pull request -->
* [X] Pull request is based on the develop branch
* [X] Pull request includes a [changelog
file](https://element-hq.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] [Code
style](https://element-hq.github.io/synapse/latest/code_style.html) is
correct (run the
[linters](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#run-the-linters))
---------
Co-authored-by: Eric Eastwood <erice@element.io>
This changes the arguments in clock functions to be `Duration` and
converts call sites and constants into `Duration`. There are still some
more functions around that should be converted (e.g.
`timeout_deferred`), but we leave that to another PR.
We also changes `.as_secs()` to return a float, as the rounding broke
things subtly. The only reason to keep it (its the same as
`timedelta.total_seconds()`) is for symmetry with `as_millis()`.
Follows on from https://github.com/element-hq/synapse/pull/19223
Spawning from adding some logcontext debug logs in
https://github.com/element-hq/synapse/pull/18966 and since we're not
logging at the `set_current_context(...)` level (see reasoning there),
this removes some usage of `set_current_context(...)`.
Specifically, `MockClock.call_later(...)` doesn't handle logcontexts
correctly. It uses the calling logcontext as the callback context
(wrong, as the logcontext could finish before the callback finishes) and
it didn't reset back to the sentinel context before handing back to the
reactor. It was like this since it was [introduced 10+ years
ago](https://github.com/element-hq/synapse/commit/38da9884e70e8e44bde14c67a7a8a9d49a8b87ac).
Instead of fixing the implementation which would just be a copy of our
normal `Clock`, we can just remove `MockClock`
### Background
As part of Element's plan to support a light form of vhosting (virtual
host) (multiple instances of Synapse in the same Python process), we're
currently diving into the details and implications of running multiple
instances of Synapse in the same Python process.
"Per-tenant logging" tracked internally by
https://github.com/element-hq/synapse-small-hosts/issues/48
### Prior art
Previously, we exposed `server_name` by providing a static logging
`MetadataFilter` that injected the values:
https://github.com/element-hq/synapse/blob/205d9e4fc4774850f34971469ae500e70119d17a/synapse/config/logger.py#L216
While this can work fine for the normal case of one Synapse instance per
Python process, this configures things globally and isn't compatible
when we try to start multiple Synapse instances because each subsequent
tenant will overwrite the previous tenant.
### What does this PR do?
We remove the `MetadataFilter` and replace it by tracking the
`server_name` in the `LoggingContext` and expose it with our existing
[`LoggingContextFilter`](https://github.com/element-hq/synapse/blob/205d9e4fc4774850f34971469ae500e70119d17a/synapse/logging/context.py#L584-L622)
that we already use to expose information about the `request`.
This means that the `server_name` value follows wherever we log as
expected even when we have multiple Synapse instances running in the
same process.
### A note on logcontext
Anywhere, Synapse mistakenly uses the `sentinel` logcontext to log
something, we won't know which server sent the log. We've been fixing up
`sentinel` logcontext usage as tracked by
https://github.com/element-hq/synapse/issues/18905
Any further `sentinel` logcontext usage we find in the future can be
fixed piecemeal as normal.
https://github.com/element-hq/synapse/blob/d2a966f922fdc95bc86f7fe55b7b54a9ab3f25c1/docs/log_contexts.md#L71-L81
### Testing strategy
1. Adjust your logging config to include `%(server_name)s` in the format
```yaml
formatters:
precise:
format: '%(asctime)s - %(server_name)s - %(name)s - %(lineno)d -
%(levelname)s - %(request)s - %(message)s'
```
1. Start Synapse: `poetry run synapse_homeserver --config-path
homeserver.yaml`
1. Make some requests (`curl
http://localhost:8008/_matrix/client/versions`, etc)
1. Open the homeserver logs and notice the `server_name` in the logs as
expected. `unknown_server_from_sentinel_context` is expected for the
`sentinel` logcontext (things outside of Synapse).
Introduce `Clock.call_when_running(...)` to wrap startup code in a
logcontext, ensuring we can identify which server generated the logs.
Background:
> Ideally, nothing from the Synapse homeserver would be logged against the `sentinel`
> logcontext as we want to know which server the logs came from. In practice, this is not
> always the case yet especially outside of request handling.
>
> Global things outside of Synapse (e.g. Twisted reactor code) should run in the
> `sentinel` logcontext. It's only when it calls into application code that a logcontext
> gets activated. This means the reactor should be started in the `sentinel` logcontext,
> and any time an awaitable yields control back to the reactor, it should reset the
> logcontext to be the `sentinel` logcontext. This is important to avoid leaking the
> current logcontext to the reactor (which would then get picked up and associated with
> the next thing the reactor does).
>
> *-- `docs/log_contexts.md`
Also adds a lint to prefer `Clock.call_when_running(...)` over
`reactor.callWhenRunning(...)`
Part of https://github.com/element-hq/synapse/issues/18905
Refactor `Measure` block metrics to be homeserver-scoped (add
`server_name` label to block metrics).
Part of https://github.com/element-hq/synapse/issues/18592
### Testing strategy
#### See behavior of previous `metrics` listener
1. Add the `metrics` listener in your `homeserver.yaml`
```yaml
listeners:
- port: 9323
type: metrics
bind_addresses: ['127.0.0.1']
```
1. Start the homeserver: `poetry run synapse_homeserver --config-path
homeserver.yaml`
1. Fetch `http://localhost:9323/metrics`
1. Observe response includes the block metrics
(`synapse_util_metrics_block_count`,
`synapse_util_metrics_block_in_flight`, etc)
#### See behavior of the `http` `metrics` resource
1. Add the `metrics` resource to a new or existing `http` listeners in
your `homeserver.yaml`
```yaml
listeners:
- port: 9322
type: http
bind_addresses: ['127.0.0.1']
resources:
- names: [metrics]
compress: false
```
1. Start the homeserver: `poetry run synapse_homeserver --config-path
homeserver.yaml`
1. Fetch `http://localhost:9322/_synapse/metrics` (it's just a `GET`
request so you can even do in the browser)
1. Observe response includes the block metrics
(`synapse_util_metrics_block_count`,
`synapse_util_metrics_block_in_flight`, etc)
Fixes various `mypy` errors associated with Twisted `24.11.0`.
Hopefully addresses https://github.com/element-hq/synapse/issues/17075,
though I've yet to test against `trunk`.
Changes should be compatible with our currently pinned Twisted version
of `24.7.0`.
The latest Twisted release changed how they implemented `__await__` on
deferreds, which broke the machinery we used to test cancellation.
This PR changes things a bit to instead patch the `__await__` method,
which is a stable API. This mostly doesn't change the core logic, except
for fixing two bugs:
- We previously did not intercept all await points
- After cancellation we now need to not only unblock currently blocked
await points, but also make sure we don't block any future await points.
c.f. https://github.com/twisted/twisted/pull/12226
---------
Co-authored-by: Devon Hudson <devon.dmytro@gmail.com>
I thought ruff check would also format, but it doesn't.
This runs ruff format in CI and dev scripts. The first commit is just a
run of `ruff format .` in the root directory.
During the migration the automated script to update the copyright
headers accidentally got rid of some of the existing copyright lines.
Reinstate them.
This reverts commit 5fe76b9434.
I think I had this accidentally commited on my local develop branch, and
so it accidentally got merged into upstream develop.
This should re-land with corrections in #16504.
Allow configuring the set of workers to proxy outbound federation traffic through (`outbound_federation_restricted_to`).
This is useful when you have a worker setup with `federation_sender` instances responsible for sending outbound federation requests and want to make sure *all* outbound federation traffic goes through those instances. Before this change, the generic workers would still contact federation themselves for things like profile lookups, backfill, etc. This PR allows you to set more strict access controls/firewall for all workers and only allow the `federation_sender`'s to contact the outside world.
