fix(dashboard): keep a save that lands while a refresh is reading the store

server/src/auth.rs

@@ -8,6 +8,7 @@ mod gitlab;
 mod namespaces;
 
 use std::collections::HashMap;
+use std::sync::atomic::{AtomicU64, Ordering};
 use std::sync::{Arc, RwLock};
 
 use axum::extract::{Path, Request, State};
@@ -72,6 +73,11 @@ pub enum Authorizer {
         // two hundred objects under one token costs one.
         budget: Budget,
         access: Arc<RwLock<Access>>,
+        // Counts the changes somebody asked for. A refresh reads the store, which
+        // is a round trip, and what it read can be stale by the time it lands: a
+        // save that arrived in between has to win, so a refresh applies only if
+        // this has not moved since it started.
+        revision: Arc<AtomicU64>,
         app: Option<Box<github::app::App>>,
     },
     Disabled,
@@ -109,6 +115,7 @@ impl Authorizer {
                     restricted: restricted.clone(),
                     allowed: allowed.clone(),
                 })),
+                revision: Arc::new(AtomicU64::new(0)),
                 app: github_app.as_ref().map(|configured| {
                     Box::new(github::app::App::load(
                         &configured.app_id,
@@ -127,10 +134,39 @@ impl Authorizer {
         }
     }
 
+    pub fn revision(&self) -> u64 {
+        match self {
+            Self::Forge { revision, .. } => revision.load(Ordering::SeqCst),
+            Self::Disabled => 0,
+        }
+    }
+
     pub fn set_access(&self, replacement: Access) {
-        if let Self::Forge { access, .. } = self {
-            *access.write().unwrap() = replacement;
+        if let Self::Forge {
+            access, revision, ..
+        } = self
+        {
+            let mut held = access.write().unwrap();
+            *held = replacement;
+            revision.fetch_add(1, Ordering::SeqCst);
+        }
+    }
+
+    pub fn refresh_access(&self, seen: u64, replacement: Access) -> bool {
+        let Self::Forge {
+            access, revision, ..
+        } = self
+        else {
+            return false;
+        };
+
+        let mut held = access.write().unwrap();
+        if revision.load(Ordering::SeqCst) != seen {
+            return false;
         }
+
+        *held = replacement;
+        true
     }
 
     pub(crate) async fn permission(

server/src/auth/tests.rs

@@ -146,3 +146,80 @@ fn an_unreachable_forge_is_never_remembered() {
         "caching an outage would turn a transient failure into a lasting denial"
     );
 }
+
+fn authorizer() -> Authorizer {
+    crate::tls::install_crypto_provider();
+
+    Authorizer::new(&Auth::Forge {
+        provider: Provider::Github,
+        api_url: "https://api.github.example".into(),
+        cache_ttl: Duration::from_secs(60),
+        rejection_ttl: Duration::from_secs(10),
+        lookup_budget: None,
+        github_app: None,
+        anonymous_read: false,
+        restricted: Namespaces::parse("LFSX_RESTRICTED", None),
+        allowed: None,
+    })
+}
+
+fn only(entries: &str) -> Access {
+    Access {
+        anonymous_read: false,
+        restricted: Namespaces::parse("LFSX_RESTRICTED", None),
+        allowed: Some(Namespaces::parse("LFSX_ALLOWED", Some(entries))),
+    }
+}
+
+#[test]
+fn a_refresh_that_nobody_overtook_is_applied() {
+    let authorizer = authorizer();
+    let seen = authorizer.revision();
+
+    assert!(authorizer.refresh_access(seen, only("Partner/*")));
+    assert_eq!(authorizer.access(), Some(only("Partner/*")));
+}
+
+#[test]
+fn a_save_that_lands_while_a_refresh_is_reading_is_not_overwritten_by_it() {
+    let authorizer = authorizer();
+    let seen = authorizer.revision();
+
+    authorizer.set_access(only("Saved/*"));
+    let applied = authorizer.refresh_access(seen, only("Stale/*"));
+
+    assert!(!applied, "what the refresh read predates the save");
+    assert_eq!(authorizer.access(), Some(only("Saved/*")));
+}
+
+#[test]
+fn applying_a_refresh_is_not_a_change_somebody_asked_for() {
+    let authorizer = authorizer();
+    let seen = authorizer.revision();
+    authorizer.refresh_access(seen, only("Partner/*"));
+
+    assert_eq!(
+        authorizer.revision(),
+        seen,
+        "a refresh that moved the counter would make the next one give up for nothing"
+    );
+}
+
+#[test]
+fn every_explicit_change_moves_the_revision() {
+    let authorizer = authorizer();
+    let before = authorizer.revision();
+
+    authorizer.set_access(only("A/*"));
+    authorizer.set_access(only("A/*"));
+
+    assert_eq!(authorizer.revision(), before + 2);
+}
+
+#[test]
+fn with_authentication_disabled_there_is_nothing_to_refresh() {
+    let disabled = Authorizer::new(&Auth::Disabled);
+
+    assert!(!disabled.refresh_access(disabled.revision(), only("A/*")));
+    assert_eq!(disabled.access(), None);
+}

server/src/console/access.rs

@@ -139,6 +139,7 @@ async fn refresh_one(state: &Shared, target: &Target<'_>) {
         return;
     };
 
+    let seen = target.authorizer.revision();
     let access = match saved(state, &target.file).await {
         Ok(None) => environment,
         Ok(Some(saved)) => match saved.access() {
@@ -162,8 +163,13 @@ async fn refresh_one(state: &Shared, target: &Target<'_>) {
         }
     };
 
-    if target.authorizer.access().as_ref() != Some(&access) {
-        target.authorizer.set_access(access);
+    if target.authorizer.access().as_ref() != Some(&access)
+        && !target.authorizer.refresh_access(seen, access)
+    {
+        tracing::debug!(
+            file = %target.file,
+            "access changed while it was being read back, so the change in force is kept"
+        );
     }
 }