diff --git a/CLAUDE.md b/CLAUDE.md index 885c6633..c7cfc62a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -9,7 +9,7 @@ Experimental package — breaking all the time and loving the learning curve. St **Repository:** NewGraphEnvironment/link **Primary Language:** R **Prefix:** `lnk_` -**Branch:** `main` (v0.45.1 as of 2026-08-06) +**Branch:** `main` (v0.46.0 as of 2026-08-28) ## Status (2026-08-06) — v0.45.1 shipped (WSG drainage closure rebuilt; #227 re-scoped) diff --git a/DESCRIPTION b/DESCRIPTION index c868b2c2..26b1db0a 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,7 +1,7 @@ Package: link Title: Stream Network Habitat Interpretation (Experimental) -Version: 0.46.0 -Date: 2026-08-28 +Version: 0.47.0 +Date: 2026-08-30 Authors@R: c( person("Allan", "Irvine", , "airvine@newgraphenvironment.com", role = c("aut", "cre"), @@ -21,6 +21,7 @@ Imports: crate (>= 0.0.2), DBI, digest, + fresh (>= 0.33.0), httr, jsonlite, RPostgres, @@ -37,7 +38,6 @@ Suggests: bcdata, bookdown, dplyr, - fresh (>= 0.33.0), gq, knitr, lintr, diff --git a/NAMESPACE b/NAMESPACE index 173e2b2b..15701ab6 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -42,6 +42,10 @@ export(lnk_pipeline_run) export(lnk_pipeline_setup) export(lnk_pipeline_species) export(lnk_points_snap) +export(lnk_preflight_fresh) +export(lnk_preflight_parity) +export(lnk_preflight_stamp) +export(lnk_preflight_vintage) export(lnk_presence) export(lnk_rollup_wsg) export(lnk_rules_build) diff --git a/NEWS.md b/NEWS.md index 9ec001f7..32a40d32 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,23 @@ +# link 0.47.0 + +Closes the three gaps that would have made a provincial rerun silently skip most of its work ([#246](https://github.com/NewGraphEnvironment/link/issues/246)), and absorbs the sibling-host parity hook ([#183](https://github.com/NewGraphEnvironment/link/issues/183)). + +**The root cause was narrower than it looked, and it was a declaration.** The cypher image bakes `fresh` 0.31.0, which exports neither `frs_wsg_drainage` nor `frs_wsg_outlets` — `frs_wsg_drainage` arrived in 0.32.0 and `frs_wsg_outlets` only in 0.33.0, so the floor is v0.33.0 rather than "newer than 0.31.0". `DESCRIPTION` already pinned `NewGraphEnvironment/fresh@v0.33.0`, but the pin was never resolved: `fresh` sat in **Suggests**, and `pak::local_install()` defaults to `dependencies = NA`, which is hard dependencies only. Meanwhile 20 files call `fresh::` with no `requireNamespace()` guard, including a default argument on an exported function — so `fresh` is a hard runtime dependency that was declared optional, and `library(link)` loaded cleanly on a host whose fresh could not run the pipeline. Moving it to **Imports** makes the existing pin load-bearing with no change to the `pak` call at all: `upgrade = FALSE` suppresses gratuitous upgrades, not required ones, and 0.31.0 does not satisfy the floor. The failure it prevents was silent in the worst way — `wsg_run_one.R` catches the missing-symbol error and `quit(status = 1)`s, the bucket loop logs `[WARN]` and continues, and the host exits 0 with none of its watershed groups persisted. + +New exported **`lnk_preflight_fresh()`** asserts symbols rather than a version string, because a version is a proxy that fails in both directions: it reads `0.33.0` on a partial install or against a shadowing library path, and reads "wrong" while every needed symbol is present. A drift guard walks link's own parsed **namespace** rather than the `R/` directory — a source scan finds nothing in an installed package and would report a clean check — so a new `fresh::` call site cannot ship undeclared. Two independent methods agree the real call-site count is 12; a naive grep would have listed 21 and included `frs_point_snap_knn`, which fresh 0.33.0 does not export and link never calls. + +**`study_area_run.sh` gains pre-flight gates in two blocks, because there are two questions.** A cypher's software is predictable from the dispatcher before the cypher exists — its link from `git reset --hard origin/$BRANCH`, its fresh from `DESCRIPTION` — so `preflight_local()` validates what the workers are going to get, free and before the spin, and `preflight_hosts()` confirms after prep that they got it. Predict before spend; verify before write. The second cannot run earlier and does not need to: it precedes any write to the persist, and a failure trips the existing EXIT trap, bounding the loss at prep time. + +The credential probe is the substantive fix. `doctl` and the tofu provider use **different** tokens; both were minted 2026-05-18 and both expired on the same day, and only the first was ever checked. `tofu plan` cannot serve as the second probe — against a workspace with no resources it returns `Plan: N to add` without contacting DigitalOcean at all. The tfvars token now goes to `/v2/account` directly, with 401, an unreachable API and any other status reported as distinct outcomes rather than collapsed into "not success". + +New exported **`lnk_preflight_vintage()`** and **`lnk_preflight_parity()`** / **`lnk_preflight_stamp()`** put the load-bearing predicates in R where testthat can prove them. Vintage: absent is not fresh, a NULL timestamp is not fresh, and an empty result set is not a pass. `last_analyze` alone turned out to be unusable — measured NULL on all ten input primitives — so the query takes `GREATEST` of it and `last_autoanalyze`. Parity keys on **`repo_sha`**, not `link_sha`: the latter is a real SHA on the `load_all` dispatcher and `NA` on every pak-installed cypher, so comparing it would fail every legitimate run, while `fresh_sha` is `NA` on both and would pass vacuously. `repo_sha` is read on each host from the checkout it installed from — evidence, rather than a restatement of what the dispatcher believes. For the same reason `cypher_prep.sh` writes each cypher's **own** observed SHA into its `~/.Renviron`, while only `FWAPG_GIT_SHA` crosses the ssh boundary, since cyphers have no fwapg checkout and would otherwise log `NULL`. + +**Two post-conditions, and a sentinel that was wrong in both directions.** `schema_consolidate` DELETEs the destination bucket and *then* COPYs, so a host that produced nothing does not merely fail to add rows — it removes the rows already there and returns `ok = TRUE`. Under #246 the run was deleting data and reporting success. Every host must now account for its whole bucket before consolidate, and every run WSG must have rows in the persist afterwards. Separately, the prep gate grepped for `snapshot_bcfp.sh: complete`, which the snapshot emits *before* `lnk_persist_init` runs: a persist_init FATAL exited 1 with the sentinel already in the log, so the gate passed and watershed groups ran against a half-prepped cypher. The snapshot's legitimate skip-if-current path, meanwhile, never emits it, so a skipped load read as FATAL. An anchored `=== READY` is correct in all four measured cases. + +New **`data-raw/study_area_buckets.R`** derives the host buckets instead of asserting them: union-find over per-WSG `frs_wsg_drainage()` closures, then greedy LPT over the resulting components, writing `research/study_areas.md`. Components rather than study areas because overlapping closures make consolidate last-writer-wins on the shared groups — whichever host finished last would silently decide the answer — so the script **asserts** disjointness rather than assuming it. It reproduces the issue's numbers from first principles: 96 focal groups, 125 in closure, 119 modelable, 22 drainage-independent components, 39 on the dispatcher, and the same six groups dropped by species presence. + +There is deliberately **no global pre-flight bypass**. `--preflight-note=""` downgrades only vintage and parity, and only with a written justification, mirroring `lnk_wsg_downstream_check(override=)`; `--auto-install` is remediation rather than a skip. New `--preflight-only` runs every local gate and exits, so the whole block is testable with one command and no spend. Every gate was exercised against **both** known answers on m1 before shipping — tables in the PR and in `RUNBOOK.md` §8d. + # link 0.46.0 Enforces the DS-first precondition that `data-raw/wsg_run_one.R` has stated in its own header since link#175 and that nothing checked ([#227](https://github.com/NewGraphEnvironment/link/issues/227)). Accessibility is not computed from the focal watershed group alone — link reads the **already-persisted** barriers of the groups downstream — so modelling a WSG out of order writes `streams_access` / `streams_mapping_code` marking segments accessible that are in fact dammed off, prints `done in N min`, and exits 0. A wrong answer indistinguishable from a right one. New exported `lnk_wsg_downstream_check()` verifies it instead: find the blocking dams on the focal WSG's downstream flow path, confirm each is already persisted as a barrier, and fail loud naming them when it is not. diff --git a/R/lnk_preflight_fresh.R b/R/lnk_preflight_fresh.R new file mode 100644 index 00000000..7e27a9ed --- /dev/null +++ b/R/lnk_preflight_fresh.R @@ -0,0 +1,189 @@ +#' Does the installed 'fresh' provide what the pipeline calls? +#' +#' `link` calls `fresh::` in a dozen places with no `requireNamespace()` +#' guard, including a default argument on an exported function +#' ([lnk_wsg_downstream_check()], whose `outlets` defaults to +#' `fresh::frs_wsg_outlets()`). A `fresh` that is merely *present* is +#' therefore not enough — it has to export the symbols. +#' +#' On the cypher droplets the installed version is whatever was baked into +#' the machine image, and the failure mode is silent: `wsg_run_one.R` +#' catches the missing-symbol error and `quit(status = 1)`s, the bucket +#' loop logs `[WARN]` and continues, the host exits 0, and the watershed +#' groups are simply absent from the persist. Nothing downstream can tell +#' "not modelled" from "modelled empty". That is the 2026-08 failure this +#' exists to stop (link#246). +#' +#' Symbols are checked rather than a version string because a version is a +#' proxy that fails in both directions: it can read `0.33.0` on a partial +#' install or against a shadowing library path, and it can read "wrong" +#' while every needed symbol is present. Loading the namespace also +#' exercises `fresh`'s own `Imports` resolution, which reading a +#' `DESCRIPTION` off disk does not. +#' +#' @param required Character vector of `fresh` exports the pipeline cannot +#' run without. Defaults to the curated list in `.lnk_fresh_required()`. +#' @param required_internal Character vector of non-exported `fresh` +#' objects reached via [utils::getFromNamespace()]. +#' @param min_version Minimum acceptable `fresh` version. Defaults to the +#' floor declared in link's own `DESCRIPTION`, so the pin lives in one +#' place. +#' @param quiet Suppress the human-readable report. The report is the +#' point on a cypher, where the log is all the operator gets. +#' +#' @return Invisibly, a list with `ok`, `version`, `version_ok`, +#' `missing`, `missing_internal` and `message`. +#' +#' @family preflight +#' +#' @export +#' +#' @examples +#' res <- lnk_preflight_fresh(quiet = TRUE) +#' res$ok +#' res$version +#' +#' # A symbol fresh does not export fails, and is named in the report: +#' bad <- lnk_preflight_fresh(required = "frs_not_a_real_export", quiet = TRUE) +#' bad$missing +lnk_preflight_fresh <- function(required = .lnk_fresh_required(), + required_internal = .lnk_fresh_required_internal(), + min_version = .lnk_fresh_floor(), + quiet = FALSE) { + stopifnot( + is.character(required), length(required) >= 1L, all(nzchar(required)), + is.character(required_internal), all(nzchar(required_internal)), + is.character(min_version), length(min_version) == 1L, nzchar(min_version), + is.logical(quiet), length(quiet) == 1L, !is.na(quiet)) + + version <- .lnk_pkg_version_or_na("fresh") + ns <- .lnk_fresh_ns() + + if (is.null(ns)) { + out <- list(ok = FALSE, version = NA_character_, version_ok = FALSE, + missing = required, missing_internal = required_internal, + message = "fresh is not installed or its namespace will not load") + } else { + missing <- setdiff(required, getNamespaceExports(ns)) + missing_internal <- required_internal[ + !vapply(required_internal, exists, logical(1), + envir = ns, inherits = FALSE)] + version_ok <- !is.na(version) && + utils::compareVersion(version, min_version) >= 0L + out <- list( + ok = length(missing) == 0L && length(missing_internal) == 0L && version_ok, + version = version, version_ok = version_ok, + missing = missing, missing_internal = missing_internal, + message = .lnk_fresh_message(version, min_version, missing, + missing_internal, version_ok)) + } + + if (!quiet) message(out$message) + invisible(out) +} + + +# Namespace lookup behind its own function so the "fresh is absent" branch +# stays testable. Mocking `base::asNamespace` instead would break every +# other namespace lookup in the same test file, including testthat's own. +.lnk_fresh_ns <- function() { + tryCatch(asNamespace("fresh"), error = function(e) NULL) +} + +# The `fresh` exports link actually calls, curated rather than derived at +# runtime. Auto-deriving by grepping for `fresh::` over-fires: most matches +# in R/ are roxygen cross-references and ordinary comments, not call sites +# (measured 2026-08-30: 21 distinct symbols mentioned, 12 genuinely called). +# `.lnk_fresh_callsites()` below is the drift guard that keeps this honest. +.lnk_fresh_required <- function() { + c("frs_break_apply", "frs_break_find", "frs_candidates_pick", + "frs_col_generate", "frs_col_join", "frs_habitat_classify", + "frs_habitat_overlay", "frs_network_features", "frs_order_child", + "frs_params", "frs_wsg_drainage", "frs_wsg_outlets") +} + +# Non-exported fresh objects link reaches via getFromNamespace(). +# R/lnk_pipeline_connect.R:101. +.lnk_fresh_required_internal <- function() { + ".frs_run_connectivity" +} + +# Every `fresh::sym` / `fresh:::sym` reached from link's own namespace, +# discovered by walking parsed function bodies rather than reading R/. +# +# Walking the namespace works for an INSTALLED package, where R/ does not +# exist. A source-directory scan would silently find nothing there and +# report a clean drift check — the "guard that matches a container rather +# than the artifact" failure. This measures the code that will actually run. +.lnk_fresh_callsites <- function(pkg = "link") { + ns <- asNamespace(pkg) + found <- character(0) + + # A formal with no default is the empty symbol. It is a perfectly good + # object to hold in a list, but passing it as an argument raises + # "argument is missing, with no default" — so filter on identity before + # recursing, never by trying to evaluate it. + drop_empty <- function(xs) { + xs[!vapply(xs, identical, logical(1), quote(expr = ))] + } + + walk <- function(x) { + if (is.call(x)) { + fn <- x[[1L]] + if (is.call(fn) && length(fn) == 3L && is.name(fn[[1L]]) && + as.character(fn[[1L]]) %in% c("::", ":::") && + identical(as.character(fn[[2L]]), "fresh")) { + found <<- c(found, as.character(fn[[3L]])) + } + } + if (is.call(x) || is.pairlist(x) || is.expression(x) || is.list(x)) { + for (el in drop_empty(as.list(x))) walk(el) + } + invisible(NULL) + } + + for (nm in ls(ns, all.names = TRUE)) { + obj <- tryCatch(get(nm, envir = ns), error = function(e) NULL) + if (!is.function(obj)) next + walk(body(obj)) + walk(formals(obj)) # default args count: frs_wsg_outlets() + } + sort(unique(found)) +} + +# Single-source the version floor from link's own DESCRIPTION, so the pin +# cannot rot in a second place. Returns "0.0.0" when no floor is declared — +# a permissive default is correct here because the symbol check, not the +# version, is what this function is actually for. +.lnk_fresh_floor <- function(desc = utils::packageDescription("link")) { + dep <- paste(c(desc$Imports, desc$Depends, desc$Suggests), collapse = ", ") + if (!nzchar(dep)) return("0.0.0") + m <- regmatches(dep, regexpr("fresh[[:space:]]*\\([[:space:]]*>=[^)]*\\)", dep)) + if (!length(m) || !nzchar(m)) return("0.0.0") + v <- gsub("[^0-9.]", "", sub(".*>=", "", m)) + if (!nzchar(v)) "0.0.0" else v +} + +.lnk_fresh_message <- function(version, min_version, missing, + missing_internal, version_ok) { + head <- sprintf("fresh %s (floor %s)", + if (is.na(version)) "NOT INSTALLED" else version, min_version) + if (length(missing) == 0L && length(missing_internal) == 0L && version_ok) { + return(paste0("[preflight] ", head, " - OK, all required symbols present")) + } + parts <- character(0) + if (!version_ok) { + parts <- c(parts, sprintf(" version below the floor declared in link's DESCRIPTION")) + } + if (length(missing)) { + parts <- c(parts, sprintf(" missing exports: %s", + paste(missing, collapse = ", "))) + } + if (length(missing_internal)) { + parts <- c(parts, sprintf(" missing internals: %s", + paste(missing_internal, collapse = ", "))) + } + parts <- c(parts, + " fix: pak::pkg_install(\"NewGraphEnvironment/fresh@\") at or above the floor") + paste(c(paste0("[preflight] ", head, " - FAILED"), parts), collapse = "\n") +} diff --git a/R/lnk_preflight_parity.R b/R/lnk_preflight_parity.R new file mode 100644 index 00000000..43607e98 --- /dev/null +++ b/R/lnk_preflight_parity.R @@ -0,0 +1,164 @@ +#' Do all hosts' pre-flight stamps agree? +#' +#' A stale install on one cypher silently produces different rollup +#' numbers: the provincial run looks fine, and the bcfishpass parity diff +#' then contains version drift as well as methodology drift, with no way to +#' separate them afterwards. This is the #183 sibling-host parity hook, +#' absorbed into link#246. +#' +#' Three properties, each of which fails toward stop: +#' +#' - **Everybody answered.** `n_expected` has no default on purpose. A +#' dropped ssh yields a short table, and a short table judged on its own +#' terms produces "no mismatches found" — an affirmative claim of +#' agreement among hosts that never replied. +#' - **Nothing is unresolved.** A field that is the literal `"NA"` on any +#' host is a failure, not a neutral. `NA == NA` is not agreement. +#' - **Nothing is dirty.** A SHA recorded against a dirty tree is a lie, +#' the same position [lnk_stamp()] already takes for packages. +#' +#' Only then are the key fields compared, against row 1 (the dispatcher) as +#' reference. `link_sha` and `fresh_sha` are deliberately **not** keys — +#' see [lnk_preflight_stamp()] for why comparing them is either guaranteed +#' to fail or guaranteed to pass vacuously. +#' +#' @param stamps A data frame, one row per host, from +#' [lnk_preflight_stamp()]. +#' @param n_expected Number of hosts that were supposed to report. +#' Required. +#' @param keys Fields that must be identical across hosts. +#' @param forbid_na Fields that may not be the literal `"NA"` on any host. +#' @param forbid_dirty Fail when any host's checkout is dirty. +#' @param quiet Suppress the human-readable report. +#' +#' @return Invisibly, a list with `ok`, `n`, `mismatches`, `reference`, +#' `offenders`, `problems` and `message`. +#' +#' @family preflight +#' +#' @export +#' +#' @examples +#' row <- function(host, ...) { +#' d <- list(host = host, link_version = "0.46.0", link_sha = "NA", +#' fresh_version = "0.33.0", fresh_sha = "NA", +#' repo_sha = "abc123def456", repo_dirty = "FALSE", +#' config_hash = "cfg012345678", fwapg_sha = "e6e1eb0aaaaa", +#' r_version = "4.4.1") +#' as.data.frame(utils::modifyList(d, list(...)), stringsAsFactors = FALSE) +#' } +#' agree <- rbind(row("m1"), row("cy-job1")) +#' lnk_preflight_parity(agree, n_expected = 2, quiet = TRUE)$ok +#' +#' drift <- rbind(row("m1"), row("cy-job1", fresh_version = "0.31.0")) +#' lnk_preflight_parity(drift, n_expected = 2, quiet = TRUE)$offenders +lnk_preflight_parity <- function(stamps, + n_expected, + keys = c("link_version", "fresh_version", + "repo_sha", "config_hash", + "fwapg_sha"), + forbid_na = c("link_version", "fresh_version", + "repo_sha", "fwapg_sha"), + forbid_dirty = TRUE, + quiet = FALSE) { + stopifnot( + is.data.frame(stamps), + is.numeric(n_expected), length(n_expected) == 1L, + !is.na(n_expected), n_expected >= 1L, + is.character(keys), length(keys) >= 1L, + is.character(forbid_na), + is.logical(forbid_dirty), length(forbid_dirty) == 1L, !is.na(forbid_dirty), + is.logical(quiet), length(quiet) == 1L, !is.na(quiet)) + + need <- unique(c("host", keys, forbid_na, if (forbid_dirty) "repo_dirty")) + absent <- setdiff(need, names(stamps)) + if (length(absent)) { + stop("stamps is missing column(s): ", paste(absent, collapse = ", "), + call. = FALSE) + } + + problems <- character(0) + + # Checked first and unconditionally: a short or empty table is the one + # failure that every other check would otherwise silently agree with. + if (nrow(stamps) != n_expected) { + problems <- c(problems, sprintf( + "expected %d host stamp(s), got %d - a host did not report", + as.integer(n_expected), nrow(stamps))) + } + + # R's own NA counts as unresolved alongside the literal "NA" string. + # `lnk_preflight_stamp()` emits the string, but the shell round-trips the + # stamps through a TSV and `utils::read.delim()` defaults to + # `na.strings = "NA"` — which turns the sentinel back into a real NA that + # `%in% c("NA", "")` does not match and `!=` silently drops. The caller + # also passes `na.strings = character(0)`; this is the belt to that + # braces, so any future reader of a stamp table is judged correctly + # regardless of how it parsed the file. + is_unresolved <- function(x) is.na(x) | x %in% c("NA", "") + for (k in intersect(forbid_na, names(stamps))) { + bad <- stamps$host[is_unresolved(stamps[[k]])] + if (length(bad)) { + problems <- c(problems, sprintf("%s unresolved on: %s", k, + paste(bad, collapse = ", "))) + } + } + + if (forbid_dirty) { + bad <- stamps$host[tolower(stamps$repo_dirty) %in% c("true", "1", "yes", "t")] + if (length(bad)) { + problems <- c(problems, sprintf("dirty checkout on: %s", + paste(bad, collapse = ", "))) + } + } + + mismatches <- data.frame(host = character(0), field = character(0), + value = character(0), reference = character(0), + stringsAsFactors = FALSE) + ref <- if (nrow(stamps)) stamps[1L, , drop = FALSE] else NULL + if (!is.null(ref) && nrow(stamps) > 1L) { + for (k in keys) { + # `!=` yields NA for an NA operand and `which()` drops it, so a host + # whose field failed to parse would silently agree. Compare on a + # sentinel-normalised copy instead. + lhs <- ifelse(is.na(stamps[[k]]), "NA", stamps[[k]]) + rhs <- if (is.na(ref[[k]])) "NA" else ref[[k]] + off <- which(lhs != rhs) + if (length(off)) { + mismatches <- rbind(mismatches, data.frame( + host = stamps$host[off], field = k, value = lhs[off], + reference = rhs, stringsAsFactors = FALSE)) + } + } + if (nrow(mismatches)) { + problems <- c(problems, sprintf("%d field mismatch(es) vs %s", + nrow(mismatches), ref$host)) + } + } + + out <- list(ok = length(problems) == 0L, + n = nrow(stamps), + mismatches = mismatches, + reference = if (is.null(ref)) NA_character_ else ref$host, + offenders = sort(unique(mismatches$host)), + problems = problems, + message = .lnk_parity_message(stamps, problems, mismatches)) + if (!quiet) message(out$message) + invisible(out) +} + + +.lnk_parity_message <- function(stamps, problems, mismatches) { + if (length(problems) == 0L) { + return(sprintf("[preflight] host parity - OK across %d host(s): %s", + nrow(stamps), paste(stamps$host, collapse = ", "))) + } + lines <- c("[preflight] host parity - FAILED", + paste0(" ", problems)) + if (nrow(mismatches)) { + lines <- c(lines, sprintf(" %s: %s = %s (reference %s)", + mismatches$host, mismatches$field, + mismatches$value, mismatches$reference)) + } + paste(lines, collapse = "\n") +} diff --git a/R/lnk_preflight_stamp.R b/R/lnk_preflight_stamp.R new file mode 100644 index 00000000..fcccf971 --- /dev/null +++ b/R/lnk_preflight_stamp.R @@ -0,0 +1,93 @@ +#' One-line provenance stamp for cross-host pre-flight parity +#' +#' The parity payload in a fixed field order, drawn from the facts +#' [lnk_stamp()] already collects plus the git state of the checkout this +#' host installed from. Kept separate from [lnk_stamp()] so the shell +#' contract — the field order — is a documented, tested thing rather than +#' an inline `Rscript -e` incantation that drifts between two call sites. +#' +#' **`repo_sha` is the load-bearing field, not `link_sha`.** +#' `.lnk_pkg_git_sha()` resolves a SHA from a `.git` beside the installed +#' package. On the dispatcher link is `pkgload::load_all`'d from a checkout, +#' so it finds one; on every cypher link is pak-installed, so it does not +#' and returns `NA`. `fresh_sha` is `NA` on both unless pak recorded a +#' `RemoteSha`. Comparing `link_sha` across hosts would therefore always +#' fail, and comparing `fresh_sha` would be a vacuous `NA == NA` pass — a +#' check that looks like a check. `repo_sha` is read from +#' `~/Projects/repo/link` **on the host itself**, which on a cypher is +#' exactly what `git reset --hard origin/` produced. It is an +#' independent observation rather than a restatement of what the +#' dispatcher believes (link#246). +#' +#' Unresolvable facts are the literal string `"NA"`, never empty, so a +#' truncated ssh response is distinguishable from a resolved absence. +#' +#' @param cfg An `lnk_config` from [lnk_config()]. Supplies `config_hash`. +#' @param repo Path to the git checkout this host's install came from. +#' +#' @return A named character vector in the documented field order. +#' +#' @family preflight +#' +#' @export +#' +#' @examples +#' s <- lnk_preflight_stamp(lnk_config("bcfishpass")) +#' names(s) +#' s[["fresh_version"]] +lnk_preflight_stamp <- function(cfg = lnk_config("bcfishpass"), repo = ".") { + stopifnot(is.character(repo), length(repo) == 1L, nzchar(repo)) + s <- lnk_stamp(cfg, conn = NULL, db_snapshot = FALSE) + g <- .lnk_repo_git_state(repo) + + na <- function(x) { + if (length(x) != 1L || is.na(x)) "NA" else as.character(x) + } + sh <- function(x) substr(na(x), 1L, 12L) + + c(host = na(s$host), + link_version = na(s$software$link$version), + link_sha = sh(s$software$link$git_sha), + fresh_version = na(s$software$fresh$version), + fresh_sha = sh(s$software$fresh$git_sha), + repo_sha = sh(g$sha), + repo_dirty = na(g$dirty), + config_hash = sh(s$config_hash), + fwapg_sha = sh(s$fwapg_sha), + r_version = na(getRversion())) +} + +# The field order of a stamp line, as one definition. `lnk_preflight_stamp()` +# builds its output in this order and `study_area_run.sh`'s judge_stamps() +# calls this function for `col.names` — so there is no second list anywhere +# and the shell cannot drift from the R side. It briefly was duplicated as a +# `STAMP_COLS` string in the shell; dropping a field from the stamp then made +# read.delim silently left-shift the remaining columns, and no test written +# in R could have caught it, because both sides of the comparison were R. +.lnk_preflight_stamp_cols <- function() { + c("host", "link_version", "link_sha", "fresh_version", "fresh_sha", + "repo_sha", "repo_dirty", "config_hash", "fwapg_sha", "r_version") +} + +# git state of a working directory, as opposed to of an installed package. +# Returns NA for both fields when `repo` is not a git checkout, which the +# parity judge treats as a failure rather than as agreement. +.lnk_repo_git_state <- function(repo) { + run <- function(args) { + out <- tryCatch( + suppressWarnings(system2("git", c("-C", repo, args), + stdout = TRUE, stderr = FALSE)), + error = function(e) NULL) + if (is.null(out)) return(NULL) + st <- attr(out, "status") + if (!is.null(st) && !identical(as.integer(st), 0L)) return(NULL) + out + } + sha <- run(c("rev-parse", "HEAD")) + if (is.null(sha) || !length(sha) || !nzchar(sha[1])) { + return(list(sha = NA_character_, dirty = NA)) + } + porcelain <- run(c("status", "--porcelain")) + list(sha = sha[1], + dirty = if (is.null(porcelain)) NA else length(porcelain) > 0L) +} diff --git a/R/lnk_preflight_vintage.R b/R/lnk_preflight_vintage.R new file mode 100644 index 00000000..b0f4f1b1 --- /dev/null +++ b/R/lnk_preflight_vintage.R @@ -0,0 +1,141 @@ +#' Are this host's DB primitives fresh enough to model against? +#' +#' `snapshot_bcfp.sh` loads four primitives from public sources into each +#' host's local fwapg. Every cypher reloads them during prep; the +#' dispatcher does not, and nothing checks. On 2026-08-30 the dispatcher's +#' `cabd.dams` was 2026-05-23 and `fresh.modelled_stream_crossings` +#' 2026-05-26 — a run started that day would have modelled one bucket on +#' May inputs and three on August inputs, produced one consolidated table +#' set, and said nothing about it anywhere (link#246). +#' +#' The other seven tables in `.lnk_input_primitives()` are bulk-restored +#' FWA. They are never `ANALYZE`d, so they carry no vintage at all and are +#' not an axis this can measure — including them would mean every host +#' failing forever on data that is not the staleness risk. +#' +#' **Absence is not a pass.** A table missing from the result, and a table +#' present with a NULL timestamp, both fail — in the same direction as a +#' stale one. A query returning nothing must never read as "nothing is +#' stale". +#' +#' `last_analyze` alone is unusable here: measured across all ten +#' primitives it is NULL on every one, and only `last_autoanalyze` is +#' populated. `GREATEST` of the two is the usable signal — in Postgres it +#' ignores NULLs and is NULL only when every argument is. +#' +#' @param conn A [DBI::DBIConnection-class] to the host's local fwapg, or +#' `NULL` when `vintage` is supplied directly. +#' @param max_age_days Maximum acceptable age of the **oldest** primitive. +#' @param tables Fully-qualified table names to check. Defaults to the +#' snapshot-loaded set. +#' @param now Reference time. Injectable so tests are not clock-dependent. +#' @param vintage A data frame with `table_name` and `last_analyze`. +#' Defaults to reading `conn`; pass directly to test, or to judge a +#' stamp collected on another host. +#' @param quiet Suppress the human-readable report. +#' +#' @return Invisibly, a list with `ok`, `vintage`, `stale`, `missing`, +#' `oldest_days` and `message`. +#' +#' @family preflight +#' +#' @export +#' +#' @examples +#' # Judge a vintage table without touching a database: +#' now <- as.POSIXct("2026-08-30 12:00:00", tz = "UTC") +#' v <- data.frame( +#' table_name = link:::.lnk_vintage_primitives(), +#' last_analyze = now - c(1, 2, 1, 99) * 86400) +#' res <- lnk_preflight_vintage(vintage = v, now = now, max_age_days = 7, +#' quiet = TRUE) +#' res$ok +#' res$stale +lnk_preflight_vintage <- function(conn = NULL, + max_age_days = 7, + tables = .lnk_vintage_primitives(), + now = Sys.time(), + vintage = .lnk_vintage_read(conn, tables), + quiet = FALSE) { + stopifnot( + is.numeric(max_age_days), length(max_age_days) == 1L, + !is.na(max_age_days), max_age_days > 0, + is.character(tables), length(tables) >= 1L, all(nzchar(tables)), + inherits(now, "POSIXct"), length(now) == 1L, !is.na(now), + is.data.frame(vintage), + all(c("table_name", "last_analyze") %in% names(vintage)), + is.logical(quiet), length(quiet) == 1L, !is.na(quiet)) + + found <- vintage[vintage$table_name %in% tables, , drop = FALSE] + + # Both flavours of "no answer" converge here: a table absent from the + # result and a table present with no timestamp. + missing <- sort(union(setdiff(tables, found$table_name), + found$table_name[is.na(found$last_analyze)])) + + age <- as.numeric(difftime(now, found$last_analyze, units = "days")) + stale <- sort(found$table_name[!is.na(age) & age > max_age_days]) + oldest <- if (any(!is.na(age))) max(age, na.rm = TRUE) else NA_real_ + + out <- list(ok = length(missing) == 0L && length(stale) == 0L, + vintage = found, stale = stale, missing = missing, + oldest_days = oldest, + message = .lnk_vintage_message(found, stale, missing, + max_age_days, now)) + if (!quiet) message(out$message) + invisible(out) +} + + +# The snapshot-loaded primitives, derived from the existing dictionary so +# the set is single-sourced. Everything whose source is not fwapg — the FWA +# tables are bulk-restored and carry no analyze timestamp by construction. +.lnk_vintage_primitives <- function() { + p <- .lnk_input_primitives() + sort(p$table_name[!startsWith(p$source, "fwapg/")]) +} + +.lnk_vintage_read <- function(conn, tables) { + empty <- data.frame(table_name = character(0), + last_analyze = as.POSIXct(character(0)), + stringsAsFactors = FALSE) + if (is.null(conn)) return(empty) + + vals <- paste(sprintf("(%s)", vapply( + tables, function(t) as.character(DBI::dbQuoteLiteral(conn, t)), + character(1))), collapse = ", ") + + # LEFT JOIN so a table that does not exist yields a row with a NULL + # timestamp rather than no row — both are failures, and both should be + # reported by name rather than inferred from a short result. + DBI::dbGetQuery(conn, sprintf( + "SELECT p.tbl AS table_name, + GREATEST(s.last_analyze, s.last_autoanalyze) AS last_analyze + FROM (VALUES %s) AS p(tbl) + LEFT JOIN pg_class c ON c.oid = to_regclass(p.tbl) + LEFT JOIN pg_stat_user_tables s ON s.relid = c.oid", vals)) +} + +.lnk_vintage_message <- function(found, stale, missing, max_age_days, now) { + head <- sprintf("primitive vintage (window %g d)", max_age_days) + if (length(stale) == 0L && length(missing) == 0L) { + age <- as.numeric(difftime(now, found$last_analyze, units = "days")) + return(sprintf("[preflight] %s - OK, oldest %.1f d (%s)", + head, max(age), + found$table_name[which.max(age)])) + } + parts <- character(0) + if (length(stale)) { + age <- as.numeric(difftime(now, found$last_analyze, units = "days")) + names(age) <- found$table_name + parts <- c(parts, sprintf(" stale: %s", + paste(sprintf("%s (%.0f d)", stale, age[stale]), collapse = ", "))) + } + if (length(missing)) { + parts <- c(parts, sprintf(" never loaded / absent: %s", + paste(missing, collapse = ", "))) + } + parts <- c(parts, + " fix: bash data-raw/snapshot_bcfp.sh --with-bcfp-views --force") + paste(c(sprintf("[preflight] %s - FAILED", head), parts), collapse = "\n") +} diff --git a/RUNBOOK.md b/RUNBOOK.md index 64a1d831..80197401 100644 --- a/RUNBOOK.md +++ b/RUNBOOK.md @@ -612,6 +612,75 @@ WSG draining by two independent paths would be under-covered. --- +## 8d. Pre-flight gates on a multi-host run (link#246) + +A study-area run spends money and writes to a shared persist, so the gates in +`study_area_run.sh` sit in **two** blocks answering two different questions. + +**Why two, not one.** A cypher's software is *predictable* from the dispatcher +before the cypher exists: its link comes from `git reset --hard +origin/$LINK_BRANCH`, its fresh from link's `DESCRIPTION`. So `preflight_local()` +validates what the workers are *going to get* — free, before the spin — and +`preflight_hosts()` confirms after prep that they got it. Predict before spend; +verify before write. Framing it as one gate forces a false choice between +checking early and checking truthfully. + +`preflight_hosts()` genuinely cannot run earlier, and that is fine: it runs +before any `wsg_run_one.R` touches the persist, and a failure exits 1, which +trips the EXIT trap and burns the cyphers. The loss is bounded at spin + prep +rather than a whole run of two mixed model versions landing in one schema with +no `log` table to tell them apart. + +**No global bypass, on purpose.** An unconditional `--skip-preflight` is the +affordance that let this class of failure happen. `--preflight-note=""` +downgrades *only* vintage and parity, and only with a written justification — +the same position `lnk_wsg_downstream_check(override=)` takes. `--auto-install` +is remediation, not a skip: it re-runs the cyphers' install stage (which re-runs +the fresh assertion) and re-checks exactly once. + +### Three things that look like checks and are not + +| looks like | actually | +|---|---| +| `tofu plan` proves the DO token | against a zero-resource workspace it returns `Plan: N to add` **without contacting DO**. And `do_token` in tfvars is a *different* credential from doctl's — both were minted 2026-05-18 and both expired 2026-08-30. Probe each against `/v2/account` | +| comparing `link_sha` across hosts | it is a real SHA on the `load_all` dispatcher and `NA` on every pak-installed cypher, so it can only ever fail. `fresh_sha` is `NA` on both, so it can only ever pass. Key on **`repo_sha`**, read on each host from the checkout it installed from | +| `max(last_analyze)` for vintage | NULL on all ten primitives, measured. Empty in bash reads as "nothing to see". Use `GREATEST(last_analyze, last_autoanalyze)` | + +### The prep sentinel + +`cypher_prep.sh` ends with a bare `=== READY`, and the umbrella greps it +**anchored** (`grep -qx`). Do not revert this to `snapshot_bcfp.sh: complete`, +which was wrong in both directions: the snapshot emits it *before* +`lnk_persist_init` runs, so a persist_init FATAL passed the gate and WSGs ran +against a half-prepped cypher; and the snapshot's legitimate skip-if-current +path never emits it at all, so a skipped load read as FATAL. The `-x` anchor is +what stops `=== READY (install stage only; ...)` satisfying a full-prep check. + +### Two post-conditions, and why they are not optional + +`schema_consolidate` DELETEs the destination bucket +(`schema_consolidate.R:272-276`) and *then* COPYs (`:313-316`). A host that +produced nothing therefore does not merely fail to add rows — it **removes** the +rows already there for those WSGs and returns `ok = TRUE`. So: + +- before consolidate, every host must account for its whole bucket + (`[wsg_run_one] … done|SKIP` lines counted against the bucket size); +- after consolidate, every run WSG must have rows in `.streams`. + +The second is detection rather than prevention, and it is the one that would +have caught link#246 on day one regardless of cause. + +### Host buckets are derived, not chosen + +`data-raw/study_area_buckets.R` partitions the focal set into +drainage-independent components by union-find over per-WSG +`frs_wsg_drainage()` closures, then LPT-packs the components onto hosts and +writes `research/study_areas.md`. Overlapping closures would make consolidate +last-writer-wins on the shared WSGs, so the script **asserts** disjointness. +Never partition by `wscode_ltree` root — see §8b. + +--- + ## 8. Fast verification recipes ```bash diff --git a/data-raw/cypher_prep.sh b/data-raw/cypher_prep.sh index 2471e8ee..1c9529b0 100755 --- a/data-raw/cypher_prep.sh +++ b/data-raw/cypher_prep.sh @@ -12,37 +12,83 @@ # # What it does: # 1. git pull/reset the link branch to the orchestrator's current ref -# 2. pak::local_install link to pick up any package changes -# 3. snapshot_bcfp.sh — load PSCIS / CABD / modelled_crossings / bcfishobs +# 2. pak::local_install link to pick up any package changes. This now +# also installs `fresh` — see "the image is not load-bearing" below +# 3. record this host's OWN observed git state in ~/.Renviron, so the +# run log can attribute rows to a commit (link#246) +# 4. assert the installed fresh exports what the pipeline calls +# 5. snapshot_bcfp.sh — load PSCIS / CABD / modelled_crossings / bcfishobs # from public sources into the cypher's local fwapg -# 4. lnk_persist_init(force_recreate = TRUE) — DROPs any stale +# 6. lnk_persist_init(force_recreate = TRUE) — DROPs any stale # `fresh.streams` table whose DDL has unexpected GENERATED ALWAYS # columns (cypher snapshot artifact from when `frs_col_generate()` # was previously run on it; link#162 Phase 7 hardening detects this # mismatch and the force_recreate flag clears it) # +# The image is not load-bearing (link#246): +# `cypher-20260512-warm` bakes link 0.35.0 + fresh 0.31.0, and fresh +# 0.31.0 exports neither `frs_wsg_drainage` nor `frs_wsg_outlets`. +# link's wsg_run_one.R calls lnk_wsg_downstream_check(), whose `outlets` +# argument defaults to `fresh::frs_wsg_outlets()`; the resulting error is +# caught by a tryCatch that does quit(status = 1), and the umbrella's +# bucket loop logs `[WARN]` and moves on. Net effect: every WSG on the +# host fails, the host exits 0, and only the dispatcher's WSGs land. +# +# The fix is a declaration fix, not an install line. `fresh` moved from +# Suggests to Imports in link's DESCRIPTION, so `pak::local_install` +# below is now obliged to resolve it, and resolution consults +# `Remotes: NewGraphEnvironment/fresh@vX`. `upgrade = FALSE` suppresses +# gratuitous upgrades, not required ones. One pin, in one place, moved by +# the same PR that starts needing a newer fresh. +# # Pre-conditions on the cypher: # - Docker fresh-db running on localhost:5432 (Postgres + PostGIS) # - link cloned at ~/Projects/repo/link -# - homebrew bcdata, ogr2ogr, libpq psql, R + fresh + link installed -# (all baked into the cypher--warm snapshot) +# - homebrew bcdata, ogr2ogr, libpq psql, R installed +# (baked into the cypher--warm snapshot) +# - link and fresh are INSTALLED BY THIS SCRIPT. Whatever the image +# happens to carry is overwritten and must never be relied on. # # Branch override: set CYPHER_PREP_BRANCH (env var) to use a non-default # branch. Default is `main` — every host runs released code unless the # operator explicitly opts into testing a branch. # +# Stage: CYPHER_PREP_STAGE=all (default) | install +# `install` stops after the package install + assertion, skipping the +# snapshot and persist_init. That is the remediation path used by +# study_area_run.sh --auto-install, where a re-prep costs ~3 min rather +# than ~20 and the export assertion still gates it. +# +# fresh override: CYPHER_PREP_FRESH_REF installs a specific fresh ref on +# top of the DESCRIPTION pin — for testing an unreleased engine. The ref +# must still satisfy link's Imports floor, or pak will pull it straight +# back. That is deliberate: running an OLDER fresh requires relaxing the +# floor on your branch, where the change is reviewable. +# # Examples: # bash cypher_prep.sh # main # CYPHER_PREP_BRANCH=feat/foo bash cypher_prep.sh +# CYPHER_PREP_STAGE=install bash cypher_prep.sh +# CYPHER_PREP_FRESH_REF=NewGraphEnvironment/fresh@feat/bar bash cypher_prep.sh # # The override-aware default protects against the cognitive trap of # "which branch is everyone on?" — under the default path, all hosts -# converge on main and the orchestrator's preflight version-check -# (data-raw/dispatch_provincial.sh) confirms link versions match. +# converge on main and the orchestrator's preflight version check +# (data-raw/wsgs_dispatch.sh, and study_area_run.sh's preflight_hosts) +# confirms link and fresh agree across hosts. set -euo pipefail BRANCH="${CYPHER_PREP_BRANCH:-main}" +STAGE="${CYPHER_PREP_STAGE:-all}" +FRESH_REF="${CYPHER_PREP_FRESH_REF:-}" + +case "$STAGE" in + all|install) ;; + *) echo "FATAL: CYPHER_PREP_STAGE must be 'all' or 'install' (got '$STAGE')" >&2 + echo " a typo must not silently skip the snapshot" >&2 + exit 1 ;; +esac cd ~/Projects/repo/link git fetch origin @@ -63,8 +109,108 @@ if ! Rscript -e "pak::local_install(upgrade = FALSE, ask = FALSE)" > "$TMP_PAK_L fi tail -3 "$TMP_PAK_LOG" rm -f "$TMP_PAK_LOG" + +# Optional: an unreleased fresh on top of the DESCRIPTION pin. `upgrade = +# TRUE` here because the whole point is to move off whatever pak just +# resolved. See the header for why the Imports floor still applies. +if [ -n "$FRESH_REF" ]; then + echo "=== fresh override: $FRESH_REF" + TMP_FRS_PAK=$(mktemp) + if ! Rscript -e "pak::pkg_install(commandArgs(TRUE)[1], upgrade = TRUE, ask = FALSE)" \ + "$FRESH_REF" > "$TMP_FRS_PAK" 2>&1; then + echo "FATAL: pak::pkg_install('$FRESH_REF') failed; full log:" >&2 + cat "$TMP_FRS_PAK" >&2 + rm -f "$TMP_FRS_PAK" + exit 1 + fi + tail -3 "$TMP_FRS_PAK" + rm -f "$TMP_FRS_PAK" +fi + +# Record THIS host's own observed git state so fresh.log can attribute rows +# to a commit. .lnk_pkg_git_sha() (R/lnk_stamp.R) reads _GIT_SHA as its +# first tier and otherwise looks for a .git beside the installed package — +# which a pak install does not have, so every cypher row lands link_sha=NA +# today (link#246 Phase 5). +# +# Deliberately NOT inherited from the dispatcher over ssh. A SHA the +# dispatcher asserts is the dispatcher's claim; a SHA the cypher reads from +# the checkout it just reset is evidence, and it is what makes the +# cross-host parity gate a real comparison instead of a restatement. +LINK_SHA=$(git rev-parse HEAD) || { echo "FATAL: could not resolve link HEAD" >&2; exit 1; } +[ -n "$LINK_SHA" ] || { echo "FATAL: empty link HEAD sha" >&2; exit 1; } +if LINK_PORCELAIN=$(git status --porcelain); then + [ -z "$LINK_PORCELAIN" ] && LINK_DIRTY=false || LINK_DIRTY=true +else + echo "FATAL: could not read git status" >&2; exit 1 +fi +# A local/source install has no RemoteSha; empty is the honest answer and +# the parity gate reports it as unresolved rather than treating NA == NA as +# agreement. +FRESH_SHA=$(Rscript -e \ + 'x <- packageDescription("fresh")$RemoteSha; cat(if (is.null(x) || is.na(x)) "" else x)' \ + 2>/dev/null) || FRESH_SHA="" + +# Strip only the keys we own, then append — never rewrite wholesale, since +# the image may keep unrelated settings here. +RENV="$HOME/.Renviron" +touch "$RENV" +# `>` creates the temp at the ambient umask, so a 0600 ~/.Renviron would come +# back 0644 after the mv — and this stack keeps PG_PASSWORD_SHARE in that +# file. The intermediate is world-readable at a predictable path too, so the +# umask has to cover the redirect itself rather than being a chmod after it. +RENV_UMASK=$(umask) +umask 077 +# grep exits 1 for "no lines matched" and >=2 for a real read error. `|| true` +# would collapse those, and since `>` truncates the target before grep runs, +# an error would replace ~/.Renviron with the empty file — destroying exactly +# the unrelated settings this filter exists to preserve. Branch on the status. +# `|| RC=$?` is required, not decorative: under `set -e` a bare grep aborts +# the script the moment it selects no lines (exit 1), which would make the +# check below unreachable AND kill prep in the most ordinary case there is — +# an empty ~/.Renviron on a fresh droplet, or a re-prep of a host whose file +# holds only these three keys. +RC=0 +grep -vE '^(LINK_GIT_SHA|LINK_GIT_DIRTY|FRESH_GIT_SHA)=' "$RENV" > "$RENV.tmp" || RC=$? +if [ "$RC" -gt 1 ]; then + echo "FATAL: could not read $RENV (grep exit $RC); refusing to overwrite it" >&2 + rm -f "$RENV.tmp" + exit 1 +fi +mv "$RENV.tmp" "$RENV" +{ + printf 'LINK_GIT_SHA=%s\n' "$LINK_SHA" + printf 'LINK_GIT_DIRTY=%s\n' "$LINK_DIRTY" + [ -n "$FRESH_SHA" ] && printf 'FRESH_GIT_SHA=%s\n' "$FRESH_SHA" +} >> "$RENV" +chmod 600 "$RENV" +umask "$RENV_UMASK" +echo "=== provenance: link_sha=${LINK_SHA:0:12} dirty=$LINK_DIRTY fresh_sha=${FRESH_SHA:0:12}" + +# Assert the installed fresh actually exports what the pipeline calls. +# +# NOT `$(Rscript ...)`: command substitution discards the exit status under +# set -e, which is the bug the surrounding blocks were already rewritten to +# avoid. Same tempfile + `if !` idiom as the pak and snapshot blocks. +TMP_FRS_LOG=$(mktemp) +if ! Rscript -e 'q(status = if (isTRUE(link::lnk_preflight_fresh()$ok)) 0L else 1L)' \ + > "$TMP_FRS_LOG" 2>&1; then + echo "FATAL: fresh export assertion failed; full log:" >&2 + cat "$TMP_FRS_LOG" >&2 + rm -f "$TMP_FRS_LOG" + exit 1 +fi +cat "$TMP_FRS_LOG" +rm -f "$TMP_FRS_LOG" echo "=== link: $(Rscript -e "cat(as.character(packageVersion(\"link\")))") fresh: $(Rscript -e "cat(as.character(packageVersion(\"fresh\")))")" +if [ "$STAGE" = "install" ]; then + # Distinct sentinel. The umbrella's prep gate greps for an ANCHORED + # "=== READY", so this cannot be mistaken for a completed full prep. + echo "=== READY (install stage only; snapshot + persist_init NOT run)" + exit 0 +fi + cd data-raw export PGUSER=postgres PGPASSWORD=postgres PGHOST=localhost PGPORT=5432 PGDATABASE=fwapg diff --git a/data-raw/host_stamp.R b/data-raw/host_stamp.R new file mode 100644 index 00000000..0eb36eb8 --- /dev/null +++ b/data-raw/host_stamp.R @@ -0,0 +1,30 @@ +#!/usr/bin/env Rscript +# host_stamp.R — emit this host's parity stamp as one tab-separated line. +# +# Byte-identical on the dispatcher and every cypher; consumed by +# study_area_run.sh's preflight_hosts(), which rbinds the lines and hands +# them to lnk_preflight_parity() (link#246, absorbing #183). +# +# A driver script rather than an inline `Rscript -e` over ssh for the same +# reason wsg_run_one.R exists: the ssh leg's nested quoting is where these +# things rot, and the cyphers already carry data-raw/* from their +# `git reset --hard origin/$BRANCH`. +# +# `repo = getwd()` is the point of the whole exercise — the stamp reports +# the git state of the checkout THIS host installed from, observed here. +# See lnk_preflight_stamp() for why that is the honest key and link_sha is +# not. +# +# Usage: [LNK_LOAD=loadall] Rscript data-raw/host_stamp.R [config] + +args <- commandArgs(trailingOnly = TRUE) +config <- if (length(args) >= 1L && nzchar(args[1])) args[1] else "bcfishpass" + +if (identical(Sys.getenv("LNK_LOAD"), "loadall")) { + suppressPackageStartupMessages(pkgload::load_all(quiet = TRUE)) +} else { + suppressPackageStartupMessages(library(link)) +} + +cat(paste(lnk_preflight_stamp(lnk_config(config), repo = getwd()), + collapse = "\t"), "\n", sep = "") diff --git a/data-raw/host_vintage.R b/data-raw/host_vintage.R new file mode 100644 index 00000000..1c699436 --- /dev/null +++ b/data-raw/host_vintage.R @@ -0,0 +1,42 @@ +#!/usr/bin/env Rscript +# host_vintage.R — assert this host's snapshot primitives are recent enough. +# +# Byte-identical invocation on the dispatcher and on every cypher, the same +# host-agnostic contract as wsg_run_one.R. Exits 0 when every primitive is +# inside the window, 1 otherwise — including when a primitive has never been +# loaded, which is a failure and not a neutral (link#246). +# +# Connects to the LOCAL docker fwapg explicitly rather than through +# lnk_db_conn()'s env-var defaults, which resolve to the :63333 bcfp tunnel +# on some hosts — a different database with a different load state (#222). +# +# Usage: [LNK_LOAD=loadall] Rscript data-raw/host_vintage.R [max_age_days] +# LNK_LOAD=loadall -> pkgload::load_all() (dispatcher dev checkout) +# default -> library(link) (pak-installed, e.g. cyphers) + +args <- commandArgs(trailingOnly = TRUE) +max_days <- if (length(args) >= 1L && nzchar(args[1])) { + suppressWarnings(as.numeric(args[1])) +} else { + 7 +} +if (is.na(max_days) || max_days <= 0) { + stop("max_age_days must be a positive number (got '", + if (length(args) >= 1L) args[1] else "", "')", call. = FALSE) +} + +if (identical(Sys.getenv("LNK_LOAD"), "loadall")) { + suppressPackageStartupMessages(pkgload::load_all(quiet = TRUE)) +} else { + suppressPackageStartupMessages(library(link)) +} +suppressPackageStartupMessages({ + library(DBI); library(RPostgres) +}) + +conn <- lnk_db_conn(dbname = "fwapg", host = "localhost", port = 5432L, + user = "postgres", password = "postgres") +on.exit(try(DBI::dbDisconnect(conn), silent = TRUE), add = TRUE) + +res <- lnk_preflight_vintage(conn, max_age_days = max_days) +quit(status = if (isTRUE(res$ok)) 0L else 1L) diff --git a/data-raw/study_area_buckets.R b/data-raw/study_area_buckets.R new file mode 100644 index 00000000..514dd7f7 --- /dev/null +++ b/data-raw/study_area_buckets.R @@ -0,0 +1,384 @@ +#!/usr/bin/env Rscript +# study_area_buckets.R — partition the focal watershed groups into +# drainage-INDEPENDENT components, then pack those components onto hosts. +# +# Why components rather than study areas (link#246): +# +# Cross-WSG `;DAM` requires a host's bucket to be drainage-closed and run +# downstream-first. If two hosts' closures overlap, the overlapping WSGs +# are modelled twice and `schema_consolidate` resolves the collision +# last-writer-wins — so which host finished last silently decides the +# answer. Partitioning into components whose closures are disjoint makes +# that impossible by construction. +# +# Do NOT partition by `wscode_ltree` root. That reproduces the sliver +# misclassification RUNBOOK section 8b documents: NATR files under Fraser +# though it drains to the Peace, and SPAT under Skeena though it drains +# the Stikine. Closure is measure-aware; a wscode root is not. +# +# Method: union-find over per-WSG `fresh::frs_wsg_drainage()` closures. Two +# focal WSGs share a component iff their closures intersect. Each component +# is then resolved through `lnk_wsg_resolve()` for the species filter and +# the downstream-first order, and the components are packed onto hosts by +# greedy LPT (longest processing time first) — the same algorithm +# wsgs_dispatch.sh uses, with a component rather than a single WSG as the +# indivisible atom. +# +# Usage: +# [LNK_LOAD=loadall] Rscript data-raw/study_area_buckets.R \ +# [--hosts=4] [--config=bcfishpass] [--focal=A,B,C] [--write] +# +# --hosts=N number of hosts to pack onto (default 4: dispatcher + 3) +# --focal= override the focal set (default: the baked-in 96) +# --write rewrite research/study_areas.md from this run's output +# +# Stdout is the report. Nothing is written unless --write is passed. + +args <- commandArgs(trailingOnly = TRUE) +arg_val <- function(flag, default) { + hit <- grep(paste0("^", flag, "="), args, value = TRUE) + if (!length(hit)) return(default) + sub(paste0("^", flag, "="), "", hit[1]) +} +n_hosts <- as.integer(arg_val("--hosts", "4")) +config <- arg_val("--config", "bcfishpass") +do_write <- "--write" %in% args +if (is.na(n_hosts) || n_hosts < 1L) { + stop("--hosts must be a positive integer", call. = FALSE) +} + +if (identical(Sys.getenv("LNK_LOAD"), "loadall")) { + suppressPackageStartupMessages(pkgload::load_all(quiet = TRUE)) +} else { + suppressPackageStartupMessages(library(link)) +} +suppressPackageStartupMessages({ + library(DBI); library(RPostgres) +}) + +# The focal set, baked in rather than derived from whatever is currently +# persisted. A default that reads `fresh.streams` would give a different +# answer before and after the Phase 3 wipe, which is the opposite of what a +# reproducible derivation is for. These are the 93 groups persisted as of +# 2026-08-30 plus Columbia's KOTL / LARL / SLOC +# (rtj/scripts/gis/projects/nelson/project.yml). +FOCAL_DEFAULT <- c( + "ALBN", "BBAR", "BONP", "BRKS", "BULK", "CARP", "CARR", "CHES", "CHIR", + "CHWK", "CLRH", "COAL", "COTR", "COWN", "CRKD", "DOGC", "DUNC", "ELKR", + "FINA", "FINL", "FIRE", "FONT", "FOXR", "FRAN", "FRCN", "GATA", "GOLD", + "HARR", "HOMA", "INGR", "KETL", "KISP", "KITL", "KITR", "KLUM", "KOTR", + "KTSU", "LBTN", "LCHL", "LCHR", "LDEN", "LFRA", "LILL", "LKEL", "LNTH", + "LOMI", "LPCE", "LPRO", "LSAL", "LSKE", "LSTR", "MDEA", "MESI", "MFRA", + "MORK", "MORR", "MSKE", "MSTR", "NARC", "NASR", "NATR", "NECR", "NICL", + "OSPK", "PARA", "PARS", "PCEA", "QUES", "SAJR", "SALR", "SETN", "SHER", + "SMAR", "SPAT", "SUST", "TABR", "TAKL", "TATR", "TOOD", "TSIT", "TWAC", + "UBTN", "UFRA", "UJER", "UKEC", "UNRS", "UNUR", "UOMI", "UPCE", "USKE", + "UTRE", "WILL", "ZYMO", + # Columbia + "KOTL", "LARL", "SLOC") + +focal_arg <- arg_val("--focal", "") +focal <- if (nzchar(focal_arg)) { + toupper(trimws(strsplit(focal_arg, ",")[[1]])) +} else { + FOCAL_DEFAULT +} +focal <- sort(unique(focal[nzchar(focal)])) + +# Explicit local docker fwapg — lnk_db_conn()'s env defaults land on the +# :63333 bcfp tunnel, a different database with a different load state +# (#222). Same reasoning as study_area_wsgs.R. +conn <- DBI::dbConnect(RPostgres::Postgres(), host = "localhost", port = 5432, + dbname = "fwapg", user = "postgres", password = "postgres") +on.exit(try(DBI::dbDisconnect(conn), silent = TRUE), add = TRUE) + +cfg <- lnk_config(config) +loaded <- lnk_load_overrides(cfg) + +message(sprintf("focal: %d WSGs | config: %s | hosts: %d", + length(focal), config, n_hosts)) + +# --- 1. per-focal drainage closures --------------------------------------- +message("resolving per-WSG drainage closures ...") +closures <- stats::setNames( + lapply(focal, function(w) fresh::frs_wsg_drainage(conn, w)), focal) + +bad <- names(closures)[vapply(closures, length, integer(1)) == 0L] +if (length(bad)) { + stop("frs_wsg_drainage returned nothing for: ", paste(bad, collapse = ", "), + " - an empty closure is a failure, not an isolated component", + call. = FALSE) +} + +# --- 2. union-find --------------------------------------------------------- +parent <- stats::setNames(seq_along(focal), focal) +find <- function(i) { + while (parent[[i]] != i) { + parent[[i]] <<- parent[[parent[[i]]]] + i <- parent[[i]] + } + i +} +union2 <- function(a, b) { + ra <- find(a); rb <- find(b) + if (ra != rb) parent[[ra]] <<- rb + invisible(NULL) +} + +# Index every closure member back to the focal WSGs that reach it; any two +# focal WSGs sharing a member belong to one component. +seen <- new.env(parent = emptyenv()) +for (i in seq_along(focal)) { + for (m in closures[[i]]) { + prev <- seen[[m]] + if (is.null(prev)) seen[[m]] <- i else union2(i, prev) + } +} + +comp_id <- vapply(seq_along(focal), find, integer(1)) +components <- split(focal, comp_id) +names(components) <- NULL +# Largest first, ties broken alphabetically so the output is deterministic. +components <- components[order(-lengths(components), + vapply(components, `[`, character(1), 1))] + +message(sprintf("union-find: %d focal WSGs -> %d drainage-independent components", + length(focal), length(components))) + +# --- 3. resolve each component: closure + species filter + DS-first -------- +message("resolving modelable DS-first sets per component ...") +resolved <- lapply(components, function(f) { + suppressMessages(lnk_wsg_resolve(cfg, loaded, wsgs = f, conn = conn)) +}) + +# Disjointness is the whole point of the partition, so assert it rather than +# trusting the algorithm. An overlap means consolidate would be +# last-writer-wins on the shared WSGs. +flat <- unlist(resolved) +dup <- unique(flat[duplicated(flat)]) +if (length(dup)) { + stop("components overlap on: ", paste(dup, collapse = ", "), + " - the partition is not drainage-independent", call. = FALSE) +} + +# --- 4. weights ------------------------------------------------------------ +# Stream-segment count per WSG is a far better proxy for modelling work than +# a WSG count, and it is one query. Groups absent from the network table get +# the median rather than zero, so an unknown never packs as free. +w <- DBI::dbGetQuery(conn, + "SELECT watershed_group_code AS wsg, count(*)::numeric AS n + FROM whse_basemapping.fwa_stream_networks_sp GROUP BY 1") +wmap <- stats::setNames(w$n, w$wsg) +med <- stats::median(w$n) +weight_of <- function(wsgs) { + v <- wmap[wsgs] + v[is.na(v)] <- med + sum(v) +} +comp_weight <- vapply(resolved, weight_of, numeric(1)) + +# --- 5. greedy LPT pack ---------------------------------------------------- +ord <- order(-comp_weight) +load_h <- rep(0, n_hosts) +assign_h <- integer(length(resolved)) +for (i in ord) { + pick <- which.min(load_h) + assign_h[i] <- pick + load_h[pick] <- load_h[pick] + comp_weight[i] +} + +# Host 1 is the dispatcher: free and fast, so it should carry the most. +# Reorder host labels by descending load so that is true by construction. +host_order <- order(-load_h) +relabel <- stats::setNames(seq_along(host_order), host_order) +assign_h <- as.integer(relabel[as.character(assign_h)]) +load_h <- load_h[host_order] + +host_focal <- lapply(seq_len(n_hosts), function(h) { + unlist(components[assign_h == h], use.names = FALSE) +}) +host_wsgs <- lapply(seq_len(n_hosts), function(h) { + unlist(resolved[assign_h == h], use.names = FALSE) +}) + +# --- assert the property the whole partition exists to provide ------------- +# Each host's bucket must be drainage-CLOSED: for every WSG it models, every +# modelable WSG downstream of that one must also be on the same host. That is +# what lets accessibility read already-persisted downstream barriers, and it +# is the precondition lnk_wsg_downstream_check() enforces per WSG at run time. +# +# It does follow from packing whole components whose closures are disjoint — +# but "follows by construction" is a claim, and a cheap one to check against +# the closures already in hand. Asserted rather than reasoned about. +for (h in seq_len(n_hosts)) { + if (!length(host_focal[[h]])) next + need <- intersect(unique(unlist(closures[host_focal[[h]]])), flat) + miss <- setdiff(need, host_wsgs[[h]]) + if (length(miss)) { + stop(sprintf( + "host %d bucket is not drainage-closed - missing %s. A WSG would be modelled before its downstream barriers exist.", + h, paste(sort(miss), collapse = ", ")), call. = FALSE) + } +} + +# And the DS-first order has to hold WITHIN each host: a WSG must appear +# after everything downstream of it that the same host models. Concatenating +# several components is safe precisely because they are drainage-independent, +# so no flow path crosses a component boundary — but that is the claim, so +# check it against real closures rather than restating it. +# +# `closures` is keyed by FOCAL WSG only; a bucket also holds closure members +# that were never focal, so their closures are resolved here. +message("verifying DS-first order within each host ...") +closure_of <- new.env(parent = emptyenv()) +for (nm in names(closures)) assign(nm, closures[[nm]], envir = closure_of) +for (x in flat) { + if (!exists(x, envir = closure_of, inherits = FALSE)) { + assign(x, fresh::frs_wsg_drainage(conn, x), envir = closure_of) + } +} +for (h in seq_len(n_hosts)) { + b <- host_wsgs[[h]] + if (length(b) < 2L) next + pos <- stats::setNames(seq_along(b), b) + for (x in b) { + ds <- intersect(setdiff(get(x, envir = closure_of), x), b) + late <- ds[pos[ds] > pos[[x]]] + if (length(late)) { + stop(sprintf("host %d: %s is ordered before its downstream %s", h, x, + paste(late, collapse = ", ")), call. = FALSE) + } + } +} + +# --- 6. report ------------------------------------------------------------- +host_label <- function(h) if (h == 1L) "dispatcher (m1)" else sprintf("job%d", h - 1L) + +cat("\n## Components\n\n") +cat(sprintf("%d focal WSGs -> %d drainage-independent components\n\n", + length(focal), length(components))) +cat("| # | focal | modelable | host | focal WSGs |\n") +cat("|---|---|---|---|---|\n") +for (i in seq_along(components)) { + cat(sprintf("| %d | %d | %d | %s | %s |\n", i, + length(components[[i]]), length(resolved[[i]]), + host_label(assign_h[i]), + paste(components[[i]], collapse = " "))) +} + +cat("\n## Host buckets\n\n") +cat("| host | components | focal | modelable | weight (segments) |\n") +cat("|---|---|---|---|---|\n") +for (h in seq_len(n_hosts)) { + cat(sprintf("| %s | %d | %d | %d | %s |\n", host_label(h), + sum(assign_h == h), length(host_focal[[h]]), + length(host_wsgs[[h]]), format(load_h[h], big.mark = ","))) +} +cat(sprintf("\ntotal modelable: %d | dropped by species presence: %d\n", + length(flat), + length(unique(unlist(closures))) - length(flat))) + +cat("\n## --focal= strings\n\n") +cat("```\n") +for (h in seq_len(n_hosts)) { + cat(sprintf(" --focal=%s \\\n", paste(sort(host_focal[[h]]), collapse = ","))) +} +cat("```\n") + +cat("\n## DS-first order per host\n\n") +for (h in seq_len(n_hosts)) { + cat(sprintf("- **%s** (%d): %s\n", host_label(h), length(host_wsgs[[h]]), + paste(host_wsgs[[h]], collapse = ", "))) +} + +# --- 7. optionally rewrite the research doc -------------------------------- +if (do_write) { + out <- file.path(dirname(dirname(normalizePath( + sub("^--file=", "", grep("^--file=", commandArgs(FALSE), value = TRUE)[1]), + mustWork = FALSE))), "research", "study_areas.md") + if (!dir.exists(dirname(out))) { + stop("cannot locate research/ from the script path", call. = FALSE) + } + con <- file(out, open = "wt") + on.exit(close(con), add = TRUE) + wr <- function(...) cat(..., "\n", sep = "", file = con) + + wr("# Study areas — drainage-independent components and host buckets") + wr("") + wr("") + wr("") + wr(sprintf("Generated from %d focal watershed groups, config `%s`, %d hosts.", + length(focal), config, n_hosts)) + wr("") + wr("## Why components, not study areas") + wr("") + wr("Cross-WSG `;DAM` needs each host's bucket drainage-closed and run") + wr("downstream-first. Where two hosts' closures overlap, the shared WSGs are") + wr("modelled twice and `schema_consolidate` resolves the collision") + wr("last-writer-wins — so whichever host finished last silently decides the") + wr("answer. Partitioning into components whose closures are disjoint removes") + wr("that ambiguity by construction, and the derivation asserts disjointness") + wr("rather than assuming it.") + wr("") + wr("Partitioning by `wscode_ltree` root does **not** work: it reproduces the") + wr("sliver misclassification RUNBOOK section 8b documents — NATR filed under") + wr("Fraser though it drains to the Peace, SPAT under Skeena though it drains") + wr("the Stikine. Closure is measure-aware; a wscode root is not.") + wr("") + wr("## Host buckets") + wr("") + wr("| host | components | focal | modelable | weight (segments) |") + wr("|---|---|---|---|---|") + for (h in seq_len(n_hosts)) { + wr(sprintf("| %s | %d | %d | %d | %s |", host_label(h), + sum(assign_h == h), length(host_focal[[h]]), + length(host_wsgs[[h]]), format(load_h[h], big.mark = ","))) + } + wr("") + wr(sprintf("%d focal WSGs resolve to %d components and %d modelable WSGs.", + length(focal), length(components), length(flat))) + wr("") + wr("Components are indivisible, so the packing is a greedy LPT over") + wr("component weights, not over WSGs. Weight is stream-segment count from") + wr("`fwa_stream_networks_sp`, which tracks modelling work far better than a") + wr("WSG count does — a one-WSG component can outweigh a three-WSG one. The") + wr("dispatcher is relabelled to whichever host draws the heaviest load,") + wr("since it is the free, fast local machine while the cyphers are paid.") + wr("") + wr("The component decomposition is a property of the drainage network and is") + wr("stable; the host assignment is only as stable as the weights, so expect") + wr("it to shift if the network table is reloaded.") + wr("") + wr("## `--focal=` strings") + wr("") + wr("```") + wr("bash data-raw/study_area_run.sh \\") + wr(sprintf(" --cy-workspaces=%s \\", + paste(sprintf("job%d", seq_len(n_hosts - 1L)), collapse = ","))) + for (h in seq_len(n_hosts)) { + wr(sprintf(" --focal=%s \\", paste(sort(host_focal[[h]]), collapse = ","))) + } + wr(" --config=bcfishpass") + wr("```") + wr("") + wr("## Downstream-first order per host") + wr("") + for (h in seq_len(n_hosts)) { + wr(sprintf("**%s** (%d WSGs)", host_label(h), length(host_wsgs[[h]]))) + wr("") + wr(sprintf(" %s", paste(host_wsgs[[h]], collapse = ", "))) + wr("") + } + wr("## Components") + wr("") + wr("| # | focal | modelable | host | focal WSGs |") + wr("|---|---|---|---|---|") + for (i in seq_along(components)) { + wr(sprintf("| %d | %d | %d | %s | %s |", i, + length(components[[i]]), length(resolved[[i]]), + host_label(assign_h[i]), + paste(components[[i]], collapse = " "))) + } + message("wrote ", out) +} diff --git a/data-raw/study_area_run.sh b/data-raw/study_area_run.sh index 8502a902..4f3d5ed6 100755 --- a/data-raw/study_area_run.sh +++ b/data-raw/study_area_run.sh @@ -26,12 +26,40 @@ # --focal= \ # [--config=bcfishpass] [--schema=] [--keep-cyphers] # +# bash data-raw/study_area_run.sh --preflight-only # gates only, no spend +# # The number of --focal flags MUST equal 1 (dispatcher) + N cyphers, in # order: first --focal -> dispatcher, the rest -> cyphers in --cy-workspaces # order. Put the LARGEST area on the dispatcher (first --focal): it is the # fast, free local host, while cyphers are slower + paid — give them the # smaller areas so they finish + burn sooner. Cyphers burn right after # consolidate (minimise idle); a trap EXIT is the safety net. +# +# Pre-flight gates (link#246) — two blocks, because they answer two +# different questions: +# +# preflight_local() before the spin. Everything knowable without a +# droplet: fwapg up, bcfp view present, dispatcher's +# fresh complete, branch pushed + tree clean, fwapg +# SHA resolvable, primitive vintage, and BOTH DO +# credentials forced through a real API call. +# preflight_hosts() after prep, before any WSG writes: cross-host +# version parity and cypher primitive vintage. Cannot +# run earlier — the hosts do not exist yet. A failure +# burns via the EXIT trap. +# +# Post-conditions: every host must account for its whole bucket before +# consolidate, and every run WSG must have rows in the persist afterwards. +# +# Extra flags: +# --preflight-only run the local gates and exit (no spend) +# --refresh-primitives snapshot_bcfp.sh --force first (default off) +# --auto-install on a parity mismatch, re-run the cyphers' +# install stage and re-check once +# --vintage-max-days=N primitive staleness window (default 7) +# --preflight-note="why" downgrade ONLY vintage + parity to warnings, +# and only with a written reason. There is no +# global bypass on purpose. set -euo pipefail @@ -40,6 +68,11 @@ CY_WS="" CONFIG="bcfishpass" SCHEMA_OVERRIDE="" KEEP_CYPHERS=0 +PREFLIGHT_ONLY=0 +REFRESH_PRIMITIVES=0 +AUTO_INSTALL=0 +VINTAGE_MAX_DAYS=7 +PREFLIGHT_NOTE="" FOCAL_ARR=() for arg in "$@"; do case "$arg" in @@ -49,17 +82,58 @@ for arg in "$@"; do [ -n "$SCHEMA_OVERRIDE" ] || { echo "FATAL: --schema= requires a non-empty value" >&2; exit 1; } ;; --focal=*) FOCAL_ARR+=("${arg#--focal=}") ;; --keep-cyphers) KEEP_CYPHERS=1 ;; + --preflight-only) PREFLIGHT_ONLY=1 ;; + --refresh-primitives) REFRESH_PRIMITIVES=1 ;; + --auto-install) AUTO_INSTALL=1 ;; + --vintage-max-days=*) + VINTAGE_MAX_DAYS="${arg#--vintage-max-days=}" + case "$VINTAGE_MAX_DAYS" in + ''|*[!0-9]*) echo "FATAL: --vintage-max-days= needs a positive integer (got '$VINTAGE_MAX_DAYS')" >&2; exit 1 ;; + esac + [ "$VINTAGE_MAX_DAYS" -gt 0 ] || { echo "FATAL: --vintage-max-days must be > 0" >&2; exit 1; } ;; + # Downgrades ONLY the vintage and parity gates to warnings, and only + # with a written reason. Mirrors lnk_wsg_downstream_check(override=): + # the justification IS the mechanism, so a bare boolean is rejected. + # There is deliberately no global --skip-preflight — an unconditional + # bypass is the affordance that let #246 happen in the first place. + --preflight-note=*) + PREFLIGHT_NOTE="${arg#--preflight-note=}" + [ -n "$PREFLIGHT_NOTE" ] || { echo "FATAL: --preflight-note= requires a written justification, not an empty value" >&2; exit 1; } ;; *) echo "unknown arg: $arg" >&2; exit 1 ;; esac done -IFS=',' read -ra CY_WS_ARR <<< "$CY_WS" -[ -n "$CY_WS" ] || CY_WS_ARR=() -N_CY=${#CY_WS_ARR[@]} -N_FOCAL=${#FOCAL_ARR[@]} -EXPECT=$((N_CY + 1)) -if [ "$N_FOCAL" -ne "$EXPECT" ]; then - echo "FATAL: need exactly $EXPECT --focal flags (1 dispatcher + $N_CY cyphers); got $N_FOCAL" >&2 +# --preflight-only needs no --focal set: it exits before the buckets are +# resolved, and demanding them would make the cheap dry check as awkward as +# the real thing. +if [ "$PREFLIGHT_ONLY" = "0" ]; then + IFS=',' read -ra CY_WS_ARR <<< "$CY_WS" + [ -n "$CY_WS" ] || CY_WS_ARR=() + N_CY=${#CY_WS_ARR[@]} + N_FOCAL=${#FOCAL_ARR[@]} + EXPECT=$((N_CY + 1)) + if [ "$N_FOCAL" -ne "$EXPECT" ]; then + echo "FATAL: need exactly $EXPECT --focal flags (1 dispatcher + $N_CY cyphers); got $N_FOCAL" >&2 + exit 1 + fi +else + IFS=',' read -ra CY_WS_ARR <<< "$CY_WS" + [ -n "$CY_WS" ] || CY_WS_ARR=() + N_CY=${#CY_WS_ARR[@]} +fi + +# --- persist-schema guard (link#246) --- +# Both `bcfishpass` and `default` resolve $pipeline$schema to `fresh` +# (measured 2026-08-30), so `--config=default` with no --schema= persists a +# DIFFERENT bundle's output into the SAME tables as the bcfishpass run and +# overwrites it in place. There is no recovery and no signal: the tables +# look fine, they are just a mixture of two methodologies. +if [ "$CONFIG" != "bcfishpass" ] && [ -z "$SCHEMA_OVERRIDE" ]; then + echo "FATAL: --config=$CONFIG requires an explicit --schema=." >&2 + echo " '$CONFIG' resolves pipeline\$schema to the same target as" >&2 + echo " --config=bcfishpass, so running it bare would overwrite the" >&2 + echo " bcfishpass persist in place." >&2 + echo " e.g. --config=$CONFIG --schema=fresh_${CONFIG}" >&2 exit 1 fi @@ -80,11 +154,24 @@ LINK_BRANCH="$(git -C "$REPO_ROOT" branch --show-current)" # (e.g. for side-by-side bundle compares: --config=default --schema=fresh_default # keeps the bcfp-config run intact in `fresh`). All R scripts read LNK_SCHEMA # below and override `cfg$pipeline$schema` if it is non-empty. +resolve_schema() { # $1 = config name; prints the schema, non-zero on R failure + (cd "$REPO_ROOT" && Rscript -e \ + 'cat(link::lnk_config(commandArgs(TRUE)[1])$pipeline$schema)' "$1") +} + if [ -n "$SCHEMA_OVERRIDE" ]; then SCHEMA="$SCHEMA_OVERRIDE" else - SCHEMA=$(cd "$REPO_ROOT" && Rscript -e \ - 'cat(link::lnk_config(commandArgs(TRUE)[1])$pipeline$schema)' "$CONFIG" 2>/dev/null || true) + # `2>/dev/null || true` is correct here and only looks like the bug shape: + # an R failure yields an empty $SCHEMA, which the FATAL below catches. + SCHEMA=$(resolve_schema "$CONFIG" 2>/dev/null || true) + # There was a "second layer" collision check here comparing $SCHEMA to + # bcfishpass's. It was unreachable and has been removed rather than left + # as decoration: this branch runs only when --schema= is absent, and the + # guard above already exited for every non-bcfishpass config in that case, + # so its `[ "$CONFIG" != "bcfishpass" ]` test was always false. A guard + # that cannot go red is worse than none — it reads as coverage. + # The name-based guard above is the real one and covers every config. fi [ -n "$SCHEMA" ] || { echo "FATAL: could not resolve persist schema for --config=$CONFIG"; exit 1; } export LNK_SCHEMA="$SCHEMA" @@ -95,6 +182,9 @@ echo " branch: $LINK_BRANCH (cyphers run this ref)" echo " persist: $SCHEMA" echo " cyphers: ${CY_WS_ARR[*]:-} ($N_CY)" echo " log dir: $LOG_DIR" +echo " vintage max: ${VINTAGE_MAX_DAYS} d" +[ "$PREFLIGHT_ONLY" = "0" ] || echo " MODE: --preflight-only (no spend, no writes)" +[ -z "$PREFLIGHT_NOTE" ] || echo " OVERRIDE: vintage+parity downgraded to warnings — $PREFLIGHT_NOTE" # --- trap: burn cyphers on exit (safety net; explicit burn after consolidate) --- CYPHERS_UP=0 @@ -119,32 +209,372 @@ burn_cyphers() { n=$(cd "$CYPHER_TF" && TF_WORKSPACE="$WS" tofu state list 2>/dev/null | wc -l | tr -d ' ') || n="?" echo " cy[$WS]: $n tofu resources (expect 0)"; [ "$n" = "0" ] || clean=0 done - if doctl compute droplet list --no-header 2>/dev/null | grep -qi cypher; then - echo " ✗ doctl still shows cypher droplets"; clean=0 - else echo " ✓ doctl: no cypher droplets"; fi + # Three outcomes, not two. The old form piped doctl into grep, so a doctl + # failure produced no output, grep found no match, and a leaked droplet + # billing indefinitely was reported as "✓ no cypher droplets". Capture + # first, test the exit status, then test the value (link#246). + local dl + if dl=$(doctl compute droplet list --no-header 2>/dev/null); then + if printf '%s' "$dl" | grep -qi cypher; then + echo " ✗ doctl still shows cypher droplets"; clean=0 + else + echo " ✓ doctl: no cypher droplets" + fi + else + echo " ✗ could not query doctl — droplet state UNKNOWN, check manually"; clean=0 + fi [ "$clean" = "1" ] && echo " ✓ burn clean" || echo " ✗ BURN INCOMPLETE — investigate" CYPHERS_UP=0 return $rc } trap burn_cyphers EXIT -# --- pre-flight (tunnel-free) --- -echo "=== pre-flight ===" -fail=0 -pg_isready -h localhost -p 5432 >/dev/null 2>&1 || { echo " ✗ local fwapg down (:5432)"; fail=1; } -# bcfp reference view is a constant (fresh.streams_vw_bcfp) — it lives in -# its own schema independent of $SCHEMA (the persist target). All compare -# code paths (R/lnk_compare_mapping_code.R:78 default) read it from `fresh`. -HAS_VW=$(PGPASSWORD=postgres psql -h localhost -p 5432 -U postgres -d fwapg -t -A -c \ - "SELECT 1 FROM information_schema.tables WHERE table_schema='fresh' AND table_name='streams_vw_bcfp'" 2>/dev/null || true) -[ "$HAS_VW" = "1" ] || { echo " ✗ fresh.streams_vw_bcfp missing (run snapshot_bcfp.sh --with-bcfp-views)"; fail=1; } -if [ "$N_CY" -gt 0 ]; then - doctl compute droplet list --no-header >/dev/null 2>&1 || { echo " ✗ doctl not authed"; fail=1; } - (cd "$CYPHER_TF" && tofu workspace list >/dev/null 2>&1) || { echo " ✗ tofu workspace list failed"; fail=1; } +# --- pre-flight, local: everything answerable before a droplet exists ------ +# +# A cypher's software is PREDICTABLE from here: its link comes from +# `git reset --hard origin/$LINK_BRANCH`, its fresh from link's DESCRIPTION. +# So the pre-spin gate validates what the cyphers are GOING to get, and +# preflight_hosts() below confirms they actually got it. Predict before +# spend; verify before write (link#246). +# +# Accumulate into `fail` rather than exiting early, so one run reports every +# problem — an operator fixing an expired token should not then discover the +# branch is unpushed on the next attempt. +preflight_local() { + echo "=== pre-flight (local; pre-spend) ===" + local fail=0 + + pg_isready -h localhost -p 5432 >/dev/null 2>&1 \ + || { echo " ✗ local fwapg down (:5432)"; fail=1; } + + # bcfp reference view is a constant (fresh.streams_vw_bcfp) — it lives in + # its own schema independent of $SCHEMA (the persist target). All compare + # code paths (R/lnk_compare_mapping_code.R:78 default) read it from `fresh`. + local has_vw + has_vw=$(PGPASSWORD=postgres psql -h localhost -p 5432 -U postgres -d fwapg -t -A -c \ + "SELECT 1 FROM information_schema.tables WHERE table_schema='fresh' AND table_name='streams_vw_bcfp'" 2>/dev/null || true) + [ "$has_vw" = "1" ] \ + || { echo " ✗ fresh.streams_vw_bcfp missing (run snapshot_bcfp.sh --with-bcfp-views)"; fail=1; } + + # --- gate: dispatcher's fresh provides what the pipeline calls ---------- + # The same assertion cypher_prep runs on each worker, run here too so the + # dispatcher cannot be the odd one out. + # + # The expression loads the package itself — LNK_LOAD is only read by the + # driver scripts, so an `-e` relying on it finds no such function, exits + # non-zero, and reports "missing symbols" for a broken invocation rather + # than a broken fresh. Exit 2 separates those two states, because sending + # someone to debug fresh when the harness is what failed is its own bug. + local fresh_out fresh_rc + fresh_out=$(cd "$REPO_ROOT" && Rscript -e ' +suppressPackageStartupMessages(pkgload::load_all(quiet = TRUE)) +res <- lnk_preflight_fresh(quiet = TRUE) +cat(res$message, "\n", sep = "") +quit(status = if (isTRUE(res$ok)) 0L else 1L) +' 2>&1) && fresh_rc=0 || fresh_rc=$? + case "$fresh_rc" in + 0) echo " ✓ dispatcher fresh exports what the pipeline calls" ;; + 1) printf '%s\n' "$fresh_out" | sed 's/^/ /' + echo " ✗ dispatcher fresh is missing required symbols"; fail=1 ;; + *) printf '%s\n' "$fresh_out" | sed 's/^/ /' + echo " ✗ could not run the fresh check (R error, not a fresh problem)"; fail=1 ;; + esac + + # --- gate: branch pushed + worktree clean (link#246) -------------------- + # Cyphers do `git fetch origin && git reset --hard origin/$BRANCH` + # (cypher_prep.sh). Anything unpushed does not exist on them, and the host + # silently runs older driver scripts against a newer dispatcher. + if [ "$N_CY" -gt 0 ]; then + # Fetch FIRST: @{upstream} is a LOCAL ref, so without this the check + # compares against a stale copy and is a false green. And a FAILED fetch + # leaves exactly that stale ref, so it cannot be waved through with + # `|| true` — that would print "origin/$BRANCH is at HEAD" on the + # strength of a comparison against a ref that was never updated, and the + # run would spin droplets before cypher_prep's own + # `git reset --hard origin/$BRANCH` discovered the problem. + if ! git -C "$REPO_ROOT" fetch --quiet origin "$LINK_BRANCH" 2>/dev/null; then + echo " ✗ could not fetch origin/$LINK_BRANCH — cannot verify the cyphers' ref" + fail=1 + elif ! git -C "$REPO_ROOT" rev-parse --abbrev-ref '@{upstream}' >/dev/null 2>&1; then + echo " ✗ branch '$LINK_BRANCH' has no upstream — git push -u origin $LINK_BRANCH" + fail=1 + else + # Assign, test the EXIT STATUS, then test the value. A rev-list error + # prints nothing, and empty must never read as "0 commits ahead". + local ahead + if ahead=$(git -C "$REPO_ROOT" rev-list --count '@{upstream}..HEAD' 2>/dev/null); then + if [ "$ahead" != "0" ]; then + echo " ✗ $ahead local commit(s) not on origin/$LINK_BRANCH — cyphers would run older code" + fail=1 + else + echo " ✓ origin/$LINK_BRANCH is at HEAD" + fi + else + echo " ✗ could not compare HEAD to @{upstream}" + fail=1 + fi + fi + fi + + # Uncommitted work is unpushable by definition, so for a multi-host run it + # is the same failure. With no cyphers there is no drift axis, only a + # provenance-honesty concern, so it warns rather than blocks local dev. + local dirty + if dirty=$(git -C "$REPO_ROOT" status --porcelain 2>/dev/null); then + if [ -n "$dirty" ]; then + if [ "$N_CY" -gt 0 ]; then + echo " ✗ dispatcher checkout dirty; cyphers reset to origin/$LINK_BRANCH and cannot see it" + fail=1 + else + echo " WARN: dispatcher checkout dirty — log.link_sha will not describe what ran" + fi + fi + else + echo " ✗ could not read git status"; fail=1 + fi + + # --- gate: fwapg SHA resolvable and exported (link#246) ----------------- + # .lnk_fwapg_sha() reads FWAPG_GIT_SHA, then FWAPG_DIR, then + # ~/Projects/repo/fwapg, else NA. Cyphers have NO fwapg checkout, so + # without this export every cypher row lands fwapg_sha = NA — the exact + # provenance hole this issue exists to close. Resolve once here and hand + # the same value to every host. + local fwapg_dir fwdirty + fwapg_dir="${FWAPG_DIR:-$HOME/Projects/repo/fwapg}" + if FWAPG_SHA=$(git -C "$fwapg_dir" rev-parse HEAD 2>/dev/null) && [ -n "$FWAPG_SHA" ]; then + if fwdirty=$(git -C "$fwapg_dir" status --porcelain 2>/dev/null); then + [ -z "$fwdirty" ] \ + || { echo " ✗ fwapg checkout dirty ($fwapg_dir) — the SHA stamped into log.fwapg_sha would be a lie"; fail=1; } + fi + export FWAPG_GIT_SHA="$FWAPG_SHA" + echo " ✓ fwapg_sha ${FWAPG_SHA:0:12} (exported to all hosts)" + else + echo " ✗ no fwapg checkout at $fwapg_dir — set FWAPG_DIR, or every row lands fwapg_sha=NA" + fail=1 + fi + + # --- gate: primitive vintage on the dispatcher -------------------------- + # The predicate lives in R (lnk_preflight_vintage) because "absent is not + # fresh" and the NULL-timestamp branch are what testthat can prove and + # shell cannot. `last_analyze` alone is NULL on every primitive; the R + # side uses GREATEST(last_analyze, last_autoanalyze). + if (cd "$REPO_ROOT" && LNK_LOAD=loadall Rscript data-raw/host_vintage.R "$VINTAGE_MAX_DAYS" 2>&1 | sed 's/^/ /'); then + echo " ✓ dispatcher primitives within ${VINTAGE_MAX_DAYS} d" + else + if [ -n "$PREFLIGHT_NOTE" ]; then + echo " WARN: dispatcher primitives stale — proceeding on --preflight-note" + else + echo " ✗ dispatcher primitives stale/absent — re-run with --refresh-primitives," + echo " or: bash data-raw/snapshot_bcfp.sh --with-bcfp-views --force" + fail=1 + fi + fi + + # --- gate: BOTH DigitalOcean credentials, each forced to a real API call - + if [ "$N_CY" -gt 0 ]; then + # Leg 1 — doctl's own token. This DOES call the API; its weakness was + # asserting only an exit status on a command whose healthy answer is an + # empty list, so branch on status and never on the output. + if doctl compute droplet list --no-header >/dev/null 2>&1; then + echo " ✓ doctl token valid (DO API reachable)" + else + echo " ✗ doctl token invalid/expired — doctl auth init" + fail=1 + fi + + # Leg 2 — the token tofu actually spins droplets with. A DIFFERENT + # credential; both were minted 2026-05-18 and both expired 2026-08-30, + # and probing only leg 1 lets leg 2 surface mid-spin with half-created + # droplets and a held state lock. + # + # `tofu plan` is NOT a valid probe: against a workspace with no + # resources it returns "Plan: N to add" without ever contacting DO. + # BSD sed on this PATH, so POSIX classes only — no \s, no \+. + local tok code do_url + # Test seam. Restricted to https so a stray value cannot send a live + # bearer token to an arbitrary host over plaintext. + do_url="${LNK_PREFLIGHT_DO_URL:-https://api.digitalocean.com/v2/account}" + case "$do_url" in + https://*) ;; + *) echo " ✗ LNK_PREFLIGHT_DO_URL must be https (got '$do_url')"; fail=1; do_url="" ;; + esac + tok="${LNK_PREFLIGHT_DO_TOKEN:-$(sed -nE \ + 's/^[[:space:]]*do_token[[:space:]]*=[[:space:]]*"([^"]+)".*/\1/p' \ + "$CYPHER_TF/terraform.tfvars" 2>/dev/null | head -1)}" + [ -n "$do_url" ] || tok="" + if [ -z "$tok" ]; then + echo " ✗ could not read do_token from $CYPHER_TF/terraform.tfvars" + fail=1 + else + # --config - keeps the token out of argv, which `ps` exposes. + code=$(printf 'header = "Authorization: Bearer %s"\n' "$tok" \ + | curl -sS --config - --max-time 20 -o /dev/null -w '%{http_code}' \ + "$do_url" 2>/dev/null) || code="000" + case "$code" in + 200) echo " ✓ tofu do_token valid (HTTP 200 /v2/account)" ;; + 401) echo " ✗ tofu do_token expired/revoked (HTTP 401) — mint a new PAT and update $CYPHER_TF/terraform.tfvars"; fail=1 ;; + 000) echo " ✗ could not reach the DO API (network/DNS)"; fail=1 ;; + *) echo " ✗ tofu do_token probe returned HTTP $code"; fail=1 ;; + esac + fi + unset tok + + # Leg 3 — the s3 state backend. This exercises the AWS credentials, NOT + # DigitalOcean. Labelling it a DO check is what kept the old pre-flight + # green through a dead DO token. + if (cd "$CYPHER_TF" && tofu workspace list >/dev/null 2>&1); then + echo " ✓ tofu s3 backend reachable" + else + echo " ✗ tofu s3 backend unreachable (aws creds / not initialized)" + fail=1 + fi + fi + + return "$fail" +} + +# --- pre-flight, hosts: post-prep, pre-write ------------------------------- +# Cannot run earlier — the cyphers do not exist before the spin and their +# packages are not installed before prep. It still runs before any +# wsg_run_one.R touches the persist, so it is a "fail before WRITE" gate +# even though it is not a "fail before SPEND" one. A failure here exits 1, +# which trips the EXIT trap and burns the cyphers, bounding the loss at +# prep time rather than a whole run. +collect_stamps() { # $1 = destination tsv + local tsv="$1" out ws + : > "$tsv" + # Assign first, THEN test the exit status. `out=$(... || echo ERROR)` + # would make a failed collector look like a row. + if ! out=$(cd "$REPO_ROOT" && LNK_LOAD=loadall Rscript data-raw/host_stamp.R "$CONFIG" 2>/dev/null); then + echo " ✗ dispatcher stamp failed"; return 1 + fi + [ -n "$out" ] || { echo " ✗ dispatcher returned an empty stamp"; return 1; } + printf '%s\n' "$out" >> "$tsv" + for ws in "${CY_WS_ARR[@]}"; do + if ! out=$(ssh -o BatchMode=yes -o ConnectTimeout=15 "cypher@${CY_IP[$ws]}" \ + "cd ~/Projects/repo/link && export FWAPG_GIT_SHA='${FWAPG_GIT_SHA:-}' && Rscript data-raw/host_stamp.R '$CONFIG'" 2>/dev/null); then + echo " ✗ cy[$ws] stamp failed (ssh or Rscript) — treated as a FAILURE, not a skip" + return 1 + fi + [ -n "$out" ] || { echo " ✗ cy[$ws] returned an empty stamp"; return 1; } + printf '%s\n' "$out" >> "$tsv" + done + return 0 +} + +judge_stamps() { # $1 = tsv + (cd "$REPO_ROOT" && LNK_LOAD=loadall Rscript -e ' +a <- commandArgs(TRUE) +suppressPackageStartupMessages(pkgload::load_all(quiet = TRUE)) +# Column names come from .lnk_preflight_stamp_cols(), the SAME function +# lnk_preflight_stamp() builds its output from — deliberately not a second +# list in this file. A shell-side copy would be an invariant enforced only +# by two lists happening to agree: drop a field from the R stamp and +# read.delim silently left-shifts the rest, padding the last column, so a +# cypher on a different commit passes as "parity OK". No test written in R +# can catch that, because both sides of the comparison would be R. +# +# na.strings = character(0) is load-bearing too. lnk_preflight_stamp() emits +# the literal string "NA" for anything it could not resolve; read.delim +# defaults to na.strings = "NA" and would turn that sentinel back into a +# real NA, which the unresolved check would not match — so a run with +# fwapg_sha unresolved on every host reported "host parity clean". (The R +# side treats NA as unresolved now as well; this keeps the data faithful.) +s <- utils::read.delim(a[1], header = FALSE, colClasses = "character", + na.strings = character(0), + col.names = .lnk_preflight_stamp_cols()) +res <- lnk_preflight_parity(s, n_expected = as.integer(a[2])) +quit(status = if (isTRUE(res$ok)) 0L else 1L) +' "$1" "$((N_CY + 1))") +} + +preflight_hosts() { + [ "$N_CY" -gt 0 ] || { echo " ✓ host pre-flight: no cyphers, nothing to compare"; return 0; } + echo "=== pre-flight (hosts; post-prep, pre-write) ===" + local fail=0 ws + local tsv="$LOG_DIR/${TS}_stamps.tsv" + + if collect_stamps "$tsv" && judge_stamps "$tsv"; then + echo " ✓ host parity clean ($((N_CY + 1)) hosts)" + elif [ "$AUTO_INSTALL" = "1" ]; then + # Remediation, NOT a skip: re-run cypher_prep's install stage — which + # re-runs the fresh export assertion — then re-check exactly once. + echo " → --auto-install: reinstalling on cyphers and re-checking once" + for ws in "${CY_WS_ARR[@]}"; do + ssh "cypher@${CY_IP[$ws]}" \ + "CYPHER_PREP_BRANCH='$LINK_BRANCH' CYPHER_PREP_STAGE=install bash /tmp/cypher_prep.sh" \ + >> "$LOG_DIR/${TS}_autoinstall.log" 2>&1 \ + || { echo " ✗ cy[$ws] reinstall failed; see $LOG_DIR/${TS}_autoinstall.log"; fail=1; } + done + if [ "$fail" = "0" ] && collect_stamps "$tsv" && judge_stamps "$tsv"; then + echo " ✓ host parity clean after reinstall" + else + echo " ✗ host parity STILL failing after --auto-install" + fail=1 + fi + elif [ -n "$PREFLIGHT_NOTE" ]; then + echo " WARN: host parity failed — proceeding on --preflight-note" + else + echo " ✗ host parity failed (re-run with --auto-install to remediate)" + fail=1 + fi + + # Cypher primitive vintage. Not a formality: prep just ran + # snapshot_bcfp.sh, and this verifies the primitives actually landed + # rather than trusting the prep sentinel. + for ws in "${CY_WS_ARR[@]}"; do + if ssh -o BatchMode=yes -o ConnectTimeout=15 "cypher@${CY_IP[$ws]}" \ + "cd ~/Projects/repo/link && Rscript data-raw/host_vintage.R '$VINTAGE_MAX_DAYS'" \ + >> "$LOG_DIR/${TS}_vintage.log" 2>&1; then + echo " ✓ cy[$ws] primitives within ${VINTAGE_MAX_DAYS} d" + else + if [ -n "$PREFLIGHT_NOTE" ]; then + echo " WARN: cy[$ws] primitives stale — proceeding on --preflight-note" + else + echo " ✗ cy[$ws] primitives stale/absent — see $LOG_DIR/${TS}_vintage.log" + fail=1 + fi + fi + done + + return "$fail" +} + +# --- optional: refresh the dispatcher's primitives before anything else ---- +# Default OFF. Auto-running an ~8-minute data pull inside a paid-droplet +# orchestration is worse than stopping with the exact remediation printed. +if [ "$REFRESH_PRIMITIVES" = "1" ]; then + echo "=== --refresh-primitives: snapshot_bcfp.sh --with-bcfp-views --force ===" + ( cd "$REPO_ROOT" && PGUSER=postgres PGPASSWORD=postgres PGHOST=localhost \ + PGPORT=5432 PGDATABASE=fwapg \ + bash data-raw/snapshot_bcfp.sh --with-bcfp-views --force ) \ + > "$LOG_DIR/${TS}_snapshot.log" 2>&1 \ + || { echo "FATAL: snapshot_bcfp.sh failed; see $LOG_DIR/${TS}_snapshot.log"; exit 1; } + echo " ✓ primitives refreshed (see $LOG_DIR/${TS}_snapshot.log)" fi -[ "$fail" = "0" ] || { echo "FATAL: pre-flight failed; aborting before spend"; exit 1; } + +preflight_local || { echo "FATAL: pre-flight failed; aborting before spend"; exit 1; } echo " ✓ pre-flight clean (tunnel-free)" +if [ "$PREFLIGHT_ONLY" = "1" ]; then + echo "=== --preflight-only: local gates passed; exiting before spin ===" + # Report what was NOT checked. Several gates are gated on N_CY > 0, so a + # bare --preflight-only silently skips the credential probes — which are + # the highest-value ones, since an expired token is what motivated them. + # Absence of evidence has to be reported as absence, not read as a pass. + if [ "$N_CY" -eq 0 ]; then + echo " NOT CHECKED (no --cy-workspaces given, so no cypher gates ran):" + echo " - doctl credential, tofu do_token, tofu s3 backend" + echo " - branch-pushed assertion" + echo " - host parity and cypher primitive vintage (need live cyphers)" + echo " For the full pre-spend set, pass the workspaces you intend to use:" + echo " bash data-raw/study_area_run.sh --preflight-only --cy-workspaces=job1,job2,job3" + else + echo " NOT CHECKED (require live cyphers): host parity, cypher primitive vintage." + fi + exit 0 +fi + # --- resolve drainage-closed DS-first buckets --- echo "=== resolve drainage-closed DS-first buckets ===" DISP_BUCKET=$(cd "$REPO_ROOT" && Rscript data-raw/study_area_wsgs.R "${FOCAL_ARR[0]}") @@ -162,9 +592,14 @@ done # computed on both and consolidate is last-writer-wins. Harmless when focal # sets are drainage-independent (Peace/Fraser/Skeena are distinct roots), but # surface an accidental overlap so it's visible rather than silent. +# sed, not `grep -v '^$'`: grep exits 1 when it selects nothing, which under +# `set -euo pipefail` aborts a plain assignment. Both instances here are +# provably unreachable today (study_area_wsgs.R stop()s on an empty resolve, +# so a bucket cannot be blank), but the unsafe form has already caused two +# real aborts in this script and is exactly what gets copied next. DUP=$( { echo "$DISP_BUCKET" | tr ',' '\n' for WS in "${CY_WS_ARR[@]}"; do echo "${CY_BUCKET[$WS]}" | tr ',' '\n'; done -} | grep -v '^$' | sort | uniq -d | paste -sd, - ) +} | sed '/^[[:space:]]*$/d' | sort | uniq -d | paste -sd, - ) [ -z "$DUP" ] || echo " WARN: buckets overlap on: $DUP (computed on multiple hosts; consolidate last-writer-wins)" # --- spin + prep cyphers --- @@ -199,11 +634,33 @@ if [ "$N_CY" -gt 0 ]; then && ssh "cypher@$IP" "CYPHER_PREP_BRANCH='$LINK_BRANCH' bash /tmp/cypher_prep.sh" ) > "$LOG_DIR/${TS}_prep_$WS.log" 2>&1 & done wait + # Grep the ANCHORED "=== READY" (cypher_prep.sh's last line), not + # "snapshot_bcfp.sh: complete" (link#246). The old sentinel was wrong in + # both directions: + # + # fail-toward-PASS — snapshot_bcfp.sh prints "complete." and + # cypher_prep.sh's `tail -5` copies it into this log BEFORE + # lnk_persist_init runs. A persist_init FATAL therefore exits 1 with the + # sentinel already logged, this grep succeeds, and WSGs run against a + # half-prepped cypher. + # + # fail-toward-stop — the snapshot's skip-if-current path prints + # "snapshot_bcfp: ... skipping." (no ".sh") and exits 0 without ever + # emitting the sentinel, so a legitimately-skipped load read as FATAL. + # + # -x so "=== READY (install stage only; ...)" cannot satisfy a full-prep + # check. Only a complete prep prints the bare line. for WS in "${CY_WS_ARR[@]}"; do - grep -q "snapshot_bcfp.sh: complete" "$LOG_DIR/${TS}_prep_$WS.log" 2>/dev/null \ + grep -qx "=== READY" "$LOG_DIR/${TS}_prep_$WS.log" 2>/dev/null \ || { echo "FATAL: cypher[$WS] prep failed; see $LOG_DIR/${TS}_prep_$WS.log"; exit 1; } done echo " ✓ cyphers prepped" + + # Post-prep, pre-write. exit 1 here trips the EXIT trap, which burns the + # cyphers — the loss is bounded at spin + prep rather than a whole run of + # two mixed model versions landing in one schema. + preflight_hosts \ + || { echo "FATAL: host pre-flight failed; aborting before any WSG writes"; exit 1; } fi # --- run buckets DS-first (dispatcher local + cyphers, parallel) --- @@ -223,7 +680,16 @@ LOCAL_PID=$! declare -A CY_PID for WS in "${CY_WS_ARR[@]}"; do IP="${CY_IP[$WS]}"; B_SPACE=$(echo "${CY_BUCKET[$WS]}" | tr ',' ' ') - ssh "cypher@$IP" "cd ~/Projects/repo/link && export LNK_SCHEMA='$SCHEMA' && export LNK_GUARD_DOWNSTREAM=warn && for w in $B_SPACE; do Rscript data-raw/wsg_run_one.R \$w '$CONFIG' || echo \"[WARN] cy WSG \$w failed\"; done" \ + # FWAPG_GIT_SHA is resolved once on the dispatcher (preflight_local) and + # handed to every host: cyphers have no ~/Projects/repo/fwapg, so without + # it .lnk_fwapg_sha() returns NA and every cypher row lands with a NULL + # fwapg_sha (link#246 Phase 5). + # + # LINK_GIT_SHA / FRESH_GIT_SHA are deliberately NOT exported here. Each + # cypher writes its OWN observed values into ~/.Renviron during prep; + # pushing the dispatcher's values across would launder a claim into the + # worker's provenance and make the parity gate circular. + ssh "cypher@$IP" "cd ~/Projects/repo/link && export LNK_SCHEMA='$SCHEMA' && export LNK_GUARD_DOWNSTREAM=warn && export FWAPG_GIT_SHA='${FWAPG_GIT_SHA:-}' && for w in $B_SPACE; do Rscript data-raw/wsg_run_one.R \$w '$CONFIG' || echo \"[WARN] cy WSG \$w failed\"; done" \ > "$LOG_DIR/${TS}_run_$WS.log" 2>&1 & CY_PID[$WS]=$! done @@ -236,20 +702,111 @@ for WS in "${CY_WS_ARR[@]}"; do done echo " ✓ host runs finished (per-WSG soft-fail; gaps surface in compare)" +# --- completeness accounting, per host, BEFORE consolidate (link#246) ------ +# The per-WSG soft-fail above is deliberate, but it means "0 of 28 succeeded" +# and "28 of 28 succeeded" produce the same exit status. That is exactly the +# 2026-05 failure: every WSG on every cypher errored, the hosts exited 0, and +# nothing said so until the compare. +# +# It must NOT abort here, though. An abort at this point runs with +# CYPHERS_UP=1, so the EXIT trap burns the cyphers and destroys the WSGs that +# *did* succeed — one bad WSG on one host throwing away the whole paid run, +# which is the exact accident the soft-fail comment above exists to prevent. +# +# Instead, narrow each host's consolidate bucket to the WSGs it actually +# reported. That also removes the reason the abort was here: schema_consolidate +# DELETEs its bucket before COPYing, and a bucket containing only WSGs that are +# about to be re-COPYed cannot delete anything it does not replace. The gap +# then surfaces at the coverage post-condition after the burn, by which point +# the successful work is safely on the dispatcher. +# Split a CSV into lines, dropping blanks. `sed` deleting every line still +# exits 0, where `grep -v '^$'` exits 1 — and under `set -euo pipefail` that +# aborts the script from inside a plain assignment. That trap has now bitten +# this diff twice (cypher_prep's ~/.Renviron filter, then the consolidate +# bucket builder below), so the safe form lives in one helper rather than +# being remembered at each call site. +# +# `printf '%s\n'`, not `printf '%s'`: without the trailing newline `wc -l` +# counts separators rather than items and reports one fewer than there is, so +# a host that completed its whole bucket would be reported incomplete. The +# empty case still yields 0, because sed drops the resulting blank line. +csv_lines() { printf '%s\n' "${1:-}" | tr ',' '\n' | sed '/^[[:space:]]*$/d'; } +csv_count() { csv_lines "${1:-}" | wc -l | tr -d ' '; } + +bucket_done() { # $1 = logfile; prints the WSGs the host reported, one per line + # Matches the WSG code, not the surrounding prose, so a reworded cat() in + # wsg_run_one.R degrades to "this host reported nothing" — handled loudly + # below — rather than to a wrong set. + # + # The readability test is not decorative: `sed` on a missing file exits + # non-zero, which under `set -e` would abort into the EXIT trap and burn + # the cyphers that DID succeed. An unreadable log means "reported nothing". + [ -r "$1" ] || return 0 + sed -nE 's/^\[wsg_run_one\] ([A-Z]{4}) .*(done|SKIP).*/\1/p' "$1" | sort -u +} + +report_completeness() { # $1 = label, $2 = logfile, $3 = expected csv + local exp_n got_n warn_n + exp_n=$(csv_count "$3") + # bucket_done's `sort -u` terminates its last line, so wc -l is right here; + # counted the same way as exp_n regardless, so the two cannot drift. + got_n=$(csv_count "$(bucket_done "$2" | paste -sd, -)") + warn_n=$(grep -c '^\[WARN\] ' "$2" 2>/dev/null) || warn_n=0 + if [ "$got_n" = "$exp_n" ]; then + echo " ✓ $1: $got_n/$exp_n WSGs accounted for" + return 0 + fi + echo " ✗ $1: only $got_n/$exp_n WSGs accounted for ($warn_n [WARN]) — see $2" + return 1 +} + +echo "=== per-host completeness ===" +complete_fail=0 +report_completeness "dispatcher" "$LOG_DIR/${TS}_run_local.log" "$DISP_BUCKET" \ + || complete_fail=1 +declare -A CY_BUCKET_DONE +for WS in "${CY_WS_ARR[@]}"; do + report_completeness "cy[$WS]" "$LOG_DIR/${TS}_run_$WS.log" "${CY_BUCKET[$WS]}" \ + || complete_fail=1 + CY_BUCKET_DONE[$WS]=$(bucket_done "$LOG_DIR/${TS}_run_$WS.log" | paste -sd, -) +done +[ "$complete_fail" = "0" ] || { + echo " WARN: consolidating only the WSGs each host reported. The run will" + echo " finish so nothing already computed is lost, then exit non-zero." +} + # --- consolidate cyphers -> dispatcher --- if [ "$N_CY" -gt 0 ]; then echo "=== consolidate cyphers -> dispatcher ($SCHEMA) ===" SRC_R="list(" first=1 + n_src=0 for WS in "${CY_WS_ARR[@]}"; do IP="${CY_IP[$WS]}" - bucket_r=$(echo "${CY_BUCKET[$WS]}" | tr ',' '\n' | grep -v '^$' | sed "s/.*/'&'/" | paste -sd, -) + # The bucket is what the host REPORTED, not what it was asked to do. + # schema_consolidate DELETEs its bucket before COPYing, so a bucket + # holding only WSGs that are about to be re-COPYed cannot delete + # anything it does not replace. A host that reported nothing is skipped + # entirely rather than handed an empty bucket — an empty one would make + # schema_consolidate stop() and take the other hosts' work with it. + bucket_r=$(csv_lines "${CY_BUCKET_DONE[$WS]:-}" | sed "s/.*/'&'/" | paste -sd, -) + if [ -z "$bucket_r" ]; then + echo " WARN: cy[$WS] reported no WSGs — skipping it in consolidate" + continue + fi [ "$first" = "1" ] || SRC_R="$SRC_R, " SRC_R="$SRC_R list(host = 'cypher@$IP', via = 'docker', bucket = c($bucket_r))" first=0 + n_src=$((n_src + 1)) done SRC_R="$SRC_R)" - ( cd "$REPO_ROOT" && Rscript -e " + if [ "$n_src" -eq 0 ]; then + # Zero sources gets its own branch: `list()` would make + # schema_consolidate a no-op that returns cleanly, which reads as + # "consolidated" when nothing was. + echo " ✗ no cypher reported any WSG — nothing to consolidate" + else + ( cd "$REPO_ROOT" && Rscript -e " suppressPackageStartupMessages(pkgload::load_all(quiet = TRUE)) source('data-raw/schema_consolidate.R') res <- schema_consolidate(schema = '$SCHEMA', sources = $SRC_R, backup = TRUE) @@ -257,8 +814,9 @@ print(res) ok <- all(vapply(res\$sources, function(s) isTRUE(s\$ok), logical(1))) quit(status = if (ok) 0 else 1) " ) > "$LOG_DIR/${TS}_consolidate.log" 2>&1 \ - || { echo " ✗ consolidate failed; see $LOG_DIR/${TS}_consolidate.log"; exit 1; } - echo " ✓ consolidated (see $LOG_DIR/${TS}_consolidate.log)" + || { echo " ✗ consolidate failed; see $LOG_DIR/${TS}_consolidate.log"; exit 1; } + echo " ✓ consolidated $n_src/$N_CY cypher(s) (see $LOG_DIR/${TS}_consolidate.log)" + fi fi # --- burn cyphers now (work is consolidated; minimise idle) --- @@ -267,9 +825,50 @@ burn_cyphers || true # WSG set across all hosts. ALL_WSGS=$( { echo "$DISP_BUCKET" | tr ',' '\n' for WS in "${CY_WS_ARR[@]}"; do echo "${CY_BUCKET[$WS]}" | tr ',' '\n'; done -} | grep -v '^$' | sort -u | paste -sd, - ) +} | sed '/^[[:space:]]*$/d' | sort -u | paste -sd, - ) COMPARE_CSV="$LOG_DIR/${TS}_compare.csv" +# --- coverage post-condition (link#246) ------------------------------------ +# Detection rather than prevention: this fires whatever the cause, including +# causes nobody has thought of yet. Placed AFTER the burn so a failure cannot +# leak spend, and BEFORE the recompute so a partial result is never painted +# as complete. +# +# Necessary because schema_consolidate DELETEs the destination bucket +# (schema_consolidate.R:272-276) and then COPYs (:313-316). A source that +# produced nothing therefore removes the destination's prior rows for those +# WSGs and still returns ok = TRUE. +echo "=== verify WSG coverage in $SCHEMA ===" +if MISSING=$(PGPASSWORD=postgres psql -h localhost -p 5432 -U postgres -d fwapg -t -A -c " + WITH expect(w) AS (SELECT unnest(string_to_array('$ALL_WSGS', ','))) + SELECT coalesce(string_agg(e.w, ',' ORDER BY e.w), '') + FROM expect e + LEFT JOIN (SELECT DISTINCT watershed_group_code w FROM ${SCHEMA}.streams) g + ON g.w = e.w + WHERE g.w IS NULL") +then + # coalesce(...,'') so zero missing returns an empty string rather than + # NULL — and the empty string is only trusted on this success branch. + [ -z "$MISSING" ] || { + echo "FATAL: consolidate left $SCHEMA with no rows for: $MISSING"; exit 1; } + echo " ✓ every run WSG has rows in $SCHEMA.streams" +else + echo "FATAL: could not verify WSG coverage in $SCHEMA"; exit 1 +fi + +# This check asks "are there rows", NOT "are they from this run". The persist +# accumulates across runs and consolidate's DELETE is bucket-scoped, so a WSG +# that was excluded from a narrowed bucket keeps its PREVIOUS run's rows and +# passes here. It is a guard against consolidate destroying data, not a +# substitute for the completeness accounting — an earlier revision of this +# script leaned on it as the backstop for an incomplete run, and it cannot +# carry that. `$RUN_INCOMPLETE` is what carries it, at the very end. +RUN_INCOMPLETE="$complete_fail" +if [ "$RUN_INCOMPLETE" != "0" ]; then + echo " NOTE: some WSGs were excluded from consolidate this run; any rows" + echo " they show above are from an EARLIER run, not this one." +fi + # --- post-consolidate recompute: settle cross-WSG access (link#205) --- # Drainage-closed + DS-first per-host is NOT sufficient: a WSG's downstream # barriers can be cross-bucket or arrive late in DS-first order, so its access @@ -302,4 +901,17 @@ echo "=== summary ===" echo " run WSGs: $ALL_WSGS" echo " compare CSV: $COMPARE_CSV" tail -40 "$LOG_DIR/${TS}_compare.log" || true + +# An incomplete run must not exit 0. The failure is reported HERE rather than +# at the point of detection so that everything already computed is +# consolidated, recomputed, compared and written out first — the operator gets +# the artifacts AND an accurate exit status, instead of one at the cost of the +# other. A caller that only checks the exit code still learns the truth. +if [ "${RUN_INCOMPLETE:-0}" != "0" ]; then + echo "=== study_area_run INCOMPLETE ===" + echo " At least one host did not account for its whole bucket; only the" + echo " WSGs it reported were consolidated. Artifacts above are valid for" + echo " those WSGs. Re-run the missing ones before trusting the compare." + exit 1 +fi echo "=== study_area_run done ===" diff --git a/data-raw/wsgs_run_pipeline.sh b/data-raw/wsgs_run_pipeline.sh index 9f6fa229..1a943e47 100755 --- a/data-raw/wsgs_run_pipeline.sh +++ b/data-raw/wsgs_run_pipeline.sh @@ -257,14 +257,26 @@ if [ "$N_CY" -gt 0 ]; then # --- Step 4: per-cypher prep (parallel) --- echo "=== Step 4: cypher_prep.sh on $N_CY cypher$([ $N_CY -eq 1 ] || echo s) ===" + # Pass the dispatcher's branch, as study_area_run.sh does. Without it + # cypher_prep defaults to `main` and installs main's link, while the + # cypher_prep.sh being executed is the one just scp'd from this checkout — + # so from a feature branch the prep script calls a link function the + # installed link does not have, and every cypher FATALs. Harmless-looking + # divergence between two callers of the same script (link#246). + PREP_BRANCH="$(git -C "$REPO_ROOT" branch --show-current)" for WS in "${CY_WS_ARR[@]}"; do IP="${CY_IP[$WS]}" ( scp -q data-raw/cypher_prep.sh "cypher@$IP:/tmp/cypher_prep.sh" && \ - ssh "cypher@$IP" "bash /tmp/cypher_prep.sh" ) > "$LOG_DIR/${TS}_prep_$WS.log" 2>&1 & + ssh "cypher@$IP" "CYPHER_PREP_BRANCH='$PREP_BRANCH' bash /tmp/cypher_prep.sh" ) > "$LOG_DIR/${TS}_prep_$WS.log" 2>&1 & done wait + # Anchored "=== READY", not "snapshot_bcfp.sh: complete" — the old + # sentinel is emitted before lnk_persist_init runs, so a persist_init + # FATAL passed this check and the run continued against a half-prepped + # cypher. Full rationale at the sibling site in study_area_run.sh + # (link#246). for WS in "${CY_WS_ARR[@]}"; do - if ! grep -q "snapshot_bcfp.sh: complete" "$LOG_DIR/${TS}_prep_$WS.log" 2>/dev/null; then + if ! grep -qx "=== READY" "$LOG_DIR/${TS}_prep_$WS.log" 2>/dev/null; then echo "FATAL: cypher[$WS] prep failed; see $LOG_DIR/${TS}_prep_$WS.log" exit 1 fi diff --git a/man/lnk_preflight_fresh.Rd b/man/lnk_preflight_fresh.Rd new file mode 100644 index 00000000..92f2a0c8 --- /dev/null +++ b/man/lnk_preflight_fresh.Rd @@ -0,0 +1,70 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/lnk_preflight_fresh.R +\name{lnk_preflight_fresh} +\alias{lnk_preflight_fresh} +\title{Does the installed 'fresh' provide what the pipeline calls?} +\usage{ +lnk_preflight_fresh( + required = .lnk_fresh_required(), + required_internal = .lnk_fresh_required_internal(), + min_version = .lnk_fresh_floor(), + quiet = FALSE +) +} +\arguments{ +\item{required}{Character vector of \code{fresh} exports the pipeline cannot +run without. Defaults to the curated list in \code{.lnk_fresh_required()}.} + +\item{required_internal}{Character vector of non-exported \code{fresh} +objects reached via \code{\link[utils:getFromNamespace]{utils::getFromNamespace()}}.} + +\item{min_version}{Minimum acceptable \code{fresh} version. Defaults to the +floor declared in link's own \code{DESCRIPTION}, so the pin lives in one +place.} + +\item{quiet}{Suppress the human-readable report. The report is the +point on a cypher, where the log is all the operator gets.} +} +\value{ +Invisibly, a list with \code{ok}, \code{version}, \code{version_ok}, +\code{missing}, \code{missing_internal} and \code{message}. +} +\description{ +\code{link} calls \verb{fresh::} in a dozen places with no \code{requireNamespace()} +guard, including a default argument on an exported function +(\code{\link[=lnk_wsg_downstream_check]{lnk_wsg_downstream_check()}}, whose \code{outlets} defaults to +\code{fresh::frs_wsg_outlets()}). A \code{fresh} that is merely \emph{present} is +therefore not enough — it has to export the symbols. +} +\details{ +On the cypher droplets the installed version is whatever was baked into +the machine image, and the failure mode is silent: \code{wsg_run_one.R} +catches the missing-symbol error and \code{quit(status = 1)}s, the bucket +loop logs \verb{[WARN]} and continues, the host exits 0, and the watershed +groups are simply absent from the persist. Nothing downstream can tell +"not modelled" from "modelled empty". That is the 2026-08 failure this +exists to stop (link#246). + +Symbols are checked rather than a version string because a version is a +proxy that fails in both directions: it can read \verb{0.33.0} on a partial +install or against a shadowing library path, and it can read "wrong" +while every needed symbol is present. Loading the namespace also +exercises \code{fresh}'s own \code{Imports} resolution, which reading a +\code{DESCRIPTION} off disk does not. +} +\examples{ +res <- lnk_preflight_fresh(quiet = TRUE) +res$ok +res$version + +# A symbol fresh does not export fails, and is named in the report: +bad <- lnk_preflight_fresh(required = "frs_not_a_real_export", quiet = TRUE) +bad$missing +} +\seealso{ +Other preflight: +\code{\link{lnk_preflight_parity}()}, +\code{\link{lnk_preflight_stamp}()}, +\code{\link{lnk_preflight_vintage}()} +} +\concept{preflight} diff --git a/man/lnk_preflight_parity.Rd b/man/lnk_preflight_parity.Rd new file mode 100644 index 00000000..4630226c --- /dev/null +++ b/man/lnk_preflight_parity.Rd @@ -0,0 +1,81 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/lnk_preflight_parity.R +\name{lnk_preflight_parity} +\alias{lnk_preflight_parity} +\title{Do all hosts' pre-flight stamps agree?} +\usage{ +lnk_preflight_parity( + stamps, + n_expected, + keys = c("link_version", "fresh_version", "repo_sha", "config_hash", "fwapg_sha"), + forbid_na = c("link_version", "fresh_version", "repo_sha", "fwapg_sha"), + forbid_dirty = TRUE, + quiet = FALSE +) +} +\arguments{ +\item{stamps}{A data frame, one row per host, from +\code{\link[=lnk_preflight_stamp]{lnk_preflight_stamp()}}.} + +\item{n_expected}{Number of hosts that were supposed to report. +Required.} + +\item{keys}{Fields that must be identical across hosts.} + +\item{forbid_na}{Fields that may not be the literal \code{"NA"} on any host.} + +\item{forbid_dirty}{Fail when any host's checkout is dirty.} + +\item{quiet}{Suppress the human-readable report.} +} +\value{ +Invisibly, a list with \code{ok}, \code{n}, \code{mismatches}, \code{reference}, +\code{offenders}, \code{problems} and \code{message}. +} +\description{ +A stale install on one cypher silently produces different rollup +numbers: the provincial run looks fine, and the bcfishpass parity diff +then contains version drift as well as methodology drift, with no way to +separate them afterwards. This is the #183 sibling-host parity hook, +absorbed into link#246. +} +\details{ +Three properties, each of which fails toward stop: +\itemize{ +\item \strong{Everybody answered.} \code{n_expected} has no default on purpose. A +dropped ssh yields a short table, and a short table judged on its own +terms produces "no mismatches found" — an affirmative claim of +agreement among hosts that never replied. +\item \strong{Nothing is unresolved.} A field that is the literal \code{"NA"} on any +host is a failure, not a neutral. \code{NA == NA} is not agreement. +\item \strong{Nothing is dirty.} A SHA recorded against a dirty tree is a lie, +the same position \code{\link[=lnk_stamp]{lnk_stamp()}} already takes for packages. +} + +Only then are the key fields compared, against row 1 (the dispatcher) as +reference. \code{link_sha} and \code{fresh_sha} are deliberately \strong{not} keys — +see \code{\link[=lnk_preflight_stamp]{lnk_preflight_stamp()}} for why comparing them is either guaranteed +to fail or guaranteed to pass vacuously. +} +\examples{ +row <- function(host, ...) { + d <- list(host = host, link_version = "0.46.0", link_sha = "NA", + fresh_version = "0.33.0", fresh_sha = "NA", + repo_sha = "abc123def456", repo_dirty = "FALSE", + config_hash = "cfg012345678", fwapg_sha = "e6e1eb0aaaaa", + r_version = "4.4.1") + as.data.frame(utils::modifyList(d, list(...)), stringsAsFactors = FALSE) +} +agree <- rbind(row("m1"), row("cy-job1")) +lnk_preflight_parity(agree, n_expected = 2, quiet = TRUE)$ok + +drift <- rbind(row("m1"), row("cy-job1", fresh_version = "0.31.0")) +lnk_preflight_parity(drift, n_expected = 2, quiet = TRUE)$offenders +} +\seealso{ +Other preflight: +\code{\link{lnk_preflight_fresh}()}, +\code{\link{lnk_preflight_stamp}()}, +\code{\link{lnk_preflight_vintage}()} +} +\concept{preflight} diff --git a/man/lnk_preflight_stamp.Rd b/man/lnk_preflight_stamp.Rd new file mode 100644 index 00000000..32422660 --- /dev/null +++ b/man/lnk_preflight_stamp.Rd @@ -0,0 +1,52 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/lnk_preflight_stamp.R +\name{lnk_preflight_stamp} +\alias{lnk_preflight_stamp} +\title{One-line provenance stamp for cross-host pre-flight parity} +\usage{ +lnk_preflight_stamp(cfg = lnk_config("bcfishpass"), repo = ".") +} +\arguments{ +\item{cfg}{An \code{lnk_config} from \code{\link[=lnk_config]{lnk_config()}}. Supplies \code{config_hash}.} + +\item{repo}{Path to the git checkout this host's install came from.} +} +\value{ +A named character vector in the documented field order. +} +\description{ +The parity payload in a fixed field order, drawn from the facts +\code{\link[=lnk_stamp]{lnk_stamp()}} already collects plus the git state of the checkout this +host installed from. Kept separate from \code{\link[=lnk_stamp]{lnk_stamp()}} so the shell +contract — the field order — is a documented, tested thing rather than +an inline \code{Rscript -e} incantation that drifts between two call sites. +} +\details{ +\strong{\code{repo_sha} is the load-bearing field, not \code{link_sha}.} +\code{.lnk_pkg_git_sha()} resolves a SHA from a \code{.git} beside the installed +package. On the dispatcher link is \code{pkgload::load_all}'d from a checkout, +so it finds one; on every cypher link is pak-installed, so it does not +and returns \code{NA}. \code{fresh_sha} is \code{NA} on both unless pak recorded a +\code{RemoteSha}. Comparing \code{link_sha} across hosts would therefore always +fail, and comparing \code{fresh_sha} would be a vacuous \code{NA == NA} pass — a +check that looks like a check. \code{repo_sha} is read from +\verb{~/Projects/repo/link} \strong{on the host itself}, which on a cypher is +exactly what \verb{git reset --hard origin/} produced. It is an +independent observation rather than a restatement of what the +dispatcher believes (link#246). + +Unresolvable facts are the literal string \code{"NA"}, never empty, so a +truncated ssh response is distinguishable from a resolved absence. +} +\examples{ +s <- lnk_preflight_stamp(lnk_config("bcfishpass")) +names(s) +s[["fresh_version"]] +} +\seealso{ +Other preflight: +\code{\link{lnk_preflight_fresh}()}, +\code{\link{lnk_preflight_parity}()}, +\code{\link{lnk_preflight_vintage}()} +} +\concept{preflight} diff --git a/man/lnk_preflight_vintage.Rd b/man/lnk_preflight_vintage.Rd new file mode 100644 index 00000000..441e4b86 --- /dev/null +++ b/man/lnk_preflight_vintage.Rd @@ -0,0 +1,79 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/lnk_preflight_vintage.R +\name{lnk_preflight_vintage} +\alias{lnk_preflight_vintage} +\title{Are this host's DB primitives fresh enough to model against?} +\usage{ +lnk_preflight_vintage( + conn = NULL, + max_age_days = 7, + tables = .lnk_vintage_primitives(), + now = Sys.time(), + vintage = .lnk_vintage_read(conn, tables), + quiet = FALSE +) +} +\arguments{ +\item{conn}{A \link[DBI:DBIConnection-class]{DBI::DBIConnection} to the host's local fwapg, or +\code{NULL} when \code{vintage} is supplied directly.} + +\item{max_age_days}{Maximum acceptable age of the \strong{oldest} primitive.} + +\item{tables}{Fully-qualified table names to check. Defaults to the +snapshot-loaded set.} + +\item{now}{Reference time. Injectable so tests are not clock-dependent.} + +\item{vintage}{A data frame with \code{table_name} and \code{last_analyze}. +Defaults to reading \code{conn}; pass directly to test, or to judge a +stamp collected on another host.} + +\item{quiet}{Suppress the human-readable report.} +} +\value{ +Invisibly, a list with \code{ok}, \code{vintage}, \code{stale}, \code{missing}, +\code{oldest_days} and \code{message}. +} +\description{ +\code{snapshot_bcfp.sh} loads four primitives from public sources into each +host's local fwapg. Every cypher reloads them during prep; the +dispatcher does not, and nothing checks. On 2026-08-30 the dispatcher's +\code{cabd.dams} was 2026-05-23 and \code{fresh.modelled_stream_crossings} +2026-05-26 — a run started that day would have modelled one bucket on +May inputs and three on August inputs, produced one consolidated table +set, and said nothing about it anywhere (link#246). +} +\details{ +The other seven tables in \code{.lnk_input_primitives()} are bulk-restored +FWA. They are never \code{ANALYZE}d, so they carry no vintage at all and are +not an axis this can measure — including them would mean every host +failing forever on data that is not the staleness risk. + +\strong{Absence is not a pass.} A table missing from the result, and a table +present with a NULL timestamp, both fail — in the same direction as a +stale one. A query returning nothing must never read as "nothing is +stale". + +\code{last_analyze} alone is unusable here: measured across all ten +primitives it is NULL on every one, and only \code{last_autoanalyze} is +populated. \code{GREATEST} of the two is the usable signal — in Postgres it +ignores NULLs and is NULL only when every argument is. +} +\examples{ +# Judge a vintage table without touching a database: +now <- as.POSIXct("2026-08-30 12:00:00", tz = "UTC") +v <- data.frame( + table_name = link:::.lnk_vintage_primitives(), + last_analyze = now - c(1, 2, 1, 99) * 86400) +res <- lnk_preflight_vintage(vintage = v, now = now, max_age_days = 7, + quiet = TRUE) +res$ok +res$stale +} +\seealso{ +Other preflight: +\code{\link{lnk_preflight_fresh}()}, +\code{\link{lnk_preflight_parity}()}, +\code{\link{lnk_preflight_stamp}()} +} +\concept{preflight} diff --git a/planning/active/findings.md b/planning/active/findings.md new file mode 100644 index 00000000..9733401e --- /dev/null +++ b/planning/active/findings.md @@ -0,0 +1,286 @@ +# Findings — #246 pre-flight gates + +All measured on **m1** (the dispatcher), 2026-08-30. Nothing here is inferred. + +## Issue claims verified + +| claim | measured | +|---|---| +| `fresh` schema has no log tables | confirmed — `fresh_default` has all 4 | +| 93 WSGs / 2,865,775 rows | exact match | +| orphan state | 31 `working_*` schemas, 49 `zz_lnk_mc_scratch_*` tables | +| primitives stale | `cabd.dams` 2026-05-23, `fresh.modelled_stream_crossings` 2026-05-26 | +| cypher image | `cypher-20260512-warm` bakes link 0.35.0 + fresh 0.31.0 | +| both configs → same schema | `bcfishpass` → `fresh`, `default` → `fresh` | + +`zz_lnk_mc_scratch_*` live **inside** the persist schema +(`wsg_recompute_one.R:76`, `paste0(sch, ".", mc_name)`), so +`DROP SCHEMA fresh CASCADE` does take them. An `on.exit` drops them per run; +the 49 survivors are from crashed runs. + +## Root cause is narrower than the issue states + +`DESCRIPTION:34` already pins `NewGraphEnvironment/fresh@v0.33.0` and `:40` +declares `fresh (>= 0.33.0)`. **The pin is never resolved** because `fresh` is +in **Suggests** and `pak::local_install()` defaults to `dependencies = NA` +(verified: `formals(pak::local_install)$dependencies` is `NA`). + +20 files under `R/` call `fresh::` with **zero** `requireNamespace()` guards, +including a default argument on an exported function +(`lnk_wsg_downstream_check.R:335`, `outlets = fresh::frs_wsg_outlets()`), and +`lnk_pipeline_connect.R:101` does `getFromNamespace(".frs_run_connectivity")`. +fresh is a hard runtime dependency mis-declared as optional. + +**So the fix is a declaration fix, not an install line.** `upgrade = FALSE` +suppresses *gratuitous* upgrades, not *required* ones — 0.31.0 does not satisfy +`>= 0.33.0`, so pak is forced to resolve it and consults `Remotes`. Line 58 of +`cypher_prep.sh` needs no edit; it starts doing the right thing the moment the +declaration is honest. + +## Version boundary + +| fresh tag | `frs_wsg_drainage` | `frs_wsg_outlets` | +|---|---|---| +| v0.31.0 | absent | absent | +| v0.32.0 | **exported** | absent | +| v0.33.0 | exported | **exported** | + +Floor is **v0.33.0**, not "newer than 0.31.0". The issue is imprecise here. + +## The sentinel bug fails toward PASS (found post-approval) + +`study_area_run.sh:203` greps the prep log for `snapshot_bcfp.sh: complete`. +Two independent defects: + +1. **Fail-toward-pass (dangerous).** `snapshot_bcfp.sh:277` emits the sentinel; + `cypher_prep.sh:85`'s `tail -5` copies it into the prep log. `lnk_persist_init` + then runs at lines 100–126. A persist_init FATAL exits 1 **with the sentinel + already logged**, so `grep -q` succeeds, the umbrella prints + `✓ cyphers prepped`, and WSGs run against a half-prepped cypher. + `cypher_prep.sh:98`'s own comment anticipates this class inside prep — but + the umbrella still reads the wrong line. +2. **Fail-toward-stop (false alarm).** The skip-if-current path at + `snapshot_bcfp.sh:111–113` prints `snapshot_bcfp: …skipping.` (note: no + `.sh`) and exits 0, never reaching the sentinel → spurious FATAL. + +`=== READY` (`cypher_prep.sh:127`) is the only line implying every stage passed. +Same bug at `wsgs_run_pipeline.sh:267`. + +## Parity must key on `repo_sha`, not `link_sha` + +`.lnk_pkg_git_sha()` (`lnk_stamp.R:264`) resolves from `.git` in the package +dir. On the dispatcher link is `pkgload::load_all`'d from a checkout → real SHA. +On cyphers it is pak-installed → `NA`. `fresh_sha` is `NA` on **both**. So a +naive `link_sha` comparison always fails and a `fresh_sha` comparison is a +vacuous `NA == NA` pass — a check that looks like a check. + +The honest key is `repo_sha`: the git state of `~/Projects/repo/link` observed +**on each host**, which on a cypher is exactly what +`git reset --hard origin/$BRANCH` produced (`cypher_prep.sh:51`). + +**Subtle distinction, easy to conflate:** the cypher writing its *own* observed +SHA into its *own* `~/.Renviron` is evidence. The dispatcher exporting +`LINK_GIT_SHA` over ssh would launder the dispatcher's claim into the cypher's +provenance and make the gate circular. Do the first, never the second. +`FWAPG_GIT_SHA` is the exception — cyphers have no fwapg checkout, so the +dispatcher's value is the only one available and is explicitly a shared input. + +## `last_analyze` is NULL on every primitive + +The issue's Phase 2 bullet says assert `max(last_analyze)`. Measured: **NULL for +all ten** primitive tables; only `last_autoanalyze` is populated. Empty in bash +reads as "nothing to see" — the exact anti-pattern CLAUDE.md documents. + +Use `GREATEST(last_analyze, last_autoanalyze)`. Postgres `GREATEST` ignores +NULLs (unlike MySQL) and is NULL only when every argument is — the behaviour +wanted. The seven FWA tables are bulk-restored and never analyzed, so they carry +no vintage at all and are excluded; the four snapshot-loaded primitives are the +axis. + +## `tofu plan` is a false-green credential probe + +Two distinct DO credentials, both minted 2026-05-18, both expired 2026-08-30: +`access-token` in doctl's config (used by pre-flight and reserved-IP recovery) +and `do_token` in `rtj/env/do/dev/cypher/terraform.tfvars` (what actually spins +droplets). `tofu plan` against a zero-resource workspace returns `Plan: 2 to add` +without ever calling the DO API. + +The existing `doctl compute droplet list` **does** hit the API — its weakness is +asserting only an exit status on a command whose healthy answer is an empty +list. `tofu workspace list` exercises the **s3 backend** (AWS creds), not DO; +labelling it a DO check is what let the old pre-flight stay green. + +## `schema_consolidate` deletes before it copies + +`schema_consolidate.R:272–276` DELETEs the destination bucket, `:313–316` COPYs. +A cypher that produced nothing therefore **deletes** the dispatcher's prior rows +for those WSGs and returns `ok = TRUE`. Under #246 the run was not merely +failing to add data — it was removing it and reporting success. This is why the +post-consolidate coverage assertion is the highest-value item. + +Tables are selected solely by having a `watershed_group_code` column +(`:155–161`), so `log` / `log_input` travel but `log_parameters_fresh` / +`log_dimensions` never do (RUNBOOK:464–467 states this). + +## Errors Encountered + +| Error | Resolution | +|-------|------------| +| | | + +## Sentinel gate — both known answers, measured + +Four fixtures, old grep vs new anchored grep: + +| fixture | old `grep -q "snapshot_bcfp.sh: complete"` | new `grep -qx "=== READY"` | +|---|---|---| +| snapshot completed, **persist_init FATAL** | **PASS** (the bug) | FATAL | +| full prep succeeded | PASS | PASS | +| `CYPHER_PREP_STAGE=install` partial prep | FATAL | FATAL | +| snapshot legitimately **skipped**, prep OK | FATAL (false alarm) | PASS | + +Both defects reproduced and both fixed. The `-x` anchor is what stops the +install-stage sentinel satisfying a full-prep check. + +## fresh symbol list cross-validated by two independent methods + +A source scan of `R/` (comments stripped) and a walk of link's own parsed +namespace both return the same 12 `fresh::` call sites. The namespace walker +is what ships, because it works for an installed package where `R/` does not +exist — a source scan would find nothing there and report a clean drift check. + +Symbols mentioned in roxygen/comments but never called: `frs_habitat`, +`frs_aggregate`, `frs_cluster`, `frs_db_conn`, `frs_edge_types`, +`frs_point_snap_knn`, `.frs_access_label_filter`, `.frs_connected_waterbody`, +`extdata`. Notably **`frs_point_snap_knn` is not exported by fresh 0.33.0** — +adding it to the required list from a naive grep would have broken the check on +every host for a reason unrelated to fresh. + +Drift guard verified by restoring the bug: dropping `frs_wsg_outlets` from +`.lnk_fresh_required()` fails `test-lnk_preflight_fresh.R:78`, with the patch +proven to have taken before the result was believed. + +## Pre-flight gates — both known answers, measured live on m1 + +Every gate exercised in both directions before shipping. No droplets, no spend. + +| gate | firing case | passing case | +|---|---|---| +| primitive vintage | `--vintage-max-days=7` → FATAL, all four primitives named with ages (100/100/97/100 d) | `--vintage-max-days=200` → `✓ oldest 99.5 d` | +| persist schema | `--config=default` → FATAL naming `--schema=` | `--config=default --schema=fresh_default` → `persist: fresh_default` | +| tfvars `do_token` | `LNK_PREFLIGHT_DO_TOKEN=dop_v1_deadbeef…` → `✗ HTTP 401` | real token → `✓ HTTP 200` | +| DO reachability | `LNK_PREFLIGHT_DO_URL=https://127.0.0.1:1/…` → `✗ could not reach the DO API` | — reported distinctly from 401, not collapsed into it | +| branch pushed | unpushed branch → `✗ has no upstream` | after push (below) | +| worktree clean | uncommitted work + `N_CY>0` → `✗` ; `N_CY=0` → `WARN` only | clean tree | +| fwapg SHA | — | `✓ fwapg_sha e6e1eb0f4718` | + +### A false positive the first run caught + +The dispatcher fresh gate initially reported `✗ missing required symbols` +against a perfectly good fresh 0.33.0. Cause: the gate ran +`LNK_LOAD=loadall Rscript -e '...lnk_preflight_fresh...'`, but `LNK_LOAD` is +only read by the *driver scripts* — a bare `-e` never loads the package, so +the function did not exist, R exited non-zero, and the gate reported an +assertion failure for a broken invocation. + +That is the same conflation the gates exist to prevent, one level up. Fixed by +loading inside the expression and giving "could not run the check" its own exit +code and its own message, so nobody is sent to debug fresh when the harness is +what broke. + +## Bucketing derivation reproduces the issue's numbers + +`data-raw/study_area_buckets.R` (union-find over per-WSG +`frs_wsg_drainage()` closures, then greedy LPT over components): + +| claim | issue | derived | +|---|---|---| +| focal WSGs | 96 | 96 | +| raw closure | "close to 125" | **125** | +| modelable | 119 | **119** | +| drainage-independent components | 22 | **22** | +| dropped by species presence | LNRS, LEUT, LFRT, MFRT, UFRT, LKEC | **exact match** | +| dispatcher bucket | 39 | **39** | +| overlap between hosts | zero | **zero** (asserted, not assumed) | + +Liard confirmed: LIAR, LMUS, ULRD, DEAR, FROG, BEAV and DUNE are all absent +from the focal set and all present in the closure — the mechanism behind the +93 → 119 growth. + +**One deliberate difference.** The cypher split is 27/26/27 where the issue +says 28/24/28. The *components* are identical; only the packing differs, +because this weights by stream-segment count from `fwa_stream_networks_sp` +rather than by WSG count. A one-WSG component can outweigh a three-WSG one, +so segment count is the better proxy for work. The dispatcher figure (39) is +unaffected and matches. + +Two consecutive `--write` runs are byte-identical, so the doc can be +regenerated in CI or by a reviewer without churning the diff. + +## /code-check: five rounds, and what each found + +| round | findings | notable | +|---|---|---| +| 1 | 6 | TSV `na.strings` seam: the parity gate printed "host parity clean" with `fwapg_sha` unresolved on every host | +| 2 | 3 | blocker **inside** round 1's fix — the new `~/.Renviron` guard was unreachable AND killed prep on every fresh droplet | +| 3 | 2 (+1 adjacent) | the completeness gate aborted with `CYPHERS_UP=1`, so the trap burned cyphers and destroyed work that had succeeded | +| 4 | 3 | round 3's two new guards were themselves unreachable — same `grep`-under-`set -e` class as round 2 | +| 5 | **0 — clean** | verified by restoring each round-4 fix and across 16 helper inputs | + +**Rounds 1→4 each landed a blocker inside the previous round's fix.** The +recurring mechanism was not carelessness about the *rule* — the rule was +written down in the very comment above each defect — it was fixing one +*instance* of a class without sweeping the diff for the others. `grep` exiting +1 under `set -euo pipefail` caused three separate aborts in three separate +places across three rounds. + +What ended it was replacing the remembered form with one that cannot be got +wrong: `csv_lines()` / `csv_count()`, built on `sed` (exits 0 having deleted +every line) rather than `grep -v '^$'` (exits 1). The two remaining +`grep -v '^$'` instances were swept even though both are provably unreachable +today, because the unsafe form is what gets copied next. + +**One bug was caught by reading a probe's own output rather than by review:** +`printf '%s'` emits no trailing newline, so `wc -l` counted separators and +`csv_count "MORR,BULK"` returned 1. A host that completed its whole bucket +would have been reported incomplete. The test printed `job1 expected=1` for a +two-element bucket; the number was the tell. + +**A backstop that did not back anything up.** Round 3's fix rested on the +coverage post-condition catching any gap. Round 4 showed it could not: it +asserts rows *exist*, not that they are *from this run*, and since the persist +accumulates and consolidate's DELETE is bucket-scoped, an excluded WSG keeps +its previous run's rows and passes. `RUN_INCOMPLETE` now carries the failure to +a non-zero exit at end-of-script — after the artifacts are written, so the +operator gets both the output and an honest status. + +## Does the cypher split respect drainage closure? — audited + +Asked post-review, checked rather than argued. Four properties, all on the +derived 4-host split: + +| property | result | +|---|---| +| raw closures of distinct components pairwise disjoint | **0 overlapping pairs** | +| every host bucket drainage-closed (`closure(w) ∩ modelable ⊆ bucket`, for every `w`) | **0 violations** | +| DS-first order valid within each host, across concatenated components | **0 violations** | +| blocking dams in the 6 species-dropped WSGs | **none** | + +The third is the one worth naming: a host can hold several components, and +its bucket is those components' DS-first lists concatenated. That is safe +*because* components are drainage-independent — no flow path crosses a +component boundary, so inter-component order is free. It is now checked +rather than reasoned about. + +The fourth closes the residual gap. LEUT, LFRT, LKEC, LNRS, MFRT and UFRT sit +in the closure but are dropped by species presence, so they are never +modelled and never persist barriers. If one carried a blocking dam, a WSG +above it would be modelled with that dam invisible — the #227 failure. None +of the six holds a dam at all, so the gap is benign. + +Both assertions are now **in the generator**, and both were verified by +restoring the bug: moving one WSG off its component's host gives +`host 1 bucket is not drainage-closed - missing HARR`, and reversing a host's +order gives `TAKL is ordered before its downstream LFRA, HARR, ...`. The +un-corrupted script exits 0 and `research/study_areas.md` is byte-identical, +so the assertions verify without changing the output. diff --git a/planning/active/progress.md b/planning/active/progress.md new file mode 100644 index 00000000..5594956f --- /dev/null +++ b/planning/active/progress.md @@ -0,0 +1,65 @@ +# Progress — #246 pre-flight gates + +## Session 2026-08-30 + +- Plan-mode exploration on m1; every locally-checkable issue claim verified + against the live DB, the fresh tags, and the cypher tfvars (findings.md). +- Two Plan-agent reviews. The second, arriving post-approval, found the + sentinel bug fails toward **pass** (not merely toward stop) and that a + `link_sha` parity key can never work — both folded into the baseline. +- Scope: Phases 1–2 of the issue plus the bucketing derivation. The wipe, the + paid run and the provenance audit are a separate session. +- Created branch `246-preflight-gates-provenanced-rerun` off main. +- Next: Phase 1. + +### Note on the branch point + +`main` was **1 commit ahead of origin** when this branch was cut (`c9e2ddb`, +a CLAUDE.md version-header sync unrelated to #246), so this branch carries it +and the PR will include it. That unpushed commit is also a free real failing +case for the branch-pushed gate — test the failing answer before pushing. + +### Phase 1 — 52a3723 + +`fresh` Suggests → Imports (the root cause: pak's `dependencies = NA` never +resolved the existing `Remotes` pin), new `lnk_preflight_fresh()` asserting +symbols with a namespace-walking drift guard, `cypher_prep.sh` rewrite +(pinned install, assertion, `~/.Renviron` provenance, `CYPHER_PREP_STAGE`), +and the prep sentinel fixed at both call sites. Drift guard verified by +restoring the bug with proof the patch took. + +### Phase 2 — fb8e642 + +Pre-flight split into `preflight_local()` (pre-spend) and +`preflight_hosts()` (post-prep, pre-write), plus `lnk_preflight_vintage()`, +`lnk_preflight_stamp()`, `lnk_preflight_parity()` and two driver scripts. +Two post-conditions and three new flags. Every gate exercised in both +directions on m1 with no spend. + +### Phase 3 — b7cb155 + +`data-raw/study_area_buckets.R` derives the host buckets by union-find over +drainage closures and regenerates `research/study_areas.md`. Reproduces the +issue's 22 components / 119 modelable / 39-on-dispatcher from first +principles; two `--write` runs are byte-identical. + +### Phase 4 — in progress + +RUNBOOK §8d, NEWS 0.47.0, version bump. Issue #246 body corrected in three +places and marked Phases 1-2 done. Follow-up #247 filed +(`fresh.snapshot_stamp`). + +**Not done, deliberately:** `/code-check` was run once over the whole branch +diff rather than per commit — recorded here rather than ticking a per-commit +box that did not happen. + +### code-check — 5 rounds, converged + +Rounds 1-4 each found a blocker inside the previous round's fix; round 5 was +clean, verified by restore-the-bug on each round-4 change. 14 real issues +total, all fixed. Full table and the mechanism analysis in findings.md. + +The recurring cause was fixing one instance of a class without sweeping the +diff for the rest — `grep` exiting 1 under `set -euo pipefail` aborted three +different code paths across three rounds. Ended by replacing the remembered +form with `csv_lines()` / `csv_count()`, which cannot be got wrong. diff --git a/planning/active/review-round1.md b/planning/active/review-round1.md new file mode 100644 index 00000000..7816f1ec --- /dev/null +++ b/planning/active/review-round1.md @@ -0,0 +1,218 @@ +# Review round 1 — `main..HEAD` (#246 pre-flight gates) + +Reviewed in full: `data-raw/study_area_run.sh`, `data-raw/cypher_prep.sh`, +`data-raw/study_area_buckets.R`, `data-raw/host_stamp.R`, +`data-raw/host_vintage.R`, `data-raw/wsgs_run_pipeline.sh`, +`R/lnk_preflight_{fresh,vintage,stamp,parity}.R`, the three new test files, +`DESCRIPTION`, `NAMESPACE`. + +All three new test files pass (`testthat::test_local(filter="preflight")`, +85 assertions, 0 failures). + +--- + +## Findings + +### 1. [bug] `data-raw/study_area_run.sh:449` — `judge_stamps` silently disables the parity gate's `forbid_na` property + +`judge_stamps()` reads the collected stamps with + +```r +s <- utils::read.delim(a[1], header = FALSE, colClasses = "character", + col.names = strsplit(a[2], ",")[[1]]) +``` + +`read.delim` applies `na.strings = "NA"` **even for `colClasses = "character"`**, +so the literal `"NA"` sentinel that `lnk_preflight_stamp()` deliberately emits +(`R/lnk_preflight_stamp.R:43-46`, documented at :22-23 as *"Unresolvable facts +are the literal string `"NA"`, never empty"*) is converted back to R's `NA` +before `lnk_preflight_parity()` ever sees it. + +Both of the function's stated properties then fail toward pass: + +* `stamps[[k]] %in% c("NA", "")` (`R/lnk_preflight_parity.R:91`) is `FALSE` for + `NA`, so the "Nothing is unresolved" check can never fire from the shell. +* `which(stamps[[k]] != ref[[k]])` (`:112`) — `"e6e1eb0" != NA` is `NA`, which + `which()` drops, so an NA-vs-value drift is not even reported as a mismatch. + +**Failure scenario (reproduced).** Dispatcher resolves `FWAPG_GIT_SHA`; a cypher +does not (ssh env stripped, `AcceptEnv`/`PermitUserEnvironment` config, or +`.lnk_fwapg_sha()` returning NA) and stamps `fwapg_sha=NA`. Same two-row TSV, +same judge: + +``` +read.delim(defaults) -> ok = TRUE | problems: +read.delim(na.strings=character(0)) -> ok = FALSE | fwapg_sha unresolved on: cy-job1 + 1 field mismatch(es) vs m1 +``` + +So the run proceeds and every cypher row lands with `fwapg_sha = NULL` — the +exact provenance hole #246 exists to close — while the gate prints +`✓ host parity clean`. Same path swallows `repo_sha=NA` (non-git host), +`fresh_version=NA` (fresh not installed) and `repo_dirty=NA`. + +**Why no test catches it:** every case in `tests/testthat/test-lnk_preflight_parity.R` +constructs the data frame in R (`row()` helper, lines 1-8) and never goes through +the TSV round-trip that `judge_stamps` performs — including +`"a stamp is judgeable by the parity function it feeds"` (:108), which is the +test written to guard this seam. The fixture set is structurally incapable of +reaching the defect. + +**Fix:** `na.strings = character(0)` in `judge_stamps`. Worth also asserting the +column count — `read.delim` defaults to `fill = TRUE`, so a truncated ssh +response is padded rather than rejected (it happens to fail today because the +pad for a character column is `""`, which *is* in the `forbid_na` set, but that +is luck rather than design). And add one test that writes a TSV and reads it +back the way the shell does. + +--- + +### 2. [fragile] `data-raw/study_area_run.sh:171-176` — the "second layer" schema-collision guard is unreachable + +```sh +if [ -n "$SCHEMA" ] && [ "$CONFIG" != "bcfishpass" ] && [ "$SCHEMA" = "$BCFP_SCHEMA" ]; then +``` + +Line 131 already exits when `CONFIG != bcfishpass && -z SCHEMA_OVERRIDE`, and +this block is inside the `else` of `if [ -n "$SCHEMA_OVERRIDE" ]` (:162). So by +the time control reaches :172, `SCHEMA_OVERRIDE` is empty, which means `CONFIG` +is necessarily `bcfishpass`, which means `[ "$CONFIG" != "bcfishpass" ]` is +always false. The guard cannot go red under any input. + +The comment claims it *"catches a future third config that happens to +collide"* — it cannot: such a config without `--schema=` is rejected at :131, +and with `--schema=` this branch is never entered. The extra `resolve_schema +bcfishpass` R invocation at :171 is pure cost. + +To actually provide the second layer, compare the **resolved** `$SCHEMA` +(override included) against `bcfishpass`'s resolved schema for any +`CONFIG != bcfishpass`, outside the `else`. + +--- + +### 3. [fragile] `data-raw/study_area_run.sh:29 / 109-123 / 278 / 359` — the documented bare `--preflight-only` skips every gate it was written for + +The header advertises + +``` +# bash data-raw/study_area_run.sh --preflight-only # gates only, no spend +``` + +and describes `preflight_local()` as covering *"BOTH DO credentials forced +through a real API call"*. With no `--cy-workspaces`, `N_CY=0`, and the +branch-pushed/`@{upstream}` check (:278), both DigitalOcean credential probes +(:359-397) and the tofu s3-backend probe (:403) are all inside +`if [ "$N_CY" -gt 0 ]`. None run, and nothing prints to say they were skipped. + +Confirmed by running the documented invocation on this checkout — output shows +only the fresh-symbol, fwapg-sha and vintage gates; no `origin/... is at HEAD`, +no doctl, no tofu lines. Had vintage passed, it would have printed +`✓ pre-flight clean` having never touched a credential. + +That is the opposite of the stated motivation (:370-373: *"both were minted +2026-05-18 and both expired 2026-08-30"*). An operator who dry-runs the gates, +gets a clean bill, then launches for real still discovers the dead token +mid-spin. + +Either have `--preflight-only` imply the cypher legs, or print an explicit +`⊘ skipped (no --cy-workspaces)` line per skipped gate so silence is never read +as a pass. + +--- + +### 4. [fragile] `data-raw/cypher_prep.sh:158-159` — `|| true` on the `.Renviron` rewrite can wipe the file it says it is preserving + +```sh +grep -vE '^(LINK_GIT_SHA|LINK_GIT_DIRTY|FRESH_GIT_SHA)=' "$RENV" > "$RENV.tmp" || true +mv "$RENV.tmp" "$RENV" +``` + +The comment directly above states the intent: *"never rewrite wholesale, since +the image may keep unrelated settings here."* But `|| true` collapses grep's +exit 1 (*no lines survived* — legitimate) with exit ≥2 (*read error*, unreadable +file, `-v` on a file grep decides is binary). On the error path the redirect has +already created an empty `$RENV.tmp`, and the unconditional `mv` installs it as +`~/.Renviron` — destroying every unrelated setting, permanently, with no +message. Classic "empty result is not a pass" plus "`cmd > file` truncates +before `cmd` runs". + +Branch on the status instead: + +```sh +rc=0; grep -vE '^(LINK_GIT_SHA|LINK_GIT_DIRTY|FRESH_GIT_SHA)=' "$RENV" > "$RENV.tmp" || rc=$? +[ "$rc" -le 1 ] || { rm -f "$RENV.tmp"; echo "FATAL: could not read $RENV" >&2; exit 1; } +mv "$RENV.tmp" "$RENV" +``` + +--- + +### 5. [fragile] `data-raw/wsgs_run_pipeline.sh:263 / 272` — new sentinel + new prep script, but this caller still pins the cypher to `origin/main` + +The sentinel changed to `grep -qx "=== READY"`, which only the new +`cypher_prep.sh` emits. This caller scps the **dispatcher's working-copy** prep +script (:263) but runs it with no `CYPHER_PREP_BRANCH` (:264), so the cypher +does `git reset --hard origin/main` and `pak::local_install`s **main's** link. +The new prep script then calls `link::lnk_preflight_fresh()` (`cypher_prep.sh:173`), +which does not exist in main's link until this branch merges — so every cypher +FATALs at prep for anyone running `wsgs_run_pipeline.sh` from a feature branch. + +`study_area_run.sh:580` avoids this by passing `CYPHER_PREP_BRANCH='$LINK_BRANCH'`. +Fail-loud rather than silent, and it resolves on merge, but it is a new coupling +introduced by this diff and the same one-word fix applies. + +--- + +### 6. [security, low] `data-raw/study_area_run.sh:389` — `LNK_PREFLIGHT_DO_URL` redirects a live bearer token to an arbitrary host + +```sh +code=$(printf 'header = "Authorization: Bearer %s"\n' "$tok" \ + | curl -sS --config - --max-time 20 -o /dev/null -w '%{http_code}' \ + "${LNK_PREFLIGHT_DO_URL:-https://api.digitalocean.com/v2/account}" ...) +``` + +Keeping the token out of `argv` via `--config -` is right. But the URL it is +sent to is an unvalidated env-var override, so anything that can set +`LNK_PREFLIGHT_DO_URL` in the dispatcher's environment exfiltrates the +DigitalOcean PAT on the next pre-flight, and the only visible symptom is a +non-200 status. Cheap hardening: require the override to match +`^https://` plus an allowlisted host, or gate it behind the same +`LNK_PREFLIGHT_DO_TOKEN` test seam so a real credential is never sent to an +overridden URL. + +--- + +## Checked and found clean (no action) + +* **BSD grep alternation** in `check_bucket_complete` (`study_area_run.sh:662`). + `\(done\|SKIP\)` was tested against `/usr/bin/grep` (BSD grep 2.6.0-FreeBSD, + "GNU compatible") on this machine: 2/2 matches, and the pattern matches both + real `wsg_run_one.R` outputs (`... done in %.1f min`, `... SKIP — no modeled + species`, R/`wsg_run_one.R:56,88`). Scripts resolve `grep` to `/usr/bin/grep` + (the agent shell's `grep` is a ugrep wrapper — not what the script sees). +* **`set -e` and the `{ ...; } >> "$RENV"` group** in `cypher_prep.sh:160-164`. + A trailing `[ -n "$FRESH_SHA" ] && printf ...` with an empty `FRESH_SHA` + does **not** abort the script — the failing command precedes `&&`, so errexit + is exempt. Verified empirically. +* **`psql` exit status** for the coverage post-condition + (`study_area_run.sh:731-746`). A bad-schema query returns 1 locally, so the + `else` FATAL branch is reachable and the empty-string result is only trusted + on the success branch, as the comment claims. +* **`local rc=$?`** in `burn_cyphers` — `$?` is expanded before `local` runs, so + this is not the `local x=$(cmd)` status-swallowing trap. +* **`fresh_out=$(...) && fresh_rc=0 || fresh_rc=$?`** (:265) correctly captures + the R exit status; the 0/1/other tri-state is sound. +* **Empty-array expansion under `set -u`** — the script runs under homebrew + bash 5.3 (it already requires `declare -A`), and every bare `"${CY_WS_ARR[@]}"` + is either gated on `N_CY -gt 0` or reached only when `CYPHERS_UP=1`. +* **`.lnk_vintage_primitives()`** returns exactly 4 tables, matching the length-4 + age vector in the `lnk_preflight_vintage` `@examples`. +* **`DESCRIPTION`** — `fresh (>= 0.33.0)` in Imports is consistent with + `Remotes: NewGraphEnvironment/fresh@v0.33.0`; `.lnk_fresh_floor()` parses it + correctly and the floor test asserts it. +* **`study_area_buckets.R`** union-find (path halving, `<<-` into the global + `parent`), the `new.env()` membership index (`e[["missing"]]` returns `NULL`, + not an error), the LPT pack and the host relabel-by-descending-load all check + out; the disjointness assertion at :157-162 is a real invariant that can fire. +* `on.exit()` at script top level (`host_vintage.R:69`, `study_area_buckets.R:93,253`) + is a documented no-op, but the targets are a DB connection and a file + connection that R closes and flushes at session end — no practical impact. diff --git a/planning/active/review-round2.md b/planning/active/review-round2.md new file mode 100644 index 00000000..d380bbd1 --- /dev/null +++ b/planning/active/review-round2.md @@ -0,0 +1,197 @@ +# Code review — round 2 (#246, `main..HEAD`) + +Scope: `git diff main..HEAD`, all changed files read in full from the working +tree. Focus per the round-2 brief: the six round-1 fixes in commit `0b618ca`, +and invariants held together by two lists agreeing. + +Round-1 fix verdicts: + +| # | fix | verdict | +|---|---|---| +| 1 | `is_unresolved()` / NA normalisation / `na.strings = character(0)` | **works**, both halves; verified by reproducing the drift the tests describe | +| 2 | unreachable schema-collision guard removed | **works** — the name-based guard at :131 covers every config, and the branch it was in was provably dead | +| 3 | `cypher_prep.sh` grep branches on exit status | **BROKEN — see finding 1.** The new guard is unreachable *and* it hard-aborts prep on the ordinary case | +| 4 | `wsgs_run_pipeline.sh` passes `CYPHER_PREP_BRANCH` | **works**; swept for other callers — all in-repo call sites now pass it (the only omission left is a prose snippet in `research/post_compact_provincial_handoff.md`) | +| 5 | `--preflight-only` reports what it did not check | **works** | +| 6 | `LNK_PREFLIGHT_DO_URL` restricted to https | **works** as described (TLS only — the comment claims no more than that) | + +Also checked and found clean, so they need no further attention: + +- **`study_area_buckets.R` union-find** is correct. Path-halving with `<<-` + behaves (superassignment resolves `parent` in the script's global frame and + the indexed form writes back); `seen[[m]]` on a missing key returns `NULL`, + not an error; `parent` stays integer end-to-end because both `union2()` + arguments originate in `seq_along()` (a double would coerce the vector and + break the `vapply(..., integer(1))` — verified, but not reachable here). + Replicated the whole loop on a 5-node fixture: correct components. +- **LPT pack + host relabelling** are correct and deterministic. `order(-w)` + and `which.min()` both break ties by first index; `relabel` maps old→new + correctly and `load_h[host_order]` stays consistent with it. Confirmed + against the committed `research/study_areas.md` (dispatcher = heaviest). +- **`--write` path resolution** works from the repo root via + `commandArgs(FALSE)`, and fails loud (`stop("cannot locate research/...")`) + when `--file=` is absent, e.g. under `R -f`. +- **`check_bucket_complete`**: `local done_n` is declared separately, so + `done_n=$(grep -c …) || done_n=0` captures grep's status correctly; `$3` can + never be empty (`grep -c` always prints a number, and the `||` fallback + covers the pipefail case). BSD grep on macOS *does* support `\|`/`\(` in a + BRE — measured against `/usr/bin/grep`, 2 matches — so the alternation is + fine. `wsg_run_one.R` emits both sentinels (`… done in`, `… SKIP —`) on + stdout at line start. +- **Empty arrays under `set -u`**: `"${CY_WS_ARR[@]}"` is expanded + unconditionally in several places and *does* abort under bash 3.2 — but + `declare -A` (pre-existing, :560/:578) already requires bash 4+, and + `env bash` resolves to 5.3 here. Not a new exposure. +- No other `|| true` on a truncating redirect, and no other bare-command-then-`$?` + under `set -e`, anywhere in the changed scripts. +- `devtools::test(filter = "preflight")`: `FAIL 0 | PASS 90`. + +--- + +## Findings + +### 1. **[bug]** `data-raw/cypher_prep.sh:162` — round-1's `.Renviron` guard aborts prep on every fresh cypher, and its FATAL branch can never run + +```bash +grep -vE '^(LINK_GIT_SHA|LINK_GIT_DIRTY|FRESH_GIT_SHA)=' "$RENV" > "$RENV.tmp" +RC=$? +if [ "$RC" -gt 1 ]; then ... fi +``` + +The file opens with `set -euo pipefail` (:80) and this grep is a bare simple +command at top level — not in an `if`, not in an `&&`/`||` list. `grep -v` +exits **1** when it selects no lines, so `set -e` kills the script *at the +grep*. `RC=$?` and the entire FATAL branch round 1 added are unreachable: the +exit-2 read error it was written to catch would abort the same way, silently. + +Reproduced: + +```bash +$ cat t1.sh +set -euo pipefail +: > empty.txt +grep -vE '^(A|B)=' empty.txt > empty.tmp +RC=$? +echo "REACHED RC=$RC" +$ bash t1.sh; echo "exit=$?" +exit=1 # "REACHED" never prints +``` + +When it fires: + +- **`~/.Renviron` empty or absent** — `touch` creates it empty, grep selects + nothing, exit 1. This is the expected state of a fresh droplet, so it is the + *first* prep on every cypher. +- **Idempotent re-prep** of a host whose `~/.Renviron` contains only the three + keys this block owns — same result. The header advertises "Idempotent — safe + to re-run", and `study_area_run.sh --auto-install` (:481) re-runs + `cypher_prep.sh` on this same path. + +Blast radius: the prep dies with **no message** (the `echo` is past the abort), +after `git reset`, `pak::local_install` and the provenance line have already +run. `study_area_run.sh`'s `grep -qx "=== READY"` then reports `FATAL: +cypher[$WS] prep failed` and the EXIT trap burns — so spin cost is paid and the +operator gets a log that just stops mid-file. Round 1 traded a +fail-toward-silence bug for a fail-toward-abort one on the common path. + +Fix — keep the branch, make the status reachable: + +```bash +RC=0 +grep -vE '^(LINK_GIT_SHA|LINK_GIT_DIRTY|FRESH_GIT_SHA)=' "$RENV" > "$RENV.tmp" || RC=$? +if [ "$RC" -gt 1 ]; then + echo "FATAL: could not read $RENV (grep exit $RC); refusing to overwrite it" >&2 + rm -f "$RENV.tmp"; exit 1 +fi +``` + +Test it against both known answers before shipping: an empty `.Renviron` (must +proceed) and an unreadable one, e.g. `chmod 000` (must print FATAL and exit 1). +Neither case currently reaches the code that distinguishes them. + +--- + +### 2. **[bug]** `data-raw/study_area_run.sh:426` vs `R/lnk_preflight_stamp.R:62` — nothing enforces `STAMP_COLS` matches the R field order, and a one-column drift makes the parity gate report "OK" for hosts on different commits + +The mechanism question. `judge_stamps()` passes the shell constant + +``` +STAMP_COLS="host,link_version,link_sha,fresh_version,fresh_sha,repo_sha,repo_dirty,config_hash,fwapg_sha,r_version" +``` + +as `col.names` to `read.delim()`, while the row itself is produced by +`lnk_preflight_stamp()`. Two independently-maintained lists. + +`test-lnk_preflight_parity.R:99-106` carries the comment *"This is what keeps +the shell's STAMP_COLS and the R field order from silently diverging"* — but it +asserts `names(s) == .lnk_preflight_stamp_cols()`, i.e. **R against R**. Nothing +in the repo reads `study_area_run.sh`. The invariant is held only by the two +lists happening to agree today. + +`read.delim()` does not fail on the drift. With **fewer** data fields than +`col.names` it left-shifts every column past the removed one and pads the last +with `""` — no warning: + +```r +# R's stamp drops `fresh_sha`; STAMP_COLS unchanged. cy-job2 is on a +# DIFFERENT commit (clean). +s <- read.delim(tsv, header = FALSE, colClasses = "character", + na.strings = character(0), col.names = shell_cols) +lnk_preflight_parity(s, n_expected = 3, quiet = TRUE) +#> ok = TRUE +#> [preflight] host parity - OK across 3 host(s): m1, cy-job1, cy-job2 +``` + +The real `repo_sha` lands in the `fresh_sha` column, which is deliberately not +a key; the `repo_sha` *key* now holds `repo_dirty` ("FALSE" everywhere), so it +agrees. `forbid_na` sees nothing empty or `"NA"`. A cypher running a different +commit passes — exactly the failure #246 exists to close, reported as clean. +(A `+1` drift is loud but for the wrong reason: the extra field wraps onto a new +row and the gate reports "a host did not report".) + +Fix, and it removes the second list rather than guarding it: `judge_stamps()` +already runs `pkgload::load_all()`, so it can read the column names from the one +definition and drop `STAMP_COLS` entirely — + +```r +cols <- link:::.lnk_preflight_stamp_cols() +s <- utils::read.delim(a[1], header = FALSE, colClasses = "character", + na.strings = character(0), col.names = cols) +``` + +then assert `ncol(s) == length(cols)` and that no row parsed short, so a +future shape change fails on the shape rather than on a shifted comparison. + +--- + +### 3. **[fragile]** `data-raw/study_area_run.sh:279` — `git fetch … || true` lets the branch-pushed gate print a false green + +```bash +git -C "$REPO_ROOT" fetch --quiet origin "$LINK_BRANCH" 2>/dev/null || true +``` + +The comment two lines above states the fetch is load-bearing: *"@{upstream} is +a LOCAL ref, so without this the check compares against a stale copy and is a +false green."* `|| true` then makes a failed fetch indistinguishable from a +successful one, and the `rev-list --count '@{upstream}..HEAD'` below runs +against exactly the stale ref the comment warns about — printing +`✓ origin/$LINK_BRANCH is at HEAD`. + +Concretely: the remote branch has been deleted (or auth/network failed) while a +local `refs/remotes/origin/` still exists at HEAD → gate green → the +run spins droplets → `cypher_prep`'s `git reset --hard origin/$BRANCH` fails → +caught only by the READY grep, after paying for the spin. The whole point of +`preflight_local` is "predict before spend". + +Not rated higher because `preflight_hosts`' `repo_sha` comparison is a genuine +backstop for the drift direction that survives to prep — but that check runs +*after* the spin, which is what this gate was added to avoid. + +Fix: branch on the fetch, the same shape used everywhere else in this file. + +```bash +if ! git -C "$REPO_ROOT" fetch --quiet origin "$LINK_BRANCH" 2>/dev/null; then + echo " ✗ could not fetch origin/$LINK_BRANCH — cannot verify the branch is pushed" + fail=1 +fi +``` diff --git a/planning/active/review-round3.md b/planning/active/review-round3.md new file mode 100644 index 00000000..2cf101a9 --- /dev/null +++ b/planning/active/review-round3.md @@ -0,0 +1,186 @@ +# Code review — round 3 (#246 branch, `main..HEAD` @ 02b2e44) + +Scope: narrow + deep on the files edited most (`data-raw/study_area_run.sh`, +`data-raw/cypher_prep.sh`), verification of round 2's three fixes, then a sweep +of the whole diff for the recurring classes. + +## Round 2's fixes — verified + +### 1. `cypher_prep.sh:167-174` `.Renviron` strip-and-append — **behaviour correct** + +Exercised the exact block in a sandbox across all five cases. Results: + +| case | outcome | +|---|---| +| `.Renviron` absent | `touch` creates it, RC=1, keys appended, exit 0 | +| empty | RC=1, keys appended, exit 0 | +| only owned keys | RC=1, old keys stripped, new appended, exit 0 | +| unrelated keys only | RC=0, unrelated preserved, keys appended | +| mixed | RC=0, unrelated preserved, owned key replaced (not duplicated) | + +`RC > 1` FATAL is reachable for a genuine read error, and the `rm -f "$RENV.tmp"` +before `exit 1` means a real grep error leaves the original file untouched. If +the redirect itself fails, RC=1 (indistinguishable from "no lines matched") but +the subsequent `mv` then fails loudly under `set -e`. `touch "$RENV"` failing +also aborts under `set -e`. Correct in all cases. *(One non-functional problem +with this block — see finding 2.)* + +Also checked the adjacent `{ ...; [ -n "$FRESH_SHA" ] && printf ...; } >> "$RENV"`. +CLAUDE.md warns that a false leading test in a bare `&&` list aborts under +`set -e`; measured on bash 5.3 it does **not** — errexit stays suspended for the +whole `&&` list and for the enclosing brace group, and execution continues. +Confirmed empirically with an empty `FRESH_SHA`. Not a bug. + +Swept the rest of `cypher_prep.sh` for bare commands whose exit 1 is ordinary: +the pak block, the fresh-override block, the export assertion, the snapshot and +`persist_init` all use the `if ! cmd > tmp` idiom; `git rev-parse` / `git status` +are assign-then-test. Nothing else abort-prone. + +### 2. `study_area_run.sh:454-478` `judge_stamps()` — **arg shift correct** + +`judge_stamps` now takes 2 args (`$1` tsv, `$((N_CY+1))`), read as `a[1]`/`a[2]`; +`STAMP_COLS` is gone and `col.names` comes from `.lnk_preflight_stamp_cols()`. +Verified `.lnk_preflight_stamp_cols()` resolves under `pkgload::load_all()` in a +fresh `Rscript` (export_all default) — returns 10 names. Ran `host_stamp.R` +end-to-end: emits exactly 10 tab-separated fields, no stray stdout. `$1` is an +absolute path so the `cd "$REPO_ROOT"` is harmless. + +The "two lists happening to agree" residue — `lnk_preflight_stamp()`'s literal +field order (`R/lnk_preflight_stamp.R:48-57`) vs `.lnk_preflight_stamp_cols()` +(`:67-70`) — **is guarded**: `tests/testthat/test-lnk_preflight_parity.R:107` +asserts `identical(names(s), .lnk_preflight_stamp_cols())`. Measured the failure +direction anyway: a dropped field left-shifts and pads (silent false green), an +added field wraps into a second row (`nrow != n_expected`, fails). The test +closes the dangerous direction. + +### 3. `study_area_run.sh:284-305` branch gate `if/elif/else` — **no orphaned arm** + +Three arms, all setting `fail=1` or printing `✓`: fetch-failed, no-upstream, and +the else with its own assign-then-test on `rev-list --count` plus an explicit +error arm. Detached HEAD (`LINK_BRANCH=""`) makes `git fetch origin ''` fail → +arm 1 → `fail=1`, so it cannot silently ship cyphers to `main`. + +## Sweep — other recurring classes + +- **Bare exit-1-normal commands.** Every `grep`/`pg_isready` added by the diff is + guarded (`|| RC=$?`, `|| n=0`, `|| { …; fail=1; }`, `if !`). `$(( ))` is + arithmetic *expansion*, not the `(( ))` command, so no exit-status hazard. + `check_bucket_complete`'s two `grep -c` calls are `$(...) || var=0` and are + additionally inside a function invoked via `||` (errexit suspended) — safe. +- **`grep -c '^\[wsg_run_one\] .* \(done\|SKIP\)'`** uses GNU BRE `\|`. Tested + against `/usr/bin/grep` on this Darwin box: alternation works, counts 2/2 on + real-shaped input. Not the documented BSD portability trap. +- **`grep -qx "=== READY"`** correctly rejects `=== READY (install stage only; …)`. + Verified the anchoring claim in the comment. +- **Coverage post-condition** (`:769-784`): `psql -c` returns **1** on a SQL error + (measured against the live fwapg), so a missing `${SCHEMA}.streams` takes the + `else` FATAL branch rather than producing an empty `$MISSING` false green. +- **Empty-bucket paths**: `study_area_wsgs.R:42` `stop()`s on an empty resolve, so + the `grep -v '^$'` pipelines at `:584` / `:731` / `:753` cannot be reached with + all-empty input under `pipefail`. Not a live abort. +- **`export FWAPG_GIT_SHA`** inside `preflight_local` reaches the parent shell — + the function is not a subshell and `||` does not make it one. Verified the + consumers at `:444` and `:676`. +- **R side**: all 90 preflight tests pass; all four new `@examples` blocks run + clean via `pkgload::run_example()`. `.lnk_fresh_required()` is drift-guarded by + `.lnk_fresh_callsites()` (namespace walk, includes `formals()`), the version + floor is derived from DESCRIPTION and cross-checked, `.lnk_vintage_primitives()` + is derived from `.lnk_input_primitives()`. Union-find in + `study_area_buckets.R:114-138` is correct (verified against a hand-built case), + and the host relabel at `:192-195` does put the heaviest bucket on host 1 + (traced with concrete loads). Disjointness is asserted, not assumed. + +## Findings + +### 1. [bug — data loss] `data-raw/study_area_run.sh:717-722` + +The new completeness gate `exit 1`s **before** consolidate while `CYPHERS_UP=1` +(set at `:604`), so the `trap burn_cyphers EXIT` at `:219` destroys every cypher +droplet. Every WSG the cyphers *successfully* modelled — sitting in their local +persists, not yet copied — goes with them. One failed WSG on any one host throws +away the whole paid run, unrecoverably. + +This is the accident the comment 60 lines above names verbatim (`:651-655`): + +> It must NEVER abort the host and trip the trap-burn before consolidate — that +> lost a whole run + the cyphers' data on 2026-05-25 (one species-less WSG -> +> exit 1 -> FATAL -> burn). + +The gate's own comment reasons only about the DELETE ("Refusing to consolidate — +consolidate DELETEs before it COPYs"), which is correct, and says nothing about +the burn that follows. Refusing is right; burning first is not. The operator's +only pre-emptive escape is having passed `--keep-cyphers` at launch. + +**Amplifier.** `done_n` comes from `grep`ping a literal log string produced by a +*different* file — `wsg_run_one.R:56` (`SKIP —`) and `:88` (`done in %.1f min`). +Nothing ties the regex to those `cat()` calls; no test crosses that seam. Reword +either message and `done_n` becomes 0 on **every** host simultaneously, which +routes straight through this gate to the burn. The failure direction of a +string-matching contract is normally "stop", which is fine — here "stop" means +"destroy the run's output". + +Failure scenario: 3 cyphers × 28 WSGs, one WSG on cy[job2] errors (a DB blip, a +guard error, anything the soft-fail was built to absorb). `check_bucket_complete` +reports 27/28, `complete_fail=1`, `exit 1`, trap fires, all three droplets +destroyed. 83 successfully-modelled WSGs lost; hours of paid compute; no recovery +path. + +Suggested fix, keeping the spend safety-net intact: consolidate the *successful* +subset — `schema_consolidate`'s DELETE and COPY are both scoped to the per-source +`bucket=` (`schema_consolidate.R:272-276`, `:313-316`), so narrowing each source's +bucket to the WSGs that actually reported `done`/`SKIP` consolidates what exists +without deleting anything for the missing ones — then fail loudly *after* the burn +with the incomplete WSGs named. Cheaper alternative: set `KEEP_CYPHERS=1` and +print the exact manual `schema_consolidate` invocation plus the IPs before +`exit 1` (accepting the idle spend as the price of not discarding the work). + +### 2. [security] `data-raw/cypher_prep.sh:156-174` + +The strip-and-append rewrite replaces `~/.Renviron` with `$RENV.tmp`, which is +created by a plain `>` redirect at the default umask — so the file's mode is not +preserved. Measured: + +``` +before: -rw------- .Renviron (PG_PASSWORD_SHARE=…) +after: -rw-r--r-- .Renviron (same contents + the three new keys) +``` + +`~/.Renviron` is exactly where this stack keeps DB credentials — CLAUDE.md +"Database Connection" documents `PG_*_SHARE` / `PG*` env vars as the connection +path, and this block's own header comment assumes "the image may keep unrelated +settings here". Every re-prep silently widens the mode; it never narrows back. +The intermediate `~/.Renviron.tmp` holds the full contents at the same widened +mode, at a predictable path, for the duration of the block. + +Low blast radius (single-user droplet, short-lived), but it is a credentials file +being world-readable by a script whose stated purpose is to preserve what is in +it. + +Fix — one of: + +```bash +umask 077 # around the block, restore after +# or +grep -vE '…' "$RENV" > "$RENV.tmp" || RC=$? +chmod 600 "$RENV.tmp" # before the mv +``` + +Better still, mirror the original: `chmod --reference` is GNU-only, so capture +`stat -f '%Lp'` (BSD) before and re-apply after the `mv`. + +## Adjacent, pre-existing (not introduced by this diff) + +`burn_cyphers` at `:212-214` fails toward "clean": + +```bash +if doctl compute droplet list --no-header 2>/dev/null | grep -qi cypher; then + echo " ✗ doctl still shows cypher droplets"; clean=0 +else echo " ✓ doctl: no cypher droplets"; fi +``` + +An expired/unreachable `doctl` produces empty output, `grep` exits 1, and the +`else` prints an affirmative "no cypher droplets" on the strength of a command +that never ran successfully — leaked droplets, reported clean. Unchanged by this +branch, but this PR's own pre-flight exists *because* both DO tokens expired on +2026-08-30, so the scenario is live, and finding 1 makes the burn path much more +reachable. Branch on `doctl`'s exit status separately from the grep. diff --git a/planning/active/review-round4.md b/planning/active/review-round4.md new file mode 100644 index 00000000..7b66dc48 --- /dev/null +++ b/planning/active/review-round4.md @@ -0,0 +1,238 @@ +# Code review — round 4 (#246 branch, narrow: commit `f146b2e` only) + +Scope as briefed: only round 3's three fixes and their immediate blast radius. +Not a re-review of the whole diff. + +- **A.** completeness gate no longer aborts; consolidate buckets narrowed + (`data-raw/study_area_run.sh:700-795`) +- **B.** `~/.Renviron` umask (`data-raw/cypher_prep.sh:158-187`) +- **C.** `burn_cyphers` doctl capture (`data-raw/study_area_run.sh:212-225`) + +Verdict: **B and C are sound. A is not** — the two guards it adds are +structurally unreachable, and the path that replaces them is the same +trap-burn data loss round 3 set out to remove. + +--- + +## Findings + +### 1. [BLOCKER — data loss] `data-raw/study_area_run.sh:767` + +`bucket_r=$(printf '%s' "${CY_BUCKET_DONE[$WS]}" | tr ',' '\n' | grep -v '^$' | ...)` + +When `CY_BUCKET_DONE[$WS]` is empty, `grep -v '^$'` selects no lines and exits +**1**. Under `set -o pipefail` the pipeline is non-zero, so the bare assignment +is non-zero, so `set -e` **aborts the script on this line** — one line *above* +the `if [ -z "$bucket_r" ]` branch written to handle exactly this case. + +The abort happens with `CYPHERS_UP=1` (set at `:615`), so `trap burn_cyphers +EXIT` at `:230` destroys every droplet, including the ones that succeeded and +whose rows have not been consolidated yet. That is verbatim the accident the +commit message says it removed. + +**Consequences, both structural:** + +- `:768-771` — the "reported no WSGs — skipping it in consolidate" branch + **can never execute**. The only way to have an empty `bucket_r` is the empty + `CY_BUCKET_DONE`, which aborts first. +- `:778-782` — the `n_src -eq 0` branch **can never execute** either. Reaching + `n_src == 0` with `N_CY > 0` requires the loop to `continue` for every host, + which requires reaching the dead branch above. So the answer to "is `n_src` + correct when every cypher is skipped?" is: the state is unreachable. Both new + guards are decoration in the CLAUDE.md sense — a guard that cannot fire. + +**The trigger is the motivating case, not a corner case.** An empty +`CY_BUCKET_DONE[$WS]` means "this host reported nothing", which is precisely +the 2026-05 failure the completeness accounting exists for, and the failure +`cypher_prep.sh`'s own header describes (fresh 0.31.0 → every WSG on the host +fails → host exits 0). It is also the degradation `bucket_done`'s comment at +`:718-720` deliberately routes a reworded `cat()` into. + +Reproduced against a faithful extract of `:717-795` (2 cyphers, job1 reports +both its WSGs, job2 reports none): + +``` +=== per-host completeness === + ok dispatcher: 1/1 + ok cy[job1]: 2/2 + X cy[job2]: only 0/2 WSGs accounted for (2 [WARN]) + WARN: consolidating only the WSGs each host reported +=== consolidate === +>>> BURN CYPHERS (trap EXIT) — droplets destroyed +SCRIPT EXIT=1 +``` + +job1's two successfully-modelled WSGs were never consolidated and the droplet +holding them is gone. Neither WARN branch printed. + +**Fix** (verified as valid bash) — test the source string, which is the +condition the branch is actually about, instead of inferring it from an exit +status that `pipefail` turns into an abort: + +```bash +bucket_r="" +if [ -n "${CY_BUCKET_DONE[$WS]}" ]; then + bucket_r=$(printf '%s' "${CY_BUCKET_DONE[$WS]}" | tr ',' '\n' \ + | grep -v '^$' | sed "s/.*/'&'/" | paste -sd, -) +fi +if [ -z "$bucket_r" ]; then + echo " WARN: cy[$WS] reported no WSGs — skipping it in consolidate" + continue +fi +``` + +A bare `|| bucket_r=""` also stops the abort but swallows a real pipeline +error into "reported nothing"; the guarded form keeps the two distinguishable. + +**Then restore the bug and confirm the guard fires.** Both branches have +existed since `f146b2e` without ever having been executed; feed the block a +cypher log containing only `[WARN]` lines and assert the WARN prints. + +--- + +### 2. [HIGH — data loss, same class, different line] `data-raw/study_area_run.sh:746` + +`CY_BUCKET_DONE[$WS]=$(bucket_done "$LOG_DIR/${TS}_run_$WS.log" | paste -sd, -)` + +`bucket_done` is `sed ... "$1" 2>/dev/null | sort -u`. `2>/dev/null` silences +sed's message but not its **exit status**, so an absent or unreadable log file +makes the pipeline non-zero under `pipefail`, the bare assignment non-zero, and +`set -e` aborts — again at `CYPHERS_UP=1`, again into the trap burn. Measured: + +| log file | `bucket_done \| paste` | script | +|---|---|---| +| missing | non-zero | **aborts, exit 1** | +| exists, no matching lines | 0, empty output | survives | +| exists, matches | 0 | survives | + +Lower likelihood than finding 1 — `:688`'s redirect creates the file for every +`WS` — but it is reachable on a failed redirect (full disk, unwritable +`LOG_DIR`) and the blast radius is identical. Note that the *empty-file* case, +which is the common one, is safe: sed and sort both exit 0. + +**Fix:** + +```bash +if ! CY_BUCKET_DONE[$WS]=$(bucket_done "$LOG_DIR/${TS}_run_$WS.log" | paste -sd, -); then + echo " WARN: could not read cy[$WS] run log — treating as reported nothing" + CY_BUCKET_DONE[$WS]="" +fi +``` + +--- + +### 3. [HIGH] `data-raw/study_area_run.sh:739-751` — `complete_fail` is computed and then discarded, and the named backstop cannot see the gap + +`complete_fail` is set at `:741`/`:745`, read once at `:748` to print a WARN, +and **never read again**. There is no non-zero exit anywhere after it. A run +that knowingly modelled 27 of 28 WSGs on a host prints `=== study_area_run +done ===` and exits **0**. + +The commit accepts that trade explicitly — "the gap is reported by the coverage +check after the cyphers are burned" — but the coverage check at `:816-832` +cannot report it in the normal case. It asserts only that each expected WSG has +**≥1 row** in `${SCHEMA}.streams`: + +```sql +LEFT JOIN (SELECT DISTINCT watershed_group_code w FROM ${SCHEMA}.streams) g +``` + +The persist accumulates across runs — nothing in `study_area_run.sh` calls +`lnk_persist_init`, and `schema_consolidate`'s DELETE is bucket-scoped, so a +WSG excluded from a narrowed bucket **keeps its rows from a previous run**. The +check goes green, the compare then runs against a mixture of this run's output +and an older run's, and the exit status says success. The backstop only works +on a first-ever run into an empty schema. + +Same hole covers the dispatcher question in the brief: the dispatcher writes +straight to the persist with no consolidate step, so a half-failed dispatcher +leaves the persist holding its successful WSGs plus stale rows for the failed +ones. `report_completeness "dispatcher"` detects it, sets `complete_fail`, and +the value is dropped. + +**Fix** — carry the flag to the exit status, after the burn and the coverage +check so it cannot leak spend or pre-empt the compare: + +```bash +[ "$complete_fail" = "0" ] || { + echo "FATAL: run incomplete — see the per-host completeness block above" + exit 1 +} +``` + +Strengthening the coverage check to assert *this run's* rows (join on +`.log`, or compare a timestamp) is the durable fix and a larger change; +the exit-status fix is what makes the current WARN honest in the meantime. + +--- + +## Verified sound — no change needed + +**The narrowing safety claim holds at the scope level.** Checked +`data-raw/schema_consolidate.R` as briefed: + +- DELETE (`:272-276`) iterates `wgc_tables` with `wsg_list_sql`; COPY + (`:312-316`) iterates the **same** `wgc_tables` with the **same** + `wsg_list_sql`. No table where the two scopes differ. +- `wgc_tables = intersect(src_wgc, dest_wgc)` (`:192`), so `skipped_dest_only` + tables are correctly excluded from the DELETE as well as the COPY — they are + not emptied and left unrepopulated. +- `keep_source = FALSE` post-COPY source DELETE (`:423-456`) uses + `copied_tables` ⊆ `wgc_tables` and the same `wsg_list_sql`, so a narrowed + bucket removes from the source exactly what was transferred. Un-reported + WSGs' rows stay on the source (and are then burned, which is correct — they + were never copied). +- Residual, **pre-existing and not worsened by the narrowing**: a per-table + COPY failure (`:338-347`, `:357-365`) does `next` after the DELETE already + ran, so that one table loses the bucket's destination rows without + replacement. Narrowing strictly reduces the set exposed to this. + +**`CY_BUCKET_DONE` reachability and `set -u`.** `declare -A` at `:742` is at +top level in the main shell, not inside a function, so it is the same global +the consolidate loop reads at `:767`. Every key read there is written by the +`:743-747` loop over the same `CY_WS_ARR`, so there is no unset-key `set -u` +hazard. The whole block is inside `if [ "$N_CY" -gt 0 ]`, so `N_CY == 0` never +reaches it. + +**`report_completeness` returning non-zero.** Called only as +`report_completeness ... || complete_fail=1`, which suspends errexit for the +whole function body, and each of its three `grep -c` calls additionally carries +`|| var=0`. `grep -c` prints `0` and exits 1 on no match; the `|| var=0` lands +the same value. No abort, correct counts. + +**`bucket_done`'s regex.** `([A-Z]{4})` matches all 246 watershed group codes +in `fresh`'s `wsg_outlet.csv` — checked, zero non-4-alpha codes. Matches both +`wsg_run_one.R:56` (`SKIP —`) and `:88` (`done in %.1f min`). Anchoring on the +code rather than the prose is the right call. + +**B — `cypher_prep.sh:158-187` umask.** Correct. +- `RENV_UMASK=$(umask)` prints octal (`0022`); `umask "$RENV_UMASK"` accepts it. +- The `exit 1` at `:178` between set and restore is harmless — umask is + per-process and the process is ending. +- The window is `:163-187` only; `snapshot_bcfp.sh` and `lnk_persist_init` run + after the restore at `:187`, so nothing downstream inherits `077`. +- `mv` preserves the tmp's `0600`, and the explicit `chmod 600` covers the + append. The pre-`umask` `touch` at `:157` can create a `0644` file, but only + an empty one, and the `mv` replaces it before anything is written. + +**C — `burn_cyphers` doctl capture.** Correct. +- `local rc=$?` is still the first statement in the function; `local dl` is + declared at `:216`, well after `rc` is read, so `$?` is not clobbered. +- `local dl` on its own line then `if dl=$(...)` is the right split — + `local dl=$(...)` would return `local`'s status and re-introduce the bug. +- Under the EXIT trap with `set -e`, an `if` condition suspends errexit, so a + failing `doctl` takes the else branch rather than aborting the trap. +- Empty `$dl` on the success path correctly reads as "no cypher droplets"; + a `doctl` failure is now a third, distinct outcome that sets `clean=0`. +- `return $rc` preserved. + +--- + +## Summary + +Finding 1 is the same shape as rounds 2 and 3: the fix contains a defect of the +class it was written to remove. Rounds 1→2→3 each landed a blocker inside the +previous round's fix, and round 3's two new guards are unreachable, so this is +the third consecutive instance and **not convergence**. Recommend a round 5 +scoped to the round-4 fixes alone, with the restore-the-bug check run against +both new branches before it is called clean. diff --git a/planning/active/review-round5.md b/planning/active/review-round5.md new file mode 100644 index 00000000..14f9424a --- /dev/null +++ b/planning/active/review-round5.md @@ -0,0 +1,142 @@ +# Code review — round 5 (#246 branch, narrow: commit `13051e2` only) + +Scope as briefed: round 4's five changes and their immediate blast radius in +`data-raw/study_area_run.sh`. Not a re-review of the rest of the diff. + +Every claim below was **executed**, not read. Test harnesses were faithful +extracts of `:717-912` run under `set -euo pipefail` on the same bash the +script uses. + +## Verdict: Clean + +Round 4 is the first round that does not contain a defect of the class it was +written to remove. All five changes are sound and all five are load-bearing — +each was verified by restoring the bug and confirming the failure returns. + +--- + +## Restore-the-bug: every fix is load-bearing, not decoration + +| fix | reverted form | round 4 form | +|---|---|---| +| `csv_lines` on an empty bucket | `grep -v '^$'` → **exit 1, script aborts, guard unreachable** | exit 0, `WARN: cy[job2] reported no WSGs — skipping` **prints** | +| `bucket_done` on a missing log | `sed … 2>/dev/null \| sort -u` → **exit 1, script aborts** | exit 0, bucket `[]` | +| `printf '%s\n'` | `printf '%s'` on 2 items → `wc -l` = **1** | `wc -l` = **2** | + +The first two both aborted with `CYPHERS_UP=1` in round 3, i.e. into the +trap-burn. Both now reach their guards. + +## `csv_lines` / `csv_count` — correct across 16 inputs + +``` +empty string count=0 only blanks ",,," count=0 only spaces " " count=0 +single "ADMS" count=1 trailing comma count=2 leading comma count=1 +two count=2 embedded blank count=2 +value with space count=2 glob char "*" count=2 (literal `*`, no expansion) +single quote count=2 double quote count=2 +"-n" / "-e" count=2 (operands, not printf options — format is arg 1) +backslash 'A\nB' count=2 (literal — %s does not interpret escapes) +csv_count with NO argument -> 0 (`${1:-}` covers set -u) +csv_count "${assoc[missing]:-}" -> 0 +``` + +No word-splitting or globbing hazard: `printf '%s\n' "${1:-}"` is fully +quoted, and the format string is a literal so a value beginning `-` cannot be +read as an option. `sed '/^[[:space:]]*$/d'` deletes only all-blank lines — it +does not trim interior whitespace, so `"ADMS , BULK"` yields `"ADMS "` and +`" BULK"`. Not reachable: `CY_BUCKET_DONE` comes from `bucket_done`'s +`([A-Z]{4})` capture, and `CY_BUCKET` is `tr -d '[:space:]'`-ed at `:581`/`:587`. + +## No remaining pipeline in the completeness/consolidate path aborts + +Measured, all under `set -euo pipefail`, all `rc=0`: + +| pipeline | full | empty file | no matching lines | no trailing newline | missing file | unreadable (mode 000) | +|---|---|---|---|---|---|---| +| `bucket_done \| paste -sd, -` (`:748`, `:766`) | 0 | 0 | 0 | 0 | **0** | **0** | +| `csv_lines \| sed \| paste` (`:787`) | 0 | 0 | 0 | — | — | — | +| `grep -c '^\[WARN\] '` (`:749`) | 0 | 0 | 0 | — | 0 | 0 | + +- `paste -sd, -` on empty input emits a bare `\n` and exits 0; `$( )` strips it + to `""`, which `csv_count` reports as 0. `sort -u` on empty input emits + nothing and exits 0. +- `:766` is the one call **not** wrapped in `||`, so it is the one that had to + be made exit-0 in its own right. It is — via `[ -r "$1" ]`. +- `grep -c` exits 1 on zero matches but is caught by `|| warn_n=0`, and prints + `0` first so the value is right either way. + +## `complete_fail` / `RUN_INCOMPLETE` + +`complete_fail` is a top-level global (`:759`), never `local`, so it is in +scope at `:861`. `report_completeness` is called only as `f || complete_fail=1`, +which suspends errexit for the whole function body — confirmed by the `nologs` +scenario, where all three `csv_count` calls run against absent files and the +script survives. + +Five scenarios, end to end: + +| scenario | per-host | `complete_fail` | consolidate | post-burn `CYPHERS_UP` | trap | exit | +|---|---|---|---|---|---|---| +| all hosts complete | 2/2 ×3 | 0 | both cyphers | 0 | no-op | **0** | +| one cypher reported nothing | 0/2 on job2 | 1 | job1 only, job2 skipped **loudly** | 0 | no-op | **1** | +| every host reported nothing | 0/2 ×3 | 1 | `✗ no cypher reported any WSG` | 0 | no-op | **1** | +| no log files at all | 0/2 ×3 | 1 | same | 0 | no-op | **1** | +| dispatcher failed, cyphers fine | 0/2 disp | 1 | both cyphers | 0 | no-op | **1** | + +`RUN_INCOMPLETE` is reachable: `:861` is unconditional in the main flow, and +every `exit` between it and `:905` (`:891` compare) is `exit 1`. There is no +`exit 0` and no early return in that range, so `${RUN_INCOMPLETE:-0}` cannot +mask anything — the `:-0` is redundant belt-and-braces, not a hole. The +coverage check at `:848`/`:851` can `exit 1` first; that is correct and still +non-zero. + +`N_CY = 0` case: `:861` and `:905` are outside the `if [ "$N_CY" -gt 0 ]` +block, so a dispatcher-only run with a partial bucket still exits 1. + +## Trap interaction with the new `exit 1` + +At `:905`, `CYPHERS_UP` is 0 — set by `burn_cyphers`'s own `:227` during the +explicit `burn_cyphers || true` at `:818`. Verified in all five scenarios: the +trap re-entry prints only `trap: CYPHERS_UP=0, no-op (rc=1)` and **does not +re-burn**. `exit 1` survives the trap's `return $rc` — `SCRIPT EXIT=1`. + +`--keep-cyphers` leaves `CYPHERS_UP=1` (the early return at `:196` skips +`:227`), so the trap prints `=== trap EXIT: --keep-cyphers; NOT burning ===` a +second time. Verified it does **not** burn, and the exit status is still 1. +Cosmetic duplicate line, pre-existing shape, no behavioural consequence. + +--- + +## Notes — not findings + +**Two `grep -v '^$'` instances remain un-swept** (`:597` DUP, `:823` +`ALL_WSGS`). Round 4's stated aim was to put the safe form in one helper +rather than remember it per call site, and these two were not converted. +Both are **unreachable today**, proven rather than assumed: + +- Each aborts only if *every* bucket string is blank. +- `study_area_wsgs.R:44` `stop()`s when the resolve returns zero WSGs, so the + bare assignments at `:580`/`:586` abort first under `set -e` — `DISP_BUCKET` + can never be empty. +- `:597` and `:823` read the *same* input set, so `:823` additionally cannot + fire where `:597` did not, and `:597` runs pre-spin (no spend, no data). + +Confirmed the shape does abort if the premise is removed (`DISP_BUCKET=""` → +exit 1 before the next line). Worth converting for uniformity, but it is not a +live failure and changing it now would widen a round scoped to converge. + +**`exp_n` counts raw CSV items, `got_n` counts `sort -u` output.** A duplicate +WSG inside a single host's bucket would read as incomplete. Not reachable +(`frs_wsg_drainage` returns a closure set) and not a round-4 change — round 3's +`grep -c '[^[:space:]]'` had the identical asymmetry. + +--- + +## Summary + +Rounds 1→4 each landed a blocker inside the previous round's fix. Round 4 does +not. The centralised helper is correct on every input tried including the +adversarial ones, the two abort paths that caused the round-3 trap-burn are +closed and their guards now demonstrably execute, and `RUN_INCOMPLETE` reaches +the end of the script and produces a non-zero exit in every incomplete +scenario without re-triggering the burn. This is convergence. diff --git a/planning/active/task_plan.md b/planning/active/task_plan.md new file mode 100644 index 00000000..b7c6f1d0 --- /dev/null +++ b/planning/active/task_plan.md @@ -0,0 +1,81 @@ +# Task: Provenanced rerun of all WSGs — pre-flight gates (#246), Phases 1–2 + +`fresh` holds 93 WSGs / 2,865,775 rows accumulated 2026-05 → 2026-08 across +several link and fresh versions, with **no `log` / `log_input` tables at all**. +Not one row is traceable to a run, a config hash, or a date. Before that can be +rebuilt cleanly, three defects have to close — a run started today would +**silently skip 80 of 119 WSGs and exit 0**. + +This branch is **Phases 1–2 of the issue plus the bucketing derivation**. The +destructive multi-host wipe, the paid 4-host run and the provenance audit are a +separate session driven by the merged, hardened script. + +## Phase 1 — make fresh a real dependency + +- [x] `DESCRIPTION`: move `fresh (>= 0.33.0)` Suggests → Imports (keep the + `Remotes: NewGraphEnvironment/fresh@v0.33.0` pin) +- [x] `R/lnk_preflight_fresh.R` — exported `lnk_preflight_fresh()`, internal + `.lnk_fresh_required()` (curated symbol list) and `.lnk_fresh_floor()` + (parses the floor out of link's own DESCRIPTION, single-sourced) +- [x] `tests/testthat/test-lnk_preflight_fresh.R` — both known answers +- [x] `cypher_prep.sh`: `CYPHER_PREP_FRESH_REF` override + export assertion + replacing the assert-nothing echo at line 66, using the file's own + tempfile + `if !` idiom (never `$( )`, which discards exit status) +- [x] `cypher_prep.sh`: `CYPHER_PREP_STAGE=install|all` so `--auto-install` + costs ~3 min not ~20 +- [x] `cypher_prep.sh`: write `~/.Renviron` with the cypher's **own** observed + `LINK_GIT_SHA` / `LINK_GIT_DIRTY` / `FRESH_GIT_SHA` +- [x] `cypher_prep.sh`: header rewrite (the baked-fresh assumption is what + broke) + fix stale `dispatch_provincial.sh` ref at line 41 +- [x] **Sentinel fix** — `study_area_run.sh:203` and `wsgs_run_pipeline.sh:267` + grep `=== READY`, not `snapshot_bcfp.sh: complete` + +## Phase 2 — pre-flight gates in `study_area_run.sh` + +### 2a `preflight_local()` — pre-spin, zero cost +- [x] Gate 5: persist-schema guard (arg-time + resolved-collision second layer) +- [x] Gate 4: branch pushed (fetch first) + worktree clean +- [x] Gate 6: resolve and export `FWAPG_GIT_SHA`; fail when unresolvable +- [x] Gate 2: credential probe — doctl leg, tfvars `do_token` leg via direct + DO API call, s3 backend leg labelled honestly +- [x] `R/lnk_preflight_vintage.R` + `data-raw/host_vintage.R` + tests +- [x] Gate 3a: dispatcher primitive vintage, `--vintage-max-days=N` (default 7) + +### 2b `preflight_hosts()` — post-prep, pre-write +- [x] `R/lnk_preflight_stamp.R` + `R/lnk_preflight_parity.R` + + `data-raw/host_stamp.R` + tests +- [x] Gate 1: cross-host parity keyed on `repo_sha` (**not** `link_sha` — that + is NA on every pak-installed cypher by construction) +- [x] Gate 3b: cypher primitive vintage +- [x] `--auto-install` remediation (re-run install stage, re-check once) +- [x] Export `FWAPG_GIT_SHA` on the run ssh leg (line 226) + +### 2c post-condition + ergonomics +- [x] Gate 7: per-host completeness count before consolidate +- [x] Coverage assertion after burn, before recompute +- [x] `--preflight-only` +- [x] `--refresh-primitives` (must land with gate 3a — m1 is 3 months stale) + +## Phase 3 — bucketing derivation + +- [x] `data-raw/study_area_buckets.R` — union-find over `frs_wsg_drainage()` + closures, LPT-pack components into N hosts, emit `--focal=` strings +- [x] Rewrite `research/study_areas.md` from the script's own output +- [x] Verify against the issue's asserted 22 components / 119 modelable + +## Phase 4 — testing, docs, release + +- [x] Both-known-answers run for every gate, recorded +- [x] `devtools::test()`, `devtools::document()`, `lintr::lint_package()` +- [x] RUNBOOK section; `NEWS.md`; version bump as the final commit +- [x] Edit issue #246 body (three corrections — see findings.md) +- [x] File follow-up #247 (`fresh.snapshot_stamp`). The `fresh::` call-site + lint was not deferred after all — it shipped as `.lnk_fresh_callsites()` + plus its drift test. doctl/tofu account-UUID match drafted, not filed. + +## Validation + +- [x] Tests pass (FAIL 0 | PASS 1603; R CMD check 0 errors) +- [x] `/code-check` — 5 rounds over the branch diff (not per commit; see progress.md). Round 5 clean. +- [x] PWF checkboxes match landed work +- [ ] `/planning-archive` on completion diff --git a/research/study_areas.md b/research/study_areas.md index 2f65e600..9ab31534 100644 --- a/research/study_areas.md +++ b/research/study_areas.md @@ -1,34 +1,100 @@ -# FWCP study areas — Peace / Fraser / Skeena - -The 3 FWCP regions and their watershed groups — the parity-run scope for link -(link#175). Authoritative source = the `wsg_code` / `wsg` param in each -`fish_passage_*_reporting` repo (`index.Rmd` for Peace/Fraser, -`scripts/02_reporting/0160-load-bcfishpass-data.R` for Skeena). - -## Focal watershed groups - -- **Peace** — `NewGraphEnvironment/fish_passage_peace_2025_reporting` - (`index.Rmd` `wsg_code`, 16): CARP, CRKD, FINA, FINL, FIRE, FOXR, INGR, LOMI, - MESI, NATR, OSPK, PARA, PARS, PCEA, TOOD, UOMI -- **Fraser** — `NewGraphEnvironment/fish_passage_fraser_2025_reporting` - (`index.Rmd` `wsg_code`, 8): LCHL, NECR, FRAN, MORK, UFRA, WILL, TABR, LSAL -- **Skeena** — `NewGraphEnvironment/fish_passage_skeena_2024_reporting` - (`0160-load-bcfishpass-data.R` `wsg <- c(...)`, 5): BULK, MORR, ZYMO, KISP, KLUM - -## Drainage closure - -29 focal WSGs → **~52 with downstream-closure** (every WSG each drains through — -e.g. PARS → PCEA / UPCE / LPCE / FINA / PARA / LBTN). Closure + downstream-first -order come from `lnk_wsg_resolve()`, which delegates to `fresh::frs_wsg_drainage()` -(fresh >= 0.33.0). **Do not hand-roll this from `wscode_ltree` ancestry (`@>`)** — -that was the pre-#227 method and it over-includes: two WSGs on the same stream share -an outlet code, so `@>` calls each downstream of the other. The closure is -measure-aware, not code-aware. See RUNBOOK section 8b. Major drainages by root -wscode: Fraser `100` (68 WSGs), Peace `200` (65), Columbia `300`/ELKR (17 — NOT a -study area), Skeena `400` (12). - -**Why closure matters:** mapping_code `;DAM` is cross-WSG — a headwater WSG only -emits `;DAM` once its downstream dam-bearing WSGs are persisted. So a study-area -parity run must be drainage-closed downstream **and** DS-first ordered (or -two-passed). This is the crux of #175's orchestrator work; run procedure in -`research/study_area_run.md`. +# Study areas — drainage-independent components and host buckets + + + +Generated from 96 focal watershed groups, config `bcfishpass`, 4 hosts. + +## Why components, not study areas + +Cross-WSG `;DAM` needs each host's bucket drainage-closed and run +downstream-first. Where two hosts' closures overlap, the shared WSGs are +modelled twice and `schema_consolidate` resolves the collision +last-writer-wins — so whichever host finished last silently decides the +answer. Partitioning into components whose closures are disjoint removes +that ambiguity by construction, and the derivation asserts disjointness +rather than assuming it. + +Partitioning by `wscode_ltree` root does **not** work: it reproduces the +sliver misclassification RUNBOOK section 8b documents — NATR filed under +Fraser though it drains to the Peace, SPAT under Skeena though it drains +the Stikine. Closure is measure-aware; a wscode root is not. + +## Host buckets + +| host | components | focal | modelable | weight (segments) | +|---|---|---|---|---| +| dispatcher (m1) | 1 | 33 | 39 | 696,262 | +| job1 | 8 | 15 | 27 | 626,778 | +| job2 | 6 | 25 | 26 | 617,223 | +| job3 | 7 | 23 | 27 | 616,960 | + +96 focal WSGs resolve to 22 components and 119 modelable WSGs. + +Components are indivisible, so the packing is a greedy LPT over +component weights, not over WSGs. Weight is stream-segment count from +`fwa_stream_networks_sp`, which tracks modelling work far better than a +WSG count does — a one-WSG component can outweigh a three-WSG one. The +dispatcher is relabelled to whichever host draws the heaviest load, +since it is the free, fast local machine while the cyphers are paid. + +The component decomposition is a property of the drainage network and is +stable; the host assignment is only as stable as the weights, so expect +it to shift if the network table is reloaded. + +## `--focal=` strings + +``` +bash data-raw/study_area_run.sh \ + --cy-workspaces=job1,job2,job3 \ + --focal=BBAR,BONP,CARR,CHES,CHIR,CHWK,COTR,DOGC,FRAN,FRCN,HARR,LCHL,LCHR,LFRA,LILL,LNTH,LSAL,MFRA,MORK,NARC,NECR,NICL,QUES,SAJR,SALR,SETN,TABR,TAKL,TWAC,UFRA,UNRS,UTRE,WILL \ + --focal=ALBN,COAL,FONT,GATA,KETL,LPRO,LSTR,MDEA,MSTR,SHER,SPAT,TSIT,UJER,UKEC,UNUR \ + --focal=BRKS,CARP,CRKD,FINA,FINL,FIRE,FOXR,GOLD,INGR,KITL,KITR,LBTN,LOMI,LPCE,MESI,NATR,OSPK,PARA,PARS,PCEA,TATR,TOOD,UBTN,UOMI,UPCE \ + --focal=BULK,CLRH,COWN,DUNC,ELKR,HOMA,KISP,KLUM,KOTL,KOTR,KTSU,LARL,LDEN,LKEL,LSKE,MORR,MSKE,NASR,SLOC,SMAR,SUST,USKE,ZYMO \ + --config=bcfishpass +``` + +## Downstream-first order per host + +**dispatcher (m1)** (39 WSGs) + + LFRA, HARR, CHWK, FRCN, LILL, SETN, THOM, BBAR, BONP, LNIC, LNTH, DOGC, NICL, MFRA, LCHR, TWAC, CHIR, NARC, SAJR, COTR, QUES, CARR, TABR, LCHL, LSAL, MORK, WILL, NECR, SALR, STUR, UFRA, CHES, FRAN, STUL, LTRE, UTRE, MIDR, UNRS, TAKL + +**job1** (27 WSGs) + + DUNE, BEAV, LIAR, COAL, LMUS, FONT, LPRO, ULRD, DEAR, MDEA, UKEC, FROG, GATA, LSTR, STIR, MSTR, USTK, SPAT, ALBN, KETL, INKR, SHER, TSIT, TESR, JENR, UJER, UNUR + +**job2** (26 WSGs) + + LPCE, LBTN, UPCE, PCEA, UBTN, PARA, CARP, FINA, NATR, PARS, CRKD, FINL, INGR, LOMI, MESI, OSPK, TOOD, UOMI, FIRE, FOXR, BRKS, GOLD, TSAY, KITL, KITR, TATR + +**job3** (27 WSGs) + + LSKE, KLUM, LKEL, KISP, ZYMO, BULK, MSKE, MORR, SUST, USKE, LARL, KOTL, SLOC, UARL, BULL, DUNC, REVL, CLRH, ELKR, SMAR, KOTR, COWN, HOMA, KTSU, LDEN, LNAR, NASR + +## Components + +| # | focal | modelable | host | focal WSGs | +|---|---|---|---|---| +| 1 | 33 | 39 | dispatcher (m1) | BBAR BONP CARR CHES CHIR CHWK COTR DOGC FRAN FRCN HARR LCHL LCHR LFRA LILL LNTH LSAL MFRA MORK NARC NECR NICL QUES SAJR SALR SETN TABR TAKL TWAC UFRA UNRS UTRE WILL | +| 2 | 20 | 20 | job2 | CARP CRKD FINA FINL FIRE FOXR INGR LBTN LOMI LPCE MESI NATR OSPK PARA PARS PCEA TOOD UBTN UOMI UPCE | +| 3 | 10 | 10 | job3 | BULK KISP KLUM LKEL LSKE MORR MSKE SUST USKE ZYMO | +| 4 | 8 | 11 | job3 | CLRH DUNC ELKR KOTL KOTR LARL SLOC SMAR | +| 5 | 6 | 13 | job1 | COAL FONT GATA LPRO MDEA UKEC | +| 6 | 3 | 5 | job1 | LSTR MSTR SPAT | +| 7 | 1 | 1 | job1 | ALBN | +| 8 | 1 | 1 | job2 | BRKS | +| 9 | 1 | 1 | job3 | COWN | +| 10 | 1 | 1 | job2 | GOLD | +| 11 | 1 | 1 | job3 | HOMA | +| 12 | 1 | 1 | job1 | KETL | +| 13 | 1 | 2 | job2 | KITL | +| 14 | 1 | 1 | job2 | KITR | +| 15 | 1 | 1 | job3 | KTSU | +| 16 | 1 | 1 | job3 | LDEN | +| 17 | 1 | 2 | job3 | NASR | +| 18 | 1 | 2 | job1 | SHER | +| 19 | 1 | 1 | job2 | TATR | +| 20 | 1 | 1 | job1 | TSIT | +| 21 | 1 | 3 | job1 | UJER | +| 22 | 1 | 1 | job1 | UNUR | diff --git a/tests/testthat/test-lnk_preflight_fresh.R b/tests/testthat/test-lnk_preflight_fresh.R new file mode 100644 index 00000000..fa979e39 --- /dev/null +++ b/tests/testthat/test-lnk_preflight_fresh.R @@ -0,0 +1,110 @@ +test_that("lnk_preflight_fresh passes against the real required set", { + skip_if_not_installed("fresh") + res <- lnk_preflight_fresh(quiet = TRUE) + expect_true(res$ok) + expect_length(res$missing, 0L) + expect_length(res$missing_internal, 0L) + expect_true(res$version_ok) +}) + +test_that("lnk_preflight_fresh fails when a required export is absent", { + skip_if_not_installed("fresh") + # No mocking needed: this name is absent by construction. The premise is + # asserted rather than assumed, so a future fresh that DID export it fails + # here — naming the stale fixture — instead of on the behaviour assertion. + absent <- "frs_definitely_not_exported_by_fresh" + expect_false(absent %in% getNamespaceExports(asNamespace("fresh"))) + + res <- lnk_preflight_fresh(required = c("frs_wsg_outlets", absent), + quiet = TRUE) + expect_false(res$ok) + expect_identical(res$missing, absent) + expect_match(res$message, absent, fixed = TRUE) +}) + +test_that("lnk_preflight_fresh fails when a required internal is absent", { + skip_if_not_installed("fresh") + res <- lnk_preflight_fresh(required_internal = ".frs_not_an_internal", + quiet = TRUE) + expect_false(res$ok) + expect_identical(res$missing_internal, ".frs_not_an_internal") +}) + +test_that("lnk_preflight_fresh fails a version floor above the installed", { + skip_if_not_installed("fresh") + res <- lnk_preflight_fresh(min_version = "999.0.0", quiet = TRUE) + expect_false(res$ok) + expect_false(res$version_ok) +}) + +test_that("lnk_preflight_fresh reports rather than errors when fresh is absent", { + # The absent-package branch must return ok = FALSE, not throw — the shell + # caller needs to distinguish "assertion failed" from "R itself blew up". + # + # Mocked at link's own `.lnk_fresh_ns()`, not at `base::asNamespace`: + # mocking base breaks every other namespace lookup in the file, including + # testthat's own, and turns this into an error rather than a result. + local_mocked_bindings( + .lnk_fresh_ns = function() NULL, + .lnk_pkg_version_or_na = function(pkg) NA_character_) + res <- lnk_preflight_fresh(quiet = TRUE) + expect_false(res$ok) + expect_true(is.na(res$version)) + expect_setequal(res$missing, .lnk_fresh_required()) + expect_match(res$message, "not installed") +}) + +test_that("the required set names only symbols fresh actually exports", { + # Guards a typo in the curated list itself, which would otherwise make the + # check fail on every host for a reason that has nothing to do with fresh. + skip_if_not_installed("fresh") + expect_length( + setdiff(.lnk_fresh_required(), getNamespaceExports(asNamespace("fresh"))), + 0L) + expect_true(exists(.lnk_fresh_required_internal(), + envir = asNamespace("fresh"), inherits = FALSE)) +}) + +test_that("every fresh:: call site in link is declared as required", { + # The drift guard. A PR that adds a `fresh::frs_new_thing()` call without + # declaring it would otherwise ship a check that passes while the pipeline + # breaks on any host with an older fresh. + # + # `.lnk_fresh_callsites()` walks link's own namespace, so it measures the + # code that will actually run rather than a source directory that does not + # exist in an installed package. + callsites <- .lnk_fresh_callsites() + expect_gt(length(callsites), 0L) # an empty scan is not a pass + expect_length(setdiff(callsites, .lnk_fresh_required()), 0L) +}) + +test_that(".lnk_fresh_callsites finds a symbol used only as a default argument", { + # lnk_wsg_downstream_check(outlets = fresh::frs_wsg_outlets()) is reachable + # only through formals(), and it is one of the two symbols link#246 is about. + # A walker that read bodies alone would miss it and report a clean scan. + expect_true("frs_wsg_outlets" %in% .lnk_fresh_callsites()) +}) + +test_that(".lnk_fresh_floor parses the pin out of DESCRIPTION", { + expect_identical( + .lnk_fresh_floor(list(Imports = "DBI, fresh (>= 0.33.0), yaml")), + "0.33.0") + expect_identical( + .lnk_fresh_floor(list(Imports = "DBI, fresh(>=1.2.3)")), + "1.2.3") + expect_identical(.lnk_fresh_floor(list(Imports = "DBI, yaml")), "0.0.0") + expect_identical(.lnk_fresh_floor(list()), "0.0.0") +}) + +test_that("link's real DESCRIPTION declares a fresh floor", { + # If fresh is ever moved back to Suggests or the floor dropped, this fires. + # The floor is what forces pak to resolve the Remotes pin on the cyphers. + expect_true(utils::compareVersion(.lnk_fresh_floor(), "0.33.0") >= 0L) +}) + +test_that("lnk_preflight_fresh validates its arguments", { + expect_error(lnk_preflight_fresh(required = character(0))) + expect_error(lnk_preflight_fresh(required = "")) + expect_error(lnk_preflight_fresh(min_version = "")) + expect_error(lnk_preflight_fresh(quiet = NA)) +}) diff --git a/tests/testthat/test-lnk_preflight_parity.R b/tests/testthat/test-lnk_preflight_parity.R new file mode 100644 index 00000000..d50d10fd --- /dev/null +++ b/tests/testthat/test-lnk_preflight_parity.R @@ -0,0 +1,172 @@ +row <- function(host, ...) { + d <- list(host = host, link_version = "0.46.0", link_sha = "NA", + fresh_version = "0.33.0", fresh_sha = "NA", + repo_sha = "abc123def456", repo_dirty = "FALSE", + config_hash = "cfg012345678", fwapg_sha = "e6e1eb0aaaaa", + r_version = "4.5.2") + as.data.frame(utils::modifyList(d, list(...)), stringsAsFactors = FALSE) +} + + +test_that("parity passes when all hosts agree", { + s <- rbind(row("m1"), row("cy-job1"), row("cy-job2")) + res <- lnk_preflight_parity(s, n_expected = 3, quiet = TRUE) + expect_true(res$ok) + expect_identical(res$n, 3L) + expect_identical(res$reference, "m1") +}) + +test_that("parity does NOT over-fire on link_sha, which is NA on installed hosts", { + # The trap this design exists to avoid: link_sha is a real SHA on a + # load_all dispatcher and NA on every pak-installed cypher. Comparing it + # would fail every legitimate run, so it is reported and never keyed on. + s <- rbind(row("m1", link_sha = "deadbeef1234"), row("cy-job1")) + expect_true(lnk_preflight_parity(s, n_expected = 2, quiet = TRUE)$ok) +}) + +test_that("parity does not key on fresh_sha either", { + # And the mirror: keying on it would be a vacuous NA == NA pass. + s <- rbind(row("m1"), row("cy-job1", fresh_sha = "abc999888777")) + expect_true(lnk_preflight_parity(s, n_expected = 2, quiet = TRUE)$ok) +}) + +test_that("parity fails on a fresh_version mismatch and names the host", { + # The literal #246 scenario: a cypher still on the image's fresh. + s <- rbind(row("m1"), row("cy-job1", fresh_version = "0.31.0")) + res <- lnk_preflight_parity(s, n_expected = 2, quiet = TRUE) + expect_false(res$ok) + expect_identical(res$offenders, "cy-job1") + expect_true("fresh_version" %in% res$mismatches$field) + expect_match(res$message, "0.31.0", fixed = TRUE) +}) + +test_that("parity fails on a repo_sha mismatch", { + s <- rbind(row("m1"), row("cy-job1", repo_sha = "000000000000")) + expect_false(lnk_preflight_parity(s, n_expected = 2, quiet = TRUE)$ok) +}) + +test_that("parity fails on a config_hash mismatch", { + s <- rbind(row("m1"), row("cy-job1", config_hash = "cfgXXXXXXXXX")) + expect_false(lnk_preflight_parity(s, n_expected = 2, quiet = TRUE)$ok) +}) + +test_that("parity fails when a host did not report - a short table is not agreement", { + s <- rbind(row("m1"), row("cy-job1")) + res <- lnk_preflight_parity(s, n_expected = 3, quiet = TRUE) + expect_false(res$ok) + expect_match(res$message, "did not report") +}) + +test_that("parity fails on ZERO rows", { + # A loop that collected nothing must not produce "no mismatches found". + res <- lnk_preflight_parity(row("m1")[0, ], n_expected = 3, quiet = TRUE) + expect_false(res$ok) +}) + +test_that("parity fails on an unresolved fwapg_sha - NA == NA is not agreement", { + s <- rbind(row("m1", fwapg_sha = "NA"), row("cy-job1", fwapg_sha = "NA")) + res <- lnk_preflight_parity(s, n_expected = 2, quiet = TRUE) + expect_false(res$ok) + expect_match(res$message, "fwapg_sha unresolved") +}) + +test_that("parity fails on an empty-string field, not just the literal NA", { + s <- rbind(row("m1", repo_sha = ""), row("cy-job1")) + expect_false(lnk_preflight_parity(s, n_expected = 2, quiet = TRUE)$ok) +}) + +test_that("parity fails when any host is dirty", { + s <- rbind(row("m1", repo_dirty = "TRUE"), row("cy-job1")) + res <- lnk_preflight_parity(s, n_expected = 2, quiet = TRUE) + expect_false(res$ok) + expect_match(res$message, "dirty checkout") +}) + +test_that("parity errors loudly on a malformed stamps table", { + expect_error(lnk_preflight_parity(data.frame(host = "m1"), n_expected = 1), + "missing column") +}) + +test_that("parity requires n_expected - it has no default", { + expect_error(lnk_preflight_parity(row("m1"))) +}) + +test_that("a single-host run with n_expected = 1 passes", { + expect_true(lnk_preflight_parity(row("m1"), n_expected = 1, quiet = TRUE)$ok) +}) + + +test_that("lnk_preflight_stamp returns the documented field order", { + # An honest scope note: this asserts the stamp matches its own declared + # contract. It does NOT prove the shell agrees, and an earlier version of + # this comment claimed it did while comparing R against R. The shell now + # calls .lnk_preflight_stamp_cols() directly for col.names, so there is no + # second list to diverge from - the duplication was removed rather than + # tested around. + s <- lnk_preflight_stamp(lnk_config("bcfishpass")) + expect_identical(names(s), .lnk_preflight_stamp_cols()) + expect_true(all(nzchar(s))) # never empty; the literal "NA" instead + expect_type(s, "character") +}) + +test_that("a stamp is judgeable by the parity function it feeds", { + # Guards the seam: the collector's output must satisfy the judge's + # column contract, or the gate errors at run time on a live host. + s <- lnk_preflight_stamp(lnk_config("bcfishpass")) + df <- as.data.frame(as.list(s), stringsAsFactors = FALSE) + expect_no_error(lnk_preflight_parity(df, n_expected = 1, quiet = TRUE)) +}) + +test_that("an unresolved field survives the TSV round-trip the shell performs", { + # The version of this test that built the frame in R passed while the real + # gate was broken: the shell writes stamps to a TSV and reads them with + # read.delim(), whose default na.strings = "NA" turned the deliberate "NA" + # sentinel into a real NA. `%in% c("NA","")` did not match it and `!=` + # dropped it, so a run with fwapg_sha unresolved on EVERY host printed + # "host parity clean". Reproduced 2026-08-30, then fixed on both sides. + # + # This test crosses the seam: write the file the way collect_stamps() does, + # read it back the way judge_stamps() does. + cols <- .lnk_preflight_stamp_cols() + mk <- function(host, fwapg) { + v <- c(host, "0.47.0", "abc123def456", "0.33.0", "NA", + "deadbeef0001", "FALSE", "cfg012345678", fwapg, "4.5.2") + paste(v, collapse = "\t") + } + tsv <- withr::local_tempfile() + writeLines(c(mk("m1", "NA"), mk("cy-job1", "NA")), tsv) + + s <- utils::read.delim(tsv, header = FALSE, colClasses = "character", + na.strings = character(0), col.names = cols) + res <- lnk_preflight_parity(s, n_expected = 2, quiet = TRUE) + expect_false(res$ok) + expect_match(res$message, "fwapg_sha unresolved") + + # And the belt: even parsed with read.delim's defaults, where the sentinel + # has already become a real NA, the judge must still refuse it. + s_na <- utils::read.delim(tsv, header = FALSE, colClasses = "character", + col.names = cols) + expect_true(all(is.na(s_na$fwapg_sha))) # premise: the NAs are real + expect_false(lnk_preflight_parity(s_na, n_expected = 2, quiet = TRUE)$ok) +}) + +test_that("a real NA in a key field is a mismatch, not silent agreement", { + # `!=` returns NA for an NA operand and which() drops it, so without + # normalisation a host whose field failed to parse would agree with + # everything. + s <- rbind(row("m1"), row("cy-job1", repo_sha = NA_character_)) + res <- lnk_preflight_parity(s, n_expected = 2, quiet = TRUE) + expect_false(res$ok) +}) + +test_that(".lnk_repo_git_state reports NA for a non-git directory", { + d <- withr::local_tempdir() + g <- .lnk_repo_git_state(d) + expect_true(is.na(g$sha)) + expect_true(is.na(g$dirty)) +}) + +test_that("a non-git host stamp fails parity rather than passing vacuously", { + s <- rbind(row("m1"), row("cy-job1", repo_sha = "NA", repo_dirty = "NA")) + expect_false(lnk_preflight_parity(s, n_expected = 2, quiet = TRUE)$ok) +}) diff --git a/tests/testthat/test-lnk_preflight_vintage.R b/tests/testthat/test-lnk_preflight_vintage.R new file mode 100644 index 00000000..e07ab79d --- /dev/null +++ b/tests/testthat/test-lnk_preflight_vintage.R @@ -0,0 +1,98 @@ +now <- as.POSIXct("2026-08-30 12:00:00", tz = "UTC") +all_four <- .lnk_vintage_primitives() + +# Build a vintage frame from named ages in days. +v <- function(ages) { + data.frame(table_name = names(ages), + last_analyze = now - unname(ages) * 86400, + stringsAsFactors = FALSE) +} +fresh_ages <- function() stats::setNames(rep(2, length(all_four)), all_four) + + +test_that(".lnk_vintage_primitives selects the snapshot-loaded tables only", { + # The seven FWA tables are bulk-restored and never ANALYZEd, so including + # them would fail every host forever on data that is not the staleness risk. + expect_setequal(all_four, + c("bcfishobs.observations", "whse_fish.pscis_assessment_svw", + "cabd.dams", "fresh.modelled_stream_crossings")) + expect_false(any(grepl("^whse_basemapping\\.", all_four))) +}) + +test_that("vintage passes when every primitive is inside the window", { + res <- lnk_preflight_vintage(vintage = v(fresh_ages()), now = now, + max_age_days = 7, quiet = TRUE) + expect_true(res$ok) + expect_length(res$stale, 0L) + expect_length(res$missing, 0L) +}) + +test_that("vintage fails when one primitive is outside the window", { + # cabd.dams at 2026-05-23 against a 2026-08-30 run — the real dispatcher + # state that motivated the gate. + ages <- fresh_ages() + ages[["cabd.dams"]] <- 99 + res <- lnk_preflight_vintage(vintage = v(ages), now = now, + max_age_days = 7, quiet = TRUE) + expect_false(res$ok) + expect_identical(res$stale, "cabd.dams") + expect_match(res$message, "cabd.dams", fixed = TRUE) +}) + +test_that("vintage boundary: exactly max_age_days passes, a hair more fails", { + ages <- stats::setNames(rep(7, length(all_four)), all_four) + expect_true(lnk_preflight_vintage(vintage = v(ages), now = now, + max_age_days = 7, quiet = TRUE)$ok) + ages[[1]] <- 7 + 1 / 24 + expect_false(lnk_preflight_vintage(vintage = v(ages), now = now, + max_age_days = 7, quiet = TRUE)$ok) +}) + +test_that("a NULL timestamp FAILS - never loaded is not fresh", { + x <- v(fresh_ages()) + x$last_analyze[2] <- NA + res <- lnk_preflight_vintage(vintage = x, now = now, quiet = TRUE) + expect_false(res$ok) + expect_true(all_four[2] %in% res$missing) +}) + +test_that("an EMPTY result set FAILS - zero rows is not a pass", { + # The failure this gate exists to avoid inheriting: a query that returns + # nothing must not read as "nothing is stale". + empty <- data.frame(table_name = character(0), + last_analyze = as.POSIXct(character(0)), + stringsAsFactors = FALSE) + res <- lnk_preflight_vintage(vintage = empty, now = now, quiet = TRUE) + expect_false(res$ok) + expect_setequal(res$missing, all_four) +}) + +test_that("a partial result FAILS and names the absent table", { + ages <- fresh_ages()[-1] + res <- lnk_preflight_vintage(vintage = v(ages), now = now, quiet = TRUE) + expect_false(res$ok) + expect_identical(res$missing, all_four[1]) +}) + +test_that("extra unrequested tables cannot mask a missing required one", { + x <- v(c(some.other_table = 1)) + res <- lnk_preflight_vintage(vintage = x, now = now, quiet = TRUE) + expect_false(res$ok) + expect_setequal(res$missing, all_four) +}) + +test_that(".lnk_vintage_read returns an empty frame of the right shape on NULL conn", { + out <- .lnk_vintage_read(NULL, all_four) + expect_s3_class(out, "data.frame") + expect_identical(nrow(out), 0L) + expect_true(all(c("table_name", "last_analyze") %in% names(out))) +}) + +test_that("lnk_preflight_vintage validates its arguments", { + x <- v(fresh_ages()) + expect_error(lnk_preflight_vintage(vintage = x, max_age_days = 0)) + expect_error(lnk_preflight_vintage(vintage = x, max_age_days = NA)) + expect_error(lnk_preflight_vintage(vintage = x, max_age_days = -1)) + expect_error(lnk_preflight_vintage(vintage = x, tables = character(0))) + expect_error(lnk_preflight_vintage(vintage = data.frame(a = 1))) +})