fix(scheduler): free registry entries after their tables at core teardown; fix dup_val and the unmeet re-init leak (bug-1884, part 5)

A failed key_add() runs the table's datakiller on the rejected value, and
callers keep using it (JOBS_PUSH merges a duplicate job into the existing
one; sched_init_queues() re-adds every queue on each config apply), so the
job, queue, worker and resource datakillers must stay shallow. Part 4 made
kill_sched_job() free the whole job; that is reverted. Instead
xce_sched_core_fini() collects each table's entries, destroys the table,
then frees each entry with a full destructor (core.c). The resource table
is released the same way (a reference per resobj).

Also:
- dup_val() tested which == XCE_VAL_STRING (0, the resource value type,
  equal to Value_u_ivalue32): real string values were shallow-copied and a
  nonzero int32 went to makeString() as a pointer. It now tests
  Value_u_svalue and strdup()s, as free_Value() expects.
- The shared/host satisfy checks re-initialize the unmeet vector they are
  given; the allocators passed the ctx's live one, dropping its buffers on
  every candidate host. The callers finalize it first.
- add_vres() registers a resobj via makeResource(), like every other
  resource, instead of a bare calloc(); test_limit.c drops it with
  dereference().
- plugin_register_core() rejects a duplicate name before init_fn/key_add,
  since a failed add now finalizes (frees) the plugin and callers free it
  on failure.
- sharedInUse gets kill_sched_resval, as put_resvals() fills it.

Exit-time leak on the reproduction replay 7,469 -> 928 bytes.

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

src/lib/testsuite/test_limit.c

@@ -660,11 +660,10 @@ static void test_fmt_limit_wrapper_matches_fmt_limit_units_raw_mode(void)
  * private and its existing white-box test harness stubs get_resvals()/
  * put_resvals() as no-ops, see test_repl_limit_table.c).
  *
- * add_vres() (xcelimit.c) key_add()s the limit's own new "limit=<name>"
- * vres into resmgr with no datakiller, so each test frees that one entry
- * by hand before tearing down the shared make_test_resmgr() fixture --
- * mirroring free_test_resmgr()'s own manual-free pattern for its 8
- * pre-registered resources.
+ * add_vres() (xcelimit.c) registers the limit's own new "limit=<name>"
+ * vres resobj in resmgr, and this fixture's resmgr has no datakiller, so
+ * each test drops that one entry by hand before tearing down the shared
+ * make_test_resmgr() fixture.
  */
 static void free_instantiated_vres(var resmgr, xce_vres_t* vres)
 {
@@ -674,8 +673,7 @@ static void free_instantiated_vres(var resmgr, xce_vres_t* vres)
 
     if (vres->vres != NULL) {
         key_del(resmgr, (void*)vres->vres->name);
-        FREEUP(vres->vres->name);
-        FREEUP(vres->vres);
+        dereference(vres->vres); /* a resobj since bug-1884 */
     }
 
     FREEUP(vres);

src/lib/xcelimit.c

@@ -8,6 +8,7 @@
 
 #include "group.h"
 #include "kvmgr.h"
+#include "resobj.h"
 #include "utils.h"
 #include "xce/types.h"
 #include "xcelimit.h"
@@ -507,40 +508,31 @@ static const char* percent_capacity_resname(const char* resname)
     return NULL;
 }
 
