fix(cli): address review findings on the update config surface

The unrecognized-mode warning was printed inside resolve(), before the
precedence rules it sits behind had been applied, so it escaped onto stderr
during --format json, in CI, with a piped stderr and under --quiet. It is now
held on the policy and emitted at the one point where the run is known to be
speaking at all.

The notify interval is keyed to the announced version rather than to time
alone. Time alone swallowed the NEXT release whenever it landed inside the
window, so a week-long interval set to stop nagging about one version also hid
the version that fixed it -- the opposite of what the setting says it does.

The interval comparison is unsigned. Duration::as_secs() as i64 can go
negative for a large enough configured value, and a negative interval compares
as already-elapsed, which would notify every run.

Cache writes survive a second perry process. Each write now uses its own
temporary file instead of one shared name that two writers rename over each
other, leaving a file the winner is still writing. The read-modify-write pairs
are serialized by a lock file, and a refresh re-reads the notice state inside
that lock rather than using a value read before its request went out -- which
would otherwise drop a notice recorded while the request was in flight and
tell the user twice.

Author: jdalton

changelog.d/7749-update-config-surface.md

@@ -99,3 +99,18 @@ erasure regression itself. Verified by sabotage — marking the new field
 `#[serde(skip)]` turns the erasure test red.
 
 `cargo test -p perry`: 902 passed, 0 failed.
+
+**Review follow-ups.** The unrecognized-mode warning is now held on the policy
+and printed only once the run is known to be speaking, so it can no longer
+appear during `--format json`, in CI, with a piped stderr, or under `--quiet` —
+the precedence rules exist to keep those runs silent and the warning was
+escaping them. The notify interval is keyed to the announced VERSION, not only
+to time: throttling on time alone swallowed the next release whenever it landed
+inside the window, so a long interval set to stop nagging about one version also
+hid the version that fixed it. The interval comparison is unsigned, because
+`Duration::as_secs() as i64` can go negative and a negative interval reads as
+already-elapsed. And the cache write is safe against a second `perry`: each
+write uses its own temporary file rather than one shared name that two writers
+would rename over each other, and the read-modify-write pairs are serialized by
+a lock file, with a refresh re-reading the notice state inside the lock instead
+of using a value read before its request went out.

crates/perry/src/main.rs

@@ -542,6 +542,11 @@ 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()
@@ -558,10 +563,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 +579,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,38 @@ 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()?;
+    lock.lock().ok()?;
+    Some(lock)
+}
+
 /// 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);
 }
 
@@ -347,16 +385,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);
@@ -1553,6 +1597,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();

crates/perry/src/update_policy.rs

@@ -124,6 +124,15 @@ pub(crate) struct UpdatePolicy {
     pub(crate) check_interval: Duration,
     pub(crate) notify_interval: Duration,
     pub(crate) prompt_default: bool,
+    /// A complaint about the config, to print only if this run is going to
+    /// speak at all.
+    ///
+    /// Emitting it from `resolve` would write to stderr before the precedence
+    /// rules below have been applied — so `--format json`, `CI`, a piped stderr
+    /// or `--quiet` would each get a stray line in the middle of output nobody
+    /// asked to be interrupted. The whole point of those rules is that this run
+    /// stays silent.
+    pub(crate) config_warning: Option<&'static str>,
 }
 
 /// The environment inputs, gathered in one place so the decision itself is a
@@ -219,20 +228,21 @@ impl UpdatePolicy {
         let config = crate::commands::publish::load_config()
             .update
             .unwrap_or_default();
-        if matches!(config.mode, Some(UpdateMode::Unknown)) {
-            // One line, once, on the way past. Loud enough to fix, quiet
-            // enough not to be the thing the user remembers about the run.
-            eprintln!(
-                "warning: unrecognized `[update] mode` in ~/.perry/config.toml; \
-                 using \"notify\". Valid values: off, notify, prompt, auto."
-            );
-        }
+        // Held, not printed. Emitting here would write to stderr before the
+        // precedence rules above have been applied, so a `--format json` run,
+        // a CI job, a piped stderr or `--quiet` would each get a stray line in
+        // the middle of output nobody asked to have interrupted.
+        let config_warning = matches!(config.mode, Some(UpdateMode::Unknown)).then_some(
+            "warning: unrecognized `[update] mode` in ~/.perry/config.toml; using \
+             \"notify\". Valid values: off, notify, prompt, auto.",
+        );
 
         Self {
             mode: resolve_mode(env, config.mode),
             check_interval: config.check_interval(),
             notify_interval: config.notify_interval(),
             prompt_default: config.prompt_default.unwrap_or(false),
+            config_warning,
         }
     }
 
