diff --git a/hive-c0re/Cargo.toml b/hive-c0re/Cargo.toml index e3da65b7..ac74773f 100644 --- a/hive-c0re/Cargo.toml +++ b/hive-c0re/Cargo.toml @@ -62,5 +62,8 @@ problem_details = { version = "0.9.0", features = ["axum"] } utoipa.workspace = true utoipa-axum.workspace = true +# `test-util` for `#[tokio::test(start_paused = true)]`: `forge::bounded`'s +# test asserts the deadline fires without waiting it out. [dev-dependencies] tempfile = "3" +tokio = { workspace = true, features = ["test-util"] } diff --git a/hive-c0re/src/forge/mod.rs b/hive-c0re/src/forge/mod.rs index 7e1741bd..ed84f5ab 100644 --- a/hive-c0re/src/forge/mod.rs +++ b/hive-c0re/src/forge/mod.rs @@ -20,6 +20,7 @@ pub use repos::{ }; pub use users::core_token; +use std::future::IntoFuture; use std::sync::OnceLock; use std::time::{Duration, Instant}; @@ -195,6 +196,25 @@ pub(crate) fn api(token: &str) -> Result { Forgejo::new(Auth::Token(token), url).context("build forgejo api client") } +/// Upper bound on one Forgejo API request. `forgejo-api` takes no +/// caller-built client, so the request itself has no timeout; every call +/// through [`api`] is wrapped in [`bounded`]. +const API_TIMEOUT: Duration = Duration::from_secs(10); + +/// Await one Forgejo API request for at most [`API_TIMEOUT`]. +/// +/// The outer `Err` is the deadline, naming `op`; the inner `Result` is the +/// request's own outcome, left to the caller so it can still match on the +/// error (`409 already exists`, `401`, ...). +async fn bounded(op: &str, request: F) -> Result> +where + F: IntoFuture>, +{ + tokio::time::timeout(API_TIMEOUT, request) + .await + .map_err(|_| anyhow::anyhow!("{op}: no answer from the forge within {API_TIMEOUT:?}")) +} + /// Per-agent forge sync: grant read access to `core/meta`, and wire the /// `meta` remote in its proposed repo. All operations are idempotent; /// failures are logged as warnings but don't abort the caller. @@ -271,8 +291,6 @@ async fn ensure_all_orgs_and_repos(token: &str) { /// matching by path: the same path on another base is another hive's hook. /// Idempotent, and a no-op when `HYPERHIVE_HIVE_DOMAIN` is unset. async fn remove_config_pr_webhook(core_token: &str) -> Result<()> { - const HTTP_TIMEOUT: Duration = Duration::from_secs(10); - let Some(domain) = std::env::var("HYPERHIVE_HIVE_DOMAIN") .ok() .filter(|v| !v.is_empty()) @@ -281,17 +299,19 @@ async fn remove_config_pr_webhook(core_token: &str) -> Result<()> { }; let own_url = config_pr_hook_url(&domain); let client = api(core_token)?; - let hooks = tokio::time::timeout(HTTP_TIMEOUT, client.org_list_hooks(CONFIG_ORG).send()) - .await - .map_err(anyhow::Error::from) - .and_then(|r| r.map_err(anyhow::Error::from)) - .with_context(|| format!("list webhooks for org {CONFIG_ORG}"))?; + let hooks = bounded( + &format!("list webhooks for org {CONFIG_ORG}"), + client.org_list_hooks(CONFIG_ORG).send(), + ) + .await? + .with_context(|| format!("list webhooks for org {CONFIG_ORG}"))?; for id in hook_ids_at(&hooks, &own_url) { - tokio::time::timeout(HTTP_TIMEOUT, client.org_delete_hook(CONFIG_ORG, id).send()) - .await - .map_err(anyhow::Error::from) - .and_then(|r| r.map_err(anyhow::Error::from)) - .with_context(|| format!("delete webhook {id} on org {CONFIG_ORG}"))?; + bounded( + &format!("delete webhook {id} on org {CONFIG_ORG}"), + client.org_delete_hook(CONFIG_ORG, id).send(), + ) + .await? + .with_context(|| format!("delete webhook {id} on org {CONFIG_ORG}"))?; tracing::info!(%own_url, id, "forge: removed this hive's config-pr webhook"); } Ok(()) @@ -423,13 +443,44 @@ pub async fn ensure_all() { #[cfg(test)] mod tests { - use super::{config_pr_hook_url, describe_forge_admin, git_url_with_base, hook_ids_at}; + use super::{ + API_TIMEOUT, bounded, config_pr_hook_url, describe_forge_admin, git_url_with_base, + hook_ids_at, + }; fn hook(id: i64, url: &str) -> forgejo_api::structs::Hook { serde_json::from_value(serde_json::json!({ "id": id, "config": { "url": url } })) .expect("hook json") } + /// A forge that never answers surfaces as an error naming the operation, + /// at exactly [`API_TIMEOUT`] — not as a hang. + #[tokio::test(start_paused = true)] + async fn a_request_the_forge_never_answers_errors_at_the_deadline() { + let started = tokio::time::Instant::now(); + let err = bounded( + "post PR comment", + std::future::pending::>(), + ) + .await + .expect_err("a pending request must hit the deadline"); + assert_eq!(started.elapsed(), API_TIMEOUT); + assert!(err.to_string().contains("post PR comment"), "{err}"); + } + + /// The request's own result passes through for the caller to classify. + #[tokio::test(start_paused = true)] + async fn an_answered_request_keeps_its_own_result() { + let ok = bounded("op", async { Ok::<_, ()>(7) }) + .await + .expect("in time"); + assert_eq!(ok, Ok(7)); + let failed = bounded("op", async { Err::<(), _>("409") }) + .await + .expect("in time"); + assert_eq!(failed, Err("409")); + } + /// The same path on another base is another hive's hook, and removing it /// would cut that hive off. Only an exact URL match is this hive's. #[test] diff --git a/hive-c0re/src/forge/pr_comment.rs b/hive-c0re/src/forge/pr_comment.rs index a9244afa..3891b95c 100644 --- a/hive-c0re/src/forge/pr_comment.rs +++ b/hive-c0re/src/forge/pr_comment.rs @@ -4,7 +4,7 @@ use anyhow::{Context, Result}; -use super::{CONFIG_ORG, api, core_token, reader_token}; +use super::{CONFIG_ORG, api, bounded, core_token, reader_token}; /// Full `owner/name` path of an agent's config repo on the forge. pub fn config_repo(agent: &str) -> String { @@ -20,10 +20,12 @@ pub async fn merged_pr_for_commit(repo: &str, sha: &str) -> Result { let (owner, name) = repo .split_once('/') .with_context(|| format!("forge repo `{repo}` is not owner/name"))?; - let pull = api(reader.value())? - .repo_get_commit_pull_request(owner, name, sha) - .await - .context("GET pull request of commit")?; + let pull = bounded( + "GET pull request of commit", + api(reader.value())?.repo_get_commit_pull_request(owner, name, sha), + ) + .await? + .context("GET pull request of commit")?; pull.number .and_then(|n| u64::try_from(n).ok()) .with_context(|| format!("pull request of {sha} in {repo} carries no number")) @@ -40,8 +42,9 @@ pub async fn post_pr_comment(repo: &str, pr: u64, body: &str) -> Result<()> { .split_once('/') .with_context(|| format!("forge repo `{repo}` is not owner/name"))?; let index = i64::try_from(pr).with_context(|| format!("PR index {pr} overflows i64"))?; - api(&token)? - .issue_create_comment( + bounded( + "post PR comment", + api(&token)?.issue_create_comment( owner, name, index, @@ -49,8 +52,9 @@ pub async fn post_pr_comment(repo: &str, pr: u64, body: &str) -> Result<()> { body: body.to_owned(), updated_at: None, }, - ) - .await - .context("post PR comment")?; + ), + ) + .await? + .context("post PR comment")?; Ok(()) } diff --git a/hive-c0re/src/forge/repos.rs b/hive-c0re/src/forge/repos.rs index 6718d2ae..fecb73c6 100644 --- a/hive-c0re/src/forge/repos.rs +++ b/hive-c0re/src/forge/repos.rs @@ -16,8 +16,8 @@ use reqwest::StatusCode; use crate::coordinator::Coordinator; use super::{ - CONFIG_ORG, api, core_auth_header, core_token, forge_git_url, forge_http_base, is_present, - reader_token, + CONFIG_ORG, api, bounded, core_auth_header, core_token, forge_git_url, forge_http_base, + is_present, reader_token, }; /// Creation options for an empty repo defaulting to `main`. @@ -79,10 +79,13 @@ fn created_or_exists(res: Result, label: &str) -> Resu /// to the user we want the repo owned by (we use `core`'s token for /// `core/meta`). Idempotent. pub async fn ensure_repo(name: &str, token: &str) -> Result<()> { - let res = api(token)? - .create_current_user_repo(repo_option(name, true)) - .await; - created_or_exists(res, &format!("core/{name}")) + let label = format!("core/{name}"); + let res = bounded( + &format!("create repo {label}"), + api(token)?.create_current_user_repo(repo_option(name, true)), + ) + .await?; + created_or_exists(res, &label) } /// Dashboard-warning guard for the meta non-fast-forward condition. @@ -397,21 +400,24 @@ async fn add_collaborator( permission: AddCollaboratorOptionPermission, token: &str, ) -> Result<()> { - let res = api(token)? - .repo_add_collaborator( + let op = format!("add collaborator {user} to {owner}/{repo}"); + let res = bounded( + &op, + api(token)?.repo_add_collaborator( owner, repo, user, AddCollaboratorOption { permission: Some(permission), }, - ) - .await; + ), + ) + .await?; match res { Ok(()) => {} Err(ForgejoError::UnexpectedStatusCode(s)) if s == StatusCode::CREATED => {} Err(e) => { - return Err(e).with_context(|| format!("add collaborator {user} to {owner}/{repo}")); + return Err(e).context(op); } } tracing::debug!(%owner, %repo, %user, ?permission, "forge: collaborator set"); diff --git a/hive-c0re/src/forge/users.rs b/hive-c0re/src/forge/users.rs index cd511bb5..ba1b3ded 100644 --- a/hive-c0re/src/forge/users.rs +++ b/hive-c0re/src/forge/users.rs @@ -10,7 +10,7 @@ use forgejo_api::structs::UpdateUserAvatarOption; use forgejo_api::{ApiErrorKind, ForgejoError}; use reqwest::StatusCode; -use super::{api, forge_admin}; +use super::{api, bounded, forge_admin}; const TOKEN_NAME_PREFIX: &str = "hyperhive"; // Where the host-side `core` admin token lives. Used by hive-c0re itself @@ -172,12 +172,14 @@ pub(super) async fn ensure_core_avatar(token: &str) -> Result<()> { // `token` IS the core user's own token though, so updating "the // current user's avatar" (`POST /user/avatar`) is behaviorally // identical. - api(token)? - .user_update_avatar(UpdateUserAvatarOption { + bounded( + "set core avatar", + api(token)?.user_update_avatar(UpdateUserAvatarOption { image: Some(base64::engine::general_purpose::STANDARD.encode(&png_bytes)), - }) - .await - .context("set core avatar")?; + }), + ) + .await? + .context("set core avatar")?; if let Some(parent) = marker.parent() { std::fs::create_dir_all(parent).ok(); } @@ -232,7 +234,11 @@ async fn check_core_token(token: &str) -> CoreTokenCheck { let Ok(client) = api(token) else { return CoreTokenCheck::Indeterminate; }; - match client.user_get_current().await { + let Ok(probe) = bounded("probe core token", client.user_get_current()).await else { + tracing::debug!("forge: core-token probe timed out; treating as indeterminate"); + return CoreTokenCheck::Indeterminate; + }; + match probe { Ok(_) => CoreTokenCheck::Valid, Err(e) => { let outcome = classify_core_token_error(&e);