feat(resource-value-verification): add apiserver resval trace log (task 4.2)

One logger.Debug("resource-location stored", ...) call in
OnResourceLocationUpdated, immediately after store.ResourceLocations.Put,
logging resource/host/value as zap fields. Debug-level only, matches this
package's existing logging convention (reuses convert.go's field names).

Live-confirmed against a real deployment with logging.level: debug.

Also surfaced (but did not fix, out of boundary - pure Go change, no
possible causal link) a pre-existing xce_test segfault on string-typed
resupdate (type/model) - uninitialized stack memory, non-deterministic,
invisible to ASan. Logged as bug-1574; flagged in tasks.md as a risk for
task 7.1's default resource set and task 8.1's live e2e run.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Rongsong Shen 2026-08-18 02:51:34 +00:00
parent 04ecfa5cc1
commit 8b4fbffa51
Signed by: rshen
SSH key fingerprint: SHA256:8Vf2pZ3h6rJa7EhGK+WaAQ9Mta/apZvteW6g9U/WKKk
2 changed files with 24 additions and 1 deletions

View file

@ -48,7 +48,7 @@
- Observable completion: a live scheduler daemon's log file contains one line per applied resource update, in the format task 6.3 will grep for; existing scheduler tests are unaffected
- _Requirements: 4.1, 4.2, 4.3_
- [ ] 4.2 (P) Add a trace log line to the apiserver's resource-update storage handling
- [x] 4.2 (P) Add a trace log line to the apiserver's resource-update storage handling
- In `OnResourceLocationUpdated` (`src/frontend/apiserver/backend/handler.go:92-95`), add one `logger.Debug(...)` call (zap, per this component's existing convention) immediately after `store.ResourceLocations.Put(key, *res)`, logging the resource name, location/host, and stored value
- Confirm existing apiserver backend tests still pass unchanged — same rationale as 4.1, no new unit test required for a pure log addition; content/format confirmed live in task 8.1
- Observable completion: a live apiserver's log file contains one line per stored resource-location update, in the format task 6.4 will grep for; existing backend tests are unaffected
@ -116,6 +116,9 @@
- _Depends: 7.1_
## Implementation Notes
- 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": "<name>"` / `"host": "<host>"` / `"value": <val>` 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.
- Consolidated list of pre-existing (not-fixed-here) bugs surfaced by this feature's own live testing so far: bug-1571 (apiserver `RegisterToBackend` gate checks `Address` before `Peers` — worked around in generated config, not source, by task 3.1), bug-1572 (scheduler double-free/crash on a resource's 2nd periodic report — task 4.1), bug-1573 (see buglog.json, task 4.1), bug-1574 (`xce_test` segfault on string-typed `resupdate` — task 4.2, described above).
- Task 4.1: the scheduler's trace log line format (task 6.3 must grep for exactly this) is `resval applied host=<H> resource=<R> value=<V>` at `DEBUG3` level (requires `-vvv`, per task 3.1's `start-all.sh`). Numeric resources use `%f` (fixed 6-decimal, e.g. `12345.500000` — parse-and-compare as float, not exact string match); string-typed resources (`type`/`model`) use `%s`. Any other `Value_u_which` tag (e.g. `Value_u_bvalue`, boolean resources) is silently NOT logged by this line (matches this file's own sibling-function convention) — not relevant to this feature's resource set, which is entirely fvalue/svalue.
- Task 4.1 (round 2) discovered and logged, but deliberately did NOT fix (out of boundary, confirmed pre-existing since 2026-08-03/04, unrelated to this feature): **bug-1572** — the live scheduler daemon double-frees and silently crashes on a worker's SECOND periodic resource report for the same resource (`core_events.c`'s RESVALS_DELTA case stores `rv->val` into `w->resvals` without `MOVE_PTR`, while `driver.c`'s `release_translated_payload()` unconditionally frees the same list). This feature's own resupdate-based injection flow (tasks 5.1+) should avoid resupdate-ing the exact same resource name twice against the same live scheduler within one run until bug-1572 is fixed separately, or the live e2e task (8.1) could intermittently crash the scheduler mid-run.
- 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.

View file

@ -92,6 +92,26 @@ func (h *DefaultHandler) OnResourceUpdated(res *models.ResourceDefinitionInfo) {
func (h *DefaultHandler) OnResourceLocationUpdated(res *models.ResourceDistributionInfo) {
key := res.ResourceName + ":" + res.Location
store.ResourceLocations.Put(key, *res)
// resource-value-verification task 4.2 (Requirements 5.1, 5.2, 5.4): trace
// the stored value at its point of application, mirroring the scheduler's
// sibling trace log added in task 4.1 (core_events.c). One typed "value"
// field carrying whichever of StringValue/NumericValue/BooleanValue is
// live for res.ValueType, so a downstream grep sees exactly what was
// stored, not every possibly-zero field on the struct.
var valueField zap.Field
switch res.ValueType {
case int(models.STRING):
valueField = zap.String("value", res.StringValue)
case int(models.BOOLEAN):
valueField = zap.Bool("value", res.BooleanValue)
default:
valueField = zap.Float64("value", res.NumericValue)
}
logger.Debug("resource-location stored",
zap.String("resource", res.ResourceName),
zap.String("host", res.Location),
valueField)
}
// dispatchJobArray delivers a locally-expanded compact array push to the