diff --git a/.kiro/steering/roadmap.md b/.kiro/steering/roadmap.md index 092197d5..0666aa2b 100644 --- a/.kiro/steering/roadmap.md +++ b/.kiro/steering/roadmap.md @@ -94,6 +94,7 @@ Smaller items that likely don't warrant a full spec; pick up opportunistically o - [→] **perf-tuning-extraction** — **PROMOTED to spec 2026-07-20** (`clusterconfig-schema-split`, `.kiro/specs/clusterconfig-schema-split/`). Originally recorded 2026-07-06 from admin-config-tool discovery as a documentation/audit item; the audit (2026-07-20) found the fields are load-bearing enough to need real code changes, not just a doc pass — `candidates`/`vcpus`/hash-tuning fields (`jobmgr`/`workermgr`/`qmgr`/`resmgr`/`xstrings`) are already dead weight in the `ClusterConfig` capnp blob (superseded by `--peers`/`--vcpus` CLI flags or never read into runtime state — see `src/docs/cluster-config-split-analysis.md`). **`port` is NOT dead** — a second review pass (2026-07-20) found `cluster_open_listener` (`cluster.c:1216-1258`) falls back to the blob's `port` field, and single-node mode (no `--peers` flag at all, `main.c:293-294`) depends on that fallback entirely; the spec now requires a replacement port-sourcing mechanism before the field can be removed (`clusterconfig-schema-split` Requirement 1.7), not a bare deletion. `heartbeatInterval`/`inactiveHeartbeatCount` (controller→worker liveness, NOT the HA/Raft election heartbeat) were investigated and deliberately kept IN `ClusterConfig` for operational uniformity despite passing the independence rule — see `.kiro/steering/config-design.md`. `fetchJobInterval`/`netioTimeout` left unresolved, flagged out-of-scope in the new spec for a future pass. - [ ] **Raft/HA heartbeat interval+timeout as CLI parameters** — **NEW 2026-07-20 from clusterconfig-schema-split discovery.** The controller-to-controller HA/Raft election heartbeat (`ha->config.heartbeat_interval_ms`/`heartbeat_timeout_ms` — the one where a cross-node mismatch risks spurious failover) is 100% hardcoded today (`HA_DEFAULT_HEARTBEAT_INTERVAL_MS`/`HA_DEFAULT_HEARTBEAT_TIMEOUT_MS`, `cluster.c:1131`) — there is no `ClusterConfig` field for it, and per user decision (2026-07-20) there should never be one: these values must be available to a controller BEFORE it has validated/received cluster configuration (a controller needs its own election-timing parameters to even participate in the election that delivers config in the first place — a bootstrap/chicken-egg constraint, not an independence/frequency judgment call). Target: `--heartbeat-interval-ms`/`--heartbeat-timeout-ms` CLI flags following the exact `--vcpus` pattern (`OPT_*` + `struct arguments` + `myconf_t` field, `0` sentinel = use hardcoded default). Kept hardcoded for now; this is a future item, not blocking `clusterconfig-schema-split`. - [ ] **scheduler-sim-diagnostics** — **NEW 2026-08-10 (`/kiro-discovery`).** Open-ended, continuously-extended spec for scheduler simulation-mode debug/troubleshoot capability — added to over time as new investigation needs surface from discussion or from other in-flight scheduler specs (`scheduler-allocation`, `scheduler-preemption`). Extends `scheduler-core`'s REPL (`info job/queue/worker/resource/summary`, breakpoints, snapshots). Two seed capabilities identified so far: (1) **limit inspection** — `xce_limit_t`/`xce_vres_t` config + live usage currently live only in the `limit` plugin's private kvmgr tables, invisible to the REPL; needs a new `info limit ` command + accessor plumbing. (2) **decision-reasoning trace** — `xce_pendreason_t` is captured on pend decisions but never printed by the REPL, and dispatch decisions have no reasoning trail at all (no record of which limit/resource checks passed) — needs new instrumentation in the allocation path (`resource.c`'s `sched_try_alloc_on_host_`, `plugin.c`'s `limit_prepare_allocation`) plus a REPL command to surface it. Explicitly simulation-mode only (no live/production controller-connected diagnostics) and read-only (no state mutation) per user decision 2026-08-10. Brief: `.kiro/specs/scheduler-sim-diagnostics/brief.md`. Not started — requirements to be added per-capability, not fully speced upfront. Next: `/kiro-spec-init scheduler-sim-diagnostics` when ready to begin. +- [ ] **xcecd_virtual concurrent-job support (MXJ > 1)** — **NEW 2026-08-19, recorded during bug-1589 investigation** (`.wolf/buglog.json`). Live testing found that when a job array (multiple elements) is dispatched to a single `xcecd_virtual` worker in quick succession, only the first element's start ever gets confirmed back to the controller (`on_job_start`) — the rest are dispatched but never confirmed. Root-caused (or being root-caused — see bug-1589 for the confirmed mechanism once the in-flight investigation lands) to `xcecd_virtual`'s job-simulation path: unlike the real `xcecd` binary, which launches an actual dbus-managed worker process per job (tracked by real pid, real IPC status reports), `xcecd_virtual` (`src/worker/src/xcecd.c`'s `xcecd_process_command()`, `VIRTUAL_CLUSTER` branch) never launches a real process at all — it assigns a bare incrementing fake pid (`static int virtual_pid`, `xcecd.c:27`) and simulates job execution via a sleep-timer mechanism (`xcecd_worker_utils.h`'s `VIRTUAL_CLUSTER`-only `xcecd_worker_t` fields `start_time`/`sleep_time`/`killed_signal`, `check_worker_sleep_time_elapsed()`). The immediate bug-1589 fix (once landed) addresses the specific reported symptom; this item is the broader follow-up: **properly support N concurrent simulated jobs per `xcecd_virtual` worker**, since MXJ (max jobs per host) > 1 is a legitimate, common configuration to want to exercise in virtual-cluster-mode testing/CI (this project's own dev workflow relies on `xcecd_virtual` for HA soak tests, resource-value verification, and now scheduler-core's live validation — several `.wolf/buglog.json` entries this session alone used it), not just a single-job-at-a-time edge case. Scope sketch (to refine once bug-1589's root cause is fully confirmed): audit every VIRTUAL_CLUSTER-only code path in `src/worker/` (`xcecd.c`, `main.c`, `xcecd_worker_utils.h`/`.c`, `process_worker_manager.c`) for single-job assumptions (a lone poll/early-return that only processes one pending simulated job per tick, a shared mutable scratch struct reused across concurrent jobs, etc.) and generalize each to iterate the full set of in-flight simulated jobs, matching the pattern the real-process path (`process_manager.c`'s `process_info_t manager->processes[]`, a proper per-pid array) already establishes correctly. Dependencies: bug-1589 (must land first — its root-cause trace is the concrete map of what needs generalizing). Promote via `/kiro-discovery` or `/kiro-spec-init` once bug-1589 is closed and the exact mechanism is confirmed. ## Superseded / Absorbed Items diff --git a/src/worker/src/xcecd_dbus_manager.c b/src/worker/src/xcecd_dbus_manager.c index c5fda459..3b69dcd2 100644 --- a/src/worker/src/xcecd_dbus_manager.c +++ b/src/worker/src/xcecd_dbus_manager.c @@ -188,6 +188,25 @@ int dbus_workers_update(xcecd_worker_t** worker_list, xcecd_job_t* job, xcecd_worker_t* worker = *worker_list; while (worker) { + /* bug-1589: capture the NEXT node before doing any work on this one. + * Every early "nothing more to do for this worker on this pass" + * exit below must advance to `next` and CONTINUE scanning the rest + * of the list -- not silently stay stuck on this same node (several + * branches below used a bare `continue`, which re-tests the SAME + * `worker` pointer forever) and not RETURN out of the whole function + * (the old `worker->worker_status == WORKER_DONE` branch did + * `return true` -- true == 1 == XCECD_ERROR, and worse, it aborted + * the scan of every worker after the first DONE/EXITED one it saw). + * Net effect on a virtual/real worker with N concurrently-dispatched + * jobs in worker_list: only the head of the list ever got processed + * -- job [0] of a job array got its on_job_start report, jobs + * [1..N-1] were starved forever (until job [0] finished AND got + * reaped by cleanup_worker_list(), at which point the NEXT worker + * became the new head and hit the identical bug). Capturing `next` + * up front and always advancing to it is the general fix for every + * branch in this loop, real-mode and VIRTUAL_CLUSTER alike. */ + xcecd_worker_t* next = worker->next; + #ifdef VIRTUAL_CLUSTER if (worker->worker_status == WORKER_RUNNING && worker->job_status == JOB_STARTING) { @@ -198,14 +217,15 @@ int dbus_workers_update(xcecd_worker_t** worker_list, xcecd_job_t* job, ret = update_worker_status(*worker_list, worker->worker_pid, WORKER_RUNNING, job, db); + worker = next; continue; } #endif if (worker->worker_status == WORKER_DONE || worker->worker_status == WORKER_EXITED) { - worker = worker->next; - return true; + worker = next; + continue; } char unit_name[256]; @@ -222,6 +242,7 @@ int dbus_workers_update(xcecd_worker_t** worker_list, xcecd_job_t* job, if (worker->job_status == JOB_STARTING) { worker->job_status = JOB_RUNNING; worker->worker_status = WORKER_RUNNING; + worker = next; continue; } @@ -238,6 +259,7 @@ int dbus_workers_update(xcecd_worker_t** worker_list, xcecd_job_t* job, ret = update_worker_status(*worker_list, worker->worker_pid, WORKER_EXITED, job, db); } + worker = next; continue; #endif XCECD_LOG(XCE_LOG_DEBUG, "Worker %d is done", @@ -265,6 +287,7 @@ int dbus_workers_update(xcecd_worker_t** worker_list, xcecd_job_t* job, ret = update_worker_status(*worker_list, worker->worker_pid, WORKER_DONE, job, db); } + worker = next; continue; #endif XCECD_LOG(XCE_LOG_WARN, "Failed to get worker exit code"); @@ -288,6 +311,7 @@ int dbus_workers_update(xcecd_worker_t** worker_list, xcecd_job_t* job, if (worker->job_status == JOB_STARTING) { worker->job_status = JOB_RUNNING; worker->worker_status = WORKER_RUNNING; + worker = next; continue; } @@ -329,7 +353,7 @@ int dbus_workers_update(xcecd_worker_t** worker_list, xcecd_job_t* job, // worker->worker_pid); } } - worker = worker->next; + worker = next; } return ret; diff --git a/src/worker/testsuite/meson.build b/src/worker/testsuite/meson.build index e68ab141..449e145b 100644 --- a/src/worker/testsuite/meson.build +++ b/src/worker/testsuite/meson.build @@ -58,3 +58,22 @@ test('worker xcecd_startup_order test', xcecd_startup_order_test, suite: 'worker', timeout: 60, env: ['ASAN_OPTIONS=detect_leaks=0']) + +# bug-1589: dbus_workers_update() (xcecd_dbus_manager.c) worker-list +# traversal regression test -- see test_dbus_workers_update.c's header +# comment for the full root-cause trace. Same -DDBUS_MANAGER +# -DVIRTUAL_CLUSTER flag pair the real `xcecd_virtual` executable uses +# (worker/meson.build); DBUS_MANAGER has no effect on xcecd_dbus_manager.c +# itself (no #ifdef DBUS_MANAGER branches there) but is included to match +# production's exact build flags. +dbus_workers_update_test = executable('dbus_workers_update_test', + 'test_dbus_workers_update.c', + c_args: ['-DDBUS_MANAGER', '-DVIRTUAL_CLUSTER'], + include_directories: [worker_test_incdir], + dependencies: worker_test_deps, + install: false) + +test('worker dbus_workers_update test', dbus_workers_update_test, + suite: 'worker', + timeout: 60, + env: ['ASAN_OPTIONS=detect_leaks=0']) diff --git a/src/worker/testsuite/test_dbus_workers_update.c b/src/worker/testsuite/test_dbus_workers_update.c new file mode 100644 index 00000000..ca19ff83 --- /dev/null +++ b/src/worker/testsuite/test_dbus_workers_update.c @@ -0,0 +1,347 @@ +/* + * test_dbus_workers_update.c — regression test for bug-1589: xcecd_virtual's + * (and real xcecd's) dbus_workers_update() (xcecd_dbus_manager.c) starved + * every worker in xcecd_ctx.worker_list after the first one it visited. + * + * User-reported symptom: a 5-element job array (base_jobid=0-7-1787111880-0, + * array_size=5) was fully dispatched by the controller (dispatch_a_job_ + * fired for [0]-[4]) and elected by the scheduler (limit_job_election fired + * for [0]-[4]), but only element [0] EVER produced an on_job_start log line + * -- elements [1]-[4] were dispatched but never reported as started, in + * either log file, ever. + * + * Root cause, confirmed by tracing xcecd_virtual's real build (worker/ + * meson.build: xcecd_virtual == xcecd_srcs + main.c, -DVIRTUAL_CLUSTER + * -DDBUS_MANAGER -- the SAME DBUS_MANAGER flag the real `xcecd` binary also + * gets, so main.c's `#ifndef DBUS_MANAGER` call to process_worker_manager.c's + * worker_process_update() is dead code for BOTH binaries; both actually call + * dbus_workers_update() every ~10ms from main.c's run loop): + * + * dbus_workers_update() iterates xcecd_ctx.worker_list (one xcecd_worker_t + * node per concurrently-dispatched job) with a single `while (worker) { ... + * worker = worker->next; }` loop, but SEVERAL branches inside the loop body + * used a bare `continue` (re-testing the SAME `worker` pointer forever + * instead of advancing) or, worse, `return true` the moment ANY worker was + * WORKER_DONE/WORKER_EXITED (aborting the scan of the REST of the list + * entirely, not just skipping that one finished worker). Net effect: once + * the function started making progress on the head of the list, every OTHER + * worker queued behind it was starved of update_worker_status() (which is + * what fires xcecd_reply_startjob() -- the on_job_start report) for as long + * as the head worker stayed un-reaped -- exactly the observed "[0] started, + * [1]-[4] never did" symptom for a 5-element array dispatched to one worker + * within the SAME dbus_workers_update() call. + * + * Fix (xcecd_dbus_manager.c): capture `next = worker->next` once at the top + * of every loop iteration and always advance to it -- every `continue` + * becomes `worker = next; continue;`, and the `return true` early-abort + * becomes `worker = next; continue;` (skip this already-finished worker, + * keep scanning the rest of the list). + * + * White-box test: #include "../src/xcecd_dbus_manager.c" directly (same + * technique as test_xcecd_startup_order.c/test_controller_connector.c) so + * the REAL dbus_workers_update() traversal logic runs under test. + * update_worker_status()/is_systemd_job_active()/get_systemd_job_exit_code() + * /stop_systemd_job()/make_job_sd_unit_name() are stubbed to isolate the + * loop's own control-flow bug from unrelated dependencies (real systemd/ + * dbus, real sqlite3, real IPC-to-controller reporting) -- the + * update_worker_status() stub mirrors just enough of the real function's + * state machine (JOB_STARTING+WORKER_RUNNING -> JOB_RUNNING on a + * WORKER_RUNNING call; -> WORKER_DONE/JOB_DONE on a WORKER_DONE call) to + * drive dbus_workers_update()'s own control flow identically to production, + * while recording every call so the test can assert ALL workers were + * visited, not just the first. + * + * Build flags: -DVIRTUAL_CLUSTER (matches the real xcecd_virtual executable, + * worker/meson.build -- DBUS_MANAGER is irrelevant here since xcecd_dbus_ + * manager.c itself has no #ifdef DBUS_MANAGER branches, only VIRTUAL_CLUSTER + * ones, which is exactly the code path this bug lives in). + */ +#include +#include +#include +#include +#include + +#include + +/* Pull the real dbus_workers_update() traversal logic under test into this + * TU. */ +#include "../src/xcecd_dbus_manager.c" + +/* ════════════════════════════════════════════════════════════════════════ + * Link-satisfying stubs + * ════════════════════════════════════════════════════════════════════════ */ + +#define MAX_CALLS 64 +static int g_uws_pid[MAX_CALLS]; +static int g_uws_worker_status[MAX_CALLS]; +static int g_uws_ncalls; + +/* Mirrors just the pieces of the REAL update_worker_status() + * (xcecd_worker_utils.c) that affect dbus_workers_update()'s own control + * flow: looks the worker up by pid in the full list (same as production), + * transitions JOB_STARTING->JOB_RUNNING on a WORKER_RUNNING call and + * ->WORKER_DONE/JOB_DONE on a WORKER_DONE call. Does NOT reimplement + * xcecd_reply_startjob()/DB/IPC side effects -- those are outside this + * loop-traversal bug's boundary and are covered by other tests. */ +int update_worker_status(xcecd_worker_t* worker_list, int worker_pid, + int worker_status, xcecd_job_t* job, sqlite3* db) +{ + (void)db; + + if (g_uws_ncalls < MAX_CALLS) { + g_uws_pid[g_uws_ncalls] = worker_pid; + g_uws_worker_status[g_uws_ncalls] = worker_status; + g_uws_ncalls++; + } + + xcecd_worker_t* w = worker_list; + while (w && w->worker_pid != worker_pid) { + w = w->next; + } + if (!w) { + return XCECD_NOT_FOUND; + } + + job->job_id = w->job_info.jobid; + + switch (worker_status) { + case WORKER_RUNNING: + if (w->job_status == JOB_STARTING && job->job_status == JOB_RUNNING) { + w->job_status = JOB_RUNNING; + } + break; + case WORKER_DONE: + w->worker_status = WORKER_DONE; + w->job_status = JOB_DONE; + break; + case WORKER_EXITED: + w->worker_status = WORKER_EXITED; + w->job_status = JOB_EXITED; + break; + default: + break; + } + + return XCECD_OK; +} + +/* No real systemd/dbus unit ever exists for a VIRTUAL_CLUSTER fake-pid job + * (xcecd.c's xcecd_process_command() never calls dbus_start_worker() when + * VIRTUAL_CLUSTER is defined) -- matches both production virtual-cluster + * reality and this sandbox (no dbus session). */ +bool is_systemd_job_active(const char* unit_name) +{ + (void)unit_name; + return false; +} + +/* get_systemd_job_exit_code() (xcecd_cgroups.c) returns -1 on any sd_bus + * failure, which is exactly what happens for a nonexistent unit -- verified + * against its real implementation. */ +int get_systemd_job_exit_code(const char* unit_name) +{ + (void)unit_name; + return -1; +} + +int stop_systemd_job(const char* unit_name) +{ + (void)unit_name; + return 0; +} + +int make_job_sd_unit_name(char* buf, size_t bufsize, const char* prefix, + uint64_t jobid) +{ + return snprintf(buf, bufsize, "%s-%llu.service", prefix, + (unsigned long long)jobid); +} + +/* Link-satisfying stubs: xcecd_dbus_manager.c (#include'd whole) also + * defines dbus_start_worker()/dbus_get_worker_pid() (never exercised by + * this test -- only dbus_workers_update()'s traversal is under test), which + * reference these two real-systemd helpers (normally defined in + * xcecd_cgroups.c, not linked into this white-box test binary). */ +char* start_systemd_job(const char* command, const res_limits_t* limits, + const char* unit_name_prefix, uint64_t job_id, + char* extra_args) +{ + (void)command; + (void)limits; + (void)unit_name_prefix; + (void)job_id; + (void)extra_args; + return NULL; +} + +pid_t get_systemd_job_pid(const char* unit_name) +{ + (void)unit_name; + return -1; +} + +/* Real check_worker_sleep_time_elapsed() (xcecd_worker_utils.c) is not + * linked into this white-box test binary (its own file isn't #include'd, + * to keep the stub surface for update_worker_status() etc. above simple + * and hermetic) -- this reimplementation is byte-identical to the real + * function's logic and is exactly what dbus_workers_update() needs to make + * its own VIRTUAL_CLUSTER JOB_RUNNING->WORKER_DONE decision, which this + * test's fixtures (sleep_time == 0) deliberately drive down the "already + * elapsed" path for fast, deterministic execution. */ +bool check_worker_sleep_time_elapsed(xcecd_worker_t* worker) +{ + if (!worker) { + return true; + } + if (worker->sleep_time <= 0) { + return true; + } + time_t current_time = time(NULL); + if (current_time == (time_t)-1) { + return true; + } + return difftime(current_time, worker->start_time) >= worker->sleep_time; +} + +/* ════════════════════════════════════════════════════════════════════════ + * Fixtures + * ════════════════════════════════════════════════════════════════════════ */ + +static xcecd_worker_t* make_starting_worker(int pid, uint64_t jobid) +{ + xcecd_worker_t* w = calloc(1, sizeof(*w)); + w->worker_pid = pid; + w->worker_status = WORKER_RUNNING; + w->job_status = JOB_STARTING; + w->job_info.jobid = jobid; + /* sleep_time == 0 -> check_worker_sleep_time_elapsed() returns true + * immediately (matches a job whose command is not a literal "sleep N", + * i.e. most real test payloads) -- keeps this test deterministic and + * fast (no real elapsed-time dependency) while still exercising the + * exact branches production hits. */ + w->sleep_time = 0; + w->start_time = time(NULL); + return w; +} + +static void link_worker(xcecd_worker_t** list, xcecd_worker_t* w) +{ + if (!*list) { + *list = w; + return; + } + xcecd_worker_t* cur = *list; + while (cur->next) { + cur = cur->next; + } + cur->next = w; + w->prev = cur; +} + +static void free_worker_list(xcecd_worker_t* list) +{ + while (list) { + xcecd_worker_t* next = list->next; + free(list); + list = next; + } +} + +static bool started_call_seen(int pid) +{ + for (int i = 0; i < g_uws_ncalls; i++) { + if (g_uws_pid[i] == pid && g_uws_worker_status[i] == WORKER_RUNNING) { + return true; + } + } + return false; +} + +void setUp(void) +{ + g_uws_ncalls = 0; +} + +void tearDown(void) { return; } + +/* ════════════════════════════════════════════════════════════════════════ + * bug-1589 — every JOB_STARTING worker in the list must get its start + * transition (the event that fires xcecd_reply_startjob()/on_job_start in + * production) within one dbus_workers_update() call, not just the first. + * ════════════════════════════════════════════════════════════════════════ */ + +static void test_all_array_elements_get_started_in_one_pass(void) +{ + /* Mirrors the user's live report: a 5-element job array dispatched to + * one worker, all within one control loop tick. */ + xcecd_worker_t* list = NULL; + xcecd_worker_t* workers[5]; + for (int i = 0; i < 5; i++) { + workers[i] = make_starting_worker(1000 + i, 100 + (uint64_t)i); + link_worker(&list, workers[i]); + } + + xcecd_job_t job = { 0 }; + int ret = dbus_workers_update(&list, &job, NULL); + + TEST_ASSERT_EQUAL_INT(XCECD_OK, ret); + + for (int i = 0; i < 5; i++) { + char msg[160]; + snprintf(msg, sizeof(msg), + "array element [%d] (pid %d) never received its " + "WORKER_RUNNING/on_job_start transition -- starved by " + "an earlier worker in the list (bug-1589)", + i, 1000 + i); + TEST_ASSERT_TRUE_MESSAGE(started_call_seen(1000 + i), msg); + } + + free_worker_list(list); +} + +/* Regression guard for the specific `return true` abort: a worker that is + * ALREADY WORKER_DONE at the head of the list (e.g. a stale entry not yet + * reaped by cleanup_worker_list()) must not stop the scan of the rest of + * the list. */ +static void test_done_head_worker_does_not_block_rest_of_list(void) +{ + xcecd_worker_t* list = NULL; + + xcecd_worker_t* done_head = calloc(1, sizeof(*done_head)); + done_head->worker_pid = 2000; + done_head->worker_status = WORKER_DONE; + done_head->job_status = JOB_DONE; + done_head->job_info.jobid = 200; + link_worker(&list, done_head); + + xcecd_worker_t* starting = make_starting_worker(2001, 201); + link_worker(&list, starting); + + xcecd_job_t job = { 0 }; + int ret = dbus_workers_update(&list, &job, NULL); + + TEST_ASSERT_EQUAL_INT(XCECD_OK, ret); + TEST_ASSERT_TRUE_MESSAGE( + started_call_seen(2001), + "a DONE worker at the head of the list must not abort the scan " + "before the still-JOB_STARTING worker behind it is reached"); + + free_worker_list(list); +} + +/* ════════════════════════════════════════════════════════════════════════ + * main + * ════════════════════════════════════════════════════════════════════════ */ + +int main(void) +{ + xce_init_xcecd_log("/tmp/test_dbus_workers_update.log", 1024 * 1024, 2); + + UNITY_BEGIN(); + + RUN_TEST(test_all_array_elements_get_started_in_one_pass); + RUN_TEST(test_done_head_worker_does_not_block_rest_of_list); + + return UNITY_END(); +}