diff --git a/.wolf/buglog.json b/.wolf/buglog.json index 46899fab..f61e4f44 100644 --- a/.wolf/buglog.json +++ b/.wolf/buglog.json @@ -21436,15 +21436,15 @@ "error_message": "OPEN, NOT A CRASH -- known residual gap explicitly carved out of bug-1789's fix, filed for its own tracking per user request. A hypothetical NEW global limit defined on a per-host resource OTHER than \"slots\" (e.g. a future absolute-value global mem cap, as opposed to the existing percent-based host-scoped mem limits in limits.yaml) would silently under-enforce the same way bug-1789's slots limits did, for a different reason than bug-1789's root cause.", "file": "src/lib/resreq.c", "root_cause": "bug-1789 fixed sched_fill_limit_requests()'s READ side: it now picks whichever of alloc_req->shared/alloc_req->host to read a limit's underlying resource's want from by checking that resource's own XCE_RESF_GLOBAL flag on vres->res (and separately picks the WRITE destination for the limit's own vres entry by checking vres->vres's flag) -- see resource.c:928-955. That fix only works because alloc_req->host already has SOME value for \"slots\" to read, staged by seqjob_prepare_allocation()/parjob_prepare_allocation() via a separate, unconditional step outside the normal rusages loop, in EVERY phase including the global one. No other per-host resource gets this special treatment. The general population path -- calc_wanted_global_resvals() (src/lib/resreq.c:734) and calc_wanted_host_resvals() (src/lib/resreq.c:793), both called from seqjob_allocator.c/parjob_allocator.c's prepare_allocation hooks -- each unconditionally skip any resource whose XCE_RESF_GLOBAL flag doesn't match their own phase (calc_wanted_global_resvals() line 763: `if ((r->flags & XCE_RESF_GLOBAL) == 0) continue;`; calc_wanted_host_resvals() line 822: `if ((r->flags & XCE_RESF_GLOBAL) != 0) continue;`), and only calc_wanted_global_resvals() runs during the ctx->worker==NULL global phase. So for any per-host resource other than \"slots\", alloc_req->host is simply never populated with a want during the global phase -- sched_fill_limit_requests()'s now-correct read-side dispatch would read a genuine zero from an empty vector, not a population bug on its own side, but the net effect (a global limit on a per-host resource never seeing a nonzero want) is the same silent under-enforcement bug-1789 fixed for slots.", - "fix": "NOT FIXED -- deliberately out of scope, no limit in the current limits.yaml needs it (the two mem-based limits, night_mem_pct/limits2, are both host-scoped, unaffected either way). Suggested direction (per user note, 2026-09-07): apply the SAME flag-based-dispatch pattern bug-1789 already established on the read side to the population/write side too -- i.e. during the global phase, also check each matched global limit's underlying resource's own XCE_RESF_GLOBAL flag (vres->res->flags, the same check sched_fill_limit_requests() already performs) and, for any resource that flag marks non-global, additionally invoke calc_wanted_host_resvals() (or an equivalent per-resource want computation) to populate alloc_req->host for it before sched_fill_limit_requests() reads it -- rather than relying on a resource-specific special case like slots's separate staging step. This is a materially larger change than bug-1789's (touches the allocator plugins' prepare_allocation hooks, not just resource.c) and should get its own TDD cycle if/when a real limit needs it.", + "fix": "FIXED (2026-09-07, follow-up session, user-driven design). Corrected an earlier, wrong candidate fix (calling calc_wanted_host_resvals() during the global phase) identified during design discussion as invalid: that function's PER_HOST formula returns one host's own share (v->value, no multiplier) and, for parjob specifically, its nslots argument is reset PER BUCKET (parjob_allocator.c's parjob_try_plan_(), ctx->nslots = plan->bucket_slots[b], set right before each candidate host is tried) -- neither matches what a GLOBAL limit needs, which is the job-WIDE total, not any one host's/bucket's share.\n\nREAL FIX: added calc_wanted_host_resval_job_totals() (src/lib/resreq.c), a new function for resources NOT flagged XCE_RESF_GLOBAL that computes the JOB-WIDE TOTAL want using the SAME formulas calc_wanted_global_resvals() already uses for GLOBAL resources (PER_TASK: value*nslots, PER_JOB: value, PER_HOST: value*nhosts -- nslots/nhosts being the whole job's totals, set during the global phase, not any bucket's share). Both functions now share a common static helper (calc_wanted_job_total_resvals_()) parameterized only by which flag direction to match, avoiding duplicating the ~30-line loop body. seqjob_prepare_allocation()/parjob_prepare_allocation()'s global-phase branch (ctx->worker == NULL) now calls this new function into ctx->alloc_req.host immediately alongside the existing calc_wanted_global_resvals() call into .shared.\n\nSAFETY, traced precisely rather than assumed: plugin_prepare_allocation() (plugin.c:607-613) resets ONLY .shared at the top of a global-phase call and ONLY .host at the top of a per-host call -- never the other -- so .host is NOT reset when this new write happens. sched_fill_limit_requests() for obj->glimits runs later in that SAME plugin_prepare_allocation() invocation (plugin.c:644-647), reading and transferring the value into the limit's own permanent .shared record BEFORE any later per-host call ever resets .host. The value is therefore fully consumed and irrelevant by the time .host gets correctly wiped and repopulated with real per-host/per-bucket data for the actual capacity check -- two different consumers at two different times, not a conflict, confirmed by reading parjob_allocator.c's own per-bucket reset code directly (not inferred).\n\nTDD: two new regression tests calling seqjob_prepare_allocation()/parjob_prepare_allocation() directly with ctx.worker == NULL and a \"mem\"-named (non-\"slots\", non-global) rusage entry -- test_seqjob_prepare_allocation_global_phase_stores_host_resource_job_total (test_seqjob_allocator.c) and test_parjob_prepare_allocation_global_phase_stores_host_resource_job_total (test_parjob_combinatorial.c, maxTasks=3 specifically to prove the formula uses the job's WHOLE nslots=3, giving 2.0*3=6.0, not any bucket's share). Both assert .shared stays empty (mem correctly excluded, not globally flagged) and .host carries both the new mem entry and the pre-existing unconditional \"slots\" entry. Both PASS standalone with full Unity PASS lines confirmed (not just suite-level OK).\n\nFull `xce-build.sh all asan --incremental`: 92/94 Ok. The 2 failures are both pre-existing and unrelated, confirmed by cross-referencing this exact buglog: test_job_rusage_reconciles_mem_reservation_against_usage (bug-1762) and libxce test's 456-byte/29-allocation leak in test_xcealloc.c's make_alloc()/make_host_alloc() calls (bug-1733's fix note already confirms this exact leak signature -- 456 bytes, 29 allocations -- present byte-for-byte identically via an independent git-stash A/B rebuild in an earlier, unrelated session). No live limits.yaml entry exercises this path yet (both current mem-based limits are host-scoped), so this fix is unit/integration-tested but not live-deployed -- live verification should accompany the first real global limit on a non-slots per-host resource, when one is actually added.", "tags": [ "scheduler", "resource-accounting", "limits", "global-limit", "pre-existing", - "out-of-scope", - "severity-low" + "severity-low", + "fixed" ], "related_bugs": [ "bug-1789" diff --git a/src/include/resreq.h b/src/include/resreq.h index 92cee071..773534f9 100644 --- a/src/include/resreq.h +++ b/src/include/resreq.h @@ -315,6 +315,8 @@ extern int calc_wanted_global_resvals(xvec_idval_t*, var, xlist_t*, rusage_per_type_t, int, int); extern int calc_wanted_host_resvals(xvec_idval_t*, var, xlist_t*, rusage_per_type_t, int, int); +extern int calc_wanted_host_resval_job_totals(xvec_idval_t*, var, xlist_t*, + rusage_per_type_t, int, int); /* * Overwrite minTasks/maxTasks on resreq's own top-level primary with the diff --git a/src/lib/resreq.c b/src/lib/resreq.c index 811205cf..53165d44 100644 --- a/src/lib/resreq.c +++ b/src/lib/resreq.c @@ -731,9 +731,21 @@ int format_resreq(char** buf, xce_resreq_t* resreq) return rc; } -int calc_wanted_global_resvals(xvec_idval_t* resvals, var resmgr, - xlist_t* rusages, rusage_per_type_t perflag, - int nhosts, int nslots) +/* + * calc_wanted_job_total_resvals_ - shared body for calc_wanted_global_resvals() + * and calc_wanted_host_resval_job_totals(): computes each matching rusage + * entry's JOB-WIDE TOTAL want (PER_TASK: value*nslots, PER_JOB: value, + * PER_HOST: value*nhosts -- nslots/nhosts are the whole job's totals, not + * any one host's share). `match_global` selects which resources are + * "matching": nonzero processes only XCE_RESF_GLOBAL resources (the + * original calc_wanted_global_resvals() behavior), zero processes only + * non-GLOBAL (per-host) ones instead. + */ +static int calc_wanted_job_total_resvals_(xvec_idval_t* resvals, var resmgr, + xlist_t* rusages, + rusage_per_type_t perflag, + int nhosts, int nslots, + int match_global) { int i; @@ -760,8 +772,14 @@ int calc_wanted_global_resvals(xvec_idval_t* resvals, var resmgr, continue; } - if ((r->flags & XCE_RESF_GLOBAL) == 0) { - continue; + if (match_global) { + if ((r->flags & XCE_RESF_GLOBAL) == 0) { + continue; + } + } else { + if ((r->flags & XCE_RESF_GLOBAL) != 0) { + continue; + } } perflag1 = v->per_flag; @@ -790,6 +808,47 @@ int calc_wanted_global_resvals(xvec_idval_t* resvals, var resmgr, return 0; } +int calc_wanted_global_resvals(xvec_idval_t* resvals, var resmgr, + xlist_t* rusages, rusage_per_type_t perflag, + int nhosts, int nslots) +{ + return calc_wanted_job_total_resvals_(resvals, resmgr, rusages, perflag, + nhosts, nslots, 1); +} + +/* + * calc_wanted_host_resval_job_totals - bug-1790: like + * calc_wanted_global_resvals(), but for resources NOT flagged + * XCE_RESF_GLOBAL (per-host resources) -- computes each such resource's + * JOB-WIDE TOTAL want, not any one host's own share (calc_wanted_host_ + * resvals(), below, computes that instead, for the real per-host capacity + * check). Exists so a GLOBAL limit built on top of a per-host resource + * (e.g. a future absolute global mem cap, "slots" already being the one + * bug-1789 fixed) can read a real aggregate instead of an always-missing + * value. + * + * seqjob_prepare_allocation()/parjob_prepare_allocation()'s global-phase + * branch (ctx->worker == NULL) calls this into ctx->alloc_req.host + * immediately alongside its existing calc_wanted_global_resvals() call. + * This is safe despite .host normally being per-host/per-bucket-varying + * data (see parjob_allocator.c's own per-bucket reset): plugin_prepare_ + * allocation() (plugin.c) only resets .host at the START of a PER-HOST + * call (ctx->worker != NULL), never during the global phase, so the value + * written here survives long enough for sched_fill_limit_requests() + * (resource.c) to read it -- called later in the SAME global-phase + * invocation, for obj->glimits -- before any later per-host call wipes + * and correctly repopulates .host with that specific host's own share via + * calc_wanted_host_resvals(). + */ +int calc_wanted_host_resval_job_totals(xvec_idval_t* resvals, var resmgr, + xlist_t* rusages, + rusage_per_type_t perflag, int nhosts, + int nslots) +{ + return calc_wanted_job_total_resvals_(resvals, resmgr, rusages, perflag, + nhosts, nslots, 0); +} + int calc_wanted_host_resvals(xvec_idval_t* resvals, var resmgr, xlist_t* rusages, rusage_per_type_t perflag, int nhosts, int nslots) diff --git a/src/scheduler/src/plugins/parjob_allocator.c b/src/scheduler/src/plugins/parjob_allocator.c index fa37f028..cd344122 100644 --- a/src/scheduler/src/plugins/parjob_allocator.c +++ b/src/scheduler/src/plugins/parjob_allocator.c @@ -116,6 +116,15 @@ static int parjob_prepare_allocation(void* arg, xce_sched_ctx_t* ctx) calc_wanted_global_resvals(&(ctx->alloc_req.shared), sched->core.resmgr, &(primary->rusages), PER_TASK, ctx->nhosts, ctx->nslots); + /* bug-1790: also compute this job's job-wide total want for any + * per-host resource in its own rusages (e.g. a future global limit + * built on top of "mem") -- see calc_wanted_host_resval_job_totals()'s + * own comment (resreq.c) for why writing into .host here, during the + * global phase, is safe. */ + calc_wanted_host_resval_job_totals(&(ctx->alloc_req.host), + sched->core.resmgr, + &(primary->rusages), PER_TASK, + ctx->nhosts, ctx->nslots); } else { calc_wanted_host_resvals(&(ctx->alloc_req.host), sched->core.resmgr, &(primary->rusages), PER_TASK, ctx->nhosts, diff --git a/src/scheduler/src/plugins/seqjob_allocator.c b/src/scheduler/src/plugins/seqjob_allocator.c index 134117d3..4e3675ec 100644 --- a/src/scheduler/src/plugins/seqjob_allocator.c +++ b/src/scheduler/src/plugins/seqjob_allocator.c @@ -84,6 +84,15 @@ static int seqjob_prepare_allocation(void* arg, xce_sched_ctx_t* ctx) ctx->nslots = 1; calc_wanted_global_resvals(&(ctx->alloc_req.shared), sched->core.resmgr, &(primary->rusages), PER_TASK, 1, 1); + /* bug-1790: also compute this job's job-wide total want for any + * per-host resource in its own rusages (e.g. a future global limit + * built on top of "mem") -- see calc_wanted_host_resval_job_totals()'s + * own comment (resreq.c) for why writing into .host here, during the + * global phase, is safe. */ + calc_wanted_host_resval_job_totals(&(ctx->alloc_req.host), + sched->core.resmgr, + &(primary->rusages), PER_TASK, 1, + 1); } else { calc_wanted_host_resvals(&(ctx->alloc_req.host), sched->core.resmgr, &(primary->rusages), PER_TASK, 1, 1); diff --git a/src/scheduler/testsuite/test_parjob_combinatorial.c b/src/scheduler/testsuite/test_parjob_combinatorial.c index e4717914..44021928 100644 --- a/src/scheduler/testsuite/test_parjob_combinatorial.c +++ b/src/scheduler/testsuite/test_parjob_combinatorial.c @@ -1134,6 +1134,93 @@ static void test_shared_resource_debited_once_regardless_of_host_count(void) sched_ctx_fini(&ctx); } +/* + * bug-1790: same regression as test_seqjob_allocator.c's own sibling test, + * for parjob's prepare_allocation hook -- a GLOBAL limit built on a + * per-host resource OTHER than "slots" (e.g. a future global mem cap) + * needs ctx->alloc_req.host to carry that resource's JOB-WIDE TOTAL want + * during the global phase (ctx->worker == NULL) so sched_fill_limit_ + * requests() (resource.c) can read it. Uses maxTasks=3 (nslots=3 during + * the global phase) specifically to prove the PER_TASK formula uses the + * job's WHOLE nslots, not any one bucket's share (parjob_try_plan_() only + * assigns a per-bucket nslots later, in the per-host phase -- unaffected + * by this test, called directly with ctx.worker == NULL). + */ +static void +test_parjob_prepare_allocation_global_phase_stores_host_resource_job_total( + void) +{ + xce_scheduler_t sched; + xce_resource_t* slots_res; + xce_resource_t* mem_res; + xce_resource_t mem_res_in = { 0 }; + xce_sched_obj_t obj; + xce_primary_schedobj_t primary_obj; + xce_primary_resreq_t primary; + xce_sched_ctx_t ctx; + xce_sched_plugin_t plugin; + rusage_value_t rv; + int rc; + + memset(&sched, 0, sizeof(sched)); + TEST_ASSERT_EQUAL_INT(0, xce_sched_core_init(&(sched.core))); + + slots_res = register_slots_resource(sched.core.resmgr); + TEST_ASSERT_NOT_NULL(slots_res); + + /* "mem" -- per-host resource (NOT XCE_RESF_GLOBAL), the shape a future + * GLOBAL limit on top of it (bug-1790) would need populated. */ + mem_res_in.name = strdup(SCHED_MEM_RESOURCE_NAME); + mem_res = makeResource(sched.core.resmgr, &mem_res_in); + TEST_ASSERT_NOT_NULL(mem_res); + + memset(&primary, 0, sizeof(primary)); + primary.minTasks = 1; + primary.maxTasks = 3; /* parjob's job, not seqjob's -- see nslots note above */ + primary.select = NULL; + + rc = xlist_init(&(primary.rusages), 4); + TEST_ASSERT_EQUAL_INT(0, rc); + + memset(&rv, 0, sizeof(rv)); + rv.res = strdup(SCHED_MEM_RESOURCE_NAME); + rv.value = 2.0f; + rv.per_flag = PER_TASK; + rc = xlist_store(&(primary.rusages), &rv); + TEST_ASSERT_TRUE(rc >= 0); + + init_primary_obj(&obj, &primary_obj, &primary); + + TEST_ASSERT_EQUAL_INT(0, sched_ctx_init(&ctx, &sched, &obj)); + + memset(&plugin, 0, sizeof(plugin)); + plugin.sched = &sched; + + ctx.worker = NULL; + rc = parjob_prepare_allocation(&plugin, &ctx); + TEST_ASSERT_EQUAL_INT(0, rc); + + /* "mem" is not XCE_RESF_GLOBAL, so it's correctly excluded from + * .shared -- only a genuinely global resource would land there. */ + TEST_ASSERT_EQUAL_INT(0, ctx.alloc_req.shared.count); + + /* THE regression assertion: .host now carries "mem"'s job-wide total + * (PER_TASK: value * nslots == 2.0 * 3, the job's WHOLE maxTasks, not + * any one bucket's share) alongside the pre-existing unconditional + * "slots" entry (also job-wide here: ctx->nslots itself, per the + * existing xvec_idval_store() call below in this same branch). */ + TEST_ASSERT_EQUAL_INT(2, ctx.alloc_req.host.count); + TEST_ASSERT_EQUAL_INT(mem_res->idx, ((int*)ctx.alloc_req.host.idxs.data)[0]); + TEST_ASSERT_EQUAL_FLOAT(6.0f, ((float*)ctx.alloc_req.host.vals.data)[0]); + TEST_ASSERT_EQUAL_INT(slots_res->idx, + ((int*)ctx.alloc_req.host.idxs.data)[1]); + TEST_ASSERT_EQUAL_FLOAT(3.0f, ((float*)ctx.alloc_req.host.vals.data)[1]); + + xlist_fini(&(primary.rusages)); + FREEUP(rv.res); + sched_ctx_fini(&ctx); +} + void setUp(void) { return; } void tearDown(void) { return; } @@ -1153,5 +1240,7 @@ int main(void) test_mid_plan_bucket_failure_rolls_back_before_later_plan_succeeds); RUN_TEST(test_returns_zero_when_every_plan_is_exhausted); RUN_TEST(test_shared_resource_debited_once_regardless_of_host_count); + RUN_TEST( + test_parjob_prepare_allocation_global_phase_stores_host_resource_job_total); return UNITY_END(); } diff --git a/src/scheduler/testsuite/test_seqjob_allocator.c b/src/scheduler/testsuite/test_seqjob_allocator.c index 09d7d8d0..fe679586 100644 --- a/src/scheduler/testsuite/test_seqjob_allocator.c +++ b/src/scheduler/testsuite/test_seqjob_allocator.c @@ -361,11 +361,100 @@ test_seqjob_prepare_allocation_guards_against_maxtasks_gt_1_fanout(void) sched_ctx_fini(&ctx); } +/* + * bug-1790: a GLOBAL limit built on a per-host resource OTHER than "slots" + * (e.g. a hypothetical global mem cap) needs ctx->alloc_req.host to carry + * that resource's JOB-WIDE TOTAL want during the global phase (ctx->worker + * == NULL) so sched_fill_limit_requests() (resource.c) can read it -- see + * that function's own bug-1789 comment. Before this fix, only "slots" got + * this treatment (via seqjob_prepare_allocation()'s own unconditional + * store below); any OTHER per-host resource declared in the job's own + * rusages (here "mem") was silently never populated into .host during the + * global phase, since calc_wanted_host_resvals() (the thing that DOES + * compute a per-host resource's want) only ever ran in the per-host + * branch. + */ +static void +test_seqjob_prepare_allocation_global_phase_stores_host_resource_job_total( + void) +{ + xce_scheduler_t sched; + xce_resource_t* slots_res; + xce_resource_t* mem_res; + xce_resource_t mem_res_in = { 0 }; + xce_sched_obj_t obj; + xce_primary_schedobj_t primary_obj; + xce_primary_resreq_t primary; + xce_sched_ctx_t ctx; + xce_sched_plugin_t plugin; + rusage_value_t rv; + int rc; + + memset(&sched, 0, sizeof(sched)); + TEST_ASSERT_EQUAL_INT(0, xce_sched_core_init(&(sched.core))); + + slots_res = register_slots_resource(sched.core.resmgr); + TEST_ASSERT_NOT_NULL(slots_res); + + /* "mem" -- per-host resource (NOT XCE_RESF_GLOBAL), same convention + * every real host-capacity resource uses -- the exact shape a future + * GLOBAL limit on top of it (bug-1790) would need populated. */ + mem_res_in.name = strdup(SCHED_MEM_RESOURCE_NAME); + mem_res = makeResource(sched.core.resmgr, &mem_res_in); + TEST_ASSERT_NOT_NULL(mem_res); + + memset(&primary, 0, sizeof(primary)); + primary.minTasks = 1; + primary.maxTasks = 1; + primary.select = NULL; + + rc = xlist_init(&(primary.rusages), 4); + TEST_ASSERT_EQUAL_INT(0, rc); + + memset(&rv, 0, sizeof(rv)); + rv.res = strdup(SCHED_MEM_RESOURCE_NAME); + rv.value = 2.0f; + rv.per_flag = PER_TASK; + rc = xlist_store(&(primary.rusages), &rv); + TEST_ASSERT_TRUE(rc >= 0); + + init_primary_obj(&obj, &primary_obj, &primary); + + TEST_ASSERT_EQUAL_INT(0, sched_ctx_init(&ctx, &sched, &obj)); + + memset(&plugin, 0, sizeof(plugin)); + plugin.sched = &sched; + + ctx.worker = NULL; + rc = seqjob_prepare_allocation(&plugin, &ctx); + TEST_ASSERT_EQUAL_INT(0, rc); + + /* "mem" is not XCE_RESF_GLOBAL, so it's correctly excluded from + * .shared -- only a genuinely global resource would land there. */ + TEST_ASSERT_EQUAL_INT(0, ctx.alloc_req.shared.count); + + /* THE regression assertion: .host now carries "mem"'s job-wide total + * (PER_TASK: value * nslots == 2.0 * 1) alongside the pre-existing + * unconditional "slots" entry -- both present, in staging order. */ + TEST_ASSERT_EQUAL_INT(2, ctx.alloc_req.host.count); + TEST_ASSERT_EQUAL_INT(mem_res->idx, ((int*)ctx.alloc_req.host.idxs.data)[0]); + TEST_ASSERT_EQUAL_FLOAT(2.0f, ((float*)ctx.alloc_req.host.vals.data)[0]); + TEST_ASSERT_EQUAL_INT(slots_res->idx, + ((int*)ctx.alloc_req.host.idxs.data)[1]); + TEST_ASSERT_EQUAL_FLOAT(1.0f, ((float*)ctx.alloc_req.host.vals.data)[1]); + + xlist_fini(&(primary.rusages)); + FREEUP(rv.res); + sched_ctx_fini(&ctx); +} + int main(void) { UNITY_BEGIN(); RUN_TEST( test_my_alloc_allocates_single_matching_host_through_shared_iterators); RUN_TEST(test_seqjob_prepare_allocation_guards_against_maxtasks_gt_1_fanout); + RUN_TEST( + test_seqjob_prepare_allocation_global_phase_stores_host_resource_job_total); return UNITY_END(); }