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

Experimental MSC3890 Implementation: Fix deleting account data when using an account data writer worker #14869

Merged
merged 3 commits into from
Mar 3, 2023

Conversation

anoadragon453
Copy link
Member

@anoadragon453 anoadragon453 commented Jan 19, 2023

This PR fixes a bug introduced in #14714 with two one major change:

  • Stop providing a content kwarg to ReplicationRemoveUserAccountDataRestServlet, which didn't take that argument. This was causing a TypeError when trying to remove user account when experimental MSC3890 support was enabled and an account data writer worker was in use.
  • Set WAIT_FOR_STREAMS to False for ReplicationRemoveUserAccountDataRestServlet. Don't update the account_data stream when a delete comes in for data that doesn't exist. This stops the master process waiting for the account_data stream to increment after issuing a replication command. In the case of attempting to delete an account data entry which doesn't exist, we don't update the account data stream. This saves up from needing to wake up any listeners when a change in state hasn't actually occurred.

The only other change is to return the maximum stream ID (one that has not been incremented) instead of None in the response to the replication command. This means that in the case of a replication call to remove account data that didn't exist, the replication response body is now {"max_stream_id": current_stream_id} instead of {"max_stream_id": None}.

Nothing actually read this response body other than the replication stream code, but it feels better to respond with the actual maximum stream ID in this case, rather than None.

Thanks to @realtyem for pointing out the bug in #synapse-dev.

@anoadragon453 anoadragon453 marked this pull request as ready for review January 19, 2023 16:13
@anoadragon453 anoadragon453 requested a review from a team as a code owner January 19, 2023 16:13
anoadragon453 added a commit that referenced this pull request Jan 19, 2023
We were missing some possible worker types from the list that is used
in CI and complement.sh. This will likely cause CI to be slower, but
comes with the benefit of more complete test coverage.

'account_data' being missing from this list caused CI to not catch
#14869.
@squahtx
Copy link
Contributor

squahtx commented Feb 1, 2023

The content kwarg fix looks good. I'm not sure about the rationale behind the WAIT_FOR_STREAMS change.

@anoadragon453
Copy link
Member Author

It's possible that this bug was actually only introduced with #14820, which added WAIT_FOR_STREAMS.

@anoadragon453
Copy link
Member Author

Since the rework and adding a get_next_txn method to StreamIdGenerator has bumped the lines changed of this PR 10x fold, I've split out the latter to make review easier: #15191.

_remove_user_data_client does not take a `content` argument.
...instead of returning None when there was not actually any account
data to delete.

This means that in the case of a replication call to remove account data
that didn't exist, the replication response body is now {max_stream_id:
current_stream_id} instead of {max_stream_id: None}.
@anoadragon453 anoadragon453 force-pushed the anoa/fix_remove_account_data_stream branch 2 times, most recently from b2cd3b8 to e0ae9cf Compare March 2, 2023 18:20
@anoadragon453
Copy link
Member Author

anoadragon453 commented Mar 2, 2023

And due to #14869 (comment), the line diff is now back down closer to where it was originally :)

I went for a rebase of the PR here due to so much code being ripped out that it seems easier to just review afresh.

Copy link
Contributor

@squahtx squahtx left a comment

Choose a reason for hiding this comment

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

lgtm!

@realtyem
Copy link
Contributor

realtyem commented Mar 2, 2023

Did you re-test it with the account_data worker added?

@anoadragon453
Copy link
Member Author

Did you re-test it with the account_data worker added?

I have been doing so locally with:

COMPLEMENT_DIR="../complement" WORKERS=1 WORKER_TYPES=account_data ./scripts-dev/complement.sh -run "TestLogout"

and it does pass :)

@anoadragon453 anoadragon453 merged commit 15e975f into develop Mar 3, 2023
@anoadragon453 anoadragon453 deleted the anoa/fix_remove_account_data_stream branch March 3, 2023 10:51
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Apr 9, 2023
Synapse 1.80.0 (2023-03-28)
===========================

