Skip to content

Commit ce7f3ef

Browse files
fix(backend): key the remote version cache by listing tool options
`ls-remote` returned a stale list when a tool option that reshapes the listing changed between invocations. `github:Azure/azure-cli` and `github:Azure/azure-cli[version_prefix=azure-cli-]` both resolve to `<cache>/github-azure-azure-cli/remote_versions-e1b39.msgpack.z`, so whichever ran first decided the answer for the other until the cache was cleared -- in both directions. The cached value is genuinely option-dependent: `_list_remote_versions` filters tags by `version_prefix` and strips it before storing them, and `api_url` decides which host answered. But the key is not: it is built from the tool's cache path plus mise's own version/os/arch, and only `remote_version_cache_context` can add to it, which the github backend does not implement. Backends already declare which option keys reshape their listing, through `remote_version_listing_tool_option_keys`. Digest the values of those keys into the cache context so the entries are partitioned, combining with any context the backend supplies of its own. The digest is produced only when the values come from a local source -- config, backend alias, inline arg, install manifest -- which is already the exact condition under which the versions host is skipped, so the host decision is unchanged: every case that newly gets a context was already short-circuited to `false` one branch further down. Those two branches are swapped so each trace message still names its real cause; both evaluate to the same `false`. A registry-supplied value is identical for every user, so it deliberately produces no context and the shared host stays available for the default case. This is the shared listing path, so github, gitlab, forgejo, ubi, spm, http and s3 are all covered. conda and java override the hook outright and were never affected -- confirmed by measurement for conda. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 93a5786 commit ce7f3ef

1 file changed

Lines changed: 206 additions & 5 deletions

File tree

src/backend/mod.rs

Lines changed: 206 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,25 @@ fn has_local_version_listing_option_override(
219219
resolved_opts
220220
.has_any_key_from_sources(version_listing_opt_keys, VERSIONS_HOST_LOCAL_OPT_SOURCES)
221221
}
222+
223+
/// Digest of the listing-relevant tool options, used to partition the remote-version cache.
224+
///
225+
/// The cached list is *shaped* by these options — `version_prefix` decides which tags survive
226+
/// and how they are spelled, `api_url` decides which host answered — so two different sets of
227+
/// values must not share a cache entry. Collected into a `BTreeMap` so the digest depends on
228+
/// the values rather than on the order the options were inserted, the same way asdf's
229+
/// `version_listing_cache_context` does.
230+
///
231+
/// `get_string` rather than `get`: the latter yields `None` for any non-string TOML scalar,
232+
/// which would quietly collapse two distinct values into one key.
233+
fn listing_option_digest(opts: &ToolVersionOptions, version_listing_opt_keys: &[&str]) -> String {
234+
let values: BTreeMap<&str, String> = version_listing_opt_keys
235+
.iter()
236+
.filter_map(|key| opts.get_string(key).map(|value| (*key, value)))
237+
.collect();
238+
hash::hash_to_str(&values)
239+
}
240+
222241
/// Remaps a backend-discovered path from the concrete install dir to the
223242
/// runtime path users put on PATH.
224243
///
@@ -1188,6 +1207,76 @@ mod tests {
11881207
));
11891208
}
11901209

