From 09e0a50210db99040dfdadadc9c211861d2e6b1e Mon Sep 17 00:00:00 2001 From: Raymond Khalife Date: Sun, 30 Aug 2026 20:21:59 -0400 Subject: [PATCH] fix: keep a binding's stamped id when this run cannot resolve it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `set_env_vars_for_canister` wrote the canister's manifest variables plus the bindings it could resolve, and `update_settings` replaces the whole list. So a binding whose target had no id in the environment's store was not merely skipped — the variable a full deploy had stamped was deleted, and the canister read an empty value from then on, with nothing said anywhere. An unresolved binding means "no id in *this* store", not "not wired": the referenced canister lies outside the run's scope, or was deployed from another project root, which is what a workspace member deployed on its own sees of its dependencies. The `resolve_targets` guard catches one shape of this (a member-scoped deploy that names no canisters) and does not catch the rest, because naming canisters explicitly skips it. Carry the current value over for a binding this run cannot compute, and warn with the canister that could not be resolved. Resolved bindings still overwrite, and a variable the manifest declares still outranks a stamped one, so nothing else about the namespace changes. The canister's status is read only when there is an unresolved binding, so the common path still writes without reading first. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 1 + crates/icp-cli/tests/dependency_tests.rs | 153 +++++++++++++++++- crates/icp/src/operations/binding_env_vars.rs | 99 ++++++++++-- 3 files changed, 236 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 48ee145a8..3aa4ea649 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ air-gapped signing * fix: `icp canister logs` output formats are corrected. `--json` now emits machine-readable JSON and the default emits the human-readable lines (the two were swapped), and `--follow --json` emits newline-delimited JSON, one record per line, streamed as each record arrives. This is breaking for scripts: parsing the default output as JSON now requires `--json`, and consumers of `--follow --json` must read one JSON object per line. * fix: `icp canister status` again falls back on the publicly readable state-tree information when the caller may not read the status. Replicas now reject those calls with `IC0542`, which the fallback did not recognise, so the command failed with `Error looking up canister ` instead of printing the controllers and module hash. `IC0541`, returned on subnets with administrators, is now recognised too, and the fallback no longer depends on whether the rejection arrives certified or uncertified. * fix: `icp canister settings sync` now applies the settings the selected environment declares, not only the canister's base settings. It resolved the canister from the project's pre-override record, so a `settings:` block under `environments:` — including the one a vendored member declares for its own environments — was ignored: a setting declared only there was never written, and the command reported success having changed nothing. `icp deploy` already read the environment's record, so the two disagreed about the same canister in the same environment. +* fix: a deploy that targets only some of a project's canisters no longer unwires them from the ones it did not target. `PUBLIC_CANISTER_ID:` variables are computed from the ids in the environment's store and written as the canister's whole variable list, so a run that could not see a referenced canister's id — it lies outside the run's scope, or was deployed from another project root, which is how a workspace member deployed on its own sees its dependencies — silently dropped the variable a full deploy had stamped, and the canister read an empty value from then on. Such a variable now keeps the value it has, and the run reports which canister it could not resolve. ## Experimental diff --git a/crates/icp-cli/tests/dependency_tests.rs b/crates/icp-cli/tests/dependency_tests.rs index ca0cdceef..ec863089d 100644 --- a/crates/icp-cli/tests/dependency_tests.rs +++ b/crates/icp-cli/tests/dependency_tests.rs @@ -2,7 +2,11 @@ use indoc::formatdoc; use predicates::{prelude::PredicateBooleanExt, str::contains}; use crate::common::{ENVIRONMENT_RANDOM_PORT, NETWORK_RANDOM_PORT, TestContext, clients}; -use icp::{fs::write_string, prelude::*}; +use icp::{ + fs::{read_to_string, write_string}, + prelude::*, +}; +use std::collections::BTreeMap; mod common; @@ -524,3 +528,150 @@ async fn deploy_with_shared_dependency_dedups_to_one_instance() { ); } } + +/// The canister id store the CLI keeps for `env` under `project_dir`. +fn ids_store_path(project_dir: &Path, env: &str) -> PathBuf { + for sub in ["cache", "data"] { + let p = project_dir.join(format!(".icp/{sub}/mappings/{env}.ids.json")); + if p.exists() { + return p; + } + } + panic!("no canister id store for environment '{env}' under {project_dir}/.icp"); +} + +/// A binding whose target has no id in the store must not unwire the canister. +/// A deploy scoped to one canister cannot recompute an id it cannot see — the +/// dependency was deployed from another project root, or lies outside this +/// run's scope — so it keeps the id already stamped and says why, instead of +/// replacing the canister's variables with a set that silently omits it. +#[tokio::test] +async fn scoped_deploy_keeps_a_dependency_id_it_cannot_resolve() { + let ctx = TestContext::new(); + let project_dir = ctx.create_project_dir("icp"); + let wasm = ctx.make_asset("example_icp_mo.wasm"); + + let dep_dir = project_dir.join("vendor/openemail"); + std::fs::create_dir_all(&dep_dir).expect("failed to create dependency dir"); + let dep_manifest = formatdoc! {r#" + canisters: + - name: backend + build: + steps: + - type: script + command: cp '{wasm}' "$ICP_WASM_OUTPUT_PATH" + + environments: + - name: random-environment + "#}; + write_string(&dep_dir.join("icp.yaml"), &dep_manifest) + .expect("failed to write dependency manifest"); + + let pm = formatdoc! {r#" + canisters: + - name: app + build: + steps: + - type: script + command: cp '{wasm}' "$ICP_WASM_OUTPUT_PATH" + + dependencies: + - name: openemail + path: ./vendor/openemail + + {NETWORK_RANDOM_PORT} + {ENVIRONMENT_RANDOM_PORT} + "#}; + write_string(&project_dir.join("icp.yaml"), &pm).expect("failed to write project manifest"); + + let _g = ctx.start_network_in(&project_dir, "random-network").await; + ctx.ping_until_healthy(&project_dir, "random-network"); + + clients::icp(&ctx, &project_dir, Some("random-environment".to_string())) + .mint_cycles(200 * TRILLION); + + // A full workspace deploy stamps the real dependency id. + ctx.icp() + .current_dir(&project_dir) + .args(["deploy", "--environment", "random-environment"]) + .assert() + .success(); + + let assert = ctx + .icp() + .current_dir(&project_dir) + .args([ + "canister", + "status", + "--environment", + "random-environment", + "vendor/openemail:backend", + "--id-only", + ]) + .assert() + .success(); + let openemail_id = String::from_utf8(assert.get_output().stdout.clone()) + .expect("canister id should be valid utf-8") + .trim() + .to_string(); + assert!( + !openemail_id.is_empty(), + "expected an openemail canister id" + ); + + let stamped = + || contains("PUBLIC_CANISTER_ID:openemail:backend").and(contains(openemail_id.clone())); + ctx.icp() + .current_dir(&project_dir) + .args([ + "canister", + "settings", + "show", + "app", + "--environment", + "random-environment", + ]) + .assert() + .success() + .stdout(stamped()); + + // Drop the dependency from the id store this run reads — the state a service + // deployed from its own project root, or a fresh checkout, is in. + let store = ids_store_path(&project_dir, "random-environment"); + let mut ids: BTreeMap = + serde_json::from_str(&read_to_string(&store).expect("failed to read the id store")) + .expect("the id store should map canister names to principals"); + ids.remove("vendor/openemail:backend") + .expect("the dependency should be in the store"); + write_string( + &store, + &serde_json::to_string(&ids).expect("failed to serialize the id store"), + ) + .expect("failed to write the id store"); + + // Re-deploying just the app cannot recompute the id, and says so... + ctx.icp() + .current_dir(&project_dir) + .args(["deploy", "app", "--environment", "random-environment"]) + .assert() + .success() + .stderr( + contains("vendor/openemail:backend") + .and(contains("PUBLIC_CANISTER_ID:openemail:backend")), + ); + + // ...so it keeps the id already stamped, rather than dropping the variable. + ctx.icp() + .current_dir(&project_dir) + .args([ + "canister", + "settings", + "show", + "app", + "--environment", + "random-environment", + ]) + .assert() + .success() + .stdout(stamped()); +} diff --git a/crates/icp/src/operations/binding_env_vars.rs b/crates/icp/src/operations/binding_env_vars.rs index 8025a0b03..ed5b9f276 100644 --- a/crates/icp/src/operations/binding_env_vars.rs +++ b/crates/icp/src/operations/binding_env_vars.rs @@ -3,12 +3,14 @@ use std::collections::{BTreeMap, HashSet}; use crate::Canister; use futures::{StreamExt, stream::FuturesOrdered}; use ic_agent::{Agent, export::Principal}; -use ic_management_canister_types::{CanisterSettings, EnvironmentVariable, UpdateSettingsArgs}; +use ic_management_canister_types::{ + CanisterIdRecord, CanisterSettings, EnvironmentVariable, UpdateSettingsArgs, +}; use icp_events::TaskOutcome; use crate::operations::task::{Reporter, Task}; -use snafu::Snafu; -use tracing::error; +use snafu::{ResultExt, Snafu}; +use tracing::{error, warn}; use super::proxy::UpdateOrProxyError; use super::proxy_management; @@ -21,6 +23,12 @@ pub enum BindingEnvVarsOperationError { canister_names: Vec, }, + #[snafu(display("failed to fetch current canister settings for canister {canister}"))] + FetchCurrentSettings { + source: UpdateOrProxyError, + canister: Principal, + }, + #[snafu(transparent)] UpdateOrProxy { source: UpdateOrProxyError }, } @@ -31,12 +39,27 @@ pub struct SetBindingEnvVarsManyError { names: Vec, } +/// The environment-variable namespace the project's bindings are stamped into. +const BINDING_PREFIX: &str = "PUBLIC_CANISTER_ID:"; + +/// Write a canister's environment variables: the ones its manifest declares, +/// with the ids it is wired to stamped over them. +/// +/// `unresolved` names the bindings this run could not compute, as +/// `(variable, referenced canister)`. Their current values are carried over, +/// because the update below replaces the canister's whole variable list, and an +/// unresolved binding means "no id in *this* store" — the dependency was +/// deployed from another project root, or lies outside this command's scope — +/// not "this canister is not wired to it". Without that, a deploy scoped to one +/// service would delete the id a full workspace deploy had stamped, leaving the +/// canister to read an empty value with nothing said anywhere. pub async fn set_env_vars_for_canister( agent: &Agent, proxy: Option, canister_id: &Principal, canister_info: &Canister, binding_vars: &[(String, String)], + unresolved: &[(String, String)], ) -> Result<(), BindingEnvVarsOperationError> { let mut environment_variables = canister_info .settings @@ -44,6 +67,37 @@ pub async fn set_env_vars_for_canister( .to_owned() .unwrap_or_default(); + // Only an unresolved binding needs the canister's current state, so the + // common path still writes without reading first. + if !unresolved.is_empty() { + let status = proxy_management::canister_status( + agent, + proxy, + CanisterIdRecord { + canister_id: *canister_id, + }, + ) + .await + .context(FetchCurrentSettingsSnafu { + canister: *canister_id, + })?; + + for (variable, _) in unresolved { + let Some(current) = status + .settings + .environment_variables + .iter() + .find(|v| &v.name == variable) + else { + continue; + }; + // A value the manifest declares still outranks a stamped one. + environment_variables + .entry(variable.to_owned()) + .or_insert_with(|| current.value.clone()); + } + } + // inject the ids of the other canisters for (k, v) in binding_vars.iter() { environment_variables.insert(k.to_string(), v.to_string()); @@ -120,22 +174,35 @@ pub async fn set_binding_env_vars_many( // their aliases), resolved to the ids that exist in this environment. // A project without dependencies wires every canister to every sibling, // reproducing the previous flat behavior. - let binding_vars: Vec<(String, String)> = info - .bindings - .iter() - .filter_map(|(env_name, referenced_key)| { - canister_list.get(referenced_key).map(|principal| { - ( - format!("PUBLIC_CANISTER_ID:{env_name}"), - principal.to_text(), - ) - }) - }) - .collect(); + // + // A binding whose target has no id in this environment is kept aside + // rather than dropped: the canister may already carry the value, and + // silently writing it away is how a scoped deploy used to unwire a + // canister from its dependency. + let mut binding_vars: Vec<(String, String)> = Vec::new(); + let mut unresolved: Vec<(String, String)> = Vec::new(); + for (env_name, referenced_key) in &info.bindings { + let variable = format!("{BINDING_PREFIX}{env_name}"); + match canister_list.get(referenced_key) { + Some(principal) => binding_vars.push((variable, principal.to_text())), + None => unresolved.push((variable, referenced_key.to_owned())), + } + } let agent = agent.clone(); futs.push_back(async move { - let result = set_env_vars_for_canister(&agent, proxy, &cid, &info, &binding_vars).await; + for (variable, referenced_key) in &unresolved { + warn!( + "Canister '{}' is wired to '{referenced_key}', which has no id in environment \ + '{environment_name}'; leaving '{variable}' as it is. Deploy \ + '{referenced_key}' to stamp its id.", + info.name + ); + } + + let result = + set_env_vars_for_canister(&agent, proxy, &cid, &info, &binding_vars, &unresolved) + .await; match &result { Ok(()) => task.finish(TaskOutcome::succeeded()),