diff --git a/swarm-controller/src/forge.rs b/swarm-controller/src/forge.rs index dc1f67da..21961310 100644 --- a/swarm-controller/src/forge.rs +++ b/swarm-controller/src/forge.rs @@ -191,7 +191,11 @@ impl Client { } /// Create `name` inside [`CONFIG_ORG`]. Idempotent: an existing repo - /// (409/422) is folded into success. + /// (409, or a 422 confirmed by a follow-up `repo_get`) is folded into + /// success. A 422 that is *not* an existing repo — forgejo also uses + /// it for other repo-create validation failures — fails with + /// forgejo's own message rather than being folded silently (see + /// [`folds_into_success`]). async fn ensure_org_repo(&self, name: &str) -> Result<()> { match self .api @@ -202,11 +206,17 @@ impl Client { tracing::info!(%name, "swarm forge: created repo in {CONFIG_ORG}"); Ok(()) } - Err(e) if is_already_exists(&e) => { - tracing::debug!(%name, "swarm forge: repo already exists"); - Ok(()) + Err(e) => { + let existing = is_ambiguous_validation_failure(&e) + && self.api.repo_get(CONFIG_ORG, name).await.is_ok(); + if folds_into_success(&e, existing) { + tracing::debug!(%name, "swarm forge: repo already exists"); + Ok(()) + } else { + tracing::error!(%name, error = %e, "swarm forge: forge refused to create repo"); + Err(e).with_context(|| format!("create repo {CONFIG_ORG}/{name}")) + } } - Err(e) => Err(e).with_context(|| format!("create repo {CONFIG_ORG}/{name}")), } } @@ -362,25 +372,25 @@ impl Client { /// but not its mechanism: that function shells out to the local /// `forgejo admin` CLI, which assumes co-location with the forge host. /// This daemon has no such assumption — like every other call in this - /// file, it only ever talks to the forge over HTTP — so this goes - /// through `admin_create_user` instead. + /// file, it only ever talks to the forge over HTTP — so this goes through + /// `admin_create_user` instead. /// /// The password itself is a throwaway: 32 random bytes, generated once, /// never persisted anywhere, and never needed again (unlike /// `hive-c0re`'s CLI path, which can ask forgejo to `--random-password` /// on its own, the HTTP admin API requires a real value up front — see - /// [`crate::webhook::generate_hex_secret`], reused here rather than - /// duplicated for the same reason a webhook secret and this password - /// are both "32 random bytes nothing reads back"). + /// [`crate::webhook::generate_hex_secret`], reused here for the same + /// reason a webhook secret and this password are both "32 random bytes + /// nothing reads back"). /// - /// Idempotent: an existing user (409/422) is folded into success, same - /// as [`Self::ensure_org_repo`]. Deliberately does not attempt to align - /// the account's email or disable its own repo-creation rights the way - /// `hive-c0re`'s per-hive provisioning does (`ensure_user_email`, - /// `ensure_repo_creation_disabled`) — this account only exists so - /// `AddRepoMember` has something to add, and the agent's owning hive - /// still runs its own full provisioning pass once the agent actually - /// spawns there, which self-heals both of those. + /// Idempotent, same as [`Self::ensure_org_repo`]: 409 folds to success + /// unconditionally, 422 only after a follow-up `user_get` confirms the + /// user (see [`folds_into_success`] — not every 422 means it exists). + /// Deliberately does not align the account's email or disable its own + /// repo-creation rights the way `hive-c0re`'s per-hive provisioning does + /// (`ensure_user_email`, `ensure_repo_creation_disabled`) — this account + /// only exists so `AddRepoMember` has something to add, and the owning + /// hive's own provisioning pass self-heals both once the agent spawns. pub async fn ensure_agent_user(&self, agent: &str) -> Result<()> { let password = crate::webhook::generate_hex_secret() .context("generating a throwaway password for the agent's forge account")?; @@ -405,11 +415,17 @@ impl Client { tracing::info!(%agent, "swarm forge: created agent forge user"); Ok(()) } - Err(e) if is_already_exists(&e) => { - tracing::debug!(%agent, "swarm forge: agent forge user already exists"); - Ok(()) + Err(e) => { + let existing = + is_ambiguous_validation_failure(&e) && self.api.user_get(agent).await.is_ok(); + if folds_into_success(&e, existing) { + tracing::debug!(%agent, "swarm forge: agent forge user already exists"); + Ok(()) + } else { + tracing::error!(%agent, error = %e, "swarm forge: forge refused to create agent user"); + Err(e).with_context(|| format!("create forge user {agent}")) + } } - Err(e) => Err(e).with_context(|| format!("create forge user {agent}")), } } @@ -1048,6 +1064,55 @@ fn is_already_exists(e: &ForgejoError) -> bool { } } +/// Whether a create-call failure is forgejo's unambiguous "already +/// exists" signal — HTTP 409. Unlike a 422 (see +/// [`is_ambiguous_validation_failure`]), nothing else produces a 409 on +/// these create calls, so folding it into success needs no follow-up +/// confirmation. +fn is_confirmed_conflict(e: &ForgejoError) -> bool { + match e { + ForgejoError::ApiError(api) => { + matches!(api.error_kind(), ApiErrorKind::Other(s) if *s == StatusCode::CONFLICT) + } + ForgejoError::UnexpectedStatusCode(s) => *s == StatusCode::CONFLICT, + _ => false, + } +} + +/// Whether a create-call failure is forgejo's 422 "validation failed" — +/// ambiguous on its own. Forgejo folds several distinct causes into this +/// one status; an admin user create alone answers 422 for +/// `ErrUserAlreadyExist`, `ErrEmailAlreadyUsed`, `ErrNameReserved`, +/// `ErrNameCharsNotAllowed`, `ErrEmailInvalid` and +/// `ErrNamePatternNotAllowed` — only the first of those means the object +/// exists. A 422 alone is never enough to fold into success; the caller +/// must confirm with a follow-up read first (see [`folds_into_success`]). +fn is_ambiguous_validation_failure(e: &ForgejoError) -> bool { + match e { + ForgejoError::ApiError(api) => matches!(api.error_kind(), ApiErrorKind::ValidationFailed), + ForgejoError::UnexpectedStatusCode(s) => *s == StatusCode::UNPROCESSABLE_ENTITY, + _ => false, + } +} + +/// Whether a create-call failure `e` should be folded into success, given +/// whether a follow-up read found the object already present. +/// `existing` is only consulted for the ambiguous 422 case — a 409 is +/// trusted on its own, and any other error always fails. +fn folds_into_success(e: &ForgejoError, existing: bool) -> bool { + is_confirmed_conflict(e) || (is_ambiguous_validation_failure(e) && existing) +} + +#[cfg(test)] +fn validation_failed_error() -> ForgejoError { + ForgejoError::ApiError(ApiErrorKind::ValidationFailed.into()) +} + +#[cfg(test)] +fn conflict_error() -> ForgejoError { + ForgejoError::ApiError(ApiErrorKind::Other(StatusCode::CONFLICT).into()) +} + #[cfg(test)] mod tests { use super::*; @@ -1110,4 +1175,40 @@ mod tests { let adj = HashMap::from([(1, vec![2]), (2, vec![1])]); assert_eq!(transitive_reach(1, &adj), 1); } + + // The three cases from the `forge-422-fold` issue: a create call's + // 422 with the object confirmed absent must fail (case 1), the same + // 422 with the object confirmed present must fold to success (case + // 2), and a 409 must fold to success without needing a follow-up + // read at all (case 3). + #[test] + fn folds_into_success_case1_422_absent_fails() { + assert!(!folds_into_success(&validation_failed_error(), false)); + } + + #[test] + fn folds_into_success_case2_422_present_is_done() { + assert!(folds_into_success(&validation_failed_error(), true)); + } + + #[test] + fn folds_into_success_case3_conflict_is_done() { + // `existing` is never consulted for a 409 — pass `false` to prove + // that, not just `true`. + assert!(folds_into_success(&conflict_error(), false)); + } + + // Invert-proof: the old `is_already_exists` folded every 422 into + // success regardless of whether the object existed — exactly the + // defect `folds_into_success` fixes. Demonstrate the old logic + // getting case 1 wrong before trusting the new one. + #[test] + fn old_is_already_exists_wrongly_returns_true_for_an_absent_object() { + let e = validation_failed_error(); + assert!(is_already_exists(&e), "old logic: false positive on 422"); + assert!( + !folds_into_success(&e, false), + "new logic: correctly refuses to fold a 422 with no confirmed existence" + ); + } }