fix(scheduler): fix limit-plugin usergroup/scope/error-code cascade (gitea#46)

While live-verifying gitea#46, found and fixed a cascade of 5 previously
undetected bugs that together made every usergroup-scoped limit in
limits.yaml silently non-functional:

- Named usergroups/hostgroups (users.yaml/hosts.yaml) were never
  registered into the scheduler's group managers (groupmgr_add() had zero
  production callers), so any limit referencing one by name could never
  resolve real membership.
- An unconfigured limit scope (e.g. no "projects:") arrived as an
  empty-but-present capnp group, not NULL, so build_consumer() built a
  real empty group (matching no one) instead of falling back to "all".
- Three separate call sites conflated key_find()'s negative "not found"
  return with a real error, short-circuiting before ever reaching the
  "instantiate on first use" logic right below them -- the same defect
  class already fixed once before in this codebase (bug-1495).
- Two call sites passed &(sched->core.resmgr) where a plain handle was
  expected, one level of indirection too many, causing RTTI's key_find()
  to reject it as an invalid handle.

All five are live-verified: fresh cluster deploys now correctly match and
instantiate usage tracking for all 7 limits in limits.yaml on every job
submission, reproduced across multiple independent from-scratch redeploys.

A 6th, architecturally distinct issue remains open (bug-1789, logged, not
fixed): sched_fill_limit_requests() computes a job's consumption of a
slots-scoped global limit by reading the wrong request vector, so the
numeric cap itself still never binds. This needs a design decision on
intended cross-scope semantics, not a mechanical fix, so it's left for a
follow-up.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Hb6Edtj4KbS8CrgEWYdA2E

src/include/group.h

@@ -13,6 +13,7 @@ extern xce_group_t* makeGroup(var, xce_group_config_t*, char*);
 extern int          groupmgr_init(var*, int, int);
 extern void         groupmgr_fini(var);
 extern int          groupmgr_add(var, xce_group_t*);
+extern int          groupmgr_load_configs(var, int, xce_group_config_t**);
 extern xce_group_t* groupmgr_lock(var, char*, int);
 extern void         groupmgr_del(var, xce_group_t*);
 extern int          groupmgr_iter(var, xlist_t*, void*);

src/lib/group.c

@@ -303,6 +303,43 @@ int groupmgr_add(var mgr, xce_group_t* g)
     return rc;
 }
 
+/* bug-1784: registers a cluster config's NAMED group list (e.g.
+ * xce_conf4sched_t.ugroups/hgroups, sourced from users.yaml's top-level
+ * "usergroups:"/hosts.yaml's "hostgroups:") into mgr by name, via the
+ * existing makeGroup(). Without this, a group config's own name is never
+ * added to mgr's kvtab under that name, so any OTHER group config that
+ * references it as a member (e.g. a limit's per-limit consumer group,
+ * plugins/limit.c's build_consumer()) can never resolve it: make_g_member()
+ * falls back to treating the name as a literal, ungrouped member that will
+ * never match a real username/hostname. Callers (e.g. scheduler.c's
+ * sched_init_config()) must call this BEFORE building anything that may
+ * reference these groups by name. */
+int groupmgr_load_configs(var mgr, int count, xce_group_config_t** configs)
+{
+    int i;
+
+    if (mgr == NULL) {
+        return -EINVAL;
+    }
+
+    for (i = 0; i < count; i++) {
+        xce_group_config_t* config = configs[i];
+        xce_group_t*        g;
+
+        if ((config == NULL) || (config->name == NULL)
+            || (strlen(config->name) == 0)) {
+            continue;
+        }
+
+        g = makeGroup(mgr, config, config->name);
+        if (g == NULL) {
+            return -errno;
+        }
+    }
+
+    return 0;
+}
+
 xce_group_t* groupmgr_lock(var mgr, char* name, int op)
 {
     int          rc;

src/lib/testsuite/test_group.c

@@ -255,6 +255,110 @@ static void test_nest2(void)
     destroy(mgr);
 }
 
