hive-c0re: bound forge API calls so a stalled forge can't wedge the swarm event loop
forgejo-api 0.11.1 takes no caller-built client, so a request to a forge that accepts the connection and never answers waited forever. Every forge::api() call site now goes through forge::bounded, a 10s timeout that errors naming the operation. The PR-comment calls run inline in swarm_status::drain_swarm_events, the one task serving both swarm subscriptions, so a stalled forge stopped both. Refs #5066
This commit is contained in:
parent
7214b4c22f
commit
2b0154be28
5 changed files with 111 additions and 41 deletions
|
|
@ -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> {
|
|||
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<F, T, E>(op: &str, request: F) -> Result<std::result::Result<T, E>>
|
||||
where
|
||||
F: IntoFuture<Output = std::result::Result<T, E>>,
|
||||
{
|
||||
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::<Result<(), forgejo_api::ForgejoError>>(),
|
||||
)
|
||||
.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]
|
||||
|
|
|
|||
|
|
@ -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<u64> {
|
|||
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(())
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<Repository, ForgejoError>, 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");
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
Loading…
Reference in a new issue