lei.chen/myxce
1
0
Fork 1
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(scheduler): 3 --sim-verify false positives + snapshot resreq_fp gap (bug-1844/1845/1772)

Browse source 
/xce-audit-invariants scheduler dimension-1/4 audit, prompted by a fresh
user-captured .replay file that still showed massive --sim-verify mismatch
volume after bug-1843's fix. Root-caused via temporary diagnostic
instrumentation against the real capture (reverted, never committed) rather
than trusting the surface symptom.

bug-1844: sim_verify_alloc_eq() (sim_verify.c) compared `instid`, a bare
per-process monotonic counter (scheduler.c's sched_get_id(), core->nextid)
stamped at allocation-build time -- not semantic content. The original
recording's counter and a fresh replay's counter have no reason to reach
the same value for "the same" dispatch. Live-verified: every DISPATCH
mismatch showed identical value/duration/resname/reserve, differing only
in instid (e.g. 826 vs 1). Dropped from the comparison, same treatment as
bug-1842's `from` exclusion.

bug-1845: sim_verify_job_preason_eq() required PEND decisions' full
host/resource reason-detail lists to match. core.h documents "simulation
isolation" as deliberate design: sim/replay never registers
core->unsatisfiable_lookup_cb, so it can never hit the unsatisfiable-cache
main.c's normal-mode branch registers. A live cache hit
(sched_pend_from_cached_reason_()) records only main_reason with zero
detail; replay always falls through to the full per-host computation and
always produces detail. Same semantic outcome (PEND, same main_reason) at
two architecturally-legitimate detail levels. Relaxed the comparator to
main_reason only. Separately verified the cache itself is sound (key is
safe within a round since capacity only decreases and the cache clears
every round) -- ruling it out as the cause of a second, still-open
divergence (bug-1846: one job genuinely dispatches one round early in
replay vs. the original recording -- a suspected resource-accounting
parity gap, not yet root-caused, logged for follow-up).

bug-1772 (previously deferred, incorrectly tagged "fixed"): re-found
independently by a dimension-4 snapshot-field-completeness audit subagent.
snapshot_build_primary_schedobj() (snapshot_load.c) never computed
resreq_fp, unlike its declared model core_build_primary_schedobj()
(core_events.c) -- every snapshot-restored job got a zeroed fingerprint,
which alloc_cache.c's cache key folds in directly alongside
user/queue/project, so two distinct restored jobs sharing those three
fields could silently share an incorrect unsatisfiable-cache verdict.
Added the same 4-field computation the live path uses.

TDD throughout: each fix has a dedicated regression test, and each was
verified via genuine red (revert fix, confirm the new test fails) then
green (restore, full 85/85 scheduler suite). Also ran three parallel
dimension audits (core I/O isolation, picker/allocator jbuf rendezvous) --
both clean, no violations found.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Rongsong Shen 2026-09-23 13:40:16 +00:00
parent 5ee329b1b8
commit fcf119b4ac
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
6 changed files with 309 additions and 50 deletions
Download patch file Download diff file Expand all files Collapse all files

89
src/scheduler/src/sim_verify.c
View file

@ -109,6 +109,21 @@ static int sim_verify_str_eq(const char* a, const char* b)
* expiry/accounting purposes), not part of the decision's own identity --
* unlike `duration`, which IS a deterministic, request-derived value and
* stays part of this comparison.
*
* `instid` is ALSO deliberately excluded (bug-1844, same class of issue as
* `from` above): stamped from `sched->core.nextid++` (scheduler.c's
* sched_get_id()) at allocation-build time -- a bare per-process monotonic
* counter, not a semantic property of what got allocated. It counts how
* many allocation instances THIS core has built over its own entire
* execution history, so the ORIGINAL recording's counter (which may have
* been running, and building allocations, for far longer before this
* particular capture window even started) and a fresh replay's own counter
* (starting from whatever its loaded snapshot's nextid happens to be) have
* no reason to reach the same value for "the same" dispatch, even when
* every other field -- value, duration, resname, reserve, host -- matches
* exactly. Live-verified: a real captured .replay file showed every single
* DISPATCH's `instid` differing (e.g. recorded=826 vs replay=1, both
* otherwise byte-identical) while zero other fields ever differed.
*/
static int sim_verify_alloc_eq(const xce_alloc_t* a, const xce_alloc_t* b)
{
@ -119,8 +134,7 @@ static int sim_verify_alloc_eq(const xce_alloc_t* a, const xce_alloc_t* b)
return 0;
}
return (a->instid == b->instid) && (a->value == b->value)
&& (a->duration == b->duration)
return (a->value == b->value) && (a->duration == b->duration)
&& sim_verify_str_eq(a->resname, b->resname)
&& sim_verify_str_eq(a->reserve, b->reserve);
}
@ -200,62 +214,43 @@ static int sim_verify_job_alloc_eq(const xce_job_alloc_t* a,
/*
* sim_verify_job_preason_eq - xce_job_preason_t (a PEND decision's reason
* payload) equality: main_reason plus (as a deeper, not-strictly-mandatory
* but cheap-to-add check per this task's own "don't leave an obviously-
* wrong shallow check if the deep one is a few extra lines" guidance) the
* full host-based and resource-based reason lists.
* payload) equality: main_reason ONLY.
*
* bug-1845 fix: this used to also require the full host-based/resource-
* based reason detail lists to match (added as an optional "cheap to add"
* deepening of the check, never proven necessary by a dedicated test).
* That is wrong: core.h documents, as a deliberate "simulation isolation"
* design decision, that "a simulation-mode session never touches the real
* cache implementation at all" -- core->unsatisfiable_lookup_cb/
* unsatisfiable_populate_cb are registered only by main.c's normal-mode
* branch (alloc_cache.c), never by sim_loop.c. sched_make_primary_alloc_()
* (scheduler.c) consults that cache FIRST; on a hit it records only
* main_reason via sched_pend_from_cached_reason_() (no host/resource
* detail at all -- that is the entire point of the cache, skipping the
* expensive per-host iteration that would normally produce that detail).
* Replay, having no cache, always falls through to the full per-host
* computation and therefore always produces full detail. Two independent,
* architecturally-intentional detail levels for the exact same semantic
* outcome (this job is PEND, for this main reason) -- not a decision
* correctness signal, and requiring them to match produced systematic
* false MISMATCH reports on every cache-hit PEND decision in the original
* recording. Live-verified against a real captured .replay file: expected
* (recorded, cache-hit) reason showed
* {main_reason=0, nhost_reasons=0, nres_reasons=0}; actual (replay, full
* computation) showed {main_reason=0, nhost_reasons=0, nres_reasons=2} --
* main_reason matched, detail did not, and never architecturally can.
*/
static int sim_verify_job_preason_eq(const xce_job_preason_t* a,
const xce_job_preason_t* b)
{
int i;
if (a == b) {
return 1;
}
if ((a == NULL) || (b == NULL)) {
return 0;
}
if ((a->main_reason != b->main_reason)
|| (a->nhost_reasons != b->nhost_reasons)
|| (a->nres_reasons != b->nres_reasons)) {
return 0;
}
for (i = 0; i < a->nhost_reasons; i++) {
const xce_host_reason_t* ha = a->host_based_reasons[i];
const xce_host_reason_t* hb = b->host_based_reasons[i];
if (ha == hb) {
continue;
}
if ((ha == NULL) || (hb == NULL)) {
return 0;
}
if ((ha->reason != hb->reason)
|| !sim_verify_str_eq(ha->host, hb->host)) {
return 0;
}
}
for (i = 0; i < a->nres_reasons; i++) {
const xce_res_reason_t* ra = a->res_based_reasons[i];
const xce_res_reason_t* rb = b->res_based_reasons[i];
if (ra == rb) {
continue;
}
if ((ra == NULL) || (rb == NULL)) {
return 0;
}
if ((ra->reason != rb->reason)
|| !sim_verify_str_eq(ra->resource, rb->resource)
|| !sim_verify_str_eq(ra->host, rb->host)) {
return 0;
}
}
return 1;
return a->main_reason == b->main_reason;
}
/*

19
src/scheduler/src/snapshot_load.c
View file

@ -93,6 +93,25 @@ static void snapshot_build_primary_schedobj(xce_sched_obj_t* obj,
primary->reason = NULL;
primary->owner = obj;
/* bug-1772 fix (xce-audit-invariants scheduler, dimension 4): this
* function's own header comment documents it as a field-for-field port
* of core_build_primary_schedobj() (core_events.c) -- it had ported
* every field except this one. Without it, `primary->resreq_fp` stays
* all-zero (calloc'd) for every job restored from a snapshot, and
* alloc_cache.c's build_key_() folds resreq_fp directly into the
* unsatisfiable-cache key alongside borrowed user/queue/project
* identity -- two genuinely different restored jobs sharing a
* user/queue/project would collapse to the identical zero fingerprint
* and silently share (and incorrectly reuse) each other's
* unsatisfiable-cache verdict. Same formula, same field order, as
* core_build_primary_schedobj()'s own Req 7.1 comment. */
primary->resreq_fp.min_tasks = resreq->minTasks;
primary->resreq_fp.max_tasks = resreq->maxTasks;
primary->resreq_fp.select_node_id =
(resreq->select != NULL && resreq->select->root != NULL)
? resreq->select->root->node_id : 0;
primary->resreq_fp.rusage_hash = resreq->rusage_hash;
obj->u.primary = primary;
}

