From 77acb8a6d75a1ad2d0be2aee5320dacb84bc106d Mon Sep 17 00:00:00 2001 From: Raymond Khalife Date: Thu, 13 Aug 2026 08:46:20 +0000 Subject: [PATCH 1/2] refactor: split recipe resolution into fetch and render stages `canister::recipe::handlebars` did two unrelated jobs in one method: it fetched a recipe's Handlebars template (local read, HTTP download, package cache) and then rendered that template into build/sync steps. Rendering was therefore only reachable through something that owned an HTTP client and a package cache, and could not be tested without writing a temp file first. Split it in two: - `recipe::fetch::RecipeFetcher` (was `Handlebars`) retrieves the template text and nothing else, behind the `Resolve` seam, which now returns a `String` instead of `(BuildSteps, SyncSteps)`. - `recipe::render_recipe` is a pure function from template text to steps. Its five tests are plain `#[test]`s over string inputs; the four they replace each needed a tempdir, a package cache and a tokio runtime. Two behavioural notes: - A rendered document that is not a valid build/sync manifest was a `panic!`; it is now `RenderRecipeError::Parse`. The rendered YAML moves to a `debug!` line, so `--debug` still shows it. - A remote template is now cached once its checksum verifies, rather than after it also renders. The checksum is what establishes the bytes are the bytes we asked for, and rendering depends on per-canister context that has nothing to do with the template's cacheability. `ConsolidateManifestError::Recipe` splits into `FetchRecipe` and `RenderRecipe` accordingly, so a failure names the stage that failed. Salvaged from PR 660, originally authored by Adam Spofford, re-executed on current `main`: that branch introduced this split as part of a larger crate reorganization, and the split stands on its own. --- crates/icp/src/canister/recipe/fetch.rs | 306 ++++++++++ crates/icp/src/canister/recipe/handlebars.rs | 559 ------------------- crates/icp/src/canister/recipe/mod.rs | 65 +-- crates/icp/src/canister/recipe/render.rs | 260 +++++++++ crates/icp/src/context/init.rs | 4 +- crates/icp/src/lib.rs | 38 +- crates/icp/src/project.rs | 36 +- 7 files changed, 623 insertions(+), 645 deletions(-) create mode 100644 crates/icp/src/canister/recipe/fetch.rs delete mode 100644 crates/icp/src/canister/recipe/handlebars.rs create mode 100644 crates/icp/src/canister/recipe/render.rs diff --git a/crates/icp/src/canister/recipe/fetch.rs b/crates/icp/src/canister/recipe/fetch.rs new file mode 100644 index 000000000..d90345218 --- /dev/null +++ b/crates/icp/src/canister/recipe/fetch.rs @@ -0,0 +1,306 @@ +//! Stage one of recipe resolution: get the template text. + +use std::{str::FromStr, string::FromUtf8Error}; + +use async_trait::async_trait; +use reqwest::{Method, Request, Url}; +use sha2::{Digest, Sha256}; +use snafu::prelude::*; +use tracing::debug; +use url::ParseError; + +use crate::{ + fs::read, + manifest::recipe::{Recipe, RecipeType}, + package::{ + PackageCache, cache_registry_recipe, cache_uri_recipe, read_cached_registry_recipe, + read_cached_uri_recipe, + }, + prelude::*, +}; + +use super::{FetchSnafu, Resolve, ResolveError}; + +/// Fetches recipe templates over HTTP, caching downloads in the package cache. +/// Template *rendering* is a separate stage +/// ([`render_recipe`](super::render_recipe)); this only produces the raw template +/// text. +pub struct RecipeFetcher { + /// Http client for fetching remote recipe templates + pub http_client: reqwest::Client, + /// Package cache for caching downloaded recipe templates + pub pkg_cache: PackageCache, +} + +enum TemplateSource { + LocalPath(PathBuf), + RemoteUrl(String), + + /// Template originating in a remote registry, e.g `@dfinity/rust@v1.0.2` + Registry(String, String, String), +} + +#[derive(Debug, Snafu)] +pub enum RecipeFetchError { + #[snafu(display("failed to read local recipe template file"))] + ReadFile { source: crate::fs::IoError }, + + #[snafu(display("failed to decode UTF-8 string"))] + DecodeUtf8 { source: FromUtf8Error }, + + #[snafu(display("failed to parse user-provided url"))] + UrlParse { source: ParseError }, + + #[snafu(display("failed to execute http request"))] + HttpRequest { source: reqwest::Error }, + + #[snafu(display("request to '{url}' returned '{status}' status-code"))] + HttpStatus { url: String, status: u16 }, + + #[snafu(display( + "sha256 checksum mismatch for recipe template: expected {expected}, actual {actual}" + ))] + ChecksumMismatch { expected: String, actual: String }, + + #[snafu(display("failed to read cached recipe template"))] + ReadCache { + source: crate::package::RecipeCacheError, + }, + + #[snafu(display("failed to cache recipe template"))] + CacheRecipe { + source: crate::package::RecipeCacheError, + }, + + #[snafu(display("failed to acquire lock on package cache"))] + LockCache { source: crate::fs::lock::LockError }, +} + +impl RecipeFetcher { + /// Fetch a recipe's Handlebars template text: read a local file, or fetch + /// (and cache) a remote URL or registry recipe. Verifies `sha256` when set. + async fn fetch_recipe(&self, recipe: &Recipe) -> Result { + // Determine the template source + let tmpl_source = match &recipe.recipe_type { + RecipeType::File(path) => TemplateSource::LocalPath(Path::new(&path).into()), + RecipeType::Url(url) => TemplateSource::RemoteUrl(url.to_owned()), + RecipeType::Registry { + name, + recipe, + version, + } => TemplateSource::Registry(name.to_owned(), recipe.to_owned(), version.to_owned()), + }; + + // Retrieve the template, using cache for remote/registry sources + let (tmpl, should_cache) = match &tmpl_source { + TemplateSource::LocalPath(path) => { + let bytes = read(path).context(ReadFileSnafu)?; + (parse_bytes_to_string(bytes)?, false) + } + + TemplateSource::RemoteUrl(u) => { + // Check cache + let maybe_cached = self + .pkg_cache + .with_read(async |r| { + read_cached_uri_recipe(r, u, recipe.sha256.as_deref()) + .context(ReadCacheSnafu) + }) + .await + .context(LockCacheSnafu)?; + if let Some(cached) = maybe_cached? { + debug!("Using cached recipe template for {u}"); + (parse_bytes_to_string(cached)?, false) + } else { + // Download the template + let tmpl = self.fetch_remote_bytes(u).await?; + (parse_bytes_to_string(tmpl)?, true) + } + } + + // TMP(or.ricon): Temporarily hardcode a dfinity registry + TemplateSource::Registry(registry, recipe_name, version) => { + if registry != "dfinity" { + panic!("only the dfinity registry is currently supported"); + } + + let package = format!("@{registry}/{recipe_name}"); + let release_tag = format!("{recipe_name}-{version}"); + + // Check cache + let maybe_cached = self + .pkg_cache + .with_read(async |r| { + read_cached_registry_recipe(r, &package, version).context(ReadCacheSnafu) + }) + .await + .context(LockCacheSnafu)?; + if let Some(cached) = maybe_cached? { + debug!("Using cached recipe template for {package}@{version}"); + (parse_bytes_to_string(cached)?, false) + } else { + // Download the template + let url = format!( + "https://github.com/dfinity/icp-cli-recipes/releases/download/{release_tag}/recipe.hbs" + ); + let bytes = self.fetch_remote_bytes(&url).await?; + + (parse_bytes_to_string(bytes)?, true) + } + } + }; + + let hash = if let Some(sha256) = &recipe.sha256 { + verify_checksum(tmpl.as_bytes(), sha256)? + } else { + Sha256::digest(tmpl.as_bytes()).into() + }; + + // Cache the fetched template if it was remote. Rendering happens after + // this stage, so a template that renders badly is still cached — the + // checksum above is what decides whether the bytes are trustworthy. + if should_cache { + match tmpl_source { + TemplateSource::LocalPath(_) => unreachable!("local files are never cached"), + TemplateSource::RemoteUrl(u) => { + self.pkg_cache + .with_write(async |w| { + cache_uri_recipe(w, &u, &hex::encode(hash), tmpl.as_bytes()) + .context(CacheRecipeSnafu)?; + Ok(()) + }) + .await + .context(LockCacheSnafu)??; + } + TemplateSource::Registry(registry, recipe_name, version) => { + let package = format!("@{registry}/{recipe_name}"); + self.pkg_cache + .with_write(async |w| { + cache_registry_recipe( + w, + &package, + &version, + &hex::encode(hash), + tmpl.as_bytes(), + ) + .context(CacheRecipeSnafu) + }) + .await + .context(LockCacheSnafu)??; + } + } + } + Ok(tmpl) + } + + /// Fetch raw bytes from a remote URL. + async fn fetch_remote_bytes(&self, url: &str) -> Result, RecipeFetchError> { + let u = Url::from_str(url).context(UrlParseSnafu)?; + debug!("Requesting template from: {u}"); + + let resp = self + .http_client + .execute(Request::new(Method::GET, u.clone())) + .await + .context(HttpRequestSnafu)?; + + if !resp.status().is_success() { + return HttpStatusSnafu { + url: u.to_string(), + status: resp.status().as_u16(), + } + .fail(); + } + + Ok(resp.bytes().await.context(HttpRequestSnafu)?.to_vec()) + } +} + +#[async_trait] +impl Resolve for RecipeFetcher { + async fn resolve(&self, recipe: &Recipe) -> Result { + self.fetch_recipe(recipe).await.context(FetchSnafu) + } +} + +/// Helper function to verify sha256 checksum of recipe template bytes +fn verify_checksum(bytes: &[u8], expected: &str) -> Result<[u8; 32], RecipeFetchError> { + let actual_hash = { + let mut h = Sha256::new(); + h.update(bytes); + h.finalize() + }; + let actual = hex::encode(actual_hash); + if actual != expected { + return ChecksumMismatchSnafu { + expected: expected.to_string(), + actual, + } + .fail(); + } + Ok(actual_hash.into()) +} + +/// Helper function to parse bytes into a UTF-8 string +fn parse_bytes_to_string(bytes: Vec) -> Result { + String::from_utf8(bytes).context(DecodeUtf8Snafu) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::manifest::recipe::{Recipe, RecipeType}; + + fn fetcher(cache_dir: &Path) -> RecipeFetcher { + RecipeFetcher { + http_client: reqwest::Client::new(), + pkg_cache: PackageCache::new(cache_dir.to_owned()).unwrap(), + } + } + + /// A local recipe file is read back verbatim — rendering is a later stage, so + /// the fetched text still contains its unexpanded `{{...}}` expressions. + #[tokio::test] + async fn local_recipe_is_read_verbatim() { + let tmp = camino_tempfile::Utf8TempDir::new().unwrap(); + let tmpl_path = tmp.path().join("recipe.hbs"); + let body = indoc::indoc! {r#" + build: + steps: + - type: script + command: "build {{_.canister.name}}" + "#}; + std::fs::write(&tmpl_path, body).unwrap(); + + let recipe = Recipe { + recipe_type: RecipeType::File(tmpl_path.to_string()), + configuration: Default::default(), + sha256: None, + }; + + let fetched = fetcher(&tmp.path().join("pkg")) + .fetch_recipe(&recipe) + .await + .unwrap(); + assert_eq!(fetched, body); + } + + /// A sha256 that does not match the template contents is rejected. + #[tokio::test] + async fn checksum_mismatch_is_rejected() { + let tmp = camino_tempfile::Utf8TempDir::new().unwrap(); + let tmpl_path = tmp.path().join("recipe.hbs"); + std::fs::write(&tmpl_path, "build:\n steps: []\n").unwrap(); + + let recipe = Recipe { + recipe_type: RecipeType::File(tmpl_path.to_string()), + configuration: Default::default(), + sha256: Some("00".repeat(32)), + }; + + assert!(matches!( + fetcher(&tmp.path().join("pkg")).fetch_recipe(&recipe).await, + Err(RecipeFetchError::ChecksumMismatch { .. }) + )); + } +} diff --git a/crates/icp/src/canister/recipe/handlebars.rs b/crates/icp/src/canister/recipe/handlebars.rs deleted file mode 100644 index 11c7c59ba..000000000 --- a/crates/icp/src/canister/recipe/handlebars.rs +++ /dev/null @@ -1,559 +0,0 @@ -use std::{str::FromStr, string::FromUtf8Error}; - -use async_trait::async_trait; -use handlebars::{Context, Helper, HelperDef, HelperResult, Output}; -use indoc::formatdoc; -use reqwest::{Method, Request, Url}; -use serde::Deserialize; -use sha2::{Digest, Sha256}; -use snafu::prelude::*; -use tracing::debug; -use url::ParseError; - -use crate::{ - fs::read, - manifest::{ - canister::{BuildSteps, SyncSteps}, - recipe::{Recipe, RecipeType}, - }, - package::{ - PackageCache, cache_registry_recipe, cache_uri_recipe, read_cached_registry_recipe, - read_cached_uri_recipe, - }, - prelude::*, -}; - -use super::{Resolve, ResolveError}; - -pub struct Handlebars { - /// Http client for fetching remote recipe templates - pub http_client: reqwest::Client, - /// Package cache for caching downloaded recipe templates - pub pkg_cache: PackageCache, -} - -pub enum TemplateSource { - LocalPath(PathBuf), - RemoteUrl(String), - - /// Template originating in a remote registry, e.g `@dfinity/rust@v1.0.2` - Registry(String, String, String), -} - -#[derive(Debug, Snafu)] -pub enum HandlebarsError { - #[snafu(display("failed to read local recipe template file"))] - ReadFile { source: crate::fs::IoError }, - - #[snafu(display("failed to decode UTF-8 string"))] - DecodeUtf8 { source: FromUtf8Error }, - - #[snafu(display("failed to parse user-provided url"))] - UrlParse { source: ParseError }, - - #[snafu(display("failed to execute http request"))] - HttpRequest { source: reqwest::Error }, - - #[snafu(display("request to '{url}' returned '{status}' status-code"))] - HttpStatus { url: String, status: u16 }, - - #[snafu(display("the recipe template for recipe type '{recipe}' failed to be rendered"))] - Render { - source: handlebars::RenderError, - recipe: String, - template: String, - }, - - #[snafu(display( - "sha256 checksum mismatch for recipe template: expected {expected}, actual {actual}" - ))] - ChecksumMismatch { expected: String, actual: String }, - - #[snafu(display("failed to read cached recipe template"))] - ReadCache { - source: crate::package::RecipeCacheError, - }, - - #[snafu(display("failed to cache recipe template"))] - CacheRecipe { - source: crate::package::RecipeCacheError, - }, - - #[snafu(display("failed to acquire lock on package cache"))] - LockCache { source: crate::fs::lock::LockError }, -} - -impl Handlebars { - async fn resolve_impl( - &self, - recipe: &Recipe, - recipe_context: &super::RecipeContext, - ) -> Result<(BuildSteps, SyncSteps), HandlebarsError> { - // Determine the template source - let tmpl_source = match &recipe.recipe_type { - RecipeType::File(path) => TemplateSource::LocalPath(Path::new(&path).into()), - RecipeType::Url(url) => TemplateSource::RemoteUrl(url.to_owned()), - RecipeType::Registry { - name, - recipe, - version, - } => TemplateSource::Registry(name.to_owned(), recipe.to_owned(), version.to_owned()), - }; - - // Retrieve the template, using cache for remote/registry sources - let (tmpl, should_cache) = match &tmpl_source { - TemplateSource::LocalPath(path) => { - let bytes = read(path).context(ReadFileSnafu)?; - (parse_bytes_to_string(bytes)?, false) - } - - TemplateSource::RemoteUrl(u) => { - // Check cache - let maybe_cached = self - .pkg_cache - .with_read(async |r| { - read_cached_uri_recipe(r, u, recipe.sha256.as_deref()) - .context(ReadCacheSnafu) - }) - .await - .context(LockCacheSnafu)?; - if let Some(cached) = maybe_cached? { - debug!("Using cached recipe template for {u}"); - (parse_bytes_to_string(cached)?, false) - } else { - // Download the template - let tmpl = self.fetch_remote_bytes(u).await?; - (parse_bytes_to_string(tmpl)?, true) - } - } - - // TMP(or.ricon): Temporarily hardcode a dfinity registry - TemplateSource::Registry(registry, recipe_name, version) => { - if registry != "dfinity" { - panic!("only the dfinity registry is currently supported"); - } - - let package = format!("@{registry}/{recipe_name}"); - let release_tag = format!("{recipe_name}-{version}"); - - // Check cache - let maybe_cached = self - .pkg_cache - .with_read(async |r| { - read_cached_registry_recipe(r, &package, version).context(ReadCacheSnafu) - }) - .await - .context(LockCacheSnafu)?; - if let Some(cached) = maybe_cached? { - debug!("Using cached recipe template for {package}@{version}"); - (parse_bytes_to_string(cached)?, false) - } else { - // Download the template - let url = format!( - "https://github.com/dfinity/icp-cli-recipes/releases/download/{release_tag}/recipe.hbs" - ); - let bytes = self.fetch_remote_bytes(&url).await?; - - (parse_bytes_to_string(bytes)?, true) - } - } - }; - - let hash = if let Some(sha256) = &recipe.sha256 { - verify_checksum(tmpl.as_bytes(), sha256)? - } else { - Sha256::digest(tmpl.as_bytes()).into() - }; - - // Load the template via handlebars - let mut reg = handlebars::Handlebars::new(); - - // Disable HTML escaping since the output is YAML, not HTML - reg.register_escape_fn(handlebars::no_escape); - - // Register helpers - reg.register_helper("replace", Box::new(ReplaceHelper)); - - // Reject unset template variables - reg.set_strict_mode(true); - - debug!( - "{}", - formatdoc! {r#" - Loaded template: - ------ - {tmpl} - ------ - "#} - ); - - // Build render context: user-provided configuration plus injected _.* variables. - // The _ key is reserved and always overrides any user-supplied value. - let mut render_context = recipe.configuration.clone(); - render_context.insert("_".to_string(), recipe_context.to_yaml()); - - // Render the template to YAML - let out = reg - .render_template(&tmpl, &render_context) - .context(RenderSnafu { - recipe: recipe.recipe_type.clone(), - template: tmpl.to_owned(), - })?; - - // Read the rendered YAML canister manifest - // Recipes can only render build/sync - #[derive(Deserialize)] - struct BuildSyncHelper { - build: BuildSteps, - #[serde(default)] - sync: SyncSteps, - } - - let insts = serde_yaml::from_str::(&out); - let (build, sync) = match insts { - Ok(helper) => (helper.build, helper.sync), - Err(e) => panic!( - "{}", - formatdoc! {r#" - Unable to render recipe {} template into valid yaml: {e} - - Rendered content: - ------ - {out} - ------ - "#, recipe.recipe_type} - ), - }; - - // The template is verified good - now cache it if it was remote - if should_cache { - match tmpl_source { - TemplateSource::LocalPath(_) => unreachable!("local files are never cached"), - TemplateSource::RemoteUrl(u) => { - self.pkg_cache - .with_write(async |w| { - cache_uri_recipe(w, &u, &hex::encode(hash), tmpl.as_bytes()) - .context(CacheRecipeSnafu)?; - Ok(()) - }) - .await - .context(LockCacheSnafu)??; - } - TemplateSource::Registry(registry, recipe_name, version) => { - let package = format!("@{registry}/{recipe_name}"); - self.pkg_cache - .with_write(async |w| { - cache_registry_recipe( - w, - &package, - &version, - &hex::encode(hash), - tmpl.as_bytes(), - ) - .context(CacheRecipeSnafu) - }) - .await - .context(LockCacheSnafu)??; - } - } - } - Ok((build, sync)) - } - - /// Fetch raw bytes from a remote URL. - async fn fetch_remote_bytes(&self, url: &str) -> Result, HandlebarsError> { - let u = Url::from_str(url).context(UrlParseSnafu)?; - debug!("Requesting template from: {u}"); - - let resp = self - .http_client - .execute(Request::new(Method::GET, u.clone())) - .await - .context(HttpRequestSnafu)?; - - if !resp.status().is_success() { - return HttpStatusSnafu { - url: u.to_string(), - status: resp.status().as_u16(), - } - .fail(); - } - - Ok(resp.bytes().await.context(HttpRequestSnafu)?.to_vec()) - } -} - -#[async_trait] -impl Resolve for Handlebars { - async fn resolve( - &self, - recipe: &Recipe, - recipe_context: &super::RecipeContext, - ) -> Result<(BuildSteps, SyncSteps), ResolveError> { - self.resolve_impl(recipe, recipe_context) - .await - .context(super::HandlebarsSnafu) - } -} - -/// Handlebars helper for string replacement operations -/// Usage: {{ replace "from" "to" value }} -#[derive(Clone, Copy)] -struct ReplaceHelper; - -impl HelperDef for ReplaceHelper { - fn call<'reg: 'rc, 'rc>( - &self, - h: &Helper, - _: &'reg handlebars::Handlebars<'reg>, - _: &Context, - _: &mut handlebars::RenderContext<'reg, 'rc>, - out: &mut dyn Output, - ) -> HelperResult { - let (from, to) = ( - h.param(0).unwrap().render(), // from - h.param(1).unwrap().render(), // to - ); - - let v = h.param(2).unwrap().render(); - out.write(&v.replace(&from, &to))?; - - Ok(()) - } -} - -/// Helper function to verify sha256 checksum of recipe template bytes -fn verify_checksum(bytes: &[u8], expected: &str) -> Result<[u8; 32], HandlebarsError> { - let actual_hash = { - let mut h = Sha256::new(); - h.update(bytes); - h.finalize() - }; - let actual = hex::encode(actual_hash); - if actual != expected { - return ChecksumMismatchSnafu { - expected: expected.to_string(), - actual, - } - .fail(); - } - Ok(actual_hash.into()) -} - -/// Helper function to parse bytes into a UTF-8 string -fn parse_bytes_to_string(bytes: Vec) -> Result { - String::from_utf8(bytes).context(DecodeUtf8Snafu) -} - -#[cfg(test)] -mod tests { - use super::*; - use crate::canister::recipe::RecipeContext; - use crate::manifest::recipe::{Recipe, RecipeType}; - use std::collections::HashMap; - - fn recipe_context(canister_name: &str) -> RecipeContext { - RecipeContext { - canister_name: canister_name.to_string(), - } - } - - #[tokio::test] - async fn template_values_are_not_html_escaped() { - // Create a recipe template that interpolates a value containing - // characters that Handlebars would normally HTML-escape (", =, &, <, >). - // Use double-stache {{ }} which is what real recipe templates use. - let tmp = camino_tempfile::Utf8TempDir::new().unwrap(); - let tmpl_path = tmp.path().join("recipe.hbs"); - std::fs::write( - &tmpl_path, - indoc::indoc! {r#" - build: - steps: - - type: script - command: "{{ command }}" - "#}, - ) - .unwrap(); - - let cache_dir = tmp.path().join("pkg"); - let pkg_cache = PackageCache::new(cache_dir).unwrap(); - let hbs = Handlebars { - http_client: reqwest::Client::new(), - pkg_cache, - }; - - let mut configuration = HashMap::new(); - configuration.insert( - "command".to_string(), - serde_yaml::Value::String("SITE=https://example.com&foo=bar npm run build".to_string()), - ); - - let recipe = Recipe { - recipe_type: RecipeType::File(tmpl_path.to_string()), - configuration, - sha256: None, - }; - - let (build, _sync) = hbs - .resolve_impl(&recipe, &recipe_context("my-canister")) - .await - .unwrap(); - let cmd = build.steps[0].clone(); - - match cmd { - crate::manifest::canister::BuildStep::Script(adapter) => { - let commands = adapter.command.as_vec(); - assert_eq!( - commands[0], "SITE=https://example.com&foo=bar npm run build", - "Template values must not be HTML-escaped (= and & must be preserved)" - ); - } - other => panic!("Expected Script build step, got: {other:?}"), - } - } - - #[tokio::test] - async fn canister_name_is_injected() { - let tmp = camino_tempfile::Utf8TempDir::new().unwrap(); - let tmpl_path = tmp.path().join("recipe.hbs"); - std::fs::write( - &tmpl_path, - indoc::indoc! {r#" - build: - steps: - - type: script - command: "build {{_.canister.name}}" - "#}, - ) - .unwrap(); - - let pkg_cache = PackageCache::new(tmp.path().join("pkg")).unwrap(); - let hbs = Handlebars { - http_client: reqwest::Client::new(), - pkg_cache, - }; - - let recipe = Recipe { - recipe_type: RecipeType::File(tmpl_path.to_string()), - configuration: HashMap::new(), - sha256: None, - }; - - let (build, _sync) = hbs - .resolve_impl(&recipe, &recipe_context("my-canister")) - .await - .unwrap(); - - match build.steps[0].clone() { - crate::manifest::canister::BuildStep::Script(adapter) => { - assert_eq!(adapter.command.as_vec()[0], "build my-canister"); - } - other => panic!("Expected Script build step, got: {other:?}"), - } - } - - #[tokio::test] - async fn canister_name_works_with_replace_helper() { - let tmp = camino_tempfile::Utf8TempDir::new().unwrap(); - let tmpl_path = tmp.path().join("recipe.hbs"); - std::fs::write( - &tmpl_path, - indoc::indoc! {r#" - build: - steps: - - type: script - command: "cp {{ replace "-" "_" _.canister.name }}.wasm out.wasm" - "#}, - ) - .unwrap(); - - let pkg_cache = PackageCache::new(tmp.path().join("pkg")).unwrap(); - let hbs = Handlebars { - http_client: reqwest::Client::new(), - pkg_cache, - }; - - let recipe = Recipe { - recipe_type: RecipeType::File(tmpl_path.to_string()), - configuration: HashMap::new(), - sha256: None, - }; - - let (build, _sync) = hbs - .resolve_impl(&recipe, &recipe_context("my-canister")) - .await - .unwrap(); - - match build.steps[0].clone() { - crate::manifest::canister::BuildStep::Script(adapter) => { - assert_eq!(adapter.command.as_vec()[0], "cp my_canister.wasm out.wasm"); - } - other => panic!("Expected Script build step, got: {other:?}"), - } - } - - #[tokio::test] - async fn reserved_namespace_cannot_be_overridden_by_user_config() { - let tmp = camino_tempfile::Utf8TempDir::new().unwrap(); - let tmpl_path = tmp.path().join("recipe.hbs"); - std::fs::write( - &tmpl_path, - indoc::indoc! {r#" - build: - steps: - - type: script - command: "build {{_.canister.name}}" - "#}, - ) - .unwrap(); - - let pkg_cache = PackageCache::new(tmp.path().join("pkg")).unwrap(); - let hbs = Handlebars { - http_client: reqwest::Client::new(), - pkg_cache, - }; - - let mut configuration = HashMap::new(); - configuration.insert( - "_".to_string(), - serde_yaml::Value::Mapping({ - let mut m = serde_yaml::Mapping::new(); - m.insert( - serde_yaml::Value::String("canister".to_string()), - serde_yaml::Value::Mapping({ - let mut inner = serde_yaml::Mapping::new(); - inner.insert( - serde_yaml::Value::String("name".to_string()), - serde_yaml::Value::String("user-override".to_string()), - ); - inner - }), - ); - m - }), - ); - - let recipe = Recipe { - recipe_type: RecipeType::File(tmpl_path.to_string()), - configuration, - sha256: None, - }; - - let (build, _sync) = hbs - .resolve_impl(&recipe, &recipe_context("real-name")) - .await - .unwrap(); - - match build.steps[0].clone() { - crate::manifest::canister::BuildStep::Script(adapter) => { - assert_eq!( - adapter.command.as_vec()[0], - "build real-name", - "_ namespace must not be overridable by user configuration" - ); - } - other => panic!("Expected Script build step, got: {other:?}"), - } - } -} diff --git a/crates/icp/src/canister/recipe/mod.rs b/crates/icp/src/canister/recipe/mod.rs index 9c43424ef..7a2bafb9c 100644 --- a/crates/icp/src/canister/recipe/mod.rs +++ b/crates/icp/src/canister/recipe/mod.rs @@ -1,55 +1,36 @@ +//! Recipe resolution, split into two stages. +//! +//! [`fetch`] retrieves a recipe's Handlebars template — reading a local file, or +//! downloading and caching a remote URL or registry recipe — and returns the raw +//! template text. [`render`] turns that text into concrete build/sync steps. The +//! first stage does I/O and nothing else; the second is a pure function. +//! +//! The [`Resolve`] seam therefore covers only the fetching half, so a caller that +//! already has a template (or must not touch the network) can render without +//! going through a resolver at all. + use async_trait::async_trait; use snafu::prelude::*; -use crate::manifest::{ - canister::{BuildSteps, SyncSteps}, - recipe::Recipe, -}; - -pub mod handlebars; - -/// Context passed to a recipe resolver, describing the canister being built. -/// -/// Serializes to the shape injected into recipe templates under the `_` namespace: -/// -/// ```yaml -/// canister: -/// name: -/// ``` -pub struct RecipeContext { - pub canister_name: String, -} - -impl RecipeContext { - /// Builds the YAML value injected into recipe templates under the `_` namespace. - /// Constructing the mapping directly is infallible, unlike `serde` serialization. - pub fn to_yaml(&self) -> serde_yaml::Value { - use serde_yaml::{Mapping, Value}; +use crate::manifest::recipe::Recipe; - let mut canister = Mapping::new(); - canister.insert("name".into(), Value::String(self.canister_name.clone())); +pub mod fetch; +pub mod render; - let mut root = Mapping::new(); - root.insert("canister".into(), Value::Mapping(canister)); +pub use render::{RecipeContext, RenderRecipeError, render_recipe}; - Value::Mapping(root) - } -} - -/// A recipe resolver takes a recipe that is specified in a canister manifest -/// and resolves it into a set of build/sync steps +/// Retrieves the recipe templates a project references. +/// +/// Only *fetching* is behind this trait: rendering a fetched template into build +/// and sync steps is [`render_recipe`], which needs no I/O and so needs no seam. #[async_trait] pub trait Resolve: Sync + Send { - #[allow(clippy::result_large_err)] - async fn resolve( - &self, - recipe: &Recipe, - recipe_context: &RecipeContext, - ) -> Result<(BuildSteps, SyncSteps), ResolveError>; + /// Fetch the Handlebars template for `recipe`, returning its raw source. + async fn resolve(&self, recipe: &Recipe) -> Result; } #[derive(Debug, Snafu)] pub enum ResolveError { - #[snafu(display("failed to resolve handlebars template"))] - Handlebars { source: handlebars::HandlebarsError }, + #[snafu(display("failed to fetch recipe template"))] + Fetch { source: fetch::RecipeFetchError }, } diff --git a/crates/icp/src/canister/recipe/render.rs b/crates/icp/src/canister/recipe/render.rs new file mode 100644 index 000000000..0bd66b8ff --- /dev/null +++ b/crates/icp/src/canister/recipe/render.rs @@ -0,0 +1,260 @@ +//! Stage two of recipe resolution: turn template text into build/sync steps. + +use std::collections::HashMap; + +use handlebars::{Context, Handlebars, Helper, HelperDef, HelperResult, Output, RenderContext}; +use serde::Deserialize; +use snafu::prelude::*; +use tracing::debug; + +use crate::manifest::{ + canister::{BuildSteps, SyncSteps}, + recipe::{Recipe, RecipeType}, +}; + +/// Context passed to a recipe resolver, describing the canister being built. +/// +/// Serializes to the shape injected into recipe templates under the `_` namespace: +/// +/// ```yaml +/// canister: +/// name: +/// ``` +pub struct RecipeContext { + pub canister_name: String, +} + +impl RecipeContext { + /// Builds the YAML value injected into recipe templates under the `_` namespace. + /// Constructing the mapping directly is infallible, unlike `serde` serialization. + pub fn to_yaml(&self) -> serde_yaml::Value { + use serde_yaml::{Mapping, Value}; + + let mut canister = Mapping::new(); + canister.insert("name".into(), Value::String(self.canister_name.clone())); + + let mut root = Mapping::new(); + root.insert("canister".into(), Value::Mapping(canister)); + + Value::Mapping(root) + } +} + +#[derive(Debug, Snafu)] +pub enum RenderRecipeError { + #[snafu(display("recipe template for '{recipe}' failed to render"))] + Render { + // Boxed to keep `Result<_, RenderRecipeError>` small; `RenderError` + // alone is well over a hundred bytes. + #[snafu(source(from(handlebars::RenderError, Box::new)))] + source: Box, + recipe: RecipeType, + }, + + #[snafu(display("recipe '{recipe}' did not render into a valid build/sync manifest"))] + Parse { + source: serde_yaml::Error, + recipe: RecipeType, + }, +} + +/// Render a recipe's Handlebars `template` into concrete build/sync steps. +/// +/// The template is rendered with the recipe's `configuration` plus the reserved +/// `_` namespace (the `_` key always overrides any user-supplied value), then the +/// resulting YAML is parsed. A recipe may only produce `build` and `sync`. +pub fn render_recipe( + template: &str, + recipe: &Recipe, + recipe_context: &RecipeContext, +) -> Result<(BuildSteps, SyncSteps), RenderRecipeError> { + let mut reg = Handlebars::new(); + // The output is YAML, not HTML, so disable HTML escaping. + reg.register_escape_fn(handlebars::no_escape); + reg.register_helper("replace", Box::new(ReplaceHelper)); + // Reject unset template variables. + reg.set_strict_mode(true); + + // User-provided configuration plus the injected `_.*` variables. The `_` key + // is reserved and always overrides any user-supplied value. + let mut render_context: HashMap = recipe.configuration.clone(); + render_context.insert("_".to_string(), recipe_context.to_yaml()); + + debug!("Rendering recipe template:\n------\n{template}\n------"); + + let out = reg + .render_template(template, &render_context) + .context(RenderSnafu { + recipe: recipe.recipe_type.clone(), + })?; + + // Logged rather than carried in `Parse` below: a recipe author debugging a + // malformed render needs the whole document, which is too much for an error + // message. + debug!("Rendered recipe template:\n------\n{out}\n------"); + + // Recipes can only render `build`/`sync`. + #[derive(Deserialize)] + struct BuildSyncHelper { + build: BuildSteps, + #[serde(default)] + sync: SyncSteps, + } + + let helper: BuildSyncHelper = serde_yaml::from_str(&out).context(ParseSnafu { + recipe: recipe.recipe_type.clone(), + })?; + Ok((helper.build, helper.sync)) +} + +/// Handlebars helper for string replacement operations. +/// Usage: `{{ replace "from" "to" value }}` +#[derive(Clone, Copy)] +struct ReplaceHelper; + +impl HelperDef for ReplaceHelper { + fn call<'reg: 'rc, 'rc>( + &self, + h: &Helper, + _: &'reg Handlebars<'reg>, + _: &Context, + _: &mut RenderContext<'reg, 'rc>, + out: &mut dyn Output, + ) -> HelperResult { + let (from, to) = ( + h.param(0).unwrap().render(), // from + h.param(1).unwrap().render(), // to + ); + + let v = h.param(2).unwrap().render(); + out.write(&v.replace(&from, &to))?; + + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::manifest::canister::BuildStep; + + fn recipe(config: &[(&str, &str)]) -> Recipe { + Recipe { + recipe_type: RecipeType::File("recipe.hbs".to_owned()), + configuration: config + .iter() + .map(|(k, v)| ((*k).to_owned(), serde_yaml::Value::String((*v).to_owned()))) + .collect(), + sha256: None, + } + } + + fn ctx(name: &str) -> RecipeContext { + RecipeContext { + canister_name: name.to_owned(), + } + } + + /// The only build step's command, for a recipe that renders a single script step. + fn rendered_command(template: &str, recipe: &Recipe, context: &RecipeContext) -> String { + let (build, _sync) = render_recipe(template, recipe, context).unwrap(); + match &build.steps[0] { + BuildStep::Script(adapter) => adapter.command.as_vec()[0].clone(), + other => panic!("expected a script build step, got {other:?}"), + } + } + + /// Interpolated values are not HTML-escaped (the output is YAML): `=` and `&` + /// must survive. + #[test] + fn template_values_are_not_html_escaped() { + let template = indoc::indoc! {r#" + build: + steps: + - type: script + command: "{{ command }}" + "#}; + let r = recipe(&[("command", "SITE=https://example.com&foo=bar npm run build")]); + assert_eq!( + rendered_command(template, &r, &ctx("my-canister")), + "SITE=https://example.com&foo=bar npm run build" + ); + } + + /// The canister name is injected under the reserved `_` namespace. + #[test] + fn canister_name_is_injected() { + let template = indoc::indoc! {r#" + build: + steps: + - type: script + command: "build {{_.canister.name}}" + "#}; + assert_eq!( + rendered_command(template, &recipe(&[]), &ctx("my-canister")), + "build my-canister" + ); + } + + /// The `_` namespace works through the `replace` helper. + #[test] + fn canister_name_works_with_replace_helper() { + let template = indoc::indoc! {r#" + build: + steps: + - type: script + command: "cp {{ replace "-" "_" _.canister.name }}.wasm out.wasm" + "#}; + assert_eq!( + rendered_command(template, &recipe(&[]), &ctx("my-canister")), + "cp my_canister.wasm out.wasm" + ); + } + + /// User configuration cannot override the reserved `_` namespace. + #[test] + fn reserved_namespace_cannot_be_overridden_by_user_config() { + let template = indoc::indoc! {r#" + build: + steps: + - type: script + command: "build {{_.canister.name}}" + "#}; + let mut r = recipe(&[]); + r.configuration.insert( + "_".to_owned(), + serde_yaml::from_str("canister:\n name: user-override").unwrap(), + ); + assert_eq!( + rendered_command(template, &r, &ctx("real-name")), + "build real-name" + ); + } + + /// A template referencing an unset variable is a `Render` error, because + /// strict mode is on. + #[test] + fn unset_template_variable_is_a_render_error() { + let template = indoc::indoc! {r#" + build: + steps: + - type: script + command: "{{ never_set }}" + "#}; + assert!(matches!( + render_recipe(template, &recipe(&[]), &ctx("c")), + Err(RenderRecipeError::Render { .. }) + )); + } + + /// A template that renders to invalid build/sync YAML is a `Parse` error, + /// not a panic. + #[test] + fn invalid_rendered_yaml_is_a_parse_error() { + let template = "not: a valid build manifest\n"; + assert!(matches!( + render_recipe(template, &recipe(&[]), &ctx("c")), + Err(RenderRecipeError::Parse { .. }) + )); + } +} diff --git a/crates/icp/src/context/init.rs b/crates/icp/src/context/init.rs index 8a274415b..a12c581ed 100644 --- a/crates/icp/src/context/init.rs +++ b/crates/icp/src/context/init.rs @@ -3,7 +3,7 @@ use std::{env::current_dir, sync::Arc}; use snafu::prelude::*; use crate::canister::build::Builder; -use crate::canister::recipe::handlebars::Handlebars; +use crate::canister::recipe::fetch::RecipeFetcher; use crate::canister::sync::Syncer; use crate::context::Context; use crate::directories::{Access as _, Directories}; @@ -90,7 +90,7 @@ pub fn initialize( let pkg_cache = dirs.package_cache().context(PackageCacheSnafu)?; // Recipes - let recipe = Arc::new(Handlebars { + let recipe = Arc::new(RecipeFetcher { http_client, pkg_cache, }); diff --git a/crates/icp/src/lib.rs b/crates/icp/src/lib.rs index b03f808d0..12f75f6ee 100644 --- a/crates/icp/src/lib.rs +++ b/crates/icp/src/lib.rs @@ -695,12 +695,8 @@ impl ProjectLoad for NoProjectLoader { #[cfg(test)] mod tests { use super::*; - use crate::canister::recipe::{RecipeContext, Resolve, ResolveError}; - use crate::manifest::{ - ProjectRootLocate, ProjectRootLocateError, - canister::{BuildSteps, SyncSteps}, - recipe::Recipe, - }; + use crate::canister::recipe::{Resolve, ResolveError}; + use crate::manifest::{ProjectRootLocate, ProjectRootLocateError, recipe::Recipe}; use camino_tempfile::Utf8TempDir; use indoc::indoc; @@ -728,27 +724,15 @@ mod tests { #[async_trait] impl Resolve for MockRecipeResolver { - async fn resolve( - &self, - _recipe: &Recipe, - _context: &RecipeContext, - ) -> Result<(BuildSteps, SyncSteps), ResolveError> { - use crate::manifest::adapter::prebuilt::{ - Adapter as PrebuiltAdapter, LocalSource, SourceField, - }; - use crate::manifest::canister::BuildStep; - - // Create a minimal BuildSteps with a dummy prebuilt step - let build_steps = BuildSteps { - steps: vec![BuildStep::Prebuilt(PrebuiltAdapter { - source: SourceField::Local(LocalSource { - path: "dummy.wasm".into(), - }), - sha256: None, - })], - }; - - Ok((build_steps, SyncSteps::default())) + /// A minimal template rendering to a single dummy pre-built step. + async fn resolve(&self, _recipe: &Recipe) -> Result { + Ok(indoc! {r#" + build: + steps: + - type: pre-built + path: dummy.wasm + "#} + .to_owned()) } } diff --git a/crates/icp/src/project.rs b/crates/icp/src/project.rs index c1d7f3fbc..cd14d4efd 100644 --- a/crates/icp/src/project.rs +++ b/crates/icp/src/project.rs @@ -68,13 +68,20 @@ pub enum ConsolidateManifestError { #[snafu(display("failed to load {kind} manifest at: {path}"))] Failed { kind: String, path: String }, - #[snafu(display("failed to resolve canister recipe: {recipe_type:?}"))] - Recipe { + #[snafu(display("failed to fetch canister recipe: {recipe_type:?}"))] + FetchRecipe { #[snafu(source(from(recipe::ResolveError, Box::new)))] source: Box, recipe_type: RecipeType, }, + #[snafu(display("failed to render canister recipe: {recipe_type:?}"))] + RenderRecipe { + #[snafu(source(from(recipe::RenderRecipeError, Box::new)))] + source: Box, + recipe_type: RecipeType, + }, + #[snafu(display("project contains two similarly named {kind}s: '{name}'"))] Duplicate { kind: String, name: String }, @@ -380,15 +387,19 @@ async fn build_manifest_canisters( // Recipe Instructions::Recipe { recipe } => { + let template = + recipe_resolver + .resolve(recipe) + .await + .context(FetchRecipeSnafu { + recipe_type: recipe.recipe_type.clone(), + })?; let ctx = recipe::RecipeContext { canister_name: m.name.clone(), }; - recipe_resolver - .resolve(recipe, &ctx) - .await - .context(RecipeSnafu { - recipe_type: recipe.recipe_type.clone(), - })? + recipe::render_recipe(&template, recipe, &ctx).context(RenderRecipeSnafu { + recipe_type: recipe.recipe_type.clone(), + })? } }; @@ -1446,8 +1457,7 @@ pub async fn consolidate_manifest( #[cfg(test)] mod dependency_tests { use super::*; - use crate::canister::recipe::{RecipeContext, Resolve, ResolveError}; - use crate::manifest::canister::{BuildSteps, SyncSteps}; + use crate::canister::recipe::{Resolve, ResolveError}; use crate::manifest::recipe::Recipe; use camino_tempfile::Utf8TempDir; @@ -1456,11 +1466,7 @@ mod dependency_tests { #[async_trait::async_trait] impl Resolve for PanicResolver { - async fn resolve( - &self, - _recipe: &Recipe, - _context: &RecipeContext, - ) -> Result<(BuildSteps, SyncSteps), ResolveError> { + async fn resolve(&self, _recipe: &Recipe) -> Result { panic!("recipe resolver should not be called in dependency tests"); } } From d53a11c9150ba5384cb432ba4f90ea7fb9035195 Mon Sep 17 00:00:00 2001 From: Raymond Khalife Date: Thu, 13 Aug 2026 19:14:38 +0000 Subject: [PATCH 2/2] fix: do not cache an unpinned recipe template before it renders MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the fetch/render split caught a behaviour regression the split introduced, confirmed by experiment against the pre-split implementation. Splitting fetch from render moved the cache write into the fetch stage, which runs before anything knows whether the template is usable. `sha256` is optional, so an unpinned remote response that is valid UTF-8 but fails to render was written to the cache anyway — and because `read_cached_uri_recipe` returns the stored entry when no checksum is given, every later resolution read those bad bytes back instead of refetching. One bad response became sticky. Measured, serving a template that fails Handlebars strict mode once and then answering 500: - pre-split (`main`): cache holds only `.lock` after the failed render, and the second resolution goes back to the network (gets the 500). - split as submitted: cache holds `recipes//recipe.hbs`, and the second resolution succeeds from cache with the same unusable bytes. Caching is therefore a third step, not part of fetching: - A download carrying a `sha256` is still cached during the fetch. The checksum is what establishes the bytes are the ones that were asked for, and a refetch would produce the same bytes, so there is nothing to gain by waiting. - An unpinned download comes back as a `PendingCache` that the caller commits through `Resolve::commit` once `render_recipe` succeeds. Until then nothing is written, so a bad response is refetched rather than replayed. `Resolve::resolve` now returns `Fetched { template, pending_cache }`. `commit` defaults to doing nothing: only `RecipeFetcher` caches, and only it can construct a `PendingCache`, so a resolver that never defers a write never has one to commit. Three tests cover it; the first two fail if the pending write is committed eagerly. Also fixes the `RecipeContext` doc comment, which still described the context as passed to a resolver after it became render-only input. --- Cargo.lock | 1 + crates/icp/Cargo.toml | 1 + crates/icp/src/canister/recipe/fetch.rs | 303 ++++++++++++++++++++--- crates/icp/src/canister/recipe/mod.rs | 33 ++- crates/icp/src/canister/recipe/render.rs | 6 +- crates/icp/src/lib.rs | 24 +- crates/icp/src/project.rs | 35 ++- 7 files changed, 343 insertions(+), 60 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 179351e50..1d9ca596b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3627,6 +3627,7 @@ dependencies = [ "handlebars", "hex", "hmac 0.13.0", + "httptest", "hybrid-array", "ic-agent", "ic-ed25519", diff --git a/crates/icp/Cargo.toml b/crates/icp/Cargo.toml index 51c357840..40690d523 100644 --- a/crates/icp/Cargo.toml +++ b/crates/icp/Cargo.toml @@ -82,4 +82,5 @@ zeroize = { workspace = true } winreg = { workspace = true } [dev-dependencies] +httptest = { workspace = true } jsonschema = { workspace = true } diff --git a/crates/icp/src/canister/recipe/fetch.rs b/crates/icp/src/canister/recipe/fetch.rs index d90345218..cb6ca5c2d 100644 --- a/crates/icp/src/canister/recipe/fetch.rs +++ b/crates/icp/src/canister/recipe/fetch.rs @@ -19,7 +19,7 @@ use crate::{ prelude::*, }; -use super::{FetchSnafu, Resolve, ResolveError}; +use super::{CommitSnafu, FetchSnafu, Resolve, ResolveError}; /// Fetches recipe templates over HTTP, caching downloads in the package cache. /// Template *rendering* is a separate stage @@ -32,6 +32,40 @@ pub struct RecipeFetcher { pub pkg_cache: PackageCache, } +/// The result of the fetch stage. +pub struct Fetched { + /// Raw Handlebars template source. + pub template: String, + + /// A cache write deliberately held back until the template is known to + /// render; `None` when there is nothing to cache (a local file or a cache + /// hit) or when the download was already cached because it was checksummed. + /// + /// Pass to [`Resolve::commit`] after [`render_recipe`](super::render_recipe) + /// succeeds. + pub pending_cache: Option, +} + +/// A cache write for an unpinned download, held until the template renders. +/// +/// A checksummed download is cached the moment its checksum verifies: the +/// checksum is what establishes the bytes are the ones that were asked for, and +/// refetching would only produce the same bytes again. An unpinned download has +/// no such guarantee — caching it before it is known good would let a single bad +/// response become sticky, and every later resolution would read those bytes +/// back instead of refetching. +pub struct PendingCache { + target: CacheTarget, + hash: [u8; 32], + template: String, +} + +/// Where a fetched template belongs in the package cache. +enum CacheTarget { + Uri(String), + Registry { package: String, version: String }, +} + enum TemplateSource { LocalPath(PathBuf), RemoteUrl(String), @@ -77,9 +111,13 @@ pub enum RecipeFetchError { } impl RecipeFetcher { - /// Fetch a recipe's Handlebars template text: read a local file, or fetch - /// (and cache) a remote URL or registry recipe. Verifies `sha256` when set. - async fn fetch_recipe(&self, recipe: &Recipe) -> Result { + /// Fetch a recipe's Handlebars template text: read a local file, or fetch a + /// remote URL or registry recipe. Verifies `sha256` when set. + /// + /// A checksummed download is cached here. An unpinned one is returned as a + /// [`PendingCache`] for the caller to commit once it renders — see + /// [`PendingCache`] for why. + async fn fetch_recipe(&self, recipe: &Recipe) -> Result { // Determine the template source let tmpl_source = match &recipe.recipe_type { RecipeType::File(path) => TemplateSource::LocalPath(Path::new(&path).into()), @@ -156,41 +194,70 @@ impl RecipeFetcher { Sha256::digest(tmpl.as_bytes()).into() }; - // Cache the fetched template if it was remote. Rendering happens after - // this stage, so a template that renders badly is still cached — the - // checksum above is what decides whether the bytes are trustworthy. - if should_cache { - match tmpl_source { - TemplateSource::LocalPath(_) => unreachable!("local files are never cached"), - TemplateSource::RemoteUrl(u) => { - self.pkg_cache - .with_write(async |w| { - cache_uri_recipe(w, &u, &hex::encode(hash), tmpl.as_bytes()) - .context(CacheRecipeSnafu)?; - Ok(()) - }) - .await - .context(LockCacheSnafu)??; - } - TemplateSource::Registry(registry, recipe_name, version) => { - let package = format!("@{registry}/{recipe_name}"); - self.pkg_cache - .with_write(async |w| { - cache_registry_recipe( - w, - &package, - &version, - &hex::encode(hash), - tmpl.as_bytes(), - ) + // Nothing was downloaded (local file, or a cache hit): nothing to cache. + if !should_cache { + return Ok(Fetched { + template: tmpl, + pending_cache: None, + }); + } + + let target = match tmpl_source { + TemplateSource::LocalPath(_) => unreachable!("local files are never cached"), + TemplateSource::RemoteUrl(u) => CacheTarget::Uri(u), + TemplateSource::Registry(registry, recipe_name, version) => CacheTarget::Registry { + package: format!("@{registry}/{recipe_name}"), + version, + }, + }; + + let pending = PendingCache { + target, + hash, + template: tmpl, + }; + + // A checksummed download is trustworthy the moment the checksum matches, + // so cache it now. An unpinned one waits for a successful render. + if recipe.sha256.is_some() { + self.write_cache(&pending).await?; + return Ok(Fetched { + template: pending.template, + pending_cache: None, + }); + } + + Ok(Fetched { + template: pending.template.clone(), + pending_cache: Some(pending), + }) + } + + /// Write a fetched template into the package cache. + async fn write_cache(&self, pending: &PendingCache) -> Result<(), RecipeFetchError> { + let hash = hex::encode(pending.hash); + let bytes = pending.template.as_bytes(); + match &pending.target { + CacheTarget::Uri(u) => { + self.pkg_cache + .with_write(async |w| { + cache_uri_recipe(w, u, &hash, bytes).context(CacheRecipeSnafu)?; + Ok(()) + }) + .await + .context(LockCacheSnafu)??; + } + CacheTarget::Registry { package, version } => { + self.pkg_cache + .with_write(async |w| { + cache_registry_recipe(w, package, version, &hash, bytes) .context(CacheRecipeSnafu) - }) - .await - .context(LockCacheSnafu)??; - } + }) + .await + .context(LockCacheSnafu)??; } } - Ok(tmpl) + Ok(()) } /// Fetch raw bytes from a remote URL. @@ -218,9 +285,13 @@ impl RecipeFetcher { #[async_trait] impl Resolve for RecipeFetcher { - async fn resolve(&self, recipe: &Recipe) -> Result { + async fn resolve(&self, recipe: &Recipe) -> Result { self.fetch_recipe(recipe).await.context(FetchSnafu) } + + async fn commit(&self, pending: PendingCache) -> Result<(), ResolveError> { + self.write_cache(&pending).await.context(CommitSnafu) + } } /// Helper function to verify sha256 checksum of recipe template bytes @@ -282,7 +353,11 @@ mod tests { .fetch_recipe(&recipe) .await .unwrap(); - assert_eq!(fetched, body); + assert_eq!(fetched.template, body); + assert!( + fetched.pending_cache.is_none(), + "local files are never cached" + ); } /// A sha256 that does not match the template contents is rejected. @@ -303,4 +378,156 @@ mod tests { Err(RecipeFetchError::ChecksumMismatch { .. }) )); } + + /// A template that is valid UTF-8 but does not render. + const UNRENDERABLE: &str = indoc::indoc! {r#" + build: + steps: + - type: script + command: "{{ never_set }}" + "#}; + + /// Serves `body` once, then 500 for every later request. A second fetch that + /// succeeds therefore proves the cache answered it; one that fails with + /// `HttpStatus` proves it went back to the network. + fn serve_once_then_fail(body: &str) -> (httptest::Server, String) { + use httptest::{Expectation, Server, matchers::*, responders::*}; + + let server = Server::run(); + server.expect( + Expectation::matching(request::method_path("GET", "/recipe.hbs")) + .times(1..=10) + .respond_with(cycle![ + status_code(200).body(body.to_owned()), + status_code(500), + status_code(500), + status_code(500), + ]), + ); + let url = server.url("/recipe.hbs").to_string(); + (server, url) + } + + /// True when the package cache holds a stored recipe template. + fn cache_has_template(cache_dir: &Path) -> bool { + let recipes = cache_dir.join("recipes"); + let Ok(entries) = std::fs::read_dir(&recipes) else { + return false; + }; + entries.flatten().any(|e| { + PathBuf::try_from(e.path()) + .map(|p| p.join("recipe.hbs").exists()) + .unwrap_or(false) + }) + } + + /// REGRESSION: an unpinned remote template must not be cached before it is + /// known to render. Otherwise one bad response becomes sticky and every later + /// resolution reads the bad bytes back instead of refetching — which is what + /// the pre-split implementation did, because it cached only after rendering. + #[tokio::test] + async fn unpinned_download_is_not_cached_until_committed() { + let (_server, url) = serve_once_then_fail(UNRENDERABLE); + + let tmp = camino_tempfile::Utf8TempDir::new().unwrap(); + let cache_dir = tmp.path().join("pkg"); + let f = fetcher(&cache_dir); + let recipe = Recipe { + recipe_type: RecipeType::Url(url), + configuration: Default::default(), + sha256: None, + }; + + // Fetch succeeds and hands back a held-back cache write. + let fetched = f.fetch_recipe(&recipe).await.expect("first fetch"); + assert!( + fetched.pending_cache.is_some(), + "an unpinned download must defer its cache write" + ); + + // Rendering fails, so the caller never commits. + let ctx = super::super::RecipeContext { + canister_name: "c".to_owned(), + }; + assert!( + super::super::render_recipe(&fetched.template, &recipe, &ctx).is_err(), + "fixture template must fail to render" + ); + + assert!( + !cache_has_template(&cache_dir), + "a template that never rendered must not be in the cache" + ); + + // The next resolution must go back to the network rather than serve the + // bad bytes from cache. + assert!( + matches!( + f.fetch_recipe(&recipe).await, + Err(RecipeFetchError::HttpStatus { status: 500, .. }) + ), + "second resolution must refetch, not read the uncommitted template back" + ); + } + + /// Once an unpinned template renders, committing it makes it cacheable — so + /// the deferral costs nothing for templates that are actually good. + #[tokio::test] + async fn committing_an_unpinned_download_caches_it() { + let good = indoc::indoc! {r#" + build: + steps: + - type: script + command: "build {{_.canister.name}}" + "#}; + let (_server, url) = serve_once_then_fail(good); + + let tmp = camino_tempfile::Utf8TempDir::new().unwrap(); + let cache_dir = tmp.path().join("pkg"); + let f = fetcher(&cache_dir); + let recipe = Recipe { + recipe_type: RecipeType::Url(url), + configuration: Default::default(), + sha256: None, + }; + + let fetched = f.fetch_recipe(&recipe).await.expect("first fetch"); + let pending = fetched.pending_cache.expect("unpinned defers its write"); + f.write_cache(&pending).await.expect("commit"); + + assert!(cache_has_template(&cache_dir)); + + // Served from cache now, even though the server would answer 500. + let again = f.fetch_recipe(&recipe).await.expect("second fetch"); + assert_eq!(again.template, fetched.template); + assert!( + again.pending_cache.is_none(), + "a cache hit has nothing to commit" + ); + } + + /// A checksummed download is cached during the fetch: the checksum already + /// proves the bytes are the ones that were asked for, and refetching would + /// only produce the same bytes, so there is nothing to gain by waiting for a + /// render that may never succeed. + #[tokio::test] + async fn checksummed_download_is_cached_eagerly() { + let (_server, url) = serve_once_then_fail(UNRENDERABLE); + + let tmp = camino_tempfile::Utf8TempDir::new().unwrap(); + let cache_dir = tmp.path().join("pkg"); + let f = fetcher(&cache_dir); + let recipe = Recipe { + recipe_type: RecipeType::Url(url), + configuration: Default::default(), + sha256: Some(hex::encode(Sha256::digest(UNRENDERABLE.as_bytes()))), + }; + + let fetched = f.fetch_recipe(&recipe).await.expect("first fetch"); + assert!( + fetched.pending_cache.is_none(), + "a checksummed download is cached during the fetch" + ); + assert!(cache_has_template(&cache_dir)); + } } diff --git a/crates/icp/src/canister/recipe/mod.rs b/crates/icp/src/canister/recipe/mod.rs index 7a2bafb9c..edb92caf3 100644 --- a/crates/icp/src/canister/recipe/mod.rs +++ b/crates/icp/src/canister/recipe/mod.rs @@ -1,13 +1,21 @@ //! Recipe resolution, split into two stages. //! //! [`fetch`] retrieves a recipe's Handlebars template — reading a local file, or -//! downloading and caching a remote URL or registry recipe — and returns the raw -//! template text. [`render`] turns that text into concrete build/sync steps. The -//! first stage does I/O and nothing else; the second is a pure function. +//! downloading a remote URL or registry recipe — and returns the raw template +//! text. [`render`] turns that text into concrete build/sync steps. The first +//! stage does I/O and nothing else; the second is a pure function. //! //! The [`Resolve`] seam therefore covers only the fetching half, so a caller that //! already has a template (or must not touch the network) can render without //! going through a resolver at all. +//! +//! Caching a download is a third step, because whether a template is worth +//! keeping is not known until it renders. A download that carried a `sha256` is +//! cached during the fetch — the checksum already proves the bytes are the ones +//! that were asked for. An *unpinned* download is held back as a +//! [`PendingCache`] and only committed by the caller once rendering succeeds, so +//! that one bad remote response cannot become sticky in the cache. The full +//! sequence is therefore fetch → render → [`Resolve::commit`]. use async_trait::async_trait; use snafu::prelude::*; @@ -17,6 +25,7 @@ use crate::manifest::recipe::Recipe; pub mod fetch; pub mod render; +pub use fetch::{Fetched, PendingCache}; pub use render::{RecipeContext, RenderRecipeError, render_recipe}; /// Retrieves the recipe templates a project references. @@ -25,12 +34,26 @@ pub use render::{RecipeContext, RenderRecipeError, render_recipe}; /// and sync steps is [`render_recipe`], which needs no I/O and so needs no seam. #[async_trait] pub trait Resolve: Sync + Send { - /// Fetch the Handlebars template for `recipe`, returning its raw source. - async fn resolve(&self, recipe: &Recipe) -> Result; + /// Fetch the Handlebars template for `recipe`, returning its raw source and + /// any cache write held back until the template is known to render. + async fn resolve(&self, recipe: &Recipe) -> Result; + + /// Write a held-back download to the cache, now that it has rendered. + /// + /// Defaults to doing nothing: only [`fetch::RecipeFetcher`] caches, and only + /// it can construct the [`PendingCache`] that reaches this method, so a + /// resolver that never defers a write never has one to commit. + async fn commit(&self, pending: PendingCache) -> Result<(), ResolveError> { + let _ = pending; + Ok(()) + } } #[derive(Debug, Snafu)] pub enum ResolveError { #[snafu(display("failed to fetch recipe template"))] Fetch { source: fetch::RecipeFetchError }, + + #[snafu(display("failed to cache recipe template"))] + Commit { source: fetch::RecipeFetchError }, } diff --git a/crates/icp/src/canister/recipe/render.rs b/crates/icp/src/canister/recipe/render.rs index 0bd66b8ff..f59941c73 100644 --- a/crates/icp/src/canister/recipe/render.rs +++ b/crates/icp/src/canister/recipe/render.rs @@ -12,7 +12,11 @@ use crate::manifest::{ recipe::{Recipe, RecipeType}, }; -/// Context passed to a recipe resolver, describing the canister being built. +/// Describes the canister being built, for the render stage. +/// +/// Belongs to rendering alone: [`Resolve::resolve`](super::Resolve::resolve) no +/// longer takes it, since fetching a template does not depend on which canister +/// the template is for. Only [`render_recipe`] consumes it. /// /// Serializes to the shape injected into recipe templates under the `_` namespace: /// diff --git a/crates/icp/src/lib.rs b/crates/icp/src/lib.rs index 12f75f6ee..357228627 100644 --- a/crates/icp/src/lib.rs +++ b/crates/icp/src/lib.rs @@ -695,7 +695,7 @@ impl ProjectLoad for NoProjectLoader { #[cfg(test)] mod tests { use super::*; - use crate::canister::recipe::{Resolve, ResolveError}; + use crate::canister::recipe::{Fetched, Resolve, ResolveError}; use crate::manifest::{ProjectRootLocate, ProjectRootLocateError, recipe::Recipe}; use camino_tempfile::Utf8TempDir; use indoc::indoc; @@ -724,15 +724,19 @@ mod tests { #[async_trait] impl Resolve for MockRecipeResolver { - /// A minimal template rendering to a single dummy pre-built step. - async fn resolve(&self, _recipe: &Recipe) -> Result { - Ok(indoc! {r#" - build: - steps: - - type: pre-built - path: dummy.wasm - "#} - .to_owned()) + /// A minimal template rendering to a single dummy pre-built step. Nothing + /// is fetched, so there is no cache write to hold back. + async fn resolve(&self, _recipe: &Recipe) -> Result { + Ok(Fetched { + template: indoc! {r#" + build: + steps: + - type: pre-built + path: dummy.wasm + "#} + .to_owned(), + pending_cache: None, + }) } } diff --git a/crates/icp/src/project.rs b/crates/icp/src/project.rs index cd14d4efd..1f5520def 100644 --- a/crates/icp/src/project.rs +++ b/crates/icp/src/project.rs @@ -82,6 +82,13 @@ pub enum ConsolidateManifestError { recipe_type: RecipeType, }, + #[snafu(display("failed to cache canister recipe: {recipe_type:?}"))] + CacheRecipe { + #[snafu(source(from(recipe::ResolveError, Box::new)))] + source: Box, + recipe_type: RecipeType, + }, + #[snafu(display("project contains two similarly named {kind}s: '{name}'"))] Duplicate { kind: String, name: String }, @@ -387,7 +394,7 @@ async fn build_manifest_canisters( // Recipe Instructions::Recipe { recipe } => { - let template = + let fetched = recipe_resolver .resolve(recipe) .await @@ -397,9 +404,25 @@ async fn build_manifest_canisters( let ctx = recipe::RecipeContext { canister_name: m.name.clone(), }; - recipe::render_recipe(&template, recipe, &ctx).context(RenderRecipeSnafu { - recipe_type: recipe.recipe_type.clone(), - })? + let steps = recipe::render_recipe(&fetched.template, recipe, &ctx).context( + RenderRecipeSnafu { + recipe_type: recipe.recipe_type.clone(), + }, + )?; + + // The template rendered, so an unpinned download is now known + // good and safe to cache. Committing only here is what keeps a + // bad remote response from becoming sticky. + if let Some(pending) = fetched.pending_cache { + recipe_resolver + .commit(pending) + .await + .context(CacheRecipeSnafu { + recipe_type: recipe.recipe_type.clone(), + })?; + } + + steps } }; @@ -1457,7 +1480,7 @@ pub async fn consolidate_manifest( #[cfg(test)] mod dependency_tests { use super::*; - use crate::canister::recipe::{Resolve, ResolveError}; + use crate::canister::recipe::{Fetched, Resolve, ResolveError}; use crate::manifest::recipe::Recipe; use camino_tempfile::Utf8TempDir; @@ -1466,7 +1489,7 @@ mod dependency_tests { #[async_trait::async_trait] impl Resolve for PanicResolver { - async fn resolve(&self, _recipe: &Recipe) -> Result { + async fn resolve(&self, _recipe: &Recipe) -> Result { panic!("recipe resolver should not be called in dependency tests"); } }