Skip to content

Commit 77dc98b

Browse files
committed
queue-runner and builder: fail fast on infrastructure errors
The queue-runner should not try to recover from a down database or invalid store state — only builder failures should be totally recoverable since builders are numerous and expected to come and go. The builder should die if it cannot communicate with the queue-runner — the queue-runner will notice and retry the step elsewhere. Also there are `nix_utils::BaseStore` trait changes: - `is_valid_path` now returns `Result<bool, Error>` instead of `bool` - `query_path_info` now returns `Result<Option<...>, Error>` instead of `Option<...>` - `query_path_infos` now returns `Result<HashMap<...>, Error>` instead of `HashMap<...>` - `compute_closure_size` now returns `Result<u64, Error>` instead of `u64` Previously the trait implementations silently swallowed FFI errors as "path not found" / empty / zero. This was the root cause of silent failures: a daemon connection error looked like "path is present" to callers, causing paths to be skipped from import and builds to fail with cryptic errors. The C++ `query_path_info` now checks `isValidPath` first and returns a `found: false` sentinel instead of throwing `InvalidPath` — eliminating brittle error-message string matching on the Rust side. All callers updated to propagate errors with `?`. **Note: Manual review of the queue runner error comments is not yet complete!** Queue-runner changes: - `create_build` returns `Result`; an invalid drv path is a hard error (indicates a GC rooting bug) rather than silently aborting the build - `handle_previous_failure` and `handle_cached_build` errors propagate (DB unreachable = stop processing) - `process_new_builds` returns `Result` and propagates child build creation errors - Queue monitor loop propagates `get_queued_builds`, `process_queue_change`, and `handle_jobset_change` errors instead of logging and continuing - Log directory creation at startup is fatal on failure - mTLS misconfiguration uses `anyhow::ensure!` - Non-fatal `tracing::error!` sites documented with comments explaining why they are correct to log-and-continue Builder changes: - `filter_missing` returns `Result` — daemon connection failure is a hard error, not "path is present" - `handle_request` errors propagate (kills the builder — queue-runner will retry) - Ping message construction failure breaks the ping stream (builder exits), with the error captured and returned from the gRPC function - `submit_build_result` helper deduplicates the retry-then-report pattern for `complete_build` calls - Build task returns `Result<()>` instead of `()` — failure to submit results after retries propagates rather than calling `process::exit(1)` - mTLS misconfiguration uses `anyhow::ensure!` - Unreachable error paths in log stream (`utils.rs`) now panic with explanatory comments - `substitute_output` S3 replication uses async-block-as-try-block pattern for clean early exit on daemon error
1 parent c27d8ba commit 77dc98b

16 files changed

Lines changed: 325 additions & 282 deletions

File tree

