Skip to content

Commit 90af869

Browse files
Ericson2314artemist
authored andcommitted
Fix two-phase CA derivation resolution and StorePath type safety
Fix several issues with the `ca-resolve-two-phase` branch: - Fix `find_build_step_outputs` SQL: `?` → `$1` (PostgreSQL), add `s.build = o.build` join condition - Fix dep graph propagation: when resolving a CA derivation, remove the original step from each dependent's deps and replace with the resolved step, so downstream steps become runnable - Skip two-phase resolution for CA floating drvs with only `Opaque` inputs (no `Built` deps) — these are already resolved - Replace `resolvedToStep` (integer FK) with `resolvedDrvPath` (text basename) so output lookups work across builds by drv path So it turns out that only scheduled or finishes steps go in the DB with the current desing. Steps which totally pending (blocked on other steps, for example) just live in memory. So we won't get our step number at resolution time to use as a foreign key. Instead, we can just write down the drv path, which is enough for the in-memory step, and only when that step is processsed will something end up in the data base. - Remove eager `create_build_step` for resolved steps — the DB entry is created when the step is actually dispatched very much corresponds to the above - Skip dyn-drv integration test (dynamic derivation resolution not yet implemented in Hydra) Our passing it was very much a lie before. Our failing it now is more honest. - Add unit tests for `resolve_drv_output_chains` with resolved steps
1 parent aa01176 commit 90af869

8 files changed

Lines changed: 266 additions & 149 deletions

File tree

subprojects/crates/db/src/connection.rs

Lines changed: 153 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -355,14 +355,20 @@ impl Connection {
355355
CROSS JOIN LATERAL (
356356
SELECT o.path
357357
FROM buildsteps s
358+
-- If this step was resolved, look up outputs from
359+
-- the resolved drv's successful buildstep instead.
360+
-- resolvedDrvPath is a bare basename; drvPath is a
361+
-- full path, so strip the directory prefix to compare.
358362
LEFT JOIN buildsteps sr
359-
ON s.build = sr.build AND s.resolvedToStep = sr.stepnr
363+
ON sr.drvPath = $2 || '/' || s.resolvedDrvPath
364+
AND sr.status = 0
360365
JOIN buildstepoutputs o
361-
ON s.build = o.build AND (s.stepnr = o.stepnr OR sr.stepnr = o.stepnr)
366+
ON o.build = COALESCE(sr.build, s.build)
367+
AND o.stepnr = COALESCE(sr.stepnr, s.stepnr)
362368
WHERE s.drvPath = r.drv_path
363369
AND o.name = i.chain[r.step]
364370
AND o.path IS NOT NULL
365-
AND (s.status = 0 OR (s.status = 13 AND sr.status = 0))
371+
AND (s.status = 0 OR s.status = 13)
366372
ORDER BY s.build DESC
367373
LIMIT 1
368374
) sub
@@ -378,6 +384,7 @@ impl Connection {
378384
",
379385
)
380386
.bind(&json_input)
387+
.bind(store_dir.to_str())
381388
.fetch_all(&mut *self.conn)
382389
.await?;
383390

@@ -698,11 +705,31 @@ impl Transaction<'_> {
698705
#[tracing::instrument(skip(self), err)]
699706
pub async fn find_build_step_outputs(
700707
&mut self,
701-
drv_path: &str,
702-
) -> sqlx::Result<BTreeMap<String, String>> {
703-
let items: Vec<(String, String)> = sqlx::query_as("SELECT o.name, o.path FROM buildstepoutputs o JOIN buildsteps s ON s.stepnr = o.stepnr WHERE s.drvpath = ? AND o.path IS NOT NULL").bind(drv_path).fetch_all(&mut *self.tx).await?;
708+
store_dir: &StoreDir,
709+
drv_path: &StorePath,
710+
) -> sqlx::Result<BTreeMap<OutputName, StorePath>> {
711+
let drv_path = store_dir.display(drv_path).to_string();
712+
let items: Vec<(String, String)> = sqlx::query_as(
713+
r"SELECT o.name, o.path
714+
FROM buildstepoutputs o
715+
JOIN buildsteps s ON s.build = o.build AND s.stepnr = o.stepnr
716+
WHERE s.drvpath = $1 AND o.path IS NOT NULL",
717+
)
718+
.bind(drv_path)
719+
.fetch_all(&mut *self.tx)
720+
.await?;
704721

705-
Ok(items.into_iter().collect())
722+
items
723+
.into_iter()
724+
.map(|(name, path)| -> anyhow::Result<_> {
725+
let name: OutputName = name.parse().context("invalid output name from DB")?;
726+
let path: StorePath = store_dir
727+
.parse(&path)
728+
.context("invalid store path from DB")?;
729+
Ok((name, path))
730+
})
731+
.collect::<anyhow::Result<_>>()
732+
.map_err(|e| sqlx::Error::Decode(e.into_boxed_dyn_error()))
706733
}
707734

708735
#[tracing::instrument(skip(self, res), err)]
@@ -953,25 +980,24 @@ impl Transaction<'_> {
953980
Ok(step_nr)
954981
}
955982

