Merge branch 'master' into fix/cylindrical-angular-flux

Author: aliencaocao

.claude/rules/common-pitfalls.md

@@ -36,6 +36,35 @@ covered in `docs/documentation/contributing.md`.
 - `@:ACC_SETUP_VFs(...)`/`@:ACC_SETUP_SFs(...)` GPU pointer setup compiles only under
   Cray. Around MPI: `GPU_UPDATE(host=...)` before send, `GPU_UPDATE(device=...)` after
   receive.
+- An array whose bound is a device global (`dimension(num_fluids)`, `dimension(num_species)`) may be
+  passed to a device routine **from a parallel-loop body, but not from inside another
+  `GPU_ROUTINE(parallelism='[seq]')`**. CCE OpenACC rejects the second form with
+  `ftn-7066 ... Global in accelerator routine without declare -- num_fluids`, and reports it at
+  whatever line it gave up on: remove one trigger and the message *walks forward* to the next call,
+  so the reported line is not the cause. Only the plain lanes fail - under `--case-optimization`
+  those bounds are `parameter`s, so a green Case Opt lane beside a failing plain one is the
+  signature. Every accepted call site in the tree already obeys this (`m_cbc`, `m_ibm`,
+  `m_bubbles_EL`, `s_compute_cell_state`): form such a call in the loop body and pass scalars
+  deeper. Neither `cray_inline` nor a `num_fluids_max` bound nor dropping optional dummies helps -
+  all three were measured.
+- nvfortran 23.11/24.1 segfault (`fort2 TERMINATED by signal 11`) on a caller that passes a
+  `parameter` array from `m_thermochem` (e.g. `molecular_weights`) into a declare-target routine.
+  Read such arrays directly in the kernel, or pass a plain local computed from them.
+- The `USING_AMD` fypp guards (86 sites, `#:set` in `src/common/include/shared_parallel_macros.fpp`) are
+  load-bearing, not a stale workaround - do not "modernize" them away. They swap a device-global array
+  bound for a literal: `dimension(3)` for `num_dims`/`num_fluids` when case optimization is off (64
+  sites), and `dimension(20)` for `sys_size` in `m_compute_cbc` (21 sites, with a matching
+  `@:PROHIBIT` in `m_start_up` capping `sys_size <= 20` under AMD+CBC). Setting `USING_AMD = False`
+  and rebuilding amdflang `--gpu mp` without case optimization compiles CLEAN - 728 s, zero
+  diagnostics - and then NaNs at step 50 in CBC, riemann `wave_speeds=2`, IBM, surface tension,
+  QBMM/viscous and MHD HLLD, while both Lagrange bubble cases *complete* with out-of-tolerance
+  answers. Measured 2026-08-29 on MI210. A compile-only check returns green, so any future attempt to
+  drop these must run the tests, not just build.
+- The same "call it from the loop body" rule covers `m_thermochem`: calling `get_species_*` from
+  inside a `GPU_ROUTINE` rather than from the kernel gave CCE OpenMP a runtime
+  `Memory access fault by GPU node-N ... Reason: Unknown` on the first step (exit 134), while every
+  other backend ran. Evaluate them at the call site and pass the arrays in. Note this one only shows
+  at runtime, and only on a case that reaches the path - the build is clean.
 
 ## Parameters
 
@@ -105,6 +134,27 @@ covered in `docs/documentation/contributing.md`.
 - Tests are generated programmatically in `toolchain/mfc/test/cases.py` (parameter
   modifications on `BASE_CFG` via the `CaseGeneratorStack` push/pop pattern); test UUID =
   CRC32 of the trace string; `./mfc.sh test -l` lists all.
