From e8cb6d57439a3e2d024fecdc92b3522304403fe0 Mon Sep 17 00:00:00 2001 From: "lei.chen" Date: Thu, 22 Oct 2026 07:17:01 +0000 Subject: [PATCH] worker: handle pre-exec failure without dying; persist: skip snapshot queues not in config Two independent fixes: 1. Worker pre-exec failure (TODO fix). queue_pre_exec()/job_pre_exec() previously called exit(1) on failure, killing the one-shot worker before any status report reached the master -- the job then hung with no user-visible reason and the worker process vanished. Now the failure path reports the job FAILED via ipc_worker_send_status with stage JOB_EXEC_STAGE_{Q_PRE,PRE}_EXEC and the command's real exit code, skips the job body (no post-exec -- the body never ran, mirroring LSF pre-exec semantics), and takes the same tail as the VNC/X11 start failures. main.c now carries job_status->stage through to xcecd_job_finished() so the stage (not the MAIN default) reaches the controller's fail fact, deciding both the exit-status merge (apply_job_fail_transition) and whether the exit code is recorded (apply_job_fail_exit_code). Also fixes the error print: uv_proc_pre_exec returns a WEXITSTATUS, not a uv errno, so uv_strerror mangled a plain exit status into "Unknown system error N". E2E verified on t1: job-level pre-exec `exit 7` -> job EXIT, worker logs "Job pre-exec command failed with exit code 7", marker file shows only the pre-exec ran (no post-exec, no job body), and the exit code reaches the apiserver's OnJobFinished (exit_code=7) and the committed WAL_JOB_FAIL fact. 2. Snapshot queue no longer in config wedged election forever. Checkpoint snapshots record every queue in the live qmgr; if a queue is deleted from queues.yaml between the checkpoint and a restart, restore_queue_state returns -ENOENT for it ("queue not yet built from config -- skip" per its own contract), but load_snapshot_entities treated any non-zero as fatal -> persist_recover failed -> cluster_init_phase2 retried forever (bug-1477 never self-terminates) and no primary was ever elected. The snapshot queue loop now treats -ENOENT/-EINVAL as a benign skip with a WARN, matching the same documented policy as apply_committed_entry's and apply_wal_entry's WAL_QUEUE_CTRL cases; every other non-zero return stays a genuine failure. Doc comments updated accordingly. Live-reproduced and verified on t1 (2026-10-22): deleting a test queue + restarting the controller previously wedged election permanently; with the fix the controller boots past the stale frame with a WARN and reaches CTRL_PRIMARY. Co-Authored-By: Claude Code --- src/controller/persist.c | 48 ++++++++++++++---- src/worker/src/main.c | 8 +++ src/worker/src/xcecd_worker.c | 95 ++++++++++++++++++++++++++--------- 3 files changed, 115 insertions(+), 36 deletions(-) diff --git a/src/controller/persist.c b/src/controller/persist.c index c4afef88..4782ab3f 100644 --- a/src/controller/persist.c +++ b/src/controller/persist.c @@ -3875,12 +3875,15 @@ static int persist_checkpoint_marker_triggered(persist_t* p, * own documented idempotent-replay guard (job already present), needed so a * retried load — after an earlier partial attempt already created some jobs * before failing on a later one — can continue past already-restored entries - * rather than aborting on them again. restore_queue_state and - * restore_host_state have no such idempotent-duplicate case of their own to - * special-case here (re-applying the same status is already a harmless - * overwrite/OR, and restore_host_state's create-if-missing placeholder path - * is independently idempotent under retry — Requirement 5.8, worker.c), so - * any non-zero return from either is a genuine failure. */ + * rather than aborting on them again. restore_host_state has no + * idempotent-duplicate case of its own to special-case here (re-applying the + * same status is already a harmless overwrite/OR, and its create-if-missing + * placeholder path is independently idempotent under retry — Requirement 5.8, + * worker.c), so any non-zero return from it is a genuine failure. + * restore_queue_state's -ENOENT/-EINVAL are NOT failures (see the queue loop + * below): a snapshot can name a queue the current boot's config no longer + * provisions, and that frame is skipped with a WARN — every other non-zero + * return from it is a genuine failure. */ static int load_snapshot_entities(int fd, off_t file_size, off_t consumed, persist_t* p, uint64_t stored_lsn) { @@ -3967,9 +3970,28 @@ static int load_snapshot_entities(int fd, off_t file_size, off_t consumed, free(buf); int qrc = restore_queue_state(p->cluster, decoded.name, decoded.status); - free_QueueAdminState(&decoded); - if (qrc != 0) + if (qrc == -ENOENT || qrc == -EINVAL) { + /* The snapshot can legitimately name a queue this boot's config + * no longer provisions (queue deleted from queues.yaml between + * the checkpoint and this restart): restore_queue_state's + * "queue not yet built from config" contract means there is + * nothing to apply the admin state to. Treat it as a benign + * skip — the SAME policy apply_committed_entry's and + * apply_wal_entry's WAL_QUEUE_CTRL cases already document + * ("restore_queue_state's -ENOENT ... benign skip") — rather + * than a fatal restore error. Failing loud here wedges the + * member permanently: cluster_init_phase2 retries forever + * (bug-1477 never self-terminates) and the cluster never + * elects a primary, live-reproduced 2026-10-22 on t1 after + * removing a test queue + restarting the controller. */ + WARN("persist_recover: snapshot queue '%s' not in this boot's " + "config (rc=%d) — skipping its admin state", + decoded.name ? decoded.name : "(null)", qrc); + } else if (qrc != 0) { + free_QueueAdminState(&decoded); return qrc; + } + free_QueueAdminState(&decoded); } /* Host section (config-upgrade-snapshot task 4, Req 1.10): new in @@ -3981,7 +4003,9 @@ static int load_snapshot_entities(int fd, off_t file_size, off_t consumed, * — Design Invariant 1) and idempotent under retry on its own * (Requirement 5.8), so this loop applies the exact same "any failure * returns immediately" discipline as the job/queue loops above without - * needing any extra idempotency handling here. */ + * needing any extra idempotency handling here (the queue loop's one + * exception — a benign -ENOENT/-EINVAL skip for a queue this boot's + * config no longer provisions — is documented there). */ uint32_t host_count = 0; if (read(fd, &host_count, sizeof(host_count)) != (ssize_t)sizeof(host_count)) @@ -4062,8 +4086,10 @@ static int load_snapshot_entities(int fd, off_t file_size, off_t consumed, * * Returns 0 on success, -EINVAL on bad magic / unrecognized entity format, * -EIO on a short/corrupt read, -ENOMEM on allocation failure, -errno on - * open/fstat failure, or a restore_job/restore_queue_state/restore_host_state - * error code. */ + * open/fstat failure, or a genuine restore_job/restore_queue_state/ + * restore_host_state error code (restore_queue_state's -ENOENT/-EINVAL for a + * queue no longer in this boot's config is a documented benign skip, not a + * failure). */ static int load_snapshot(persist_t* p, const char* path, uint64_t lsn, bool* out_entities_restored) { diff --git a/src/worker/src/main.c b/src/worker/src/main.c index 104e2332..bd27e9d6 100644 --- a/src/worker/src/main.c +++ b/src/worker/src/main.c @@ -179,6 +179,14 @@ static void master_job_status_callback(job_status_t* job_status) job.job_status = job_status->status_code; job.job_pid = job_status->job_pid; job.exit_code = job_status->exit_code; + /* carry the reporting stage through: xcecd_job_finished() turns a + * nonzero exit into notify_job_fail(job.stage, job.exit_code), and a + * pre-exec failure must reach the controller's fail fact as + * JOB_EXEC_STAGE_{Q_PRE,PRE}_EXEC, not the MAIN default -- the stage + * decides the controller's exit-status merge and whether the exit code + * is recorded at all (apply_job_fail_transition/apply_job_fail_exit_ + * code). */ + job.stage = job_status->stage; strncpy(job.exdata, job_status->start_exdata, sizeof(job.exdata) - 1); job.exdata[sizeof(job.exdata) - 1] = '\0'; diff --git a/src/worker/src/xcecd_worker.c b/src/worker/src/xcecd_worker.c index 8dafcd0a..7b6eed13 100644 --- a/src/worker/src/xcecd_worker.c +++ b/src/worker/src/xcecd_worker.c @@ -439,7 +439,14 @@ static void alarm_handler(int sig) } } -static void queue_pre_exec() +/* Returns 0 on success. On failure the job is reported FAILED with the + * command's exit status (JOB_EXEC_STAGE_Q_PRE_EXEC fact, so the controller + * records the real code) and -1 is returned: the caller must skip the job + * body and leave through worker_job_callback's failure tail. This replaces + * the previous bare exit(1), which killed the one-shot worker before any + * status report reached the master, leaving the job with no user-visible + * reason. */ +static int queue_pre_exec() { int ret; @@ -449,17 +456,33 @@ static void queue_pre_exec() ret = uv_proc_pre_exec(&uv_proc_manager, job_runtime_info.q_pre_exec, NULL); if (ret != 0) { - XCECD_WORKER_LOG(XCE_LOG_ERROR, "Queue pre-exec command failed: %s", - uv_strerror(ret)); + /* uv_proc_pre_exec returns the command's WEXITSTATUS (or -1 on a + * spawn/wait failure), not a uv errno -- printing it through + * uv_strerror would mangle a plain shell exit status into + * "Unknown system error N". */ + XCECD_WORKER_LOG(XCE_LOG_ERROR, + "Queue pre-exec command failed with exit code %d", + ret); - job_runtime_info.exit_code = ret; - job_runtime_info.stage = JOB_EXEC_STAGE_Q_PRE_EXEC; - // TODO: Handle pre-exec failure - exit(1); - } else { - XCECD_WORKER_LOG(XCE_LOG_DEBUG, - "Queue pre-exec command completed successfully"); + check_and_reconnect(); + + job_runtime_info.job_pid = 0; + job_runtime_info.job_status = JOB_FAILED; + job_runtime_info.worker_status = WORKER_EXITED; + job_runtime_info.exit_code = ret; + job_runtime_info.stage = JOB_EXEC_STAGE_Q_PRE_EXEC; + + ipc_worker_send_status(&job_runtime_info, + "Queue level pre-exec failed (check worker " + "log)"); + worker_exit_flag = 1; + return_code = ret < 0 ? 1 : ret; + return -1; } + + XCECD_WORKER_LOG(XCE_LOG_DEBUG, + "Queue pre-exec command completed successfully"); + return 0; } static void queue_post_exec() @@ -489,7 +512,10 @@ static void queue_post_exec() } } -static void job_pre_exec() +/* Same contract as queue_pre_exec: 0 on success; on failure report the job + * FAILED with a JOB_EXEC_STAGE_PRE_EXEC fact carrying the real exit code + * and return -1 for worker_job_callback's failure tail. */ +static int job_pre_exec() { int ret; @@ -500,16 +526,28 @@ static void job_pre_exec() NULL); if (ret != 0) { - XCECD_WORKER_LOG(XCE_LOG_ERROR, "Job pre-exec command failed: %s", - uv_strerror(ret)); - job_runtime_info.exit_code = ret; - job_runtime_info.stage = JOB_EXEC_STAGE_PRE_EXEC; - // TODO: Handle pre-exec failure - exit(1); - } else { - XCECD_WORKER_LOG(XCE_LOG_DEBUG, - "Job pre-exec command completed successfully"); + XCECD_WORKER_LOG(XCE_LOG_ERROR, + "Job pre-exec command failed with exit code %d", ret); + + check_and_reconnect(); + + job_runtime_info.job_pid = 0; + job_runtime_info.job_status = JOB_FAILED; + job_runtime_info.worker_status = WORKER_EXITED; + job_runtime_info.exit_code = ret; + job_runtime_info.stage = JOB_EXEC_STAGE_PRE_EXEC; + + ipc_worker_send_status(&job_runtime_info, + "Job level pre-exec failed (check worker " + "log)"); + worker_exit_flag = 1; + return_code = ret < 0 ? 1 : ret; + return -1; } + + XCECD_WORKER_LOG(XCE_LOG_DEBUG, + "Job pre-exec command completed successfully"); + return 0; } static void job_post_exec() @@ -1706,14 +1744,21 @@ static void worker_job_callback(job_assignment_t* info) } } - /* Queue pre-exec */ - if (job_runtime_info.q_pre_exec[0] != '\0') { - queue_pre_exec(); + /* Queue pre-exec: a failure already reported the job FAILED with the + * command's exit code -- skip the body and take the same tail as the + * VNC/X11 start failures above (no post-exec: the job body never ran, + * mirroring LSF's pre-exec semantics). */ + if (job_runtime_info.q_pre_exec[0] != '\0' && queue_pre_exec() != 0) { + FREEUP(job_cmd); + ipc_worker_run(UV_RUN_NOWAIT); + return; } /* Job pre-exec */ - if (job_runtime_info.job_pre_exec[0] != '\0') { - job_pre_exec(); + if (job_runtime_info.job_pre_exec[0] != '\0' && job_pre_exec() != 0) { + FREEUP(job_cmd); + ipc_worker_run(UV_RUN_NOWAIT); + return; } job_runtime_info.stage = JOB_EXEC_STAGE_MAIN;