diff --git a/rs/registry/helpers/src/subnet.rs b/rs/registry/helpers/src/subnet.rs index 109a83755ea9..5d375b95198f 100644 --- a/rs/registry/helpers/src/subnet.rs +++ b/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 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, 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 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);