forked from lei.chen/myxce
fix: staging_merge_complete bootstrap deadlock and missing recompact_node_ids
Two correctness fixes found during final Phase 6 code review: F1 (CRITICAL): GC permanently disabled — staging_merge_complete bootstrapping deadlock. sched_begin_session reset staging_merge_complete to 0 but never set it back to 1, so sched_end_session's guard (!staging_merge_complete) always fired, preventing GC from ever running. select-node memory leaked indefinitely across scheduling sessions. Fix: set staging_merge_complete=1 at the end of sched_begin_session after all staging work (sched_account_jobs etc.) completes. F2 (HIGH): recompact_node_ids() omitted from GC protocol in sched_end_session. The protocol in resreq.h requires gc_sweep → recompact_node_ids. Without recompaction, sel_node_next_id grows monotonically across sessions and select_eval_ctx_t arrays (sized by max node_id) grow without bound. Fix: call recompact_node_ids() after gc_sweep_select_nodes. Also adds test_staging_merge_complete_gc_guard to document the bootstrap invariant: sched_begin_session must leave staging_merge_complete=1 so the GC guard in sched_end_session does not fire.
This commit is contained in:
parent
5fcf85c494
commit
d438742f1a
2 changed files with 36 additions and 0 deletions
|
|
@ -770,6 +770,8 @@ void sched_begin_session(xce_scheduler_t* sched)
|
|||
|
||||
sched_account_jobs(sched);
|
||||
|
||||
/* Staging merge done — sched_end_session's GC guard reads this field. */
|
||||
atomic_store_explicit(&sched->staging_merge_complete, 1, memory_order_release);
|
||||
return;
|
||||
}
|
||||
|
||||
|
|
@ -875,6 +877,7 @@ void sched_end_session(xce_scheduler_t* sched)
|
|||
atomic_thread_fence(memory_order_seq_cst);
|
||||
|
||||
gc_sweep_select_nodes(NULL, 0);
|
||||
recompact_node_ids();
|
||||
free(live_jobs);
|
||||
|
||||
atomic_store_explicit(&sched->staging_merge_complete, 1, memory_order_release);
|
||||
|
|
|
|||
|
|
@ -221,6 +221,38 @@ test_gc_predicates_idle_invariant(void)
|
|||
sel_optimize_test_reset();
|
||||
}
|
||||
|
||||
static void
|
||||
test_staging_merge_complete_gc_guard(void)
|
||||
{
|
||||
/* Regression: sched_begin_session must leave staging_merge_complete=1 so
|
||||
* sched_end_session's guard (!staging_merge_complete) does not fire.
|
||||
*
|
||||
* sched_begin_session: sets staging_merge_complete=0, does all session setup,
|
||||
* then sets staging_merge_complete=1 (staging done).
|
||||
* sched_end_session: guard skips GC when staging_merge_complete==0.
|
||||
*
|
||||
* Without the trailing set-to-1 in sched_begin_session, staging_merge_complete
|
||||
* stays 0 forever and GC never runs — select-node memory leaks indefinitely. */
|
||||
xce_scheduler_t sched;
|
||||
memset(&sched, 0, sizeof(sched));
|
||||
|
||||
/* Simulate the post-begin_session state the fix produces */
|
||||
atomic_store(&sched.session_active, 0); /* session ended */
|
||||
atomic_store(&sched.eval_in_progress, 0);
|
||||
atomic_store(&sched.staging_merge_complete, 1); /* fix: begin_session sets this */
|
||||
|
||||
/* The GC guard in sched_end_session: must NOT fire */
|
||||
int guard_fires = atomic_load(&sched.eval_in_progress)
|
||||
|| !atomic_load(&sched.staging_merge_complete);
|
||||
TEST_ASSERT_EQUAL_INT(0, guard_fires);
|
||||
|
||||
/* Confirm the unfixed state (staging=0) would block GC */
|
||||
atomic_store(&sched.staging_merge_complete, 0);
|
||||
int guard_fires_unfixed = atomic_load(&sched.eval_in_progress)
|
||||
|| !atomic_load(&sched.staging_merge_complete);
|
||||
TEST_ASSERT_EQUAL_INT(1, guard_fires_unfixed);
|
||||
}
|
||||
|
||||
static void
|
||||
test_meet_host_selection_numeric_match(void)
|
||||
{
|
||||
|
|
@ -359,6 +391,7 @@ int main(int argc, char* argv[])
|
|||
TEST(test_meet_host_selection_always_true_passes);
|
||||
TEST(test_meet_host_selection_always_false_fails);
|
||||
TEST(test_gc_predicates_idle_invariant);
|
||||
TEST(test_staging_merge_complete_gc_guard);
|
||||
TEST(test_meet_host_selection_numeric_match);
|
||||
TEST(test_meet_host_selection_string_match);
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue