feat(orchestrator): Make Orchestrator's get_replica_version method a thin wrapper...

rs/orchestrator/BUILD.bazel

@@ -137,6 +137,7 @@ rust_test(
         "@crate_index//:assert_matches",
         "@crate_index//:async-stream",
         "@crate_index//:hyper-util",
+        "@crate_index//:lazy_static",
         "@crate_index//:libc",
         "@crate_index//:mockall",
         "@crate_index//:rcgen",

rs/orchestrator/Cargo.toml

@@ -81,6 +81,7 @@ qrcode = { workspace = true }
 async-stream = { workspace = true }
 assert_matches = { workspace = true }
 hyper-util = { workspace = true }
+lazy_static = { workspace = true }
 ic-crypto-temp-crypto = { path = "../crypto/temp_crypto" }
 ic-crypto-test-utils-canister-threshold-sigs = { path = "../crypto/test_utils/canister_threshold_sigs" }
 ic-crypto-test-utils-crypto-returning-ok = { path = "../crypto/test_utils/crypto_returning_ok" }

rs/orchestrator/src/registry_helper.rs

@@ -241,9 +241,18 @@ impl RegistryHelper {
         subnet_id: SubnetId,
         registry_version: RegistryVersion,
     ) -> OrchestratorResult<ReplicaVersion> {
-        let subnet_record = self.get_subnet_record(subnet_id, registry_version)?;
-        ReplicaVersion::try_from(subnet_record.replica_version_id.as_ref())
-            .map_err(OrchestratorError::ReplicaVersionParseError)
+        let replica_version = self
+            .registry_client
+            .get_replica_version(subnet_id, registry_version)?;
+
+        let Some(replica_version) = replica_version else {
+            return Err(OrchestratorError::SubnetMissingError(
+                subnet_id,
+                registry_version,
+            ));
+        };
+
+        Ok(replica_version)
     }
 
     /// Get the recalled replica versions of the given subnet in the given registry
@@ -403,3 +412,7 @@ impl RegistryHelper {
         result.ok_or_else(|| OrchestratorError::DomainNameMissingError(self.node_id, version))
     }
 }
+
+#[cfg(test)]
+#[path = "./registry_helper_tests.rs"]
+mod tests;

rs/orchestrator/src/registry_helper_tests.rs

