Add HostConfig + Cluster fields to models and update docs

2026-04-16 09:24:22 +00:00 · 2026-04-16 09:24:22 +00:00 · 00c4472d6c
commit 00c4472d6c
parent 960ef62342
9 changed files with 1805 additions and 9 deletions
--- a/.gitignore
+++ b/.gitignore
@ -16,3 +16,5 @@ src/frontend/xcecli/xcecli
 # Logs and tokens (runtime state)
 src/frontend/apiserver/logs/
 src/frontend/apiserver/tokens/
+# Added by code-review-graph
+.code-review-graph/
--- a/CLAUDE.md
+++ b/CLAUDE.md
@ -66,3 +66,42 @@ Request → Gin Router → Middleware (CORS/RateLimit/IPWhitelist/JWT) → RBAC
 - `src/worker/` — See `src/worker/CLAUDE.md`
 - `src/scheduler/` — See `src/scheduler/CLAUDE.md`
 - `src/lib/` — See `src/lib/CLAUDE.md`
+
+<!-- code-review-graph MCP tools -->
+## MCP Tools: code-review-graph
+
+**IMPORTANT: This project has a knowledge graph. ALWAYS use the
+code-review-graph MCP tools BEFORE using Grep/Glob/Read to explore
+the codebase.** The graph is faster, cheaper (fewer tokens), and gives
+you structural context (callers, dependents, test coverage) that file
+scanning cannot.
+
+### When to use graph tools FIRST
+
+- **Exploring code**: `semantic_search_nodes` or `query_graph` instead of Grep
+- **Understanding impact**: `get_impact_radius` instead of manually tracing imports
+- **Code review**: `detect_changes` + `get_review_context` instead of reading entire files
+- **Finding relationships**: `query_graph` with callers_of/callees_of/imports_of/tests_for
+- **Architecture questions**: `get_architecture_overview` + `list_communities`
+
+Fall back to Grep/Glob/Read **only** when the graph doesn't cover what you need.
+
+### Key Tools
+
+| Tool | Use when |
+|------|----------|
+| `detect_changes` | Reviewing code changes — gives risk-scored analysis |
+| `get_review_context` | Need source snippets for review — token-efficient |
+| `get_impact_radius` | Understanding blast radius of a change |
+| `get_affected_flows` | Finding which execution paths are impacted |
+| `query_graph` | Tracing callers, callees, imports, tests, dependencies |
+| `semantic_search_nodes` | Finding functions/classes by name or keyword |
+| `get_architecture_overview` | Understanding high-level codebase structure |
+| `refactor_tool` | Planning renames, finding dead code |
+
+### Workflow
+
+1. The graph auto-updates on file changes (via hooks).
+2. Use `detect_changes` for code review.
+3. Use `get_affected_flows` to understand impact.
+4. Use `query_graph` pattern="tests_for" to check coverage.
--- a/src/docs/apiserver-backend-enhancement-plan.md
+++ b/src/docs/apiserver-backend-enhancement-plan.md
--- a/src/docs/apiserver-backend-gaps.md
+++ b/src/docs/apiserver-backend-gaps.md
@ -0,0 +1,225 @@
+# API Server Backend Gap Analysis
+
+Source: `src/controller/testsuite/test.c` (C test tool) vs `src/frontend/apiserver/` (Go API server)
+
+## Overview
+
+The C test tool (`test.c`) directly calls `xcecli_*` C library functions to interact
+with the controller. The Go API server currently operates in a **push-based, read-only-from-store**
+model: the controller pushes data (jobs, workers, queues, resources) via Cap'n Proto, and
+the API server reads from its in-memory stores. Write/control operations are forwarded
+via the CGO backend layer, but only a subset of the C API is wired up.
+
+---
+
+## Currently Implemented (Backend Forwards to C Controller)
+
+| C Test Command | C API Call             | Go Backend Function     | REST Endpoint                      |
+|----------------|------------------------|-------------------------|------------------------------------|
+| `jobsub`       | `xcecli_jobsubmit()`   | `backend.SubmitJob()`   | `POST /xce/v1/jobs/create`        |
+| `jobkill/jobckpt` | `xcecli_jobctrl()`  | `backend.ControlJob()` | `POST /xce/v1/jobs/:jobids/control` |
+| `qctrl`        | `xcecli_queuectrl()`   | `backend.ControlQueue()`| `POST /xce/v1/queues/control`     |
+| *(partial)*    | `xcecli_hostctrl()` (N/A) | `backend.ControlHost()` **stub only** | `POST /xce/v1/hosts/control` |
+
+Push notifications (read path, already working via `handlePush`):
+
+- Job updates via `OnJobUpdated` -> `store.Jobs`
+- Worker/host updates via `OnHostUpdated` -> `store.Hosts`
+- Queue updates via `OnQueueUpdated` -> `store.Queues`
+- Resource definitions via `OnResourceUpdated` -> `store.Resources`
+- Resource locations via `OnResourceLocationUpdated` -> `store.ResourceLocations`
+
+---
+
+## NOT Implemented -- Backend Request Gaps
+
+### 1. Resource Value Query (`cmd_resval` -> `xcecli_resval()`)
+
+- **What it does**: Queries current numeric/string values of a named resource on a
+  specific host (or cluster-wide). Returns typed values (float, string, bool).
+- **REST impact**: No `GET /xce/v1/resources/values` endpoint exists. The resource
+  controller only serves definitions and distributions, not live values.
+- **Priority**: **High** -- Essential for monitoring dashboards to show real-time
+  `mem`, `r1m`, `slots` etc.
+
+[feedback] api server will not pull resources from controller, it just waits controller to push
+           updated resource information, so api server just handle resource information update will be OK. 
+
+### 2. Resource Value Update (`cmd_resupdate` -> `xcecli_update_resvals()`)
+
+- **What it does**: Worker pushes updated resource values to the controller.
+- **REST impact**: No equivalent. Currently workers call the C API directly. If the
+  API server should support an admin endpoint to update resource values (e.g., for
+  testing or override), this is missing.
+- **Priority**: **Low-Medium** -- Workers typically call this directly; rarely needed
+  via REST. Could be useful for admin overrides.
+
+[feedback] This item will not be applied to api server, resource update function need to be
+           used to update resources information of worker node(such as cpu/memory/..., and api
+           server will not talke workload, so it do not need this)
+
+### 3. Job Dispatch (`cmd_jobrun` -> `xcecli_jobdispatch()`)
+
+- **What it does**: Dispatches a job to specific hosts with allocation info
+  (host:ntasks pairs). This is how the scheduler tells the controller to start
+  running a job on specific workers.
+- **REST impact**: Not exposed. The `POST /xce/v1/jobs/create` handles submission,
+  but **dispatch** (assignment to hosts) is a separate operation currently done by
+  the scheduler directly.
+- **Priority**: **Low** -- Dispatch is an internal scheduler->controller operation.
+  Not typically user-facing.
+
+[feedback] treat this as low priority, IBM LSF has a brun command which issued by user to 
+           force a job to run in given host, we may need this. also cmd_jobrun() has
+           been used to test execution flow/protocol between controller and work daemon
+
+### 4. Job Info / Single Job Detail (`cmd_jobinfo` -> `xcecli_jobinfo()`)
+
+- **What it does**: Queries detailed info for a single job by ID, including pend
+  reasons.
+- **REST impact**: The `GET /xce/v1/jobs/:jobids` endpoint reads from the in-memory
+  store, which is populated by push notifications. This works for already-known jobs
+  but **cannot query a job that hasn't been pushed yet**. The C `xcecli_jobinfo()` is
+  a synchronous request-response to the controller -- fundamentally different from
+  the store-read pattern.
+- **Priority**: **Medium** -- Store-based reads are sufficient for most cases. The
+  sync query path would only matter if pushes are delayed or the store is stale.
+
+[feedback] no need this, as job information will be pushed to api server
+
+### 5. Cluster Configuration (`cmd_getconf` -> `xcecli_clusterconf()`)
+
+- **What it does**: Synchronously fetches the full cluster config (resources, queues,
+  workers, host groups, user groups, global configs).
+- **REST impact**: Currently the API server relies entirely on push notifications to
+  populate stores. There's no way to do a one-shot **full sync** with the controller.
+  On startup, it joins via `xcecli_join(HostRole_apiserver)` but doesn't explicitly
+  request the current state.
+- **Priority**: **High** -- Without an initial sync, if the API server restarts
+  mid-session, it loses all state until the next push. The store is stale until a
+  new push arrives.
+
+[feedback] api server need handle pushed cluster configuration
+
+### 6. Host Control (`backend.ControlHost()` -> `xcecli_hostctrl()`)
+
+- **What it does**: The C library doesn't expose `xcecli_hostctrl()` yet (the Go
+  stub returns an error).
+- **REST impact**: `POST /xce/v1/hosts/control` exists but will always fail:
+  `"host control not yet supported via C library"`.
+- **Priority**: **High** -- Users cannot open/close hosts via the API.
+
+[feedback] we need this
+
+### 7. Set Pending Reason (`cmd_setpend` -> `xcecli_set_pend_reason()`)
+
+- **What it does**: Scheduler sets pend reasons on a job (host-based or
+  resource-based).
+- **REST impact**: No equivalent endpoint. Job pend reasons are currently populated
+  only via push data.
+- **Priority**: **Low** -- This is a scheduler->controller internal operation.
+
+[feedback] set pending reason is for scheduler to set why job will be pend, this is 
+           used for test scheduler to controller job pending api
+
+### 8. Run Tasks (`cmd_runtasks` -> `xcecli_runtask()`)
+
+- **What it does**: Launches individual tasks on specific hosts (not a full job, but
+  sub-task execution).
+- **REST impact**: No endpoint exists.
+- **Priority**: **Low-Medium** -- Used for advanced task launch scenarios. Could be
+  needed for MPI job management.
+
+[feedback] this function is used by worker to run non rank 0 tasks(this is for MPI jobs) and api server
+           need not handle this
+
+### 9. Shutdown (`cmd_shutdown` -> `xcecli_shutdown()`)
+
+- **What it does**: Sends shutdown command to the cluster controller.
+- **REST impact**: No endpoint. Currently cluster management operations (shutdown,
+  restart) are not available via REST.
+- **Priority**: **Medium** -- Operations teams need remote cluster control capability.
+
+[feedback] we may need to admin tool, which talk to controller directly  for this, as normal
+           user do not need this at all
+
+### 10. Resource Requirement Select (`FilterHostByResReqSelect`)
+
+- **What it does**: The hosts `POST /select` endpoint exists but returns **empty
+  results** -- it's a stub: `processHostData([]utils.OrderMap{}, "")`.
+- **REST impact**: `POST /xce/v1/hosts/select` is defined but non-functional.
+- **Priority**: **Medium** -- This is the XCE equivalent of `lsload -R` / host
+  matching by resource requirements. Important for job submission workflows to
+  find eligible hosts.
+
+[feedback] we may need this
+
+---
+
+## Enhancement Proposal (Prioritized)
+
+| Priority | Enhancement                                 | Effort     | Description |
+|----------|---------------------------------------------|------------|-------------|
+| **P0**   | Initial state sync via `xcecli_clusterconf()` | Medium  | On startup (after `RegisterToBackend`), call `xcecli_clusterconf()` to do a full state sync into stores. Prevents stale stores after API server restart. |
+| **P0**   | Implement `ControlHost()` in C library + Go wrapper | Medium | Expose `xcecli_hostctrl()` in `client.h`, then replace the Go stub in `clib.go` with a real CGO call. |
+| **P1**   | Resource value query endpoint               | Medium     | Add `GET /xce/v1/resources/values?host=X&name=Y` -> `backend.QueryResourceValues()` -> `xcecli_resval()`. This is a synchronous request-response pattern (different from push). |
+| **P1**   | Implement `FilterHostByResReqSelect`        | Medium-High| Wire `POST /xce/v1/hosts/select` to evaluate resource requirement expressions against current host state from the store. |
+| **P2**   | Cluster shutdown endpoint                   | Low        | Add `POST /xce/v1/clusters/control` -> `backend.ShutdownCluster()` -> `xcecli_shutdown()`. Should be admin-only with RBAC. |
+| **P2**   | Resource value update endpoint              | Low        | Add `POST /xce/v1/resources/values/update` -> `backend.UpdateResourceValues()` -> `xcecli_update_resvals()`. Admin/testing use. |
+| **P3**   | Task run endpoint                           | Low-Medium | Add `POST /xce/v1/tasks/run` -> `backend.RunTask()` -> `xcecli_runtask()`. For advanced MPI task management. |
+| **P3**   | Set pending reason endpoint                 | Low        | Add `POST /xce/v1/jobs/:jobid/pending-reason` -> `backend.SetPendReason()` -> `xcecli_set_pend_reason()`. Internal/scheduler-facing only. |
+
+---
+
+## Architecture Note: Synchronous vs Push-Based
+
+The biggest design decision lurking here: several of these C APIs (`xcecli_resval()`,
+`xcecli_clusterconf()`, `xcecli_jobinfo()`) are **synchronous request-response** calls
+-- the client sends a request and blocks until the controller replies. This is
+fundamentally different from the current **push-based** pattern where the controller
+asynchronously pushes updates.
+
+To implement these, we'd need to either:
+
+1. **Direct CGO call** (simpler but blocks the Go goroutine -- acceptable if calls
+   are fast and we set timeouts)
+2. **Request-response via push channel** (more complex -- send request, register a
+   wait, receive push reply)
+
+Recommendation: option 1 for initial implementation (it's what `SubmitJob` and
+`ControlJob` already do), with timeouts and goroutine isolation to prevent a slow
+controller from blocking the API server.
+
+---
+
+## Additional C API Calls Not Used Anywhere (Reference)
+
+These exist in `client.h` but are not called by `test.c` either:
+
+| C API Call                | Purpose |
+|--------------------------|---------|
+| `xcecli_hello()`         | Health check / greeting |
+| `xcecli_nop()`           | No-op heartbeat |
+| `xcecli_heartbeat()`     | Periodic heartbeat |
+| `xcecli_notify_jobstart()`| Worker notifies job started (with PID info) |
+| `xcecli_notify_jobfail()` | Worker notifies job failed |
+| `xcecli_notify_jobstop()` | Worker notifies job stopped |
+| `xcecli_notify_jobresume()`| Worker notifies job resumed |
+| `xcecli_notify_jobpostdone()`| Worker notifies job post-exec done |
+| `xcecli_update_jusage()` | Worker updates job resource usage |
+| `xcecli_notify()`        | Generic notification |
+| `xcecli_notify_taskstart()`| Worker notifies task started |
+| `xcecli_notify_taskfail()`| Worker notifies task failed |
+| `xcecli_notify_taskfinish()`| Worker notifies task finished |
+| `xcecli_clear_reason()`  | Clear pend reasons for jobs |
+| `xcecli_block_tasks()`   | Block task processing |
+| `xcecli_unblock_tasks()` | Unblock task processing |
+| `xcecli_get_my_endpoint()`| Get local endpoint info |
+| `xcecli_get_myinfo()`    | Get local peer info |
+
+[feedback] I think we just need heartbeat to tell controller that I'm alive, and other
+           functions for worker and scheduler
+
+These are primarily worker->controller notifications. They would be needed if
+the API server were to act as a proxy for worker daemons, which is not the
+current design.
--- a/src/docs/apiserver-backend-reviews.md
+++ b/src/docs/apiserver-backend-reviews.md
@ -0,0 +1,325 @@
+---
+reviewers: [adversarial, feasibility, cgo-correctness, security, architecture]
+reviewed_at: 2026-04-16T07:00:00Z
+document_reviewed: apiserver-backend-enhancement-plan.md
+prior_review_at: 2026-04-15T04:00:00Z
+---
+
+# Final Cross-Perspective Review — API Server Backend Enhancement Plan
+
+> **Prior review** (adversarial + feasibility, 2026-04-15) is summarised at the bottom.
+> This document covers only **new findings** from the final three-reviewer pass.
+> All issues flagged in the prior review are confirmed fixed in the current plan.
+
+---
+
+## Round 2 Verification (2026-04-16)
+
+### Claims Verified Against Codebase
+
+**RBAC `cluster_admin` role**: ✅ PASS. `rbac.yaml:88` assigns `action: host:control` to `cluster_admin` role with `domain: type: all`. `routers.go:112` already wires `RequirePermission("host:control")` for `POST /xce/v1/hosts/control`. No `host_admin` role exists. The plan's update to `cluster_admin` is correct.
+
+**`go_push_handler` is single-threaded**: ✅ PASS. `clib.go:32` calls `xcecli_set_handler(ctx, handler, NULL)` — this registers ONE handler function. `clib.go:150` `go_push_handler` runs on the C library's callback thread. The C library dispatches events sequentially on this thread. There is no concurrent push scenario for a single cluster.
+
+### Remaining HIGH Issues Still in the Plan
+
+| # | Issue | Status |
+|---|-------|--------|
+| CGO-1 | `unsafe.Slice(conf.hconfigs, n)` — missing `conf.hconfigs == nil` guard | **FIXED** — plan now guards `if n == 0 \|\| conf.hconfigs == nil { return nil }` |
+| CGO-2 | `unsafe.Slice(rvl.vals, n)` — missing `rvl.vals == nil` guard | **FIXED** — plan now guards `if n == 0 \|\| rvl.vals == nil { return 0 }` |
+| SEC-1 | `recover()` claim in plan said it catches C crashes | **FIXED** — plan now states `recover()` does NOT catch C SIGSEGV; C-level hardening required |
+| SEC-2 | `allFlag` package-level data race in `hosts.go:22` | **FIXED** — plan now specifies converting `allFlag` to local return value |
+| ARCH-1 | `cResourceToModel` naming collision — duplicate function name | **FIXED** — P0-1 new function renamed to `cResdefToModel(r *C.xce_resdef_t) *models.ResourceInfo` |
+| ARCH-4 | No RED test for `OnHostUpdated` behavioral change | **FIXED** — plan now requires `backend/handler_test.go` before implementation |
+| ARCH-5 | No unit tests for `rawHostConfigToModel`/`rawClusterToModel` | **FIXED** — plan now requires table-driven tests in `convert_test.go` |
+| ARCH-6 | `HostStateStr(0)` = `"OK"` for hosts with no worker data | **FIXED** — plan now specifies `HostStateStr(0)` → `"UNKNOWN"` + `LastConfigSeenAt`/`LastWorkerSeenAt` timestamps |
+
+### Fresh Findings
+
+**SEC-7 — `C.GoString(nil)` undefined behavior on NULL C strings (MEDIUM)**
+- `cClusterConfToClusterInfo` calls `C.GoString(conf.clustername)`. If `conf->clustername` is NULL, `C.GoString(nil)` is undefined behavior per CGO rules.
+- The pattern in existing `convert.go` (e.g., `cResourceToModel` line 98) uses `goString(res.name)` — a helper that should guard against NULL.
+- **Verdict**: The plan should use `C.GoString` only after a nil guard, or use the existing `goString` helper which presumably handles nil.
+- **Severity**: MEDIUM (Go's `C.GoString` typically panics on nil, but this depends on C library always providing a non-NULL string for `clustername`)
+
+**ARCH-12 — Action string-to-integer mapping not documented (LOW)**
+- The Go HTTP handler receives JSON `{"action": "open"}` but C `HOST_OPEN`/`HOST_CLOSE` are integer enum constants. The plan's P0-2 does not document where this string-to-integer mapping happens.
+- The existing `ValidateControlAction` at `controllers/hosts.go:557` validates the string. The actual mapping to C constants must happen in `ControlHost()`.
+- **Verdict**: Include a comment or table in `ControlHost()` documenting the mapping: `"open"` → `HOST_OPEN` (C constant), `"close"` → `HOST_CLOSE`.
+
+**ARCH-13 — `parse_select` errmsg ownership not specified (LOW)**
+- P1-2 does `C.free(unsafe.Pointer(errmsg))` when errmsg != nil. This is correct only if `parse_select` allocates errmsg with `malloc` (or equivalent). The plan should explicitly state the memory ownership contract for the errmsg output parameter.
+- **Verdict**: Add a one-line contract note: "parse_select allocates errmsg via malloc on error; caller owns and must free it."
+
+---
+
+## CGO / C Interop Correctness Review
+
+### CGO-1 — `unsafe.Slice(conf.hconfigs, n)` panics when pointer is nil (HIGH)
+
+**Location:** P0-1, `cClusterConfHostConfigs()` in `backend/convert.go`
+
+**Bug:** The guard is `if n == 0 { return nil }` but there is no check for `conf.hconfigs == nil`. The Go specification states that `unsafe.Slice(ptr, n)` panics if `ptr == nil` and `n > 0`. A controller or partial-initialization path that sets `nhconfigs = N` and leaves `hconfigs = NULL` will crash the apiserver. This pattern is a systemic gap: the same nil guard is missing in the existing `cClusterConfQueues` and `cClusterConfResources` functions — the new function replicates the existing defect.
+
+**Fix:**
+```go
+if n == 0 || conf.hconfigs == nil {
+    return nil
+}
+```
+
+### CGO-2 — `unsafe.Slice(rvl.vals, n)` panics when pointer is nil (HIGH)
+
+**Location:** P1-1, `handleNotifyData()` in `backend/clib.go`
+
+**Bug:** The guard `if rvl == nil { return 0 }` does not check `rvl.vals`. `xce_resval_list_t` in `typemap.h` has `int count` and `xce_resval_t** vals` — a null `vals` with non-zero `count` is a valid C state on error paths or partial serialization. `unsafe.Slice(rvl.vals, n)` with `n > 0` and `rvl.vals == nil` panics.
+
+**Fix:**
+```go
+if rvl == nil || rvl.vals == nil || n == 0 {
+    return 0
+}
+```
+
+### CGO-3 — `StartHeartbeat` goroutine blocked in CGO during shutdown (MEDIUM)
+
+**Location:** P1-3, `StartHeartbeat()` in `backend/clib.go`
+
+**Issue:** `C.xcecli_heartbeat()` is a blocking CGO call. When `ctx.Done()` fires, the goroutine can only exit after the current C call returns. If the controller is unreachable and the C call has a long (or no) timeout, the process shutdown is delayed by up to one heartbeat cycle. This is not a goroutine leak but will delay `defer cancel()` in `main.go`. Add a comment documenting this known limitation. No API change needed for initial implementation.
+
+### CGO-4 — Items confirmed safe (informational)
+
+- `req.body_which` field access: `xce_request_t.body_which` is a plain `int` at struct level, not inside the union. CGO-accessible. ✓
+- `nd.event` field: `xce_notify_data_t.event` is a plain top-level `int`. CGO-accessible. ✓
+- `(*[0]byte)(C.xce_select_get_attr)`: correct standard CGO idiom for `//export` function pointer. ✓
+- `C.uintptr_t(h)` where `h` is `cgo.Handle`: integer-to-integer cast, not a pointer conversion. Avoids `go vet` warning. ✓
+
+---
+
+## Security Review
+
+### SEC-1 — `recover()` does NOT catch C SIGSEGV — plan's stated protection is false (HIGH)
+
+**Location:** P1-2, "Input safety" section — "Wrap the EvalSelectForHost call in a `recover()` block"
+
+**Bug:** Go's `recover()` catches Go panics only. A null pointer dereference or out-of-bounds access inside `eval_select` or `parse_select` (C code) raises SIGSEGV via the OS, which terminates the process immediately — Go defers and recover() are bypassed. The plan's claim that `recover()` protects the server from C-side crashes is incorrect and must not be used as justification for skipping C-level validation.
+
+**Fix:** Remove or correct the recover() claim. The actual protection must come from:
+1. Hardening `parse_select`/`eval_select` with internal bounds checking and null guards in C.
+2. Fuzz testing the PEG parser with a corpus before shipping.
+3. Optionally running the parse/eval in a subprocess with a timeout (more complex, not required for initial implementation, but the only true isolation).
+
+Add a comment in the Go handler: `// Note: recover() does not catch C SIGSEGV. C-level null dereference in parse_select/eval_select will crash the process. Rely on C-level validation, not Go recovery.`
+
+### SEC-2 — `allFlag` package-level global is a data race on concurrent requests (HIGH)
+
+**Location:** `controllers/hosts.go:22`, `controllers/queues.go` (pre-existing, exposed by this plan)
+
+**Bug:** `var allFlag bool` is a package-level shared variable. `preCheckHostControlRequest` sets it, `ControlHost` reads it — with no synchronization between goroutines. Two concurrent `POST /hosts/control` requests can interleave: Request A sets `allFlag = false` (targeted), Request B sets `allFlag = true` (all hosts), then Request A reads `true` and silently operates on the entire cluster instead of a single host. This is a logic race that can trigger unintended cluster-wide operations.
+
+The plan's P0-2 test coverage (Verification step 2) will make this race more observable. The architecture reviewer confirmed it is present in the existing code; it must be fixed before P0-2 lands since the plan adds the real implementation path.
+
+**Fix:** Eliminate the package-level variable. Return the `allFlag` value from `preCheckHostControlRequest` as a local `bool` and pass it to `ControlHost`. The same fix applies to `queues.go`.
+
+### SEC-3 — In-process push data has no Go-side range check on `nrows` (MEDIUM)
+
+**Location:** `backend/clib.go`, `handleDataset()` — pre-existing, not introduced by this plan
+
+**Issue:** `unsafe.Slice(C.dataset_jobs(d), n)` constructs a Go slice over C memory where `n = int(d.nrows)`. If the C library delivers a dataset with a crafted or corrupted `nrows` exceeding the actual allocation, the slice construction succeeds but iteration reads out-of-bounds C memory. This can corrupt the in-memory store. The plan does not address this because it is pre-existing, but the new P1-1 resource push handler follows the same pattern and has the same risk.
+
+**Fix for P1-1:** Add a sanity bound on `rvl.count` before the `unsafe.Slice` call:
+```go
+const maxResVals = 65536 // sanity cap
+if n > maxResVals {
+    logger.Warn("resval count exceeds sanity limit, truncating", zap.Int("count", n))
+    n = maxResVals
+}
+```
+
+### SEC-4 — PEG parser DoS: 1024-byte limit alone is insufficient (MEDIUM)
+
+**Location:** P1-2, "Input safety" — expression length limit
+
+**Issue:** PEG parsers without memoization (packrat parsing) have O(2^n) backtracking on adversarial inputs like `((((((((a`. A 1024-byte limit does not prevent CPU exhaustion — it sets the budget. 1024 bytes of adversarial expression can still pin a CPU core for seconds in a naive PEG implementation.
+
+**Mitigation required:** Verify that `xce_select.peg` generates a **packrat** (memoized) parser — packcc supports this with the `--packrat` flag or equivalent option. Document this requirement explicitly in `lib/meson.build`'s `custom_target` invocation. Without packrat, the DoS risk is real. `/hosts/select` rate limiting (already inherited from RateLimit middleware) reduces but does not eliminate the risk.
+
+### SEC-5 — Error message leaks internal C error codes (LOW)
+
+**Location:** P0-2, `ControlHost()` — `fmt.Errorf("xcecli_hostctrl failed: %d", int(rc))`
+
+**Issue:** Negative C return codes propagate to the HTTP response body. In HPC contexts these are typically library-specific codes (not OS errno values) and are not meaningfully sensitive. However, the current stub error at `clib.go:311` already leaks the hostname and action integer in the error string — this must be cleaned up when the stub is replaced.
+
+**Fix:** Map C error codes to generic messages at the API boundary: `"host control failed"` (with the code logged server-side at WARN level, not returned to the client).
+
+### SEC-6 — Confirmed safe (informational)
+
+- `host:view` permission for all authenticated users: intentional HPC design. Users need host capacity info to craft job resource requests. ✓
+- Hostname as `C.CString()`: the store lookup in `validateHost` acts as an allowlist — only controller-pushed hostnames reach C. ✓
+
+---
+
+## Architecture & Design Consistency Review
+
+### ARCH-1 — `cResourceToModel` naming collision: plan adds a duplicate with a different signature (HIGH)
+
+**Location:** P0-1, "convert.go" section; P1-1, "cResourceToModel note"
+
+**Bug:** `convert.go` already contains `cResourceToModel(res *C.xce_resource_t) *models.ResourceDefinitionInfo` (line 95-104). The plan's P0-1 describes adding `cResourceToModel(r *C.xce_resdef_t) *models.ResourceInfo` — a different input type (`xce_resdef_t` vs `xce_resource_t`) and a different output type (`ResourceInfo` vs `ResourceDefinitionInfo`). Two functions with the same name in the same package is a compile error.
+
+The plan's P1-1 note "cResourceToModel already exists — do NOT add a duplicate" is internally contradictory: it refers to the **existing** function (xce_resource_t → ResourceDefinitionInfo), not the new P0-1 function described in the same document.
+
+**Fix:** Rename the new function in P0-1 to `cResdefToModel(r *C.xce_resdef_t) *models.ResourceInfo`. Update all call sites. P1-1 can continue to use the existing `cResourceToModel` (the old one) without conflict.
+
+### ARCH-2 — `host_admin` RBAC role referenced in plan does not exist (HIGH)
+
+**Location:** P0-2, "Go Side / router/routers.go"
+
+**Bug:** The plan states "Permitted roles: admin users with the `host_admin` role." The actual `rbac.yaml` has three roles: `basic_user`, `cluster_admin`, `queue_admin`. There is no `host_admin` role. The `host:control` permission is already assigned to `cluster_admin`. Both routes at `routers.go:111-112` already exist with `RequirePermission("host:control")` and `RequirePermission("host:view")`.
+
+**Fix:** Remove all references to `host_admin` from the plan. Clarify: `host:control` → `cluster_admin` (already configured). Verification step 7 should read "returns 403 for `basic_user` and `queue_admin`, succeeds for `cluster_admin`."
+
+### ARCH-3 — P0-2 stubs reference P1-3 symbols: CGO build fails between phases (HIGH)
+
+**Location:** P0-2, "backend/stub.go"
+
+**Issue:** The plan adds `StartHeartbeat(ctx context.Context) {}` and `SetHeartbeatInterval(secs int) {}` stubs to `stub.go` in P0-2, but the corresponding CGO implementations in `clib.go` are not added until P1-3. The `!cgo` build will compile correctly with the stubs. The CGO build (used for integration testing) will fail with "undefined: backend.StartHeartbeat" if P0-2 is tested before P1-3.
+
+**Fix:** Add a note: "The CGO build will fail to compile after P0-2 until P1-3 adds `StartHeartbeat` and `SetHeartbeatInterval` to `clib.go`. Implement P0-2 stub additions and P1-3 `clib.go` additions in the same commit, or accept a temporary CGO build break between phases."
+
+### ARCH-4 — `OnHostUpdated` behavioral change has no RED test written first (HIGH)
+
+**Location:** P0-1, "OnHostUpdated fix"
+
+**Issue:** The plan changes `handler.go:27` from full-overwrite to partial merge — a behavioral change to existing code. There are zero tests for `DefaultHandler` in the entire test suite. CLAUDE.md mandates TDD: "Write a failing test first." A wrong field classification in the partial merge (e.g., accidentally preserving a worker field that should be updated) will silently corrupt host state with no test catching it.
+
+**Fix:** Before implementing the `OnHostUpdated` change, add `backend/handler_test.go` with:
+- A test that proves the current full-overwrite behavior (RED for the new code)
+- Tests for `OnHostConfigUpdated` partial merge: config fields update, worker fields preserved
+- Tests for `OnHostUpdated` partial merge: worker fields update, config fields preserved
+- Tests for `OnClusterUpdated` partial merge
+
+Use `store.InitStores()` for store reset between test cases.
+
+### ARCH-5 — Pure conversion functions have no unit tests specified (HIGH)
+
+**Location:** P0-1, "backend/convert_pure.go"
+
+**Issue:** `rawHostConfigToModel` and `rawClusterToModel` are placed in `convert_pure.go` (no CGO, fully testable) but the plan's Verification section specifies only post-hoc integration checks (start server, hit endpoints). The `convert_test.go` file already tests `rawJobToModel` and `rawWorkerToModel` with table-driven tests — the same pattern must be applied here per TDD rules.
+
+**Fix:** Add to the P0-1 implementation instructions: "Before implementing `rawHostConfigToModel` and `rawClusterToModel`, add table-driven tests in `backend/convert_test.go` covering: nil/empty string fields, zero integer fields (MaxSlot=0, HeartbeatInterval=0), normal values."
+
+### ARCH-6 — First-startup host state appears "OK" with zero worker fields (HIGH)
+
+**Location:** P0-1, "Partial merge semantics"
+
+**Issue:** After P0-1, the config push fires and creates host entries with `HostModel`, `HostType`, `MaxSlot` set but all worker-sourced fields (`HostStatus`, `MaxMem`, `NumRun`, etc.) at zero. `HostStateStr` in `models/hosts.go:20` maps `state=0` to `"OK"`. A `GET /xce/v1/hosts` call in the window before any worker has checked in will return hosts showing status `"OK"` with 0 CPUs and 0 memory. Operators and monitoring dashboards will see misleading data.
+
+**Fix (minimal, non-breaking):** Change `HostStateStr(0)` from `"OK"` to `"UNKNOWN"` or `"PENDING"`. This is a one-line change in `models/hosts.go` that accurately reflects the not-yet-reported state. Alternatively, add a `WorkerDataReceived bool` field to `HostInfo` set by `OnHostUpdated`.
+
+### ARCH-7 — `meson.build` custom_target block is not specified (MEDIUM)
+
+**Location:** P1-2, "Build Integration"
+
+**Issue:** The plan says "add `xce_select.peg` to that explicit list." The actual `meson.build` pattern requires a full `custom_target` block mirroring the existing `xce_resreq_parser` block — the packcc tool, input/output filenames, command invocation — followed by adding `xce_select_parser[0]` to `libxce_src`. Simply adding the filename to `libxce_src` will fail because meson does not auto-run packcc on `.peg` files.
+
+**Fix:** The plan must include the complete `custom_target` block for `xce_select.peg`.
+
+### ARCH-8 — Heartbeat test uses `time.Sleep` despite plan prohibiting it (MEDIUM)
+
+**Location:** Verification section, item 5
+
+**Issue:** The plan states "The test must not depend on real time" then immediately provides a test using `time.Sleep(3500ms)`. This is a 3.5-second wall-clock delay per test run, and the pass/fail boundary (`n < 2 || n > 5`) will be flaky on loaded CI systems.
+
+**Fix:** Expose the ticker as injectable: `StartHeartbeat(ctx, tickCh <-chan time.Time)` where production code passes `time.NewTicker(...).C` and tests pass a manually-driven `chan time.Time`. The test sends exactly N ticks and asserts the counter equals N — zero wall-clock time required.
+
+### ARCH-9 — No code-level TODO for O(H) parse-per-call bottleneck (MEDIUM)
+
+**Location:** P1-2, "Performance" note
+
+**Issue:** The plan acknowledges that `EvalSelectForHost` calls `parse_select` once per host (O(H) parse calls per request) and says "do not over-engineer." The optimization path is described only in the design document — future developers reading the code will not see it.
+
+**Fix:** Add this comment in `EvalSelectForHost` in `clib.go`:
+```go
+// TODO(perf): parse_select is called once per host. For large clusters, pre-parse
+// the expression once and pass the *C.xce_select_t to a batch evaluator.
+// See EvalSelectForHostParsed design note in apiserver-backend-enhancement-plan.md.
+```
+
+### ARCH-10 — Stale decommissioned hosts have no API signal (MEDIUM)
+
+**Location:** P0-1, "Partial merge semantics" — host deletion paragraph
+
+**Issue:** Hosts removed from the cluster configuration remain in `store.Hosts` indefinitely. There is no `LastSeenAt` field, no stale flag, no way for operators to distinguish a decommissioned host from a healthy-but-unreachable one. The plan defers TTL, but adds no groundwork for it.
+
+**Fix:** Add `LastConfigSeenAt time.Time` and `LastWorkerSeenAt time.Time` fields to `HostInfo`. Set them in `OnHostConfigUpdated` and `OnHostUpdated` respectively. These fields cost nothing in correctness and provide the data source for a future TTL implementation without requiring a model change later.
+
+---
+
+## Consensus Summary
+
+### Issues Requiring Plan Correction Before Implementation
+
+| # | Finding | Severity | Phase |
+|---|---------|----------|-------|
+| CGO-1 | `unsafe.Slice` nil pointer — `cClusterConfHostConfigs` | HIGH | P0-1 |
+| CGO-2 | `unsafe.Slice` nil pointer — `handleNotifyData` | HIGH | P1-1 |
+| SEC-1 | `recover()` does not catch C SIGSEGV — stated protection is false | HIGH | P1-2 |
+| SEC-2 | `allFlag` package-level data race — concurrent requests corrupt state | HIGH | P0-2 |
+| ARCH-1 | `cResourceToModel` naming collision — compile error | HIGH | P0-1 |
+| ARCH-2 | `host_admin` role does not exist in `rbac.yaml` | HIGH | P0-2 |
+| ARCH-3 | P0-2 stubs cause CGO link failure before P1-3 | HIGH | P0-2 |
+| ARCH-4 | `OnHostUpdated` change has no RED test (TDD violation) | HIGH | P0-1 |
+| ARCH-5 | `rawHostConfigToModel`/`rawClusterToModel` have no unit tests specified | HIGH | P0-1 |
+| ARCH-6 | Zero worker fields at first startup show as `"OK"` status | HIGH | P0-1 |
+
+### Issues Requiring Plan Clarification
+
+| # | Finding | Severity | Phase |
+|---|---------|----------|-------|
+| SEC-3 | No Go-side range check on `nrows` before `unsafe.Slice` in push handler | MEDIUM | P1-1 |
+| SEC-4 | PEG parser packrat requirement not specified (DoS risk) | MEDIUM | P1-2 |
+| ARCH-7 | `meson.build` custom_target block missing from plan | MEDIUM | P1-2 |
+| ARCH-8 | Heartbeat test uses `time.Sleep` despite plan prohibiting it | MEDIUM | P1-3 |
+| ARCH-9 | No code-level TODO for O(H) parse-per-call bottleneck | MEDIUM | P1-2 |
+| ARCH-10 | Stale decommissioned hosts have no API signal | MEDIUM | P0-1 |
+| CGO-3 | `StartHeartbeat` goroutine cannot be interrupted during CGO call | MEDIUM | P1-3 |
+| SEC-5 | C error codes leak to HTTP response body | LOW | P0-2 |
+
+### Confirmed Strengths (all reviewers agreed)
+
+- Partial merge semantics are correctly designed: field ownership lists are explicit and the read-then-write pattern is sound for the single-threaded push handler.
+- `OPCode_notify` placed at the top-level switch (not inside `handlePush`) is architecturally correct.
+- `cgo.Handle` usage for the select callback context is the right CGO approach.
+- `RegisterToBackend` being synchronous ensures heartbeat starts with the correct interval.
+- PEG grammar placed in `libxce` (not Go layer) is correct for scheduler reuse.
+- The two-step extraction (`xce_request_body` then `xce_notify_data_ptr`) is now correct in the plan.
+
+### Items Confirmed Not Issues
+
+- `req.body_which` and `nd.event` are plain struct fields, CGO-accessible. ✓
+- `(*[0]byte)(C.xce_select_get_attr)` is the correct CGO function pointer idiom. ✓
+- `C.uintptr_t(h)` for cgo.Handle is an integer cast (not pointer), go-vet clean. ✓
+- `host:view` for all authenticated users is intentional HPC design. ✓
+- `xcecli_hostctrl` blocking CGO: consistent with existing queuectrl pattern, local socket. ✓
+
+---
+
+## Prior Review Summary (2026-04-15)
+
+The prior adversarial + feasibility review identified the following issues, all of which are **confirmed fixed** in the current plan:
+
+| Issue | Fix Confirmed |
+|-------|---------------|
+| `handleNotifyData()` used wrong function to extract body | Fixed: plan now uses `xce_request_body(req)` first |
+| `xce_val_t.which` referenced as `.type` | Fixed: plan uses `val.which` throughout |
+| `handleUpdate()` union field accessed directly | Fixed: plan explicitly states OPCode_update is NOT used by apiserver |
+| P1-2 CGO map-as-context was undefined behavior | Fixed: plan uses `cgo.Handle` |
+| `stub.go` stubs omitted for `!cgo` builds | Fixed: comprehensive stub section added |
+| `helper.c` missing from file list | Fixed: included with `Request_body_hostctrlmsg` case |
+| `free_select()` / `kill_resreq()` linkage — memory leak | Fixed: plan shows both `free_select` + `FREEUP(resreq->select)` |
+| Heartbeat interval not reconfigurable after start | Fixed: `SetHeartbeatInterval` + `ticker.Reset` per tick |
+
+---
+
+*To incorporate this feedback: update `apiserver-backend-enhancement-plan.md` addressing all HIGH-severity items before implementation begins. MEDIUM items should be resolved before the first PR is opened.*
--- a/src/frontend/apiserver/models/clusters.go
+++ b/src/frontend/apiserver/models/clusters.go
@ -23,6 +23,9 @@ type ClusterInfo struct {
 	HostTypes              string `json:"host_types"`
 	HostModels             string `json:"host_models"`
 	ClusterAdmins          string `json:"cluster_admins"`
+	HeartbeatInterval      int    `json:"heartbeat_interval"`
+	Vcpus                  int    `json:"vcpus"`
+	InactiveHeartbeatCount int    `json:"inactive_heartbeat_count"`
 }

 var CLUSTER_SUPPORT_FILTERS = []string{
--- a/src/frontend/apiserver/models/clusters_test.go
+++ b/src/frontend/apiserver/models/clusters_test.go
@ -0,0 +1,125 @@
+package models
+
+import (
+	"encoding/json"
+	"testing"
+)
+
+func TestClusterInfo_NewFields(t *testing.T) {
+	tests := []struct {
+		name   string
+		cluster ClusterInfo
+		field  string
+		want    interface{}
+	}{
+		{
+			name:   "HeartbeatInterval field exists and serializes",
+			cluster: ClusterInfo{ClusterName: "test", HeartbeatInterval: 30},
+			field:  "heartbeat_interval",
+			want:   float64(30),
+		},
+		{
+			name:   "Vcpus field exists and serializes",
+			cluster: ClusterInfo{ClusterName: "test", Vcpus: 16},
+			field:  "vcpus",
+			want:   float64(16),
+		},
+		{
+			name:   "InactiveHeartbeatCount field exists and serializes",
+			cluster: ClusterInfo{ClusterName: "test", InactiveHeartbeatCount: 3},
+			field:  "inactive_heartbeat_count",
+			want:   float64(3),
+		},
+		{
+			name:   "HeartbeatInterval zero value serializes as 0 not omitted",
+			cluster: ClusterInfo{ClusterName: "test", HeartbeatInterval: 0, Vcpus: 0, InactiveHeartbeatCount: 0},
+			field:  "heartbeat_interval",
+			want:   float64(0),
+		},
+		{
+			name:   "Vcpus zero value serializes as 0 not omitted",
+			cluster: ClusterInfo{ClusterName: "test", HeartbeatInterval: 0, Vcpus: 0, InactiveHeartbeatCount: 0},
+			field:  "vcpus",
+			want:   float64(0),
+		},
+		{
+			name:   "InactiveHeartbeatCount zero value serializes as 0 not omitted",
+			cluster: ClusterInfo{ClusterName: "test", HeartbeatInterval: 0, Vcpus: 0, InactiveHeartbeatCount: 0},
+			field:  "inactive_heartbeat_count",
+			want:   float64(0),
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			data, err := json.Marshal(tt.cluster)
+			if err != nil {
+				t.Fatalf("json.Marshal failed: %v", err)
+			}
+			var m map[string]interface{}
+			if err := json.Unmarshal(data, &m); err != nil {
+				t.Fatalf("json.Unmarshal failed: %v", err)
+			}
+			got, ok := m[tt.field]
+			if !ok {
+				t.Errorf("field %q not found in JSON output; available keys: %v", tt.field, mapKeys(m))
+			} else if got != tt.want {
+				t.Errorf("field %q = %v, want %v", tt.field, got, tt.want)
+			}
+		})
+	}
+}
+
+func TestClusterInfo_ExistingFieldsUnchanged(t *testing.T) {
+	c := ClusterInfo{
+		ClusterName:    "mycluster",
+		Version:        "1.0",
+		Status:         1,
+		ControllerName: "ctrl1",
+		NumServers:     5,
+		Resources:      `{"res1":"val1"}`,
+		HostTypes:      "LINUX64",
+		HostModels:     "modelA",
+		ClusterAdmins:  "admin1,admin2",
+	}
+
+	data, err := json.Marshal(c)
+	if err != nil {
+		t.Fatalf("json.Marshal failed: %v", err)
+	}
+	var m map[string]interface{}
+	if err := json.Unmarshal(data, &m); err != nil {
+		t.Fatalf("json.Unmarshal failed: %v", err)
+	}
+
+	// Verify all existing fields still serialize correctly
+	existing := map[string]string{
+		"cluster_name":    c.ClusterName,
+		"version":         c.Version,
+		"controller_name": c.ControllerName,
+		"resources":       c.Resources,
+		"host_types":      c.HostTypes,
+		"host_models":     c.HostModels,
+		"cluster_admins":  c.ClusterAdmins,
+	}
+	for jsonKey, wantVal := range existing {
+		got, _ := m[jsonKey].(string)
+		if got != wantVal {
+			t.Errorf("existing field %q = %q, want %q", jsonKey, got, wantVal)
+		}
+	}
+	if got, _ := m["status"].(float64); int(got) != c.Status {
+		t.Errorf("existing field status = %v, want %d", got, c.Status)
+	}
+	if got, _ := m["num_servers"].(float64); int(got) != c.NumServers {
+		t.Errorf("existing field num_servers = %v, want %d", got, c.NumServers)
+	}
+}
+
+func mapKeys(m map[string]interface{}) []string {
+	keys := make([]string, 0, len(m))
+	for k := range m {
+		keys = append(keys, k)
+	}
+	return keys
+}
--- a/src/frontend/apiserver/models/hosts.go
+++ b/src/frontend/apiserver/models/hosts.go
@ -18,6 +18,9 @@ var HOSTSTAT = map[string]int{
 }

 func HostStateStr(state int) string {
+	if state == 0 {
+		return "UNKNOWN"
+	}
 	if (state & HOSTSTAT["UNAVAIL"]) != 0 {
 		return "UNAVAIL"
 	} else if (state & HOSTSTAT["CLOSED"]) != 0 {
--- a/src/frontend/apiserver/models/hosts_test.go
+++ b/src/frontend/apiserver/models/hosts_test.go
@ -0,0 +1,61 @@
+package models
+
+import "testing"
+
+func TestHostStateStr(t *testing.T) {
+	tests := []struct {
+		name  string
+		state int
+		want  string
+	}{
+		{
+			name:  "zero value returns UNKNOWN when no worker has reported",
+			state: 0,
+			want:  "UNKNOWN",
+		},
+		{
+			name:  "UNAVAIL flag",
+			state: HOSTSTAT["UNAVAIL"],
+			want:  "UNAVAIL",
+		},
+		{
+			name:  "CLOSED flag",
+			state: HOSTSTAT["CLOSED"],
+			want:  "CLOSED",
+		},
+		{
+			name:  "BUSY flag",
+			state: HOSTSTAT["BUSY"],
+			want:  "BUSY",
+		},
+		{
+			name:  "LOCKEDU flag",
+			state: HOSTSTAT["LOCKEDU"],
+			want:  "LOCKU",
+		},
+		{
+			name:  "LOCKEDW flag",
+			state: HOSTSTAT["LOCKEDW"],
+			want:  "LOCKW",
+		},
+		{
+			name:  "no flags set returns OK for real host status",
+			state: 0x00000001, // some non-zero, non-flag value
+			want:  "OK",
+		},
+		{
+			name:  "combined UNAVAIL+CLOSED flags",
+			state: HOSTSTAT["UNAVAIL"] | HOSTSTAT["CLOSED"],
+			want:  "UNAVAIL", // UNAVAIL takes priority
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := HostStateStr(tt.state)
+			if got != tt.want {
+				t.Errorf("HostStateStr(%d) = %q, want %q", tt.state, got, tt.want)
+			}
+		})
+	}
+}