c0re: route approval execution through rebuild_queue (closes #436)
This commit is contained in:
parent
d494413c7f
commit
a4789760ed
2 changed files with 313 additions and 121 deletions
|
|
@ -50,7 +50,7 @@
|
|||
//! current run started).
|
||||
|
||||
use std::collections::VecDeque;
|
||||
use std::sync::{Arc, Mutex};
|
||||
use std::sync::Mutex;
|
||||
|
||||
use serde::Serialize;
|
||||
use tokio::sync::Notify;
|
||||
|
|
@ -103,6 +103,11 @@ pub enum QueueSource {
|
|||
/// Crash recovery path (future use — currently no auto-rebuild on
|
||||
/// crash, but the variant exists for the imminent feature).
|
||||
CrashRecover,
|
||||
/// Operator approved a pending `Approval` row on the dashboard.
|
||||
/// `QueueEntry.approval_id` points back at the source row so the
|
||||
/// worker can fetch the kind-specific payload (commit_ref, inputs,
|
||||
/// description) before dispatching.
|
||||
Approval,
|
||||
}
|
||||
|
||||
impl QueueSource {
|
||||
|
|
@ -112,6 +117,7 @@ impl QueueSource {
|
|||
QueueSource::MetaUpdate => "meta_update",
|
||||
QueueSource::AutoUpdate => "auto_update",
|
||||
QueueSource::CrashRecover => "crash_recover",
|
||||
QueueSource::Approval => "approval",
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -175,6 +181,14 @@ pub struct QueueEntry {
|
|||
/// serialised) when the entry kind doesn't have meaningful inputs.
|
||||
#[serde(default, skip_serializing_if = "Vec::is_empty")]
|
||||
pub inputs: Vec<String>,
|
||||
/// Source approval row id when this entry was created by an
|
||||
/// operator-approve POST (`source == Approval`). The worker uses
|
||||
/// it to re-fetch the kind-specific payload (commit_ref / inputs /
|
||||
/// description / fetched_sha) and to fire `ApprovalResolved` on
|
||||
/// completion. `None` for non-approval entries — preserved on
|
||||
/// the wire that way too.
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub approval_id: Option<i64>,
|
||||
}
|
||||
|
||||
/// How many terminal-state entries (`Done` / `Failed` / `Cancelled`)
|
||||
|
|
@ -240,7 +254,7 @@ impl RebuildQueue {
|
|||
reason: String,
|
||||
parent_id: Option<u64>,
|
||||
) -> u64 {
|
||||
self.enqueue_with_inputs(kind, agent, source, reason, parent_id, Vec::new())
|
||||
self.enqueue_full(kind, agent, source, reason, parent_id, Vec::new(), None)
|
||||
}
|
||||
|
||||
/// Same as `enqueue` but carries an `inputs` payload — used by
|
||||
|
|
@ -256,16 +270,43 @@ impl RebuildQueue {
|
|||
reason: String,
|
||||
parent_id: Option<u64>,
|
||||
inputs: Vec<String>,
|
||||
) -> u64 {
|
||||
self.enqueue_full(kind, agent, source, reason, parent_id, inputs, None)
|
||||
}
|
||||
|
||||
/// Full-shape enqueue — every `QueueEntry` field that's settable
|
||||
/// at submit time. Existing `enqueue` / `enqueue_with_inputs`
|
||||
/// delegate to this with `approval_id: None`; the approval-driven
|
||||
/// POST handlers (#436) call it directly with the source row's id
|
||||
/// so the worker can re-fetch the kind-specific payload.
|
||||
// 8/7 args: the queue entry has 6 independent submit-time fields plus
|
||||
// the inputs/approval_id pair specific to MetaUpdate and approval
|
||||
// entries. A builder struct would obscure the call sites; the
|
||||
// shorter `enqueue` / `enqueue_with_inputs` wrappers already cover
|
||||
// the common cases.
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
pub fn enqueue_full(
|
||||
&self,
|
||||
kind: QueueKind,
|
||||
agent: String,
|
||||
source: QueueSource,
|
||||
reason: String,
|
||||
parent_id: Option<u64>,
|
||||
inputs: Vec<String>,
|
||||
approval_id: Option<i64>,
|
||||
) -> u64 {
|
||||
let mut inner = self.inner.lock().expect("rebuild_queue mutex poisoned");
|
||||
// Dedup against a pending entry with the same (kind, agent) —
|
||||
// and, for MetaUpdate, the same `inputs` list (see method
|
||||
// docstring + #365 for why).
|
||||
// docstring + #365 for why). Approval-driven entries also
|
||||
// require the approval_id to match so two distinct approvals
|
||||
// for the same agent never collapse into one queue slot.
|
||||
for entry in inner.entries.iter_mut() {
|
||||
if entry.state == QueueState::Queued
|
||||
&& entry.kind == kind
|
||||
&& entry.agent == agent
|
||||
&& (kind != QueueKind::MetaUpdate || entry.inputs == inputs)
|
||||
&& entry.approval_id == approval_id
|
||||
{
|
||||
if !entry.reason.contains(&reason) {
|
||||
entry.reason.push_str(&format!("\nalso requested by: {reason}"));
|
||||
|
|
@ -288,6 +329,7 @@ impl RebuildQueue {
|
|||
finished_at: None,
|
||||
error: None,
|
||||
inputs,
|
||||
approval_id,
|
||||
};
|
||||
inner.entries.push_back(entry);
|
||||
// Wake the worker. `notify_one` is a no-op when there's no
|
||||
|
|
@ -462,31 +504,44 @@ pub async fn run_worker(coord: std::sync::Arc<crate::coordinator::Coordinator>)
|
|||
|
||||
/// Run a single queue entry to completion. Kind-dispatched; failures
|
||||
/// bubble up to the worker which marks the entry `Failed`.
|
||||
///
|
||||
/// Approval-driven entries (`approval_id.is_some()`) route through
|
||||
/// `actions::run_approval_*` which carry the kind-specific commit
|
||||
/// pipeline + the `ApprovalResolved` event fan-out. Non-approval
|
||||
/// entries hit the original auto/manual rebuild paths.
|
||||
async fn dispatch(
|
||||
coord: &std::sync::Arc<crate::coordinator::Coordinator>,
|
||||
entry: &QueueEntry,
|
||||
) -> anyhow::Result<()> {
|
||||
match entry.kind {
|
||||
QueueKind::Rebuild => {
|
||||
match (entry.kind, entry.approval_id) {
|
||||
(QueueKind::Rebuild, Some(approval_id)) => {
|
||||
crate::actions::run_approval_apply_commit(coord, approval_id).await
|
||||
}
|
||||
(QueueKind::Rebuild, None) => {
|
||||
let current_rev = crate::auto_update::current_flake_rev(&coord.hyperhive_flake)
|
||||
.unwrap_or_default();
|
||||
crate::auto_update::rebuild_agent(coord, &entry.agent, ¤t_rev).await
|
||||
}
|
||||
QueueKind::MetaUpdate => run_meta_update(coord, entry).await,
|
||||
QueueKind::Spawn => {
|
||||
// First-deploy spawns route through `actions::approve_spawn`
|
||||
// / `actions::approve_apply_commit` today; they enqueue a
|
||||
// Spawn entry only to claim the queue slot, the actual
|
||||
// spawn work runs inside those handlers before completion.
|
||||
// Keeping this arm a no-op so we don't double-run.
|
||||
(QueueKind::MetaUpdate, Some(approval_id)) => {
|
||||
crate::actions::run_approval_update_meta_inputs(coord, approval_id).await
|
||||
}
|
||||
(QueueKind::MetaUpdate, None) => run_meta_update(coord, entry).await,
|
||||
(QueueKind::Spawn, Some(approval_id)) => {
|
||||
crate::actions::run_approval_spawn(coord, approval_id).await
|
||||
}
|
||||
(QueueKind::Spawn, None) => {
|
||||
// No non-approval Spawn caller today. The variant exists so
|
||||
// operator-triggered `RequestSpawn` (deprecated) and the
|
||||
// future direct-spawn admin path can route through here
|
||||
// without a wire change.
|
||||
tracing::debug!(
|
||||
id = entry.id,
|
||||
agent = %entry.agent,
|
||||
"rebuild_queue: Spawn entry is a queue claim; actual work elsewhere"
|
||||
"rebuild_queue: Spawn entry without approval_id is a no-op"
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
QueueKind::Destroy => {
|
||||
(QueueKind::Destroy, _) => {
|
||||
// Reserved for future `destroy --purge` integration.
|
||||
anyhow::bail!("Destroy kind not yet implemented in rebuild_queue worker");
|
||||
}
|
||||
|
|
@ -893,6 +948,66 @@ mod tests {
|
|||
assert_eq!(find(c).state, QueueState::Queued);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn approval_entries_keep_approval_id() {
|
||||
let q = RebuildQueue::new();
|
||||
let id = q.enqueue_full(
|
||||
QueueKind::Rebuild,
|
||||
"agent-a".to_owned(),
|
||||
QueueSource::Approval,
|
||||
"approval #42 apply commit".to_owned(),
|
||||
None,
|
||||
Vec::new(),
|
||||
Some(42),
|
||||
);
|
||||
let snap = q.snapshot();
|
||||
let entry = snap.iter().find(|e| e.id == id).expect("entry present");
|
||||
assert_eq!(entry.approval_id, Some(42));
|
||||
assert_eq!(entry.source, QueueSource::Approval);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn approval_entries_dedup_only_on_matching_id() {
|
||||
// Two pending approval-driven entries for the same agent but
|
||||
// DIFFERENT approval ids must NOT collapse — each operator
|
||||
// approve click is a separate piece of work even when the
|
||||
// (kind, agent) pair matches.
|
||||
let q = RebuildQueue::new();
|
||||
let a = q.enqueue_full(
|
||||
QueueKind::Rebuild,
|
||||
"agent-a".to_owned(),
|
||||
QueueSource::Approval,
|
||||
"approval #1".to_owned(),
|
||||
None,
|
||||
Vec::new(),
|
||||
Some(1),
|
||||
);
|
||||
let b = q.enqueue_full(
|
||||
QueueKind::Rebuild,
|
||||
"agent-a".to_owned(),
|
||||
QueueSource::Approval,
|
||||
"approval #2".to_owned(),
|
||||
None,
|
||||
Vec::new(),
|
||||
Some(2),
|
||||
);
|
||||
assert_ne!(a, b);
|
||||
assert_eq!(q.snapshot().len(), 2);
|
||||
// Same approval_id submitted twice DOES dedup (rapid double-
|
||||
// click on the dashboard's approve button is a single op).
|
||||
let c = q.enqueue_full(
|
||||
QueueKind::Rebuild,
|
||||
"agent-a".to_owned(),
|
||||
QueueSource::Approval,
|
||||
"approval #1 (duplicate)".to_owned(),
|
||||
None,
|
||||
Vec::new(),
|
||||
Some(1),
|
||||
);
|
||||
assert_eq!(a, c);
|
||||
assert_eq!(q.snapshot().len(), 2);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn cancel_children_skips_running_and_terminal() {
|
||||
let q = RebuildQueue::new();
|
||||
|
|
|
|||
Loading…
Reference in a new issue