From c443f6774f5da4a39df6d5545737c1d122b52d24 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 16 Aug 2026 15:58:24 +0000 Subject: [PATCH] fix(ci): Upload a shard's results three times, not once Shard 16 of run 31951756102 was three minutes into a 196-minute check budget when its runner went away: ``` the runner service is stopped, or a manually started runner is canceled. ``` Nothing the driver did: 143 is SIGTERM, and the other 39 shards of that wave carried on. The runner was reclaimed. What made it expensive is ours. The driver has written its results as it goes since #2836 -- one manifest line per package, appended, precisely so that a shard killed part way through still accounts for what it finished. But the upload was a single step after the checks, and `if: always()` cannot save a step on a runner that is no longer there: it was skipped, and all 87 packages came back `missing` after an hour of runner time. The checks now run in three slices, each followed by an upload. The slices share the results directory and overwrite one artifact name, so the newest upload to survive is a superset of the ones before it, and the loss is bounded to a third of the shard. The packages are dealt round robin rather than in blocks. `runnable` is heaviest first, so a contiguous cut would put every long check in the first slice and leave the last one nothing but cheap ones. Three things had to become slice-aware: * the end-of-run "whatever the loop never reached" pass reads the manifest back and skips packages already accounted for, so slice 2 cannot bury slice 1's results under a `deferred` line that a later line wins with. It still writes `deferred` for *later* slices' packages, which is the truth at that moment and better than the `missing` the collector reconciles into; * `timing.json` accumulates `checks` and `check_seconds` across slices, since the collector fits the cost model from them and a third of a shard's checks next to a whole job's `script_seconds` would price every shard at a third of what it costs; * the per-slice summary covers that slice's packages, not the whole shard's, whose other two thirds are still at their initial `deferred` in this process. `CHECK_SLICE` is unset outside the workflow, and then nothing slices. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01D1xpHRV7yVfgtJg4vp9P7z --- .github/workflows/revdep2.yaml | 88 ++++++++++++++++++++---- .github/workflows/revdep2/README.md | 2 +- .github/workflows/revdep2/shard.R | 103 ++++++++++++++++++++++++++-- 3 files changed, 175 insertions(+), 18 deletions(-) diff --git a/.github/workflows/revdep2.yaml b/.github/workflows/revdep2.yaml index 870d71fa6fe..d8aeb4eef91 100644 --- a/.github/workflows/revdep2.yaml +++ b/.github/workflows/revdep2.yaml @@ -688,27 +688,90 @@ jobs: Rscript ./.github/workflows/revdep2/shard.R shell: bash - # `!cancelled()` rather than the default `success()`: an install step - # that *failed* still leaves a shard full of packages to account for, and - # the driver reports a package it could not install as a depfail, which - # is a result. Skipping this step instead left all 50 of shard 3's - # packages as `missing` in run 31893156685 -- the one outcome that tells - # nobody anything. - - name: Check the shard + # The checks run in three slices, each followed by an upload. + # + # The driver has always written its results as it goes, so that a shard + # killed part way through still accounts for what it finished -- but the + # upload was one step at the end, and `if: always()` cannot save a step on + # a runner that is no longer there. Shard 16 of run 31951756102 got three + # minutes into a 196-minute check budget before + # + # ##[error]The runner has received a shutdown signal. + # ##[error]Process completed with exit code 143. + # + # took the whole job with it. The upload was skipped and all 87 packages + # were reported `missing`, having cost a runner an hour. At three slices + # that is bounded to a third of the shard, and the interim artifacts also + # say `deferred` for the packages not yet reached, which is truer than the + # `missing` the collector would otherwise reconcile them into. + # + # Three rather than more because each upload is a step, and the artifact + # carries the whole results directory every time; the shard's own deadline + # already caps the tail. + # + # `!cancelled()` rather than the default `success()`: an install step that + # *failed* still leaves a shard full of packages to account for, and the + # driver reports a package it could not install as a depfail, which is a + # result. Skipping this step instead left all 50 of shard 3's packages as + # `missing` in run 31893156685 -- the one outcome that tells nobody + # anything. The later slices additionally tolerate an earlier one having + # failed, for the same reason. + - name: Check the shard (1 of 3) if: '!cancelled()' env: PHASE: check + CHECK_SLICE: 1/3 run: | Rscript ./.github/workflows/revdep2/shard.R shell: bash # Named per attempt: a re-run of one shard must not overwrite the results # the other shards uploaded in the first attempt; the collector reads - # every attempt and lets the later one win per package. - # Before the upload, so the last sample and the OOM verdict ride in the - # artifact. It cannot run when the *runner* died -- hence the sampler - # above -- but it is what answers "was it memory?" in every case where the - # job merely failed. + # every attempt and lets the later one win per package. `overwrite` so the + # slices replace one another under the one name -- they share a results + # directory, so the newest upload is a superset of the ones before it. + - name: Upload the shard results (1 of 3) + uses: actions/upload-artifact@v6 + if: always() + with: + name: revdep2-results-${{ matrix.shard }}-${{ github.run_attempt }} + path: ${{ runner.temp }}/results + if-no-files-found: ignore + overwrite: true + retention-days: 30 + + - name: Check the shard (2 of 3) + if: '!cancelled()' + env: + PHASE: check + CHECK_SLICE: 2/3 + run: | + Rscript ./.github/workflows/revdep2/shard.R + shell: bash + + - name: Upload the shard results (2 of 3) + uses: actions/upload-artifact@v6 + if: always() + with: + name: revdep2-results-${{ matrix.shard }}-${{ github.run_attempt }} + path: ${{ runner.temp }}/results + if-no-files-found: ignore + overwrite: true + retention-days: 30 + + - name: Check the shard (3 of 3) + if: '!cancelled()' + env: + PHASE: check + CHECK_SLICE: 3/3 + run: | + Rscript ./.github/workflows/revdep2/shard.R + shell: bash + + # Before the final upload, so the last sample and the OOM verdict ride in + # the artifact. It cannot run when the *runner* died -- hence the sampler + # started before the install -- but it is what answers "was it memory?" in + # every case where the job merely failed. - name: Report what the shard consumed if: always() run: | @@ -724,6 +787,7 @@ jobs: name: revdep2-results-${{ matrix.shard }}-${{ github.run_attempt }} path: ${{ runner.temp }}/results if-no-files-found: ignore + overwrite: true retention-days: 30 collect: diff --git a/.github/workflows/revdep2/README.md b/.github/workflows/revdep2/README.md index 62a2339017e..b04f5d8cf3a 100644 --- a/.github/workflows/revdep2/README.md +++ b/.github/workflows/revdep2/README.md @@ -736,7 +736,7 @@ the report is about *results*, a retry is about *coverage*. | The preflight job itself dies | the shards run anyway and install their own unions, the collector still reports; only the free rebuild and the early diagnosis are lost | | A shard hits its deadline | remaining packages `deferred`; finished old-halves still uploaded and baseline-fed | | The runner stops answering the service | "The hosted runner lost communication with the server" names CPU, memory and network starvation as its causes, and a dead runner takes its `if: always()` steps with it — so both the preflight and the shards stream a resource sample every 30 s while they work, and the checks run under `nice -n 10` (and `ionice -c3` where it exists) so the agent is never the process that loses | -| A shard job dies hard | the collector reconciles against the plan: its packages are reported `missing`, naming the shard, and `retry-run` re-checks exactly them | +| A shard job dies hard | the checks run in three slices, each followed by an upload, so at most a third is lost and the rest is already in the artifact; packages a later slice never reached are `deferred`, anything with no line at all the collector reconciles against the plan as `missing`, naming the shard, and `retry-run` re-checks exactly those | | Every shard dies | the report is still written, with every package `missing`; the artifact download is tolerated, not required | | The batch is too big for 250 shards | the plan refuses before anything starts, and names the `part` split that fits | | A shard is re-run | new artifact per attempt; the collector lets the later attempt win per package | diff --git a/.github/workflows/revdep2/shard.R b/.github/workflows/revdep2/shard.R index e482dd4e3a6..033b280074b 100644 --- a/.github/workflows/revdep2/shard.R +++ b/.github/workflows/revdep2/shard.R @@ -107,6 +107,41 @@ do_install <- phase %in% c("all", "install") do_check <- phase %in% c("all", "check") install_state <- file.path(work, "install-state.json") +# Which slice of the shard's packages this invocation checks, as `i/n`. +# +# The driver has always written its results as it goes -- one manifest line per +# package, appended -- so that a shard killed part way through still accounts +# for what it finished. That only helps if someone *uploads* them, and the +# upload was one step at the very end. Shard 16 of run 31951756102 got three +# minutes into a 196-minute check budget before its runner was reclaimed: +# +# ##[error]The runner has received a shutdown signal. +# ##[error]Process completed with exit code 143. +# +# `if: always()` cannot help there -- a reclaimed runner runs nothing further, +# so the upload was skipped and all 87 packages came back `missing`. Slicing +# the check phase into several steps, each followed by an upload, bounds that +# loss to one slice. The slices share `OUT_DIR`, and the artifact is overwritten +# under one name, so the last upload to survive carries everything before it. +check_slice <- local({ + raw <- trimws(env_chr("CHECK_SLICE")) + if (!nzchar(raw)) { + return(list(index = 1L, of = 1L)) + } + parts <- suppressWarnings(as.integer(strsplit(raw, "/", fixed = TRUE)[[1]])) + if ( + length(parts) != 2 || + anyNA(parts) || + parts[[1]] < 1 || + parts[[2]] < 1 || + parts[[1]] > parts[[2]] + ) { + stop("CHECK_SLICE must be `i/n` with 1 <= i <= n, not ", raw, call. = FALSE) + } + list(index = parts[[1]], of = parts[[2]]) +}) +last_slice <- check_slice$index == check_slice$of + inform( "Shard ", shard_index, @@ -424,7 +459,11 @@ if (!file.exists(install_state)) { ) } append_summary(c( - sprintf("### Shard %d", shard_index), + if (check_slice$of > 1L) { + sprintf("### Shard %d, slice %d/%d", shard_index, check_slice$index, check_slice$of) + } else { + sprintf("### Shard %d", shard_index) + }, "", sprintf("%d package(s) not checked: %s.", length(members), reason) )) @@ -570,6 +609,23 @@ for (name in runnable) { } runnable <- names(sources) +# Dealt round robin rather than in blocks. `runnable` is heaviest first, so a +# contiguous cut would put every long check in the first slice and leave the +# last one with nothing but the cheap ones -- and the deadline, which stops the +# shard when the next check will not fit, would then bite unevenly. Round robin +# gives every slice the same mix. +if (check_slice$of > 1L) { + mine <- seq(check_slice$index, length(runnable), by = check_slice$of) + inform(sprintf( + "Slice %d/%d: %d of this shard's %d runnable package(s)", + check_slice$index, + check_slice$of, + length(mine), + length(runnable) + )) + runnable <- runnable[mine] +} + # ------------------------------------------------------------------ checks --- # Stop before a check the trailing estimate says will not finish -- but always @@ -1149,10 +1205,36 @@ for (position in seq_along(runnable)) { # Whatever the loop never reached: deferred packages, and the ones a depfail or # a missing source knocked out before it started. -for (name in setdiff(members, reported)) { +# +# Under slicing this also covers the packages belonging to *later* slices, which +# is deliberate: an interim artifact that says `deferred` for them is the truth +# at that moment, and better than the `missing` the collector would otherwise +# reconcile them into. What it must not do is overwrite a result an *earlier* +# slice already wrote -- those packages are still `deferred` in this process's +# memory, and a later line wins in the collector. So the manifest is read back +# and anything already accounted for is left alone. +already <- if (file.exists(manifest_path)) { + lines <- readLines(manifest_path, warn = FALSE) + lines <- lines[nzchar(trimws(lines))] + vapply( + lines, + function(line) jsonlite::fromJSON(line, simplifyVector = FALSE)$package, + character(1), + USE.NAMES = FALSE + ) +} else { + character() +} +for (name in setdiff(members, c(reported, already))) { write_manifest_line(get(name, envir = state)) } -entries <- lapply(members, function(name) get(name, envir = state)) +# The summary below is this slice's, not the shard's: the other slices' packages +# are still at their initial `deferred` in this process and would pad every +# table with rows that say nothing. +entries <- lapply( + if (check_slice$of > 1L) reported else members, + function(name) get(name, envir = state) +) # ----------------------------------------------------------------- timings --- @@ -1161,16 +1243,26 @@ entries <- lapply(members, function(name) get(name, envir = state)) # cost model from them. The job's own minutes -- the runner image, R, TinyTeX, # the artifact downloads before this script even starts -- are not visible from # here; the collector reads those off the API and adds them. +# Across slices, not per slice: the collector fits the cost model from these, +# and a `check_seconds` covering a third of the shard next to a `script_seconds` +# covering the job would make every shard look three times cheaper than it is. +earlier <- if (file.exists(file.path(out_dir, "timing.json"))) { + tryCatch(read_json(file.path(out_dir, "timing.json")), error = function(e) { + NULL + }) +} else { + NULL +} write_json( list( index = shard_index, packages = length(members), - checks = checks_started, + checks = checks_started + (earlier$checks %||% 0L), install_packages = installed_state$install_packages, restored = installed_state$restored, restore_seconds = installed_state$restore_seconds, install_seconds = installed_state$install_seconds, - check_seconds = round(check_seconds, 1), + check_seconds = round(check_seconds + (earlier$check_seconds %||% 0), 1), # Both phases, because the collector fits `setup_minutes` as # `job_minutes - script_minutes` -- the minutes before the driver starts. # Reporting only this process would have charged the whole install phase to @@ -1182,6 +1274,7 @@ write_json( 1 ), started_at = installed_state$started_at %||% + earlier$started_at %||% format(script_started, "%Y-%m-%dT%H:%M:%SZ", tz = "UTC"), finished_at = now_utc(), planned_minutes = shard$estimate_minutes,