myxce/src/docs/apiserver-backend-reviews.md

reviewers	reviewed_at	document_reviewed	prior_review_at
adversarial feasibility cgo-correctness security architecture	2026-04-16T07:00:00Z	apiserver-backend-enhancement-plan.md	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:

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:

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:

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:

// 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.