lei.chen/myxce
1
0
Fork 1
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(merge): restore bug-1490 job control + p2-hang flusher fix + B3 test reconciliation

Browse source 
Merge follow-up fixes found during comprehensive unit-test + live-cluster
verification of the scheduler-framework merge:

- apiserver/backend/clib.go (ControlJob): Rongsong's clib.go had reverted
  our bug-1490 fix — ctrl.action must be the capnp JobControl union
  DISCRIMINANT (0=siginfo,1=ckptoption), not the models.JOB_* constant
  (1/2/3/8), or every kill/stop/resume fails -EINVAL. Restored
  ctrl.action = C.int(jobCtrlDiscriminant(action)); verified live (job
  kill RUN->EXIT).

- controller/persist.c (fsync_worker): the merged bug-1563 no-progress
  backoff used the PRE-FLUSH `pending` snapshot, so a no-op flush (another
  path advanced durable_lsn to tip mid-flush) also slept wal_sync_ms and
  raced commit_engine_fini in teardown, hanging the p2_integration suite
  ~60% (bug-1515). Re-evaluate the level AFTER the flush (pending_after)
  so only a genuine sustained fdatasync failure backoffs. 20/20 clean p2
  runs (was 3/5 hang). Also carries the bug-1509 B3 RUN->UNKWN marking in
  persist_recover.

- scheduler/worker testsuite meson.build: add unity_dep parent includedir
  (unity.pc Cflags only add -I${includedir}/unity, so <unity/unity.h> was
  unresolvable) — same fix as src/lib/meson.build.

- test_handlers.c: re-point notify.data.dispatchfail after the first
  sched_handle_notify NULLs it on success (code=0 branch was -EINVAL).

- test_controller_connector.c: give the rejoin-success fixtures a valid
  worker_list pointer (my bug-1509 resend_job_status deref was NULL-
  crashing).

- test_wal_job_dispatch_fact.c / test_dispatch_commit.c: Rongsong's
  WAL_JOB_DISPATCH replay tests asserted JOB_STAT_RUN after persist_recover,
  conflicting with bug-1509 B3 (RUN->UNKWN on restart). Assert JOB_IS_RUN
  (base RUN recovered, incl. bug-1678 allocs) + UNKWN flag instead.

All suites green: controller 1145/0, scheduler 57/57, worker 15/15,
lib 303/0, apiserver CGO build ok.
This commit is contained in:
lei.chen 2026-08-26 10:52:35 +00:00
parent 7ea117dafa
commit 8858afed3a
8 changed files with 88 additions and 11 deletions
Download patch file Download diff file Expand all files Collapse all files

27
src/controller/persist.c
View file