@@ -0,0 +1,140 @@
+use super::*;
+use ic_logger::no_op_logger;
+use ic_protobuf::registry::standard_engine_replica_version::v1::StandardEngineReplicaVersionRecord;
+use ic_registry_client_fake::FakeRegistryClient;
+use ic_registry_keys::{make_standard_engine_replica_version_record_key, make_subnet_record_key};
+use ic_registry_proto_data_provider::ProtoRegistryDataProvider;
+use ic_types::PrincipalId;
+use lazy_static::lazy_static;
+
+fn subnet_id(id: u64) -> SubnetId {
+    SubnetId::from(PrincipalId::new_subnet_test_id(id))
+}
+
+lazy_static! {
+    static ref REGISTRY_VERSION: RegistryVersion = RegistryVersion::new(42);
+    static ref SUBNET_ID: SubnetId = subnet_id(777);
+}
+
+fn new_registry_helper(data_provider: ProtoRegistryDataProvider) -> RegistryHelper {
+    let registry_client = Arc::new(FakeRegistryClient::new(Arc::new(data_provider)));
+    registry_client.update_to_latest_version();
+
+    RegistryHelper::new(
+        NodeId::from(PrincipalId::new_node_test_id(1)),
+        registry_client,
+        no_op_logger(),
+    )
+}
+
+#[test]
+fn test_get_replica_version_when_specified_directly() {
+    // Step 1: Prepare the world.
+
+    let data_provider = ProtoRegistryDataProvider::new();
+
+    data_provider
+        .add(
+            &make_subnet_record_key(*SUBNET_ID),
+            *REGISTRY_VERSION,
+            Some(SubnetRecord {
+                replica_version_id: "some_version".to_string(),
+                ..Default::default()
+            }),
+        )
+        .unwrap();
+
+    let registry = new_registry_helper(data_provider);
+
+    // Step 2: Run the code under test.
+    let result = registry.get_replica_version(*SUBNET_ID, *REGISTRY_VERSION);
+
+    // Step 3: Verify result(s).
+    assert_eq!(
+        result.unwrap(),
+        ReplicaVersion::try_from("some_version").unwrap(),
+    );
+}
+
+#[test]
+fn test_get_replica_version_standard_engine_replica_version() {
+    for (deployment_progress, expected_replica_version_id) in [(1.0, "new"), (0.0, "old")] {
+        // Step 1: Prepare the world.
+
+        let data_provider = ProtoRegistryDataProvider::new();
+
+        // CloudEngine that follows the standard replica version, i.e. has blank
+        // replica_version_id.
+        data_provider
+            .add(
+                &make_subnet_record_key(*SUBNET_ID),
+                *REGISTRY_VERSION,
+                Some(SubnetRecord {
+                    replica_version_id: "".to_string(),
+                    subnet_type: SubnetType::CloudEngine as i32,
+                    ..Default::default()
+                }),
+            )
+            .unwrap();
+
+        // Standard engine replica version.
+        data_provider
+            .add(
+                &make_standard_engine_replica_version_record_key(),
+                *REGISTRY_VERSION,
+                Some(StandardEngineReplicaVersionRecord {
+                    new_replica_version_id: "new".to_string(),
+                    old_replica_version_id: "old".to_string(),
+                    deployment_progress,
+                }),
+            )
+            .unwrap();
+
+        let registry = new_registry_helper(data_provider);
+
+        // Step 2: Run the code under test.
+        let result = registry.get_replica_version(*SUBNET_ID, *REGISTRY_VERSION);
+
+        // Step 3: Verify result(s).
+        assert_eq!(
+            result.unwrap(),
+            ReplicaVersion::try_from(expected_replica_version_id).unwrap(),
+        );
+    }
+}
+
+#[test]
+fn get_replica_version_is_an_error_when_the_subnet_is_missing() {
+    // Step 1: Prepare the world.
+
+    let data_provider = ProtoRegistryDataProvider::new();
+
+    // This isn't really used by this test, but in order for this test to be
+    // not completely trivial, we need SOME registry data.
+    data_provider
+        .add(
+            &make_subnet_record_key(*SUBNET_ID),
+            *REGISTRY_VERSION,
+            Some(SubnetRecord {
+                replica_version_id: "some_version".to_string(),
+                ..Default::default()
+            }),
+        )
+        .unwrap();
+
+    let registry = new_registry_helper(data_provider);
+
+    // Step 2: Run the code under test.
+    let result = registry.get_replica_version(subnet_id(0x_DEAD_BEEF), *REGISTRY_VERSION);
+
+    // Step 3: Verify result(s).
+    match result {
+        Err(OrchestratorError::SubnetMissingError(observed_subnet_id, observed_version)) => {
+            assert_eq!(
+                (observed_subnet_id, observed_version),
+                (subnet_id(0x_DEAD_BEEF), *REGISTRY_VERSION),
+            );
+        }
+        wrong => panic!("{wrong:?}"),
+    }
+}

rs/registry/helpers/src/subnet.rs

