diff --git a/src/scheduler/src/core_events.c b/src/scheduler/src/core_events.c index ad96dbcd..69a18aae 100644 --- a/src/scheduler/src/core_events.c +++ b/src/scheduler/src/core_events.c @@ -452,7 +452,29 @@ static void core_apply_worker_resvals(xce_sched_core_t* core, xce_resource_t* r = NULL; xce_val_t* val = NULL; - if ((nv == NULL) || (nv->name == NULL)) { + /* bug-1688: nv->val legitimately decodes to NULL (controller.capnp.c's + * decode_Value_ptr() sets *d = NULL whenever the wire's NameValue.val + * capnp pointer is CAPN_NULL) whenever a WORKERS_PUSH re-announces a + * worker's resource NAME list without fresh values for some/all of + * them (live-traced: a repeat WORKERS_PUSH for an already-registered + * worker can carry 20 resval names with nv->val==NULL for every one). + * Before this fix, the code below unconditionally MOVE_PTR'd this + * NULL into `val` and called zarray_put(&worker->resvals, r->idx, + * NULL) -- silently overwriting a perfectly good previously-known + * resource value with NULL. Because zarray_put_ex() (zarray.c) only + * increments array->count when the slot being written was NULL + * beforehand, the first such NULL-write left resvals.count + * unchanged (old slot was non-NULL, so the "else count++" branch + * never fired) while silently nulling the slot's DATA -- then a + * SECOND repeat push, now finding every slot already NULL, hit the + * "else count++" branch for all of them, producing the count- + * doubling anomaly (20 -> 40) this bug was originally reported + * with. Treating a NULL nv->val as "no update for this resource in + * this push" (skip, preserve whatever value is already there) is + * the same defensive "continue" posture this loop already applies + * to nv==NULL/nv->name==NULL above -- it protects the scheduler's + * own state regardless of why the wire push omitted a value. */ + if ((nv == NULL) || (nv->name == NULL) || (nv->val == NULL)) { continue; } @@ -468,7 +490,7 @@ static void core_apply_worker_resvals(xce_sched_core_t* core, * src/controller/dataex.c's collect_resval() TRACE (the matching * "about to be wire-packaged" point) for the same resource/ * worker/timestamp to isolate the wire encode/decode step. */ - if (nv->val != NULL && nv->val->which == Value_u_fvalue) { + if (nv->val->which == Value_u_fvalue) { TRACE("%s worker '%s' resource '%s' received fvalue=%f", __func__, w->name ? w->name : "(null)", nv->name, nv->val->u.fvalue); diff --git a/src/scheduler/src/repl_info.c b/src/scheduler/src/repl_info.c index 710e28c9..ddcd888b 100644 --- a/src/scheduler/src/repl_info.c +++ b/src/scheduler/src/repl_info.c @@ -321,7 +321,19 @@ static void repl_info_worker_one(xce_sched_sim_driver_t* sim, iter_destroy(&it); } - for (i = 0; i < w->resvals.count; i++) { + /* bug-1688: `.count` (number of currently-populated slots) is NOT a + * safe index upper bound for iterating this zarray BY INDEX -- it can + * legitimately be less than the highest occupied index once entries + * are sparse/non-contiguous (e.g. zarray_put_ex()'s count bookkeeping, + * zarray.c, only tracks "how many slots read NULL at write time", not + * "the current occupied-index high-water mark"). `.caps` (allocated + * capacity, monotonic, never shrinks) is the correct bound for + * index-based iteration -- mirrors resource.c:228's + * `int num_res = (int)w->resvals.caps;` for this identical structure. + * A `.caps`-bound loop can hit legitimately-empty slots that a + * `.count`-bound loop never would, so avail_v/rname's existing + * NULL-skip below (unchanged) is what keeps this safe. */ + for (i = 0; i < w->resvals.caps; i++) { xce_val_t* avail_v = (xce_val_t*)zarray_get(&(w->resvals), i); xce_val_t* used_v = (xce_val_t*)zarray_get(&(w->resInUse), i); const char* rname = find_resource_name_by_idx(res_arr, res_n, i); diff --git a/src/scheduler/testsuite/test_core_events.c b/src/scheduler/testsuite/test_core_events.c index 34bee2d4..4d7c715c 100644 --- a/src/scheduler/testsuite/test_core_events.c +++ b/src/scheduler/testsuite/test_core_events.c @@ -644,6 +644,147 @@ static void test_workers_push_twice_updates_existing_worker_in_place(void) TEST_ASSERT_EQUAL_FLOAT(200.0f, (float)applied->u.fvalue); } +/* + * bug-1688 regression: a WORKERS_PUSH re-announcing an already-registered + * worker's resource NAME list can legitimately carry nv->val == NULL for + * some/all entries (controller.capnp.c's decode_Value_ptr() decodes a wire + * NameValue.val CAPN_NULL pointer to exactly this -- live-traced against a + * real replay: a repeat push for worker 'pc13v' carried 20 resource names + * with nv->val == NULL for every one). Before this fix, + * core_apply_worker_resvals() unconditionally zarray_put()'d this NULL, + * silently clobbering a previously-known-good resource value with NULL. + * Because zarray_put_ex() (zarray.c) only increments array->count when the + * slot being written was NULL beforehand, that first NULL-write left + * resvals.count unchanged (the old slot was non-NULL) while zeroing the + * slot's DATA -- so a THIRD push, now finding every slot already NULL, hit + * the "was NULL, count++" branch for each one, doubling resvals.count (the + * literal 20 -> 40 anomaly this bug was reported with) despite carrying the + * exact same resource names both times. Two resources (not one) are used + * here so the count-doubling assertion is meaningful, not a 1-vs-2 + * coincidence. + */ +static void test_workers_push_with_null_val_does_not_clobber_existing_resval( + void) +{ + xce_sched_core_t core; + xce_core_event_t event; + xce_resource_t wire_mem; + xce_resource_t wire_r1m; + xce_worker_t wire_worker; + xce_worker_t* worker_ptrs[1]; + xce_val_t val_mem; + xce_val_t val_r1m; + xce_name_val_t nv_mem; + xce_name_val_t nv_r1m; + xce_name_val_t* nv_ptrs[2]; + xce_name_val_t nv_mem_null; + xce_name_val_t nv_r1m_null; + xce_name_val_t* nv_null_ptrs[2]; + xce_sched_worker_t* w; + xce_resource_t* r_mem = NULL; + xce_resource_t* r_r1m = NULL; + xce_val_t* applied; + int rc; + + fixture_init(&core, &g_full_plugin); + + memset(&wire_mem, 0, sizeof(wire_mem)); + wire_mem.name = "mem"; + TEST_ASSERT_NOT_NULL(makeResource(core.resmgr, &wire_mem)); + memset(&wire_r1m, 0, sizeof(wire_r1m)); + wire_r1m.name = "r1m"; + TEST_ASSERT_NOT_NULL(makeResource(core.resmgr, &wire_r1m)); + + TEST_ASSERT_TRUE(key_find(core.resmgr, (void*)"mem", (void**)&r_mem) >= 0); + TEST_ASSERT_NOT_NULL(r_mem); + TEST_ASSERT_TRUE(key_find(core.resmgr, (void*)"r1m", (void**)&r_r1m) >= 0); + TEST_ASSERT_NOT_NULL(r_r1m); + + /* First push: worker "hostA" joins with real mem/r1m values. */ + memset(&val_mem, 0, sizeof(val_mem)); + val_mem.which = Value_u_fvalue; + val_mem.u.fvalue = 100.0; + memset(&nv_mem, 0, sizeof(nv_mem)); + nv_mem.name = "mem"; + nv_mem.val = &val_mem; + + memset(&val_r1m, 0, sizeof(val_r1m)); + val_r1m.which = Value_u_fvalue; + val_r1m.u.fvalue = 5.0; + memset(&nv_r1m, 0, sizeof(nv_r1m)); + nv_r1m.name = "r1m"; + nv_r1m.val = &val_r1m; + + nv_ptrs[0] = &nv_mem; + nv_ptrs[1] = &nv_r1m; + + memset(&wire_worker, 0, sizeof(wire_worker)); + wire_worker.name = "hostA"; + wire_worker.status = 0; + wire_worker.nresvals_to_sched = 2; + wire_worker.resvals_to_sched = nv_ptrs; + worker_ptrs[0] = &wire_worker; + + memset(&event, 0, sizeof(event)); + event.type = XCE_CORE_EVENT_WORKERS_PUSH; + event.u.workers_push.nworkers = 1; + event.u.workers_push.workers = worker_ptrs; + + rc = xce_core_push_events(&core, &event, 1); + TEST_ASSERT_EQUAL_INT(0, rc); + + w = sched_get_worker(&core, "hostA"); + TEST_ASSERT_NOT_NULL(w); + TEST_ASSERT_EQUAL_INT(2, w->resvals.count); + applied = (xce_val_t*)zarray_get(&(w->resvals), r_mem->idx); + TEST_ASSERT_NOT_NULL(applied); + TEST_ASSERT_EQUAL_FLOAT(100.0f, (float)applied->u.fvalue); + applied = (xce_val_t*)zarray_get(&(w->resvals), r_r1m->idx); + TEST_ASSERT_NOT_NULL(applied); + TEST_ASSERT_EQUAL_FLOAT(5.0f, (float)applied->u.fvalue); + + /* Second push: SAME worker, SAME resource names, but nv->val == NULL + * for both -- the exact "re-announce names, no fresh values" shape + * live-traced from the real replay. Must NOT clobber the existing + * values, and must NOT change resvals.count. */ + memset(&nv_mem_null, 0, sizeof(nv_mem_null)); + nv_mem_null.name = "mem"; + nv_mem_null.val = NULL; + memset(&nv_r1m_null, 0, sizeof(nv_r1m_null)); + nv_r1m_null.name = "r1m"; + nv_r1m_null.val = NULL; + nv_null_ptrs[0] = &nv_mem_null; + nv_null_ptrs[1] = &nv_r1m_null; + + wire_worker.resvals_to_sched = nv_null_ptrs; + + rc = xce_core_push_events(&core, &event, 1); + TEST_ASSERT_EQUAL_INT(0, rc); + + w = sched_get_worker(&core, "hostA"); + TEST_ASSERT_NOT_NULL(w); + TEST_ASSERT_EQUAL_INT(2, w->resvals.count); + applied = (xce_val_t*)zarray_get(&(w->resvals), r_mem->idx); + TEST_ASSERT_NOT_NULL(applied); + TEST_ASSERT_EQUAL_FLOAT(100.0f, (float)applied->u.fvalue); + applied = (xce_val_t*)zarray_get(&(w->resvals), r_r1m->idx); + TEST_ASSERT_NOT_NULL(applied); + TEST_ASSERT_EQUAL_FLOAT(5.0f, (float)applied->u.fvalue); + + /* Third push: SAME shape again (still NULL vals) -- if the bug were + * present, resvals.count would double here (20 -> 40 in the live + * trace; 2 -> 4 here) because the SECOND push (above) would have + * already nulled the slots, making THIS push's writes look like + * fresh/first-time occupancy. Confirms the fix holds across repeats, + * not just once. */ + rc = xce_core_push_events(&core, &event, 1); + TEST_ASSERT_EQUAL_INT(0, rc); + + w = sched_get_worker(&core, "hostA"); + TEST_ASSERT_NOT_NULL(w); + TEST_ASSERT_EQUAL_INT(2, w->resvals.count); +} + /* JOB_RUSAGE's on_job_rusage hook is OPTIONAL (Req 5 AC 4) -- a picker * without it must still apply the state mutation and must not error or * crash. */ @@ -834,6 +975,7 @@ int main(void) RUN_TEST(test_jobs_push_fires_on_job_push_once_per_job); RUN_TEST(test_non_picker_events_mutate_state_but_never_call_hooks); RUN_TEST(test_workers_push_twice_updates_existing_worker_in_place); + RUN_TEST(test_workers_push_with_null_val_does_not_clobber_existing_resval); RUN_TEST(test_job_rusage_applies_state_when_hook_is_null); RUN_TEST(test_config_push_decomposes_queues_resources_and_resvals); RUN_TEST(test_push_events_rejects_with_ebusy_when_session_active_no_mutation); diff --git a/src/scheduler/testsuite/test_repl_breakpoints_inspection.c b/src/scheduler/testsuite/test_repl_breakpoints_inspection.c index bc9875b0..fb162610 100644 --- a/src/scheduler/testsuite/test_repl_breakpoints_inspection.c +++ b/src/scheduler/testsuite/test_repl_breakpoints_inspection.c @@ -927,6 +927,112 @@ static void test_info_worker_reports_string_and_boolean_resource_values(void) "PASS\n"); } +/* + * bug-1688: repl_info_worker_one()'s resource-print loop used + * `for (i = 0; i < w->resvals.count; i++)` -- treating `.count` (number of + * currently-populated slots) as a safe index upper bound for iterating the + * zarray BY INDEX. `.count` can legitimately be less than the highest + * occupied index once entries are sparse/non-contiguous (zarray_put_ex()'s + * count bookkeeping, zarray.c, only tracks "how many slots read NULL at + * write time" -- it is not recomputed from the array's actual contents). + * `.caps` (allocated capacity, monotonic, never shrinks) is this codebase's + * own established convention for this identical structure -- see + * resource.c:228's `int num_res = (int)w->resvals.caps;`. Regression: a + * worker whose resvals zarray has TWO populated slots (idx 0 "mem", idx 1 + * "r1m") but whose `.count` field has been driven down to 1 (the exact + * post-corruption shape bug-1688's OTHER root cause -- the NULL-nv->val + * WORKERS_PUSH overwrite, core_events.c -- can leave resvals in, and + * reproducible directly here without needing that whole mechanism) must + * still show BOTH resource lines, not silently truncate to the first one. + */ +static void test_info_worker_shows_all_populated_resources_when_count_is_stale( + void) +{ + xce_sched_sim_driver_t sim; + xce_core_event_t events[1]; + xce_resource_t wire_mem; + xce_resource_t wire_r1m; + xce_worker_t wire_worker; + xce_worker_t* worker_ptrs[1]; + xce_val_t val_mem; + xce_val_t val_r1m; + xce_name_val_t nv_mem; + xce_name_val_t nv_r1m; + xce_name_val_t* nv_ptrs[2]; + xce_sched_worker_t* sw; + char* captured; + int rc; + + memset(&sim, 0, sizeof(sim)); + rc = xce_sched_core_init(&(sim.core)); + TEST_ASSERT_EQUAL_INT(0, rc); + + memset(&wire_mem, 0, sizeof(wire_mem)); + wire_mem.name = "mem"; + TEST_ASSERT_NOT_NULL(makeResource(sim.core.resmgr, &wire_mem)); + memset(&wire_r1m, 0, sizeof(wire_r1m)); + wire_r1m.name = "r1m"; + TEST_ASSERT_NOT_NULL(makeResource(sim.core.resmgr, &wire_r1m)); + + memset(&val_mem, 0, sizeof(val_mem)); + val_mem.which = Value_u_fvalue; + val_mem.u.fvalue = 100.0; + memset(&nv_mem, 0, sizeof(nv_mem)); + nv_mem.name = "mem"; + nv_mem.val = &val_mem; + + memset(&val_r1m, 0, sizeof(val_r1m)); + val_r1m.which = Value_u_fvalue; + val_r1m.u.fvalue = 5.0; + memset(&nv_r1m, 0, sizeof(nv_r1m)); + nv_r1m.name = "r1m"; + nv_r1m.val = &val_r1m; + + nv_ptrs[0] = &nv_mem; + nv_ptrs[1] = &nv_r1m; + + memset(&wire_worker, 0, sizeof(wire_worker)); + wire_worker.name = "hostA"; + wire_worker.status = 0; + wire_worker.nresvals_to_sched = 2; + wire_worker.resvals_to_sched = nv_ptrs; + worker_ptrs[0] = &wire_worker; + + memset(events, 0, sizeof(events)); + events[0].type = XCE_CORE_EVENT_WORKERS_PUSH; + events[0].u.workers_push.nworkers = 1; + events[0].u.workers_push.workers = worker_ptrs; + + rc = xce_core_push_events(&(sim.core), events, 1); + TEST_ASSERT_EQUAL_INT(0, rc); + + sw = sched_get_worker(&(sim.core), "hostA"); + TEST_ASSERT_NOT_NULL(sw); + TEST_ASSERT_EQUAL_INT(2, sw->resvals.count); + + /* Directly force the sparse-vs-stale-count shape: both idx 0 ("mem") + * and idx 1 ("r1m") stay populated, but .count now under-reports the + * true occupied high-water mark -- exactly what a `.count`-bound loop + * gets wrong and a `.caps`-bound loop (this fix) does not. */ + sw->resvals.count = 1; + + captured = capture_dispatch(&sim, "info worker hostA"); + + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(captured, "resource mem: value=100.00"), + "the first populated slot must still display"); + TEST_ASSERT_NOT_NULL_MESSAGE( + strstr(captured, "resource r1m: value=5.00"), + "a populated slot at an index >= the (stale) .count must still " + "display -- iterating by .count instead of .caps (bug-1688) " + "silently truncates output here"); + + free(captured); + + printf("test_info_worker_shows_all_populated_resources_when_count_is_" + "stale: PASS\n"); +} + /* * bug-1590: repl_info_resource_one() (the "info resource " * aggregate-across-workers command) had the same defect one layer up -- @@ -1880,6 +1986,7 @@ int main(void) RUN_TEST(test_info_worker_reports_resource_values); RUN_TEST(test_info_worker_reports_consumable_value_and_used); RUN_TEST(test_info_worker_reports_string_and_boolean_resource_values); + RUN_TEST(test_info_worker_shows_all_populated_resources_when_count_is_stale); RUN_TEST(test_info_resource_reports_string_value); RUN_TEST(test_info_resource_reports_consumable_value_and_used); RUN_TEST(test_info_resource_all_lists_only_global_shared_resources);