lei.chen/myxce
1
0
Fork 1
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Merge branch 'working' into job_peek_lchen

Browse source 
job_peek_lchen: Add fetchfile feature (job output/error peek proxy)
working: HA/Raft infrastructure (consensus, replication, config mgmt)

Conflict resolution summary:
- controller.capnp: fetchfile @41, fetchfilereply @18, fetchfilemsg @30
- cluster.c: bifurcated OPCode_reply for fetchfile + config-push ACK
- job.c: kept full fetchfile pending-request subsystem (~183 lines)
- request.c: kept both xce_fetchfile_req() and XCE_RETRY_DEFAULTS
- helper.c: added missing preVoteRequest/preVoteReply mappings
- All 15 new test registrations (test_cbuf.c + 14 HA tests)
This commit is contained in:
Lei Chen 2026-07-22 13:57:25 +08:00
commit 3a2094a461
529 changed files with 345174 additions and 5048 deletions
Download patch file Download diff file Expand all files Collapse all files

7
src/docs/FUTURE_WORK.md Normal file
View file

@ -0,0 +1,7 @@
# XCE Controller: Future Work Items
> **Moved (2026-06-10)**: All items merged into the single consolidated future-work document at
> [`.kiro/steering/roadmap.md`](../../.kiro/steering/roadmap.md).
>
> Section mapping: §1+§8 → admin-config-tool (backlog); §2 → superseded by ha-consistency-invariants Req 5;
> §3, §4, §5, §6, §7 → Hardening / Direct-Implementation Items.

1013
src/docs/apiserver-backend-enhancement-plan.md
View file

File diff suppressed because it is too large Load diff

225
src/docs/apiserver-backend-gaps.md
View file

@ -1,225 +0,0 @@
# 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.

459
src/docs/apiserver-backend-integration-test-plan.md
View file

