diff --git a/CHANGELOG.md b/CHANGELOG.md index 84d6dfce5..d6e74346f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ bump. Currently experimental: project bundling, project dependencies # Unreleased +* feat(sync-plugin): the compute-time limit for `plugin` sync steps is now configurable via the `ICP_CLI_PLUGIN_COMPUTE_LIMIT_SECS` environment variable (default `60`). Raise it for compute-heavy plugins (e.g. brotli-compressing a large asset bundle) that legitimately exceed the default, especially on slower CI runners. The limit-exceeded error now names the variable and the current limit, and a malformed value is rejected rather than silently ignored. * feat: `icp canister link` assigns an existing canister principal to a project canister * feat: `icp canister create --with-icp` (not supported in `icp deploy`) uses the CMC to create canisters. Only needed for deploying to restricted system subnets. * feat: `icp deploy --no-create` will error if any canisters do not exist, rather than creating them. diff --git a/crates/icp-cli/tests/sync_tests.rs b/crates/icp-cli/tests/sync_tests.rs index a51361f46..31f72ef48 100644 --- a/crates/icp-cli/tests/sync_tests.rs +++ b/crates/icp-cli/tests/sync_tests.rs @@ -465,6 +465,63 @@ async fn sync_plugin_registers_seed_data() { ); } +/// A malformed `ICP_CLI_PLUGIN_COMPUTE_LIMIT_SECS` must abort the sync with an +/// actionable error rather than being silently ignored. This also exercises the +/// end-to-end wiring: it proves the override is actually read on the real plugin +/// sync path (if it weren't, the bogus value would be ignored and the sync would +/// proceed), which the unit tests can't cover on their own. +#[tokio::test] +async fn sync_plugin_rejects_invalid_compute_limit_env() { + let ctx = TestContext::new(); + let project_dir = ctx.create_project_dir("icp"); + + let (canister_wasm, plugin_wasm) = build_sync_plugin_example(); + + let seed_data = project_dir.join("seed-data"); + create_dir_all(&seed_data).expect("failed to create seed-data"); + write_string(&seed_data.join("fruit-01.txt"), "apple").expect("failed to write fruit-01.txt"); + + let pm = formatdoc! {r#" + canisters: + - name: my-canister + build: + steps: + - type: script + command: cp '{canister_wasm}' "$ICP_WASM_OUTPUT_PATH" + sync: + steps: + - type: plugin + path: {plugin_wasm} + dirs: + - seed-data + + {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(10 * TRILLION); + + // deploy runs the sync step, which reads ICP_CLI_PLUGIN_COMPUTE_LIMIT_SECS. + // A non-integer value must abort with a message that names the variable and + // echoes the offending value. + ctx.icp() + .current_dir(&project_dir) + .env("NO_COLOR", "1") + .env("ICP_CLI_PLUGIN_COMPUTE_LIMIT_SECS", "not-a-number") + .args(["deploy", "--environment", "random-environment"]) + .assert() + .failure() + .stderr( + contains("invalid ICP_CLI_PLUGIN_COMPUTE_LIMIT_SECS value") + .and(contains("not-a-number")), + ); +} + /// A `dirs:` entry that is a symlink (here pointing outside the project) is /// rejected before the plugin runs, so a preopen cannot escape the canister /// directory. Symlinks are forbidden outright for now — see diff --git a/crates/icp-sync-plugin/DESIGN.md b/crates/icp-sync-plugin/DESIGN.md index bcf5ebab4..099a14714 100644 --- a/crates/icp-sync-plugin/DESIGN.md +++ b/crates/icp-sync-plugin/DESIGN.md @@ -69,6 +69,7 @@ pub fn run_plugin( proxy: Option, identity_principal: Principal, environment: String, + compute_limit_secs: u64, stdio: Option>, ) -> Result, RunPluginError> ``` @@ -139,6 +140,10 @@ the elapsed time and the `epoch_deadline_callback` grants it back via `epoch_extension` — so network latency is *not* charged against the limit. The ticker thread stops when its RAII guard drops at the end of `run_plugin`. +The deadline in seconds is the `compute_limit_secs` parameter. The CLI resolves +it from the `ICP_CLI_PLUGIN_COMPUTE_LIMIT_SECS` environment variable, defaulting +to `DEFAULT_PLUGIN_COMPUTE_LIMIT_SECS` (60) when unset. + ### stdio capture `LineCapture` implements `StdoutStream`/`OutputStream`, splits guest output on diff --git a/crates/icp-sync-plugin/src/lib.rs b/crates/icp-sync-plugin/src/lib.rs index cdeee3e2e..dfffddc4c 100644 --- a/crates/icp-sync-plugin/src/lib.rs +++ b/crates/icp-sync-plugin/src/lib.rs @@ -1,4 +1,6 @@ mod path; mod runtime; -pub use runtime::{RunPluginError, run_plugin}; +pub use runtime::{ + DEFAULT_PLUGIN_COMPUTE_LIMIT_SECS, PLUGIN_COMPUTE_LIMIT_ENV, RunPluginError, run_plugin, +}; diff --git a/crates/icp-sync-plugin/src/runtime.rs b/crates/icp-sync-plugin/src/runtime.rs index 947c21c68..fb284fb7f 100644 --- a/crates/icp-sync-plugin/src/runtime.rs +++ b/crates/icp-sync-plugin/src/runtime.rs @@ -9,8 +9,16 @@ use std::time::{Duration, Instant}; const MAX_PLUGIN_OUTPUT: usize = 1024 * 1024; // 1 MiB per stream // Maximum wasm call-stack depth (in bytes). const MAX_WASM_STACK: usize = 512 * 1024; -// How many seconds of pure wasm compute a plugin may use (host-call latency is excluded). -const PLUGIN_COMPUTE_LIMIT_SECS: u64 = 60; +/// Default seconds of pure wasm compute a plugin may use (host-call latency is +/// excluded). This is a runaway guard, not a security boundary: the plugin runs +/// locally in a read-only WASI sandbox, so the limit only protects the machine +/// running `icp sync` from a plugin that never terminates. Legitimately heavy +/// plugins (e.g. brotli-compressing a large asset bundle) can exceed it, +/// especially on slower CI runners, so it is overridable via the +/// [`PLUGIN_COMPUTE_LIMIT_ENV`] environment variable. +pub const DEFAULT_PLUGIN_COMPUTE_LIMIT_SECS: u64 = 60; +/// Environment variable that overrides [`DEFAULT_PLUGIN_COMPUTE_LIMIT_SECS`]. +pub const PLUGIN_COMPUTE_LIMIT_ENV: &str = "ICP_CLI_PLUGIN_COMPUTE_LIMIT_SECS"; use bytes::Bytes; use camino::Utf8PathBuf; @@ -124,8 +132,12 @@ impl SyncPluginImports for HostState { // must return wasmtime::Error (= anyhow::Error). Snafu derives std::error::Error // so .into() converts it via anyhow's blanket From. #[derive(Debug, Snafu)] -#[snafu(display("plugin exceeded the {PLUGIN_COMPUTE_LIMIT_SECS}s compute time limit"))] -struct ComputeTimeLimitExceeded; +#[snafu(display( + "plugin exceeded the {limit_secs}s compute-time limit. If this plugin legitimately needs more compute time (e.g. brotli-compressing a large asset bundle), raise the limit by setting {PLUGIN_COMPUTE_LIMIT_ENV} above {limit_secs}s." +))] +struct ComputeTimeLimitExceeded { + limit_secs: u64, +} #[derive(Debug, Snafu)] pub enum RunPluginError { @@ -189,6 +201,7 @@ pub enum RunPluginError { PluginFailed { message: String }, } +#[allow(clippy::too_many_arguments)] pub fn run_plugin( wasm_path: Utf8PathBuf, base_dir: Utf8PathBuf, @@ -199,6 +212,7 @@ pub fn run_plugin( proxy: Option, identity_principal: Principal, environment: String, + compute_limit_secs: u64, stdio: Option>, ) -> Result, RunPluginError> { use wasmtime::component::{Component, Linker}; @@ -312,13 +326,16 @@ pub fn run_plugin( )?; let mut store = Store::new(&engine, host_state); - store.set_epoch_deadline(PLUGIN_COMPUTE_LIMIT_SECS); + store.set_epoch_deadline(compute_limit_secs); store.epoch_deadline_callback(move |_| { let extra = epoch_extension.swap(0, Ordering::Relaxed); if extra > 0 { Ok(wasmtime::UpdateDeadline::Continue(extra)) } else { - Err(ComputeTimeLimitExceeded.into()) + Err(ComputeTimeLimitExceeded { + limit_secs: compute_limit_secs, + } + .into()) } }); @@ -538,11 +555,25 @@ mod tests { None, anon(), "test".to_string(), + DEFAULT_PLUGIN_COMPUTE_LIMIT_SECS, None, ); assert!(matches!(result, Err(RunPluginError::LoadComponent { .. }))); } + #[test] + fn compute_time_limit_error_reflects_the_configured_limit() { + // The remediation must anchor to the actual limit (not a hardcoded + // literal), so it reads correctly whether the limit is the default or + // an env-var override. Use a distinctive value to catch a regression. + let msg = ComputeTimeLimitExceeded { limit_secs: 120 }.to_string(); + assert!(msg.contains("exceeded the 120s"), "got: {msg}"); + // The suggestion tells the user to go above the current limit — the + // value must flow into the remediation clause too. + assert!(msg.contains("above 120s"), "got: {msg}"); + assert!(msg.contains(PLUGIN_COMPUTE_LIMIT_ENV), "got: {msg}"); + } + // ------------------------------------------------------------------------- // Fixture-dependent tests // ------------------------------------------------------------------------- @@ -562,6 +593,7 @@ mod tests { None, anon(), "test".to_string(), + DEFAULT_PLUGIN_COMPUTE_LIMIT_SECS, None, ); assert!(matches!(result, Err(RunPluginError::PreopenDir { .. }))); @@ -589,6 +621,7 @@ mod tests { None, anon(), "test".to_string(), + DEFAULT_PLUGIN_COMPUTE_LIMIT_SECS, None, ); assert!(matches!(result, Err(RunPluginError::SymlinkDir { .. }))); @@ -609,6 +642,7 @@ mod tests { None, anon(), "test".to_string(), + DEFAULT_PLUGIN_COMPUTE_LIMIT_SECS, None, ); assert!(matches!(result, Err(RunPluginError::ReadFile { .. }))); @@ -636,6 +670,7 @@ mod tests { None, anon(), "test".to_string(), + DEFAULT_PLUGIN_COMPUTE_LIMIT_SECS, None, ); assert!(matches!(result, Err(RunPluginError::SymlinkFile { .. }))); @@ -656,6 +691,7 @@ mod tests { None, anon(), "ok".to_string(), + DEFAULT_PLUGIN_COMPUTE_LIMIT_SECS, None, ); assert!(result.is_ok()); @@ -676,6 +712,7 @@ mod tests { None, anon(), "error".to_string(), + DEFAULT_PLUGIN_COMPUTE_LIMIT_SECS, None, ); assert!(matches!( @@ -684,6 +721,41 @@ mod tests { )); } + #[test] + fn plugin_exceeding_compute_limit_is_trapped() { + let Some(wasm_path) = option_env!("TEST_PLUGIN_WASM") else { + return; + }; + // The "spin" fixture busy-loops forever; a 1-second limit keeps the + // test fast while still exercising the epoch-interruption trap. + let result = run_plugin( + wasm_path.into(), + ".".into(), + vec![], + vec![], + anon(), + dummy_agent(), + None, + anon(), + "spin".to_string(), + 1, + None, + ); + let err = result.expect_err("spinning plugin should hit the compute limit"); + // The trap surfaces through the CallExec source chain, so walk it and + // assert the message names both the limit and the override env var. + let mut chain = err.to_string(); + let mut cur: &dyn std::error::Error = &err; + while let Some(src) = cur.source() { + chain = format!("{chain}: {src}"); + cur = src; + } + assert!( + chain.contains("compute-time limit") && chain.contains(PLUGIN_COMPUTE_LIMIT_ENV), + "unexpected error chain: {chain}" + ); + } + #[tokio::test(flavor = "multi_thread")] async fn plugin_stdout_forwarded_through_stdio_channel() { let Some(wasm_path) = option_env!("TEST_PLUGIN_WASM") else { @@ -701,6 +773,7 @@ mod tests { None, anon(), "print".to_string(), + DEFAULT_PLUGIN_COMPUTE_LIMIT_SECS, Some(tx), ) }); @@ -726,6 +799,7 @@ mod tests { None, anon(), "hello".to_string(), + DEFAULT_PLUGIN_COMPUTE_LIMIT_SECS, Some(tx), ) }); diff --git a/crates/icp-sync-plugin/tests/fixtures/test-plugin/src/lib.rs b/crates/icp-sync-plugin/tests/fixtures/test-plugin/src/lib.rs index 622817f25..47b82c26a 100644 --- a/crates/icp-sync-plugin/tests/fixtures/test-plugin/src/lib.rs +++ b/crates/icp-sync-plugin/tests/fixtures/test-plugin/src/lib.rs @@ -19,6 +19,17 @@ impl Guest for TestPlugin { println!("stdout from plugin"); Ok(()) } + "spin" => { + // Busy-loop forever to exercise the host's compute-time limit. + // The epoch-interruption check at the loop back-edge traps this, + // so it never returns. `black_box` keeps the loop from being + // optimized away. + let mut x: u64 = 0; + loop { + x = x.wrapping_add(1); + std::hint::black_box(x); + } + } _ => Ok(()), } } diff --git a/crates/icp/src/canister/sync/plugin.rs b/crates/icp/src/canister/sync/plugin.rs index be7820013..97056d64d 100644 --- a/crates/icp/src/canister/sync/plugin.rs +++ b/crates/icp/src/canister/sync/plugin.rs @@ -1,7 +1,9 @@ use camino::Utf8PathBuf; use candid::Principal; use ic_agent::Agent; -use icp_sync_plugin::{RunPluginError, run_plugin}; +use icp_sync_plugin::{ + DEFAULT_PLUGIN_COMPUTE_LIMIT_SECS, PLUGIN_COMPUTE_LIMIT_ENV, RunPluginError, run_plugin, +}; use snafu::prelude::*; use tokio::sync::mpsc::Sender; @@ -17,10 +19,43 @@ pub enum PluginError { #[snafu(display("failed to get identity principal: {err}"))] GetIdentityPrincipal { err: String }, + #[snafu(display( + "invalid {PLUGIN_COMPUTE_LIMIT_ENV} value '{value}': expected a positive integer number of seconds" + ))] + InvalidComputeLimit { value: String }, + #[snafu(display("failed to run plugin"))] Run { source: RunPluginError }, } +/// Resolve the plugin compute-time limit, honoring the +/// [`PLUGIN_COMPUTE_LIMIT_ENV`] override. Fails loudly on a malformed value so +/// a typo doesn't silently fall back to the default and leave the caller +/// wondering why their raised limit had no effect. +fn resolve_compute_limit_secs() -> Result { + match std::env::var(PLUGIN_COMPUTE_LIMIT_ENV) { + Ok(value) => parse_compute_limit(&value), + // Only a genuinely unset variable selects the default. A variable that + // is present but not valid UTF-8 is a malformed value, not "unset", so + // it must be rejected to honor the fail-loudly contract. + Err(std::env::VarError::NotPresent) => Ok(DEFAULT_PLUGIN_COMPUTE_LIMIT_SECS), + Err(std::env::VarError::NotUnicode(raw)) => InvalidComputeLimitSnafu { + value: raw.to_string_lossy().into_owned(), + } + .fail(), + } +} + +fn parse_compute_limit(value: &str) -> Result { + match value.trim().parse::() { + Ok(secs) if secs >= 1 => Ok(secs), + _ => InvalidComputeLimitSnafu { + value: value.to_owned(), + } + .fail(), + } +} + pub(super) async fn sync( adapter: &Adapter, params: &Params, @@ -30,6 +65,11 @@ pub(super) async fn sync( stdio: Option>, pkg_cache: &PackageCache, ) -> Result, PluginError> { + // 0. Resolve the compute-time limit up front so a malformed + // ICP_CLI_PLUGIN_COMPUTE_LIMIT_SECS fails fast — before downloading the + // wasm or touching the network — rather than after doing that work. + let compute_limit_secs = resolve_compute_limit_secs()?; + // 1. Determine the on-disk path for the wasm. run_plugin needs a path, not raw bytes. // - Local: sha256 is verified if present, then the original path is returned. // - Remote: downloaded to cache (sha256 required, enforced at parse time) and the @@ -71,8 +111,33 @@ pub(super) async fn sync( proxy, identity_principal, environment_owned, + compute_limit_secs, stdio_clone, ) }) .context(RunSnafu) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parse_compute_limit_accepts_positive_integers() { + assert_eq!(parse_compute_limit("300").unwrap(), 300); + // Surrounding whitespace is tolerated. + assert_eq!(parse_compute_limit(" 42 ").unwrap(), 42); + } + + #[test] + fn parse_compute_limit_rejects_invalid_values() { + for bad in ["0", "abc", "30O", "-5", "1.5", ""] { + let err = + parse_compute_limit(bad).expect_err(&format!("expected '{bad}' to be rejected")); + assert!( + matches!(err, PluginError::InvalidComputeLimit { .. }), + "unexpected error for '{bad}': {err}" + ); + } + } +} diff --git a/docs/concepts/sync-plugins.md b/docs/concepts/sync-plugins.md index 09e7bc36e..b586ac0d5 100644 --- a/docs/concepts/sync-plugins.md +++ b/docs/concepts/sync-plugins.md @@ -119,11 +119,11 @@ The plugin runs with a deliberately narrow capability surface. | Resource | Limit | |----------|-------| | Wasm call-stack depth | 512 KiB | -| Pure compute time | 60 seconds | +| Pure compute time | 60 seconds (default) | | Linear memory | wasm32 address space (≤ 4 GiB) | | stdout / stderr per stream | 1 MiB | -The 60-second budget counts only wasm instruction execution. Time spent waiting for a `canister-call` to return over the network is **not** charged against it — the host grants that time back when the call completes. A plugin can make as many canister calls as it needs without the network latency eating into its compute limit. +The compute-time budget defaults to 60 seconds and is overridable with the [`ICP_CLI_PLUGIN_COMPUTE_LIMIT_SECS`](../reference/environment-variables.md#icp_cli_plugin_compute_limit_secs) environment variable — raise it for compute-heavy plugins (e.g. compressing a large asset bundle) that legitimately need more time, especially on slower CI runners. The budget counts only wasm instruction execution: time spent waiting for a `canister-call` to return over the network is **not** charged against it — the host grants that time back when the call completes. A plugin can make as many canister calls as it needs without the network latency eating into its compute limit. ## Next Steps diff --git a/docs/reference/environment-variables.md b/docs/reference/environment-variables.md index 0085b17b0..f225cb20c 100644 --- a/docs/reference/environment-variables.md +++ b/docs/reference/environment-variables.md @@ -139,6 +139,24 @@ export ICP_CLI_NETWORK_LAUNCHER_PATH=/path/to/icp-cli-network-launcher Download the launcher manually from [icp-cli-network-launcher releases](https://github.com/dfinity/icp-cli-network-launcher/releases). +### `ICP_CLI_PLUGIN_COMPUTE_LIMIT_SECS` + +Maximum seconds of pure WebAssembly compute a [sync plugin](../concepts/sync-plugins.md) may use during `icp sync`. Defaults to `60`. + +This is a runaway guard, not a security limit: sync plugins run locally in a read-only sandbox, so the limit only protects the machine running `icp sync` from a plugin that never terminates. Network/canister-call latency is already excluded from the budget, so the limit counts only time the plugin spends executing. + +Legitimately heavy plugins — for example, brotli-compressing a large asset bundle — can exceed the default, especially on slower CI runners where the same work takes more wall-clock time. Raise the limit if you hit `plugin exceeded the 60s compute-time limit`: + +```bash +export ICP_CLI_PLUGIN_COMPUTE_LIMIT_SECS=300 +``` + +An invalid value (non-integer or `0`) is rejected rather than silently ignored, so a typo can't leave you thinking you raised the limit when you didn't. + +**Use cases:** +- CI jobs syncing large asset bundles that trip the default limit +- Compression-heavy or otherwise compute-intensive sync plugins + ## Windows-Specific Variables ### `ICP_CLI_BASH_PATH`