1210+
/// The digest keys a cache, so it has to depend on the values and not on the order the
1211+
/// options happened to be inserted in — `opts` is insertion-ordered.
1212+
#[test]
1213+
fn test_listing_option_digest_is_stable_and_order_independent() {
1214+
use crate::toolset::ToolVersionOptions;
1215+
1216+
let keys = &["api_url", "version_prefix"];
1217+
let api_url = || toml::Value::String("https://github.example.com/api/v3".into());
1218+
let prefix = || toml::Value::String("release-".into());
1219+
1220+
let mut forward = ToolVersionOptions::default();
1221+
forward.opts.insert("api_url".to_string(), api_url());
1222+
forward.opts.insert("version_prefix".to_string(), prefix());
1223+
1224+
let mut reverse = ToolVersionOptions::default();
1225+
reverse.opts.insert("version_prefix".to_string(), prefix());
1226+
reverse.opts.insert("api_url".to_string(), api_url());
1227+
1228+
assert_eq!(
1229+
listing_option_digest(&forward, keys),
1230+
listing_option_digest(&reverse, keys),
1231+
);
1232+
assert_eq!(
1233+
listing_option_digest(&forward, keys),
1234+
listing_option_digest(&forward, keys),
1235+
);
1236+
}
1237+
1238+
/// Every distinction the version listing depends on has to reach the digest, and nothing
1239+
/// else may: an install-time option that cannot change the list must not split the cache.
1240+
#[test]
1241+
fn test_listing_option_digest_tracks_declared_keys_only() {
1242+
use crate::toolset::ToolVersionOptions;
1243+
1244+
let keys = &["api_url", "version_prefix"];
1245+
let empty = ToolVersionOptions::default();
1246+
1247+
let mut prefix_a = ToolVersionOptions::default();
1248+
prefix_a.opts.insert(
1249+
"version_prefix".to_string(),
1250+
toml::Value::String("a-".into()),
1251+
);
1252+
1253+
let mut prefix_b = ToolVersionOptions::default();
1254+
prefix_b.opts.insert(
1255+
"version_prefix".to_string(),
1256+
toml::Value::String("b-".into()),
1257+
);
1258+
1259+
// The defect this guards: two prefixes that produce different listings sharing an entry.
1260+
assert_ne!(
1261+
listing_option_digest(&prefix_a, keys),
1262+
listing_option_digest(&prefix_b, keys),
1263+
);
1264+
assert_ne!(
1265+
listing_option_digest(&empty, keys),
1266+
listing_option_digest(&prefix_a, keys),
1267+
);
1268+
1269+
let mut with_install_opt = prefix_a.clone();
1270+
with_install_opt.opts.insert(
1271+
"asset_pattern".to_string(),
1272+
toml::Value::String("tool-{{version}}.tar.gz".into()),
1273+
);
1274+
assert_eq!(
1275+
listing_option_digest(&prefix_a, keys),
1276+
listing_option_digest(&with_install_opt, keys),
1277+
);
1278+
}
1279+
11911280
#[test]
11921281
fn test_backend_arg_matches_registry_backend_ignores_inline_opts() {
11931282
let ba = BackendArg::new(
@@ -2028,7 +2117,23 @@ pub trait Backend: Debug + Send + Sync {
20282117
refresh: bool,
20292118
has_local_version_listing_override: bool,
20302119
) -> eyre::Result<Vec<VersionInfo>> {
2031-
let cache_context = self.remote_version_cache_context(config).await?;
2120+
// The listing-relevant options shape the cached list, so they belong in its key.
2121+
// Only local overrides count: a registry-supplied value is identical for everyone, so
2122+
// one entry is correct for it, and leaving it out keeps the shared versions host
2123+
// available for the default case.
2124+
let opt_context = has_local_version_listing_override.then(|| {
2125+
listing_option_digest(listing_opts, self.remote_version_listing_tool_option_keys())
2126+
});
2127+
let cache_context = match (
2128+
self.remote_version_cache_context(config).await?,
2129+
opt_context,
2130+
) {
2131+
(Some(backend_context), Some(opt_context)) => {
2132+
Some(hash::hash_to_str(&(backend_context, opt_context)))
2133+
}
2134+
(Some(context), None) | (None, Some(context)) => Some(context),
2135+
(None, None) => None,
2136+
};
20322137
let remote_versions = match cache_context.as_deref() {
20332138
Some(context) => self.get_remote_version_cache_with_context(Some(context)),
20342139
None => self.get_remote_version_cache(),
@@ -2072,15 +2177,17 @@ pub trait Backend: Debug + Send + Sync {
20722177
ba.short, backend_type
20732178
);
20742179
false
2075-
} else if cache_context.is_some() {
2180+
} else if has_local_version_listing_override {
2181+
// Checked before the context: an option override now also produces a cache
2182+
// context, and this is the message that names the actual cause.
20762183
trace!(
2077-
"Skipping versions host for {} because local context affects remote version listing",
2184+
"Skipping versions host for {} because local backend opts affect remote version listing",
20782185
ba.short,
20792186
);
20802187
false
2081-
} else if has_local_version_listing_override {
2188+
} else if cache_context.is_some() {
20822189
trace!(
2083-
"Skipping versions host for {} because local backend opts affect remote version listing",
2190+
"Skipping versions host for {} because local context affects remote version listing",
20842191
ba.short,
20852192
);
20862193
false
@@ -4105,6 +4212,7 @@ mod latest_version_tests {
41054212
stable_result: Option<String>,
41064213
stable_info: Option<VersionInfo>,
41074214
remote_versions: Vec<VersionInfo>,
4215+
listing_keys: &'static [&'static str],
41084216
stable_calls: AtomicUsize,
41094217
stable_info_calls: AtomicUsize,
41104218
list_calls: AtomicUsize,
@@ -4128,6 +4236,7 @@ mod latest_version_tests {
41284236
..Default::default()
41294237
},
41304238
],
4239+
listing_keys: &[],
41314240
stable_calls: AtomicUsize::new(0),
41324241
stable_info_calls: AtomicUsize::new(0),
41334242
list_calls: AtomicUsize::new(0),
@@ -4149,6 +4258,13 @@ mod latest_version_tests {
41494258
self
41504259
}
41514260

4261+
/// Declare tool options that shape this backend's version listing, the way the real
4262+
/// github/spm/ubi/http/s3 backends do.
4263+
fn with_listing_keys(mut self, listing_keys: &'static [&'static str]) -> Self {
4264+
self.listing_keys = listing_keys;
4265+
self
4266+
}
4267+
41524268
fn stable_calls(&self) -> usize {
41534269
self.stable_calls.load(Ordering::SeqCst)
41544270
}
@@ -4172,6 +4288,10 @@ mod latest_version_tests {
41724288
&self.ba
41734289
}
41744290

4291+
fn remote_version_listing_tool_option_keys(&self) -> &'static [&'static str] {
4292+
self.listing_keys
4293+
}
4294+
41754295
async fn _list_remote_versions(
41764296
&self,
41774297
_config: &Arc<Config>,
@@ -4624,6 +4744,86 @@ mod latest_version_tests {
46244744
);
46254745
}
46264746

4747+
/// The regression this fixes: two option values that produce different listings shared one
4748+
/// cache entry, so whichever ran first answered for both. `short` is the same for the two
4749+
/// (inline opts are stripped from it), so they do share a cache *directory* — only the key
4750+
/// keeps them apart.
4751+
#[tokio::test]
4752+
async fn test_remote_versions_cache_is_partitioned_by_listing_options() {
4753+
let config = Config::get().await.unwrap();
4754+
let version = |v: &str| VersionInfo {
4755+
version: v.to_string(),
4756+
..Default::default()
4757+
};
4758+
4759+
let alpha = LatestBackend::new("test-listing-opts-partition[version_prefix=a-]")
4760+
.with_listing_keys(&["version_prefix"])
4761+
.with_remote_versions(vec![version("1.0.0")]);
4762+
let beta = LatestBackend::new("test-listing-opts-partition[version_prefix=b-]")
4763+
.with_listing_keys(&["version_prefix"])
4764+
.with_remote_versions(vec![version("2.0.0")]);
4765+
// Same value as `alpha`, different canned list: it must never be asked for it.
4766+
let alpha_again = LatestBackend::new("test-listing-opts-partition[version_prefix=a-]")
4767+
.with_listing_keys(&["version_prefix"])
4768+
.with_remote_versions(vec![version("3.0.0")]);
4769+
assert_eq!(alpha.ba().cache_path, beta.ba().cache_path);
4770+
let _ = fs::remove_dir_all(&alpha.ba().cache_path);
4771+
4772+
assert_eq!(
4773+
alpha.list_remote_versions(&config).await.unwrap(),
4774+
vec!["1.0.0".to_string()]
4775+
);
4776+
// The defect: this read back the list `alpha` had cached under the shared key.
4777+
assert_eq!(
4778+
beta.list_remote_versions(&config).await.unwrap(),
4779+
vec!["2.0.0".to_string()]
4780+
);
4781+
// Same option value, same entry — which is what shows the key follows the value rather
4782+
// than the instance, and that the fix did not trade staleness for a refetch every time.
4783+
assert_eq!(
4784+
alpha_again.list_remote_versions(&config).await.unwrap(),
4785+
vec!["1.0.0".to_string()]
4786+
);
4787+
assert_eq!(alpha_again.list_calls(), 0);
4788+
}
4789+
4790+
/// The other half of it: declaring listing options must not partition anything on its own.
4791+
/// With no local override there is no context, so the list has to land on the contextless
4792+
/// entry — that is the state in which the shared versions host stays available, and a
4793+
/// context here would take it away from every default installation of the tool.
4794+
#[tokio::test]
4795+
async fn test_declared_listing_keys_without_override_use_the_default_cache_entry() {
4796+
let config = Config::get().await.unwrap();
4797+
let backend = LatestBackend::new("test-listing-opts-shared")
4798+
.with_listing_keys(&["api_url", "version_prefix"])
4799+
.with_remote_versions(vec![VersionInfo {
4800+
version: "1.0.0".to_string(),
4801+
..Default::default()
4802+
}]);
4803+
backend
4804+
.get_remote_version_cache()
4805+
.lock()
4806+
.await
4807+
.clear()
4808+
.unwrap();
4809+
4810+
assert_eq!(
4811+
backend.list_remote_versions(&config).await.unwrap(),
4812+
vec!["1.0.0".to_string()]
4813+
);
4814+
4815+
// `get_remote_version_cache()` is the `context: None` handle. Had a context been
4816+
// produced, the list would have been written somewhere else and this would be empty.
4817+
let cached = backend
4818+
.get_remote_version_cache()
4819+
.lock()
4820+
.await
4821+
.get_cached()
4822+
.unwrap();
4823+
assert_eq!(cached.len(), 1);
4824+
assert_eq!(cached[0].version, "1.0.0");
4825+
}
4826+
46274827
#[tokio::test]
46284828
async fn test_offline_latest_uses_fast_path_when_available() {
46294829
let config = Config::get().await.unwrap();
@@ -4721,6 +4921,7 @@ mod latest_version_tests {
47214921
stable_result: Some("9.9.9".to_string()),
47224922
stable_info: None,
47234923
remote_versions: vec![],
4924+
listing_keys: &[],
47244925
stable_calls: AtomicUsize::new(0),
47254926
stable_info_calls: AtomicUsize::new(0),
47264927
list_calls: AtomicUsize::new(0),

0 commit comments

Comments
 (0)