@ -1,459 +0,0 @@
# Integration Test Plan — P0 + P1 Features
## Purpose
Verify end-to-end correctness of every feature delivered by tasks P0-1 through P1-3,
exercising the full stack: handler → store → handler merge → store readback.
These tests **do not** require a live C controller or CGO — they use the pure-Go stores
and `DefaultHandler` directly, which is what unit tests already do. The integration layer
adds: **multi-step sequences**, **cross-feature interactions**, and **edge-case coverage**
that unit tests leave out.
---
## Test Environment
- All tests run with `CGO_ENABLED=0` (pure Go)
- Each test calls `store.InitStores()` to reset all global state
- `utils.Log = zap.NewNop()` to suppress log output
- **§1–§5, §7, §8**: call handler/store methods directly — no HTTP server required
- **§6 (RBAC)**: requires a test HTTP server via `httptest.NewRecorder` with a full Gin router; JWT tokens must be real HS256-signed tokens using the configured signing secret (typically `"xce-api-key"` in test config); `ctx.Set("user", "admin")` does **not** satisfy `GetUsernameFromToken()` which reads and parses the `Authorization` header
- **JWT IP binding:** `isValidToken` checks `claims.IP == ctx.ClientIP()`; in `httptest`, `ClientIP()` reads from `req.RemoteAddr` — set `req.RemoteAddr = "127.0.0.1:0"` on every test request and embed `"ip": "127.0.0.1"` in the JWT claims; a mismatch returns 401 for the wrong reason, silently invalidating the test
- **Auth config:** `isValidToken` dereferences `utils.AuthenticationYamlConfig`, a package-level pointer that is `nil` until `LoadConfig()` is called — before any JWT test set `utils.AuthenticationYamlConfig = &utils.Authentication{Token: utils.TokenConfig{SecretKey: "xce-api-key"}}` directly; do not call `LoadConfig()` (reads a file that won't exist in CI)
---
## 1. P0-1: Cluster Config Push
### 1.1 Config push populates all stores in a single call
**Steps:**
1. Call `dispatchConfigPush(handler, queues, resources, hostConfigs, cluster)` with non-nil data for all four.
2. Verify `store.Queues`, `store.Resources`, `store.Hosts`, `store.Clusters` all contain the pushed data.
**Asserts:** A single config push updates all four store types in sequence (queues → resources → hosts → cluster); the four updates are NOT atomic — a concurrent reader may see a partial state.
### 1.2 Second config push merges (does not replace)
**Steps:**
1. Seed `store.Hosts` with worker-sourced data (HostStatus, CPUFactor, NumCPUs, etc.)
2. Call `h.OnHostConfigUpdated()` with config-sourced fields (HostModel, HostType, MaxSlot)
3. Call `h.OnHostConfigUpdated()` a second time with `HostModel: "modelB"` (distinct from step 2's "modelA")
4. Verify worker-sourced fields from step 1 are still preserved
5. Verify `HostModel == "modelB"` (config-sourced field reflects the second push, not the first)
**Asserts:** Partial merge is idempotent and correctly preserves the other source's fields across multiple pushes.
### 1.3 Cluster update with zero HeartbeatInterval preserves existing value
**Steps:**
1. `store.Clusters.Put("c1", ClusterInfo{HeartbeatInterval: 45})`
2. Call `h.OnClusterUpdated(&ClusterInfo{ClusterName: "c1", HeartbeatInterval: 0, Vcpus: 8})`
3. Verify `HeartbeatInterval` is still 45 (zero is ignored per `if incoming.HeartbeatInterval > 0` guard)
**Asserts:** Cluster push with `HeartbeatInterval=0` does not zero out the stored interval.
### 1.4 Config push with nil cluster argument is a no-op
**Steps:**
1. Call `dispatchConfigPush(handler, nil, nil, nil, nil)`
2. Verify `store.Clusters.Count() == 0`
**Covers:** `dispatchConfigPush` skips the cluster update when the cluster argument is nil. (The empty-name guard lives in the CGO layer `cClusterConfToClusterInfo` and is not reachable in `CGO_ENABLED=0` tests; a `ClusterInfo{ClusterName:""}` passed directly would be stored under an empty key.)
### 1.5 Config push with nil queues/resources/hosts is safe
**Steps:**
1. Call `dispatchConfigPush(handler, nil, nil, nil, nil)`
2. Verify no panics, no store entries.
**Covers:** Nil-safety of the dispatch loop.
---
## 2. P0-2: Host Control via Backend
### 2.1 Valid control action (open/close) routes through to backend
**Steps:**
1. `store.Hosts.Put("node01", HostInfo{HostName: "node01"})`
2. Call `preCheckHostControlRequest(&HostControlRequest{ControlAction: "open", Hostname: []string{"node01"}})`
3. Assert `allFlag == false`, `err == nil`
**Asserts:** Valid actions pass pre-check.
### 2.2 Invalid action is rejected
**Steps:**
1. Call `preCheckHostControlRequest(&HostControlRequest{ControlAction: "reboot", Hostname: []string{"node01"}})`
2. Assert error is returned.
**Asserts:** Only "open" and "close" are accepted.
### 2.3 Comment length limit (512 chars)
**Steps:**
1. Call `preCheckHostControlRequest` with `Comment` of 513 chars
2. Assert error
3. Call with 512 chars — assert no error
**Already covered in unit tests.** Integration adds: verify the error message is human-readable.
### 2.4 Hostname validation checks store existence
**Steps:**
1. `store.Hosts.Put("node01", HostInfo{HostName: "node01"})`
2. Call `validateHost(ctx, action, []string{"node01", "nonexistent"})`
3. Assert `node01` in success list, `nonexistent` in failed map
**Note:** `validateHost` calls `GetUsernameFromToken()` internally, requiring the same JWT setup as §6 — set `utils.AuthenticationYamlConfig`, set `req.RemoteAddr = "127.0.0.1:0"`, embed `"ip": "127.0.0.1"` in the JWT claims, and pass the token via `Authorization` header. `ctx.Set("user", "admin")` is **not** sufficient. See §6 Test Environment notes for the full setup checklist. Unlike §2.1–2.3, this test is grouped with the §6 JWT-required tests in `controllers/integration_test.go`.
### 2.5 RBAC: host:control requires cluster_admin role
**Steps:**
1. Load RBAC config with `cluster_admin` having `host:control`
2. Use `httptest.NewRecorder` + Gin router with a spy handler that sets a response body
3. Call with a user that is NOT cluster_admin — assert 403 response
4. Call with cluster_admin user — assert 2xx and spy handler body present
**Note:** `RequirePermission` short-circuits for `cluster_admin` via early `return` (without calling `ctx.Next()`). Asserting `ctx.IsAborted() == false` gives a **false pass** — the middleware doesn't abort, but the handler is also never called. Use an HTTP recorder to verify the handler response actually appeared.
**Covers:** Route middleware integration.
### 2.6 RBAC: host:view accessible to basic_user
**Steps:**
1. Load RBAC config with `basic_user` having `host:view`
2. Call `RequirePermission("host:view")` with basic_user
3. Assert request proceeds
### 2.7 Concurrent control requests do not race
**Already covered:** `TestPreCheckHostControlRequest_NoRace` (50 goroutines).
Integration adds: test that concurrent `ControlHost` calls to the same host don't corrupt store state.
### 2.8 Batch response construction
**Steps:**
1. Build success and failed slices directly (do NOT call `ControlHost` — it calls the CGO stub which returns `errNoCGO` under `CGO_ENABLED=0`):
- `succeededItems := []map[string]interface{}{{"node": "node01"}, {"node": "node02"}}`
- `failedItems := []map[string]interface{}{{"node": "bad", "reason": "not found"}}`
2. Call `BuildBatchOperationResponse("close", "host", succeededItems, failedItems)`
3. Verify returned status is `"partially_accepted"`, success list has 2 entries, failed map has 1
**Covers:** Batch response construction logic; the CGO backend call is out of scope for pure-Go integration tests.
---
## 3. P1-1: Resource Value Push
### 3.1 All 7 value types convert correctly (pure-Go path)
**Steps:**
1. For each value type (i32, i64, u32, u64, fval, sval, bval):
- Create `rawResvalData` with that type
- Call `rawResvalToDistribution(r)`
- Assert `ValueType`, `NumericValue`/`StringValue`/`BooleanValue` match expected
**Already covered in unit tests.** Integration adds: round-trip through `OnResourceLocationUpdated`:
2. Call `h.OnResourceLocationUpdated(rd)` for each type
3. Read back from `store.ResourceLocations` using composite key `name:host`
4. Assert stored values match
### 3.2 Nil val produces nil return (no store update)
**Steps:**
1. Create `rawResvalData{valNil: true, ...}`
2. Assert `rawResvalToDistribution` returns nil
3. Assert `store.ResourceLocations.Count() == 0`
### 3.3 Unknown discriminant produces nil return with warn log
**Steps:**
1. Create `rawResvalData{valWhich: 99, ...}`
2. Assert `rawResvalToDistribution` returns nil
3. Assert `store.ResourceLocations.Count() == 0`
### 3.4 Multiple resource values in a single push
**Steps:**
1. Call `h.OnResourceLocationUpdated(rd1)` then `h.OnResourceLocationUpdated(rd2)` with different `name:host` keys
2. Assert `store.ResourceLocations.Count() == 2`
3. Assert each key returns the correct value
### 3.5 Resource value update overwrites previous value
**Steps:**
1. `h.OnResourceLocationUpdated(rd)` with `NumericValue: 100.0`
2. `h.OnResourceLocationUpdated(rd)` with same `name:host` but `NumericValue: 200.0`
3. Assert stored value is 200.0 (last-write-wins)
**Covers:** Store `Put` is a full replacement, not merge.
### 3.6 Composite key format is `res_name:location`
**Steps:**
1. Push a resource with `ResourceName: "mem"` and `Location: "node01"`
2. Assert `store.ResourceLocations.Has("mem:node01") == true`
3. Assert `store.ResourceLocations.Has("mem") == false`
---
## 4. P1-3: Heartbeat Keep-Alive
> **Isolation requirement:** `heartbeatIntervalSecs` is a package-level `var int32` in the `backend` package shared across all tests in that package. Tests in this section MUST NOT call `t.Parallel()`. Run the full heartbeat test group sequentially to avoid cross-test interference via `SetHeartbeatInterval`.
### 4.1 Heartbeat interval updates on cluster config push
**Steps:**
1. `atomic.StoreInt32(&heartbeatIntervalSecs, 30)` (default)
2. Call `h.OnClusterUpdated(&ClusterInfo{ClusterName: "c1", HeartbeatInterval: 45})`
3. Assert `atomic.LoadInt32(&heartbeatIntervalSecs) == 45`
**Covers:** The P1-3 integration point — cluster config push triggers heartbeat interval change.
### 4.2 Cluster push with HeartbeatInterval=0 preserves current interval
**Steps:**
1. `atomic.StoreInt32(&heartbeatIntervalSecs, 45)`
2. Call `h.OnClusterUpdated(&ClusterInfo{ClusterName: "c1", HeartbeatInterval: 0})`
3. Assert `atomic.LoadInt32(&heartbeatIntervalSecs) == 45` (unchanged)
**Covers:** The `> 0` guard in `OnClusterUpdated` and `SetHeartbeatInterval`.
---
## 5. Cross-Feature Integration
### 5.1 Config push updates heartbeat interval AND stores cluster data
**Steps:**
1. Default `heartbeatIntervalSecs = 30`
2. Call `dispatchConfigPush(h, nil, nil, nil, &ClusterInfo{ClusterName: "c1", HeartbeatInterval: 60, Vcpus: 32})`
3. Assert `store.Clusters.Get("c1")` has `HeartbeatInterval=60, Vcpus=32`
4. Assert `atomic.LoadInt32(&heartbeatIntervalSecs) == 60`
**Covers:** P0-1 + P1-3 interaction in a single dispatch.
### 5.2 Config push + host control on same host
**Steps:**
1. `dispatchConfigPush(h, nil, nil, []HostInfo{{HostName: "node01", HostModel: "x86_64"}}, nil)`
2. Assert `store.Hosts.Get("node01")` has `HostModel="x86_64"`
3. Assert `store.Hosts.Has("node01") == true` — verifies config-push data is visible in the host store
4. Call `preCheckHostControlRequest(&HostControlRequest{ControlAction: "close", Hostname: []string{"node01"}})` — assert `allFlag == false`, `err == nil` (validates action format; store lookup is handled by `validateHost` which requires JWT — see §2.4 note)
**Covers:** Data created by config push is written to the host store and is visible to subsequent operations.
### 5.3 Resource push after host config push — same host referenced
**Steps:**
1. `h.OnHostConfigUpdated(&HostInfo{HostName: "node01", HostModel: "x86_64"})`
2. `h.OnResourceLocationUpdated(&ResourceDistributionInfo{ResourceName: "mem", Location: "node01", NumericValue: 65536.0, ValueType: NUMERIC, IsDynamic: true})`
3. Assert `store.Hosts.Get("node01")` has `HostModel="x86_64"` (preserved)
4. Assert `store.ResourceLocations.Get("mem:node01")` has `NumericValue=65536.0`
5. Assert `store.Hosts.Get("node01").HostModel` still "x86_64" (host config was not overwritten)
**Covers:** Two independent push paths don't interfere with each other's store data.
### 5.4 Worker push after config push preserves config fields
**Steps:**
1. `h.OnHostConfigUpdated(&HostInfo{HostName: "node01", HostModel: "x86_64", MaxSlot: 8})`
2. `h.OnHostUpdated(&HostInfo{HostName: "node01", HostStatus: 3, CPUFactor: 2.5, NumCPUs: 16})`
3. Assert `got.HostModel == "x86_64" && got.MaxSlot == 8` (config preserved)
4. Assert `got.HostStatus == 3 && got.CPUFactor == 2.5` (worker updated)
**Covers:** Two-push merge correctness — already tested in unit tests but verified here at integration scope.
### 5.5 Multiple sequential config pushes (idempotent)
**Steps:**
1. Push config with `ClusterInfo{Vcpus: 16, HeartbeatInterval: 30}`
2. Push config with `ClusterInfo{Vcpus: 32, HeartbeatInterval: 60}`
3. Assert store has `Vcpus=32, HeartbeatInterval=60` (last-write-wins for config fields)
4. Assert `heartbeatIntervalSecs == 60`
**Covers:** Rapid config push updates don't leave stale data.
---
## 6. RBAC Integration (End-to-End)
> **RBAC isolation:** Call `resetRBACState()` (not `store.InitStores()`) at the start of each RBAC test. `resetRBACState()` zeros `rbacConfig`, preventing `AddPermissionsForRole` from appending duplicate permissions across test reloads. `store.InitStores()` does not reset RBAC state.
### 6.1 Full middleware chain: JWT → RBAC → handler
**Steps:**
1. Set up Gin router with real middleware (`RequirePermission("host:control")`)
2. Create a valid JWT token for a cluster_admin user
3. POST `/xce/v1/hosts/control` with `action=close, host_name=["node01"]`
4. Assert 202 Accepted (or mocked backend response)
5. Create a valid JWT token for a basic_user
6. POST `/xce/v1/hosts/control` — assert 403 Forbidden
7. POST `/xce/v1/hosts/select` with basic_user — assert passes (host:view)
**Note:** This requires spinning up a test HTTP server via `httptest.NewRecorder`. Do not mock at the middleware level — the purpose of §6 is to exercise the real middleware chain end-to-end.
### 6.2 RBAC: cluster_admin bypasses permission check
**Steps:**
1. Set up Gin router with `RequirePermission("host:control")`
2. Sign a JWT for "admin1" with role "cluster_admin" and issue `POST /xce/v1/hosts/control`
3. Assert response status is **not 403** — verifies the middleware did not block the request
**Note:** `RequirePermission` returns early for `cluster_admin` without calling `ctx.Next()`, so downstream handlers are **never reached**. Do not assert a spy handler body or `ctx.IsAborted() == false` — both give false results. To verify full handler execution for a privileged user, write a separate test with a non-cluster_admin user that has explicit `host:control` permission (see §6.3).
### 6.3 RBAC: basic_user cannot control hosts but can view
**Steps:**
1. Assign user "user1" role "basic_user"
2. Call `RequirePermission("host:control")` — assert 403
3. Call `RequirePermission("host:view")` — assert passes
### 6.4 JWT algorithm confusion is rejected
**Steps:**
1. Configure test server with HS256 signing secret `"xce-api-key"`
2. Craft a token header claiming `"alg": "none"` or a mismatched algorithm (RS256)
3. POST to `/xce/v1/hosts/control` with that token (use valid IP binding so IP mismatch is not the cause of rejection)
4. Assert 401 Unauthorized
5. Assert response body contains a token/signature error string (e.g., `"token"`, `"invalid"`, or `"signature"`) — not `"ip"` or `"expired"` — to confirm the rejection is due to algorithm/signature failure, not an unrelated check
**Covers:** Prevents algorithm confusion attacks where an attacker downgrades signing to `none`.
### 6.5 Expired JWT is rejected
**Steps:**
1. Sign a JWT with `exp = time.Now().Unix() - 3600` (expired 1 hour ago)
2. POST to any protected endpoint
3. Assert 401 Unauthorized
**Covers:** Token replay after expiry — ensures expired tokens cannot be reused.
### 6.6 Known gap: hostgroup domain restriction bypass
> **Documents a known limitation, not a working feature.** `validateHost` at `hosts.go:505-507` contains a `// TODO` that silently skips hostgroup domain enforcement. This test makes the gap explicit so it is visible in code review and prevents accidental regression if the TODO is removed.
**Steps:**
1. Provision a user with `host:control` scoped to hostgroup "gpu-nodes"
2. Issue `POST /xce/v1/hosts/control` targeting a host NOT in "gpu-nodes"
3. **Assert request is accepted** (not 403) — documents that hostgroup scoping is not yet enforced
**Note:** When P1-2b implements hostgroup enforcement, flip this assertion to assert 403 and remove the `// TODO` marker.
### 6.7 Post-logout token reuse
> **Design gap:** JWTs are stateless and the current server does not maintain a JTI blacklist. A token valid at logout time remains valid until expiry. This test documents the current behavior as an explicit design decision.
**Steps:**
1. Sign a valid JWT with a 1-hour expiry
2. Call `POST /xce/v1/users/logout` (or equivalent) — record any logout response
3. Immediately reuse the same token on `GET /xce/v1/hosts` — assert **200 OK** (not 401)
**Note:** If a JTI blacklist is added in future, flip this assertion to expect 401 and add the blacklist provisioning step.
---
## 7. Store Thread Safety
> **CI requirement:** All §7 tests must run with `-race`: `go test -race ./store/...`. A test that passes without `-race` provides no guarantee of absence of data races — the Go race detector instruments memory accesses at runtime and is the authoritative check.
### 7.1 Concurrent config pushes don't corrupt data
**Steps:**
1. Start 10 goroutines, each calling `h.OnHostConfigUpdated(&HostInfo{HostName: "node01", ...})` with slightly different HostModel values
2. Wait for all goroutines to complete
3. Assert `store.Hosts.Get("node01")` returns a valid HostInfo (no zero-value corruption)
4. Assert `HostName` field is "node01"
**Race detection note:** A simple value-validity assertion cannot prove absence of a data race — concurrent writes all produce plausible values. Run this test with `go test -race ./store/...` to detect RMW races via the Go race detector. The assertion above verifies structural integrity; `-race` detects the underlying race condition.
### 7.2 Concurrent host control + config push
**Steps:**
1. Goroutine A: call `OnHostConfigUpdated` for "node01" in a loop
2. Goroutine B: call `validateHost` checking "node01" existence in a loop
3. Assert no panics and no store corruption after 100 iterations
### 7.3 Concurrent resource value pushes for same key
**Steps:**
1. Start 10 goroutines, each calling `OnResourceLocationUpdated` for `"mem:node01"` with different NumericValue
2. Assert final stored value is one of the 10 values (no corruption)
3. Assert key exists exactly once
---
## 8. Input Safety
### 8.1 Hostname length limits
> **Skip until P1-2b is implemented.** No hostname length validation exists in `preCheckHostControlRequest` yet — this test will pass vacuously. Add `//go:build p1_2b` build tag or comment out pending that task.
**Steps:**
1. Call `preCheckHostControlRequest` with hostname exceeding the P1-2b limit (>1024 chars)
2. Assert `err != nil` with message indicating length exceeded
### 8.2 Empty hostname list
**Steps:**
1. Call `preCheckHostControlRequest` with `Hostname: []`
2. Assert `err != nil` — inspect the actual error message from `preCheckHostControlRequest` and assert it contains the expected string (e.g., "no hostnames provided" or similar)
### 8.3 Select expression length (current limit: 4094)
**Steps:**
1. Call handler with `Select` string > 4094 chars
2. Assert error
3. Call with 4094 chars — assert accepted
### 8.4 Special characters in hostnames
**Steps:**
1. Call with hostname containing `<script>alert(1)</script>`
2. Assert the hostname is stored as a string without interpretation (XSS safety at API layer)
---
## Test File Organization
| File | Tests |
|------|-------|
| `backend/integration_test.go` | §1 (config push), §3 (resource push), §4 (heartbeat), §5 (cross-feature) |
| `controllers/integration_test.go` | §2.1–2.3, §2.5–2.8 (host control — no JWT required), §2.4 (host validation — **requires full JWT setup; see §6 prerequisites**), §6 (RBAC middleware), §8 (input safety) |
| `store/concurrency_test.go` | §7 (thread safety) |
All test files use `//go:build !cgo` or `CGO_ENABLED=0` compatible patterns.
**Package declarations:**
- `backend/integration_test.go` must declare `package backend` (not `package backend_test`) to access unexported symbols: `heartbeatIntervalSecs`, `rawResvalToDistribution`, `dispatchConfigPush`
- `controllers/integration_test.go` may use `package controllers_test` for black-box tests, but any test accessing unexported helpers must use `package controllers`
**JWT signing secret in tests:** Integration tests in `controllers/integration_test.go` that create real JWTs must use a test-only signing secret (e.g., `"xce-api-key"` from the test config file). Never use the production signing secret. Document the expected test secret in a comment at the top of the test file so it is clearly identifiable during security audits.
---
## Priority Classification
| Priority | Tests | Rationale |
|----------|-------|-----------|
| **P0 — Must have** | 1.1, 1.2, 1.3, 2.1, 2.2, 2.5, 3.1, 3.4, 4.1, 5.1 | Core functionality and merge correctness |
| **P1 — Should have** | 1.4, 1.5, 2.4, 2.6, 2.8, 3.2, 3.3, 3.5, 3.6, 4.2, 5.2, 5.3, 6.1, 6.2, 6.3, 6.4, 6.5, 6.6, 6.7 | Edge cases, RBAC, JWT security, and known gap documentation |
| **P2 — Nice to have** | 2.3, 2.7, 5.4, 5.5, 7.1, 7.2, 7.3, 8.1–8.4 | Robustness and stress |
---
## Existing Test Coverage
The following unit tests already cover much of the ground above and should NOT be duplicated:
| Existing Test File | What It Covers | Integration Gap |
|--------------------|----------------|-----------------|
| `backend/handler_test.go` | Single-push merge for hosts, clusters, dispatch | Multi-push sequences, cross-feature |
| `backend/convert_test.go` | All 7 value types, nil val, unknown discriminant | Store round-trip after conversion |
| `backend/heartbeat_test.go` | SetHeartbeatInterval, runHeartbeat call/cancel | Interval change triggered by OnClusterUpdated |
| `controllers/host_control_test.go` | preCheck validation, concurrent no-race | Full handler flow with store interaction |
| `controllers/rbac_check_test.go` | host:control/host:view permissions | Full middleware chain with JWT |

325
src/docs/apiserver-backend-reviews.md
View file

@ -1,325 +0,0 @@
---
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.*

271
src/docs/cluster-config-split-analysis.md Normal file
View file

@ -0,0 +1,271 @@
# ClusterConfig Schema Split Analysis
> Analysis date: 2026-06-08. Branch: persistent_ha.
> Source schema: `src/schema/controller.capnp:680`, struct `ClusterConfig`.
## Summary
The `ClusterConfig` struct (20 fields) mixes three distinct configuration concerns:
cluster-wide business config, HA peer membership, and per-controller performance
tunables. These should be split into separate objects so each can evolve
independently without forcing a re-push of the full config blob to all standbys.
## Current State Discovery
### Peer list is already structurally separate
The `--peers` CLI flag (`main.c:178-210`) parses into `myconf.peers_cfg`
(`xce_cluster_config_t`), which is a **separate struct** from the config blob's
`cluster_config_t`. At startup, `cluster_init_phase1` uses `peers_cfg.candidates`
to determine `my_name`, `my_port`, `my_priority`, and the peer list — all before
the config blob is loaded (`cluster.c:865-880`). The config blob's `candidates`
field is **not consulted** for HA peer discovery; it is carried for backward
compatibility in the blob but superseded by `--peers`.
### HA tunables are hardcoded at phase1
`cluster_init_phase1` uses compile-time defaults for all HA timing parameters
(`cluster.c:854-863`):
```c
ha_config_t ha_cfg = {
.heartbeat_interval_ms = HA_DEFAULT_HEARTBEAT_INTERVAL_MS,
.heartbeat_timeout_ms = HA_DEFAULT_HEARTBEAT_TIMEOUT_MS,
.election_timeout_ms = HA_DEFAULT_ELECTION_TIMEOUT_MS,
...
};
```
The `heartbeatInterval` and `inactiveHeartbeatCount` fields in
`ClusterConfig` are consumed later (e.g., `cluster.c:581-585` writes them into
a config key-value store for runtime access), but the HA init path does not
read them. The heartbeat timeout validation in `startup_validation.c:144` works
against the hardcoded defaults, not the config blob values.
### vcpus is already deprecated
Field `@7 vcpus` is marked deprecated in the schema comment: "use --vcpus CLI
flag; kept for config blob backward compatibility." `cluster_init_phase2`
reads `c->myconf->vcpus` (set from CLI), not the blob's `vcpus` field
(`cluster.c:1045`).
## Proposed Split: Three Configuration Objects
```
┌──────────────────────────────┐
│ Current ClusterConfig │
│ (20 fields) │
└──────────────┬───────────────┘
│
┌───────────────────┼───────────────────┐
▼ ▼ ▼
┌───────────────────┐ ┌─────────────────┐ ┌──────────────────────┐
│ ClusterConfig │ │ PeersConfig │ │ ControllerConfig │
│ (cluster-wide) │ │ (HA membership) │ │ (per-controller) │
└───────────────────┘ └─────────────────┘ └──────────────────────┘
```
### Group 1: Cluster-Wide Business Config (keep in `ClusterConfig`)
These fields define the cluster's behavior, resources, and policy. They **must
be identical** across all controllers. When modified via `OP_upgradeConfig`,
the new blob is pushed to all standbys and committed on majority.
| Field | Ord | Type | Rationale |
|-------|-----|------|-----------|
| `clusterName` | @0 | Text | Cluster identity — must be identical cluster-wide |
| `resources` | @8 | List(Resource) | Resource catalog — shared state across all nodes |
| `queues` | @9 | List(QueueConf) | Queue definitions — cluster scheduling policy |
| `ugroups` | @10 | List(GroupConfig) | User group membership — access control |
| `hgroups` | @11 | List(GroupConfig) | Host group membership — scheduling constraints |
| `hostConfigs` | @12 | List(HostConfig) | Per-host overrides — cluster-wide host knowledge |
| `hproviders` | @18 | List(ScalerConfig) | Autoscaler provider config — cluster infrastructure |
| `limits` | @19 | List(LimitConfig) | Resource limits — cluster policy enforcement |
| `configGeneration` | @20 | UInt64 | Monotonic version counter — replication consistency |
**How consumed**: `cluster_init_phase2` (after election and config load)
iterates these to populate `c->resmgr`, `c->jobmgr`, queue tables, host
groups, etc.
### Group 2: HA Peer Membership (extract to `PeersConfig`)
These fields define **who** is in the cluster. They change independently when
controllers are added, removed, or re-IP'd — without any change to the
business config.
| Field | Ord | Type | Rationale |
|-------|-----|------|-----------|
| `candidates` | @2 | List(MasterCandidate) | HA peer list — changes independently of business config |
| `port` | @1 | UInt32 | Fallback listener port — derived from candidates; per-controller in practice |
**Already structurally separate**: The `--peers` CLI flag populates
`myconf.peers_cfg` (`xce_cluster_config_t` in `src/include/clusterconfig.h`),
a struct distinct from the config blob's `cluster_config_t`. The candidates
list in the blob is carried for backward compatibility but is not read for HA
peer discovery at startup. Making the separation formal means:
- `PeersConfig` travels via its own capnp message type and replication path
- Cluster config upgrades via `OP_upgradeConfig` no longer need to carry the candidate list
- Peer list changes (add/remove controllers) are a dedicated operation with its own quorum semantics
**Candidate list has special quorum requirements**: Changing the peer set is a
Raft cluster membership change. It must not use the same
`repl_wait_majority_ack` path as business config because the majority
denominator itself is changing. A separate message type enables a two-phase
membership change protocol (joint consensus) in the future.
**Impact of removing `candidates` from `ClusterConfig` (verified against codebase):**
`cluster_build_config()` (`cluster.c:575–609`) is the **only** place that reads
`cluster_config->candidates` and `cluster_config->ncandidates` — it copies them
into the runtime KV store as `ncandidates` and `candidate_N_{name,host,port}`.
No other component (scheduler, apiserver, opget, sched) reads those KV keys.
The live peer list used by `ha.c:3266–3272` and `cluster_open_listener` comes
exclusively from `peers_cfg.candidates[]` (parsed by `--peers`). Removing
candidates from the blob is therefore a **zero-blast-radius** change.
| File | Location | Change |
|------|----------|--------|
| `src/controller/cluster.c` | 587–606 | Remove `ncandidates` + `candidate_N_*` KV loop from `cluster_build_config()` |
| `src/include/clusterconfig.h` | 34–35 | Remove `int ncandidates` and `master_candidate_t** candidates` from `cluster_config_t` |
| `src/schema/controller.capnp` | 683 | Remove `candidates @2: List(MasterCandidate)` from `ClusterConfig` |
**`MasterCandidate` fields**:
```
struct MasterCandidate {
name @0: Text; // controller identity (e.g., "ctrl-1")
host @1: Text; // hostname or IP
port @2: UInt32; // peer listener port
}
```
### Group 3: Per-Controller Performance Tunables (extract to `ControllerConfig`)
These fields configure **how** an individual controller process behaves. They
can legitimately differ across controllers (e.g., a lower-memory node with
fewer vcpus, or a WAN-connected standby with a longer heartbeat timeout).
| Field | Ord | Type | Rationale |
|-------|-----|------|-----------|
| `heartbeatInterval` | @3 | UInt32 | Primary heartbeat send interval — can be tuned per primary |
| `inactiveHeartbeatCount` | @4 | UInt32 | Missed-heartbeat tolerance multiplier |
| `fetchJobInterval` | @5 | UInt32 | Job fetch poll interval — local scheduler interaction |
| `netioTimeout` | @6 | UInt32 | Network I/O timeout — environment-specific |
| `vcpus` | @7 | UInt32 | **Already deprecated** — use `--vcpus` CLI flag; remove from schema |
| `jobmgr` | @13 | HashZArrayConfig | Job manager hash table sizing (nHashSlots, delta) |
| `workermgr` | @14 | HashZArrayConfig | Worker manager hash table sizing |
| `qmgr` | @15 | HashZArrayConfig | Queue manager hash table sizing |
| `resmgr` | @16 | ResmgrConfig | Resource manager hash + instance table sizing |
| `xstrings` | @17 | HashZArrayConfig | String interning table sizing |
**How consumed**:
- `jobmgr`/`workermgr`/`qmgr`/`resmgr`/`xstrings` are read once at
`cluster_init_phase2` to construct the internal key-value stores
(`cluster.c:803-808`). After construction, the config values are not referenced again.
- `heartbeatInterval`/`inactiveHeartbeatCount` are read at runtime by the
heartbeat and liveness tasks (`cluster.c:1180, 1195`).
- `fetchJobInterval` drives the scheduler polling loop.
- `netioTimeout` is used by the iomgr layer for TCP timeouts.
**Why these can differ per controller**: A controller on a high-memory machine
can be configured with larger hash tables. A controller on a high-latency WAN
link can have a longer netioTimeout. These differences do not affect cluster
correctness — they only affect local process behavior.
**`vcpus` removal**: Already deprecated. The `--vcpus` CLI flag
(`main.c:1045`) is the canonical source. Remove field @7 in the next
schema revision.
## Fields With Dual Usage (require care)
### `port` (@1)
**Decision: Remove. No fallback bind port.**
`cluster_config->port` currently serves as a fallback in `cluster_open_listener`
(`cluster.c:953–975`) when `--peers` does not specify a port for the matching
candidate. This fallback is removed: the `--peers` port is mandatory.
**Affected code (all changes mechanical):**
| File | Location | Change |
|------|----------|--------|
| `src/controller/cluster.c` | 953–975 | Remove the `if (my_port == 0) { cluster_config->port }` branch; return `EINVAL` immediately if no port found in `peers_cfg` |
| `src/controller/myconf.c` | 74 | Remove `SET_DEFAULT_IF_UNSET(c->port, 0, 9990)` |
| `src/controller/config_repl.c` | 55 | Remove `SET_DEFAULT_IF_UNSET(c->port, 0, 9990)` |
| `src/include/clusterconfig.h` | 33 | Remove `int port` from `cluster_config_t` |
| `src/schema/controller.capnp` | 682 | Remove `port @1: UInt32` from `ClusterConfig` |
Note: `"port"` in `testsuite/test.c:124,266` is the scheduler/worker
connect-to-primary port (via `get_setting("port")`), not the listener bind
port. It is unaffected.
## Implementation Approach
### Phase 1: Remove `port` and `candidates` from `ClusterConfig` (immediate)
No backward-compat transition needed for these two fields — the live code
already ignores both (peer discovery uses `peers_cfg`; port fallback is being
deleted). All changes are mechanical.
1. `src/schema/controller.capnp` — Remove `port @1` and `candidates @2` from
`ClusterConfig`; mark or remove `MasterCandidate` struct if no longer
referenced elsewhere
2. `src/include/clusterconfig.h` — Remove `int port`, `int ncandidates`,
`master_candidate_t** candidates` from `cluster_config_t`
3. `src/controller/cluster.c` — Remove `if (my_port == 0)` fallback block in
`cluster_open_listener` (lines 967–975); remove `ncandidates` + `candidate_N_*`
KV loop in `cluster_build_config` (lines 587–606)
4. `src/controller/myconf.c` and `src/controller/config_repl.c` — Remove
`SET_DEFAULT_IF_UNSET(c->port, 0, 9990)` from `fill_default_cluster_config`
5. Regenerate capnp bindings
### Phase 2: Schema split (wire-compatible)
1. Define `PeersConfig` and `ControllerConfig` structs in `controller.capnp`
2. Add them as new fields in the top-level `ConfigData` union alongside
the existing `clusterconfig @2`
3. Regenerate capnp bindings
4. **Keep the remaining `ClusterConfig` fields intact** during transition —
new fields are additive
### Phase 3: Controller reads from new structs
1. `cluster_init_phase2`: read `ControllerConfig` for tunables
2. `cluster_config_t` retains business config fields only
3. Fall back to old `ClusterConfig` fields when new structs are absent
(backward compatibility for existing `cluster.bin` files)
### Phase 4: Remove superseded tunables from `ClusterConfig`
1. Remove `heartbeatInterval`, `inactiveHeartbeatCount`, `fetchJobInterval`,
`netioTimeout`, `vcpus` from `ClusterConfig`
2. Remove `jobmgr`, `workermgr`, `qmgr`, `resmgr`, `xstrings` from
`ClusterConfig`
### Phase 5 (future): Peer list replication
1. Define `OPCode_peersUpdate` / `OPCode_peersUpdateAck` for peer list
replication with joint-consensus semantics
2. Cluster membership changes become a dedicated operation with its own
quorum protocol
## Effort and Risk
| Phase | Effort | Risk | Justification |
|-------|--------|------|---------------|
| 1 — Remove port + candidates | XS (½d) | Low | No transition needed; dead code removal + schema field removal |
| 2 — Schema split | S (1d) | Low | Additive capnp change; old fields preserved |
| 3 — Read from new structs | M (2-3d) | Medium | Touches cluster_init paths; backward compat needed |
| 4 — Remove superseded tunables | S (1d) | Low | Mechanical removal after Phase 3 is validated |
| 5 — Peer replication | L (1-2wk) | High | New protocol; joint consensus; extensive testing |
## References
- `src/schema/controller.capnp:680-700` — `ClusterConfig` definition
- `src/schema/controller.capnp:253` — `ConfigData.clusterconfig @2`
- `src/controller/main.c:178-213` — `--peers` CLI parsing into `peers_cfg`
- `src/controller/cluster.c:854-880` — `ha_config_t` hardcoded defaults + priority derivation
- `src/controller/cluster.c:945-977` — port derivation from candidates vs config blob
- `src/controller/cluster.c:803-808` — internal manager construction
- `src/controller/ha.h:134` — `ha_config_t.heartbeat_interval_ms` default via `ha_init`
- `src/controller/startup_validation.c:133-163` — heartbeat config validation
- `src/include/clusterconfig.h` — `xce_cluster_config_t` (separate peers struct)

2350
src/docs/config-replication-design.md Normal file
View file

File diff suppressed because it is too large Load diff

116
src/docs/config-replication-impl-status.md Normal file
View file

@ -0,0 +1,116 @@
# Config Replication — Implementation Status
> **Branch**: `persistent_ha`
> **Date**: 2026-05-22 (updated)
> **Design doc**: `config-replication-design.md`
---
## Phase 1 (Pull-only replication) — WIRE PROTOCOL COMPLETE
Phase 1 is fully implemented. Standbys fetch `ClusterConfig` from the primary on
every startup over a real SSL connection, eliminating manual per-node provisioning.
The wire protocol (capnp encode/decode, SHA-256 verify, disk save) is complete as
of 2026-05-22 (commits `2ed7e34`–`5053c8d`).
| Component | Status | Commit | Notes |
|---|---|---|---|
| Cap'n Proto schema additions | ✅ Done | `683c092` | 3 opcodes, 6 structs, `configGeneration`, `HeartbeatMsg` |
| Struct fields (`myconf_t`, `cluster_t`, `ha_state_t`, `persist_t`) | ✅ Done | `2c375cf` | `config_version_tag`, `config_sha256`, `conf_keep_n`, `conf_stale_timeout`, `restart_requested`, `standby_endpoints`, `wal_record_count`, `checkpoint_in_progress` |
| `byteorder.h` portability header | ✅ Done | `2c375cf` | `be64toh()` for glibc + musl + macOS |
| HEAD-file config storage (`config_repl.c` Phase 1) | ✅ Done | `491c255` | `conf_save_config`, `conf_read_current_filename`, `conf_read_raw`, `conf_get_saved_tag`, `conf_check_stale`, `conf_trim_old_versions` |
| `conf_save_config()` path fix | ✅ Done | `2ed7e34` | Was writing to `workdir/conf`; fixed to `workdir/name/conf` to match `load_primary_config()` |
| `main.c` — null `conffile`, `--conf-keep-n`, `--conf-stale-timeout` | ✅ Done | `e74fd2a` | Stack buffer removed; conffile heap-allocated at runtime |
| `load_primary_config()` + `myconf_fini()` | ✅ Done | `cd02cf4` | FATAL on missing config; SHA-256 cached; reserved-name check |
| Real `am_i_primary()` — election probing | ✅ Done | `fe3cc57` | Non-blocking connect + poll; priority step + xorshift64 jitter; `ha_is_primary()`, `ha_check_priority_stepdown()` |
| SSL double-call fix + TOCTOU fix | ✅ Done | `ec3cab4` | `SSL_get_error` called once; `ha->lock` held in stepdown |
| `cluster_init` phase split + `myapp_run()` two-phase flow | ✅ Done | `6b06aa0` | Phase 1: hooks/HA/cbuf; Phase 2: iomgr/kvmgr/persist; `am_i_primary` gates config load |
| `ha->role` set before Phase 2 | ✅ Done | `93b8616` | Election result stored via `atomic_store` before `cluster_init_phase2` |
| Raw capnp shims (`request.c`) | ✅ Done | `26393e9` | `xce_replconfig_reply_raw()`, `xce_replconfig_req_ssl()`, `xce_replconfig_reply_ssl_decode()`, `xce_upgrade_config_req_extract_raw()` — bypass `xce_data_t` for raw bytes |
| `handle_config_request()` (primary handler) | ✅ Done | `4ef2089` | Tag compare, SHA-256 double-check; sends real bytes via `xce_replconfig_reply_raw()` |
| `fetch_config_from_primary()` + `do_one_config_fetch()` | ✅ Done | `2aae45f` | Full SSL roundtrip: non-blocking connect+poll, `xce_replconfig_req_ssl()`, SHA-256 verify, `conf_save_config()`, reload; 20-attempt exponential backoff |
| `handle_controller_join()` | ✅ Done | `8f4db30` | Registers endpoint in `standby_endpoints` under lock |
| WAL record-count `schedule_checkpoint()` | ✅ Done | `a95476b` | Idempotent CAS; coalesced via `delaymgr` 1s delay; bounds `persist_recover` to ≤10k records |
| `typemap.h` union arms + capnp shims | ✅ Done | `491c255` | `replconfigreplymsg`, `upgradeconfigreplymsg`, etc. wired |
| API server rejects `lost_and_found` job submissions | ✅ Done | `bc92166` | `preCheckJobRequest()` returns "internal recovery queue" error before store lookup; 3 tests in `jobs_test.go` |
---
## Phase 2 (Remote config push) — COMPILED, GATED
Phase 2 infrastructure ships compiled but inert. The `CONF_ENABLE_UPGRADE_CONFIG`
compile-time gate (default: 0) prevents `OP_upgradeConfig` from being reachable
in production builds until both prerequisites are met (see §5.3 and §2 of the
design doc).
| Component | Status | Notes |
|---|---|---|
| `handle_upgrade_config()` | ✅ Wired, gated | `5053c8d` — pre-decode opcode peek in `myreq_handler()` captures raw packet bytes; `xce_upgrade_config_req_extract_raw()` extracts sha256+bytes; delegates to `do_upgrade_config_with_bytes()`. Active only when `CONF_ENABLE_UPGRADE_CONFIG=1` |
| `push_config_to_standbys()` | ✅ Timeout enforced | `f3babec` — `wait_all_with_timeout()` with `CONF_PUSH_TIMEOUT_MS` (500ms) aggregate deadline. Push fan-out wire protocol still best-effort; standbys catch up via pull on reconnect |
| `create_lost_and_found()` | ✅ Done | `91e66b8` | Inserts synthetic queue via `create_queue()` and host via `makeWorker()` before `persist_recover()`; gated with `#ifndef XCE_TESTING` for unit test binary |
**Prerequisites before enabling** (`CONF_ENABLE_UPGRADE_CONFIG=1`):
1. `validate_host()` implemented with real CN allowlist check (see `FUTURE_WORK.md §6`)
2. Admin config tool exists and tested end-to-end (see `FUTURE_WORK.md §1`)
---
## Test Coverage
| Suite | Tests | Covers |
|---|---|---|
| `test_config_repl` | 24 | Storage, SHA-256, trim, stale, sanitise_cn, path consistency, push timeout, capnp shim roundtrips |
| `test_am_i_primary` | 3 | Priority-0 immediate, priority-0 with peers, deadline-expire |
| `test_ha_role` | (existing) | Role transitions, watchdog, redirect |
| `test_persist_wal` | 6 (+3 new) | WAL append, CAS idempotency, `max_wal_records` default |
| `test_ha_cluster` | 5 | Cluster HA integration |
| `test_p1_integration` | 5 | Phase 1 init structure |
| All other suites | ~41 | 0 failures |
| **Total** | **84** | **0 failures, 0 ignored** |
---
## Known Limitations / Deferred Work
| Item | Severity | Tracked |
|---|---|---|
| Heartbeat listener (`start_heartbeat_listener`) is a no-op stub — simultaneous-boot symmetry relies on priority-step delay only | LOW | `FUTURE_WORK.md §7` |
| Peers cross-validation hash in `OP_heartbeat` not implemented | LOW | `FUTURE_WORK.md` |
| `OP_replConfigPush` standby-side handler not implemented — pushed config accepted only via pull on reconnect | LOW | `FUTURE_WORK.md` |
| WAL routing helpers (`resolve_queue_or_lost_and_found` etc.) not wired in `apply_wal_entry()` | LOW | `FUTURE_WORK.md` |
---
## Files Changed Summary
| File | Change |
|---|---|
| `src/schema/controller.capnp` | +3 opcodes, +6 structs, `configGeneration`, `HeartbeatMsg` |
| `src/include/myconf.h` | +6 fields to `myconf_t`; `load_primary_config`, `fetch_config_from_primary`, `myconf_fini` declared |
| `src/include/xce/cluster_def.h` | +4 fields to `cluster_t` |
| `src/controller/ha.h` | +`probe_ctx_t`; +3 fields to `ha_state_t`; +2 fields to `ha_config_t`; `xorshift64_next` inline |
| `src/include/persist.h` | +`max_wal_records`; +`wal_record_count`, `checkpoint_in_progress` atomics |
| `src/include/byteorder.h` | **New** — portable `be64toh()` |
| `src/include/config.h` | +`DEFAULT_MAX_WAL_RECORDS 10000` |
| `src/include/typemap.h` | +7 union arms for new message types |
| `src/include/request.h` | +4 raw shim declarations; `SSL*` parameters |
| `src/lib/request.c` | +`xce_replconfig_reply_raw()`, `xce_replconfig_req_ssl()`, `xce_replconfig_reply_ssl_decode()`, `xce_upgrade_config_req_extract_raw()` |
| `src/controller/config_repl.h` | **New** — Phase 1+2 API declarations; `handle_upgrade_config` raw-pkt params |
| `src/controller/config_repl.c` | **New** — ~900 LOC: storage, SHA-256, full SSL fetch, handlers, push timeout, upgrade wiring |
| `src/controller/ha.c` | Real `am_i_primary()`, `probe_thread()`, `ha_is_primary()`, `ha_check_priority_stepdown()`, `ha_seed_jitter()` |
| `src/controller/myconf.c` | `load_primary_config()`, `myconf_fini()`, static `cf` global removed |
| `src/controller/cluster.c` | `cluster_init_phase1/2` split; opcode dispatch; pre-decode peek for `OP_upgradeConfig`; `standby_endpoints` pruning; `restart_requested` check |
| `src/controller/myapp.c` | Two-phase startup; `am_i_primary` gates config load; `myconf_fini` on all exits |
| `src/controller/join.c` | `handle_controller_join` implemented (was `-ENOSYS`) |
| `src/controller/persist.c` | `schedule_checkpoint()`, WAL record-count trigger, `persist_init` defaults |
| `src/controller/main.c` | `conffile=NULL`; `--conf-keep-n`; `--conf-stale-timeout`; peers validation |
| `src/controller/meson.build` | `config_repl.c` added; `ssl_dep` for unit tests |
| `src/controller/include/cluster.h` | `cluster_init_phase1/2` declared |
| `src/controller/testsuite/test_config_repl.c` | 24 unit tests (was 18; +path consistency, +push timeout, +capnp roundtrips) |
| `src/controller/testsuite/test_ha_role.c` | +3 election probing tests |
| `src/controller/testsuite/test_persist_wal.c` | +3 WAL bound tests |
| `src/controller/testsuite/controller_test.c` | New suites registered |
| `src/frontend/apiserver/controllers/jobs.go` | +reserved-name guard in `preCheckJobRequest()` |
| `src/frontend/apiserver/controllers/jobs_test.go` | **New** — 3 tests: lost_and_found rejected, normal queue passes, empty queue passes |
| `src/docs/config-replication-design.md` | §3.2 `effective_dir` definition; §6.3 timeout implementation note |
| `src/docs/config-replication-impl-status.md` | This file |

77
src/docs/p1-2-host-select.md
View file

@ -1,71 +1,52 @@
# P1-2: Host Resource Requirement Select — Investigation
## Status
- [>] Under investigation (moved from apiserver-backend-tasks.md)
- **C layer (P1-2a): COMPLETE** — `eval_select_simple` wired in `xce_select.c`
(PEG grammar → AST → CSE, memoized eval). Scheduler integration (Phase 6) done.
See `src/docs/resreq-ast-cse-design.md` for architecture reference.
- **Go bridge (P1-2b): PENDING** — `EvalSelectForHost` remains a stub returning
`errNoCGO`. CGO boundary, resource value lookup from in-memory store, and
`FilterHostByResReqSelect` are still to implement.
---
## Background
The `select` field in an LSF-style resource requirement string (e.g. `select[mem>4096 && type==X86_64]`)
filters candidate hosts before slot allocation. Currently `EvalSelectForHost` is a no-CGO stub that
always returns `errNoCGO`.
The `select` field in an LSF-style resource requirement string (e.g.
`select[mem>4096 && type==X86_64]`) filters candidate hosts before slot
allocation. Currently `EvalSelectForHost` is a no-CGO stub that always returns
`errNoCGO`.
The full feature requires two layers:
1. **C parser + evaluator** — parse the boolean select expression into an AST and evaluate it against
a host's resource values.
2. **Go bridge** — call the C evaluator via CGO and wire it into `FilterHostByResReqSelect` in the
controllers layer.
1. **C parser + evaluator** — parse the boolean select expression into an
AST and evaluate it against a host's resource values.
2. **Go bridge** — call the C evaluator via CGO and wire it into
`FilterHostByResReqSelect` in the controllers layer.
---
## Sub-tasks
### #11 P1-2a: C select expression parser (PEG + eval)
### #11 P1-2a: C select expression parser (PEG + eval) ✓ COMPLETE
**Files implemented:**
- `include/resreq.h` — expression-tree types, enums, `extern` declarations.
- `lib/xce_select.peg` — PEG grammar for select boolean expressions.
- `lib/xce_select.c` — `parse_select()`, `eval_select_simple()`, CSE tree.
- `lib/meson.build` — `xce_select.peg` in build.
### #12 P1-2b: Go EvalSelectForHost + FilterHostByResReqSelect (remaining)
**Files to touch:**
- `include/resreq.h` — Replace empty `xce_resreq_select_` stub with expression-tree types, enums,
and `extern` declarations for `parse_select`, `eval_select`, `free_select`.
- `lib/xce_select.peg` — New PEG grammar for select boolean expressions (right-recursive, handles
`&&`, `||`, `!`, comparison operators, numeric literals, string literals, resource names).
- `lib/resreq.c` — Implement `parse_select()`, `eval_select()`, `free_select()`; update
`kill_primary_resreq()` to call `free_select()`.
- `lib/meson.build` — Add `xce_select.peg` to explicit source list so the PEG tool generates C.
- `lib/testsuite/` — TDD tests for parse/eval before implementing.
**Open questions:**
- Which PEG library is already available in the Nix/Meson build? (`peg`, `lemon`, `re2c`?)
- Exact grammar for comparison operators — LSF allows `mem > 4096`, `type == X86_64`, `swp >= 0`.
Need to confirm all supported operators and value types.
- Does `eval_select` receive a flat key-value map of resource values, or a pointer to the full
`xce_host_t` struct?
- Error reporting strategy — return a bool+error-string, or a tri-state (true/false/parse-error)?
### #12 P1-2b: Go EvalSelectForHost + FilterHostByResReqSelect (blocked by #11)
**Files to touch:**
- `backend/clib.go` — Implement `EvalSelectForHost(hostname, selectExpr string) error` using
`cgo.Handle` for callback context; add `#include "resreq.h"`, extern C declarations,
`xce_select_destroy` helper.
- `backend/clib.go` — Implement `EvalSelectForHost(hostname, selectExpr string) error`
using `cgo.Handle` for callback context; add `#include "resreq.h"`, extern C
declarations, `xce_select_destroy` helper.
- `controllers/hosts.go` — Implement `FilterHostByResReqSelect(hosts []models.HostInfo, expr string)`,
update `preCheckHostRequest()` limit from 4094 → 1024 bytes.
**Open questions:**
- Does `EvalSelectForHost` need to look up live resource values from the in-memory store, or are
values passed in directly by the C layer?
- `cgo.Handle` usage pattern — confirm whether a per-call handle or a session-level handle is needed.
- Does `EvalSelectForHost` look up live resource values from the in-memory
store, or are values passed directly from C?
- `cgo.Handle` usage pattern — per-call or session-level?
- Input validation: max expression length (1024 bytes), allowed characters.
---
## Investigation Checklist
- [ ] Survey existing PEG tooling in the Meson/Nix build environment.
- [ ] Read `include/resreq.h` current state to understand `xce_resreq_select_` stub.
- [ ] Trace how `kill_primary_resreq()` works today and where `free_select()` must be called.
- [ ] Confirm resource value structure passed to `eval_select`.
- [ ] Confirm LSF select grammar spec (operators, value types, nesting depth).
- [ ] Prototype PEG grammar for a subset (numeric comparisons only) and get it compiling.
- [ ] Decide error propagation strategy across the CGO boundary.
- [ ] Write TDD tests for C parser before any implementation.

1738
src/docs/persistence-and-ha-design.md
View file

File diff suppressed because it is too large Load diff

21
src/docs/phase6-audit.md
View file

@ -1,21 +0,0 @@
# Phase 6 Pre-Audit: Single-Threaded Invariant and Stub Locations
## Audit Results
- **sched_meet_host_selection stub confirmed** at `src/scheduler/src/resource.c:127-131`
- Declaration: `src/scheduler/include/scheduler.h:111`
- Call site: `src/scheduler/src/resource.c:368` (within sched_try_alloc_on_host_)
- Stub body: returns 1 unconditionally
- **Allocator-thread-only invariant confirmed**: Single `while(1)` loop in `src/scheduler/src/main.c:119`
- `sched_begin_session()` at line 121
- `sched_go()` at line 122
- `sched_end_session()` at line 161
- All three calls executed sequentially in the same thread context
- **eval_select_simple signature confirmed**: `int eval_select_simple(const xce_select_node_t* root, const host_eval_ctx_t* host)`
- Declaration: `src/include/resreq.h:110-111`
- Definition: `src/lib/xce_select.c:558`
- Returns: 1 (match), 0 (no match), negative (error)
No unexpected references to `sched_meet_host_selection`, `eval_select`, `alloc_thread`, or `sched_session` found in scheduler source.

1284
src/docs/phase6-scheduler-integration-plan.md
View file

File diff suppressed because it is too large Load diff

233
src/docs/resreq-ast-cse-impl-plan.md
View file

@ -1,233 +0,0 @@
# Resreq AST + CSE Implementation Plan
> Generated from: `resreq-ast-cse-design.md`
> Date: 2026-04-28
> Branch: merge_api_server
---
## Summary Priority Order
```
P0 (blocking): makeString() TOCTOU fix, section.peg boundary bug ✓ DONE
Phase 1: TDD test skeletons (parallel with Phase 3) ✓ DONE
Phase 2: Header definitions + ABI audit ✓ DONE
Phase 3: PEG grammar (parallel with Phase 1) ✓ DONE 143 PASS / 3 FAIL (stubs)
Phase 4a: Core interning + sel_optimize_level ✓ DONE 145 PASS / 1 FAIL (Phase 4b stub)
Phase 4b: Type validation (gated on P0-1) ✓ DONE 203 PASS (2026-04-29)
Phase 4c: AND/OR associativity flattening ✓ DONE 157 PASS (2026-04-29)
Phase 4d: Const-fold transform ✓ DONE (2026-04-29)
Phase 4e: Memoized eval ✓ DONE 73 PASS (2026-04-29)
Phase 4f: GC + recompaction ✓ DONE 73 PASS (2026-04-29)
Phase 5: Build wiring ✓ DONE
Phase 6: Scheduler integration ✓ DONE 10 PASS (2026-04-30)
```
---
## P0 — Critical Prerequisites (blocking gate)
Must complete before any Phase 4 work begins.
| # | Task | Section | Rationale |
|---|------|---------|-----------|
| P0-1 | ~~**Fix `makeString()` TOCTOU in `src/lib/xstring.c`**~~ **DONE** — extended lock region to cover `makeInstance` + `key_add`, eliminating the TOCTOU window. `key_add` can no longer return `-EEXIST` on a miss path. | §5.4 | Silent NULL return corrupts the interned AST in all §7 numeric-to-string coercions; blocks Phase 4b entirely |
| P0-2 | ~~**Fix `section.peg` clause-boundary grammar**~~ **DONE** — Root cause: the original grammar used multiple ordered-choice alternatives in `select_statement` / `usage_statement` / `span_statement`, but PEG's `e*` always succeeds (matches empty), so alternatives 2+ were dead code (confirmed by packcc generating "defined but not used" for all `pcc_action_select_statement_N` functions). Fix: collapsed each statement rule into a single alternative with a compound negative lookahead covering all subsequent clause keywords. `select_statement` now stops at any of `rusage[`, `span[`, `same[`, `affinity[`, or `]`; `usage_statement` stops at `span[`, `same[`, `affinity[`, or `]`; `span_statement` stops at `same[`, `affinity[`, or `]`. Both implicit (`type==any span[ptile=1]`) and explicit (`select[type==any] span[ptile=1]`) select forms verified by TDD: `test_parse_section3` (implicit), `test_parse_section_explicit_select_with_span`, `test_parse_section_explicit_select_rusage_span`, `test_parse_section_span_not_in_select` — all PASS, no packcc dead-code warnings. | §3.1 | Silent span/rusage content bleed into selectString in all implicit-select + multi-clause inputs |
---
## Phase 1 — TDD Tests (write failing skeletons first) ✓ DONE
**Priority: HIGH — must come before implementation (TDD contract)**
Develop concurrently with Phase 3 (grammar is the test oracle).
| # | Task | Notes |
|---|------|-------|
| 1-1 | ~~Write `lib/testsuite/test_select.c` skeletons: parse valid expressions, unknown resource names (error), type mismatches (error)~~ **DONE** | |
| 1-2 | ~~Write tests: `SELECT_MAX_INPUT_LEN` (8192) overflow, `SELECT_MAX_DEPTH` (64) nesting overflow~~ **DONE** | |
| 1-3 | ~~Write CSE tests: `A&&B` and `B&&A` produce same pointer; two `makeSelectNode` calls with same args return same pointer~~ **DONE** — FAIL until Phase 4a | |
| 1-4 | ~~Write eval tests: numeric `>/<`/`==`, string `==`, bool standalone, `!`/`&&`/`||`, short-circuit~~ **DONE** | |
| 1-5 | ~~Write GC tests: unlink job root → `gc_sweep_select_nodes` frees unreachable node; subsequent `makeSelectNode` re-interns fresh node~~ **DONE** — FAIL until Phase 4f | |
| 1-6 | ~~Write GC predicate contract tests (`sched_session_active`, `sched_eval_in_progress`, `staging_merge_complete`) using `fake_scheduler_t` scaffold in `test_select.c`~~ **DONE** — FAIL until Phase 4f | Phase 6 wires real scheduler — signatures stable |
| 1-7 | ~~Create shared resmgr fixture: `src/lib/testsuite/test_resmgr_fixture.h` + `.c` with `make_test_resmgr()`/`free_test_resmgr()`~~ **DONE** | |
---
## Phase 2 — Header Definitions ✓ DONE
**Priority: HIGH — unblocks all implementation work**
| # | Task | Notes |
|---|------|-------|
| 2-1 | ~~Add `res_type_t`, `sel_node_type_t`, `xce_select_node_t` typedef + struct to `include/resreq.h`~~ **DONE** | |
| 2-2 | ~~Fill `struct xce_resreq_select_` with `xce_select_node_t* root`~~ **DONE** | |
| 2-3 | ~~Declare all public APIs: `makeSelectNode`, `eval_select`, `eval_select_simple`, `select_eval_ctx_*`, `sel_node_table_*`, `gc_sweep_select_nodes`, `recompact_node_ids`~~ **DONE** | `gc_mark_tree` stays `static` in `.c` |
| 2-4 | ~~Add `var resmgr` field to `resreq_context_t` (outer PEG auxil)~~ **DONE** | |
| 2-5 | ~~Add `valtype_to_res_type` helper; omit `res_type_to_valtype` (no current consumer — YAGNI)~~ **DONE** | |
| 2-6 | ~~Add `xce_str_match` as `static inline` wrapping `strcmp`~~ **DONE** | |
| 2-7 | ~~**CGO audit**: `grep -rn 'parse_resreq\|C\.parse_resreq' src/frontend/`~~ **DONE** — no CGO callers found | |
| 2-8 | ~~Update `parse_resreq_primary` and `parse_resreq` signatures to accept `var resmgr`~~ **DONE** — call sites updated: `schedobj.c:171`, `test_resreq.c:244/301/330` | |
---
## Phase 3 — PEG Grammar (`lib/xce_select.peg`) ✓ DONE
**Priority: HIGH — develop concurrently with Phase 1**
| # | Task | Notes |
|---|------|-------|
| 3-1 | ~~Implement `select_expr` grammar (§3.1): `\|\|`, `&&`, `!`, comparisons, resname, literals, `any`~~ **DONE** | |
| 3-2 | ~~Implement `select_context_t` auxil with `resmgr`, `depth`, `errmsg[256]`, `has_error`~~ **DONE** | |
| 3-3 | ~~Implement `resname` action: `key_find`, `res_idx` validity guard, `valtype_to_res_type` mapping~~ **DONE** | |
| 3-4 | ~~**Clause-keyword conflict check** in `resname` action: check against `{select, rusage, span, same, affinity}`~~ **DONE** | |
| 3-5 | ~~Implement `SELECT_MAX_INPUT_LEN` (8192) and `SELECT_MAX_DEPTH` (64) guards~~ **DONE** | |
| 3-6 | ~~LSF-compatible disambiguation: bare word vs known resource name → `SEL_RESNAME` vs `SEL_STR`; `any` excluded by grammar; `true`/`false` produce migration diagnostic~~ **DONE** | |
| 3-R1 | ~~**[Review fix — HIGH]** Switch `SEL_STR` from `strdup`/`free` to `makeString`/`dereference` in `xce_select.peg` and `xce_select.c`~~ **DONE** — xstring-interned strings now refcounted; `kill_select_node` calls `dereference()` | |
| 3-R2 | ~~**[Review fix — MEDIUM]** Add `_Static_assert(sizeof(uintptr_t) >= sizeof(double))` before `makeSelectNode` in `xce_select.c`~~ **DONE** — guards the `SEL_NUM` bit-cast at compile time | |
| 3-R3 | ~~**[Review fix — MEDIUM]** Add `!("any" ![a-zA-Z_0-9])` negative lookahead to `resname_node` and `resname_leaf` in `xce_select.peg`~~ **DONE** — prevents `any` keyword from being tried as a resource name | |
---
## Phase 4a — Core Node Allocation and Interning ✓ DONE
**Priority: HIGH — foundational; unblocks 4b–4f**
| # | Task | Notes |
|---|------|-------|
| 4a-1 | ~~Implement `sel_optimize_level` (`static _Atomic int`, sentinel -1) with `sel_optimize_set()` write-once CAS and `sel_get_optimize_level()` accessor~~ **DONE** | |
| 4a-2 | ~~Implement `sel_make_key()` — zero padding bytes via struct literal init; SEL_RESNAME zeroes `op2` for key purposes (res_type stored in node, not in key)~~ **DONE** | |
| 4a-3 | ~~Implement `sel_key_hash` over the 3 key fields~~ **DONE** | |
| 4a-4 | ~~Implement `sel_node_table_init` / `sel_node_table_fini` — `makeInstance(&kvtab_type, &params)` with **`datakiller=NULL`**, `zarray_init` for linear nodes array~~ **DONE** | |
| 4a-5 | ~~Implement `makeSelectNode` — CSE-disabled fast path; CSE-enabled path with commutativity sort, key lookup, TOCTOU double-check under lock, node_id assignment via `atomic_fetch_add`, zarray rollback via `atomic_fetch_sub`~~ **DONE** | |
| 4a-6 | ~~Implement `sel_node_to_key()` for GC key reconstruction (must apply same commutativity sort as `makeSelectNode`)~~ **DONE** | |
| 4a-7 | ~~Add `sel_optimize_test_reset()` test-only helper (`#ifdef UNIT_TEST`): `atomic_store_explicit` reset to -1 plus `sel_node_table_fini`~~ **DONE** | |
| 4a-R1 | ~~**[Review fix — HIGH]** Move `node_id` assignment inside lock after `key_add` succeeds so TOCTOU losers need no counter rollback~~ **DONE** | Counter divergence under concurrent load |
| 4a-R2 | ~~**[Review fix — HIGH]** Add `dereference((char*)op1)` on first-lock cache hit for SEL_STR — caller transfers xstring ownership~~ **DONE** | Ref leak on every cache hit for string nodes |
| 4a-R3 | ~~**[Review fix — HIGH]** Add `SEL_MAX_INTERNED_NODES` cap check in both lock sections (first miss + TOCTOU re-check)~~ **DONE** | Unbounded table growth without cap |
| 4a-R4 | ~~**[Review fix — HIGH]** Fix `sel_node_table_fini` to iterate and free all interned nodes before `destroy(table)` — NULL datakiller does not free them~~ **DONE** | All nodes leaked at teardown; LSAN clean now |
| 4a-R5 | ~~**[Review fix — MEDIUM]** Set `node->hashcode = sel_key_hash_fn(&key)` in CSE path — was left 0, breaking Phase 4e memoization~~ **DONE** | |
| 4a-R6 | ~~**[Review fix — MEDIUM]** Add `dereference((char*)op1)` on `calloc` failure in level==0 path~~ **DONE** | xstring ref leak on OOM |
| 4a-R7 | ~~**[Review fix — MEDIUM]** Add `-EINVAL` guard on `key_find` — corrupt table must not silently proceed~~ **DONE** | |
| 4a-R8 | ~~**[Review fix — MEDIUM]** Add `-EEXIST` recovery in `key_add` failure path — re-find and return the racing winner~~ **DONE** | Rare concurrent insert race |
| 4a-R9 | ~~**[Review fix — LOW]** Change `_Static_assert(sizeof(uintptr_t) >= ...)` to `==` per spec requirement~~ **DONE** | |
---
## Phase 4b — Parse-Time Type Validation ✓ DONE (2026-04-29)
**Priority: HIGH — gate: P0-1 `makeString()` fix must be merged first**
| # | Task | Notes |
|---|------|-------|
| 4b-1 | ~~Wire `parse_select()` into `parse_resreq_primary` **before** `kill_resreq_data`~~ **DONE** | Use-after-free if called after `kill_resreq_data` frees `data.selectString` |
| 4b-2 | ~~Implement EQ/NE type validation: reject BOOL×STR; coerce SEL_NUM to SEL_STR when comparing with string resource (`snprintf "%.17g"`), NULL-check `makeString` result, symmetric coercion~~ **DONE** | Free orphaned SEL_NUM node in unoptimized mode before overwriting pointer |
| 4b-3 | ~~Implement `>/<`/`>=`/`<=` type validation: reject non-numeric operands~~ **DONE** | `log_fatal` (not `assert`) for belt-and-suspenders at eval time too |
| 4b-4 | ~~Implement `res_idx` validity guard and unknown-resource error; add migration diagnostic for `true`/`false` identifiers (parse as SEL_RESNAME with no match)~~ **DONE** | |
| 4b-5 | ~~Update `kill_primary_resreq` for conditional teardown: CSE-enabled → null root only; CSE-disabled → `free_select_tree()` then `FREEUP`~~ **DONE** | NULL store before `FREEUP` to prevent GC mark dangling-pointer race |
| 4b-6 | ~~**LC_NUMERIC enforcement**: wrap PEG parser call in `parse_select` with `newlocale(LC_NUMERIC_MASK, "C")` / `uselocale` / `freelocale`; `log_fatal` on failure in scheduler, `log_error`+return in API server~~ **DONE** | `_XOPEN_SOURCE 700` required before system headers |
| 4b-7 | ~~**strtod locale coverage check**~~ **DONE — SAFE**: packcc emits single `strtod` in `pcc_action_value_node_1` (line 1593 of generated file), inside the `uselocale` scope — no locale vulnerability | |
| 4b-R1 | ~~**[Review fix — HIGH]** Add `lock(auxil->resmgr, RTTI_LOCK_READ)` / `unlock(auxil->resmgr)` around `key_find` in `resolve_resname()` (`xce_select.peg:38`) — missing read lock caused data race in multi-threaded scheduler~~ **DONE** | |
| 4b-R2 | **[Review check — F2]** STRING×NUM coercion error paths reviewed: `makeSelectNode` releases the xstring on OOM failure (level-0 path calls `dereference(op1)` before returning NULL); outer `makeSelectNode(op_type)` failure correctly calls `SEL_DISCARD(l)` + `SEL_DISCARD(r)` — **no leak, false positive** | |
---
## Phase 4c — Commutativity + Associativity Flattening ✓ DONE (2026-04-29)
| # | Task | Notes |
|---|------|-------|
| 4c-1 | ~~Verify commutativity sort in `makeSelectNode` step 1 covers AND/OR/EQ/NE; non-commutative GT/LT/GE/LE/NOT unchanged~~ **DONE** — `test_cse_gt_not_commutative` confirms GT is not commutative; AND/OR/EQ/NE sort verified. | Already integrated in §5.4 design |
| 4c-2 | ~~Implement post-parse **flatten pass** for `&&`/`||` chains: `do { changed = flatten_pass(root); } while (changed)`~~ **DONE** — `sel_flatten_pass` added to `xce_select.c`; do-while loop in `parse_select()` in `resreq.c`; 7 tests: noop, single-level, deep 3+, OR, mixed-ops, CSE mode. 157 PASS. | Repeat-until-stable needed for depth-3+ explicit parenthesization |
| 4c-3 | ~~In unoptimized mode: `flatten_pass` must `free()` each orphaned node — not its children (re-parented children must not be freed)~~ **DONE** — level==0 path frees orphaned intermediate node (`free(new_right); free(node)`) after re-parenting A/B/C; SEL_NOT branch frees `node` only. | Children freed would cause double-free when `free_select_tree` walks from root |
**Senior review fixes applied (2026-04-29):**
- **[MEDIUM — OOM safety]** In AND/OR branch: update `node->u.binop.left/right` in-place (level==0) immediately after recursive calls, before any `makeSelectNode`. This ensures `return node` after OOM returns a valid tree with live (not dangling) children.
- **[LOW — dead code]** Removed dead `if (node->type == SEL_STR ...)` / `if (new_right->type == SEL_STR ...)` guards in AND/OR and SEL_NOT branches — those nodes are never SEL_STR in those arms.
---
## Phase 4d — Const-Fold Transform ✓ DONE (2026-04-29)
| # | Task | Notes |
|---|------|-------|
| 4d-1 | ~~`resname == any` → `SEL_TRUE`; `resname != any` → `SEL_FALSE` — check **both** child slots for SEL_ANY pointer (commutativity sort is ASLR-nondeterministic across runs)~~ **DONE** | |
| 4d-2 | ~~Identity laws: `SEL_TRUE && X` → `X`; `SEL_FALSE \|\| X` → `X`~~ **DONE** | |
| 4d-3 | ~~Annihilation laws: `SEL_FALSE && X` → `SEL_FALSE`; `SEL_TRUE \|\| X` → `SEL_TRUE`~~ **DONE** | |
| 4d-4 | ~~Complement laws: `!SEL_TRUE` → `SEL_FALSE`; `!SEL_FALSE` → `SEL_TRUE`~~ **DONE** | Single-pass — const-fold shrinks trees, does not create new flattening opportunities |
---
## Phase 4e — Evaluation with Memoization ✓ DONE (2026-04-29)
| # | Task | Notes |
|---|------|-------|
| 4e-1 | ~~Implement `eval_get_bool`/`eval_set_bool` 2-bit bitmap accessors with `node_id >= bool_bits_cap` guards using `log_fatal`~~ **DONE** | `_Static_assert(EVAL_NONE == 0)` for memset-reset correctness |
| 4e-2 | ~~Implement `eval_select_inner` — recursive eval with **explicit** error propagation (`if (lv < 0) return -1`), NOT C `&&`/`||` (C's `-1` is truthy, would swallow errors)~~ **DONE** | SEL_ANY reaching eval → `log_fatal("SEL_ANY reached eval — const-fold not applied")` |
| 4e-3 | ~~Leaf cache writes in `eval_select_inner`: SEL_RESNAME writes to `ctx->num_vals[node_id]` or `ctx->str_vals[node_id]`; comparison ops call `eval_select(child, ctx)` then read cached arrays~~ **DONE** | **Critical**: `get_leaf_value()` must NOT be called directly — bypasses cache writes, silently reads 0.0/NULL |
| 4e-4 | ~~Implement memoized `eval_select` wrapper — bitmap cache check before calling `eval_select_inner`; cache EVAL_ERROR too (node-structural errors repeat per host)~~ **DONE** | |
| 4e-5 | ~~Implement `eval_select_simple` for unoptimized path — plain recursive tree-walk, no `select_eval_ctx_t`, thread-safe (no shared state)~~ **DONE** | |
| 4e-6 | ~~Add `eval_select` guard: `log_fatal` if `sel_get_optimize_level() <= 0`~~ **DONE** | |
| 4e-7 | ~~Implement `select_eval_ctx_init` / `select_eval_ctx_reset` / `select_eval_ctx_fini`~~ **DONE** | `node_count` snapshot at init; `reset` uses snapshot, never re-reads `sel_node_next_id` |
| 4e-8 | ~~Pre-session capacity check: `memset` extended bool_bits bytes after realloc (realloc does not zero extension); partial-OOM contract — all three reallocs (bool_bits, num_vals, str_vals) must succeed atomically or `log_fatal`~~ **DONE** | Mismatched capacities allow out-of-bounds write to num_vals with no inline bounds check |
---
## Phase 4f — Mark-and-Sweep GC + Recompaction ✓ DONE (2026-04-29)
| # | Task | Notes |
|---|------|-------|
| 4f-1 | ~~Implement `gc_mark_tree` — recursive DFS, early return on `node == NULL \|\| node->gc_mark`; handles leaf / SEL_NOT / binary node types~~ **DONE** — `__attribute__((unused))` for Phase 6 wire-in | Recursion bounded by `SELECT_MAX_DEPTH` (64); any increase requires stack analysis |
| 4f-2 | ~~Implement `gc_sweep_select_nodes` — Phase 6 guard placed **before** mark-clear (prevents two-call sweep-of-reachable-nodes bug); mark-clear iterates `caps`; sweep iterates `caps`, `zarray_del` before `free`~~ **DONE** — live_jobs traversal deferred to Phase 6 (sched/types.h not in libxce scope) | GC does NOT acquire `sel_node_table.lock` (idle-phase invariant) |
| 4f-3 | Insert **`atomic_thread_fence(memory_order_seq_cst)`** after all `primary->select->root = NULL` unlinks and before `gc_sweep_select_nodes` call — **Phase 6 call-site task** | Without fence: CPU/compiler may reorder NULL store after GC mark pass |
| 4f-4 | Runtime GC idle invariant guards at GC call site (`sched_session_active`, `sched_eval_in_progress`, `staging_merge_complete`) — `log_fatal` not `assert` — **Phase 6 call-site task** → see 6-3/6-4 | `assert` compiled out by NDEBUG; use-after-free would result |
| 4f-5 | ~~Implement `recompact_node_ids` — iterate `nodes` zarray over `caps` slots (not `count`, gaps exist after `zarray_del`), assign sequential `node_id`, reset `sel_node_next_id` via `atomic_store_explicit(..., memory_order_release)`~~ **DONE** | `node_id` and zarray slot index are **decoupled** — do not conflate |
| 4f-R1 | ~~**[Review fix — HIGH]** Move Phase 6 early-return guard to before mark-clear loop — prior ordering allowed two consecutive calls (njobs>0 then njobs==0) to sweep still-reachable nodes~~ **DONE** | Two-call scenario: first call clears all marks and returns; second call sweeps everything |
| 4f-R2 | ~~**[Review fix — HIGH]** Swap `free(node)` / `zarray_del` order — `zarray_del` NULLs slot first, then `free`; eliminates latent use-after-free if killer is ever installed on nodes zarray~~ **DONE** | |
| 4f-R3 | ~~**[Bug fix]** `sel_node_table_fini` iterated `nodes.count` instead of `nodes.caps` — after `zarray_del` creates gaps, new nodes slot into positions beyond `count` boundary and leaked~~ **DONE** | Manifested as 32-byte ASAN leak after GC sweep + reintern in same lifetime |
---
## Phase 5 — Build Wiring
| # | Task | Notes |
|---|------|-------|
| 5-1 | ~~Add `custom_target` for `xce_select.peg` in `lib/meson.build` matching `section_parser`/`rusage_parser` pattern~~ **DONE** | Verify `-o 'lib/xce_select'` path format: `grep -A8 'custom_target' src/lib/meson.build \| grep '\-o'` |
| 5-2 | ~~Add `xce_select_parser[0]` (generated `.c`) to `libxce_src`~~ **DONE** | |
| 5-3 | ~~Add `test_resmgr_fixture.c` to testsuite `executable()` sources in `lib/meson.build`~~ **DONE** | |
---
## Phase 6 — Scheduler Integration
**Status: ✓ DONE — 2026-04-30 — 10/10 scheduler tests pass**
| # | Task | Notes |
|---|------|-------|
| 6-1 | ~~**Pre-Phase-6 audit**: confirm allocator-thread-only invariant~~ **DONE** `bb4a95c` | Single-threaded confirmed; stub at `resource.c:127` |
| 6-2 | ~~**Blocking gate**: all `sched_meet_host_selection` call sites accounted for~~ **DONE** `bb4a95c` | No missed call sites |
| 6-3 | ~~Add `_Atomic int session_active`, `eval_in_progress`, `staging_merge_complete` to `xce_scheduler_t`~~ **DONE** `823a66e` | Use `_Atomic int` not `volatile int` — ARM atomicity guarantee |
| 6-4 | ~~Implement GC predicate wrappers referencing real `xce_scheduler_t` fields~~ **DONE** `acda9bb` | Signatures unchanged; only wrapper body changed |
| 6-5 | ~~Implement `sched_meet_host_selection` via `eval_select_simple`~~ **DONE** `40261b7` | Full implementation replacing stub that always returned 1 |
| 6-6 | ~~`sched_meet_host_selection` returns int (0/1); preason on fail~~ **DONE** `40261b7` | int return kept; preason wired |
| 6-7 | ~~Implement `build_host_eval_ctx`~~ **DONE** `907d8d8` | Flattens `worker->resvals` zarray to `numvals`/`strvals` using `Value_u_fvalue/svalue/bvalue` |
| 6-8 | ~~Materialize `live_jobs` flat array from `sched->jobs` kvmgr~~ **DONE** `5267d36` | Growth-loop heap alloc; OOM-safe (double-guarded: `calloc` fail + `realloc` null-sentinel `742d79e`, `fb5127a`) |
| 6-9 | ~~Wire `sel_optimize_set(1)` at scheduler startup~~ **DONE** `acda9bb` | Called in `sched_init`; no `sel_optimize_set(0)` needed (API server has separate process) |
| 6-10 | ~~Set `sel_alloc_thread_id`; add thread-ID checks in `eval_select` and `gc_sweep_select_nodes`~~ **DONE** `b722e5c` `1eb4eb1` | `_Atomic int` registered flag + `pthread_equal` guard; registered before `while(1)` loop |
| 6-11 | ~~Write integration/parity tests against known-good resreq/host pairs~~ **DONE** `5fcf85c` | Numeric (`SEL_GE`) and string (`SEL_EQ`) pass/fail cases |
| 6-12 | ~~`atomic_thread_fence(memory_order_seq_cst)` before `gc_sweep_select_nodes`~~ **DONE** `5267d36` | After all `gc_mark_select_tree` calls in `sched_end_session` |
| R1 | ~~**[Review fix — CRITICAL]** `staging_merge_complete` bootstrap deadlock — GC never ran~~ **DONE** `d438742` | `sched_begin_session` never set it to 1; guard in `sched_end_session` always fired |
| R2 | ~~**[Review fix — HIGH]** `recompact_node_ids()` missing from GC protocol~~ **DONE** `d438742` | Added after `gc_sweep_select_nodes` in `sched_end_session` |
---
## Key Implementation Traps
1. **`free_select_tree` vs GC mode confusion** — calling `free_select_tree` in CSE-enabled mode or forgetting it in disabled mode both produce memory corruption. The `sel_get_optimize_level()` guard inside `free_select_tree` catches this with `log_fatal`.
2. **`get_leaf_value()` bypass bug** — calling `get_leaf_value()` directly in comparison branches bypasses the `num_vals`/`str_vals` cache write. Comparisons silently read 0.0/NULL (calloc default) instead of actual host values. Every comparison operator must call `eval_select(child, ctx)` first, then read the typed cache array.
3. **TOCTOU in `makeSelectNode`** — the double-check under lock (step 5) prevents node leaks when two threads race to intern the same node. The re-find must happen inside the lock or the winner could be freed before the loser reads it.
4. **`atomic_thread_fence` before GC** — root-unlink writes (`primary->select->root = NULL`) must be visible to the GC mark pass. Without a seq_cst fence, CPU/compiler reordering causes GC to retain a node that should be swept (or vice versa).
5. **Partial-OOM in `select_eval_ctx_t` resize** — if one of the three reallocs (bool_bits, num_vals, str_vals) succeeds and another fails, the capacities diverge and `eval_select_inner` can write out-of-bounds to `num_vals` (no inline bounds guard at write site). Treat all three as atomic — `log_fatal` on partial failure.

50
src/docs/xce.md
View file

@ -1,32 +1,58 @@
Help of XCE
===========
This is the entrypoint of xce tool. Currently, we have following sub-commands available:
This is the entrypoint of xce tool. Currently, we have following sub-commands
available:
- controller
- worker
- scheduler
- apiserver
- shell
- **controller** — start xce controller daemon
- **worker** — start xcecd worker daemon
- **scheduler** — start xce scheduler daemon
- **apiserver** — start xce REST API server
- **shell** — interactive bash shell for troubleshooting
- **xcecli** — CLI tool for cluster interaction (job submit, query, admin)
If you run xce without any parameter or with parameter which is not in above, then it will show this help screen
If you run xce without any parameter or with a parameter not listed above, it
will show this help screen.
## controller
This command with start xce controller with given parameters after controller sub-command
Start the xce controller daemon with the given parameters after the
`controller` sub-command.
Example:
```
xce controller --config /etc/xce/controller.yaml
```
## scheduler
This command with start xce scheduler with given parameters after scheduler sub-command
Start the xce scheduler daemon with the given parameters after the
`scheduler` sub-command.
## worker
This command will start xcecd with given parameters after worker sub-command
Start xcecd with the given parameters after the `worker` sub-command.
## apiserver
This command will start xce apiserver with given parameters after apiserver sub-command
Start the xce REST API server with the given parameters after the
`apiserver` sub-command. The API server provides HTTP endpoints for job
submission, queue management, and cluster queries, backed by JWT
authentication and RBAC authorization.
## xcecli
The xce CLI (`src/frontend/xcecli/`) provides command-line access to the
cluster. Common operations:
- `xcecli job submit <jobspec>` — submit a job
- `xcecli job list` — list jobs
- `xcecli queue status` — show queue state
Built as a standalone Go binary; communicates with the API server over
its REST interface.
## shell
An interactive bash shell to start, this is just for trouble shoot
An interactive bash shell used for troubleshooting and inspection. Not
intended for normal cluster operation.