From ef728435d4ddb306b8fe0e1110821d0ba6411e3f Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Thu, 1 Oct 2026 09:24:00 +0000 Subject: [PATCH] feat(job-array-one-based-index): array ID allocation numbered from 1 (task 1.3) idgen_array() allocates the next whole 32768 block and returns its first element (array tag, index 1), refusing n outside 1..32767 and a block past the 20-bit sequence (-EOVERFLOW, context unchanged). The reset-on-new-second step is shared with idgen() via idgen_tick_locked(); idgen() itself is unchanged until task 9.1. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../specs/job-array-one-based-index/tasks.md | 3 +- src/include/idgen.h | 8 + src/lib/idgen.c | 55 +++++- src/lib/testsuite/test_idgen.c | 156 ++++++++++++++++++ 4 files changed, 212 insertions(+), 10 deletions(-) diff --git a/.kiro/specs/job-array-one-based-index/tasks.md b/.kiro/specs/job-array-one-based-index/tasks.md index 1816c163..264cbed9 100644 --- a/.kiro/specs/job-array-one-based-index/tasks.md +++ b/.kiro/specs/job-array-one-based-index/tasks.md @@ -16,7 +16,7 @@ - Done when the libxce suite passes with new cases for a NULL buffer, a range, an oversized field and an index above 32767 all returning invalid-argument, the existing round trips still succeed, and the scheduler and harness suites still pass - _Requirements: 5.7, 5.14, 5.15_ -- [ ] 1.3 (P) Add array ID allocation numbered from 1 +- [x] 1.3 (P) Add array ID allocation numbered from 1 - Allocate the first element of an n-element array (1 to 32767) as the next whole 32768 block with element number 1, advancing the context past the block - Refuse a block that would leave the 20-bit sequence with an overflow result and leave the context unchanged; refuse n of 0 or above 32767 - Leave the existing ordinary-ID allocation untouched in this task (its multi-element path is removed in 9.1, after its callers move) @@ -239,3 +239,4 @@ ## Implementation Notes - 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. diff --git a/src/include/idgen.h b/src/include/idgen.h index d03874f1..d10d8efd 100644 --- a/src/include/idgen.h +++ b/src/include/idgen.h @@ -20,4 +20,12 @@ extern int idgen_ctx_init(idgen_ctx_t*); extern int idgen(idgen_ctx_t*, size_t, size_t, size_t, uint64_t*); extern void idgen_ctx_fini(idgen_ctx_t*); +/* First element ID of a new n-element array, numbered from 1. + * 1 <= n <= MAX_ARRAY_IDX (32767). On success *first = MAKE_JOBID(c, a, ts, + * TAG_JOB_ARRAY, (block << 15) | 1), the context is advanced past the whole + * block and n is returned. -EINVAL for bad args or n; -EOVERFLOW if the next + * block would leave the sequence field (the context is left unchanged). */ +extern int idgen_array(idgen_ctx_t* ctx, size_t clusterId, size_t apiserverId, + size_t n, uint64_t* first); + #endif diff --git a/src/lib/idgen.c b/src/lib/idgen.c index 306d0ab5..06724ff4 100644 --- a/src/lib/idgen.c +++ b/src/lib/idgen.c @@ -19,10 +19,23 @@ int idgen_ctx_init(idgen_ctx_t* ctx) return 0; } +/* Caller holds ctx->lock. Starts a fresh sequence when the second changes + * and returns the timestamp to stamp the new ID with. */ +static time_t idgen_tick_locked(idgen_ctx_t* ctx) +{ + time_t now = time(NULL); + + if (now != ctx->timestamp) { + ctx->timestamp = now; + ctx->sequence = 0; + } + + return ctx->timestamp; +} + int idgen(idgen_ctx_t* ctx, size_t clusterId, size_t apiserverId, size_t arraySize, uint64_t* pidx) { - time_t now; uint64_t idx; uint64_t ts, tag, seq; @@ -33,14 +46,7 @@ int idgen(idgen_ctx_t* ctx, size_t clusterId, size_t apiserverId, } mylock_lock(&(ctx->lock)); - now = time(NULL); - - if (now != ctx->timestamp) { - ctx->timestamp = now; - ctx->sequence = 0; - } - - ts = ctx->timestamp; + ts = idgen_tick_locked(ctx); if (arraySize == 1) { /* non array job */ @@ -60,6 +66,37 @@ int idgen(idgen_ctx_t* ctx, size_t clusterId, size_t apiserverId, return arraySize; } +int idgen_array(idgen_ctx_t* ctx, size_t clusterId, size_t apiserverId, + size_t n, uint64_t* first) +{ + uint64_t ts, block_start; + + if ((ctx == NULL) || (clusterId > MAX_CLUSTER_ID) + || (apiserverId > MAX_APISERVER_ID) || (n < 1) || (n > MAX_ARRAY_IDX) + || (first == NULL)) { + return -EINVAL; + } + + mylock_lock(&(ctx->lock)); + ts = idgen_tick_locked(ctx); + block_start = ROUNDUP((uint64_t)ctx->sequence, N_ARRAY_IDX_BITS); + + if (block_start > UMAX_BITS(N_SEQUENCE_BITS)) { + /* the next whole block lies outside the sequence field; a tick + * above cannot get here (it resets the sequence to 0), so the + * context is unchanged */ + mylock_unlock(&(ctx->lock)); + return -EOVERFLOW; + } + + ctx->sequence = (uint32_t)(block_start + ((uint64_t)1 << N_ARRAY_IDX_BITS)); + mylock_unlock(&(ctx->lock)); + + *first = MAKE_JOBID(clusterId, apiserverId, ts, TAG_JOB_ARRAY, + block_start | 1); + return (int)n; +} + void idgen_ctx_fini(idgen_ctx_t* ctx) { if (ctx == NULL) { diff --git a/src/lib/testsuite/test_idgen.c b/src/lib/testsuite/test_idgen.c index 3031a560..3498215f 100644 --- a/src/lib/testsuite/test_idgen.c +++ b/src/lib/testsuite/test_idgen.c @@ -6,6 +6,8 @@ #include "idgen.h" #include "utils.h" #include +#include +#include static void test_idgen_invalid(void) { @@ -85,6 +87,157 @@ static void test_idgen_array(void) idgen_ctx_fini(&ctx); } +#define ARRAY_BLOCK ((uint32_t)1 << N_ARRAY_IDX_BITS) + +static void test_idgen_array_invalid(void) +{ + idgen_ctx_t ctx; + uint64_t first = 0; + + idgen_ctx_init(&ctx); + + TEST_ASSERT_EQUAL_INT(-EINVAL, idgen_array(NULL, 0, 0, 2, &first)); + TEST_ASSERT_EQUAL_INT(-EINVAL, idgen_array(&ctx, 0, 0, 2, NULL)); + TEST_ASSERT_EQUAL_INT(-EINVAL, + idgen_array(&ctx, MAX_CLUSTER_ID + 1, 0, 2, &first)); + TEST_ASSERT_EQUAL_INT( + -EINVAL, idgen_array(&ctx, 0, MAX_APISERVER_ID + 1, 2, &first)); + TEST_ASSERT_EQUAL_INT(-EINVAL, idgen_array(&ctx, 0, 0, 0, &first)); + TEST_ASSERT_EQUAL_INT(-EINVAL, idgen_array(&ctx, 0, 0, 32768, &first)); + TEST_ASSERT_EQUAL_INT(-EINVAL, idgen_array(&ctx, 0, 0, SIZE_MAX, &first)); + + idgen_ctx_fini(&ctx); +} + +/* Seed the context and call idgen_array within one wall-clock second. + * idgen resets the sequence when the second changes, so a call that + * straddles a second boundary is retried. Returns idgen_array's rc and + * stores the pinned timestamp in *ts. */ +static int idgen_array_seeded(idgen_ctx_t* ctx, uint32_t seed, size_t n, + uint64_t* first, time_t* ts) +{ + uint64_t first0 = *first; /* restored per attempt: a discarded attempt + * that succeeded must not leak into *first */ + + for (;;) { + time_t t0 = time(NULL); + int rc; + + *first = first0; + ctx->timestamp = t0; + ctx->sequence = seed; + rc = idgen_array(ctx, 5, 3, n, first); + if (time(NULL) == t0) { + *ts = t0; + return rc; + } + } +} + +static void check_array_first(uint64_t first, size_t n, time_t ts, + uint32_t block) +{ + uint64_t last = first + n - 1; + + TEST_ASSERT_EQUAL_UINT64(TAG_JOB_ARRAY, EXTRACT_ID_TAG(first)); + TEST_ASSERT_EQUAL_UINT64(5, EXTRACT_ID_CLUSTER(first)); + TEST_ASSERT_EQUAL_UINT64(3, EXTRACT_ID_APISERVER(first)); + TEST_ASSERT_EQUAL_UINT64((uint64_t)ts, EXTRACT_ID_TIMESTAMP(first)); + TEST_ASSERT_EQUAL_UINT64(1, EXTRACT_ARRAY_IDX(first)); + TEST_ASSERT_EQUAL_UINT64((uint64_t)block * ARRAY_BLOCK, + EXTRACT_ARRAY_JOB(first)); + + /* the last element shares the array identity and has index n */ + TEST_ASSERT_EQUAL_UINT64(n, EXTRACT_ARRAY_IDX(last)); + TEST_ASSERT_EQUAL_UINT64(EXTRACT_ARRAY_BASEID(first), + EXTRACT_ARRAY_BASEID(last)); + TEST_ASSERT_EQUAL_UINT64(TAG_JOB_ARRAY, EXTRACT_ID_TAG(last)); +} + +static void test_idgen_array_one_based(void) +{ + idgen_ctx_t ctx; + uint64_t first = 0; + time_t ts; + size_t sizes[] = { 1, 2, 7, 1000, 32766, 32767 }; + size_t i; + + idgen_ctx_init(&ctx); + + for (i = 0; i < sizeof(sizes) / sizeof(sizes[0]); i++) { + int rc = idgen_array_seeded(&ctx, 0, sizes[i], &first, &ts); + + TEST_ASSERT_EQUAL_INT((int)sizes[i], rc); + check_array_first(first, sizes[i], ts, 0); + /* the context is advanced past the whole block */ + TEST_ASSERT_EQUAL_UINT32(ARRAY_BLOCK, ctx.sequence); + TEST_ASSERT_EQUAL_INT64((int64_t)ts, (int64_t)ctx.timestamp); + } + + /* a partly used block is skipped: the next whole block is taken */ + TEST_ASSERT_EQUAL_INT( + 3, idgen_array_seeded(&ctx, ARRAY_BLOCK + 5, 3, &first, &ts)); + check_array_first(first, 3, ts, 2); + TEST_ASSERT_EQUAL_UINT32(3 * ARRAY_BLOCK, ctx.sequence); + + /* consecutive arrays get distinct identities */ + { + uint64_t second = 0; + int rc; + + rc = idgen_array_seeded(&ctx, 0, 4, &first, &ts); + TEST_ASSERT_EQUAL_INT(4, rc); + rc = idgen_array(&ctx, 5, 3, 4, &second); + TEST_ASSERT_EQUAL_INT(4, rc); + TEST_ASSERT_TRUE(EXTRACT_ARRAY_BASEID(first) + != EXTRACT_ARRAY_BASEID(second)); + TEST_ASSERT_EQUAL_UINT64(1, EXTRACT_ARRAY_IDX(second)); + } + + idgen_ctx_fini(&ctx); +} + +static void test_idgen_array_overflow(void) +{ + idgen_ctx_t ctx; + uint64_t first = 0; + time_t ts; + uint32_t over[] + = { 31 * ARRAY_BLOCK + 1, 31 * ARRAY_BLOCK + 12345, (1u << 20) - 1 }; + uint32_t ok[] = { 0, 1, 29 * ARRAY_BLOCK, 30 * ARRAY_BLOCK - 1 }; + size_t i; + + idgen_ctx_init(&ctx); + + /* a seed in (31 * 32768, 2^20) would leave the 20-bit sequence */ + for (i = 0; i < sizeof(over) / sizeof(over[0]); i++) { + first = 0xdeadbeefULL; + TEST_ASSERT_EQUAL_INT( + -EOVERFLOW, idgen_array_seeded(&ctx, over[i], 2, &first, &ts)); + TEST_ASSERT_EQUAL_UINT32(over[i], ctx.sequence); + TEST_ASSERT_EQUAL_INT64((int64_t)ts, (int64_t)ctx.timestamp); + TEST_ASSERT_EQUAL_UINT64(0xdeadbeefULL, first); + } + + /* exactly 31 blocks: the last block (31) is still usable */ + TEST_ASSERT_EQUAL_INT( + 32767, idgen_array_seeded(&ctx, 31 * ARRAY_BLOCK, 32767, &first, &ts)); + check_array_first(first, 32767, ts, 31); + TEST_ASSERT_EQUAL_UINT32(32 * ARRAY_BLOCK, ctx.sequence); + + /* seeds that round to block 30 or below succeed */ + for (i = 0; i < sizeof(ok) / sizeof(ok[0]); i++) { + uint32_t block = (ok[i] + ARRAY_BLOCK - 1) / ARRAY_BLOCK; + + TEST_ASSERT_EQUAL_INT(2, + idgen_array_seeded(&ctx, ok[i], 2, &first, &ts)); + check_array_first(first, 2, ts, block); + TEST_ASSERT_EQUAL_UINT32((block + 1) * ARRAY_BLOCK, ctx.sequence); + } + + idgen_ctx_fini(&ctx); +} + void test_idgen(void) { printf("\n>> idgen test\n\n"); @@ -92,4 +245,7 @@ void test_idgen(void) RUN_TEST(test_idgen_invalid); RUN_TEST(test_idgen1); RUN_TEST(test_idgen_array); + RUN_TEST(test_idgen_array_invalid); + RUN_TEST(test_idgen_array_one_based); + RUN_TEST(test_idgen_array_overflow); }