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 <noreply@anthropic.com>
This commit is contained in:
parent
5b9d727086
commit
e8cb6d5743
3 changed files with 115 additions and 36 deletions
|
|
@ -3875,12 +3875,15 @@ static int persist_checkpoint_marker_triggered(persist_t* p,
|
||||||
* own documented idempotent-replay guard (job already present), needed so a
|
* own documented idempotent-replay guard (job already present), needed so a
|
||||||
* retried load — after an earlier partial attempt already created some jobs
|
* retried load — after an earlier partial attempt already created some jobs
|
||||||
* before failing on a later one — can continue past already-restored entries
|
* before failing on a later one — can continue past already-restored entries
|
||||||
* rather than aborting on them again. restore_queue_state and
|
* rather than aborting on them again. restore_host_state has no
|
||||||
* restore_host_state have no such idempotent-duplicate case of their own to
|
* idempotent-duplicate case of its own to special-case here (re-applying the
|
||||||
* special-case here (re-applying the same status is already a harmless
|
* same status is already a harmless overwrite/OR, and its create-if-missing
|
||||||
* overwrite/OR, and restore_host_state's create-if-missing placeholder path
|
* placeholder path is independently idempotent under retry — Requirement 5.8,
|
||||||
* is independently idempotent under retry — Requirement 5.8, worker.c), so
|
* worker.c), so any non-zero return from it is a genuine failure.
|
||||||
* any non-zero return from either 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,
|
static int load_snapshot_entities(int fd, off_t file_size, off_t consumed,
|
||||||
persist_t* p, uint64_t stored_lsn)
|
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);
|
free(buf);
|
||||||
|
|
||||||
int qrc = restore_queue_state(p->cluster, decoded.name, decoded.status);
|
int qrc = restore_queue_state(p->cluster, decoded.name, decoded.status);
|
||||||
free_QueueAdminState(&decoded);
|
if (qrc == -ENOENT || qrc == -EINVAL) {
|
||||||
if (qrc != 0)
|
/* 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;
|
return qrc;
|
||||||
|
}
|
||||||
|
free_QueueAdminState(&decoded);
|
||||||
}
|
}
|
||||||
|
|
||||||
/* Host section (config-upgrade-snapshot task 4, Req 1.10): new in
|
/* 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
|
* — Design Invariant 1) and idempotent under retry on its own
|
||||||
* (Requirement 5.8), so this loop applies the exact same "any failure
|
* (Requirement 5.8), so this loop applies the exact same "any failure
|
||||||
* returns immediately" discipline as the job/queue loops above without
|
* 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;
|
uint32_t host_count = 0;
|
||||||
if (read(fd, &host_count, sizeof(host_count)) !=
|
if (read(fd, &host_count, sizeof(host_count)) !=
|
||||||
(ssize_t)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,
|
* Returns 0 on success, -EINVAL on bad magic / unrecognized entity format,
|
||||||
* -EIO on a short/corrupt read, -ENOMEM on allocation failure, -errno on
|
* -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
|
* open/fstat failure, or a genuine restore_job/restore_queue_state/
|
||||||
* error code. */
|
* 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,
|
static int load_snapshot(persist_t* p, const char* path, uint64_t lsn,
|
||||||
bool* out_entities_restored)
|
bool* out_entities_restored)
|
||||||
{
|
{
|
||||||
|
|
|
||||||
|
|
@ -179,6 +179,14 @@ static void master_job_status_callback(job_status_t* job_status)
|
||||||
job.job_status = job_status->status_code;
|
job.job_status = job_status->status_code;
|
||||||
job.job_pid = job_status->job_pid;
|
job.job_pid = job_status->job_pid;
|
||||||
job.exit_code = job_status->exit_code;
|
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);
|
strncpy(job.exdata, job_status->start_exdata, sizeof(job.exdata) - 1);
|
||||||
job.exdata[sizeof(job.exdata) - 1] = '\0';
|
job.exdata[sizeof(job.exdata) - 1] = '\0';
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -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;
|
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);
|
ret = uv_proc_pre_exec(&uv_proc_manager, job_runtime_info.q_pre_exec, NULL);
|
||||||
|
|
||||||
if (ret != 0) {
|
if (ret != 0) {
|
||||||
XCECD_WORKER_LOG(XCE_LOG_ERROR, "Queue pre-exec command failed: %s",
|
/* uv_proc_pre_exec returns the command's WEXITSTATUS (or -1 on a
|
||||||
uv_strerror(ret));
|
* 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;
|
check_and_reconnect();
|
||||||
job_runtime_info.stage = JOB_EXEC_STAGE_Q_PRE_EXEC;
|
|
||||||
// TODO: Handle pre-exec failure
|
job_runtime_info.job_pid = 0;
|
||||||
exit(1);
|
job_runtime_info.job_status = JOB_FAILED;
|
||||||
} else {
|
job_runtime_info.worker_status = WORKER_EXITED;
|
||||||
XCECD_WORKER_LOG(XCE_LOG_DEBUG,
|
job_runtime_info.exit_code = ret;
|
||||||
"Queue pre-exec command completed successfully");
|
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()
|
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;
|
int ret;
|
||||||
|
|
||||||
|
|
@ -500,16 +526,28 @@ static void job_pre_exec()
|
||||||
NULL);
|
NULL);
|
||||||
|
|
||||||
if (ret != 0) {
|
if (ret != 0) {
|
||||||
XCECD_WORKER_LOG(XCE_LOG_ERROR, "Job pre-exec command failed: %s",
|
XCECD_WORKER_LOG(XCE_LOG_ERROR,
|
||||||
uv_strerror(ret));
|
"Job pre-exec command failed with exit code %d", ret);
|
||||||
job_runtime_info.exit_code = ret;
|
|
||||||
job_runtime_info.stage = JOB_EXEC_STAGE_PRE_EXEC;
|
check_and_reconnect();
|
||||||
// TODO: Handle pre-exec failure
|
|
||||||
exit(1);
|
job_runtime_info.job_pid = 0;
|
||||||
} else {
|
job_runtime_info.job_status = JOB_FAILED;
|
||||||
XCECD_WORKER_LOG(XCE_LOG_DEBUG,
|
job_runtime_info.worker_status = WORKER_EXITED;
|
||||||
"Job pre-exec command completed successfully");
|
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()
|
static void job_post_exec()
|
||||||
|
|
@ -1706,14 +1744,21 @@ static void worker_job_callback(job_assignment_t* info)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/* Queue pre-exec */
|
/* Queue pre-exec: a failure already reported the job FAILED with the
|
||||||
if (job_runtime_info.q_pre_exec[0] != '\0') {
|
* command's exit code -- skip the body and take the same tail as the
|
||||||
queue_pre_exec();
|
* 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 */
|
/* Job pre-exec */
|
||||||
if (job_runtime_info.job_pre_exec[0] != '\0') {
|
if (job_runtime_info.job_pre_exec[0] != '\0' && job_pre_exec() != 0) {
|
||||||
job_pre_exec();
|
FREEUP(job_cmd);
|
||||||
|
ipc_worker_run(UV_RUN_NOWAIT);
|
||||||
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
job_runtime_info.stage = JOB_EXEC_STAGE_MAIN;
|
job_runtime_info.stage = JOB_EXEC_STAGE_MAIN;
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue