From c15580a7f33e5b5a1ae835fdff70f88194084b65 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Fri, 2 Oct 2026 09:42:27 +0000 Subject: [PATCH] fix(frontend): peek and restart take [k] and written job IDs - apiserver peek (parseJobIDParam): [k] now resolves to the element; it used to drop [k] and look up the array identity itself (and panicked in parseArrayName on some inputs). A job number still names that job exactly (an element by its own XCE_JOBID); ranges and invalid selections are rejected. - apiserver restart: the decimal-only pre-check (isValidJobId, now unused and removed) rejected the written form c-a-ts-seq[k]; ParseIdentitySelection, already used for the lookup, validates it. - xcecli jobs restart converts its argument with ToAPIJobID like the control commands, so written IDs work and invalid ones fail locally. - docs: the peek/restart caveat in the Job arrays section. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/docs/xce.md | 7 +-- src/frontend/apiserver/controllers/jobs.go | 15 ------ .../controllers/jobs_handler_test.go | 2 + src/frontend/apiserver/controllers/output.go | 37 +++++++------- .../controllers/output_jobid_test.go | 50 +++++++++++++++++++ src/frontend/xcecli/cmd/jobs/restart.go | 8 +++ src/frontend/xcecli/cmd/jobs/restart_test.go | 42 ++++++++++++++++ 7 files changed, 126 insertions(+), 35 deletions(-) create mode 100644 src/frontend/apiserver/controllers/output_jobid_test.go create mode 100644 src/frontend/xcecli/cmd/jobs/restart_test.go diff --git a/src/docs/xce.md b/src/docs/xce.md index 4dbd9a70..79a08dcc 100644 --- a/src/docs/xce.md +++ b/src/docs/xce.md @@ -91,9 +91,10 @@ supports a subset of the LSF array syntax. - In listing and control commands, a bare element job number (for example the value of `XCE_JOBID`) is rejected: name the element as `[k]`. - - `xcecli jobs peek` and `xcecli jobs restart` do not accept - `[k]` yet: peek an element by its own job number - (`XCE_JOBID`); restart takes job IDs as before. + - `xcecli jobs peek` and `xcecli jobs restart` take one job: a job ID, + or one element as `[k]` (not a range). `peek` also accepts + an element's own job number (`XCE_JOBID`). Restarting a bare array + identity is refused: restart one element at a time. - **Element number in the job's environment.** A running element finds its own number in `XCE_JOBINDEX` (1..N). An ordinary job gets `XCE_JOBINDEX=0`. A user-supplied `XCE_JOBINDEX` is overridden, with a diff --git a/src/frontend/apiserver/controllers/jobs.go b/src/frontend/apiserver/controllers/jobs.go index 26db72ed..05e6b06a 100644 --- a/src/frontend/apiserver/controllers/jobs.go +++ b/src/frontend/apiserver/controllers/jobs.go @@ -839,12 +839,6 @@ func (j JobController) RestartJobByID(ctx *gin.Context) { logger.Debug("Receive job restart request:", zap.String("jobId", jobIDStr), zap.String("restarter", restarter)) - if err = isValidJobId(jobIDStr); err != nil { - logger.Error(err.Error()) - ReturnError(ctx, common.INVALID_PARAMS_CODE, common.INVALID_PARAMS_MSG+": "+err.Error(), nil) - return - } - // check post body whether correct var req models.JobRestartRequest if err := utils.BindJSONStrict(ctx, &req); err != nil { @@ -1775,15 +1769,6 @@ func isValidArrayName(arrayName string) error { return nil } -func isValidJobId(jobID string) error { - matched, _ := regexp.MatchString(`^\d+$`, jobID) - matchedIndex, _ := regexp.MatchString(`^\d+\[\d+\]$`, jobID) - if !matched && !matchedIndex { - return fmt.Errorf("jobid '%s' contains invalid characters. Allowed characters are 0-9 or with index value", jobID) - } - return nil -} - func validateAskedHosts(askedHosts string) error { if askedHosts == "" { return nil // Nothing to validate diff --git a/src/frontend/apiserver/controllers/jobs_handler_test.go b/src/frontend/apiserver/controllers/jobs_handler_test.go index fdbd0787..ec62a2fd 100644 --- a/src/frontend/apiserver/controllers/jobs_handler_test.go +++ b/src/frontend/apiserver/controllers/jobs_handler_test.go @@ -1470,6 +1470,8 @@ func TestJobHandler_RestartNamedJob(t *testing.T) { }{ {"ordinary job by decimal id", strconv.FormatUint(ord, 10), "ordinary"}, {"array element by [k]", strconv.FormatUint(arr, 10) + "[2]", "elem-2"}, + {"array element by written [k]", backend.FormatJobID(int64(arr)) + "[2]", "elem-2"}, + {"ordinary job by written id", backend.FormatJobID(int64(ord)), "ordinary"}, } for _, c := range cases { before := len(f.submitted) diff --git a/src/frontend/apiserver/controllers/output.go b/src/frontend/apiserver/controllers/output.go index 312349f7..ab9eedd7 100644 --- a/src/frontend/apiserver/controllers/output.go +++ b/src/frontend/apiserver/controllers/output.go @@ -9,7 +9,6 @@ import ( "apiserver/common" "apiserver/models" "apiserver/store" - "apiserver/utils" logger "apiserver/utils" @@ -220,25 +219,29 @@ func resolveOutputPath(path string, job models.JobInfo) string { return path } -// parseJobIDParam parses a job ID string and returns the corresponding JobInfo. +// parseJobIDParam parses the job peek names and returns the corresponding +// JobInfo: a job number (an ordinary job, or an element by its own number, +// XCE_JOBID) names that job exactly; [k] names element k. A range +// or an invalid selection is an error -- peek shows one job. func parseJobIDParam(jobidStr string) (models.JobInfo, error) { - // Use the same parsing logic as GetJobByIDs - jobArrInfo, err := parseArrayName(jobidStr) - if err != nil { - return models.JobInfo{}, err + raw := strings.TrimSpace(jobidStr) + if n, err := strconv.ParseUint(raw, 10, 64); err == nil { + return models.JobInfo{Unique_JobID: int64(n), ArrayIdx: backend.ExtractArrayIdx(n)}, nil } - if !utils.IsNumeric(jobArrInfo.KeyID) { - return models.JobInfo{}, fmt.Errorf("bad jobid: %s", jobArrInfo.KeyID) + sel, selErr := ParseIdentitySelection(raw) + if selErr != nil { + return models.JobInfo{}, selErr } - - uniqueJobID, err := strconv.ParseInt(jobArrInfo.KeyID, 10, 64) - if err != nil { - return models.JobInfo{}, fmt.Errorf("bad jobid: %s", jobArrInfo.KeyID) + switch sel.Form { + case SelSingle: + return models.JobInfo{ + Unique_JobID: int64(backend.MakeElementID(sel.Key, sel.Lo)), + ArrayIdx: sel.Lo, + }, nil + case SelRange: + return models.JobInfo{}, fmt.Errorf("'%s' names several jobs; peek one element as [k]", raw) + default: + return models.JobInfo{Unique_JobID: int64(sel.Key)}, nil } - - return models.JobInfo{ - Unique_JobID: uniqueJobID, - ArrayIdx: 0, // peek defaults to base job; array idx can be added later - }, nil } diff --git a/src/frontend/apiserver/controllers/output_jobid_test.go b/src/frontend/apiserver/controllers/output_jobid_test.go new file mode 100644 index 00000000..23c017f6 --- /dev/null +++ b/src/frontend/apiserver/controllers/output_jobid_test.go @@ -0,0 +1,50 @@ +package controllers + +import ( + "strconv" + "testing" + + "apiserver/backend" +) + +// Peek names one job: an ordinary job or element by its own job number +// (XCE_JOBID), or an element as [k]; parseJobIDParam used to drop +// the [k] and look up the array identity itself. +func TestParseJobIDParam(t *testing.T) { + arr := handlerTestArrayBase + 50<<15 + elem2 := backend.MakeElementID(arr, 2) + ord := handlerTestOrdinaryBase + 51<<15 + human := backend.FormatJobID(int64(arr)) + + ok := []struct { + in string + want uint64 + }{ + {strconv.FormatUint(ord, 10), ord}, + {strconv.FormatUint(elem2, 10), elem2}, // the element's own number + {strconv.FormatUint(arr, 10) + "[2]", elem2}, + {human + "[2]", elem2}, + {" " + strconv.FormatUint(arr, 10) + "[2] ", elem2}, + } + for _, c := range ok { + got, err := parseJobIDParam(c.in) + if err != nil { + t.Errorf("%q: unexpected error %v", c.in, err) + continue + } + if uint64(got.Unique_JobID) != c.want { + t.Errorf("%q: job %d, want %d", c.in, got.Unique_JobID, c.want) + } + } + + for _, in := range []string{ + strconv.FormatUint(arr, 10) + "[1-3]", // a range is several jobs + strconv.FormatUint(arr, 10) + "[40000]", + "garbage", + "", + } { + if _, err := parseJobIDParam(in); err == nil { + t.Errorf("%q: want an error", in) + } + } +} diff --git a/src/frontend/xcecli/cmd/jobs/restart.go b/src/frontend/xcecli/cmd/jobs/restart.go index 171c299a..a4c246f5 100644 --- a/src/frontend/xcecli/cmd/jobs/restart.go +++ b/src/frontend/xcecli/cmd/jobs/restart.go @@ -3,6 +3,7 @@ package jobs import ( "encoding/json" "fmt" + "xcecli/common/format" "xcecli/common/request" "xcecli/config" "xcecli/models" @@ -35,6 +36,13 @@ func (c *JobRestart) ParseCmdArgs(cmd *cobra.Command, args []string) error { // if no job IDs or filter conditions are specified, default to processing all jobs of the current user return fmt.Errorf("no job ID is specified") } + // the API server takes the decimal key, or [k] for an element; + // restart names one job, so no range + apiID, err := format.ToAPIJobID(c.jobID, false) + if err != nil { + return err + } + c.jobID = apiID return nil } diff --git a/src/frontend/xcecli/cmd/jobs/restart_test.go b/src/frontend/xcecli/cmd/jobs/restart_test.go new file mode 100644 index 00000000..276a126f --- /dev/null +++ b/src/frontend/xcecli/cmd/jobs/restart_test.go @@ -0,0 +1,42 @@ +package jobs + +import ( + "strconv" + "strings" + "testing" + + "xcecli/common/format" +) + +// restart sends the API server the decimal job key (or [k]); a job ID +// in its written form c-a-ts-seq used to be sent unconverted and rejected. +func TestRestartParseCmdArgsConvertsJobID(t *testing.T) { + written := format.FormatJobID(int64(testArrayKey)) + key := strconv.FormatUint(testArrayKey, 10) + cases := []struct{ in, want string }{ + {key, key}, + {written, key}, + {written + "[2]", key + "[2]"}, + {key + "[2]", key + "[2]"}, + } + for _, c := range cases { + r := NewJobRestart(nil) + if err := r.ParseCmdArgs(nil, []string{c.in}); err != nil { + t.Errorf("%q: unexpected error %v", c.in, err) + continue + } + if r.jobID != c.want { + t.Errorf("%q: sends %q, want %q", c.in, r.jobID, c.want) + } + } + + for _, in := range []string{written + "[1-3]", "garbage"} { + r := NewJobRestart(nil) + err := r.ParseCmdArgs(nil, []string{in}) + if err == nil { + t.Errorf("%q: want an error", in) + } else if !strings.Contains(err.Error(), in) { + t.Errorf("%q: error %q does not name the selection", in, err) + } + } +}