From d2a550e685dca5e8e3730965ea5e855383e6acd5 Mon Sep 17 00:00:00 2001 From: atlas Date: Wed, 19 Aug 2026 19:25:30 +0200 Subject: [PATCH] feat(#3255): hives stop owning the knowledge webhook, and clean up their own MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A webhook has exactly one target URL, so every hive registering one against the shared internal/knowledge repository was last-writer-wins rather than idempotent: all but the most recent silently stopped receiving deliveries. The swarm controller holds the single registration and now addresses an event to each hive over the queue instead. This is a migration, not a deletion. Not registering any more fixes nothing on a hive that has already run — the hook it created persists on the forge, so the contention would survive on exactly the deployments that have it while fresh installs looked fixed. The hive that created a hook removes it. It removes only its OWN, matched on the full URL rather than the /webhook/knowledge suffix. A hook with that suffix and a different base belongs to another hive, possibly one not yet upgraded, and deleting it would break that hive's knowledge sync until it caught up. Reaping a neighbour's registration is the behaviour being removed here; doing it while fixing it would only invert the direction. The predecessor did reap by suffix, to clear loopback hooks left by an older single-hive layout. That was safe when a hive was alone on its forge and is not safe now. The hive-side registrars also acted as reapers of hooks under their own path, which is why the swarm hook lives under /webhook/forge/; removing this registrar removes that reaper too. Intended, and stated because no reviewer would infer it from the diff. The receive endpoint goes with it. A live HMAC-verified /webhook/knowledge that nothing can legitimately reach would tell the next reader that this is how a hive learns about knowledge changes. Docs move in the same commit: docs/swarm/README.md said two hooks exist per swarm-wide repo and neither should be deleted, which is now true for agent-configs and wrong for internal/knowledge — a half-correct description being worse than an uncorrected one. --- docs/knowledge.md | 27 ++--- docs/swarm/README.md | 21 ++-- hive-c0re/src/dashboard/mod.rs | 2 - hive-c0re/src/dashboard/webhook.rs | 110 ++------------------- hive-c0re/src/job_queue/exec.rs | 16 ++- hive-c0re/src/workers/knowledge.rs | 153 ++++++++++++----------------- 6 files changed, 111 insertions(+), 218 deletions(-) diff --git a/docs/knowledge.md b/docs/knowledge.md index 5f2b00d1..604ad8be 100644 --- a/docs/knowledge.md +++ b/docs/knowledge.md @@ -38,18 +38,23 @@ create a new document. hive-c0re maintains the local clone at `/var/lib/hyperhive/knowledge` via two paths: -1. **Forgejo push webhook** — `ensure_webhook` registers a push - hook on `internal/knowledge` at startup pointing at - `https:///webhook/knowledge` (routed through the - gateway, avoiding the Forgejo SSRF guard that blocks loopback - delivery). On any push to main (including merge commits) hive-c0re - runs `git pull` so agents see the new content on their next turn. - The endpoint is protected by an auto-generated HMAC secret that - hive-c0re verifies on every delivery. +1. **Swarm event** — the swarm controller holds the single push hook on + `internal/knowledge` (see `docs/swarm/README.md` § Swarm-wide forge + webhooks). On any push to main, including merge commits, it sends an + event to every hive over the swarm queue and each hive runs `git + pull`, so agents see the new content on their next turn. - On a swarm, a **second** hook on the same repo points at the - swarm controller — see `docs/swarm/README.md` § Swarm-wide forge - webhooks. Both are expected; neither should be deleted. + A hive that is offline when the event is sent does not get it on + reconnect — the periodic pull below is what closes that gap. So one + hive briefly showing older `/knowledge` content than another is + expected, and resolves by itself within the fallback interval. + + **Do not add a per-hive hook.** A webhook has exactly one target + URL, so a second registration against the same repo does not add a + recipient — it takes delivery away from whoever registered first. + Earlier versions had each hive register its own; hive-c0re now + removes its own leftover at startup, so no operator step is needed + to migrate. 2. **Periodic pull** — a background task in `hive-c0re::main` pulls on a fixed cadence as a fallback (webhook missed, c0re diff --git a/docs/swarm/README.md b/docs/swarm/README.md index 2338b0fd..853a44cc 100644 --- a/docs/swarm/README.md +++ b/docs/swarm/README.md @@ -356,18 +356,23 @@ itself — a `push` hook on `internal/knowledge` and a `pull_request` hook on the `agent-configs` org, both under `https:///webhook/forge/`. -**Two hooks exist per swarm-wide repo: each hive's own, plus the -controller's.** Both are expected — **do not delete either.** Removing a -hive's hook stops that hive acting on knowledge pushes and config PRs; -removing the controller's just gets recreated on its next start. - The controller **interprets** a delivery and sends hives a specific message — *the knowledge repo changed*, *deploy agent `foo` at rev `abc123`* — rather than forwarding forge payloads for each hive to re-derive. Approval happens once, at the swarm level: a hive receives a -decision, not an event to adjudicate. Today the controller logs each -verified delivery and sends nothing, because the swarm→hive channel does -not exist yet; the hive-side hooks are what act in the meantime. +decision, not an event to adjudicate. + +**`internal/knowledge` is on that path.** The controller's is the only +hook on it: hives no longer register their own, and each removes its +leftover at startup. A webhook has exactly one target URL, so per-hive +registration never added a recipient — it took delivery away from +whichever hive registered before it. + +**The `agent-configs` org is not yet.** Each hive still registers its own +`pull_request` hook there, so that repo has two — the hive's and the +controller's — and **both are expected; do not delete either.** Removing +a hive's stops it acting on config PRs; removing the controller's just +gets recreated on its next start. Nothing to configure. The hooks are registered only when this host also serves the swarm UI vhost — that is what publishes the endpoint, and a diff --git a/hive-c0re/src/dashboard/mod.rs b/hive-c0re/src/dashboard/mod.rs index 9fc491b2..1a820d13 100644 --- a/hive-c0re/src/dashboard/mod.rs +++ b/hive-c0re/src/dashboard/mod.rs @@ -184,7 +184,6 @@ pub async fn serve( .routes(routes!(schedules::post_schedule_resume)) .routes(routes!(schedules::post_schedule_fire_now)) .routes(routes!(schedules::post_rebuild_queue_cancel)) - .routes(routes!(webhook::post_webhook_knowledge)) .routes(routes!(webhook::post_webhook_config_pr)) .routes(routes!(approvals::post_approve)) .routes(routes!(approvals::post_deny)) @@ -430,7 +429,6 @@ mod router_build_probe { .routes(routes!(schedules::post_schedule_resume)) .routes(routes!(schedules::post_schedule_fire_now)) .routes(routes!(schedules::post_rebuild_queue_cancel)) - .routes(routes!(webhook::post_webhook_knowledge)) .routes(routes!(webhook::post_webhook_config_pr)) .routes(routes!(approvals::post_approve)) .routes(routes!(approvals::post_deny)) diff --git a/hive-c0re/src/dashboard/webhook.rs b/hive-c0re/src/dashboard/webhook.rs index 248c6c03..ce993841 100644 --- a/hive-c0re/src/dashboard/webhook.rs +++ b/hive-c0re/src/dashboard/webhook.rs @@ -1,12 +1,18 @@ //! Forgejo webhook endpoints. //! -//! - **`/webhook/knowledge`** — push events on `internal/knowledge` trigger a -//! `git pull` on the local clone so agents see up-to-date docs. //! - **`/webhook/config-pr`** — `pull_request` events on any `agent-configs/*` //! repo queue a [`hive_sh4re::approvals::ApprovalKind::MergeConfigPr`] approval row //! so the operator can review + approve the merge from the dashboard. //! -//! Both endpoints are reached via the gateway (HTTPS, public domain URL) so +//! There was a second endpoint here, `/webhook/knowledge`, which pulled the +//! local `internal/knowledge` clone on push. It is gone along with the +//! per-hive registration that fed it: a webhook has exactly one target URL, +//! so every hive registering one against the shared repository was +//! last-writer-wins. The swarm controller now holds the single registration +//! and addresses an event to each hive over the queue, which +//! [`crate::workers::knowledge`] documents. +//! +//! The endpoint is reached via the gateway (HTTPS, public domain URL) so //! Forgejo's SSRF guard does not block delivery. Each delivery is verified //! against the `X-Hub-Signature-256` HMAC header Forgejo attaches; the //! shared secret is auto-generated at startup and persisted to @@ -42,104 +48,6 @@ fn verify_hmac(state: &AppState, headers: &HeaderMap, body: &Bytes) -> Result<() crate::webhook_secret::verify_signature(secret, body, sig).map_err(|e| e.to_string()) } -// ── knowledge webhook ────────────────────────────────────────────────────────── - -/// Minimal Forgejo push-webhook payload — only the fields we care about. -#[derive(Deserialize)] -pub(super) struct PushWebhookPayload { - #[serde(rename = "ref")] - git_ref: Option, - repository: Option, -} - -#[derive(Deserialize)] -pub(super) struct PushWebhookRepo { - full_name: Option, -} - -/// POST `/webhook/knowledge` — Forgejo push webhook for -/// `internal/knowledge`. -/// -/// Runs `git pull` on the local clone so agents see up-to-date documents -/// on their next turn. -/// -/// Expected Forgejo webhook configuration: -/// - URL: `https:///webhook/knowledge` -/// - Content type: `application/json` -/// - Event: "Push" (fires on merge commits to main as well) -/// - Secret: auto-generated HMAC key (see [`crate::webhook_secret`]) -/// -/// The gateway routes `/webhook/` → hive-c0re; the HMAC secret protects -/// the endpoint from unauthenticated callers. -#[utoipa::path( - post, - path = "/webhook/knowledge", - request_body( - content = String, - content_type = "application/json", - description = "Forgejo push-webhook payload, taken as raw bytes \ - (not a typed extractor) so HMAC verification runs \ - over the exact wire bytes before any JSON parsing" - ), - responses( - (status = 200, description = "processed (pull triggered or ignored)", body = String), - (status = 400, description = "invalid JSON payload"), - (status = 401, description = "bad or missing HMAC signature"), - (status = 503, description = "HMAC secret unavailable at startup"), - ), - tag = "webhook" -)] -pub(super) async fn post_webhook_knowledge( - State(state): State, - headers: HeaderMap, - body: Bytes, -) -> Response { - if let Err(e) = verify_hmac(&state, &headers, &body) { - tracing::warn!("webhook/knowledge: HMAC verification failed: {e}"); - let status = if e.contains("unavailable") { - StatusCode::SERVICE_UNAVAILABLE - } else { - StatusCode::UNAUTHORIZED - }; - return (status, e).into_response(); - } - - let payload = match serde_json::from_slice::(&body) { - Ok(p) => p, - Err(e) => { - tracing::warn!("webhook/knowledge: JSON parse error: {e}"); - return (StatusCode::BAD_REQUEST, "invalid JSON").into_response(); - } - }; - - let expected_repo = format!("{}/{}", crate::knowledge::ORG, crate::knowledge::REPO); - let full_name = payload - .repository - .as_ref() - .and_then(|r| r.full_name.as_deref()) - .unwrap_or(""); - if full_name != expected_repo { - tracing::debug!( - full_name, - "webhook/knowledge: ignoring push from unexpected repo" - ); - return (StatusCode::OK, "ignored").into_response(); - } - let git_ref = payload.git_ref.as_deref().unwrap_or(""); - if git_ref != "refs/heads/main" { - tracing::debug!(git_ref, "webhook/knowledge: ignoring non-main push"); - return (StatusCode::OK, "ignored").into_response(); - } - tracing::info!("webhook/knowledge: pull triggered by push to {expected_repo}"); - let coord = state.coord.clone(); - tokio::spawn(async move { - if let Err(e) = crate::knowledge::pull(&coord).await { - tracing::warn!(error = ?e, "webhook/knowledge: pull failed"); - } - }); - (StatusCode::OK, "ok").into_response() -} - // ── config-PR webhook ────────────────────────────────────────────────────────── /// Minimal Forgejo `pull_request`-webhook payload. diff --git a/hive-c0re/src/job_queue/exec.rs b/hive-c0re/src/job_queue/exec.rs index 51578c0e..5d55b9cf 100644 --- a/hive-c0re/src/job_queue/exec.rs +++ b/hive-c0re/src/job_queue/exec.rs @@ -179,10 +179,18 @@ async fn run_matrix_sweep() -> Result<()> { } } -/// Boot-time Forgejo webhook registration as a DAG node — see +/// Boot-time Forgejo webhook management as a DAG node — see /// [`NodeKind::WebhookRegister`]. Mirrors the guard chain the /// `tokio::spawn` block it replaced used: no-op (not an error) when the /// HMAC secret, core token, or hive domain aren't available yet. +/// +/// The node now does one of each: it still registers the config-PR hook, +/// and it *removes* the knowledge one. A knowledge push is delivered to +/// the swarm controller, which addresses an event to each hive over the +/// queue — so a hive holding its own registration is holding a shared +/// resource only one party can own. The removal runs every boot rather +/// than behind a marker because it is already idempotent: it is a no-op +/// the moment the hook is gone. async fn run_webhook_register() -> Result<()> { let Ok(webhook_secret) = crate::webhook_secret::load_or_generate() else { tracing::debug!("webhook secret unavailable; skipping hook registration"); @@ -198,10 +206,8 @@ async fn run_webhook_register() -> Result<()> { tracing::debug!("HYPERHIVE_HIVE_DOMAIN unset; skipping webhook registration"); return Ok(()); }; - if let Err(e) = - crate::workers::knowledge::ensure_webhook(&token, &domain, &webhook_secret).await - { - tracing::warn!(error = ?e, "knowledge: ensure_webhook failed"); + if let Err(e) = crate::workers::knowledge::remove_webhook(&token, &domain).await { + tracing::warn!(error = ?e, "knowledge: remove_webhook failed"); } if let Err(e) = crate::forge::ensure_config_pr_webhook(&token, &domain, &webhook_secret).await { tracing::warn!(error = ?e, "forge: ensure_config_pr_webhook failed"); diff --git a/hive-c0re/src/workers/knowledge.rs b/hive-c0re/src/workers/knowledge.rs index dbd249a9..c2e36079 100644 --- a/hive-c0re/src/workers/knowledge.rs +++ b/hive-c0re/src/workers/knowledge.rs @@ -6,15 +6,18 @@ //! contribute by forking the repo and opening PRs — they never write //! to the bind-mounted path inside the container. //! -//! hive-c0re maintains the local clone. A Forgejo webhook notifies it -//! on push to main so agents always see an up-to-date snapshot. The -//! webhook is auto-created by [`ensure_webhook`] at startup. A +//! hive-c0re maintains the local clone. It learns that the repository +//! moved from the **swarm controller**, which owns the one Forgejo +//! webhook and addresses an event to each hive over the queue; a //! periodic pull in `main.rs` provides a fallback cadence. - -use std::collections::BTreeMap; +//! +//! A hive used to register that webhook itself, pointing at its own +//! `/webhook/knowledge`. A webhook has exactly one target URL, so with +//! more than one hive that was last-writer-wins rather than idempotent — +//! every hive but the most recent silently stopped receiving deliveries. +//! [`remove_webhook`] is the migration off it. use anyhow::{Context, Result}; -use forgejo_api::structs::{CreateHookOption, CreateHookOptionConfig, CreateHookOptionType}; use crate::coordinator::Coordinator; use crate::forge::{core_auth_header, forge_git_url}; @@ -142,103 +145,71 @@ async fn seed_readme(core_token: &str) -> Result<()> { } } -/// Ensure a Forgejo push webhook for `internal/knowledge` exists and -/// points at hive-c0re's `/webhook/knowledge` endpoint. Idempotent — -/// lists existing hooks first and skips creation when one is already -/// targeting the correct URL. +/// Delete this hive's own `internal/knowledge` push webhook if it is +/// still registered, so the swarm controller is the only party holding +/// one. /// -/// `hive_domain` is the public domain name of the hive; the webhook URL is -/// `https:///webhook/knowledge` (routed through the gateway, -/// avoiding the Forgejo SSRF guard that blocks loopback delivery). +/// # Why this is a migration and not just a deletion /// -/// `webhook_secret` is the HMAC secret Forgejo will attach as -/// `X-Hub-Signature-256` on each delivery; hive-c0re verifies this header -/// in the dashboard webhook handler (`post_webhook_knowledge`). +/// Not registering any more fixes nothing on a hive that has already +/// run: the hook it created persists on the forge, so the contention +/// this removes would survive on exactly the deployments that have it +/// while fresh installs looked fixed. The hive that created a hook is +/// the one that removes it. /// -/// Called at startup alongside [`ensure_local_clone`]. No-op when the -/// core token is absent (forge not yet provisioned). -pub async fn ensure_webhook( - core_token: &str, - hive_domain: &str, - webhook_secret: &str, -) -> Result<()> { +/// # It removes only its OWN hook, never a neighbour's +/// +/// The match is the full URL, not the `/webhook/knowledge` suffix. A +/// hook with that suffix and a different base belongs to *another hive* — +/// one that may not have been upgraded yet — and deleting it would break +/// its knowledge sync until it was. Reaping a neighbour's registration is +/// the very behaviour this issue is about; doing it in the name of fixing +/// it would just invert the direction. +/// +/// (The predecessor did reap by suffix, to clear loopback hooks left by +/// an older single-hive layout. That was safe when a hive was alone on +/// its forge and is not safe now.) +/// +/// A listing failure is an error rather than a silent skip: there is no +/// create attempt left to fall through to, so swallowing it would leave +/// the hook in place with nothing said. The caller logs and continues — +/// boot does not depend on this. +pub async fn remove_webhook(core_token: &str, hive_domain: &str) -> Result<()> { // The typed client carries no per-request timeout, so each call is // wrapped in one: this runs as a detached startup task, and a forge - // that accepts connections but never answers would otherwise hang - // it forever (and the hourly pull fallback masks the missing hook). + // that accepts connections but never answers would otherwise hang it + // forever. const HTTP_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); - let target_url = format!("https://{hive_domain}/webhook/knowledge"); + let own_url = format!("https://{hive_domain}/webhook/knowledge"); let client = crate::forge::api(core_token)?; - // List existing hooks — skip creation if ours is already there. - // Best-effort like the raw-HTTP predecessor: a listing failure - // falls through to the create attempt. - let listed = tokio::time::timeout(HTTP_TIMEOUT, client.repo_list_hooks(ORG, REPO).all()) - .await - .map_err(anyhow::Error::from) - .and_then(|r| r.map_err(anyhow::Error::from)); - match listed { - Ok(hooks) => { - let already_exists = hooks.iter().any(|h| { - h.config - .as_ref() - .and_then(|c| c.get("url")) - .map(String::as_str) - == Some(target_url.as_str()) - }); - if already_exists { - tracing::debug!(%target_url, "knowledge: push webhook already configured"); - return Ok(()); - } - // Delete stale hooks that point at our path but a different base - // (e.g. old loopback hooks from before the SSRF-bypass migration). - for h in &hooks { - let hook_url = h - .config - .as_ref() - .and_then(|c| c.get("url")) - .map_or("", String::as_str); - if hook_url.ends_with("/webhook/knowledge") - && hook_url != target_url - && let Some(id) = h.id - { - tracing::info!(hook_url, "knowledge: deleting stale webhook (wrong base)"); - let _ = tokio::time::timeout( - HTTP_TIMEOUT, - client.repo_delete_hook(ORG, REPO, id).send(), - ) - .await; - } - } - } - Err(e) => { - tracing::debug!(error = %e, "knowledge: listing hooks failed; attempting create"); - } - } - - // Create the webhook. - let mut additional = BTreeMap::new(); - additional.insert("secret".to_owned(), webhook_secret.to_owned()); - - let hook = CreateHookOption { - active: Some(true), - authorization_header: None, - branch_filter: None, - config: CreateHookOptionConfig { - content_type: "json".to_owned(), - url: url::Url::parse(&target_url).context("parse webhook target url")?, - additional, - }, - events: Some(vec!["push".to_owned()]), - r#type: CreateHookOptionType::Forgejo, - }; - tokio::time::timeout(HTTP_TIMEOUT, client.repo_create_hook(ORG, REPO, hook)) + let hooks = tokio::time::timeout(HTTP_TIMEOUT, client.repo_list_hooks(ORG, REPO).all()) .await .map_err(anyhow::Error::from) .and_then(|r| r.map_err(anyhow::Error::from)) - .with_context(|| format!("create webhook for {ORG}/{REPO}"))?; - tracing::info!(%target_url, "knowledge: push webhook created"); + .with_context(|| format!("list webhooks for {ORG}/{REPO}"))?; + + for h in &hooks { + let hook_url = h + .config + .as_ref() + .and_then(|c| c.get("url")) + .map_or("", String::as_str); + if hook_url == own_url + && let Some(id) = h.id + { + tokio::time::timeout(HTTP_TIMEOUT, client.repo_delete_hook(ORG, REPO, id).send()) + .await + .map_err(anyhow::Error::from) + .and_then(|r| r.map_err(anyhow::Error::from)) + .with_context(|| format!("delete webhook {id} for {ORG}/{REPO}"))?; + tracing::info!( + %own_url, + "knowledge: removed this hive's push webhook — the swarm controller owns it now" + ); + } + } Ok(()) }