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 new file mode 100644 index 000000000..cb6ca5c2d --- /dev/null +++ b/crates/icp/src/canister/recipe/fetch.rs @@ -0,0 +1,533 @@ +//! 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::{CommitSnafu, 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, +} + +/// 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), + + /// 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 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()), + 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() + }; + + // 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)??; + } + } + Ok(()) + } + + /// 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) + } + + 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 +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.template, body); + assert!( + fetched.pending_cache.is_none(), + "local files are never cached" + ); + } + + /// 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 { .. }) + )); + } + + /// 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/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..edb92caf3 100644 --- a/crates/icp/src/canister/recipe/mod.rs +++ b/crates/icp/src/canister/recipe/mod.rs @@ -1,55 +1,59 @@ +//! Recipe resolution, split into two stages. +//! +//! [`fetch`] retrieves a recipe's Handlebars template — reading a local file, or +//! 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::*; -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 fetch::{Fetched, PendingCache}; +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 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 resolve handlebars template"))] - Handlebars { source: handlebars::HandlebarsError }, + #[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 new file mode 100644 index 000000000..f59941c73 --- /dev/null +++ b/crates/icp/src/canister/recipe/render.rs @@ -0,0 +1,264 @@ +//! 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}, +}; + +/// 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: +/// +/// ```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..357228627 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::{Fetched, Resolve, ResolveError}; + use crate::manifest::{ProjectRootLocate, ProjectRootLocateError, recipe::Recipe}; use camino_tempfile::Utf8TempDir; use indoc::indoc; @@ -728,27 +724,19 @@ 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. 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 c1d7f3fbc..1f5520def 100644 --- a/crates/icp/src/project.rs +++ b/crates/icp/src/project.rs @@ -68,8 +68,22 @@ 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("failed to cache canister recipe: {recipe_type:?}"))] + CacheRecipe { #[snafu(source(from(recipe::ResolveError, Box::new)))] source: Box, recipe_type: RecipeType, @@ -380,15 +394,35 @@ async fn build_manifest_canisters( // Recipe Instructions::Recipe { recipe } => { + let fetched = + 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 { + 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 } }; @@ -1446,8 +1480,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::{Fetched, Resolve, ResolveError}; use crate::manifest::recipe::Recipe; use camino_tempfile::Utf8TempDir; @@ -1456,11 +1489,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"); } }