From 148557a5d5c6fb33c482ca4294ddedf4d348abe9 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sat, 10 Oct 2026 09:07:38 +0000 Subject: [PATCH] fix(worker): deliver the job-control signal the user asked for (bug-1954) worker_signal_callback() sent SIGTERM for a KILL request without force and SIGSTOP for any TSTP request. job_signal_to_deliver() (new job_signal.c) now delivers the requested signal; only a forced terminate escalates to SIGKILL. The apiserver's default signal for a plain stop becomes SIGSTOP (it was TSTP and relied on the worker's substitution), so a plain stop still suspends jobs that ignore SIGTSTP. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../apiserver/backend/convert_pure.go | 4 ++- .../apiserver/backend/jobctrl_params_test.go | 17 +++++++++- src/frontend/apiserver/models/jobs.go | 2 +- src/worker/include/job_signal.h | 10 ++++++ src/worker/meson.build | 1 + src/worker/src/job_signal.c | 15 ++++++++ src/worker/src/xcecd_worker.c | 14 ++++---- src/worker/testsuite/meson.build | 13 +++++++ src/worker/testsuite/test_job_signal.c | 34 +++++++++++++++++++ 9 files changed, 99 insertions(+), 11 deletions(-) create mode 100644 src/worker/include/job_signal.h create mode 100644 src/worker/src/job_signal.c create mode 100644 src/worker/testsuite/test_job_signal.c diff --git a/src/frontend/apiserver/backend/convert_pure.go b/src/frontend/apiserver/backend/convert_pure.go index 15b036cd..04383aa3 100644 --- a/src/frontend/apiserver/backend/convert_pure.go +++ b/src/frontend/apiserver/backend/convert_pure.go @@ -284,7 +284,9 @@ func jobCtrlParams(action int, content string) (jobCtrlArgs, error) { func defaultActionSignal(action int) int { switch action { case models.JOB_STOP: - return models.JobSignalMapTable["TSTP"] // 20 + // bug-1954: the worker delivers the requested signal, so a plain + // stop asks for the uncatchable SIGSTOP + return models.JobSignalMapTable["STOP"] // 19 case models.JOB_RESUME: return models.JobSignalMapTable["CONT"] // 18 } diff --git a/src/frontend/apiserver/backend/jobctrl_params_test.go b/src/frontend/apiserver/backend/jobctrl_params_test.go index 01f34fe3..c36dfd8b 100644 --- a/src/frontend/apiserver/backend/jobctrl_params_test.go +++ b/src/frontend/apiserver/backend/jobctrl_params_test.go @@ -55,7 +55,7 @@ func TestJobCtrlParams_DefaultSignalPerAction(t *testing.T) { want int }{ {models.JOB_KILL, 15}, - {models.JOB_STOP, 20}, + {models.JOB_STOP, 19}, // bug-1954: SIGSTOP {models.JOB_RESUME, 18}, } for _, tc := range cases { @@ -116,3 +116,18 @@ func TestJobCtrlParams_CheckpointCarriesKillAndMethod(t *testing.T) { got.CkptKill, got.Method) } } + +// bug-1954: the worker now delivers the requested signal as is, so a plain +// stop must ask for SIGSTOP (a job may ignore SIGTSTP). +func TestJobCtrlParams_PlainStopSendsSigstop(t *testing.T) { + args, err := jobCtrlParams(models.JOB_STOP, "") + if err != nil { + t.Fatalf("jobCtrlParams: %v", err) + } + if args.Signo != models.JobSignalMapTable["STOP"] { + t.Errorf("plain stop: signo %d, want SIGSTOP (%d)", args.Signo, models.JobSignalMapTable["STOP"]) + } + if models.DefaultActionSignal["stop"] != "STOP" { + t.Errorf("DefaultActionSignal[stop] = %q, want STOP", models.DefaultActionSignal["stop"]) + } +} diff --git a/src/frontend/apiserver/models/jobs.go b/src/frontend/apiserver/models/jobs.go index 29a4423b..5c30b4b5 100644 --- a/src/frontend/apiserver/models/jobs.go +++ b/src/frontend/apiserver/models/jobs.go @@ -127,7 +127,7 @@ var JobKillSignalControlMap = map[int][]int{ var DefaultActionSignal = map[string]string{ "kill": "TERM", - "stop": "TSTP", + "stop": "STOP", // bug-1954: the worker delivers exactly this signal "resume": "CONT", } diff --git a/src/worker/include/job_signal.h b/src/worker/include/job_signal.h new file mode 100644 index 00000000..02643ae5 --- /dev/null +++ b/src/worker/include/job_signal.h @@ -0,0 +1,10 @@ +#ifndef XCECD_JOB_SIGNAL_H +#define XCECD_JOB_SIGNAL_H + +#include + +/* The signal the job process group receives for a job-control request of + * `signal` with the force flag. */ +int job_signal_to_deliver(int signal, bool force); + +#endif diff --git a/src/worker/meson.build b/src/worker/meson.build index 4a339a44..abf45a02 100644 --- a/src/worker/meson.build +++ b/src/worker/meson.build @@ -8,6 +8,7 @@ xcecd_worker_srcs = [ 'src/xcecd_cgroups.c', 'src/xcecd_worker_utils.c', 'src/pending_signal.c', + 'src/job_signal.c', 'src/xcecd_resync.c', 'src/controller_connector.c', 'src/xcecd_dbus_manager.c', diff --git a/src/worker/src/job_signal.c b/src/worker/src/job_signal.c new file mode 100644 index 00000000..4461fa26 --- /dev/null +++ b/src/worker/src/job_signal.c @@ -0,0 +1,15 @@ +#include + +#include "job_signal.h" + +/* bug-1954: deliver the requested signal; only a forced terminate escalates + * to SIGKILL. A plain stop is sent as SIGSTOP by its sender (apiserver + * default), so an explicit SIGTSTP stays catchable. */ +int job_signal_to_deliver(int signal, bool force) +{ + if ((signal == SIGTERM) && force) { + return SIGKILL; + } + + return signal; +} diff --git a/src/worker/src/xcecd_worker.c b/src/worker/src/xcecd_worker.c index 1e16acb7..85961016 100644 --- a/src/worker/src/xcecd_worker.c +++ b/src/worker/src/xcecd_worker.c @@ -1,3 +1,4 @@ +#include "job_signal.h" #include "inter_job_utils.h" #include "process_manager.h" #include "static_array.h" @@ -1116,18 +1117,15 @@ static void worker_signal_callback(uint64_t job_id, int signal, bool force) case SIGTERM: case SIGKILL: XCECD_WORKER_LOG(XCE_LOG_INFO, "Terminating job %s with signal %d", - jobidStr, signal); - if (force) { - success = signal_gid_except_self(SIGKILL); - } else { - success = signal_gid_except_self(SIGTERM); - } + jobidStr, job_signal_to_deliver(signal, force)); + success = signal_gid_except_self(job_signal_to_deliver(signal, force)); break; case SIGSTOP: case SIGTSTP: - XCECD_WORKER_LOG(XCE_LOG_INFO, "Pausing job %s", jobidStr); - success = signal_gid_except_self(SIGSTOP); + XCECD_WORKER_LOG(XCE_LOG_INFO, "Pausing job %s with signal %d", + jobidStr, signal); + success = signal_gid_except_self(job_signal_to_deliver(signal, force)); if (success) { XCECD_WORKER_LOG(XCE_LOG_DEBUG2, "Job %s paused successfully", jobidStr); diff --git a/src/worker/testsuite/meson.build b/src/worker/testsuite/meson.build index 5618f604..0fc2f8ed 100644 --- a/src/worker/testsuite/meson.build +++ b/src/worker/testsuite/meson.build @@ -259,3 +259,16 @@ test('worker pending_signal test', pending_signal_test, suite: 'worker', timeout: 60, env: ['ASAN_OPTIONS=detect_leaks=0']) + +# bug-1954: job_signal_to_deliver() (job_signal.c), the signal a job-control +# request delivers to the job. +job_signal_test = executable('job_signal_test', + ['test_job_signal.c', '../src/job_signal.c'], + include_directories: [worker_test_incdir], + dependencies: worker_test_deps, + install: false) + +test('worker job_signal test', job_signal_test, + suite: 'worker', + timeout: 60, + env: ['ASAN_OPTIONS=detect_leaks=0']) diff --git a/src/worker/testsuite/test_job_signal.c b/src/worker/testsuite/test_job_signal.c new file mode 100644 index 00000000..90e5650f --- /dev/null +++ b/src/worker/testsuite/test_job_signal.c @@ -0,0 +1,34 @@ +/* bug-1954: the job receives the signal the user asked for. */ + +#include + +#include + +#include "job_signal.h" + +void setUp(void) {} +void tearDown(void) {} + +static void test_requested_signal_is_delivered(void) +{ + TEST_ASSERT_EQUAL_INT(SIGKILL, job_signal_to_deliver(SIGKILL, false)); + TEST_ASSERT_EQUAL_INT(SIGTSTP, job_signal_to_deliver(SIGTSTP, false)); + TEST_ASSERT_EQUAL_INT(SIGSTOP, job_signal_to_deliver(SIGSTOP, false)); + TEST_ASSERT_EQUAL_INT(SIGTERM, job_signal_to_deliver(SIGTERM, false)); + TEST_ASSERT_EQUAL_INT(SIGUSR1, job_signal_to_deliver(SIGUSR1, false)); + TEST_ASSERT_EQUAL_INT(SIGCONT, job_signal_to_deliver(SIGCONT, false)); +} + +static void test_force_kill_escalates_terminate(void) +{ + TEST_ASSERT_EQUAL_INT(SIGKILL, job_signal_to_deliver(SIGTERM, true)); + TEST_ASSERT_EQUAL_INT(SIGKILL, job_signal_to_deliver(SIGKILL, true)); +} + +int main(void) +{ + UNITY_BEGIN(); + RUN_TEST(test_requested_signal_is_delivered); + RUN_TEST(test_force_kill_escalates_terminate); + return UNITY_END(); +}