From 7bbd34bc3a6c2d437bea1b18ae98ad49d7bb8a79 Mon Sep 17 00:00:00 2001 From: Huskies Agent Date: Sun, 19 Jul 2026 20:41:34 +0000 Subject: [PATCH] huskies: merge 1231 story project-rebuild must not silently downgrade a live sled to the stale image binary --- script/build-project-images | 11 + script/release | 10 + .../matrix/bot/messages/on_room_message.rs | 1 + .../chat/transport/matrix/project_rebuild.rs | 314 +++++++++++++++++- .../src/chat/transport/matrix/sled_upgrade.rs | 17 +- server/src/http/gateway/mcp.rs | 1 + 6 files changed, 344 insertions(+), 10 deletions(-) diff --git a/script/build-project-images b/script/build-project-images index 7b8a53fe..37960b16 100755 --- a/script/build-project-images +++ b/script/build-project-images @@ -6,6 +6,17 @@ set -euo pipefail # # Run this after `script/docker_rebuild` or whenever you add a new stack. # Safe to re-run: each step re-tags the image with the latest layers. +# +# IMPORTANT (story 1231): this script is NOT part of `script/release`. The +# huskies-project-* images bake whatever `huskies` binary happened to be built +# locally the last time this ran — as of this comment that's 0.13.0, several +# releases behind the current fleet artifact. `project-rebuild` self-heals a +# sled that comes back on a stale baked binary (it re-upgrades it in place +# from the gateway's published artifact), but that costs an extra +# download+restart cycle every time. Run `script/build-project-images` after +# every `script/release` — ideally as a step in the release flow itself — so +# freshly rebuilt sleds start on a current binary instead of relying on the +# self-heal. cd "$(dirname "$0")/.." diff --git a/script/release b/script/release index ae37dc86..25ce44f2 100755 --- a/script/release +++ b/script/release @@ -1,6 +1,16 @@ #!/usr/bin/env bash set -euo pipefail +# NOTE (story 1231): this script publishes the `huskies` binary artifact +# (consumed by the `upgrade`/`upgrade all` chat commands) but does NOT +# rebuild the huskies-project-* Docker images that `project-rebuild` uses. +# Those images bake whatever binary was locally built the last time +# `script/build-project-images` ran, so they silently drift stale otherwise +# (currently baking 0.13.0). Run `script/build-project-images` after this +# script — or better, fold it into this release flow — so a `project-rebuild` +# right after a release doesn't need to self-heal a stale sled back up to the +# version just published here. + # ── Configuration ────────────────────────────────────────────── GITEA_URL="https://code.crashlabs.io" REPO="crashlabs/huskies" diff --git a/server/src/chat/transport/matrix/bot/messages/on_room_message.rs b/server/src/chat/transport/matrix/bot/messages/on_room_message.rs index f728fead..8a27fe22 100644 --- a/server/src/chat/transport/matrix/bot/messages/on_room_message.rs +++ b/server/src/chat/transport/matrix/bot/messages/on_room_message.rs @@ -900,6 +900,7 @@ pub(in crate::chat::transport::matrix::bot) async fn on_room_message( rebuild_cmd.drain_timeout_secs, rebuild_cmd.force, store, + ctx.gateway_port, &ctx.services.project_root, ) .await diff --git a/server/src/chat/transport/matrix/project_rebuild.rs b/server/src/chat/transport/matrix/project_rebuild.rs index 793e8872..ecb96536 100644 --- a/server/src/chat/transport/matrix/project_rebuild.rs +++ b/server/src/chat/transport/matrix/project_rebuild.rs @@ -20,6 +20,7 @@ use crate::service::gateway::io::save_config; use std::collections::BTreeMap; use std::path::Path; use std::sync::Arc; +use std::time::Duration; use tokio::sync::RwLock; /// Default seconds to wait for in-flight work to drain before refusing. @@ -101,6 +102,7 @@ pub async fn handle_project_rebuild( drain_timeout_secs: u64, force: bool, projects_store: &Arc>>, + gateway_port: Option, config_dir: &Path, ) -> String { // ── 1. Validate project ────────────────────────────────────────────────── @@ -140,6 +142,21 @@ pub async fn handle_project_rebuild( ); } + // ── 1b. Capture the running binary's git_hash before we tear it down ────── + // Best-effort: an unreachable or pre-version-endpoint sled just yields `None`, + // in which case the post-rebuild reconciliation below can't compare old vs + // new and falls back to whatever the published fleet artifact says. + let http_client = reqwest::Client::builder() + .timeout(Duration::from_secs(15)) + .build() + .unwrap_or_default(); + let old_git_hash = match project_url.as_deref() { + Some(url) => super::sled_upgrade::fetch_sled_version(&http_client, url) + .await + .map(|(_version, hash)| hash), + None => None, + }; + // ── 2. Drain check ─────────────────────────────────────────────────────── let container_name = format!("huskies-{name}"); if !force @@ -288,8 +305,8 @@ pub async fn handle_project_rebuild( let container_short: String = container_id.chars().take(12).collect(); // ── 6. Persist updated config (URL is unchanged; project already registered) ──── + let container_url = format!("http://127.0.0.1:{port}"); { - let container_url = format!("http://127.0.0.1:{port}"); let mut projects = projects_store.write().await; if let Some(entry) = projects.get_mut(name) { entry.url = Some(container_url.clone()); @@ -300,6 +317,20 @@ pub async fn handle_project_rebuild( crate::slog!("[project-rebuild] Rebuilt '{name}': image={image_hash} container={container_id}"); + // ── 7. Make sure the rebuilt sled isn't running an older binary than before ── + let artifact_source = super::sled_upgrade::resolve_artifact_source(gateway_port); + let version_note = reconcile_post_rebuild_version( + name, + old_git_hash.as_deref(), + &container_url, + artifact_source, + projects_store, + config_dir, + &http_client, + 60, + ) + .await; + format!( "Project **{name}** rebuilt.\n\ - New image: `{image}` (`{image_short}…`)\n\ @@ -307,10 +338,103 @@ pub async fn handle_project_rebuild( - State: `pipeline.db` and CRDT preserved (same volume bind-mount)\n\ - Port: {port} (unchanged)\n\ \n\ - Use `switch {name}` then `status` to verify the pipeline." + Use `switch {name}` then `status` to verify the pipeline.{version_note}" ) } +/// Ensure the freshly-rebuilt sled at `container_url` isn't running an older +/// binary than the one it had before the rebuild (AC 1–3, story 1231). +/// +/// Base images are only refreshed by a manual `script/build-project-images` +/// run, so `project-rebuild` can otherwise silently downgrade a sled that was +/// live-upgraded past whatever binary is baked into the image. This waits for +/// the new container to come up, compares its `git_hash` against the +/// gateway's published fleet artifact, and — when they differ — self-heals by +/// running the same in-place upgrade `upgrade ` uses. When there's no +/// published artifact to converge to, it falls back to comparing against the +/// pre-rebuild hash so a real downgrade is at least surfaced as a warning +/// instead of passing silently. +/// +/// `artifact_source` is the already-resolved published-artifact lookup (see +/// [`super::sled_upgrade::resolve_artifact_source`]) — `Err` means no fleet +/// artifact has been published yet. +/// +/// Returns a Markdown snippet (starting with `\n\n`) to append to the rebuild +/// reply, or an empty string when the sled already matches the current build. +#[allow(clippy::too_many_arguments)] +async fn reconcile_post_rebuild_version( + name: &str, + old_git_hash: Option<&str>, + container_url: &str, + artifact_source: Result<(String, Option), String>, + projects_store: &Arc>>, + config_dir: &Path, + client: &reqwest::Client, + health_timeout_secs: u64, +) -> String { + let health_url = format!("{}/health", container_url.trim_end_matches('/')); + if !super::sled_upgrade::wait_for_health(client, &health_url, health_timeout_secs).await { + return format!( + "\n\n⚠️ **Warning:** the rebuilt sled did not respond to `/health` within 60s — \ + cannot verify its binary version. Check `docker logs huskies-{name}` and run \ + `upgrade {name}` once it's reachable." + ); + } + + let Some((_new_version, new_hash)) = + super::sled_upgrade::fetch_sled_version(client, container_url).await + else { + return "\n\n⚠️ **Warning:** the rebuilt sled is healthy but `/api/version` is unavailable — \ + cannot verify it isn't running a stale binary baked into the base image." + .to_string(); + }; + + let (source_url, expected_hash) = match artifact_source { + Ok(v) => v, + Err(_) => { + // No published fleet artifact to converge to — the best we can do is + // flag an actual change from what was running before the rebuild. + return match old_git_hash { + Some(old) if old != new_hash => format!( + "\n\n⚠️ **Warning:** the rebuilt sled is now on `{new_hash}` (was `{old}`) and \ + no fleet artifact is published to verify or fix this. Run `release` then \ + `upgrade {name}`." + ), + _ => String::new(), + }; + } + }; + + if expected_hash.as_deref() == Some(new_hash.as_str()) { + return String::new(); // base image already bakes the current fleet build. + } + + // The base image baked an older (or merely different) binary than the fleet + // is currently on — self-heal the same way `upgrade ` would. + let outcome = super::sled_upgrade::run_sled_upgrade( + name, + container_url, + &source_url, + expected_hash, + projects_store, + config_dir, + |_| async {}, + ) + .await; + + if outcome.contains("matches published artifact") || outcome.starts_with("upgraded to v") { + format!( + "\n\n♻️ The base image baked an older binary (`{new_hash}`); self-healed in place — \ + {outcome}" + ) + } else { + format!( + "\n\n⚠️ **Warning:** the base image baked an older binary (`{new_hash}`) and the \ + automatic self-heal failed: {outcome}\nRun `upgrade {name}` manually." + ) + } +} + /// Wait for active Claude agent processes in the container to exit. /// /// Polls every 5 seconds until no `claude` processes remain or `timeout_secs` elapses. @@ -510,7 +634,7 @@ mod tests { async fn rebuild_unknown_project_returns_error() { let store = make_store(vec![]); let dir = tempfile::tempdir().unwrap(); - let result = handle_project_rebuild("nonexistent", 0, true, &store, dir.path()).await; + let result = handle_project_rebuild("nonexistent", 0, true, &store, None, dir.path()).await; assert!( result.contains("not found"), "expected 'not found': {result}" @@ -530,7 +654,7 @@ mod tests { }, )]); let dir = tempfile::tempdir().unwrap(); - let result = handle_project_rebuild("myapp", 0, true, &store, dir.path()).await; + let result = handle_project_rebuild("myapp", 0, true, &store, None, dir.path()).await; assert!( result.contains("host_path"), "expected 'host_path' mention: {result}" @@ -550,7 +674,7 @@ mod tests { }, )]); let dir = tempfile::tempdir().unwrap(); - let result = handle_project_rebuild("myapp", 0, true, &store, dir.path()).await; + let result = handle_project_rebuild("myapp", 0, true, &store, None, dir.path()).await; assert!( result.contains("does not exist"), "expected 'does not exist': {result}" @@ -586,7 +710,8 @@ mod tests { )]); let config_dir = tempfile::tempdir().unwrap(); - let result = handle_project_rebuild("myapp", 0, true, &store, config_dir.path()).await; + let result = + handle_project_rebuild("myapp", 0, true, &store, None, config_dir.path()).await; // (a) Step naming: one of several possible failure steps depending on what Docker // binaries are available in the test environment, or a success reply. @@ -607,4 +732,181 @@ mod tests { "project 'myapp' must remain registered after failed rebuild: {result}" ); } + + // ── reconcile_post_rebuild_version (AC 1–3) ──────────────────────────── + + /// Spawn a minimal HTTP server that answers every `/health` request with + /// 200 and every `/api/version` request with `git_hash`. Serves + /// connections in a loop so both `wait_for_health` and + /// `fetch_sled_version` can hit it independently within one test. + fn spawn_version_server(git_hash: &str) -> (String, tokio::task::JoinHandle<()>) { + let git_hash = git_hash.to_string(); + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + listener.set_nonblocking(true).unwrap(); + let listener = tokio::net::TcpListener::from_std(listener).unwrap(); + let port = listener.local_addr().unwrap().port(); + let handle = tokio::spawn(async move { + loop { + let Ok((mut stream, _)) = listener.accept().await else { + break; + }; + let git_hash = git_hash.clone(); + tokio::spawn(async move { + use tokio::io::{AsyncReadExt, AsyncWriteExt}; + let mut buf = [0u8; 4096]; + let n = stream.read(&mut buf).await.unwrap_or(0); + let req = String::from_utf8_lossy(&buf[..n]); + let path = req.lines().next().unwrap_or("").to_string(); + let body = if path.contains("/api/version") { + serde_json::json!({"version": "0.14.2", "git_hash": git_hash}).to_string() + } else { + "ok".to_string() + }; + let resp = format!( + "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\n\r\n{}", + body.len(), + body + ); + let _ = stream.write_all(resp.as_bytes()).await; + }); + } + }); + (format!("http://127.0.0.1:{port}"), handle) + } + + #[tokio::test] + async fn reconcile_empty_when_new_hash_matches_published_artifact() { + let (url, srv) = spawn_version_server("current-hash"); + let store = make_store(vec![]); + let config_dir = tempfile::tempdir().unwrap(); + let client = reqwest::Client::new(); + + let note = reconcile_post_rebuild_version( + "myapp", + Some("old-hash"), + &url, + Ok(( + "http://unused".to_string(), + Some("current-hash".to_string()), + )), + &store, + config_dir.path(), + &client, + 2, + ) + .await; + + assert_eq!(note, "", "already-current build must produce no note"); + srv.abort(); + } + + #[tokio::test] + async fn reconcile_empty_when_no_artifact_and_hash_unchanged() { + let (url, srv) = spawn_version_server("same-hash"); + let store = make_store(vec![]); + let config_dir = tempfile::tempdir().unwrap(); + let client = reqwest::Client::new(); + + let note = reconcile_post_rebuild_version( + "myapp", + Some("same-hash"), + &url, + Err("no published artifact".to_string()), + &store, + config_dir.path(), + &client, + 2, + ) + .await; + + assert_eq!(note, "", "unchanged hash must produce no note"); + srv.abort(); + } + + #[tokio::test] + async fn reconcile_warns_when_no_artifact_and_hash_changed() { + let (url, srv) = spawn_version_server("stale-baked-hash"); + let store = make_store(vec![]); + let config_dir = tempfile::tempdir().unwrap(); + let client = reqwest::Client::new(); + + let note = reconcile_post_rebuild_version( + "myapp", + Some("newer-hash-that-was-running"), + &url, + Err("no published artifact".to_string()), + &store, + config_dir.path(), + &client, + 2, + ) + .await; + + assert!(note.contains("Warning"), "expected a warning: {note}"); + assert!( + note.contains("stale-baked-hash") && note.contains("newer-hash-that-was-running"), + "warning should name both hashes: {note}" + ); + srv.abort(); + } + + #[tokio::test] + async fn reconcile_warns_when_health_probe_fails() { + let store = make_store(vec![]); + let config_dir = tempfile::tempdir().unwrap(); + let client = reqwest::Client::builder() + .timeout(std::time::Duration::from_millis(200)) + .build() + .unwrap(); + + let note = reconcile_post_rebuild_version( + "myapp", + Some("old-hash"), + "http://127.0.0.1:1", // nothing listens on port 1 + Err("no published artifact".to_string()), + &store, + config_dir.path(), + &client, + 2, + ) + .await; + + assert!( + note.contains("did not respond to `/health`"), + "expected a health-probe warning: {note}" + ); + } + + #[tokio::test] + async fn reconcile_attempts_self_heal_and_reports_failure_when_artifact_unreachable() { + let (url, srv) = spawn_version_server("stale-baked-hash"); + let store = make_store(vec![]); + let config_dir = tempfile::tempdir().unwrap(); + let client = reqwest::Client::new(); + + let note = reconcile_post_rebuild_version( + "myapp", + Some("stale-baked-hash"), + &url, + Ok(( + "http://127.0.0.1:1/api/artifacts/huskies-linux-arm64".to_string(), + Some("current-fleet-hash".to_string()), + )), + &store, + config_dir.path(), + &client, + 2, + ) + .await; + + assert!( + note.contains("self-heal failed"), + "mismatch should trigger a self-heal attempt that reports failure: {note}" + ); + assert!( + note.contains("stale-baked-hash"), + "note should name the stale hash: {note}" + ); + srv.abort(); + } } diff --git a/server/src/chat/transport/matrix/sled_upgrade.rs b/server/src/chat/transport/matrix/sled_upgrade.rs index ab0732e1..9579f5a3 100644 --- a/server/src/chat/transport/matrix/sled_upgrade.rs +++ b/server/src/chat/transport/matrix/sled_upgrade.rs @@ -181,7 +181,9 @@ pub async fn handle_upgrade_list_projects( /// /// Returns `Err` with a user-facing message when no artifact has been /// published yet. -fn resolve_artifact_source(gateway_port: Option) -> Result<(String, Option), String> { +pub(crate) fn resolve_artifact_source( + gateway_port: Option, +) -> Result<(String, Option), String> { if let Ok(url) = std::env::var("HUSKIES_GATEWAY_BINARY_URL") { return Ok((url, None)); } @@ -311,7 +313,7 @@ where /// resolved. Split from [`handle_sled_upgrade`] so tests can drive the wire /// behaviour without a published artifact on the host. #[allow(clippy::too_many_arguments)] -async fn run_sled_upgrade( +pub(crate) async fn run_sled_upgrade( project: &str, sled_url: &str, source_url: &str, @@ -427,7 +429,11 @@ where /// Poll `GET {health_url}` every 3 seconds until it returns 200 or `timeout_secs` elapses. /// /// Returns `true` when the probe succeeds, `false` on timeout. -async fn wait_for_health(client: &reqwest::Client, health_url: &str, timeout_secs: u64) -> bool { +pub(crate) async fn wait_for_health( + client: &reqwest::Client, + health_url: &str, + timeout_secs: u64, +) -> bool { let deadline = std::time::Instant::now() + Duration::from_secs(timeout_secs); let poll = Duration::from_secs(3); loop { @@ -446,7 +452,10 @@ async fn wait_for_health(client: &reqwest::Client, health_url: &str, timeout_sec /// /// Returns `None` when the endpoint is unreachable or malformed — e.g. a sled /// still running a binary that predates the endpoint. -async fn fetch_sled_version(client: &reqwest::Client, sled_url: &str) -> Option<(String, String)> { +pub(crate) async fn fetch_sled_version( + client: &reqwest::Client, + sled_url: &str, +) -> Option<(String, String)> { let url = format!("{}/api/version", sled_url.trim_end_matches('/')); let val: serde_json::Value = client.get(&url).send().await.ok()?.json().await.ok()?; let version = val.get("version").and_then(|v| v.as_str())?.to_string(); diff --git a/server/src/http/gateway/mcp.rs b/server/src/http/gateway/mcp.rs index decf450c..cc4e3f3b 100644 --- a/server/src/http/gateway/mcp.rs +++ b/server/src/http/gateway/mcp.rs @@ -1211,6 +1211,7 @@ async fn handle_project_rebuild_tool( drain_timeout_secs, force, &state.projects, + Some(state.port), &state.config_dir, ) .await;