Skip to content

remote-fs-accessor: return null for missing store objects #16408

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

Open
domenkozar wants to merge 1 commit into
base: master
Choose a base branch
from
+26 −14
Open

remote-fs-accessor: return null for missing store objects #16408

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
10 changes: 10 additions & 0 deletions src/libstore-tests/local-binary-cache-store.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include <gtest/gtest.h>

#include "nix/store/local-binary-cache-store.hh"
#include "nix/util/file-system.hh"

namespace nix {

Expand All @@ -22,4 +23,13 @@ TEST(LocalBinaryCacheStore, constructConfig)
EXPECT_EQ(config.binaryCacheDir, "/foo/bar/baz");
}

TEST(LocalBinaryCacheStore, fsAccessorsHandleMissingObject)
{
auto cacheDir = createTempDir();
AutoDelete delCacheDir{cacheDir};
auto store = make_ref<LocalBinaryCacheStoreConfig>(cacheDir, LocalBinaryCacheStoreConfig::Params{})->openStore();
EXPECT_EQ(store->getFSAccessor(StorePath::dummy), nullptr);
EXPECT_EQ(store->getFSAccessor()->maybeLstat(CanonPath(StorePath::dummy.to_string())), std::nullopt);
}

} // namespace nix
4 changes: 0 additions & 4 deletions src/libstore/include/nix/store/remote-fs-accessor.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ class RemoteFSAccessor : public SourceAccessor

NarCache narCache;

bool requireValidPath;

std::pair<ref<SourceAccessor>, CanonPath> fetch(const CanonPath & path);

friend struct BinaryCacheStore;
Expand All @@ -33,8 +31,6 @@ public:

/**
* @return nullptr if the store does not contain any object at that path.
*
* @todo This actually doesn't return nullptr, but throws on invalid paths.
*/
std::shared_ptr<SourceAccessor> accessObject(const StorePath & path);

Expand Down
26 changes: 16 additions & 10 deletions src/libstore/remote-fs-accessor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,19 @@ namespace nix {

void RemoteFSAccessor::anchor() {}

RemoteFSAccessor::RemoteFSAccessor(ref<Store> store, bool requireValidPath, std::optional<AbsolutePath> cacheDir)

@xokdvium xokdvium Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why leave the signature unchanged if the argument is fully unused?

RemoteFSAccessor::RemoteFSAccessor(ref<Store> store, bool, std::optional<AbsolutePath> cacheDir)
: store(store)
, narCache(cacheDir)
, requireValidPath(requireValidPath)
{
}

std::pair<ref<SourceAccessor>, CanonPath> RemoteFSAccessor::fetch(const CanonPath & path)
{
auto [storePath, restPath] = store->toStorePath(store->storeDir + path.abs());
if (requireValidPath && !store->isValidPath(storePath))
auto accessor = accessObject(storePath);
if (!accessor)
throw InvalidPath("path '%1%' is not a valid store path", store->printStorePath(storePath));
return {ref{accessObject(storePath)}, restPath};
return {ref{std::move(accessor)}, restPath};
}

std::shared_ptr<SourceAccessor> RemoteFSAccessor::accessObject(const StorePath & storePath)
Expand All @@ -25,8 +25,12 @@ std::shared_ptr<SourceAccessor> RemoteFSAccessor::accessObject(const StorePath &
if (auto * narHash = get(narHashes, storePath.hashPart()))
return narCache.getOrInsert(*narHash, [&](Sink & sink) { store->narFromPath(storePath, sink); });

// Query the path info to get the NAR hash
auto info = store->queryPathInfo(storePath);
std::shared_ptr<const ValidPathInfo> info;
try {
info = store->queryPathInfo(storePath);
} catch (InvalidPath &) {
return nullptr;
}

// Cache the mapping from store path to NAR hash
narHashes.emplace(storePath.hashPart(), info->narHash);
Expand All @@ -39,10 +43,12 @@ std::optional<SourceAccessor::Stat> RemoteFSAccessor::maybeLstat(const CanonPath
{
if (path.isRoot())
return Stat{.type = tDirectory};
/* FIXME: Correctly handle invalid names (return nullopt) and don't fail on
non-existent paths. */
auto res = fetch(path);
return res.first->maybeLstat(res.second);
/* FIXME: Correctly handle invalid names (return nullopt). */
auto [storePath, restPath] = store->toStorePath(store->storeDir + path.abs());
auto accessor = accessObject(storePath);

@xokdvium xokdvium Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we copy the fix for that in LocalFSStore accessor? The whole storeDir + path.abs() is pretty unnecessary. No idea why it was done this way

@Ericson2314 Ericson2314 Sep 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh we should just share a CanonPath -> std::pair<StorePath, CanonPath> function for both, and maybe that obviates the need for my maybeFetch because that new function is the actual used-many-times nugget here.

@Ericson2314 Ericson2314 Sep 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Store::toStorePath can also reuse that function in its own definition.

@xokdvium xokdvium Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh we should just share a CanonPath -> std::pair<StorePath, CanonPath> function for both

It's a bit more complicated though, because we also need to handle the case of "this CanonPath can't be a valid store path name, so return nullopt". See #16017

@xokdvium xokdvium Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I had previously fixed that stuff for the local store in #16072 and sprinkled around all these FIXMEs.

if (!accessor)
return std::nullopt;
return accessor->maybeLstat(restPath);
Comment on lines +46 to +51

@Ericson2314 Ericson2314 Sep 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This makes me think we should have a

std::pair<std::shared_ptr<SourceAccessor>, CanonPath> RemoteFSAccessor::maybeFetch(const CanonPath & path);

that fetch wraps

@xokdvium xokdvium Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure it's needed - the only caller would be maybeLstat which is specific enough that inlining would yield cleaner code.

@Ericson2314 Ericson2314 Sep 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, it is only needed once, but I also think that the fetch wrapper around maybeFetch is very trivial. It is just throwing if the first thing is non-null, and then returning std::pair<ref<SourceAccessor>, CanonPath> to "prove" that that check was done.

@Ericson2314 Ericson2314 Sep 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we don't do this, then fetch version and the maybeLstat version can go out of sink --- arguably we are getting bit by that a bit already. Forcing them to stay in sync is what I am trying to accomplish here, not "DRY for DRY's sake".

}

SourceAccessor::DirEntries RemoteFSAccessor::readDirectory(const CanonPath & path)
Expand Down
Loading