Skip to content

Commit

Permalink
fix: Correct the value returned for replicated vs non replicated exec…
Browse files Browse the repository at this point in the history
…ution in system api (#3540)

This PR fixes some bugs in how the system API was reporting replicated
vs non-replicated mode. Specifically, it was incorrectly reporting that
`inspect_message`, a composite query `reply_callback` and
`reject_callback` were running in replicated mode, although they are
really running in non-replicated mode.
  • Loading branch information
dsarlis authored Jan 21, 2025
1 parent 0b30ddf commit 82576ad
Show file tree
Hide file tree
Showing 3 changed files with 69 additions and 4 deletions.
11 changes: 7 additions & 4 deletions rs/system_api/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3396,15 +3396,18 @@ impl SystemApi for SystemApiImpl {
let result = match &self.api_type {
ApiType::Start { .. }
| ApiType::Init { .. }
| ApiType::ReplyCallback { .. }
| ApiType::RejectCallback { .. }
| ApiType::Cleanup { .. }
| ApiType::PreUpgrade { .. }
| ApiType::InspectMessage { .. }
| ApiType::Update { .. }
| ApiType::SystemTask { .. }
| ApiType::ReplicatedQuery { .. } => Ok(1),
ApiType::NonReplicatedQuery { .. } => Ok(0),
ApiType::ReplyCallback { .. } | ApiType::RejectCallback { .. } => {
match self.execution_parameters.execution_mode {
ExecutionMode::NonReplicated => Ok(0),
ExecutionMode::Replicated => Ok(1),
}
}
ApiType::InspectMessage { .. } | ApiType::NonReplicatedQuery { .. } => Ok(0),
};
trace_syscall!(self, ic0_in_replicated_execution, result);
result
Expand Down
25 changes: 25 additions & 0 deletions rs/system_api/tests/common/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,10 @@ impl ApiTypeBuilder {
)
}

pub fn build_replicated_query_api() -> ApiType {
ApiType::replicated_query(UNIX_EPOCH, vec![], user_test_id(1).get())
}

pub fn build_non_replicated_query_api() -> ApiType {
ApiType::non_replicated_query(
UNIX_EPOCH,
Expand Down Expand Up @@ -175,6 +179,27 @@ impl ApiTypeBuilder {
0.into(),
)
}

pub fn build_inspect_message_api() -> ApiType {
ApiType::inspect_message(
PrincipalId::new_anonymous(),
"test".to_string(),
vec![],
UNIX_EPOCH,
)
}

pub fn build_start_api() -> ApiType {
ApiType::start(UNIX_EPOCH)
}

pub fn build_init_api() -> ApiType {
ApiType::init(UNIX_EPOCH, vec![], user_test_id(1).get())
}

pub fn build_pre_upgrade_api() -> ApiType {
ApiType::pre_upgrade(UNIX_EPOCH, user_test_id(1).get())
}
}

pub fn get_system_api(
Expand Down
37 changes: 37 additions & 0 deletions rs/system_api/tests/system_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2069,3 +2069,40 @@ fn test_save_log_message_keeps_total_log_size_limited() {
assert_eq!(log.records().len(), initial_records_number + 1);
assert_le!(log.used_space(), MAX_ALLOWED_CANISTER_LOG_BUFFER_SIZE);
}

#[test]
fn in_replicated_execution_works_correctly() {
// The following should execute in replicated mode.
for api_type in &[
ApiTypeBuilder::build_update_api(),
ApiTypeBuilder::build_system_task_api(),
ApiTypeBuilder::build_start_api(),
ApiTypeBuilder::build_init_api(),
ApiTypeBuilder::build_pre_upgrade_api(),
ApiTypeBuilder::build_replicated_query_api(),
ApiTypeBuilder::build_reply_api(Cycles::new(0)),
ApiTypeBuilder::build_reject_api(RejectContext::new(RejectCode::CanisterReject, "error")),
] {
let cycles_account_manager = CyclesAccountManagerBuilder::new().build();
let system_state = SystemStateBuilder::default().build();
let api = get_system_api(api_type.clone(), &system_state, cycles_account_manager);
assert_eq!(api.ic0_in_replicated_execution(), Ok(1));
}

// The following should execute in non-replicated mode.
for api_type in &[
ApiTypeBuilder::build_non_replicated_query_api(),
ApiTypeBuilder::build_composite_query_api(),
ApiTypeBuilder::build_composite_reply_api(Cycles::new(0)),
ApiTypeBuilder::build_composite_reject_api(RejectContext::new(
RejectCode::CanisterReject,
"error",
)),
ApiTypeBuilder::build_inspect_message_api(),
] {
let cycles_account_manager = CyclesAccountManagerBuilder::new().build();
let system_state = SystemStateBuilder::default().build();
let api = get_system_api(api_type.clone(), &system_state, cycles_account_manager);
assert_eq!(api.ic0_in_replicated_execution(), Ok(0));
}
}

0 comments on commit 82576ad

Please sign in to comment.