diff --git a/.kiro/specs/scheduler-controller-msg-gap/design.md b/.kiro/specs/scheduler-controller-msg-gap/design.md index bf0f6572..172f36cd 100644 --- a/.kiro/specs/scheduler-controller-msg-gap/design.md +++ b/.kiro/specs/scheduler-controller-msg-gap/design.md @@ -288,9 +288,11 @@ These are unreachable because `vtask_entry()` in `src/lib/xcectx.c` rewrites all **Requirement traceability:** 2.1, 2.2 -### Component 8: helper.c / helper.h — Name-lookup functions (Req 8) +### Component 8: Name-lookup functions (Req 8) -#### New function: `xce_notify_event_name` +> **Layering correction (bug-765 G-7, 2026-06-18):** these two helpers live in DIFFERENT layers. `xce_notify_event_name(int)` is lib-level and lives in `src/lib/helper.c` / `src/include/helper.h`. The scheduler-op name lookup operates on `xce_sched_op_t`, a **scheduler-only** type (`scheduler.h`); `src/lib` must not include scheduler headers (no layering inversion), so it is a **static `sched_op_name(xce_sched_op_t)` in `src/scheduler/src/scheduler.c`**, NOT `xce_sched_op_name` in lib/helper. The original "both in helper.c" text below is superseded for the scheduler-op helper. + +#### New function: `xce_notify_event_name` (lib — `src/lib/helper.c`) ``` Signature: char* xce_notify_event_name(int event) @@ -301,16 +303,16 @@ Fallback: "UNKNOWN(%d)" with numeric substitution. Matches pattern of `xce_dataset_table()` — takes int, returns `char*`. -#### New function: `xce_sched_op_name` +#### Scheduler-op name lookup: static `sched_op_name` (scheduler — `src/scheduler/src/scheduler.c`) ``` -Signature: char* xce_sched_op_name(xce_sched_op_t op) +Signature: static char* sched_op_name(xce_sched_op_t op) Behavior: Switch on XCE_SCHED_OP_* enum values, return symbolic string. Fallback: "UNKNOWN(%d)" with numeric substitution. ``` -Required for Requirement 7's error logging in `sched_update()`. +Kept in the scheduler layer (NOT lib/helper as `xce_sched_op_name`) because `xce_sched_op_t` is a scheduler-only type and `src/lib` must not depend on scheduler headers — see the layering correction above. Required for Requirement 7's error logging in `sched_update()`. #### Changes to `handler.c` logging @@ -327,7 +329,7 @@ In the `sched_update()` main loop, after the `switch (req->op)` block, when `rc ``` WARN("%s: op %s failed: %s (%d)", __func__, - xce_sched_op_name(req->op), strerror(-rc), rc); + sched_op_name(req->op), strerror(-rc), rc); ``` **Requirement traceability:** 7.1 @@ -443,8 +445,8 @@ xce_push_data_req() | `src/scheduler/include/plugin.h` | Modified — add rusage hook typedef and global function | 6 | | `src/scheduler/src/plugin.c` | Modified — add `plugin_job_rusage_update()` implementation | 6 | | `src/scheduler/src/schedobj.c` | Modified — add `sched_update_existing_queue()` | 4 | -| `src/lib/helper.c` | Modified — add `xce_notify_event_name()`, `xce_sched_op_name()` | 8 | -| `src/include/helper.h` | Modified — add function declarations | 8 | +| `src/lib/helper.c` | Modified — add `xce_notify_event_name()` (lib-level, int arg); the scheduler-op name lookup is a static `sched_op_name()` in `scheduler.c` instead — see Component 8 layering note | 8 | +| `src/include/helper.h` | Modified — declare `xce_notify_event_name()` | 8 | | `src/scheduler/testsuite/test.c` | Modified — add unit tests for all new handlers | All | ## Build Order @@ -466,4 +468,4 @@ xce_push_data_req() | Memory ownership of queued data — capnp-decoded structs freed before dequeue processes them | High | Follow existing pattern: for dataset paths, nullify `dataset->nrows`/`dataset->rows` after enqueue to transfer ownership. For notify paths, copy essential data into the request struct rather than holding pointers to capnp memory. | | `sched_update_existing_job` must handle concurrent modification — handler thread enqueues while main thread processes | Medium | Already mitigated by existing double-buffer swap in `sched_update()`. The handler thread only touches `pend_updates[0]` while the main thread drains `pend_updates[1]`. | | New `UPDATE_QUEUE` op requires queue name lookup — potential lock contention | Low | `sched_get_queue()` already uses read lock. Queue updates are infrequent. | -| `xce_sched_op_name` must handle future enum additions | Low | Follow `xce_dataset_table` pattern — return `"UNKNOWN(%d)"` for unrecognized values. | \ No newline at end of file +| `sched_op_name` (static, scheduler.c) must handle future enum additions | Low | Follow `xce_dataset_table` pattern — return `"UNKNOWN(%d)"` for unrecognized values. | \ No newline at end of file diff --git a/.kiro/specs/scheduler-controller-msg-gap/tasks.md b/.kiro/specs/scheduler-controller-msg-gap/tasks.md index 45c82909..03fd96f4 100644 --- a/.kiro/specs/scheduler-controller-msg-gap/tasks.md +++ b/.kiro/specs/scheduler-controller-msg-gap/tasks.md @@ -10,15 +10,17 @@ Implementation tasks for filling gaps in the scheduler's handling of controller --- -## Task 1: Add name-lookup helpers to lib/helper +## Task 1: Add name-lookup helpers **Requirements:** 8 -Add `xce_notify_event_name()` and `xce_sched_op_name()` to `src/lib/helper.c` and declare them in `src/include/helper.h`, following the exact pattern of the existing `xce_dataset_table()` helper (int argument, `char*` return, `"UNKNOWN(%d)"` fallback). +Add the op/event name-lookup helpers, following the `xce_dataset_table()` pattern (`char*` return, `"UNKNOWN(%d)"` fallback). -- [x] 1.1 Declare `xce_notify_event_name(int event)` and `xce_sched_op_name(xce_sched_op_t op)` in `src/include/helper.h` +> **Layering note (bug-765 G-7 reconciliation, 2026-06-18):** the two helpers live in DIFFERENT layers by necessity. `xce_notify_event_name(int)` is lib-level (it takes an `int` NotifyData event) and lives in `src/lib/helper.c` (declared in `src/include/helper.h`). The scheduler-op name lookup operates on `xce_sched_op_t`, which is a **scheduler-only** type (`src/scheduler/include/scheduler.h`); `src/lib` must not depend on scheduler headers (no layering inversion), so it is implemented as a **static `sched_op_name(xce_sched_op_t)` in `src/scheduler/src/scheduler.c`** — NOT as `xce_sched_op_name` in lib/helper. The original design/tasks text placing it in lib/helper did not account for this layering; the as-built scheduler-layer placement is correct. + +- [x] 1.1 Declare `xce_notify_event_name(int event)` in `src/include/helper.h`. (The scheduler-op name lookup is scheduler-internal — see the layering note — so it is a static `sched_op_name` in `scheduler.c`, not declared in `helper.h`.) - [x] 1.2 Implement `xce_notify_event_name()` in `src/lib/helper.c` — switch on all `NotifyData_data_*` enum values; return symbolic string; fall back to `"UNKNOWN(%d)"` -- [x] 1.3 Implement `xce_sched_op_name()` in `src/lib/helper.c` — switch on all `XCE_SCHED_OP_*` enum values; return symbolic string; fall back to `"UNKNOWN(%d)"` +- [x] 1.3 Implement the scheduler-op name lookup as static `sched_op_name(xce_sched_op_t op)` in `src/scheduler/src/scheduler.c` — switch on all `XCE_SCHED_OP_*` enum values; return symbolic string; fall back to `"UNKNOWN(%d)"`. (Kept in the scheduler layer per the layering note above, not in lib/helper.) --- @@ -66,7 +68,7 @@ Wire the five new op types into the `switch (req->op)` in `sched_update()`, impl - [x] 5.4 Add `XCE_SCHED_OP_UPDATE_RESVALS` case — iterate `resvals->vals[]`, find worker by host name via `sched_get_worker()`, update worker's `resvals` zarray; silently skip unknown hosts - [x] 5.5 Add `XCE_SCHED_OP_JOB_RUSAGE` case — find job by jobid, **store rusage data ON THE JOB** (`xce_job_rusages_t`/`xce_sched_job_rusages_t` field added to `xce_sched_job_t`, replacing any prior, freed in `kill_sched_job`), call `plugin_job_rusage_update()`; return `-ENOENT` if job not found. _Remediation (bug-765, gap G-5): rusage was never stored — `xce_sched_job_t` has no rusage field; Req 6 UB-6.1 unmet. See research.md §Gap Re-Audit._ - [x] 5.6 Implement AND WIRE `sched_update_existing_job()` — find job by jobid via `sched_get_job()`, update ALL mutable fields per Req 3 (state, priority, queue, resource request — resreq string realloc like `sched_update_existing_queue`), call `plugin_job_update()`; return `-ENOENT` when jobid unknown, `-EINVAL` on NULL. **CRITICAL routing fix:** the notify `schedUpdateJob` handler (`sched_handle_sched_update_job`, handler.c:200) and the dataset bulk-load (`sched_on_jobs`, handler.c:31) BOTH enqueue `XCE_SCHED_OP_UPDATE_JOB`, and the dequeue (scheduler.c:755) routes BOTH to `sched_update_jobs()` (create/replace). Distinguish the notify path (introduce a distinct op, e.g. `XCE_SCHED_OP_UPDATE_JOB_FIELDS`, enqueued only by the notify handler, dequeued to `sched_update_existing_job`) so a notify update mutates-in-place / returns `-ENOENT` and NEVER creates a job (Req 3: jobs created only via the dataset path). _Remediation (bug-765, gap G-8+G-4): `sched_update_existing_job` was dead code (0 callers) and copied only `state`; UB-3.1/UB-3.2 unmet. See research.md §Gap Re-Audit._ -- [x] 5.7 Add WARN log in `sched_update()` after switch block for non-zero `rc` — include symbolic op name via `xce_sched_op_name()` and `strerror(-rc)` +- [x] 5.7 Add WARN log in `sched_update()` after switch block for non-zero `rc` — include symbolic op name via the static `sched_op_name()` (scheduler.c, see Task 1 layering note) and `strerror(-rc)` - [x] 5.8 Free new-op payloads in `sched_pend_update_killer` (and any dequeue early-exit) for all new op types (`UPDATE_QUEUE`/`UPDATE_RESOURCES`/`UPDATE_RESVALS_FULL`/`UPDATE_RESVALS`/`JOB_RUSAGE`/the new `UPDATE_JOB_FIELDS`) — ownership IS transferred to the request (handler nullifies the notify/dataset payload pointers), so the empty `break;` cases leak. Use the lib deep-free helpers; validate ASAN-clean (no double-free). _Remediation (bug-765, gap G-3): killer had no-op `break` cases → confirmed per-message leak. See research.md §Gap Re-Audit._ --- @@ -116,7 +118,7 @@ Delete the `Request_body_pushmsg` case block from `scheduler_req_handler()` and Extend `src/scheduler/testsuite/test.c` with unit tests covering every acceptance criterion. Tests must run without a live server — use stub schedulers, table-driven cases, and the existing testsuite harness. -- [x] 9.1 Test `xce_notify_event_name()` and `xce_sched_op_name()` — verify known values return correct strings, unknown values return `"UNKNOWN(%d)"` format +- [x] 9.1 Test `xce_notify_event_name()` (lib helper) — verify known values return correct strings, unknown values return `"UNKNOWN(%d)"` format. (The scheduler-op name lookup is a static `sched_op_name` in scheduler.c — scheduler-internal, not directly unit-testable from `test.c`; it is exercised indirectly via the Req 7 WARN path.) - [x] 9.2 Test `sched_handle_resources()` and `sched_handle_resvals()` handler functions — verify enqueue on valid input, skip on NULL/empty, `-ENOMEM` path - [x] 9.3 Test `sched_handle_sched_update_job()`, `sched_handle_sched_update_queue()`, `sched_handle_sched_update_resvals()`, `sched_handle_sched_job_rusage()` — verify `-EINVAL` on NULL, enqueue on valid input - [x] 9.4 Test `sched_update_existing_job()` — verify `-ENOENT` for unknown jobid, correct field update and plugin call for known jobid @@ -130,3 +132,4 @@ Extend `src/scheduler/testsuite/test.c` with unit tests covering every acceptanc - 5.6 (bug-765 remediation, gaps G-8+G-4, done 2026-06-18): Introduced a distinct op `XCE_SCHED_OP_UPDATE_JOB_FIELDS` (+ `jobfieldsupdate.job` union member) so the notify `schedUpdateJob` handler routes to `sched_update_existing_job` (now in schedobj.c) which mutates state/priority/queue/resreq IN PLACE and returns `-ENOENT` for unknown jobid — never creating a job. The dataset bulk-create path (`sched_on_jobs` → `XCE_SCHED_OP_UPDATE_JOB` → `sched_update_jobs`) is unchanged. Killer frees the new op's payload via `free_Job_ptr`. New white-box test `testsuite/test_job_update.c` (separate meson executable `sched_job_update_test`, white-box `#include "../src/schedobj.c"` + stubs for scheduler-internal externs): 3/3 PASS, regression `sched_test` 15/15, ASAN clean (leaks-off per suite convention). VERIFY NOTES: (1) the handler→dequeue ROUTING is verified by code review, NOT an automated integration test (an end-to-end test through `sched_handle_notify` needs NotifyData capnp plumbing — follow-up); (2) `free(job->resreq)` on replace is shallow but consistent with the codebase — `kill_sched_job` is a no-op stub, so sched jobs are not deep-freed anywhere; the inner-resreq leak is pre-existing, not introduced here. (3) test_job_update.c originally had an accidental `*/` inside a comment (`make_*/kill_*`) + non-ASCII chars + a `make_sched_obj` stub conflicting with the static def in schedobj.c — rewritten ASCII-clean with correct stub set. Task 9.4 (test sched_update_existing_job) is now genuinely satisfied by this real test. - 5.5 (bug-765 remediation, gap G-5, done 2026-06-18): Added `xce_job_rusages_t* rusages` to `xce_sched_job_t` (sched/types.h). New `sched_apply_job_rusage()` (schedobj.c) stores the report ON the job (ownership transferred), replacing any prior via the generated `free_JobRusages_ptr`; returns `-ENOENT` unknown / `-EINVAL` NULL. The dequeue `JOB_RUSAGE` case calls it and NULLs `req->u.job_rusage.rusages` on success so the killer doesn't double-free; `kill_sched_job` (was a no-op stub) now releases `job->rusages`. NOTE: used the generated capnp deep-freer `free_JobRusages_ptr` (NOT a hand-rolled freer — it correctly frees pids/pgids/usages incl. resname per the capnp allocation model; an earlier hand-rolled `free_job_rusages` was removed). 4 new white-box tests (store / replace-frees-prior / -ENOENT / -EINVAL): sched_job_update_test 7/7. - 5.8 (bug-765 remediation, gap G-3, done 2026-06-18): `sched_pend_update_killer` now frees all five previously-leaked op payloads with the exact generated deep-freers — `free_QueueConf_ptr` (xce_queue_t), `free_Resource_ptr[]`+free(arr) (xce_resource_t), `free_ResourceVal_ptr[]`+free(arr) (xce_resval_t), `free_ResValList_ptr` (xce_resval_list_t), `free_JobRusages_ptr` (xce_job_rusages_t). Safe because: ownership is transferred (notify `sched_handle_notify` and dataset `sched_handle_dataset` nullify their pointers on enqueue), `makeResource` constructs a fresh instance via `makeInstance` (no alias), and the dequeue copies values (no alias). VERIFY NOTE: the JOB_RUSAGE killer path interacts with 5.5's ownership; the other four killer frees are review-verified (type-exact generated freers + confirmed transfer/non-alias) but NOT runtime-ASAN-exercised — those notify/dataset ops are not emitted to the scheduler in production today (research R-3 dormant paths) and constructing capnp payloads for a dequeue test is a follow-up. regression sched_test 15/15; no bad-free/double-free under ASAN. +- G-7 (bug-765 doc drift, resolved 2026-06-18, DOC-ONLY): the scheduler-op name lookup is `static sched_op_name(xce_sched_op_t)` in `scheduler.c`, NOT `xce_sched_op_name` in `lib/helper`. RESOLUTION = fix the docs (code is correct): `xce_sched_op_t` is a scheduler-only type (`scheduler.h`) and `src/lib` has zero scheduler-header dependencies, so placing the helper in lib/helper would be a layering inversion. Only `xce_notify_event_name(int)` (lib-level) belongs in lib/helper. Updated Task 1 (intro + 1.1/1.3 + 5.7 + 9.1) and design.md Component 8/9 + file table + risks to reflect the scheduler-layer placement. No code change (kept `sched_op_name` in origin per user direction).