diff --git a/backend/windmill-store/src/resources.rs b/backend/windmill-store/src/resources.rs index f68c5f0c7e..e50154e7bb 100644 --- a/backend/windmill-store/src/resources.rs +++ b/backend/windmill-store/src/resources.rs @@ -2613,13 +2613,19 @@ struct HubCached { value: T, } -/// Ids only change when a resource type is published to the hub; picks move slowly and -/// only reorder a list. Both are read on every drawer open, hence caching at all. -const HUB_RT_IDS_TTL: std::time::Duration = std::time::Duration::from_secs(60 * 60); +/// The index changes only when a resource type is published to the hub; picks move slowly +/// and only reorder a list. Both are read on every drawer open, hence caching at all. +const HUB_RT_INDEX_TTL: std::time::Duration = std::time::Duration::from_secs(60 * 60); const HUB_RT_PICKS_TTL: std::time::Duration = std::time::Duration::from_secs(5 * 60); -static HUB_RT_IDS: LazyLock< - std::sync::RwLock>>>, +/// How long a *failed* index read is remembered. Short, and deliberately not the hour a +/// success is good for: this read is on the path of every picker open, so an unreachable +/// hub must not cost an outbound timeout each time, while one blip must not silence pick +/// reporting for an hour. +const HUB_RT_INDEX_FAILURE_TTL: std::time::Duration = std::time::Duration::from_secs(60); + +static HUB_RT_INDEX: LazyLock< + std::sync::RwLock>>>>, > = LazyLock::new(|| std::sync::RwLock::new(None)); static HUB_RT_PICKS: LazyLock>>>> = LazyLock::new(|| std::sync::RwLock::new(None)); @@ -2649,7 +2655,10 @@ fn hub_cache_put(cache: &std::sync::RwLock>>, hub_base_ur struct HubResourceTypeEntry { id: i64, name: String, - app: String, + /// Optional so a hub that does not send it costs only the mapping. Required, it would + /// fail the whole parse and take pick reporting — which needs just the id — with it. + #[serde(default)] + app: Option, } #[derive(Clone)] @@ -2662,38 +2671,64 @@ struct HubResourceType { app: String, } +/// Reads the index cache, choosing the TTL by what is stored: a failure expires far sooner +/// than a success. `None` is a miss, `Some(None)` a remembered failure. +fn hub_index_cached(hub_base_url: &str) -> Option>> { + let guard = HUB_RT_INDEX.read().ok()?; + let entry = guard.as_ref()?; + if entry.hub_base_url != hub_base_url { + return None; + } + let ttl = if entry.value.is_some() { + HUB_RT_INDEX_TTL + } else { + HUB_RT_INDEX_FAILURE_TTL + }; + (entry.fetched_at.elapsed() < ttl).then(|| entry.value.clone()) +} + /// What the hub knows about every published resource type, keyed by the name Windmill /// addresses it by. `None` when the hub cannot be reached or does not answer with a list. async fn hub_resource_types( db: &DB, hub_base_url: &str, ) -> Option> { - if let Some(index) = hub_cache_get(&HUB_RT_IDS, hub_base_url, HUB_RT_IDS_TTL) { - return Some(index); + if let Some(cached) = hub_index_cached(hub_base_url) { + return cached; } - let response = windmill_common::utils::http_get_from_hub( - &windmill_common::utils::HTTP_CLIENT, - &format!("{hub_base_url}/resource_types/list"), - false, - None, - Some(db), - ) - .await - .ok()?; - if !response.status().is_success() { - return None; - } - // Only the id and the app are kept. That listing carries every type's schema — around - // a megabyte — and neither reporting a pick nor grouping types by integration needs it. - let index: HashMap = response - .json::>() + let index = async { + let response = windmill_common::utils::http_get_from_hub( + &windmill_common::utils::HTTP_CLIENT, + &format!("{hub_base_url}/resource_types/list"), + false, + None, + Some(db), + ) .await - .ok()? - .into_iter() - .map(|rt| (rt.name, HubResourceType { id: rt.id, app: rt.app })) - .collect(); - hub_cache_put(&HUB_RT_IDS, hub_base_url, index.clone()); - Some(index) + .ok()?; + if !response.status().is_success() { + return None; + } + // Only the id and the app are kept. That listing carries every type's schema — + // around a megabyte — and neither reporting a pick nor grouping types by + // integration needs it. + Some( + response + .json::>() + .await + .ok()? + .into_iter() + .map(|rt| { + let app = rt.app.unwrap_or_else(|| rt.name.clone()); + (rt.name, HubResourceType { id: rt.id, app }) + }) + .collect::>(), + ) + } + .await; + + hub_cache_put(&HUB_RT_INDEX, hub_base_url, index.clone()); + index } #[derive(Serialize)] @@ -2741,7 +2776,7 @@ async fn pick_hub_resource_type( Ok(Json(PickHubResourceTypeResult { success })) } -#[derive(Serialize, Deserialize, Clone)] +#[derive(Deserialize, Clone)] struct HubResourceTypePicks { name: String, /// The hub counts picks in a bigint, which its driver serialises as a string. @@ -2756,7 +2791,34 @@ struct HubPickedResourceTypes { #[cfg(test)] mod hub_picks_tests { - use super::HubPickedResourceTypes; + use super::*; + + /// Two properties of the index cache that a later edit could quietly drop: it is keyed on + /// the hub it came from, and a failure is forgotten long before a success is. + #[test] + fn the_index_cache_is_keyed_on_the_hub_and_forgets_failures_sooner() { + let index = || { + Some(HashMap::from([( + "slack".to_string(), + HubResourceType { id: 1, app: "slack".to_string() }, + )])) + }; + + hub_cache_put(&HUB_RT_INDEX, "https://hub.example", index()); + assert!(hub_index_cached("https://hub.example").is_some_and(|v| v.is_some())); + // Switching hubs must miss rather than serve the previous hub's mapping. + assert!(hub_index_cached("https://other.example").is_none()); + + // A remembered failure reads as a hit (so the hub is not re-attempted) carrying + // nothing, and only until the shorter of the two TTLs. + hub_cache_put(&HUB_RT_INDEX, "https://hub.example", None); + assert!(hub_index_cached("https://hub.example").is_some_and(|v| v.is_none())); + assert!(HUB_RT_INDEX_FAILURE_TTL < HUB_RT_INDEX_TTL); + + if let Ok(mut guard) = HUB_RT_INDEX.write() { + *guard = None; + } + } /// The hub counts picks in a bigint, which postgres.js serialises as a string. Typing /// the field as a plain i64 fails the whole response, and the ranking silently empties. @@ -2778,7 +2840,7 @@ mod hub_picks_tests { } /// One hub resource type as the pickers need it. -#[derive(Serialize, Clone)] +#[derive(Serialize)] struct HubResourceTypeInfo { name: String, /// The integration it belongs to, so a caller can total a workspace's resources per @@ -2830,22 +2892,30 @@ async fn list_hub_resource_type_info( } }; - let picks_by_name: HashMap = + let mut picks_by_name: HashMap = picks.into_iter().map(|rt| (rt.name, rt.picks)).collect(); let index = hub_resource_types(&db, &hub_base_url) .await .unwrap_or_default(); - Ok(Json( - index + let mut info: Vec = index + .into_iter() + .map(|(name, rt)| HubResourceTypeInfo { + picks: picks_by_name.remove(&name).unwrap_or(0), + name, + app: rt.app, + }) + .collect(); + // What the index did not account for is a type the picks read knows and the listing does + // not — which is what a hub answering only the second read looks like. Its own name is + // the same guess a caller makes for any type the mapping misses. + info.extend( + picks_by_name .into_iter() - .map(|(name, rt)| HubResourceTypeInfo { - picks: picks_by_name.get(&name).copied().unwrap_or(0), - name, - app: rt.app, - }) - .collect(), - )) + .map(|(name, picks)| HubResourceTypeInfo { app: name.clone(), name, picks }), + ); + + Ok(Json(info)) } async fn get_resource_type( diff --git a/frontend/src/lib/components/AppConnectInner.svelte b/frontend/src/lib/components/AppConnectInner.svelte index aa2d05ca1d..7f2b646f6b 100644 --- a/frontend/src/lib/components/AppConnectInner.svelte +++ b/frontend/src/lib/components/AppConnectInner.svelte @@ -1025,7 +1025,15 @@ (x) => x.key, (x) => resourceTypeDescriptions[x.key] ) - : [...items].sort((a, b) => popularity(a.key, b.key))) + : // Both signals are keyed by resource type, and a sandbox client is a second row + // against one (`salesforce_sandbox` saves a `salesforce`), so it ranks on the + // parent's popularity. Its own key still breaks the tie the pair then have, or + // the two would order arbitrarily. + [...items].sort( + (a, b) => + popularity(stripSandboxSuffix(a.key), stripSandboxSuffix(b.key)) || + a.key.localeCompare(b.key) + )) let rankedConnects = $derived(rank(filteredConnects)) let rankedConnectsManual = $derived( rank(filteredConnectsManual) as typeof filteredConnectsManual | undefined