From d4f093923ae3a041f5463e98d18cc7e38ab4753e Mon Sep 17 00:00:00 2001 From: pat-s Date: Wed, 2 Sep 2026 07:11:10 +0000 Subject: [PATCH] fix(build): strip the s3 scheme so per-minor objects reach their pass (#192) ## Why #191 gave the existence cache a per-minor view of the slot, but the paths it filters never carried a minor prefix, so the fix could not take effect. `s3fs::s3_dir_ls()` returns keys with the `s3://` scheme attached: ``` s3://devxy-rpkgs-binaries/amd64/resolute/latest/src/contrib/4.4/oeli_0.7.6.tar.gz ``` The contrib prefix was stripped as a *fixed substring*, so it was removed from the middle of the key and the scheme survived: ``` s3://4.4/oeli_0.7.6.tar.gz ``` That leading `s3://` defeats both `^/` and `^[0-9]+\.[0-9]+/`, so all 21212 per-minor objects on amd64/resolute were classified as flat-slot objects. Pipeline 12011 shows it exactly: ``` sensitive pass: 0 files (0 of 119053 objects apply to this pass) primary pass: 119053 files (119053 of 119053 objects apply to this pass) ``` Each sensitive pass therefore ran with an empty cache and recompiled all 10248 sensitive packages it already had. ## What changed - `local/packages-to-build.R`: anchor the contrib prefix and swallow an optional `s3://` with it. - `local/packages-to-build.R`: abort when the strip leaves fewer per-minor paths than the listing held. The failure mode is silent and only surfaces as a multi-hour rebuild, and both counts derive from the same listing so they must agree exactly. - `local/packages-to-build.R` / `local/build-all.R`: mark the cache with a `slot_relative` attribute and read that, instead of sniffing for a `/`. A slot-relative cache for a slot with no per-minor or Archive object holds bare names too, and would have been misread as legacy and used unfiltered, which makes a per-minor pass believe the flat slot's binaries are its own and build nothing. - `scripts/purge_cdn_zone.sh`: indent the jq continuation lines by a multiple of two. This is unrelated, but it fails editorconfig-checker on `main` and blocks `prek run -a` for everyone. jq ignores the whitespace, and both response shapes still resolve. ## Verification Replaying the real key shapes through the old and new code: ``` sensitive(4.6) primary OLD paths: 0 8 <- reproduces production NEW paths: 3 3 Invariant NEW: listing=4 stripped=4 -> PASS Invariant OLD: listing=4 stripped=0 -> ABORT (would have caught this) ``` Per-minor `Archive/` objects are attributed to their minor, and the flat bucket keeps its own `Archive/`. `prek run -a` passes. Pipelines 12011-12015 were stopped rather than left to spend hours recompiling what they already had. Their uploads are not lost, so a fresh run inherits them. Reviewed-on: https://git.devxy.io/devxy/build-cran-binaries/pulls/192 --- local/build-all.R | 13 ++++++++---- local/packages-to-build.R | 42 ++++++++++++++++++++++++++++++++++++--- scripts/purge_cdn_zone.sh | 4 ++-- 3 files changed, 50 insertions(+), 9 deletions(-) diff --git a/local/build-all.R b/local/build-all.R index eda92e9..349092f 100644 --- a/local/build-all.R +++ b/local/build-all.R @@ -201,10 +201,15 @@ s3_cache_paths <- readRDS("/mnt/cache/packages/s3_cache.rds") # nothing; hand it nothing and it rebuilds what it already has. amd64/resolute # recompiled 8683 packages that way. select_cache_for_pass <- function(paths, sensitive_only, r_minor) { - # A cache written before this change holds bare basenames. Filtering those by - # path would select nothing and trigger a full rebuild, so use them as they - # are; #190's staleness check replaces it on the next pipeline anyway. - if (!any(grepl("/", paths, fixed = TRUE))) { + # A cache written before #191 holds bare basenames. Filtering those by path + # would select nothing and trigger a full rebuild, so use them as they are; + # #190's staleness check replaces it on the next pipeline anyway. + # + # The format is read from the marker `packages-to-build.R` sets, not guessed + # from the content: a slot-relative cache with no per-minor or Archive object + # holds bare names too, and treating that as legacy would hand a per-minor + # pass the flat slot's binaries and build nothing. + if (!isTRUE(attr(paths, "slot_relative"))) { message("S3 cache is in the legacy basename format; using it unfiltered.") return(paths) } diff --git a/local/packages-to-build.R b/local/packages-to-build.R index 64a2f5f..9e17f4d 100644 --- a/local/packages-to-build.R +++ b/local/packages-to-build.R @@ -190,14 +190,44 @@ binary_cache <- setdiff(file_names, source_served) # So the cache keeps the path relative to the slot, and `build-all.R` selects # the part that matches the pass it is running: the flat slot for the primary, # `/` for a per-minor pass. +# +# `s3_dir_ls()` returns keys with the `s3://` scheme attached, so stripping the +# prefix as a fixed substring takes it out of the middle and leaves +# `s3://4.4/curl_1.0.tar.gz`. That leading scheme defeats the `^/` test +# downstream, so every per-minor object is read as a flat-slot object: the +# sensitive passes see an empty cache and recompile everything they already +# have. Anchor the pattern and swallow the scheme with it. contrib_prefix <- sprintf( - "devxy-rpkgs-binaries/%s/%s/latest/src/contrib/", + "^(s3://)?devxy-rpkgs-binaries/%s/%s/latest/src/contrib/", arch, codename ) -relative_paths <- sub(contrib_prefix, "", s3_pkgs, fixed = TRUE) +relative_paths <- sub(contrib_prefix, "", s3_pkgs) + +# The strip is load-bearing and fails silently, so assert it. Both counts are +# derived from the same listing and use the same shape of pattern, so they must +# agree exactly; a mismatch means the prefix no longer describes the keys. +stripped_per_minor <- grepl("^[0-9]+\\.[0-9]+/[^/]+$", relative_paths) +if (sum(stripped_per_minor) != sum(per_minor_object)) { + stop(sprintf( + paste0( + "S3 prefix strip failed: %d per-minor objects in the listing, %d after ", + "stripping /%s/. Example key: %s" + ), + sum(per_minor_object), + sum(stripped_per_minor), + contrib_prefix, + if (any(per_minor_object)) { + s3_pkgs[which(per_minor_object)[1L]] + } else { + "" + } + )) +} source_basenames <- source_served -existence_cache <- relative_paths[!basename(relative_paths) %in% source_basenames] +existence_cache <- relative_paths[ + !basename(relative_paths) %in% source_basenames +] cat(sprintf( "S3 cache: %d objects, %d served as CRAN source, %d usable binaries\n", length(file_names), @@ -208,6 +238,12 @@ cat(sprintf( # Save the S3 file listing for the build step to use as s3_package_cache. # This avoids loading s3fs/reticulate in the build container, saving memory for # the dependency-installer subprocesses +# Mark the format explicitly. `build-all.R` has to tell a slot-relative cache +# from a pre-#191 basename one, and sniffing for a "/" cannot: a new-format +# cache for a slot with no per-minor or Archive objects holds bare names too, +# and would be read as legacy and used unfiltered, which makes a per-minor pass +# believe the flat slot's binaries are its own and build nothing. +attr(existence_cache, "slot_relative") <- TRUE saveRDS(existence_cache, "/mnt/cache/packages/s3_cache.rds") # Built from the filtered listing, not the raw one: `s3_dt` is subtracted from # the build list below, so a source fallback left in here would exclude the very diff --git a/scripts/purge_cdn_zone.sh b/scripts/purge_cdn_zone.sh index 6c07eef..8e85cb6 100755 --- a/scripts/purge_cdn_zone.sh +++ b/scripts/purge_cdn_zone.sh @@ -70,8 +70,8 @@ resolve_zone_id() { zone_id=$( jq -r --arg hostname "${zone}" \ '(if type == "object" then (.Items // []) else . end)[] - | select(any(.Hostnames[]?; .Value == $hostname)) - | .Id' \ + | select(any(.Hostnames[]?; .Value == $hostname)) + | .Id' \ "${response_file}" ) rm -f "${response_file}"