Skip to content

Commit 4030c68

Browse files
artemistEricson2314
authored andcommitted
make get_input_drvs return relation for dynamic inputs
wip Add dynamic derivation resolution Do dynamic derivation resolution, still doesn't splice tree Revert "Do dynamic derivation resolution, still doesn't splice tree" This reverts commit 389cc05. Simpler initial dynamic resolution dynamic dep prototype, needs testing Remove dynamic_deps, we can store relation in dynamic_rdeps instead enable dyn-drv test Let Claude fix dynamic derivation building in queue runner This was basically unsupervised, except for me running the test and feeding it the test logs. Four bugs prevented dynamic derivations from being built: - `get_dependents` only walked `rdeps`, not `dynamic_rdeps`, so steps connected via dynamic dependencies appeared orphaned and were "maybe cancelled" instead of dispatched. - `succeed_step` used `get_direct_builds` to find the owning build for dynamic rdeps processing. Intermediate steps (like `hello.drv.drv`) have no direct builds, so we now fall back to `get_dependents` to walk the dependency chain. - `make_rdeps_runnable` only processed `rdeps`, never removing the finished step from `dynamic_rdeps` dependents' forward `deps`. This left the wrapper permanently stuck with an unsatisfied dependency. - `resolve_dynamic_input` used `1..outputs.len()` which produces an empty range for single-element chains (the common case). Changed to `1..=outputs.len()` so the chain is actually resolved and the produced `.drv` file is discovered and built as a new step. Test updated from 2 to 5 expected build steps now that Hydra properly tracks the full dynamic derivation chain. Combine `rdeps` and `dynamic_rdeps` into a single `rdeps` list Both were reverse dependency lists differing only in whether they carried a `relation` (output name chain for dynamic derivations). Having two separate lists meant every operation that walked one had to remember to also walk the other — the exact class of bug fixed in the previous commit. Now `StepState::rdeps` is `Vec<ReverseDep>` where an empty `relation` signifies a regular (non-dynamic) reverse dependency. `get_dependents`, `make_rdeps_runnable`, and `add_referring_data` all operate on the single unified list. The `dynamic_rdeps_len` atomic counter, `clone_dyn_rdeps` method, and `DynamicReverseDep` type are removed in favor of the renamed `ReverseDep`. Resolve dynamic derivation chains one level at a time Instead of resolving the entire chain at once via a recursive SQL query in `resolve_dynamic_input`, resolve one level per step completion: when a step finishes, `pop_dynamic_rdeps` pops the next output name from the relation stack, and `succeed_step` looks it up directly in the build output — no DB round-trip needed. The relation vector is now stored in reverse order so that `pop()` gives the next level to resolve in O(1). `resolve_dynamic_input` is removed from both `State` and `db::Connection` since it is no longer needed. The `create_step` input processing no longer attempts eager resolution either; the dynamic rdeps mechanism in `succeed_step` handles it naturally as each step completes. TEMP add some asserts, see failure Fix CA resolution for intermediate steps, extract `DynDrvRelation` type Three changes: - **Fix premature build completion**: `realise_drv_on_valid_machine` passed `referring_build: Some(build)` when creating resolved steps for ALL CA derivations, not just the toplevel. This caused `succeed_step` to mark the entire build as finished when an intermediate resolved step completed (e.g. resolved `build-b`), orphaning downstream steps (d, e, wrapper). Now `referring_build` is only set when the unresolved step was the build's toplevel. - **`DynDrvRelation` newtype**: Extracts the reversed output-name stack into its own type, documenting the reversal invariant in the type name and providing `is_empty()`/`pop()` methods. Used throughout `ReverseDep`, `add_referring_data`, `create_step`, and `pop_dynamic_rdeps`. - **`input_drvs` free function**: Walks `SingleDerivedPath` directly instead of going through `DerivationInputs::from`. Skips `Opaque` (source) inputs — only `Built` derivation dependencies are returned. The outermost output name (what we consume) is discarded; intermediate names form the `DynDrvRelation`. Called inline in `create_step` before `set_drv` consumes the derivation. Non-trivial dyn-drv test now expects 12 build steps covering the full chain: makeDerivations → a → b,c (resolve+build) → d (resolve+build) → e (resolve+build) → wrapper (resolve+build). Extract `state::drv` module with `OutputNameChain` and `SingleDerivedPath` helpers Move `OutputNameChain` (renamed from `DynDrvRelation`), `input_drvs`, and `flatten_chain` into a new `state::drv` module. Add `flatten_path` as the base case for mutual recursion with `flatten_chain`. `flatten_path`/`flatten_chain` now return `OutputNameChain` directly in stack order (outermost first), eliminating the separate reverse step. `input_drvs` calls `flatten_path` on the inner `drv_path` of each `Built` input, naturally discarding the outermost output name. `flatten_chain` (previously in `step_info.rs`) and `input_drvs` (previously in `step.rs`) are unified in one place. The SQL caller in `step_info.rs` reverses to forward order where needed.
1 parent 7c69e41 commit 4030c68

