fix(#2591): validate() rejects out-of-bounds/forward parent index (argus review)

This commit is contained in:
atlas 2026-07-20 21:58:42 +02:00 committed by mara
commit 456847eaa1
2 changed files with 29 additions and 3 deletions

View file

@ -256,9 +256,9 @@ pub fn meta_update(
// per-agent child DAGs.
/// Validate a spec before it enters the queue: node ids are dense
/// (index = id), deps reference existing nodes, and the dep graph is
/// acyclic (petgraph `toposort`). Rejecting cycles here fixes the old
/// queue's documented "circular dep silently deadlocks forever" caveat.
/// (index = id), deps + parents reference existing *earlier* nodes, and the
/// dep graph is acyclic (petgraph `toposort`). Rejecting cycles here fixes the
/// old queue's documented "circular dep silently deadlocks forever" caveat.
pub fn validate(spec: &DagSpec) -> Result<()> {
if spec.nodes.is_empty() {
bail!("dag spec {:?} has no nodes", spec.template);
@ -269,6 +269,17 @@ pub fn validate(spec: &DagSpec) -> Result<()> {
.map(|i| graph.add_node(u32::try_from(i).unwrap_or(u32::MAX)))
.collect();
for (i, node) in spec.nodes.iter().enumerate() {
// A `parent` must index an earlier node — `insert_group` resolves it to
// an already-inserted `NodeId`, so a forward/out-of-bounds parent would
// otherwise panic there.
if let Some(p) = node.parent
&& usize::try_from(p).is_ok_and(|p| p >= i)
{
bail!(
"dag spec {:?} node {i} has invalid parent {p} (must be an earlier node)",
spec.template
);
}
for dep in &node.deps {
let Some(&dep_idx) = usize::try_from(dep.on).ok().and_then(|i| idx.get(i)) else {
bail!(

View file

@ -147,6 +147,21 @@ fn unknown_dep_is_rejected_at_submit() {
assert!(q.submit(spec).is_err());
}
#[test]
fn invalid_parent_is_rejected_at_submit() {
let q = JobQueue::new(1);
let mut spec = rebuild("agent-a", "bad parent");
// A forward/out-of-bounds parent index must be refused at validate, not
// panic in `insert_group`.
spec.nodes = vec![NodeSpec {
agent: "agent-a".to_owned(),
kind: NodeKind::Reconcile,
deps: Vec::new(),
parent: Some(3),
}];
assert!(q.submit(spec).is_err());
}
// ---- dependency order within a DAG ----
#[test]