No significant changes since 1.80.0rc2.


Synapse 1.80.0rc2 (2023-03-22)
==============================

Bugfixes
--------

- Fix a bug in which the [`POST /_matrix/client/v3/rooms/{roomId}/report/{eventId}`](https://spec.matrix.org/v1.6/client-server-api/#post_matrixclientv3roomsroomidreporteventid) endpoint would return the wrong error if the user did not have permission to view the event. This aligns Synapse's implementation with [MSC2249](matrix-org/matrix-spec-proposals#2249). ([\#15298](matrix-org/synapse#15298), [\#15300](matrix-org/synapse#15300))
- Fix a bug introduced in Synapse 1.75.0rc1 where the [SQLite port_db script](https://matrix-org.github.io/synapse/latest/postgres.html#porting-from-sqlite)
  would fail to open the SQLite database. ([\#15301](matrix-org/synapse#15301))


Synapse 1.80.0rc1 (2023-03-21)
==============================

Features
--------

- Stabilise support for [MSC3966](matrix-org/matrix-spec-proposals#3966): `event_property_contains` push condition. ([\#15187](matrix-org/synapse#15187))
- Implement [MSC2659](matrix-org/matrix-spec-proposals#2659): application service ping endpoint. Contributed by Tulir @ Beeper. ([\#15249](matrix-org/synapse#15249))
- Allow loading `/register/available` endpoint on workers. ([\#15268](matrix-org/synapse#15268))
- Improve performance of creating and authenticating events. ([\#15195](matrix-org/synapse#15195))
- Add topic and name events to group of events that are batch persisted when creating a room. ([\#15229](matrix-org/synapse#15229))


Bugfixes
--------

- Fix a long-standing bug in which the user directory would assume any remote membership state events represent a profile change. ([\#14755](matrix-org/synapse#14755), [\#14756](matrix-org/synapse#14756))
- Implement [MSC3873](matrix-org/matrix-spec-proposals#3873) to fix a long-standing bug where properties with dots were handled ambiguously in push rules. ([\#15190](matrix-org/synapse#15190))
- Faster joins: Fix a bug introduced in Synapse 1.66 where spurious "Failed to find memberships ..." errors would be logged. ([\#15232](matrix-org/synapse#15232))
- Fix a long-standing error when sending message into deleted room. ([\#15235](matrix-org/synapse#15235))


Updates to the Docker image
---------------------------

- Ensure the Dockerfile builds on platforms that don't have a `cryptography` wheel. ([\#15239](matrix-org/synapse#15239))
- Mirror images to the GitHub Container Registry (`ghcr.io/matrix-org/synapse`). ([\#15281](matrix-org/synapse#15281), [\#15282](matrix-org/synapse#15282))


Improved Documentation
----------------------

- Add a missing endpoint to the workers documentation. ([\#15223](matrix-org/synapse#15223))


Internal Changes
----------------

- Add additional functionality to declaring worker types when starting Complement in worker mode. ([\#14921](matrix-org/synapse#14921))
- Add `Synapse-Trace-Id` to `access-control-expose-headers` header. ([\#14974](matrix-org/synapse#14974))
- Make the `HttpTransactionCache` use the `Requester` in addition of the just the `Request` to build the transaction key. ([\#15200](matrix-org/synapse#15200))
- Improve log lines when purging rooms. ([\#15222](matrix-org/synapse#15222))
- Improve type hints. ([\#15230](matrix-org/synapse#15230), [\#15231](matrix-org/synapse#15231), [\#15238](matrix-org/synapse#15238))
- Move various module API callback registration methods to a dedicated class. ([\#15237](matrix-org/synapse#15237))
- Configure GitHub Actions for merge queues. ([\#15244](matrix-org/synapse#15244))
- Add schema comments about the `destinations` and `destination_rooms` tables. ([\#15247](matrix-org/synapse#15247))
- Skip processing of auto-join room behaviour if there are no auto-join rooms configured. ([\#15262](matrix-org/synapse#15262))
- Remove unused store method `_set_destination_retry_timings_emulated`. ([\#15266](matrix-org/synapse#15266))
- Reorganize URL preview code. ([\#15269](matrix-org/synapse#15269))
- Clean-up direct TCP replication code. ([\#15272](matrix-org/synapse#15272), [\#15274](matrix-org/synapse#15274))
- Make `configure_workers_and_start` script used in Complement tests compatible with older versions of Python. ([\#15275](matrix-org/synapse#15275))
- Add a `/versions` flag for [MSC3952](matrix-org/matrix-spec-proposals#3952). ([\#15293](matrix-org/synapse#15293))
- Bump hiredis from 2.2.1 to 2.2.2. ([\#15252](matrix-org/synapse#15252))
- Bump serde from 1.0.152 to 1.0.155. ([\#15253](matrix-org/synapse#15253))
- Bump pysaml2 from 7.2.1 to 7.3.1. ([\#15254](matrix-org/synapse#15254))
- Bump msgpack from 1.0.4 to 1.0.5. ([\#15255](matrix-org/synapse#15255))
- Bump gitpython from 3.1.30 to 3.1.31. ([\#15256](matrix-org/synapse#15256))
- Bump cryptography from 39.0.1 to 39.0.2. ([\#15257](matrix-org/synapse#15257))
- Bump pydantic from 1.10.4 to 1.10.6. ([\#15286](matrix-org/synapse#15286))
- Bump serde from 1.0.155 to 1.0.157. ([\#15287](matrix-org/synapse#15287))
- Bump anyhow from 1.0.69 to 1.0.70. ([\#15288](matrix-org/synapse#15288))
- Bump txredisapi from 1.4.7 to 1.4.9. ([\#15289](matrix-org/synapse#15289))
- Bump pygithub from 1.57 to 1.58.1. ([\#15290](matrix-org/synapse#15290))
- Bump types-requests from 2.28.11.12 to 2.28.11.15. ([\#15291](matrix-org/synapse#15291))



Synapse 1.79.0 (2023-03-14)
===========================

No significant changes since 1.79.0rc2.


Synapse 1.79.0rc2 (2023-03-13)
==============================

Bugfixes
--------

- Fix a bug introduced in Synapse 1.79.0rc1 where attempting to register a `on_remove_user_third_party_identifier` module API callback would be a no-op. ([\#15227](matrix-org/synapse#15227))
- Fix a rare bug introduced in Synapse 1.73 where events could remain unsent to other homeservers after a faster-join to a room. ([\#15248](matrix-org/synapse#15248))


Internal Changes
----------------

- Refactor `filter_events_for_server`. ([\#15240](matrix-org/synapse#15240))


Synapse 1.79.0rc1 (2023-03-07)
==============================

Features
--------

- Add two new Third Party Rules module API callbacks: [`on_add_user_third_party_identifier`](https://matrix-org.github.io/synapse/v1.79/modules/third_party_rules_callbacks.html#on_add_user_third_party_identifier) and [`on_remove_user_third_party_identifier`](https://matrix-org.github.io/synapse/v1.79/modules/third_party_rules_callbacks.html#on_remove_user_third_party_identifier). ([\#15044](matrix-org/synapse#15044))
- Experimental support for [MSC3967](matrix-org/matrix-spec-proposals#3967) to not require UIA for setting up cross-signing on first use. ([\#15077](matrix-org/synapse#15077))
- Add media information to the command line [user data export tool](https://matrix-org.github.io/synapse/v1.79/usage/administration/admin_faq.html#how-can-i-export-user-data). ([\#15107](matrix-org/synapse#15107))
- Add an [admin API](https://matrix-org.github.io/synapse/latest/usage/administration/admin_api/index.html) to delete a [specific event report](https://spec.matrix.org/v1.6/client-server-api/#reporting-content). ([\#15116](matrix-org/synapse#15116))
- Add support for knocking to workers. ([\#15133](matrix-org/synapse#15133))
- Allow use of the `/filter` Client-Server APIs on workers. ([\#15134](matrix-org/synapse#15134))
- Update support for [MSC2677](matrix-org/matrix-spec-proposals#2677): remove support for server-side aggregation of reactions. ([\#15172](matrix-org/synapse#15172))
- Stabilise support for [MSC3758](matrix-org/matrix-spec-proposals#3758): `event_property_is` push condition. ([\#15185](matrix-org/synapse#15185))


Bugfixes
--------

- Fix a bug introduced in Synapse 1.75 that caused experimental support for deleting account data to raise an internal server error while using an account data writer worker. ([\#14869](matrix-org/synapse#14869))
- Fix a long-standing bug where Synapse handled an unspecced field on push rules. ([\#15088](matrix-org/synapse#15088))
- Fix a long-standing bug where a URL preview would break if the discovered oEmbed failed to download. ([\#15092](matrix-org/synapse#15092))
- Fix a long-standing bug where an initial sync would not respond to changes to the list of ignored users if there was an initial sync cached. ([\#15163](matrix-org/synapse#15163))
- Add the `transaction_id` in the events included in many endpoints' responses. ([\#15174](matrix-org/synapse#15174))
- Fix a bug introduced in Synapse 1.78.0 where requests to claim dehydrated devices would fail with a `405` error. ([\#15180](matrix-org/synapse#15180))
- Stop applying edits when bundling aggregations, per [MSC3925](matrix-org/matrix-spec-proposals#3925). ([\#15193](matrix-org/synapse#15193))
- Fix a long-standing bug where the user directory search was not case-insensitive for accented characters. ([\#15143](matrix-org/synapse#15143))


Updates to the Docker image
---------------------------

- Improve startup logging in the with-workers Docker image. ([\#15186](matrix-org/synapse#15186))


Improved Documentation
----------------------

- Document how to use caches in a module. ([\#14026](matrix-org/synapse#14026))
- Clarify which worker processes the ThirdPartyRules' [`on_new_event`](https://matrix-org.github.io/synapse/v1.78/modules/third_party_rules_callbacks.html#on_new_event) module API callback runs on. ([\#15071](matrix-org/synapse#15071))
- Document using [Shibboleth](https://www.shibboleth.net/) as an OpenID Provider. ([\#15112](matrix-org/synapse#15112))
- Correct reference to `federation_verify_certificates` in configuration documentation. ([\#15139](matrix-org/synapse#15139))
- Correct small documentation errors in some `MatrixFederationHttpClient` methods. ([\#15148](matrix-org/synapse#15148))
- Correct the description of the behavior of `registration_shared_secret_path` on startup. ([\#15168](matrix-org/synapse#15168))


Deprecations and Removals
-------------------------

- Deprecate the `on_threepid_bind` module callback, to be replaced by [`on_add_user_third_party_identifier`](https://matrix-org.github.io/synapse/v1.79/modules/third_party_rules_callbacks.html#on_add_user_third_party_identifier). See [upgrade notes](https://github.com/matrix-org/synapse/blob/release-v1.79/docs/upgrade.md#upgrading-to-v1790). ([\#15044](matrix-org/synapse#15044))
- Remove the unspecced `room_alias` field from the [`/createRoom`](https://spec.matrix.org/v1.6/client-server-api/#post_matrixclientv3createroom) response. ([\#15093](matrix-org/synapse#15093))
- Remove the unspecced `PUT` on the `/knock/{roomIdOrAlias}` endpoint. ([\#15189](matrix-org/synapse#15189))
- Remove the undocumented and unspecced `type` parameter to the `/thumbnail` endpoint. ([\#15137](matrix-org/synapse#15137))
- Remove unspecced and buggy `PUT` method on the unstable `/rooms/<room_id>/batch_send` endpoint. ([\#15199](matrix-org/synapse#15199))


Internal Changes
----------------

- Run the integration test suites with the asyncio reactor enabled in CI. ([\#14101](matrix-org/synapse#14101))
- Batch up storing state groups when creating a new room. ([\#14918](matrix-org/synapse#14918))
- Update [MSC3952](matrix-org/matrix-spec-proposals#3952) support based on changes to the MSC. ([\#15051](matrix-org/synapse#15051))
- Refactor writing json data in `FileExfiltrationWriter`. ([\#15095](matrix-org/synapse#15095))
- Tighten the login ratelimit defaults. ([\#15135](matrix-org/synapse#15135))
- Fix a typo in an experimental config setting. ([\#15138](matrix-org/synapse#15138))
- Refactor the media modules. ([\#15146](matrix-org/synapse#15146), [\#15175](matrix-org/synapse#15175))
- Improve type hints. ([\#15164](matrix-org/synapse#15164))
- Move `get_event_report` and `get_event_reports_paginate` from `RoomStore` to `RoomWorkerStore`. ([\#15165](matrix-org/synapse#15165))
- Remove dangling reference to being a reference implementation in docstring. ([\#15167](matrix-org/synapse#15167))
- Add an option to force a rebuild of the "editable" complement image. ([\#15184](matrix-org/synapse#15184))
- Use nightly rustfmt in CI. ([\#15188](matrix-org/synapse#15188))
- Add a `get_next_txn` method to `StreamIdGenerator` to match `MultiWriterIdGenerator`. ([\#15191](matrix-org/synapse#15191))
- Combine `AbstractStreamIdTracker` and `AbstractStreamIdGenerator`. ([\#15192](matrix-org/synapse#15192))
- Automatically fix errors with `ruff`. ([\#15194](matrix-org/synapse#15194))
- Refactor database transaction for query users' devices to reduce database pool contention. ([\#15215](matrix-org/synapse#15215))
- Correct `test_icu_word_boundary_punctuation` so that it passes with the ICU versions available in Alpine and macOS. ([\#15177](matrix-org/synapse#15177))

<details><summary>Locked dependency updates</summary>

  - Bump actions/checkout from 2 to 3. ([\#15155](matrix-org/synapse#15155))
  - Bump black from 22.12.0 to 23.1.0. ([\#15103](matrix-org/synapse#15103))
  - Bump dawidd6/action-download-artifact from 2.25.0 to 2.26.0. ([\#15152](matrix-org/synapse#15152))
  - Bump docker/login-action from 1 to 2. ([\#15154](matrix-org/synapse#15154))
  - Bump matrix-org/backend-meta from 1 to 2. ([\#15156](matrix-org/synapse#15156))
  - Bump ruff from 0.0.237 to 0.0.252. ([\#15159](matrix-org/synapse#15159))
  - Bump serde_json from 1.0.93 to 1.0.94. ([\#15214](matrix-org/synapse#15214))
  - Bump types-commonmark from 0.9.2.1 to 0.9.2.2. ([\#15209](matrix-org/synapse#15209))
  - Bump types-opentracing from 2.4.10.1 to 2.4.10.3. ([\#15158](matrix-org/synapse#15158))
  - Bump types-pillow from 9.4.0.13 to 9.4.0.17. ([\#15211](matrix-org/synapse#15211))
  - Bump types-psycopg2 from 2.9.21.4 to 2.9.21.8. ([\#15210](matrix-org/synapse#15210))
  - Bump types-pyopenssl from 22.1.0.2 to 23.0.0.4. ([\#15213](matrix-org/synapse#15213))
  - Bump types-setuptools from 67.3.0.1 to 67.4.0.3. ([\#15160](matrix-org/synapse#15160))
  - Bump types-setuptools from 67.4.0.3 to 67.5.0.0. ([\#15212](matrix-org/synapse#15212))
  - Bump typing-extensions from 4.4.0 to 4.5.0. ([\#15157](matrix-org/synapse#15157))
</details>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants