From 75a0839d473ab597517a09b7be83e32b6708cc87 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Tue, 18 Aug 2026 08:36:56 +0000 Subject: [PATCH] feat(resource-value-verification): worker join and resource injection (task 6.1) inject_resources() joins a synthetic worker through xce_test's worker context and runs resupdate for the requested resource=value pairs (default: Requirement 1's numeric gap set + mem/cpuf regression names, deliberately excluding type/model due to bug-1574's live segfault risk). Per-name accept/reject results captured into INJECTED_VALUES/ INJECTED_RESULTS plus injection-results.tsv for tasks 6.2-6.4. Also suppresses the real xcecd_virtual worker daemon that deploy-ha.sh's pre-existing worker branch would otherwise silently start whenever the build dir contains one (XCE_WORKER_BIN=/bin/true), which would have violated Requirement 2.1 - bug-1577. Blocked pending a separate prerequisite fix (bug-1578, commit b406dd7): live testing surfaced a pre-existing controller crash on worker disconnect, unrelated to this feature, now fixed with regression coverage. Live- reconfirmed post-fix: controller survives the full join+resupdate+quit flow indefinitely. Co-Authored-By: Claude Sonnet 5 --- .../xce-test-resources/run-resource-verify.sh | 223 +++++++++++++++--- .../resource-value-verification/tasks.md | 4 +- 2 files changed, 198 insertions(+), 29 deletions(-) diff --git a/.claude/skills/xce-test-resources/run-resource-verify.sh b/.claude/skills/xce-test-resources/run-resource-verify.sh index 2d61e70d..00e7ea9d 100755 --- a/.claude/skills/xce-test-resources/run-resource-verify.sh +++ b/.claude/skills/xce-test-resources/run-resource-verify.sh @@ -5,17 +5,30 @@ # 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 SCOPE: this file currently -# implements ONLY the deployment step -- 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, and clean up (trap-based, covering a -# mid-deployment abort, not just a completed run). Injection (task 6.1), -# per-layer verification (tasks 6.2-6.4), and verdict aggregation / +# 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 +# (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 / # 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 5.3, 6.1, 6.5 for this task; the full skill covers 3.1-6.5. +# Requirements 2.1, 2.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 +# 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. # # 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` @@ -38,16 +51,22 @@ # convention. $XCE_RESVERIFY_TOP overrides the default # when --dir is not given (same precedence pattern as # xce-test-ha's $XCE_HA_TOP). -# resource=value NOT YET IMPLEMENTED (task 6.1 -- worker join + -# injection). Accepted and parsed here so argument -# handling doesn't need reshaping later, but currently -# unused by this task's deployment-only scope. +# resource=value One or more resources to inject via resupdate, e.g. +# `r15m=1.5 pg=3`. May repeat. When none are given, +# defaults to Requirement 1's gap-resource set plus a +# couple of already-numeric names for regression +# coverage (see the top-of-file bug-1574 note for why +# the default deliberately excludes type/model). # # Exit 0 == this task's scope succeeded (deployment stood up, all three -# components confirmed up, cleaned up per --keep-running). Non-zero == -# deployment failed at some step (see the printed "deployment failed at -# step X" message). Once tasks 6.1-7.1 land, the exit code instead reflects -# the injection/verification PASS/FAIL verdict. +# 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. # set -u @@ -59,6 +78,10 @@ BUILD_DIR="${XCE_RESVERIFY_BUILD_DIR:-$REPO/src/xce.build}" BINARY="${XCE_RESVERIFY_BIN:-$BUILD_DIR/controller/xce_controller}" SCHED_BIN="${XCE_RESVERIFY_SCHED_BIN:-$BUILD_DIR/scheduler/xce_scheduler}" APISERVER_BIN="${XCE_RESVERIFY_APISERVER_BIN:-/tmp/apiserver}" +# xce_test -- drives injection (task 6.1) via its worker-context `join`/ +# `resupdate` commands (same binary xce-test-ha's run-ha-soak.sh uses, same +# build dir as $BINARY since both come from the controller meson target). +XT="${XCE_RESVERIFY_XT:-$BUILD_DIR/controller/xce_test}" # Single-node cluster, deliberately -- this skill verifies resource-value # PROPAGATION (controller -> scheduler -> apiserver), not raft/HA failover @@ -84,8 +107,10 @@ Usage: $0 [--keep-running] [--dir DIR] [resource=value ...] always preserved either way. --dir DIR scratch/deploy directory (default: fresh mktemp -d, or \$XCE_RESVERIFY_TOP). - resource=value reserved for task 6.1 (injection); accepted but not yet - acted on by this task's deployment-only scope. + 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 + bug-1574 note at top of file). EOF } @@ -200,6 +225,7 @@ precondition_checks() { [ -x "$BINARY" ] || fail "controller binary missing: $BINARY (run /xce-build controller)" [ -x "$SCHED_BIN" ] || fail "scheduler binary missing: $SCHED_BIN (run /xce-build scheduler) -- required, this skill verifies the scheduler layer" [ -x "$APISERVER_BIN" ] || fail "apiserver binary missing: $APISERVER_BIN (cd src/frontend && make build-server, or the CGO-flags workaround -- see tasks.md's Implementation Notes) -- required, this skill verifies the apiserver layer" + [ -x "$XT" ] || fail "xce_test binary missing: $XT (run /xce-build controller) -- required to join a synthetic worker and run resupdate" command -v nix >/dev/null 2>&1 || fail "nix not found on PATH -- required to resolve the deploy-environment bash and invoke deploy-ha.sh" } @@ -221,7 +247,26 @@ deploy() { # ── Step 2: start-all.sh -- controller -> health-check -> scheduler/apiserver start_all() { - log "invoking start-all.sh (controller, scheduler at -vvv, apiserver with debug logging)" + # deploy-ha.sh's Step 3 also generates + start-all.sh also launches a + # REAL worker daemon (xcecd_virtual) whenever one happens to exist + # alongside $BINARY in the build dir (its own pre-existing, opt-in-by- + # presence worker/scheduler convention, predating this feature -- see + # task 3.1's notes). This skill's whole point (Requirement 2.1) is that + # NO real worker daemon process is needed -- task 6.1 injects via a + # SYNTHETIC xce_test-joined worker instead -- and a real worker sitting + # in the background for this run's whole duration is worse than just + # off-contract: it periodically re-reports its OWN resources (~every + # 15s), and a resource's SECOND periodic report is bug-1572 (open, + # unfixed scheduler double-free/crash) -- a real background worker is a + # live crash risk to this run, not just noise. start-worker.sh reads + # $XCE_WORKER_BIN itself (falling back to the real xcecd_virtual path) + # -- point it at a real no-op binary so start-all.sh's worker step exits + # immediately instead of leaving a daemon running; deploy-ha.sh itself + # is untouched (out of boundary), this only affects what THIS skill's + # own start-all.sh invocation actually launches. + export XCE_WORKER_BIN="/bin/true" + + log "invoking start-all.sh (controller, scheduler at -vvv, apiserver with debug logging; real worker daemon suppressed via XCE_WORKER_BIN=/bin/true -- see comment above)" if ! "$BASH_BIN" "$TOP/scripts/start-all.sh"; then fail "deployment failed at step start-all.sh (nonzero exit)" fi @@ -267,6 +312,130 @@ confirm_deployment_up() { log "all three components confirmed up under $TOP" } +# ── Step 4: join a synthetic worker + inject resource values (task 6.1) ──── +# Requirement 1's gap-resource set (numeric) plus a couple of already-numeric +# 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). +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 +) + +# Fixed synthetic worker identity for this run -- tags every resupdate'd +# value (Requirement 2.2), exactly like `set myname ` + `worker` + +# `join` in xce-test-ha's run-ha-soak.sh (identical established pattern). +# A fresh $TOP per invocation means no cross-run collision risk. +WORKER_HOST="resverify-worker" + +declare -A INJECTED_VALUES=() # resource name -> injected value, as sent +declare -A INJECTED_RESULTS=() # resource name -> "accepted" | "rejected: " | "rejected: no result line observed" + +# Run a batch of xce_test commands (stdin script). Two things this MUST do, +# both hard-won (see cerebrum.md / buglog.json): +# - ASAN_OPTIONS=detect_leaks=0: xce_test is an ASAN build with small +# pre-existing intentional REPL-lifetime leaks; its default leak-detect +# exit path can swallow buffered stdout on a piped/redirected run, +# losing the very accept/reject lines this function parses. +# - wrapped in `nix develop` (like deploy()'s own BASH_BIN resolution +# above): unlike $BINARY/$SCHED_BIN/$APISERVER_BIN (started via +# generated scripts whose binaries carry a baked RPATH), xce_test's own +# runtime linkage has not been confirmed independent of the nix +# devShell's library path -- wrap defensively, mirroring deploy()'s own +# "shell out to nix develop for the steps that need it" posture. +xtest() { + nix develop "$REPO/src" --command env ASAN_OPTIONS=detect_leaks=0 timeout 60 "$XT" 2>&1 +} + +# inject_resources [resource=value ...] +# No args -> DEFAULT_RESOURCE_ARGS. Joins one synthetic worker (no real +# worker daemon process anywhere in this flow -- Requirement 2.1) and runs +# ONE resupdate invocation carrying every pair, so task 2.2's per-name +# accept/reject accounting covers the whole batch in a single RPC round +# trip. Populates INJECTED_VALUES/INJECTED_RESULTS (and $TOP/logs/ +# injection-results.tsv) for tasks 6.2-6.4's extension points to consume. +inject_resources() { + local pairs=("$@") + local pair name value resupdate_line out result + + if [ "${#pairs[@]}" -eq 0 ]; then + pairs=("${DEFAULT_RESOURCE_ARGS[@]}") + fi + + resupdate_line="" + for pair in "${pairs[@]}"; do + name="${pair%%=*}" + value="${pair#*=}" + if [ -z "$name" ] || [ "$name" = "$pair" ] || [ -z "$value" ]; then + fail "malformed resource argument (expected resource=value): $pair" + fi + INJECTED_VALUES["$name"]="$value" + resupdate_line="$resupdate_line -n $name -v $value" + done + + log "joining synthetic worker '$WORKER_HOST' (masters=$PEERS) and injecting: ${pairs[*]}" + + local script + script="$(printf 'set masters %s\nset myname %s\nworker\njoin\nresupdate%s\nquit\n' \ + "$PEERS" "$WORKER_HOST" "$resupdate_line")" + + out="$(printf '%s' "$script" | xtest)" + + printf '%s\n' "$out" | grep -Fxq -- "host join ok" \ + || fail "worker join failed -- no synthetic worker daemon process exists so this is the whole injection path; xce_test output: +$out" + + for pair in "${pairs[@]}"; do + name="${pair%%=*}" + if printf '%s\n' "$out" | grep -Fxq -- "${name}: accepted"; then + result="accepted" + elif printf '%s\n' "$out" | grep -Fxq -- "${name}: rejected: unknown resource"; then + result="rejected: unknown resource" + elif printf '%s\n' "$out" | grep -Fxq -- "${name}: rejected: value mismatch"; then + result="rejected: value mismatch" + else + result="rejected: no result line observed" + fi + INJECTED_RESULTS["$name"]="$result" + log "resupdate: $name=${INJECTED_VALUES[$name]} -> $result" + done + + # Manifest for post-run inspection and for tasks 6.2-6.4 to consume + # (this same script process already has INJECTED_VALUES/INJECTED_RESULTS + # in memory; the file is a durable, human-inspectable mirror of the + # same data, alongside the always-preserved logs/ convention). + mkdir -p "$TOP/logs" + { + for pair in "${pairs[@]}"; do + name="${pair%%=*}" + printf '%s\t%s\t%s\n' "$name" "${INJECTED_VALUES[$name]}" "${INJECTED_RESULTS[$name]}" + done + } > "$TOP/logs/injection-results.tsv" + + # This flow never starts a real worker daemon (xcecd/xcecd_virtual) -- + # structurally guaranteed by construction (the only process this + # function starts is xce_test itself, and only its worker CONTEXT, not + # a separate daemon binary). Confirm explicitly rather than just assert + # it in a comment, since Requirement 2.1/6.4's whole point is "no real + # worker daemon process required". + if pgrep -f -- 'xcecd_virtual|/xcecd( |$)' >/dev/null 2>&1; then + log "note: a process matching xcecd/xcecd_virtual is running on this machine -- NOT started by this script (it never invokes those binaries), so unrelated to this run; investigate separately if unexpected" + fi + + local failed=0 + for pair in "${pairs[@]}"; do + name="${pair%%=*}" + case "${INJECTED_RESULTS[$name]}" in + "rejected: no result line observed") + log "FAIL: '$name' produced no observable accept/reject outcome -- structural injection failure, not a legitimate rejection" + failed=1 + ;; + esac + done + [ "$failed" -eq 0 ] || fail "injection did not produce a definitive accept/reject outcome for every requested resource (see above)" +} + main() { parse_args "$@" resolve_top @@ -284,20 +453,18 @@ main() { start_all confirm_deployment_up - # --- Extension point (task 6.1): join a synthetic worker via xce_test's - # worker context and run resupdate for RESOURCE_ARGS (or the default - # gap-resource set if RESOURCE_ARGS is empty) here. --- + inject_resources "${RESOURCE_ARGS[@]}" + # --- Extension point (tasks 6.2-6.4): controller/scheduler/apiserver - # per-layer verification checks here. --- + # 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). --- # --- 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. --- - if [ "${#RESOURCE_ARGS[@]}" -gt 0 ]; then - log "note: resource=value arguments (${RESOURCE_ARGS[*]}) were accepted but are not yet acted on -- injection lands in task 6.1" - fi - - log "PASS (task 5.1 scope): controller, scheduler, and apiserver confirmed up under $TOP; cleaning up" + 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" exit 0 } diff --git a/.kiro/specs/resource-value-verification/tasks.md b/.kiro/specs/resource-value-verification/tasks.md index 94c71093..c78078a8 100644 --- a/.kiro/specs/resource-value-verification/tasks.md +++ b/.kiro/specs/resource-value-verification/tasks.md @@ -63,7 +63,7 @@ - _Depends: 1.2, 3.1, 4.1, 4.2_ - [ ] 6. Core: resource injection and per-layer verification checks -- [ ] 6.1 Implement worker join and resource injection +- [x] 6.1 Implement worker join and resource injection - Join a synthetic worker through the `xce_test` worker context (no real worker daemon process), then run `resupdate` for one or more resource names/values - Observable completion: after this step runs, `resupdate` has been accepted for every requested resource name with zero real worker daemon processes running - _Requirements: 2.1, 2.2, 6.1, 6.4_ @@ -116,6 +116,8 @@ - _Depends: 7.1_ ## Implementation Notes +- 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. - Task 4.2: the apiserver's trace log line (task 6.4 must grep for exactly this) is zap message `"resource-location stored"` with fields `resource` (string), `host` (string), `value` (float64 for numeric resources — the common case; string for `type`/`model`; bool for boolean-typed resources), e.g. `{"resource": "r15m", "host": "review-worker", "value": 1.75}`. Grep the log for `resource-location stored`, then parse `"resource": ""` / `"host": ""` / `"value": ` from the trailing field block. - **Task 4.2 discovered bug-1574, which will affect task 8.1 and possibly task 7.1's default resource set — read before implementing either**: `xce_test` can SEGFAULT on `resupdate -n type -v ...` / `resupdate -n model -v ...` (the two `Value_u_svalue`/string-typed resources). Root cause: `set_resval()`'s switch has no `Value_u_svalue` case, so `vals[idx].name` is left pointing at whatever garbage was already on `cmd_resupdate()`'s never-zero-initialized stack array — reading genuinely uninitialized stack memory, invisible to ASan (would need MemorySanitizer), and NON-DETERMINISTIC (task 2.2's reviewer saw `-n type -v foo` NOT crash under full ASAN; task 4.2's implementer saw `-n model -v CustomModelX` crash immediately — different argv/stack layout, same underlying UB). Task 1.2's `SKILL.md` scaffold already documents `type` as part of an illustrative default resource set "for string-path regression coverage" — **before task 7.1 finalizes the actual default resource set, either exclude `type`/`model` from any default until bug-1574 is fixed, or fix bug-1574 first** (it's a small, well-understood fix: zero-initialize `vals[]` in `cmd_resupdate()` and/or add the missing `Value_u_svalue` case to `set_resval()`'s value-population switch — out of boundary for tasks 4.1-4.2, in boundary for whichever task next touches `test.c`'s `cmd_resupdate()`/`set_resval()`, most likely as a prerequisite fix before task 8.1's live e2e run). A crash mid-run would make task 8.1's or the skill's own live verification flaky/non-deterministic, undermining the whole feature's purpose.