diff --git a/docs/architecture.md b/docs/architecture.md index 07ce2375..05bda59e 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -129,19 +129,64 @@ client REST handler (internal/api/jobs.go) │ ├─ Parse body → raw template bytes + detected content-type - ├─ openjd.Parse(template) → structured JobTemplate - ├─ openjd.Validate(template) → []ValidationError (reject if non-empty) - ├─ openjd.ExpandParameterSpace(...) → []TaskParams (one per parameter combination) │ - ├─ store.CreateJob(template, steps, tasks) - │ Writes in a single transaction: - │ jobs row (status=pending, template verbatim) - │ steps rows (one per step) - │ tasks rows (one per expanded task, status=pending or ready) + ├─ openjd.Submitter.Submit(...) + │ openjd.Parse(template) → structured JobTemplate + │ openjd.Validate(template) → []ValidationError (reject if non-empty) + │ openjd.ExpandParameterSpace(...) → []TaskParams (one per parameter combination) + │ Expansion runs to completion in memory first: a template that cannot + │ expand never reaches the store. + │ + │ store.CreateJobSubmission(job, dependsOn, steps, tasks) + │ Writes in a single transaction: + │ jobs row (status=pending or blocked, template verbatim) + │ job_dependencies rows (one per cross-job dependency edge) + │ steps rows (one per step) + │ tasks rows (one per expanded task, status=pending or ready) │ └─ HTTP 201 Created { id, name, status, step_count, task_count } ``` +That single write is **load-bearing, not incidental**. Submission used to write +those rows through separate store calls, which left two defects: a failure +partway through stranded a `pending` job that nothing reaps, and a *store* +failure on a later step left the earlier steps persisted while that step's row +was lost entirely — producing a job that `checkJobCompletion`, which derives job +status from the steps that *exist*, would later mark `completed` having silently +lost work. (An *expansion* failure produced only the first: the step row was +written before its tasks were expanded, so the job kept all its steps and simply +hung `pending`.) Both are properties of partial creation, so splitting the write +back up reintroduces both. + +**It also stalls every other database user for its full duration, and a large +submission can fail the readiness probe.** The SQLite pool is +`SetMaxOpenConns(1)` (`internal/store/sqlite/store.go`), so one submission holds +the only connection from `BeginTx` to `Commit`. Lease replies, sweeps and REST +reads no longer interleave between per-row inserts; they queue in Go's +`database/sql` pool, which is not `SQLITE_BUSY` and which `busy_timeout` does not +affect — nothing surfaces it as a lock error, it simply stalls. `GET /readyz` +queues with them: its `sqlite` checker is `Store.Ping`, and `internal/health` +gives all checkers a **5 s** budget per request, so a submission holding the +connection longer than that returns HTTP 503 `degraded` — endpoint removal under +an orchestrator. Measured on an M-series Mac (single step, one +`CreateJobSubmission` call, `/readyz` issued 50 ms in): + +| tasks | transaction | `/readyz` | +|---|---|---| +| 1,000 | 57 ms | ok | +| 10,000 | 683 ms | ok | +| 25,000 | 1.73 s | ok | +| 50,000 | 3.53 s | ok | +| 75,000 | 5.40 s | **503** (`context deadline exceeded`) | + +So an ordinary large render — ~65k tasks is 6.5% of one step's legal maximum — +reads as an outage to an orchestrator. `GET /healthz` (liveness) registers no +checkers and is unaffected, so this does not become a restart loop. The stall is +not *new* cost for the inserts themselves — batching them is faster than the +per-row path (measured 7.4 s versus 8.8 s for 100,000 tasks) — but the window +during which everything else waits is now one contiguous transaction instead of +N gaps. + **Cross-job dependencies (`depends_on`).** A submission — raw `POST /api/v1/jobs` or `POST /api/v1/products/{name}/jobs`, from the REST API, the web UI, or the Python SDK (`submit_job`/`submit_and_wait`/`submit_product_job`) — may include @@ -180,9 +225,12 @@ reached `succeeded`. A `blocked` job's steps and tasks skip this evaluation at submit time and are all held `pending` regardless of step dependencies, until the job is released and this same evaluation runs (see above). -This evaluation runs inside the `CreateJob` transaction for the initial set, -and again via the scheduler's `handleTaskTerminal` → `propagateStepDependencies` -path whenever a task reaches a terminal state. +For the initial set this evaluation happens **before** the write, not inside it: +`buildStepWithTasks` decides each step's and task's starting status while +expanding the template in memory, and the `CreateJobSubmission` transaction only +persists the statuses it already chose. It runs again via the scheduler's +`handleTaskTerminal` → `propagateStepDependencies` path whenever a task reaches a +terminal state. ### 3. Assignment (lease-on-request) diff --git a/internal/openjd/submit.go b/internal/openjd/submit.go index 0373c537..50900e48 100644 --- a/internal/openjd/submit.go +++ b/internal/openjd/submit.go @@ -125,9 +125,16 @@ type SubmitResult struct { // with dependencies start in [store.StepStatusPending]. Tasks inherit their // step's initial status. // -// Submit does not run in a database transaction. If it fails partway through, -// orphaned rows may remain; the REST layer or a cleanup sweep should handle -// such cases by checking job.Status == pending with no tasks. +// Everything one submission creates — the job row, its cross-job dependency +// edges, every step and every task — is written by a single +// [store.JobStore.CreateJobSubmission] call, which is atomic on both store +// backends. A failure at any point therefore leaves nothing behind: there is no +// orphaned pending job for a sweep to reap, and no job missing the steps that +// failed to write (which checkJobCompletion, deriving job status from the steps +// that exist, would have reported completed). +// +// Expansion runs to completion before that write, so a template that cannot +// expand never reaches the store at all. func (s *Submitter) Submit( ctx context.Context, rawTemplate string, @@ -173,15 +180,18 @@ func (s *Submitter) Submit( priority = 50 } - // ── 4. Create Job row (verbatim template stored as-is) ──────────────── + // ── 4. Build the Job row (verbatim template stored as-is) ───────────── now := time.Now().UTC() jobName := tmpl.Name if opts.Name != "" { jobName = opts.Name } - // The job row is always created pending, even when it will ultimately be - // blocked on cross-job dependencies. See the comment below (after steps and - // tasks are created) for why blocked is the LAST status transition. + // A job with an unsatisfied cross-job dependency is created blocked, in the + // same write as the dependency edges that justify it — see step 6. + jobStatus := store.JobStatusPending + if blocked { + jobStatus = store.JobStatusBlocked + } job := store.Job{ ID: uuid.NewString(), FarmID: opts.FarmID, @@ -190,7 +200,7 @@ func (s *Submitter) Submit( Owner: opts.Owner, Submitter: opts.Submitter, Priority: priority, - Status: store.JobStatusPending, + Status: jobStatus, Project: opts.Project, RawTemplate: rawTemplate, TemplateFormat: format, @@ -204,74 +214,74 @@ func (s *Submitter) Submit( UpdatedAt: now, } - job, err = s.st.CreateJob(ctx, job) - if err != nil { - return nil, fmt.Errorf("openjd: submit: create job: %w", err) - } - - if len(opts.DependsOn) > 0 { - if err := s.st.CreateJobDependencies(ctx, job.ID, opts.DependsOn); err != nil { - return nil, fmt.Errorf("openjd: submit: create job dependencies: %w", err) - } - } - - result := &SubmitResult{Job: job, BoundParameters: boundParams} - - // ── 5. Create Step and Task rows ────────────────────────────────────── + // ── 5. Expand every step and task into memory ───────────────────────── + // Nothing is written yet. Expansion runs to completion first so that a + // template which cannot expand never reaches the store at all, and so that + // everything this submission creates can be handed to a single write. // Each step is handled by a helper to keep Submit's cyclomatic complexity // within bounds. deriveBounds := tmpl.hasExtension("SQI_CHUNK_BOUNDS") + steps := make([]store.Step, 0, len(tmpl.Steps)) + var tasks []store.Task for i, stepTmpl := range tmpl.Steps { - steps, tasks, err := s.createStepWithTasks(ctx, job, stepTmpl, i, boundParams, deriveBounds, blocked, now) + step, stepTasks, err := s.buildStepWithTasks(job, stepTmpl, i, boundParams, deriveBounds, blocked, now) if err != nil { return nil, err } - result.Steps = append(result.Steps, steps...) - result.Tasks = append(result.Tasks, tasks...) - } - - // ── 6. Flip to blocked LAST ──────────────────────────────────────────── - if err := s.finalizeBlockedStatus(ctx, job, blocked, result); err != nil { - return nil, err + steps = append(steps, step) + tasks = append(tasks, stepTasks...) + } + + // ── 6. Persist the whole submission in one atomic write ─────────────── + // + // The blocked status travels with the dependency edges, deliberately. + // + // It used to be written last, by a separate UpdateJobStatus after + // everything else was durable, because Submit was not transactional and the + // heartbeat sweep (scheduler.sweepBlockedJobs) scans for status=blocked jobs + // and releases any whose edges are all satisfied. Creating the job + // already-blocked let a sweep tick land in the window after the job row + // existed but before its edges were written, see a blocked job with ZERO + // edges, read that as "nothing left to wait on", and release it to pending. + // Submit would then write the edges and pending tasks anyway, leaving a job + // that is neither blocked nor scheduled — the sweep never revisits a + // non-blocked job, so it hung forever. + // + // That window cannot exist now: the job row and its edges commit together, + // so no sweep can observe one without the other, and the status write that + // used to close the window is gone. It had a failure mode of its own — + // succeeding here and then failing left the job stranded in pending with + // pending tasks, which reconcileBlockedJob skips (it early-returns unless + // the status is blocked) and the scheduler never leases. + // + // Splitting this back into separate writes recreates one hang or the other. + out, err := s.st.CreateJobSubmission(ctx, store.JobSubmission{ + Job: job, + DependsOn: opts.DependsOn, + Steps: steps, + Tasks: tasks, + }) + if err != nil { + return nil, fmt.Errorf("openjd: submit: create job: %w", err) } - return result, nil + return &SubmitResult{ + Job: out.Job, + Steps: out.Steps, + Tasks: out.Tasks, + BoundParameters: boundParams, + }, nil } -// finalizeBlockedStatus marks job blocked, but only after everything it -// depends on (edges, steps, tasks) is already durable — i.e. called as the -// LAST write in [Submitter.Submit]. It is extracted from Submit to keep that -// function's cyclomatic complexity within bounds. +// buildStepWithTasks builds one [store.Step] value and all of its [store.Task] +// values for a single step template. It performs NO store writes: everything +// one submission creates is written by a single +// [store.JobStore.CreateJobSubmission] call in [Submitter.Submit], so a failure +// anywhere — including in this function's expansion — leaves nothing behind. // -// This ordering closes a permanent-hang race: Submit is not transactional, -// and the heartbeat sweep (sweepBlockedJobs) scans for status=blocked jobs -// and releases any whose dependency edges are all satisfied. If the job were -// created already-blocked (as it used to be, up front), a sweep tick landing -// in the window after the job row exists but before CreateJobDependencies ran -// would see a blocked job with ZERO edges, read that as "nothing left to wait -// on", and release it to pending. Submit would then go on to write the edges -// and pending tasks anyway, leaving a job that is neither blocked nor -// scheduled to run — the sweep never revisits a non-blocked job, so it hangs -// forever. Flipping status to blocked only after everything it needs is -// durable means a sweep racing in that window instead sees a plain pending -// job and skips it. -func (s *Submitter) finalizeBlockedStatus(ctx context.Context, job store.Job, blocked bool, result *SubmitResult) error { - if !blocked { - return nil - } - if err := s.st.UpdateJobStatus(ctx, job.ID, store.JobStatusBlocked); err != nil { - return fmt.Errorf("openjd: submit: mark job blocked: %w", err) - } - job.Status = store.JobStatusBlocked - result.Job.Status = store.JobStatusBlocked - return nil -} - -// createStepWithTasks creates one [store.Step] row and all of its [store.Task] -// rows for a single step template. It is extracted from [Submit] to reduce -// that function's cyclomatic complexity. -func (s *Submitter) createStepWithTasks( - ctx context.Context, +// It is extracted from [Submitter.Submit] to reduce that function's cyclomatic +// complexity. +func (s *Submitter) buildStepWithTasks( job store.Job, stepTmpl StepTemplate, stepIdx int, @@ -279,7 +289,7 @@ func (s *Submitter) createStepWithTasks( deriveBounds bool, holdPending bool, now time.Time, -) (steps []store.Step, tasks []store.Task, err error) { +) (step store.Step, tasks []store.Task, err error) { // Collect dependency names from the template. dependsOn := make([]string, 0, len(stepTmpl.Dependencies)) for _, dep := range stepTmpl.Dependencies { @@ -295,7 +305,7 @@ func (s *Submitter) createStepWithTasks( hostReqs, computeLoc := toStoreHostRequirements(stepTmpl.HostRequirements) - step := store.Step{ + step = store.Step{ ID: uuid.NewString(), JobID: job.ID, Name: stepTmpl.Name, @@ -308,11 +318,6 @@ func (s *Submitter) createStepWithTasks( UpdatedAt: now, } - step, err = s.st.CreateStep(ctx, step) - if err != nil { - return nil, nil, fmt.Errorf("openjd: submit: create step %q: %w", stepTmpl.Name, err) - } - // Task status mirrors the step's initial status. taskStatus := store.TaskStatusReady if stepStatus == store.StepStatusPending { @@ -322,17 +327,18 @@ func (s *Submitter) createStepWithTasks( // ── Expand parameter space ────────────────────────────────────────────── taskParamList, err := s.expandStepTaskParams(stepTmpl, stepIdx, boundParams, deriveBounds) if err != nil { - return nil, nil, err + return store.Step{}, nil, err } - // ── Create one Task row per parameter combination ─────────────────────── + // ── Build one Task row per parameter combination ──────────────────────── var reqCores *int if hostReqs != nil { reqCores = requiredCoresFromAmounts(hostReqs.Amounts) } + tasks = make([]store.Task, 0, len(taskParamList)) for j, params := range taskParamList { - task := store.Task{ + tasks = append(tasks, store.Task{ ID: uuid.NewString(), JobID: job.ID, StepID: step.ID, @@ -342,26 +348,16 @@ func (s *Submitter) createStepWithTasks( RequiredCores: reqCores, CreatedAt: now, UpdatedAt: now, - } - - task, err = s.st.CreateTask(ctx, task) - if err != nil { - return nil, nil, fmt.Errorf( - "openjd: submit: create task %d of step %q: %w", - j, stepTmpl.Name, err, - ) - } - - tasks = append(tasks, task) + }) } - return []store.Step{step}, tasks, nil + return step, tasks, nil } // expandStepTaskParams resolves {{Param.*}} / {{RawParam.*}} references in the // step's parameter space, re-validates the resolved space's quantitative // limits, expands it into one parameter set per task, and derives chunk -// bounds when requested. It is extracted from [Submitter.createStepWithTasks] +// bounds when requested. It is extracted from [Submitter.buildStepWithTasks] // to keep that function's cyclomatic complexity within bounds. func (s *Submitter) expandStepTaskParams( stepTmpl StepTemplate, diff --git a/internal/openjd/submit_atomic_test.go b/internal/openjd/submit_atomic_test.go new file mode 100644 index 00000000..e396bf99 --- /dev/null +++ b/internal/openjd/submit_atomic_test.go @@ -0,0 +1,551 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package openjd_test + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/uberware/sqi/internal/openjd" + "github.com/uberware/sqi/internal/store" + "github.com/uberware/sqi/internal/store/fake" +) + +// ── helpers ─────────────────────────────────────────────────────────────────── + +// submitSpy wraps the fake store and records six of its methods: the five row +// creators a submission could use — CreateJob, CreateJobDependencies, +// CreateStep, CreateTask, CreateJobSubmission — and UpdateJobStatus. It +// delegates each one, so the store still behaves exactly like the fake; the +// counters only observe. +// +// It is NOT a general write recorder. It embeds [fake.Store], so every other +// method reaches the fake untouched and is invisible to `writes`: a regression +// that persisted something through some other store method would leave the +// counters unchanged and TestSubmit_PersistsInASingleCall green. The six are +// the ones the pre-atomic Submit used, plus the one that replaced them, which +// is what the counters exist to detect a return to. +// +// It exists because a failed submission must leave no rows AND, once expansion +// runs to completion first, must not have attempted a write at all. The former +// is observable from the store; the latter is not, because a perfectly +// rolled-back write is indistinguishable from no write by inspection alone. +type submitSpy struct { + *fake.Store + + // jobIDs collects the ID of every job whose creation was attempted, by + // either the per-row or the bulk path. Steps have no store-wide listing, so + // this is the only handle on which job's steps to look for. + jobIDs []string + // writes counts every attempted row-creating call of any kind. + writes int + // submissions counts CreateJobSubmission calls specifically. + submissions int + // lastSubmission is the argument of the most recent CreateJobSubmission + // call. It is how a test inspects what Submit asked the store to write, as + // opposed to what the store then made of it. + lastSubmission store.JobSubmission + // statusUpdates counts UpdateJobStatus calls. A submission must make none: + // every status a submission decides is part of the one atomic write. + statusUpdates int + // failStatusUpdate, when non-nil, is returned by UpdateJobStatus instead of + // delegating. It injects the failure of the write that used to run after + // the atomic one. + failStatusUpdate error + // failSubmission, when non-nil, is returned by CreateJobSubmission instead + // of delegating. It injects a store failure during the submission write -- + // a client disconnect, a full disk, a transient DB error -- which is the + // only way to fail a submission that has already passed expansion. + failSubmission error +} + +func (s *submitSpy) CreateJob(ctx context.Context, job store.Job) (store.Job, error) { + s.jobIDs = append(s.jobIDs, job.ID) + s.writes++ + return s.Store.CreateJob(ctx, job) +} + +func (s *submitSpy) CreateJobDependencies(ctx context.Context, jobID string, dependsOn []string) error { + s.writes++ + return s.Store.CreateJobDependencies(ctx, jobID, dependsOn) +} + +func (s *submitSpy) CreateStep(ctx context.Context, step store.Step) (store.Step, error) { + // Record the job ID here too, not just in CreateJob/CreateJobSubmission. + // + // Without this, a regression that writes steps per-step and never reaches + // the bulk call leaves jobIDs EMPTY, so the surviving-step-rows loop in + // TestSubmit_FailedSubmissionLeavesNoRows never executes and the test + // passes while a step row genuinely survives -- defect 2 exactly. That was + // demonstrated by sabotage during review, not theorized. + s.jobIDs = append(s.jobIDs, step.JobID) + s.writes++ + return s.Store.CreateStep(ctx, step) +} + +func (s *submitSpy) CreateTask(ctx context.Context, task store.Task) (store.Task, error) { + s.jobIDs = append(s.jobIDs, task.JobID) + s.writes++ + return s.Store.CreateTask(ctx, task) +} + +func (s *submitSpy) CreateJobSubmission(ctx context.Context, sub store.JobSubmission) (store.JobSubmission, error) { + s.jobIDs = append(s.jobIDs, sub.Job.ID) + s.writes++ + s.submissions++ + s.lastSubmission = sub + if s.failSubmission != nil { + return store.JobSubmission{}, s.failSubmission + } + return s.Store.CreateJobSubmission(ctx, sub) +} + +func (s *submitSpy) UpdateJobStatus(ctx context.Context, id string, status store.JobStatus) error { + s.statusUpdates++ + if s.failStatusUpdate != nil { + return s.failStatusUpdate + } + return s.Store.UpdateJobStatus(ctx, id, status) +} + +// twoStepsSecondOverTaskCap returns a two-step template whose SECOND step +// cannot expand. +// +// The over-cap step must fail at EXPANSION, not at validation, or the test +// proves nothing: validation already precedes every write today, so a template +// rejected there never reaches the store either way. Two INT parameters of +// 1024 values each are individually legal — maxTaskParamValues is 1024 and the +// check is "greater than" — but their Cartesian product is 1,048,576, over +// expand.go's always-on maxTasksPerStep of 1,000,000. countCombNode multiplies +// rather than materializing, so this is fast, and it is the exact case that +// constant's own doc comment cites as its rationale. +func twoStepsSecondOverTaskCap(name string) string { + return `{ + "specificationVersion": "jobtemplate-2023-09", + "name": "` + name + `", + "steps": [ + { + "name": "Step1", + "script": { "actions": { "onRun": { "command": "echo", "args": ["hello"] } } } + }, + { + "name": "Step2", + "script": { "actions": { "onRun": { "command": "echo", "args": ["world"] } } }, + "parameterSpace": { + "taskParameterDefinitions": [ + { "name": "Frame", "type": "INT", "range": "1-1024" }, + { "name": "Layer", "type": "INT", "range": "1-1024" } + ] + } + } + ] +}` +} + +// twoStepsBothValid returns a two-step template that expands cleanly, so a +// submission of it reaches the store. It is the fixture for store-failure +// tests, where the point is what the write leaves behind rather than whether +// the template is acceptable. +func twoStepsBothValid(name string) string { + return `{ + "specificationVersion": "jobtemplate-2023-09", + "name": "` + name + `", + "steps": [ + { + "name": "Step1", + "script": { "actions": { "onRun": { "command": "echo", "args": ["hello"] } } } + }, + { + "name": "Step2", + "script": { "actions": { "onRun": { "command": "echo", "args": ["world"] } } } + } + ] +}` +} + +// ── tests ───────────────────────────────────────────────────────────────────── + +// TestSubmit_FailedSubmissionLeavesNoRows drives the failure mode of DEFECT 1, +// orphaned pending jobs: a submission that failed partway used to leave a job +// row that no sweep reaps — retention deletes only terminal statuses, +// demoteStalledJobs needs a running job with live tasks, and the handler never +// learns the job ID because Submit returns nil on error. +// +// Be precise about what this fixture demonstrates and what it does not. +// Measured against main, an expansion failure on step 2 left BOTH step rows, +// not one: createStepWithTasks wrote each step row BEFORE expanding its tasks, +// so the job persisted with all its steps and Step2 simply had no tasks — an +// orphan hung in pending, which is defect 1. It is NOT defect 2's mechanism; +// reaching a job that checkJobCompletion mis-reports as completed needs a step +// row to be missing entirely, which needs a STORE failure. That case is +// TestSubmit_StoreFailureLeavesNoRows below. +// +// Both defects are properties of partial creation, and the assertion here is +// the general one that closes them: a failed submission leaves ZERO rows. A +// step-count guard would only prove the guard works. +func TestSubmit_FailedSubmissionLeavesNoRows(t *testing.T) { + inner := fake.New() + farmID, queueID := seedSubmitPrereqs(t, inner) + st := &submitSpy{Store: inner} + sub := openjd.NewSubmitter(st) + + _, err := sub.Submit(t.Context(), twoStepsSecondOverTaskCap("PartialJob"), store.TemplateFormatJSON, openjd.SubmitOptions{ + FarmID: farmID, + QueueID: queueID, + Owner: "alice", + }) + if err == nil { + t.Fatal("Submit accepted a template whose second step cannot expand") + } + // Guard the fixture itself: the failure must come from the task-count cap + // applied during expansion. If it ever starts failing in validation the + // test still errors, but it stops saying anything about partial writes. + if !strings.Contains(err.Error(), "too many tasks") { + t.Fatalf("expected the step to fail at expansion (maxTasksPerStep), got: %v", err) + } + + jobs, err := st.ListJobs(t.Context(), store.ListJobsOptions{}) + if err != nil { + t.Fatalf("ListJobs: %v", err) + } + if len(jobs.Items) != 0 { + t.Errorf("%d job rows survived a failed submission, want 0", len(jobs.Items)) + } + + tasks, err := st.ListTasks(t.Context(), store.ListTasksOptions{}) + if err != nil { + t.Fatalf("ListTasks: %v", err) + } + if len(tasks.Items) != 0 { + t.Errorf("%d task rows survived a failed submission, want 0", len(tasks.Items)) + } + + // Steps have no store-wide listing, so they are checked against every job + // ID whose creation was attempted. When nothing was attempted there is + // nothing to look up — which is itself the property under test, and is + // asserted directly by TestSubmit_ExpansionFailureNeverTouchesTheStore. + for _, jobID := range st.jobIDs { + steps, err := st.ListSteps(t.Context(), jobID) + if err != nil { + t.Fatalf("ListSteps(%s): %v", jobID, err) + } + if len(steps) != 0 { + t.Errorf("%d step rows survived a failed submission for job %s, want 0", len(steps), jobID) + } + } +} + +// TestSubmit_StoreFailureLeavesNoRows drives the failure DEFECT 2 actually +// needs: the store failing during the write, as a client disconnect, a full +// disk or a transient DB error would. An expansion failure cannot produce it +// (see TestSubmit_FailedSubmissionLeavesNoRows), which is why this exists as a +// separate case rather than as prose attached to that one. +// +// Reproduced on main by failing the second CreateStep: +// +// SUBMIT err=openjd: submit: create step "Step2": injected store failure +// template declares 2 steps; 1 persisted: "Step1" status="ready" +// DEFECT 2: job that silently lost Step2 has final status = "completed" +// +// checkJobCompletion derives job status from the steps that EXIST, so a job +// missing Step2 entirely was reported completed having silently lost work. +// +// What this pins on HEAD is the property that makes that unconstructible: a +// store failure surfaces to the caller and leaves no job, step or task row, so +// there is no truncated job for checkJobCompletion to see. The rollback itself +// is proven at the store layer on both backends by +// TestJobStore_CreateJobSubmission_RollsBackEntirely; what is proven here is +// that Submit routes the entire submission through that one guarded call, so a +// store failure has nothing partial to leave behind. +// +// The injection is on CreateJobSubmission, which also keeps the test honest +// against a regression: a Submit that went back to per-row writes would never +// call it, would therefore SUCCEED, and would trip the fatal below rather than +// passing vacuously. +func TestSubmit_StoreFailureLeavesNoRows(t *testing.T) { + inner := fake.New() + farmID, queueID := seedSubmitPrereqs(t, inner) + st := &submitSpy{Store: inner, failSubmission: errors.New("injected store failure")} + sub := openjd.NewSubmitter(st) + + // A well-formed two-step template: it must reach the store, unlike the + // over-cap fixture, or the store failure never gets a chance to fire. + _, err := sub.Submit(t.Context(), twoStepsBothValid("StoreFailureJob"), store.TemplateFormatJSON, openjd.SubmitOptions{ + FarmID: farmID, + QueueID: queueID, + Owner: "alice", + }) + if err == nil { + t.Fatal("Submit reported success although the store failed the write") + } + if !strings.Contains(err.Error(), "injected store failure") { + t.Fatalf("the store's error did not reach the caller: %v", err) + } + if st.submissions != 1 { + t.Fatalf("CreateJobSubmission called %d times, want 1", st.submissions) + } + + jobs, err := st.ListJobs(t.Context(), store.ListJobsOptions{}) + if err != nil { + t.Fatalf("ListJobs: %v", err) + } + if len(jobs.Items) != 0 { + t.Errorf("%d job rows survived a failed store write, want 0", len(jobs.Items)) + } + + tasks, err := st.ListTasks(t.Context(), store.ListTasksOptions{}) + if err != nil { + t.Fatalf("ListTasks: %v", err) + } + if len(tasks.Items) != 0 { + t.Errorf("%d task rows survived a failed store write, want 0", len(tasks.Items)) + } + + // Steps have no store-wide listing, so they are checked against every job + // ID whose creation was attempted -- which here is a real ID, because the + // submission did reach the store. + for _, jobID := range st.jobIDs { + steps, serr := st.ListSteps(t.Context(), jobID) + if serr != nil { + t.Fatalf("ListSteps(%s): %v", jobID, serr) + } + if len(steps) != 0 { + t.Errorf("%d step rows survived a failed store write for job %s, want 0", len(steps), jobID) + } + } + // The step rows the failed write would have created must have been part of + // that one call, not written ahead of it: a job persisted with fewer steps + // than its template declares is precisely defect 2. + if got := len(st.lastSubmission.Steps); got != 2 { + t.Errorf("the submission handed to the store carried %d steps, want both", got) + } +} + +// TestSubmit_ExpansionFailureNeverTouchesTheStore pins the ordering property +// that makes the above hold for free: expansion now completes entirely before +// the single write, so the common bad-template case never reaches the store. +// +// Without this, a future change could restore per-step writes and still pass +// the test above by getting the rollback right — while reintroducing the long +// window this ordering removes. +func TestSubmit_ExpansionFailureNeverTouchesTheStore(t *testing.T) { + inner := fake.New() + farmID, queueID := seedSubmitPrereqs(t, inner) + st := &submitSpy{Store: inner} + sub := openjd.NewSubmitter(st) + + if _, err := sub.Submit(t.Context(), twoStepsSecondOverTaskCap("NoWriteJob"), store.TemplateFormatJSON, openjd.SubmitOptions{ + FarmID: farmID, + QueueID: queueID, + }); err == nil { + t.Fatal("Submit accepted a template whose second step cannot expand") + } + + if st.writes != 0 { + t.Errorf("a failed expansion attempted %d row-creating store calls, want 0", st.writes) + } +} + +// TestSubmit_PersistsInASingleCall pins that a successful submission reaches +// the store exactly once, through the atomic creator. A submission spread over +// several calls is what made a partial write possible at all, so "one call" is +// the property, not an implementation detail. +func TestSubmit_PersistsInASingleCall(t *testing.T) { + inner := fake.New() + farmID, queueID := seedSubmitPrereqs(t, inner) + st := &submitSpy{Store: inner} + sub := openjd.NewSubmitter(st) + + result, err := sub.Submit(t.Context(), minimalJSON("SingleWriteJob"), store.TemplateFormatJSON, openjd.SubmitOptions{ + FarmID: farmID, + QueueID: queueID, + }) + if err != nil { + t.Fatalf("Submit: %v", err) + } + + if st.submissions != 1 { + t.Errorf("CreateJobSubmission called %d times, want 1", st.submissions) + } + if st.writes != 1 { + t.Errorf("%d row-creating store calls, want exactly 1 (the atomic submission)", st.writes) + } + + // The result must still carry the stored rows, unchanged in shape. + if len(result.Steps) != 1 || len(result.Tasks) != 1 { + t.Fatalf("result has %d steps and %d tasks, want 1 and 1", len(result.Steps), len(result.Tasks)) + } + if result.Steps[0].JobID != result.Job.ID { + t.Errorf("step.JobID = %q, want the job's ID %q", result.Steps[0].JobID, result.Job.ID) + } + if result.Tasks[0].StepID != result.Steps[0].ID { + t.Errorf("task.StepID = %q, want the step's ID %q", result.Tasks[0].StepID, result.Steps[0].ID) + } +} + +// ── blocked submissions ─────────────────────────────────────────────────────── + +// newBlockedSubmitFixture returns a spy-wrapped fake store, a submitter over it, +// the farm and queue to submit into, and the ID of an upstream job left pending +// so that anything depending on it is created blocked. +func newBlockedSubmitFixture(t *testing.T) (st *submitSpy, sub *openjd.Submitter, farmID, queueID, upstreamID string) { + t.Helper() + inner := fake.New() + farmID, queueID = seedSubmitPrereqs(t, inner) + st = &submitSpy{Store: inner} + sub = openjd.NewSubmitter(st) + + up, err := sub.Submit(t.Context(), minimalJSON("UpstreamJob"), store.TemplateFormatJSON, openjd.SubmitOptions{ + FarmID: farmID, + QueueID: queueID, + }) + if err != nil { + t.Fatalf("Submit(upstream): %v", err) + } + return st, sub, farmID, queueID, up.Job.ID +} + +// TestSubmit_BlockedStatusIsPartOfTheAtomicWrite pins where the blocked status +// is decided: inside the single [store.JobStore.CreateJobSubmission] call, next +// to the dependency edges that justify it. +// +// It used to be a separate UpdateJobStatus issued after that call, so the two +// halves of one decision — "this job is blocked" and "here is what it is +// blocked on" — committed independently. Asserting that Submit issues no status +// update at all is what makes the coupling structural rather than incidental: a +// change that reintroduces the second write fails here even if it happens to +// leave the end state correct. +func TestSubmit_BlockedStatusIsPartOfTheAtomicWrite(t *testing.T) { + st, sub, farmID, queueID, upstreamID := newBlockedSubmitFixture(t) + + result, err := sub.Submit(t.Context(), minimalJSON("BlockedJob"), store.TemplateFormatJSON, openjd.SubmitOptions{ + FarmID: farmID, + QueueID: queueID, + DependsOn: []string{upstreamID}, + }) + if err != nil { + t.Fatalf("Submit: %v", err) + } + + if got := st.lastSubmission.Job.Status; got != store.JobStatusBlocked { + t.Errorf("the submission handed to the store carried status %q, want blocked", got) + } + if got := st.lastSubmission.DependsOn; len(got) != 1 || got[0] != upstreamID { + t.Errorf("the submission handed to the store carried DependsOn %v, want [%s]", got, upstreamID) + } + if st.statusUpdates != 0 { + t.Errorf("Submit issued %d UpdateJobStatus calls, want 0 (the status belongs to the atomic write)", st.statusUpdates) + } + if result.Job.Status != store.JobStatusBlocked { + t.Errorf("result job status = %q, want blocked", result.Job.Status) + } +} + +// TestSubmit_BlockedStatusIsAtomicWithTheRows is the failure-mode test, and the +// reason folding the status write in was mandatory rather than tidy. +// +// While the status was written separately, a failure of that write left the +// job, its edges, its steps and its tasks all durable with the job stranded in +// pending — and stranded is literal. buildStepWithTasks creates every task +// pending whenever the job is held, so nothing runs; reconcileBlockedJob +// (internal/scheduler/jobdeps.go) early-returns unless status is blocked, so +// neither sweepBlockedJobs nor ReconcileDependents ever revisits the row; and +// the scheduler leases only ready tasks. The job hangs until an operator +// intervenes. +// +// That is why the assertion here is that NO row for the job exists. A test +// written to the failure mode this one was first thought to have — "assert the +// tasks are not ready" — passes without the fix, because they were never ready. +// +// What it actually exercises on HEAD, stated plainly: the injected failure is on +// UpdateJobStatus, which Submit no longer calls, so err is always nil here and +// the err != nil branch — the zero-rows assertion — is DEAD CODE today. The live +// assertion is the success branch: the submission completed and the persisted +// job is blocked. The dead branch is kept deliberately, as the guard for a +// regression that reintroduces a separate status write: such a change would make +// this injection fire again, and the branch would then assert what it was +// written to assert. Either branch is atomic; a job row surviving in the wrong +// status is not. +func TestSubmit_BlockedStatusIsAtomicWithTheRows(t *testing.T) { + st, sub, farmID, queueID, upstreamID := newBlockedSubmitFixture(t) + st.failStatusUpdate = errors.New("status write failed") + + result, err := sub.Submit(t.Context(), minimalJSON("BlockedJob"), store.TemplateFormatJSON, openjd.SubmitOptions{ + FarmID: farmID, + QueueID: queueID, + DependsOn: []string{upstreamID}, + }) + + // The job ID is taken from the spy rather than the result, because a failed + // Submit returns nil and the caller never learns which row to look for -- + // which is exactly why a stranded row could not be cleaned up. + jobID := st.lastSubmission.Job.ID + if jobID == "" { + t.Fatal("Submit never reached the store; the test cannot observe what it left behind") + } + + if err != nil { + if _, gerr := st.GetJob(t.Context(), jobID); !errors.Is(gerr, store.ErrNotFound) { + t.Fatalf("a job row survived a failed submission (GetJob(%s) = %v), want ErrNotFound", jobID, gerr) + } + return + } + + // The status write is gone, so the submission succeeded: it must be whole. + job, gerr := st.GetJob(t.Context(), jobID) + if gerr != nil { + t.Fatalf("GetJob(%s): %v", jobID, gerr) + } + if job.Status != store.JobStatusBlocked { + t.Errorf("persisted job status = %q, want blocked", job.Status) + } + if result.Job.Status != store.JobStatusBlocked { + t.Errorf("result job status = %q, want blocked", result.Job.Status) + } +} + +// TestSubmit_BlockedJobIsNeverObservableWithoutItsEdges pins the end state that +// the old ordering existed to protect. +// +// Creating a job already-blocked used to let a sweepBlockedJobs tick land after +// the job row existed but before its edges were written, see a blocked job with +// ZERO edges, read that as "nothing left to wait on", and release it — leaving a +// job neither blocked nor scheduled, which the sweep never revisits. Writing the +// status last is how that was avoided; writing status and edges in one +// transaction is how it is avoided now. +// +// The window itself is not observable from a single-threaded test. What is +// observable, and what this asserts, is that the two always arrive together: +// a persisted blocked job has its edges. +func TestSubmit_BlockedJobIsNeverObservableWithoutItsEdges(t *testing.T) { + st, sub, farmID, queueID, upstreamID := newBlockedSubmitFixture(t) + + result, err := sub.Submit(t.Context(), minimalJSON("BlockedJob"), store.TemplateFormatJSON, openjd.SubmitOptions{ + FarmID: farmID, + QueueID: queueID, + DependsOn: []string{upstreamID}, + }) + if err != nil { + t.Fatalf("Submit: %v", err) + } + + blocked, err := st.ListBlockedJobs(t.Context()) + if err != nil { + t.Fatalf("ListBlockedJobs: %v", err) + } + if len(blocked) != 1 || blocked[0].ID != result.Job.ID { + t.Fatalf("ListBlockedJobs = %d jobs, want just %s", len(blocked), result.Job.ID) + } + + // This is the read sweepBlockedJobs makes of every job it finds blocked. + // Zero edges here is what it would misread as "nothing left to wait on". + ids, err := st.ListJobDependencyIDs(t.Context(), blocked[0].ID) + if err != nil { + t.Fatalf("ListJobDependencyIDs: %v", err) + } + if len(ids) != 1 || ids[0] != upstreamID { + t.Fatalf("a blocked job's dependency edges = %v, want [%s]", ids, upstreamID) + } +} diff --git a/internal/presetgen/appledouble_test.go b/internal/presetgen/appledouble_test.go new file mode 100644 index 00000000..2abfcc6e --- /dev/null +++ b/internal/presetgen/appledouble_test.go @@ -0,0 +1,58 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package presetgen_test + +import ( + "os" + "path/filepath" + "testing" + + "github.com/uberware/sqi/internal/presetgen" +) + +// TestBuild_IgnoresAppleDoubleSidecars pins that a macOS AppleDouble sidecar +// in the presets directory is not read as a preset. +// +// macOS writes "._name" alongside a file whenever extended attributes cannot +// be stored natively, which is the case on non-APFS volumes. A plain `git +// checkout` that rewrites a preset is enough to create one: the rewritten file +// gets a com.apple.provenance xattr, and the xattr lands in the sidecar. +// +// The sidecar matches *.yaml, so without this filter Build reads it, fails to +// parse it ("control characters are not allowed"), and returns a hard error -- +// meaning a release-time publish breaks on a developer machine for a reason +// that has nothing to do with the presets. This is not hypothetical: it +// happened in this repo and broke `make ci` for every package that globs the +// presets directory. +func TestBuild_IgnoresAppleDoubleSidecars(t *testing.T) { + dir := t.TempDir() + + // One real preset, copied from the authored set. + authored, err := os.ReadFile(filepath.Join(presetsDir, "blender-batch-render.yaml")) + if err != nil { + t.Fatalf("read source preset: %v", err) + } + if err := os.WriteFile(filepath.Join(dir, "blender-batch-render.yaml"), authored, 0o600); err != nil { + t.Fatalf("write preset: %v", err) + } + + // The sidecar. Real ones begin with the AppleDouble magic number and are + // full of NUL bytes, which is exactly what makes the YAML parser reject + // them -- so use bytes of that shape rather than something that might + // accidentally parse. + sidecar := append([]byte{0x00, 0x05, 0x16, 0x07}, make([]byte, 128)...) + if err := os.WriteFile(filepath.Join(dir, "._blender-batch-render.yaml"), sidecar, 0o600); err != nil { + t.Fatalf("write sidecar: %v", err) + } + + got, err := presetgen.Build(dir, "sqi") + if err != nil { + t.Fatalf("Build: %v", err) + } + if len(got) != 1 { + t.Fatalf("got %d presets, want 1: the AppleDouble sidecar was read as a preset", len(got)) + } + if got[0].Entry.Name != "blender-batch-render" { + t.Errorf("name = %q, want blender-batch-render", got[0].Entry.Name) + } +} diff --git a/internal/presetgen/presetgen.go b/internal/presetgen/presetgen.go index cda72c14..78e593e8 100644 --- a/internal/presetgen/presetgen.go +++ b/internal/presetgen/presetgen.go @@ -44,6 +44,8 @@ type Index struct { // root used for the index's definition field and the published file location. // The sha256 is computed over the raw file bytes so the served file always // matches the fingerprint the index vouches for. +// +// macOS AppleDouble sidecars ("._name") are skipped -- see [isAppleDouble]. func Build(presetsDir, definitionDir string) ([]Generated, error) { matches, err := filepath.Glob(filepath.Join(presetsDir, "*.yaml")) if err != nil { @@ -51,6 +53,9 @@ func Build(presetsDir, definitionDir string) ([]Generated, error) { } out := make([]Generated, 0, len(matches)) for _, p := range matches { + if isAppleDouble(p) { + continue + } data, err := os.ReadFile(p) if err != nil { return nil, fmt.Errorf("read %s: %w", p, err) @@ -146,3 +151,17 @@ func Publish(presetsDir, outDir, definitionDir string) error { } return nil } + +// isAppleDouble reports whether p is a macOS AppleDouble sidecar ("._name"). +// +// macOS writes one alongside a file whenever extended attributes cannot be +// stored natively, which is the case on non-APFS volumes. A plain `git +// checkout` that rewrites a preset is enough to create one: the rewritten file +// picks up a com.apple.provenance xattr and the xattr lands in the sidecar. +// +// It matters here because a sidecar matches *.yaml, so without this it is read +// as a preset, fails to parse, and turns a release-time publish into a hard +// error for a reason unrelated to the presets themselves. +func isAppleDouble(p string) bool { + return strings.HasPrefix(filepath.Base(p), "._") +} diff --git a/internal/presetlib/dccpresets_test.go b/internal/presetlib/dccpresets_test.go index 3b5c2824..eb9c5d0e 100644 --- a/internal/presetlib/dccpresets_test.go +++ b/internal/presetlib/dccpresets_test.go @@ -19,9 +19,24 @@ import ( // The DCC reference presets must round-trip through the real library install // path (index → fetch → fingerprint verify → parsed product). func TestDCCReferencePresetsInstallLoop(t *testing.T) { - paths, err := filepath.Glob(filepath.Join("..", "..", "presets", "sqi", "*.yaml")) - if err != nil || len(paths) == 0 { - t.Fatalf("glob presets/sqi: %v (%d files)", err, len(paths)) + matches, err := filepath.Glob(filepath.Join("..", "..", "presets", "sqi", "*.yaml")) + if err != nil { + t.Fatalf("glob presets/sqi: %v", err) + } + // Drop macOS AppleDouble sidecars ("._name"), which match *.yaml. They + // appear on non-APFS volumes whenever a checkout rewrites a preset, and + // serving one as a preset definition fails this test for a reason that + // has nothing to do with the presets. internal/presetgen filters them in + // production code for the same reason -- see isAppleDouble there. + paths := make([]string, 0, len(matches)) + for _, p := range matches { + if strings.HasPrefix(filepath.Base(p), "._") { + continue + } + paths = append(paths, p) + } + if len(paths) == 0 { + t.Fatalf("glob presets/sqi matched no presets (%d before filtering)", len(matches)) } files := map[string][]byte{} var entries []IndexEntry diff --git a/internal/product/dccpresets_test.go b/internal/product/dccpresets_test.go index 253217ce..5265535a 100644 --- a/internal/product/dccpresets_test.go +++ b/internal/product/dccpresets_test.go @@ -20,10 +20,22 @@ func TestDCCReferencePresets(t *testing.T) { "nuke-script-render": {"SceneFile", "Frames"}, "blender-batch-render": {"SceneFile", "Frames", "OutputPath"}, } - paths, err := filepath.Glob(filepath.Join("..", "..", "presets", "sqi", "*.yaml")) + matches, err := filepath.Glob(filepath.Join("..", "..", "presets", "sqi", "*.yaml")) if err != nil { t.Fatalf("glob: %v", err) } + // Drop macOS AppleDouble sidecars ("._name"), which match *.yaml. They + // appear on non-APFS volumes whenever a checkout rewrites a preset, and + // counting one as a preset fails this test for a reason that has nothing + // to do with the presets. internal/presetgen filters them for the same + // reason -- see isAppleDouble there. + paths := make([]string, 0, len(matches)) + for _, p := range matches { + if strings.HasPrefix(filepath.Base(p), "._") { + continue + } + paths = append(paths, p) + } if len(paths) != len(want) { t.Fatalf("expected %d presets, found %d: %v", len(want), len(paths), paths) } diff --git a/internal/product/testingpresets_test.go b/internal/product/testingpresets_test.go index 42892c27..7686fd05 100644 --- a/internal/product/testingpresets_test.go +++ b/internal/product/testingpresets_test.go @@ -27,10 +27,22 @@ func TestTestingPresets(t *testing.T) { "test-steps-bash": sharedParams, "test-steps-powershell": sharedParams, } - paths, err := filepath.Glob(filepath.Join("..", "..", "presets", "testing", "*.yaml")) + matches, err := filepath.Glob(filepath.Join("..", "..", "presets", "testing", "*.yaml")) if err != nil { t.Fatalf("glob: %v", err) } + // Drop macOS AppleDouble sidecars ("._name"), which match *.yaml. They + // appear on non-APFS volumes whenever a checkout rewrites a preset, and + // counting one as a preset fails this test for a reason that has nothing + // to do with the presets. internal/presetgen filters them for the same + // reason -- see isAppleDouble there. + paths := make([]string, 0, len(matches)) + for _, p := range matches { + if strings.HasPrefix(filepath.Base(p), "._") { + continue + } + paths = append(paths, p) + } if len(paths) != len(want) { t.Fatalf("expected %d testing presets, found %d: %v", len(want), len(paths), paths) } diff --git a/internal/store/fake/job.go b/internal/store/fake/job.go index 1dc7ba54..e19b865f 100644 --- a/internal/store/fake/job.go +++ b/internal/store/fake/job.go @@ -21,6 +21,136 @@ func (s *Store) CreateJob(_ context.Context, job store.Job) (store.Job, error) { return job, nil } +// CreateJobSubmission implements [store.JobStore]. It validates the whole +// submission before mutating anything, so a rejected submission leaves the +// store untouched — the in-memory equivalent of the SQLite implementation's +// transaction. +func (s *Store) CreateJobSubmission(_ context.Context, sub store.JobSubmission) (store.JobSubmission, error) { + s.mu.Lock() + defer s.mu.Unlock() + + if err := s.validateSubmission(sub); err != nil { + return store.JobSubmission{}, err + } + + now := time.Now().UTC() + out := store.JobSubmission{ + DependsOn: copySlice(sub.DependsOn), + Steps: make([]store.Step, 0, len(sub.Steps)), + Tasks: make([]store.Task, 0, len(sub.Tasks)), + } + + job := sub.Job + job.Parameters = copyMap(job.Parameters) + job.CreatedAt, job.UpdatedAt = now, now + s.jobs[job.ID] = job + out.Job = job + + existing := s.jobDependencies[job.ID] + for _, up := range sub.DependsOn { + if slices.Contains(existing, up) { + continue + } + existing = append(existing, up) + } + if len(existing) > 0 { + s.jobDependencies[job.ID] = existing + } + + // Each step and task is stamped with its own time.Now(), mirroring the + // SQLite implementation, where tasks within a step sharing one created_at + // would silently disable the ready-task ordering tiebreaker and destabilize + // ListTasks paging (see insertTasksTx in sqlite/job.go). + for _, step := range sub.Steps { + rowNow := time.Now().UTC() + step.DependsOn = copySlice(step.DependsOn) + step.CreatedAt, step.UpdatedAt = rowNow, rowNow + s.steps[step.ID] = step + out.Steps = append(out.Steps, step) + } + for _, task := range sub.Tasks { + rowNow := time.Now().UTC() + task.Parameters = copyMap(task.Parameters) + task.CreatedAt, task.UpdatedAt = rowNow, rowNow + s.tasks[task.ID] = task + out.Tasks = append(out.Tasks, task) + } + + return out, nil +} + +// validateSubmission runs, before any mutation happens, the checks that make +// the fake reject what SQLite's schema would reject. Callers must hold s.mu. +// +// It mirrors exactly three constraints: the jobs primary key, the +// steps_job_name_unique UNIQUE (job_id, name) constraint, and the steps and +// tasks primary keys — each checked both within the submission and against +// what is already stored. Without the primary-key checks the fake does not +// merely accept a duplicate ID, it silently LOSES the row (the map assignment +// overwrites) and still reports success, so a Submit regression that reused an +// ID would be green through every fake-backed test and ErrConflict only in +// production. +// +// It does NOT mirror SQLite's foreign keys: a submission naming a nonexistent +// farm, queue or step is accepted here. That gap is pre-existing in CreateJob, +// CreateStep and CreateTask and is deliberately left alone rather than closed +// only on this one path. (job_dependencies.depends_on_job_id carries no FK at +// all, so accepting an edge to a nonexistent upstream job is correct parity.) +func (s *Store) validateSubmission(sub store.JobSubmission) error { + if _, exists := s.jobs[sub.Job.ID]; exists { + return store.ErrConflict + } + if err := s.validateSubmissionSteps(sub.Steps); err != nil { + return err + } + return s.validateSubmissionTasks(sub.Tasks) +} + +// validateSubmissionTasks rejects a task ID that collides within the +// submission or with a stored task. Callers must hold s.mu. +func (s *Store) validateSubmissionTasks(tasks []store.Task) error { + ids := make(map[string]struct{}, len(tasks)) + for _, task := range tasks { + if _, dup := ids[task.ID]; dup { + return store.ErrConflict + } + if _, exists := s.tasks[task.ID]; exists { + return store.ErrConflict + } + ids[task.ID] = struct{}{} + } + return nil +} + +// validateSubmissionSteps rejects a step ID or a (job_id, name) pair that +// collides within the submission or with a stored step. Callers must hold +// s.mu. +func (s *Store) validateSubmissionSteps(steps []store.Step) error { + ids := make(map[string]struct{}, len(steps)) + names := make(map[string]struct{}, len(steps)) + for _, step := range steps { + if _, dup := ids[step.ID]; dup { + return store.ErrConflict + } + if _, exists := s.steps[step.ID]; exists { + return store.ErrConflict + } + ids[step.ID] = struct{}{} + + key := step.JobID + "\x00" + step.Name + if _, dup := names[key]; dup { + return store.ErrConflict + } + names[key] = struct{}{} + for _, existing := range s.steps { + if existing.JobID == step.JobID && existing.Name == step.Name { + return store.ErrConflict + } + } + } + return nil +} + // GetJob returns the job with the given ID, or [store.ErrNotFound]. func (s *Store) GetJob(_ context.Context, id string) (store.Job, error) { s.mu.Lock() diff --git a/internal/store/job.go b/internal/store/job.go index 00913bd5..0840a495 100644 --- a/internal/store/job.go +++ b/internal/store/job.go @@ -120,17 +120,71 @@ type DeletedJob struct { QueueID string } +// JobSubmission is everything one job submission creates. +// +// It exists so a job, its dependency edges, its steps and its tasks are +// created together or not at all — see [JobStore.CreateJobSubmission]. +type JobSubmission struct { + Job Job + DependsOn []string + Steps []Step + Tasks []Task +} + // JobStore is the persistence interface for [Job] records. type JobStore interface { // CreateJob inserts a new job with all fields populated by the caller. + // + // It has NO production callers. Submission was its only one and now goes + // through [JobStore.CreateJobSubmission]; the same is true of + // [JobStore.CreateJobDependencies], [StepStore.CreateStep] and + // [TaskStore.CreateTask]. All four are test-only API surface kept for + // fixture construction, with two consequences worth knowing: + // + // - No production test exercises them, so they can drift from the path + // production actually takes without anything going red. The fake's + // CreateStep and CreateTask already differ: they do not stamp + // CreatedAt/UpdatedAt, while its CreateJobSubmission does (per row, so + // tasks within a step get distinct created_at values — SQLite relies on + // that for the ready-task ordering tiebreaker and ListTasks paging). + // A fixture built from these creators therefore has zero timestamps + // where a real submission has meaningful ones. + // - A behavior change made here does not reach production. Change + // CreateJobSubmission too, or the change is cosmetic. CreateJob(ctx context.Context, job Job) (Job, error) + // CreateJobSubmission atomically creates a job, its dependency edges, its + // steps and its tasks. On ANY error nothing is written. + // + // It exists because creating those rows through separate calls left two + // defects with no cure at the call site: a failed submission stranded a + // pending job that no sweep reaps, and a submission whose write failed + // after some steps were persisted produced a job whose missing steps made + // checkJobCompletion — which derives job status from the steps that exist — + // report it completed. The second needs a STORE failure specifically: an + // expansion failure left the step row too, because the old code wrote it + // before expanding its tasks, so that case hung pending rather than + // completing. Both are properties of partial creation, so both end here. + // + // The returned JobSubmission carries the rows as stored, the way + // [JobStore.CreateJob], [StepStore.CreateStep] and [TaskStore.CreateTask] + // each return theirs. Read the edges back from its DependsOn field, not + // from its Job.DependsOn: the returned [Job] is scanned straight from the + // insert, which does not join the edge table, so Job.DependsOn is + // backend-dependent and must not be relied on. Only [JobStore.GetJob] + // populates it. + CreateJobSubmission(ctx context.Context, sub JobSubmission) (JobSubmission, error) + // GetJob returns the job with the given ID, or [ErrNotFound]. GetJob(ctx context.Context, id string) (Job, error) // CreateJobDependencies records that jobID waits on each ID in upstreamIDs - // (whole-job cross-job dependencies). Duplicate edges are ignored. Called - // right after CreateJob during submission. + // (whole-job cross-job dependencies). Duplicate edges are ignored. + // + // Submission no longer calls this: the edges are written by + // [JobStore.CreateJobSubmission], in the same transaction as the job row + // whose blocked status they justify. See [JobStore.CreateJob] on what that + // leaves this method. CreateJobDependencies(ctx context.Context, jobID string, upstreamIDs []string) error // ListJobDependencyIDs returns the IDs of the upstream jobs jobID waits on, diff --git a/internal/store/jobsubmission_test.go b/internal/store/jobsubmission_test.go new file mode 100644 index 00000000..4ededca4 --- /dev/null +++ b/internal/store/jobsubmission_test.go @@ -0,0 +1,357 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package store_test + +import ( + "context" + "testing" + "time" + + "github.com/uberware/sqi/internal/store" +) + +// submissionFixture builds a two-step, three-task submission on a fresh farm +// and queue. Both are created first because the job row references them. +func submissionFixture(ctx context.Context, t *testing.T, st store.Store) store.JobSubmission { + t.Helper() + if _, err := st.CreateFarm(ctx, store.Farm{ID: "farm-1", Name: "f"}); err != nil { + t.Fatalf("CreateFarm: %v", err) + } + if _, err := st.CreateQueue(ctx, store.Queue{ID: "queue-1", FarmID: "farm-1", Name: "q"}); err != nil { + t.Fatalf("CreateQueue: %v", err) + } + return store.JobSubmission{ + Job: store.Job{ + ID: "job-1", FarmID: "farm-1", QueueID: "queue-1", + Name: "j", Status: store.JobStatusPending, + }, + Steps: []store.Step{ + {ID: "step-1", JobID: "job-1", Name: "a", StepOrder: 0, Status: store.StepStatusReady}, + {ID: "step-2", JobID: "job-1", Name: "b", StepOrder: 1, Status: store.StepStatusPending}, + }, + Tasks: []store.Task{ + {ID: "task-1", JobID: "job-1", StepID: "step-1", Name: "a-0", Status: store.TaskStatusReady}, + {ID: "task-2", JobID: "job-1", StepID: "step-1", Name: "a-1", Status: store.TaskStatusReady}, + {ID: "task-3", JobID: "job-1", StepID: "step-2", Name: "b-0", Status: store.TaskStatusPending}, + }, + } +} + +// assertFreshTimestamps checks that a row the store just created carries a +// CreatedAt that is actually "now" and an UpdatedAt equal to it, rather than +// merely a non-zero value. +func assertFreshTimestamps(t *testing.T, what string, createdAt, updatedAt time.Time) { + t.Helper() + if createdAt.IsZero() { + t.Errorf("%s has a zero CreatedAt; it was not populated by the store", what) + return + } + if !updatedAt.Equal(createdAt) { + t.Errorf("%s has UpdatedAt %v, want it equal to CreatedAt %v", what, updatedAt, createdAt) + } + if skew := time.Since(createdAt); skew < -time.Second || skew > time.Second { + t.Errorf("%s has CreatedAt %v, which is %v away from now", what, createdAt, skew) + } +} + +// TestJobStore_CreateJobSubmission_WritesEverything pins the happy path on both +// backends: one call produces the job, its steps and its tasks, and returns +// them populated the way the per-row creators do. +func TestJobStore_CreateJobSubmission_WritesEverything(t *testing.T) { + for name, st := range newStores(t) { + t.Run(name, func(t *testing.T) { + ctx := context.Background() + sub := submissionFixture(ctx, t, st) + + out, err := st.CreateJobSubmission(ctx, sub) + if err != nil { + t.Fatalf("CreateJobSubmission: %v", err) + } + + if out.Job.ID != "job-1" { + t.Errorf("returned job ID = %q, want job-1", out.Job.ID) + } + if len(out.Steps) != 2 { + t.Errorf("returned %d steps, want 2", len(out.Steps)) + } + if len(out.Tasks) != 3 { + t.Errorf("returned %d tasks, want 3", len(out.Tasks)) + } + // The rows come back the way the per-row creators return theirs: + // timestamps populated by the store, not the caller's zero values. + // A non-zero check alone would pass on a value a century off, or + // on a CreatedAt stamped without its UpdatedAt, so both backends + // are held to "now, and the same on both fields". + assertFreshTimestamps(t, "job "+out.Job.ID, out.Job.CreatedAt, out.Job.UpdatedAt) + for _, s := range out.Steps { + assertFreshTimestamps(t, "step "+s.ID, s.CreatedAt, s.UpdatedAt) + } + for _, tk := range out.Tasks { + assertFreshTimestamps(t, "task "+tk.ID, tk.CreatedAt, tk.UpdatedAt) + } + + if _, err := st.GetJob(ctx, "job-1"); err != nil { + t.Errorf("GetJob: %v", err) + } + steps, err := st.ListSteps(ctx, "job-1") + if err != nil || len(steps) != 2 { + t.Errorf("ListSteps = %d steps, %v; want 2, nil", len(steps), err) + } + tasks, err := st.ListTasks(ctx, store.ListTasksOptions{JobID: "job-1"}) + if err != nil { + t.Fatalf("ListTasks: %v", err) + } + if len(tasks.Items) != 3 { + t.Errorf("ListTasks = %d tasks, want 3", len(tasks.Items)) + } + }) + } +} + +// TestJobStore_CreateJobSubmission_RollsBackEntirely is the whole point of the +// method, and of this change. +// +// A submission that fails partway must leave NOTHING: not the job row, not the +// steps that already inserted, not their tasks. Before this method existed, +// Submit wrote those rows one call at a time and a mid-way failure stranded a +// pending job that no sweep reaps and that checkJobCompletion would later mark +// completed despite missing steps. +// +// The induced failure is a duplicate step name, which violates the (JobID, +// Name) uniqueness both backends enforce (see store/step.go's CreateStep doc). +// It fires on the SECOND step, so the job row and the first step have already +// been written inside the transaction when it hits. +// +// The sqlite subtest carries the whole test. Sabotaged by replacing SQLite's +// deferred Rollback with a Commit, it fails with the job row and exactly ONE +// step row surviving — which is what proves the conflict fires after real +// writes rather than before any. The fake subtest is VACUOUS BY CONSTRUCTION +// and stays green under that same sabotage: validateSubmission runs to +// completion before the first map assignment, so on the failing path the fake +// never wrote anything to roll back. That is the fake's intended design, not +// an oversight, but it means this test's non-vacuity rests entirely on sqlite. +func TestJobStore_CreateJobSubmission_RollsBackEntirely(t *testing.T) { + for name, st := range newStores(t) { + t.Run(name, func(t *testing.T) { + ctx := context.Background() + sub := submissionFixture(ctx, t, st) + sub.Steps[1].Name = sub.Steps[0].Name // duplicate -> conflict + + if _, err := st.CreateJobSubmission(ctx, sub); err == nil { + t.Fatal("CreateJobSubmission accepted a duplicate step name") + } + + if _, err := st.GetJob(ctx, "job-1"); err == nil { + t.Error("the job row survived a failed submission; the write was not rolled back") + } + steps, err := st.ListSteps(ctx, "job-1") + if err == nil && len(steps) != 0 { + t.Errorf("%d step rows survived a failed submission, want 0", len(steps)) + } + tasks, err := st.ListTasks(ctx, store.ListTasksOptions{JobID: "job-1"}) + if err == nil && len(tasks.Items) != 0 { + t.Errorf("%d task rows survived a failed submission, want 0", len(tasks.Items)) + } + }) + } +} + +// TestJobStore_CreateJobSubmission_WritesDependencyEdges pins that the +// dependency edges are part of the same atomic write, which is what lets the +// job be created directly in blocked status (see Task 3): a sweep can never +// observe a blocked job with zero edges if both commit together. +func TestJobStore_CreateJobSubmission_WritesDependencyEdges(t *testing.T) { + for name, st := range newStores(t) { + t.Run(name, func(t *testing.T) { + ctx := context.Background() + sub := submissionFixture(ctx, t, st) + + // An upstream job for the edge to point at. + upstream := sub.Job + upstream.ID = "job-upstream" + upstream.Name = "up" + if _, err := st.CreateJob(ctx, upstream); err != nil { + t.Fatalf("CreateJob(upstream): %v", err) + } + + sub.Job.Status = store.JobStatusBlocked + sub.DependsOn = []string{"job-upstream"} + + if _, err := st.CreateJobSubmission(ctx, sub); err != nil { + t.Fatalf("CreateJobSubmission: %v", err) + } + + ids, err := st.ListJobDependencyIDs(ctx, "job-1") + if err != nil { + t.Fatalf("ListJobDependencyIDs: %v", err) + } + if len(ids) != 1 || ids[0] != "job-upstream" { + t.Errorf("dependency IDs = %v, want [job-upstream]", ids) + } + + job, err := st.GetJob(ctx, "job-1") + if err != nil { + t.Fatalf("GetJob: %v", err) + } + if job.Status != store.JobStatusBlocked { + t.Errorf("status = %q, want blocked", job.Status) + } + }) + } +} + +// TestJobStore_CreateJobSubmission_DoesNotAliasCallerMemory pins the defensive +// copying the per-row creators already do: mutating the slices and maps handed +// to CreateJobSubmission after it returns must not change what is stored. +// +// This is effectively a FAKE-ONLY test wearing a cross-backend harness, and a +// later reader should not over-trust the fact that it passes on both. SQLite +// marshals every one of these fields to JSON on the way in and re-scans it on +// the way out, so aliasing caller memory is impossible there no matter what +// the code does; only the fake, which stores Go values directly, can fail it. +// It is run on both anyway so the contract is stated once rather than twice. +func TestJobStore_CreateJobSubmission_DoesNotAliasCallerMemory(t *testing.T) { + for name, st := range newStores(t) { + t.Run(name, func(t *testing.T) { + ctx := context.Background() + sub := submissionFixture(ctx, t, st) + // Job.Parameters is here because the fake's copyMap on it is one of + // the deviations from its own per-row CreateJob, which copies + // nothing; without this the deviation would be untested. + sub.Job.Parameters = map[string]string{"k": "v"} + sub.Steps[1].DependsOn = []string{"a"} + sub.Tasks[0].Parameters = map[string]string{"frame": "1"} + + if _, err := st.CreateJobSubmission(ctx, sub); err != nil { + t.Fatalf("CreateJobSubmission: %v", err) + } + + sub.Job.Parameters["k"] = "mutated" + sub.Steps[1].DependsOn[0] = "mutated" + sub.Tasks[0].Parameters["frame"] = "mutated" + + job, err := st.GetJob(ctx, "job-1") + if err != nil { + t.Fatalf("GetJob: %v", err) + } + if job.Parameters["k"] != "v" { + t.Errorf("stored job parameter = %q, want v; the store aliased caller memory", job.Parameters["k"]) + } + + steps, err := st.ListSteps(ctx, "job-1") + if err != nil { + t.Fatalf("ListSteps: %v", err) + } + for _, s := range steps { + if s.ID == "step-2" && (len(s.DependsOn) != 1 || s.DependsOn[0] != "a") { + t.Errorf("stored step depends_on = %v, want [a]; the store aliased caller memory", s.DependsOn) + } + } + task, err := st.GetTask(ctx, "task-1") + if err != nil { + t.Fatalf("GetTask: %v", err) + } + if task.Parameters["frame"] != "1" { + t.Errorf("stored task parameter = %q, want 1; the store aliased caller memory", task.Parameters["frame"]) + } + }) + } +} + +// TestJobStore_CreateJobSubmission_RejectsDuplicateIDs pins that a submission +// reusing a step or task ID is REJECTED rather than accepted with rows +// silently dropped. +// +// SQLite gets this from the steps and tasks PRIMARY KEY. The fake had to be +// taught it: its maps are keyed by ID, so a duplicate overwrote, and the call +// returned a JobSubmission of the submitted length while ListSteps returned +// one fewer — reporting success having lost a row. A Submit regression that +// reused an ID would have been green through every fake-backed test in +// internal/openjd, internal/api and internal/scheduler, and ErrConflict only +// in production. +func TestJobStore_CreateJobSubmission_RejectsDuplicateIDs(t *testing.T) { + cases := map[string]func(sub *store.JobSubmission){ + "duplicate step ID": func(sub *store.JobSubmission) { sub.Steps[1].ID = sub.Steps[0].ID }, + "duplicate task ID": func(sub *store.JobSubmission) { sub.Tasks[1].ID = sub.Tasks[0].ID }, + } + for caseName, mutate := range cases { + // newStores is called per case so each subtest gets a store with no + // farm-1/queue-1 left over from the previous one. + for name, st := range newStores(t) { + t.Run(caseName+"/"+name, func(t *testing.T) { + ctx := context.Background() + sub := submissionFixture(ctx, t, st) + mutate(&sub) + + if _, err := st.CreateJobSubmission(ctx, sub); err == nil { + t.Fatalf("CreateJobSubmission accepted a %s", caseName) + } + if _, err := st.GetJob(ctx, "job-1"); err == nil { + t.Error("the job row survived a rejected submission") + } + steps, err := st.ListSteps(ctx, "job-1") + if err == nil && len(steps) != 0 { + t.Errorf("%d step rows survived a rejected submission, want 0", len(steps)) + } + }) + } + } +} + +// TestJobStore_CreateJobSubmission_StampsDistinctRowTimestamps pins that every +// step and task in one submission gets its OWN created_at. +// +// It is sqlite-only on purpose. Two SQLite consumers depend on this — the +// t.created_at tiebreaker in sqlListReadyTasks and ListTasks' single-column +// ORDER BY with LIMIT, which has no secondary key and therefore no stable page +// boundaries when the sort key ties (see insertTasksTx). Neither exists in the +// fake, whose insert loop is also far faster than this platform's wall clock +// advances, so asserting distinctness there would be flaky for no benefit. +// +// A future reintroduction of one shared timestamp for the whole batch must +// fail here rather than pass and quietly change dispatch order. +func TestJobStore_CreateJobSubmission_StampsDistinctRowTimestamps(t *testing.T) { + st, ok := newStores(t)["sqlite"] + if !ok { + t.Fatal("newStores did not provide a sqlite backend") + } + ctx := context.Background() + sub := submissionFixture(ctx, t, st) + + out, err := st.CreateJobSubmission(ctx, sub) + if err != nil { + t.Fatalf("CreateJobSubmission: %v", err) + } + + // task-1 and task-2 are both in step-1, which is exactly where + // sqlListReadyTasks relies on created_at to break the tie. + seen := make(map[time.Time]string, len(out.Tasks)) + for _, tk := range out.Tasks { + if other, dup := seen[tk.CreatedAt]; dup { + t.Errorf("tasks %s and %s share created_at %v; the ordering tiebreaker is inert", + other, tk.ID, tk.CreatedAt) + } + seen[tk.CreatedAt] = tk.ID + } + + stepTimes := make(map[time.Time]string, len(out.Steps)) + for _, s := range out.Steps { + if other, dup := stepTimes[s.CreatedAt]; dup { + t.Errorf("steps %s and %s share created_at %v", other, s.ID, s.CreatedAt) + } + stepTimes[s.CreatedAt] = s.ID + } + + // The stored rows, not just the returned ones. + stored, err := st.ListTasks(ctx, store.ListTasksOptions{JobID: "job-1"}) + if err != nil { + t.Fatalf("ListTasks: %v", err) + } + storedTimes := make(map[time.Time]string, len(stored.Items)) + for _, tk := range stored.Items { + if other, dup := storedTimes[tk.CreatedAt]; dup { + t.Errorf("stored tasks %s and %s share created_at %v", other, tk.ID, tk.CreatedAt) + } + storedTimes[tk.CreatedAt] = tk.ID + } +} diff --git a/internal/store/sqlite/job.go b/internal/store/sqlite/job.go index ca47f77f..7a2c5acd 100644 --- a/internal/store/sqlite/job.go +++ b/internal/store/sqlite/job.go @@ -6,6 +6,7 @@ import ( "context" "database/sql" "fmt" + "slices" "strings" "time" @@ -215,6 +216,144 @@ func (s *Store) CreateJob(ctx context.Context, job store.Job) (store.Job, error) return out, mapErr(err) } +// CreateJobSubmission implements [store.JobStore]. Every row one submission +// creates — the job, its dependency edges, its steps and its tasks — is +// written in a single transaction, so a failure at any point leaves nothing +// behind. +// +// Rows are inserted in foreign-key order (job, edges, steps, tasks) and each +// insert uses raw SQL via tx rather than the prepared statements the per-row +// creators use: a statement bound into a transaction with tx.StmtContext must +// itself be closed, and the other transactional writers here take the same +// approach (see UpdateTaskStatus in task.go). +// +// Each step and task row is stamped with its OWN time.Now(), exactly as +// CreateStep and CreateTask do — see insertTasksTx for why sharing one +// timestamp across the batch would be a behavior change, not an optimization. +func (s *Store) CreateJobSubmission(ctx context.Context, sub store.JobSubmission) (store.JobSubmission, error) { + tx, err := s.db.BeginTx(ctx, nil) + if err != nil { + return store.JobSubmission{}, mapErr(err) + } + defer func() { _ = tx.Rollback() }() //nolint:errcheck // rollback after commit is a no-op + + now := timeToText(time.Now().UTC()) + + // DependsOn is cloned rather than aliased so a caller mutating the returned + // slice cannot reach back into its own input, matching the fake. + out := store.JobSubmission{DependsOn: slices.Clone(sub.DependsOn)} + if out.Job, err = insertJobTx(ctx, tx, sub.Job, now); err != nil { + return store.JobSubmission{}, err + } + for _, up := range sub.DependsOn { + if _, err := tx.ExecContext(ctx, sqlInsertJobDependency, sub.Job.ID, up, now); err != nil { + return store.JobSubmission{}, fmt.Errorf("sqlite: create job dependency %s->%s: %w", sub.Job.ID, up, mapErr(err)) + } + } + if out.Steps, err = insertStepsTx(ctx, tx, sub.Steps); err != nil { + return store.JobSubmission{}, err + } + if out.Tasks, err = insertTasksTx(ctx, tx, sub.Tasks); err != nil { + return store.JobSubmission{}, err + } + + if err := tx.Commit(); err != nil { + return store.JobSubmission{}, mapErr(err) + } + return out, nil +} + +// insertJobTx inserts one job row inside tx, mirroring CreateJob's argument +// order exactly. +func insertJobTx(ctx context.Context, tx *sql.Tx, job store.Job, now string) (store.Job, error) { + paramsJSON, err := marshalJSON(job.Parameters) + if err != nil { + return store.Job{}, err + } + row := tx.QueryRowContext(ctx, sqlInsertJob, + job.ID, job.FarmID, job.QueueID, job.Name, job.Owner, job.Submitter, + job.Priority, string(job.Status), job.Project, + job.RawTemplate, string(job.TemplateFormat), paramsJSON, + now, now, + nullTimeToText(job.StartedAt), nullTimeToText(job.CompletedAt), + job.FailedAttempts, nullInt(job.MaxAttempts), nullInt(job.RetryDelaySeconds), nullInt(job.FailureLimit), + job.ParkReason) + out, err := scanJob(row) + return out, mapErr(err) +} + +// insertStepsTx inserts every step inside tx, mirroring CreateStep's argument +// order exactly — including its per-row time.Now() (see insertTasksTx). +func insertStepsTx(ctx context.Context, tx *sql.Tx, steps []store.Step) ([]store.Step, error) { + out := make([]store.Step, 0, len(steps)) + for _, step := range steps { + dependsOnJSON, err := marshalJSON(step.DependsOn) + if err != nil { + return nil, err + } + hostReqJSON, err := marshalJSON(step.HostRequirements) + if err != nil { + return nil, err + } + now := timeToText(time.Now().UTC()) + row := tx.QueryRowContext(ctx, sqlInsertStep, + step.ID, step.JobID, step.Name, dependsOnJSON, + step.StepOrder, string(step.Status), + hostReqJSON, step.ComputeLocation, + now, now) + stored, err := scanStep(row) + if err != nil { + return nil, mapErr(err) + } + out = append(out, stored) + } + return out, nil +} + +// insertTasksTx inserts every task inside tx, mirroring CreateTask's argument +// order exactly — including its per-row time.Now(). +// +// The per-row stamp is load-bearing, not incidental. Two consumers depend on +// tasks within one step having DISTINCT created_at values: +// +// - sqlListReadyTasks (task.go) ends its ORDER BY with "t.created_at ASC", +// documented there as the stable tiebreaker within a step. One shared +// timestamp makes that clause inert and frames dispatch in query-planner +// order rather than expansion order. +// - ListTasks (task.go) orders by a single column with LIMIT/OFFSET and no +// secondary key. SQLite's ORDER BY with LIMIT is a partial sort, so equal +// keys make page boundaries unstable: the same task can appear on two +// pages while another is omitted. +// +// A UUID tiebreaker in the SQL would be deterministic but would order frames +// randomly rather than in expansion order, which is a different behavior +// change wearing the same clothes. Stamping per row is what preserves today's +// behavior, since CreateTask calls time.Now() per row. +func insertTasksTx(ctx context.Context, tx *sql.Tx, tasks []store.Task) ([]store.Task, error) { + out := make([]store.Task, 0, len(tasks)) + for _, task := range tasks { + paramsJSON, err := marshalJSON(task.Parameters) + if err != nil { + return nil, err + } + var reqCores sql.NullInt64 + if task.RequiredCores != nil { + reqCores = sql.NullInt64{Int64: int64(*task.RequiredCores), Valid: true} + } + now := timeToText(time.Now().UTC()) + row := tx.QueryRowContext(ctx, sqlInsertTask, + task.ID, task.JobID, task.StepID, task.Name, paramsJSON, string(task.Status), + nullString(task.AssignedWorkerID), nullTimeToText(task.AssignedAt), now, now, reqCores, + task.UnschedulableReason, task.FailedAttempts, nullTimeToText(task.RetryAfter), task.FailureReason) + stored, err := scanTask(row) + if err != nil { + return nil, mapErr(err) + } + out = append(out, stored) + } + return out, nil +} + // GetJob implements [store.JobStore]. func (s *Store) GetJob(ctx context.Context, id string) (store.Job, error) { row := s.stmtGetJob.QueryRowContext(ctx, id) diff --git a/internal/store/step.go b/internal/store/step.go index 5f465197..8b52a19f 100644 --- a/internal/store/step.go +++ b/internal/store/step.go @@ -118,6 +118,10 @@ type StepAttributeRequirement struct { type StepStore interface { // CreateStep inserts a new step. The (JobID, Name) pair must be unique // within the job; returns [ErrConflict] if violated. + // + // It has no production callers — submission writes steps through + // [JobStore.CreateJobSubmission]. See [JobStore.CreateJob] for what that + // means for anyone changing this method or building fixtures with it. CreateStep(ctx context.Context, step Step) (Step, error) // GetStep returns the step with the given ID, or [ErrNotFound]. diff --git a/internal/store/task.go b/internal/store/task.go index 7bba7821..823bf622 100644 --- a/internal/store/task.go +++ b/internal/store/task.go @@ -118,6 +118,10 @@ const ( type TaskStore interface { // CreateTask inserts a new task. The caller must populate all fields // including a unique ID. + // + // It has no production callers — submission writes tasks through + // [JobStore.CreateJobSubmission]. See [JobStore.CreateJob] for what that + // means for anyone changing this method or building fixtures with it. CreateTask(ctx context.Context, task Task) (Task, error) // GetTask returns the task with the given ID, or [ErrNotFound].