Skip to content

fix: don't remove timeouts when a member leaves a server #409

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2,228 changes: 1,395 additions & 833 deletions Cargo.lock

Large diffs are not rendered by default.

4 changes: 3 additions & 1 deletion crates/core/database/src/models/server_members/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ auto_derived!(
Avatar,
Roles,
Timeout,
JoinedAt,
}

/// Member removal intention
Expand Down Expand Up @@ -186,6 +187,7 @@ impl Member {

pub fn remove_field(&mut self, field: &FieldsMember) {
match field {
FieldsMember::JoinedAt => (),
FieldsMember::Avatar => self.avatar = None,
FieldsMember::Nickname => self.nickname = None,
FieldsMember::Roles => self.roles.clear(),
Expand Down Expand Up @@ -224,7 +226,7 @@ impl Member {
intention: RemovalIntention,
silent: bool,
) -> Result<()> {
db.delete_member(&self.id).await?;
db.soft_delete_member(&self.id).await?;

EventV1::ServerMemberLeave {
id: self.id.server.to_string(),
Expand Down
12 changes: 10 additions & 2 deletions crates/core/database/src/models/server_members/ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,14 @@ pub trait AbstractServerMembers: Sync + Send {
remove: Vec<FieldsMember>,
) -> Result<()>;

/// Delete a server member by their id
async fn delete_member(&self, id: &MemberCompositeKey) -> Result<()>;
/// Marks a user as no longer a member of a server, while retaining the database value.
/// This is used to keep information such as timeouts in place, but will remove information such as join date and applied roles.
async fn soft_delete_member(&self, id: &MemberCompositeKey) -> Result<()>;

/// Forcibly delete a server member by their id.
/// This will cancel any pending timeouts or other longer term actions, and they will not be reapplied on rejoin.
async fn force_delete_member(&self, id: &MemberCompositeKey) -> Result<()>;

/// Fetch all members who have been marked for deletion.
async fn remove_dangling_members(&self) -> Result<()>;
}
102 changes: 95 additions & 7 deletions crates/core/database/src/models/server_members/ops/mongodb.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
use bson::Document;
use futures::StreamExt;
use iso8601_timestamp::Timestamp;
use mongodb::options::ReadConcern;
use revolt_result::Result;

Expand All @@ -13,7 +15,39 @@ static COL: &str = "server_members";
impl AbstractServerMembers for MongoDb {
/// Insert a new server member into the database
async fn insert_member(&self, member: &Member) -> Result<()> {
Comment on lines 16 to 17
Copy link
Member

Choose a reason for hiding this comment

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

For clarity, I think we should change the comment / rename the function here.
Specifically, something such as insert_or_merge_member, with the comment Insert a new server member (or use the existing member if one is found)

query!(self, insert_one, COL, &member).map(|_| ())
let existing: Result<Option<Document>> = query!(
self,
find_one,
COL,
doc! {
"_id.server": &member.id.server,
"_id.user": &member.id.user,
"pending_deletion_at": {"$exists": true}
}
);
// Update the existing record if it exist, otherwise make a new record
if existing.is_ok_and(|x| x.is_some()) {
self.col::<Document>(COL)
.update_one(
doc! {
"_id.server": &member.id.server,
"_id.user": &member.id.user,
},
doc! {
"$set": {
"joined_at": member.joined_at.duration_since(Timestamp::UNIX_EPOCH).whole_seconds(),
},
"$unset": {
"pending_deletion_at": ""
}
},
)
.await
.map(|_| ())
.map_err(|_| create_database_error!("update_one", COL))
} else {
query!(self, insert_one, COL, &member).map(|_| ())
}
}

/// Fetch a server member by their id
Expand All @@ -24,7 +58,8 @@ impl AbstractServerMembers for MongoDb {
COL,
doc! {
"_id.server": server_id,
"_id.user": user_id
"_id.user": user_id,
"pending_deletion_at": {"$exists": false}
}
)?
.ok_or_else(|| create_error!(NotFound))
Expand All @@ -35,7 +70,8 @@ impl AbstractServerMembers for MongoDb {
Ok(self
.col::<Member>(COL)
.find(doc! {
"_id.server": server_id
"_id.server": server_id,
"pending_deletion_at": {"$exists": false}
})
.await
.map_err(|_| create_database_error!("find", COL))?
Expand Down Expand Up @@ -143,7 +179,8 @@ impl AbstractServerMembers for MongoDb {
Ok(self
.col::<Member>(COL)
.find(doc! {
"_id.user": user_id
"_id.user": user_id,
"pending_deletion_at": {"$exists": false}
})
.await
.map_err(|_| create_database_error!("find", COL))?
Expand All @@ -164,6 +201,7 @@ impl AbstractServerMembers for MongoDb {
.col::<Member>(COL)
.find(doc! {
"_id.server": server_id,
"pending_deletion_at": {"$exists": false},
"_id.user": {
"$in": ids
}
Expand All @@ -185,7 +223,8 @@ impl AbstractServerMembers for MongoDb {
async fn fetch_member_count(&self, server_id: &str) -> Result<usize> {
self.col::<Member>(COL)
.count_documents(doc! {
"_id.server": server_id
"_id.server": server_id,
"pending_deletion_at": {"$exists": false}
})
.await
.map(|c| c as usize)
Expand All @@ -196,7 +235,8 @@ impl AbstractServerMembers for MongoDb {
async fn fetch_server_count(&self, user_id: &str) -> Result<usize> {
self.col::<Member>(COL)
.count_documents(doc! {
"_id.user": user_id
"_id.user": user_id,
"pending_deletion_at": {"$exists": false}
})
.await
.map(|c| c as usize)
Expand Down Expand Up @@ -225,8 +265,42 @@ impl AbstractServerMembers for MongoDb {
.map(|_| ())
}

/// Marks a member for deletion.
/// This will remove the record if the user has no pending actions (eg. timeout),
/// otherwise will slate the record for deletion by revolt_crond once the actions expire.
async fn soft_delete_member(&self, id: &MemberCompositeKey) -> Result<()> {
let member = self.fetch_member(&id.server, &id.user).await;
if let Ok(member) = member {
if member.in_timeout() {
self.col::<Document>(COL)
.update_many(
doc! {
"_id.server": &id.server,
"_id.user": &id.user,
},
doc! {
"$set": {"pending_deletion_at": format!("{}", member.timeout.unwrap().format())},
"$unset": {
"joined_at": "",
"avatar": "",
"nickname": "",
"roles": ""
}
},
)
.await
.map(|_| ())
.map_err(|_| create_database_error!("update_many", COL))
} else {
self.force_delete_member(id).await
}
} else {
Err(create_database_error!("fetch_member", COL))
}
}

/// Delete a server member by their id
async fn delete_member(&self, id: &MemberCompositeKey) -> Result<()> {
async fn force_delete_member(&self, id: &MemberCompositeKey) -> Result<()> {
query!(
self,
delete_one,
Expand All @@ -238,11 +312,25 @@ impl AbstractServerMembers for MongoDb {
)
.map(|_| ())
}

async fn remove_dangling_members(&self) -> Result<()> {
let now = Timestamp::now_utc();
let date = format!("{}", now.format_short()); // I hate this library

self.col::<Document>(COL)
.delete_many(doc! {
"pending_deletion_at": {"$lt": date}
})
.await
.map(|_| ())
.map_err(|_| create_database_error!("count_documents", COL))
}
}

impl IntoDocumentPath for FieldsMember {
fn as_path(&self) -> Option<&'static str> {
Some(match self {
FieldsMember::JoinedAt => "joined_at",
FieldsMember::Avatar => "avatar",
FieldsMember::Nickname => "nickname",
FieldsMember::Roles => "roles",
Expand Down
24 changes: 23 additions & 1 deletion crates/core/database/src/models/server_members/ops/reference.rs
Original file line number Diff line number Diff line change
Expand Up @@ -169,13 +169,35 @@ impl AbstractServerMembers for ReferenceDb {
}
}

/// Soft delete a member
async fn soft_delete_member(&self, id: &MemberCompositeKey) -> Result<()> {
let mut server_members = self.server_members.lock().await;

let member = server_members.get_mut(id);
if let Some(member) = member {
if member.in_timeout() {
panic!("Soft deletion is not implemented.")
Copy link
Member

Choose a reason for hiding this comment

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

Missing impl.

Copy link
Member Author

Choose a reason for hiding this comment

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

Impl not possible without modifying the member object since the reference db uses members directly.

} else if server_members.remove(id).is_some() {
Ok(())
} else {
Err(create_error!(NotFound))
}
} else {
Err(create_error!(NotFound))
}
}

/// Delete a server member by their id
async fn delete_member(&self, id: &MemberCompositeKey) -> Result<()> {
async fn force_delete_member(&self, id: &MemberCompositeKey) -> Result<()> {
let mut server_members = self.server_members.lock().await;
if server_members.remove(id).is_some() {
Ok(())
} else {
Err(create_error!(NotFound))
}
}

async fn remove_dangling_members(&self) -> Result<()> {
todo!()
Copy link
Member

Choose a reason for hiding this comment

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

Missing impl.

Copy link
Member Author

Choose a reason for hiding this comment

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

Impl not possible without modifying the member object since the reference db uses members directly.

}
}
4 changes: 3 additions & 1 deletion crates/core/database/src/util/bridge/v0.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use revolt_models::v0::*;
use revolt_permissions::{calculate_user_permissions, UserPermission};

use crate::{util::permissions::DatabasePermissionQuery, Database, FileUsedFor};
use crate::{util::permissions::DatabasePermissionQuery, Database};

impl crate::Bot {
pub fn into_public_bot(self, user: crate::User) -> PublicBot {
Expand Down Expand Up @@ -696,6 +696,7 @@ impl From<crate::FieldsMember> for FieldsMember {
crate::FieldsMember::Nickname => FieldsMember::Nickname,
crate::FieldsMember::Roles => FieldsMember::Roles,
crate::FieldsMember::Timeout => FieldsMember::Timeout,
crate::FieldsMember::JoinedAt => FieldsMember::JoinedAt,
}
}
}
Expand All @@ -707,6 +708,7 @@ impl From<FieldsMember> for crate::FieldsMember {
FieldsMember::Nickname => crate::FieldsMember::Nickname,
FieldsMember::Roles => crate::FieldsMember::Roles,
FieldsMember::Timeout => crate::FieldsMember::Timeout,
FieldsMember::JoinedAt => crate::FieldsMember::JoinedAt,
}
}
}
Expand Down
1 change: 1 addition & 0 deletions crates/core/models/src/v0/server_members.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ auto_derived!(
Avatar,
Roles,
Timeout,
JoinedAt,
}

/// Member removal intention
Expand Down
5 changes: 3 additions & 2 deletions crates/daemons/crond/src/main.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use revolt_config::configure;
use revolt_database::DatabaseInfo;
use revolt_result::Result;
use tasks::{file_deletion, prune_dangling_files};
use tasks::{file_deletion, prune_dangling_files, prune_members};
use tokio::try_join;

pub mod tasks;
Expand All @@ -13,7 +13,8 @@ async fn main() -> Result<()> {
let db = DatabaseInfo::Auto.connect().await.expect("database");
try_join!(
file_deletion::task(db.clone()),
prune_dangling_files::task(db)
prune_dangling_files::task(db.clone()),
prune_members::task(db.clone())
)
.map(|_| ())
}
1 change: 1 addition & 0 deletions crates/daemons/crond/src/tasks/mod.rs
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
pub mod file_deletion;
pub mod prune_dangling_files;
pub mod prune_members;
18 changes: 18 additions & 0 deletions crates/daemons/crond/src/tasks/prune_members.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
use std::time::Duration;

use log::warn;
use revolt_database::Database;
use revolt_result::Result;
use tokio::time::sleep;

pub async fn task(db: Database) -> Result<()> {
loop {
let success = db.remove_dangling_members().await;
if let Err(s) = success {
revolt_config::capture_error(&s);
warn!("Failed to prune dangling members: {:?}", &s);
}

sleep(Duration::from_secs(90)).await;
}
}
1 change: 1 addition & 0 deletions crates/delta/src/util/ratelimiter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ fn resolve_bucket<'r>(request: &'r rocket::Request<'_>) -> (&'r str, Option<&'r
};

if let Some(segment) = segment {
#[allow(clippy::redundant_locals)]
let resource = resource;

let method = request.method();
Expand Down
Loading