@@ -167,7 +167,7 @@ pub trait SubnetRegistry {
     /// [StandardEngineReplicaVersionRecord]. Non-blank `replica_version_id`
     /// means the same thing as in the non-CloudEngine case.
     ///
-    /// Err is returned in various cases, but call out a couple in particular,
+    /// Err is returned in various cases, but we call out a few in particular,
     /// because the error type is unintuitive: DecodeError is returned in the
     /// following cases:
     ///
@@ -176,6 +176,11 @@ pub trait SubnetRegistry {
     ///
     /// 2. Non-CloudEngine with blank replica_version_id.
     ///
+    /// 3. Replica version ID string cannot be converted to a ReplicaVersion
+    ///    object. This means that the string contains some illegal characters.
+    ///    In particular, only latin letters, digits, dot, dash, and underscore
+    ///    are allowed (as of July 2026).
+    ///
     /// In practice, such data problems are prevented from happening elsewhere
     /// (specifically, Registry's invariants checks), but we mention them here
     /// for completeness.
@@ -437,10 +442,29 @@ impl<T: RegistryClient + ?Sized> SubnetRegistry for T {
             return Ok(None);
         };
 
-        // Engines may opt out of the standard deployment by setting their own
-        // replica_version_id; if they have, that is authoritative.
+        let str_to_result = |replica_version_id: &str,
+                             case: &str|
+         -> Result<Option<ReplicaVersion>, RegistryClientError> {
+            let ok = ReplicaVersion::try_from(replica_version_id)
+                // This wouldn't happen in practice (because of validation that
+                // happens elsewhere), but we handle it here anyway, because
+                // bugs.
+                .map_err(|err| DecodeError {
+                    error: format!(
+                        "get_replica_version({subnet_id}): {case}: '{replica_version_id}' is not a valid \
+                        ReplicaVersion: {err}"
+                    ),
+                })?;
+
+            Ok(Some(ok))
+        };
+
+        // Specified directly in SubnetRecord.
         if !subnet_record.replica_version_id.is_empty() {
-            return Ok(ReplicaVersion::try_from(subnet_record.replica_version_id.as_ref()).ok());
+            return str_to_result(
+                &subnet_record.replica_version_id,
+                "specified directly in SubnetRecord",
+            );
         }
 
         // Only engines are allowed to have a blank replica_version_id (i.e.
@@ -480,7 +504,10 @@ impl<T: RegistryClient + ?Sized> SubnetRegistry for T {
 
         // At this point, resolved_replica_version_id should be a git commit ID
         // in the ic repo. This just converts it from a raw string.
-        Ok(ReplicaVersion::try_from(resolved_replica_version_id.as_ref()).ok())
+        str_to_result(
+            &resolved_replica_version_id,
+            "using standard engine replica version",
+        )
     }
 
     fn get_replica_version_record(
@@ -994,6 +1021,74 @@ mod tests {
         assert_matches!(result, Err(RegistryClientError::DecodeError { .. }));
     }
 
+    // This wouldn't occur in practice, so this test is "just" for completeness.
+    #[test]
+    fn replica_version_id_with_illegal_characters_is_an_error() {
+        // Step 1: Prepare the world. A SubnetRecord whose replica_version_id
+        // contains a character that ReplicaVersion::try_from rejects.
+        let data_provider = ProtoRegistryDataProvider::new();
+        data_provider
+            .add(
+                &make_subnet_record_key(subnet_id(1)),
+                RegistryVersion::from(2),
+                Some(SubnetRecord {
+                    replica_version_id: "G@RBAGE".to_string(),
+                    ..Default::default()
+                }),
+            )
+            .unwrap();
+        let registry = FakeRegistryClient::new(Arc::new(data_provider));
+        registry.update_to_latest_version();
+
+        // Step 2: Run the code under test.
+        let result = registry.get_replica_version(subnet_id(1), RegistryVersion::from(2));
+
+        // Step 3: Verify result(s).
+        assert_matches!(result, Err(RegistryClientError::DecodeError { .. }));
+    }
+
+    // This wouldn't occur in practice, so this test is "just" for completeness.
+    #[test]
+    fn resolved_standard_engine_replica_version_id_with_illegal_characters_is_an_error() {
+        // Step 1: Prepare the world. An engine subnet with a blank
+        // replica_version_id, resolving (via
+        // StandardEngineReplicaVersionRecord) to a new_replica_version_id
+        // that ReplicaVersion::try_from rejects.
+        let data_provider = ProtoRegistryDataProvider::new();
+        data_provider
+            .add(
+                &make_subnet_record_key(subnet_id(1)),
+                RegistryVersion::from(2),
+                Some(SubnetRecord {
+                    replica_version_id: "".to_string(),
+                    subnet_type: SubnetType::CloudEngine as i32,
+                    ..Default::default()
+                }),
+            )
+            .unwrap();
+        data_provider
+            .add(
+                &make_standard_engine_replica_version_record_key(),
+                RegistryVersion::from(2),
+                Some(StandardEngineReplicaVersionRecord {
+                    new_replica_version_id: "G@RBAGE".to_string(),
+                    old_replica_version_id: "old".to_string(),
+                    // Guarantees priority <= this, so new_replica_version_id
+                    // (the illegal one) is the one that gets resolved.
+                    deployment_progress: 1.0,
+                }),
+            )
+            .unwrap();
+        let registry = FakeRegistryClient::new(Arc::new(data_provider));
+        registry.update_to_latest_version();
+
+        // Step 2: Run the code under test.
+        let result = registry.get_replica_version(subnet_id(1), RegistryVersion::from(2));
+
+        // Step 3: Verify result(s).
+        assert_matches!(result, Err(RegistryClientError::DecodeError { .. }));
+    }
+
     #[test]
     fn can_get_is_halted_from_subnet() {
         let subnet_id = subnet_id(4);