From d2c87f185aa62c732004d73e4ebe902ef0cb6412 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Thu, 1 Oct 2026 10:47:22 +0000 Subject: [PATCH] feat(job-array-one-based-index): harness jobsub -a numbers arrays from 1 (task 4.1) jobsub -a parses its value strictly (digits only, leading zeros ok): 1 submits an ordinary job; 2..32767 allocate with idgen_array, whose result is checked; missing, non-digit and 0 values are refused with the 1-32767 range and values above 32767 with the limit, submitting nothing. The success line and $jobid name the array identity; the random sequence seed is bounded so the first array cannot overflow; help text updated. Fixes bug-1896 for the harness. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../specs/job-array-one-based-index/tasks.md | 3 +- src/harness/cmdutil.c | 32 ++++++- src/harness/include/cmdutil.h | 7 ++ src/harness/test.c | 91 ++++++++++++++++--- src/harness/testsuite/test_cmdutil.c | 49 ++++++++++ src/harness/testsuite/test_test_cli.c | 62 +++++++++++++ 6 files changed, 228 insertions(+), 16 deletions(-) diff --git a/.kiro/specs/job-array-one-based-index/tasks.md b/.kiro/specs/job-array-one-based-index/tasks.md index dc5a6145..45b0e3a3 100644 --- a/.kiro/specs/job-array-one-based-index/tasks.md +++ b/.kiro/specs/job-array-one-based-index/tasks.md @@ -69,7 +69,7 @@ - _Depends: 1.1_ - [ ] 4. Test harness -- [ ] 4.1 Submit harness arrays numbered from 1 (fixes bug-1896 for the harness) +- [x] 4.1 Submit harness arrays numbered from 1 (fixes bug-1896 for the harness) - Parse `-a` strictly (digits only, leading zeros allowed): 1 submits an ordinary job; 2..32767 allocate with the new array allocator and check its result; missing, non-digit, 0 and above-32767 values are refused with the accepted range or limit, submitting nothing - Bound the random sequence seed so the first array of a run cannot overflow - Print the array identity on success and set `$jobid` to it; update the jobsub help text @@ -241,3 +241,4 @@ - 1.2: `REGEX_JOBID`, `REGEX_JOBARRAY_ID` and `N_MAX_FIELDS_JOBID` (`src/include/config.h`) are now unused, and comments still describe regex-based `parse_jobid` in `src/scheduler/src/repl.c:143`, `src/harness/testsuite/test_test_cli.c:2324-2326` and `src/frontend/xcecli/common/format/jobid.go:86-93`; clean these up in 4.3 / 8.1 / 9.x where those files are touched. A direct `parse_jobid("...[0]")` accept test is a cheap addition when next touching `test_jobsel.c`. - 1.3: plain `idgen()` still has no sequence-overflow guard; after `idgen_array` takes block 31 the context sequence is 2^20, so a plain `idgen()` in the same second would spill into the timestamp (pre-existing class). Add an `-EOVERFLOW` check to plain `idgen()` in 9.1. - 2.2: two extra XCE_TESTING seams exist (`jobsubmit_test_release_hook`, `jobsubmit_inflight_try_add_for_test`); the controller test fixture has no WAL, so async commit-failure runs synchronously inside handle_jobsubmit — reuse it in 2.3. Stale comment at `src/controller/testsuite/test_commit_barrier_reply.c:1458` (claims release via jobsubmit_async_ctx_release) — fix in 2.3. +- 4.1: after `jobsub -a N`, `$jobid` is the array identity, so `expect-job $jobid` currently reports not found; 4.3 must make it behave per 4.7/4.9 (not-found hint naming `[k]`, or exact-ID match). Nit for 4.3: the `refused` early return in `cmd_jobsub` sits between the bug-786 ack comment and the code it describes — move it above the comment. diff --git a/src/harness/cmdutil.c b/src/harness/cmdutil.c index e359f820..5f186e6a 100644 --- a/src/harness/cmdutil.c +++ b/src/harness/cmdutil.c @@ -11,7 +11,8 @@ #include #include "cmdutil.h" -#include "utils.h" /* xce_strerror_r */ +#include "config.h" /* MAX_ARRAY_IDX */ +#include "utils.h" /* xce_strerror_r */ #include "xce/errnox.h" /* strtol over [s, s + len): the whole span must be digits (optional @@ -53,6 +54,35 @@ int cmdutil_parse_long(const char* s, long lo, long hi, long* out) return parse_span(s, (s == NULL) ? 0 : strlen(s), lo, hi, out); } +int cmdutil_parse_array_size(const char* text, uint32_t* n) +{ + uint32_t v = 0; + int over = 0; + + if ((text == NULL) || (text[0] == '\0')) { + return -EINVAL; + } + for (const char* p = text; *p != '\0'; p++) { + if ((*p < '0') || (*p > '9')) { + return -EINVAL; + } + /* saturate instead of wrapping: any value past the limit is + * -ERANGE no matter how many digits follow */ + if (!over) { + v = v * 10 + (uint32_t)(*p - '0'); + over = (v > MAX_ARRAY_IDX); + } + } + if (over) { + return -ERANGE; + } + if (v == 0) { + return -EINVAL; + } + *n = v; + return 0; +} + int cmdutil_split_host_tasks(const char* arg, char* host, size_t hostsz, int* ntasks) { diff --git a/src/harness/include/cmdutil.h b/src/harness/include/cmdutil.h index 68e0ee81..617608bc 100644 --- a/src/harness/include/cmdutil.h +++ b/src/harness/include/cmdutil.h @@ -24,6 +24,13 @@ int cmdutil_split_host_tasks(const char* arg, char* host, size_t hostsz, * (not numeric, or max < min) or -ERANGE (min < 1, or beyond UINT32_MAX). */ int cmdutil_parse_ntasks(const char* arg, uint32_t* min, uint32_t* max); +/* Parse jobsub's "-a " array size (job-array-one-based-index Req 4.1-4.4): + * one or more decimal digits only (leading zeros allowed; no sign, no + * whitespace, no other characters). Returns 0 with *n in [1, 32767], + * -EINVAL (NULL/empty, any non-digit, or value 0) or -ERANGE (value above + * 32767, MAX_ARRAY_IDX); *n is left unchanged on error. */ +int cmdutil_parse_array_size(const char* text, uint32_t* n); + /* jobsub's default exdata: when *exdata is NULL, set it to a newly * allocated {"sub_env_vars":{"PATH":}} JSON string; an exdata the * caller already set (-x) is left alone. Returns 0 or -ENOMEM. */ diff --git a/src/harness/test.c b/src/harness/test.c index e41eddda..d7382d93 100644 --- a/src/harness/test.c +++ b/src/harness/test.c @@ -609,8 +609,12 @@ static const char USAGE_JOBSUB_HEAD[] = " default.\n"; static const char USAGE_JOBSUB_TAIL[] = + " -a N submits one array whose elements are numbered 1 through N;\n" + " N may be at most 32767 (digits only), and -a 1 submits an ordinary\n" + " job.\n" " On success, captures the submitted JobId into the $jobid\n" - " auto-variable.\n" + " auto-variable (for an array, the array identity without an\n" + " element number; name element k as $jobid[k]).\n" "example: jobsub -u alice -q normal -c \"sleep 10\" -n 1,4\n"; static const char USAGE_RESVAL_HEAD[] = @@ -3613,6 +3617,10 @@ int cmd_jobsub(int argc, char* argv[]) uint64_t jobid = 0; uint32_t min = 1, max = 1; uint32_t arsize = 1; + /* set when jobsub refuses its own arguments or cannot allocate an ID: the + * message is already printed and no RPC was made, so the "jobsubmit + * failed" report below must not follow it */ + int refused = 0; char jobidStr[128]; struct option long_opts[JOBSUB_NFLAGS + 1]; char optstring[32]; @@ -3668,12 +3676,37 @@ int cmd_jobsub(int argc, char* argv[]) } break; case 'a': - arsize = atoi(optarg); - if (arsize <= 0) { - arsize = 1; + /* job-array-one-based-index Req 4.1-4.4: digits only; 0 and + * anything malformed used to fall back silently to 1 (atoi) */ + rc = cmdutil_parse_array_size(optarg, &arsize); + if (rc == -ERANGE) { + fprintf(stderr, + "jobsub: -a %.40s exceeds the maximum array size of " + "%d elements\n", + optarg, (int)MAX_ARRAY_IDX); + } else if (rc < 0) { + fprintf(stderr, + "jobsub: -a expects the number of array elements, a " + "whole number from 1 to %d, got '%.40s'\n", + (int)MAX_ARRAY_IDX, optarg); + } + if (rc < 0) { + rc = -EINVAL; + refused = 1; + goto cleanup; } break; case '?': + if (optopt == 'a') { + /* missing value (Req 4.4): state the accepted range */ + fprintf(stderr, + "jobsub: -a expects the number of array elements, a " + "whole number from 1 to %d, got none\n", + (int)MAX_ARRAY_IDX); + rc = -EINVAL; + refused = 1; + goto cleanup; + } rc = flagspec_reject( "jobsub", jobsub_flags, JOBSUB_NFLAGS, argv[optind - 1], "jobsub -u -q -c [-R ] " @@ -3718,8 +3751,29 @@ int cmd_jobsub(int argc, char* argv[]) * pid into apiserverId gives each xce_test invocation its own slice of * that same partitioning, making a same-second cross-process collision * structurally impossible here too. */ - idgen(&idctx, 0, (size_t)((uint64_t)getpid() & MAX_APISERVER_ID), arsize, - &(jobsubmit.jobid)); + { + size_t apiserverId = (size_t)((uint64_t)getpid() & MAX_APISERVER_ID); + + /* job-array-one-based-index Req 4.1, 4.2, 4.6: -a 1 is an ordinary + * job; N >= 2 takes a whole 32768 block numbered from 1, and a + * failure to obtain it submits nothing. */ + if (arsize == 1) { + rc = idgen(&idctx, 0, apiserverId, 1, &(jobsubmit.jobid)); + } else { + rc = idgen_array(&idctx, 0, apiserverId, arsize, + &(jobsubmit.jobid)); + } + if (rc < 0) { + char ebuf[128]; + + fprintf(stderr, "jobsub: cannot allocate a job ID for %s: %s\n", + (arsize == 1) ? "the job" : "the array", + cmdutil_errstr(rc, ebuf, sizeof(ebuf))); + refused = 1; + goto cleanup; + } + rc = 0; + } if (NULLSTR(submit.start_info->execuser)) { submit.start_info->execuser = strdup(submit.user); @@ -3809,6 +3863,11 @@ cleanup: * reply.data.jobid = job_submit() = base_id = submit->jobid; verified * round-trip). `jobid != jobsubmit.jobid` covers BOTH the never-written * (zero) case AND the stale non-zero garbage case in one uint64 compare. */ + if ((rc < 0) && refused) { + SET_ERROR(-rc); + return rc; + } + if (rc < 0) { char ebuf[128]; @@ -3857,13 +3916,13 @@ cleanup: fmt_jobid(jobidStr, sizeof(jobidStr) - 1, array_jobid(jobid)); - /* Capture $jobid (Req 3.2) using the SAME representation cmd_joblist prints - * for a job's JobId column — fmt_jobid(jobs[i]->jobid), the full jobid - * WITHOUT array_jobid() stripping — so a later `$jobid` substitution and - * `expect-job $jobid` compare equal to the joblist entry for this job. Only - * set on this clean-ack + valid-id path; never on a failure path, so an - * unset $jobid still fails per Req 3.6. */ - fmt_jobid(g_last_jobid, sizeof(g_last_jobid) - 1, jobid); + /* Capture $jobid (Req 3.2): for an ordinary job array_jobid() is the + * identity itself, the same text cmd_joblist prints. For an array it is + * the array identity without an element number (job-array-one-based-index + * Req 4.5), so elements are named `$jobid[k]`. Only set on this clean-ack + * + valid-id path; never on a failure path, so an unset $jobid still + * fails per Req 3.6. */ + fmt_jobid(g_last_jobid, sizeof(g_last_jobid) - 1, array_jobid(jobid)); printf("jobsubmit ok: JobId: %s\n", jobidStr); @@ -6895,8 +6954,12 @@ int main(int argc, char* argv[]) { struct timespec ts; clock_gettime(CLOCK_MONOTONIC, &ts); + /* job-array-one-based-index: bounded to [0, 30 * 32768) rather than + * the whole 20-bit field, so at least one whole array block always + * remains -- a seed above 31 blocks would make the first idgen_array + * of the second fail with -EOVERFLOW (about 1 run in 32). */ idctx.sequence = ((uint32_t)getpid() ^ (uint32_t)ts.tv_nsec) - & (uint32_t)UMAX_BITS(N_SEQUENCE_BITS); + % (uint32_t)(30u << N_ARRAY_IDX_BITS); } /* errexit default, modelled on shell `-e`/`+e`, keyed on the stdin mode: diff --git a/src/harness/testsuite/test_cmdutil.c b/src/harness/testsuite/test_cmdutil.c index 7a0d3b62..be9d625b 100644 --- a/src/harness/testsuite/test_cmdutil.c +++ b/src/harness/testsuite/test_cmdutil.c @@ -146,6 +146,52 @@ static void test_parse_ntasks_rejects_malformed(void) TEST_ASSERT_EQUAL_INT(-ERANGE, cmdutil_parse_ntasks("0", &mn, &mx)); } +/* ── cmdutil_parse_array_size (job-array-one-based-index Req 4.1-4.4) ──── */ + +static void test_parse_array_size_accepts_digits(void) +{ + uint32_t n = 0; + + TEST_ASSERT_EQUAL_INT(0, cmdutil_parse_array_size("1", &n)); + TEST_ASSERT_EQUAL_UINT32(1, n); + TEST_ASSERT_EQUAL_INT(0, cmdutil_parse_array_size("01", &n)); + TEST_ASSERT_EQUAL_UINT32(1, n); + TEST_ASSERT_EQUAL_INT(0, cmdutil_parse_array_size("3", &n)); + TEST_ASSERT_EQUAL_UINT32(3, n); + TEST_ASSERT_EQUAL_INT(0, cmdutil_parse_array_size("32767", &n)); + TEST_ASSERT_EQUAL_UINT32(32767, n); +} + +static void test_parse_array_size_rejects_non_digits_and_zero(void) +{ + static const char* const bad[] + = { "0", "00", "-3", "+5", " 5", "5 ", "5.0", "0x5", "abc", "", NULL }; + uint32_t n; + + for (size_t i = 0; i < sizeof(bad) / sizeof(bad[0]); i++) { + n = 77; + TEST_ASSERT_EQUAL_INT_MESSAGE(-EINVAL, + cmdutil_parse_array_size(bad[i], &n), + bad[i] ? bad[i] : "(NULL)"); + TEST_ASSERT_EQUAL_UINT32_MESSAGE(77, n, "*n must be left unchanged"); + } +} + +static void test_parse_array_size_rejects_above_limit(void) +{ + char big[64]; + uint32_t n = 77; + + TEST_ASSERT_EQUAL_INT(-ERANGE, cmdutil_parse_array_size("32768", &n)); + TEST_ASSERT_EQUAL_INT(-ERANGE, cmdutil_parse_array_size("40000", &n)); + /* far more digits than any integer type holds: no wrap to a small value */ + memset(big, '9', sizeof(big) - 1); + big[sizeof(big) - 1] = '\0'; + TEST_ASSERT_EQUAL_INT(-ERANGE, cmdutil_parse_array_size(big, &n)); + TEST_ASSERT_EQUAL_INT(-ERANGE, cmdutil_parse_array_size("4294967297", &n)); + TEST_ASSERT_EQUAL_UINT32(77, n); +} + /* ── cmdutil_default_exdata (bug-1705 jobsub -x) ────────────────────────── */ static void test_default_exdata_keeps_caller_value(void) @@ -205,6 +251,9 @@ int main(void) RUN_TEST(test_parse_ntasks_single_and_range); RUN_TEST(test_parse_ntasks_rejects_long_min_without_overflow); RUN_TEST(test_parse_ntasks_rejects_malformed); + RUN_TEST(test_parse_array_size_accepts_digits); + RUN_TEST(test_parse_array_size_rejects_non_digits_and_zero); + RUN_TEST(test_parse_array_size_rejects_above_limit); RUN_TEST(test_default_exdata_keeps_caller_value); RUN_TEST(test_default_exdata_builds_path_json_when_unset); RUN_TEST(test_errstr_reports_the_rpc_code_not_errno); diff --git a/src/harness/testsuite/test_test_cli.c b/src/harness/testsuite/test_test_cli.c index 92ba56cc..12cc6189 100644 --- a/src/harness/testsuite/test_test_cli.c +++ b/src/harness/testsuite/test_test_cli.c @@ -3546,6 +3546,64 @@ static void test_jobsub_overlong_ntasks_rejected_without_overflow(void) batch_free(&out, &err); } +/* job-array-one-based-index Req 4.4: `-a 0` used to fall back silently to + * an ordinary job (atoi + `arsize <= 0 -> 1`); now refused with the + * accepted range, before any RPC. */ +static void test_jobsub_array_size_zero_refused(void) +{ + clitest_buf_t out, err; + + TEST_ASSERT_EQUAL_INT( + 1, batch_run("client", "jobsub -u u -q normal -c ls -a 0", &out, &err)); + TEST_ASSERT_NOT_NULL_MESSAGE(strstr(err.data, "1 to 32767"), + "the refusal must state the accepted range"); + TEST_ASSERT_NULL_MESSAGE(strstr(err.data, "jobsubmit failed"), + "a refused -a must not reach the RPC"); + batch_free(&out, &err); +} + +/* Req 4.3: an array size above 32767 is refused with the limit stated. */ +static void test_jobsub_array_size_above_limit_refused(void) +{ + clitest_buf_t out, err; + + TEST_ASSERT_EQUAL_INT(1, batch_run("client", + "jobsub -u u -q normal -c ls -a 40000", + &out, &err)); + TEST_ASSERT_NOT_NULL_MESSAGE(strstr(err.data, "32767"), + "the refusal must state the limit"); + TEST_ASSERT_NULL_MESSAGE(strstr(err.data, "jobsubmit failed"), + "a refused -a must not reach the RPC"); + batch_free(&out, &err); +} + +/* Req 4.4: a missing -a value is refused with the accepted range. */ +static void test_jobsub_array_size_missing_refused(void) +{ + clitest_buf_t out, err; + + TEST_ASSERT_EQUAL_INT( + 1, batch_run("client", "jobsub -u u -q normal -c ls -a", &out, &err)); + TEST_ASSERT_NOT_NULL_MESSAGE(strstr(err.data, "1 to 32767"), + "the refusal must state the accepted range"); + TEST_ASSERT_NULL_MESSAGE(strstr(err.data, "jobsubmit failed"), + "a refused -a must not reach the RPC"); + batch_free(&out, &err); +} + +/* Req 9.3: `help jobsub` states the 1..N numbering and the 32767 limit. */ +static void test_help_jobsub_states_array_numbering_and_limit(void) +{ + clitest_buf_t out, err; + + TEST_ASSERT_EQUAL_INT(0, batch_run("client", "help jobsub", &out, &err)); + TEST_ASSERT_NOT_NULL_MESSAGE(strstr(out.data, "numbered 1 through N"), + "help must state elements are numbered 1..N"); + TEST_ASSERT_NOT_NULL_MESSAGE(strstr(out.data, "at most 32767"), + "help must state N may be at most 32767"); + batch_free(&out, &err); +} + /* bug-1708: `-s SIGKILL` became signal 0 and the kill was still sent. */ static void test_jobkill_non_numeric_signal_rejected(void) { @@ -3669,6 +3727,10 @@ void test_test_cli(void) RUN_TEST(test_set_list_success_result_on_stdout_not_stderr); RUN_TEST(test_jobrun_overlong_host_rejected_without_overflow); RUN_TEST(test_jobsub_overlong_ntasks_rejected_without_overflow); + RUN_TEST(test_jobsub_array_size_zero_refused); + RUN_TEST(test_jobsub_array_size_above_limit_refused); + RUN_TEST(test_jobsub_array_size_missing_refused); + RUN_TEST(test_help_jobsub_states_array_numbering_and_limit); RUN_TEST(test_jobkill_non_numeric_signal_rejected); RUN_TEST(test_jobinfo_without_valid_jobid_fails); RUN_TEST(test_hello_nonpositive_count_rejected);