Skip to content

'Gracefully' fallback from failing substituters #13301

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Ericson2314 merged 1 commit into from
Sep 12, 2025
Merged

'Gracefully' fallback from failing substituters #13301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 15 additions & 10 deletions src/libstore/build/substitution-goal.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,14 @@ Goal::Co PathSubstitutionGoal::init()
auto subs = settings.useSubstitutes ? getDefaultSubstituters() : std::list<ref<Store>>();

bool substituterFailed = false;
std::optional<Error> lastStoresException = std::nullopt;

for (const auto & sub : subs) {
trace("trying next substituter");
if (lastStoresException.has_value()) {
logError(lastStoresException->info());
lastStoresException.reset();
}

cleanup();

Expand All @@ -80,19 +85,13 @@ Goal::Co PathSubstitutionGoal::init()
try {
// FIXME: make async
info = sub->queryPathInfo(subPath ? *subPath : storePath);
} catch (InvalidPath &) {
} catch (InvalidPath & e) {
continue;
} catch (SubstituterDisabled & e) {
if (settings.tryFallback)
continue;
else
throw e;
continue;
Copy link
Copy Markdown
Member

@Ericson2314 Ericson2314 Sep 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
continue;
if (!settings.tryFallback)
throw e;
continue;

I think we want this, the saving exception thing should just be for the no-fallback case, right?

Copy link
Copy Markdown
Member

@Ericson2314 Ericson2314 Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh my bad, I think I am wrong and this is fine. You tell can confirm, though.

Copy link
Copy Markdown
Contributor Author

@jadewilk jadewilk Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think there has been some confusion when implementing this loop that means tryFallback has been coerced into meaning both "try the next substituter" and "try building the path ourselves" - I am quite sure it should only be used for the latter. Otherwise... that would mean we are ok with trying the next substituter if the first one doesn't have a path, but not if it errors out and gets disabled... really weird.

Copy link
Copy Markdown
Member

@Ericson2314 Ericson2314 Sep 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was some other PR where I left a big comment trying to explain the theory.

Here's what I think: "!tryFallback" is a coherence trick: you only build/use a substituter if you are sure that the previous substituters don't have the store object in question. If you can access them and they don't have it, you are sure. If you can't access it, you don't know, and thus you could be getting/build the "wrong version".

That actually would be a reason to have what I wrote above.

Copy link
Copy Markdown
Member

@Ericson2314 Ericson2314 Sep 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#7188 (comment) ah this is the comment I had in mind

Copy link
Copy Markdown
Contributor Author

@jadewilk jadewilk Sep 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps im ignorant, but in what case could we get a path that has different versions depending on the substituter used, would this not result in a different path entirely?

I don't think the state of the substituter should affect the contents of the path we get back?

Copy link
Copy Markdown
Member

@Ericson2314 Ericson2314 Sep 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With input-addressing, the a store path may be allowed to point to different contents if the derivation is non-deterministic.

} catch (Error & e) {
if (settings.tryFallback) {
logError(e.info());
continue;
} else
throw e;
lastStoresException = std::make_optional(std::move(e));
continue;
}

if (info->path != storePath) {
Expand Down Expand Up @@ -156,6 +155,12 @@ Goal::Co PathSubstitutionGoal::init()
worker.failedSubstitutions++;
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.
In that case the calling derivation should just do a
Expand Down
25 changes: 16 additions & 9 deletions src/libstore/store-api.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#include "nix/util/logging.hh"
#include "nix/util/signature/local-keys.hh"
#include "nix/util/source-accessor.hh"
#include "nix/store/globals.hh"
Expand Down Expand Up @@ -392,11 +393,14 @@ void Store::querySubstitutablePathInfos(const StorePathCAMap & paths, Substituta
{
if (!settings.useSubstitutes)
return;
for (auto & sub : getDefaultSubstituters()) {
for (auto & path : paths) {
if (infos.count(path.first))
// Choose first succeeding substituter.
continue;
Comment on lines -397 to -399
Copy link
Copy Markdown
Contributor

@xokdvium xokdvium Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this get lost? We seem to be querying all of the substituters now, which is clearly unintended?

#14836

@thomasjm, looks like this is the offending change.

Copy link
Copy Markdown
Contributor Author

@jadewilk jadewilk Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it'd make more sense to put a break at L435 https://github.com/NixOS/nix/pull/13301/changes/BASE..11d7c80370a01e96bceb9dbe5124d3e91c841c7d#diff-e89a2224d1d4c39bf5a2d448b7b2fac1e3e75c7e08e414ee448846266c8bfeabL435

Copy link
Copy Markdown
Contributor

@xokdvium xokdvium Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup.

Copy link
Copy Markdown
Contributor

@xokdvium xokdvium Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#14837


for (auto & path : paths) {
std::optional<Error> lastStoresException = std::nullopt;
for (auto & sub : getDefaultSubstituters()) {
Comment thread
jadewilk marked this conversation as resolved.
if (lastStoresException.has_value()) {
logError(lastStoresException->info());
lastStoresException.reset();
}

auto subPath(path.first);

Expand Down Expand Up @@ -437,12 +441,15 @@ void Store::querySubstitutablePathInfos(const StorePathCAMap & paths, Substituta
} catch (InvalidPath &) {
} catch (SubstituterDisabled &) {
} catch (Error & e) {
if (settings.tryFallback)
logError(e.info());
else
throw;
lastStoresException = std::make_optional(std::move(e));
}
}
if (lastStoresException.has_value()) {
if (!settings.tryFallback) {
throw *lastStoresException;
} else
logError(lastStoresException->info());
}
}
}

Expand Down
Loading