5 files changed

Lines changed: 240 additions & 70 deletions

File tree

subprojects/hydra-queue-runner/src/state/drv.rs

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
use std::collections::BTreeSet;
2+
13
use nix_utils::SingleDerivedPath;
24

35
/// Output names of intermediate derivations for a dynamic derivation
@@ -42,3 +44,21 @@ pub fn flatten_chain(
4244
chain.0.push(output_name.clone());
4345
(root, chain)
4446
}
47+
48+
/// Extract `Built` input dependencies from a derivation.
49+
///
50+
/// Returns `(root_drv_path, relation)` pairs. `Opaque` (source) inputs are
51+
/// skipped — only derivation build dependencies are returned. For each
52+
/// `Built` input, the outermost output name (what we consume) is discarded;
53+
/// intermediate output names form the [`OutputNameChain`].
54+
pub fn input_drvs(
55+
drv: &nix_utils::Derivation,
56+
) -> BTreeSet<(nix_utils::StorePath, OutputNameChain)> {
57+
drv.inputs
58+
.iter()
59+
.filter_map(|sdp| match sdp {
60+
SingleDerivedPath::Opaque(_) => None,
61+
SingleDerivedPath::Built { drv_path, .. } => Some(flatten_path(drv_path)),
62+
})
63+
.collect()
64+
}

subprojects/hydra-queue-runner/src/state/mod.rs

