diff --git a/.wolf/buglog.json b/.wolf/buglog.json index 0fa07b54..a42368c5 100644 --- a/.wolf/buglog.json +++ b/.wolf/buglog.json @@ -21206,6 +21206,59 @@ "related_bugs": [], "occurrences": 1, "last_seen": "2026-09-07T07:55:00Z" + }, + { + "id": "bug-1781", + "timestamp": "2026-09-07T08:35:00Z", + "gitea_issue": 45, + "error_message": "Gitea issue #45 REOPENED after bug-1742's fix. User-reproduced: after bug-1742 fixed the earlier panic/empty-error-body symptom, `xcecli jobs kill ` (and stop/resume/checkpoint) now fails with a real, non-empty error instead: `request to /xce/v1/jobs//control failed: [failed to write data: xcecli_jobctrl failed: -22]` (-22 = -EINVAL). Reproduced live for kill/stop/resume/checkpoint, all failing.", + "file": "src/frontend/apiserver/backend/clib.go (ControlJob), src/lib/request.c (xce_jobctrl_req), src/lib/controller.capnp.h (JobControl_u_which), src/include/xce/types.h (xce_jobctrl_t), src/controller/job.c (job_ctrl)", + "root_cause": "ControlJob() (clib.go) set xce_jobctrl_t.action to the job_ctrl_type_t action code (models.JOB_STOP=1, JOB_RESUME=2, JOB_KILL=3, JOB_CHKPNT=8, src/include/xce/types.h job_ctrl_type_t) -- but that field is actually the capnp JobControl_u_which UNION DISCRIMINANT (JobControl_u_siginfo=0, JobControl_u_ckptoption=1, src/lib/controller.capnp.h), a completely different enum that says which branch of xce_jobctrl_t.u (siginfo vs ckptoption) is populated. xce_jobctrl_req() (src/lib/request.c:2200) and the controller's job_ctrl() (src/controller/job.c:1972, for a job with no allocs yet) both switch on ctrinfo->action expecting the discriminant values 0/1, not the job_ctrl_type_t codes. Consequences per action: kill(3) and resume(2) never match 0 or 1, so xce_jobctrl_req's `default: return -EINVAL` fires client-side before the request is even sent to the controller -- this is the exact -22 in the bug report. checkpoint(8) hits the same default-EINVAL path, so checkpoint was ALSO silently broken (never reached the controller's real checkpoint-eligibility check). stop(1) is the worst case: it numerically collides with JobControl_u_ckptoption(1), so xce_jobctrl_req took the ckptoption branch and read `ctrinfo->u.ckptoption->method` through a pointer that was actually populated as `xce_signal_t*` (via jobctrl_set_siginfo, clib.go) -- a wrong-typed union-member read, undefined behavior in the apiserver's own address space, not merely a clean rejection. The `signo`-based dispatch already correctly threaded through job_ctrl()'s inner switch (SIGCONT->resume, SIGSTOP/SIGTSTP->stop, SIGKILL/SIGTERM/default->kill on the not-yet-started-job branch) proves the intended design: `action` is the union tag, and `u.siginfo->signo` is what actually distinguishes stop/resume/kill -- ControlJob() conflated the two enums when translating the caller's action into the wire struct.", + "fix": "FIXED and LIVE-VERIFIED (2026-09-07). Added jobCtrlUnionDiscriminant(action int) int to convert_pure.go (pure Go, no CGO types, alongside resolveExecUser from bug-1780) -- returns JobControl_u_ckptoption(1) for JOB_CHKPNT, JobControl_u_siginfo(0) for JOB_STOP/JOB_RESUME/JOB_KILL. ControlJob() (clib.go) now sets `ctrl.action = C.int(jobCtrlUnionDiscriminant(action))` instead of the raw action code; the existing jobctrl_set_siginfo/jobctrl_set_ckptoption C helpers (already correct, only ever touched ctrl.u) are unchanged. TDD: TestJobCtrlUnionDiscriminant_StopResumeKillUseSiginfo and TestJobCtrlUnionDiscriminant_ChkptUsesCkptOption (convert_test.go), passing under CGO_ENABLED=0 go test (RED before the function existed). Verified with a full /xce-build apiserver asan --incremental (exit 0, apiserver/backend real-CGO suite green). LIVE-VERIFIED beyond static analysis (learning from bug-1742's own gap, which was static-analysis-only and left this exact defect in place): deployed a real single-node cluster (controller+scheduler+worker+apiserver) via deploy-ha.sh, submitted a job that stays PEND (unsatisfiable select[] so it never dispatches -- exercises job_ctrl()'s not-yet-started branch, the exact code path this bug hits, without needing job execution), and confirmed via the real built xcecli binary: `jobs kill` succeeds (job removed from listing, previously -22), `jobs stop`/`jobs resume` succeed with no apiserver crash (previously read through the wrong union member -- confirmed apiserver process survived and stayed responsive), and `jobs checkpoint` on an unstarted job now correctly gets a clean business-rule rejection (\"cannot be \") instead of a spurious client-side -EINVAL.", + "tags": [ + "apiserver", + "capnp", + "clib.go", + "job-control", + "kill", + "stop", + "resume", + "checkpoint", + "union-discriminant", + "undefined-behavior", + "gitea-45", + "live-verified", + "fixed" + ], + "related_bugs": [ + "bug-1742" + ], + "occurrences": 1, + "last_seen": "2026-09-07T08:35:00Z" + }, + { + "id": "bug-1782", + "timestamp": "2026-09-07T08:40:00Z", + "gitea_issue": 45, + "error_message": "Not a defect -- API ergonomics follow-up discussed while fixing bug-1781/gitea#45. The job control REST endpoint (`/xce/v1/jobs/{jobids}/control`) only accepted the packed numeric job ID; xcecli already translates the human-readable form (e.g. `1-0-1788768547-0`, as printed by `jobs list`) to that numeric form client-side (format.ToAPIJobID) before calling the API, so a caller hitting the endpoint directly (not through xcecli) had to reimplement that bit-packing itself.", + "file": "src/frontend/apiserver/backend/convert_pure.go (ParseHumanJobID, new), src/frontend/apiserver/controllers/jobs.go (ControlJobByID)", + "root_cause": "N/A -- enhancement, not a bug. Existing numeric-only behavior was correct and intentional.", + "fix": "ADDED (2026-09-07), additive/backward-compatible -- the numeric form is untouched and still works exactly as before. Added ParseHumanJobID(s string) (uint64, bool) to convert_pure.go: parses a plain human-readable job ID (\"{cluster}-{apiserver}-{timestamp}-{sequence}\", no array brackets -- controllers/jobs.go's parseArrayName already strips any \"[idx]\" suffix before this runs) back into the packed numeric ID, reusing the bit-layout constants this file already duplicates from xcecli/common/format/jobid.go for the read path (FormatJobID/ExtractArrayBaseID). Works correctly for both a plain job's own ID and an array child's dash-quad prefix (verified equal to ExtractArrayBaseID(original) in the array case -- the two enums are NOT ambiguous here, unlike bug-1781). ControlJobByID (jobs.go) now falls back to ParseHumanJobID when jobid.KeyID isn't numeric, before erroring; numeric IDs skip this path entirely. TDD: TestParseHumanJobID_NonArrayRoundTripsWithFormatJobID, TestParseHumanJobID_ArrayChildPrefixMatchesBaseID, TestParseHumanJobID_RejectsNumeric, TestParseHumanJobID_RejectsGarbage (convert_test.go), passing under CGO_ENABLED=0 go test. Verified with a full /xce-build apiserver asan --incremental (exit 0). LIVE-VERIFIED: against the same deployed cluster used for bug-1781, called POST /xce/v1/jobs/{jobids}/control directly via curl with the URL-encoded human-readable ID (bypassing xcecli's own translation entirely) and confirmed the job was correctly found and killed (HTTP 202, job removed from listing).", + "tags": [ + "apiserver", + "api-design", + "job-control", + "human-readable-id", + "gitea-45", + "enhancement", + "live-verified", + "fixed" + ], + "related_bugs": [ + "bug-1781" + ], + "occurrences": 1, + "last_seen": "2026-09-07T08:40:00Z" } ] } diff --git a/src/frontend/apiserver/backend/clib.go b/src/frontend/apiserver/backend/clib.go index 3ae3d48f..95c76da4 100644 --- a/src/frontend/apiserver/backend/clib.go +++ b/src/frontend/apiserver/backend/clib.go @@ -664,7 +664,10 @@ func SubmitJob(job *models.JobInfo, arraySize int) error { func ControlJob(jobid uint64, action int, content string) error { var ctrl C.xce_jobctrl_t ctrl.jobid = C.uint64_t(jobid) - ctrl.action = C.int(action) + // bug-1781/gitea#45: xce_jobctrl_t.action holds the JobControl_u_which + // capnp union discriminant, not the job_ctrl_type_t action code -- the + // two enums must not be conflated. + ctrl.action = C.int(jobCtrlUnionDiscriminant(action)) // For signal-based actions set signo; for checkpoint set ckpt options. // xce_jobctrl_t.u is a C union — we use a static C helper to set the diff --git a/src/frontend/apiserver/backend/convert_pure.go b/src/frontend/apiserver/backend/convert_pure.go index 2e265bac..2925fba2 100644 --- a/src/frontend/apiserver/backend/convert_pure.go +++ b/src/frontend/apiserver/backend/convert_pure.go @@ -3,6 +3,8 @@ package backend import ( "apiserver/models" "encoding/json" + "regexp" + "strconv" "time" "fmt" ) @@ -98,12 +100,60 @@ func FormatJobID(uniqueJobID int64) string { clusterID, apiserverID, timestamp, sequence) } +// humanJobIDPattern matches a plain human-readable job ID, "{cluster}- +// {apiserver}-{timestamp}-{sequence}" (no array-index brackets). +var humanJobIDPattern = regexp.MustCompile(`^(\d+)-(\d+)-(\d+)-(\d+)$`) + +// ParseHumanJobID parses a plain human-readable job ID (as produced by +// FormatJobID, brackets already stripped by the caller) back into the +// packed numeric job ID the job_id/unique_job_id DB columns store. Returns +// ok=false if s doesn't match that shape (e.g. it's already numeric). +// +// Deliberately does not itself handle an "[idx]" array-child suffix: +// controllers/jobs.go's parseArrayName already splits that off (for the +// API's own array-index filter syntax) before this ever runs, leaving a +// plain quad whose 4th number is the packed ID's low sequence bits with +// tag=0 -- true for a real non-array job's own ID, and also true for an +// array child's dash-quad prefix (FormatJobID's arrayBase is those same +// bits already shifted into position with tag/idx zeroed, i.e. exactly +// ExtractArrayBaseID(original)). +func ParseHumanJobID(s string) (uint64, bool) { + m := humanJobIDPattern.FindStringSubmatch(s) + if m == nil { + return 0, false + } + clusterID, _ := strconv.ParseUint(m[1], 10, 64) + apiserverID, _ := strconv.ParseUint(m[2], 10, 64) + timestamp, _ := strconv.ParseUint(m[3], 10, 64) + sequence, _ := strconv.ParseUint(m[4], 10, 64) + return (clusterID << clusterIDShift) | + (apiserverID << apiserverIDShift) | + (timestamp << timestampShift) | + sequence, true +} + // WorkerDefaultOutputPath returns the output file path the worker uses by default // for the given job ID and task index, matching uv_proc.c make_job_output_path(). func WorkerDefaultOutputPath(uniqueJobID int64, taskIdx int) string { return fmt.Sprintf("/tmp/job_%s_task%d.out", FormatJobID(uniqueJobID), taskIdx) } +// jobCtrlUnionDiscriminant returns the value xce_jobctrl_t.action must carry +// for the given job_ctrl_type_t action (models.JOB_STOP/RESUME/KILL/CHKPNT): +// the capnp JobControl_u_which union discriminant (JobControl_u_siginfo=0, +// JobControl_u_ckptoption=1 -- src/lib/controller.capnp.h), NOT the action +// code itself. xce_jobctrl_req() (src/lib/request.c) switches on this field +// and rejects anything else with -EINVAL; ControlJob (clib.go) previously +// stored the raw action code there instead, so kill(3)/resume(2) always hit +// that -EINVAL default case (gitea issue #45) while stop(1) happened to +// collide with ckptoption(1) and read through the wrong union member. +func jobCtrlUnionDiscriminant(action int) int { + if action == models.JOB_CHKPNT { + return 1 // JobControl_u_ckptoption + } + return 0 // JobControl_u_siginfo (stop/resume/kill all signal the job) +} + // resolveExecUser returns the effective exec user for a job submission: the // caller-specified ExecUser if present, otherwise the submitting user // (bug-1780: an unset execuser in the startjob request reaches xcecd as diff --git a/src/frontend/apiserver/backend/convert_test.go b/src/frontend/apiserver/backend/convert_test.go index 57a9807a..a751c00b 100644 --- a/src/frontend/apiserver/backend/convert_test.go +++ b/src/frontend/apiserver/backend/convert_test.go @@ -3,6 +3,7 @@ package backend import ( "apiserver/models" "encoding/json" + "fmt" "testing" ) @@ -28,6 +29,76 @@ func TestResolveExecUser_EmptyWhenBothEmpty(t *testing.T) { } } +// gitea#45/bug-1783: xce_jobctrl_t.action must carry the capnp +// JobControl_u_which union discriminant (siginfo=0, ckptoption=1), not the +// job_ctrl_type_t action code -- conflating the two made every kill/resume +// call hit request.c's -EINVAL default case, and every stop call +// (action code 1) collide with ckptoption's discriminant (also 1) and read +// through the wrong union member. +func TestJobCtrlUnionDiscriminant_StopResumeKillUseSiginfo(t *testing.T) { + for _, action := range []int{models.JOB_STOP, models.JOB_RESUME, models.JOB_KILL} { + if got, want := jobCtrlUnionDiscriminant(action), 0; got != want { + t.Errorf("jobCtrlUnionDiscriminant(%d): got %d, want %d (JobControl_u_siginfo)", action, got, want) + } + } +} + +func TestJobCtrlUnionDiscriminant_ChkptUsesCkptOption(t *testing.T) { + if got, want := jobCtrlUnionDiscriminant(models.JOB_CHKPNT), 1; got != want { + t.Errorf("jobCtrlUnionDiscriminant(JOB_CHKPNT): got %d, want %d (JobControl_u_ckptoption)", got, want) + } +} + +// gitea#45 follow-up: the job control endpoint additionally accepts a +// human-readable job ID (as printed by FormatJobID/`jobs list`) alongside +// the numeric one, so a caller doesn't have to reimplement xcecli's ID +// packing just to hit the REST API directly. +func TestParseHumanJobID_NonArrayRoundTripsWithFormatJobID(t *testing.T) { + const packed = uint64(1)<