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

MSC4140: Cancellable delayed events #4140

Open
wants to merge 85 commits into
base: main
Choose a base branch
from

Conversation

@toger5 toger5 force-pushed the toger5/expiring-events-keep-alive branch from 2bc07c4 to 0eb1abc Compare May 7, 2024 17:03
@toger5 toger5 force-pushed the toger5/expiring-events-keep-alive branch from 0eb1abc to 8bf6db7 Compare May 8, 2024 15:49
Signed-off-by: Timo K <[email protected]>
@turt2live turt2live changed the title Draft for expiring event PR MSC4140: Expiring events with keep alive endpoint May 9, 2024
@turt2live turt2live added proposal A matrix spec change proposal client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels May 9, 2024
@toger5 toger5 force-pushed the toger5/expiring-events-keep-alive branch from 3e54c2a to c82adf7 Compare May 10, 2024 17:54
@toger5 toger5 force-pushed the toger5/expiring-events-keep-alive branch from c82adf7 to 54fff99 Compare May 10, 2024 18:08
…is used to trigger on of the actions

Signed-off-by: Timo K <[email protected]>
Add event type to the body
Add event id template variable
proposals/4140-delayed-events-futures.md Outdated Show resolved Hide resolved
proposals/4140-delayed-events-futures.md Outdated Show resolved Hide resolved
proposals/4140-delayed-events-futures.md Outdated Show resolved Hide resolved
proposals/4140-delayed-events-futures.md Outdated Show resolved Hide resolved
proposals/4140-delayed-events-futures.md Outdated Show resolved Hide resolved
proposals/4140-delayed-events-futures.md Outdated Show resolved Hide resolved
proposals/4140-delayed-events-futures.md Outdated Show resolved Hide resolved
proposals/4140-delayed-events-futures.md Outdated Show resolved Hide resolved
proposals/4140-delayed-events-futures.md Outdated Show resolved Hide resolved
proposals/4140-delayed-events-futures.md Outdated Show resolved Hide resolved
proposals/4140-delayed-events-futures.md Outdated Show resolved Hide resolved
proposals/4140-delayed-events-futures.md Outdated Show resolved Hide resolved
This reverts commit 772590f.
Clients should keep track of transaction IDs themselves.
@AndrewFerr
Copy link
Member

AndrewFerr commented Sep 19, 2024

As suggested during a review of Complement tests, this MSC should specify what happens when a user leaves a room they have pending delayed messages for.

The most "unsurprising" thing to do is to cancel all delayed events for a room when leaving it.

On the other hand, not cancelling them is an option as well:

  • If the user rejoins a room they had left, any of their still-pending delayed messages for that room will end up getting sent
  • If the user is not in a room by the time any of their delayed messages for that room are to be sent, the event sending will simply fail with a 400-class error

The latter is in the same vein as evaluating power levels at the point of sending.

Note that the Synapse implementation does not cancel delayed events for left rooms.

Comment on lines +255 to +256
Power levels are evaluated for each event only once the delay has occurred and it will be distributed/inserted into the
DAG. This implies a delayed event can fail if it violates power levels at the time the delay passes.
Copy link
Member

Choose a reason for hiding this comment

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

Should users be notified of delayed events that hit power level failures (or any 400-class error) upon being sent?

If so, perhaps delayed event send failures can be included in /sync responses, for clients to handle however they like.

If not, then this spec should make it explicit that delayed event send failures are effectively unhandled.

Copy link
Author

Choose a reason for hiding this comment

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

That is a really good comment.
Adding it to the sync response Sounds as if we would need to introduce a place for error messages there. Which be a very new concept I think.

What about adding it to the get delayed events endpoint.
We don't known if the client will be online if the failiour occurs so it is a very async check for errors anyways so making it part of a request does not sound too bad.
I don't know how to remove errors from the get delayed events list. Maybe as soon as they are fetched once they get removed?

Is that an issue if there are more clients getting futures?

