Skip to content

Commit d971419

Browse files
Do not sweep conversion to ReplicaVerion errors under the rug.
1 parent 72f3292 commit d971419

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
/// (specifically, Registry's invariants checks), but we mention them here
181186
/// for completeness.
@@ -437,10 +442,29 @@ impl<T: RegistryClient + ?Sized> SubnetRegistry for T {
437442
return Ok(None);
438443
};
439444

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

446470
// Only engines are allowed to have a blank replica_version_id (i.e.
@@ -480,7 +504,10 @@ impl<T: RegistryClient + ?Sized> SubnetRegistry for T {
480504

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

486513
fn get_replica_version_record(

0 commit comments

Comments
 (0)