fix(scheduler): the normal-mode round frees each decision after publishing it (bug-1885)

xce_sched_driver_run_normal_mode_round() drained every decision, handed it
to the recorder and the decision callback, and never freed the struct
make_*_decision() allocates: 64 bytes per decision for the life of the
scheduler. The struct is freed after the callback returns; its allocation
or pend reason belongs to the job, and the callback and recorder copy
what they keep.

Test: test_round_frees_decision_after_callback (ASAN shadow check) RED
before, GREEN after.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Rongsong Shen 2026-09-29 09:57:26 +00:00
parent 5b4d61180b
commit d21b3d8f98
Signed by untrusted user: rshen
GPG key ID: 80533536B0EB651C
2 changed files with 87 additions and 2 deletions

View file

@ -1070,6 +1070,11 @@ void xce_sched_driver_run_normal_mode_round(void* driver_ctx)
"op=%d", "op=%d",
__func__, (int)decision->op); __func__, (int)decision->op);
} }
/* bug-1885: the decision struct is the round's (make_*_decision());
* its allocation or pend reason belongs to the job, and the
* callback and the recorder copy what they keep */
free(decision);
} }
plugin_end_session(&(sched->core)); plugin_end_session(&(sched->core));

View file

@ -141,6 +141,10 @@
#include <unity/unity.h> #include <unity/unity.h>
#if defined(__SANITIZE_ADDRESS__)
#include <sanitizer/asan_interface.h>
#endif
#include "controller.capnp.h" #include "controller.capnp.h"
#include "kvmgr.h" #include "kvmgr.h"
#include "preason.h" #include "preason.h"
@ -673,6 +677,10 @@ static void* picker_thread_fn(void* arg)
/* test function itself can play this role sequentially). */ /* test function itself can play this role sequentially). */
/* ---------------------------------------------------------------------- */ /* ---------------------------------------------------------------------- */
/* bug-1885: when set, the stand-in emits one PEND decision carrying this
* (test-owned) reason before the end-of-session marker */
static xce_pendreason_t* g_emit_pend_reason;
static void* stand_in_allocator_thread_fn(void* arg) static void* stand_in_allocator_thread_fn(void* arg)
{ {
xce_sched_core_t* core = (xce_sched_core_t*)arg; xce_sched_core_t* core = (xce_sched_core_t*)arg;
@ -683,6 +691,12 @@ static void* stand_in_allocator_thread_fn(void* arg)
(void)jobid; /* always 0 here -- this fixture pushes no jobs */ (void)jobid; /* always 0 here -- this fixture pushes no jobs */
log_event(EVT_ALLOCATOR_DRAINED_JBUF_SENTINEL); log_event(EVT_ALLOCATOR_DRAINED_JBUF_SENTINEL);
if (g_emit_pend_reason != NULL) {
xce_decision_t* pend = make_pend_decision(g_emit_pend_reason);
cbuf_put(&(core->decisions), &pend);
}
cbuf_put(&(core->decisions), &decision); cbuf_put(&(core->decisions), &decision);
log_event(EVT_ALLOCATOR_PUSHED_END_DECISION); log_event(EVT_ALLOCATOR_PUSHED_END_DECISION);
@ -695,15 +709,16 @@ static void* stand_in_allocator_thread_fn(void* arg)
/* ---------------------------------------------------------------------- */ /* ---------------------------------------------------------------------- */
typedef struct { typedef struct {
int calls; int calls;
xce_decision_t* last;
} test_driver_ctx_t; } test_driver_ctx_t;
static int test_on_decision(void* driver_ctx, xce_decision_t* decision) static int test_on_decision(void* driver_ctx, xce_decision_t* decision)
{ {
test_driver_ctx_t* ctx = (test_driver_ctx_t*)driver_ctx; test_driver_ctx_t* ctx = (test_driver_ctx_t*)driver_ctx;
(void)decision;
ctx->calls++; ctx->calls++;
ctx->last = decision;
return 0; return 0;
} }
@ -938,6 +953,70 @@ static void test_recorded_begin_round_equals_session_start(void)
TEST_ASSERT_EQUAL_UINT64(sched.core.session_start, g_record_begin_ts); TEST_ASSERT_EQUAL_UINT64(sched.core.session_start, g_record_begin_ts);
} }
/*
* bug-1885: the round loop frees each decision struct once the decision
* callback has returned; the pend reason (or allocation) it points at is
* not the round's and stays valid. Checked through ASAN's shadow memory,
* so only meaningful in a sanitizer build.
*/
static void test_round_frees_decision_after_callback(void)
{
#if defined(__SANITIZE_ADDRESS__)
xce_scheduler_t sched;
pthread_t picker_tid;
pthread_t allocator_tid;
test_driver_ctx_t driver_ctx;
xce_pendreason_t* reason;
int rc;
memset(&g_events, 0, sizeof(g_events));
memset(&sched, 0, sizeof(sched));
memset(&driver_ctx, 0, sizeof(driver_ctx));
rc = xce_sched_core_init(&(sched.core));
TEST_ASSERT_EQUAL_INT(0, rc);
lock(sched.core.plugins, RTTI_LOCK_WRITE);
rc = key_add(sched.core.plugins, (void*)g_stub_picker_plugin.name,
(void*)&g_stub_picker_plugin);
TEST_ASSERT_GREATER_OR_EQUAL(0, rc);
unlock(sched.core.plugins);
sched.core.job_pick_plugin = g_stub_picker_plugin.name;
reason = make_pendreason(4242);
TEST_ASSERT_NOT_NULL(reason);
g_emit_pend_reason = reason;
rc = xce_core_set_decision_cb(&(sched.core), test_on_decision, &driver_ctx);
TEST_ASSERT_EQUAL_INT(0, rc);
rc = pthread_create(&picker_tid, NULL, picker_thread_fn, &(sched.core));
TEST_ASSERT_EQUAL_INT(0, rc);
rc = pthread_create(&allocator_tid, NULL, stand_in_allocator_thread_fn,
&(sched.core));
TEST_ASSERT_EQUAL_INT(0, rc);
xce_sched_driver_run_normal_mode_round(&sched);
TEST_ASSERT_EQUAL_INT(0, pthread_join(picker_tid, NULL));
TEST_ASSERT_EQUAL_INT(0, pthread_join(allocator_tid, NULL));
g_emit_pend_reason = NULL;
TEST_ASSERT_EQUAL_INT(1, driver_ctx.calls);
TEST_ASSERT_NOT_NULL(driver_ctx.last);
TEST_ASSERT_TRUE_MESSAGE(
__asan_address_is_poisoned(driver_ctx.last),
"the round must free the decision struct after publishing it");
TEST_ASSERT_FALSE_MESSAGE(__asan_address_is_poisoned(reason),
"the pend reason is not the round's to free");
TEST_ASSERT_EQUAL_UINT64(4242, reason->jobid);
free_PendReason_ptr(&reason);
#else
TEST_IGNORE_MESSAGE("needs an ASAN build");
#endif
}
int main(void) int main(void)
{ {
UNITY_BEGIN(); UNITY_BEGIN();
@ -945,5 +1024,6 @@ int main(void)
test_round_runs_plugin_begin_and_account_jobs_before_picker_signaled); test_round_runs_plugin_begin_and_account_jobs_before_picker_signaled);
RUN_TEST(test_null_driver_ctx_is_safe_noop); RUN_TEST(test_null_driver_ctx_is_safe_noop);
RUN_TEST(test_recorded_begin_round_equals_session_start); RUN_TEST(test_recorded_begin_round_equals_session_start);
RUN_TEST(test_round_frees_decision_after_callback);
return UNITY_END(); return UNITY_END();
} }