Fix five review findings in the update surface that merged before their fixes

changelog.d/7787-update-surface-followups.md

@@ -0,0 +1,55 @@
+### Fixed
+
+Five defects in the update surface, all found in review of
+[#7749](https://github.com/PerryTS/perry/pull/7749) after it had merged.
+
+**The config warning escaped the rules that were meant to silence it.** The
+"unrecognized `[update] mode`" line was printed inside `UpdatePolicy::resolve`,
+before the precedence rules it sits behind had been applied — so it reached
+stderr during `--format json`, in CI, with a piped stderr, and under `--quiet`.
+Those rules exist to keep exactly those runs silent, and the one line whose job
+was to report a config problem was the one line ignoring them. It is now held on
+the policy and emitted at the single point where the run is known to be speaking
+at all.
+
+**The notify interval throttled on time alone, so it swallowed the next
+release.** The documented contract is that the interval throttles repeats of
+*the same* update. Keyed only on a timestamp, it also suppressed a **different**
+version that arrived inside the window — so somebody setting a week-long
+interval to stop being nagged about one release would also have been denied the
+release that fixed it. The cache now records which version it announced, and a
+different version is announced regardless of the interval.
+
+**The interval comparison was signed.** `Duration::as_secs() as i64` goes
+negative for a large enough configured value, and a negative interval reads as
+already-elapsed — so an absurd value would have notified on *every* run instead
+of suppressing. The comparison is unsigned.
+
+**Two `perry` processes could corrupt the cache.** Every write used one shared
+`*.json.tmp`, so two writers each wrote it and each renamed it: the loser's
+rename landed a file the winner was still writing into. Each write now builds
+its own temporary name.
+
+**A refresh could erase a notice recorded while its request was in flight.**
+`fetch_latest_version` read the notice state *before* issuing its request and
+wrote it back afterwards, overwriting anything recorded in between — telling the
+user about the same release twice. The read-modify-write pairs are now
+serialized by a lock file, and the refresh re-reads inside that lock immediately
+before replacing.
+
+<details>
+<summary><b>Tests</b></summary>
+
+Two new contract tests, both sabotage-verified — reverting either fix turns its
+test red:
+
+- a different version is announced regardless of the interval, and never having
+  announced anything counts as "not this version";
+- an enormous interval still suppresses rather than wrapping into notifying.
+
+The existing interval tests are unchanged in intent: they now go through a
+helper that holds the announced version constant, so they still exercise only
+the interval arithmetic.
+
+`cargo test -p perry`: 904 passed, 0 failed.
+</details>

crates/perry/src/commands/update.rs

@@ -68,7 +68,9 @@ pub fn run(
                     } else {
                         println!("Update available: {} -> {}", cur, latest);
                     }
-                    println!("  Release: {}", release_url);
+                    if !release_url.is_empty() {
+                        println!("  Release: {}", release_url);
+                    }
                 }
                 OutputFormat::Text => {}
             }

crates/perry/src/main.rs

