From 8f97cf99ef490642b62509d0d9f30de339accc86 Mon Sep 17 00:00:00 2001 From: pat-s Date: Tue, 1 Sep 2026 21:54:13 +0000 Subject: [PATCH] fix(build): give the existence cache a per-minor view of the slot (#191) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Motivation A restarted run recompiled everything it had already built. ``` attempted: 8683 already exists in S3: 3 actually compiled: 8683 ``` `amd64/resolute` had uploaded roughly 3000 binaries before being restarted, and rebuilt all of them. ## Cause This is a regression from #189, which I introduced. `s3_cache.rds` is derived from the same `file_names` that #189 filtered: ```r binary_cache <- setdiff(file_names, source_served) saveRDS(binary_cache, "/mnt/cache/packages/s3_cache.rds") ``` Removing per-minor objects from `file_names` was right for the candidate list and wrong for the existence cache. `build_binary_package()` was handed a cache containing no per-minor object at all, so its "not present in the remote bucket" check answered not-present for every one of them. The two consumers need opposite views of one listing: - the **candidate list** must ignore per-minor objects, or a package present under one minor prunes itself from every other minor's work; - the **existence cache** must not ignore them, or the work is redone. Conflating them was wrong in both directions: before #189 a per-minor pass believed the flat slot's binaries were its own and built nothing; after #189 it believed it had nothing and rebuilt the lot. ## Change The cache keeps paths relative to the slot, and `build-all.R` selects the part matching the pass it is running. `build_binary_package()` compares basenames and cannot distinguish `4.4/curl_1.0.tar.gz` from `curl_1.0.tar.gz`, so the choice has to be made where the running R minor is known. Verified: | pass | selects | |---|---| | primary (writes flat) | `curl_1.0`, `jsonlite_2.0`, `Archive/curl_0.9`, `PACKAGES.gz` | | `--sensitive-only` under 4.4 | `curl_1.0`, `rlang_1.3.0` (from `4.4/` only) | | `--sensitive-only` under 4.6 | `rlang_1.3.0` (from `4.6/` only) | | legacy basename cache | passthrough, unfiltered | `Archive/` counts as present for the primary pass: those are versions built and later superseded. ## Backwards compatibility A cache written before this change holds bare basenames. Filtering those by path would select nothing and trigger the very rebuild this prevents, so they are detected and used unfiltered; #190's staleness check replaces them on the next pipeline. ## Note on the running builds The five pipelines currently running are recompiling packages they already have. Their output is still correct — the binaries carry the right `Built` stamp for their slot — but the work is wasted. They should be restarted once this is merged. Reviewed-on: https://git.devxy.io/devxy/build-cran-binaries/pulls/191 --- local/build-all.R | 38 ++++++++++++++++++++++++++++++++++++-- local/packages-to-build.R | 24 +++++++++++++++++++++++- 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/local/build-all.R b/local/build-all.R index c4d7f97..eda92e9 100644 --- a/local/build-all.R +++ b/local/build-all.R @@ -189,8 +189,42 @@ sprintf( # Read pre-computed S3 listing from install-deps step # This avoids loading s3fs/reticulate/Python in the build container, # saving significant memory for the dependency-installer subprocesses -s3_cache <- readRDS("/mnt/cache/packages/s3_cache.rds") -sprintf("S3 cache: %s files", length(s3_cache)) +s3_cache_paths <- readRDS("/mnt/cache/packages/s3_cache.rds") + +# The cache is stored as paths relative to the slot, so a pass can select the +# objects that belong to it. `build_binary_package()` compares basenames, which +# cannot distinguish `4.4/curl_1.0.tar.gz` from `curl_1.0.tar.gz`, so the choice +# has to be made here where the running R minor is known. +# +# Getting this wrong is expensive in both directions: hand it everything and a +# per-minor pass believes the flat slot's binaries are its own and builds +# 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))) { + message("S3 cache is in the legacy basename format; using it unfiltered.") + return(paths) + } + in_minor <- grepl(sprintf("^%s/", r_minor), paths) + if (sensitive_only) { + basename(paths[in_minor]) + } else { + # The primary pass writes the flat slot. Archive/ counts as present there: + # those are versions built and later superseded. + basename(paths[!grepl("^[0-9]+\\.[0-9]+/", paths)]) + } +} + +s3_cache <- select_cache_for_pass(s3_cache_paths, sensitive_only, r_minor) +sprintf( + "S3 cache: %s files (%s of %s objects apply to this pass)", + length(s3_cache), + length(s3_cache), + length(s3_cache_paths) +) n <- nrow(chunk) mapply( diff --git a/local/packages-to-build.R b/local/packages-to-build.R index 9cc1ed6..64a2f5f 100644 --- a/local/packages-to-build.R +++ b/local/packages-to-build.R @@ -176,6 +176,28 @@ source_served <- tryCatch( ) binary_cache <- setdiff(file_names, source_served) + +# The existence cache and the candidate list need different views of the same +# listing, and conflating them is what made this wrong in both directions. +# +# The candidate list must ignore per-minor objects, or a package present under +# one minor prunes itself from every other minor's work (#189). The existence +# cache must NOT ignore them, or `build_binary_package()` is told nothing is +# present under any minor and recompiles the lot: amd64/resolute recompiled +# 8683 packages it had already built, reporting "already exists in S3" three +# times. +# +# 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. +contrib_prefix <- sprintf( + "devxy-rpkgs-binaries/%s/%s/latest/src/contrib/", + arch, + codename +) +relative_paths <- sub(contrib_prefix, "", s3_pkgs, fixed = TRUE) +source_basenames <- source_served +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), @@ -186,7 +208,7 @@ 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 -saveRDS(binary_cache, "/mnt/cache/packages/s3_cache.rds") +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 # package that needs building.