956-
/// Set resolvedToBuild/resolvedToStep on a dependency step after the
957-
/// resolved step has been created, linking the dependency to its resolution.
983+
/// Record which resolved drv path this step was resolved to.
958984
#[tracing::instrument(skip(self), err)]
959985
pub async fn set_resolved_to(
960986
&mut self,
961987
origin_build_id: crate::models::BuildID,
962988
origin_step_nr: i32,
963-
resolved_step_nr: i32,
989+
resolved_drv_path: &StorePath,
964990
) -> sqlx::Result<()> {
965991
sqlx::query(
966992
r"
967993
UPDATE buildsteps
968-
SET resolvedToStep = $3
994+
SET resolvedDrvPath = $3
969995
WHERE build = $1 AND stepnr = $2
970996
",
971997
)
972998
.bind(origin_build_id)
973999
.bind(origin_step_nr)
974-
.bind(resolved_step_nr)
1000+
.bind(resolved_drv_path.to_string())
9751001
.execute(&mut *self.tx)
9761002
.await?;
9771003
Ok(())
@@ -1281,11 +1307,24 @@ mod tests {
12811307
}
12821308

12831309
async fn insert_step(conn: &mut Connection, build: i32, stepnr: i32, drv_path: &StorePath) {
1310+
insert_step_with_status(conn, build, stepnr, drv_path, 0, None).await;
1311+
}
1312+
1313+
async fn insert_step_with_status(
1314+
conn: &mut Connection,
1315+
build: i32,
1316+
stepnr: i32,
1317+
drv_path: &StorePath,
1318+
status: i32,
1319+
resolved_drv_path: Option<&StorePath>,
1320+
) {
12841321
let sd = test_store_dir();
1285-
sqlx::query("INSERT INTO BuildSteps (build, stepnr, type, busy, drvPath, status) VALUES ($1, $2, 0, 0, $3, 0)")
1322+
sqlx::query("INSERT INTO BuildSteps (build, stepnr, type, busy, drvPath, status, resolvedDrvPath) VALUES ($1, $2, 0, 0, $3, $4, $5)")
12861323
.bind(build)
12871324
.bind(stepnr)
12881325
.bind(sd.display(drv_path).to_string())
1326+
.bind(status)
1327+
.bind(resolved_drv_path.map(|p| p.to_string()))
12891328
.execute(&mut *conn.conn)
12901329
.await
12911330
.unwrap();
@@ -1423,6 +1462,70 @@ mod tests {
14231462
);
14241463
}
14251464

1465+
/// A step that was resolved (status=13) with `resolvedDrvPath` pointing
1466+
/// to a different drv whose successful buildstep has the outputs.
1467+
#[tokio::test]
1468+
async fn resolve_through_resolved_step() {
1469+
let (_pg, mut conn) = setup().await;
1470+
1471+
// Step 1: unresolved ca-depending-on-ca.drv, status=Resolved(13),
1472+
// resolvedDrvPath points to the resolved drv
1473+
insert_step_with_status(
1474+
&mut conn,
1475+
1,
1476+
1,
1477+
&sp("unresolved.drv"),
1478+
13,
1479+
Some(&sp("resolved.drv")),
1480+
)
1481+
.await;
1482+
// A successful buildstep for the resolved drv (could be any build)
1483+
insert_step(&mut conn, 2, 1, &sp("resolved.drv")).await;
1484+
insert_output(&mut conn, 2, 1, "out", &sp("result")).await;
1485+
1486+
// Looking up via the unresolved drv path should find the output
1487+
// through the resolvedDrvPath chain.
1488+
let results = conn
1489+
.resolve_drv_output_chains(&test_store_dir(), &[(&sp("unresolved.drv"), &[&on("out")])])
1490+
.await
1491+
.unwrap();
1492+
assert_eq!(results, vec![Some(sp("result"))]);
1493+
}
1494+
1495+
/// A depth-2 chain where the first step was resolved:
1496+
/// unresolved.drv (status=13, resolvedDrvPath→resolved.drv) →
1497+
/// resolved.drv outputs a .drv path → that .drv has the final output.
1498+
#[tokio::test]
1499+
async fn resolve_depth_2_through_resolved_step() {
1500+
let (_pg, mut conn) = setup().await;
1501+
1502+
// Build 1: unresolved step, resolved to bbb-resolved.drv
1503+
insert_step_with_status(
1504+
&mut conn,
1505+
1,
1506+
1,
1507+
&sp("unresolved.drv"),
1508+
13,
1509+
Some(&sp("resolved.drv")),
1510+
)
1511+
.await;
1512+
insert_step(&mut conn, 2, 1, &sp("resolved.drv")).await;
1513+
insert_output(&mut conn, 2, 1, "out", &sp("intermediate.drv")).await;
1514+
1515+
// Build 3: the intermediate drv
1516+
insert_step(&mut conn, 3, 1, &sp("intermediate.drv")).await;
1517+
insert_output(&mut conn, 3, 1, "out", &sp("final")).await;
1518+
1519+
let results = conn
1520+
.resolve_drv_output_chains(
1521+
&test_store_dir(),
1522+
&[(&sp("unresolved.drv"), &[&on("out"), &on("out")])],
1523+
)
1524+
.await
1525+
.unwrap();
1526+
assert_eq!(results, vec![Some(sp("final"))]);
1527+
}
1528+
14261529
/// Batch with ragged depths: one depth-1 (Opaque), one depth-2 (Built),
14271530
/// one depth-3 (Built(Built(...))).
14281531
#[tokio::test]
@@ -1467,4 +1570,41 @@ mod tests {
14671570
]
14681571
);
14691572
}
1573+
1574+
/// Depth-1 lookup where the only buildstep for the drv has
1575+
/// status=Resolved(13) with `resolvedDrvPath` pointing to
1576+
/// a different drv whose successful buildstep has the outputs.
1577+
/// This matches the production scenario: ca-depending-on-ca.drv
1578+
/// was resolved to a different drv, and ca-depending-on-ca-
1579+
/// depending-on-ca needs to look up its output.
1580+
#[tokio::test]
1581+
async fn resolve_depth_1_via_resolved_step() {
1582+
let (_pg, mut conn) = setup().await;
1583+
1584+
// Build 1, step 1: unresolved ca-depending-on-ca.drv
1585+
// status=13 (Resolved), resolvedDrvPath points to the resolved drv
1586+
insert_step_with_status(
1587+
&mut conn,
1588+
1,
1589+
1,
1590+
&sp("unresolved-ca-dep.drv"),
1591+
13,
1592+
Some(&sp("resolved-ca-dep.drv")),
1593+
)
1594+
.await;
1595+
// Build 2: the resolved drv was built successfully
1596+
insert_step(&mut conn, 2, 1, &sp("resolved-ca-dep.drv")).await;
1597+
insert_output(&mut conn, 2, 1, "out", &sp("ca-dep-output")).await;
1598+
1599+
// Depth-1 chain: look up "out" of the unresolved drv path.
1600+
// The query should follow resolvedDrvPath to find the output.
1601+
let results = conn
1602+
.resolve_drv_output_chains(
1603+
&test_store_dir(),
1604+
&[(&sp("unresolved-ca-dep.drv"), &[&on("out")])],
1605+
)
1606+
.await
1607+
.unwrap();
1608+
assert_eq!(results, vec![Some(sp("ca-dep-output"))]);
1609+
}
14701610
}

0 commit comments

Comments
 (0)