-
Notifications
You must be signed in to change notification settings - Fork 598
MSC4140: update error responses #19539
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
Merged
AndrewFerr
merged 68 commits into
element-hq:develop
from
AndrewFerr:msc4140-error-updates
Jul 11, 2026
Merged
Changes from all commits
Commits
Show all changes
68 commits
Select commit
Hold shift + click to select a range
c2d1039
MSC4140: impose limit of scheduled delayed events
AndrewFerr c70c428
MSC4140: update error codes to match latest MSC
AndrewFerr 3b51b48
Add changelogs
AndrewFerr ac2bed9
Assert against variable message, not literal
AndrewFerr e02f554
Put MSC4140 config near other one & allow stable
AndrewFerr 2e01d7f
In test, assign default error message to variable
AndrewFerr 79a0218
Explain reason for location of experimental config
AndrewFerr a614ebb
Leave new config as experimental only
AndrewFerr 448bf17
Set Retry-After on delayed event limit error
AndrewFerr 43e14e9
Enforce max delayed event config to be positive
AndrewFerr f69ddc1
Update copyright dates
AndrewFerr 437a034
Revert "Update copyright dates"
AndrewFerr 5dfc8b8
Merge with 'develop'
AndrewFerr 1b19c13
Use capabilities to convey delayed event limits
AndrewFerr 267a7e5
Allow delayed event limit to be set to 0
AndrewFerr df09f8d
Rename variable to better indicate it as a time
AndrewFerr 3804db0
Elaborate comment on make_request time step
AndrewFerr 184b32d
Remove unnecessary wait in test
AndrewFerr c19df5d
Clarify config limits in errors & test comments
AndrewFerr 377cac4
Lint
AndrewFerr 0b0aab1
Document special-case error for limit <= 0
AndrewFerr d62f440
Cover case of limit << num existing delayed events
AndrewFerr 4a9464c
Clarify test on ratelimit override
AndrewFerr 79a6f31
Simplify delayed event limit test
AndrewFerr 34c5ce9
Use consistent keyvalues when applying limit
AndrewFerr 2eea7f5
Use "err" instead of "e"
AndrewFerr 87ce5f3
Fixups
AndrewFerr e1a0ba4
Test limit << num existing delayed events
AndrewFerr 1a2895b
Add alias to sub-SELECT, needed for PostgreSQL <16
AndrewFerr 7bc51d7
Apply suggestions from code review
AndrewFerr 56439ef
Improve test comments
AndrewFerr d2e84b7
Fix typo in test docstring
AndrewFerr af16899
Move all delayed event limit processing in handler
AndrewFerr 24a543a
Update imports
AndrewFerr b6e61ef
Run entire user limit test with ratelimit disabled
AndrewFerr 2278272
Add test comment to explain purpose of Retry-After
AndrewFerr d805c4d
Test for Retry-After having only a single value
AndrewFerr 5390153
Use TestCase assert for a non-narrowing assertion
AndrewFerr a3d0def
Rename capability field to `max_delay_ms`
AndrewFerr 45b6fc9
In test, lift ratelimit before making any request
AndrewFerr da30132
Test that is_processed events count against limit
AndrewFerr bcd5b61
In test, rename time values to proper unit
AndrewFerr 80b47aa
Enforce & document positive limit in storage fn
AndrewFerr 06a6a9e
Restore comment on Retry-After lookup query
AndrewFerr ba17cda
In tests, use closures instead of passing `*args`
AndrewFerr 17d1bfe
Tweak user limit tweak test
AndrewFerr 2f0a302
Use M_FORBIDDEN/403 for disallowed/exceeded delay
AndrewFerr 4e0f0bc
Apply suggestions from code review
AndrewFerr 2b41710
Include values in error msg of too-large delay
AndrewFerr ce02497
Rename function for parsing delay from request
AndrewFerr c73d85f
Add schema comment to explain is_processed
AndrewFerr 4a61644
Comment that test doesn't need specific errcode
AndrewFerr 73513bd
Replace ValueError with assert
AndrewFerr 71942b0
Delay Retry-After from response time, not request
AndrewFerr 287eedf
Comment query on num_existing > limit case
AndrewFerr c186d1d
Fix test coverage of num_existing > limit
AndrewFerr bf70a71
Test with a Duration for improved readability
AndrewFerr dce57e5
Use simpler query for num_existing == limit
AndrewFerr 5cd7c10
Reword comments on num_existing > limit case
AndrewFerr e828384
Replace final send_after_ms in test with Duration
AndrewFerr 43ebc76
Use Duration, not int, for delay & max delay
AndrewFerr 5208473
Revert query split, and elaborate comment
AndrewFerr 0859de0
Merge with 'develop' & fix conflict in /versions
AndrewFerr c33d443
Check msc4140_enabled in handler
AndrewFerr b384bb8
Rename limit-checking tests
AndrewFerr 0cab550
Reword comment in limit exceeded test
AndrewFerr c5b8248
Test error shape, not namespace of errcode
AndrewFerr 4c597e4
Merge with 'develop'
AndrewFerr File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| [MSC4140: Cancellable delayed events](https://github.com/matrix-org/matrix-spec-proposals/pull/4140): Update error responses to match their format in the current draft of the MSC. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| [MSC4140: Cancellable delayed events](https://github.com/matrix-org/matrix-spec-proposals/pull/4140): Limit how many delayed events a user may have scheduled at once. |
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
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
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
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
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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explain more why and what do do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
267a7e5 rewords the error message to be a bit more descriptive, and to follow the same format used by similar errors in this module.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the original intentions, I was more looking for something like this:
It seems like ideally, we would have had this kind of structure for delayed event config:
If we're going with
0as a valid value to disable delayed events, I guess the updated error message works ⏩There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That new config does look better, though I'd prefer using a dedicated PR to change it, given that
max_event_delay_durationhas been around for a while now & moving it would be a breaking change.I'd also prefer to keep config suggestions in the documentation instead of error messages, to reduce churn on code changes while the MSC is still unstable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like
msc4140_enabledwas added. Are we interested in getting the config shape correct from the beginning?Generally, we try not to break peoples homeserver config. And changing this later means even more complication for the backwards compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
msc4140_enabledisn't a real config key. It's just a computed value to make it easier to have/versionsreport whether delayed events are enabled via the other config keys, done since 0859de0. (It is also used in c33d443.)Which is to say, the config shape has not changed, nor do I intend to change it.