subprojects/crates/binary-cache/src/lib.rs

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -56,15 +56,16 @@ pub async fn path_to_narinfo(
5656
store: &nix_utils::LocalStore,
5757
path: &StorePath,
5858
) -> Result<NarInfo, CacheError> {
59-
let Some(path_info) = store.query_path_info(path).await else {
60-
return Err(CacheError::PathNotFound {
59+
let path_info = store
60+
.query_path_info(path)
61+
.await?
62+
.ok_or_else(|| CacheError::PathNotFound {
6163
path: path.to_string(),
62-
});
63-
};
64+
})?;
6465
let narinfo = narinfo_simple(path, path_info, Compression::None);
6566
let queried_references = store
6667
.query_path_infos(&narinfo.info.info.references.iter().collect::<Vec<_>>())
67-
.await;
68+
.await?;
6869
for r in &narinfo.info.info.references {
6970
if !queried_references.contains_key(r) {
7071
return Err(CacheError::ReferenceVerifyError(narinfo.path, r.to_owned()));
@@ -549,11 +550,13 @@ impl S3BinaryCacheClient {
549550
store: &nix_utils::LocalStore,
550551
path: &StorePath,
551552
) -> Result<NarInfo, CacheError> {
552-
let Some(path_info) = store.query_path_info(path).await else {
553-
return Err(CacheError::PathNotFound {
554-
path: path.to_string(),
555-
});
556-
};
553+
let path_info =
554+
store
555+
.query_path_info(path)
556+
.await?
557+
.ok_or_else(|| CacheError::PathNotFound {
558+
path: path.to_string(),
559+
})?;
557560
let narinfo = narinfo_from_path_info(
558561
path,
559562
path_info,
@@ -563,7 +566,7 @@ impl S3BinaryCacheClient {
563566
);
564567
let queried_references = store
565568
.query_path_infos(&narinfo.info.info.references.iter().collect::<Vec<_>>())
566-
.await;
569+
.await?;
567570
for r in &narinfo.info.info.references {
568571
if !queried_references.contains_key(r) {
569572
return Err(CacheError::ReferenceVerifyError(narinfo.path, r.to_owned()));
Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,14 @@
11
use nix_utils::{self, BaseStore as _};
22

33
#[tokio::main]
4-
async fn main() {
4+
async fn main() -> Result<(), Box<dyn std::error::Error>> {
55
let store = nix_utils::LocalStore::init();
66
let store_dir = nix_utils::get_store_dir();
77
println!(
88
"storepath={store_dir} valid={}",
99
store
1010
.is_valid_path(&AsRef::<str>::as_ref(&store_dir).parse().unwrap())
11-
.await
11+
.await?
1212
);
13+
Ok(())
1314
}

subprojects/crates/nix-utils/examples/path_infos.rs

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use nix_utils::BaseStore as _;
22

33
#[tokio::main]
4-
async fn main() {
4+
async fn main() -> Result<(), Box<dyn std::error::Error>> {
55
let local = nix_utils::LocalStore::init();
66

77
let p1 = "ihl4ya67glh9815v1lanyqph0p7hdzfb-hdf5-cpp-1.14.6-bin"
@@ -19,16 +19,17 @@ async fn main() {
1919
println!("{infos:?}");
2020
println!(
2121
"closure_size {p1}: {}",
22-
local.compute_closure_size(&p1).await
22+
local.compute_closure_size(&p1).await?
2323
);
2424
println!(
2525
"closure_size {p2}: {}",
26-
local.compute_closure_size(&p2).await
26+
local.compute_closure_size(&p2).await?
2727
);
2828
println!(
2929
"closure_size {p3}: {}",
30-
local.compute_closure_size(&p3).await
30+
local.compute_closure_size(&p3).await?
3131
);
3232

3333
println!("stats: {:?}", local.get_store_stats());
34+
Ok(())
3435
}

subprojects/crates/nix-utils/src/drv.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ pub async fn query_drv(
3434
return Ok(None);
3535
}
3636

37-
if !store.is_valid_path(drv).await {
37+
if !store.is_valid_path(drv).await? {
3838
return Ok(None);
3939
}
4040

subprojects/crates/nix-utils/src/lib.rs

Lines changed: 41 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@ mod ffi {
7979

8080
#[derive(Debug, Clone)]
8181
struct InternalPathInfo {
82+
found: bool,
8283
store_dir: String,
8384
deriver: String,
8485
nar_hash: Vec<u8>,
@@ -368,6 +369,7 @@ fn to_internal_path_info(
368369
let nar_hash_bytes: Vec<u8> = AsRef::<[u8]>::as_ref(&info.nar_hash).to_vec();
369370

370371
ffi::InternalPathInfo {
372+
found: true,
371373
store_dir: store_dir.to_str().to_owned(),
372374
deriver: info
373375
.deriver
@@ -386,19 +388,18 @@ fn to_internal_path_info(
386388
}
387389

388390
pub trait BaseStore {
389-
#[must_use]
390391
/// Check whether a path is valid.
391-
fn is_valid_path(&self, path: &StorePath) -> impl Future<Output = bool>;
392+
fn is_valid_path(&self, path: &StorePath) -> impl Future<Output = Result<bool, Error>>;
392393

393394
fn query_path_info(
394395
&self,
395396
path: &StorePath,
396-
) -> impl Future<Output = Option<UnkeyedValidPathInfo>>;
397+
) -> impl Future<Output = Result<Option<UnkeyedValidPathInfo>, Error>>;
397398
fn query_path_infos(
398399
&self,
399400
paths: &[&StorePath],
400-
) -> impl Future<Output = HashMap<StorePath, UnkeyedValidPathInfo>>;
401-
fn compute_closure_size(&self, path: &StorePath) -> impl Future<Output = u64>;
401+
) -> impl Future<Output = Result<HashMap<StorePath, UnkeyedValidPathInfo>, Error>>;
402+
fn compute_closure_size(&self, path: &StorePath) -> impl Future<Output = Result<u64, Error>>;
402403

403404
fn clear_path_info_cache(&self);
404405

@@ -529,33 +530,35 @@ where
529530

530531
impl BaseStore for BaseStoreImpl {
531532
#[inline]
532-
async fn is_valid_path(&self, path: &StorePath) -> bool {
533+
async fn is_valid_path(&self, path: &StorePath) -> Result<bool, Error> {
533534
let store = self.wrapper.clone();
534535
let path = self.print_store_path(path);
535-
asyncify(move || ffi::is_valid_path(store.as_raw(), &path))
536-
.await
537-
.unwrap_or(false)
536+
asyncify(move || ffi::is_valid_path(store.as_raw(), &path)).await
538537
}
539538

540539
#[inline]
541-
async fn query_path_info(&self, path: &StorePath) -> Option<UnkeyedValidPathInfo> {
540+
async fn query_path_info(
541+
&self,
542+
path: &StorePath,
543+
) -> Result<Option<UnkeyedValidPathInfo>, Error> {
542544
let store = self.wrapper.clone();
543545
let path = self.print_store_path(path);
544546
asyncify(move || {
545-
Ok(ffi::query_path_info(store.as_raw(), &path)
546-
.ok()
547-
.map(Into::into))
547+
let info = ffi::query_path_info(store.as_raw(), &path)?;
548+
if info.found {
549+
Ok(Some(info.into()))
550+
} else {
551+
Ok(None)
552+
}
548553
})
549554
.await
550-
.ok()
551-
.flatten()
552555
}
553556

554557
#[inline]
555558
async fn query_path_infos(
556559
&self,
557560
paths: &[&StorePath],
558-
) -> HashMap<StorePath, UnkeyedValidPathInfo> {
561+
) -> Result<HashMap<StorePath, UnkeyedValidPathInfo>, Error> {
559562
let paths = paths.iter().map(|v| (*v).to_owned()).collect::<Vec<_>>();
560563

561564
asyncify({
@@ -564,27 +567,22 @@ impl BaseStore for BaseStoreImpl {
564567
let mut res = HashMap::with_capacity(paths.len());
565568
for p in paths {
566569
let full_path = self_.print_store_path(&p);
567-
if let Some(info) = ffi::query_path_info(self_.wrapper.as_raw(), &full_path)
568-
.ok()
569-
.map(Into::into)
570-
{
571-
res.insert(p, info);
570+
let info = ffi::query_path_info(self_.wrapper.as_raw(), &full_path)?;
571+
if info.found {
572+
res.insert(p, info.into());
572573
}
573574
}
574575
Ok(res)
575576
}
576577
})
577578
.await
578-
.unwrap_or_default()
579579
}
580580

581581
#[inline]
582-
async fn compute_closure_size(&self, path: &StorePath) -> u64 {
582+
async fn compute_closure_size(&self, path: &StorePath) -> Result<u64, Error> {
583583
let store = self.wrapper.clone();
584584
let path = self.print_store_path(path);
585-
asyncify(move || ffi::compute_closure_size(store.as_raw(), &path))
586-
.await
587-
.unwrap_or_default()
585+
asyncify(move || ffi::compute_closure_size(store.as_raw(), &path)).await
588586
}
589587

590588
#[inline]
@@ -726,7 +724,7 @@ impl LocalStore {
726724
tokio_stream::iter(outputs)
727725
.map(|(name, path)| async move {
728726
match path {
729-
Some(p) if self.is_valid_path(&p).await => None,
727+
Some(p) if self.is_valid_path(&p).await.unwrap_or(false) => None,
730728
other => Some((name, other)),
731729
}
732730
})
@@ -778,25 +776,28 @@ impl LocalStore {
778776

779777
impl BaseStore for LocalStore {
780778
#[inline]
781-
async fn is_valid_path(&self, path: &StorePath) -> bool {
779+
async fn is_valid_path(&self, path: &StorePath) -> Result<bool, Error> {
782780
self.base.is_valid_path(path).await
783781
}
784782

785783
#[inline]
786-
async fn query_path_info(&self, path: &StorePath) -> Option<UnkeyedValidPathInfo> {
784+
async fn query_path_info(
785+
&self,
786+
path: &StorePath,
787+
) -> Result<Option<UnkeyedValidPathInfo>, Error> {
787788
self.base.query_path_info(path).await
788789
}
789790

790791
#[inline]
791792
async fn query_path_infos(
792793
&self,
793794
paths: &[&StorePath],
794-
) -> HashMap<StorePath, UnkeyedValidPathInfo> {
795+
) -> Result<HashMap<StorePath, UnkeyedValidPathInfo>, Error> {
795796
self.base.query_path_infos(paths).await
796797
}
797798

798799
#[inline]
799-
async fn compute_closure_size(&self, path: &StorePath) -> u64 {
800+
async fn compute_closure_size(&self, path: &StorePath) -> Result<u64, Error> {
800801
self.base.compute_closure_size(path).await
801802
}
802803

@@ -907,7 +908,7 @@ impl RemoteStore {
907908

908909
tokio_stream::iter(paths)
909910
.map(|p| async move {
910-
if self.is_valid_path(&p).await {
911+
if self.is_valid_path(&p).await.unwrap_or(false) {
911912
None
912913
} else {
913914
Some(p)
@@ -929,7 +930,7 @@ impl RemoteStore {
929930
tokio_stream::iter(outputs)
930931
.map(|(name, path)| async move {
931932
match path {
932-
Some(p) if self.is_valid_path(&p).await => None,
933+
Some(p) if self.is_valid_path(&p).await.unwrap_or(false) => None,
933934
other => Some((name, other)),
934935
}
935936
})
@@ -942,25 +943,28 @@ impl RemoteStore {
942943

943944
impl BaseStore for RemoteStore {
944945
#[inline]
945-
async fn is_valid_path(&self, path: &StorePath) -> bool {
946+
async fn is_valid_path(&self, path: &StorePath) -> Result<bool, Error> {
946947
self.base.is_valid_path(path).await
947948
}
948949

949950
#[inline]
950-
async fn query_path_info(&self, path: &StorePath) -> Option<UnkeyedValidPathInfo> {
951+
async fn query_path_info(
952+
&self,
953+
path: &StorePath,
954+
) -> Result<Option<UnkeyedValidPathInfo>, Error> {
951955
self.base.query_path_info(path).await
952956
}
953957

954958
#[inline]
955959
async fn query_path_infos(
956960
&self,
957961
paths: &[&StorePath],
958-
) -> HashMap<StorePath, UnkeyedValidPathInfo> {
962+
) -> Result<HashMap<StorePath, UnkeyedValidPathInfo>, Error> {
959963
self.base.query_path_infos(paths).await
960964
}
961965

962966
#[inline]
963-
async fn compute_closure_size(&self, path: &StorePath) -> u64 {
967+
async fn compute_closure_size(&self, path: &StorePath) -> Result<u64, Error> {
964968
self.base.compute_closure_size(path).await
965969
}
966970

subprojects/crates/nix-utils/src/nix.cpp

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,11 @@ bool is_valid_path(const StoreWrapper &wrapper, rust::Str path) {
113113

114114
InternalPathInfo query_path_info(const StoreWrapper &wrapper, rust::Str path) {
115115
auto store = wrapper._store;
116-
auto info = store->queryPathInfo(store->parseStorePath(AS_VIEW(path)));
116+
auto storePath = store->parseStorePath(AS_VIEW(path));
117+
if (!store->isValidPath(storePath)) {
118+
return InternalPathInfo{false, "", "", {}, 0, 0, {}, {}, ""};
119+
}
120+
auto info = store->queryPathInfo(storePath);
117121

118122
// Return raw SHA256 bytes (32 bytes) — Rust side converts to NarHash directly.
119123
rust::Vec<uint8_t> narhash_bytes;
@@ -133,6 +137,7 @@ InternalPathInfo query_path_info(const StoreWrapper &wrapper, rust::Str path) {
133137

134138
// TODO(conni2461): Replace "" with option
135139
return InternalPathInfo{
140+
true,
136141
rust::String(store->storeDir.data(), store->storeDir.size()),
137142
extract_opt_path(info->deriver),
138143
narhash_bytes,

0 commit comments

Comments
 (0)