From f5390c14e22f39da6b8510fc10dd74dc0204d88c Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Tue, 18 Aug 2026 00:24:44 +0000 Subject: [PATCH] feat(resource-value-verification): per-name accept/reject verification in resupdate (task 2.2) cmd_resupdate() now issues a follow-up xcecli_resval() query per submitted (resource, worker) pair after the update RPC, since update_resval_list()'s reply can't carry per-item status. Reports accepted/rejected per name, with a bounded retry (5x100ms) against propagation races. Live-confirmed: `resupdate -n mem -v 5 -n not-a-real-resource -v 1` reports mem accepted and not-a-real-resource rejected in the same invocation. Co-Authored-By: Claude Sonnet 5 --- .../resource-value-verification/tasks.md | 3 +- src/controller/testsuite/test.c | 123 +++++++++++++++++- 2 files changed, 123 insertions(+), 3 deletions(-) diff --git a/.kiro/specs/resource-value-verification/tasks.md b/.kiro/specs/resource-value-verification/tasks.md index d38d01b0..ca91608c 100644 --- a/.kiro/specs/resource-value-verification/tasks.md +++ b/.kiro/specs/resource-value-verification/tasks.md @@ -23,7 +23,7 @@ - _Requirements: 1.1, 1.4_ - _Depends: 1.1_ -- [ ] 2.2 Implement per-name accept/reject verification in resupdate +- [x] 2.2 Implement per-name accept/reject verification in resupdate - After `cmd_resupdate()` sends the update RPC, issue one follow-up `xcecli_resval()` query per submitted `(resource, worker)` pair (the same client-library call `resval`/`cmd_resval` already uses) and compare the returned value against what was just sent - Report "accepted" for a matching value, "rejected: unknown resource" for a not-found result, with a short bounded retry before concluding rejection (guards against a genuinely-registered resource whose value hasn't propagated to the query path yet) - Report every submitted name's individual accept/reject outcome when a single invocation carries multiple `-n`/`-v` pairs @@ -118,4 +118,5 @@ ## Implementation Notes - Task 1.1: `resval_typeresolve.c`'s new function is `int resval_resolve_type(const char* name, int type_override)`, returning a `Value_u_which` tag or `RESVAL_TYPE_UNKNOWN` (-1, doesn't collide with the real 0-6 enum range). Task 2.1 should build on this exact signature rather than inventing a new one. `test.c`'s `set_resval()`/`cmd_resupdate()` were deliberately left untouched by task 1.1 — task 2.1 is the one that makes `set_resval()` a thin wrapper. - `controller_unit_test` (the big legacy Unity binary) has a pre-existing, unrelated flaky failure in `test_config_repl.c` (bug-1561, overlayfs `fdatasync errno=5` sandbox artifact) that shows up on every `/xce-build controller` run in this environment. It is NOT caused by this feature's changes — do not chase it in later tasks; cross-check `.wolf/buglog.json`'s bug-1561 entry before treating a `controller_unit_test` failure as real, and confirm via `git status` that `test_config_repl.c`/`config_repl.c` are untouched by the current task's diff. +- Task 2.2: `src/controller/meson.build` compiles `test.c` only into the `xce_test` binary (`xce_test_src`), never into `controller_unit_test` (`controller_unit_test_src`) — a `test.c`-only diff structurally cannot regress `controller_unit_test`'s results. Also: `controller_unit_test` is built with ASAN `detect_leaks=0` as pre-existing, repo-wide meson policy (not something any task in this feature turned off) — to actually check a `test.c` change for new leaks, run `xce_test` directly with leak detection left ON, don't rely on the meson suite's leak-detection status. - Task 2.1 (round 1 REJECTED, round 2 fixed): refactoring a getopt-loop's flush/finalization timing (needed to let a trailing flag like `-t` apply to its preceding `-n`/`-v` pair) can silently drop an unrelated existing guard (here: duplicate-`-v` rejection) that depended on the old immediate-finalization timing. When touching `cmd_resupdate()`-shaped CLI parsers in this codebase again, explicitly re-trace EVERY branch of the old getopt loop against the new one, not just the branches directly related to the new flag — a "traced every path" claim is only as good as the list of paths actually enumerated. `xce_test` also needs `ASAN_OPTIONS=detect_leaks=0` for reliable non-interactive stdin-piped smoke testing (its default leak-detection exit path can swallow buffered stdout). diff --git a/src/controller/testsuite/test.c b/src/controller/testsuite/test.c index 93554689..43920b4e 100644 --- a/src/controller/testsuite/test.c +++ b/src/controller/testsuite/test.c @@ -3364,12 +3364,116 @@ static int set_resval( vals[idx].host = get_setting("myname"); return rc; } + +/* + * resource-value-verification task 2.2: the controller's own update RPC + * reply can never distinguish per-name accept/reject -- update_resval_list() + * (src/controller/worker.c:185-198) unconditionally returns success + * regardless of any per-item failure (design.md's "set_resval()/ + * cmd_resupdate" component, Responsibilities & Constraints). To recover + * per-name accept/reject (Requirements 1.2, 1.3), cmd_resupdate() issues one + * follow-up xcecli_resval() query per submitted (resource, worker) pair -- + * the exact client-library call resval/cmd_resval already use -- and + * compares the returned value against what was just sent. + */ +#define RESVAL_VERIFY_MAX_ATTEMPTS (5) +#define RESVAL_VERIFY_RETRY_MS (100L) + +/* Compare a just-submitted value against a follow-up query's result. Both + * sides are xce_val_t (wire value union) -- a match requires the same + * `which` tag and an equal payload. */ +static int resval_value_matches(xce_val_t* sent, xce_val_t* got) +{ + if ((sent == NULL) || (got == NULL) || (sent->which != got->which)) { + return 0; + } + + switch (sent->which) { + case Value_u_ivalue32: + return sent->u.ivalue32 == got->u.ivalue32; + case Value_u_uvalue32: + return sent->u.uvalue32 == got->u.uvalue32; + case Value_u_ivalue64: + return sent->u.ivalue64 == got->u.ivalue64; + case Value_u_uvalue64: + return sent->u.uvalue64 == got->u.uvalue64; + case Value_u_svalue: + return EQUAL_STRING(sent->u.svalue, got->u.svalue); + case Value_u_fvalue: + return float_equal(sent->u.fvalue, got->u.fvalue); + case Value_u_bvalue: + return (sent->u.bvalue == 0) == (got->u.bvalue == 0); + default: + return 0; + } +} + +/* + * Query a just-submitted (name, host) pair back from the controller and + * print its per-name accepted/rejected verdict (Requirements 1.2, 1.3). A + * short bounded retry guards against a genuinely-registered resource whose + * value hasn't propagated to the query path yet (design.md's Implementation + * Notes: sub-second in practice for a controller-local query) -- NOT an + * arbitrary long poll. + */ +static void resupdate_report_one(char* name, char* host, xce_val_t* sent) +{ + int attempt; + int ever_found = 0; + + if (name == NULL) { + /* set_resval()'s Value_u_svalue fallback path leaves .name unset + * (pre-existing gap predating this task -- see set_resval()'s own + * comment above); nothing usable to query or report here. */ + return; + } + + for (attempt = 0; attempt < RESVAL_VERIFY_MAX_ATTEMPTS; attempt++) { + xce_resval_t** got = NULL; + int rc; + + if (attempt > 0) { + usleep((useconds_t)(RESVAL_VERIFY_RETRY_MS * US_PER_MS)); + } + + rc = xcecli_resval(active_ctx->ctx, host, name, &got); + if (rc > 0) { + int matched = resval_value_matches(sent, got[0]->val); + int i; + + ever_found = 1; + + for (i = 0; i < rc; i++) { + free_ResourceVal_ptr(&got[i]); + } + FREEUP(got); + + if (matched) { + printf("%s: accepted\n", name); + return; + } + /* Found but the value differs -- could still be a stale value + * about to be overwritten by the just-sent update's propagation, + * so keep retrying within the bounded budget rather than giving + * up immediately. */ + continue; + } + } + + if (ever_found) { + printf("%s: rejected: value mismatch\n", name); + } else { + printf("%s: rejected: unknown resource\n", name); + } +} + int cmd_resupdate(int argc, char* argv[]) { #define MAX_N_RES (16) int nres; xce_resval_t vals[MAX_N_RES]; xce_resval_t* valptrs[MAX_N_RES]; + char* subnames[MAX_N_RES]; char* resname = NULL; char* resval_str = NULL; int type_override = RESVAL_TYPE_UNKNOWN; @@ -3394,7 +3498,12 @@ int cmd_resupdate(int argc, char* argv[]) } rc = set_resval(vals, nres, resname, resval_str, type_override); if (rc >= 0) { - valptrs[nres] = &vals[nres]; + valptrs[nres] = &vals[nres]; + /* Tracked independently of vals[nres].name: set_resval() + * leaves .name unset on the Value_u_svalue fallback path + * (see its own comment), but `resname` here is always the + * exact name that was actually submitted for this pair. */ + subnames[nres] = resname; nres++; } resname = NULL; @@ -3436,14 +3545,24 @@ int cmd_resupdate(int argc, char* argv[]) if (resname != NULL && resval_str != NULL) { rc = set_resval(vals, nres, resname, resval_str, type_override); if (rc >= 0) { - valptrs[nres] = &vals[nres]; + valptrs[nres] = &vals[nres]; + subnames[nres] = resname; nres++; } } rc = xcecli_update_resvals(active_ctx->ctx, nres, &valptrs[0]); if (rc >= 0) { + int i; + printf("resource update ok!\n"); + + /* The update RPC's own reply can't carry per-name accept/reject + * (see the comment above resupdate_report_one()) -- verify each + * submitted pair individually via a follow-up query instead. */ + for (i = 0; i < nres; i++) { + resupdate_report_one(subnames[i], vals[i].host, vals[i].val); + } } else { SET_ERROR(-rc); perror("xcecli_update_resval");