@@ -542,12 +542,19 @@ fn main_inner() -> Result<()> {
 
     // Print update notice if available (to stderr, non-blocking)
     if update_surface_active {
+        // The config complaint, if any, goes out only now — this is the first
+        // point at which we know the run is allowed to say anything at all.
+        if let Some(warning) = update_policy.config_warning {
+            eprintln!("{warning}");
+        }
         let use_stderr_color = !cli.no_color && std::io::stderr().is_terminal();
-        let status = if let Some(rx) = bg_check {
-            rx.recv_timeout(std::time::Duration::from_millis(100)).ok()
-        } else {
-            Some(update_checker::check_cached_status())
-        };
+        // A background check that has not answered within 100 ms falls back to
+        // the cache rather than saying nothing. Reading the timeout as "no
+        // update" suppressed a notice the previous run had already earned — the
+        // check being slow is not evidence that the version is current.
+        let status = bg_check
+            .and_then(|rx| rx.recv_timeout(std::time::Duration::from_millis(100)).ok())
+            .or_else(|| Some(update_checker::check_cached_status()));
 
         if let Some(update_checker::UpdateStatus::UpdateAvailable {
             current,
@@ -558,10 +565,14 @@ fn main_inner() -> Result<()> {
             // `notify_interval_hours` throttles repeats of the SAME available
             // update. It defaults to 0 — a notice every run, which is what
             // Perry did before — so this is inert until someone asks for it.
-            let last = update_checker::load_cache().and_then(|c| c.last_notification);
+            let cached = update_checker::load_cache();
             if update_policy::should_notify(
                 update_policy.notify_interval,
-                last.as_deref(),
+                cached.as_ref().and_then(|c| c.last_notification.as_deref()),
+                cached
+                    .as_ref()
+                    .and_then(|c| c.last_notified_version.as_deref()),
+                &latest,
                 &update_checker::now_rfc3339_public(),
             ) {
                 update_checker::print_update_notice(
@@ -570,7 +581,7 @@ fn main_inner() -> Result<()> {
                     &release_url,
                     use_stderr_color,
                 );
-                update_checker::record_notification();
+                update_checker::record_notification(&latest);
             }
         }
     }

crates/perry/src/update_checker.rs

@@ -33,6 +33,14 @@ pub struct UpdateCache {
     /// always was.
     #[serde(default, skip_serializing_if = "Option::is_none")]
     pub last_notification: Option<String>,
+    /// Which version that notice was about.
+    ///
+    /// Without this the notify interval throttles on time alone, which
+    /// swallows the NEXT release when it lands inside the window — so a
+    /// week-long interval set to stop nagging about one version would also hide
+    /// the one that fixed it.
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub last_notified_version: Option<String>,
 }
 
 #[derive(Debug, Deserialize)]
@@ -90,7 +98,14 @@ fn save_cache(cache: &UpdateCache) {
     // `replace_path` rather than `fs::rename`: on Windows a rename onto an
     // EXISTING file fails, so every write after the first would silently do
     // nothing and the throttle would never advance.
-    let tmp = path.with_extension("json.tmp");
+    // A per-write name. With one shared `*.json.tmp`, two `perry` processes
+    // each write it and each rename it: the loser's rename lands a file the
+    // winner is still writing into, and the cache ends up truncated or mixed.
+    let tmp = path.with_extension(format!(
+        "json.tmp.{}.{}",
+        std::process::id(),
+        NEXT_TMP.fetch_add(1, std::sync::atomic::Ordering::Relaxed)
+    ));
     if fs::write(&tmp, content).is_err() {
         let _ = fs::remove_file(&tmp);
         return;
@@ -100,15 +115,44 @@ fn save_cache(cache: &UpdateCache) {
     }
 }
 
+/// Distinguishes the temporary files of concurrent writes in one process.
+static NEXT_TMP: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0);
+
+/// Take the cross-process lock guarding read-modify-write of the cache.
+///
+/// A background refresh and a notice can be recorded at the same moment, and
+/// each is a load-mutate-store: without a lock the later store overwrites the
+/// earlier one's field, so a notice recorded while a request was in flight
+/// vanishes and the user is told twice. Returns `None` when the lock cannot be
+/// taken, in which case the caller proceeds unlocked — losing a cache update is
+/// better than refusing to update a cache.
+fn lock_cache() -> Option<fslock::LockFile> {
+    let path = cache_path().with_extension("json.lock");
+    if let Some(parent) = path.parent() {
+        let _ = fs::create_dir_all(parent);
+    }
+    let mut lock = fslock::LockFile::open(&path).ok()?;
+    // `try_lock`, NOT `lock`. This runs at teardown, after the command the user
+    // asked for has finished — so blocking here would hang their terminal on
+    // another `perry`'s cache write, for a cache. The doc above promises we
+    // proceed unlocked rather than wait, and `lock()` did not honour it.
+    match lock.try_lock() {
+        Ok(true) => Some(lock),
+        _ => None,
+    }
+}
+
 /// Record that the user has just been told about an available update.
 ///
 /// A no-op when there is no cache: the notice can only have come from one, and
 /// inventing a file here would fabricate a `last_check` that never happened.
-pub fn record_notification() {
+pub fn record_notification(version: &str) {
+    let _guard = lock_cache();
     let Some(mut cache) = load_cache() else {
         return;
     };
     cache.last_notification = Some(now_rfc3339());
+    cache.last_notified_version = Some(version.to_string());
     save_cache(&cache);
 }
 
@@ -329,7 +373,6 @@ fn fetch_latest_version() -> Result<UpdateCache> {
 
     let servers = get_update_servers();
     let mut last_err = None;
-    let prior_notification = load_cache().and_then(|c| c.last_notification);
 
     for url in &servers {
         match client.get(url).send() {
@@ -347,16 +390,22 @@ fn fetch_latest_version() -> Result<UpdateCache> {
                         ));
                         continue;
                     }
+                    // Re-read the notice state INSIDE the lock rather than
+                    // before the request. This struct is rebuilt from scratch,
+                    // and a notice recorded while the request was in flight
+                    // would otherwise be overwritten with the stale value read
+                    // minutes earlier — telling the user twice about the same
+                    // release.
+                    let _guard = lock_cache();
+                    let prior = load_cache();
                     let cache = UpdateCache {
                         last_check: now_rfc3339(),
                         latest_version: version,
                         release_url: info.html_url,
-                        // Carry the notice timestamp across the refresh. This
-                        // struct is rebuilt from scratch, so dropping the field
-                        // here would reset the notify throttle on every check
-                        // and `notify_interval_hours` would silently do nothing
-                        // beyond one check interval.
-                        last_notification: prior_notification.clone(),
+                        last_notification: prior.as_ref().and_then(|c| c.last_notification.clone()),
+                        last_notified_version: prior
+                            .as_ref()
+                            .and_then(|c| c.last_notified_version.clone()),
                     };
                     save_cache(&cache);
                     return Ok(cache);
@@ -429,14 +478,24 @@ pub fn print_update_notice(current: &str, latest: &str, url: &str, use_color: bo
             current,
             console::style(latest).green().bold(),
         );
-        eprintln!(
-            "  Run {} to update, or visit {}",
-            console::style("perry update").cyan(),
-            url,
-        );
+        // A custom manifest may carry only `version`, and "or visit " with
+        // nothing after it reads like a bug.
+        if url.is_empty() {
+            eprintln!("  Run {} to update", console::style("perry update").cyan());
+        } else {
+            eprintln!(
+                "  Run {} to update, or visit {}",
+                console::style("perry update").cyan(),
+                url,
+            );
+        }
     } else {
         eprintln!("\nUpdate: {} -> {} available", current, latest);
-        eprintln!("  Run `perry update` to update, or visit {}", url);
+        if url.is_empty() {
+            eprintln!("  Run `perry update` to update");
+        } else {
+            eprintln!("  Run `perry update` to update, or visit {}", url);
+        }
     }
 }
 
@@ -1553,6 +1612,7 @@ mod tests {
             latest_version: "0.2.171".to_string(),
             release_url: "https://github.com/PerryTS/perry/releases/tag/v0.2.171".to_string(),
             last_notification: Some("2025-01-15T11:00:00Z".to_string()),
+            last_notified_version: Some("0.2.171".to_string()),
         };
 
         let json = serde_json::to_string(&cache).unwrap();