Maybe each client should get the errors once?

Copy link
Author

Choose a reason for hiding this comment

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

I added this into the GET endpoint.

Comment on lines +109 to +110
- `delay` - Optional number of milliseconds the homeserver should wait before sending the event. If no `delay` is provided,
the event is sent immediately as normal.

Choose a reason for hiding this comment

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

Would it be possible to support setting the absolute target time rather than relative?
I was thinking of possibly using this as a "hack" to make sure a bunch of state event changes are batched in a single federation transaction by scheduling them at a single point in time ~5-10 seconds in the future. (eg. a moderation bot doing wildcard kicks/bans, or redacting tens if not hundreds of spam messages, this would allow for far higher event throughput and reduce federation overhead, where the bot may already be exempted from rate limits)

Just a thought, no intent to block this MSC.

Copy link
Member

Choose a reason for hiding this comment

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

The federation overhead should be the same, because the same amount of content (in your case, the moderation actions) will be sent in the request whether they're delayed or immediate events.

Unless you meant something else?

toger5 and others added 7 commits October 2, 2024 13:21
…hort term and longer term send_pdus is the better solution.
 - we have two GET endpoints `/shedueled` `/terminated` now.
 - The rule for when a state delayed event is cancelled changed to include a sender user condition.
yingziwu added a commit to yingziwu/synapse that referenced this pull request Oct 17, 2024
No significant changes since 1.117.0rc1.

- Add config option `redis.password_path`. ([\#17717](element-hq/synapse#17717))

- Fix a rare bug introduced in v1.29.0 where invalidating a user's access token from a worker could raise an error. ([\#17779](element-hq/synapse#17779))
- In the response to `GET /_matrix/client/versions`, set the `unstable_features` flag for [MSC4140](matrix-org/matrix-spec-proposals#4140) to `false` when server configuration disables support for delayed events. ([\#17780](element-hq/synapse#17780))
- Improve input validation and room membership checks in admin redaction API. ([\#17792](element-hq/synapse#17792))

- Clarify the docstring of `test_forget_when_not_left`. ([\#17628](element-hq/synapse#17628))
- Add documentation note about PYTHONMALLOC for accurate jemalloc memory tracking. Contributed by @hensg. ([\#17709](element-hq/synapse#17709))
- Remove spurious "TODO UPDATE ALL THIS" note in the Debian installation docs. ([\#17749](element-hq/synapse#17749))
- Explain how load balancing works for `federation_sender_instances`. ([\#17776](element-hq/synapse#17776))

- Minor performance increase for large accounts using sliding sync. ([\#17751](element-hq/synapse#17751))
- Increase performance of the notifier when there are many syncing users. ([\#17765](element-hq/synapse#17765), [\#17766](element-hq/synapse#17766))
- Fix performance of streams that don't change often. ([\#17767](element-hq/synapse#17767))
- Improve performance of sliding sync connections that do not ask for any rooms. ([\#17768](element-hq/synapse#17768))
- Reduce overhead of sliding sync E2EE loops. ([\#17771](element-hq/synapse#17771))
- Sliding sync minor performance speed up using new table. ([\#17787](element-hq/synapse#17787))
- Sliding sync minor performance improvement by omitting unchanged data from incremental responses. ([\#17788](element-hq/synapse#17788))
- Speed up sliding sync when there are many active subscriptions. ([\#17789](element-hq/synapse#17789))
- Add missing license headers on new source files. ([\#17799](element-hq/synapse#17799))

* Bump phonenumbers from 8.13.45 to 8.13.46. ([\#17773](element-hq/synapse#17773))
* Bump python-multipart from 0.0.10 to 0.0.12. ([\#17772](element-hq/synapse#17772))
* Bump regex from 1.10.6 to 1.11.0. ([\#17770](element-hq/synapse#17770))
* Bump ruff from 0.6.7 to 0.6.8. ([\#17774](element-hq/synapse#17774))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.