@@ -269,11 +279,21 @@ fn structured_output_selected() -> bool {
 pub(crate) fn should_notify(
     notify_interval: Duration,
     last_notification: Option<&str>,
+    last_notified_version: Option<&str>,
+    latest: &str,
     now_rfc3339: &str,
 ) -> bool {
     if notify_interval.is_zero() {
         return true;
     }
+    // The interval throttles repeats of the SAME update, which is what it has
+    // always said it does. Keying it on time alone silently swallowed the next
+    // release whenever it arrived inside the window — so a user who set a
+    // week-long interval to stop being nagged about one version would also miss
+    // the one that fixed it.
+    if last_notified_version != Some(latest) {
+        return true;
+    }
     let (Some(last), Some(now)) = (
         crate::update_checker::parse_rfc3339(last_notification.unwrap_or("")),
         crate::update_checker::parse_rfc3339(now_rfc3339),
@@ -283,7 +303,7 @@ pub(crate) fn should_notify(
         // cache would hide updates indefinitely.
         return true;
     };
-    now.saturating_sub(last) >= notify_interval.as_secs() as i64
+    now.saturating_sub(last).max(0) as u64 >= notify_interval.as_secs()
 }
 
 #[cfg(test)]
@@ -454,10 +474,20 @@ mod tests {
         );
     }
 
+    /// The interval alone, with the announced version held constant — which is
+    /// what these cases were written to exercise.
+    fn notified_before(interval: Duration, last_notification: Option<&str>, now: &str) -> bool {
+        should_notify(interval, last_notification, Some("1.0.0"), "1.0.0", now)
+    }
+
     #[test]
     fn the_notify_throttle_defaults_to_every_run() {
-        assert!(should_notify(Duration::ZERO, None, "2026-08-10T00:00:00Z"));
-        assert!(should_notify(
+        assert!(notified_before(
+            Duration::ZERO,
+            None,
+            "2026-08-10T00:00:00Z"
+        ));
+        assert!(notified_before(
             Duration::ZERO,
             Some("2026-08-10T00:00:00Z"),
             "2026-08-10T00:00:01Z"
@@ -468,27 +498,79 @@ mod tests {
     fn the_notify_throttle_honours_its_interval() {
         let day = Duration::from_secs(24 * 3600);
         assert!(
-            !should_notify(day, Some("2026-08-10T00:00:00Z"), "2026-08-10T01:00:00Z"),
+            !notified_before(day, Some("2026-08-10T00:00:00Z"), "2026-08-10T01:00:00Z"),
             "an hour into a one-day throttle must stay quiet"
         );
         assert!(
-            should_notify(day, Some("2026-08-09T00:00:00Z"), "2026-08-10T01:00:00Z"),
+            notified_before(day, Some("2026-08-09T00:00:00Z"), "2026-08-10T01:00:00Z"),
             "past the interval it must speak up"
         );
     }
 
+    /// ★ The interval throttles repeats of the SAME update, which is what it
+    /// always claimed. Keyed on time alone it swallowed the NEXT release
+    /// whenever that landed inside the window — so a week-long interval set to
+    /// stop nagging about one version would also hide the version that fixed
+    /// it.
+    #[test]
+    fn a_different_version_is_announced_regardless_of_the_interval() {
+        let week = Duration::from_secs(7 * 24 * 3600);
+        // One minute into a week-long throttle: the same version stays quiet...
+        assert!(!should_notify(
+            week,
+            Some("2026-08-10T00:00:00Z"),
+            Some("1.0.0"),
+            "1.0.0",
+            "2026-08-10T00:01:00Z"
+        ));
+        // ...and a different one is announced anyway.
+        assert!(should_notify(
+            week,
+            Some("2026-08-10T00:00:00Z"),
+            Some("1.0.0"),
+            "1.0.1",
+            "2026-08-10T00:01:00Z"
+        ));
+        // Never having announced anything is also "not this version".
+        assert!(should_notify(
+            week,
+            Some("2026-08-10T00:00:00Z"),
+            None,
+            "1.0.0",
+            "2026-08-10T00:01:00Z"
+        ));
+    }
+
+    /// An enormous configured interval must still suppress, not wrap around
+    /// into announcing every run. `as i64` on a `Duration`'s seconds can go
+    /// negative, and a negative interval compares as "already elapsed".
+    #[test]
+    fn an_enormous_interval_still_suppresses() {
+        let absurd = Duration::from_secs(u64::MAX);
+        assert!(
+            !should_notify(
+                absurd,
+                Some("2026-08-10T00:00:00Z"),
+                Some("1.0.0"),
+                "1.0.0",
+                "2026-08-10T00:01:00Z"
+            ),
+            "a signed conversion here would read as already-elapsed and notify"
+        );
+    }
+
     /// A cache this build cannot read must not silence the notice forever —
     /// that would turn one bad write into a permanently muted checker.
     #[test]
     fn an_unreadable_timestamp_notifies_rather_than_staying_silent() {
         let day = Duration::from_secs(24 * 3600);
-        assert!(should_notify(day, None, "2026-08-10T00:00:00Z"));
-        assert!(should_notify(
+        assert!(notified_before(day, None, "2026-08-10T00:00:00Z"));
+        assert!(notified_before(
             day,
             Some("not-a-date"),
             "2026-08-10T00:00:00Z"
         ));
-        assert!(should_notify(
+        assert!(notified_before(
             day,
             Some("2026-08-10T00:00:00Z"),
             "also-not-a-date"