+- `--only` matches whole trace *elements*, not substrings, and `_filter_only`
+  (`toolchain/mfc/test/test.py`) **ANDs labels while ORing UUIDs**. So `--only bubbles` matches
+  nothing (the element is `Bubbles`), and `--only low_Mach=1 low_Mach=2` asks for cases carrying
+  both and also matches nothing. It then exits **143**, which reads like an external kill rather
+  than an empty filter. Pass UUIDs whenever you want the union of several groups.
+- Sibling `define_case_d` calls off the same stack level are never *combined*. Two switches that
+  only matter together (`avg_state=1` needs `wave_speeds=2` to be read at all) therefore get zero
+  effective coverage unless something pushes one and defines the other beneath it. Check
+  reachability before trusting that a flag is tested.
+- `--no-build` silently runs whatever binary is on disk for a configuration it did not build.
+  Chemistry has its own config (`gpu-mp-chem-*`) that a plain `./mfc.sh build` never produces, so
+  a `--no-build` run reports failures from stale binaries and hides real compile breaks. Run
+  chemistry-touching sets without it.
+- Pick the newest binary by the *binary's* mtime (`ls -t build/install/*/bin/simulation`), not the
+  install directory's - a stale config's directory can be newer than a fresh build's.
+- The pre-commit hook lives in the main repo's `.git/hooks/` and git exports `GIT_DIR` there
+  during a commit, so from a worktree the toolchain lint enumerates the *other* checkout and
+  fails. Reproduce with `GIT_DIR=<main>/.git ./mfc.sh precheck`. Run precheck by hand and commit
+  with `--no-verify`.
+- `/tmp` is node-local: scratch does not survive a compute-node change, and its absence is
+  silence, not an error. Keep patches and resource baselines on a shared filesystem.
 - Golden files are tolerance-compared. Regenerate only the affected tests
   (`./mfc.sh test --generate --only <tests>`) — an unexplained golden-file diff is a bug
   report, not noise to be regenerated away.

.github/scripts/check_coverage_map_health.py

@@ -1,11 +1,10 @@
 """Fail loudly if the committed coverage map is stale or under-covers. Used by coverage-health.yml."""
 import datetime
-import subprocess
 import sys
 from pathlib import Path
 
 sys.path.insert(0, str(Path(__file__).resolve().parents[2] / "toolchain"))
-from mfc.test.coverage import COVERAGE_MAP_PATH, load_map, map_health  # noqa: E402
+from mfc.test.coverage import COVERAGE_MAP_PATH, load_map, map_health, run_git  # noqa: E402
 from mfc.test.cases import list_cases  # noqa: E402  (returns the current test list)
 
 MAX_AGE_DAYS = 10
@@ -40,7 +39,7 @@ def verified_sha(cwd=None):
     caller must read that as undeterminable and fall back to the wall-clock age rule, not
     as a failure -- an absent ref is not evidence of a broken refresh.
     """
-    rev = subprocess.run(["git", "rev-parse", "--verify", "--quiet", f"{VERIFIED_REF}^{{commit}}"], capture_output=True, text=True, check=False, cwd=cwd)
+    rev = run_git(["rev-parse", "--verify", "--quiet", f"{VERIFIED_REF}^{{commit}}"], cwd)
     return rev.stdout.strip() or None
 
 
@@ -53,10 +52,10 @@ def verified_after_last_change(git_sha, cwd=None):
     """
     if not git_sha:
         return None
-    last = subprocess.run(["git", "log", "-1", "--format=%H", "--", *COVERAGE_RELEVANT_PATHS], capture_output=True, text=True, check=False, cwd=cwd)
+    last = run_git(["log", "-1", "--format=%H", "--", *COVERAGE_RELEVANT_PATHS], cwd)
     if last.returncode != 0 or not last.stdout.strip():
         return None  # shallow clone or no such commit -> fall back to the age rule
-    ancestor = subprocess.run(["git", "merge-base", "--is-ancestor", last.stdout.strip(), git_sha], capture_output=True, check=False, cwd=cwd)
+    ancestor = run_git(["merge-base", "--is-ancestor", last.stdout.strip(), git_sha], cwd)
     return {0: True, 1: False}.get(ancestor.returncode)  # anything else -> None (unknown sha, shallow history)
 
 

.github/scripts/monitor_slurm_job.sh

@@ -35,6 +35,15 @@ output_file="$2"
 echo "Submitted batch job $job_id"
 echo "Monitoring output file: $output_file"
 
+# Put the one thing a reader needs on the run's summary page. Without this,
+# learning why a job failed means opening a log of tens of thousands of lines --
+# and an infrastructure fault looks exactly like a test failure until you do.
+# Silent when not running under Actions.
+ci_summary() {
+  [ -n "${GITHUB_STEP_SUMMARY:-}" ] || return 0
+  printf '%b\n' "$1" >> "$GITHUB_STEP_SUMMARY"
+}
+
 # Robustly check SLURM job state using squeue with sacct fallback.
 # Returns the state string (PENDING, RUNNING, COMPLETED, FAILED, etc.)
 # or "UNKNOWN" if both commands fail.
