From f02218873e846d93e079b96de3a2ba1bb369c12a Mon Sep 17 00:00:00 2001 From: Bernardo Meurer Costa Date: Fri, 10 Oct 2025 19:39:09 +0000 Subject: [PATCH] fix(libstore): improve http-binary-cache-store S3 compatibility This commit adds two key fixes to http-binary-cache-store.cc to properly support the new curl-based S3 implementation: 1. **Consistent cache key handling**: Use `getReference().render(withParams=false)` for disk cache keys instead of `cacheUri.to_string()`. This ensures cache keys are consistent with the S3 implementation and don't include query parameters, which matches the behavior expected by Store::queryPathInfo() lookups. 2. **S3 query parameter preservation**: When generating file transfer requests for S3 URLs, preserve query parameters from the base URL (region, endpoint, etc.) when the relative path doesn't have its own query parameters. This ensures S3-specific configuration is propagated to all requests. --- src/libstore/http-binary-cache-store.cc | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/libstore/http-binary-cache-store.cc b/src/libstore/http-binary-cache-store.cc index 5d4fba163..8d5f427af 100644 --- a/src/libstore/http-binary-cache-store.cc +++ b/src/libstore/http-binary-cache-store.cc @@ -78,7 +78,11 @@ public: void init() override { // FIXME: do this lazily? - if (auto cacheInfo = diskCache->upToDateCacheExists(config->cacheUri.to_string())) { + // For consistent cache key handling, use the reference without parameters + // This matches what's used in Store::queryPathInfo() lookups + auto cacheKey = config->getReference().render(/*withParams=*/false); + + if (auto cacheInfo = diskCache->upToDateCacheExists(cacheKey)) { config->wantMassQuery.setDefault(cacheInfo->wantMassQuery); config->priority.setDefault(cacheInfo->priority); } else { @@ -87,8 +91,7 @@ public: } catch (UploadToHTTP &) { throw Error("'%s' does not appear to be a binary cache", config->cacheUri.to_string()); } - diskCache->createCache( - config->cacheUri.to_string(), config->storeDir, config->wantMassQuery, config->priority); + diskCache->createCache(cacheKey, config->storeDir, config->wantMassQuery, config->priority); } } @@ -184,7 +187,16 @@ protected: field which is `nar/15f99rdaf26k39knmzry4xd0d97wp6yfpnfk1z9avakis7ipb9yg.nar?hash=zphkqn2wg8mnvbkixnl2aadkbn0rcnfj` (note the query param) and that gets passed here. */ - return FileTransferRequest(parseURLRelative(path, cacheUriWithTrailingSlash)); + auto result = parseURLRelative(path, cacheUriWithTrailingSlash); + + /* For S3 URLs, preserve query parameters from the base URL when the + relative path doesn't have its own query parameters. This is needed + to preserve S3-specific parameters like endpoint and region. */ + if (config->cacheUri.scheme == "s3" && result.query.empty()) { + result.query = config->cacheUri.query; + } + + return FileTransferRequest(result); } void getFile(const std::string & path, Sink & sink) override