From 139d5e94a9cc919f2c53ccac6ef7dc824967987d Mon Sep 17 00:00:00 2001 From: BFlat Date: Fri, 30 Oct 2026 21:42:49 +0700 Subject: [PATCH] Server tests: rate limits measured at a clock the tests hold still (#1662) downloads_have_a_share_per_address failed once under a full gate: its 40 library fetches could take long enough for the address's download share, measured against Instant::now() and refilling 1 MiB a second, to refill one more. Store::clock() is now what every rate limit the store checks reads: the clock, or in test builds a time the test holds it at (a #[cfg(test)] field). The hardening, download burst and admin budget tests hold it still; the admin test is back to its exact budget (#1564 had allowed for refilling). Found flaking in gates. --- apps/figcraft-server/src/admin_budget_tests.rs | 14 +++----------- apps/figcraft-server/src/download_burst_tests.rs | 2 ++ apps/figcraft-server/src/hardening_tests.rs | 3 +++ apps/figcraft-server/src/store.rs | 14 ++++++++++++++ apps/figcraft-server/src/store/backend.rs | 12 ++++++------ apps/figcraft-server/src/store/files.rs | 4 ++-- apps/figcraft-server/src/store/library.rs | 4 ++-- 7 files changed, 32 insertions(+), 21 deletions(-) diff --git a/apps/figcraft-server/src/admin_budget_tests.rs b/apps/figcraft-server/src/admin_budget_tests.rs index fed3e5d91..71c62897c 100644 --- a/apps/figcraft-server/src/admin_budget_tests.rs +++ b/apps/figcraft-server/src/admin_budget_tests.rs @@ -21,18 +21,10 @@ fn a_flood_of_changes_from_one_address_is_held_back_and_others_are_not() { let mut store = Store::open(dir.path(), config(dir.path()).store()).unwrap(); let (file, ..) = store.publish("Poster", &snapshot("Poster")).unwrap(); let flooder = ip("198.51.100.1"); - let start = std::time::Instant::now(); + // The clock held still, so the budget doesn't refill while the requests run. + store.frozen = Some(std::time::Instant::now()); let done = (0..100).filter(|_| rename(&mut store, flooder, &file.id) == AdminReply::Done).count(); - // The budget refills one request a second, so a slow run (a loaded test machine) lets a few more in. - let budget = ADMIN_PER_MINUTE as usize; - let refilled = start.elapsed().as_secs() as usize + 1; - assert!((budget..=budget + refilled).contains(&done), "a minute's worth, then wait: {done} done"); - // Use up whatever refilled meanwhile; the request right after is refused. - for _ in 0..refilled { - if rename(&mut store, flooder, &file.id) != AdminReply::Done { - break; - } - } + assert_eq!(done, ADMIN_PER_MINUTE as usize, "a minute's worth, then wait"); assert_eq!(rename(&mut store, flooder, &file.id), AdminReply::Failed { reason: BUSY_ADMIN.into() }); // An owner token's requests share the address's budget. let hosted = store.hosted(flooder, Some(&file.id), Admin::Rename { file: file.id.clone(), name: "Mine".into() }, &|_| None).reply; diff --git a/apps/figcraft-server/src/download_burst_tests.rs b/apps/figcraft-server/src/download_burst_tests.rs index 61f367305..1a1f0f5df 100644 --- a/apps/figcraft-server/src/download_burst_tests.rs +++ b/apps/figcraft-server/src/download_burst_tests.rs @@ -58,6 +58,8 @@ fn large_downloads_keep_an_address_to_its_rate() { fn a_library_larger_than_the_burst_is_downloaded_then_the_address_waits() { let dir = TempDir::new("biglib"); let mut store = Store::open(dir.path(), config(dir.path()).store()).unwrap(); + // The clock held still, so the address's share doesn't refill while the test runs. + store.frozen = Some(Instant::now()); let (file, ..) = store.publish("Library", &snapshot("Library")).unwrap(); store.publish_library(&file.id, &archive(0, 40 * MIB)).unwrap(); let fetch = |s: &mut Store, who: &str| s.library(ip(who), &file.id, Access::View, LibraryReq::Fetch { have: None, usage: None }); diff --git a/apps/figcraft-server/src/hardening_tests.rs b/apps/figcraft-server/src/hardening_tests.rs index be28d22d6..186b8d9c0 100644 --- a/apps/figcraft-server/src/hardening_tests.rs +++ b/apps/figcraft-server/src/hardening_tests.rs @@ -525,6 +525,9 @@ fn downloads_have_a_share_per_address() { // A team library is served within it, then refused, saying why; another address still gets it. let dir = TempDir::new("libshare"); let mut store = Store::open(dir.path(), config(dir.path()).store()).unwrap(); + // The clock held still: on a loaded machine the 40 fetches can take seconds, long enough for + // the address's share to refill one more. + store.frozen = Some(t); let (file, ..) = store.publish("Library", &snapshot("Library")).unwrap(); store.publish_library(&file.id, &archive(0, 4 << 20)).unwrap(); let fetch = |s: &mut Store, who: &str| s.library(ip(who), &file.id, Access::View, LibraryReq::Fetch { have: None, usage: None }); diff --git a/apps/figcraft-server/src/store.rs b/apps/figcraft-server/src/store.rs index 9e12d328a..1feaf706b 100644 --- a/apps/figcraft-server/src/store.rs +++ b/apps/figcraft-server/src/store.rs @@ -286,6 +286,9 @@ pub struct Store { abuse: Abuse, /// Team library bytes served, everyone together. served: figcraft_collab_net::net::Bucket, + /// Tests: the time the rate limits read, held still (`None`: the clock). + #[cfg(test)] + pub(crate) frozen: Option, /// Usage reports by library file. usage: BTreeMap, blocked: BTreeSet, @@ -391,6 +394,15 @@ fn write_key(root: &Path) -> Result { const MAX_FIRST_LIBRARY_VERSION: u64 = 1 << 32; impl Store { + /// The time the rate limits are measured at: the clock, or in tests the time they hold it at. + pub(crate) fn clock(&self) -> Instant { + #[cfg(test)] + if let Some(t) = self.frozen { + return t; + } + Instant::now() + } + /// Use the data directory `root` (made if missing). Fails when another server or command holds /// it. Files that can't be read are skipped and listed in [`problems`](Self::problems). pub fn open(root: &Path, cfg: StoreConfig) -> Result { @@ -420,6 +432,8 @@ impl Store { directory, abuse, served: figcraft_collab_net::net::Bucket::new(LIBRARY_RATE, LIBRARY_BURST), + #[cfg(test)] + frozen: None, usage: BTreeMap::new(), blocked: BTreeSet::new(), reports: Vec::new(), diff --git a/apps/figcraft-server/src/store/backend.rs b/apps/figcraft-server/src/store/backend.rs index c7aa07e1c..ecef744c1 100644 --- a/apps/figcraft-server/src/store/backend.rs +++ b/apps/figcraft-server/src/store/backend.rs @@ -1,7 +1,7 @@ use super::journal::flush_log; use super::{ - Access, Action, Admin, AdminReply, Admission, BUSY_ADMIN, BUSY_DOWNLOADS, Backend, ClientMsg, Closing, Handled, HostInfo, Instant, IpAddr, - LibraryReply, LibraryReq, OpenFile, PluginReply, PluginReq, Server, Store, disk, hash, now, same_secret, + Access, Action, Admin, AdminReply, Admission, BUSY_ADMIN, BUSY_DOWNLOADS, Backend, ClientMsg, Closing, Handled, HostInfo, IpAddr, LibraryReply, + LibraryReq, OpenFile, PluginReply, PluginReq, Server, Store, disk, hash, now, same_secret, }; /// Whether `req` changes the server's files (a write, a removal or a join code's stretching): @@ -123,7 +123,7 @@ impl Backend for Store { if self.blocked.contains(&ip) { return Err("this server doesn't accept connections from your address".into()); } - if !self.abuse.allow(ip, Action::Connect, Instant::now()) { + if !self.abuse.allow(ip, Action::Connect, self.clock()) { return Err("too many connections from your address lately: wait a minute and try again".into()); } Ok(()) @@ -133,7 +133,7 @@ impl Backend for Store { // Cursors and cursor chat (many people behind one address each send ~30 a second while // they move) have a budget of their own, beside everything else. let what = if matches!(msg, ClientMsg::Presence(_) | ClientMsg::Signal(_) | ClientMsg::Ping) { Action::Presence } else { Action::Message }; - self.abuse.allow(ip, what, Instant::now()) + self.abuse.allow(ip, what, self.clock()) } fn hosting(&mut self, file: &str) -> Option { @@ -186,7 +186,7 @@ impl Backend for Store { fn admin(&mut self, ip: IpAddr, req: Admin, open: &dyn Fn(&str) -> Option) -> Handled { let failed = |reason: String| Handled::from(AdminReply::Failed { reason }); - if changes_files(&req) && !self.abuse.allow(ip, Action::Admin, Instant::now()) { + if changes_files(&req) && !self.abuse.allow(ip, Action::Admin, self.clock()) { log::info!("event=refused ip={ip} reason=admin_per_address"); return failed(BUSY_ADMIN.into()); } @@ -253,7 +253,7 @@ impl Backend for Store { // One address's share first (before everyone's budget is spent or anything is read). if let PluginReq::Fetch { id, version } = &req && let Some(size) = self.directory.size_of(id, version.as_deref()) - && !self.abuse.allow(ip, Action::Download(usize::try_from(size).unwrap_or(usize::MAX)), Instant::now()) + && !self.abuse.allow(ip, Action::Download(usize::try_from(size).unwrap_or(usize::MAX)), self.clock()) { return PluginReply::Failed { reason: BUSY_DOWNLOADS.into() }; } diff --git a/apps/figcraft-server/src/store/files.rs b/apps/figcraft-server/src/store/files.rs index f0d8ddcb0..dba351861 100644 --- a/apps/figcraft-server/src/store/files.rs +++ b/apps/figcraft-server/src/store/files.rs @@ -47,7 +47,7 @@ impl Store { if !open { return Err(KEY_ONLY.into()); } - if !self.abuse.allow(ip, Action::Publish, Instant::now()) { + if !self.abuse.allow(ip, Action::Publish, self.clock()) { return Err("too many files published from your address lately: try again in an hour".into()); } let blocks = self.per_block(&ip.to_string()); @@ -340,7 +340,7 @@ impl Store { return Err("say what is wrong with the file".into()); } let name = self.entry(file)?.meta.name.clone(); - if !self.abuse.allow(ip, Action::Report, Instant::now()) { + if !self.abuse.allow(ip, Action::Report, self.clock()) { return Err("too many reports from your address lately: try again in an hour".into()); } self.reports.push(ReportInfo { diff --git a/apps/figcraft-server/src/store/library.rs b/apps/figcraft-server/src/store/library.rs index 2e59e8215..4e76bc5fd 100644 --- a/apps/figcraft-server/src/store/library.rs +++ b/apps/figcraft-server/src/store/library.rs @@ -1,5 +1,5 @@ use super::{ - Action, BUSY_DOWNLOADS, Cursor, FILES, FileUsage, Instant, IpAddr, LIBRARY, LibraryMeta, LibraryReply, MAX_FIRST_LIBRARY_VERSION, MAX_REPORTED, + Action, BUSY_DOWNLOADS, Cursor, FILES, FileUsage, IpAddr, LIBRARY, LibraryMeta, LibraryReply, MAX_FIRST_LIBRARY_VERSION, MAX_REPORTED, MAX_USAGE_FILE, MAX_USERS, META, NO_PUBLIC_LIBRARY, Path, PathBuf, Store, USAGE, USAGE_WRITE_EVERY, Usage, UsageCache, clean_name, disk, fs, now, write_json, }; @@ -15,7 +15,7 @@ impl Store { } let path = library_file(&dir, lib.version); let size = fs::metadata(&path).map_or(0, |m| m.len()); - let now = Instant::now(); + let now = self.clock(); if !self.abuse.allow(ip, Action::Download(usize::try_from(size).unwrap_or(usize::MAX)), now) || !self.served.take_spread(now, size as f64) { return Err(BUSY_DOWNLOADS.into()); }