@ -576,7 +576,23 @@ static void* fsync_worker(void* arg)
* pending but made no progress (after == before) still waits out the
* normal wal_sync_ms cadence before retrying — same bounded-latency
* budget the idle path already uses, no new tunable needed. */
if (pending && after == before &&
/* Re-evaluate the level AFTER the flush, not with the pre-flush
* `pending` snapshot: `after == before` alone cannot distinguish a
* genuine sustained fdatasync failure (pending work that made no
* progress — the spin this block exists to throttle) from a flush that
* was a no-op because another path advanced durable_lsn to the tip
* mid-flush (e.g. persist_stop's final sync, or a concurrent
* persist_wal_sync_append). In the no-op case durable is caught up, so
* there is nothing to retry and waiting wal_sync_ms is pure delay —
* and that spurious delay sits directly in the fsync_worker's
* teardown window, where it races persist_stop()/commit_engine_fini()
* (bug-1515: p2_integration hang). Only wait when the level is STILL
* pending after the flush (real failure). */
bool pending_after;
pthread_mutex_lock(&p->lock);
pending_after = flusher_work_pending_locked(p);
pthread_mutex_unlock(&p->lock);
if (pending_after && after == before &&
!atomic_load_explicit(&p->fsync_stop, memory_order_acquire)) {
long ms = (long)p->config.wal_sync_ms;
struct timespec dl;
@ -5668,6 +5684,15 @@ int persist_recover(persist_t* p)
* own walk drains fully from last_applied_lsn+1 through the current
* committed_lsn, so this is a complete, ordered catch-up, not a partial
* one; a harmless no-op if nothing queued. */
/* bug-1509 B3: after restart, RUN jobs' state is uncertain (worker
* connectivity unknown) — mark them UNKWN+TRANSIENT in-memory only.
* No WAL append: persist_recover runs before the controller knows it
* will be primary, and a standby must never append. Idempotent. */
if (p->cluster) {
mark_all_run_jobs_unknown_in_memory(p->cluster);
}
atomic_store_explicit(&p->recovery_complete, true, memory_order_release);
/* n_vcpus==0 is this codebase's own established "not yet initialized"
* sentinel for vtaskmgr_t (mirrors cluster_init_phase2's own

5
src/controller/testsuite/test_dispatch_commit.c
View file

@ -679,10 +679,13 @@ static void test_dc_success_appends_wal_job_dispatch_durably(void)
unlock(c2.jobmgr);
TEST_ASSERT_NOT_NULL_MESSAGE(j2,
"restart replay must reconstruct the job via WAL_JOB_SUBMIT");
TEST_ASSERT_EQUAL_UINT32_MESSAGE(JOB_STAT_RUN, j2->job.state,
TEST_ASSERT_TRUE_MESSAGE(JOB_IS_RUN(j2->job.state),
"restart replay must recover the job as RUNNING from the "
"WAL_JOB_DISPATCH fact appended by dispatch_a_job_()'s real "
"success path -- no WAL_JOB_START anywhere in this log");
TEST_ASSERT_TRUE_MESSAGE((j2->job.state & JOB_STAT_UNKWN) != 0,
"bug-1509 B3: a RUN job recovered on restart is marked UNKWN "
"(in-memory) awaiting worker reconnect/resend reconciliation");
/* bug-1678: restart replay must recover not just STATE but the
* resource allocation itself, or EXEC_HOST is permanently blank for

13
src/controller/testsuite/test_wal_job_dispatch_fact.c
View file

@ -434,10 +434,14 @@ test_wjd_restart_replay_recovers_running_without_start_fact(void)
job_t* j = wjd_find_job(71003);
TEST_ASSERT_NOT_NULL_MESSAGE(j,
"restart replay must construct the job via WAL_JOB_SUBMIT");
TEST_ASSERT_EQUAL_UINT32_MESSAGE(JOB_STAT_RUN, j->job.state,
TEST_ASSERT_TRUE_MESSAGE(JOB_IS_RUN(j->job.state),
"restart replay must recover the job as RUNNING from the "
"WAL_JOB_DISPATCH fact alone, with no WAL_JOB_START present "
"in the log at all");
TEST_ASSERT_TRUE_MESSAGE((j->job.state & JOB_STAT_UNKWN) != 0,
"bug-1509 B3: a RUN job recovered on restart is marked UNKWN "
"(in-memory) awaiting the worker reconnect/resend to reconcile "
"the true state");
wjd_teardown();
wjd_teardown_persist(p2);
@ -529,7 +533,12 @@ test_wjd_restart_replay_recovers_running_and_allocs_with_dispatch_fact(void)
job_t* j = wjd_find_job(71006);
TEST_ASSERT_NOT_NULL_MESSAGE(j,
"restart replay must construct the job via WAL_JOB_SUBMIT");
TEST_ASSERT_EQUAL_UINT32(JOB_STAT_RUN, j->job.state);
TEST_ASSERT_TRUE_MESSAGE(JOB_IS_RUN(j->job.state),
"restart replay must recover the job as RUNNING from the "
"WAL_JOB_DISPATCH fact's state");
TEST_ASSERT_TRUE_MESSAGE((j->job.state & JOB_STAT_UNKWN) != 0,
"bug-1509 B3: a RUN job recovered on restart is marked UNKWN "
"(in-memory) awaiting worker reconnect/resend reconciliation");
TEST_ASSERT_NOT_NULL_MESSAGE(j->job.allocs,
"restart replay must recover job.allocs from the "
"WAL_JOB_DISPATCH fact's trailing JobAlloc bytes (bug-1678), "

8
src/frontend/apiserver/backend/clib.go
View file

@ -664,7 +664,13 @@ func SubmitJob(job *models.JobInfo, arraySize int) error {
func ControlJob(jobid uint64, action int, content string) error {
var ctrl C.xce_jobctrl_t
ctrl.jobid = C.uint64_t(jobid)
ctrl.action = C.int(action)
// ctrl.action is the capnp JobControl union DISCRIMINANT (0=siginfo,
// 1=ckptoption), NOT the models.JOB_* constant. bug-1490: setting it to
// JOB_STOP/RESUME/KILL/CHKPNT (1/2/3/8) made the controller's job_ctrl()
// select the wrong union branch (or none) and reject every control with
// -EINVAL, so hold/release/cancel never reached the job. (Re-applied on
// the merge branch — Rongsong's clib.go had reverted it to action.)
ctrl.action = C.int(jobCtrlDiscriminant(action))
// For signal-based actions set signo; for checkpoint set ckpt options.
// xce_jobctrl_t.u is a C union — we use a static C helper to set the

6
src/scheduler/testsuite/meson.build
View file

@ -14,7 +14,11 @@ scheduler_test_src = [
scheduler_test_deps = [libxce_deps, librtti_dep, unity_dep, libxce_dep,
libedit_dep]
incdir = include_directories('../include', '../../include', '.', '../src', '../../lib')
# unity.pc's Cflags only adds -I${includedir}/unity, so <unity.h> resolves
# but <unity/unity.h> (used by every suite) does not — add the parent
# includedir as well (same fix as src/lib/meson.build).
incdir = include_directories('../include', '../../include', '.', '../src', '../../lib',
unity_dep.get_variable('includedir'))
scheduler_test = executable('sched_test',
scheduler_test_src,

9
src/scheduler/testsuite/test_handlers.c
View file

@ -257,10 +257,11 @@ static void test_handle_notify_job_dispatch_fail_enqueues_pend(void)
* re-dispatches a job whose original instance keeps running, spawning
* duplicate instances). The ack is consumed as a no-op. */
fixture_init(&sched);
info.jobid = 123;
info.code = 0;
jobid = 0;
state = (uint32_t)-1;
info.jobid = 123;
info.code = 0;
jobid = 0;
state = (uint32_t)-1;
notify.data.dispatchfail = &info; /* first notify NULLed it on success */
TEST_ASSERT_EQUAL_INT(0, sched_handle_notify(&sched, &notify));
TEST_ASSERT_EQUAL_INT(0, pop_jstate_change(&sched, &jobid, &state));
}

6
src/worker/testsuite/meson.build
View file

@ -6,8 +6,12 @@
# for why controller_connector.c is not also listed in xcecd_srcs here (it
# would double-define every symbol in that file).
# unity.pc's Cflags only adds -I${includedir}/unity, so <unity.h> resolves
# but <unity/unity.h> (used by every suite) does not — add the parent
# includedir as well (same fix as src/lib/meson.build).
worker_test_incdir = include_directories('../include', '../../include', '.',
'../src', '../../lib')
'../src', '../../lib',
unity_dep.get_variable('includedir'))
worker_test_deps = [xcecd_worker_deps, unity_dep]

