-
Notifications
You must be signed in to change notification settings - Fork 11
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
[Bug]: /v1/topics can recurse forever if there are not enough communities with activity to fill WWP #47
Comments
Copy-paste from Discord: The root cause of this is that the calculation assumes there is 10 communities with at least 1 post each, and so it pulls from the const popularCommunities = await Post.aggregate<{ _id: null; count: number; }>([
{
$match: {
created_at: {
$gte: last24Hours
},
message_to_pid: null,
community_id: {
$in: communityIDs
}
}
},
{
$group: {
_id: '$community_id',
count: {
$sum: 1
}
}
},
{
$limit: limit
},
{
$sort: {
count: -1
}
}
]); That's how it knows if a community is "popular", it's based on the number of interactions a community has had within a given timeframe. If a community has 0 posts, then this will obviously not return anything for that community The "within a given timeframe" part is why this recursion exists in the first place, to widen the search range in case a community has not been interacted with in a while I'm bouncing some stuff around in my head to try and see if we can just do this in one shot, a single aggregation call. But that might require us to redefine what we call "popular" Right now, the way we define "popular" is the number of posts within a timeframe ("hot" rather than "popular", I guess is a better way to say it). But if we redefine "popular" to be "total number of posts" then I think this can easily be done in a single shot? Though both definitions have their pros and cons The definition we have right now means that every community has a fair shot of being in WWP even if the community is new/smaller (if CommunityA only has 15 posts but they were all posted in the past 10 minutes, and CommunityB has 100 posts but was last interacted with 6 months ago, CommunityA wins), at the cost of worse DB calls "Total number of posts" would likely have better DB calls and remove the recursion, but at the cost of smaller/newer communities always being overshadowed by the older, larger, ones (it wont matter if a CommunityA is getting all the attention while CommunityB has not has users in months, if CommunityB has more posts overall then it wins) |
I think a good first step here would be to update this sanity check to also ensure that the communities it checks for has at least 1 post in them miiverse-api/src/services/api/routes/topics.ts Lines 222 to 243 in 28ef808
If there aren't 10 valid communities with posts then bail early. Otherwise, the existing system SHOULD work just fine? |
Checked Existing
What happened?
(The root cause appears to be https://github.com/PretendoNetwork/miiverse-api/blob/dev/src/services/api/routes/topics.ts#L275-L277)
If at least 10 communities with posts do not exist, the above code will loop forever, hammering the DB, and causing immense CPU and RAM usage.
What did you expect to happen?
One of 2 things:
Steps to reproduce?
I personally have not encountered the issue, but a self-hoster on Discord has in dev-supporters:
https://discord.com/channels/408718485913468928/882269480246472744/1327642983172018328
Other relevant information. (OPTIONAL)
No response
The text was updated successfully, but these errors were encountered: