diff --git a/changelog.d/7787-update-surface-followups.md b/changelog.d/7787-update-surface-followups.md
new file mode 100644
index 0000000000..0875a51024
--- /dev/null
+++ b/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.
+
+
+Tests
+
+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.
+
diff --git a/crates/perry/src/commands/update.rs b/crates/perry/src/commands/update.rs
index 45c41070b8..787d000da3 100644
--- a/crates/perry/src/commands/update.rs
+++ b/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 => {}
}
diff --git a/crates/perry/src/main.rs b/crates/perry/src/main.rs
index 36105e894e..afe58680a2 100644
--- a/crates/perry/src/main.rs
+++ b/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);
}
}
}
diff --git a/crates/perry/src/update_checker.rs b/crates/perry/src/update_checker.rs
index 43ab5efb84..d216fd87d0 100644
--- a/crates/perry/src/update_checker.rs
+++ b/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,
+ /// 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,
}
#[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 {
+ 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 {
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 {
));
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();
diff --git a/crates/perry/src/update_policy.rs b/crates/perry/src/update_policy.rs
index 261e801bae..52cb32b29f 100644
--- a/crates/perry/src/update_policy.rs
+++ b/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"