Skip to content

Commit 4d5a24f

Browse files
Do not sweep conversion to ReplicaVerion errors under the rug.
1 parent 4b10792 commit 4d5a24f

1 file changed

Lines changed: 32 additions & 5 deletions

File tree

rs/registry/helpers/src/subnet.rs

Lines changed: 32 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ pub trait SubnetRegistry {
167167
/// [StandardEngineReplicaVersionRecord]. Non-blank `replica_version_id`
168168
/// means the same thing as in the non-CloudEngine case.
169169
///
170-
/// Err is returned in various cases, but call out a couple in particular,
170+
/// Err is returned in various cases, but we call out a few in particular,
171171
/// because the error type is unintuitive: DecodeError is returned in the
172172
/// following cases:
173173
///
@@ -176,6 +176,11 @@ pub trait SubnetRegistry {
176176
///
177177
/// 2. Non-CloudEngine with blank replica_version_id.
178178
///
179+
/// 3. Replica version ID string cannot be converted to a ReplicaVersion
180+
/// object. This means that the string contains some illegal characters.
181+
/// In particular, only latin letters, digits, dot, dash, and underscore
182+
/// are allowed (as of July 2026).
183+
///
179184
/// In practice, such data problems are prevented from happening elsewhere,
180185
/// but we mention them here for completeness.
181186
fn get_replica_version(
@@ -436,10 +441,29 @@ impl<T: RegistryClient + ?Sized> SubnetRegistry for T {
436441
return Ok(None);
437442
};
438443

439-
// Engines may opt out of the standard deployment by setting their own
440-
// replica_version_id; if they have, that is authoritative.
444+
let str_to_result = |replica_version_id: &str,
445+
case: &str|
446+
-> Result<Option<ReplicaVersion>, RegistryClientError> {
447+
let ok = ReplicaVersion::try_from(replica_version_id)
448+
// This wouldn't happen in practice (because of validation that
449+
// happens elsewhere), but we handle it here anyway, because
450+
// bugs.
451+
.map_err(|err| DecodeError {
452+
error: format!(
453+
"get_replica_version({subnet_id}): {case}: '{replica_version_id}' is not a valid \
454+
ReplicaVersion: {err}"
455+
),
456+
})?;
457+
458+
Ok(Some(ok))
459+
};
460+
461+
// Specified directly in SubnetRecord.
441462
if !subnet_record.replica_version_id.is_empty() {
442-
return Ok(ReplicaVersion::try_from(subnet_record.replica_version_id.as_ref()).ok());
463+
return str_to_result(
464+
&subnet_record.replica_version_id,
465+
"specified directly in SubnetRecord",
466+
);
443467
}
444468

445469
// Only engines are allowed to have a blank replica_version_id (i.e.
@@ -479,7 +503,10 @@ impl<T: RegistryClient + ?Sized> SubnetRegistry for T {
479503

480504
// At this point, resolved_replica_version_id should be a git commit ID
481505
// in the ic repo. This just converts it from a raw string.
482-
Ok(ReplicaVersion::try_from(resolved_replica_version_id.as_ref()).ok())
506+
str_to_result(
507+
&resolved_replica_version_id,
508+
"using standard engine replica version",
509+
)
483510
}
484511

485512
fn get_replica_version_record(

0 commit comments

Comments
 (0)