+/* bug-1784: xce_conf4sched_t's n_ugroups/ugroups (and n_hgroups/hgroups)
+ * carry the cluster's NAMED usergroups/hostgroups (e.g. users.yaml's
+ * top-level "usergroups: [{groupname: ug1, members: [user1, user2]}]")
+ * down to the scheduler, but nothing ever registered them into a group
+ * manager by name -- groupmgr_add() (the obvious candidate) has zero
+ * production callers. Left unregistered, any OTHER group config that
+ * references "ug1" as a member (e.g. a limit's own per-limit consumer
+ * group, built via build_consumer()/makeGroup() in plugins/limit.c) can
+ * never resolve it: make_g_member() calls get_group(mgr, "ug1"), finds
+ * nothing, and falls back to treating "ug1" as a literal, ungrouped member
+ * name that will never strcmp-match a real username -- so a limit scoped
+ * to a named usergroup silently never matches anyone. Live-confirmed via
+ * gitea#46 investigation: a real ug1 member ran 10 jobs against a
+ * configured cap of 8 with zero enforcement.
+ *
+ * groupmgr_load_configs() is the fix: a bulk loader for exactly the
+ * config-push shape (a count + xce_group_config_t** array) that registers
+ * each entry by name via the existing, already-correct makeGroup(), so
+ * later group configs (limits, or another named group) can resolve it via
+ * get_group() by name, the same way test_nest() above proves manual
+ * back-to-back makeGroup() calls already work.
+ */
+static void test_load_configs_registers_each_group_by_name(void)
+{
+    var                 mgr;
+    xce_group_config_t  g1 = {
+        .name    = "ug1",
+        .members = { .count = 2, .data = (char*[]) { "user1", "user2" } }
+    };
+    xce_group_config_t  g2 = {
+        .name    = "ug2",
+        .members = { .count = 1, .data = (char*[]) { "user3" } }
+    };
+    xce_group_config_t* configs[2] = { &g1, &g2 };
+    int                  rc;
+
+    groupmgr_init(&mgr, 16, 16);
+
+    rc = groupmgr_load_configs(mgr, 2, configs);
+    TEST_ASSERT_EQUAL_INT(0, rc);
+
+    TEST_ASSERT_TRUE(get_group(mgr, "ug1") != NULL);
+    TEST_ASSERT_TRUE(get_group(mgr, "ug2") != NULL);
+    TEST_ASSERT_TRUE(is_in_group(get_group(mgr, "ug1"), "user1") > 0);
+    TEST_ASSERT_TRUE(is_in_group(get_group(mgr, "ug1"), "user2") > 0);
+    TEST_ASSERT_TRUE(is_in_group(get_group(mgr, "ug1"), "user3") == 0);
+
+    destroy(mgr);
+}
+
+/* THE regression, reproduced directly at the group layer: a limit-style
+ * consumer group (empty name -- build_consumer() always leaves per-limit
+ * group configs unnamed, deriving a default name instead, config-design.md
+ * "combined scope" convention) that references a NAMED group ("ug1") as a
+ * member must resolve real membership through it, exactly as a real
+ * xce_limit_t's CONSUMER_USER group would when it references
+ * users.yaml's "ug1". Before the fix (no bulk loader called, "ug1" never
+ * registered), this fails: is_in_group() falls through to a literal
+ * strcmp("ug1", "user1") that never matches.
+ */
+static void test_load_configs_enables_group_name_as_member_reference(void)
+{
+    var                 mgr;
+    xce_group_config_t  ug1 = {
+        .name    = "ug1",
+        .members = { .count = 2, .data = (char*[]) { "user1", "user2" } }
+    };
+    xce_group_config_t* configs[1] = { &ug1 };
+    xce_group_config_t  limit_group_cfg = {
+        .members = { .count = 1, .data = (char*[]) { "ug1" } }
+    };
+    xce_group_t* limit_group;
+    int          rc;
+
+    groupmgr_init(&mgr, 16, 16);
+
+    rc = groupmgr_load_configs(mgr, 1, configs);
+    TEST_ASSERT_EQUAL_INT(0, rc);
+
+    /* mirrors build_consumer()'s own call shape: no config->name, a
+     * caller-supplied default_g_name instead. */
+    limit_group = makeGroup(mgr, &limit_group_cfg, "some_limit-ug-group");
+    TEST_ASSERT_TRUE(limit_group != NULL);
+
+    TEST_ASSERT_TRUE(is_in_group(limit_group, "user1") > 0);
+    TEST_ASSERT_TRUE(is_in_group(limit_group, "user2") > 0);
+    TEST_ASSERT_TRUE(is_in_group(limit_group, "user3") == 0);
+
+    destroy(mgr);
+}
+
+static void test_load_configs_zero_count_is_a_noop(void)
+{
+    var mgr;
+    int rc;
+
+    groupmgr_init(&mgr, 16, 16);
+
+    rc = groupmgr_load_configs(mgr, 0, NULL);
+    TEST_ASSERT_EQUAL_INT(0, rc);
+
+    destroy(mgr);
+}
+
 static void test_reconfigure_same_group_members(void)
 {
     var                mgr;
@@ -311,6 +415,9 @@ void test_group(void)
     RUN_TEST(test_nest);
     RUN_TEST(test_nest2);
     RUN_TEST(test_reconfigure_same_group_members);
+    RUN_TEST(test_load_configs_registers_each_group_by_name);
+    RUN_TEST(test_load_configs_enables_group_name_as_member_reference);
+    RUN_TEST(test_load_configs_zero_count_is_a_noop);
 
     xstring_table_fini();
 }

