From 39a0a313dc4abc85590193344c8afd84ecbe5bd8 Mon Sep 17 00:00:00 2001 From: atlas Date: Wed, 26 Aug 2026 22:10:04 +0200 Subject: [PATCH] hive-priv: delete the runner credentials before restarting the CI runner Upstream's ExecStartPre re-registers only when .runner is absent, the labels changed, or the registration token hash changed -- never when the instance URL changed. So writing a fresh token and restarting the runner registers only if the hash happens to differ, which is Forgejo's choice to make: whether the admin registration-token endpoint mints a new token per request or hands back a stable one is not ours to depend on. hive-c0re already decides correctly -- ensure_ci_runner_registered only reaches this helper once it has concluded the credentials are absent or stale -- but the remediation was a no-op, so re-registration was requested every boot and never happened. Remove .runner before the restart so upstream takes its absence branch, the one it evaluates unconditionally. NotFound is success; any other error propagates rather than reporting Ok for a registration that never ran. --- hive-priv/src/main.rs | 110 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 108 insertions(+), 2 deletions(-) diff --git a/hive-priv/src/main.rs b/hive-priv/src/main.rs index 7abaf198..53a4bceb 100644 --- a/hive-priv/src/main.rs +++ b/hive-priv/src/main.rs @@ -853,6 +853,37 @@ async fn restart_matrix_daemon(agent_name: &str) -> Result<(String, String)> { )) } +/// Host path to the hive-ci runner's persisted registration credentials. +/// +/// Paired with `hive-c0re`'s `forge::ci_runner::RUNNER_FILE`, which reads the +/// same file to decide whether a runner is registered and whether it still +/// names the configured forge host. Deliberately duplicated rather than shared: +/// `hive-priv` is the minimal root helper and does not depend on `hive-c0re`. +const RUNNER_CREDENTIALS: &str = + "/var/lib/nixos-containers/hive-ci/var/lib/gitea-runner/hive/.runner"; + +/// Delete the runner's persisted credentials so upstream's `ExecStartPre` takes +/// its **absence** branch on the next start. +/// +/// Absence is the state we want, so `NotFound` is success. Anything else — a +/// permission error above all — is NOT swallowed: it means the file is still +/// there, the restart will take upstream's already-registered branch, and the +/// caller would return `Ok` for a registration that never happened. That is the +/// same shape as a precondition that "passes" because it could not read the file +/// it was checking, and it is worth failing loudly to avoid. +/// +/// Split from [`register_ci_runner`] purely so this rule is testable without a +/// container or a `systemctl`. +fn clear_runner_credentials(path: &str) -> Result<()> { + match std::fs::remove_file(path) { + Ok(()) => Ok(()), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(e) => { + Err(anyhow::Error::new(e).context(format!("remove stale runner credentials {path}"))) + } + } +} + /// `RegisterCiRunner` — write the runner registration token to the host-side /// `/run/hive-ci/runner-token` env-file, then restart the in-container runner /// so it re-registers. The forge admin token never enters the container; only @@ -875,6 +906,27 @@ async fn register_ci_runner(token: &str) -> Result<(String, String)> { .with_context(|| format!("write {token_path}"))?; std::fs::set_permissions(token_path, std::fs::Permissions::from_mode(0o600)) .with_context(|| format!("chmod {token_path}"))?; + // Remove the persisted credentials BEFORE restarting, or the restart is a + // no-op as far as registration goes. + // + // Upstream's `ExecStartPre` only re-registers when `.runner` is absent, the + // labels changed, or the *registration token hash* changed — never when the + // instance URL changed. c0re only calls this helper once it has already + // decided the existing credentials are absent or stale + // (`forge::ci_runner::ensure_ci_runner_registered` returns early otherwise), + // so by the time we are here a re-registration is exactly what is wanted and + // deleting the file is the narrow way to guarantee it happens. + // + // Writing a fresh token is NOT sufficient on its own: whether the hash + // changes depends on whether the forge mints a new registration token per + // request or hands back a stable one, which is Forgejo's behaviour to + // choose and change. Gating our remediation on the absence branch — the one + // upstream evaluates unconditionally — makes that question moot instead of + // load-bearing. + // + // See [`clear_runner_credentials`] for why absence is the branch we aim at + // and why only `NotFound` counts as success. + clear_runner_credentials(RUNNER_CREDENTIALS)?; // Restart the in-container runner so it reads the new token and registers. let out = Command::new("systemctl") .args(["--machine=hive-ci", "restart", "gitea-runner-hive.service"]) @@ -2624,8 +2676,8 @@ async fn sync_agent_tmpfiles(agents: &[AgentTmpfilesEntry]) -> Result<(String, S mod tests { use super::{ BindMount, OwnedFd, PAUSED_MARKER_FILE, PrivRequest, check_fd_agreement, - contains_secret_shaped_run, git_overlay_flags, limits_dropin_body, redact_secret_line, - remove_marker_in, write_state_file_nofollow, + clear_runner_credentials, contains_secret_shaped_run, git_overlay_flags, + limits_dropin_body, redact_secret_line, remove_marker_in, write_state_file_nofollow, }; use std::path::PathBuf; use std::sync::atomic::{AtomicU32, Ordering}; @@ -2947,6 +2999,60 @@ mod tests { std::fs::remove_dir_all(&dir).ok(); } + /// The runner-credential clear, in all three states that matter. The + /// PRESENCE arm is the load-bearing one: an implementation that did nothing + /// at all would pass the "absent is fine" arm perfectly, and the whole point + /// of the call is that the file is *gone* afterwards — upstream re-registers + /// on absence and on nothing else we control. + #[test] + fn clearing_runner_credentials_removes_it_and_tolerates_absence() { + let dir = scratch(); + let path = dir.join(".runner"); + let path_str = path.to_str().unwrap(); + + // Absent → success (this is the state we are aiming for). + assert!(!path.exists()); + clear_runner_credentials(path_str).unwrap(); + + // Present → success AND actually gone. Without this arm a no-op passes. + std::fs::write(&path, r#"{"id":7,"address":"http://old.invalid"}"#).unwrap(); + assert!( + path.exists(), + "control: the file must exist before the clear" + ); + clear_runner_credentials(path_str).unwrap(); + assert!( + !path.exists(), + "stale credentials must be GONE, or the restart takes upstream's \ + already-registered branch and registration silently never happens" + ); + + std::fs::remove_dir_all(&dir).ok(); + } + + /// A failure that is not `NotFound` must propagate, never read as success. + /// A directory in the file's place makes `remove_file` fail with a non- + /// `NotFound` error without needing to drop privileges in a test. + #[test] + fn clearing_runner_credentials_propagates_a_real_failure() { + let dir = scratch(); + let path = dir.join(".runner"); + std::fs::create_dir(&path).unwrap(); + + let err = clear_runner_credentials(path.to_str().unwrap()) + .expect_err("a non-NotFound failure must NOT be reported as success"); + assert!( + format!("{err:#}").contains("remove stale runner credentials"), + "error must name what it failed to do, got: {err:#}" + ); + assert!( + path.exists(), + "nothing was removed, and the caller must know" + ); + + std::fs::remove_dir_all(&dir).ok(); + } + /// The pause marker round-trips through the same root-only path the /// credential writes use, and BOTH directions are idempotent — the /// dashboard toggle and `hivectl pause|resume` fire blind, without