Skip to content

Commit aef431f

Browse files
authored
bugfix/3514: do not throw on substituter errors if other substituters are still enabled (NixOS#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 NixOS#3514 (comment) is the earliest issue I could find, but there are many duplicates. There is an initial PR at NixOS#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 NixOS#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 NixOS#7188 (comment) 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.
1 parent 92df965 commit aef431f

File tree

2 files changed

+31
-19
lines changed

2 files changed

+31
-19
lines changed

src/libstore/build/substitution-goal.cc

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -55,9 +55,14 @@ Goal::Co PathSubstitutionGoal::init()
5555
auto subs = settings.useSubstitutes ? getDefaultSubstituters() : std::list<ref<Store>>();
5656

5757
bool substituterFailed = false;
58+
std::optional<Error> lastStoresException = std::nullopt;
5859

5960
for (const auto & sub : subs) {
6061
trace("trying next substituter");
62+
if (lastStoresException.has_value()) {
63+
logError(lastStoresException->info());
64+
lastStoresException.reset();
65+
}
6166

6267
cleanup();
6368

@@ -80,19 +85,13 @@ Goal::Co PathSubstitutionGoal::init()
8085
try {
8186
// FIXME: make async
8287
info = sub->queryPathInfo(subPath ? *subPath : storePath);
83-
} catch (InvalidPath &) {
88+
} catch (InvalidPath & e) {
8489
continue;
8590
} catch (SubstituterDisabled & e) {
86-
if (settings.tryFallback)
87-
continue;
88-
else
89-
throw e;
91+
continue;
9092
} catch (Error & e) {
91-
if (settings.tryFallback) {
92-
logError(e.info());
93-
continue;
94-
} else
95-
throw e;
93+
lastStoresException = std::make_optional(std::move(e));
94+
continue;
9695
}
9796

9897
if (info->path != storePath) {
@@ -156,6 +155,12 @@ Goal::Co PathSubstitutionGoal::init()
156155
worker.failedSubstitutions++;
157156
worker.updateProgress();
158157
}
158+
if (lastStoresException.has_value()) {
159+
if (!settings.tryFallback) {
160+
throw *lastStoresException;
161+
} else
162+
logError(lastStoresException->info());
163+
}
159164

160165
/* Hack: don't indicate failure if there were no substituters.
161166
In that case the calling derivation should just do a

src/libstore/store-api.cc

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
#include "nix/util/logging.hh"
12
#include "nix/util/signature/local-keys.hh"
23
#include "nix/util/source-accessor.hh"
34
#include "nix/store/globals.hh"
@@ -392,11 +393,14 @@ void Store::querySubstitutablePathInfos(const StorePathCAMap & paths, Substituta
392393
{
393394
if (!settings.useSubstitutes)
394395
return;
395-
for (auto & sub : getDefaultSubstituters()) {
396-
for (auto & path : paths) {
397-
if (infos.count(path.first))
398-
// Choose first succeeding substituter.
399-
continue;
396+
397+
for (auto & path : paths) {
398+
std::optional<Error> lastStoresException = std::nullopt;
399+
for (auto & sub : getDefaultSubstituters()) {
400+
if (lastStoresException.has_value()) {
401+
logError(lastStoresException->info());
402+
lastStoresException.reset();
403+
}
400404

401405
auto subPath(path.first);
402406

@@ -437,12 +441,15 @@ void Store::querySubstitutablePathInfos(const StorePathCAMap & paths, Substituta
437441
} catch (InvalidPath &) {
438442
} catch (SubstituterDisabled &) {
439443
} catch (Error & e) {
440-
if (settings.tryFallback)
441-
logError(e.info());
442-
else
443-
throw;
444+
lastStoresException = std::make_optional(std::move(e));
444445
}
445446
}
447+
if (lastStoresException.has_value()) {
448+
if (!settings.tryFallback) {
449+
throw *lastStoresException;
450+
} else
451+
logError(lastStoresException->info());
452+
}
446453
}
447454
}
448455

0 commit comments

Comments
 (0)