fix(scheduler): never elect a job without a schedobj (bug-1897)

A job whose snapshot resreq fails to parse (any replay captured before
the bug-1888 fix) is kept by xce_sched_core_load_snapshot() with no
schedobj, but was still made a picker candidate -- by the loader and, in
simulation mode, by sim_bootstrap_picker()'s picker rebuild. The
allocator thread then dereferenced the NULL schedobj in
limit_job_election() and xce_scheduler --sim crashed.

Both push sites now skip such a job (it stays in the core so a restored
allocation is still charged), and sched_allocator() skips any picked job
without a schedobj as a backstop.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

src/scheduler/src/sched_election.c

@@ -230,6 +230,16 @@ void* sched_allocator(void* arg)
             continue;
         }
 
+        /* bug-1897: election and allocation both work on job->schedobj; a
+         * job without one (its resreq failed to parse) cannot be scheduled.
+         * Skip it like a job that cannot be found. */
+        if (job->schedobj == NULL) {
+            fmt_jobid(jobidString, sizeof(jobidString), jobid);
+            ERROR("%s job %s has no scheduling object, skipped", __func__,
+                  jobidString);
+            continue;
+        }
+
         /* scheduler-sim-live-allocation spec, task 1.2: plugin_job_election()
          * is core-typed (mirrors plugin_begin_session(core) in this same
          * codebase's driver files) -- pass core directly. */

src/scheduler/src/sim_picker.c

@@ -114,6 +114,11 @@ void sim_bootstrap_picker(xce_sched_sim_driver_t* sim)
                 if ((rc < 0) || (rc == RTTI_ITER_NO_DATA)) {
                     break;
                 }
+                /* bug-1897: same skip as the load itself -- a job
+                 * without a schedobj is never a picker candidate. */
+                if (job->schedobj == NULL) {
+                    continue;
+                }
                 plugin->picker_iface->on_job_push(plugin, job);
             }
             iter_destroy(&it);

src/scheduler/src/snapshot_load.c