+/* add_vres - registers a limit's virtual resource. bug-1884: built with
+ * makeResource() like every other resmgr entry (a reference-counted
+ * resobj the manager releases), not a bare calloc(). */
 int add_vres(xce_resource_t** pvres, var resmgr, char* name, int global)
 {
-    xce_resource_t* vres = NULL;
-    int             rc;
+    xce_resource_t tmpl;
 
-    vres   = (xce_resource_t*)calloc(1, sizeof(*vres));
-    *pvres = vres;
-
-    if (vres == NULL) {
-        return -ENOMEM;
-    }
-
-    vres->name    = strdup(name);
-    vres->idx     = -1;
-    vres->desc    = NULL;
-    vres->valtype = XCE_VAL_NUMERIC;
-    vres->ordtype = XCE_ORD_DECR;
-    vres->flags   = XCE_RESF_LIMIT | XCE_RESF_CONSUMABLE | XCE_RESF_RELEASE;
+    memset(&tmpl, 0, sizeof(tmpl));
+    tmpl.name    = name;
+    tmpl.idx     = -1;
+    tmpl.valtype = XCE_VAL_NUMERIC;
+    tmpl.ordtype = XCE_ORD_DECR;
+    tmpl.flags   = XCE_RESF_LIMIT | XCE_RESF_CONSUMABLE | XCE_RESF_RELEASE;
     if (global) {
-        vres->flags |= (XCE_RESF_SHARED | XCE_RESF_GLOBAL);
+        tmpl.flags |= (XCE_RESF_SHARED | XCE_RESF_GLOBAL);
     } else {
-        vres->flags |= XCE_RESF_DEFAULT_HOST;
+        tmpl.flags |= XCE_RESF_DEFAULT_HOST;
     }
 
-    lock(resmgr, RTTI_LOCK_WRITE);
-    rc = key_add(resmgr, (void*)vres->name, (void*)vres);
-    unlock(resmgr);
-
-    if (rc < 0) {
-        return rc;
+    *pvres = makeResource(resmgr, &tmpl);
+    if (*pvres == NULL) {
+        return (errno > 0) ? -errno : -ENOMEM;
     }
 
-    vres->idx = rc;
-    return rc;
+    return (*pvres)->idx;
 }
 
 int instantiate_limit(xce_vres_t** pvres, var resmgr, xce_limit_t* limit,

src/scheduler/include/sched/types.h

@@ -221,7 +221,6 @@ extern int  sched_ctx_get_uncomit_shared(xvec_t**, xce_sched_ctx_t*);
 extern int  sched_ctx_get_uncomit_host(xvec_t**, xce_sched_ctx_t*, int);
 extern int  sched_ctx_uncommit_host(xce_sched_ctx_t*, int, xvec_idval_t*);
 extern void reset_sched_obj(xce_sched_obj_t*);
-extern void kill_sched_obj(xce_sched_obj_t*);
 /* item killer for xce_sched_obj_t.hlimits: frees one per-worker xlist_t
  * (never its vres entries, which the limit plugin's instance table owns). */
 extern void kill_hlimits_list(void*);

src/scheduler/src/core.c

@@ -7,7 +7,9 @@
 #include "core.h"
 #include "decision.h"
 #include "group.h"
+#include "misc.h"
 #include "plugin.h"
+#include "resreq.h"
 #include "sched/types.h"
 #include "xstring.h"
 
@@ -196,11 +198,159 @@ int xce_sched_core_init(xce_sched_core_t* core)
 
     zarray_init(&(core->sharedInst), 128,
                 (zarray_item_killer_t)kill_sched_resval);
-    zarray_init(&(core->sharedInUse), 128, NULL);
+    /* bug-1884: put_resvals() fills this with values it allocates */
+    zarray_init(&(core->sharedInUse), 128,
+                (zarray_item_killer_t)kill_sched_resval);
 
     return 0;
 }
 
+/* bug-1884: full destructors for the job, queue and worker tables' entries,
+ * used only by xce_sched_core_fini() after the table (and its shallow
+ * datakiller) is gone -- see core_destroy_and_free_(). Here rather than in
+ * schedobj.c so the core's teardown links on its own. */
+static void core_schedobj_free_(xce_sched_obj_t* obj)
+{
+    int i;
+
+    if (obj == NULL) {
+        return;
+    }
+
+    if (obj->user != NULL) {
+        dereference(obj->user);
+    }
+    if (obj->queue != NULL) {
+        dereference(obj->queue);
+    }
+    if (obj->project != NULL) {
+        dereference(obj->project);
+    }
+
+    xlist_fini(&(obj->glimits));
+    zarray_fini(&(obj->hlimits));
+
+    switch (obj->tag) {
+    case XCE_RESREQ_PRIMARY:
+        if (obj->u.primary != NULL) {
+            free_PendReason_ptr(&(obj->u.primary->reason));
+            free(obj->u.primary);
+        }
+        break;
+    case XCE_RESREQ_COMPOUND:
+        if (obj->u.compound != NULL) {
+            free_PendReason_ptr(&(obj->u.compound->reason));
+            free(obj->u.compound);
+        }
+        break;
+    case XCE_RESREQ_ALTER:
+        for (i = 0; i < obj->u.alters.count; i++) {
+            xce_compound_schedobj_t* cobj
+                = (xce_compound_schedobj_t*)xlist_get(&(obj->u.alters), i);
+
+            if (cobj != NULL) {
+                free_PendReason_ptr(&(cobj->reason));
+                free(cobj);
+            }
+        }
+        xlist_fini(&(obj->u.alters));
+        break;
+    default:
+        break;
+    }
+
+    free_JobAlloc_ptr(&(obj->alloc));
+    free_PendReason_ptr(&(obj->reason));
+    free(obj);
+}
+
+/* the schedobj's resreqs point into job->resreq, freed after it */
+static void core_job_free_(xce_sched_job_t* job)
+{
+    free_JobRusages_ptr(&(job->rusages));
+    core_schedobj_free_(job->schedobj);
+    if (job->resreq != NULL) {
+        kill_resreq(job->resreq);
+        free(job->resreq);
+    }
+    free_JobAlloc_ptr(&(job->allocs));
+    free_job_account(job->accounts);
+    free(job);
+}
+
+static void core_queue_free_(xce_sched_queue_t* queue)
+{
+    if (queue->name != NULL) {
+        dereference(queue->name);
+    }
+    if (queue->resreq != NULL) {
+        dereference(queue->resreq);
+    }
+    free(queue);
+}
+
+/* kill_sched_worker(), the table's datakiller, released the host eval
+ * context already */
+static void core_worker_free_(xce_sched_worker_t* worker)
+{
+    if (worker->name != NULL) {
+        dereference(worker->name);
+    }
+    zarray_fini(&(worker->resvals));
+    zarray_fini(&(worker->resInUse));
+    free(worker);
+}
+
+/* core_destroy_and_free_ - bug-1884: collects every entry of the registry
+ * *mgr, destroys the registry (running its datakiller on each entry), then
+ * calls free_fn on each entry. */
+static void core_destroy_and_free_(var* mgr, void (*free_fn)(void*))
+{
+    iterator_t it;
+    void**     items = NULL;
+    int        n     = 0;
+    int        cap   = 0;
+    int        i;
+
+    if (*mgr == NULL) {
+        return;
+    }
+
+    if (iter_make(&it, *mgr) >= 0) {
+        for (;;) {
+            void* item = NULL;
+            int   rc   = iter_next(&it, &item);
+
+            if ((rc < 0) || (rc == RTTI_ITER_NO_DATA)) {
+                break;
+            }
+            if (item == NULL) {
+                continue;
+            }
+            if (n == cap) {
+                int    ncap = cap ? (cap * 2) : 64;
+                void** tmp  = (void**)realloc(items, ncap * sizeof(*tmp));
+
+                if (tmp == NULL) {
+                    break; /* OOM at exit: the rest stays unfreed */
+                }
+                items = tmp;
+                cap   = ncap;
+            }
+            items[n++] = item;
+        }
+        iter_destroy(&it);
+    }
+
+    destroy(*mgr);
+    *mgr = NULL;
+
+    for (i = 0; i < n; i++) {
+        free_fn(items[i]);
+    }
+    free(items);
+}
+
 void xce_sched_core_fini(xce_sched_core_t* core)
 {
     if (core == NULL) {
@@ -222,23 +372,18 @@ void xce_sched_core_fini(xce_sched_core_t* core)
     core->pend_updates[1] = NULL;
 
     /* bug-1884: every registry xce_sched_core_init() makes. Plugins go
-     * first -- their fini hooks may still read the entity tables. */
+     * first -- their fini hooks may still read the entity tables. The job,
+     * worker, queue and resource tables keep shallow datakillers (a failed
+     * add runs them too, and callers keep using that value), so their
+     * entries are collected, the table destroyed, then each entry freed. */
     if (core->plugins != NULL) {
         destroy(core->plugins);
         core->plugins = NULL;
     }
-    if (core->jobs != NULL) {
-        destroy(core->jobs);
-        core->jobs = NULL;
-    }
-    if (core->workers != NULL) {
-        destroy(core->workers);
-        core->workers = NULL;
-    }
-    if (core->queues != NULL) {
-        destroy(core->queues);
-        core->queues = NULL;
-    }
+    core_destroy_and_free_(&(core->jobs), (void (*)(void*))core_job_free_);
+    core_destroy_and_free_(&(core->workers),
+                           (void (*)(void*))core_worker_free_);
+    core_destroy_and_free_(&(core->queues), (void (*)(void*))core_queue_free_);
     if (core->users != NULL) {
         destroy(core->users);
         core->users = NULL;
@@ -250,7 +395,7 @@ void xce_sched_core_fini(xce_sched_core_t* core)
     zarray_fini(&(core->sharedInst));
     zarray_fini(&(core->sharedInUse));
 
-    destroy(core->resmgr);
+    core_destroy_and_free_(&(core->resmgr), (void (*)(void*))dereference);
 
     destroy(core->ugmgr);
     destroy(core->qgmgr);

src/scheduler/src/host_iter.c

@@ -155,6 +155,8 @@ xce_sched_worker_t* sched_host_iter_next(xce_host_iter_t* iter)
                 continue;
             }
 
+            /* bug-1884: the check (re)initializes unmeet itself */
+            xvec_idval_fini(&(ctx->unmeet));
             rc = sched_check_host_satisfy(core, w, uncomit,
                                           &(ctx->alloc_req.host),
                                           &(ctx->unmeet));

src/scheduler/src/plugin.c

@@ -189,6 +189,22 @@ int plugin_register_core(xce_sched_core_t* core, xce_sched_plugin_t* plugin)
      * plugin registered via this entry point had no way to reach shared
      * core state at all. plugin->sched is deliberately left NULL, exactly
      * as before. */
+    /* bug-1884: reject a duplicate name before init_fn and key_add(): a
+     * failed key_add() runs the registry's datakiller (plugin_unregister(),
+     * which frees the plugin), and callers free the plugin themselves on
+     * any failure. */
+    {
+        void* existing = NULL;
+
+        lock(core->plugins, RTTI_LOCK_READ);
+        rc = key_find(core->plugins, (void*)plugin->name, &existing);
+        unlock(core->plugins);
+        if ((rc > 0) && (existing != NULL)) {
+            return -EEXIST;
+        }
+        rc = 0;
+    }
+
     plugin->core = core;
 
     if (plugin->init_fn != NULL) {

src/scheduler/src/plugins/parjob_allocator.c

@@ -609,6 +609,8 @@ static int my_alloc(xce_sched_plugin_t* plugin, xce_sched_ctx_t* ctx)
      * takes core directly (already core-typed since task 1.3) -- read
      * from the plugin's own honest `core` field, not derived from
      * plugin->sched. */
+    /* bug-1884: the check (re)initializes unmeet itself */
+    xvec_idval_fini(&(ctx->unmeet));
     rc = sched_check_shared_satisfy(core, uncomit,
                                     &(ctx->alloc_req.shared), &(ctx->unmeet));
     if (rc < 0) {

src/scheduler/src/plugins/seqjob_allocator.c

@@ -248,6 +248,8 @@ static int my_alloc(xce_sched_plugin_t* plugin, xce_sched_ctx_t* ctx)
      * takes core directly (already core-typed since task 1.3) -- read
      * from the plugin's own honest `core` field, not derived from
      * plugin->sched. */
+    /* bug-1884: the check (re)initializes unmeet itself */
+    xvec_idval_fini(&(ctx->unmeet));
     rc = sched_check_shared_satisfy(core, uncomit,
                                     &(ctx->alloc_req.shared), &(ctx->unmeet));
     if (rc < 0) {

src/scheduler/src/resource.c

@@ -537,6 +537,8 @@ int sched_try_alloc_on_host_(xce_sched_worker_t* w, xce_sched_core_t* core,
         return rc;
     }
 
+    /* bug-1884: the check (re)initializes unmeet itself */
+    xvec_idval_fini(&(ctx->unmeet));
     rc = sched_check_host_satisfy(core, w, uncomit,
                                   &(ctx->alloc_req.host), &(ctx->unmeet));
     if (rc < 0) {

src/scheduler/src/schedobj.c

@@ -5,8 +5,6 @@
 
 #include "controller.capnp.h"
 #include "kvmgr.h"
-#include "misc.h"
-#include "resreq.h"
 #include "sched/types.h"
 #include "scheduler.h"
 #include "utils.h"
@@ -16,67 +14,6 @@
 #include "xvec.h"
 #include "xstring.h"
 
-/* kill_sched_obj - bug-1884: frees a job's schedobj: its interned names,
- * limit lists, the per-primary/compound wrappers and their pend reasons,
- * and the allocation/pend reason of its last attempt. The resreqs the
- * wrappers point at belong to the job (job->resreq). */
-void kill_sched_obj(xce_sched_obj_t* obj)
-{
-    int i;
-
-    if (obj == NULL) {
-        return;
-    }
-
-    if (obj->user != NULL) {
-        dereference(obj->user);
-    }
-    if (obj->queue != NULL) {
-        dereference(obj->queue);
-    }
-    if (obj->project != NULL) {
-        dereference(obj->project);
-    }
-
-    xlist_fini(&(obj->glimits));
-    zarray_fini(&(obj->hlimits));
-
-    switch (obj->tag) {
-    case XCE_RESREQ_PRIMARY:
-        if (obj->u.primary != NULL) {
-            free_PendReason_ptr(&(obj->u.primary->reason));
-            FREEUP(obj->u.primary);
-        }
-        break;
-    case XCE_RESREQ_COMPOUND:
-        if (obj->u.compound != NULL) {
-            free_PendReason_ptr(&(obj->u.compound->reason));
-            FREEUP(obj->u.compound);
-        }
-        break;
-    case XCE_RESREQ_ALTER:
-        for (i = 0; i < obj->u.alters.count; i++) {
-            xce_compound_schedobj_t* cobj
-                = (xce_compound_schedobj_t*)xlist_get(&(obj->u.alters), i);
-
-            if (cobj != NULL) {
-                free_PendReason_ptr(&(cobj->reason));
-                free(cobj);
-            }
-        }
-        xlist_fini(&(obj->u.alters));
-        break;
-    default:
-        break;
-    }
-
-    free_JobAlloc_ptr(&(obj->alloc));
-    free_PendReason_ptr(&(obj->reason));
-    free(obj);
-}
-
-/* kill_sched_job - datakiller of core->jobs (core.c). bug-1884: frees the
- * whole job, not only its rusage report (Req 6 / bug-765 G-5). */
 void kill_sched_job(void* arg)
 {
     xce_sched_job_t* job = (xce_sched_job_t*)arg;
@@ -85,15 +22,9 @@ void kill_sched_job(void* arg)
         return;
     }
 
+    /* release the stored rusage report, if any (Req 6 / bug-765 G-5).
+     * free_JobRusages_ptr is NULL-safe and clears the field. */
     free_JobRusages_ptr(&job->rusages);
-    kill_sched_obj(job->schedobj);
-    if (job->resreq != NULL) {
-        kill_resreq(job->resreq);
-        FREEUP(job->resreq);
-    }
-    free_JobAlloc_ptr(&(job->allocs));
-    free_job_account(job->accounts);
-    free(job);
 }
 
 request_res_t* make_resource_request(char* res, float value)

src/scheduler/src/snapshot_wire.c

@@ -48,10 +48,15 @@ xce_val_t* dup_val(const xce_val_t* src)
         return NULL;
     }
 
+    /* bug-1884: `which` is the wire union tag, so a string is
+     * Value_u_svalue -- XCE_VAL_STRING (0) is the resource value TYPE and
+     * equals Value_u_ivalue32, so strings were shallow-copied (shared with
+     * the source) and a nonzero int32 was passed to makeString() as a
+     * pointer. The copy is strdup()'d: its owners release it with
+     * free_Value(), which free()s the string. */
     v->which = src->which;
-    if (src->which == XCE_VAL_STRING) {
-        v->u.svalue
-            = (src->u.svalue != NULL) ? makeString(src->u.svalue) : NULL;
+    if (src->which == Value_u_svalue) {
+        v->u.svalue = (src->u.svalue != NULL) ? strdup(src->u.svalue) : NULL;
     } else {
         v->u = src->u;
     }