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,