diff --git a/src/docs/apiserver-backend-tasks.md b/src/docs/apiserver-backend-tasks.md index 5fbe3f3e..f5887434 100644 --- a/src/docs/apiserver-backend-tasks.md +++ b/src/docs/apiserver-backend-tasks.md @@ -105,10 +105,12 @@ ## P1-3: Heartbeat Keep-Alive -- [ ] **#13 P1-3: Heartbeat keep-alive implementation** - - `backend/clib.go` — Add `heartbeatIntervalSecs int32` (default 30), `SetHeartbeatInterval()`, `heartbeatFn` var, `StartHeartbeat(ctx context.Context)` - - `backend/handler.go` — `OnClusterUpdated` calls `SetHeartbeatInterval(cluster.HeartbeatInterval)` after store update - - `cmd/main.go` — Call `backend.StartHeartbeat(ctx)` after `backend.RegisterToBackend(...)` +- [x] **#13 P1-3: Heartbeat keep-alive implementation** + - `backend/clib.go` — Added `heartbeatIntervalSecs int32` (default 30), `SetHeartbeatInterval()`, `heartbeatFn` var, `StartHeartbeat(ctx context.Context)` goroutine with ticker + - `backend/handler.go` — `OnClusterUpdated` calls `SetHeartbeatInterval(cluster.HeartbeatInterval)` after store update (zero-preserve: skips if interval==0) + - `cmd/main.go` — Calls `backend.StartHeartbeat(ctx)` after `backend.RegisterToBackend(...)` + - `backend/handler_test.go` — Tests for heartbeat interval update, zero-preserve, timer leak, test drain, store consistency + - Commits: `ffc5b20` (goroutine), `0a3de8b` (leak/drain/consistency fixes) --- @@ -119,4 +121,11 @@ - `ninja -C build` for C components passes — required meson reconfigure + CGO val_helpers.h fix - RBAC verification — `host:control` (cluster_admin) and `host:view` (all authenticated) confirmed in routes + rbac.yaml - Input safety — comment limit 512 chars; select expression limit 4094 chars (1024 reduction deferred to P1-2b) - - **CGO fix applied:** extracted `static` val helpers from `clib.go` preamble into `val_helpers.h` shared header; `convert.go` now includes it; added explicit `float64()` cast for `C.double` return \ No newline at end of file + - **CGO fix applied:** extracted `static` val helpers from `clib.go` preamble into `val_helpers.h` shared header; `convert.go` now includes it; added explicit `float64()` cast for `C.double` return + +- [x] **#15 Integration test suite** — see [`apiserver-backend-integration-test-plan.md`](apiserver-backend-integration-test-plan.md) + - `backend/integration_test.go` — §1 (config push), §3 (resource values), §4 (heartbeat), §5 (cross-feature): 17 tests + - `controllers/integration_test.go` — §2 (host control), §6 (RBAC), §8 (input safety): 17 tests + 1 skip (§8.1 pending P1-2b) + - `store/concurrency_test.go` — §7 (thread safety): 3 concurrent-access tests + - All use `//go:build !cgo`; run with `CGO_ENABLED=0 go test ./backend/... ./controllers/... ./store/...` + - Commit: `86a65d6` \ No newline at end of file diff --git a/src/docs/phase6-scheduler-integration-plan.md b/src/docs/phase6-scheduler-integration-plan.md new file mode 100644 index 00000000..a0dd51ca --- /dev/null +++ b/src/docs/phase6-scheduler-integration-plan.md @@ -0,0 +1,1284 @@ +# Phase 6 — Scheduler Integration Implementation Plan + +> **Status: ✓ COMPLETE — 2026-04-30 — 10/10 scheduler tests pass** | branch: `merge_api_server` + +**Goal:** Wire the select-expression AST evaluator (`eval_select_simple`) into the scheduler so that `sched_meet_host_selection` filters hosts by actual job select expressions instead of always returning 1. + +**Architecture:** Export GC mark primitives from `libxce` to break the circular dependency; add `_Atomic` GC-predicate fields to `xce_scheduler_t`; implement `build_host_eval_ctx` to project `xce_sched_worker_t.resvals` into `host_eval_ctx_t`; call `eval_select_simple` in the stub replacement; drive the full mark→sweep GC from `sched_end_session`. + +**Tech Stack:** C99, Unity test framework, Meson/Ninja build, `nix develop` environment, `libxce` (`src/lib`), scheduler (`src/scheduler`) + +--- + +## File Map + +| File | Change | +|------|--------| +| `src/lib/xce_select.c` | Export `gc_mark_select_tree`; add `gc_clear_marks`; remove clear from `gc_sweep`; remove Phase 6 guard; add `sel_alloc_thread_id` + thread checks | +| `src/include/resreq.h` | Declare `gc_mark_select_tree`, `gc_clear_marks`, `sel_register_alloc_thread` | +| `src/lib/testsuite/test_select.c` | Prefix every `gc_sweep_select_nodes(NULL,0)` call with `gc_clear_marks()` | +| `src/scheduler/include/scheduler.h` | Add `_Atomic int session_active/eval_in_progress/staging_merge_complete` to `xce_scheduler_t`; declare `sched_session_active/sched_eval_in_progress/staging_merge_complete` | +| `src/scheduler/src/scheduler.c` | Add predicate static helpers; wire `sel_optimize_set(1)` in `sched_init`; drive GC from `sched_end_session` with atomic fence | +| `src/scheduler/src/resource.c` | Add `build_host_eval_ctx`; replace `sched_meet_host_selection` stub | +| `src/scheduler/testsuite/meson.build` | Link `resource.c` + `scheduler.c` for integration tests | +| `src/scheduler/testsuite/test.c` | Add `test_meet_host_selection` integration test | + +--- + +## Task 1: Pre-audit — confirm single-threaded invariant and stub locations (6-1, 6-2) + +**Files:** +- Read: `src/scheduler/src/resource.c` +- Read: `src/scheduler/src/main.c` + +- [x] **Step 1: Run the blocking-gate grep** + +```bash +cd /workspace +grep -rn 'sched_meet_host_selection\|meet_host_selection' src/scheduler/ src/lib/ +grep -rn 'eval_select\|alloc_thread\|sched_session' src/scheduler/src/ +``` + +Expected: `sched_meet_host_selection` appears in exactly 3 locations — `scheduler.h` (decl), `resource.c` (stub def), `resource.c` (call site at `sched_try_alloc_on_host_`). No other call sites. + +- [x] **Step 2: Confirm allocator-thread-only invariant** + +```bash +grep -n 'sched_begin_session\|sched_end_session\|sched_go' src/scheduler/src/main.c +``` + +Expected: all three called from the same `while(1)` loop in `main`. Document result: "confirmed single-threaded allocator loop in main.c lines ~119-168". + +- [x] **Step 3: Confirm `eval_select_simple` signature** + +```bash +grep -n 'eval_select_simple' src/include/resreq.h src/lib/xce_select.c +``` + +Expected: `int eval_select_simple(const xce_select_node_t* root, const host_eval_ctx_t* host)` — returns 1 (match), 0 (no match), negative (error). + +- [x] **Step 4: Commit audit result** + +```bash +git add -p # no code changes +git commit -m "chore: phase 6 pre-audit — allocator single-threaded confirmed, stub at resource.c:sched_meet_host_selection" +``` + +--- + +## Task 2: Export GC mark primitives — split gc_sweep_select_nodes (6-8 prerequisite) + +The circular dependency between `xce_select.c` (libxce) and `sched/types.h` (scheduler) means the GC mark pass cannot live inside `gc_sweep_select_nodes`. Solution: export `gc_clear_marks` and `gc_mark_select_tree` so the **scheduler** owns the mark phase. + +`gc_sweep_select_nodes` currently clears marks internally before sweeping. After this task it only sweeps — callers must call `gc_clear_marks()` first. The Phase 6 guard (`if (live_jobs != NULL && njobs > 0) return;`) is removed. + +**Files:** +- Modify: `src/lib/xce_select.c` +- Modify: `src/include/resreq.h` +- Modify: `src/lib/testsuite/test_select.c` + +- [x] **Step 1: Write the failing test** + +In `src/lib/testsuite/test_select.c`, add after the existing GC tests (around line 800): + +```c +static void +test_gc_clear_marks_exported(void) +{ + /* After this call, all nodes in the intern table must have gc_mark == 0. + * We verify by immediately calling gc_sweep (which only sweeps unmarked + * nodes), expecting no crash and consistent node count. */ + sel_optimize_test_reset(); + sel_optimize_set(1); + xstring_table_init(64 * 1024); + sel_node_table_init(); + + /* intern two nodes */ + xce_select_node_t* n1 = makeSelectNode(SEL_NUM, (uintptr_t)1.0, 0); + xce_select_node_t* n2 = makeSelectNode(SEL_NUM, (uintptr_t)2.0, 0); + TEST_ASSERT_NOT_NULL(n1); + TEST_ASSERT_NOT_NULL(n2); + + /* mark n1 manually */ + n1->gc_mark = 1; + + /* clear should reset n1->gc_mark */ + gc_clear_marks(); + TEST_ASSERT_EQUAL_INT(0, n1->gc_mark); + + /* mark n2 so sweep preserves it; n1 (unmarked) is swept */ + gc_mark_select_tree(n2); + TEST_ASSERT_EQUAL_INT(1, n2->gc_mark); + + gc_sweep_select_nodes(NULL, 0); + /* n1 swept — n2 still alive */ + TEST_ASSERT_EQUAL_INT(1, n2->gc_mark); /* mark survives sweep */ + + sel_node_table_fini(); + sel_optimize_test_reset(); +} +``` + +In the `main` function of `test_select.c`, register the test: + +```c +TEST(test_gc_clear_marks_exported); +``` + +- [x] **Step 2: Run test to verify it fails** + +```bash +nix develop --command bash -c "cd /workspace/src && meson setup build --wipe 2>/dev/null || true && ninja -C build lib/testsuite/xce_test 2>&1 | tail -30" +``` + +Expected: compile error — `gc_clear_marks` and `gc_mark_select_tree` undeclared. + +- [x] **Step 3: Add declarations to resreq.h** + +In `src/include/resreq.h`, after the `gc_sweep_select_nodes` declaration (line ~121): + +```c +/* GC mark primitives — called by the scheduler to implement the mark phase. + * Call order: gc_clear_marks() → gc_mark_select_tree(root) per live node + * → gc_sweep_select_nodes(NULL, 0) */ +extern void gc_clear_marks(void); +extern void gc_mark_select_tree(xce_select_node_t* node); +``` + +- [x] **Step 4: Implement gc_clear_marks and gc_mark_select_tree in xce_select.c** + +In `src/lib/xce_select.c`, locate the `gc_mark_tree` function at line ~805: + +```c +static void __attribute__((unused)) gc_mark_tree(xce_select_node_t* node) +``` + +Replace with (non-static, renamed): + +```c +void gc_mark_select_tree(xce_select_node_t* node) +{ + if (node == NULL || node->gc_mark) return; + node->gc_mark = 1; + switch (node->type) { + case SEL_RESNAME: case SEL_NUM: case SEL_STR: + case SEL_ANY: case SEL_TRUE: case SEL_FALSE: + break; + case SEL_NOT: + gc_mark_select_tree(node->u.operand); + break; + default: + gc_mark_select_tree(node->u.binop.left); + gc_mark_select_tree(node->u.binop.right); + break; + } +} +``` + +Above that, add `gc_clear_marks`: + +```c +void gc_clear_marks(void) +{ + if (sel_node_table.table == NULL) return; + int caps = sel_node_table.nodes.caps; + for (int i = 0; i < caps; i++) { + xce_select_node_t* node = + (xce_select_node_t*)zarray_get(&sel_node_table.nodes, i); + if (node == NULL) continue; + node->gc_mark = 0; + } +} +``` + +- [x] **Step 5: Modify gc_sweep_select_nodes — remove clear loop and Phase 6 guard** + +In `src/lib/xce_select.c`, in `gc_sweep_select_nodes` (line ~826): + +Remove the Phase 6 guard block: +```c + if (live_jobs != NULL && njobs > 0) { + return; + } +``` + +Remove the clear loop (step 1, lines ~841-852): +```c + /* 1. Clear all gc_mark bits ... */ + for (int i = 0; i < caps; i++) { + xce_select_node_t* node = + (xce_select_node_t*)zarray_get(&sel_node_table.nodes, i); + if (node == NULL) continue; + node->gc_mark = 0; + } +``` + +The function now starts directly with the `int caps = sel_node_table.nodes.caps;` line and the sweep loop. + +- [x] **Step 6: Update test_select.c — prefix all gc_sweep_select_nodes(NULL,0) calls** + +```bash +grep -n 'gc_sweep_select_nodes(NULL, 0)' src/lib/testsuite/test_select.c +``` + +For every occurrence, prepend `gc_clear_marks();` on the line before. There should be 4 occurrences. Example — each: + +```c +/* before */ +gc_sweep_select_nodes(NULL, 0); + +/* after */ +gc_clear_marks(); +gc_sweep_select_nodes(NULL, 0); +``` + +- [x] **Step 7: Build and run tests** + +```bash +nix develop --command bash -c "cd /workspace/src && ninja -C build && build/lib/testsuite/xce_test -t test_gc_clear_marks_exported" +``` + +Expected: `test_gc_clear_marks_exported` PASS. Run full suite: + +```bash +nix develop --command bash -c "cd /workspace/src && build/lib/testsuite/xce_test" +``` + +Expected: all GC tests PASS. + +- [x] **Step 8: Commit** + +```bash +git add src/lib/xce_select.c src/include/resreq.h src/lib/testsuite/test_select.c +git commit -m "feat: export gc_clear_marks + gc_mark_select_tree; remove clear from gc_sweep" +``` + +--- + +## Task 3: Add _Atomic GC-predicate fields to xce_scheduler_t (6-3) + +**Files:** +- Modify: `src/scheduler/include/scheduler.h` + +- [x] **Step 1: Write the failing test** + +In `src/scheduler/testsuite/test.c`, add: + +```c +#include +#include "scheduler.h" + +static void +test_scheduler_t_has_atomic_gc_fields(void) +{ + xce_scheduler_t sched; + memset(&sched, 0, sizeof(sched)); + + /* Verify atomic fields exist and are zero-initialized */ + TEST_ASSERT_EQUAL_INT(0, atomic_load(&sched.session_active)); + TEST_ASSERT_EQUAL_INT(0, atomic_load(&sched.eval_in_progress)); + TEST_ASSERT_EQUAL_INT(0, atomic_load(&sched.staging_merge_complete)); +} +``` + +In `main`, add: `TEST(test_scheduler_t_has_atomic_gc_fields);` + +- [x] **Step 2: Run to verify it fails** + +```bash +nix develop --command bash -c "cd /workspace/src && ninja -C build scheduler/testsuite/sched_test 2>&1 | tail -20" +``` + +Expected: compile error — `session_active` not a member of `xce_scheduler_t`. + +- [x] **Step 3: Add fields to xce_scheduler_t** + +In `src/scheduler/include/scheduler.h`, add three fields at the end of `xce_scheduler_t` (before the closing `}`, after `cbuf_t decisions;`): + +```c + _Atomic int session_active; /* 1 during scheduling session */ + _Atomic int eval_in_progress; /* 1 while eval_select loop runs */ + _Atomic int staging_merge_complete; /* 1 after staging merge completes */ +``` + +Also add `#include ` near the top of `scheduler.h` (after `#include `). + +- [x] **Step 4: Build and run** + +```bash +nix develop --command bash -c "cd /workspace/src && ninja -C build && build/scheduler/testsuite/sched_test" +``` + +Expected: `test_scheduler_t_has_atomic_gc_fields` PASS. + +- [x] **Step 5: Commit** + +```bash +git add src/scheduler/include/scheduler.h src/scheduler/testsuite/test.c +git commit -m "feat: add _Atomic gc-predicate fields to xce_scheduler_t (6-3)" +``` + +--- + +## Task 4: Add GC-predicate statics in scheduler.c + wire sel_optimize_set (6-4, 6-9) + +**Files:** +- Modify: `src/scheduler/src/scheduler.c` +- Modify: `src/scheduler/include/scheduler.h` + +The predicates are scheduler-internal — `static` helpers in `scheduler.c`. They are called at the GC call site (Task 7). The `fake_scheduler_t` stubs in `test_select.c` remain (unit tests still use them). + +- [x] **Step 1: Write failing test** + +In `src/scheduler/testsuite/test.c`, add: + +```c +static void +test_sel_optimize_level_set_at_init(void) +{ + /* After sched_init, sel_get_optimize_level() must return 1. */ + /* We can check it directly since it's a library call. */ + /* We can't call sched_init (it starts threads) so check indirectly: + * the field is set by sel_optimize_set — test the setter directly. */ + sel_optimize_set(1); + TEST_ASSERT_EQUAL_INT(1, sel_get_optimize_level()); + sel_optimize_set(0); /* reset for other tests */ +} +``` + +Add `#include "resreq.h"` to test includes. + +In `main`, add: `TEST(test_sel_optimize_level_set_at_init);` + +- [x] **Step 2: Run to verify it fails** + +```bash +nix develop --command bash -c "cd /workspace/src && ninja -C build scheduler/testsuite/sched_test 2>&1 | tail -20" +``` + +Expected: compile error — `sel_optimize_set` undeclared (not linked yet — Task 8 will fix meson.build; for now just verify the include path works). + +Note: If it fails on `sel_optimize_set` not linked, proceed to Step 3 and come back. + +- [x] **Step 3: Add static predicate helpers to scheduler.c** + +In `src/scheduler/src/scheduler.c`, after the `#include` block (around line 20), add: + +```c +#include "resreq.h" + +static int sched_session_active(xce_scheduler_t* sched) +{ + return atomic_load_explicit(&sched->session_active, memory_order_acquire); +} + +static int sched_eval_in_progress(xce_scheduler_t* sched) +{ + return atomic_load_explicit(&sched->eval_in_progress, memory_order_acquire); +} + +static int sched_staging_merge_complete(xce_scheduler_t* sched) +{ + return atomic_load_explicit(&sched->staging_merge_complete, + memory_order_acquire); +} +``` + +- [x] **Step 4: Wire sel_optimize_set(1) in sched_init** + +In `src/scheduler/src/scheduler.c`, in `sched_init` (line ~318), after `memset(sched, 0x0, sizeof(*sched));` (line ~357), add: + +```c + sel_optimize_set(1); +``` + +This must come before any `makeSelectNode` calls (which happen during job parsing). The write-once guard in `sel_optimize_set` ensures it can only be set once per process. + +- [x] **Step 5: Build** + +```bash +nix develop --command bash -c "cd /workspace/src && ninja -C build 2>&1 | grep -E 'error:|warning:|FAILED|OK' | head -20" +``` + +Expected: builds cleanly (the test executable link will be fixed in Task 9 meson update). + +- [x] **Step 6: Commit** + +```bash +git add src/scheduler/src/scheduler.c +git commit -m "feat: add sched GC predicate statics; wire sel_optimize_set(1) in sched_init (6-4, 6-9)" +``` + +--- + +## Task 5: Implement build_host_eval_ctx (6-7) + +`build_host_eval_ctx` flattens `xce_sched_worker_t.resvals` (a `zarray_t` of `xce_val_t*` indexed by `res_idx`) into the `numvals`/`strvals` arrays expected by `host_eval_ctx_t`. + +Capnp constants (from `controller.capnp.h`): `Value_u_fvalue = 5` (float), `Value_u_svalue = 4` (string), `Value_u_bvalue = 6` (bool → stored as 0.0/1.0 in `numvals`). + +**Files:** +- Modify: `src/scheduler/src/resource.c` + +- [x] **Step 1: Write the failing test** + +In `src/scheduler/testsuite/test.c`, add: + +```c +#include "sched/types.h" +#include "resreq.h" +#include "zarray.h" +#include "controller.capnp.h" + +/* build_host_eval_ctx is static — declare prototype for test access */ +static host_eval_ctx_t* build_host_eval_ctx(const xce_sched_worker_t* w, + int num_res); + +static void +test_build_host_eval_ctx_float(void) +{ + /* Build a worker with one float resource at res_idx=2 (value=4.0) */ + xce_sched_worker_t w; + memset(&w, 0, sizeof(w)); + zarray_init(&w.resvals, 8, NULL); + + xce_val_t v; + memset(&v, 0, sizeof(v)); + v.which = Value_u_fvalue; + v.u.fvalue = 4.0f; + xce_val_t* vp = &v; + + /* slot 0, 1 = NULL; slot 2 = &v */ + zarray_set(&w.resvals, 0, NULL); + zarray_set(&w.resvals, 1, NULL); + zarray_set(&w.resvals, 2, &vp); + + host_eval_ctx_t* ctx = build_host_eval_ctx(&w, 3); + TEST_ASSERT_NOT_NULL(ctx); + TEST_ASSERT_EQUAL_DOUBLE(4.0, ctx->numvals[2]); + TEST_ASSERT_NULL(ctx->strvals[2]); + + free((void*)ctx->numvals); + free((void*)ctx->strvals); + free(ctx); + zarray_fini(&w.resvals); +} +``` + +In `main`, add: `TEST(test_build_host_eval_ctx_float);` + +- [x] **Step 2: Run to verify it fails** + +```bash +nix develop --command bash -c "cd /workspace/src && ninja -C build scheduler/testsuite/sched_test 2>&1 | tail -20" +``` + +Expected: compile error — `build_host_eval_ctx` not yet defined. + +- [x] **Step 3: Implement build_host_eval_ctx in resource.c** + +In `src/scheduler/src/resource.c`, add `#include "controller.capnp.h"` to the include block, then add before `sched_meet_host_selection`: + +```c +/* Build a host_eval_ctx_t from a scheduler worker's resource-value zarray. + * Allocates numvals and strvals on the heap; caller frees all three. + * Returns NULL on OOM. */ +static host_eval_ctx_t* +build_host_eval_ctx(const xce_sched_worker_t* w, int num_res) +{ + host_eval_ctx_t* ctx = calloc(1, sizeof(*ctx)); + if (ctx == NULL) return NULL; + + double* numvals = calloc(num_res, sizeof(double)); + const char** strvals = calloc(num_res, sizeof(char*)); + if (numvals == NULL || strvals == NULL) { + free(numvals); + free(strvals); + free(ctx); + return NULL; + } + + for (int i = 0; i < num_res; i++) { + xce_val_t** slot = (xce_val_t**)zarray_get(&w->resvals, i); + if (slot == NULL || *slot == NULL) continue; + xce_val_t* v = *slot; + switch (v->which) { + case Value_u_fvalue: + numvals[i] = (double)v->u.fvalue; + break; + case Value_u_bvalue: + numvals[i] = v->u.bvalue ? 1.0 : 0.0; + break; + case Value_u_svalue: + strvals[i] = v->u.svalue; + break; + } + } + + ctx->num_res = num_res; + ctx->numvals = numvals; + ctx->strvals = strvals; + return ctx; +} + +static void +free_host_eval_ctx(host_eval_ctx_t* ctx) +{ + if (ctx == NULL) return; + free((void*)ctx->numvals); + free((void*)ctx->strvals); + free(ctx); +} +``` + +- [x] **Step 4: Check xce_val_t layout** + +```bash +grep -n 'xce_val_t\|fvalue\|svalue\|bvalue' src/include/xce/types.h | head -20 +grep -n 'Value_u_fvalue\|Value_u_svalue\|Value_u_bvalue' src/scheduler/src/resource.c || \ + grep -rn 'Value_u_fvalue' src/controller/ | head -3 +``` + +Verify the `u.fvalue`, `u.svalue`, `u.bvalue` field names match `xce_val_t` in `controller.capnp.h`. + +- [x] **Step 5: Build and run test** + +```bash +nix develop --command bash -c "cd /workspace/src && ninja -C build && build/scheduler/testsuite/sched_test -t test_build_host_eval_ctx_float" +``` + +Expected: PASS. + +- [x] **Step 6: Commit** + +```bash +git add src/scheduler/src/resource.c +git commit -m "feat: implement build_host_eval_ctx — project resvals zarray to host_eval_ctx_t (6-7)" +``` + +--- + +## Task 6: Implement sched_meet_host_selection (6-5) + +Replace the stub with `eval_select_simple`. This is the pragmatic Phase 6 approach (no session restructuring — deferred to a later phase). The `int` return value and `add_job_preason` call in the caller remain unchanged. + +**Files:** +- Modify: `src/scheduler/src/resource.c` + +- [x] **Step 1: Write the failing test** + +In `src/scheduler/testsuite/test.c`, add: + +```c +#include "kvmgr.h" + +static void +test_meet_host_selection_null_select_passes(void) +{ + /* A NULL select pointer means no expression — always pass */ + xce_scheduler_t sched; + memset(&sched, 0, sizeof(sched)); + xce_sched_worker_t w; + memset(&w, 0, sizeof(w)); + zarray_init(&w.resvals, 4, NULL); + + int rc = sched_meet_host_selection(&sched, &w, NULL); + TEST_ASSERT_EQUAL_INT(1, rc); + + zarray_fini(&w.resvals); +} + +static void +test_meet_host_selection_always_true_passes(void) +{ + xce_scheduler_t sched; + memset(&sched, 0, sizeof(sched)); + sel_optimize_set(1); + sel_node_table_init(); + + xce_sched_worker_t w; + memset(&w, 0, sizeof(w)); + zarray_init(&w.resvals, 4, NULL); + + /* SEL_TRUE root — must pass any host */ + xce_select_node_t* root = makeSelectNode(SEL_TRUE, 0, 0); + TEST_ASSERT_NOT_NULL(root); + + xce_select_t sel = { .root = root }; + int rc = sched_meet_host_selection(&sched, &w, &sel); + TEST_ASSERT_EQUAL_INT(1, rc); + + zarray_fini(&w.resvals); + gc_clear_marks(); + gc_sweep_select_nodes(NULL, 0); + sel_node_table_fini(); + sel_optimize_test_reset(); +} + +static void +test_meet_host_selection_always_false_fails(void) +{ + xce_scheduler_t sched; + memset(&sched, 0, sizeof(sched)); + sel_optimize_set(1); + sel_node_table_init(); + + xce_sched_worker_t w; + memset(&w, 0, sizeof(w)); + zarray_init(&w.resvals, 4, NULL); + + /* SEL_FALSE root — must fail every host */ + xce_select_node_t* root = makeSelectNode(SEL_FALSE, 0, 0); + TEST_ASSERT_NOT_NULL(root); + + xce_select_t sel = { .root = root }; + int rc = sched_meet_host_selection(&sched, &w, &sel); + TEST_ASSERT_EQUAL_INT(0, rc); + + zarray_fini(&w.resvals); + gc_clear_marks(); + gc_sweep_select_nodes(NULL, 0); + sel_node_table_fini(); + sel_optimize_test_reset(); +} +``` + +In `main`, register all three tests. + +- [x] **Step 2: Run to verify failures** + +```bash +nix develop --command bash -c "cd /workspace/src && ninja -C build scheduler/testsuite/sched_test 2>&1 | tail -20" +``` + +Expected: link errors (scheduler sources not yet in meson) OR `test_meet_host_selection_always_false_fails` FAIL (stub returns 1 for SEL_FALSE). + +Proceed to Step 3. + +- [x] **Step 3: Replace the stub in resource.c** + +In `src/scheduler/src/resource.c`, replace the `sched_meet_host_selection` function body (currently lines ~415-422): + +```c +int sched_meet_host_selection(xce_scheduler_t* sched, xce_sched_worker_t* w, + xce_select_t* s) +{ + if (s == NULL || s->root == NULL) return 1; + + /* Count resources: use resvals.caps as upper bound on res_idx */ + int num_res = (int)w->resvals.caps; + + host_eval_ctx_t* hctx = build_host_eval_ctx(w, num_res); + if (hctx == NULL) { + /* OOM: conservatively pass host — scheduler will reject on resource + * quantities anyway */ + return 1; + } + + int rc = eval_select_simple(s->root, hctx); + free_host_eval_ctx(hctx); + + /* eval_select_simple: 1=match, 0=no-match, <0=error (treat as no-match) */ + return (rc > 0) ? 1 : 0; +} +``` + +Add `#include "resreq.h"` to `resource.c` includes if not already present. + +- [x] **Step 4: Build and run** + +```bash +nix develop --command bash -c "cd /workspace/src && ninja -C build 2>&1 | grep -E 'error:|FAILED' | head -10" +``` + +Expected: builds cleanly. + +- [x] **Step 5: Commit** + +```bash +git add src/scheduler/src/resource.c +git commit -m "feat: implement sched_meet_host_selection using eval_select_simple (6-5)" +``` + +--- + +## Task 7: Drive GC from sched_end_session with live_jobs + atomic fence (6-8, 6-4, 6-12) + +`sched_end_session` collects all live jobs, performs the mark phase, inserts the seq_cst fence, then sweeps. The GC idle invariant (6-4) is enforced by the three predicate guards. + +**Files:** +- Modify: `src/scheduler/src/scheduler.c` + +- [x] **Step 1: Write the failing test** + +In `src/scheduler/testsuite/test.c`, add: + +```c +static void +test_gc_predicates_idle_invariant(void) +{ + /* session_active=0, eval_in_progress=0, staging_merge_complete=0 after + * memset — GC call site guard must NOT log_fatal. We can only test + * indirectly by calling sched_end_session on an empty scheduler. */ + xce_scheduler_t sched; + memset(&sched, 0, sizeof(sched)); + /* staging_merge_complete must be 1 for GC to proceed */ + atomic_store(&sched.staging_merge_complete, 1); + + sel_optimize_set(1); + sel_node_table_init(); + + /* sched_end_session with no jobs — should call gc with empty live_jobs */ + /* We can't call sched_end_session directly (it calls plugin_end_session + * which requires full init). Test the predicate reads directly. */ + TEST_ASSERT_EQUAL_INT(0, atomic_load(&sched.session_active)); + TEST_ASSERT_EQUAL_INT(0, atomic_load(&sched.eval_in_progress)); + TEST_ASSERT_EQUAL_INT(1, atomic_load(&sched.staging_merge_complete)); + + gc_clear_marks(); + gc_sweep_select_nodes(NULL, 0); + + sel_node_table_fini(); + sel_optimize_test_reset(); +} +``` + +In `main`, add: `TEST(test_gc_predicates_idle_invariant);` + +- [x] **Step 2: Run to verify** + +```bash +nix develop --command bash -c "cd /workspace/src && ninja -C build scheduler/testsuite/sched_test 2>&1 | tail -20" +``` + +- [x] **Step 3: Add live_jobs collection and GC call to sched_end_session** + +In `src/scheduler/src/scheduler.c`, replace `sched_end_session` (line ~752): + +```c +void sched_end_session(xce_scheduler_t* sched) +{ + plugin_end_session(sched); + + /* GC idle invariant guard — any violation is a use-after-free risk */ + if (sched_session_active(sched) || sched_eval_in_progress(sched) + || !sched_staging_merge_complete(sched)) { + log_fatal("sched_end_session: GC idle invariant violated: " + "session_active=%d eval_in_progress=%d staging_merge=%d", + sched_session_active(sched), + sched_eval_in_progress(sched), + sched_staging_merge_complete(sched)); + return; + } + + /* Collect live jobs into a flat array for GC mark phase */ + rtti_iter_t it; + xce_sched_job_t* job = NULL; + int njobs = 0; + int cap = 64; + xce_sched_job_t** live_jobs = calloc(cap, sizeof(*live_jobs)); + if (live_jobs == NULL) goto sweep; + + iter_init(&it); + lock(sched->jobs, RTTI_LOCK_READ); + while (iter_next(&it, sched->jobs, (void**)&job) == RTTI_ITER_OK) { + if (njobs >= cap) { + int newcap = cap * 2; + xce_sched_job_t** tmp = realloc(live_jobs, + newcap * sizeof(*live_jobs)); + if (tmp == NULL) { + unlock(sched->jobs); + iter_destroy(&it); + goto sweep_with_jobs; + } + live_jobs = tmp; + cap = newcap; + } + live_jobs[njobs++] = job; + } + unlock(sched->jobs); + iter_destroy(&it); + +sweep_with_jobs: + /* Mark phase: clear all gc_marks, then mark every select root reachable + * from live job resreq trees */ + gc_clear_marks(); + for (int i = 0; i < njobs; i++) { + xce_resreq_t* rr = live_jobs[i]->resreq; + if (rr == NULL) continue; + /* dispatch on resreq tag to reach all select roots */ + if (rr->tag == XCE_RESREQ_PRIMARY) { + if (rr->u.primary && rr->u.primary->select) + gc_mark_select_tree(rr->u.primary->select->root); + } else if (rr->tag == XCE_RESREQ_COMPOUND) { + if (rr->u.compound) { + xlist_iter_t li; + xce_primary_resreq_t* pr; + xlist_for_each(&rr->u.compound->primaries, &li, + (void**)&pr) { + if (pr && pr->select) + gc_mark_select_tree(pr->select->root); + } + } + } else if (rr->tag == XCE_RESREQ_ALTER) { + if (rr->u.alter) { + xlist_iter_t li; + xce_compound_resreq_t* cp; + xlist_for_each(&rr->u.alter->compounds, &li, + (void**)&cp) { + if (cp == NULL) continue; + xlist_iter_t li2; + xce_primary_resreq_t* pr; + xlist_for_each(&cp->primaries, &li2, (void**)&pr) { + if (pr && pr->select) + gc_mark_select_tree(pr->select->root); + } + } + } + } + } + + /* Fence: ensure all gc_mark writes are visible before the sweep reads + * them. Also ensures NULL-stores to unlinked roots (if any) are visible + * before gc_mark_select_tree reads root pointers. */ + atomic_thread_fence(memory_order_seq_cst); + +sweep: + gc_sweep_select_nodes(NULL, 0); + free(live_jobs); +} +``` + +Add `#include ` to `scheduler.c` includes, and ensure `#include "resreq.h"` is present. + +Check `xlist_for_each` macro — if it doesn't exist, use `xlist_iter_t` loop pattern from existing code in the file: + +```bash +grep -n 'xlist_for_each\|xlist_iter' src/scheduler/src/scheduler.c | head -5 +``` + +If `xlist_for_each` is not available, replace with the pattern used elsewhere in `scheduler.c`. + +- [x] **Step 4: Set/clear session_active and staging_merge_complete at session boundaries** + +In `sched_begin_session` (line ~740), at the START: + +```c + atomic_store_explicit(&sched->session_active, 1, memory_order_release); + atomic_store_explicit(&sched->staging_merge_complete, 0, memory_order_release); +``` + +In `sched_end_session`, at the END (after the GC sweep), add: + +```c + atomic_store_explicit(&sched->staging_merge_complete, 1, memory_order_release); +``` + +And in `sched_end_session` at the VERY START (before the guard), add: + +```c + atomic_store_explicit(&sched->session_active, 0, memory_order_release); +``` + +- [x] **Step 5: Build** + +```bash +nix develop --command bash -c "cd /workspace/src && ninja -C build 2>&1 | grep -E 'error:|FAILED' | head -20" +``` + +Expected: builds cleanly. + +- [x] **Step 6: Commit** + +```bash +git add src/scheduler/src/scheduler.c +git commit -m "feat: drive GC from sched_end_session — live_jobs mark, seq_cst fence, gc_sweep (6-8, 6-12)" +``` + +--- + +## Task 8: Add sel_alloc_thread_id thread-ownership checks (6-10) + +**Files:** +- Modify: `src/lib/xce_select.c` +- Modify: `src/include/resreq.h` +- Modify: `src/scheduler/src/scheduler.c` (register allocator thread) + +- [x] **Step 1: Write the failing test** + +In `src/lib/testsuite/test_select.c`, add: + +```c +static void +test_sel_register_alloc_thread(void) +{ + /* sel_alloc_thread_id is 0 (unregistered) before registration — + * thread checks must be skipped. After registration they must + * match pthread_self(). */ + sel_optimize_test_reset(); /* resets sel_alloc_thread_id to 0 */ + sel_optimize_set(1); + sel_node_table_init(); + + /* Should not crash when unregistered */ + xce_select_node_t* n = makeSelectNode(SEL_TRUE, 0, 0); + TEST_ASSERT_NOT_NULL(n); + + /* Register current thread */ + sel_register_alloc_thread(); + + /* makeSelectNode from same thread must still work */ + xce_select_node_t* n2 = makeSelectNode(SEL_FALSE, 0, 0); + TEST_ASSERT_NOT_NULL(n2); + + gc_clear_marks(); + gc_sweep_select_nodes(NULL, 0); + sel_node_table_fini(); + sel_optimize_test_reset(); +} +``` + +In `main`, add: `TEST(test_sel_register_alloc_thread);` + +- [x] **Step 2: Run to verify it fails** + +```bash +nix develop --command bash -c "cd /workspace/src && ninja -C build lib/testsuite/xce_test 2>&1 | tail -15" +``` + +Expected: compile error — `sel_register_alloc_thread` undeclared. + +- [x] **Step 3: Add sel_alloc_thread_id to xce_select.c** + +In `src/lib/xce_select.c`, add `#include ` to the include block. + +After the `sel_optimize_level` static variable declaration, add: + +```c +static pthread_t sel_alloc_thread_id; /* 0 = unregistered, skip checks */ +``` + +Add the registration function before `eval_select`: + +```c +void sel_register_alloc_thread(void) +{ + sel_alloc_thread_id = pthread_self(); +} +``` + +Extend `sel_optimize_test_reset` to also reset `sel_alloc_thread_id`: + +```c +void sel_optimize_test_reset(void) +{ + atomic_store_explicit(&sel_optimize_level, -1, memory_order_seq_cst); + sel_alloc_thread_id = 0; +} +``` + +- [x] **Step 4: Add thread checks in eval_select and gc_sweep_select_nodes** + +In `eval_select` (line ~633), after the `sel_get_optimize_level()` guard, add: + +```c + if (sel_alloc_thread_id != 0 && pthread_self() != sel_alloc_thread_id) { + ERROR("eval_select called from non-allocator thread"); + return -1; + } +``` + +In `gc_sweep_select_nodes` (line ~826), at the top after the `table == NULL` check, add: + +```c + if (sel_alloc_thread_id != 0 && pthread_self() != sel_alloc_thread_id) { + ERROR("gc_sweep_select_nodes called from non-allocator thread"); + return; + } +``` + +- [x] **Step 5: Declare sel_register_alloc_thread in resreq.h** + +In `src/include/resreq.h`, after `sel_optimize_set`/`sel_get_optimize_level`: + +```c +/* Register the current thread as the sole allocator thread. + * Must be called once before any eval_select or gc_sweep_select_nodes call. + * A value of 0 (default) disables the check (unit-test mode). */ +extern void sel_register_alloc_thread(void); +``` + +- [x] **Step 6: Wire registration in scheduler main loop** + +In `src/scheduler/src/main.c`, in the allocator thread's first `while(1)` iteration (line ~119, before `sched_begin_session`), add a one-time call: + +```c + /* Register this thread as the sole eval/GC thread (write-once) */ + static int sel_thread_registered = 0; + if (!sel_thread_registered) { + sel_register_alloc_thread(); + sel_thread_registered = 1; + } +``` + +Add `#include "resreq.h"` to `main.c` includes. + +- [x] **Step 7: Build and run** + +```bash +nix develop --command bash -c "cd /workspace/src && ninja -C build && build/lib/testsuite/xce_test -t test_sel_register_alloc_thread" +``` + +Expected: PASS. Run full lib suite: + +```bash +nix develop --command bash -c "cd /workspace/src && build/lib/testsuite/xce_test" +``` + +Expected: all tests PASS. + +- [x] **Step 8: Commit** + +```bash +git add src/lib/xce_select.c src/include/resreq.h src/scheduler/src/main.c +git commit -m "feat: add sel_alloc_thread_id ownership check in eval_select and gc_sweep (6-10)" +``` + +--- + +## Task 9: Scheduler testsuite — link resource.c + integration tests (6-11) + +**Files:** +- Modify: `src/scheduler/testsuite/meson.build` +- Modify: `src/scheduler/testsuite/test.c` + +- [x] **Step 1: Update meson.build to link scheduler source files** + +Replace `src/scheduler/testsuite/meson.build` with: + +```meson +scheduler_test_src = [ + 'test.c', + '../src/resource.c', +] + +scheduler_test_deps = [libxce_deps, librtti_dep, unity_dep] + +incdir = include_directories('../include', '../../include', '.') + +scheduler_test = executable('sched_test', + scheduler_test_src, + include_directories:[incdir], + dependencies: [scheduler_test_deps], + install: false) +``` + +Note: Only `resource.c` is needed — it implements the functions under test. `scheduler.c` (which calls `sched_init` and starts threads) is intentionally excluded to keep tests free of thread/network side effects. + +- [x] **Step 2: Write integration test with a known-good resreq/host pair** + +In `src/scheduler/testsuite/test.c`, add: + +```c +#include "xstring.h" + +static void +test_meet_host_selection_numeric_match(void) +{ + /* Job requires: ncpus >= 4. + * Worker has ncpus=8 at res_idx=0 → must match (rc=1). + * Worker has ncpus=2 at res_idx=0 → must not match (rc=0). */ + + sel_optimize_set(1); + xstring_table_init(64 * 1024); + sel_node_table_init(); + + /* Build resreq AST: ncpus >= 4 + * SEL_GE(SEL_RESNAME(res_idx=0, RES_TYPE_NUMERIC), SEL_NUM(4.0)) */ + xce_select_node_t* resname = makeSelectNode(SEL_RESNAME, 0, + RES_TYPE_NUMERIC); + xce_select_node_t* num4 = makeSelectNode(SEL_NUM, + (uintptr_t)(double)4.0, 0); + xce_select_node_t* ge = makeSelectNode(SEL_GE, + (uintptr_t)resname, + (uintptr_t)num4); + TEST_ASSERT_NOT_NULL(ge); + + xce_select_t sel = { .root = ge }; + + /* --- host with 8 cpus --- */ + xce_sched_worker_t w8; + memset(&w8, 0, sizeof(w8)); + zarray_init(&w8.resvals, 2, NULL); + + xce_val_t v8; + memset(&v8, 0, sizeof(v8)); + v8.which = Value_u_fvalue; + v8.u.fvalue = 8.0f; + xce_val_t* v8p = &v8; + zarray_set(&w8.resvals, 0, &v8p); + + xce_scheduler_t sched; + memset(&sched, 0, sizeof(sched)); + + TEST_ASSERT_EQUAL_INT(1, sched_meet_host_selection(&sched, &w8, &sel)); + + /* --- host with 2 cpus --- */ + xce_sched_worker_t w2; + memset(&w2, 0, sizeof(w2)); + zarray_init(&w2.resvals, 2, NULL); + + xce_val_t v2; + memset(&v2, 0, sizeof(v2)); + v2.which = Value_u_fvalue; + v2.u.fvalue = 2.0f; + xce_val_t* v2p = &v2; + zarray_set(&w2.resvals, 0, &v2p); + + TEST_ASSERT_EQUAL_INT(0, sched_meet_host_selection(&sched, &w2, &sel)); + + zarray_fini(&w8.resvals); + zarray_fini(&w2.resvals); + gc_clear_marks(); + gc_sweep_select_nodes(NULL, 0); + sel_node_table_fini(); + sel_optimize_test_reset(); +} + +static void +test_meet_host_selection_string_match(void) +{ + /* Job requires: hosttype == "gpu". + * Worker has hosttype="gpu" at res_idx=1 → must match. + * Worker has hosttype="cpu" at res_idx=1 → must not match. */ + + sel_optimize_set(1); + xstring_table_init(64 * 1024); + sel_node_table_init(); + + /* Build resreq AST: hosttype == "gpu" */ + xce_select_node_t* resname = makeSelectNode(SEL_RESNAME, 1, + RES_TYPE_STRING); + const char* gpu_str = xstring_get("gpu", 3); + xce_select_node_t* str_gpu = makeSelectNode(SEL_STR, + (uintptr_t)gpu_str, 0); + xce_select_node_t* eq = makeSelectNode(SEL_EQ, + (uintptr_t)resname, + (uintptr_t)str_gpu); + TEST_ASSERT_NOT_NULL(eq); + + xce_select_t sel = { .root = eq }; + + /* helper to build worker with string at res_idx=1 */ + xce_scheduler_t sched; + memset(&sched, 0, sizeof(sched)); + + xce_sched_worker_t wgpu; + memset(&wgpu, 0, sizeof(wgpu)); + zarray_init(&wgpu.resvals, 2, NULL); + + xce_val_t vgpu; + memset(&vgpu, 0, sizeof(vgpu)); + vgpu.which = Value_u_svalue; + vgpu.u.svalue = "gpu"; + xce_val_t* vgpup = &vgpu; + zarray_set(&wgpu.resvals, 1, &vgpup); + + TEST_ASSERT_EQUAL_INT(1, sched_meet_host_selection(&sched, &wgpu, &sel)); + + xce_sched_worker_t wcpu; + memset(&wcpu, 0, sizeof(wcpu)); + zarray_init(&wcpu.resvals, 2, NULL); + + xce_val_t vcpu; + memset(&vcpu, 0, sizeof(vcpu)); + vcpu.which = Value_u_svalue; + vcpu.u.svalue = "cpu"; + xce_val_t* vcpup = &vcpu; + zarray_set(&wcpu.resvals, 1, &vcpup); + + TEST_ASSERT_EQUAL_INT(0, sched_meet_host_selection(&sched, &wcpu, &sel)); + + zarray_fini(&wgpu.resvals); + zarray_fini(&wcpu.resvals); + gc_clear_marks(); + gc_sweep_select_nodes(NULL, 0); + sel_node_table_fini(); + sel_optimize_test_reset(); +} +``` + +In `main`, register: +```c + TEST(test_meet_host_selection_null_select_passes); + TEST(test_meet_host_selection_always_true_passes); + TEST(test_meet_host_selection_always_false_fails); + TEST(test_build_host_eval_ctx_float); + TEST(test_meet_host_selection_numeric_match); + TEST(test_meet_host_selection_string_match); + TEST(test_scheduler_t_has_atomic_gc_fields); + TEST(test_gc_predicates_idle_invariant); + TEST(test_sel_optimize_level_set_at_init); +``` + +- [x] **Step 3: Check xce_val_t field names** + +```bash +grep -n 'svalue\|fvalue\|bvalue\|xce_val_t' src/include/xce/types.h | head -20 +``` + +Adjust `v.u.fvalue`, `v.u.svalue`, `v.u.bvalue` in the tests if the generated capnp struct uses different field names. + +- [x] **Step 4: Build and run all scheduler tests** + +```bash +nix develop --command bash -c "cd /workspace/src && ninja -C build && build/scheduler/testsuite/sched_test" +``` + +Expected: all tests PASS. + +- [x] **Step 5: Run full build + lib testsuite to verify no regressions** + +```bash +nix develop --command bash -c "cd /workspace/src && ninja -C build && build/lib/testsuite/xce_test" +``` + +Expected: all tests PASS. + +- [x] **Step 6: Commit** + +```bash +git add src/scheduler/testsuite/meson.build src/scheduler/testsuite/test.c +git commit -m "test: scheduler integration tests for sched_meet_host_selection (6-11)" +``` + +--- + +## Self-Review Checklist + +| Spec item | Task | +|-----------|------| +| 6-1 pre-audit | Task 1 | +| 6-2 blocking gate grep | Task 1 | +| 6-3 _Atomic fields | Task 3 | +| 6-4 GC predicate wrappers (real scheduler) | Task 4 | +| 6-5 implement sched_meet_host_selection | Task 6 | +| 6-6 (deferred — void return + session restructure) | — | +| 6-7 build_host_eval_ctx | Task 5 | +| 6-8 live_jobs flat array + mark phase | Task 7 | +| 6-9 sel_optimize_set(1) | Task 4 | +| 6-10 sel_alloc_thread_id | Task 8 | +| 6-11 integration tests | Task 9 | +| 6-12 atomic_thread_fence | Task 7 | + +### Post-review fixes (final code review, 2026-04-30) + +| # | Severity | Finding | Fix | Commit | +|---|----------|---------|-----|--------| +| R1 | **CRITICAL** | `staging_merge_complete` bootstrap deadlock — `sched_begin_session` reset it to 0 but never set it back to 1; `sched_end_session`'s guard `(!staging_merge_complete)` always fired → GC never ran, select-node memory leaked indefinitely | Added `atomic_store_explicit(&sched->staging_merge_complete, 1, memory_order_release)` at end of `sched_begin_session` after `sched_account_jobs` | `d438742` | +| R2 | **HIGH** | `recompact_node_ids()` missing from GC protocol in `sched_end_session` — without recompaction `sel_node_next_id` grows monotonically; `select_eval_ctx_t` arrays (sized by max node_id) grow without bound across GC cycles | Added `recompact_node_ids()` call after `gc_sweep_select_nodes(NULL, 0)` | `d438742` | + +### Known deferred items + +- **6-6** (outer-host/inner-job session restructuring + `void` return): The current implementation uses `eval_select_simple` which re-evaluates the expression for every (host, job) call independently. The design doc describes a future optimisation where a single `select_eval_ctx_t` is reused per host across all jobs in a session. Deferred to a follow-on plan. + +- **`eval_in_progress` guard**: The `sched_eval_in_progress` check in `sched_end_session` is always false (field never set to 1). Correct under the current single-threaded model; wiring is deferred to when concurrent eval is introduced. diff --git a/src/docs/resreq-ast-cse-design.md b/src/docs/resreq-ast-cse-design.md index 8d72d4d6..0d8964b7 100644 --- a/src/docs/resreq-ast-cse-design.md +++ b/src/docs/resreq-ast-cse-design.md @@ -1,7 +1,8 @@ # XCE Resource Requirement AST & CSE Design > **Status**: Draft -> **Last Updated**: 2026-04-21 +> **Last Updated**: 2026-04-27 +> **Revision**: 6 (multi-round review: resmgr API, atomic memory order, sel_make_key op2, makeString TOCTOU) > **Target**: `src/lib/` (C shared library), `src/include/resreq.h` --- @@ -16,7 +17,7 @@ 6. [Commutativity Canonicalization](#6-commutativity-canonicalization) 7. [Parse-Time Type Validation](#7-parse-time-type-validation) 8. [Evaluation in Host Context](#8-evaluation-in-host-context) -9. [Job-Level Sharing](#9-job-level-sharing) +9. [Job-Level Sharing](#9-job-level-sharing) *(§9.2–§9.3 are future work, not in current scope)* 10. [Scheduling Session Semantics](#10-scheduling-session-semantics) 11. [Implementation Plan](#11-implementation-plan) 12. [Appendix: Data Structures](#12-appendix-data-structures) @@ -37,11 +38,19 @@ xce_resreq_t (alter / compound / primary) └─ xce_affinity_t ← EMPTY STUB ``` -`section.peg` splits the raw resreq string into clause sub-strings -(`selectString`, `rusageString`, `spanString`, …). `rusage.peg` fully -parses the rusage clause. The select clause is extracted but never parsed -— `EvalSelectForHost` in the Go backend is a no-CGO stub that always -returns `errNoCGO`. +`section.peg` splits the raw resreq string into clause sub-strings. +The `select` clause may appear in two forms: explicit (`select[expr]`) +or implicit (bare expression before any other clause keyword). The +parser extracts `selectString`, `rusageString`, `spanString`, etc. +`rusage.peg` fully parses the rusage clause. The select clause is +extracted but never parsed — `EvalSelectForHost` in the Go backend is a +no-CGO stub that always returns `errNoCGO`. Phase 6 of this design +wires evaluation directly into the C scheduler; the Go stub is not +replaced — the Go API server uses the unoptimized parse path +(`sel_optimize_level=0`, §5.0). The API server may also call +`eval_select_simple` for client-side host-filtering queries (§5.0), but +at far lower frequency than the scheduler. The primary host-matching +evaluation loop runs in the C scheduler (`sel_optimize_level=1`). This document describes the design to: @@ -57,15 +66,26 @@ This document describes the design to: array, all elements of the array share the same resource requirement string, so CSE allows a single interned AST to represent thousands of jobs. In a busy EDA cluster (10,000 nodes, 10M jobs, 1M/hour -throughput), the number of unique EDA tool configurations is limited -(< 100), so many jobs share the same select expression. Node-level CSE -provides structural sharing even when full resreq strings differ (e.g., -same `select[...]` but different `rusage[...]`), which string-level -interning alone cannot achieve. + throughput), the number of unique EDA tool configurations is limited + (< 100), so many jobs share the same select expression. Node-level CSE + provides structural sharing even when full resreq strings differ (e.g., + same `select[...]` but different `rusage[...]`), which string-level + interning alone cannot achieve. +- **Evaluate each unique AST node at most once per host per scheduling + session** — the core performance guarantee of this design. Because CSE + ensures all structurally identical sub-expressions resolve to a single + shared `xce_select_node_t*`, a per-host memoization bitmap (`bool_bits` + in §8.5) can record whether a node has already been evaluated for the + current host. In a session with 10,000 jobs all sharing the node + `type==linux`, that node is evaluated at most **once** per host per + evaluation path that reaches it — AND/OR short-circuit may skip nodes + in un-taken branches, but any node that IS reached is evaluated at most + once. Without this guarantee, evaluation cost would scale with + jobs × hosts rather than with unique_nodes × hosts. - Evaluate that AST efficiently in the context of a host's live resource values. - Run a **mark-and-sweep GC** between scheduling sessions to recycle - interned nodes that are no longer reachable from any live job (§5.6). + interned nodes that are no longer reachable from any live job (§5.5). --- @@ -82,15 +102,55 @@ These are advanced features; their design and implementation are separate future work. The CSE and intern table technique described here is applicable to them but should not be considered final for those use cases. +**Scope of this document:** The `select` clause grammar, AST, CSE, and evaluation are in scope. The `rusage`, `span`, `same`, and `affinity` clauses are out of scope; they are listed above for context only. + +**In-scope implementation requirements for scheduler integration (Phase 6):** +- GC idle invariant predicates (`sched_session_active`, `sched_eval_in_progress`, + `staging_merge_complete`) — required to enforce the §5.5 runtime guard that + prevents use-after-free when GC runs. These are tested as stubs in Phase 1 + and wired to real `xce_scheduler_t` fields in Phase 6 (see Phase 6 for + the stub-to-production migration contract). +- live_jobs materialization — iterating `sched->jobs` to produce the flat + `const xce_sched_job_t**` array required by `gc_sweep_select_nodes`. + This is a Phase 6 deliverable, not a standalone library feature. + +**Note on implicit select syntax:** The implicit select form (bare expression before any other clause keyword, §3.1) requires the select parser to recognize `rusage`, `span`, `same`, and `affinity` as clause-boundary tokens in order to know where the select expression ends. Recognizing these keyword boundaries is in scope; implementing the other clauses is not. + The resreq string grammar (outer structure, already implemented): ``` resreq ::= alter_resreq | compound_resreq | primary_resreq alter_resreq ::= '{' compound '}' ( '||' '{' compound '}' )* compound_resreq ::= n '*' '{' primary '}' ( '+' n '*' '{' primary '}' )* -primary_resreq ::= select[...] rusage[...] span[...] same[...] affinity[...] +primary_resreq ::= select_clause rusage_clause span_clause same_clause affinity_clause +select_clause ::= 'select' '[' select_expr ']' | select_expr (see §3) +rusage_clause ::= 'rusage' '[' rusage_list ']' | ε +span_clause ::= 'span' '[' span_spec ']' | ε +same_clause ::= 'same' '[' same_spec ']' | ε +affinity_clause ::= 'affinity' '[' affinity_spec ']' | ε ``` +**Clause keyword semantics when empty (ε):** + +| Clause | Meaning when absent | Equivalent | +|--------|-------------------|------------| +| `select_clause` | Match all hosts | `type == any` → `SEL_TRUE` | +| `rusage_clause` | No per-job resource consumption; scheduler checks only user/queue/project limits | — | +| `span_clause` | Skip span constraint handling | — | +| `same_clause` | Skip same-node co-allocation constraint | — | +| `affinity_clause` | Skip hardware topology affinity constraint | — | + +**Implicit select (bracket-free):** If the first token of a `primary_resreq` is not one of the clause keywords (`select`, `rusage`, `span`, `same`, `affinity`), the parser treats it as the start of a `select_expr`. This means the `select` keyword and brackets can be omitted when select is the first (and often only) clause: + +``` +type==linux && gpu ← implicit select (no keyword, no brackets) +select[type==linux && gpu] ← explicit select (keyword + brackets) +rusage[mem=100] ← no select clause, match all hosts +type==linux rusage[mem=100] ← implicit select + rusage +``` + +The implicit form is the common case in LSF-compatible workflows. The explicit bracketed form (`select[...]`) is kept for compatibility and for cases where the expression starts with a clause keyword name. For example, if a resource is named `rusage`, writing `rusage > 5` unquoted would be parsed as the start of a `rusage[...]` clause — use `select[rusage > 5]` instead. + --- ## 3. Select Expression AST @@ -105,13 +165,73 @@ select_not ::= '!' select_primary | select_primary select_primary ::= '(' select_or ')' | comparison | resname comparison ::= resname cmp_op value cmp_op ::= '>=' | '<=' | '!=' | '==' | '>' | '<' -resname ::= [a-zA-Z_][a-zA-Z_0-9]* +resname ::= [a-zA-Z_][a-zA-Z_0-9]* (excluding 'any') value ::= string_literal | number | ANY ANY ::= 'any' number ::= [0-9]+ ('.' [0-9]*)? string_literal ::= [a-zA-Z_][a-zA-Z_0-9]* (unquoted word, excluding 'any') ``` +**Resource name / clause keyword conflict (implementation requirement):** +The `resname` production matches any identifier except `any`. This means +a resource named `rusage`, `span`, `same`, `affinity`, or `select` is +syntactically valid in the inner `xce_select.peg` grammar, but causes +ambiguity in the outer `section.peg` implicit-select boundary detection +(see §2 implicit select discussion). The document at §2 explains the +workaround (use explicit `select[...]` form), but this relies on users +knowing the constraint. + +**Runtime enforcement required (Phase 4b `resname` PEG action):** In the +`resname` action of `xce_select.peg`, after the identifier is matched, +add a validation check before calling `makeSelectNode`: +```c +/* In the resname action — after identifier capture: */ +static const char* clause_keywords[] = { + "select", "rusage", "span", "same", "affinity", NULL +}; +for (int i = 0; clause_keywords[i]; i++) { + if (strcmp(ident, clause_keywords[i]) == 0) { + snprintf(auxil->errmsg, ERRMSG_MAX, + "resource name '%s' conflicts with a clause keyword — " + "use explicit select[%s ...] form if this is intentional", + ident, ident); + return -1; /* parse failure */ + } +} +``` +This gives a clear error at parse time rather than silently misparsing. +`any` is already excluded by the grammar rule; the clause keywords are +not and must be checked explicitly in the action. + +**No `true`/`false` keywords.** The words `true` and `false` are **not** +reserved and are **not** part of the select grammar. Boolean resources +use the standalone idiom only: + +- `gpu` — the resource's value IS the boolean (idiomatic) +- `!gpu` — negation of a boolean resource + +Writing `gpu == true` or `gpu == false` is a **type mismatch parse error**: +`true`/`false` are not recognized as keywords, so they are parsed as +`SEL_STR` string literals. Since `gpu` is a `RES_TYPE_BOOL` resource, +comparing it to a string literal (`SEL_STR`) fails the type-compatibility +check in the `==` action (§7). This produces a clear error message at +job submission time rather than silently const-folding as in the previous +design. + +**Why remove `true`/`false`:** The previous design parsed `true`/`false` +as keywords and const-folded `bool_res == true` → `bool_res`. This was +confusing for two reasons: (1) it encouraged a non-idiomatic form that +only works for boolean resources — writing `type == true` for a string +resource would const-fold incorrectly; (2) the const-fold made `gpu == +true` and `gpu` behave identically, obscuring whether the user understood +boolean resource semantics. Rejecting `gpu == true` with a type error +forces the idiomatic `gpu` form and makes the type system visible to +users. + +`SEL_TRUE` and `SEL_FALSE` still exist as **internal node types** produced +by const-folding (e.g., `type == any` → `SEL_TRUE`), but they are never +produced directly from user input — only by transform passes. + **LSF-compatible disambiguation:** `resname` and `string_literal` share the same lexical pattern. The parser resolves the ambiguity using the resource manager: if a bare word matches a known resource name, it is @@ -127,38 +247,22 @@ only valid as the right-hand side of `==` or `!=` (e.g., `type == any`, The const-fold pass (§6.3) reduces `resname == any` to `SEL_TRUE`, so `eval_select` never encounters `SEL_ANY` at runtime. -**Note on `!= any`:** `resname != any` is a parse error. However, -`!(resname == any)` const-folds to `!SEL_TRUE` which reduces to -`SEL_FALSE` — these are semantically equivalent. This inconsistency -exists because the parse-time rejection of `!= any` provides a clearer -error message for users, while the const-fold path handles the negated -form implicitly. If this inconsistency proves confusing in practice, -a future change can allow `resname != any` at parse time and const-fold -it to `SEL_FALSE`. - -**Boolean keywords `true` and `false`:** These are parsed directly as -`SEL_TRUE` and `SEL_FALSE` singleton nodes — there is no intermediate -node type. They are not part of the `value` production (the RHS of -comparisons) because boolean resources should be used as standalone -operands, not compared to `true`/`false`. The idiomatic form is: - -- `gpu` (standalone boolean resource — the resource's value IS the boolean) -- **Not** `gpu == true` — though the parser accepts this, const-folding - reduces `bool_res == SEL_TRUE` → `SEL_RESNAME(bool_res)` and - `bool_res == SEL_FALSE` → `SEL_NOT(SEL_RESNAME(bool_res))` - -Since const-folding always eliminates these comparisons, a dedicated -`SEL_BOOL_LITERAL` type would never survive to runtime. Parsing -`true` → `SEL_TRUE` and `false` → `SEL_FALSE` directly avoids an -intermediate type. `true` and `false` are reserved keywords excluded -from the `string_literal` pattern. +**`!= any` is valid and const-folds to `SEL_FALSE`:** Both +`resname != any` and `!(resname == any)` are accepted at parse time and +produce identical results. `resname == any` const-folds to `SEL_TRUE` +(§6.3); `resname != any` const-folds directly to `SEL_FALSE` (§6.3); +`!(resname == any)` becomes `!SEL_TRUE` → `SEL_FALSE` via the complement +law. All three paths yield the same interned singleton `SEL_FALSE` — +no host matches, since `any` matches all resource values. Accepting +`!= any` explicitly removes a surprising asymmetry and avoids user +confusion. **String literals in comparisons:** Unquoted words that are not known -resource names and not reserved keywords (`any`, `true`, `false`) are -treated as string literals. For example, `type == x86_64` — `type` -resolves to `SEL_RESNAME` (known resource), `x86_64` resolves to -`SEL_STR` (not a known resource name). String values in comparisons -do not need quotes — LSF select expressions use unquoted words. +resource names and not the reserved keyword `any` are treated as string +literals. For example, `type == x86_64` — `type` resolves to +`SEL_RESNAME` (known resource), `x86_64` resolves to `SEL_STR` (not a +known resource name). String values in comparisons do not need quotes +— LSF select expressions use unquoted words. ### 3.2 Node Types @@ -176,9 +280,11 @@ typedef enum { SEL_RESNAME, /* leaf: resource variable */ SEL_NUM, /* leaf: numeric literal */ SEL_STR, /* leaf: string literal */ - SEL_ANY, /* leaf: any keyword (const-folded to SEL_TRUE) */ - SEL_TRUE, /* leaf: constant true — also produced by keyword `true` */ - SEL_FALSE, /* leaf: constant false — also produced by keyword `false` */ + SEL_ANY, /* leaf: any keyword — always eliminated by const-fold: + * resname == any → SEL_TRUE + * resname != any → SEL_FALSE (see §6.3) */ + SEL_TRUE, /* leaf: constant true — produced by const-fold only, not a keyword */ + SEL_FALSE, /* leaf: constant false — produced by const-fold only, not a keyword */ } sel_node_type_t; ``` @@ -232,12 +338,19 @@ Operator validity by resource type: | Operator | Numeric | String | Boolean | |----------|:-------:|:------:|:-------:| | `>` `<` `>=` `<=` | ✓ | parse error | parse error | -| `==` `!=` | ✓ | ✓ | ✓ | +| `==` `!=` | ✓ | ✓ | ✓ (vs numeric literal, string literal, or boolean;¹ `true`/`false` not keywords) | | standalone (no op) | ✓ (non-zero) | parse error | ✓ | -The `res_type` field is stored in the `SEL_RESNAME` node for eval-time -dispatch. It is **not** part of the CSE hash key — `res_idx` alone -identifies the resource uniquely, and a resource has exactly one type. +¹ Boolean resources in `==`/`!=` comparisons: the RHS can be a boolean + resource (e.g., `gpu == gpu2`: BOOL vs BOOL) or a numeric literal + (e.g., `gpu == 1`, `gpu != 0`: BOOL vs NUMERIC). String literals + cause a type mismatch error (e.g., `gpu == true` is rejected: BOOL vs + STRING, since `true`/`false` are not keywords and parse as strings). + **Migration diagnostic:** When `true` or `false` appears as an identifier + (parsed as SEL_RESNAME with no matching resource), the parser should emit a + helpful error: `"unknown resource 'true' — did you mean the boolean resource + name directly (e.g. 'gpu') or a numeric literal (1/0)?"`. This aids + migration from IBM LSF where `true`/`false` are accepted boolean literals. **Note:** `res_type_t` maps directly to the existing `XCE_VAL_*` constants in `src/include/xce/types.h` (`XCE_VAL_STRING=0`, `XCE_VAL_NUMERIC=1`, @@ -245,7 +358,12 @@ in `src/include/xce/types.h` (`XCE_VAL_STRING=0`, `XCE_VAL_NUMERIC=1`, mapping is required: ```c -/* res_type_t → existing valtype mapping */ +/* res_type_t → existing valtype mapping. + * NOTE: This function has no consumer in the core eval path and is + * intentionally omitted from the Phase 2 build — see Phase 2 scope decision + * ("res_type_to_valtype has no current consumer … omit it to avoid dead code"). + * It is documented here for reference only; do not compile it into the + * production build without a concrete consumer. */ static inline int res_type_to_valtype(res_type_t t) { static const int map[] = { [RES_TYPE_NUMERIC] = XCE_VAL_NUMERIC, /* 0 → 1 */ @@ -255,13 +373,21 @@ static inline int res_type_to_valtype(res_type_t t) { return map[t]; } -/* existing valtype → res_type_t inverse mapping */ +/* valtype → res_type_t inverse mapping. + * Declared as an API helper in Phase 2; used in error-reporting paths + * that need to describe mismatched operand types back to the caller. + * No direct caller in the core eval path — eval works with res_type + * cached at parse time in each node. */ static inline res_type_t valtype_to_res_type(int v) { switch (v) { case XCE_VAL_NUMERIC: return RES_TYPE_NUMERIC; case XCE_VAL_STRING: return RES_TYPE_STRING; case XCE_VAL_BOOLEAN: return RES_TYPE_BOOL; - default: return RES_TYPE_NUMERIC; /* safe default */ + default: + /* Unknown valtype — should never occur. Use log_fatal (not assert) + * so the invariant is enforced in production builds too (§5.5). */ + log_fatal("unknown valtype %d in valtype_to_res_type", v); + return RES_TYPE_NUMERIC; /* unreachable — log_fatal terminates */ } } ``` @@ -271,6 +397,35 @@ resmgr by name, retrieve its `valtype` (using `XCE_VAL_*`), and map it to `res_type_t` via the inverse mapping before storing it in the SEL_RESNAME node. +**res_idx validity guard:** `key_find(resmgr, name)` returns a non-negative +index on success, or a negative sentinel (e.g., `-ENOENT`) on failure. +Before calling `makeSelectNode(SEL_RESNAME, ...)`, the action **must** check +that the returned index is non-negative: + +```c +/* PEG action for a bare word that resolves to a known resource name. + * Uses the real resmgr API: key_find returns 1 on hit (r != NULL), + * -ENOENT on miss — matching the pattern in resource.c (acct->residx = r->idx). */ +xce_resource_t* r = NULL; +int rc = key_find(auxil->resmgr, (void*)name, (void**)&r); +/* GUARD: key_find returns 1 on hit; anything <= 0 means unknown resource. + * Passing a non-negative res_idx is mandatory before calling makeSelectNode: + * a negative value cast to uintptr_t produces UINTPTR_MAX (or similar large + * value), which would intern a SEL_RESNAME node with a permanently wrong + * node_id, causing false CSE cache hits and incorrect eval results. + * Unknown resource names are a parse error — see §7. */ +if (rc <= 0 || r == NULL) { auxil->has_error = 1; $$ = NULL; } +else { + int res_idx = r->idx; + res_type_t res_type = valtype_to_res_type(r->valtype); + $$ = (char*)makeSelectNode(SEL_RESNAME, (uintptr_t)res_idx, + (uintptr_t)res_type); +} +``` + +This guard matches the stated invariant in §7: "Unknown resource names fail +the parse (rejected at job submission time)." + **String resources in host values:** `xvec_idval_t` stores only numeric resource values (float pairs). String host resources (e.g., `type`, `arch`) are resolved differently — the host's string properties are looked @@ -282,6 +437,185 @@ values in `xvec_idval_t`. See §8 for the accessor functions. ## 5. CSE via Hash-Consing +### 5.0 Optimization Level (sel_optimize_level) + +CSE (hash-consing via the intern table) and related optimizations are +controlled by a **process-lifetime tri-state flag** set once at process +startup and never changed at runtime: + +```c +static _Atomic int sel_optimize_level = -1; +/* -1 = uninitialized (default; write-once enforcement uses this as sentinel) + * 0 = optimization disabled (API server mode: plain tree, no intern table) + * >0 = CSE + const-fold enabled (scheduler mode: intern table + memoization) + * + * The sign encodes the initialization state — a separate lock variable is not + * needed. Set exactly once via sel_optimize_set() before any parse_resreq or + * makeSelectNode call. Never read or write directly; use the accessor below. */ +``` + +**Write-once enforcement via CAS on the sentinel:** The initial value of -1 +is the uninitialized sentinel. `sel_optimize_set(level)` atomically +transitions from -1 to `level` with a single compare-exchange: + +```c +void sel_optimize_set(int level) { + if (level < 0) { + log_fatal("sel_optimize_set: level must be >= 0 (got %d)", level); + return; + } + int expected = -1; + if (!atomic_compare_exchange_strong_explicit( + &sel_optimize_level, &expected, level, + memory_order_release, memory_order_acquire)) { + /* CAS failed — already initialized */ + int current = atomic_load_explicit(&sel_optimize_level, + memory_order_acquire); + if (current != level) + log_fatal("sel_optimize_level cannot be changed after first use " + "(current=%d, attempted=%d)", current, level); + /* else: idempotent re-set to the same value — tolerated for test convenience */ + } +} + +static inline int sel_get_optimize_level(void) { + return atomic_load_explicit(&sel_optimize_level, memory_order_acquire); +} +``` + +The scheduler calls `sel_optimize_set(1)` at startup; the API server calls +`sel_optimize_set(0)`. Neither assigns `sel_optimize_level` directly. +Unit tests that need the disabled path call `sel_optimize_set(0)` or rely on +the -1 sentinel (makeSelectNode treats both < 0 and == 0 as non-CSE paths; +a guard at the top of makeSelectNode logs_fatal if level is still -1 at +call time, catching unconfigured processes early). + +**Test setup for CSE-enabled path:** Unit tests that exercise CSE interning +(`sel_optimize_level > 0`) must call `sel_optimize_set(1)` **at test setup**, +before any `makeSelectNode` call. Each test that needs a fresh CSE state must +call `sel_node_table_fini()` and then reset `sel_optimize_level` to -1 (via +`atomic_store_explicit(&sel_optimize_level, -1, memory_order_seq_cst)`) in +a teardown step, then call `sel_optimize_set(1)` again in the next test's +setup. A shared `sel_optimize_test_reset()` helper (in a test-only header, +guarded by `#ifdef UNIT_TEST`) should encapsulate this teardown — including +the `atomic_store_explicit` — to prevent copy-paste in individual test files. +All test-spawned threads that call `makeSelectNode` must be joined before +`sel_optimize_test_reset()` is called, to avoid a concurrent CAS racing +against the reset. + +| Process | Level | Rationale | +|---------|-------|-----------| +| **Scheduler** | `sel_optimize_set(1)` | Evaluates thousands of jobs per session; CSE eliminates redundant node allocation and enables evaluate-once-per-host memoization (§8.5). | +| **API server** | `sel_optimize_set(0)` | Parses resreq for validation and optionally evaluates select expressions for client-side host-filtering queries (much less frequently than the scheduler — see below). No intern table is created; `eval_select_simple()` handles host evaluation without memoization overhead. | + +**API server host evaluation (unoptimized):** The API server may evaluate +select expressions against host data for client-side host-filtering queries +(e.g., a REST endpoint returning matching hosts for a given resreq, or a +dry-run submission check). This happens far less frequently than the +scheduler's per-session evaluation, so memoization is not needed. The API +server uses `eval_select_simple(root, host)` — a plain recursive tree-walk +that takes no `select_eval_ctx_t` and performs no caching: + +```c +/* Simple (non-memoized) tree-walk evaluator for sel_optimize_level == 0. + * Returns 1 (match), 0 (no match), -1 (error). Thread-safe — no shared state. */ +int eval_select_simple(const xce_select_node_t* root, + const host_eval_ctx_t* host); +``` + +`eval_select_simple` is declared in `include/resreq.h` and shares the +`host_eval_ctx_t` definition with the scheduler. It does not use `node_id` +(all nodes have `node_id=0` in unoptimized mode — no collision risk because +no shared bitmap exists). + +When `sel_optimize_level == 0`, `makeSelectNode` allocates a fresh node on +every call (no intern table lookup or insertion), `gc_sweep_select_nodes` +and `recompact_node_ids` are no-ops, and `sel_node_table_init` skips +kvtab allocation. The `node_id` field is intentionally left as 0 (calloc +zero-initialization) — it has no meaning without an intern table. +`select_eval_ctx_t` is never constructed in this mode. + +**`eval_select` guard (optimized path only):** The memoized `eval_select` +requires an initialized intern table and valid `node_id` values. It must +reject calls in unoptimized mode: + +```c +/* At the top of eval_select / select_eval_ctx_init */ +if (sel_get_optimize_level() <= 0) { + log_fatal("eval_select called without optimization — use eval_select_simple"); + return -1; +} +``` + +Phase 4e must add this guard. A corresponding unit test verifies that +`eval_select` is never reached when `sel_get_optimize_level() <= 0`. + +**Memory management in unoptimized mode:** Since nodes are not tracked in the +intern table, the caller (parse layer) is responsible for freeing the AST +when it is no longer needed. `kill_primary_resreq` must call a recursive +`free_select_tree(root)` helper (not needed in enabled mode where GC handles +reclamation) to walk and free each node, then free the `xce_select_t` +container struct itself. For SEL_STR nodes, call +`dereference(node->u.strval)` before `free(node)`, as all string values are +xstring-interned via `makeString()`. **Conditional cleanup contract:** +When `sel_get_optimize_level() == 0`, `kill_primary_resreq` must call +`free_select_tree(primary->select->root)` then `FREEUP(primary->select)`. +When `sel_get_optimize_level() > 0`, `kill_primary_resreq` must only null the +root pointer (`primary->select->root = NULL`) — GC between sessions +handles node reclamation. A debug assertion in `free_select_tree` should +verify `sel_get_optimize_level() <= 0` to prevent accidental use in the wrong mode. + +```c +/* Free a select AST tree (unoptimized path only). + * When sel_optimize_level > 0, GC reclaims nodes — do NOT call this. + * Walks the tree post-order: free children before parent. */ +static void free_select_tree(xce_select_node_t* node) { + if (sel_get_optimize_level() > 0) { + log_fatal("free_select_tree called in CSE-enabled mode — use GC for node reclamation"); + return; + } + if (node == NULL) return; + switch (node->type) { + case SEL_AND: case SEL_OR: + case SEL_GT: case SEL_LT: case SEL_GE: case SEL_LE: + case SEL_EQ: case SEL_NE: + free_select_tree(node->u.binop.left); + free_select_tree(node->u.binop.right); + break; + case SEL_NOT: + free_select_tree(node->u.operand); + break; + case SEL_STR: + dereference(node->u.strval); /* release xstring refcount */ + break; + default: + break; /* SEL_RESNAME, SEL_NUM, SEL_ANY, SEL_TRUE, SEL_FALSE: no extra allocs */ + } + free(node); +} +/* STACK DEPTH NOTE: free_select_tree has the same recursion structure as + * eval_select_inner. Depth is bounded by SELECT_MAX_DEPTH (64) because + * parse_select enforces that limit. Stack usage per frame is comparable to + * eval_select_inner (~80-120 B), so the same 8 KB analysis applies — well + * within the 8 MB default Linux thread stack. Any increase to SELECT_MAX_DEPTH + * must revisit this analysis for BOTH eval_select_inner AND free_select_tree. */ +``` + +**Unoptimized ownership model (single rule):** When `sel_optimize_level == 0`, every allocated `xce_select_node_t` has exactly one owner at all times. Initially the PEG action that calls `makeSelectNode` owns the fresh allocation. Ownership transfers via two paths: (1) the node becomes a child of a composite node (parent owns it), or (2) the flatten pass (§6.2) replaces it — the replacing node becomes the new child and the original becomes an orphan that the flatten pass must free immediately (see below). At no point is a node both interned (tracked by the table) and individually owned — the two modes are mutually exclusive. `free_select_tree` walks from root and frees every reachable node; the flatten pass must free every node it replaces *before* returning, ensuring `free_select_tree` never encounters a freed node and never leaks an unreachable one. **Precondition:** `free_select_tree` must only be called after `sel_optimize_set(0)` has been called — calling it with `sel_optimize_level == -1` (uninitialized) is undefined behavior. A startup guard in `makeSelectNode` (`log_fatal` if level == -1) ensures this precondition is met before any tree is constructed. + +**Flatten-pass orphan handling in unoptimized mode:** The flatten pass (§6.2) replaces sub-trees via `makeSelectNode`, obtaining a new node and discarding the old one. When `sel_optimize_level > 0` this is safe — the intern table tracks all nodes and GC reclaims them. When `sel_optimize_level == 0`, the flatten pass must explicitly free every replaced (orphaned) node, because no intern table tracks them and `free_select_tree` only walks the final root. Implementation contract: `flatten_pass` must maintain a list of nodes it replaces (e.g. a stack-local `zarray_t`), and after re-interning the parent it must call `free(old_node)` (with `dereference(old_node->u.strval)` first for `SEL_STR`) on each orphaned node before returning. **Important:** only the replaced node itself is freed — its children are not, because the children have been re-parented (they are now children of the new node or of the existing interned node that `makeSelectNode` returned). Freeing a child that has been re-parented would cause a double-free when `free_select_tree` later walks the tree from root. + +**Concurrency in unoptimized mode:** The API server calls `parse_resreq` from +multiple goroutines concurrently. When `sel_optimize_level == 0`, `makeSelectNode` +**bypasses the shared intern table entirely** — it allocates a private (non-shared) +node tree directly via `XCEMALLOC` without touching `xce_select_table`. There is +therefore no shared intern table state, and concurrent parse calls are safe as long +as each uses an independent `select_context_t` (stack-allocated per call — already +the case). The private tree is owned by the caller and freed via `free_select_tree` +when the request completes. Concurrent `eval_select_simple` calls are also safe — +the function takes no shared state beyond the read-only AST tree and the caller-supplied +`host_eval_ctx_t`. + ### 5.1 Principle Every `xce_select_node_t` is interned in a global table keyed by its @@ -289,6 +623,14 @@ Every `xce_select_node_t` is interned in a global table keyed by its return the **same pointer**. This is **hash-consing**, used in compilers and functional language runtimes. +**Pointer equality guarantee scope:** When `sel_optimize_level > 0`, +pointer equality is guaranteed — `A && B` and `B && A` produce the +same `xce_select_node_t*`, and equality checks can use `==` on pointers. +When `sel_optimize_level == 0` (API server path), no interning occurs and +each `makeSelectNode` call returns a fresh allocation. Callers in the +disabled path must not rely on pointer equality for comparison — use +structural equality or compare by value instead. + The packcc PEG parser is bottom-up: leaves are reduced before their parents. By the time a parent node is constructed, all its children are already interned canonical pointers. Therefore: @@ -297,6 +639,17 @@ interned canonical pointers. Therefore: - No recursive comparison is ever needed. - Hash and equality are both O(1) regardless of expression depth. +**Immutability invariant:** Once a node is inserted into the intern table, its +content (`type`, `u.*`, `hashcode`) is **never mutated**. `node_id` is +reassigned by `recompact_node_ids` in the pre-session idle phase (§8.5.4) — +this is the **only** permitted mutation of an interned node field, and it +occurs only when no scheduling session is active and no `eval_select` call is +in progress. The `type`, `u.*`, and `hashcode` fields are load-bearing: +`eval_select` reads them lock-free during a session (§8.4), and the GC mark +pass reads `type` and child pointers without holding any lock. Any code that +modifies `type`, `u.*`, or `hashcode` of an interned node after insertion +violates this invariant and is incorrect. + ### 5.2 CSE Key ```c @@ -315,18 +668,30 @@ Key encoding per node type: | `SEL_GT` `SEL_LT` `SEL_GE` `SEL_LE` | `(uintptr_t)left` | `(uintptr_t)right` | | `SEL_EQ` `SEL_NE` | `(uintptr_t)min(left,right)` | `(uintptr_t)max(left,right)` | | `SEL_NOT` | `(uintptr_t)operand` | `0` | -| `SEL_RESNAME` | `(uintptr_t)res_idx` | `0` | +| `SEL_RESNAME` | `(uintptr_t)res_idx` | `0` (key only; caller passes `(uintptr_t)res_type` as op2, which `sel_make_key` zeroes for key purposes since `res_idx` uniquely determines `res_type`; the fill step stores the original op2 as `node->u.resname.res_type`) | | `SEL_NUM` | bit-cast of `double` | `0` | | `SEL_STR` | `(uintptr_t)xstring_ptr` | `0` | | `SEL_ANY` | `0` | `0` | | `SEL_TRUE` | `0` | `0` | | `SEL_FALSE` | `0` | `0` | +**Singleton disambiguation:** `SEL_ANY`, `SEL_TRUE`, and `SEL_FALSE` all encode `(op1=0, op2=0)`, so their CSE keys differ only in the `type` field. Since `type` is always compared first in `sel_key_cmp`, they produce three distinct intern table entries with no collision. + `SEL_NUM` uses the exact bit-pattern of the `double` value reinterpreted as -`uintptr_t` (via `memcpy` or union cast). Two numeric literals are -equivalent for CSE if their bit-patterns are identical; `0.0` and `-0.0` -are distinct CSE keys (they differ in the sign bit). NaN payloads are -also compared by bit-pattern. +`uintptr_t` (via `memcpy` or union cast). This is safe on all supported +64-bit platforms where `sizeof(uintptr_t) >= sizeof(double)`, but must be +guarded by a compile-time assertion in `xce_select.c`: +`static_assert(sizeof(uintptr_t) == sizeof(double), "uintptr_t must exactly hold a double bit-pattern — equality required, not merely >=, so sizeof(op1) copies all 8 bytes without truncation");` +Two numeric literals are equivalent for CSE if their bit-patterns are +identical; `0.0` and `-0.0` are distinct CSE keys (they differ in the sign +bit). NaN payloads are also compared by bit-pattern. **Boundary note:** while IEEE 754 defines +`0.0 == -0.0` as true, CSE treats them as distinct interned nodes. +At evaluation time, both produce the same boolean result (both are 0.0), +so distinct node IDs for `0.0` and `-0.0` merely occupy two memoization +slots that return the identical value — a minor memory cost, not a +correctness issue. In HPC resource expressions, `-0.0` is not a meaningful +resource value and will not appear in practice, but the bit-pattern +comparison ensures CSE correctness for all float representations. `SEL_STR` uses the interned `xstring` pointer — since `makeString()` already canonicalizes strings (libxce's string de-dup mechanism maps identical @@ -334,23 +699,96 @@ strings to the same pointer), pointer equality IS string equality. `SEL_ANY`, `SEL_TRUE`, and `SEL_FALSE` are singletons — all instances share the same interned pointer. +**SEL_ANY interning requirement:** `SEL_ANY` must always be created via +`makeSelectNode(SEL_ANY, 0, 0)` — never by allocating a raw +`xce_select_node_t` and setting `type = SEL_ANY` directly. Only +`makeSelectNode` assigns a `node_id` and adds the node to the intern +table. A raw allocation bypasses interning, producing a node that is +invisible to GC (leaked permanently) and has `node_id = 0` (conflicting +with any other node at slot 0). The same rule applies to `SEL_TRUE` and +`SEL_FALSE` — always use `makeSelectNode`, never construct them by hand. + ### 5.3 Intern Table ```c +#define SEL_MAX_INTERNED_NODES 1000000 /* hard cap on intern table size — + * prevents unbounded memory growth from + * malicious or burst job submissions. + * + * Failure mode: when makeSelectNode is called + * after the cap is reached, it must: + * 1. Log a warning (not fatal — the scheduler + * can still run existing jobs). + * 2. Set errno = ENOMEM and return NULL. + * 3. The parse_select caller must treat a NULL + * return as a parse failure and reject the + * incoming job with PREASON_RESREQ_PARSE_FAIL. + * + * Sizing: 1,000,000 nodes × ~64 B/node ≈ 64 MB. + * For typical HPC workloads (≤10 nodes/job, + * ≤100,000 concurrent jobs) the effective high-water + * mark is ~10,000 nodes post-GC dedup. 1,000,000 + * provides a 100× safety margin. Tune via + * compile-time override if workload analysis shows + * a different bound is appropriate. */ + +static _Atomic uint32_t sel_node_next_id = 0; /* monotonically increasing, reassigned by recompact; + * all writes occur under sel_node_table.lock and use + * memory_order_release: atomic_fetch_add_explicit for + * increment (§5.4), atomic_fetch_sub_explicit for rollback + * (§5.4), and atomic_store_explicit for recompact (§8.5.4). + * The pre-session read in select_eval_ctx_init + * (§8.5.3) occurs WITHOUT the lock and uses + * atomic_load_explicit(..., memory_order_acquire) + * to ensure the final makeSelectNode store is + * visible even when staging and session-init run + * in the same thread but the compiler or CPU would + * otherwise reorder the stores past the read. */ + static struct { - mylock_t lock; - var table; /* kvtab keyed by sel_node_key_t */ + mylock_t lock; + var table; /* kvtab keyed by sel_node_key_t — hash lookup only */ + zarray_t nodes; /* all interned nodes in insertion order — iterated by GC and recompact */ } sel_node_table; ``` Modelled on `xstring_table` in `lib/xstring.c`. +`sel_node_table.nodes` is an embedded `zarray_t` of `xce_select_node_t*` entries (not a pointer — allocated via `zarray_init` on the embedded struct, matching the codebase convention where all `zarray_t` fields are embedded rather than heap-allocated separately). It is the **only** data structure iterated by the mark-and-sweep GC (`gc_sweep_select_nodes`) and by `recompact_node_ids` — neither function requires an iterator over the kvtab. The kvtab exists solely for O(1) intern-key lookup; GC and compaction walk the linear `nodes` array instead. + ### 5.4 `makeSelectNode` ```c xce_select_node_t* makeSelectNode(sel_node_type_t type, uintptr_t op1, uintptr_t op2) { + /* CSE-disabled fast path (API server, sel_optimize_level == 0): + * allocate a fresh node every call, no interning, node_id intentionally + * left 0 (not meaningful without an intern table). */ + /* ATOMIC READ: must use sel_get_optimize_level() (atomic_load_explicit with + * memory_order_acquire), NOT a direct load of sel_optimize_level, which is + * _Atomic int. A direct (non-atomic) load of an _Atomic variable is UB under + * C11 §5.1.2.4 when the variable can be concurrently written. */ + if (sel_get_optimize_level() <= 0) { + xce_select_node_t* n = calloc(1, sizeof(*n)); + if (n == NULL) { + /* OOM in unoptimized mode: release caller's xstring ref (mirrors + * the optimized-mode dereference calls at steps 3a/4/5 below). + * Contract is symmetric: caller passes ownership of the xstring + * refcount; makeSelectNode either stores it in the node (success) + * or releases it (failure). Caller must NEVER dereference op1 + * after this call regardless of outcome. */ + if (type == SEL_STR) dereference((char*)op1); + errno = ENOMEM; + return NULL; + } + n->type = type; + /* fill n->u.* from op1/op2 by type (same logic as step 4 below) */ + /* SEL_STR ownership: node owns the xstring ref stored in n->u.strval. + * free_select_tree calls dereference(n->u.strval) when freeing. */ + return n; + } + /* 1. canonicalize commutative operators (see §6) */ switch (type) { case SEL_AND: case SEL_OR: case SEL_EQ: case SEL_NE: @@ -360,24 +798,57 @@ xce_select_node_t* makeSelectNode(sel_node_type_t type, break; } - /* 2. build lookup key */ - sel_node_key_t key = { type, op1, op2 }; + /* 2. build lookup key — use sel_make_key() to zero padding bytes */ + sel_node_key_t key = sel_make_key(type, op1, op2); /* 3. lookup */ xce_select_node_t* node = NULL; mylock_lock(&sel_node_table.lock); int rc = key_find(sel_node_table.table, &key, (void**)&node); - if (rc > 0) { mylock_unlock(&sel_node_table.lock); return node; } + if (rc > 0) { + mylock_unlock(&sel_node_table.lock); + if (type == SEL_STR) dereference((char*)op1); /* release caller's ref — intern table already holds one */ + return node; + } + /* 3a. intern table size check — must come AFTER the cache-hit path above. + * A table at cap still serves already-interned nodes via key_find. Only + * reject new insertions when the cap is reached. */ + if (sel_node_next_id >= SEL_MAX_INTERNED_NODES) { + mylock_unlock(&sel_node_table.lock); + if (type == SEL_STR) dereference((char*)op1); /* cap reached — release caller's ref */ + errno = ENOMEM; + return NULL; + } mylock_unlock(&sel_node_table.lock); - if (rc < 0) return NULL; /* key_find error (not ENOENT) */ + /* Return value semantics — MUST branch explicitly before proceeding: + * rc > 0 → cache hit (handled above — already returned) + * rc == -ENOENT → cache miss — proceed to allocate below + * rc == -EINVAL → bad arguments (null table, null key, etc.) — log_fatal */ + if (rc == -EINVAL) { + /* Intern table is in an invalid state; allocation would corrupt it. */ + if (type == SEL_STR) dereference((char*)op1); /* release caller's ref */ + log_fatal("makeSelectNode: key_find returned -EINVAL — intern table corrupt or uninitialized"); + return NULL; + } + /* rc == -ENOENT: proceed to allocate */ - /* 4. alloc and fill */ + /* 4. alloc and fill (node_id assigned inside lock in step 5) */ node = calloc(1, sizeof(*node)); - if (node == NULL) { errno = ENOMEM; return NULL; } + if (node == NULL) { + if (type == SEL_STR) dereference((char*)op1); /* alloc failed — release caller's ref */ + errno = ENOMEM; + return NULL; + } node->type = type; node->hashcode = sel_key_hash(&key); - node->node_id = sel_node_next_id++; /* dense index for eval context */ /* fill u.* from op1/op2 by type … */ + /* ORDERING INVARIANT: the fill of node->u.* from op1/op2 MUST occur here in + * step 4, BEFORE the mylock_lock in step 5. Step 5 calls + * dereference(node->u.strval) on the TOCTOU rollback path — this is only + * correct if u.strval has already been set to (char*)op1. An implementation + * that fills u.* after the step-5 lock acquisition would call + * dereference(NULL) on the calloc-zeroed field, corrupting the xstring + * refcount of any string that happens to match that refcount slot. */ /* 5. insert — re-check under lock for TOCTOU safety */ mylock_lock(&sel_node_table.lock); @@ -386,9 +857,19 @@ xce_select_node_t* makeSelectNode(sel_node_type_t type, if (rc2 > 0) { /* another thread inserted first — use theirs, free ours */ mylock_unlock(&sel_node_table.lock); + if (node->type == SEL_STR) dereference(node->u.strval); /* release xstring ref before free */ free(node); return existing; } + /* Re-check cap under lock — another thread may have filled the table + * between the first check and this second lock acquisition. */ + if (sel_node_next_id >= SEL_MAX_INTERNED_NODES) { + mylock_unlock(&sel_node_table.lock); + if (node->type == SEL_STR) dereference(node->u.strval); /* release xstring ref before free */ + free(node); + errno = ENOMEM; + return NULL; + } int rc3 = key_add(sel_node_table.table, &key, node); if (rc3 < 0) { /* key_add failed — if EEXIST from a race, re-find and return existing */ @@ -396,13 +877,62 @@ xce_select_node_t* makeSelectNode(sel_node_type_t type, xce_select_node_t* raced = NULL; int rc4 = key_find(sel_node_table.table, &key, (void**)&raced); mylock_unlock(&sel_node_table.lock); + if (node->type == SEL_STR) dereference(node->u.strval); /* release xstring ref before free */ free(node); return (rc4 > 0) ? raced : NULL; /* return existing or NULL on double-failure */ } mylock_unlock(&sel_node_table.lock); + if (node->type == SEL_STR) dereference(node->u.strval); /* release xstring ref before free */ free(node); return NULL; /* ENOMEM or other error */ } + /* Assign node_id and store in the linear nodes array — both under lock + * to prevent concurrent staging-phase callers from producing duplicate IDs. + * zarray_store returns the SLOT INDEX (>= 0, 0-based) on success or a + * negative errno code on failure. Use `zrc < 0` (not `zrc == 0` or + * `!zrc`) to detect failure — index 0 is a valid success return for the + * first node stored and must NOT be treated as an error. */ + node->node_id = atomic_fetch_add_explicit(&sel_node_next_id, 1, memory_order_release); + int zrc = zarray_store(&sel_node_table.nodes, node); + if (zrc < 0) { + /* zarray_store failed (OOM) — roll back the ID and remove the kvtab entry. + * memory_order_release is required on the fetch_sub even though the mutex is + * held: select_eval_ctx_init reads sel_node_next_id WITHOUT the mutex (§5.3) + * using memory_order_acquire. The release here synchronizes with that + * lock-free reader, ensuring the decremented value is visible. The mutex + * alone does not establish a happens-before with a lock-free reader, so + * memory_order_relaxed would be incorrect. + * + * Lock-free reader race: if select_eval_ctx_init fires between the + * atomic_fetch_add above and this fetch_sub, it may read the briefly- + * incremented value (N+1) and size its eval context arrays to N+1. After + * the rollback, the next makeSelectNode re-uses ID N for a legitimately + * inserted node. An eval context sized to N+1 has one extra unused slot — + * harmless (no overflow given the SEL_MAX_INTERNED_NODES cap) and correct. + * + * Rollback race safety: the freed node briefly has a valid node_id (N) + * before sel_node_next_id is decremented back to N. A subsequent + * allocation will reuse that ID for a different node. This is safe + * because: + * (a) makeSelectNode is only called from the allocator thread or staging + * phase — never concurrently with eval_select (§8.4 invariant). + * ENFORCEMENT REQUIREMENT: staging must be strictly single-threaded + * (no thread-pool dispatch of parse_resreq); Phase 6 wires a + * runtime sel_alloc_thread_id check to catch violations. Until + * Phase 6 is complete, tests must not invoke makeSelectNode from + * two threads simultaneously. + * (b) No caller ever sees the rolled-back node's pointer (makeSelectNode + * returns NULL) so no eval context can hold a pointer to it. + * If the allocator-thread-only invariant is ever relaxed, this rollback + * protocol must be re-evaluated before making that change. */ + atomic_fetch_sub(&sel_node_next_id, 1, memory_order_release); + key_del(sel_node_table.table, &key); + mylock_unlock(&sel_node_table.lock); + if (node->type == SEL_STR) dereference(node->u.strval); /* release xstring refcount */ + free(node); + errno = ENOMEM; + return NULL; + } mylock_unlock(&sel_node_table.lock); return node; @@ -415,10 +945,100 @@ select intern table adds the re-check to prevent node leaks under concurrent construction. An alternative is to hold the lock through the entire sequence (simpler but longer critical section). -`kill_primary_resreq()` in `resreq.c` must unlink the select root -pointer (`primary->select->root = NULL`) before freeing the select struct. -Node reclamation is handled by mark-and-sweep GC (§5.5) — no explicit -deref function is needed. +**makeString() TOCTOU fix (prerequisite sub-task — `src/lib/xstring.c`):** +The numeric-to-string coercion in §7 PEG actions calls `makeString(buf)` +under concurrent load (API server goroutines). If two threads concurrently +coerce the same value, `makeString`'s `key_add` can return `-EEXIST`, and +the existing implementation destroys its allocation and returns **NULL** +without recovering the existing interned string — a correctness bug that +corrupts the interned AST when this function returns NULL and the caller +passes it to `makeSelectNode`. The `makeSelectNode` pattern above corrects +this with a re-find on `-EEXIST`. This same fix must be applied to +`makeString()` in `src/lib/xstring.c` as a **prerequisite** to implementing +the §7 coercion path: on `key_add` returning `-EEXIST` **only**, re-find +the existing string and return it, rather than returning NULL. If `key_add` +returns `-ENOMEM` (kvent allocation failure), nothing was inserted — +re-finding would fail and returning NULL is correct. +The required restructuring is non-trivial because the existing code +unconditionally releases the lock at `xstring.c:166` before checking `rc`. +The corrected structure (replacing lines 164-174 of `xstring.c`): +```c + /* Save newly-allocated instance as `new_str` to avoid shadowing below */ + var new_str = makeInstance(&xstring_type, s); + if (new_str == NULL) + return NULL; + + mylock_lock(&(xstring_table.lock)); + rc = key_add(xstring_table.xstr_table, (void*)s, new_str); + if (rc == -EEXIST) { + /* Lost the race: another thread interned first. Re-find while still + * holding the lock — releasing before re-find creates a window where + * the winner could be concurrently freed (dangling pointer). + * key_find cannot fail here: we hold the lock so no concurrent delete + * or resize can occur, and -EEXIST guarantees the key is present. + * Check the return value defensively nonetheless. */ + void* existing = NULL; + int rfind = key_find(xstring_table.xstr_table, (void*)s, &existing); + if (rfind <= 0 || existing == NULL) { /* key_find returns 1 on success, not 0 */ + /* Unreachable under correct locking, but do not proceed with NULL */ + mylock_unlock(&(xstring_table.lock)); + destroy_(new_str, (obj_destroy_fn_t)xstring_destroy); + errno = ENOMEM; + return NULL; + } + reference(existing); /* bump before unlock */ + mylock_unlock(&(xstring_table.lock)); + destroy_(new_str, (obj_destroy_fn_t)xstring_destroy); /* free our copy */ + return (char*)existing; + } + mylock_unlock(&(xstring_table.lock)); + if (rc < 0) { + destroy_(new_str, (obj_destroy_fn_t)xstring_destroy); + errno = -rc; + return NULL; + } + return (char*)new_str; +``` +**Deadlock guard for `reference()` under lock:** The fix calls +`reference(existing)` while holding `xstring_table.lock`. Before +implementing, verify that `reference()` does not attempt to acquire +`xstring_table.lock` internally (a nested lock → deadlock). In the +existing xstring RTTI pattern, `reference()` increments a refcount in +the object header — this is an atomic increment with no lock, so no +deadlock. Confirm this by reading `src/lib/xcealloc.c` or `wrap.c` +(whichever defines `reference()`). If the implementation changes to +use a mutex for refcount, the fix must move `reference()` outside the +lock (bump refcount after unlock, with a separate guard for the +use-after-free window). + +**Scope note:** `src/lib/xstring.c` is outside the select-clause scope of §2. +This fix must be tracked as a separate sub-task and completed before the §7 +coercion path is activated, whether in the same PR or an immediately preceding +one. It protects all `makeString` callers, not just the select coercion path. + +`kill_primary_resreq()` in `resreq.c` must follow mode-specific teardown: + +```c +if (primary->select) { + xce_select_node_t* root = primary->select->root; + primary->select->root = NULL; /* unlink first (GC reads this) */ + if (sel_get_optimize_level() <= 0 && root != NULL) + free_select_tree(root); /* disabled: free private tree */ + /* CSE-enabled: GC reclaims nodes; no free of root pointer here */ + FREEUP(primary->select); /* always: free the container struct */ +} +``` + +**Ownership contract:** +- `primary->select` (the `xce_select_t` container struct) is **always** owned + by `primary` and must always be freed by `kill_primary_resreq`. +- `primary->select->root` (the `xce_select_node_t*`) is owned differently + by mode: + - CSE-enabled: owned by the intern table; GC reclaims — never `free()` it. + - CSE-disabled: owned by the job; `free_select_tree(root)` before `FREEUP`. +- The NULL store must happen **before** `FREEUP(primary->select)` so that + any concurrent GC mark pass (§8.5.3) cannot dereference a dangling pointer + through a job whose container has already been freed. ### 5.5 Mark-and-Sweep GC Between Scheduling Sessions @@ -428,54 +1048,235 @@ is no `derefSelectNode` function — when a job completes, its resreq entry is simply unlinked from the job (see §5.4); the next GC sweep reclaims unreachable nodes. +**GC idle invariant:** `gc_sweep_select_nodes()` runs **in the +pre-session phase**, after the staging buffer has been merged into the +live job set and after all job cleanup (`primary->select->root = NULL` +for completed jobs) is complete, but before the next scheduling session +begins. At that point the scheduler is completely idle: no scheduling +session is active and no `eval_select` call is in progress. Running GC +after the merge ensures that newly arrived jobs are visible to the mark +pass — otherwise, nodes referenced only by incoming jobs could be +incorrectly swept. This is an architectural invariant of the +session-based scheduler, and **must be enforced with runtime guards +(Not just `assert()`, which is compiled out with `NDEBUG`)**: + +```c +/* Enforce at the gc_sweep_select_nodes call site in the scheduler. + * These are runtime checks — not assert() — because assert() is + * compiled out in release builds (NDEBUG), and a GC sweep that + * runs concurrently with eval_select causes use-after-free. */ +if (sched_session_active(sched) || sched_eval_in_progress(sched) + || !staging_merge_complete(sched)) { + log_fatal("gc_sweep_select_nodes called with invariant violated: " + "session_active=%d eval_in_progress=%d staging_merge=%d", + sched_session_active(sched), sched_eval_in_progress(sched), + staging_merge_complete(sched)); + return; /* do not sweep — prevent use-after-free */ +} +gc_sweep_select_nodes(live_jobs, njobs); ``` -gc_sweep_select_nodes() + +Where `sched_session_active`, `sched_eval_in_progress`, and +`staging_merge_complete` are scheduler-internal predicate functions. +These runtime guards survive release builds and log a fatal error +rather than silently proceeding with a corrupted intern table. + +**Predicate lifecycle (Phase 1 → Phase 6):** In Phases 1–5 (library +development and unit testing), these predicates are implemented as thin +wrappers around a `fake_scheduler_t` struct declared in `test_select.c` — +a test-only scaffold with the same three `_Atomic int` fields. In Phase 6 +(scheduler wiring), the wrapper bodies are updated to reference the real +`xce_scheduler_t` fields added in that phase. The function signatures do +not change between phases; only the struct pointer inside each wrapper body +changes. No Phase 1 stub code leaks into the production implementation. + +**Phase 6 predicate fields in `xce_scheduler_t`:** Three `_Atomic int` +fields must be added to `struct xce_scheduler_` (in +`src/scheduler/include/scheduler.h`): + +```c +_Atomic int session_active; /* 1 during scheduling session, 0 between */ +_Atomic int eval_in_progress; /* 1 while eval_select loop is running, 0 otherwise */ +_Atomic int staging_merge_complete; /* 1 after staging merge completes, 0 before */ +``` + +These fields are initialized to 0 in the scheduler's init function +(Phase 6 wiring) and set/cleared at the appropriate session-loop +transitions. The predicate wrappers read them via +`atomic_load_explicit(..., memory_order_acquire)` to ensure visibility +from the allocator thread's writes. The `fake_scheduler_t` in Phase 1 +tests uses the same field names and types, so the predicate wrapper +bodies are mechanically identical — only the container struct pointer +differs. + +``` +gc_sweep_select_nodes(const xce_sched_job_t** live_jobs, size_t njobs) 1. Mark pass: - a. Clear the GC mark flag on every node in sel_node_table. - b. For each live job's resreq, visit the AST tree starting from - the root node and set the GC mark flag on every reachable node. - (If a node is referenced by multiple AST trees, it is marked - only once — the flag is boolean.) - 2. Sweep pass (two-phase to avoid modifying the hash table during iteration): - a. Iterate sel_node_table using the kvtab iterator interface - (`iter_make`/`iter_next`). Collect keys of unmarked nodes into a - temporary array. - b. For each collected key: remove the node from the intern table - (`key_del`), then free the node. - c. Clear all GC mark flags on remaining (reachable) nodes. + a. Iterate sel_node_table.nodes (zarray_t) over slots 0..caps-1: + skip NULL slots (vacated by zarray_del from a prior GC sweep); + clear gc_mark on every live node. + b. For each live job in live_jobs[0..njobs-1]: dispatch on + job->resreq->tag and walk all reachable select roots, setting + gc_mark on every reachable node. (Shared sub-nodes are marked + once — gc_mark is boolean.) + - XCE_RESREQ_PRIMARY: mark from resreq->u.primary->select->root. + - XCE_RESREQ_COMPOUND: iterate resreq->u.compound->primaries and + mark each primary's select->root. + - XCE_RESREQ_ALTER: iterate resreq->u.alter->compounds, then each + compound's primaries, then each primary's select->root. + (xce_sched_job_t has xce_resreq_t* resreq; xce_job_t only has char* resreq + and cannot access the parsed AST.) + 2. Sweep pass: + a. Iterate sel_node_table.nodes (zarray_t) over slots 0..caps-1: + skip NULL slots; collect (slot_idx, node*) pairs for unmarked nodes. + **Note:** `slot_idx` here is the loop index `i` from the zarray + iteration — it is NOT stored in `xce_select_node_t` (the struct has no + `slot_idx` field). The GC sweep obtains the correct `zarray_del` argument + directly from the forward-iteration index, not from any per-node field. + b. For each unmarked node (iterate collected pairs): + - Reconstruct the intern key: `sel_node_key_t key = sel_node_to_key(node)` + (see §12 Appendix). The commutativity sort must be applied during + key reconstruction, identical to the sort applied in `makeSelectNode` + step 1, otherwise the reconstructed key hashes to a different bucket + and `key_del` silently no-ops, leaking the kvtab entry. + - Remove its key from sel_node_table.table via key_del. + (sel_node_table is initialized with datakiller=NULL so key_del + removes only the hash table entry — the node value is NOT freed by + key_del; see discussion below on datakiller.) + - If node->type == SEL_STR: call `dereference(node->u.strval)` to + release the xstring refcount acquired by `makeString()`. + All SEL_STR nodes are created via `makeString()`, so strval is + always an xstring-interned pointer — `free()` is incorrect here. + - free(node). + - zarray_del(&sel_node_table.nodes, slot_idx) — NULLs the slot and + decrements count. Does NOT swap-remove; remaining slot indices are + stable. NULL-slot skipping in step 1a handles future iterations. ``` +The `gc_mark_tree` helper does a recursive post-order mark: + +```c +/* Mark all nodes reachable from root (recursive DFS). + * Leaf nodes (SEL_RESNAME, SEL_NUM, SEL_STR, SEL_ANY, SEL_TRUE, + * SEL_FALSE) have no children — they set gc_mark and return immediately. + * SEL_NOT has one child (node->u.operand). + * SEL_AND/OR/GT/LT/GE/LE/EQ/NE have two children (node->u.binop.left, + * node->u.binop.right). + * + * Recursion depth: bounded by the post-flatten AST depth, which is at most + * SELECT_MAX_DEPTH - 1 levels (the parse-time nesting limit). Any increase + * to SELECT_MAX_DEPTH must be reviewed against stack-frame size to avoid + * stack overflow. Cycles are impossible — interned nodes form a DAG. */ +static void gc_mark_tree(xce_select_node_t* node) +{ + if (node == NULL || node->gc_mark) return; /* NULL root or already marked */ + node->gc_mark = 1; + switch (node->type) { + /* leaf types — no children */ + case SEL_RESNAME: case SEL_NUM: case SEL_STR: + case SEL_ANY: case SEL_TRUE: case SEL_FALSE: + break; + /* unary */ + case SEL_NOT: + gc_mark_tree(node->u.operand); + break; + /* binary */ + default: /* SEL_AND, SEL_OR, SEL_GT, SEL_LT, SEL_GE, SEL_LE, SEL_EQ, SEL_NE */ + gc_mark_tree(node->u.binop.left); + gc_mark_tree(node->u.binop.right); + break; + } +} +``` + +**COMPOUND/ALTER xlist iteration:** `xce_compound_resreq_t.primaries` and +`xce_alter_resreq_t.compounds` are `xlist_t` linked lists. Use the +standard `xlist_get` iteration pattern (as used elsewhere in the codebase +for `xlist_t` iteration): + +```c +/* Example: iterate primaries in a COMPOUND resreq */ +xlist_t* list = &resreq->u.compound->primaries; +for (int i = 0; i < xlist_length(list); i++) { + xce_primary_resreq_t* p = (xce_primary_resreq_t*)xlist_get(list, i); + if (p && p->select && p->select->root) + gc_mark_tree(p->select->root); +} +``` + +Do not use pointer arithmetic or `xlist_head`/`xlist_next` directly — use +`xlist_get` with a counted loop, which is the existing pattern in the +codebase (e.g., `resource.c`). + This approach is simple and correct: +- GC iterates `sel_node_table.nodes` (the linear `zarray_t`), not the + kvtab. No kvtab iterator interface is required. +- `key_del` removes only the hash table entry. `datakiller` is a + **table-level** configuration set at `kvtab_init` time — it is not a + per-call argument. `sel_node_table` MUST be initialized with + `datakiller=NULL` so that `key_del` does not free the node value; the + node is freed explicitly afterward. This avoids a double-free. Do not + pass a non-NULL `datakiller` when initializing `sel_node_table`. - A node is reachable if and only if it is visited from at least one live - job's resreq root. No reference provenance tracking is needed — the - mark flag is a single boolean per node. -- The sweep runs only between sessions (no concurrent eval), so no - read-after-free risk exists. + job's resreq root. No reference provenance tracking is needed. - `SEL_STR` nodes' `xstring` references are handled by the same mark pass — if the SEL_STR node is reachable, the string stays interned. +- **GC does not acquire `sel_node_table.lock`**: The GC sweep runs in the + pre-session phase while the scheduler is completely idle (§8.4). No + concurrent `makeSelectNode` or `key_find` calls execute during GC, so + `key_del` operates on a quiescent hash table without lock contention. + The idle invariant is enforced by runtime guards (not just `assert()`, + which is compiled out with `NDEBUG`) at the GC call site — see the + guard block above. Do not add `mylock_lock`/`mylock_unlock` around GC's + `key_del` calls; doing so would add unnecessary overhead and could + deadlock if `key_del` itself acquires an internal lock. -The scheduler is responsible for calling `gc_sweep_select_nodes()` -between scheduling sessions, typically after all dispatch/defer decisions -for the current batch of jobs have been made. **Job cleanup -(`primary->select->root = NULL`) also happens between sessions** — -there is no mid-session job deletion. When a job completes, its -entry is marked for removal; the select root pointer is unlinked and -the job struct freed at the session boundary, before GC runs. This -eliminates the risk of shared sub-nodes becoming unreachable mid-session. +**Job cleanup timing:** There is no mid-session job deletion. When a +job completes, its entry is marked for removal; the select root pointer +is unlinked and the job struct freed at the session boundary, before GC +runs. This eliminates the risk of shared sub-nodes becoming unreachable +mid-session. -After GC, node IDs are re-compacted to `0..N-1` (§8.5.4) so that the -evaluation context arrays remain dense. This is done by a linear scan -of the intern table that reassigns `node_id` sequentially. - -The function is not thread-safe with respect to concurrent -`eval_select` calls, so the scheduler must ensure no evaluations are -in progress when GC runs. +After GC, node IDs are re-compacted to `0..N-1` (§8.5.4) for two +reasons: (1) to keep the evaluation context arrays dense so per-host +allocation is bounded by the live node count; (2) to prevent +`uint32_t node_id` overflow in long-running scheduler instances — +`sel_node_next_id` would otherwise grow monotonically across thousands of +GC cycles and eventually wrap. Recompaction resets `sel_node_next_id` to +the current live count, keeping it bounded. Recompaction iterates +`sel_node_table.nodes` — no kvtab iterator is needed. Like IBM LSF, the XCE scheduler operates in sessions — each session evaluates all pending jobs against candidate hosts, dispatches what it -can, then ends. Pre/post-session work (such as GC) is a natural fit -for this architecture. +can, then ends. Pre-session work (staging merge, GC, recompaction) +runs between sessions while the scheduler is idle, ensuring new jobs +are visible to the mark pass before any nodes are reclaimed. + +**Job submission and scheduling cycle:** The controller accepts job +submissions asynchronously and pushes them to the scheduler via a +**staging buffer**. The scheduler's main loop is serial, with each cycle +following this sequence: + +1. **Staging:** New job resreq strings from the staging buffer are parsed + (calling `makeSelectNode`), and the resulting interned nodes are merged + into the intern table. Completed jobs from the previous session have + their select root pointers unlinked (`primary->select->root = NULL`). +2. **GC + recompaction:** With the staging merge complete and all live jobs + (old + newly arrived) visible, `gc_sweep_select_nodes` reclaims + unreachable nodes, then `recompact_node_ids` reassigns dense 0..N-1 + IDs. Running GC after the merge ensures nodes referenced only by + incoming jobs are marked live and not incorrectly swept. +3. **Session:** The allocator thread evaluates each job against each + candidate host via `eval_select`, dispatches what it can, and defers + the rest. All interned nodes are read-only — no `makeSelectNode` + calls occur during the session. + +This serial architecture ensures that no concurrent `makeSelectNode` +calls occur during GC sweep. The dual-buffer pattern (staging buffer +for incoming jobs + active intern table for the session) is +scheduler-internal and distinct from the controller's own persistence +mechanism (snapshot + WAL). **Pointer-based canonicalization note:** Sorting by `uintptr_t` produces a deterministic total order within a single process lifetime. Across @@ -542,16 +1343,24 @@ A **const-fold pass** simplifies the AST by evaluating expressions that are statically determined at parse time: - `resname == any` → `SEL_TRUE` (always matches, regardless of the - resource value). The `any` keyword (§3.1) is only valid on the - right-hand side of `==` or `!=`. For `!=`, `resname != any` is a - parse error (nothing would not match any). + resource value). +- `resname != any` → `SEL_FALSE` (never matches — `any` matches every + value, so nothing can fail to match it). Both `!= any` forms are + accepted at parse time; the const-fold fires immediately, yielding + the same interned `SEL_FALSE` singleton as `!(resname == any)`. + **Implementation note:** after the commutativity swap in `makeSelectNode` + (§5.4), the `SEL_ANY` singleton may appear in either the `left` or + `right` child slot of `SEL_EQ`/`SEL_NE` — the address-order sort is + non-deterministic across process lifetimes (ASLR). The const-fold + pass must check **both** `node->u.binop.left` and `node->u.binop.right` + for pointer equality with the `SEL_ANY` singleton, not only one side. - Identity laws:`SEL_TRUE && X` → `X`, `X && SEL_TRUE` → `X`; `SEL_FALSE || X` → `X`, `X || SEL_FALSE` → `X` - Annihilation laws: `SEL_FALSE && X` → `SEL_FALSE`, `X && SEL_FALSE` → `SEL_FALSE`; `SEL_TRUE || X` → `SEL_TRUE`, `X || SEL_TRUE` → `SEL_TRUE` - Complement laws: `!SEL_TRUE` → `SEL_FALSE`, `!SEL_FALSE` → `SEL_TRUE` -- Boolean equality: `bool_res == SEL_TRUE` → `bool_res` (since `x == true` is just `x`); - `bool_res == SEL_FALSE` → `SEL_NOT(bool_res)` (since `x == false` is `!x`); - `bool_res != SEL_TRUE` → `SEL_NOT(bool_res)`; `bool_res != SEL_FALSE` → `bool_res`. - These apply only when `bool_res` is a `SEL_RESNAME` with `res_type == RES_TYPE_BOOL`. +- Boolean equality: `bool_res == true` and `bool_res == false` are **parse + errors** (type mismatch — `true`/`false` are not keywords), so no const-fold + rules are needed for these patterns. The idiomatic forms `bool_res` and + `!bool_res` are the only valid boolean usage. The const-fold pass runs **after** construction and flattening, replacing subtrees with constant leaf nodes. Since all nodes are interned, @@ -572,10 +1381,43 @@ the raw tree: which performs commutativity canonicalization as part of CSE key construction) 2. Post-parse tree walk: associativity flattening (§6.2) + → re-intern modified subtrees via `makeSelectNode` 3. Post-parse tree walk: const-fold (§6.3) -4. Bottom-up re-intern via `makeSelectNode` for any modified subtrees + → re-intern modified subtrees via `makeSelectNode` ``` +**Flattening termination:** The `select_and` and `select_or` grammar +rules use `( op operand )*` repetition. A correctly implemented PEG +action that accumulates left-to-right (`$$ = makeSelectNode(AND, $$, +next)`) produces a left-associative tree directly from the grammar — +flattening is a no-op for unparenthesized chains. Flattening only +fires when the user adds explicit parentheses, e.g. `A && (B && C)`. + +For depth-2 explicit parenthesization (`A && (B && C)`), a single +post-order pass suffices: the inner `(B && C)` is processed first and +its children are leaves, so after the outer rotation no AND right-child +remains. For depth-3+ explicit parenthesization (`A && (B && (C && +D))`), a single post-order pass leaves `(A && (B && C)) && D` with +`(A && (B && C))` still having an AND right child. **The flatten pass +must therefore repeat until no AND-node has an AND right-child**: + +```c +do { changed = 0; changed |= flatten_pass(root); } while (changed); +``` + +**Note:** any orphaned-node list maintained by `flatten_pass` is +stack-local to that call — it is not accumulated across loop iterations. +Each iteration frees the orphans it discovers before returning `changed`. +This means the loop is safe to repeat without risk of double-free or +accumulated-list confusion across passes. + +In practice, HPC select expressions do not use deeply nested explicit +parenthesization (2–3 levels is the realistic maximum), so the loop +terminates in at most 2 iterations for real workloads. Const-fold does +not create new flattening opportunities because it replaces subtrees +with single leaf nodes (SEL_TRUE/SEL_FALSE), shrinking the tree rather +than restructuring it, so const-fold remains single-pass. + Note: commutativity canonicalization (operand swap) happens *inside* `makeSelectNode` as part of the CSE key lookup — it is not a separate pipeline step. The PEG action for each binary operator calls @@ -586,6 +1428,19 @@ new normalized subtrees. Since `makeSelectNode` already handles interning and canonicalization, these walks naturally produce the maximally shared tree. +**No interned-node mutation:** `flatten_pass` NEVER modifies an already-interned +node's fields. Re-association is done entirely by calling `makeSelectNode` to +construct NEW canonical nodes (e.g., `new_inner = makeSelectNode(AND, A, B)`, +then `new_outer = makeSelectNode(AND, new_inner, C)`). The old outer node +becomes an orphan. This preserves the §5.1 immutability invariant: once a +node is inserted into the intern table its content is never mutated. + +**Orphaned node ownership in CSE-disabled mode:** When `flatten_pass` +replaces a subtree, the old (orphaned) node is not tracked by the intern +table and will not be reclaimed by GC. In CSE-disabled mode, `flatten_pass` +must explicitly free every replaced node — see the full ownership contract +in §5.0 ("Flatten-pass ownership in disabled mode"). + --- ## 7. Parse-Time Type Validation @@ -594,16 +1449,39 @@ The resmgr is passed into the select parse context so resource names can be resolved to `(res_idx, res_type)` at parse time: ```c +#define SELECT_MAX_INPUT_LEN 8192 /* reject expressions longer than 8KB */ +#define SELECT_MAX_DEPTH 64 /* maximum nesting depth for &&/||/! — + * Stack analysis: eval_select_inner has ~80–120 B + * frame (pointer args + local int + switch + return addr); + * at depth 64, peak recursive stack ≈ 64 × 128 B = 8 KB. + * Default Linux thread stack is 8 MB; scheduler threads + * use the default stack. 8 KB is well within bounds. + * If SELECT_MAX_DEPTH is raised above ~500, re-measure + * actual frame size with `objdump -d` and verify against + * the thread stack size configured in §8.4. */ + typedef struct { const char* input; int pos; int err_pos; int has_error; - char* errmsg; + char errmsg[256]; /* fixed-size, no alloc, no free */ + int depth; /* current nesting depth, checked against SELECT_MAX_DEPTH */ var resmgr; /* name → (res_idx, res_type) */ } select_context_t; ``` +**resmgr locking:** PEG actions that call `key_find(resmgr, ...)` to resolve +resource names must acquire the resmgr read lock before the lookup and +release it afterward, matching the pattern used throughout the scheduler +and API server codebase (`lock(resmgr, RTTI_LOCK_READ)` before lookup, +`unlock(resmgr)` after). In CSE-enabled mode (scheduler), the staging +phase is single-threaded in practice, but the read lock is still required +for correctness — a future staging-thread change would need it, and the +lock is not on any hot path. In CSE-disabled mode (API server, concurrent +goroutines), the read lock is equally required — without it, a concurrent +`key_add` adding a new resource could corrupt the hash table read. + PEG actions validate types before calling `makeSelectNode`: ```c @@ -628,29 +1506,84 @@ the literal is coerced to the resource's type: - `mem > 100` — `mem` is `RES_TYPE_NUMERIC`, `100` is `SEL_NUM` — types match. - `gpu` — `gpu` is `RES_TYPE_BOOL`, used as a standalone boolean operand - (idiomatic LSF form). `gpu == true` is also accepted but const-folds - to just `gpu`; `gpu == false` const-folds to `!gpu`. + (the only valid form). `gpu == 1` is accepted (numeric comparison against + a boolean resource stored as 0/1). `gpu == true` is rejected as a type + mismatch (`true` is a string literal, not a boolean keyword). -This coercion guarantees that at eval time, both operands of `EQ`/`NE` -always have matching types — the `switch (lv.type)` dispatch in -`eval_select` never reads the wrong union member. +This coercion guarantees that at eval time, `EQ`/`NE` operand types are +compatible. For all type pairs except `BOOL` × `NUMERIC`, the types +match exactly. For `BOOL` × `NUMERIC` (e.g., `gpu == 1`), the eval +code coerces the boolean to `0.0`/`1.0` for comparison — see §8.3. ```c -/* action for '==' '!=' — coerce literal to resource type */ +/* action for '==' '!=' — type validation and coercion */ + +/* Reject type mismatches involving boolean resources */ +if (lnode->type == SEL_RESNAME && rnode->type == SEL_STR + && lnode->u.resname.res_type == RES_TYPE_BOOL) { + /* gpu == "true" — reject: boolean vs string */ + auxil->has_error = 1; + $$ = NULL; +} else if (rnode->type == SEL_RESNAME && lnode->type == SEL_STR + && rnode->u.resname.res_type == RES_TYPE_BOOL) { + /* "true" == gpu — reject: string vs boolean */ + auxil->has_error = 1; + $$ = NULL; +} + +/* Coerce numeric literal to string when compared with string resource */ if (lnode->type == SEL_RESNAME && rnode->type == SEL_NUM && lnode->u.resname.res_type == RES_TYPE_STRING) { - /* convert numeric literal to string: 5 → "5", preserving full double precision */ char buf[64]; snprintf(buf, sizeof(buf), "%.17g", rnode->u.numval); - rnode = makeSelectNode(SEL_STR, (uintptr_t)makeString(buf), 0); + /* NULL CHECK REQUIRED: makeString returns NULL on OOM. Passing NULL cast + * to uintptr_t (== 0) to makeSelectNode would intern a SEL_STR node with + * u.strval=NULL, which silently evaluates to non-match for every host and + * also causes dereference(0) in the step-5 TOCTOU rollback path (§5.4). */ + char* s = makeString(buf); + if (s == NULL) { auxil->has_error = 1; $$ = NULL; } else { + /* ORPHAN NOTE: rnode currently points to a SEL_NUM node allocated by + * a prior makeSelectNode call. In unoptimized mode, that node has no + * owner once rnode is overwritten — it MUST be freed before replacing + * the pointer. In optimized mode the intern table holds the only + * reference; do NOT free it (freeing an interned node corrupts the + * table). Implementation must branch on sel_get_optimize_level(): + * if (sel_get_optimize_level() == 0) free(rnode); + * (no dereference needed — SEL_NUM has no xstring field). */ + rnode = makeSelectNode(SEL_STR, (uintptr_t)s, 0); + /* NULL CHECK: makeSelectNode can return NULL (OOM, table cap, zarray fail). + * If NULL, xstring ref for s was already released by makeSelectNode. + * Do NOT pass NULL as op2 to the outer makeSelectNode — that would intern + * a malformed EQ node whose NULL right child silently matches every host. */ + if (rnode == NULL) { auxil->has_error = 1; $$ = NULL; } + } } /* symmetric: coerce if right is RESNAME and left is numeric literal */ if (rnode->type == SEL_RESNAME && lnode->type == SEL_NUM && rnode->u.resname.res_type == RES_TYPE_STRING) { char buf[64]; snprintf(buf, sizeof(buf), "%.17g", lnode->u.numval); - lnode = makeSelectNode(SEL_STR, (uintptr_t)makeString(buf), 0); + /* Same NULL CHECK and ORPHAN NOTE as above: free lnode (SEL_NUM) in + * unoptimized mode before overwriting the pointer. */ + char* s = makeString(buf); + if (s == NULL) { auxil->has_error = 1; $$ = NULL; } else { + lnode = makeSelectNode(SEL_STR, (uintptr_t)s, 0); + /* NULL CHECK: same as above — if NULL, s ref already released. */ + if (lnode == NULL) { auxil->has_error = 1; $$ = NULL; } + } } +/* Locale note: snprintf with %.17g may produce locale-dependent output + * (e.g., comma decimal separator in European locales). The select parser + * must run with C locale (LC_NUMERIC=C) to guarantee '.' as the decimal + * separator, ensuring consistent string comparison at eval time. The + * scheduler already runs with LC_ALL=C; if the API server path ever uses + * numeric-to-string coercion, it must also set LC_NUMERIC=C before calling + * parse_resreq. */ +/* Boolean vs numeric comparison is accepted: gpu == 1, gpu != 0. + * The numeric literal stays as SEL_NUM; at eval time, the boolean value + * (0 or 1) is compared as (double)bool_val == num_val. No coercion + * is needed at parse time beyond the type-validation check above (which + * rejected BOOL vs STRING). */ $$ = (char*)makeSelectNode(SEL_EQ, (uintptr_t)lnode, (uintptr_t)rnode); ``` @@ -661,19 +1594,45 @@ Where `operand_is_numeric(node)` returns true if: See §12 for the formal declaration. Unknown resource names fail the parse (rejected at job submission time). +In compound (`X * { primary } + Y * { primary }`) and alter (`{ compound } || { compound }`) +resreq strings, a parse error in any primary's select clause rejects the +entire resreq — there is no partial-accept fallback. -**resmgr=NULL fallback:** When `resmgr` is NULL (as called by the -existing `parse_resreq_primary` entry point), the parser cannot -disambiguate resname from string_literal. In this mode, all bare -words are treated as `SEL_STR` and no type validation is performed — -the caller must validate type compatibility before evaluation. The -new entry point `parse_resreq_primary_with_resmgr` should be preferred -whenever the resource manager is available. +**resmgr is mandatory:** `parse_resreq_primary` requires a non-NULL +`resmgr`. There is no NULL-fallback mode. This is a new product +with no compatibility constraint, so the interface is defined cleanly +from the start: every call site must supply the resource manager. +Without `resmgr`, the parser cannot resolve a bare word to +`SEL_RESNAME` vs `SEL_STR`, cannot validate operand types, and cannot +produce a correctly typed AST. Callers that do not have `resmgr` +available must obtain it before calling the parser. -The **resmgr** (resource manager) is simply an array of resource -definitions — each entry maps a resource name to its type and other -metadata. The API server has access to this information via the cluster -configuration and can pass it to the parser at job submission time. +**resmgr stability invariant (enforced):** Resources are never deleted +during cluster operation — they may only be added. This is an +architectural guarantee of the XCE scheduler: resmgr entries are +append-only for the process lifetime, enforced by the resmgr +implementation (no delete API is exposed). Debug builds should include +a periodic assertion in the GC mark pass that every `res_idx` stored in +an interned `SEL_RESNAME` node still resolves to a valid resmgr entry. +This guarantees that `res_idx` values embedded in interned nodes remain +valid for the process lifetime. No intern table invalidation is +required on resmgr changes. If a new resource is added, existing +interned nodes are unaffected; new parse operations will resolve the new +resource to a new `res_idx` and create new `SEL_RESNAME` nodes. + +**resmgr availability:** Both the API server and scheduler have access +to the resmgr via the cluster configuration. The resmgr is initialized +once at process startup and updated only by addition. Call sites: +- Scheduler: `sched->resmgr` is available at job-parse time in + `make_sched_job`; thread it into the resreq parse context. +- API server: obtain from the cluster configuration context available + at request time; pass into `parse_resreq` as a new parameter or via + an externally initialized `resreq_context_t`. + +The **resmgr** is an array of resource definitions — each entry maps +a resource name to its type and other metadata. Both the API server +(for validation at job submission) and the scheduler (for full CSE +parse) have access to this information via the cluster configuration. --- @@ -729,12 +1688,21 @@ per-host `select_eval_ctx_t` (§8.5) that provides resource values by evaluation results by `node_id` so each node is evaluated at most once per host across all jobs. +**Note:** The pseudocode below is the **host-access reference version** — it +reads live host data directly on every call for clarity. The Phase 4e +implementation substitutes `ctx->num_vals` / `ctx->str_vals` cache writes +in leaf branches and cache reads in comparison branches (see §8.5.2 +implementation note). This section is the behavioral spec; §8.5.2 is the +performance-correct production form. + ```c /* Inner eval — called by memoized eval_select (§8.5.2) on cache miss. - * Returns: 1 = host matches, 0 = host does not match, -1 = error */ + * Returns: 1 = host matches, 0 = host does not match, -1 = error. + * NULL node means no select constraint — all hosts match. */ static int eval_select_inner(const xce_select_node_t* node, select_eval_ctx_t* ctx) { + if (node == NULL) return 1; /* no select clause → all hosts match */ int lv, rv; const host_eval_ctx_t* host = ctx->host; /* host snapshot in ctx */ @@ -774,10 +1742,22 @@ static int eval_select_inner(const xce_select_node_t* node, switch (node->u.resname.res_type) { case RES_TYPE_BOOL: return host_get_bool (host, node->u.resname.res_idx); case RES_TYPE_NUMERIC: return host_get_float(host, node->u.resname.res_idx) != 0.0f; - default: return -1; /* string standalone rejected at parse time */ + case RES_TYPE_STRING: return -1; /* string standalone rejected at parse time (§7) */ + default: return -1; /* defensive: unrecognized res_type */ } - /* numeric comparisons — operand types validated at parse time */ + /* numeric comparisons — operand types validated at parse time. + * IMPLEMENTATION NOTE: Phase 4e must NOT call get_leaf_value() directly + * here. get_leaf_value() reads host data without writing to ctx->num_vals. + * The correct implementation must: + * 1. Call eval_select(left_leaf, ctx) — this writes ctx->num_vals[left->node_id] + * 2. Call eval_select(right_leaf, ctx) — this writes ctx->num_vals[right->node_id] + * 3. Then read ctx->num_vals[left->node_id] and ctx->num_vals[right->node_id] + * If get_leaf_value() is called directly (bypassing the cache write), and the + * leaf node was never evaluated as a standalone bool_bits entry, num_vals[] is + * 0.0 (the memset-to-zero default), and the comparison silently uses 0.0 for the + * actual host value — a correctness bug, not a performance issue. + * The code shown here uses get_leaf_value() for conceptual clarity only. */ case SEL_GT: case SEL_LT: case SEL_GE: case SEL_LE: { double l = get_leaf_value(node->u.binop.left, ctx).u.num; double r = get_leaf_value(node->u.binop.right, ctx).u.num; @@ -787,21 +1767,69 @@ static int eval_select_inner(const xce_select_node_t* node, return l <= r; } - /* type-dispatched equality — type guaranteed compatible at parse time */ + /* type-dispatched equality — parse-time validation ensures compatible types. + * Allowed pairs: NUM×NUM, STR×STR, BOOL×BOOL, BOOL×NUM, NUM×BOOL. + * Rejected at parse time: BOOL×STR, STR×BOOL. + * Coerced at parse time: numeric literal compared to string resource → string + * literal (§7 snprintf coercion), so eval sees STR×STR, not mixed types. */ case SEL_EQ: case SEL_NE: { + /* IMPLEMENTATION NOTE: Phase 4e must NOT call get_leaf_value() directly here. + * get_leaf_value() reads host data without writing to ctx->num_vals/str_vals. + * The correct implementation must: + * 1. Call eval_select(left_leaf, ctx) — this writes ctx->num_vals[left->node_id] + * or ctx->str_vals[left->node_id] for RESNAME/SEL_NUM/SEL_STR leaves + * 2. Call eval_select(right_leaf, ctx) — same + * 3. Then read ctx->num_vals[left->node_id] / ctx->str_vals[left->node_id] + * If get_leaf_value() is called directly (bypassing the cache write), and the + * leaf node was never evaluated as a standalone bool_bits entry, num_vals[] is + * 0.0 (the memset-to-zero default), and the comparison silently uses 0.0/NULL + * for the actual host value — a correctness bug, not a performance issue. + * The pseudocode here uses get_leaf_value() for conceptual clarity only. */ sel_val_t l = get_leaf_value(node->u.binop.left, ctx); sel_val_t r = get_leaf_value(node->u.binop.right, ctx); int eq; switch (l.type) { - case RES_TYPE_NUMERIC: eq = l.u.num == r.u.num; break; - case RES_TYPE_STRING: eq = xce_str_match(l.u.str, r.u.str); break; - case RES_TYPE_BOOL: eq = l.u.bval == r.u.bval; break; + case RES_TYPE_NUMERIC: + if (r.type == RES_TYPE_BOOL) + eq = l.u.num == (double)r.u.bval; /* NUM×BOOL: 1 == gpu */ + else if (r.type == RES_TYPE_NUMERIC) + eq = l.u.num == r.u.num; /* NUM×NUM */ + else + return -1; /* defensive: NUM×STR rejected at parse time */ + break; + case RES_TYPE_STRING: + /* Defensive: STR×BOOL and STR×NUM are rejected at parse time (§7), but + * a future parse-validation gap could smuggle an incompatible type here. + * Guard explicitly to prevent reading r.u.str from an uninitialized union. */ + if (r.type != RES_TYPE_STRING) return -1; + /* NULL means the resource has no value for this host (absent resource). + * Treat absent-resource comparisons as non-matching (eq=0 for ==, 1 for !=). */ + if (l.u.str == NULL || r.u.str == NULL) { eq = 0; break; } + eq = xce_str_match(l.u.str, r.u.str); /* STR×STR */ + break; + case RES_TYPE_BOOL: + if (r.type == RES_TYPE_BOOL) + eq = l.u.bval == r.u.bval; /* BOOL×BOOL: gpu == gpu2 */ + else if (r.type == RES_TYPE_NUMERIC) + eq = (double)l.u.bval == r.u.num; /* BOOL×NUM: gpu == 1 */ + else + return -1; /* defensive: BOOL×STR rejected at parse time */ + break; default: return -1; } return (node->type == SEL_EQ) ? eq : !eq; } - default: return -1; + default: + /* SEL_ANY must NEVER reach this point — const-fold (§6.3) eliminates + * all SEL_ANY nodes before any eval_select call. If SEL_ANY is seen + * here, a tree was constructed without running const-fold (e.g. a unit + * test that manually builds a tree). Emit a diagnostic to catch this + * during development: + * if (node->type == SEL_ANY) log_fatal("SEL_ANY reached eval — const-fold not applied"); + * The default return -1 still fires, but the log_fatal makes the bug + * visible instead of silently returning non-match for every host. */ + return -1; } } ``` @@ -821,32 +1849,163 @@ static int eval_select_inner(const xce_select_node_t* node, both operands of EQ/NE are type-compatible: either both numeric, both string, or a RESNAME paired with a literal whose type matches the resource's declared type. See §7 for type coercion rules. -- **Runtime type safety:** In debug builds, `eval_select` should include - `assert()` checks on operand types for `SEL_GT`/`SEL_LT`/`SEL_GE`/`SEL_LE` - branches to verify both operands are `RES_TYPE_NUMERIC`. In production - builds, these branches return `-1` on type mismatch as a defensive fallback, - since parse-time validation should prevent such cases from reaching eval. +- **Boolean normalization in EQ/NE (intentional semantic):** When a + boolean resource (`SEL_RESNAME(BOOL)`) is compared to a numeric + literal, `host_get_bool` normalizes the resource's raw float value to + 0 or 1 (any nonzero → 1). This means `gpu == 2` compares `1 == 2` + (false, even if the underlying float is 2.0). This is intentional: + boolean semantics define "presence/absence" (0 or nonzero), not a + specific numeric value. If a user writes `gpu == 2`, they intend a + boolean check against a value that happens to be 2 — the correct form + is `gpu == 1` (or the standalone boolean idiom `gpu`). This behavior + is documented here; no runtime warning is emitted for `gpu == 2`. +- **Commutativity and mixed-type dispatch:** Commutativity canonicalization + in `makeSelectNode` may swap operands so the larger-typed operand is on + the left (or whichever ordering the key chooses). The `eval_select_inner` + EQ/NE dispatch switches on `l.type` — it **must** handle every mixed-type + pair in both orders (`NUM×BOOL` and `BOOL×NUM`, etc.) because the canonical + sort order is fixed at intern time and may place either type on the left. + Adding a new mixed-type pair (e.g., `STR×NUM`) requires adding symmetric + branches for both orderings. +- **Runtime type safety:** `eval_select` should guard operand types for + `SEL_GT`/`SEL_LT`/`SEL_GE`/`SEL_LE` branches. Use `log_fatal` (not + `assert()` — see §5.5) when both operands are not `RES_TYPE_NUMERIC`, since + parse-time validation (§7) should prevent this. As a belt-and-suspenders + fallback, return `-1` after the `log_fatal` call. ### 8.4 Thread Safety -- The intern table is protected by `sel_node_table.lock` for all - insert/remove operations. -- Once a node is in the table, its content is **read-only**. Concurrent - `eval_select` calls on the same node require no locking. -- **The Go CGO bridge must call `eval_select` from a single, pinned - goroutine** (the scheduler goroutine). `eval_select` accesses interned - nodes without holding references — if a concurrent goroutine triggered - GC sweep while another was evaluating, a use-after-free would occur. - The mark-and-sweep GC (§5.5) runs only between sessions when the - scheduler goroutine is not evaluating, so single-goroutine eval - guarantees safety. -- **Runtime enforcement:** The Go layer should include a runtime assertion - that panics if `EvalSelectForHost` is called from a goroutine other than - the one that initialized the C state. This catches misuse at - development time rather than producing silent data corruption in - production. If concurrent evaluation is needed in the future, - an eval-time reference count (increment before eval, decrement after) - must be added to the C API. +The scheduler runs a **two-thread pipeline**: + +``` +Job-picker thread Allocator thread +────────────────── ────────────────────────────────────── +Find highest-priority → eval_select (per job, per host) +job for scheduling Allocation (resource reservation) + Pre-session: staging merge, GC, recompaction +``` + +`eval_select`, allocation, and all pre-session work (staging merge, +GC, `recompact_node_ids`) run exclusively in the **allocator thread**. +The job-picker thread identifies candidate jobs; it does not call +`eval_select` or `makeSelectNode` during a session. + +**Allocator-thread-only invariant (CSE-enabled mode only):** The +lock-free eval reads in §8.4 and the lock-free GC/recompaction in §5.5 +both depend on the guarantee that only the allocator thread calls +`eval_select`, `makeSelectNode`, `gc_sweep_select_nodes`, and +`recompact_node_ids`. This invariant applies **only when +`sel_optimize_level > 0`** (CSE-enabled / scheduler). When +`sel_optimize_level == 0` (API server), the intern table is bypassed — +`makeSelectNode` allocates private (non-shared) node trees — so +concurrent API server goroutine calls are safe (§5.0). In CSE-enabled +mode, violating the invariant causes use-after-free (concurrent GC + +eval) or data races (concurrent makeSelectNode + eval without the intern +table lock). Phase 6 must enforce this with a runtime thread-ID check +in `eval_select` and `gc_sweep_select_nodes`. + +**GC single-thread guarantee:** `gc_mark_tree` writes the non-atomic +`gc_mark` field on interned nodes. This is safe because `gc_sweep_select_nodes` +is called exclusively from the allocator thread (a single-threaded context +— see §8.5.3 pre-session lifecycle). All `gc_mark` writes from a single +GC sweep are sequential; no second thread accesses `gc_mark` concurrently. +If the single-thread guarantee is ever relaxed, `gc_mark` must be made +`_Atomic unsigned` to prevent a C11 data race. + +**Pre-Phase-6 audit required:** Before implementing Phase 6, verify that +the scheduler's allocator thread is indeed the sole thread calling the +relevant functions. Run: +``` +grep -rn 'eval_select\|alloc_thread\|sched_thread\|sched_session' src/scheduler/ +``` +Confirm that `sched_meet_host_selection` and any `eval_select` call site +are reachable only from the allocator thread's execution path (not from +job-submission callbacks, timer threads, or signal handlers). If any +other call site is found, the lock-free design assumption is violated and +the design must be revised before Phase 6 begins. Document the audit +result (either "confirmed single-threaded" or "found unexpected call site +at [location]") in the Phase 6 commit message. + +```c +/* In eval_select (returns int): */ +if (sel_alloc_thread_id != 0 && + pthread_self() != sel_alloc_thread_id) { + log_fatal("eval_select called from non-allocator thread"); + return -1; +} + +/* In gc_sweep_select_nodes (returns void): */ +if (sel_alloc_thread_id != 0 && + pthread_self() != sel_alloc_thread_id) { + log_fatal("gc_sweep_select_nodes called from non-allocator thread"); + return; +} +``` + +`sel_alloc_thread_id` (declared as `static pthread_t` in `resreq.c`, +requires `#include `) is set once when the allocator thread +enters its first session loop iteration (Phase 6 wiring). A value of 0 +(the zero-initialized default for `pthread_t` on Linux) means the thread +has not been registered yet — the check is skipped during Phase 1–5 +unit testing where no scheduler threads exist. This mirrors the +`sel_optimize_level` write-once pattern (§5.0). + +- **Intern table lock:** `sel_node_table.lock` protects all insert/remove + operations. New job resreq strings are parsed (calling `makeSelectNode` + under lock) in the staging phase before the allocator begins a session. + During the session itself, the allocator reads interned nodes without + holding any lock — interned nodes are read-only once inserted (§5.1). +- **Lock-free eval reads:** Within a session, `eval_select` accesses + interned nodes without locking. No concurrent writer exists during a + session — staging writes go to a separate buffer merged at the + session boundary, and the job-picker thread does not write to the + intern table. +- **GC idle invariant:** `gc_sweep_select_nodes()` runs in the + pre-session phase, after staging merge. At that point both the + job-picker and allocator threads are idle — no `eval_select` or + `makeSelectNode` call is in progress. The use-after-free concern is + structurally impossible given this idle guarantee. See §5.5. +- **Lock ordering:** `sel_node_table.lock` is the **outer** lock. + `zarray_store` and `zarray_del` (called on `sel_node_table.nodes` while + `sel_node_table.lock` is held) acquire zarray's internal lock as the + **inner** lock. No code may acquire `sel_node_table.lock` while holding + a zarray internal lock — that would invert the ordering and risk + deadlock. All zarray calls on `sel_node_table.nodes` must be made with + `sel_node_table.lock` already held; direct zarray calls on that array + outside `makeSelectNode`, GC sweep, and `recompact_node_ids` are + prohibited. **Enforcement:** any new function that needs to call a + `zarray_*` function on `sel_node_table.nodes` must acquire + `sel_node_table.lock` first, OR must be restricted to the pre-session + idle phase (GC/recompaction pattern) with a comment citing the idle + invariant. Code review must reject any `zarray_*` call on + `sel_node_table.nodes` that has neither a surrounding lock nor an + idle-phase annotation. + **GC sweep exception:** §5.5 states that `gc_sweep_select_nodes` calls + `zarray_del` *without* holding `sel_node_table.lock`. This is not a + contradiction: the lock ordering rule above applies to concurrent + execution paths where mutual exclusion is needed. GC runs in the + pre-session idle phase when no concurrent `makeSelectNode` or `key_find` + calls exist — the lock is structurally unnecessary. The §5.5 guidance + ("do not add `mylock_lock`/`mylock_unlock` around GC's `key_del` calls") + is intentional: acquiring `sel_node_table.lock` in GC would add + unnecessary overhead and is omitted because the idle invariant makes + mutual exclusion unnecessary — `kvtab_del` has no internal lock + (verified in `kv.c:kvtab_del`), and while `zarray_del` does acquire its + own per-array internal lock (verified in `zarray.c:zarray_del`), it + never acquires `sel_node_table.lock`, so calling `zarray_del` without + first holding `sel_node_table.lock` is safe and consistent with the + outer→inner ordering rule. The runtime guards (§5.5 guard block) + enforce the idle invariant that makes lock-free GC correct. The + lock-ordering rule ('outer = `sel_node_table.lock`, inner = zarray lock') + applies only to concurrent execution paths; idle-phase code holds neither + outer lock at entry and may call `zarray_*` directly. + +The same lock-free exception applies to `recompact_node_ids`: it also +runs in the pre-session idle phase, calls `zarray_get` to iterate +`sel_node_table.nodes`, and mutates `node->node_id` directly without +acquiring `sel_node_table.lock`. The structural reason is identical — +no concurrent `makeSelectNode` or `key_find` calls exist during the +idle phase, so mutual exclusion is unnecessary. ### 8.5 Evaluation Memoization @@ -877,6 +2036,16 @@ typedef struct { uint8_t* bool_bits; /* bitmap, 2 bits per node */ uint32_t bool_bits_cap; /* capacity in nodes (not bytes) */ + /* node_count: snapshot of sel_node_next_id captured at session start by + * select_eval_ctx_init. select_eval_ctx_reset uses this value — NOT a + * live re-read of sel_node_next_id — to compute the memset byte count: + * memset(bool_bits, 0, (node_count * 2 + 7) / 8) + * Invariant: node_count <= bool_bits_cap (enforced by pre-session capacity + * check). Stale entries in bool_bits[node_count .. bool_bits_cap-1] are + * never accessed during the session because staging is suspended and no + * active node has an ID >= node_count. */ + uint32_t node_count; + /* Numeric leaf cache: only populated for SEL_RESNAME(numeric) and SEL_NUM. * Indexed by node_id. Unused entries are 0.0 (which is a valid numeric * value, but leaf nodes are always evaluated before use, so the slot @@ -901,14 +2070,25 @@ at intern time. The evaluation context arrays are sized to #define EVAL_FALSE 2 /* node evaluates to false */ #define EVAL_ERROR 3 /* node evaluates to error */ +/* bool_bits reset relies on memset-to-0 == EVAL_NONE; enforce at compile time */ +_Static_assert(EVAL_NONE == 0, "bool_bits reset via memset(0) requires EVAL_NONE==0"); + static inline int eval_get_bool(const select_eval_ctx_t* ctx, uint32_t node_id) { + if (node_id >= ctx->bool_bits_cap) { + log_fatal("eval_get_bool: node_id %u >= bool_bits_cap %u", node_id, ctx->bool_bits_cap); + return EVAL_ERROR; + } uint32_t bit_off = node_id * 2; return (ctx->bool_bits[bit_off / 8] >> (bit_off % 8)) & 0x3; } static inline void eval_set_bool(select_eval_ctx_t* ctx, uint32_t node_id, int val) { + if (node_id >= ctx->bool_bits_cap) { + log_fatal("eval_set_bool: node_id %u >= bool_bits_cap %u", node_id, ctx->bool_bits_cap); + return; + } uint32_t bit_off = node_id * 2; uint32_t byte = bit_off / 8; uint32_t shift = bit_off % 8; @@ -922,6 +2102,7 @@ static inline void eval_set_bool(select_eval_ctx_t* ctx, uint32_t node_id, int v ```c int eval_select(const xce_select_node_t* node, select_eval_ctx_t* ctx) { + if (node == NULL) return 1; /* no select clause — all hosts match */ /* check cache — O(1) bitmap lookup */ int cached = eval_get_bool(ctx, node->node_id); if (cached != EVAL_NONE) @@ -930,7 +2111,12 @@ int eval_select(const xce_select_node_t* node, select_eval_ctx_t* ctx) int result = eval_select_inner(node, ctx); /* recursive eval */ - /* write cache */ + /* write cache — including EVAL_ERROR: errors are node-structural + * (type mismatch, absent resource), not job-context-dependent, so the + * same node will produce the same error for every host in the session. + * Caching EVAL_ERROR avoids re-evaluating a known-bad subtree on each + * host. The error code propagates correctly through AND/OR because + * eval_select returns -1 for EVAL_ERROR and the callers check lv < 0. */ eval_set_bool(ctx, node->node_id, result == 1 ? EVAL_TRUE : result == 0 ? EVAL_FALSE : EVAL_ERROR); @@ -944,49 +2130,234 @@ without the cache check — it is called only on cache miss. Leaf nodes (`SEL_RESNAME`, `SEL_NUM`, `SEL_STR`, `SEL_TRUE`, `SEL_FALSE`) write their results to the `num_vals` and `str_vals` arrays as well, so comparison operators can read cached leaf values without re-reading host -data. +data. **Implementation note:** the `eval_select_inner` code in §8.3 +calls `host_get_bool`/`host_get_float` and `get_leaf_value` directly for +leaf and comparison cases. The Phase 4e implementation must add +explicit writes to `ctx->num_vals[node->node_id]` and +`ctx->str_vals[node->node_id]` in the leaf-node branches of +`eval_select_inner`, and reads from these cached arrays in the +comparison-operator branches, to realize the memoization described +here. The code in §8.3 shows the host-access version for clarity; the +implementation will substitute cached-array access. + +**SEL_RESNAME leaf cache write (concrete sketch):** The `SEL_RESNAME` +branch in the Phase 4e `eval_select_inner` must write to **both** +`bool_bits` (via `eval_set_bool`) AND the typed leaf array +(`num_vals` or `str_vals`) so that comparison operators can read the +cached value without hitting live host data again: +```c +case SEL_RESNAME: { + int bool_result; + switch (node->u.resname.res_type) { + case RES_TYPE_BOOL: + bool_result = host_get_bool(ctx->host, node->u.resname.res_idx); + /* bool_bits written by eval_select wrapper — no extra write needed */ + break; + case RES_TYPE_NUMERIC: { + double v = host_get_float(ctx->host, node->u.resname.res_idx); + ctx->num_vals[node->node_id] = v; /* cache for comparison ops */ + bool_result = (v != 0.0); + break; + } + case RES_TYPE_STRING: { + const char* s = host_get_str(ctx->host, node->u.resname.res_idx); + ctx->str_vals[node->node_id] = s; /* cache for comparison ops */ + bool_result = -1; /* string standalone rejected at parse time (§7) */ + break; + } + default: bool_result = -1; + } + return bool_result; +} +``` +The comparison cases (`SEL_GT`, `SEL_LT`, `SEL_GE`, `SEL_LE`, `SEL_EQ`, +`SEL_NE`) then call `eval_select(left_leaf, ctx)` / `eval_select(right_leaf, ctx)` +(which cache-miss into the leaf branch above and write `num_vals`/`str_vals`), +then read `ctx->num_vals[left->node_id]` directly — never `get_leaf_value()`. + +**Design decision — separate `num_vals`/`str_vals` arrays (deliberate):** +An alternative is to encode leaf values in `bool_bits` (e.g., store a +union or add a `sel_val_t` array). The separate typed arrays are chosen +for three reasons: (1) they avoid a union-per-node overhead (24 B/node) +for the 80%+ of nodes that are boolean-type comparisons and never need a +leaf cache slot — those nodes only use 2 bits in `bool_bits`; (2) they +allow the comparison operator to read a typed slot directly without a +type switch; (3) they are consistent with the `host_eval_ctx_t.numvals`/ +`strvals` naming convention, making the data flow easy to follow. The +added complexity (two extra arrays + two cap fields) is justified by the +memory savings shown in §8.5.5. #### 8.5.3 Session Lifecycle ``` -┌─ Pre-session ──────────────────────────────────────────────────┐ -│ 1. Check max_node_id against eval context capacity │ -│ 2. If needed: grow bool_bits / num_vals / str_vals arrays │ -│ 3. If many nodes GC'd and eval context is oversized by 2x: │ -│ shrink arrays (optional, avoids unbounded growth) │ -└────────────────────────────────────────────────────────────────┘ +┌─ Staging + GC phase ──────────────────────────────────────────────────┐ +│ 1. Merge new jobs from staging buffer into live job set │ +│ 2. Parse new job resreq strings (calling makeSelectNode) │ +│ 3. Unlink completed jobs (primary->select->root = NULL) │ +│ At this point, all live jobs (old + newly arrived) are visible, │ +│ and no eval_select or makeSelectNode call is in progress. │ +│ *** MEMORY FENCE required here *** │ +│ atomic_thread_fence(memory_order_seq_cst) must be inserted after │ +│ all `primary->select->root = NULL` writes and before gc_sweep. │ +│ Without the fence, the CPU or compiler may reorder the NULL store │ +│ after the GC mark pass begins, causing gc_mark_tree to observe a │ +│ stale non-NULL root and retain a node that should be swept. │ +│ In C11: atomic_thread_fence(memory_order_seq_cst); │ +│ Equivalent: make `select->root` an _Atomic pointer and use │ +│ atomic_store_explicit(..., memory_order_release) for the unlink. │ +│ 4. Run gc_sweep_select_nodes(live_jobs, njobs) — mark pass visits │ +│ all live jobs including newly arrived ones, so nodes referenced │ +│ only by incoming jobs are correctly retained. │ +│ 5. Run recompact_node_ids() — reassign dense 0..N-1 IDs │ +│ After recompact: current_node_count = sel_node_next_id │ +│ = count of live interned nodes with dense IDs 0..N-1. │ +└───────────────────────────────────────────────────────────────────────┘ + ↓ +┌─ Pre-session capacity check ──────────────────────────────────────────┐ +│ Note: this snapshot happens AFTER staging + GC + recompact, so │ +│ current_node_count reflects only the live interned nodes. │ +│ │ +│ 1. Read sel_node_next_id → current_node_count │ +│ 2. If current_node_count > bool_bits_cap: │ +│ new_cap = next power-of-two >= current_node_count │ +│ old_bool_bytes = (bool_bits_cap * 2 + 7) / 8 │ +│ realloc bool_bits to (new_cap * 2 + 7) / 8 bytes │ +│ memset(bool_bits + old_bool_bytes, 0, new_bytes - old_bool_bytes) │ +│ /* REQUIRED: realloc does not zero extended bytes. Without this │ +│ * memset, nodes with IDs in old_cap..new_cap-1 have stale (non- │ +│ * zero) bool_bits in their first session, causing eval_select to │ +│ * return spurious EVAL_TRUE/FALSE cache hits for unvisited nodes. */ +│ realloc num_vals, str_vals to new_cap (calloc-style: zero entire │ +│ new array, not just the extension, since they are typed pointers/ │ +│ doubles — zeroing the extension alone is sufficient but zeroing │ +│ all is safer and the arrays are small relative to the bool_bits) │ +│ PARTIAL-OOM CONTRACT: all three realloc calls (bool_bits, │ +│ num_vals, str_vals) must be treated as an atomic unit. If any │ +│ one realloc fails after another has already succeeded, call │ +│ log_fatal — do not proceed with mismatched capacities. Proceeding │ +│ with bool_bits_cap != num_vals_cap would allow eval_select_inner │ +│ to write ctx->num_vals[node_id] beyond the allocated num_vals │ +│ array (no bounds guard at the write site; see BOUNDS NOTE below). │ +│ update bool_bits_cap, num_vals_cap, str_vals_cap atomically │ +│ (only after all three reallocs succeed) │ +│ │ +│ BOUNDS NOTE: eval_select_inner writes ctx->num_vals[node->node_id] │ +│ and ctx->str_vals[node->node_id] without an inline bounds check │ +│ (unlike eval_get_bool/eval_set_bool which guard with │ +│ node_id >= bool_bits_cap). Correctness depends entirely on the │ +│ pre-session capacity check having grown num_vals and str_vals to │ +│ at least sel_node_next_id entries before the session loop runs. │ +│ Implementation must ensure the capacity check runs and succeeds │ +│ before any eval_select_inner call in the session. │ +│ 3. If many nodes GC'd and eval context is oversized by 2x: │ +│ shrink to next power-of-two >= current_node_count │ +│ (optional, avoids unbounded growth) │ +└───────────────────────────────────────────────────────────────────────┘ ↓ ┌─ Scheduling session ─────────────────────────────────────────────┐ -│ for each host: │ -│ ctx = select_eval_ctx_for_host(host) /* allocate or reuse */ │ -│ memset(ctx->bool_bits, 0, bitmap_bytes(max_node_id + 1)) │ -│ /* num_vals and str_vals: no need to clear — │ -│ leaf resolution overwrites per-host before comparators │ -│ read them. Only bool_bits needs clearing because its │ -│ "not evaluated" state (EVAL_NONE) is represented by 0. */ │ +│ ctx = select_eval_ctx_init(max_node_id) /* allocate once */ │ +│ for each worker (xce_sched_worker_t*): │ +│ /* build_host_eval_ctx is scheduler-private (src/scheduler/) │ +│ * — not declared in resreq.h */ │ +│ host_ctx = build_host_eval_ctx(worker, num_res) /* → host_eval_ctx_t* */ │ +│ if host_ctx == NULL: log_error, continue │ +│ select_eval_ctx_reset(ctx, host_ctx) /* takes host_eval_ctx_t* */ │ +│ /* select_eval_ctx_reset clears bool_bits with: │ +│ * memset(ctx->bool_bits, 0, │ +│ * (ctx->node_count * 2 + 7) / 8) │ +│ * where ctx->node_count is a SNAPSHOT captured at session │ +│ * start (select_eval_ctx_init stores sel_node_next_id into │ +│ * ctx->node_count and MUST NOT re-read sel_node_next_id on │ +│ * each host reset). Re-reading would be wrong if any code │ +│ * path increments sel_node_next_id during the session (e.g. │ +│ * a mis-routed staging call), causing memset byte count to │ +│ * exceed bool_bits_cap and overflow the allocation. │ +│ * The byte formula accounts for 2 bits/node. │ +│ * Must check (not assert — see §5.5): │ +│ * if (ctx->bool_bits_cap < current_node_count) │ +│ * log_fatal("bool_bits_cap too small"); │ +│ * num_vals and str_vals: no need to clear between hosts. │ +│ * Correctness proof: (1) bool_bits[comparator_id] is │ +│ * EVAL_NONE after per-host reset, forcing eval_select_inner │ +│ * to run for every comparator node; (2) eval_select_inner │ +│ * evaluates leaf children (SEL_RESNAME/SEL_NUM/SEL_STR) via │ +│ * eval_select BEFORE reading str_vals/num_vals — those leaf │ +│ * nodes have their own EVAL_NONE bool_bits entries which │ +│ * force a fresh host read and write to str_vals/num_vals; │ +│ * (3) therefore every str_vals[leaf_id] and num_vals[leaf_id]│ +│ * is overwritten for the current host before any comparator │ +│ * reads it, regardless of short-circuit paths. Only │ +│ * bool_bits needs clearing because EVAL_NONE == 0. */ │ │ for each pending_job: │ -│ rc = eval_select(job->select_root, ctx) │ +│ rc = eval_select(job->resreq->u.primary->select->root, ctx) │ │ if rc < 0: log error, skip host │ │ if rc == 0: skip host (select filter failed) │ │ if rc == 1: proceed to rusage check │ -│ select_eval_ctx_discard(ctx) /* free per-host context */ │ +│ /* free_host_eval_ctx is scheduler-private (src/scheduler/) */ │ +│ free_host_eval_ctx(host_ctx) /* dereferences str refcounts */ │ +│ select_eval_ctx_fini(ctx) /* free session context after last host */ │ └──────────────────────────────────────────────────────────────────┘ - ↓ -┌─ Post-session ──────────────────────────────────────────────────┐ -│ 1. Unlink completed jobs (primary->select->root = NULL) │ -│ 2. Run gc_sweep_select_nodes() │ -│ 3. Re-allocate node IDs: linear scan of intern table, │ -│ assign 0..N-1 (dense after GC) │ -│ 4. Adjust eval context for next session capacity check │ -└─────────────────────────────────────────────────────────────────┘ ``` -**Per-host allocation and discard:** Each host gets its own -`select_eval_ctx_t` that is allocated when the scheduler begins -evaluating jobs against that host, shared across all jobs for that host -within the session, and freed when the host's evaluation is complete. -This means the context does **not** persist between hosts — the next -host gets a fresh (or reused) context with `bool_bits` cleared. +**Session-level arena allocation:** `select_eval_ctx_init` allocates the +eval context once at session start, sized to `max_node_id + 1`. Between +hosts, `select_eval_ctx_reset` clears `bool_bits` and updates the host +pointer — `num_vals` and `str_vals` are overwritten per-host before +comparators read them, so no clearing is needed. This avoids +`malloc`/`free` per host (10K hosts/session would otherwise cost 10K +allocation/deallocation pairs). The arena is freed via +`select_eval_ctx_fini` at session end and potentially reallocated at the +next session boundary after `recompact_node_ids` adjusts the +`max_node_id`. + +**Ownership lifecycle contract:** The `select_eval_ctx_t` and +`host_eval_ctx_t` have a strict parent-child ownership relationship +within a session: + +- `select_eval_ctx_t` (session-scoped): allocated by + `select_eval_ctx_init` before the host loop, freed by + `select_eval_ctx_fini` after the host loop. It owns the + `bool_bits`, `num_vals`, and `str_vals` arrays. It does **not** own + the `host` pointer — `host` is a borrowed reference that is valid only + for the duration of a single per-host eval pass (between + `select_eval_ctx_reset` and the next reset or fini). +- `host_eval_ctx_t` (per-host scoped): allocated by + `build_host_eval_ctx` (**scheduler-private** — not declared in + `resreq.h`; lives in `src/scheduler/`) before each host's eval pass, + freed by `free_host_eval_ctx` (**scheduler-private**) after each + host's eval pass. It owns the `numvals` and `strvals` arrays (and + their xstring refcounts). The `select_eval_ctx_t.host` pointer + borrows this struct — it must not be accessed after + `free_host_eval_ctx` is called. + +**Dangling pointer prevention:** The session loop pseudocode in §8.5.3 +ensures `free_host_eval_ctx` is called after the per-host inner loop, +before the next host's `build_host_eval_ctx`. The +`select_eval_ctx_t.host` pointer is stale after `free_host_eval_ctx` +and must not be dereferenced until `select_eval_ctx_reset` sets it to +the new host context. `select_eval_ctx_fini` does not dereference +`host` — it only frees the owned arrays. + +**Refcount semantics of string fields (design decision):** +`select_eval_ctx_t.str_vals` and `host_eval_ctx_t.strvals` both hold +`const char*` pointers to xstring-interned strings, but their refcount +contracts differ deliberately: + +- `host_eval_ctx_t.strvals[i]`: each non-NULL entry was acquired via + `makeString()` in `build_host_eval_ctx`, which increments the xstring + refcount. `free_host_eval_ctx` must call `dereference(strvals[i])` for + each non-NULL entry to release those refcounts. +- `select_eval_ctx_t.str_vals[i]`: these are **non-owning cached pointers** + written by `eval_select_inner` and read by comparison operators within + the same per-host eval pass. They are overwritten (not dereferenced) at + the next `select_eval_ctx_reset` call. No `dereference` is called on + `str_vals` entries — the refcount is held by the corresponding + `host_eval_ctx_t.strvals` entry, which outlives the eval pass. Calling + `dereference` on `str_vals` would double-release the same refcount. + +This design avoids an extra `makeString`/`dereference` round-trip per +leaf evaluation while keeping the xstring reference stable for the full +eval pass duration. #### 8.5.4 Node ID Re-compaction After GC @@ -994,17 +2365,37 @@ After GC removes unreachable nodes, the remaining nodes have sparse IDs. Compacting to `0..N-1` keeps the eval context arrays dense: ```c -/* Run after gc_sweep_select_nodes() */ +/* Run after gc_sweep_select_nodes() — iterates the linear zarray, not the kvtab. + * + * Rationale for recompaction: (1) keeps eval context arrays dense so per-host + * allocation is bounded by the live node count; (2) prevents uint32_t node_id + * overflow in long-running scheduler instances where sel_node_next_id would + * otherwise grow without bound across thousands of GC cycles. + * + * IMPORTANT: node_id and zarray slot index are intentionally decoupled. + * node_id is a dense 0..N-1 index used exclusively for select_eval_ctx_t + * arrays (bool_bits, num_vals, str_vals). The zarray stores node pointers in + * arbitrary-order slots with possible gaps (NULLs from zarray_del). The + * zarray slot index must NOT be used as an index into eval context arrays. */ static void recompact_node_ids(void) { uint32_t next_id = 0; - kvtab_iter_t* iter = iter_make(sel_node_table.table); - for (void* val; iter_next(iter, NULL, &val); ) { - xce_select_node_t* node = (xce_select_node_t*)val; + /* Iterate all allocated slots (caps), not just live count. zarray_del + * NULLs slots without swapping, so gaps may exist after GC. + * + * zarray_get contract: returns NULL for both (a) slots vacated by + * zarray_del (NULLed in place, index < count) and (b) indices in the + * range count..caps-1 (allocated but never written, initialized to zero + * by xcealloc/calloc). Both cases are handled by the NULL-continue below. + * Verify this contract holds in src/lib/zarray.c before relying on it. */ + int caps = sel_node_table.nodes.caps; + for (int i = 0; i < caps; i++) { + xce_select_node_t* node = + (xce_select_node_t*)zarray_get(&sel_node_table.nodes, i); + if (node == NULL) continue; /* vacated or unwritten slot — skip */ node->node_id = next_id++; } - iter_free(iter); - sel_node_next_id = next_id; /* reset counter for new allocations */ + atomic_store_explicit(&sel_node_next_id, next_id, memory_order_release); /* reset counter for new allocations */ } ``` @@ -1028,10 +2419,11 @@ only 2 bits instead of 24 bytes. #### 8.5.6 Thread Safety for Memoization The `select_eval_ctx_t` is per-host and per-session. It is not shared -between hosts or between sessions. Since the scheduler goroutine -processes one host at a time (§8.4), no concurrent access to the same -context occurs. The `bool_bits` bitmap is cleared per host with `memset`, -which is safe because no other goroutine accesses it concurrently. +between hosts or between sessions. The allocator thread processes one +host at a time within a session (§8.4), so no concurrent access to the +same context occurs. The `bool_bits` bitmap is cleared per host with +`memset`, which is safe because the allocator thread is the only writer +during a session. --- @@ -1050,38 +2442,20 @@ the AST node level rather than string-level interning. ### 9.2 Resreq Sharing is Scoped to (user, queue, project) -Two jobs share the same `xce_resreq_t*` pointer if and only if they were -submitted with the same user, same queue, same project, and the same resreq -string. The scoping is necessary because `rusage` clauses involve per-user -and per-queue allocation constraints — the same rusage string may result in -different allocation behavior for different users or queues. +> **Future work — not part of the current implementation plan (§11).** -The sharing mechanism is a second intern table at the resreq level, -modelled on `xstring.c`: - -``` -makeResreq(canonical_resreq_str) - → lookup in resreq_intern_table - → cache hit: addref, return existing xce_resreq_t* - → cache miss: parse_resreq() → intern each select node (CSE) → insert → return -``` +A second-level intern table for full `xce_resreq_t` objects (scoped to +`(user, queue, project, resreq_string)`) is a natural follow-on once +select CSE is validated in production; design it when the need is confirmed. ### 9.3 Composability of the Two Levels -Because `makeSelectNode` interns individual AST nodes, jobs that share only a -**subset** of their resreq (e.g., same `select[...]` but different `rusage[...]`) -still share the common select subtree. The sharing is structural, not just -per-string. The two levels compose: +> **Future work — depends on §9.2.** -- **Node-level CSE** (§5): global — `type==linux` is one interned subtree - used by all jobs. -- **Resreq-level sharing** (§9.2): scoped to `(user, queue, project, resreq_string)` - — because rusage clauses involve allocation constraints that differ per - user/queue/project. - -The resreq intern table key is the **canonical resreq string** (raw, after -whitespace normalization). A future enhancement could normalize clause order -(e.g., always `select[...]` before `rusage[...]`) to improve hit rates. +Node-level CSE (§5) and resreq-level sharing (§9.2) compose naturally: +`makeSelectNode` interns individual AST nodes so jobs sharing only a subset +of their resreq still share the common select subtree — design details +belong in the §9.2 follow-on work. --- @@ -1090,20 +2464,20 @@ whitespace normalization). A future enhancement could normalize clause order Within a single scheduling session: ``` +ctx = select_eval_ctx_init(max_node_id) /* allocate once per session */ for each candidate_host: - ctx = select_eval_ctx_for_host(host) /* allocate per-host eval context */ - memset(ctx->bool_bits, 0, ...) /* clear memoization cache */ + select_eval_ctx_reset(ctx, host) /* clear bool_bits, update host ptr */ for each pending_job: - 1. rc = eval_select(job.resreq.primary.select, ctx) + 1. rc = eval_select(job->resreq->u.primary->select->root, ctx) → 1 (true): proceed to rusage check → 0 (false): skip host (select filter failed) → -1 (error): log error, skip host — do NOT silently treat as false 2. can_satisfy_rusage(job.resreq.primary.rusages, host.available) → false: skip host (insufficient resources) 3. allocate(job, host) → mark job as dispatched, update host.available - select_eval_ctx_discard(ctx) /* free per-host context */ if no_host_matched_for_job: job remains unsatisfied, re-evaluated next session +select_eval_ctx_fini(ctx) /* free session context after last host */ ``` **Error handling:** `eval_select` returns `-1` for error (§8.3). The @@ -1150,16 +2524,41 @@ are made. - `lib/testsuite/test_select.c` - Parse: valid expressions, unknown resource names (expect error), type mismatches (numeric op on string resource, expect error). + - Parse: input exceeding `SELECT_MAX_INPUT_LEN` (expect error), nesting + exceeding `SELECT_MAX_DEPTH` (expect error). - CSE: `A && B` and `B && A` produce the same `xce_select_node_t*`. - CSE: two calls to `makeSelectNode` with same args return same pointer (same interned node). - Eval: numeric `>`, `<`, `==`; string `==`; bool standalone; `!`, `&&`, `||`; short-circuit evaluation. + - GC: after unlinking a job's select root, `gc_sweep_select_nodes` frees + the now-unreachable node and removes it from the intern table; a + subsequent `makeSelectNode` with the same args re-interns a fresh node. + - GC predicate functions: `sched_session_active` returns 1 while a session + is running and 0 otherwise; `sched_eval_in_progress` returns 1 only + inside the eval-select loop; `staging_merge_complete` returns 1 after + staging merge and 0 before. **Note:** Phase 1 tests the *interface + contract* of these predicates using a test-only scaffold, not the real + scheduler wiring. In Phase 1, these predicates are backed by a + **test-only `fake_scheduler_t` struct** (declared in `test_select.c`) + with the same three `_Atomic int` fields that Phase 6 will add to the + real `xce_scheduler_t` (see §5.5 "Predicate lifecycle" and Phase 6 + "Add GC predicate fields"). The Phase 1 stub functions wrap the fake + struct; Phase 6 replaces them with wrappers around the real struct. + No Phase 1 stub code leaks into the production implementation — the + predicate functions are thin wrappers whose bodies change in Phase 6, + not the test fixtures. The predicate *signatures* and *semantics* + are stable across all phases; only the backing struct changes. *Tests are written as skeletons/stubs in Phase 1. They compile but fail until Phase 4 provides the implementation. The TDD contract is: test signatures and assertions are defined before implementation code.* +*Note on sequencing: Phase 3 (PEG grammar) defines the exact syntax used in +test input strings. Implementers should review or draft Phase 3 before +finalizing Phase 1 test cases, or develop Phase 1 and Phase 3 concurrently. +The grammar is the test oracle for all parse-level assertions.* + #### Phase 2: Header definitions - `include/resreq.h` @@ -1167,15 +2566,119 @@ signatures and assertions are defined before implementation code.* struct. - Fill `struct xce_resreq_select_` with `xce_select_node_t* root`. - Declare `makeSelectNode`, `eval_select`, `eval_select_inner`, - `select_eval_ctx_for_host`, `select_eval_ctx_discard`, + `select_eval_ctx_init`, `select_eval_ctx_reset`, `select_eval_ctx_fini`, `sel_node_table_init`, `sel_node_table_fini`, `gc_sweep_select_nodes`, - `gc_mark_tree`, `recompact_node_ids`. - - Add `res_type_to_valtype` / `valtype_to_res_type` mapping helpers. - - Add `parse_resreq_primary_with_resmgr(primary, resreqString, errmsg, resmgr)` - as a new entry point. The existing `parse_resreq_primary` calls it - with `resmgr=NULL` (no previous release, so backward compatibility is - not a concern — the existing function can be updated directly if - preferred). + `recompact_node_ids`. + (`gc_mark_tree` is a `static` helper in `resreq.c` — §5.5 — and must + NOT be declared in the public header.) + - `gc_sweep_select_nodes` signature: + `void gc_sweep_select_nodes(const xce_sched_job_t** live_jobs, size_t njobs);` + (`xce_sched_job_t` has `xce_resreq_t* resreq`; `xce_job_t` has only + `char* resreq` and cannot reach the parsed AST.) + Caller: scheduler pre-session, after staging merge. Called after all job + cleanup (select root unlinks) and before `recompact_node_ids`. + Precondition: no `eval_select` call is in progress (GC idle invariant, §5.5). + - Add `var resmgr` to `resreq_context_t` (the PEG auxil struct declared in + `include/resreq.h`): this field threads the resmgr through the outer + resreq parse context so the select sub-parser receives it via + `auxil->resmgr`. (`select_context_t`, the auxil for `xce_select.peg`, + already carries `var resmgr` — see §12 Appendix.) + - Add `valtype_to_res_type` mapping helper. + **Scope decision:** `valtype_to_res_type` is used in the PEG `resname` + action to convert the resmgr's `XCE_VAL_*` value into `res_type_t` + when populating each `SEL_RESNAME` node — include it. The inverse + function `res_type_to_valtype` has no current consumer (the core eval + path never re-derives a valtype from `res_type_t`) — **omit it** to + avoid dead code. If a future diagnostics or type-error-reporting + feature needs it, add it then (YAGNI). The `res_type_to_valtype` + implementation at §4 line 326 serves as documentation of the mapping + semantics for reference but should not be compiled into the + production build without a consumer. + - Add `xce_str_match` as a `static inline` helper: + ```c + static inline int xce_str_match(const char* a, const char* b) { + return strcmp(a, b) == 0; + } + ``` + **Scope decision:** wrapping `strcmp` rather than calling it directly is + a forward-looking choice — it lets wildcard or glob semantics be added + later by changing one function body, without touching every call site in + `eval_select_inner`. IBM LSF's select clause supports `model==sun4*` + glob patterns, and XCE may need the same. This is a deliberate + abstraction, not speculative generality. + - Update `parse_resreq_primary` to accept a `resmgr` parameter + (required, never NULL). This is a new product with no prior release, + so the interface is updated directly — no backward-compatibility shim + or separate entry point is needed. **CGO binding audit (mandatory + before changing the signature):** The Go API server must NOT have + a CGO binding to `parse_resreq` — if it does, a signature change + here breaks the Go build silently at link time (CGO type errors are + not always caught at compile time). Verify before proceeding: + ``` + grep -rn 'parse_resreq\|C\.parse_resreq' src/frontend/ + ``` + Expected result: no matches. If matches exist, update the CGO + wrapper in the same PR. Before updating, find all C callers: + ``` + grep -rn 'parse_resreq\b\|parse_resreq_primary\b' src/ + ``` + Known call sites to update (verify against grep output — there may be more): + 1. `src/lib/xce_resreq.peg` PEG action — add `resmgr` field to + `resreq_context_t` (the auxil type) and pass `auxil->resmgr` to + `parse_resreq_primary`. The resmgr must also propagate through + the outer `parse_resreq()` entry point and `parse_section()` — + the full call chain is: `make_sched_job → parse_resreq(resmgr, ...) → + parse_section → parse_select(ctx_with_resmgr) → makeSelectNode`. + 2. `src/scheduler/src/schedobj.c` — `make_sched_job` line 171 calls + `parse_resreq(job->resreq, j->submit->resreq, &errmsg)` with two + args; this must be updated to the new form that includes `resmgr`. + `make_sched_job` receives a `void*` pointer castable to + `xce_scheduler_t*`; access `sched->resmgr` and thread it into + the resreq parse context. + 3. `src/lib/testsuite/test_select.c` (Phase 1) — unit tests must + construct a minimal resmgr fixture (a small in-memory resource + table mapping test resource names to types) before calling any + parse function. **Shared helper location:** Do NOT define + `make_test_resmgr()` inside `test_select.c` — both + `test_select.c` and `test_resreq.c` need it, and duplication + leads to drift. Instead, create a new shared helper: + - **`src/lib/testsuite/test_resmgr_fixture.h`** — declares + `var make_test_resmgr(void)` and `void free_test_resmgr(var)` + - **`src/lib/testsuite/test_resmgr_fixture.c`** — implements + them (creates an in-memory resmgr with a small fixed set of + test resources: one bool, one numeric, one string) + Both `test_select.c` and `test_resreq.c` `#include` the header + and link against the `.c` file. Add + `test_resmgr_fixture.c` to the testsuite `executable()` sources + in `lib/meson.build`. + 4. `src/lib/testsuite/test_resreq.c` — existing test call sites that + reach `parse_resreq_primary` must be updated to pass a resmgr. + The current signatures and their updated forms: + ```c + /* Current (before Phase 2): */ + int parse_resreq_primary(xce_primary_resreq_t* primary, + char* resreqString, char** errmsg); + int parse_resreq(xce_resreq_t* resreq, char* data, char** errmsg); + + /* Updated (Phase 2 + 4b): */ + int parse_resreq_primary(xce_primary_resreq_t* primary, + char* resreqString, var resmgr, + char** errmsg); + int parse_resreq(xce_resreq_t* resreq, char* data, + var resmgr, char** errmsg); + ``` + `resmgr` is placed into `resreq_context_t` before invoking the + packcc parse runner: + ```c + resreq_context_t ctx = { 0 }; + ctx.resmgr = resmgr; /* thread resmgr into PEG auxil */ + /* ... pxc_xce_resreq_create(&ctx) / pxc_xce_resreq_parse() ... */ + ``` + The three call sites in `test_resreq.c` (current lines 244, 301, 330) + each call `parse_resreq(&resreq, resreqString, &errmsg)` and must + become `parse_resreq(&resreq, resreqString, test_resmgr, &errmsg)`, + where `test_resmgr` is the fixture from the shared + `test_resmgr_fixture.h` helper (see item 3 above). #### Phase 3: PEG grammar @@ -1184,72 +2687,621 @@ signatures and assertions are defined before implementation code.* - `select_context_t` auxil with `resmgr` pointer. - Each action calls `makeSelectNode` with canonicalized operands. - Type validation in comparison actions. + - **Clause-keyword conflict check in `resname` action (§3.1 runtime enforcement):** + After the identifier is captured, the `resname` PEG action must check + it against the clause-keyword list (`select`, `rusage`, `span`, `same`, + `affinity`) and emit a parse error if there is a match — see §3.1 for + the full code snippet. This is a Phase 3 deliverable, not deferred to + Phase 4b; the check lives in the PEG action, not in `resreq.c`. + - **Implicit vs explicit select dispatch:** `xce_select.peg` handles + only the inner expression grammar. The implicit/explicit dispatch + happens in the existing `section.peg`, which already extracts + `selectString` from both forms (§2: `select_clause ::= + 'select' '[' select_expr ']' | select_expr`). `parse_select()` + receives the extracted `selectString` regardless of which form was + used — the PEG grammar does not need to handle the `select[...]` + wrapper or the clause-keyword boundary detection. The implicit + form's boundary detection (recognizing `rusage`, `span`, etc. as + clause boundaries) is implemented in `section.peg`. **Audit + complete (bug confirmed and fixed):** `section.peg:50` (the second + alternative of `select_statement`) previously read: + ```peg + / (!("span[" "]" / "]") .)* {$$ = strdup($0); } + ``` + This stopped on the two-token sequence `span[]` (empty span), NOT on + `span[` alone — unlike the correct pattern in `usage_statement` + (line 58): `(!("span[" / "]") .)*`. The bug caused a select + statement preceding a non-empty `span[...]` clause to consume the + span content as part of `selectString`, silently corrupting + `spanString`. The fix has been applied to `section.peg:50`: + ```peg + / (!("span[" / "]") .)* {$$ = strdup($0); } + ``` + This is a confirmed correctness fix; no further audit is required. #### Phase 4: Implementation - `lib/resreq.c` + +**ABI audit (before starting 4a):** `struct xce_resreq_select_` changes from +zero-size to `{ xce_select_node_t* root; }`. Before writing any Phase 4 code, +run both: +``` +grep -rn 'xce_select_t\|xce_resreq_select_' src/ +grep -rn 'sizeof.*xce_select_t' src/ +``` +Verify no caller holds it by value (embedded struct, stack copy, serialized). +The `sizeof` search catches size-dependent allocation or serialization that +the name search alone would miss. Convert any by-value sites to +pointer-indirect access before proceeding. +**Zero-size-to-nonzero-size transition risk:** The current `struct +xce_resreq_select_` has size 0 in C99 (implementation-defined; may be +0 or 1 depending on compiler). Any call to `XCEMALLOC(sizeof(xce_select_t))` +in the current codebase that allocates the struct by value will +under-allocate after the struct gains an 8-byte pointer field. The ABI +grep above must find and fix these sites before the struct definition is +changed. Most callers use a pointer-to-struct pattern (`primary->select` +is a `xce_select_t*`) so the risk is confined to explicit `sizeof` +allocations — but these must be found and updated. + +**4a. Core node allocation and interning (§5.2–5.4)** - Add `#include "kv.h"` (required for `kv_key_ops_t`; not transitively included via `kvmgr.h`). - `sel_node_table` static with `mylock_t`. - `sel_key_ops_t` for `sel_node_key_t` (hash, cmp, assign, destroy). + - `sel_make_key()` constructor (§12 Appendix). - `sel_node_table_init(int nslots)` / `sel_node_table_fini()`. - - `makeSelectNode` (§5.4) — with key_add return code check for TOCTOU - safety. - - `sel_key_of`, `sel_key_hash` helpers. - - `eval_select` / `eval_select_inner` (§8.3/§8.5.2) — memoized evaluation - with per-host `select_eval_ctx_t`, explicit error propagation. - - `select_eval_ctx_for_host`, `select_eval_ctx_discard` — per-host context - allocation/discard (§8.5.3). + `sel_node_table_init` allocates `sel_node_table.table` via + `makeInstance(&kvtab_type, ¶ms)` (the same public API used by + `xstring.c`; `kvtab_init` is an RTTI-internal callback, not a public + constructor) and initializes `sel_node_table.nodes` via + `zarray_init(&sel_node_table.nodes, delta, NULL)` (embedded linear + array for GC/recompaction iteration — all existing `zarray_t` usage in + the codebase uses embedded structs initialized with `zarray_init`, not + `zarray_new()`), sets `datakiller=NULL` on the kvtab (§5.5 sweep frees + nodes explicitly, not via kvtab callback), and initializes + `sel_node_next_id = 0`. **`sel_node_next_id` memory model:** declare + as `static _Atomic uint32_t` (per §5.3). All writes use + `atomic_store_explicit(..., memory_order_release)` and the + pre-session read in `select_eval_ctx_init` uses + `atomic_load_explicit(..., memory_order_acquire)` to prevent the + compiler or CPU from reordering the final `makeSelectNode` store past + the `select_eval_ctx_init` read. + **`datakiller=NULL` invariant:** `kv.c` fires `val_killer` + unconditionally on `key_del` if set, which would double-free nodes freed + explicitly by GC sweep. This is enforced by code convention — there is + no runtime assertion because `kvtab_t` is defined only inside `kv.c` + (an opaque type not exported in `kv.h`), so `sel_node_table.table->datakiller` + is not accessible outside `kv.c`. The `params` struct passed to + `makeInstance(&kvtab_type, ¶ms)` MUST either zero-initialize `datakiller` + or set it explicitly to NULL — a non-NULL `datakiller` in `params` is a + latent double-free bug that no runtime check can catch. The params literal + MUST carry a prominent comment to prevent future readers from accidentally + adding a datakiller callback: + ```c + kv_params_t params = { + .nslots = nslots, + .keyops = &sel_node_key_ops, /* field is 'keyops', not 'key_ops' — see kv.h */ + /* datakiller MUST remain NULL. GC sweep frees nodes explicitly via + * free_select_node() after key_del. A non-NULL datakiller would + * cause key_del to free the node a second time — silent double-free. */ + .datakiller = NULL, + }; + sel_node_table.table = makeInstance(&kvtab_type, ¶ms); + ``` + When `sel_optimize_level == 0`, both initializations are skipped (§5.0). + - `makeSelectNode` (§5.4) — with `sel_make_key()`, key_add return code + check for TOCTOU safety, and node_id assignment. + - `sel_key_hash` helper (§12 Appendix). + - Fix `makeString()` TOCTOU in `src/lib/xstring.c` (§5.4): on `key_add` + returning `-EEXIST` **only**, re-find the existing string **while still + holding the intern table lock** and return it, rather than returning NULL. + The re-find must happen inside the lock (before `mylock_unlock`) — releasing + the lock before re-finding creates a window where the existing string could be + dereferenced and freed by a concurrent caller, yielding a dangling pointer. + This matches the `makeSelectNode` TOCTOU pattern in §5.4 step 5 exactly. + If `key_add` returns `-ENOMEM`, nothing was + inserted — re-finding would fail and returning NULL is correct. This + protects the numeric-to-string coercion in §7 under concurrent API server + goroutines, and all other `makeString` callers. + *(Scope note: this change touches `src/lib/xstring.c`, which is outside + the select-specific scope of §2. It is included here as an explicit + Phase 4a deliverable — a silent NULL return from `makeString` in the §7 + coercion path corrupts the interned AST and must be fixed before Phase 4b + begins. If change-management policy requires isolated file ownership, + track it as a numbered sub-task with its own PR, but do not treat it as + optional or defer it beyond Phase 4a. Completion criterion: the + concurrent stress test for `makeString` under TSan passes with no races.)* + +**4b. Parse-time type validation (§7)** + - **Phase gate: `makeString()` TOCTOU fix (4a) must be merged and tested + before any 4b code is written.** The §7 PEG coercion path calls + `makeString(buf)` under concurrent API server load; if `makeString` + returns NULL on `-EEXIST`, it corrupts the interned AST silently. + **Gate verification:** before starting 4b, confirm that + `src/lib/xstring.c:makeString` handles `-EEXIST` from `key_add` by + re-finding and returning the existing interned string (rather than + returning NULL), and that the existing `test_xstring.c` suite passes + with this change. The 4a fix must land in the same PR or an + immediately preceding PR — do not begin 4b implementation until the + gate is verified. + - Update `parse_resreq_primary` to call `parse_select` with resmgr. + **Critical ordering:** `parse_select(data.selectString, resmgr, + &primary->select, errmsg)` MUST be called **before** `kill_resreq_data(&data)`. + `kill_resreq_data` frees `data.selectString` (via `FREEUP(data->selectString)` + at `resreq.c:49`). A `parse_select` call after `kill_resreq_data` passes + a freed pointer — a use-after-free bug. The correct insertion point is + after `parse_rusage` (and all other `parse_*` calls that populate `data`) + and immediately before `kill_resreq_data(&data)`: + ```c + /* After all parse_* calls: */ + if (data.selectString) { + rc = parse_select(data.selectString, resmgr, + &primary->select, errmsg); + if (rc != 0) { kill_resreq_data(&data); return rc; } + } + kill_resreq_data(&data); /* frees data.selectString — must come after */ + ``` + - Update `kill_primary_resreq` to handle CSE mode conditionally: + - CSE-enabled (`sel_optimize_level > 0`): unlink select root only + (`primary->select->root = NULL`). GC between sessions reclaims + unreachable nodes. + - CSE-disabled (`sel_optimize_level == 0`): call `free_select_tree(root)` + to walk and free the node tree, then `FREEUP(primary->select)` to + free the `xce_select_t` container struct (§5.0). + - **`kill_primary_resreq` location:** `include/resreq.h` declares + `kill_resreq` (line 128) but does not currently export + `kill_primary_resreq` as a separate symbol. Before modifying, + locate the primary-cleanup call in `src/lib/xce_resreq.c` (inside + `kill_resreq`'s dispatch on `XCE_RESREQ_PRIMARY`) and update it + there, or extract it as a named static helper if the logic warrants it. + +**4b-locale. LC_NUMERIC enforcement (§7 locale note)** + - `resreq.c` must define `_XOPEN_SOURCE 700` (or a higher value) before + any system headers to expose `newlocale`, `uselocale`, and `freelocale` + from ``. These POSIX.1-2008 extensions are not visible + without this feature-test macro. + - In `parse_select`, wrap the PEG parser call (which executes the + `snprintf` numeric-to-string coercion in §7 PEG actions) with an + explicit C-locale guard. The API server (`sel_optimize_level == 0`) calls + `parse_resreq` from goroutines that may have non-C locale settings; + without this guard, `snprintf("%.17g", val)` produces a comma decimal + separator in European locales, corrupting string comparisons at eval time. + The scheduler already runs `LC_ALL=C`, but the guard is still correct + there (a no-op in practice): + ```c + locale_t cloc = newlocale(LC_NUMERIC_MASK, "C", NULL); + if (cloc == (locale_t)0) { + /* newlocale() failed. This must never happen on the scheduler: + * a locale-sensitive strtod/snprintf in the resreq parser silently + * produces wrong numeric values (e.g. "1,5" instead of "1.5" in + * European locales), corrupting job scheduling decisions. Abort. */ + if (sel_optimize_level > 0) { + log_fatal("newlocale(LC_NUMERIC_MASK, C) failed: %s — scheduler" + " cannot safely parse numeric resreq values", strerror(errno)); + return -1; /* unreachable: log_fatal terminates */ + } + /* API server: fail hard — a corrupt locale causes parse_select to accept + * '1,5' as a valid numeric literal in European locales, then store the + * wrong string in the interned node. eval_select_simple would later + * compare "1,5" against "1.5" and silently report no match for every + * host. Newlocale failure is not recoverable at request time. */ + log_error("newlocale(LC_NUMERIC_MASK, C) failed: %s — rejecting resreq " + "parse to prevent locale-corrupted numeric comparisons", + strerror(errno)); + return -1; + } + locale_t old = uselocale(cloc); + /* ... invoke PEG parser ... */ + uselocale(old); + freelocale(cloc); /* cloc is non-NULL here; never pass (locale_t)0 to freelocale */ + ``` + Do NOT call `freelocale((locale_t)0)` — passing 0/NULL to `freelocale` + is undefined behavior per POSIX. + - **strtod locale coverage (mandatory verification):** The packcc-generated + PEG scanner may call `strtod` internally to parse number literals at + scanner level — before any PEG action fires and before the `uselocale` + scope begins. Before closing Phase 4b, verify by inspecting the generated + `lib/xce_select.c`: + ```bash + grep -n 'strtod\|atof\|strtof' build/lib/xce_select.c + ``` + If packcc inserts a `strtod` call in the scanner (tokenizer), the + `uselocale` guard in `parse_select` does NOT cover it. In that case, + either: (a) set the C locale at process startup (acceptable for scheduler; + risky for API server CGO goroutines), or (b) store the raw matched number + string from the PEG `%0` capture and call `strtod` explicitly inside the + PEG action where the `uselocale` guard is already active, discarding any + packcc-generated `strtod` call. Option (b) is preferred for correctness. + If `grep` returns no matches, document the result as a confirmed safe + finding and close this item. + +**4c. Commutativity canonicalization and associativity flattening (§6.2)** + - Commutativity sort in `makeSelectNode` (already integrated in §5.4). + - Post-parse flatten pass for `&&`/`||` chains. + - Bottom-up re-intern via `makeSelectNode` after flattening. + +**4d. Const-fold transform (§6.3)** + - `type==any` → SEL_TRUE, `!(type==any)` → SEL_FALSE, and related + constant-propagation cases. + +**4e. Evaluation with memoization (§8.3/§8.5)** + - `eval_select` / `eval_select_inner` — memoized evaluation with + per-host `select_eval_ctx_t`, explicit error propagation, NULL root guard. + - `select_eval_ctx_init`, `select_eval_ctx_reset`, `select_eval_ctx_fini` + — session-level context allocation, per-host reset, session-end teardown + (§8.5.3). - `host_resval_typed`, `host_get_bool`, `host_get_float`, `host_get_string` accessors. - - Mark-and-sweep GC: `gc_sweep_select_nodes()`, `gc_mark_tree()`. - - Node ID assignment: `node_id` field assigned by `makeSelectNode` from - a monotonically increasing counter (`sel_node_next_id++`). + +**4f. Mark-and-sweep GC and recompaction (§5.5/§8.5.4)** + - `gc_sweep_select_nodes()` with resreq->tag dispatch for COMPOUND/ALTER. + - `gc_mark_tree()` helper. - `recompact_node_ids()` — reassign dense 0..N-1 IDs after GC (§8.5.4). - - AST transform pipeline: flatten, const-fold (§6.2–6.4). - - `xce_str_match` wrapper (initially `strcmp` semantics). - - Update `kill_primary_resreq` to unlink select root (`primary->select->root = NULL`) - before freeing the select struct. GC between sessions reclaims unreachable nodes. - - Update `parse_resreq_primary` to call `parse_select` with resmgr. #### Phase 5: Build wiring -- `lib/meson.build` — add `xce_select.peg` to the source list alongside - `xce_resreq.peg`, `section.peg`, `rusage.peg`. +- `lib/meson.build` — add a `custom_target` block for `xce_select.peg` + matching the `section_parser`, `rusage_parser`, and `xce_resreq_parser` + pattern already in the file (each invokes packcc to produce a `.c`/`.h` + pair), then add `xce_select_parser[0]` to `libxce_src`. Simply listing + `xce_select.peg` in the source array is insufficient — the PEG file must + be code-generated by packcc before it can be compiled. Example: + ```meson + xce_select_parser = custom_target( + 'xce select parser', + output: ['xce_select.c', 'xce_select.h'], + input: ['xce_select.peg'], + command: [packcc, '-l', '-o', 'lib/xce_select', '@INPUT@'], + build_by_default: true) + ``` + **`-o` path verification:** The `-o 'lib/xce_select'` argument mirrors the + established pattern in `lib/meson.build` for `section_parser` (`-o 'lib/section'`), + `rusage_parser` (`-o 'lib/rusage'`), and `xce_resreq_parser` (`-o 'lib/xce_resreq'`). + Before adding the new block, verify the pattern has not changed: + ``` + grep -A8 'custom_target' src/lib/meson.build | grep '\-o' + ``` + All three existing entries must show `'lib/'` — if the path format + has changed (e.g. to an absolute path or `@OUTDIR@` form), update the + `xce_select` entry to match. + Add `xce_select_parser[0]` (the generated `.c` file) to `libxce_src`. -### Go Integration (Phase 6) +### Scheduler Integration (Phase 6) -#### Phase 6: Go bridge +#### Phase 6: C scheduler wiring -- `backend/clib.go` — replace `EvalSelectForHost` stub with real CGO call. - Must marshal a `host_eval_ctx_t` from Go to C, populating both the - numeric resource vector (`numvals`) and the string resource table - (`strvals`, indexed by `res_idx`). The Go bridge then calls - `select_eval_ctx_for_host()` to create the per-host memoization context, - evaluates each job against it via `eval_select()`, and calls - `select_eval_ctx_discard()` when the host's evaluation is complete. - The `strvals` array maps `res_idx` to interned string pointers obtained - via `makeString()` for CSE compatibility. -- `controllers/hosts.go` — wire `FilterHostByResReqSelect` into - `preCheckHostRequest`. -- **GC trigger:** Add CGO calls invoked by the scheduler goroutine at the - end of each scheduling session, after all dispatch/defer decisions and - job cleanup (unlinking select roots) are complete: - 1. `C.gc_sweep_select_nodes()` — reclaim unreachable nodes. - 2. `C.recompact_node_ids()` — reassign dense 0..N-1 IDs after GC (§8.5.4). - This must be a well-defined session boundary — no `eval_select` calls - may be in progress during the sweep or recompaction. - Before the next session, check whether the eval context arrays need to - grow or shrink to accommodate the new `max_node_id` (§8.5.3). -- **CGO concurrency:** `FilterHostByResReqSelect` must be called from a - single, pinned scheduler goroutine (§8.4). This is a hard invariant: - `eval_select` accesses interned nodes without holding references, so - concurrent evaluation while GC sweep runs would cause use-after-free. - Add a runtime assertion in the Go layer that panics if - `EvalSelectForHost` is called from a goroutine other than the one that - initialized the C state. If concurrent scheduling is needed in the - future, eval-time reference counting (increment before eval, decrement - after) must be added to the C API. +The scheduler is a C process. No CGO bridge is required. Phase 6 +wires the select evaluator into the scheduler's existing session loop: + +- **Restructure the scheduler session loop** — the cross-job CSE + memoization benefit depends on a **single `select_eval_ctx_t` being + reused across all jobs for a given host** within a session. The + existing `sched_meet_host_selection(xce_scheduler_t*, xce_sched_worker_t*, + xce_select_t*)` is called once per (job, host) pair, which means each + invocation would construct and discard a fresh eval context, silently + losing the memoization. Phase 6 must restructure the allocator + thread's session loop to an **outer-host / inner-job** pattern: + + ``` + /* select_eval_ctx_init takes only max_node_id — no host parameter. + * select_eval_ctx_reset sets the host pointer before every host's eval + * loop, so no initial host is needed at construction time. + * Consistent with the Appendix declaration and §8.5.3. */ + ctx = select_eval_ctx_init(max_node_id) /* once per session */ + for each worker (xce_sched_worker_t*): + host_ctx = build_host_eval_ctx(worker, num_res) + if host_ctx == NULL: + log_error("OOM in build_host_eval_ctx — skipping host") + continue /* skip this host; other hosts unaffected */ + select_eval_ctx_reset(ctx, host_ctx) /* reset bitmap, update host ptr */ + for each pending_job: + rc = eval_select(job->resreq->u.primary->select->root, ctx) + /* rc: 1 = pass, 0 = filter out, -1 = error */ + if rc == 1: proceed to rusage check (can_satisfy_rusage) + if rc == 0: skip job for this host + if rc < 0: log error, skip job for this host + free_host_eval_ctx(host_ctx) + select_eval_ctx_fini(ctx) /* once per session */ + ``` + + **`sched_meet_host_selection` role after restructuring:** Retain the + function as the **per-host setup entry point** (not per-job): it calls + `build_host_eval_ctx`, `select_eval_ctx_reset`, then drives the + inner-job eval loop above. Update its signature to accept the + session-level `select_eval_ctx_t*` as a parameter so the context is + allocated once per session by the caller (the session loop), not once + per host call: + + **Host iteration source compatibility:** The pseudo-code above uses + "for each worker" as a shorthand. The actual scheduler may iterate + hosts via a cbuf, linked list, or other container — the select + evaluation code does not depend on the host container type. The only + requirement is that the allocator thread can produce a + `xce_sched_worker_t*` for each candidate host in sequence, which is + already the case for the existing `sched_meet_host_selection` call + pattern. If the scheduler uses a cbuf with wraparound semantics, + the host loop iterates until the cbuf is empty (or a maximum-iteration + guard prevents infinite loops on cbuf corruption). The select eval + code is container-agnostic — it receives a `host_eval_ctx_t*` per host + and does not care how the host was obtained. + + ```c + /* Updated signature: ctx is session-level, allocated by caller */ + void sched_meet_host_selection(xce_scheduler_t* sched, + xce_sched_worker_t* worker, + select_eval_ctx_t* ctx); + ``` + + **Current implementation status:** `sched_meet_host_selection` in + `src/scheduler/src/resource.c` is currently a one-line stub: + `return 1;` (FIXME comment). Phase 6 is a **from-scratch + implementation**, not a signature migration — the entire evaluation + logic must be written. The Phase 6 integration test (`parity test`) + cannot compare against a meaningful baseline since the stub always + passes; the test must explicitly validate against known-good + resreq/host pairs from the test suite. + + Identify all callers of `sched_meet_host_selection` in the allocator + thread's session loop and update them to pass the session-level `ctx` + and call `select_eval_ctx_init`/`select_eval_ctx_fini` around the + host loop. + + **MANDATORY pre-condition (blocking gate):** Before changing the + `sched_meet_host_selection` signature, run: + ``` + grep -rn 'sched_meet_host_selection\|meet_host_selection' src/ + ``` + This grep is a **required blocking step** — Phase 6 cannot proceed until + it is completed and all results are accounted for. A missed call site + that still uses the old `int`-returning signature will compile (C allows + implicit casts on function pointers in some contexts) but invoke the + function with the wrong ABI — causing undefined behavior at runtime + where the missing `select_eval_ctx_t*` parameter causes the function to + read garbage from the stack. **PASS criterion:** all grep hits are + either: (a) the definition itself, (b) direct call sites that have been + updated, or (c) the forward declaration in `scheduler.h` that has been + updated. Any typedef, function pointer, or callback table entry must + also be updated in the same change. Do not mark Phase 6 as complete + until all hits pass this criterion. + + **Return type migration:** the existing signature returns `int`; callers + (e.g., `resource.c:368`) check the return value to gate host-skip and + reason-recording logic: + ```c + /* resource.c:368 — current caller pattern */ + rc = sched_meet_host_selection(sched, w, primary->select); + if (rc == 0) { + add_job_preason(..., PREASON_MATCH_FAIL, w->name, NULL); + return 0; /* skip to next host */ + } + ``` + The updated signature returns `void` — this control flow must move inside + the restructured function. **The restructured `sched_meet_host_selection` + must call `add_job_preason` (for each non-matching job) and handle the + skip-to-next-host logic internally**, driving the inner per-job loop to + completion before returning. The caller no longer inspects a return value; + it simply calls the function and proceeds to the next host. There is no + out-parameter or shared field to check after the call — the function owns + all per-host decision logic. + Before changing the signature: + 1. Run `grep -rn 'sched_meet_host_selection' src/` to find all call sites. + 2. Remove return-value checks at each caller. **Critical:** the + `add_job_preason(schedobj->reason->reasons, PREASON_MATCH_FAIL, + w->name, NULL)` call currently at `resource.c:369–374` must migrate + into the inner per-job loop body inside the restructured + `sched_meet_host_selection` — do not simply delete it. Removing the + call without moving it would cause the scheduler to silently skip jobs + with no pending-reason record, making those jobs appear to hang with + no explanation. + 3. Update the forward declaration at `src/scheduler/include/scheduler.h:111` + (`extern int sched_meet_host_selection(...)`) to match the new `void` return. + +- **`build_host_eval_ctx`** — a **scheduler-private** function + (lives in `src/scheduler/src/`, not in `lib/resreq.c`) that constructs + a `host_eval_ctx_t` for each candidate host. It flattens + `xce_sched_worker_t.resvals` (a `zarray_t` indexed by slot, where + the slot index is the `res_idx`) into the dense `numvals`/`strvals` + arrays expected by `eval_select`. `host_eval_ctx_t` itself is + declared in `include/resreq.h` (accessible to both lib and scheduler); + `build_host_eval_ctx` is declared only in a scheduler-local header. + + ```c + /* Signature (scheduler-private, not in resreq.h): + * host_eval_ctx_t* build_host_eval_ctx(const xce_sched_worker_t* worker, + * int num_res); + * num_res: total resource count from resmgr, used to size flat arrays. + * + * Field mapping for xce_val_t (src/include/xce/types.h): + * slot index in resvals zarray → res_idx (NOT a field in xce_val_t) + * v->which == Value_u_fvalue → numeric/bool: numvals[slot] = v->u.fvalue + * v->which == Value_u_svalue → string: strvals[slot] = makeString(v->u.svalue) + * v->which == Value_u_bvalue → bool as 0.0/1.0: numvals[slot] = (double)v->u.bvalue + * (Current scheduler code in resource.c uses Value_u_fvalue for both numeric + * and boolean resources. If a future scheduler change stores booleans with + * Value_u_bvalue, this branch handles the conversion correctly.) + * Header location for Value_u_fvalue / Value_u_bvalue / Value_u_svalue: + * These enum constants are generated by capnp from src/schema/controller.capnp. + * The generated header is controller.capnp.h (produced by the capnp build target + * in src/lib/meson.build). Do NOT search types.h — they are not defined there. + * In the scheduler build, confirm the generated header is on the include path via: + * grep -rn 'Value_u_fvalue' src/scheduler/ + * to locate the actual #include directive used by resource.c. + * Skip NULL slots (zarray_del vacated them after resource removal). + */ + host_eval_ctx_t* build_host_eval_ctx(const xce_sched_worker_t* worker, int num_res) { + /* 1. malloc sizeof(host_eval_ctx_t) */ + /* 2. malloc numvals[num_res] (double) and strvals[num_res] (char*), + * zero-initialize both. */ + /* 3. Iterate worker->resvals over all caps slots (0..caps-1): + * - slot_idx is the res_idx for the flat arrays. + * - Skip NULL slots. + * - v->which == Value_u_fvalue: numvals[slot_idx] = v->u.fvalue. + * - v->which == Value_u_svalue: strvals[slot_idx] = makeString(v->u.svalue). + * - v->which == Value_u_bvalue: numvals[slot_idx] = (double)v->u.bvalue. */ + /* 4. Set ctx->num_res = num_res, ctx->numvals = numvals, ctx->strvals = strvals. */ + /* 5. Return the populated struct. */ + } + ``` + + Pre-session sequencing: the size snapshot (`sel_node_next_id` read + for eval context sizing, §8.5.3 step 1) must happen **after** all + staging-phase `makeSelectNode` calls have completed, so that no + node added during staging falls outside the sized eval context arrays. + + After construction, the allocator thread calls + `select_eval_ctx_init()` once per session, then + `select_eval_ctx_reset()` per host, evaluates each job via + `eval_select()`, and calls `select_eval_ctx_fini()` at session end. + + **`host_eval_ctx_t` lifetime:** `host_eval_ctx_t` is constructed per + candidate host by `build_host_eval_ctx` and freed after evaluation of + that host completes. It is **not** reused across hosts — each host has + different resource values, and the `makeString` calls in `strvals` + increment the xstring refcount per host. When freeing `host_eval_ctx_t`, + iterate `strvals[0..num_res-1]` and call `dereference(strvals[i])` for + each non-NULL entry to release the xstring refcount acquired by + `makeString()`, then `free(strvals)`, `free(numvals)`, and `free(ctx)`. + This per-host lifecycle is distinct from `select_eval_ctx_t`, which is + session-level and reused across hosts via `reset` (only `bool_bits` is + cleared between hosts; `num_vals` and `str_vals` are overwritten before + being read, so no clearing or `dereference` is needed between hosts). + +- **GC trigger:** In the pre-session phase, after staging merge and job + cleanup (unlinking select roots for completed jobs), while both + job-picker and allocator threads are idle (§8.4): + **GC phase (§8.5.3 "Staging + GC phase"):** + 1. Merge new jobs from staging buffer, parse their resreq strings + (calling `makeSelectNode`), and unlink completed jobs. + 2. `gc_sweep_select_nodes(live_jobs, njobs)` — reclaim unreachable nodes. + Running GC after the staging merge ensures nodes referenced only by + newly arrived jobs are marked live and retained. + 3. `recompact_node_ids()` — reassign dense 0..N-1 IDs (§8.5.4). + + **Pre-session capacity check (§8.5.3 "Pre-session capacity check"):** + 4. Check whether eval context arrays need to grow or shrink to + accommodate the new `max_node_id` (§8.5.3 step 1). + +- **Add GC predicate fields to `xce_scheduler_t`** in + `src/scheduler/include/scheduler.h`: add `_Atomic int session_active`, + `_Atomic int eval_in_progress`, and `_Atomic int staging_merge_complete` + to the struct. These back the predicate functions below; the struct + declaration must be updated before the functions can reference them. + Use `_Atomic int` (not `volatile int`) to match the `_Atomic uint32_t + sel_node_next_id` pattern (§5.3) and to provide correct atomicity on ARM + where `volatile` alone does not guarantee atomic reads/writes. + +- **GC predicate implementation:** The `sched_session_active`, + `sched_eval_in_progress`, and `staging_merge_complete` predicates + (§5.5) wrap the new fields above added to `xce_scheduler_t` in Phase 6. + + Lifecycle of each flag (all use `_Atomic int` — use + `atomic_store_explicit(..., memory_order_release)` for writes and + `atomic_load_explicit(..., memory_order_acquire)` for reads): + + - `session_active`: set to 1 in `sched_begin_session`; cleared to 0 in + `sched_end_session`. + - `eval_in_progress`: set to 1 immediately before the per-host eval loop + begins; cleared to 0 after the loop completes. + - `staging_merge_complete`: set to 1 after staging merge finishes **and + after all completed-job root-unlink writes have been committed**; cleared + to 0 at the top of the next iteration's staging phase, *before* any new + `makeSelectNode` call. The release/acquire pair on this flag provides + the cross-thread visibility that the root-unlink stores (`primary->select->root = NULL`) + are observable to the GC guard check on the other side. The + `atomic_thread_fence(memory_order_seq_cst)` required by §8.5.3 ensures + all root-unlink stores are visible before GC reads them; the + `staging_merge_complete` release write then propagates that guarantee to + any thread reading the flag. **Clearing timing:** clear + `staging_merge_complete` *after* `gc_sweep_select_nodes` returns and + *before* the next staging phase begins — never clear it while GC is + still running. + + These flags enforce the GC idle invariant with assertions; the + scheduler's serial main loop already guarantees the invariants + structurally. + +- **`sel_optimize_level == 0` in the scheduler:** The scheduler is always + expected to run with `sel_optimize_set(1)` (set at startup before any + job parsing). Phase 6 wiring code must assert this at the start of the + session loop: + ```c + if (sel_get_optimize_level() <= 0) { + log_fatal("scheduler must run with CSE enabled; set sel_optimize_level=1 at startup"); + return; + } + ``` + If `sel_optimize_level == 0` were reached (e.g., a mis-configured test + build), the eval context would not be initialized and `eval_select` + would access uninitialized `node_id` values. This guard uses + `log_fatal` + `return` rather than `assert()` because §5.5 explicitly + requires runtime guards for safety-critical invariants — `assert()` is + compiled out with `NDEBUG` and must not be relied upon here. + +- **live_jobs materialization:** `gc_sweep_select_nodes` takes a flat + `const xce_sched_job_t**` array, but the scheduler stores jobs in + `sched->jobs` as a `var`/kvmgr keyed by jobid. Phase 6 must + materialize this array using a plain heap-allocated C array to avoid + the fragile cast assumption of `(const xce_sched_job_t**)zarray_t.data`: + + ```c + size_t live_cap = 64, n = 0; + xce_sched_job_t** live_arr = malloc(live_cap * sizeof(*live_arr)); + if (live_arr == NULL) { log_error("OOM: live_arr malloc failed"); return; } + iterator_t it; + int irc = iter_make(&it, sched->jobs); + if (irc < 0) { free(live_arr); log_error("iter_make failed"); return; } + xce_sched_job_t* job; + for (;;) { + irc = iter_next(&it, (void**)&job); + if (irc < 0 || irc == RTTI_ITER_NO_DATA) break; + if (n == live_cap) { + live_cap *= 2; + xce_sched_job_t** tmp = realloc(live_arr, live_cap * sizeof(*live_arr)); + if (tmp == NULL) { free(live_arr); log_error("OOM: live_arr realloc failed"); return; } + live_arr = tmp; + } + live_arr[n++] = job; + } + gc_sweep_select_nodes(live_arr, n); + free(live_arr); + ``` + + The kvmgr iteration uses the `iter_make` / `iter_next` pattern — + the same pattern used at `src/scheduler/src/scheduler.c:709`. + `kvmgr_first`/`kvmgr_next` do not exist in this codebase. + +**Phase 6 integration test plan:** Phase 6 is the highest-risk phase — +it restructures the live scheduler loop. The following regression +tests must pass before Phase 6 is merged: + +1. **Scheduler smoke test:** Submit 5 jobs with distinct select + expressions (`type==linux`, `gpu`, `mem > 100`, `type==linux && gpu`, + no select clause). Verify all jobs reach RUNNING or PENDING state + without scheduler crash. Run with TSan enabled. + +2. **CSE deduplication test:** Submit 1000 jobs with the same select + expression. Verify `sel_node_next_id` equals the expected number of + unique nodes (not 1000×nodes_per_job) after staging. + +3. **GC correctness test:** Submit 500 jobs, complete 250, trigger + a GC cycle. Verify the remaining 250 jobs still evaluate correctly + on a subsequent session (no use-after-free, no missing preason records). + +4. **Thread-safety test:** Run the scheduler under TSan for at least + one complete session with > 1000 jobs. No data races permitted. + +5. **`sched_meet_host_selection` parity test:** For each of 10 test + jobs with known expected host-match results, verify that Phase 6 + produces the same match/no-match outcomes as the unmodified + Phase 5 baseline. This catches any `add_job_preason` migration + regression. + +These tests must be added to the scheduler's own testsuite +(`src/scheduler/testsuite/` or equivalent) and wired into the meson build +under the scheduler test target — **not** `src/frontend/Makefile`, which is +the Go API server test harness. Phase 6 cannot be submitted for review until +all five tests pass under TSan. --- @@ -1306,11 +3358,18 @@ typedef struct { ```c typedef struct { - const xvec_idval_t* numvals; /* numeric resource values by res_idx */ - const char** strvals; /* string resource values by res_idx */ + int num_res; /* total resource count from resmgr — used for bounds checks */ + const double* numvals; /* numeric (and boolean as 0.0/1.0) resource values, + dense array indexed by res_idx; malloc'd by scheduler */ + const char** strvals; /* string resource values, dense array indexed by res_idx; + each pointer is makeString()-interned; malloc'd by scheduler */ } host_eval_ctx_t; ``` +Note: `numvals` is a plain `double*` array indexed by `res_idx`, **not** `xvec_idval_t*`. +`xvec_idval_t` is used for per-job resource allocation data elsewhere in the scheduler; +it is not used for host resource snapshots passed to `eval_select`. + ### Select Evaluation Context (per-host, §8.5) ```c @@ -1318,17 +3377,42 @@ typedef struct { const host_eval_ctx_t* host; /* host resource snapshot */ uint8_t* bool_bits; /* bitmap, 2 bits per node (eval result cache) */ uint32_t bool_bits_cap; /* capacity in nodes */ + uint32_t node_count; /* session-start snapshot of sel_node_next_id; + * select_eval_ctx_reset clears + * (node_count * 2 + 7) / 8 bytes of bool_bits — + * must NOT re-read sel_node_next_id (see §8.5.3) */ double* num_vals; /* numeric leaf cache by node_id */ uint32_t num_vals_cap; const char** str_vals; /* string leaf cache by node_id */ uint32_t str_vals_cap; } select_eval_ctx_t; -/* Allocate eval context for a host (shared across all jobs for that host) */ -select_eval_ctx_t* select_eval_ctx_for_host(const host_eval_ctx_t* host); +/* Initialize eval context for a session (sized to max_node_id + 1). + * NOT thread-safe — allocator thread processes one host at a time. + * Reused across all hosts within a session; only bool_bits is cleared between hosts. + * + * Integer overflow guard: if max_node_id == UINT32_MAX, adding 1 wraps to 0 + * and malloc(0) succeeds but returns a minimal allocation — subsequent array + * accesses at index 0..max_node_id are then out-of-bounds. The implementation + * must reject this before computing n = max_node_id + 1: + * if (max_node_id == UINT32_MAX) { log_fatal(...); return NULL; } + * In practice, SEL_MAX_INTERNED_NODES (1,000,000) << UINT32_MAX, so this + * branch is unreachable under correct operation, but the guard is required + * as a belt-and-suspenders defence against counter corruption. */ +select_eval_ctx_t* select_eval_ctx_init(uint32_t max_node_id); -/* Free per-host eval context after host scheduling is complete */ -void select_eval_ctx_discard(select_eval_ctx_t* ctx); +/* Reset eval context for the next host (clears bool_bits bitmap only; + * num_vals and str_vals are overwritten before being read by comparators). + * str_vals entries are non-owning pointers — do NOT call dereference() + * on them. The xstring refcount is held by host_eval_ctx_t.strvals, + * which outlives this eval pass (§8.5.2 refcount semantics). */ +void select_eval_ctx_reset(select_eval_ctx_t* ctx, const host_eval_ctx_t* host); + +/* Free eval context at session end. + * Frees the owned arrays (bool_bits, num_vals, str_vals) via free(). + * str_vals is freed as a raw array — no dereference() calls on entries + * because they are non-owning (§8.5.2). Does not dereference ctx->host. */ +void select_eval_ctx_fini(select_eval_ctx_t* ctx); ``` ### Select Parse Context @@ -1339,8 +3423,9 @@ typedef struct { int pos; int err_pos; int has_error; - char* errmsg; - var resmgr; /* name → (res_idx, res_type) */ + char errmsg[256]; /* fixed-size, no alloc, no free */ + int depth; /* current nesting depth, checked against SELECT_MAX_DEPTH */ + var resmgr; /* name → (res_idx, res_type) */ } select_context_t; ``` @@ -1359,21 +3444,151 @@ static int operand_is_numeric(const xce_select_node_t* node) The `sel_node_table` uses `kvtab_type` keyed by `sel_node_key_t`. A custom `kv_key_ops_t` is required since `sel_node_key_t` is a struct -with potential padding between `type` and `op1` — `sel_key_hash` and -`sel_key_cmp` must operate on the three fields individually, not by -`memcpy`-ing the entire struct (which would hash padding bytes): +with potential padding between `type` (int, 4 bytes) and `op1`/`op2` +(uintptr_t, 8 bytes each) — `sel_key_hash` and `sel_key_cmp` MUST +operate on the three fields individually. Using `memcmp` or `memhash` +on the entire struct would include undefined padding bytes and produce +incorrect or non-deterministic results: + +Always construct `sel_node_key_t` values through `sel_make_key()` to +guarantee padding bytes are zeroed before assignment: ```c +static inline sel_node_key_t sel_make_key(sel_node_type_t type, + uintptr_t op1, uintptr_t op2) +{ + sel_node_key_t k; + memset(&k, 0, sizeof k); + k.type = type; + k.op1 = op1; + /* SEL_RESNAME: res_idx (op1) uniquely determines res_type (op2), so zero + * op2 in the key — two nodes with the same res_idx are always the same + * resource regardless of the res_type value passed by the caller. + * makeSelectNode stores the original op2 (res_type) in node->u.resname.res_type + * BEFORE calling sel_make_key, so the fill data is not lost. */ + k.op2 = (type == SEL_RESNAME) ? 0 : op2; + return k; +} +``` + +```c +static int sel_key_cmp(const void* a, const void* b) { + const sel_node_key_t* ka = (const sel_node_key_t*)a; + const sel_node_key_t* kb = (const sel_node_key_t*)b; + /* Compare fields individually — do NOT memcmp the whole struct. */ + if (ka->type != kb->type) return ka->type < kb->type ? -1 : 1; + if (ka->op1 != kb->op1) return ka->op1 < kb->op1 ? -1 : 1; + if (ka->op2 != kb->op2) return ka->op2 < kb->op2 ? -1 : 1; + return 0; +} + static kv_key_ops_t sel_node_key_ops = { - .hash = sel_key_hash, - .cmp = sel_key_cmp, - .assign = sel_key_assign, /* malloc + memcpy */ - .destroy = sel_key_destroy, /* free */ + .hash = sel_key_hash, + .cmp = sel_key_cmp, + .assign = sel_key_assign, /* deep-copy: malloc + memcpy of all bytes */ + .destroy = sel_key_destroy, /* free */ }; ``` +The `assign` and `destroy` callbacks handle struct-sized keys through the +kvtab's `void**`/`void*` interface. Since `sel_make_key()` zeroes padding +before field assignment, `memcpy` of the full struct is safe for `assign`: + +```c +/* Deep-copy a sel_node_key_t for kvtab storage. + * The kvtab assign callback receives (void** dst, void* src). + * Allocate a new sel_node_key_t and copy all bytes — this is safe + * because sel_make_key() zeros padding before field assignment. */ +static int sel_key_assign(void** dst, void* src) { + sel_node_key_t* copy = malloc(sizeof(sel_node_key_t)); + if (copy == NULL) return -ENOMEM; + memcpy(copy, src, sizeof(sel_node_key_t)); + *dst = copy; + return 1; /* success — matches assign_string_key / assign_int_key convention in kv.c */ +} + +/* Free a kvtab-stored sel_node_key_t. */ +static void sel_key_destroy(void* key) { + free(key); +} +``` + +The same field-by-field rule applies to `sel_key_hash`: do NOT hash the struct as raw bytes (undefined padding bytes would produce non-deterministic hash values across builds/platforms). Instead, hash the three fields individually: + +```c +/* Example: FNV-style field-by-field hash for sel_node_key_t */ +static uint32_t sel_key_hash(const void* key) { + const sel_node_key_t* k = (const sel_node_key_t*)key; + uint32_t h = 2166136261u; + h ^= (uint32_t)k->type; h *= 16777619u; + h ^= (uint32_t)(k->op1 & 0xffffffffu); h *= 16777619u; +#if UINTPTR_MAX > 0xffffffffu + h ^= (uint32_t)(k->op1 >> 32); h *= 16777619u; +#endif + h ^= (uint32_t)(k->op2 & 0xffffffffu); h *= 16777619u; +#if UINTPTR_MAX > 0xffffffffu + h ^= (uint32_t)(k->op2 >> 32); h *= 16777619u; +#endif + return h; +} +``` + These must be defined in `resreq.c` alongside the intern table. +### GC Key Reconstruction Helper + +`gc_sweep_select_nodes` calls `key_del` with the original CSE key for each +swept node. The key must be reconstructed from the node's fields with the +**same commutativity sort** applied in `makeSelectNode` step 1 — otherwise +the reconstructed key hashes to a different bucket and `key_del` silently +no-ops, leaking the kvtab entry: + +```c +/* Reconstruct the canonicalized CSE key for an interned node. + * Used by GC sweep to call key_del with the correct sorted key. + * Must match the sort applied in makeSelectNode step 1. */ +static inline sel_node_key_t sel_node_to_key(const xce_select_node_t* node) +{ + uintptr_t op1 = 0, op2 = 0; + switch (node->type) { + case SEL_AND: case SEL_OR: case SEL_EQ: case SEL_NE: { + /* commutative: op1 = min(left, right), op2 = max(left, right) */ + uintptr_t a = (uintptr_t)node->u.binop.left; + uintptr_t b = (uintptr_t)node->u.binop.right; + op1 = (a < b) ? a : b; + op2 = (a < b) ? b : a; + break; + } + case SEL_GT: case SEL_LT: case SEL_GE: case SEL_LE: + op1 = (uintptr_t)node->u.binop.left; + op2 = (uintptr_t)node->u.binop.right; + break; + case SEL_NOT: + op1 = (uintptr_t)node->u.operand; + break; + case SEL_RESNAME: + op1 = (uintptr_t)node->u.resname.res_idx; + break; + case SEL_NUM: { + /* Bit-cast double → uintptr_t via memcpy (avoids strict-aliasing UB). + * A static_assert(sizeof(uintptr_t) == sizeof(double)) must appear + * before this function — equality (not >=) is required so that + * sizeof(op1) copies all 8 bytes of the double without truncation. + * On 32-bit builds where the sizes differ, the assert fires at + * compile time rather than silently producing hash collisions. */ + memcpy(&op1, &node->u.numval, sizeof(op1)); + break; + } + case SEL_STR: + op1 = (uintptr_t)node->u.strval; + break; + default: /* SEL_ANY, SEL_TRUE, SEL_FALSE: op1=op2=0 */ + break; + } + return sel_make_key(node->type, op1, op2); +} +``` + ### Host Resource Value Accessors `eval_select` calls accessor functions that use the `host_eval_ctx_t` @@ -1382,19 +3597,44 @@ resource allocation data; host resource values are retrieved through the evaluation context's `numvals` and `strvals` members: ```c -/* Retrieve typed value from host evaluation context */ +/* Retrieve typed value from host evaluation context. + * Precondition: 0 <= res_idx < host->num_res (checked in debug builds). */ static sel_val_t host_resval_typed(const host_eval_ctx_t* host, int res_idx, res_type_t res_type); /* Boolean resource: nonzero → true */ -static int host_get_bool(const host_eval_ctx_t* host, int res_idx); +static int host_get_bool(const host_eval_ctx_t* host, int res_idx) +{ + if (!host || res_idx < 0 || res_idx >= host->num_res) { + log_fatal("host_get_bool: invalid host=%p res_idx=%d", (void*)host, res_idx); + return 0; + } + return host->numvals[res_idx] != 0.0; +} /* Numeric resource: return float value */ -static double host_get_float(const host_eval_ctx_t* host, int res_idx); +static double host_get_float(const host_eval_ctx_t* host, int res_idx) +{ + if (!host || res_idx < 0 || res_idx >= host->num_res) { + log_fatal("host_get_float: invalid host=%p res_idx=%d", (void*)host, res_idx); + return 0.0; + } + return host->numvals[res_idx]; +} -/* String resource: return interned string pointer */ -static const char* host_get_string(const host_eval_ctx_t* host, - int res_idx); +/* String resource: return interned string pointer. + * Returns NULL if the resource has no value for this host (absent resources are + * zero-initialized in build_host_eval_ctx). Callers MUST check for NULL before + * passing the pointer to xce_str_match; absent string resources mean "no value", + * not an empty string — treat a NULL return as a filter-out (non-match). */ +static const char* host_get_string(const host_eval_ctx_t* host, int res_idx) +{ + if (!host || res_idx < 0 || res_idx >= host->num_res) { + log_fatal("host_get_string: invalid host=%p res_idx=%d", (void*)host, res_idx); + return NULL; + } + return host->strvals[res_idx]; /* may be NULL for absent resources */ +} ``` **String resource storage:** `xvec_idval_t` stores per-job resource @@ -1426,6 +3666,27 @@ PEG actions cast node pointers through `char*` return values: $$ = (char*)makeSelectNode(SEL_AND, (uintptr_t)lnode, (uintptr_t)rnode); ``` +**`parse_select` input length guard (security requirement):** The +`strlen` check against `SELECT_MAX_INPUT_LEN` must occur in `parse_select` +**before** constructing the `select_context_t` or calling +`xce_select_parse`. Placing the check inside the PEG grammar (as a +grammar rule or action) is not sufficient — a malicious caller can pass a +string that causes the PEG engine to allocate a packet-cache proportional +to the input length before the grammar can reject it. The correct +structure is: +```c +int parse_select(const char* input, ...) { + if (strlen(input) > SELECT_MAX_INPUT_LEN) { + snprintf(errmsg, ERRMSG_MAX, + "select expression exceeds maximum length (%zu > %d bytes)", + strlen(input), SELECT_MAX_INPUT_LEN); + return -1; /* reject before PEG context is allocated */ + } + select_context_t ctx = { .input = input, .resmgr = resmgr, ... }; + /* ... newlocale guard, then PEG call ... */ +} +``` + The `parse_select` function unwraps the final return: ```c diff --git a/src/flake.lock b/src/flake.lock index dc04ca1e..a8ebc429 100644 --- a/src/flake.lock +++ b/src/flake.lock @@ -117,11 +117,11 @@ "xbuild": "xbuild" }, "locked": { - "lastModified": 1772607564, - "narHash": "sha256-kBYbiOvM7bBZdIFq2idjQ3eidlAT3w6WS3GRoxHM67s=", + "lastModified": 1777021191, + "narHash": "sha256-hyyJZBX9o1TQLH/7aaEP1/rV3QQTqn70PGs2IDACNnw=", "ref": "master", - "rev": "10c32f769b4431cb657c3c8f23158f643fad74b4", - "revCount": 19, + "rev": "7212ba170d8db040ac99d0fb47c7b1df70df6e11", + "revCount": 20, "type": "git", "url": "ssh://forgejo@gitea.shenrs.eu/rshen/librtti" }, diff --git a/src/include/xce/types.h b/src/include/xce/types.h index 8611a9e4..d67f29b0 100644 --- a/src/include/xce/types.h +++ b/src/include/xce/types.h @@ -40,6 +40,11 @@ typedef struct { xce_val_t* val; } xce_val_with_idx_t; +typedef struct { + char* name; + xce_val_t* val; +} xce_name_val_t; + typedef struct { char* name; char* host; diff --git a/src/lib/htab.c b/src/lib/htab.c index 029b5ebc..9988425b 100644 --- a/src/lib/htab.c +++ b/src/lib/htab.c @@ -143,7 +143,7 @@ hent_t* ht_add(htab_t* t, void* key) e->data = -1; hc = HASH_KEY_HASH(t, key) % (t->nslots); - mylock_init(&(t->lock)); + mylock_lock(&(t->lock)); e->next = t->slots[hc]; t->slots[hc] = e; mylock_unlock(&(t->lock)); @@ -162,7 +162,7 @@ int ht_del(htab_t* t, void* key) } hc = HASH_KEY_HASH(t, key) % (t->nslots); - mylock_init(&(t->lock)); + mylock_lock(&(t->lock)); e = &(t->slots[hc]); while ((*e) != NULL) { if (HASH_KEY_CMP(t, (*e)->key, key) == 0) { diff --git a/src/lib/meson.build b/src/lib/meson.build index a15a63fa..8503f2ba 100644 --- a/src/lib/meson.build +++ b/src/lib/meson.build @@ -49,11 +49,24 @@ xce_resreq_parser = custom_target( ], build_by_default: true) +xce_select_parser = custom_target( + 'xce select expression parser', + output: ['xce_select_peg.c', 'xce_select_peg.h'], + input: ['xce_select.peg'], + command: [ + packcc, + '-l', + '-o', + 'lib/xce_select_peg', + '@INPUT@' + ], + build_by_default: true) libxce_src = [ gen_srcs[0], section_parser[0], rusage_parser[0], xce_resreq_parser[0], + xce_select_parser[0], 'client.c', 'dataq.c', 'hook.c', @@ -83,6 +96,7 @@ libxce_src = [ gen_srcs[0], 'xvec.c', 'helper.c', 'resreq.c', + 'xce_select.c', 'bitset.c', 'kv.c', 'jobj.c', @@ -141,6 +155,8 @@ libxce_test_src = ['testsuite/test.c', 'testsuite/test_limit.c', 'testsuite/test_xvec.c', 'testsuite/test_resreq.c', + 'testsuite/test_resmgr_fixture.c', + 'testsuite/test_select.c', 'testsuite/test_bitset.c', 'testsuite/test_kv.c', 'testsuite/test_jobj.c', @@ -156,4 +172,4 @@ libxce_test = executable('xcelib_test', dependencies: libxce_test_deps, install: true) -test('libxce test', libxce_test) +test('libxce test', libxce_test, timeout: 120) diff --git a/src/lib/section.peg b/src/lib/section.peg index cfc5f827..bda2207d 100644 --- a/src/lib/section.peg +++ b/src/lib/section.peg @@ -46,36 +46,22 @@ select_clause <- "select[" _ s:select_statement _ "]" { $$ = s; } / s:select_statement {$$ = s; } -select_statement <- (!("rusage[" / "]") .)* {$$ = strdup($0); } - / (!("span[" "]" / "]") .)* {$$ = strdup($0); } - / (!("same[" !"]" / "]") .)* {$$ = strdup($0);} - / (!("affinity[" / "]") !"]" .)* {$$ = strdup($0);} - +select_statement <- (!("rusage[" / "span[" / "same[" / "affinity[" / "]") .)* { $$=strdup($0); } usage_clause <- "rusage[" _ usage:usage_statement _ "]" { $$ = usage; } - -usage_statement <- (!("span[" / "]") .)* { $$ = strdup($0); } - / (!("same[" / "]") .)* { $$ = strdup($0); } - / (!("affinity[" / "]") .)* { $$ = strdup($0); } - +usage_statement <- (!("span[" / "same[" / "affinity[" / "]") .)* { $$=strdup($0); } span_clause <- "span[" _ span:span_statement _ "]" { $$ = span; } - -span_statement <- (!("same[" / "]") .)* { $$ = strdup($0); } - / (!("affinity[" / "]") .)* { $$ = strdup($0); } - +span_statement <- (!("same[" / "affinity[" / "]") .)* { $$=strdup($0); } same_clause <- "same[" _ same:same_statement _ "]" { $$ = same; } - same_statement <- (!("affinity[" / "]") .)* { $$ = strdup($0); } - affinity_clause <- "affinity[" _ affinity:affinity_statement _ "]" { $$ = affinity; } - affinity_statement <- (!"]" .)* { $$ = strdup($0); } EOL <- "\n" / "\r\n" / "\r" / "\0" diff --git a/src/lib/testsuite/test.c b/src/lib/testsuite/test.c index e9cff86a..760e5b04 100644 --- a/src/lib/testsuite/test.c +++ b/src/lib/testsuite/test.c @@ -22,6 +22,7 @@ extern void test_xlist(void); extern void test_limit(void); extern void test_xvec(void); extern void test_resreq(void); +extern void test_select(void); extern void test_bitset(void); extern void test_kv(void); extern void test_jobobj(void); @@ -119,6 +120,7 @@ int main(int argc, char* argv[]) TEST(test_limit); TEST(test_xvec); TEST(test_resreq); + TEST(test_select); TEST(test_bitset); TEST(test_kv); TEST(test_jobobj); diff --git a/src/lib/testsuite/test_resreq.c b/src/lib/testsuite/test_resreq.c index e360f4aa..94ca02c9 100644 --- a/src/lib/testsuite/test_resreq.c +++ b/src/lib/testsuite/test_resreq.c @@ -66,6 +66,35 @@ void test_parse_section_fail(void) FREEUP(errmsg); } +void test_parse_section3(void) +{ + char* resreq = "type == any span[ptile=1]"; + char* errmsg = NULL; + int rc = 0; + xce_resreq_data_t data; + + rc = parse_section(&data, (char*)resreq, &errmsg); + if (rc < 0) { + if (errmsg != NULL) { + printf("%s", errmsg); + FREEUP(errmsg); + } + } + TEST_ASSERT_TRUE(rc >= 0); + TEST_ASSERT_TRUE(data.selectString != NULL); + TEST_ASSERT_TRUE((data.rusageString == NULL) && (data.spanString != NULL) + && (data.sameString == NULL) + && (data.affinityString == NULL)); + TEST_ASSERT_TRUE(errmsg == NULL); + + printf("select: %s\n", data.selectString); + printf("span: %s\n", data.spanString); + + FREEUP(data.selectString); + FREEUP(data.spanString); +} + + void test_parse_rusage1(void) { char* rusageString @@ -173,7 +202,7 @@ void test_resreq_primary(void) int rc = 0; xce_primary_resreq_t resreq; - rc = parse_resreq_primary(&resreq, resreqString, &errmsg); + rc = parse_resreq_primary(&resreq, resreqString, NULL, &errmsg); TEST_ASSERT_TRUE(rc >= 0); if (rc >= 0) { int i; @@ -220,7 +249,7 @@ void test_resreq_primary2(void) int rc = 0; xce_primary_resreq_t resreq; - rc = parse_resreq_primary(&resreq, resreqString, &errmsg); + rc = parse_resreq_primary(&resreq, resreqString, NULL, &errmsg); TEST_ASSERT_TRUE(rc >= 0); if (rc >= 0) { int i; @@ -241,7 +270,7 @@ void test_resreq1(void) int rc = 0; xce_resreq_t resreq; - rc = parse_resreq(&resreq, resreqString, &errmsg); + rc = parse_resreq(&resreq, resreqString, NULL, &errmsg); TEST_ASSERT_TRUE(rc >= 0); if (rc >= 0) { int i; @@ -298,7 +327,7 @@ void test_resreq2(void) memset(&resreq, 0x0, sizeof(resreq)); - rc = parse_resreq(&resreq, resreqString, &errmsg); + rc = parse_resreq(&resreq, resreqString, NULL, &errmsg); TEST_ASSERT_TRUE(rc >= 0); if (rc >= 0) { char* buf = NULL; @@ -327,7 +356,7 @@ void test_resreq3(void) memset(&resreq, 0x0, sizeof(resreq)); - rc = parse_resreq(&resreq, resreqString, &errmsg); + rc = parse_resreq(&resreq, resreqString, NULL, &errmsg); TEST_ASSERT_TRUE(rc >= 0); if (rc >= 0) { char* buf = NULL; @@ -347,6 +376,111 @@ void test_resreq3(void) } } +/* + * Boundary tests for the P0-2 span-bleed fix in section.peg. + * + * Before the fix line 50 read: !("span[]" / "]") + * After the fix line 50 reads: !("span[" / "]") + * + * With the old grammar the negative lookahead would only suppress + * consumption of the literal token "span[]", so any span clause + * with content (e.g. "span[ptile=1]") was silently absorbed into + * selectString. These tests verify the corrected boundary. + */ + +/* Test 1: explicit select[...] with a non-empty span[...] clause. */ +void test_parse_section_explicit_select_with_span(void) +{ + char* resreq = "select[type==linux] span[ptile=1]"; + char* errmsg = NULL; + int rc = 0; + xce_resreq_data_t data = {0}; + + rc = parse_section(&data, (char*)resreq, &errmsg); + TEST_ASSERT_TRUE(rc >= 0); + + printf("select: %s\n", data.selectString ? data.selectString : "(null)"); + printf("span: %s\n", data.spanString ? data.spanString : "(null)"); + + /* select must contain the predicate, not bleed into the span keyword */ + TEST_ASSERT_TRUE(data.selectString != NULL); + TEST_ASSERT_TRUE(strstr(data.selectString, "type==linux") != NULL); + + /* span must be parsed separately and hold only its content */ + TEST_ASSERT_TRUE(data.spanString != NULL); + TEST_ASSERT_TRUE(strstr(data.spanString, "ptile=1") != NULL); + + /* unrelated clauses must be absent */ + TEST_ASSERT_TRUE(data.rusageString == NULL); + TEST_ASSERT_TRUE(data.sameString == NULL); + TEST_ASSERT_TRUE(data.affinityString == NULL); + + FREEUP(data.selectString); + FREEUP(data.spanString); + FREEUP(errmsg); +} + +/* Test 2: all three major clauses — select, rusage, span — in one string. */ +void test_parse_section_explicit_select_rusage_span(void) +{ + char* resreq = "select[a&&b] rusage[mem=100] span[ptile=2]"; + char* errmsg = NULL; + int rc = 0; + xce_resreq_data_t data = {0}; + + rc = parse_section(&data, (char*)resreq, &errmsg); + TEST_ASSERT_TRUE(rc >= 0); + + printf("select: %s\n", data.selectString ? data.selectString : "(null)"); + printf("rusage: %s\n", data.rusageString ? data.rusageString : "(null)"); + printf("span: %s\n", data.spanString ? data.spanString : "(null)"); + + TEST_ASSERT_TRUE(data.selectString != NULL); + TEST_ASSERT_TRUE(strstr(data.selectString, "a&&b") != NULL); + + TEST_ASSERT_TRUE(data.rusageString != NULL); + TEST_ASSERT_TRUE(strstr(data.rusageString, "mem=100") != NULL); + + TEST_ASSERT_TRUE(data.spanString != NULL); + TEST_ASSERT_TRUE(strstr(data.spanString, "ptile=2") != NULL); + + FREEUP(data.selectString); + FREEUP(data.rusageString); + FREEUP(data.spanString); + FREEUP(errmsg); +} + +/* + * Test 3: the core regression guard — selectString must NOT contain the + * substring "span" when a span clause is present. With the old buggy + * grammar the span clause text would bleed into selectString. + */ +void test_parse_section_span_not_in_select(void) +{ + char* resreq = "select[gpu] span[ptile=4]"; + char* errmsg = NULL; + int rc = 0; + xce_resreq_data_t data = {0}; + + rc = parse_section(&data, (char*)resreq, &errmsg); + TEST_ASSERT_TRUE(rc >= 0); + + printf("select: %s\n", data.selectString ? data.selectString : "(null)"); + printf("span: %s\n", data.spanString ? data.spanString : "(null)"); + + /* selectString must not contain the span keyword (bleed check) */ + TEST_ASSERT_TRUE(data.selectString != NULL); + TEST_ASSERT_TRUE(strstr(data.selectString, "span") == NULL); + + /* span content must be correctly isolated */ + TEST_ASSERT_TRUE(data.spanString != NULL); + TEST_ASSERT_TRUE(strstr(data.spanString, "ptile=4") != NULL); + + FREEUP(data.selectString); + FREEUP(data.spanString); + FREEUP(errmsg); +} + void test_resreq(void) { printf("\n>> resreq test\n\n"); @@ -354,6 +488,7 @@ void test_resreq(void) RUN_TEST(test_parse_section1); RUN_TEST(test_parse_section2); RUN_TEST(test_parse_section_fail); + RUN_TEST(test_parse_section3); RUN_TEST(test_parse_rusage1); RUN_TEST(test_parse_rusage2); RUN_TEST(test_resreq_primary); @@ -361,4 +496,7 @@ void test_resreq(void) RUN_TEST(test_resreq1); RUN_TEST(test_resreq2); RUN_TEST(test_resreq3); + RUN_TEST(test_parse_section_explicit_select_with_span); + RUN_TEST(test_parse_section_explicit_select_rusage_span); + RUN_TEST(test_parse_section_span_not_in_select); } diff --git a/src/lib/testsuite/test_xstring.c b/src/lib/testsuite/test_xstring.c index fe7a9ff2..79fca9d7 100644 --- a/src/lib/testsuite/test_xstring.c +++ b/src/lib/testsuite/test_xstring.c @@ -1,3 +1,4 @@ +#include #include #include #include @@ -6,6 +7,147 @@ #include "utils.h" #include "xstring.h" +/* ------------------------------------------------------------------ */ +/* Concurrent-test helpers */ +/* ------------------------------------------------------------------ */ + +#define NTHREADS (8) + +/* Thread argument for the same-key test */ +typedef struct { + const char* key; + char* result; +} same_key_arg_t; + +static void* thread_make_same_key(void* arg) +{ + same_key_arg_t* a = (same_key_arg_t*)arg; + a->result = makeString((char*)a->key); + return a->result; +} + +/* Thread argument for the distinct-key test */ +typedef struct { + char key[16]; + char* result; +} distinct_key_arg_t; + +static void* thread_make_distinct_key(void* arg) +{ + distinct_key_arg_t* a = (distinct_key_arg_t*)arg; + a->result = makeString(a->key); + return a->result; +} + +/* ------------------------------------------------------------------ */ +/* Concurrent tests */ +/* ------------------------------------------------------------------ */ + +/* + * test_xstring_concurrent_same_key + * + * Spawn NTHREADS threads, each racing to intern the same key. + * With the TOCTOU fix the lock is held from key_find through key_add, + * so every thread must receive the identical interned pointer. + * Without the fix, two threads could both miss key_find, both call + * makeInstance, and one would leak — and the pointers would differ. + */ +static void test_xstring_concurrent_same_key(void) +{ + pthread_t tids[NTHREADS]; + same_key_arg_t args[NTHREADS]; + int rc; + int i; + + rc = xstring_table_init(139); + TEST_ASSERT_TRUE(rc >= 0); + + for (i = 0; i < NTHREADS; i++) { + args[i].key = "concurrent_key"; + args[i].result = NULL; + rc = pthread_create(&tids[i], NULL, thread_make_same_key, &args[i]); + TEST_ASSERT_TRUE(rc == 0); + } + + for (i = 0; i < NTHREADS; i++) { + void* retval = NULL; + rc = pthread_join(tids[i], &retval); + TEST_ASSERT_TRUE(rc == 0); + } + + /* All threads must have received a non-NULL pointer */ + for (i = 0; i < NTHREADS; i++) { + TEST_ASSERT_TRUE(args[i].result != NULL); + } + + /* All pointers must be the same interned object */ + for (i = 1; i < NTHREADS; i++) { + TEST_ASSERT_EQUAL_PTR(args[0].result, args[i].result); + } + + /* + * makeString increments the refcount on a cache hit. Each thread + * got one reference; drop NTHREADS - 1 extras so xstring_table_fini + * can release the final one cleanly. + */ + for (i = 1; i < NTHREADS; i++) { + dereference((var)args[i].result); + } + + xstring_table_fini(); +} + +/* + * test_xstring_concurrent_distinct_keys + * + * Spawn NTHREADS threads, each interning a unique key. + * Every returned pointer must be non-NULL and distinct from all others, + * confirming that concurrent inserts of different keys do not collide. + */ +static void test_xstring_concurrent_distinct_keys(void) +{ + pthread_t tids[NTHREADS]; + distinct_key_arg_t args[NTHREADS]; + int rc; + int i; + int j; + + rc = xstring_table_init(139); + TEST_ASSERT_TRUE(rc >= 0); + + for (i = 0; i < NTHREADS; i++) { + snprintf(args[i].key, sizeof(args[i].key), "key_%d", i); + args[i].result = NULL; + rc = pthread_create(&tids[i], NULL, thread_make_distinct_key, &args[i]); + TEST_ASSERT_TRUE(rc == 0); + } + + for (i = 0; i < NTHREADS; i++) { + void* retval = NULL; + rc = pthread_join(tids[i], &retval); + TEST_ASSERT_TRUE(rc == 0); + } + + /* Every pointer must be non-NULL */ + for (i = 0; i < NTHREADS; i++) { + TEST_ASSERT_TRUE(args[i].result != NULL); + } + + /* Every pointer must be distinct from all others */ + for (i = 0; i < NTHREADS; i++) { + for (j = i + 1; j < NTHREADS; j++) { + TEST_ASSERT_NOT_EQUAL(args[i].result, args[j].result); + } + } + + /* Release each thread's reference */ + for (i = 0; i < NTHREADS; i++) { + dereference((var)args[i].result); + } + + xstring_table_fini(); +} + static void test_xstring_create(void) { char* s = "Hello, world"; @@ -76,6 +218,8 @@ void test_xstrings(void) RUN_TEST(test_xstring_create); RUN_TEST(test_xstring_deref); RUN_TEST(test_xstring_share); + RUN_TEST(test_xstring_concurrent_same_key); + RUN_TEST(test_xstring_concurrent_distinct_keys); return; } diff --git a/src/lib/xce_resreq.peg b/src/lib/xce_resreq.peg index 45748960..da2b7882 100644 --- a/src/lib/xce_resreq.peg +++ b/src/lib/xce_resreq.peg @@ -106,7 +106,7 @@ primary_resreq <- < (! ('{' / '}' ) .)* > { xce_primary_resreq_t *primary = (xce_primary_resreq_t *)calloc(1, sizeof(*primary)); if (primary != NULL) { auxil->primary = primary; - parse_resreq_primary(primary, (char *)$0, &(auxil->errmsg)); + parse_resreq_primary(primary, (char *)$0, auxil->resmgr, &(auxil->errmsg)); } } diff --git a/src/lib/xce_select.peg b/src/lib/xce_select.peg index 8d4a170a..9fe83207 100644 --- a/src/lib/xce_select.peg +++ b/src/lib/xce_select.peg @@ -35,7 +35,9 @@ } xce_resource_t* r = NULL; + lock(auxil->resmgr, RTTI_LOCK_READ); int rc = key_find(auxil->resmgr, (void*)name, (void**)&r); + unlock(auxil->resmgr); if (rc <= 0 || r == NULL) { if (strcmp(name, "true") == 0 || strcmp(name, "false") == 0) { snprintf(auxil->errmsg, sizeof(auxil->errmsg), @@ -182,6 +184,19 @@ comparison <- l:resname_node _ op:cmp_op _ r:value_node { } } } + } else if (l->u.resname.res_type == RES_TYPE_NUMERIC && + r->type == SEL_STR) { + /* Numeric resource vs string literal: reject — at eval time + * is_str_cmp() would return true and simple_str() would read + * h->strvals[numeric_idx] which is never populated for numeric + * resources (NULL or garbage), causing a strcmp NULL-deref. */ + snprintf(auxil->errmsg, sizeof(auxil->errmsg), + "type mismatch: numeric resource cannot be compared " + "with a string literal"); + auxil->has_error = 1; + SEL_DISCARD(l); + SEL_DISCARD(r); + $$ = NULL; } else { $$ = (xce_select_node_t*)makeSelectNode(op_type, (uintptr_t)l, diff --git a/src/lib/xstring.c b/src/lib/xstring.c index 9f175671..e26374a7 100644 --- a/src/lib/xstring.c +++ b/src/lib/xstring.c @@ -145,30 +145,30 @@ char* makeString(char* s) } mylock_lock(&(xstring_table.lock)); + rc = key_find(xstring_table.xstr_table, (void*)s, (void**)&string); if (rc > 0) { - /* add reference */ reference(string); - } - mylock_unlock(&(xstring_table.lock)); - - if (rc > 0) { + mylock_unlock(&(xstring_table.lock)); return (char*)string; } + /* Cache miss: allocate and insert under the lock so no concurrent caller + * can insert the same key between our find and our add. key_add cannot + * return -EEXIST here because we hold the lock continuously. */ string = makeInstance(&xstring_type, s); if (string == NULL) { + mylock_unlock(&(xstring_table.lock)); return NULL; } - mylock_lock(&(xstring_table.lock)); rc = key_add(xstring_table.xstr_table, (void*)s, string); mylock_unlock(&(xstring_table.lock)); if (rc < 0) { destroy_(string, (obj_destroy_fn_t)xstring_destroy); - string = NULL; - errno = -rc; + errno = -rc; + return NULL; } return (char*)string; diff --git a/src/scheduler/include/sched/types.h b/src/scheduler/include/sched/types.h index d4f4ff63..7af23b7e 100644 --- a/src/scheduler/include/sched/types.h +++ b/src/scheduler/include/sched/types.h @@ -77,7 +77,7 @@ typedef struct { char* resreq; } xce_sched_queue_t; -typedef struct { +typedef struct xce_sched_job_ { uint64_t jobid; time_t sub_time; time_t queue_time; diff --git a/src/scheduler/src/schedobj.c b/src/scheduler/src/schedobj.c index 2ae977c2..3ab21359 100644 --- a/src/scheduler/src/schedobj.c +++ b/src/scheduler/src/schedobj.c @@ -168,7 +168,7 @@ xce_sched_job_t* make_sched_job(void* arg, xce_job_t* j) return NULL; } - rc = parse_resreq(job->resreq, j->submit->resreq, &errmsg); + rc = parse_resreq(job->resreq, j->submit->resreq, sched->resmgr, &errmsg); if (rc < 0) { ERROR("%s parse_resreq failed, %s", __func__, errmsg); FREEUP(errmsg); diff --git a/src/schema/controller.capnp b/src/schema/controller.capnp index 22af69f7..0309fd06 100644 --- a/src/schema/controller.capnp +++ b/src/schema/controller.capnp @@ -161,6 +161,11 @@ struct ValueWithIdx $C.mapname("xce_val_with_idx_t") { val @1: Value $C.mapname("val"); } +struct NameValue $C.mapname("xce_name_val_t") { + name @0: Text $C.mapname("name"); + val @1: Value $C.mapname("val"); +} + struct ConfigVar $C.mapname("xce_config_t") { name @0: Text; value @1: Text;