25
src/worker/testsuite/test_controller_connector.c
View file

@ -424,6 +424,11 @@ static void test_checking_connection_skips_rejoin_when_not_pending(void)
controller_connector_ctx_t fake_cc = { 0 };
fake_cc.running = true;
cc_ctx = &fake_cc;
/* checking_connection()'s rejoin-success path reports job state via
* resend_job_status(*cc_ctx->worker_list); give the connector a valid
* (here: empty) list pointer so the resend deref is not a NULL crash. */
xcecd_worker_t* fake_workers = NULL;
fake_cc.worker_list = (void**)&fake_workers;
xcecli_ctx = NULL; /* xcecli_hello fails fast */
needs_rejoin = false;
@ -446,6 +451,11 @@ static void test_checking_connection_rejoin_failure_keeps_flag_set(void)
controller_connector_ctx_t fake_cc = { 0 };
fake_cc.running = true;
cc_ctx = &fake_cc;
/* checking_connection()'s rejoin-success path reports job state via
* resend_job_status(*cc_ctx->worker_list); give the connector a valid
* (here: empty) list pointer so the resend deref is not a NULL crash. */
xcecd_worker_t* fake_workers = NULL;
fake_cc.worker_list = (void**)&fake_workers;
xcecli_ctx = NULL;
needs_rejoin = true;
@ -470,6 +480,11 @@ static void test_checking_connection_rejoin_success_clears_flag(void)
controller_connector_ctx_t fake_cc = { 0 };
fake_cc.running = true;
cc_ctx = &fake_cc;
/* checking_connection()'s rejoin-success path reports job state via
* resend_job_status(*cc_ctx->worker_list); give the connector a valid
* (here: empty) list pointer so the resend deref is not a NULL crash. */
xcecd_worker_t* fake_workers = NULL;
fake_cc.worker_list = (void**)&fake_workers;
xcecli_ctx = NULL;
needs_rejoin = true;
@ -497,6 +512,11 @@ test_checking_connection_rejoin_success_with_nonzero_index_clears_flag(void)
controller_connector_ctx_t fake_cc = { 0 };
fake_cc.running = true;
cc_ctx = &fake_cc;
/* checking_connection()'s rejoin-success path reports job state via
* resend_job_status(*cc_ctx->worker_list); give the connector a valid
* (here: empty) list pointer so the resend deref is not a NULL crash. */
xcecd_worker_t* fake_workers = NULL;
fake_cc.worker_list = (void**)&fake_workers;
xcecli_ctx = NULL;
needs_rejoin = true;
@ -530,6 +550,11 @@ test_checking_connection_rejoin_retried_on_next_probe_after_failure(void)
controller_connector_ctx_t fake_cc = { 0 };
fake_cc.running = true;
cc_ctx = &fake_cc;
/* checking_connection()'s rejoin-success path reports job state via
* resend_job_status(*cc_ctx->worker_list); give the connector a valid
* (here: empty) list pointer so the resend deref is not a NULL crash. */
xcecd_worker_t* fake_workers = NULL;
fake_cc.worker_list = (void**)&fake_workers;
xcecli_ctx = NULL;
needs_rejoin = true;