Spec for numbering job-array elements from 1 (bug-1896): approved requirements, a design reviewed over 20 rounds until clean (log in research.md), and a 30-task plan. The design review found two live apiserver defects, logged here: bracketed job_id/job_name filters are never evaluated (bug-1898) and ControlJobByID re-dispatches earlier selections (bug-1899). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
220 lines
27 KiB
Markdown
220 lines
27 KiB
Markdown
# Research & Design Decisions
|
|
|
|
## Summary
|
|
- **Feature**: `job-array-one-based-index`
|
|
- **Discovery Scope**: Extension (light discovery; four parallel codebase investigations on 2026-10-01 at `8f5ba635`: libxce + controller, apiserver, xcecli, harness + worker + scheduler + skill scripts)
|
|
- **Key Findings**:
|
|
- The requirements' Boundary notes are accurate line for line. Discovery adds a set of defects in the same code that the design must absorb because the acceptance criteria depend on them (listed under "Discovery corrections" below).
|
|
- Every identity parser in the system (C `parse_jobid`, Go apiserver `ParseHumanJobID`/`parseArrayName`/`parseArrayIndexSection`/`IsNumeric`, Go xcecli `ParseJobID`/`ToAPIJobID`/`isValidJobId`) is unbounded and lenient in its own way. Requirements 5.7, 5.8 and 5.12-5.17 reduce to one grammar plus one set of range checks. The design therefore has exactly one bounded parser per language, with every caller routed through it.
|
|
- Nothing on the persistence, replication or scheduler-recording side depends on where element numbering starts: WAL `JOB_ARRAY_SUBMIT`, snapshots, HA apply and scheduler recording carry `(first element ID, size)` and expand with `first + i`. Scheduler expansion happens in `driver.c` translation, before core events, so the recording and the simulator see expanded jobs and the driver state-equivalence invariant is untouched.
|
|
|
|
## Research Log
|
|
|
|
### Job identity bit layout and helpers
|
|
- **Context**: Option B (decided in requirements) puts the element number in the existing 15-bit index field and submits the first element as `base | 1`.
|
|
- **Sources Consulted**: `src/include/config.h:37-72`, `src/include/utils.h:20-48`, `src/lib/utils.c:578-710`, `src/frontend/apiserver/backend/convert_pure.go:12-133`, `src/frontend/xcecli/common/format/jobid.go:9-148`.
|
|
- **Findings**:
|
|
- Layout: tag (2 bits, 62-63), cluster (6, 56-61), API server (4, 52-55), timestamp (32, 20-51), sequence (20, 0-19). For arrays, sequence = block (5 bits, 15-19) + index (15 bits, 0-14). So there are 32 array blocks per second per API server.
|
|
- `MAKE_JOBID` and both Go `MakeJobID`s OR fields without masking. An oversized field silently spills into its neighbour.
|
|
- The array identity (`EXTRACT_ARRAY_BASEID`, C `array_jobid`, Go `ExtractArrayBaseID`) is untagged with the low 15 bits clear.
|
|
- Array marking disagrees: `fmt_jobid` and xcecli test `tag & 1`; `array_jobid`/`array_idx` and apiserver `ExtractArrayBaseID` treat any non-zero tag as an array; apiserver `IsArrayJob`/`FormatJobID` test `tag == 1`.
|
|
- **Implications**: The array test becomes `tag == 1` wherever a new decision is made (controller 7.5, element-only matching, worker). The display helpers keep their behaviour, because only tags 0 and 1 have ever been issued.
|
|
|
|
### ID generation
|
|
- **Sources**: `src/lib/idgen.c:22-55`, `backend/clib.go:338-347`, `controllers/jobs.go:1185,1426`, `src/harness/test.c:3721,6893-6898`.
|
|
- **Findings**:
|
|
- `idgen(ctx, c, a, size, *out)` returns `size` on success.
|
|
- Size 1 returns an untagged ordinary ID. Size > 1 returns a tagged index-0 ID after rounding the sequence up to a 32768 block.
|
|
- Size 32768 is accepted.
|
|
- There is no sequence-overflow guard. The harness seeds the sequence randomly in [0, 2^20).
|
|
- The harness ignores the return code.
|
|
- **Implications**: A one-element array needs a distinct request (decision D2). The overflow guard in general is out of scope (separately tracked). The new array request refuses a block that would leave the 20-bit sequence, because otherwise the 1-based first ID could land in the timestamp.
|
|
|
|
### Controller submission path
|
|
- **Sources**: `src/controller/job.c:108-229, 2224-2356, 2389-2552`, `persist.c:5930-5938`, `src/include/xce/errnox.h:11-26`, `cluster.c:1685`.
|
|
- **Findings**:
|
|
- Order today: field checks → `HOOK_BEFORE_EVENT` → `jobid_range_exists` → `inflight_range_try_add` → array/ordinary build.
|
|
- Release happens on success (`:2321`), on commit failure (`:2356`) and on early errors.
|
|
- Duplicate submission replies `-EEXIST`.
|
|
- Size 0 reaches payload building and is likely never answered.
|
|
- `jobmgr` is a uint64-keyed hash with per-lookup read locks.
|
|
- **Implications**:
|
|
- Validation (7.1-7.5) becomes a pure function called before any registry or table work.
|
|
- The table checks move after registration (Boundary note).
|
|
- The array-identity check is at most 32768 hash lookups (CPU-bound, no blocking), which is acceptable in the vtask handler under the async-I/O invariant.
|
|
- 7.1 needs a new `errnox.h` value.
|
|
|
|
### API server
|
|
- **Sources**: `controllers/jobs.go` (createArrayName :1824-1979, preCheckJobRequest :1565-1578, :1734, CreateJob :1375-1523, parseArrayName :1761-1821, parseArrayIndexSection :3271-3295 and 5 callers, GetJobsInfo :227-437, GetJobByIDs :725-860, ControlJobByID :2261-2635, foundJob :3111-3131, ControlJob :2652-2830, FetchJobList :3299-3480), `utils/config.go:211-227`, `cmd/main.go:92-101`, `backend/handler.go:272-275`, `models/jobs.go:709-713`.
|
|
- **Findings**:
|
|
- `createArrayName` trims whitespace, accepts `+1`, rejects `[k-k]` and parses `%N`.
|
|
- The named-job branch returns early, so the interactive check never runs for named jobs.
|
|
- `parseArrayName` slices to `len-1`, so text after `]` is lost.
|
|
- `parseArrayIndexSection` reads `1-2-3` as `(1,1)`. All its callers replace its error with a generic one.
|
|
- `IsNumeric` is `ParseFloat`.
|
|
- `foundJob` cannot distinguish explicit `[0]` from "no index".
|
|
- The submit response has no count.
|
|
- The config rejects values above 2147483646 and does not clamp at 32767. The logger is initialised before the config, so a startup warning is possible.
|
|
- The in-memory `store.Jobs` and the existing hook-variable pattern (`validateResreqSyntax`, `fetchFile`) make all of this unit-testable without CGO.
|
|
- **Implications**: There is one selection parser and one key validator (D4), and they replace the scattered parsing. `foundJob` gains an explicit-index flag. The response gains a count field.
|
|
|
|
### xcecli
|
|
- **Sources**: `common/format/jobid.go`, `cmd/jobs/{submit,controller,list}.go`, `filters/job_filter.go`, `models/jobs.go:365-376,454-458`.
|
|
- **Findings**:
|
|
- `ParseJobID` discards `ParseUint` errors and checks neither the index nor alignment.
|
|
- `ToAPIJobID` passes anything unrecognised through, and `TestToAPIJobIDPassthrough` locks that in for `"123456[3]"`.
|
|
- Control-by-ID never validates.
|
|
- Both ID lists split at every comma.
|
|
- `FormatArrayChildID(base, 0)` prints the bare array ID.
|
|
- Checkpoint has no filter flags.
|
|
- **Implications**:
|
|
- There is one client-side identity parser that both paths use (D6).
|
|
- Numeric keys (`123456[3]`) still pass through, because only the API server can judge them (5.17, 5.8 for numeric forms). The passthrough test keeps its numeric case.
|
|
- Control results print from the element's own ID.
|
|
|
|
### Harness, worker, scheduler, skills
|
|
- **Sources**: `src/harness/test.c` (jobsub :3615-3866, substitute_autovars :1283-1360, cmd_expect_job :2787-2817, joblist_query :2543-2608, single-job commands :3327/3517/4140/4276/4921/5073/5268/5377/5481), `src/harness/cmdutil.c` + `testsuite/test_cmdutil.c`, `testsuite/test_test_cli.c`, `src/worker/src/xcecd_exdata.c:396-457`, `src/scheduler/src/driver.c:440-512`, `scheduler/testsuite/test_driver_translate.c:720-826`, both skill scripts and SKILL.md files, `src/docs/xce.md`.
|
|
- **Findings**:
|
|
- `arsize` is `uint32_t`, so a negative `-a` wraps to about 4.29e9 rather than becoming a plain job.
|
|
- `taskrun` sends job 0 for a rejected ID.
|
|
- `test.c` is not linked into any unit test. Logic must move into a paired file (as `cmdutil.c` is) to be unit-tested; otherwise it can only be tested over a pty with `test_test_cli.c`.
|
|
- The worker sets `XCE_JOBINDEX=0` and then lets user variables override it.
|
|
- The scheduler expansion has no tag or overflow guard, and the expansion tests use untagged IDs.
|
|
- `xce.md` has no array section.
|
|
|
|
## Architecture Pattern Evaluation
|
|
|
|
| Option | Description | Strengths | Risks / Limitations | Notes |
|
|
|--------|-------------|-----------|---------------------|-------|
|
|
| A: start field | Add an explicit start index to the submission | Supports any start | Wire, WAL and snapshot format change; HA replay compatibility | Rejected in requirements (decision 1) |
|
|
| B: index in ID (selected) | First element ID is `base\|k0`; elements are `first + i` | No format change; existing expansion and apply unchanged | Index range is 1..32767 (one slot lost) | Decided with the user in requirements |
|
|
| Per-caller fixes | Patch each parser where it is used | Smallest diffs | Ten-plus divergent copies of the same grammar; 5.12 precedence impossible to keep consistent | Rejected (synthesis: generalization) |
|
|
| One bounded parser per language (selected) | C `parse_jobsel`, Go apiserver `jobsel` and Go xcecli `jobsel` share one grammar and one check order | One place enforces 5.7/5.8/5.12-5.17; callers stay thin | Three implementations of one grammar (C, two Go modules) | The two Go modules cannot share code today (separate modules); a shared conformance case table keeps them aligned |
|
|
|
|
## Design Decisions
|
|
|
|
### Decision: one selection grammar and check order, implemented once per language
|
|
- **Context**: 5.7, 5.8, 5.12, 5.14, 5.15, 5.16 and 5.17 apply to the CLI, REST and harness alike, with one fixed precedence (5.15, 5.16, 5.14, 5.17, 5.8, 5.7, 5.5).
|
|
- **Alternatives Considered**: 1) fix each caller; 2) one parser per language.
|
|
- **Selected Approach**: C `parse_jobsel()` in libxce (the harness and `parse_jobid` use it); Go `jobsel` in the apiserver (all REST selection paths); Go `jobsel` in xcecli. Each returns a typed result `{key, form, lo, hi}` or the first failed check.
|
|
- **Rationale**: The precedence rule (5.12) can only hold if one function evaluates the checks in order.
|
|
- **Trade-offs**: Three implementations of one grammar, mitigated by a single conformance table (`selection-cases` in design.md's Testing Strategy) copied into each test suite.
|
|
- **Follow-up**: Keep the three case tables identical in review.
|
|
|
|
### Decision: a distinct ID request for one-element arrays
|
|
- **Context**: `idgen(..., 1, ...)` must keep returning an ordinary ID (harness `-a 1`, restart). `name[1]` must create a one-element array (1.2, 7.8).
|
|
- **Selected Approach**: A new `idgen_array(ctx, c, a, n, *first)` for 1 <= n <= 32767 returns the tagged first element `block|1`. `idgen` keeps size 1 only for ordinary IDs; its size > 1 path is removed, because both callers switch to `idgen_array`. The apiserver gets a matching cgo wrapper `GenerateArrayJobID`.
|
|
- **Rationale**: Callers name their intent; size 1 stays unambiguous.
|
|
- **Trade-offs**: A new libxce symbol; the non-cgo stub needs a matching stub.
|
|
|
|
### Decision: controller validation before collision checks, collision checks after registration
|
|
- **Context**: 7.1-7.11 and the Boundary note on the registry window.
|
|
- **Selected Approach**:
|
|
1. Pure `jobsubmit_classify(jobid, size)` → `OK | INDEX0 | INVALID`, evaluated first (7.9: INDEX0 wins over everything; INVALID wins over collisions).
|
|
2. Then `inflight_range_try_add` extended to also compare array identities among array entries.
|
|
3. Then the job-table element-range and array-identity checks; a hit releases the registration.
|
|
- **Rationale**: This closes the window between the table check and registration; the precedence matches 7.9 and 7.10.
|
|
- **Trade-offs**: A rejected collision now costs one registry add and release.
|
|
|
|
### Decision: whole-array identity in replies is derived by clients
|
|
- **Context**: Decision 3 / round 3 in the requirements. The controller reply keeps the first element ID; `clib.go` and `RecordAcceptedJob` rely on it.
|
|
- **Selected Approach**: The apiserver response `jobid` becomes `array_jobid(first)` with a new `array_size` field. The harness prints and stores `array_jobid`. The controller is unchanged.
|
|
|
|
### Decision: the worker skips a user-supplied `XCE_JOBINDEX`
|
|
- **Selected Approach**: In the user-variable loop, a variable named `XCE_JOBINDEX` is skipped with a WARN naming the job. The system value is `array_idx` for tag-1 IDs and 0 otherwise. `XCE_JOBID` behaviour is unchanged (6.5).
|
|
|
|
### Synthesis outcomes
|
|
- **Generalization**:
|
|
- All identity and selection parsing collapses into one grammar per language.
|
|
- The five apiserver `parseArrayIndexSection` callers and the `foundJob` index-0 branch become consumers of one parsed selection type.
|
|
- The harness's ten single-job commands share one argument helper.
|
|
- **Build vs adopt**: Nothing external fits. Go `strconv.ParseUint` with explicit bit-size and C `strtoull` with `errno` replace `atoi` and `ParseFloat`. No new dependencies.
|
|
- **Simplification**:
|
|
- The `idgen` size > 1 path is removed rather than kept beside `idgen_array`.
|
|
- No start field and no format change (option B).
|
|
- `%N` and steps are rejected rather than parsed and dropped.
|
|
- Start > 1 is not modelled anywhere in the clients. The controller accepts starts from 1 to 32767 only because 7.7 requires it.
|
|
|
|
## Discovery corrections to the requirements' Boundary notes (absorbed by the design)
|
|
1. `parse_jobid` dereferences its buffer before the NULL check (`utils.c:625`); the rewrite fixes this.
|
|
2. The harness `-a` value is `uint32_t`, so a negative value wraps to about 4.29e9 rather than turning into a plain job (4.4 covers it).
|
|
3. `taskrun` has no `jobid == 0` guard; the shared harness helper replaces every unchecked `parse_jobid` call.
|
|
4. `parseArrayName` has six callers: the sixth, `output.go:226`, is peek, which is out of scope and left unchanged. `FetchJobList` also replaces `parseArrayName` errors with a generic message.
|
|
5. `parseArrayIndexSection` silently reads `a-b-c` as `(a,a)`; the new parser rejects it (5.16).
|
|
6. `IsNumeric` (`ParseFloat`) accepts `1e3`, `-1`, `NaN`; the new key validator uses `ParseUint`.
|
|
7. The interactive check (`jobs.go:1734`) is unreachable for named jobs because of the early return at :1578. The 2.1 check runs before the name branch. The early return itself (a separately listed defect) is otherwise left as it is.
|
|
8. Scheduler expansion tests (`test_driver_translate.c:720-826`) use untagged IDs. The new tag guard requires them to become tagged `block|1` IDs.
|
|
9. The jobsub help is at `test.c:600-613`; :556-562 is `expect-job`'s.
|
|
10. `list.go`'s `-W` flag is commented out at :345, not :360.
|
|
11. Submissions of size 0 are probably never answered today (empty payload). 7.4 now rejects them up front.
|
|
12. **bug-1898 (found in design review round 1):** the apiserver builds bracketed `job_id`/`job_name` selections as `CompConditions` that `store.ExecuteQuery` never evaluates, so `GetJobsInfo`, `GetJobByIDs` and `FetchJobList` (hence filter-based `ControlJob`) ignore them. The design replaces them with evaluated row predicates (`QueryOptions.Predicates`), which this feature's element-only matching needs anyway.
|
|
13. The apiserver has no control-result struct; results are `map[string]interface{}`, and accepted/failed maps already carry `unique_job_id`. Only not-found maps need it.
|
|
14. Today's `substitute_autovars` also expands `$?` and `$jobcount`; the extracted `autovar_substitute` takes a context struct for them.
|
|
15. libxce and controller tests are registered in `src/lib/meson.build` / `src/controller/meson.build` plus their runners (`lib/testsuite/test.c`, `controller/testsuite/controller_test.c`), not in per-testsuite `meson.build` files.
|
|
|
|
## Design review log
|
|
- **Round 1 (2026-10-01):** two independent reviewers (requirements fidelity; code feasibility) reported 18 issues, all fixed in design.md:
|
|
- Requirements fidelity: most-recent rule scoped to filters without `[...]`; 5.8 only for keys with `[...]`; harness reversed range → 4.8; submit-number saturation for 2.9; client 5.7/5.8 on decimal keys; validate-all-first on REST; no array-name rule on name selections; 5.10 approximation documented; `primaryJobID` stays the first element for `%I`/`%J`.
|
|
- Code feasibility: unevaluated `CompConditions` (bug-1898) → predicates; no apiserver control struct; autovar context; test registration paths; peek keeps a legacy `ToAPIJobID`; one read-locked scan for the collision check; `foundJob` signature and its existing test; a dispatch-log type conversion; the harness pty command form.
|
|
|
|
- **Round 2 (2026-10-01):** 16 issues, all fixed:
|
|
- Requirements fidelity (7): control keeps identity ranges (`ToAPIJobID(s, allowRange)`); range refusal ordered after 5.16/5.14/5.17/5.8/5.7; `CreateJob` log lines name the array (5.9); the `> 2147483646` config rejection removed; element 0 by name has an explicit predicate; the count is printed once; one test bullet relocated; one upgrade order stated in both documents.
|
|
- Code feasibility (9, one already fixed by round-2 edits): 64-bit size in `jobsubmit_classify`; per-field OR / cross-field AND predicates; bracketed filters only with `eq`/`in`; whole-filter conversion; two scheduler tests and `peek_test.go` added to the plan; `cluster_t`; harness header paths; the autovar `-ENOSPC` return.
|
|
|
|
- **Round 3 (2026-10-01):** 12 issues (one already resolved by earlier round-3 edits), all fixed:
|
|
- Requirements fidelity (6): bracketed filters honour their operator instead of a new eq/in-only rejection (`notin` negates, `contains`/`startwith`/`endwith` match the name part, `between` as `in`, `ne` keeps today's rejection); REST queries evaluate identity ranges (only the xcecli list path refuses them); written-form bare keys normalised on every path; 5.9 log lines drop the raw first-element number; name-part grammar and test rows; schedulers in the upgrade order.
|
|
- Code feasibility (6): harness sequence seed bounded below `30 * 32768`; 7.2 and the scheduler guard written without addition (`size > 32768 - idx`); filter-based `ControlJob` validates names itself so rejections are 400; per-entry (not per-field) filter conversion; `resolveMaxArraySize` pure function for config tests.
|
|
|
|
- **Round 4 (2026-10-01):** 10 issues (two reviewers overlapped on the xcecli wildcard case), all fixed: xcecli checks the raw `-J` text (5.16) before wildcard mapping and picks the search rule from the name part of a bracketed value; bracketed `job_name` operators reuse today's condition semantics (`BuildFilterConditions` + `MatchesCondition`); operators outside a field's mapping table keep the existing "unsupported operator" error, and `job_id` comparison operators with brackets get the existing `ne`-class error; identity parsing limited to identity operators and converted entries; server `ParseIdentitySelection` has no range flag; `ControlJob` name pre-validation only for bracketed names; `test_idgen` seed cases pin the timestamp; idgen overflow boundary stated exactly; line references corrected.
|
|
|
|
- **Round 5 (2026-10-01):** 8 issues (one overlap), all fixed: line references; xcecli rejects an all-`*` name part; bracketed `job_name` `null`/`notnull` rejected like the `job_id` comparison operators, with `notcontains` negation scope stated; `GetJobsInfo` validates all filter entries in a pre-pass before `exec_hosts` resolution; the one new operator rejection stated honestly and added to the Error table; Allowed Dependencies split between parsers and predicate building; harness and xcecli rows completed in the Error table; pre-store key checks aligned with Matching for converted `between` entries.
|
|
|
|
- **Round 6 (2026-10-01):** requirements review found 1 issue, fixed: the round-5 client rejection of an all-`*` name part (`-J '*[1]'`) had no authorising criterion; the name part may now be empty (`contains` → every array, `eq` → no job), so no rejection is added.
|
|
Code review found 1 issue, fixed: submit-expression errors pass through `preCheckJobRequest` unwrapped and `CreateJob` maps `*ArrayExprError` to 400 via `errors.As` (it mapped every preCheck error to 500), with a typed-nil guard.
|
|
|
|
- **Round 7 (2026-10-01):** requirements review found 2 issues, fixed: `preCheckJobRequest` returns `(n, isArray, err)` and `CreateJob` branches on `isArray` (so `name[1]` is a one-element array); `createArrayName` is kept for names without brackets so their validation and errors are unchanged (1.4).
|
|
Code review found 3 issues: 2 duplicated the requirements findings (already fixed); 1 new, fixed: unnamed jobs are stored with `job_name ""`, so an empty name part with `eq`/`in`/`notin` now matches no row (otherwise `kill -J '[1]'` would hit every unnamed array).
|
|
|
|
- **Round 8 (2026-10-01):** code review found 3 low issues, fixed: the four `preCheckJobRequest` test call sites listed; error returns written for the new `(n, isArray, err)` shape; interactive passed as `req.Interactive != 0`.
|
|
Requirements review found 2 issues: 1 duplicated the code finding (fixed); 1 new, fixed: 1.4 traced to ApiCreateJob's unchanged `createArrayName` path instead of the parser.
|
|
|
|
- **Round 9 (2026-10-01):** requirements review found 2 issues, fixed: `CreateJob` test seams (`generateJobID`, `generateArrayJobID`, `submitJob` package variables) with the submit tests in a new `!cgo` file next to the JWT/RBAC fixtures; the bracketed-name success path stated (`req.JobName = arrayName`, `(n, true, nil)` at today's early return). Non-Goals wording on the interactive check corrected.
|
|
Code review found 5 issues: 1 duplicated (test seams, fixed); 4 new, fixed: line numbers corrected; the unnamed-job success return (`:1746`) stated; the REPL claim corrected (its `strtoull` fallback keeps behaviour unchanged); `ToAPIJobID` described as a signature change with all call sites listed.
|
|
|
|
- **Round 10 (2026-10-01):** code review found 1 issue, fixed: the submit parser's limit comes from `utils.JobYamlConfig.MaxArraySize`, and the `!cgo` submit tests set and restore `JobYamlConfig` (nil in tests otherwise).
|
|
Requirements review found 1 issue, fixed: a `controlJob` seam at both control dispatch sites; every handler-level submit/query/control test moved into the new `!cgo` `jobs_handler_test.go` (fixtures and dispatch observable); `jobs_test.go` keeps pure-function cases only.
|
|
|
|
- **Round 11 (2026-10-01):** code review found 2 issues, fixed: handler tests seed `store.Queues` and a controllable `job_status` (else control returns 403 before dispatch); `ControlJobByID` re-dispatches earlier selections per later selection -- logged as bug-1899, and the design now dispatches a de-duplicated list once after the selection loop.
|
|
|
|
- **Round 12 (2026-10-01):** requirements review found 1 issue, fixed: the bug-1899 "dispatch once" rule now keeps the per-selection not-found maps in the response (collected separately from the de-duplicated act-on list).
|
|
Code review found 2 issues, fixed: handler tests seed after `setupRBACTest` (which resets the stores) and assert real dispatch; `validateJobControl` runs once on the de-duplicated list after the loop, and a dispatch error stays a 500.
|
|
|
|
- **Round 13 (2026-10-01):** code review found 1 low issue, fixed: `validateJobControl` is per row, so it is called once per de-duplicated job after the loop (signature unchanged).
|
|
|
|
- **Round 14 (2026-10-01):** code review found 1 issue, fixed: `TestToAPIJobIDPassthrough`'s `"123456[3]"` is misaligned under the client's 5.8 check; replaced by `"32768[3]"` plus a rejection case; `jobid_test.go:109` added to the call-site list with `allowRange=false`.
|
|
|
|
- **Round 15 (2026-10-01):** requirements review found 1 issue (introduced by the round-14 edit), fixed: the `ToAPIJobID` call-site list now states `controller.go:52` passes `allowRange=true` and only the list/test sites pass `false`; tests added that catch a wrong flag.
|
|
|
|
- **Round 16 (2026-10-01):** code review: no issues. Requirements review found 2 issues, fixed: every rejection after registration replies before releasing the registry entry (table-hit path and the existing payload/OOM early-outs), so a submission blocks others until answered (7.11, Terms); the xcecli control test placed in a new `cmd/jobs/controller_test.go` with its httptest mechanism.
|
|
|
|
- **Round 17 (2026-10-01):** requirements review found 1 issue, fixed: reply and registry removal are now one atomic step under the registry lock (`inflight_reply_and_remove`, `xce_reply` being non-blocking) on every post-registration path, closing both ordering windows; the System Flows diagram matches.
|
|
Code review found 1 issue (the undefined "reply hook"), fixed together with the atomic step: an `XCE_TESTING` hook inside `inflight_reply_and_remove` plus a test-only overlap accessor; the concurrent check runs on a second thread because the hook holds the registry lock.
|
|
|
|
- **Round 18 (2026-10-01):** code review found 2 issues (no deadlock risk found), fixed: `inflight_reply_and_remove` is the only registry removal -- `jobsubmit_async_ctx_release` no longer calls `inflight_range_remove`, and the async paths call it inside their one-shot reply guard; the atomicity test uses deterministic in-hook checks (trylock EBUSY, reply queued, entry still present) instead of a second thread.
|
|
Requirements review found 1 issue, fixed: classification runs before the existing field checks and hook, so 7.1/7.9 hold even when an index-0 array also has an empty field.
|
|
- **Round 19 (2026-10-01):** code review found 2 low issues (and confirmed no registry entry can leak and no hook consumer acts on the moved before-event), fixed: `inflight_reply_and_remove` takes `(ep, from, reply, base, size)` so the async paths can call it; classification is anchored as the first statement after `init_reply_by_req`.
|
|
|
|
- **Round 20 (2026-10-01):** both reviewers (requirements fidelity; code feasibility) reported NO ISSUES. Review loop closed.
|
|
|
|
## Risks & Mitigations
|
|
- **Rolling upgrade: an old API server or harness sends index-0 arrays to an upgraded controller.** Rejected with the distinct `E_XCE_ARRAY_START0` status and an explanatory log line (7.1). Recommended order for clusters using arrays: API servers and harness first, then controllers (an old controller accepts `array-id | 1` submissions); controllers first is supported with that rejection window accepted (design.md, Migration Strategy).
|
|
- **Three parser implementations drift.** Shared conformance case table, plus a review checklist item in tasks.
|
|
- **Control-result display of pre-upgrade element 0 needs the element's own ID.** Accepted and failed result maps already carry `unique_job_id`; the apiserver adds it to not-found maps. xcecli prints `FormatJobID(unique_job_id)` when it is present and falls back to today's rendering otherwise (an older server).
|
|
- **The array-identity collision check walks up to 32768 hash keys inside a vtask handler.** One read-lock hold for the whole scan, which also answers the element-range check; bounded CPU, no I/O, permitted by `ha-async-io.md`. The cost is paid only by array submissions. An apply-maintained array-ID index was rejected (it would need completeness across apply, replay and snapshot load, audit dimension 7).
|
|
- **`idgen_array` refusing a block past the 20-bit sequence surfaces as a submit error after about 31 arrays in one second from one API server.** That is the same limit as today, except it is now an error instead of a corrupted ID. It is recorded as a known limit. The harness's random sequence seed is bounded below `30 * 32768` so that its first array cannot hit the limit.
|
|
|
|
## References
|
|
- `.kiro/specs/job-array-one-based-index/requirements.md`: decisions 1-10 and the Boundary notes.
|
|
- `.kiro/specs/job-array-single-wal-record/`: the array WAL record and expansion this design relies on unchanged.
|
|
- `.kiro/steering/ha-async-io.md`, `error-handling.md`, `api-standards.md`, `testing.md`, `persistence.md`.
|
|
- LSF 10.1 Administration Guide, "Chunk Jobs and Job Arrays" (p.519): index 0 is invalid; `LSB_JOBINDEX` is 0 only for non-array jobs.
|