restore remote_cache_rpc_timeout_millis/remote_cache_rpc_concurrency

docs/notes/2.34.x.md

@@ -26,6 +26,8 @@ Published Pants binaries are now compiled with a new `dist` Cargo profile that e
 
 Pants option config files are now parsed as TOML 1.1 rather than TOML 1.0. This covers `pants.toml` and any other file named by `[GLOBAL].pants_config_files`, the rcfiles named by `[GLOBAL].pantsrc_files` (`/etc/pantsrc`, `~/.pants.rc` and `.pants.rc` by default), and `.toml` files referenced by `@fromfile` option values. Inline tables may now span multiple lines and end with a trailing comma, strings may use the `\e` and `\xHH` escapes, and times may omit their seconds. TOML 1.1 only adds syntax to TOML 1.0, so existing files continue to parse unchanged. TOML files read by backends, such as `pyproject.toml`, are unaffected.
 
+The `remote_cache_rpc_timeout_millis` & `remote_cache_rpc_concurrency` options are now correctly used again after being silently ignored since 2.19.  The documented default for `remote_cache_rpc_timeout_millis` is now `30000` to align with the actual behavior since 2.19.  If you used the defaults then nothing changes; if you explicitly set them they now take effect.
+
 ### Goals
 
 ### Backends

src/python/pants/option/bootstrap_options.py

@@ -764,7 +764,9 @@ def from_options(cls, options: OptionValueContainer) -> LocalStoreOptions:
     # Remote cache setup.
     remote_cache_warnings=RemoteCacheWarningsBehavior.backoff,
     remote_cache_rpc_concurrency=128,
-    remote_cache_rpc_timeout_millis=1500,
+    # NB: Matches the store RPC timeout: from 2.19 until this option's wiring was restored, cache
+    # RPCs accidentally used the store timeout, so this preserves the effective default.
+    remote_cache_rpc_timeout_millis=30000,
     # Remote execution setup.
     remote_execution_address=None,
     remote_execution_headers={},

src/rust/engine/src/context.rs

@@ -144,6 +144,19 @@ impl RemotingOptions {
             batch_load_enabled: self.store_batch_load_enabled,
         })
     }
+
+    /// Options for the remote ActionCache provider: identical to the store options, except that
+    /// the cache-specific RPC timeout and concurrency limit apply instead of the store ones.
+    fn to_remote_cache_options(
+        &self,
+        tls_config: grpc_util::tls::Config,
+    ) -> Result<RemoteStoreOptions, String> {
+        Ok(RemoteStoreOptions {
+            timeout: self.cache_rpc_timeout,
+            concurrency_limit: self.cache_rpc_concurrency,
+            ..self.to_remote_store_options(tls_config)?
+        })
+    }
 }
 
 #[derive(Clone, Debug)]
@@ -393,7 +406,7 @@ impl Core {
                             .append_only_caches_base_path
                             .clone(),
                     },
-                    remoting_opts.to_remote_store_options(tls_config)?,
+                    remoting_opts.to_remote_cache_options(tls_config)?,
                 )
                 .await?,
             );
@@ -1010,3 +1023,57 @@ impl SessionCore {
         self.backtrack_levels.lock().get(node).cloned().unwrap_or(0)
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use std::collections::BTreeMap;
+    use std::time::Duration;
+
+    use process_execution::CacheContentBehavior;
+    use remote::remote_cache::RemoteCacheWarningsBehavior;
+    use store::RemoteProvider;
+
+    use super::RemotingOptions;
+
+    #[test]
+    fn remote_cache_options_use_cache_rpc_settings() {
+        let remoting_opts = RemotingOptions {
+            provider: RemoteProvider::Reapi,
+            execution_enable: false,
+            store_address: Some("http://localhost:0".to_owned()),
+            execution_address: None,
+            execution_process_cache_namespace: None,
+            instance_name: None,
+            root_ca_certs_path: None,
+            client_certs_path: None,
+            client_key_path: None,
+            store_headers: BTreeMap::new(),
+            store_chunk_bytes: 1024,
+            store_rpc_retries: 2,
+            store_rpc_concurrency: 64,
+            store_rpc_timeout: Duration::from_secs(30),
+            store_batch_api_size_limit: 1024,
+            store_batch_load_enabled: false,
+            cache_warnings_behavior: RemoteCacheWarningsBehavior::FirstOnly,
+            cache_content_behavior: CacheContentBehavior::Fetch,
+            cache_rpc_concurrency: 128,
+            cache_rpc_timeout: Duration::from_secs(5),
+            execution_headers: BTreeMap::new(),
+            execution_overall_deadline: Duration::from_secs(60),
+            execution_rpc_concurrency: 1,
+            append_only_caches_base_path: None,
+        };
+
+        let tls_config = grpc_util::tls::Config::default();
+
+        let store_options = remoting_opts
+            .to_remote_store_options(tls_config.clone())
+            .unwrap();
+        assert_eq!(store_options.timeout, Duration::from_secs(30));
+        assert_eq!(store_options.concurrency_limit, 64);
+
+        let cache_options = remoting_opts.to_remote_cache_options(tls_config).unwrap();
+        assert_eq!(cache_options.timeout, Duration::from_secs(5));
+        assert_eq!(cache_options.concurrency_limit, 128);
+    }
+}