From b673b7c933236f4a4fd69cc85398598771c3e5b3 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Tue, 18 Aug 2026 11:03:44 +0000 Subject: [PATCH] fix(resource-value-verification): close bug-1579 for real via xce_test's ! shell-escape Previously worked around (task 6.2) by removing the one host-attrs resource (cpuf) from the default set - a custom resource=value arg using any host-attrs name would still false-mismatch at the controller layer, since inject_resources() disconnected the worker before the separate controller- check session ran, and the controller resets host-attrs resources on disconnect. Real fix: xce_test's `!` escape is handled in the single top-level REPL loop before context-specific dispatch, so it works in any context - including worker mode - without calling leave_ctx()/disconnecting. inject_resources() now shells out via `!` to run the controller check in-line, between resupdate and quit, while the worker's connection is still open. query_controller_resvals()'s comparison logic is unchanged - this is a pure sequencing fix. cpuf restored to DEFAULT_RESOURCE_ARGS as live proof. Live-verified: cpuf now MATCHes at the controller layer (was always a false MISMATCH before); adversarial shell-injection attempt via a malicious resource name confirmed safe (printf %q quoting); SIGTERM delivered exactly during the nested check confirmed zero orphaned/zombie processes across 4 independent runs. Co-Authored-By: Claude Sonnet 5 --- .../xce-test-resources/run-resource-verify.sh | 312 +++++++++++++----- .../resource-value-verification/tasks.md | 3 +- 2 files changed, 234 insertions(+), 81 deletions(-) diff --git a/.claude/skills/xce-test-resources/run-resource-verify.sh b/.claude/skills/xce-test-resources/run-resource-verify.sh index ef55d930..97635860 100755 --- a/.claude/skills/xce-test-resources/run-resource-verify.sh +++ b/.claude/skills/xce-test-resources/run-resource-verify.sh @@ -5,21 +5,31 @@ # 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+6.2+6.3 SCOPE: this file -# currently implements deployment (task 5.1), worker join + resource -# injection (task 6.1), the controller-side per-layer verification check -# (task 6.2), AND the scheduler-side per-layer verification check (task 6.3) -# -- 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, then (a) query -# the controller (xce_test client-context `resval`) for every ACCEPTED pair -# and report MATCH/MISMATCH against the injected value, and (b) poll the live -# scheduler daemon's own log file (task 4.1's trace line) for the same -# ACCEPTED pairs, within a bounded wait, and report MATCH/MISMATCH/ -# UNREACHABLE distinctly from the controller-side result, then clean up -# (trap-based, covering a mid-deployment abort, not just a completed run). +# resource-value-verification spec, task 5.1+6.1+6.2+6.3 SCOPE (+ bug-1579 +# follow-up fix): this file currently implements deployment (task 5.1), +# worker join + resource injection (task 6.1), the controller-side per-layer +# verification check (task 6.2), AND the scheduler-side per-layer +# verification check (task 6.3) -- 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, then (a) query the controller (xce_test +# client-context `resval`) for every ACCEPTED pair and report +# MATCH/MISMATCH against the injected value, and (b) poll the live scheduler +# daemon's own log file (task 4.1's trace line) for the same ACCEPTED pairs, +# within a bounded wait, and report MATCH/MISMATCH/UNREACHABLE distinctly +# from the controller-side result, then clean up (trap-based, covering a +# mid-deployment abort, not just a completed run). +# +# bug-1579 follow-up fix (this revision): the controller-side check (a) above +# now runs NESTED inside inject_resources()'s own xce_test worker session -- +# via a `!` shell-escape line placed between `resupdate` and `quit` -- rather +# than as a wholly separate xce_test process run AFTER that session's own +# `quit` has already disconnected the synthetic worker. See the +# internal_controller_check() function and the top-of-file bug-1579 note +# below for why this matters and exactly how it works; DEFAULT_RESOURCE_ARGS +# now includes `cpuf` again as proof the fix is real (see that note). # Apiserver per-layer verification (task 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. @@ -30,30 +40,59 @@ # 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, r1m) -- NOT "type"/"model" as design.md's illustrative text +# coverage (mem, cpuf) -- 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 +# Task 6.2 remediation (bug-1579, original workaround): 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 used to end with `quit` (disconnecting +# the worker) before a separate, later xce_test process ran the controller +# query -- so cpuf always reported a false MISMATCH (observed=0.000000). +# Task 6.2's review swapped it 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. +# resource, not in worker.c's N_HOST_ATTRS list) as a workaround, not a fix -- +# the underlying disconnect-timing problem remained live for any *other* +# host-attrs name (including a user-supplied custom resource=value +# argument). +# +# bug-1579 REAL FIX (this revision): xce_test's `!` escape +# (preprocess_and_run(), test.c ~line 950-998) is handled in the single +# top-level REPL loop (test.c main(), the sole call site) BEFORE any +# context-specific command dispatch -- so it works identically in ANY +# context, including `worker`, WITHOUT calling leave_ctx()/disconnecting. +# inject_resources() now inserts a `!` line between `resupdate` and `quit` +# that invokes this very script recursively, in a hidden +# `--internal-controller-check` mode (see internal_controller_check() below), +# from INSIDE the still-open worker session. That nested invocation runs its +# own client-context xce_test session and queries `resval` for every +# requested resource name WHILE the synthetic worker is still connected -- +# so a host-attrs resource is never reset before it's checked. The nested +# check writes raw observed values to $TOP/logs/controller-check-raw.tsv; +# query_controller_resvals() (task 6.2's original function, still called at +# the same point in main(), after inject_resources() returns) now reads that +# file instead of running its own live query -- a SEQUENCING change, not a +# logic change: the MATCH/MISMATCH comparison, CONTROLLER_OBSERVED/ +# CONTROLLER_CHECK_RESULTS population, and controller-check-results.tsv +# format are all unchanged. DEFAULT_RESOURCE_ARGS now restores "cpuf" +# alongside "mem" (design.md's original "a couple of already-supported +# names for regression coverage" intent) -- this is the proof the fix is +# real: cpuf now correctly MATCHes at the controller layer instead of +# false-MISMATCHing. The alternative considered during task 6.2's review +# (keep the worker session connected across all per-layer checks by +# reordering calls) was already ruled out there as not viable without this +# `!`-escape mechanism -- xce_test's REPL has no command path from the +# `worker` context back to `client` without leave_ctx() (`..`/cmd_up), which +# disconnects exactly like `quit` does. See bug-1579 for the full history. # # 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` @@ -107,6 +146,12 @@ set -u SELF_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" REPO="$(cd "$SELF_DIR/../../.." && pwd)" +# Absolute path to this script itself -- used by inject_resources()'s `!` +# shell-escape (bug-1579 fix, see the top-of-file note) to invoke this same +# script recursively in `--internal-controller-check` mode, from INSIDE the +# xce_test worker session's `system()` call, while that session's connection +# to the controller is still open (before its `quit`). +SELF_SCRIPT="$SELF_DIR/$(basename "${BASH_SOURCE[0]}")" DEPLOY_HA="$REPO/src/tools/deploy-ha.sh" BUILD_DIR="${XCE_RESVERIFY_BUILD_DIR:-$REPO/src/xce.build}" @@ -144,7 +189,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 r1m=9.5 (numeric-only -- see + io=6.5 tmp=7.5 mem=8.5 cpuf=9.5 (numeric-only -- see bug-1574 note at top of file). EOF } @@ -352,14 +397,18 @@ 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). 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. +# is a documented, deliberate deviation, not an oversight). The two +# regression-coverage names are "mem" and "cpuf", matching design.md's +# original "a couple of already-supported names" intent -- "cpuf" is a +# worker.c N_HOST_ATTRS name and is restored here (bug-1579's real fix, see +# the top-of-file note) as live proof that the controller-side check no +# longer false-MISMATCHes a host-attrs resource: the nested `!`-escape +# controller check (internal_controller_check()) runs while the synthetic +# worker's connection is still open, so cpuf is never reset to baseline +# before it's checked. 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 r1m=9.5 + mem=8.5 cpuf=9.5 ) # Fixed synthetic worker identity for this run -- tags every resupdate'd @@ -415,9 +464,23 @@ inject_resources() { log "joining synthetic worker '$WORKER_HOST' (masters=$PEERS) and injecting: ${pairs[*]}" + # bug-1579 fix: run the controller-side check (internal_controller_check(), + # below) via a `!` shell-escape BEFORE `quit` -- while this session's + # worker connection is still open -- instead of in a separate xce_test + # process run after this session has already disconnected. `%q`-quote + # every interpolated value defensively (none of TOP/PEERS/WORKER_HOST/ + # resource names are expected to need it, but this line is re-parsed by + # a shell via xce_test's system() call, so quote anyway). + local ctrl_check_cmd + ctrl_check_cmd="bash $(printf '%q' "$SELF_SCRIPT") --internal-controller-check $(printf '%q' "$TOP") $(printf '%q' "$PEERS") $(printf '%q' "$WORKER_HOST")" + for pair in "${pairs[@]}"; do + name="${pair%%=*}" + ctrl_check_cmd="$ctrl_check_cmd $(printf '%q' "$name")" + done + local script - script="$(printf 'set masters %s\nset myname %s\nworker\njoin\nresupdate%s\nquit\n' \ - "$PEERS" "$WORKER_HOST" "$resupdate_line")" + script="$(printf 'set masters %s\nset myname %s\nworker\njoin\nresupdate%s\n!%s\nquit\n' \ + "$PEERS" "$WORKER_HOST" "$resupdate_line" "$ctrl_check_cmd")" out="$(printf '%s' "$script" | xtest)" @@ -475,7 +538,7 @@ $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) ────────────────── +# ── Step 5: controller-side verification check (task 6.2 + bug-1579 fix) ─── # 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 @@ -484,8 +547,105 @@ $out" # 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 +# query_controller_resvals() only REPORTS on names marked "accepted" in # $TOP/logs/injection-results.tsv. +# +# bug-1579 fix: the live `resval` query itself no longer happens here. It now +# happens EARLIER, nested inside inject_resources()'s own xce_test worker +# session via a `!` shell-escape (see internal_controller_check() below and +# the top-of-file bug-1579 note) -- for EVERY requested name, accepted or +# not, since accept/reject isn't known yet at that point in the sequence. +# query_controller_resvals() reads that nested query's raw results back from +# $TOP/logs/controller-check-raw.tsv and reports only on the names +# $TOP/logs/injection-results.tsv (available only now, after +# inject_resources() has returned and parsed the whole session's output) +# marks "accepted" -- a pure sequencing change: the MATCH/MISMATCH +# comparison, CONTROLLER_OBSERVED/CONTROLLER_CHECK_RESULTS population, and +# controller-check-results.tsv format below are all unchanged from task 6.2. + +# internal_controller_check TOP PEERS WORKER_HOST NAME... +# NOT a normal in-process function call -- invoked as a FRESH +# `bash $SELF_SCRIPT --internal-controller-check ...` process via +# inject_resources()'s `!` shell-escape line, from INSIDE the xce_test +# worker-context session's system() call, while the injecting synthetic +# worker's connection to the controller is STILL OPEN (i.e. before that same +# session's `quit`). This is what makes the controller-side check immune to +# the worker-disconnect host-attrs reset (worker.c's mark_worker_unavail() +# -> apply_worker_dead_mutation() -> reset_hostattrs_unknown() + +# add_hostattrs_resvals(), N_HOST_ATTRS) -- the worker never disconnects +# before this check runs. +# +# Queries resval for EVERY given name, not just ones that will turn out to be +# "accepted" -- cmd_resupdate()'s own accept/reject determination has already +# run inside THIS SAME xce_test process by this point and been printed to +# its stdout, but that text isn't available to this freshly-spawned process; +# accept/reject filtering happens later, in query_controller_resvals(), once +# the parent script has parsed the whole session's captured output. Writes +# RAW observed values (blank if not found) to +# $TOP/logs/controller-check-raw.tsv (name\tobserved) for +# query_controller_resvals() to read once inject_resources() returns. +# +# On a connection failure, records $TOP/logs/controller-check-error.txt +# instead (query_controller_resvals() fails the run on this, same failure +# semantics as task 6.2's original inline check) and deliberately still +# returns 0 either way: a non-zero exit here becomes this xce_test session's +# g_last_status and, in non-interactive/batch mode, ABORTS its REPL loop +# before `quit` runs (preprocess_and_run()'s shell-escape handling) -- +# undesirable when the failure can instead be surfaced through the raw-file/ +# error-file contract and let this session reach `quit` normally. +internal_controller_check() { + local top="$1" peers="$2" worker_host="$3" + shift 3 + local names=("$@") + + mkdir -p "$top/logs" + local raw_tsv="$top/logs/controller-check-raw.tsv" + local err_txt="$top/logs/controller-check-error.txt" + rm -f "$err_txt" + : > "$raw_tsv" + + [ "${#names[@]}" -gt 0 ] || return 0 + + local cmds="" name + for name in "${names[@]}"; do + cmds="${cmds}resval -n $name -m $worker_host"$'\n' + done + + local script out + 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 + printf '%s\n' "$out" > "$err_txt" + return 0 + fi + + # Same sequential state-machine parse query_controller_resvals() used to + # do inline pre-fix -- see its own historical comment (still applicable + # here) for why (bug-1576's short-name substring hazard, no trailing + # marker after the last query). + 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" + + for name in "${names[@]}"; do + printf '%s\t%s\n' "$name" "${parsed_values[$name]:-}" >> "$raw_tsv" + done + return 0 +} 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 ..." @@ -552,49 +712,28 @@ query_controller_resvals() { 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 + # The live `resval` query already ran EARLIER, nested inside + # inject_resources()'s own xce_test worker session via a `!` shell + # escape (bug-1579 fix -- see internal_controller_check() above), while + # the synthetic worker's connection to the controller was still open. + # This function just reads back what that nested query already + # collected -- see the "Step 5" comment block above. + local err_txt="$TOP/logs/controller-check-error.txt" + if [ -f "$err_txt" ]; then fail "controller check failed -- xce_test client context could not connect (masters=$PEERS); xce_test output: -$out" +$(cat "$err_txt")" 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 raw_tsv="$TOP/logs/controller-check-raw.tsv" + [ -f "$raw_tsv" ] || fail "controller check: $raw_tsv not found -- inject_resources()'s nested controller-check (internal_controller_check(), invoked via its xce_test session's \`!\` shell escape) did not run or did not complete" + + log "controller check: reporting on ${#accepted_names[@]} accepted resource(s) (queried during injection, see internal_controller_check()): ${accepted_names[*]}" + 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" + while IFS=$'\t' read -r name value; do + [ -n "$name" ] || continue + parsed_values["$name"]="$value" + done < "$raw_tsv" mkdir -p "$TOP/logs" local results_tsv="$TOP/logs/controller-check-results.tsv" @@ -846,4 +985,17 @@ main() { exit 0 } +# Hidden internal entry point (bug-1579 fix): inject_resources()'s `!` +# shell-escape line invokes this same script as `bash "$SELF_SCRIPT" +# --internal-controller-check TOP PEERS WORKER_HOST NAME...` from inside its +# own xce_test worker session. Intercepted here, BEFORE main()/parse_args() +# (which has no knowledge of this flag and would otherwise reject it as an +# "Unknown argument") -- runs only internal_controller_check(), none of the +# normal deploy/inject/trap flow. +if [ "${1:-}" = "--internal-controller-check" ]; then + shift + internal_controller_check "$@" + exit $? +fi + main "$@" diff --git a/.kiro/specs/resource-value-verification/tasks.md b/.kiro/specs/resource-value-verification/tasks.md index 6622bfc2..2a4a9b3e 100644 --- a/.kiro/specs/resource-value-verification/tasks.md +++ b/.kiro/specs/resource-value-verification/tasks.md @@ -116,7 +116,8 @@ - _Depends: 7.1_ ## Implementation Notes -- Task 6.3 confirmed (live, via a `cpuf` probe: controller MISMATCH + scheduler MATCH simultaneously) that the scheduler check is genuinely immune to bug-1579 — it reads task 4.1's trace log (written once, at apply-time, in the `RESVALS_DELTA` case only), never touched by a later worker-disconnect's `WORKERS_PUSH`-driven reset. **Real fix for bug-1579 identified, not yet applied** (see below) — the `!` escape in `xce_test` (`preprocess_and_run()`, `test.c` ~line 965) works in ANY context including `worker` mode, since it's handled in the single top-level REPL loop before context dispatch, not per-context. This means `inject_resources()` can stay in worker context (connection held open) after injecting, shell out via `!` to a separate `xce_test` client-mode process for the controller query, and only `quit` at the very end — fully closing bug-1579 (not just working around it in the default set) rather than needing a C-level `xce_test` change. Plan: retrofit this into `inject_resources()`/`query_controller_resvals()` as a follow-up before/alongside task 7.1, then restore a host-attrs resource (e.g. `cpuf`) to `DEFAULT_RESOURCE_ARGS` to prove it. +- **bug-1579 is now FULLY fixed** (retrofit on top of tasks 6.1/6.2, not a numbered task — user-requested follow-up). `inject_resources()`'s worker-context `xce_test` script now inserts a `!` line between `resupdate` and `quit`: `! bash