From d39efa33a129ee34dece94702d8f2ec9aef14fc8 Mon Sep 17 00:00:00 2001 From: Logan Rupe Date: Wed, 5 Aug 2026 13:35:47 +1000 Subject: [PATCH] fix(stack): recreate services when a config_files diff triggers the deploy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DeployStackIfChanged correctly detects a `config_files` content change and fires a deploy scoped to the declared services. That deploy then runs a plain `docker compose up -d `, which is a no-op: only the content behind a bind mount moved, so the service definition is unchanged and compose reports the container up-to-date. Komodo records success and advances `deployed_contents`, so the change is permanently marked deployed against a container still running the old file, and every later diff reads "unchanged". Silent, and it never self-corrects. Adds `force_recreate` to DeployStack and the periphery ComposeUp request, and appends `--force-recreate` when a config file drove the deploy. `DeployIfChangedAction::FullDeploy` gains a `force_recreate` flag, and `resolve_deploy_if_changed_action` takes `&Stack` so it can use `is_config_file` to tell a config trigger from a compose/env one. Two early returns are removed from that function: * on the first `(Redeploy, true)` hit. Compose files are registered before config files, so a commit touching both would return on the compose file and never consider the config file. It now accumulates and decides at the end. * when a file is absent from deployed_contents. Such a file was only just declared, and returning FullDeploy meant adding one `config_files` entry force-recreated the whole project. It is now simply treated as changed and scoped by its own `services` like any other changed file. The genuinely global cases still work with no special handling: compose and env files carry an empty `services` array, so they land in the `(Redeploy, true)` arm regardless. `Services { deploy }` needs no flag. Compose and env files are registered via `StackFileDependency::full_redeploy` with an empty `services` array, so they can only reach FullDeploy — that arm is config-file-only by construction. Skips injection when `extra_args` already pins `--force-recreate` or `--no-recreate`, since compose hard-errors on that pair. NOT a `compose down` first, which was the first approach tried and abandoned: `down -- ` cascades to DEPENDENTS, and the following `up -d ` restores only the target and its dependencies, leaving the dependents removed. Measured on a 5-service stack: `down -- db` took `api` and `web` with it, neither came back, and the update still reported success. Verified on a live 12-server fleet, Komodo 2.3.1, compose 5.4.0. Every arm: scoped (with and without graph edges, and mid-graph with dependents), whole project, restart, compose-only, compose+config combined, newly-declared file both scoped and global, extra_args conflict, requires="None", and combined deploy+restart. Commands were read from Log.command rather than inferred from container state. Not covered: Swarm. `docker stack deploy` has no `--force-recreate` and its destroy is `docker stack rm`, whole-stack rather than service-scoped. Refs moghtech/komodo#1381 --- bin/core/src/api/execute/stack.rs | 84 ++++++++++++++++++++----- bin/core/src/api/listener/resources.rs | 1 + bin/core/src/api/write/stack.rs | 3 + bin/core/src/helpers/procedure.rs | 1 + bin/core/src/sync/deploy.rs | 1 + bin/periphery/src/api/compose.rs | 20 +++++- client/core/rs/src/api/execute/stack.rs | 15 +++++ client/periphery/rs/src/api/compose.rs | 13 ++++ 8 files changed, 123 insertions(+), 15 deletions(-) diff --git a/bin/core/src/api/execute/stack.rs b/bin/core/src/api/execute/stack.rs index ec710f6a0d..b0aa552f0f 100644 --- a/bin/core/src/api/execute/stack.rs +++ b/bin/core/src/api/execute/stack.rs @@ -60,6 +60,7 @@ impl super::BatchExecute for BatchDeployStack { stack, services: Vec::new(), stop_time: None, + force_recreate: false, }) } } @@ -220,6 +221,7 @@ impl Resolve for DeployStack { git_token, registry_token, replacers: secret_replacers.into_iter().collect(), + force_recreate: self.force_recreate, }) .await? } @@ -423,12 +425,15 @@ impl Resolve for DeployStackIfChanged { .map(|s| s.service_name.clone()) .collect::>(); resolve_deploy_if_changed_action( + &stack, deployed_contents, latest_contents, &services, ) } - (None, _) => DeployIfChangedAction::FullDeploy, + (None, _) => DeployIfChangedAction::FullDeploy { + force_recreate: false, + }, _ => DeployIfChangedAction::Services { deploy: Vec::new(), restart: Vec::new(), @@ -439,7 +444,7 @@ impl Resolve for DeployStackIfChanged { match action { // Existing path pre 1.19.1 - DeployIfChangedAction::FullDeploy => { + DeployIfChangedAction::FullDeploy { force_recreate } => { // Don't actually send it here, let the handler send it after it can set action state. // This is usually done in crate::helpers::update::init_execution_update. update.id = add_update_without_send(&update).await?; @@ -448,6 +453,7 @@ impl Resolve for DeployStackIfChanged { stack: stack.name, services: Vec::new(), stop_time: self.stop_time, + force_recreate: force_recreate, } .resolve(&ExecuteArgs { user: user.clone(), @@ -570,6 +576,18 @@ impl Resolve for DeployStackIfChanged { services = format!("{services:?}") ) )] +/// Only ever called from `DeployStackIfChanged` with a service list derived +/// from changed `config_files` entries — compose and env files are registered +/// with an empty `services` array (see `StackFileDependency::full_redeploy`), +/// so they can never reach here. That makes `force_recreate` unconditionally +/// correct: a config file's content change does not alter the compose service +/// definition, so `up -d` alone would leave the container running the old file. +/// +/// Uses `--force-recreate` rather than a `compose down` first. `down` is +/// service-scoped in name only — it also removes every service that depends on +/// the target, and the following `up -d ` restores only the target and +/// its dependencies, leaving those dependents removed. Measured 2026-08-04: +/// `down -- db` took `api` and `web` with it and neither came back. async fn deploy_services( stack: String, services: Vec, @@ -582,6 +600,7 @@ async fn deploy_services( stack, services, stop_time: None, + force_recreate: true, }); let update = init_execution_update(&req, user).await?; let ExecuteRequest::DeployStack(req) = req else { @@ -688,7 +707,21 @@ async fn update_deployed_contents_with_latest( enum DeployIfChangedAction { /// Changes to any compose or env files /// always lead to this. - FullDeploy, + FullDeploy { + /// Whether to pass `--force-recreate`. + /// + /// True only when a `config_files` entry with an empty `services` array + /// is what triggered the deploy. Such a change is invisible to docker's + /// own diff — the compose service definitions are unchanged, only the + /// content behind a bind mount moved — so a plain `up -d` would report + /// every container up-to-date and recreate nothing. + /// + /// Deliberately NOT set when a compose or env file triggered the deploy: + /// those change the service definitions, so docker already recreates + /// exactly the affected services, and forcing would needlessly bounce + /// every other container in the project. + force_recreate: bool, + }, /// If the above is not met, then changes to /// any changed additional file with `requires = "Restart"` /// and empty services array will lead to this. @@ -705,31 +738,48 @@ enum DeployIfChangedAction { } fn resolve_deploy_if_changed_action( + stack: &Stack, deployed_contents: &[FileContents], latest_contents: &[StackRemoteFileContents], all_services: &[String], ) -> DeployIfChangedAction { + let mut full_deploy = false; + let mut full_deploy_force_recreate = false; let mut full_restart = false; let mut deploy = HashSet::::new(); let mut restart = HashSet::::new(); for latest in latest_contents { - let Some(deployed) = - deployed_contents.iter().find(|c| c.path == latest.path) - else { - // If file doesn't exist in deployed contents, do full - // deploy to align this. - return DeployIfChangedAction::FullDeploy; - }; - // Ignore unchanged files - if latest.contents == deployed.contents { + // A file absent from deployed contents was only just declared, so there + // is nothing to compare it against — treat it as changed. + // + // Deliberately NOT an early return to FullDeploy. Falling through to the + // match below means a newly declared file is scoped by its own `services` + // and `requires`, exactly like any other changed file, so adding one + // `config_files` entry no longer bounces every service in the stack. + // + // The cases that genuinely need a full pass still get one without any + // special handling: compose and env files are registered with an empty + // `services` array, so they land in the `(Redeploy, true)` arm below. + let changed = + match deployed_contents.iter().find(|c| c.path == latest.path) { + Some(deployed) => latest.contents != deployed.contents, + None => true, + }; + if !changed { continue; } match (latest.requires, latest.services.is_empty()) { (StackFileRequires::Redeploy, true) => { // File has requires = "Redeploy" at global level. - // Can do early return here. - return DeployIfChangedAction::FullDeploy; + // No early return: a config file later in the list may still + // require the destroy step, and returning here would miss it. + full_deploy = true; + // Compose / env files reach this arm too, but only a config file's + // content change is invisible to docker's own diff. + if stack.is_config_file(&latest.path) { + full_deploy_force_recreate = true; + } } (StackFileRequires::Redeploy, false) => { // Requires redeploy on specific services @@ -749,6 +799,12 @@ fn resolve_deploy_if_changed_action( } } + if full_deploy { + return DeployIfChangedAction::FullDeploy { + force_recreate: full_deploy_force_recreate, + }; + } + match (full_restart, deploy.is_empty()) { // Full restart required with NO deploys needed -> Full Restart (true, true) => DeployIfChangedAction::FullRestart, diff --git a/bin/core/src/api/listener/resources.rs b/bin/core/src/api/listener/resources.rs index 02fab8abd0..1fe5da05f0 100644 --- a/bin/core/src/api/listener/resources.rs +++ b/bin/core/src/api/listener/resources.rs @@ -312,6 +312,7 @@ impl StackExecution for DeployStack { stack: stack.id, services: Vec::new(), stop_time: None, + force_recreate: false, }); let update = init_execution_update(&req, &user).await?; let ExecuteRequest::DeployStack(req) = req else { diff --git a/bin/core/src/api/write/stack.rs b/bin/core/src/api/write/stack.rs index 275efc72ce..6704a39883 100644 --- a/bin/core/src/api/write/stack.rs +++ b/bin/core/src/api/write/stack.rs @@ -990,6 +990,9 @@ pub async fn check_stack_for_update_inner( stack: stack.id.clone(), services: deploy_services, stop_time: None, + // Image digest changes alter the service definition, so docker + // already recreates the affected services on its own. + force_recreate: false, }), auto_redeploy_user().to_owned(), ) diff --git a/bin/core/src/helpers/procedure.rs b/bin/core/src/helpers/procedure.rs index dd6fe1b132..dbc4b5c0ef 100644 --- a/bin/core/src/helpers/procedure.rs +++ b/bin/core/src/helpers/procedure.rs @@ -608,6 +608,7 @@ impl ExtendBatch for BatchDeployStack { stack, services: Vec::new(), stop_time: None, + force_recreate: false, }) } } diff --git a/bin/core/src/sync/deploy.rs b/bin/core/src/sync/deploy.rs index 7b6fb3eb03..c77aeb1045 100644 --- a/bin/core/src/sync/deploy.rs +++ b/bin/core/src/sync/deploy.rs @@ -113,6 +113,7 @@ pub async fn deploy_from_cache( stack: name.to_string(), services: Vec::new(), stop_time: None, + force_recreate: false, }); let update = init_execution_update(&req, user).await?; diff --git a/bin/periphery/src/api/compose.rs b/bin/periphery/src/api/compose.rs index 091417a845..d7920f84f5 100644 --- a/bin/periphery/src/api/compose.rs +++ b/bin/periphery/src/api/compose.rs @@ -448,6 +448,7 @@ impl Resolve for ComposeUp { git_token, registry_token, mut replacers, + force_recreate, } = self; let mut res = DeployStackResponse::default(); @@ -723,8 +724,25 @@ impl Resolve for ComposeUp { // Run compose up let extra_args = format_extra_args(&stack.config.extra_args); + // Requested by the caller for this deploy only. [DeployStackIfChanged] sets + // it when it acts on a `config_files` diff: only the content behind a bind + // mount moved, so the service definition is unchanged and a plain `up -d` + // reports the container up-to-date and recreates nothing. + // + // Skipped when the user has already pinned recreate behaviour themselves — + // `--force-recreate` and `--no-recreate` are mutually exclusive and compose + // hard-errors on the pair, which would fail the deploy outright. + let force_recreate = if force_recreate + && !stack.config.extra_args.iter().any(|arg| { + let arg = arg.trim(); + arg == "--force-recreate" || arg == "--no-recreate" + }) { + " --force-recreate" + } else { + "" + }; let command = format!( - "{docker_compose} -p {project_name} -f {file_args}{env_file_args} up -d{extra_args}{service_args}", + "{docker_compose} -p {project_name} -f {file_args}{env_file_args} up -d{extra_args}{force_recreate}{service_args}", ); let (command, _) = match maybe_wrap_command( command, diff --git a/client/core/rs/src/api/execute/stack.rs b/client/core/rs/src/api/execute/stack.rs index fc8b416ac3..e035aa116b 100644 --- a/client/core/rs/src/api/execute/stack.rs +++ b/client/core/rs/src/api/execute/stack.rs @@ -43,6 +43,21 @@ pub struct DeployStack { /// Override the default termination max time. /// Only used if the stack needs to be taken down first. pub stop_time: Option, + /// Pass `--force-recreate`, recreating the target services even if their + /// compose definition is unchanged. + /// + /// Needed when the only thing that changed is the *content* of a file + /// bind mounted into a service: the compose service definition is + /// unchanged, so `docker compose up -d` considers the container + /// up-to-date and will not recreate it. [DeployStackIfChanged] sets this + /// when it acts on a `config_files` diff. + /// + /// Ignored if `extra_args` already pins recreate behaviour, since + /// `--force-recreate` and `--no-recreate` are mutually exclusive. + /// + /// Note. For Swarm mode Stacks, this field is not supported and will be ignored. + #[serde(default)] + pub force_recreate: bool, } // diff --git a/client/periphery/rs/src/api/compose.rs b/client/periphery/rs/src/api/compose.rs index a5ac622077..dcd1c4b0d2 100644 --- a/client/periphery/rs/src/api/compose.rs +++ b/client/periphery/rs/src/api/compose.rs @@ -182,6 +182,19 @@ pub struct ComposeUp { /// Propogate any secret replacers from core interpolation. #[serde(default)] pub replacers: Vec<(String, String)>, + /// Pass `--force-recreate` to `docker compose up`. + /// + /// Set by `DeployStackIfChanged` when it acts on a `config_files` diff: + /// the compose service definition is unchanged in that case, so + /// `docker compose up -d` would consider the container up-to-date and + /// leave it running the old file contents. + /// + /// Deliberately NOT a `compose down` first. Taking a service down is + /// service-scoped in name only: `compose down ` also removes every + /// service that depends on it, and the following `up -d ` brings back + /// only the target and its dependencies — leaving the dependents removed. + #[serde(default)] + pub force_recreate: bool, } //