fix: add cache to server list queries #2620

MasterPtato · 2025-06-16T21:57:23Z

Changes

MasterPtato · 2025-06-16T21:57:58Z

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

_{An organization admin has enabled the Graphite Merge Queue in this repository.} _{Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.}

This stack of pull requests is managed by Graphite. Learn more about stacking.

greptile-apps

PR Summary

Implements caching layer for server list queries across multiple services to improve performance and reduce database load.

Added new server_discovery module in packages/core/services/cluster/src/ops/datacenter/server_discovery.rs with 5-second TTL cache
Migrated from custom types to standard rivet_api::models types in packages/common/service-discovery/src/lib.rs
Modified packages/edge/infra/guard/server/src/routing/api.rs to use service discovery instead of direct server queries
Re-enabled prewarm request error handling in packages/core/api/actor/src/route/builds.rs for build completion safety
Added exclude_installing flag across server list operations for better control of server lifecycle management

_{18 files reviewed, 6 comments}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2025-06-16T21:58:15Z

packages/core/services/cluster/src/types.rs

+			"worker" => Ok(PoolType::Worker),
+			"nats" => Ok(PoolType::Nats),
+			"guard" => Ok(PoolType::Guard),
+			_ => bail!("Invalid PoolType string: {}", s),


style: Include variant name in error message for better debugging: Invalid PoolType string '{}' (expected: job, gg, ats, etc)

greptile-apps · 2025-06-16T21:58:19Z

packages/core/services/cluster/src/ops/server/taint_with_filter.rs

 		.op(crate::ops::server::list::Input {
 			filter: input.filter.clone(),
 			include_destroyed: false,
+			exclude_installing: false,
 			exclude_draining: false,
 			exclude_no_vlan: false,
 		})


logic: All filter flags are set to false, but there's no comment explaining why we want to include all server states. Consider adding a comment explaining this design decision.

greptile-apps · 2025-06-16T21:58:20Z

packages/core/api/provision/src/route/datacenters.rs

 	let servers_res = ctx
