diff --git a/.claude/skills/xce-test-resources/run-resource-verify.sh b/.claude/skills/xce-test-resources/run-resource-verify.sh index 00e7ea9d..9589dac0 100755 --- a/.claude/skills/xce-test-resources/run-resource-verify.sh +++ b/.claude/skills/xce-test-resources/run-resource-verify.sh @@ -5,31 +5,52 @@ # resource value via xce_test's resupdate, confirm it is visible at the # controller, live scheduler, and Go apiserver). # -# resource-value-verification spec, task 5.1+6.1 SCOPE: this file currently -# implements deployment (task 5.1) PLUS worker join + resource injection -# (task 6.1) -- stand up a fresh controller + scheduler (-vvv) + apiserver +# resource-value-verification spec, task 5.1+6.1+6.2 SCOPE: this file +# currently implements deployment (task 5.1), worker join + resource +# injection (task 6.1), AND the controller-side per-layer verification check +# (task 6.2) -- stand up a fresh controller + scheduler (-vvv) + apiserver # (debug logging) deployment via xce-deploy-ha's deploy-ha.sh + generated # start-all.sh, confirm all three components are actually up, join a # SYNTHETIC worker (no real worker daemon process) through xce_test's worker -# context, run resupdate for one or more resource=value pairs, and clean up -# (trap-based, covering a mid-deployment abort, not just a completed run). -# Per-layer verification (tasks 6.2-6.4) and verdict aggregation / +# context, run resupdate for one or more resource=value pairs, then query the +# controller (xce_test client-context `resval`) for every ACCEPTED pair and +# report MATCH/MISMATCH against the injected value, and clean up (trap-based, +# covering a mid-deployment abort, not just a completed run). Scheduler/ +# apiserver per-layer verification (tasks 6.3-6.4) and verdict aggregation / # cleanup-on-every-path (task 7.1) are added by LATER tasks -- see the # "Extension points" markers in main() below for exactly where they hook in. # See .kiro/specs/resource-value-verification/{requirements,design}.md, -# Requirements 2.1, 2.2, 5.3, 6.1, 6.4, 6.5 for these tasks; the full skill -# covers 3.1-6.5. +# Requirements 2.1, 2.2, 3.1, 3.2, 5.3, 6.1, 6.4, 6.5 for these tasks; the +# full skill covers 3.1-6.5. # # Task 6.1 deliberate deviation from design.md: the default (no-args) # resource set is Requirement 1's gap-resource set (r15m, pg, ls, it, swp, # io, tmp) plus a couple of already-supported NUMERIC names for regression -# coverage (mem, cpuf) -- NOT "type"/"model" as design.md's illustrative text +# coverage (mem, r1m) -- NOT "type"/"model" as design.md's illustrative text # suggests. bug-1574 (xce_test can SEGFAULT, non-deterministically, on # `resupdate -n type/-n model ...` -- uninitialized-memory UB in # set_resval()'s Value_u_svalue fallback path) makes including a string-typed # resource in this script's own default live-testing path a real crash risk, # not a theoretical one. Revisit once bug-1574 is fixed. # +# Task 6.2 remediation (bug-1579): the regression-coverage name was +# originally "cpuf", but cpuf is one of worker.c's N_HOST_ATTRS +# (add_hostattrs_resvals()) -- the controller RESETS it to baseline the +# moment the synthetic worker disconnects (mark_worker_unavail() -> +# apply_worker_dead_mutation() -> reset_hostattrs_unknown() + +# add_hostattrs_resvals()), and inject_resources()'s xce_test session ends +# with `quit` (disconnecting the worker) before this task's separate +# resval-query session runs -- so cpuf always reported a false MISMATCH +# (observed=0.000000). Swapped for "r1m": confirmed via +# resval_typeresolve.c's resinfo[] table to be a genuinely pre-existing +# ("already-supported") resource name, and via +# src/worker/src/resource_sync.c to be a periodic/dynamic load-index +# resource (not in worker.c's N_HOST_ATTRS list), so it preserves the +# original "already-supported name for regression coverage" intent without +# the disconnect-reset problem. See bug-1579 for the full analysis and why +# the alternative fix (keep the worker session connected throughout) is out +# of scope for this remediation. +# # Must be invoked under an environment with `nix` on PATH (same as # xce-deploy-ha / xce-test-ha) -- this script shells out to `nix develop` # itself for the steps that need it (openssl, xceadmin), so it does not need @@ -61,12 +82,19 @@ # Exit 0 == this task's scope succeeded (deployment stood up, all three # components confirmed up, worker joined, resupdate ran for every requested # resource -- accepted AND rejected outcomes are both a successful run of -# THIS script's scope, see inject_resources() -- cleaned up per -# --keep-running). Non-zero == deployment failed at some step, the worker -# failed to join, or resupdate produced no observable accept/reject outcome -# for some name (a structural injection failure, distinct from a legitimate -# rejection). Once tasks 6.2-7.1 land, the exit code instead reflects the -# full injection/verification PASS/FAIL verdict. +# THIS script's scope, see inject_resources() -- and the controller-side +# check (task 6.2) ran for every accepted resource -- a MATCH or a MISMATCH +# are BOTH a successful run of this script's own scope, see +# query_controller_resvals(); only a structural failure to query the +# controller at all is fatal here -- cleaned up per --keep-running). +# Non-zero == deployment failed at some step, the worker failed to join, +# resupdate produced no observable accept/reject outcome for some name (a +# structural injection failure, distinct from a legitimate rejection), or the +# controller-side check itself could not run (e.g. the client context failed +# to connect). Once tasks 6.3-7.1 land, the exit code instead reflects the +# full injection/verification PASS/FAIL verdict -- task 6.2 deliberately does +# NOT fail the run on a controller-layer MISMATCH yet (see +# ANY_CONTROLLER_MISMATCH, groundwork for task 7.1's aggregation). # set -u @@ -109,7 +137,7 @@ Usage: $0 [--keep-running] [--dir DIR] [resource=value ...] \$XCE_RESVERIFY_TOP). resource=value inject this resource via resupdate (may repeat). Default when none given: r15m=1.5 pg=2.5 ls=3.5 it=4.5 swp=5.5 - io=6.5 tmp=7.5 mem=8.5 cpuf=9.5 (numeric-only -- see + io=6.5 tmp=7.5 mem=8.5 r1m=9.5 (numeric-only -- see bug-1574 note at top of file). EOF } @@ -317,10 +345,14 @@ confirm_deployment_up() { # names for regression coverage -- see the top-of-file bug-1574 note for why # this deliberately excludes type/model (design.md's own illustrative text # names them, but xce_test can SEGFAULT on a string-typed resupdate, so this -# is a documented, deliberate deviation, not an oversight). +# is a documented, deliberate deviation, not an oversight). The second +# regression-coverage name is "r1m" (a periodic/dynamic load-index resource), +# not a worker.c host-attrs name -- see the top-of-file bug-1579 remediation +# note for why a host-attrs name (e.g. cpuf) always false-MISMATCHes against +# this script's own two-session (inject, then query) structure. DEFAULT_RESOURCE_ARGS=( r15m=1.5 pg=2.5 ls=3.5 it=4.5 swp=5.5 io=6.5 tmp=7.5 - mem=8.5 cpuf=9.5 + mem=8.5 r1m=9.5 ) # Fixed synthetic worker identity for this run -- tags every resupdate'd @@ -436,6 +468,161 @@ $out" [ "$failed" -eq 0 ] || fail "injection did not produce a definitive accept/reject outcome for every requested resource (see above)" } +# ── Step 5: controller-side verification check (task 6.2) ────────────────── +# Requirement 3.1/3.2: for every SUCCESSFULLY-injected (accepted) resource/ +# worker pair, query the controller via xce_test's client-context `resval` +# (the same xcecli_resval() call cmd_resupdate's own task 2.2 follow-up +# verification already uses internally -- a READ query, not a mutation, low +# risk) and compare the returned value against what was injected. A REJECTED +# resource was never actually applied at the controller in the first place +# (task 2.2's own follow-up query inside resupdate already confirmed that at +# injection time) -- there is nothing to verify for it here, so +# query_controller_resvals() only queries names marked "accepted" in +# $TOP/logs/injection-results.tsv. + +declare -A CONTROLLER_OBSERVED=() # resource name -> raw value string as rendered by cmd_resval (only set for accepted names actually queried) +declare -A CONTROLLER_CHECK_RESULTS=() # resource name -> "MATCH" | "MISMATCH: injected= observed=" | "MISMATCH: controller returned no value ..." +ANY_CONTROLLER_MISMATCH=0 # groundwork for task 7.1's verdict aggregation; this task's own exit code does NOT depend on this -- see the top-of-file Exit code note and main() + +# controller_values_match INJECTED OBSERVED +# Pure comparison helper (Requirement 3.2 / design.md's Error Handling "Layer +# mismatch" convention: report both values on a mismatch). Deliberately +# factored out of query_controller_resvals() so it can be exercised directly +# with synthetic inputs, independent of any live controller query -- this +# task's validation approach exercises exactly this (see SKILL.md / task +# notes). Returns 0 (bash "true"/success) on a match, 1 on a mismatch. +# +# Numeric comparison, not string equality: cmd_resval's own controller-side +# rendering always prints fixed 6-decimal %f notation (e.g. an injected +# "1.5" comes back rendered as "1.500000"; a bytes-valued resource such as +# `mem`/`tmp` additionally appends a human-readable "(X.XX UNIT)" suffix +# after the raw float, already stripped by the caller before this function +# ever sees it) -- a literal string compare would falsely MISMATCH on every +# numeric resource. Mirrors task 4.1's identical scheduler trace-log +# parse-as-float lesson (see tasks.md's Implementation Notes). Falls back to +# exact string equality for non-numeric (string-typed, e.g. type/model) +# values -- this script's own default resource set excludes those +# (bug-1574), but the comparison helper itself stays correct for either +# kind, since a future caller (or a synthetic test invocation) may pass one. +controller_values_match() { + local injected="$1" observed="$2" + if [[ "$injected" =~ ^-?[0-9]+([.][0-9]+)?$ ]] && [[ "$observed" =~ ^-?[0-9]+([.][0-9]+)?$ ]]; then + awk -v a="$injected" -v b="$observed" \ + 'BEGIN { d = a - b; if (d < 0) d = -d; exit !(d < 0.0005) }' + else + [ "$injected" = "$observed" ] + fi +} + +# query_controller_resvals -- reads $TOP/logs/injection-results.tsv (task +# 6.1's durable, order-preserving mirror of INJECTED_VALUES/INJECTED_RESULTS +# -- bash associative arrays have no defined iteration order, the TSV does) +# and, for every name marked "accepted", issues one `resval -n -m +# ` query in the xce_test client context, batched into a single +# xtest() invocation (same "one script, one process" precedent as +# inject_resources()'s single combined resupdate call). Populates +# CONTROLLER_OBSERVED/CONTROLLER_CHECK_RESULTS and +# $TOP/logs/controller-check-results.tsv (name\tinjected\tobserved\tresult). +query_controller_resvals() { + local tsv="$TOP/logs/injection-results.tsv" + [ -f "$tsv" ] || fail "controller check: $tsv not found -- inject_resources() must run first" + + local name value result accepted_names=() skipped_names=() + while IFS=$'\t' read -r name value result; do + if [ "$result" = "accepted" ]; then + accepted_names+=("$name") + else + skipped_names+=("$name") + fi + done < "$tsv" + + if [ "${#skipped_names[@]}" -gt 0 ]; then + log "controller check: skipping ${#skipped_names[@]} non-accepted resource(s) -- never actually applied at the controller, nothing to verify there: ${skipped_names[*]}" + fi + + if [ "${#accepted_names[@]}" -eq 0 ]; then + log "controller check: no accepted resources to verify" + return 0 + fi + + log "controller check: querying resval for ${#accepted_names[@]} accepted resource(s): ${accepted_names[*]}" + + local cmds="" out + for name in "${accepted_names[@]}"; do + cmds="${cmds}resval -n $name -m $WORKER_HOST"$'\n' + done + + local script + script="$(printf 'set masters %s\nclient\n%squit\n' "$PEERS" "$cmds")" + + out="$(printf '%s' "$script" | xtest)" + + if printf '%s\n' "$out" | grep -Fq -- "enter_ctx: invalid"; then + fail "controller check failed -- xce_test client context could not connect (masters=$PEERS); xce_test output: +$out" + fi + + # Sequential state-machine parse of cmd_resval's own fixed 2-line block + # format (test.c's cmd_resval(): "resource @:\nvalue: + # \n") or its 1-line not-found message ("no resource value + # found in given host"). Keyed off the exact header line rather than a + # per-name grep -- avoids the short-resource-name substring hazard + # (bug-1576/inject_resources()'s own -Fx lesson: names like "it"/"io"/ + # "ls" can appear as a literal substring of an unrelated line) and + # avoids depending on a trailing line always existing after the LAST + # query's output (a plain `grep -A1` on the final block would silently + # come up empty if `quit` prints nothing further). + local -A parsed_values=() + local line cur_name="" + while IFS= read -r line; do + case "$line" in + "resource "*"@${WORKER_HOST}:") + cur_name="${line#resource }" + cur_name="${cur_name%@${WORKER_HOST}:}" + ;; + "value: "*) + if [ -n "$cur_name" ]; then + parsed_values["$cur_name"]="${line#value: }" + fi + cur_name="" + ;; + esac + done <<< "$out" + + mkdir -p "$TOP/logs" + local results_tsv="$TOP/logs/controller-check-results.tsv" + : > "$results_tsv" + + local injected raw observed result_str + for name in "${accepted_names[@]}"; do + injected="${INJECTED_VALUES[$name]}" + raw="${parsed_values[$name]:-}" + + if [ -z "$raw" ]; then + observed="" + result_str="MISMATCH: controller returned no value (injected=$injected observed=)" + ANY_CONTROLLER_MISMATCH=1 + else + # First whitespace-delimited token of the rendered value -- + # strips a bytes-valued resource's trailing "(X.XX UNIT)" + # suffix (e.g. "8.500000 (8.50 B)" -> "8.500000"); a no-op for + # every other rendering (plain %f, %s, or true/false). + observed="$(awk '{print $1}' <<< "$raw")" + if controller_values_match "$injected" "$observed"; then + result_str="MATCH" + else + result_str="MISMATCH: injected=$injected observed=$observed" + ANY_CONTROLLER_MISMATCH=1 + fi + fi + + CONTROLLER_OBSERVED["$name"]="$observed" + CONTROLLER_CHECK_RESULTS["$name"]="$result_str" + log "controller check: $name -> $result_str" + printf '%s\t%s\t%s\t%s\n' "$name" "$injected" "$observed" "$result_str" >> "$results_tsv" + done +} + main() { parse_args "$@" resolve_top @@ -455,16 +642,23 @@ main() { inject_resources "${RESOURCE_ARGS[@]}" - # --- Extension point (tasks 6.2-6.4): controller/scheduler/apiserver - # per-layer verification checks here, consuming - # ${INJECTED_VALUES[@]}/${INJECTED_RESULTS[@]} and $WORKER_HOST - # populated by inject_resources() above (also mirrored to - # $TOP/logs/injection-results.tsv). --- + query_controller_resvals + + # --- Extension point (tasks 6.3-6.4): scheduler/apiserver per-layer + # verification checks here, consuming ${INJECTED_VALUES[@]}/ + # ${INJECTED_RESULTS[@]} and $WORKER_HOST populated by + # inject_resources() above (also mirrored to + # $TOP/logs/injection-results.tsv), following the same pattern + # query_controller_resvals() (task 6.2) just established for its own + # layer -- CONTROLLER_OBSERVED/CONTROLLER_CHECK_RESULTS + + # $TOP/logs/controller-check-results.tsv. --- # --- Extension point (task 7.1): aggregate results into a VERDICT line # plus the terminal PASS/FAIL oracle line, replacing this task's - # placeholder success message and exit code below. --- + # placeholder success message and exit code below. ANY_CONTROLLER_MISMATCH + # (set by query_controller_resvals() above) is groundwork for this + # aggregation -- task 6.2 itself deliberately does not act on it. --- - log "PASS (task 5.1+6.1 scope): controller, scheduler, and apiserver confirmed up; worker '$WORKER_HOST' joined and resupdate ran for ${#INJECTED_RESULTS[@]} resource(s) under $TOP (see $TOP/logs/injection-results.tsv for the accept/reject detail per name -- a rejection there is a captured outcome, not a script failure at this stage); per-layer verification and the overall PASS/FAIL verdict land in tasks 6.2-7.1; cleaning up" + log "PASS (task 5.1+6.1+6.2 scope): controller, scheduler, and apiserver confirmed up; worker '$WORKER_HOST' joined and resupdate ran for ${#INJECTED_RESULTS[@]} resource(s) under $TOP (see $TOP/logs/injection-results.tsv for the accept/reject detail per name -- a rejection there is a captured outcome, not a script failure at this stage); controller-side check ran for every accepted resource (see $TOP/logs/controller-check-results.tsv -- a MISMATCH there is also a captured outcome, not a script failure at this stage, see ANY_CONTROLLER_MISMATCH); scheduler/apiserver per-layer verification and the overall PASS/FAIL verdict land in tasks 6.3-7.1; cleaning up" exit 0 } diff --git a/.kiro/specs/resource-value-verification/tasks.md b/.kiro/specs/resource-value-verification/tasks.md index c78078a8..14d50604 100644 --- a/.kiro/specs/resource-value-verification/tasks.md +++ b/.kiro/specs/resource-value-verification/tasks.md @@ -69,7 +69,7 @@ - _Requirements: 2.1, 2.2, 6.1, 6.4_ - _Depends: 2.1, 2.2, 5.1_ -- [ ] 6.2 Implement the controller-side verification check +- [x] 6.2 Implement the controller-side verification check - Query the controller via `resval` for each injected resource/worker pair and compare against the injected value - Observable completion: the check reports, per resource, whether the controller's observed value matches the injected value, including both values when it does not - _Requirements: 3.1, 3.2_ @@ -116,6 +116,7 @@ - _Depends: 7.1_ ## Implementation Notes +- Task 6.2 (round 1 REJECTED, round 2 fixed) discovered **bug-1579**: any host-attrs-derived resource (`type`/`model`/`cpuf`/`maxcpus`/`maxmem`/`maxswap`/`maxtmp`/`ndisks` — `worker.c`'s `N_HOST_ATTRS`) gets reset to baseline by the controller the instant the synthetic worker disconnects, and `inject_resources()` (6.1) always disconnects (`quit`) before each layer's separate check session runs — so a controller-layer check on any host-attrs name will ALWAYS false-MISMATCH under the current two-session structure. Fixed for the default set by swapping `DEFAULT_RESOURCE_ARGS`'s `cpuf` for `r1m` (periodic/dynamic, confirmed safe). **This risk is NOT fully closed** — a user-supplied custom `resource=value` argument using any host-attrs name will still hit this. A real fix requires either a C-level `xce_test` change (its REPL has no path from the `worker` context to the `client` context without a disconnect-equivalent `leave_ctx()`) or a bash coprocess restructuring to keep the worker connected across all layer checks — out of scope for tasks 6.1-6.4, worth a dedicated follow-up task if host-attrs resources ever need default-set coverage. - Task 6.1 uncovered and got fixed a critical, unrelated, pre-existing controller bug (**bug-1578**, commit `b406dd7`, standalone — a dangling-pointer use-after-free in `sched_worker_change()`/`sched.c` that crashed the controller on worker disconnect once a scheduler had ever joined, 5 months old, unrelated to this feature). It's fixed now with regression coverage (`test_sched_worker_change.c`). If any later task sees a live controller die unexpectedly during a worker-disconnect sequence, confirm you're building from a commit that includes `b406dd7` before assuming it's a new bug. `inject_resources()` also fixed **bug-1577** (in-boundary): `XCE_WORKER_BIN=/bin/true` is exported before invoking `start-all.sh` to suppress the REAL `xcecd_virtual` worker daemon that `deploy-ha.sh`'s pre-existing worker branch would otherwise silently start (Requirement 2.1 violation) whenever the build dir happens to contain one. - Task 6.1: injection results are stored in `INJECTED_VALUES`/`INJECTED_RESULTS` associative arrays plus `$TOP/logs/injection-results.tsv` (`name\tvalue\tresult`) for tasks 6.2-6.4's extension points, which are appended directly after `inject_resources()` in `main()` (explicit "Extension point" comments mark exactly where). `SKILL.md` is now stale (still says injection "not yet implemented", still illustrates `type` in the default set) — deliberately left unedited since task 6.1's CLI parsing shape didn't change; fix this in task 7.1 when SKILL.md is next touched. - Task 5.1: `run-resource-verify.sh` exists now (`.claude/skills/xce-test-resources/`), structured with explicit "Extension point" comments marking where task 6.1 (injection), 6.2-6.4 (per-layer checks), and 7.1 (verdict/cleanup-on-every-path) hook in — read those before starting 6.1. Trap is registered as the first statement in `main()`, before any process starts; `cleanup()` uses a single combined `pkill -9 -f` (alternation pattern) plus retry-until-`pgrep`-clean, not sequential per-process kills (a sequential version was tried first and found non-deterministically flaky under an async SIGTERM trap — bug-1575). Default deployment is single-node (not 3-node HA) — deliberate, since this skill verifies propagation not failover; override via `$XCE_RESVERIFY_PEERS`. Known non-blocking gap: `$XCE_RESVERIFY_SCHED_BIN`/`$XCE_RESVERIFY_APISERVER_BIN` only gate this script's own pre-flight check, NOT which binary `deploy-ha.sh` actually deploys (it hardcodes `/tmp/apiserver` and derives the scheduler path from `$BINARY`'s build dir) — don't rely on these two env vars to test against an alternate binary.