88
src/scheduler/testsuite/test_sim_verify.c
View file

@ -659,6 +659,42 @@ static void test_dispatch_decisions_differing_from_timestamp_still_match(void)
"PASS\n");
}
/*
* test_dispatch_decisions_differing_instid_still_match - bug-1844
* regression: `instid` (xce_alloc_t's per-process monotonic allocation-
* instance counter, stamped from `sched->core.nextid++` at allocation-build
* time -- scheduler.c's sched_get_id()) is an execution-order bookkeeping
* value, not part of a decision's semantic identity, exactly like `from`
* above (bug-1838-followup). The ORIGINAL recording's counter may have been
* running, and building allocations, for far longer before this capture
* window started; a fresh replay's own counter starts whatever its loaded
* snapshot's nextid happens to be -- the two have no reason to reach the
* same value for "the same" dispatch, even when every other field is
* byte-for-byte identical. Uses two `instid` values matching a real
* captured .replay file's own observed divergence (826 vs 1) to prove this
* is deliberate exclusion, not a fluke of small numbers happening to
* coincide.
*/
static void test_dispatch_decisions_differing_instid_still_match(void)
{
xce_alloc_t* res_a[1]
= { make_test_alloc("mem", "", 826ULL, 307200.0, 100, -1) };
xce_host_alloc_t* host_a[1] = { make_test_host_alloc("hostA", res_a, 1) };
xce_job_alloc_t* expected_alloc = make_test_job_alloc(42, host_a, 1);
xce_alloc_t* res_b[1]
= { make_test_alloc("mem", "", 1ULL, 307200.0, 100, -1) };
xce_host_alloc_t* host_b[1] = { make_test_host_alloc("hostA", res_b, 1) };
xce_job_alloc_t* actual_alloc = make_test_job_alloc(42, host_b, 1);
replay_decision_t expected = make_replay_dispatch_decision(expected_alloc);
xce_decision_t* actual = make_dispatch_decision(actual_alloc);
TEST_ASSERT_TRUE(sim_decision_eq(&expected, actual));
printf("test_dispatch_decisions_differing_instid_still_match: PASS\n");
}
static void test_pend_decisions_with_identical_content_match(void)
{
replay_decision_t expected = make_replay_pend_decision(9001ULL, 3);
@ -695,6 +731,56 @@ static void test_pend_decisions_differing_main_reason_mismatch(void)
printf("test_pend_decisions_differing_main_reason_mismatch: PASS\n");
}
/*
* test_pend_decisions_differing_reason_detail_still_match - bug-1845
* regression: the recorded (expected) side represents a live cache-hit
* PEND (core.h's documented "simulation isolation" -- sim/replay never
* registers core->unsatisfiable_lookup_cb, so it can never itself produce
* this shape, only ever receive it from a recorded normal-mode run) --
* main_reason set, zero host/resource reason detail
* (sched_pend_from_cached_reason_(), scheduler.c, never adds any). The
* live (actual) side represents replay's own full per-host computation for
* the SAME job/round -- same main_reason, but with real host- and
* resource-based reason detail attached (sched_try_alloc_on_host_(),
* resource.c). Same semantic outcome (PEND, same main reason) at two
* different, architecturally-legitimate detail levels -- must still
* compare equal.
*/
static void test_pend_decisions_differing_reason_detail_still_match(void)
{
replay_decision_t expected = make_replay_pend_decision(9001ULL, 0);
xce_pendreason_t* reason = make_pendreason(9001ULL);
xce_host_reason_t* hreasons[1];
xce_res_reason_t* rreasons[2];
xce_decision_t* actual;
hreasons[0] = (xce_host_reason_t*)calloc(1, sizeof(xce_host_reason_t));
hreasons[0]->host = strdup("workerA");
hreasons[0]->reason = 1;
rreasons[0] = (xce_res_reason_t*)calloc(1, sizeof(xce_res_reason_t));
rreasons[0]->resource = strdup("mem");
rreasons[0]->host = strdup("workerA");
rreasons[0]->reason = 2;
rreasons[1] = (xce_res_reason_t*)calloc(1, sizeof(xce_res_reason_t));
rreasons[1]->resource = strdup("slots");
rreasons[1]->host = strdup("workerA");
rreasons[1]->reason = 2;
reason->reasons->main_reason = 0;
reason->reasons->nhost_reasons = 1;
reason->reasons->host_based_reasons = hreasons;
reason->reasons->nres_reasons = 2;
reason->reasons->res_based_reasons = rreasons;
actual = make_pend_decision(reason);
TEST_ASSERT_TRUE(sim_decision_eq(&expected, actual));
printf("test_pend_decisions_differing_reason_detail_still_match: PASS\n");
}
static void test_decision_kind_mismatch_reported(void)
{
replay_decision_t expected = make_replay_pend_decision(1ULL, 0);
@ -1784,9 +1870,11 @@ int main(void)
RUN_TEST(test_dispatch_decisions_differing_host_mismatch);
RUN_TEST(test_dispatch_decisions_differing_resource_value_mismatch);
RUN_TEST(test_dispatch_decisions_differing_from_timestamp_still_match);
RUN_TEST(test_dispatch_decisions_differing_instid_still_match);
RUN_TEST(test_pend_decisions_with_identical_content_match);
RUN_TEST(test_pend_decisions_differing_jobid_mismatch);
RUN_TEST(test_pend_decisions_differing_main_reason_mismatch);
RUN_TEST(test_pend_decisions_differing_reason_detail_still_match);
RUN_TEST(test_decision_kind_mismatch_reported);
RUN_TEST(test_preempt_decision_always_reports_mismatch);
RUN_TEST(test_decision_eq_defends_against_null_inputs);

69
src/scheduler/testsuite/test_snapshot_save_load.c
View file

@ -1245,6 +1245,74 @@ test_snapshot_load_wires_real_task_count_into_resreq_primary(void)
TEST_ASSERT_EQUAL_INT(6, restored->resreq->u.primary->maxTasks);
}
/*
* bug-1772 fix (xce-audit-invariants scheduler, dimension 4): a job
* restored from a snapshot must get a genuinely-computed
* schedobj->u.primary->resreq_fp, not the all-zero calloc() default.
* snapshot_build_primary_schedobj() (snapshot_load.c) is documented as a
* field-for-field port of core_build_primary_schedobj() (core_events.c) --
* it had ported every field except this one. Without the fix, EVERY
* restored job's resreq_fp is {0,0,0,0}; alloc_cache.c's build_key_()
* folds resreq_fp directly into the unsatisfiable-cache key, so two
* distinct restored jobs sharing a user/queue/project would silently
* collapse onto the same cache entry.
*/
static void
test_snapshot_load_computes_resreq_fingerprint_for_restored_primary(void)
{
xce_sched_core_t core;
sched_snapshot_data_t in;
xce_jobs_data_t jobs_data;
xce_queue_t wire_queue;
xce_queue_t* queues[1];
xce_job_t* job;
xce_job_t* jobs[1];
xce_sched_job_t* restored;
int rc;
memset(&g_stub_state_a, 0, sizeof(g_stub_state_a));
fixture_init(&core, &g_stub_plugin_a);
memset(&wire_queue, 0, sizeof(wire_queue));
wire_queue.queue = "normal";
wire_queue.priority = 1;
wire_queue.status = QUEUE_STAT_OPEN;
queues[0] = &wire_queue;
job = make_job_with_resreq(602, JOB_STAT_PEND, "normal", "alice", "proj1",
"type == any");
job->submit->mintasks = 4;
job->submit->maxtasks = 6;
jobs[0] = job;
memset(&jobs_data, 0, sizeof(jobs_data));
jobs_data.count = 1;
jobs_data.jobs = jobs;
memset(&in, 0, sizeof(in));
in.jobs = &jobs_data;
in.queues = queues;
in.n_queues = 1;
rc = xce_sched_core_load_snapshot(&core, &in);
TEST_ASSERT_EQUAL_INT(0, rc);
restored = sched_get_job(&core, 602);
TEST_ASSERT_NOT_NULL(restored);
TEST_ASSERT_NOT_NULL(restored->schedobj);
TEST_ASSERT_EQUAL_INT(XCE_RESREQ_PRIMARY, restored->schedobj->tag);
TEST_ASSERT_NOT_NULL(restored->schedobj->u.primary);
/* The fix under test: resreq_fp must reflect the restored primary's own
* real (task 2.2-wired) task count -- not {0,0,0,0}. */
TEST_ASSERT_EQUAL_UINT32(
4, restored->schedobj->u.primary->resreq_fp.min_tasks);
TEST_ASSERT_EQUAL_UINT32(
6, restored->schedobj->u.primary->resreq_fp.max_tasks);
TEST_ASSERT_EQUAL_UINT32(
0, restored->schedobj->u.primary->resreq_fp.select_node_id);
}
/* Regression-safety test (NOT a new feature): a job whose resreq fails to
* parse on load is a pre-existing, out-of-boundary gap (design.md "Known
* limitation") that task 1.2 must NOT try to fix -- it must only make sure
@ -2545,6 +2613,7 @@ int main(void)
RUN_TEST(
test_snapshot_load_builds_schedobj_for_primary_compound_and_alter_resreq);
RUN_TEST(test_snapshot_load_wires_real_task_count_into_resreq_primary);
RUN_TEST(test_snapshot_load_computes_resreq_fingerprint_for_restored_primary);
RUN_TEST(
test_snapshot_load_leaves_schedobj_null_when_resreq_fails_to_parse);
RUN_TEST(