From d69a0727019fa512ee686993ab7344168d44e006 Mon Sep 17 00:00:00 2001
From: NathanFlurry <git@nathanflurry.com>
Date: Wed, 19 Jun 2024 22:32:42 +0000
Subject: [PATCH] fix(cache): mixed values in Cache::fetch_all (#927)

<!-- Please make sure there is an issue that this PR is correlated to. -->

## Changes

<!-- If there are frontend changes, please include screenshots. -->
---
 lib/cache/build/src/getter_ctx.rs            |  5 ++
 lib/cache/build/src/req_config.rs            | 38 +++++++-----
 svc/pkg/mm-config/ops/version-get/src/lib.rs | 65 +++++---------------
 3 files changed, 40 insertions(+), 68 deletions(-)

diff --git a/lib/cache/build/src/getter_ctx.rs b/lib/cache/build/src/getter_ctx.rs
index fb1671a04..a1d2e413e 100644
--- a/lib/cache/build/src/getter_ctx.rs
+++ b/lib/cache/build/src/getter_ctx.rs
@@ -83,6 +83,11 @@ where
 			.collect()
 	}
 
+	/// All keys.
+	pub(super) fn keys(&self) -> &[GetterCtxKey<K, V>] {
+		&self.keys[..]
+	}
+
 	/// If all keys have an associated value.
 	pub(super) fn all_keys_have_value(&self) -> bool {
 		self.keys.iter().all(|x| x.value.is_some())
diff --git a/lib/cache/build/src/req_config.rs b/lib/cache/build/src/req_config.rs
index bb4f88f35..74d09691f 100644
--- a/lib/cache/build/src/req_config.rs
+++ b/lib/cache/build/src/req_config.rs
@@ -152,9 +152,18 @@ impl RequestConfig {
 			.with_label_values(&[&base_key])
 			.inc_by(keys.len() as u64);
 
-		let redis_keys = keys
+		// Build context.
+		//
+		// Drop `keys` bc this is not the same as the keys list in `ctx`, so it should not be used
+		// again.
+		let mut ctx = GetterCtx::new(base_key.clone().into(), keys.to_vec());
+		drop(keys);
+
+		// Build keys to look up values in Redis
+		let redis_keys = ctx
+			.keys()
 			.iter()
-			.map(|key| self.cache.build_redis_cache_key(&base_key, key))
+			.map(|key| self.cache.build_redis_cache_key(&base_key, &key.key))
 			.collect::<Vec<_>>();
 
 		// Build Redis command explicitly, since `conn.get` with one value will
@@ -183,7 +192,6 @@ impl RequestConfig {
 				);
 
 				// Create the getter ctx and resolve the cached values
-				let mut ctx = GetterCtx::new(base_key.clone().into(), keys.to_vec());
 				for (i, value) in cached_values.into_iter().enumerate() {
 					if let Some(value) = value {
 						let value = decoder(&value)?;
@@ -266,12 +274,8 @@ impl RequestConfig {
 
 				// Fall back to the getter since we can't fetch the value from
 				// the cache
-				let ctx = getter(
-					GetterCtx::new(base_key.into(), keys.to_vec()),
-					keys.to_vec(),
-				)
-				.await
-				.map_err(Error::Getter)?;
+				let keys = ctx.unresolved_keys();
+				let ctx = getter(ctx, keys).await.map_err(Error::Getter)?;
 
 				Ok(ctx.into_values())
 			}
@@ -656,11 +660,11 @@ impl RequestConfig {
 	}
 }
 
-#[tracing::instrument(skip(conn))]
-async fn unwatch_gracefully(conn: &mut RedisPool) {
-	tracing::debug!("unwatching");
-	match redis::cmd("UNWATCH").query_async::<_, ()>(conn).await {
-		Ok(_) => tracing::debug!("unwatched successfully"),
-		Err(err) => tracing::error!(?err, "failed to unwatch from cache"),
-	}
-}
+// #[tracing::instrument(skip(conn))]
+// async fn unwatch_gracefully(conn: &mut RedisPool) {
+// 	tracing::debug!("unwatching");
+// 	match redis::cmd("UNWATCH").query_async::<_, ()>(conn).await {
+// 		Ok(_) => tracing::debug!("unwatched successfully"),
+// 		Err(err) => tracing::error!(?err, "failed to unwatch from cache"),
+// 	}
+// }
diff --git a/svc/pkg/mm-config/ops/version-get/src/lib.rs b/svc/pkg/mm-config/ops/version-get/src/lib.rs
index 59dc009d5..fa469a143 100644
--- a/svc/pkg/mm-config/ops/version-get/src/lib.rs
+++ b/svc/pkg/mm-config/ops/version-get/src/lib.rs
@@ -55,58 +55,21 @@ async fn handle(
 
 	// TODO: There's a bug with this that returns the lobby groups for the wrong
 	// version, can't figure this out
-	// let versions = ctx
-	// 	.cache()
-	// 	.immutable()
-	// 	.fetch_all_proto("versions", req_version_ids, |mut cache, req_version_ids| {
-	// 		let ctx = ctx.base();
-	//
-	// 		async move {
-	// 			fetch_versions(&ctx, req_version_ids)
-	// 				.await?
-	// 				.into_iter()
-	// 				.for_each(|(version_id, version)| {
-	// 					cache.resolve_with_topic(
-	// 						&version_id,
-	// 						version,
-	// 						("game_mm_versions", &version_id),
-	// 					)
-	// 				});
-	// 			Ok(cache)
-	// 		}
-	// 	})
-	// 	.await?;
+	let versions = ctx
+		.cache()
+		.immutable()
+		.fetch_all_proto("versions", req_version_ids, |mut cache, req_version_ids| {
+			let ctx = ctx.base();
 
-	// HACK: Because fetch all doesn't work, we'll use fetch one
-	// let mut versions = Vec::new();
-	// for version_id in req_version_ids {
-	// 	let version = ctx
-	// 		.cache()
-	// 		.immutable()
-	// 		.fetch_one_proto("versions2", version_id, |mut cache, req_version_id| {
-	// 			let ctx = ctx.base();
-	//
-	// 			async move {
-	// 				let versions = fetch_versions(&ctx.base(), vec![req_version_id]).await?;
-	// 				ensure!(versions.len() <= 1, "too many versions");
-	// 				if let Some((_, version)) = versions.into_iter().next() {
-	// 					cache.resolve(&version_id, version);
-	// 				}
-	//
-	// 				Ok(cache)
-	// 			}
-	// 		})
-	// 		.await?;
-	// 	if let Some(version) = version {
-	// 		versions.push(version);
-	// 	}
-	// }
-
-	let versions = fetch_versions(&ctx.base(), req_version_ids)
-		.await?
-		.into_iter()
-		.map(|x| x.1)
-		.collect::<Vec<_>>();
+			async move {
+				fetch_versions(&ctx, req_version_ids)
+					.await?
+					.into_iter()
+					.for_each(|(version_id, version)| cache.resolve(&version_id, version));
+				Ok(cache)
+			}
+		})
+		.await?;
 
 	Ok(mm_config::version_get::Response { versions })
 }