@@ -366,10 +366,9 @@ int xce_sched_core_load_snapshot(xce_sched_core_t*            core,
             /* scheduler-snapshot-job-allocation task 1.2 (Req 2 AC 1-4):
              * construct job->schedobj for every restored job whose resreq
              * parsed successfully -- guarded on job->resreq != NULL so the
-             * pre-existing, unrelated resreq-parse-failure path above
+             * resreq-parse-failure path above (which leaves job->resreq
-             * (which already leaves job->resreq NULL) is tolerated, not
+             * NULL) is tolerated here; such a job is kept out of the
-             * fixed, here (design.md "Load-Side Job Reconstruction", Known
+             * picker below (bug-1897). */
-             * limitation). */
             if (job->resreq != NULL) {
                 job->schedobj
                     = (xce_sched_obj_t*)calloc(1, sizeof(*(job->schedobj)));
@@ -403,7 +402,17 @@ int xce_sched_core_load_snapshot(xce_sched_core_t*            core,
 
             sched_add_job(core, job);
 
-            if (picker != NULL) {
+            /* bug-1897: a job without a schedobj (resreq parse failure
+             * above, or its calloc failing) stays in the core so its
+             * restored allocation is still charged and later events still
+             * find it, but must never become a picker candidate --
+             * sched_allocator() runs plugin_job_election() and
+             * sched_make_allocation() on job->schedobj. */
+            if (job->schedobj == NULL) {
+                ERROR("%s jobid %llu has no scheduling object; it will "
+                      "not be scheduled",
+                      __func__, (unsigned long long)wj->jobid);
+            } else if (picker != NULL) {
                 picker->picker_iface->on_job_push(picker, job);
             }
         }

src/scheduler/testsuite/meson.build

@@ -2216,6 +2216,27 @@ test('scheduler election sim fixture test',
      sched_election_sim_fixture_test, suite: 'scheduler',
      env: ['ASAN_OPTIONS=detect_leaks=0'])
 
+# bug-1897: sched_allocator() must skip a picked job whose schedobj is NULL
+# (resreq failed to parse on snapshot load) instead of dereferencing it.
+# Same real-scheduler_srcs link technique as the fixture test above.
+sched_election_null_schedobj_test_srcs = ['test_sched_election_null_schedobj.c']
+foreach f : scheduler_srcs
+  if f != 'src/main.c'
+    sched_election_null_schedobj_test_srcs += ['../' + f]
+  endif
+endforeach
+
+sched_election_null_schedobj_test = executable(
+    'sched_election_null_schedobj_test',
+    sched_election_null_schedobj_test_srcs,
+    include_directories:[incdir],
+    dependencies: [scheduler_test_deps],
+    install: false)
+
+test('scheduler election null schedobj test',
+     sched_election_null_schedobj_test, suite: 'scheduler',
+     env: ['ASAN_OPTIONS=detect_leaks=0'])
+
 # scheduler-sim-live-allocation spec, task 4.1 (Req 1.1, 1.2, 3.3, 4.2, 4.3,
 # 5.1, boundary: Unified Picker/Allocator Threads, Sim Driver): proves
 # sched_sim_driver_run() (sim_loop.c) itself -- not just the relocated

src/scheduler/testsuite/test_sched_election_null_schedobj.c

@@ -0,0 +1,115 @@
+/*
+ * test_sched_election_null_schedobj.c - bug-1897.
+ *
+ * sched_allocator() (sched_election.c) runs plugin_job_election() and
+ * sched_make_allocation() on job->schedobj. A job whose resreq could not be
+ * parsed has no schedobj (snapshot_load.c keeps such a job so a running
+ * job's allocation stays charged), and before the fix the allocator thread
+ * dereferenced the NULL schedobj (limit_job_election(), limit.c:336, in a
+ * replay; sched_make_allocation()'s obj->tag here, with no limit plugin).
+ *
+ * The allocator must skip such a job -- no decision for it -- and carry on
+ * with the session: the only decisions-cbuf entry is the end-of-session
+ * NULL marker.
+ *
+ * Fixture as in test_sched_election_sim_fixture.c (`wrapper` is a real
+ * xce_scheduler_t used only for plugin registration; the threads get the
+ * bare core). One RUN_TEST() only: the threads are never joined and
+ * reference stack-local state.
+ */
+#include <errno.h>
+#include <pthread.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include <unity/unity.h>
+
+#include "decision.h"
+#include "plugin.h"
+#include "sched/types.h"
+#include "scheduler.h"
+#include "xstring.h"
+
+#define TEST_JOBID ((uint64_t)189701ULL)
+#define QUEUE_NAME "normal"
+
+void setUp(void) { return; }
+void tearDown(void) { return; }
+
+static void test_sched_allocator_skips_job_without_schedobj(void)
+{
+    xce_scheduler_t     wrapper;
+    xce_sched_core_t*   core;
+    xce_sched_plugin_t* fcfs;
+    xce_sched_queue_t*  queue;
+    xce_sched_job_t*    job;
+    pthread_t           picker_tid;
+    pthread_t           allocator_tid;
+    xce_decision_t*     decision = (xce_decision_t*)0x1;
+    int                 rc;
+
+    memset(&wrapper, 0, sizeof(wrapper));
+    rc = xce_sched_core_init(&(wrapper.core));
+    TEST_ASSERT_EQUAL_INT(0, rc);
+    core = &(wrapper.core);
+
+    fcfs = builtin_fcfs_new_plugin();
+    TEST_ASSERT_NOT_NULL(fcfs);
+    rc = plugin_register(&wrapper, fcfs);
+    TEST_ASSERT_TRUE(rc >= 0);
+    core->job_pick_plugin = (char*)"builtin_fcfs";
+
+    rc = register_seqjob_allocator(&wrapper);
+    TEST_ASSERT_TRUE(rc >= 0);
+    rc = register_parjob_allocator(&wrapper);
+    TEST_ASSERT_TRUE(rc >= 0);
+
+    queue = (xce_sched_queue_t*)calloc(1, sizeof(*queue));
+    TEST_ASSERT_NOT_NULL(queue);
+    queue->name = makeString(QUEUE_NAME);
+    rc          = sched_add_queue(core, queue);
+    TEST_ASSERT_TRUE(rc >= 0);
+    fcfs->picker_iface->on_queue_update(fcfs, queue);
+
+    /* The shape snapshot_load.c leaves for a job whose resreq failed to
+     * parse: resreq and schedobj both NULL. Pushed to the picker directly,
+     * so this test covers the allocator guard on its own, independent of
+     * the loader no longer pushing such a job. */
+    job = (xce_sched_job_t*)calloc(1, sizeof(*job));
+    TEST_ASSERT_NOT_NULL(job);
+    job->jobid = TEST_JOBID;
+    job->state = JOB_STAT_PEND;
+    job->queue = queue;
+    job->user  = sched_get_user(core, "alice");
+
+    rc = sched_add_job(core, job);
+    TEST_ASSERT_TRUE(rc >= 0);
+    fcfs->picker_iface->on_job_push(fcfs, job);
+
+    rc = pthread_create(&picker_tid, NULL, sched_job_picker, core);
+    TEST_ASSERT_EQUAL_INT(0, rc);
+    rc = pthread_create(&allocator_tid, NULL, sched_allocator, core);
+    TEST_ASSERT_EQUAL_INT(0, rc);
+
+    rc = xce_core_go(core);
+    TEST_ASSERT_EQUAL_INT(0, rc);
+
+    cbuf_get(&(core->decisions), &decision);
+    TEST_ASSERT_NULL_MESSAGE(decision,
+                             "a job without a schedobj must be skipped: the "
+                             "only entry is the end-of-session NULL marker");
+
+    rc = xce_core_end_session(core);
+    TEST_ASSERT_EQUAL_INT(0, rc);
+
+    TEST_ASSERT_EQUAL_UINT(JOB_STAT_PEND, job->state);
+    TEST_ASSERT_NULL(job->allocs);
+}
+
+int main(void)
+{
+    UNITY_BEGIN();
+    RUN_TEST(test_sched_allocator_skips_job_without_schedobj);
+    return UNITY_END();
+}

src/scheduler/testsuite/test_sim_event_loop.c

@@ -899,11 +899,13 @@ test_run_deactivates_tlog_virtual_clock_logging_on_completion(void)
  * already white-box #included above) BEFORE calling sched_record_start() --
  * so xce_sched_core_save_snapshot() (snapshot.c) captures both into the
  * INITIAL snapshot itself, not as a later ReplayEvent. */
-static FILE* write_header_and_snapshot_with_pending_job(char*       out_path,
+static FILE*
-                                                         size_t out_path_len,
+write_header_and_snapshot_with_pending_job_resreq(char*       out_path,
-                                                         const char* suffix,
+                                                  size_t      out_path_len,
-                                                         const char* queue_name,
+                                                  const char* suffix,
-                                                         uint64_t    jobid)
+                                                  const char* queue_name,
+                                                  uint64_t    jobid,
+                                                  const char* saved_resreq)
 {
     char                 record_dir[PATH_MAX];
     xce_scheduler_t      sched;
@@ -945,6 +947,13 @@ static FILE* write_header_and_snapshot_with_pending_job(char*       out_path,
     TEST_ASSERT_NOT_NULL(job);
     TEST_ASSERT_GREATER_OR_EQUAL(0, sched_add_job(&(sched.core), job));
 
+    /* bug-1897: the snapshot saves resreq_str verbatim, so overwriting it
+     * here yields a snapshot whose resreq the loader cannot parse. */
+    if (saved_resreq != NULL) {
+        FREEUP(job->resreq_str);
+        job->resreq_str = strdup(saved_resreq);
+    }
+
     rc = sched_record_start(&sched, "fixture.replay");
     TEST_ASSERT_EQUAL_INT(0, rc);
     TEST_ASSERT_TRUE(strlen(sched.record_file_path) < out_path_len);
@@ -953,6 +962,16 @@ static FILE* write_header_and_snapshot_with_pending_job(char*       out_path,
     return sched.record_file;
 }
 
+static FILE* write_header_and_snapshot_with_pending_job(char*       out_path,
+                                                         size_t out_path_len,
+                                                         const char* suffix,
+                                                         const char* queue_name,
+                                                         uint64_t    jobid)
+{
+    return write_header_and_snapshot_with_pending_job_resreq(
+        out_path, out_path_len, suffix, queue_name, jobid, NULL);
+}
+
 /* Same dup()/dup2() STDOUT_FILENO capture technique test_sim_verify.c's own
  * run_sim_and_capture_stdout() establishes (this file's white-box TU does
  * not #include that file, so a plain copy is needed here rather than a
@@ -1065,6 +1084,44 @@ test_job_present_only_in_initial_snapshot_still_reaches_the_picker(void)
            "picker: PASS\n");
 }
 
+/*
+ * bug-1897: a replay whose INITIAL snapshot holds a pending job with a
+ * resreq the loader cannot parse (e.g. bug-1888's old debug-dump text).
+ * The loader keeps the job without a schedobj; sim_bootstrap_picker()'s
+ * picker rebuild used to push it anyway, and the allocator thread then
+ * dereferenced the NULL schedobj (SEGV in limit_job_election()). The run
+ * must complete, with no decision for that job.
+ */
+static void test_snapshot_job_with_unparsable_resreq_is_not_scheduled(void)
+{
+    char  path[PATH_MAX];
+    char  marker[80];
+    char  jobid_buf[64];
+    FILE* f;
+    int   rc;
+    char* captured;
+
+    f = write_header_and_snapshot_with_pending_job_resreq(
+        path, sizeof(path), "_badresreq", "normal", 4343ULL, "{");
+    TEST_ASSERT_NOT_NULL(f);
+
+    append_replay_session(f, 1000ULL);
+    append_end_of_stream_marker(f);
+    fclose(f);
+
+    captured = run_sim_and_capture_stdout(path, &rc);
+
+    fmt_jobid(jobid_buf, sizeof(jobid_buf), 4343ULL);
+    snprintf(marker, sizeof(marker), "jobid=%s", jobid_buf);
+
+    TEST_ASSERT_EQUAL_INT(0, rc);
+    TEST_ASSERT_NULL_MESSAGE(strstr(captured, marker),
+                             "a job whose snapshot resreq cannot be parsed "
+                             "must get no decision (bug-1897)");
+
+    free(captured);
+}
+
 int main(void)
 {
     UNITY_BEGIN();
@@ -1076,6 +1133,7 @@ int main(void)
     RUN_TEST(test_run_deactivates_tlog_virtual_clock_logging_on_completion);
     RUN_TEST(
         test_job_present_only_in_initial_snapshot_still_reaches_the_picker);
+    RUN_TEST(test_snapshot_job_with_unparsable_resreq_is_not_scheduled);
 
     return UNITY_END();
 }

src/scheduler/testsuite/test_snapshot_save_load.c

@@ -1313,11 +1313,11 @@ test_snapshot_load_computes_resreq_fingerprint_for_restored_primary(void)
         0, restored->schedobj->u.primary->resreq_fp.select_node_id);
 }
 
-/* Regression-safety test (NOT a new feature): a job whose resreq fails to
+/* Regression-safety test: a job whose resreq fails to parse on load keeps
- * parse on load is a pre-existing, out-of-boundary gap (design.md "Known
+ * job->resreq and job->schedobj NULL (schedobj construction is skipped
- * limitation") that task 1.2 must NOT try to fix -- it must only make sure
+ * rather than crashing on a NULL resreq, task 1.2), stays in the core, and
- * schedobj construction is skipped rather than crashing on a NULL resreq.
+ * -- bug-1897 -- is never pushed to the picker, so the allocator thread
- * Proves job->schedobj stays NULL, identical to today's behavior. */
+ * never elects a job with no schedobj. */
 static void
 test_snapshot_load_leaves_schedobj_null_when_resreq_fails_to_parse(void)
 {
@@ -1364,6 +1364,12 @@ test_snapshot_load_leaves_schedobj_null_when_resreq_fails_to_parse(void)
     /* THIS feature's guard: schedobj construction is skipped, not crashed
      * into, when resreq is NULL. */
     TEST_ASSERT_NULL(restored->schedobj);
+    /* bug-1897: a job with no schedobj must never become a picker
+     * candidate -- the allocator thread runs plugin_job_election() on
+     * job->schedobj (limit_job_election() dereferenced it as NULL). */
+    TEST_ASSERT_EQUAL_INT_MESSAGE(
+        0, g_stub_state_a.ncandidates,
+        "a job whose resreq failed to parse must not be pushed to the picker");
 }
 
 /* ---------------------------------------------------------------------- */