@@ -70,9 +79,13 @@ get_job_state() {
   echo "UNKNOWN"
 }
 
-# Check if a state is terminal (job is done, for better or worse)
-# PREEMPTED is intentionally excluded: with --requeue the job restarts under
-# the same job ID and we must keep monitoring rather than exiting early.
+# Check if a state is terminal (job is done, for better or worse).
+# PREEMPTED is handled separately (below): Phoenix preempts 'embers' jobs with
+# PreemptMode=CANCEL, not REQUEUE (verified via `scontrol show config`), so a
+# preempted job is killed outright and never restarts under the same ID.
+# --requeue is a no-op for it. It is surfaced via PREEMPT_EXIT so the submit
+# wrapper can resubmit a fresh job instead of failing the CI step.
+PREEMPT_EXIT=76
 is_terminal_state() {
   case "$1" in
     COMPLETED|FAILED|CANCELLED|CANCELLED+|TIMEOUT|OUT_OF_MEMORY|NODE_FAIL|BOOT_FAIL|DEADLINE|REVOKED)
@@ -82,18 +95,68 @@ is_terminal_state() {
   esac
 }
 
+# Optionally bound how long a job may sit un-started in the queue. On the
+# preemptible Phoenix 'embers' QOS a job routinely stays PENDING for hours and
+# needs most of the job-level `timeout-minutes` (480m) window to backfill onto a
+# free node; that job timeout is the real backstop. Default to 0 (wait
+# indefinitely, up to the job timeout) so ordinary queue pressure does not turn
+# otherwise-healthy jobs into red CI. Set SLURM_MAX_QUEUE_SECONDS>0 to opt into
+# an earlier queue-starvation cutoff where the scheduler is not preemptible.
+: "${SLURM_MAX_QUEUE_SECONDS:=0}"   # 0 = wait indefinitely (job timeout is the backstop)
+# Reject a non-integer override rather than silently skipping the budget.
+if ! [[ "$SLURM_MAX_QUEUE_SECONDS" =~ ^[0-9]+$ ]]; then
+  echo "ERROR: SLURM_MAX_QUEUE_SECONDS must be a non-negative integer (seconds), got '$SLURM_MAX_QUEUE_SECONDS'" >&2
+  exit 1
+fi
+# How long to wait between status polls and between output-stabilization
+# checks. Overridable so tests can exercise this script without sleeping
+# through it; CI leaves it at the default.
+: "${MFC_MONITOR_POLL_SECONDS:=5}"
+
+queue_start=$(date +%s)
+
+abort_queue_starvation() {
+  local waited="$1"
+  echo "##[error]SLURM job $job_id did not start within ${waited}s (SLURM_MAX_QUEUE_SECONDS=$SLURM_MAX_QUEUE_SECONDS)."
+  echo "QUEUE STARVATION: the cluster scheduler could not start this job in time."
+  echo "This is an infrastructure / queue-availability problem, NOT a code or test failure."
+  echo "Cancelling the queued job so it does not keep holding a CI runner slot."
+  scancel "$job_id" 2>/dev/null || true
+  exit 75   # EX_TEMPFAIL — distinguishes queue starvation from a real test failure
+}
+
 # Wait for file to appear, using robust state checking.
-# Never give up due to transient squeue/sacct failures — the CI job timeout
-# is the ultimate backstop.
+# Never give up due to transient squeue/sacct failures — the queue-wait budget
+# above (or the CI job timeout) is the ultimate backstop.
 echo "Waiting for job to start..."
 unknown_count=0
 while [ ! -f "$output_file" ]; do
   state=$(get_job_state "$job_id")
 
+  # A started job (RUNNING/COMPLETING) whose output file is merely NFS-delayed
+  # is exempt, so work in progress is never killed here.
+  if [ "$SLURM_MAX_QUEUE_SECONDS" -gt 0 ]; then
+    case "$state" in
+      RUNNING|COMPLETING) ;;
+      *)
+        waited=$(( $(date +%s) - queue_start ))
+        if [ "$waited" -ge "$SLURM_MAX_QUEUE_SECONDS" ]; then
+          abort_queue_starvation "$waited"
+        fi
+        ;;
+    esac
+  fi
+
   case "$state" in
