1
1
Fork 0
mirror of https://github.com/NixOS/nix.git synced 2025-11-09 03:56:01 +01:00

bugfix/3514: do not throw on substituter errors if other substituters are still enabled (#13301)

## Motivation

Nix currently hard fails if a substituter is inaccessible, even when they are other substituters available, unless `fallback = true`. 
This breaks nix build, run, shell et al entirely. 
This would modify the default behaviour so that nix would actually use the other available substituters and not hard error.

Here is an example before vs after when using dotenv where I have manually stopped my own cache to trigger this issue, before and after the patch. The initial error is really frustrating because there is other caches available.
![image](https://github.com/user-attachments/assets/b4aec474-52d1-497d-b4e8-6f5737d6acc7)
![image](https://github.com/user-attachments/assets/ee91fcd4-4a1a-4c33-bf88-3aee67ad3cc9)

## Context

https://github.com/NixOS/nix/issues/3514#issuecomment-2905056198 is the earliest issue I could find, but there are many duplicates.

There is an initial PR at https://github.com/NixOS/nix/pull/7188, but this appears to have been abandoned - over 2 years with no activity, then a no comment review in jan. There was a subsequent PR at https://github.com/NixOS/nix/pull/8983 but this was closed without merge - over a year without activity.
<!-- Non-trivial change: Briefly outline the implementation strategy. -->
I have visualised the current and proposed flows. I believe my logic flows line up with what is suggested in https://github.com/NixOS/nix/pull/7188#issuecomment-1375652870 but correct me if I am wrong.
Current behaviour:
![current](https://github.com/user-attachments/assets/d9501b34-274c-4eb3-88c3-9021a482e364)
Proposed behaviour:
![proposed](https://github.com/user-attachments/assets/8236e4f4-21ef-45d7-87e1-6c8d416e8c1c)

[Charts in lucid](https://lucid.app/lucidchart/1b51b08d-6c4f-40e0-bf54-480df322cccf/view)
<!-- Invasive change: Discuss alternative designs or approaches you considered. -->

Possible issues to think about:
- I could not figure out where the curl error is created... I can't figure out how to swallow it and turn it into a warn or better yet, a debug log.
- Unfortunately, in contrast with the previous point, I'm not sure how verbose we want to warns/traces to be - personally I think that the warn that a substituter has been disabled (when it happens) is sufficient, and that the next one is being used, but this is personal preference.
This commit is contained in:
Philip Wilk 2025-09-12 22:29:34 +01:00 committed by GitHub
parent 92df96543c
commit aef431fbd1
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 31 additions and 19 deletions

View file

@ -55,9 +55,14 @@ Goal::Co PathSubstitutionGoal::init()
auto subs = settings.useSubstitutes ? getDefaultSubstituters() : std::list<ref<Store>>(); auto subs = settings.useSubstitutes ? getDefaultSubstituters() : std::list<ref<Store>>();
bool substituterFailed = false; bool substituterFailed = false;
std::optional<Error> lastStoresException = std::nullopt;
for (const auto & sub : subs) { for (const auto & sub : subs) {
trace("trying next substituter"); trace("trying next substituter");
if (lastStoresException.has_value()) {
logError(lastStoresException->info());
lastStoresException.reset();
}
cleanup(); cleanup();
@ -80,19 +85,13 @@ Goal::Co PathSubstitutionGoal::init()
try { try {
// FIXME: make async // FIXME: make async
info = sub->queryPathInfo(subPath ? *subPath : storePath); info = sub->queryPathInfo(subPath ? *subPath : storePath);
} catch (InvalidPath &) { } catch (InvalidPath & e) {
continue; continue;
} catch (SubstituterDisabled & e) { } catch (SubstituterDisabled & e) {
if (settings.tryFallback)
continue; continue;
else
throw e;
} catch (Error & e) { } catch (Error & e) {
if (settings.tryFallback) { lastStoresException = std::make_optional(std::move(e));
logError(e.info());
continue; continue;
} else
throw e;
} }
if (info->path != storePath) { if (info->path != storePath) {
@ -156,6 +155,12 @@ Goal::Co PathSubstitutionGoal::init()
worker.failedSubstitutions++; worker.failedSubstitutions++;
worker.updateProgress(); worker.updateProgress();
} }
if (lastStoresException.has_value()) {
if (!settings.tryFallback) {
throw *lastStoresException;
} else
logError(lastStoresException->info());
}
/* Hack: don't indicate failure if there were no substituters. /* Hack: don't indicate failure if there were no substituters.
In that case the calling derivation should just do a In that case the calling derivation should just do a

View file

@ -1,3 +1,4 @@
#include "nix/util/logging.hh"
#include "nix/util/signature/local-keys.hh" #include "nix/util/signature/local-keys.hh"
#include "nix/util/source-accessor.hh" #include "nix/util/source-accessor.hh"
#include "nix/store/globals.hh" #include "nix/store/globals.hh"
@ -392,11 +393,14 @@ void Store::querySubstitutablePathInfos(const StorePathCAMap & paths, Substituta
{ {
if (!settings.useSubstitutes) if (!settings.useSubstitutes)
return; return;
for (auto & sub : getDefaultSubstituters()) {
for (auto & path : paths) { for (auto & path : paths) {
if (infos.count(path.first)) std::optional<Error> lastStoresException = std::nullopt;
// Choose first succeeding substituter. for (auto & sub : getDefaultSubstituters()) {
continue; if (lastStoresException.has_value()) {
logError(lastStoresException->info());
lastStoresException.reset();
}
auto subPath(path.first); auto subPath(path.first);
@ -437,12 +441,15 @@ void Store::querySubstitutablePathInfos(const StorePathCAMap & paths, Substituta
} catch (InvalidPath &) { } catch (InvalidPath &) {
} catch (SubstituterDisabled &) { } catch (SubstituterDisabled &) {
} catch (Error & e) { } catch (Error & e) {
if (settings.tryFallback) lastStoresException = std::make_optional(std::move(e));
logError(e.info());
else
throw;
} }
} }
if (lastStoresException.has_value()) {
if (!settings.tryFallback) {
throw *lastStoresException;
} else
logError(lastStoresException->info());
}
} }
} }