src/lib/testsuite/test_limit.c

@@ -65,6 +65,37 @@ static void test_build_consumer2(void)
     return;
 }
 
+/* bug-1785: capnp encodes LimitConfig's users/queues/projects/hosts as
+ * embedded GROUPS, not nullable pointers -- so a scope the YAML author
+ * never configured (e.g. no "projects:" block) still arrives here as a
+ * non-NULL xce_group_config_t with zero members/ex_members, not NULL.
+ * build_consumer() must treat that the same as a genuinely NULL g_config
+ * (fall back to the "all" group, matching everyone) rather than building
+ * a real, empty, unnamed group that matches no one -- the opposite of
+ * this function's own documented default, and the reason a limit scoped
+ * to only some of the four dimensions (e.g. users+queues but not
+ * projects+hosts, matching this repo's own limits.yaml) never matched
+ * anyone at all: its always-present, always-empty unconfigured scopes
+ * rejected every job.
+ */
+static void test_build_consumer_empty_config_defaults_to_all(void)
+{
+    xce_consumer_t*    c     = NULL;
+    var                mgr;
+    xce_group_config_t gdata = { 0 };
+    int                rc    = 0;
+
+    groupmgr_init(&mgr, 4, 4);
+    rc = build_consumer(&c, mgr, CONSUMER_PROJECT, 0, &gdata, "some_limit");
+
+    TEST_ASSERT_TRUE(rc == 1);
+    TEST_ASSERT_NOT_NULL(c->group);
+    TEST_ASSERT_TRUE(is_in_group(c->group, "any_project_name") > 0);
+
+    kill_consumer(c);
+    destroy(mgr);
+}
+
 static void test_build_limit_ok1(void)
 {
     var                umgr;
@@ -337,6 +368,7 @@ void test_limit(void)
     
     RUN_TEST(test_build_consumer1);
     RUN_TEST(test_build_consumer2);
+    RUN_TEST(test_build_consumer_empty_config_defaults_to_all);
     RUN_TEST(test_build_limit_ok1);
     RUN_TEST(test_match_limit_ok1);
     RUN_TEST(test_match_limit_ok2);

src/lib/xcelimit.c

@@ -76,6 +76,26 @@ int build_consumer(xce_consumer_t** pconsumer, var mgr,
     c->per_consumer = (per_consumer != 0) ? 1 : 0;
     c->group        = NULL;
 
+    /* bug-1785: a scope the limit's YAML never configured (e.g. no
+     * "projects:" block) still arrives here as a non-NULL g_config with
+     * zero members/ex_members -- capnp encodes LimitConfig's users/queues/
+     * projects/hosts as embedded GROUPS, not nullable pointers, so they are
+     * always present on the wire even when the YAML author left them out.
+     * Treating that empty-but-present config the same as a genuinely
+     * user-authored empty group (which would make this consumer match NO
+     * ONE) silently defeats every unconfigured scope instead of leaving it
+     * unrestricted -- the opposite of this function's own documented
+     * "NULL means match everyone" fallback below, and the reason a limit
+     * scoped to only e.g. users+queues never matched anyone at all (its
+     * always-present, always-empty projects/hosts scopes rejected every
+     * job). An operator who genuinely wants "no one" already has the
+     * builtin "empty" group to reference explicitly. */
+    if ((g_config != NULL)
+        && (g_config->members.count == 0)
+        && (g_config->ex_members.count == 0)) {
+        g_config = NULL;
+    }
+
     if (g_config != NULL) {
         char* default_g_name = NULL;
         char  consumer_group_name[2 * XCE_MAXNAME_LEN + 1];
@@ -437,7 +457,11 @@ int instantiate_limit(xce_vres_t** pvres, var resmgr, xce_limit_t* limit,
 
     vres->limit = limit;
 
-    if (is_host_based_limit(resmgr, limit)) {
+    /* bug-1788: is_host_based_limit() can return a negative errno (e.g. a
+     * transient key_find() miss); checking bare truthiness treated ANY
+     * such error as "yes, host-based" (a negative int is non-zero), same
+     * mistake class as this function's own two key_find() calls above. */
+    if (is_host_based_limit(resmgr, limit) > 0) {
         shared = 0;
     }
 
@@ -446,15 +470,19 @@ int instantiate_limit(xce_vres_t** pvres, var resmgr, xce_limit_t* limit,
         return rc;
     }
 
+    /* bug-1788: same class of defect as bug-1786 (match_job_global_limit_'s
+     * own vres lookup) -- key_find() on resmgr signals "not found" as a
+     * negative rc (-ENOENT), not 0, so the preceding `if (rc<0) return rc;`
+     * always caught the FIRST-ever lookup of a given vresname before this
+     * code could ever reach add_vres()'s "not found, create it" branch
+     * below. `vres->vres` is the reliable signal instead: key_find() only
+     * ever assigns it on an actual find, and it starts NULL (vres was
+     * calloc'd above), so a failed/not-found lookup leaves it untouched. */
     lock(resmgr, RTTI_LOCK_READ);
     rc = key_find(resmgr, (void*)vresname, (void**)&(vres->vres));
     unlock(resmgr);
 
-    if (rc < 0) {
-        return rc;
-    }
-
-    if (rc == 0) {
+    if (vres->vres == NULL) {
         rc = add_vres(&(vres->vres), resmgr, vresname, shared);
         if (rc < 0) {
             FREEUP(vresname);
@@ -464,15 +492,16 @@ int instantiate_limit(xce_vres_t** pvres, var resmgr, xce_limit_t* limit,
 
     FREEUP(vresname);
 
+    /* Same reasoning as above: rely on vres->res staying NULL rather than
+     * this key_find's own rc to detect "resource not found" -- "slots" (or
+     * whatever limit->val.resname names) is expected to already exist
+     * (registered by sched_init_resources() at config load), so a genuine
+     * miss here IS a real error, just not reliably signaled by rc==0. */
     lock(resmgr, RTTI_LOCK_READ);
     rc = key_find(resmgr, (void*)limit->val.resname, (void**)&(vres->res));
     unlock(resmgr);
 
-    if (rc < 0) {
-        return rc;
-    }
-
-    if (rc == 0) {
+    if (vres->res == NULL) {
         return -ENOENT;
     }
 

src/scheduler/src/plugins/limit.c

@@ -200,7 +200,21 @@ static int match_job_global_limit_(xce_sched_plugin_t* plugin,
         return -EINVAL;
     }
 
-    if (is_host_based_limit(&(sched->core.resmgr), limit) > 0) {
+    /* bug-1787: is_host_based_limit()/instantiate_limit() both take a
+     * plain `var resmgr` (core.h: `var resmgr;` -- already a handle, not
+     * a var*), matching how every other resmgr/ugmgr/etc. consumer in
+     * this codebase (get_group(), is_in_group(), key_find() itself) takes
+     * its manager by value. Passing &(sched->core.resmgr) here instead
+     * (as this and the instantiate_limit() call below previously did)
+     * handed those functions the ADDRESS of the field instead of its
+     * value -- one level of indirection too many. RTTI's key_find()
+     * dispatch then rejected that as an invalid object handle: silently
+     * inside is_host_based_limit() (its own `rc<=0` guard turns ANY
+     * key_find() failure, including this one, into a uniform -ENOENT --
+     * see bug-1786's comment above for why that's not itself a bug), but
+     * fatally inside instantiate_limit()'s own key_find() call, which
+     * returned -EINVAL and aborted vres instantiation outright. */
+    if (is_host_based_limit(sched->core.resmgr, limit) > 0) {
         return 0;
     }
 
@@ -216,18 +230,30 @@ static int match_job_global_limit_(xce_sched_plugin_t* plugin,
         return rc;
     }
 
+    /* bug-1786: key_find() returning <= 0 here means "not found" (this
+     * codebase's established kvmgr convention, matching get_group()'s and
+     * is_host_based_limit()'s own identical rc<=0 checks) -- the FIRST
+     * time ANY limit ever matches a given user/queue/project combination,
+     * this is always the case, since no vres has been instantiated for it
+     * yet. Returning early here (as this code previously did) never gave
+     * the instantiate_limit() block below a chance to run: vres stays
+     * uninitialized (calloc'd to NULL at declaration, never reassigned by
+     * a failed key_find), so `if (vres == NULL)` below is the correct,
+     * sufficient signal to instantiate -- this key_find's own rc need not
+     * gate anything further. Before this fix, bug-1784/1785's fixes made
+     * match_limit() correctly match for the first time in this codebase's
+     * history, which only then exposed this: a limit could match a job
+     * but its usage/threshold accounting could never actually initialize,
+     * so no limit's numeric cap (e.g. limits.yaml's ug1_normal_slots
+     * value: 8) was ever enforced, only the pass/fail match itself. */
     lock(privdata->instances, RTTI_LOCK_READ);
     rc = key_find(privdata->instances, (void*)vresname, (void*)&vres);
     unlock(privdata->instances);
 
-    if (rc <= 0) {
-        return rc;
-    }
-
     FREEUP(vresname);
 
     if (vres == NULL) {
-        rc = instantiate_limit(&vres, &(sched->core.resmgr), limit, obj->user,
+        rc = instantiate_limit(&vres, sched->core.resmgr, limit, obj->user,
                                obj->queue, obj->project);
         if (rc < 0) {
             return rc;
@@ -337,7 +363,7 @@ static int match_job_host_limit_(xce_sched_plugin_t* plugin,
         return 0;
     }
 
-    if (is_host_based_limit(&(sched->core.resmgr), limit) <= 0) {
+    if (is_host_based_limit(sched->core.resmgr, limit) <= 0) {
         return 0;
     }
 
@@ -376,7 +402,7 @@ static int match_job_host_limit_(xce_sched_plugin_t* plugin,
     FREEUP(vresname);
 
     if (vres == NULL) {
-        rc = instantiate_limit(&vres, &(sched->core.resmgr), limit, obj->user,
+        rc = instantiate_limit(&vres, sched->core.resmgr, limit, obj->user,
                                obj->queue, obj->project);
         if (rc < 0) {
             return rc;

src/scheduler/src/scheduler.c

@@ -11,6 +11,7 @@
 #include "client.h"
 #include "controller.capnp.h"
 #include "decision.h"
+#include "group.h"
 #include "kvmgr.h"
 #include "misc.h"
 #include "plugin.h"
@@ -1094,6 +1095,16 @@ static int sched_init_queues(xce_scheduler_t* sched)
 }
 int sched_init_config(xce_scheduler_t* sched, xce_conf4sched_t* config)
 {
+    /* bug-1784: register the cluster's NAMED usergroups/hostgroups (from
+     * users.yaml's "usergroups:"/hosts.yaml's "hostgroups:") into
+     * core.ugmgr/core.hgmgr BEFORE plugin_config_update() below -- a limit
+     * config that references one of these by name (e.g. a limit scoped to
+     * users.members=["ug1"]) resolves it via build_consumer()'s own
+     * makeGroup() call, which can only find an existing named group if it
+     * was already registered under that name. */
+    groupmgr_load_configs(sched->core.ugmgr, config->n_ugroups, config->ugroups);
+    groupmgr_load_configs(sched->core.hgmgr, config->n_hgroups, config->hgroups);
+
     plugin_config_update(sched, config);
     sched->core.config = config;