-    PENDING|CONFIGURING|PREEMPTED)
+    PREEMPTED)
+      # Preempted before producing output (embers, PreemptMode=CANCEL): the job
+      # is dead and will not requeue. Signal the caller to resubmit a fresh job.
+      echo "[$(date +%H:%M:%S)] Job $job_id PREEMPTED before start/output — signaling resubmit."
+      exit "$PREEMPT_EXIT"
+      ;;
+    PENDING|CONFIGURING)
       unknown_count=0
-      sleep 5
+      sleep "$MFC_MONITOR_POLL_SECONDS"
       ;;
     RUNNING|COMPLETING)
       unknown_count=0
@@ -106,7 +169,7 @@ while [ ! -f "$output_file" ]; do
       if [ $((unknown_count % 12)) -eq 1 ]; then
         echo "Warning: Could not query job $job_id state (SLURM may be temporarily unavailable)..."
       fi
-      sleep 5
+      sleep "$MFC_MONITOR_POLL_SECONDS"
       ;;
     *)
       # Terminal state — job finished without creating output
@@ -115,7 +178,7 @@ while [ ! -f "$output_file" ]; do
         exit 1
       fi
       # Unrecognized state, keep waiting
-      sleep 5
+      sleep "$MFC_MONITOR_POLL_SECONDS"
       ;;
   esac
 done
@@ -138,6 +201,12 @@ last_heartbeat=$(date +%s)
 while true; do
   state=$(get_job_state "$job_id")
 
+  if [ "$state" = "PREEMPTED" ]; then
+    # Preempted mid-run (embers, PreemptMode=CANCEL): dead, will not requeue.
+    echo "[$(date +%H:%M:%S)] Job $job_id PREEMPTED mid-run — signaling resubmit."
+    exit "$PREEMPT_EXIT"
+  fi
+
   if is_terminal_state "$state"; then
     echo "[$(date +%H:%M:%S)] Job $job_id reached terminal state: $state"
     break
@@ -150,11 +219,17 @@ while true; do
     last_heartbeat=$current_time
   fi
 
-  sleep 5
+  sleep "$MFC_MONITOR_POLL_SECONDS"
 done
 
-# Give tail a moment to flush the final lines, then stop streaming.
+# Give tail a moment to flush the final lines, then stop streaming. Whether it
+# was still alive decides how much needs reprinting below: if it streamed the
+# whole job, printing the file again just doubles every log.
 sleep 2
+streamed_ok=0
+if kill -0 "${tail_pid}" 2>/dev/null; then
+  streamed_ok=1
+fi
 kill "${tail_pid}" 2>/dev/null || true
 tail_pid=""
 
@@ -174,13 +249,24 @@ if [ -f "$output_file" ]; then
     if [ $same_count -ge 2 ]; then
       break
     fi
-    sleep 5
+    sleep "$MFC_MONITOR_POLL_SECONDS"
   done
 fi
 
+# Reprint only what streaming may have missed. `tail -f` above already emitted
+# the whole file as it was written, so cat'ing it again duplicated every job's
+# output -- measured at 3 copies of each line on a GPU job, and 65,000 lines of
+# offload diagnostics repeated for a single fault. The reprint exists solely as
+# a safety net for a tail that died mid-job, so it is bounded when tail survived
+# and complete only when it did not.
 echo ""
-echo "=== Final output ==="
-cat "$output_file"
+if [ "${streamed_ok:-0}" -eq 1 ]; then
+  echo "=== Final output (tail; the full log streamed above) ==="
+  tail -n "${MFC_MONITOR_FINAL_LINES:-40}" "$output_file"
+else
+  echo "=== Final output (streaming stopped early; reprinting in full) ==="
+  cat "$output_file"
+fi
 
 # Check exit status with sacct fallback
 exit_code=""
@@ -207,9 +293,32 @@ if [ -z "$exit_code" ]; then
   exit 1
 fi
 