-		.op(cluster::ops::server::list::Input {
-			filter: cluster::types::Filter {
-				datacenter_ids: Some(vec![datacenter_id]),
-				pool_types: (!query.pools.is_empty())
-					.then(|| query.pools.into_iter().map(ApiInto::api_into).collect()),
-				..Default::default()
-			},
-			include_destroyed: false,
-			exclude_draining: true,
-			exclude_no_vlan: true,
+		.op(cluster::ops::datacenter::server_discovery::Input {
+			datacenter_id,
+			pool_types: query
+				.pools
+				.into_iter()
+				.map(ApiInto::api_into)
+				.collect(),
 		})
 		.await?;


logic: Replacing server::list with server_discovery removes several critical filters (exclude_draining, exclude_no_vlan). Verify these are handled in server_discovery operation or document why they're no longer needed

greptile-apps · 2025-06-16T21:58:20Z

packages/core/api/provision/src/route/datacenters.rs

 		servers: servers_res
 			.servers
 			.into_iter()
-			// Filter out installing servers
-			.filter(|server| server.install_complete_ts.is_some())
 			.map(ApiInto::api_into)
 			.collect(),


logic: Removal of .filter(|server| server.install_complete_ts.is_some()) could expose incomplete/installing servers to clients. Verify this is handled in server_discovery or restore the filter

greptile-apps · 2025-06-16T21:58:22Z

packages/common/service-discovery/src/lib.rs

 		let client = Client::new();
 		Ok(self.fetch_inner(&client).await?.servers)


style: Creating new Client for each fetch is inefficient - consider reusing the client from fetch_inner's parameter

greptile-apps · 2025-06-16T21:58:23Z

packages/core/services/cluster/src/ops/datacenter/server_discovery.rs

+	let cache_keys = if input.pool_types.is_empty() {
+		vec![(input.datacenter_id, "all".to_string())]
+	} else {
+		input
+			.pool_types
+			.iter()
+			.map(|pool| (input.datacenter_id, pool.to_string()))
+			.collect()
+	};


style: Consider using a const for 'all' string to avoid magic strings

graphite-app · 2025-06-20T19:19:48Z

Merge activity

Jun 20, 7:19 PM UTC: MasterPtato added this pull request to the Graphite merge queue.
Jun 20, 7:21 PM UTC: CI is running for this pull request on a draft pull request (#2630) due to your merge queue CI optimization settings.
Jun 20, 7:22 PM UTC: Merged by the Graphite merge queue via draft PR: #2630.

## Changes

MasterPtato requested a review from NathanFlurry June 16, 2025 21:57

This was referenced Jun 16, 2025

fix(toolchain): attach tags to function workers #2617

Closed

fix(toolchain): creating ci env no longer fails first try #2618

Closed

feat(examples): multitenant using remote builds #2619

Closed

greptile-apps bot reviewed Jun 16, 2025

View reviewed changes

MasterPtato mentioned this pull request Jun 17, 2025

fix(pegboard): convert ws manager socket to unix socket #2478

Closed

MasterPtato changed the base branch from 06-16-fix_toolchain_attach_tags_to_function_workers to graphite-base/2620 June 18, 2025 23:46

MasterPtato changed the base branch from graphite-base/2620 to 06-16-fix_toolchain_attach_tags_to_function_workers June 18, 2025 23:48

MasterPtato mentioned this pull request Jun 19, 2025

fix: figure out actor log shipping #2622

Closed

MasterPtato changed the base branch from 06-16-fix_toolchain_attach_tags_to_function_workers to graphite-base/2620 June 20, 2025 00:58

MasterPtato mentioned this pull request Jun 20, 2025

fix: get new actors working on docker compose #2623

Closed

fix: add cache to server list queries

0e85267

MasterPtato force-pushed the graphite-base/2620 branch from 77c252b to 2125754 Compare June 20, 2025 19:09

MasterPtato force-pushed the 06-16-fix_add_cache_to_server_list_queries branch from 5d622b5 to 0e85267 Compare June 20, 2025 19:09

MasterPtato changed the base branch from graphite-base/2620 to 06-16-fix_toolchain_attach_tags_to_function_workers June 20, 2025 19:10

NathanFlurry changed the base branch from 06-16-fix_toolchain_attach_tags_to_function_workers to graphite-base/2620 June 20, 2025 19:18

graphite-app bot pushed a commit that referenced this pull request Jun 20, 2025

fix: add cache to server list queries (#2620)

e0fc4c6

 ## Changes

graphite-app bot closed this Jun 20, 2025

graphite-app bot deleted the 06-16-fix_add_cache_to_server_list_queries branch June 20, 2025 19:22

github-actions bot mentioned this pull request Jun 20, 2025

chore(main): release 25.5.1 #2423

Merged

NathanFlurry mentioned this pull request Jun 20, 2025

chore: fix merge conflicts #2634

Closed

This was referenced Jun 20, 2025

fix: add udp ports to docker compose #2635

Closed

feat: get multi actors working e2e on docker compose #2637

Closed

feat: add actor kv to runners #2638

Closed

feat: add actor queue #2644

Closed

fix: fix logs for new actors #2649

Closed

This was referenced Jun 28, 2025

fix(guard): use intercom_address for guard service discovery #2663

Closed

feat(actors): expose container system metrics #2664

Closed

chore: cargo fmt #2665

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: add cache to server list queries #2620

fix: add cache to server list queries #2620

Uh oh!

MasterPtato commented Jun 16, 2025

Uh oh!

MasterPtato commented Jun 16, 2025 •

edited by NathanFlurry

Loading

Uh oh!

greptile-apps bot left a comment

Uh oh!

greptile-apps bot Jun 16, 2025

Uh oh!

greptile-apps bot Jun 16, 2025

Uh oh!

greptile-apps bot Jun 16, 2025

Uh oh!

greptile-apps bot Jun 16, 2025

Uh oh!

greptile-apps bot Jun 16, 2025

Uh oh!

greptile-apps bot Jun 16, 2025

Uh oh!

graphite-app bot commented Jun 20, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		let client = Client::new();
		Ok(self.fetch_inner(&client).await?.servers)

fix: add cache to server list queries #2620

fix: add cache to server list queries #2620

Uh oh!

Conversation

MasterPtato commented Jun 16, 2025

Changes

Uh oh!

MasterPtato commented Jun 16, 2025 • edited by NathanFlurry Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to use the Graphite Merge Queue

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

Uh oh!

greptile-apps bot Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

graphite-app bot commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

MasterPtato commented Jun 16, 2025 •

edited by NathanFlurry

Loading

graphite-app bot commented Jun 20, 2025 •

edited

Loading