Lines changed: 102 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -510,7 +510,7 @@ impl State {
510510
// original step's drv path differs from the resolved one, so
511511
// completing the resolved step wouldn't clear the dep).
512512
for rdep in step_info.step.clone_rdeps() {
513-
if let Some(rdep) = rdep.upgrade() {
513+
if let Some(rdep) = rdep.step.upgrade() {
514514
rdep.remove_dep(&step_info.step);
515515
resolved_step.make_rdep(&rdep);
516516
}
@@ -1377,6 +1377,74 @@ impl State {
13771377
tx.commit().await?;
13781378
}
13791379

1380+
// Process dynamic rdeps first, as we must add new step dependencies for dynamically
1381+
// generated derivations
1382+
{
1383+
for (dep_step, output_name, relation) in item.step_info.step.pop_dynamic_rdeps() {
1384+
let Some(dependent_step) = dep_step.upgrade() else {
1385+
continue;
1386+
};
1387+
1388+
let resolved_drv = output.outputs.get(&output_name).cloned().ok_or_else(|| {
1389+
anyhow::anyhow!(
1390+
"Dynamic rdep references output `{output_name}` not produced by {drv_path}"
1391+
)
1392+
})?;
1393+
1394+
// Find a build associated with this step. For intermediate steps
1395+
// (not top-level), `direct` is empty, so we walk the dependency
1396+
// chain via `get_dependents` to find the owning build.
1397+
let build = if let Some(b) = direct.get(0) {
1398+
b.clone()
1399+
} else {
1400+
let mut dependents = HashSet::new();
1401+
let mut visited_steps = HashSet::new();
1402+
item.step_info
1403+
.step
1404+
.get_dependents(&mut dependents, &mut visited_steps);
1405+
let Some(b) = dependents.into_iter().next() else {
1406+
tracing::warn!("Finished step does not have associated build");
1407+
continue;
1408+
};
1409+
b
1410+
};
1411+
1412+
// Create the actual step for the new derivation.
1413+
// finished_drvs is not necessary as it is only a memoization table to reduce
1414+
// checks if a dependency is finished from the database.
1415+
// new_steps is not necessary either as
1416+
let new_runnable: Arc<parking_lot::RwLock<HashSet<Arc<Step>>>> = Default::default();
1417+
let new_step = match self
1418+
.create_step(
1419+
build.clone(),
1420+
resolved_drv.clone(),
1421+
None,
1422+
Some((dependent_step.clone(), relation)),
1423+
Default::default(),
1424+
Default::default(),
1425+
new_runnable.clone(),
1426+
)
1427+
.await
1428+
{
1429+
CreateStepResult::None => continue,
1430+
CreateStepResult::Valid(step) => step,
1431+
CreateStepResult::PreviousFailure(step) => {
1432+
if let Err(e) = self.handle_previous_failure(build.clone(), step).await {
1433+
tracing::error!("Failed to handle previous failure: {e}");
1434+
}
1435+
// TODO: figure out what to do here
1436+
continue;
1437+
}
1438+
};
1439+
1440+
for r in new_runnable.read().iter() {
1441+
r.make_runnable();
1442+
}
1443+
1444+
// create_step already added rdeps, but we need to add a forward dep as well
1445+
dependent_step.add_dep(new_step);
1446+
}
1447+
}
13801448
item.step_info.step.make_rdeps_runnable();
13811449

13821450
// always trigger dispatch, as we now might have a free machine again
@@ -1922,7 +1990,7 @@ impl State {
19221990
build: Arc<Build>,
19231991
drv_path: nix_utils::StorePath,
19241992
referring_build: Option<Arc<Build>>,
1925-
referring_step: Option<Arc<Step>>,
1993+
referring_step: Option<(Arc<Step>, drv::OutputNameChain)>,
19261994
finished_drvs: Arc<parking_lot::RwLock<HashSet<nix_utils::StorePath>>>,
19271995
new_steps: Arc<parking_lot::RwLock<HashSet<Arc<Step>>>>,
19281996
new_runnable: Arc<parking_lot::RwLock<HashSet<Arc<Step>>>>,
@@ -1936,9 +2004,13 @@ impl State {
19362004
}
19372005
}
19382006

1939-
let (step, is_new) =
1940-
self.steps
1941-
.create(&drv_path, referring_build.as_ref(), referring_step.as_ref());
2007+
let (step, is_new) = self.steps.create(
2008+
&drv_path,
2009+
referring_build.as_ref(),
2010+
referring_step
2011+
.as_ref()
2012+
.map(|(step, relation)| (step, relation.clone())),
2013+
);
19422014
if !is_new {
19432015
return CreateStepResult::Valid(step);
19442016
}
@@ -2034,6 +2106,7 @@ impl State {
20342106
self.store.query_missing_outputs(output_paths).await
20352107
};
20362108

2109+
let input_drvs = drv::input_drvs(&drv);
20372110
step.set_drv(drv);
20382111

20392112
if self.check_cached_failure(step.clone()).await {
@@ -2091,35 +2164,32 @@ impl State {
20912164
}
20922165

20932166
tracing::debug!("creating build step '{drv_path}");
2094-
let Some(input_drvs) = step.get_input_drvs() else {
2095-
// this should never happen because we always a a drv set at this point in time
2096-
return CreateStepResult::None;
2097-
};
20982167

20992168
let step2 = step.clone();
2100-
let mut stream = futures::StreamExt::map(tokio_stream::iter(input_drvs), |i| {
2101-
let build = build.clone();
2102-
let step = step2.clone();
2103-
let finished_drvs = finished_drvs.clone();
2104-
let new_steps = new_steps.clone();
2105-
let new_runnable = new_runnable.clone();
2106-
async move {
2107-
Box::pin(self.create_step(
2108-
// conn,
2109-
build,
2110-
i,
2111-
None,
2112-
Some(step),
2113-
finished_drvs,
2114-
new_steps,
2115-
new_runnable,
2116-
))
2117-
.await
2118-
}
2119-
})
2120-
.buffered(25);
2121-
while let Some(v) = tokio_stream::StreamExt::next(&mut stream).await {
2122-
match v {
2169+
let mut stream =
2170+
futures::StreamExt::map(tokio_stream::iter(input_drvs), |(input_path, relation)| {
2171+
let build = build.clone();
2172+
let step = step2.clone();
2173+
let finished_drvs = finished_drvs.clone();
2174+
let new_steps = new_steps.clone();
2175+
let new_runnable = new_runnable.clone();
2176+
2177+
async move {
2178+
Box::pin(self.create_step(
2179+
build,
2180+
input_path,
2181+
None,
2182+
Some((step, relation)),
2183+
finished_drvs,
2184+
new_steps,
2185+
new_runnable,
2186+
))
2187+
.await
2188+
}
2189+
})
2190+
.buffered(25);
2191+
while let Some(result) = tokio_stream::StreamExt::next(&mut stream).await {
2192+
match result {
21232193
CreateStepResult::None => (),
21242194
CreateStepResult::Valid(dep) => {
21252195
if !dep.get_finished() && !dep.get_previous_failure() {

subprojects/hydra-queue-runner/src/state/step.rs

Lines changed: 49 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,15 @@ use hashbrown::{HashMap, HashSet};
88
use super::{Build, Jobset};
99
use db::models::BuildID;
1010

11+
use super::drv::OutputNameChain;
12+
13+
#[derive(Debug, Clone)]
14+
pub struct ReverseDep {
15+
/// The step that depends on us
16+
pub step: Weak<Step>,
17+
pub relation: OutputNameChain,
18+
}
19+
1120
#[derive(Debug)]
1221
pub struct StepAtomicState {
1322
/// Whether the step has finished initialisation.
@@ -66,10 +75,11 @@ impl StepAtomicState {
6675

6776
#[derive(Debug)]
6877
pub(super) struct StepState {
69-
/// The build steps on which this step depends.
78+
/// The resolved build steps on which this step depends
7079
deps: HashSet<Arc<Step>>,
7180
/// The build steps that depend on this step.
72-
rdeps: Vec<Weak<Step>>,
81+
/// An empty `relation` signifies a regular (non-dynamic) reverse dependency.
82+
rdeps: Vec<ReverseDep>,
7383
/// Builds that have this step as the top-level derivation.
7484
builds: Vec<Weak<Build>>,
7585
/// Jobsets to which this step belongs. Used for determining scheduling priority.
@@ -186,16 +196,6 @@ impl Step {
186196
})
187197
}
188198

189-
pub fn get_input_drvs(&self) -> Option<Vec<nix_utils::StorePath>> {
190-
let drv = self.drv.load_full();
191-
drv.as_ref().map(|drv| {
192-
harmonia_store_core::derivation::DerivationInputs::from(&drv.inputs)
193-
.drvs
194-
.into_keys()
195-
.collect::<Vec<_>>()
196-
})
197-
}
198-
199199
pub fn get_after(&self) -> jiff::Timestamp {
200200
self.atomic_state.after.load()
201201
}
@@ -270,7 +270,9 @@ impl Step {
270270
};
271271

272272
for rdep in rdeps {
273-
let Some(rdep) = rdep.upgrade() else { continue };
273+
let Some(rdep) = rdep.step.upgrade() else {
274+
continue;
275+
};
274276
rdep.get_dependents(builds, steps);
275277
}
276278
}
@@ -286,7 +288,7 @@ impl Step {
286288

287289
let mut state = self.state.write();
288290
state.rdeps.retain(|rdep| {
289-
let Some(rdep) = rdep.upgrade() else {
291+
let Some(rdep) = rdep.step.upgrade() else {
290292
return false;
291293
};
292294

@@ -374,21 +376,45 @@ impl Step {
374376
pub fn make_rdep(self: &Arc<Self>, dep: &Arc<Self>) {
375377
dep.add_dep(self.clone());
376378
let mut state = self.state.write();
377-
state.rdeps.push(Arc::downgrade(dep));
379+
state.rdeps.push(ReverseDep {
380+
step: Arc::downgrade(dep),
381+
relation: OutputNameChain::default(),
382+
});
378383
self.atomic_state
379384
.rdeps_len
380385
.store(state.rdeps.len() as u64, Ordering::Relaxed);
381386
}
382387

383-
pub fn clone_rdeps(&self) -> Vec<Weak<Step>> {
388+
pub fn clone_rdeps(&self) -> Vec<ReverseDep> {
384389
let state = self.state.read();
385390
state.rdeps.clone()
386391
}
387392

393+
/// Pop one level of dynamic indirection from each dynamic rdep,
394+
/// returning `(dependent_step, popped_output_name, remaining_relation)` triples.
395+
///
396+
/// The rdep entries remain in the list (with shortened relations) so that
397+
/// `make_rdeps_runnable` can still clean up forward deps.
398+
///
399+
/// We collect into a `Vec` rather than returning an iterator because the
400+
/// write lock on the step's state must be released before the caller can
401+
/// do async work (e.g. `create_step`) with the results.
402+
pub fn pop_dynamic_rdeps(&self) -> Vec<(Weak<Step>, nix_utils::OutputName, OutputNameChain)> {
403+
let mut state = self.state.write();
404+
state
405+
.rdeps
406+
.iter_mut()
407+
.filter_map(|rdep| {
408+
let output_name = rdep.relation.pop()?;
409+
Some((rdep.step.clone(), output_name, rdep.relation.clone()))
410+
})
411+
.collect()
412+
}
413+
388414
pub fn add_referring_data(
389415
&self,
390416
referring_build: Option<&Arc<Build>>,
391-
referring_step: Option<&Arc<Self>>,
417+
referring_step: Option<(&Arc<Self>, OutputNameChain)>,
392418
) {
393419
if referring_build.is_none() && referring_step.is_none() {
394420
return;
@@ -398,8 +424,11 @@ impl Step {
398424
if let Some(referring_build) = referring_build {
399425
state.builds.push(Arc::downgrade(referring_build));
400426
}
401-
if let Some(referring_step) = referring_step {
402-
state.rdeps.push(Arc::downgrade(referring_step));
427+
if let Some((referring_step, relation)) = referring_step {
428+
state.rdeps.push(ReverseDep {
429+
step: Arc::downgrade(referring_step),
430+
relation,
431+
});
403432
self.atomic_state
404433
.rdeps_len
405434
.store(state.rdeps.len() as u64, Ordering::Relaxed);
@@ -531,7 +560,7 @@ impl Steps {
531560
&self,
532561
drv_path: &nix_utils::StorePath,
533562
referring_build: Option<&Arc<Build>>,
534-
referring_step: Option<&Arc<Step>>,
563+
referring_step: Option<(&Arc<Step>, OutputNameChain)>,
535564
) -> (Arc<Step>, bool) {
536565
let mut is_new = false;
537566
let mut steps = self.inner.write();

subprojects/hydra-tests/content-addressed/dyn-drv-non-trivial.t

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,38 @@ if ($wrapper) {
3939
$wrapper->discard_changes;
4040
is($wrapper->finished, 1, "wrapper should be finished");
4141
is($wrapper->buildstatus, 0, "wrapper should succeed");
42+
43+
# Full dynamic derivation chain: 12 steps total
44+
# 1. make-derivations.drv.drv (status=0, build makeDerivations)
45+
# 2. build-a.drv (status=0, build a)
46+
# 3. build-c.drv (status=13, resolve c)
47+
# 4. build-b.drv (status=13, resolve b)
48+
# 5. build-b.drv (status=0, build resolved b)
49+
# 6. build-c.drv (status=0, build resolved c)
50+
# 7. build-d.drv (status=13, resolve d)
51+
# 8. build-d.drv (status=0, build resolved d)
52+
# 9. make-derivations.drv (status=13, resolve e — named after makeDerivations output)
53+
# 10. make-derivations.drv (status=0, build resolved e)
54+
# 11. wrapper.drv (status=13, resolve wrapper)
55+
# 12. wrapper.drv (status=0, build resolved wrapper)
56+
my @steps = $wrapper->buildsteps->search({}, { order_by => 'stepnr' })->all;
57+
is(scalar @steps, 12, "wrapper should have 12 build steps");
58+
59+
# Check that derivations a-d each got a successful (status=0) build step.
60+
# build-e is named make-derivations.drv (the output of makeDerivations),
61+
# so we check for it separately.
62+
my @built = sort map {
63+
my $drv = $_->drvpath // "";
64+
(defined $_->status && $_->status == 0 && $drv =~ m{-build-([a-d])\.drv$}) ? $1 : ()
65+
} @steps;
66+
is(\@built, [qw(a b c d)], "derivations a-d should each have a successful build step");
67+
68+
# build-e is the make-derivations.drv step (status=0, not the .drv.drv)
69+
my @build_e = grep {
70+
my $drv = $_->drvpath // "";
71+
defined $_->status && $_->status == 0 && $drv =~ m{-make-derivations\.drv$}
72+
} @steps;
73+
is(scalar @build_e, 1, "build-e (make-derivations.drv) should have a successful build step");
4274
}
4375

4476
done_testing;

0 commit comments

Comments
 (0)