+# The preflight's node-fault verdict comes back as the job's own exit code.
+# Relay it verbatim: flattening it to 1 would leave the submit wrapper unable to
+# tell "this node is unusable" (exclude it and try again) from "the tests
+# failed" (report it).
+faulted_node=$(grep -oE 'MFC_FAULT_NODE=[^ ]+' "$output_file" 2>/dev/null | tail -n1 | cut -d= -f2 || true)
+
+case "$exit_code" in
+  77:*)
+    echo "Job $job_id failed preflight: the node is unusable — signaling caller to exclude it and resubmit."
+    ci_summary "### :warning: Infrastructure fault — not a code or test failure\n\nNode \`${faulted_node:-unknown}\` could not run MFC (job \`$job_id\`). It is excluded and the job resubmitted elsewhere.\n"
+    monitor_success=1
+    exit 77
+    ;;
+esac
+
 # Check if job succeeded
 if [ "$exit_code" != "0:0" ]; then
   echo "ERROR: Job $job_id failed with exit code $exit_code"
+  # A GPU memory fault explains itself in a block the test harness prints; lift
+  # it onto the summary page so the faulting kernel and source line are visible
+  # without opening the log at all.
+  if grep -q 'GPU fault summary' "$output_file" 2>/dev/null; then
+    ci_summary "### GPU memory fault\n\n\`\`\`\n$(grep -A6 'GPU fault summary' "$output_file" | head -8 | sed 's/`/'"'"'/g')\n\`\`\`\n"
+  else
+    ci_summary "### Job \`$job_id\` failed (exit $exit_code)\n\n\`\`\`\n$(tail -n 15 "$output_file" | sed 's/`/'"'"'/g')\n\`\`\`\n"
+  fi
   exit 1
 fi
 

.github/scripts/node-exclude.sh

@@ -0,0 +1,55 @@
+#!/bin/bash
+# Bookkeeping for the sbatch --exclude list used when a node fails preflight.
+#
+# The in-allocation preflight prints "MFC_FAULT_NODE=<name>" into the job's
+# output file when it finds the node unusable (dead GPU, SIGILL from a binary
+# built for another microarchitecture). The submit wrapper reads that back, adds
+# the node to --exclude and resubmits, so the retry lands elsewhere.
+#
+# Bad nodes are concentrated rather than scattered -- over 2026-08-18..31 one
+# Phoenix node accounted for 25 of 29 ECC failures -- which is why excluding the
+# offender is worth doing and why it was previously a hand-edited constant.
+#
+# Usage:
+#   node-exclude.sh node-from <output-file>   print the faulted node, if any
+#   node-exclude.sh merge <csv> <node>        print <csv> with <node> added once
+
+set -uo pipefail
+
+usage() {
+    echo "Usage: $0 {node-from <output-file>|merge <csv> <node>}" >&2
+}
+
+case "${1:-}" in
+    node-from)
+        file="${2:-}"
+        [ -f "$file" ] || exit 0
+        # Last marker wins: one output path is reused across resubmits, so an
+        # earlier attempt's marker can still be sitting above the current one.
+        sed -n 's/.*MFC_FAULT_NODE=\([A-Za-z0-9._-]\{1,\}\).*/\1/p' "$file" | tail -n1
+        ;;
+
+    merge)
+        csv="${2-}"
+        node="${3-}"
+        if [ -z "$node" ]; then
+            printf '%s\n' "$csv"
+            exit 0
+        fi
+        if [ -z "$csv" ]; then
+            printf '%s\n' "$node"
+            exit 0
+        fi
+        # Wrapping both sides in commas compares whole fields, so a shorter name
+        # that happens to be a prefix of the new one is not mistaken for a match.
+        case ",$csv," in
+            *",$node,"*) printf '%s\n' "$csv" ;;
+            *)           printf '%s,%s\n' "$csv" "$node" ;;
+        esac
+        ;;
+
+    *)
+        usage
+        exit 2
+        ;;
+esac

.github/scripts/prebuild-case-optimization.sh

@@ -90,6 +90,15 @@ if [ -n "$shard" ] && [ "$shard_count" -gt 1 ]; then
     fi
 fi
 
+# Deliberately no node probe here. This pre-build is submitted as a *cpu*
+# allocation (see test.yml: it is --dry-run, so it only builds), while the
+# binaries it produces are GPU builds. syscheck built with --gpu therefore
+# asserts omp_get_num_devices() > 0 and exits non-zero on a node that has no
+# GPU by design -- which a probe would report as a bad node. It did: three
+# healthy Phoenix nodes were condemned and two excluded before the wrapper gave
+# up. The GPU allocation that actually runs these cases is probed instead, in
+# run_case_optimization.sh.
+
 idx=0
 for case in "${benchmarks[@]}"; do
     idx=$((idx + 1))