WIP: feat: Implement /_matrix/client/v3/notifications #1345
Draft
gamesguru
wants to merge 1 commit from
gamesguru/continuwuity:feat/notification-endpoint-cinny into main
pull from: gamesguru/continuwuity:feat/notification-endpoint-cinny
merge into: continuwuation:main
continuwuation:main
continuwuation:nex/fix/informative-startup-errs
continuwuation:renovate/tokio-1.x-lockfile
continuwuation:aranje/illegal-car-mods
continuwuation:ginger/password-reset
continuwuation:ginger/no-left-room-initial-sync
continuwuation:nex/feat/policy-servers-2-electric-boogaloo
continuwuation:renovate/rust-patch-updates
continuwuation:jade/docker-entrypoint
continuwuation:jade/dehydrated-devices
continuwuation:ginger/complement-fixes
continuwuation:nex/fix/stale-destination-cache
continuwuation:nex/experiment/sync-mutex
continuwuation:tcpipuk/docker-docs
continuwuation:jade/snafu
continuwuation:renovate/serde_html_form-0.x
continuwuation:jade/rand-update
continuwuation:renovate/reqwest-0.x
continuwuation:nex/stateres-refactor
continuwuation:ginger/779-in-troubleshooting
continuwuation:jade/liveit-guide
continuwuation:jade/http3
continuwuation:nex/feat/admin-hide-empty-rooms
continuwuation:ginger/oobe
continuwuation:nex/fix/debian-thingy
continuwuation:jade/ldap-admin-check
continuwuation:nex/fix/remote-restricted-joins
continuwuation:nex/feat/msc4406-sender-ignored
continuwuation:jade/deadlock-detection
continuwuation:nex/feat/room-shutdown
continuwuation:jade/get-started
continuwuation:jade/docs-guide
continuwuation:ginger/fix-local-invites
continuwuation:nex/fix/tpi
continuwuation:nex/feat/room-deletion
continuwuation:nex/feat/msc4322-media-redaction
continuwuation:ginger/stitched-order
continuwuation:jade/build-info
continuwuation:ginger/deps/update-rspress
continuwuation:jade/admin-announce-improvements
continuwuation:ginger/xtask-improvements
continuwuation:jade/improve-admin-config-display
continuwuation:nex/fix/better-stateres-error-logs
continuwuation:jade/sender-timeouts
continuwuation:nex/feat/custom-v12-room-ids
continuwuation:ginger/update-metadata
continuwuation:nex/feat/admin-force-logout
continuwuation:tom/max-perf-docs
continuwuation:nex/fix/invalid-appservice-reg
continuwuation:nex/feat/antispam
continuwuation:nex/feat/account-locking
continuwuation:jade/logging-cleanup
continuwuation:jade/remove-legacy-appservice-auth
continuwuation:nex/fix/key-query
continuwuation:jade/update-prek
continuwuation:nex/fix/room-summaries
continuwuation:ginger/restrict-admin-commands
continuwuation:ginger/enable-console-by-default
continuwuation:jade/tag-fixes
continuwuation:jade/otlp
continuwuation:nex/meta/pull-req-template
continuwuation:nex/fix/fed-invite-compliance
continuwuation:nex/feat/build-commit
continuwuation:nex/feat/join-logging
continuwuation:jade/mailmap-updates
continuwuation:jade/hack-ci-tmp
continuwuation:jade/v12-stable
continuwuation:jade/relations
continuwuation:ginger/database-refactor
continuwuation:jade/fix-ldap-uiaa
continuwuation:nex/fix/validation
continuwuation:ginger/nuke-invalid-msc4133-fields-in-migration
continuwuation:ginger/downgrade-artifact-actions
continuwuation:oddlid/reload-fix
continuwuation:jade/fix-assert
continuwuation:ginger/sync-v3-cleanup
continuwuation:ginger/remove-absolute-action-urls
continuwuation:jade/website
continuwuation:nex/fix/backoff
continuwuation:ginger/fix-mdbook-for-0.5
continuwuation:ginger/no-docker-on-prs
continuwuation:backport/v0.5.0-rc.8-1
continuwuation:nex/fed-improvements
continuwuation:jade/rust-1.90
continuwuation:jade/mirror-dockerhub
continuwuation:jade/clippy-fixes
continuwuation:jade/fix-support
continuwuation:jade/clean-images
continuwuation:jade/wal-compression-type
continuwuation:jade/flake-clone
continuwuation:ginger/upload-rpms-on-schedule
continuwuation:nex/fix/incoming-fetch
continuwuation:nex/fix/upgrade
continuwuation:tom/ci-fedora-rpm
continuwuation:jade/ci-release-fix
continuwuation:jade/rocksdb-10-5
continuwuation:ginger/fix-msc4133-migration
continuwuation:ginger/migrate-busted-tz
continuwuation:hydra/public
continuwuation:nex/feat/manual-extremities
continuwuation:nex/feat/async-media
continuwuation:nex/feat/fast-joins-hack-do-not-use-DO-NOT-USE
continuwuation:nex/feat/better-logging
continuwuation:trigger-ci-so-latest-isnt-on-illegal-car-mods
continuwuation:nex/feat/pins-backfill
continuwuation:jade/tuwunel-2025-06-old
continuwuation:jade/ai-slop-db-docs
continuwuation:nex/fix-create-auth
continuwuation:jade/version-stats
continuwuation:jade/read-receipts
continuwuation:jade/rust-toolchain-no-targets
continuwuation:jade/logging-features
continuwuation:jade/syncv5-typing
continuwuation:jade/msc2815
continuwuation:jade/purge-sync-tokens
continuwuation:morguldir/see-eye
continuwuation:jade/css-small-screen
continuwuation:nex/wip-751
continuwuation:tuwunel-rebase
continuwuation:test
continuwuation:oddlid/rename-admin-room-bot
continuwuation:strawberry/nix-ci-stuff
continuwuation:strawberry/valgrind
continuwuation:phonemain
continuwuation:strawberry/morgs-snake-sync-jason-main
continuwuation:newer-media-endpoints
continuwuation:folly-coroutines-async-io
continuwuation:federation-retry-timer-port
continuwuation:bad-attempt-at-extracting-homeserver-signing-key
continuwuation:room-deletion-attempt-do-not-use
Labels
Clear labels
This pull request or issue is currently blocked from being merged/closed
Something isn't working as intended
Commits picked from other conduit projects
This requires or includes changes to the database
Something dependency related
Automatic dependency upgrades by Renovate
Low difficulty to implement - touches few parts of the codebase, low complexity
High difficulty to implement - touches many parts of the codebase, high complexity
Medium difficulty to implement - touches more parts of the codebase, higher complexity
Improvements or additions to documentation
New feature or request
Good for newcomers
Additional eyes and keyboards are required for this one
Issues that have been inhereted from the project pre-fork
Features pertaining to homeserver administration
Features pertaining to the appservice API
Features pertaining to authentication
Features pertaining to client-to-server interactions
Issues relating to core matrix functionality, such as state resolution and PDU formats
Features pertaining to server-to-server interactions
Issues related to room version 12 and related changes (temporary label)
Features pertaining to unstable matrix features
Features pertaining to media interactions
Changes or issues related to trust & safety tooling
Related to housekeeping, maintenance, or other repo-meta.
Issues related to CI changes
Packaging
This issue is blocking the next release
This issue is very important
This issue is of a rather low priority
This item is related to general security
This issue has enough information and is confirmed
This issue or pull request already exists
This issue doesn't seem right
This issue needs further investigation
Questions or support requests
This will not be worked on
Ci/CD
Pull requests that update Rust code
Blocked
This pull request or issue is currently blocked from being merged/closed
Bug
Something isn't working as intended
Cherry-picking
Commits picked from other conduit projects
Database
This requires or includes changes to the database
Dependencies
Something dependency related
Dependencies/Renovate
Automatic dependency upgrades by Renovate
Difficulty
Easy
Low difficulty to implement - touches few parts of the codebase, low complexity
Difficulty
Hard
High difficulty to implement - touches many parts of the codebase, high complexity
Difficulty
Medium
Medium difficulty to implement - touches more parts of the codebase, higher complexity
Documentation
Improvements or additions to documentation
Enhancement
New feature or request
Good first issue
Good for newcomers
Help wanted
Additional eyes and keyboards are required for this one
Inherited
Issues that have been inhereted from the project pre-fork
Matrix/Administration
Features pertaining to homeserver administration
Matrix/Appservices
Features pertaining to the appservice API
Matrix/Auth
Features pertaining to authentication
Matrix/Client
Features pertaining to client-to-server interactions
Matrix/Core
Issues relating to core matrix functionality, such as state resolution and PDU formats
Matrix/E2EE
Matrix/Federation
Features pertaining to server-to-server interactions
Matrix/Hydra
Issues related to room version 12 and related changes (temporary label)
Matrix/MSC
Features pertaining to unstable matrix features
Matrix/Media
Features pertaining to media interactions
Matrix/T&S
Changes or issues related to trust & safety tooling
Meta
Related to housekeeping, maintenance, or other repo-meta.
Meta/CI
Issues related to CI changes
Meta/Packaging
Packaging
Priority
Blocking
This issue is blocking the next release
Priority
High
This issue is very important
Priority
Low
This issue is of a rather low priority
Security
This item is related to general security
Status
Confirmed
This issue has enough information and is confirmed
Status
Duplicate
This issue or pull request already exists
Status
Invalid
This issue doesn't seem right
Status
Needs Investigation
This issue needs further investigation
Support
Questions or support requests
To-Merge
Wont fix
This will not be worked on
old/ci/cd
Ci/CD
Archived
old/rust
Pull requests that update Rust code
Archived
No labels
Blocked
Bug
Cherry-picking
Database
Dependencies
Dependencies/Renovate
Difficulty
Easy
Difficulty
Hard
Difficulty
Medium
Documentation
Enhancement
Good first issue
Help wanted
Inherited
Matrix/Administration
Matrix/Appservices
Matrix/Auth
Matrix/Client
Matrix/Core
Matrix/E2EE
Matrix/Federation
Matrix/Hydra
Matrix/MSC
Matrix/Media
Matrix/T&S
Meta
Meta/CI
Meta/Packaging
Priority
Blocking
Priority
High
Priority
Low
Security
Status
Confirmed
Status
Duplicate
Status
Invalid
Status
Needs Investigation
Support
To-Merge
Wont fix
old/ci/cd
old/rust
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
6 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".
No due date set.
Dependencies
No dependencies set.
Reference
continuwuation/continuwuity!1345
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "gamesguru/continuwuity:feat/notification-endpoint-cinny"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This pull request adds an endpoint for Cinny's notification panel. WIP.
Closes: #707
Pull request checklist:
mainbranch, and the branch is named something other thanmain.myself, if applicable. This includes ensuring code compiles.
Looks good, thanks for opening this!
@ -0,0 +28,4 @@) -> Result<get_notifications::v3::Response> {// Extract the `limit` and `from` query parameterslet limit = body.limit.unwrap_or(uint!(10));let _from = body.from.as_deref();Does this need declaring when it's unused?
@ -0,0 +39,4 @@let mut rooms_stream = std::pin::pin!(services.rooms.user.stream_notification_counts(sender_user));while let Some((room_id, count)) = rooms_stream.next().await {let Ok(room_id) = room_id else { continue };Should probably
warn!()if there's an error encountered here rather than silently swallowing itfeat/notification-endpoint-cinnyto feat: Implement/_matrix/client/v3/notifications@ -0,0 +21,4 @@////// Get notifications for the user.////// Currently just returns an empty response.doesn't look like an empty response to me
@ -0,0 +30,4 @@let limit = body.limit.unwrap_or(uint!(10));let _from = body.from.as_deref();let sender_user = body.sender_user.as_ref().expect("user is authenticated");this can be replaced with a call to
body.sender_user()@ -0,0 +127,4 @@// Sort by timestamp descending (newest first)notifications.sort_by(|a, b| b.ts.cmp(&a.ts));// Apply limitit would probably be more efficient to use
take()on thepdus()stream instead of limiting them hereplease see latest implementation and fully critique. Config changes and git history will be cleaned up pending general approval but comments here are welcome as well
@ -61,0 +70,4 @@.stream_prefix::<(OwnedUserId, OwnedRoomId), u64, _>(&prefix).map(|res| match res {| Ok(((_user_id, room_id), count)) => (Ok(room_id), count),| Err(e) => (Err(e), 0),@nex Would you like a info/warn or debug at least here, too? Thoughts?
A warn for the Err arm is probably a good idea, since afaik errors aren't an expected operating condition here
uh oh, "Complete Job" has been running ~15 minutes on the "prek / clippy and cargo" check. I'm unsure if I've broken something, or not?
Don't worry too much about CI, sometimes it takes a little while to clean up. Hopefully we'll get an upgrade in the next couple months.
@Jade wrote in #1345 (comment):
Everything was fast imo, except that last step, "Complete job." Not sure what it's doing
it just does that sometimes
[C10y] Video Test of notification paging & mark as read
https://imgur.com/a/wiTHdDP#
ok, i think this is good on my end. Ready to be merged in whenever.
there is a small initial delay in my video/stress test, but I think that is due to Cinny and the UI. The
/notificationnetwork calls all return in under 100 ms. Seeing as my test involved over 1,500 notifications (close to 100 pages), some lag is expected.i'm not sure what a "news fragment is" or if my PR summary counts? I may need some guidance there.
There's a runaway memory leak.
I'm assuming it's due to my branch. STOPSHIP
there's also this bug poking its head around this branch..., You might have to re-dismiss every chat, since now everything is a new notification for some reason.
@ -644,6 +644,15 @@ path = "src/main"strip = "symbols"lto = "thin"# Hybrid dev/release: "fast" here refers to compile times, not runtime speedThis should be part of a separate PR.
@ -85,3 +80,1 @@// if let Some(ref_path) = run_git_command(&["symbolic-ref", "--quiet", "HEAD"])// { println!("cargo:rerun-if-changed=.git/{ref_path}");// }// Rerun if the git HEAD changesThese should be part of a separate PR, if they're now working correctly.
@ -121,3 +121,1 @@let last_state = self.services.state.summary_stripped(pdu, room_id).await;self.mark_as_invited(user_id, room_id, pdu.sender(), Some(last_state), None).await?;let sender = pdu.sender();What is the purpose of this change and the change to
mark_as_invited?unrelated change. speculative fix moved to another branch
b2f0331c4b703f5dbae7@gamesguru wrote in #1345 (comment):
For future reference, if you mark your PR as WIP, we won't review it (unless you ask us to) or consider it for merge. You can always mark it as WIP after opening it by editing the title and prepending
WIP:@gamesguru wrote in #1345 (comment):
I have had that on server restarts sometimes, not on this branch.
feat: Implementto WIP: feat: Implement/_matrix/client/v3/notifications/_matrix/client/v3/notifications@Henry-Hiles wrote in #1345 (comment):
yeah, just happened to me again on the
0.5.4 (196...)release. Is there an open bug for it i wonderThe memory leak fwiw is no joke. After 24 hours, it had virtually seized up. A message took 5 minutes to send.
I logged into my VPS and had to rub the crush out of my eyes.
... 17 gigs deep in the swap space? I thought i only had 2 Gigs of swap, what?
My initial hypothesis is a memory leak exists in master/main but was made far worse either by greatly increased database or federation dns resolver activity.
Currently I'm looking into briefly diagnosing the memory leak, addressing unnecessary network activity on this branch, and potentially drawing a compromise (query only first 24 notifications per user) to avoid any massive unexpected overhead while simultaneously shipping a relatively important feature.
703f5dbae751f7a2bb77FWIW there may be a leak in existing code but it does not seem to be triggered in main:

51f7a2bb775dc749d2c1Ok. Seems to be healthy after stress testing. Seems a heap was appropriate here. Now I'll add a news fragment and remove the noisy info log.
here's a log of the initial query vs. the cached run times, as well as a depth probe to ~4000 notifications showing only minimal attrition.
5dc749d2c1c0865b42dcWIP: feat: Implementto feat: Implement/_matrix/client/v3/notifications/_matrix/client/v3/notificationssmall bystander nitpick here: force pushing makes it difficult to follow the changes that were made.
You can compare between force pushes, squashing to remove pointless commits is good. Forgejo not bbeing good at seperating out the rebased branch from changes in force pushes is something that can be improved, but it's not really a git issue
As far as the implementation, I'm not sure how well it scales to larger servers. I can only say I tested it with a 1-person server, although up to ~10-15K notifications/unread messages.
I'm not fully sold. This PR might actually need a 3rd evolution. Perhaps, we are better off just making another table/column in the database to store notifications? Thoughts? Preferences?
considering how well implementing sync tokens to persist in the db worked, im not convinced that's a good idea.
then again; you got to store notifications somewhere, and in-memory they'd get lost on restarts, so i suppose a new column family for notifications would be a good idea. maybe delete notifications from the table once they've been marked as read?
I mean the main issue with this is that it's quite big and quite a lot to review. It also has some non-related things that have made their way in (
src/service/sending/stats.rs). From what I can see it's roughly the right approach. Adding a new table would probably be best for performance but comes with a few issues that'd have to be solved, so I'd rather not do that yet (for example, cleanup when room access changes, and cleanup of old notifications).If this was broken down into smaller changes, that would be much easier to review and merge.
@rooot You're exactly right, the performance concerns may be significant. You can see multiple cold calls taking on the order of 1 to 10 seconds before the cache is warm. Might that completely lock up a server with 200 users? Not sure. At least if it did, they should hopefully be able to configure lower limits.
But actually "synapse"-spec notifications are far less common. You might have 10,000 unread messages and only 100 or 500 meaningful notifications. My branch is was more verbose. It just gives you all 10,000 or however many unread messages your push rules and read receipts dictate.
This branch is probably cheaper in terms of developer effort, but more expensive in terms of CPU usage, than a table-based implementation.
I do not think the comparison to sync state tokens is necessarily accurate. I hope it won't take up anywhere near that much disc space, but I more or less agree with Jade that the current implementation on this branch is adequate (pending rigorous time/space complexity analysis).
@Jade wrote in #1345 (comment):
Hi Jade, I tried my best to not have any scope creep. But the metric/logging stuff was relevant given the severe memory leak at one point.
I'm not sure which other "smaller changes" you're referring to. I can try to separate some out, and perhaps we can compromise on some? If it satisfies a legitimate logging need, doesn't leak anything potentially secret to the console, and supports (perhaps slightly indirectly) the development efforts at hand?
feat: Implementto WIP: feat: Implement/_matrix/client/v3/notifications/_matrix/client/v3/notificationsActual notification impl looks good but there's a lot of scope creep, potential performance issues, and a few (opinionated) codestyle issues
@ -0,0 +3,4 @@use futures::StreamExt;use ruma::{MilliSecondsSinceUnixEpoch, UInt,api::client::push::{get_notifications, get_notifications::v3 as r},ris not a descriptive import name@ -0,0 +27,4 @@// Wrapper to order notifications by timestamp#[derive(Debug)]struct NotificationItem(r::Notification);It'd be cleaner to define this struct and associated impls outside of the route function
@ -0,0 +109,4 @@.is_joined(sender_user, &room_id).await{continue;Might be a good idea to clean up the counts for "stale rooms" here, otherwise this work will keep being repeated for each subsequent call
@ -0,0 +163,4 @@}// Skip events sent by the user themselvesif pdu.sender == *sender_user {I think some of these
ifcontinue/breakconditions can be combined@ -0,0 +175,4 @@.get_actions(sender_user, &ruleset, &power_levels, &pdu_raw, &room_id).await;let mut notify = false;Any reason for mutability? would
actions.iter().any(|v| matches!(v, &Action::Notify))not suffice?@ -0,0 +208,4 @@}// Capture heap stats before consuminglet heap_count = notifications.len();This probably isn't worth including especially when it's still executed even in release mode despite no relevant log
hi, thanks for the detailed feedback. i read all your other comments and am agreed with implementing them. Adding it to my list now.
I agree that the calculation here feels a bit clumsy and awkwardly placed, intruding in the code. iirc
sizewas constant, so not worth the effort.if depth, time or total count are easier to introspect (i.e., 2 lines or less) i would like to see it as info level on some subservice. Haven't looked into whether the repo supports database-specific log_level or filtering. If not, i can remove this i guess.
@ -56,22 +55,69 @@ impl crate::Service for Service {}async fn worker(self: Arc<Self>) -> Result<()> {use std::collections::HashMap;I think changes to presence should go in their own PR as they affect different areas
I definitely agree. Not sure how this stuck around here. Think I planned to include it w the server startup optimizations or a related PR.
@ -119,2 +119,4 @@},| MembershipState::Invite => {// Check invite filter before expensive stripped state computationlet sender = pdu.sender();This should go in its own PR as it is unrelated to notifications
@ -121,6 +125,19 @@ impl crate::Service for Service {joinset});// Periodic federation stats reporter (tracked via JoinSet so itThis should be its own PR as this isn't related to notifications
@ -843,6 +848,15 @@ impl Service {return Ok(Destination::Federation(server));}// Track federation statsThis should be its own PR as this isn't related to notifications
@ -0,0 +5,4 @@/// Lightweight atomic counters for federation activity./// Logged periodically and reset after each report.#[derive(Default)]pub struct FederationStats {This should be its own PR as this isn't related to notifications
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.