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

@@ -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;

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 });

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 });

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<Instant>,
     /// Usage reports by library file.
     usage: BTreeMap<String, UsageCache>,
     blocked: BTreeSet<IpAddr>,
@@ -391,6 +394,15 @@ fn write_key(root: &Path) -> Result<String, String> {
 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<Store, String> {
@@ -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(),

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<HostInfo> {
@@ -186,7 +186,7 @@ impl Backend for Store {
 
     fn admin(&mut self, ip: IpAddr, req: Admin, open: &dyn Fn(&str) -> Option<OpenFile>) -> 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() };
         }

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 {

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());
         }