conductor

CI task system
git clone git://git.finwo.net/app/conductor
Log | Files | Refs | README | LICENSE

commit 2b2db4d40d286b825ed500f7314f92eaf995c063
parent 49200bfa627cba63b79b35ebd205221095e28aa1
Author: finwo <finwo@pm.me>
Date:   Mon, 21 Sep 2026 21:59:00 +0200

Give every API response one shape

Diffstat:
Mdeploy/hub/conductor-worker.md | 22++++++++++++++--------
Mdeploy/hub/conductor.md | 19++++++++++++-------
Mdocs/api.md | 117+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------
Mdocs/deployment.md | 13++++++++++---
Mpackage.json | 3++-
Msrc/conductor/app.js | 21+++++++--------------
Asrc/conductor/routes/api.js | 23+++++++++++++++++++++++
Msrc/conductor/routes/artifacts.js | 5+++--
Asrc/conductor/routes/envelope.js | 54++++++++++++++++++++++++++++++++++++++++++++++++++++++
Msrc/conductor/routes/jobs.js | 90+++++++++++++++++++++++++++++++++++++------------------------------------------
Msrc/conductor/routes/tasks.js | 43+++++++++++++++++++++++--------------------
Msrc/conductor/routes/workers.js | 84+++++++++++++++++++++++++++++++++++++++----------------------------------------
Msrc/conductor/scheduler.js | 8+++++---
Asrc/lib/errors.js | 144+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Msrc/lib/page.js | 58++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Msrc/worker/client.js | 25++++++++++++++++++-------
Atest/api.test.js | 233+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Mtest/conductor.test.js | 177+++++++++++++++++++++++++++++++++++++++++--------------------------------------
Mtest/dialects.test.js | 18+++++++++++++++++-
Mtest/helpers/harness.js | 16++++++++++++++++
Mtest/retention.test.js | 4++--
Mtest/ui.test.js | 14+++++++-------
22 files changed, 914 insertions(+), 277 deletions(-)

diff --git a/deploy/hub/conductor-worker.md b/deploy/hub/conductor-worker.md @@ -11,17 +11,23 @@ Source and issues: https://github.com/finwo/conductor | Tag | What it is | | ---------------- | ------------------------------------------ | | `latest` | the most recent release | -| `0.4.0` | an exact version | -| `0.4` | the latest patch of that minor series | -| `<commit>` | an exact build from main, twelve hex chars | +| `0.5.0` | an exact version | +| `0.5` | the latest patch of that minor series | +| `main` | the tip of the default branch | Built for `linux/amd64`, `linux/arm64` and `linux/riscv64`. -**Upgrade the conductor and its workers together.** In 0.3.0 a job became -one run of a pipeline and a task became the unit this image runs. Workers -claim from `/api/v1/tasks/claim`, so a 0.2.x worker will not run anything -for a 0.3 or newer conductor. Pin both to the same version if they are -upgraded separately. +**Upgrade the conductor and its workers together.** Two releases have +changed what this image has to understand: + +- **0.5.0** put every API response in an envelope: `{"ok":true,"data":...}` + on success, `{"ok":false,"error":{"name":...}}` on failure. A worker older + than 0.5.0 reads the claim response one level too high and will not run + anything for a 0.5 or newer conductor. +- **0.3.0** made a job one run of a pipeline and a task the unit this image + runs. A 0.2.x worker cannot claim from a 0.3 or newer conductor. + +Pin both to the same version if they are upgraded separately. ## Quick start diff --git a/deploy/hub/conductor.md b/deploy/hub/conductor.md @@ -10,16 +10,21 @@ Source and issues: https://github.com/finwo/conductor | Tag | What it is | | ---------------- | ------------------------------------------ | | `latest` | the most recent release | -| `0.4.0` | an exact version | -| `0.4` | the latest patch of that minor series | -| `<commit>` | an exact build from main, twelve hex chars | +| `0.5.0` | an exact version | +| `0.5` | the latest patch of that minor series | +| `main` | the tip of the default branch | Built for `linux/amd64`, `linux/arm64` and `linux/riscv64`. -**Upgrade the conductor and its workers together.** In 0.3.0 a job became -one run of a pipeline and a task became the unit a worker runs. Workers -claim from `/api/v1/tasks/claim`, so a 0.2.x worker will not run anything -for a 0.3 or newer conductor. +**Upgrade the conductor and its workers together.** Two releases have +changed what a worker has to understand: + +- **0.5.0** put every API response in an envelope: `{"ok":true,"data":...}` + on success, `{"ok":false,"error":{"name":...}}` on failure. A worker older + than 0.5.0 reads the claim response one level too high and will not run + anything for a 0.5 or newer conductor. +- **0.3.0** made a job one run of a pipeline and a task the unit a worker + runs. A 0.2.x worker cannot claim from a 0.3 or newer conductor. ## Quick start diff --git a/docs/api.md b/docs/api.md @@ -31,9 +31,47 @@ itself is a public read; everything below it is the worker protocol and needs a token. They are separate plugins for that reason, so the token requirement cannot accidentally spread to the first or lapse on the rest. -Errors are `{"error": "..."}` with a meaningful status. A resource the -caller may not see returns 404 rather than 403, so absence and denial are -indistinguishable. +Responses +--------- + +Every response carries `ok`, so success and failure are told apart by one +key rather than inferred from which others turned up. + +```json +{ "ok": true, "data": { "...": "..." } } +``` + +```json +{ "ok": false, "error": { "name": "UnknownTask", "message": "No such task" } } +``` + +`error.name` is the identifier to branch on, and is stable. `error.message` +is for a person to read and may be reworded. Some errors add `error.details` +with machine-readable context; a 500 carries `details.request_id`, which is +the id the fault was logged under. + +A resource the caller may not see returns 404 rather than 403, so absence +and denial are indistinguishable. + +Responses holding a collection add `pagination`: + +```json +{ + "ok": true, + "data": { "...": "..." }, + "pagination": { + "next": "https://ci.example.com/api/v1/tasks/<id>?offset=50&limit=50", + "offset": 0, + "total": 128 + } +} +``` + +Ask with `?offset=` and `?limit=`. The limit defaults to 50 and is capped at +200; an offset or limit that cannot be read falls back rather than failing. +Follow `next` rather than building the URL, and stop when it is absent: +`next` is omitted on the last page rather than sent as null, as is `details` +when there is none. Starting a job -------------- @@ -80,20 +118,20 @@ so a job started this way stays distinguishable from one a hook forwarded. Both return: ```json -{ "status": "created", "job_id": "m2y5rj7ykdm9sfbr54hf", "tasks": 6 } +{ "ok": true, "data": { "status": "created", "job_id": "m2y5rj7ykdm9sfbr54hf" } } ``` -| Status | Meaning | -| ------ | ---------------------------------------------- | -| 200 | job created, or ignored for a branch deletion | -| 400 | no usable commit in the payload | -| 401 | signature missing or wrong | -| 404 | no such project | -| 409 | project disabled | -| 422 | the pipeline at that commit is invalid | +| Status | `error.name` | Meaning | +| ------ | -------------------------------------- | --------------------------------------------- | +| 200 | none | job created, or ignored for a branch deletion | +| 400 | `NoCommitInPayload`, `InvalidSha`, `ShaRequired`, `MalformedJson` | nothing usable to build | +| 401 | `InvalidSignature` | signature missing or wrong | +| 404 | `UnknownProject` | no such project | +| 409 | `ProjectDisabled` | project disabled | -A 422 carries `problems`, an array of `{path, message}`, so a broken -pipeline says which key is wrong rather than only that something is. +The job is recorded and the response returns immediately; the pipeline is +compiled afterwards. A pipeline that will not compile therefore surfaces as +the job going to `failed` with the reason on it, not as an error here. Reading a job back ------------------ @@ -106,15 +144,21 @@ Returns the job and the state of every task in it. ```json { - "job": { "id": "...", "number": 12, "state": "running", "ref": "refs/heads/main", - "head_sha": "...", "visibility": "private", "created_at": 1800000000000 }, - "tasks": [ - { "id": "...", "name": "build:arch=x86_64", "state": "success", "exit_code": 0, - "arch": "x86_64", "attempt": 1, "worker_name": "builder-1" } - ] + "ok": true, + "data": { + "job": { "id": "...", "number": 12, "state": "running", "ref": "refs/heads/main", + "head_sha": "...", "visibility": "private", "created_at": 1800000000000 }, + "tasks": [ + { "id": "...", "name": "build:arch=x86_64", "state": "success", "exit_code": 0, + "arch": "x86_64", "attempt": 1, "worker_name": "builder-1" } + ] + }, + "pagination": { "offset": 0, "total": 6 } } ``` +`pagination` describes `tasks`. + Three ways to be allowed: the job is public, the caller holds the project's trigger secret, or the caller is signed in and may manage the project. One credential therefore covers starting a build and watching it. @@ -145,6 +189,8 @@ empty body. ```json { + "ok": true, + "data": { "task": { "id": "m2y5t7deprrj9s6jvv4d", "name": "package:arch=x86_64,pkg=musl", "base_name": "package", "state": "success", "arch": "x86_64", @@ -162,11 +208,13 @@ empty body. "created_at": 1800000000000, "expires_at": null, "url": "https://ci.example.com/api/v1/projects/demo/jobs/<job>/tasks/<task>/artifacts/<id>" } ] + }, + "pagination": { "offset": 0, "total": 1 } } ``` -`artifact_paths` is what the pipeline told the task to collect; `artifacts` -is what it actually stored. Each carries a ready download `url`, which is a +`pagination` describes `artifacts`. `artifact_paths` is what the pipeline +told the task to collect; `artifacts` is what it actually stored. Each carries a ready download `url`, which is a convenience rather than the permission: that route checks visibility again for itself. @@ -204,6 +252,8 @@ of the surface is discovered rather than assembled: ```json { + "ok": true, + "data": { "task": { "id": "m2y5t7deprrj9s6jvv4d", "name": "build:arch=x86_64", @@ -222,6 +272,7 @@ of the surface is discovered rather than assembled: "done": "https://ci.example.com/api/v1/tasks/<id>/complete" } } + } } ``` @@ -231,20 +282,34 @@ strings the worker copies into the container without reading. Log appends send `Content-Type: application/octet-stream` with `X-Log-Offset` set to where the worker believes it is writing. An -overlapping chunk is trimmed and a gap is refused with 409 and -`expected_offset`, which makes a retry after a dropped connection safe. +overlapping chunk is trimmed, and a gap is refused with a 409 naming +`LogOffsetConflict` and carrying where to resume: + +```json +{ + "ok": false, + "error": { + "name": "LogOffsetConflict", + "message": "The log chunk does not continue from the end of the log", + "details": { "expected_offset": 4096 } + } +} +``` Artifact uploads send the bytes with `X-Artifact-Path` and a `Content-Length`. The path is sanitised, so traversal is stripped rather than honoured. Bodies are streamed and may be arbitrarily large; a body that does not match its declared length is rejected. +`complete` reports the outcome, and is sent a body rather than given one. +Requests are not enveloped; only responses are: + ```json { "success": true, "exit_code": 0, "error": null } ``` -`complete` reports the outcome. A failure with attempts remaining requeues -the task; otherwise everything downstream of it is skipped. +A failure with attempts remaining requeues the task; otherwise everything +downstream of it is skipped. Artifacts --------- diff --git a/docs/deployment.md b/docs/deployment.md @@ -274,9 +274,16 @@ CONDUCTOR_WORKER_IMAGE=finwo/conductor-worker:1.2.3 \ docker compose -f deploy/docker-compose.yml up -d ``` -Workers can be upgraded independently and in any order. The worker API is -the boundary between them, and a worker that goes away mid-task has that task -requeued after `scheduler.heartbeat_timeout`. +Workers are usually upgraded independently and in any order. The worker API +is the boundary between them, and a worker that goes away mid-task has that +task requeued after `scheduler.heartbeat_timeout`. + +Two releases break that, and need the conductor and every worker restarted +together: **0.5.0**, which put every API response in an envelope, and +**0.3.0**, which changed what a worker claims. Across either boundary a +worker of the older version cannot read what the conductor sends, so it +claims nothing until it is upgraded. Pull on every host first and restart +afterwards, so the window is seconds rather than the length of a download. Publishing ---------- diff --git a/package.json b/package.json @@ -1,6 +1,6 @@ { "name": "conductor", - "version": "0.4.4", + "version": "0.5.0", "description": "Stand-alone CI system: in-repo pipelines, containerised jobs, pull-based workers", "type": "module", "main": "src/conductor/index.js", @@ -15,6 +15,7 @@ "@fastify/cors": "^11.3.0", "fastify": "^5.12.5", "jose": "^6.2.12", + "serialize-error-cjs": "^0.3.0", "yaml": "^2.8.1" }, "optionalDependencies": { diff --git a/src/conductor/app.js b/src/conductor/app.js @@ -20,10 +20,8 @@ import { createAuth } from '../lib/auth/index.js'; import { createScheduler } from './scheduler.js'; import { createRetention } from './retention.js'; -import workerRoutes from './routes/workers.js'; -import taskRoutes from './routes/tasks.js'; -import jobRoutes from './routes/jobs.js'; -import artifactRoutes from './routes/artifacts.js'; +import apiRoutes from './routes/api.js'; +import { ok } from './routes/envelope.js'; import uiRoutes from './ui/routes.js'; import staticRoutes from '../lib/static.js'; @@ -74,8 +72,10 @@ export async function buildServer(services, options = {}) { await fastify.register(cors, { origin: true }); - fastify.get('/health', async () => ({ - ok: true, + // Enveloped like the rest of the API, so one reader handles every + // response the conductor gives. The container healthcheck reads the + // status line rather than the body, and is unaffected either way. + fastify.get('/health', async (req, reply) => ok(reply, { service: 'conductor', database: services.db.dialect, storage: services.storage.driver, @@ -85,14 +85,7 @@ export async function buildServer(services, options = {}) { // The only HTTP surface besides the interface: creating and reading jobs, // reading tasks back, the edge workers poll, and artifact downloads. // Everything else is the UI. - // - // workerRoutes and taskRoutes share the /api/v1/tasks prefix deliberately. - // They stay separate plugins because the worker token hook covers its own - // plugin entirely, and reading a task back is not behind it. - await fastify.register(jobRoutes, { ...services, prefix: '/api/v1' }); - await fastify.register(workerRoutes, { ...services, prefix: '/api/v1' }); - await fastify.register(taskRoutes, { ...services, prefix: '/api/v1' }); - await fastify.register(artifactRoutes, { ...services, prefix: '/api/v1' }); + await fastify.register(apiRoutes, { ...services, prefix: '/api/v1' }); // Signing in, managing projects and registering workers all happen in the // interface; there is no separate management API. diff --git a/src/conductor/routes/api.js b/src/conductor/routes/api.js @@ -0,0 +1,23 @@ +// src/conductor/routes/api.js - everything served under /api/v1 +// +// Fastify allows one not-found handler per prefix, so something has to own +// the scope these four share. workerRoutes and taskRoutes overlap on +// /api/v1/tasks deliberately: the worker token hook covers its own plugin +// entirely, and reading a task back is not behind it. + +import artifactRoutes from './artifacts.js'; +import jobRoutes from './jobs.js'; +import taskRoutes from './tasks.js'; +import workerRoutes from './workers.js'; +import { envelopeFailures } from './envelope.js'; + +// prefix is dropped rather than passed on: it applied when this plugin was +// registered, and handing it to the children would apply it a second time. +export default async function apiRoutes(fastify, { prefix, ...services }) { + envelopeFailures(fastify); + + await fastify.register(jobRoutes, services); + await fastify.register(workerRoutes, services); + await fastify.register(taskRoutes, services); + await fastify.register(artifactRoutes, services); +} diff --git a/src/conductor/routes/artifacts.js b/src/conductor/routes/artifacts.js @@ -10,6 +10,7 @@ // cannot reach anything. import { StorageNotFound } from '../../lib/storage/index.js'; +import { ArtifactNotStored, UnknownArtifact } from '../../lib/errors.js'; export default async function artifactRoutes(fastify, { db, storage, auth = null }) { async function currentUser(req) { @@ -51,7 +52,7 @@ export default async function artifactRoutes(fastify, { db, storage, auth = null ...visible.params, } ); - if (!artifact) return reply.code(404).send({ error: 'unknown artifact' }); + if (!artifact) throw new UnknownArtifact(); // Hand the client straight to the object store when that is possible. const url = await storage.presign(artifact.storage_key, { expires: 300 }); @@ -64,7 +65,7 @@ export default async function artifactRoutes(fastify, { db, storage, auth = null reply.header('content-disposition', `attachment; filename="${encodeURIComponent(basename(artifact.path))}"`); return reply.send(object.stream); } catch (e) { - if (e instanceof StorageNotFound) return reply.code(404).send({ error: 'artifact is no longer stored' }); + if (e instanceof StorageNotFound) throw new ArtifactNotStored(); throw e; } }); diff --git a/src/conductor/routes/envelope.js b/src/conductor/routes/envelope.js @@ -0,0 +1,54 @@ +// src/conductor/routes/envelope.js - the shape every API response takes +// +// { "ok": true, "data": ..., "pagination": { ... } } +// { "ok": false, "error": { "name": ..., "message": ..., "details": ... } } +// +// Optional keys are left out rather than sent as null, so an absent key +// cannot be mistaken for a meaningful one. + +import { serializeError } from 'serialize-error-cjs'; +import { ApiError, BadRequest, InternalError, NotFound } from '../../lib/errors.js'; + +export function ok(reply, data, pagination = null) { + const body = { ok: true, data }; + if (pagination) body.pagination = pagination; + return reply.send(body); +} + +// Installed on the API plugin alone, so a mistyped page url still reaches +// the interface and gets HTML rather than being answered in JSON. +export function envelopeFailures(fastify) { + fastify.setNotFoundHandler((req, reply) => fail(req, reply, new NotFound())); + fastify.setErrorHandler((e, req, reply) => fail(req, reply, e)); +} + +function fail(req, reply, e) { + const known = asApiError(e); + + // A fault is the server's to explain. The error goes to the log under the + // request id and the response carries only that id: a stack would hand out + // paths and versions, and CORS here is open to any origin. + if (known.code >= 500) { + req.log.error({ err: serializeError(e), request_id: req.id }, 'request failed'); + return send(reply, new InternalError(undefined, { details: { request_id: req.id } })); + } + + return send(reply, known); +} + +function send(reply, e) { + const error = { name: e.name, message: e.message }; + if (e.details !== undefined) error.details = e.details; + return reply.code(e.code).send({ ok: false, error }); +} + +// Fastify's own failures carry a numeric statusCode; their code is a string +// like FST_ERR_CTP_EMPTY_JSON_BODY and is not what we mean by one. +function asApiError(e) { + if (e instanceof ApiError) return e; + + const status = Number.isInteger(e?.statusCode) ? e.statusCode : 500; + if (status >= 400 && status < 500) return new BadRequest(e.message, { code: status }); + + return new InternalError(); +} diff --git a/src/conductor/routes/jobs.js b/src/conductor/routes/jobs.js @@ -21,10 +21,18 @@ import { SHA_PATTERN } from '../../lib/git.js'; import { canManageProject } from '../../lib/projects.js'; import { verifySignature } from '../../lib/signature.js'; +import { ok } from './envelope.js'; +import { + InvalidSha, InvalidSignature, MalformedJson, NoCommitInPayload, + ProjectDisabled, ShaRequired, UnknownJob, UnknownProject, +} from '../../lib/errors.js'; +import { limitOf, offsetOfQuery, pageBlock, paginateOffset } from '../../lib/page.js'; const ZERO_SHA = '0'.repeat(40); -export default async function jobRoutes(fastify, { auth, db, projects, scheduler, logger }) { +export default async function jobRoutes(fastify, { cfg, auth, db, projects, scheduler, logger }) { + const base = cfg.server.public_url.replace(/\/+$/, ''); + // HMAC is computed over the exact bytes received, so the raw body has to // survive JSON parsing. fastify.addContentTypeParser('application/json', { parseAs: 'buffer' }, (req, body, done) => { @@ -32,31 +40,20 @@ export default async function jobRoutes(fastify, { auth, db, projects, scheduler if (body.length === 0) return done(null, {}); try { done(null, JSON.parse(body.toString('utf8'))); - } catch (e) { - e.statusCode = 400; - done(e); + } catch { + done(new MalformedJson()); } }); // Shared by both creation routes: resolve the project, check the secret. - // Returns null once it has already answered. - async function authorizedProject(req, reply) { + async function authorizedProject(req) { const project = await projects.get(req.params.project); - if (!project) { - reply.code(404).send({ error: 'unknown project' }); - return null; - } - if (project.enabled !== 1) { - reply.code(409).send({ error: 'project is disabled' }); - return null; - } + if (!project) throw new UnknownProject(); + if (project.enabled !== 1) throw new ProjectDisabled(); const secret = projects.triggerSecret(project); if (secret) { - if (!verifySignature(req, secret)) { - reply.code(401).send({ error: 'invalid or missing signature' }); - return null; - } + if (!verifySignature(req, secret)) throw new InvalidSignature(); } else { logger.warn?.(`project ${project.id} accepted an unsigned request; set a trigger secret`); } @@ -65,54 +62,49 @@ export default async function jobRoutes(fastify, { auth, db, projects, scheduler // Records the job as pending and returns immediately. Pipeline compilation // runs in the background; errors surface as the job transitioning to 'failed'. - async function create(reply, req, project, { ref, base, sha, actor, trigger }) { + async function create(reply, project, { ref, base: baseSha, sha, actor, trigger }) { const { jobId } = await scheduler.createJob(project, { ref, - baseSha: base, + baseSha, headSha: sha, trigger, actor, }); - return reply.send({ status: 'created', job_id: jobId }); + return ok(reply, { status: 'created', job_id: jobId }); } fastify.post('/projects/:project/trigger', async (req, reply) => { - const project = await authorizedProject(req, reply); - if (!project) return reply; + const project = await authorizedProject(req); const push = normalizePush(req.body); - if (!push) { - return reply.code(400).send({ - error: 'could not read a commit from the payload; expected sha, or a GitHub, Gitea or GitLab push event', - }); - } + if (!push) throw new NoCommitInPayload(); + // A branch deletion is not a thing to build, and saying so is more // useful than refusing it. if (push.sha === ZERO_SHA) { - return reply.send({ status: 'ignored', reason: 'branch deletion' }); + return ok(reply, { status: 'ignored', reason: 'branch deletion' }); } if (!SHA_PATTERN.test(push.sha)) { - return reply.code(400).send({ error: `not a full commit id: ${JSON.stringify(push.sha)}` }); + throw new InvalidSha(`Not a full commit id: ${JSON.stringify(push.sha)}`); } - return create(reply, req, project, { ...push, trigger: 'push' }); + return create(reply, project, { ...push, trigger: 'push' }); }); // The deliberate form. Same authentication, no payload archaeology: a // caller that means to start a build says so, and a missing or malformed // commit is an error rather than something to infer. fastify.post('/projects/:project/jobs', async (req, reply) => { - const project = await authorizedProject(req, reply); - if (!project) return reply; + const project = await authorizedProject(req); const body = req.body ?? {}; const sha = typeof body.sha === 'string' ? body.sha : null; - if (!sha) return reply.code(400).send({ error: 'sha is required' }); + if (!sha) throw new ShaRequired(); if (!SHA_PATTERN.test(sha)) { - return reply.code(400).send({ error: `not a full commit id: ${JSON.stringify(sha)}` }); + throw new InvalidSha(`Not a full commit id: ${JSON.stringify(sha)}`); } - return create(reply, req, project, { + return create(reply, project, { sha, base: usableSha(body.base), ref: typeof body.ref === 'string' ? body.ref : null, @@ -130,7 +122,7 @@ export default async function jobRoutes(fastify, { auth, db, projects, scheduler // which jobs exist. fastify.get('/projects/:project/jobs/:job', async (req, reply) => { const project = await projects.get(req.params.project); - if (!project) return reply.code(404).send({ error: 'unknown job' }); + if (!project) throw new UnknownJob(); const job = await db.get( `SELECT id, project_id, number, ref, head_sha, trigger_type, actor, title, @@ -139,22 +131,24 @@ export default async function jobRoutes(fastify, { auth, db, projects, scheduler WHERE id = {id} AND project_id = {project}`, { id: req.params.job, project: project.id } ); - if (!job) return reply.code(404).send({ error: 'unknown job' }); + if (!job) throw new UnknownJob(); if (job.visibility !== 'public' && !(await mayRead(req, project))) { - return reply.code(404).send({ error: 'unknown job' }); + throw new UnknownJob(); } - const tasks = await db.all( - `SELECT id, name, state, arch, attempt, exit_code, error, - worker_name, created_at, started_at, finished_at - FROM tasks - WHERE job_id = {job} - ORDER BY created_at, name`, - { job: job.id } - ); + const window = await paginateOffset(db, { + select: `id, name, state, arch, attempt, exit_code, error, + worker_name, created_at, started_at, finished_at`, + from: 'tasks', + where: 'job_id = {job}', + order: 'created_at, name', + params: { job: job.id }, + offset: offsetOfQuery(req.query), + limit: limitOf(req.query), + }); - return reply.send({ job, tasks }); + return ok(reply, { job, tasks: window.rows }, pageBlock({ ...window, url: base + req.url })); }); async function mayRead(req, project) { diff --git a/src/conductor/routes/tasks.js b/src/conductor/routes/tasks.js @@ -31,6 +31,9 @@ import { canManageProject } from '../../lib/projects.js'; import { verifySignature } from '../../lib/signature.js'; +import { ok } from './envelope.js'; +import { UnknownTask } from '../../lib/errors.js'; +import { limitOf, offsetOfQuery, pageBlock, paginateOffset } from '../../lib/page.js'; export default async function taskRoutes(fastify, { cfg, db, projects, auth = null }) { const base = cfg.server.public_url.replace(/\/+$/, ''); @@ -59,16 +62,20 @@ export default async function taskRoutes(fastify, { cfg, db, projects, auth = nu // Everything a caller is allowed to see, assembled once so the two routes // cannot drift into answering differently. - async function represent(reply, row) { + async function represent(req, reply, row) { const spec = readSpec(row.spec); - const artifacts = await db.all( - `SELECT id, path, size, sha256, created_at, expires_at - FROM artifacts WHERE task_id = {task} ORDER BY path`, - { task: row.id } - ); + const artifacts = await paginateOffset(db, { + select: 'id, path, size, sha256, created_at, expires_at', + from: 'artifacts', + where: 'task_id = {task}', + order: 'path', + params: { task: row.id }, + offset: offsetOfQuery(req.query), + limit: limitOf(req.query), + }); - return reply.send({ + return ok(reply, { task: { id: row.id, name: row.name, @@ -102,7 +109,7 @@ export default async function taskRoutes(fastify, { cfg, db, projects, auth = nu sha: row.head_sha, visibility: row.visibility, }, - artifacts: artifacts.map((a) => ({ + artifacts: artifacts.rows.map((a) => ({ id: a.id, path: a.path, size: a.size, @@ -114,7 +121,7 @@ export default async function taskRoutes(fastify, { cfg, db, projects, auth = nu url: `${base}/api/v1/projects/${encodeURIComponent(row.project_id)}/jobs/${row.job_id}` + `/tasks/${row.id}/artifacts/${a.id}`, })), - }); + }, pageBlock({ ...artifacts, url: base + req.url })); } // A task id is unique by itself, so the project need not be named. Without @@ -122,16 +129,14 @@ export default async function taskRoutes(fastify, { cfg, db, projects, auth = nu // or a session that may manage the project. fastify.get('/tasks/:task', async (req, reply) => { const row = await db.get(`${TASK_QUERY} WHERE t.id = {id}`, { id: req.params.task }); - if (!row) return reply.code(404).send({ error: 'unknown task' }); + if (!row) throw new UnknownTask(); if (row.visibility !== 'public') { const project = await projects.get(row.project_id); - if (!canManageProject(await currentUser(req), project)) { - return reply.code(404).send({ error: 'unknown task' }); - } + if (!canManageProject(await currentUser(req), project)) throw new UnknownTask(); } - return represent(reply, row); + return represent(req, reply, row); }); // The same task named in full. The project in the path must be the one the @@ -139,19 +144,17 @@ export default async function taskRoutes(fastify, { cfg, db, projects, auth = nu // happens to be readable. fastify.get('/projects/:project/tasks/:task', async (req, reply) => { const project = await projects.get(req.params.project); - if (!project) return reply.code(404).send({ error: 'unknown task' }); + if (!project) throw new UnknownTask(); const row = await db.get( `${TASK_QUERY} WHERE t.id = {id} AND j.project_id = {project}`, { id: req.params.task, project: project.id } ); - if (!row) return reply.code(404).send({ error: 'unknown task' }); + if (!row) throw new UnknownTask(); - if (row.visibility !== 'public' && !(await mayRead(req, project))) { - return reply.code(404).send({ error: 'unknown task' }); - } + if (row.visibility !== 'public' && !(await mayRead(req, project))) throw new UnknownTask(); - return represent(reply, row); + return represent(req, reply, row); }); async function mayRead(req, project) { diff --git a/src/conductor/routes/workers.js b/src/conductor/routes/workers.js @@ -22,29 +22,38 @@ import { hashingTransform } from '../../lib/stream.js'; import { sanitizeRelativePath, keys as storageKeys } from '../../lib/storage/index.js'; import { maskBuffer } from '../../lib/variables.js'; import { newArtifactId } from '../../lib/ids.js'; +import { ok } from './envelope.js'; +import { + ArtifactSizeMismatch, ContentLengthRequired, InvalidArtifactPath, InvalidLogOffset, + InvalidWorkerToken, LogChunkTooLarge, LogOffsetConflict, TaskNotHeld, TaskNotRunning, + UnknownProject, UnknownTask, +} from '../../lib/errors.js'; // Log chunks are small and frequent; artifacts are streamed, so this only // bounds a single log append. const LOG_CHUNK_LIMIT = 1024 * 1024; +const COMPLETE_FAILURES = { + 'unknown-task': UnknownTask, + 'not-held': TaskNotHeld, + 'not-running': TaskNotRunning, +}; + export default async function workerRoutes(fastify, { cfg, db, git, logs, storage, projects, scheduler, workerTokens, variables }) { // Raw bodies: log chunks and artifacts arrive as octet streams and must // not be parsed. fastify.addContentTypeParser('application/octet-stream', (req, payload, done) => done(null, payload)); - async function requireWorker(req, reply) { + async function requireWorker(req) { const header = req.headers.authorization || ''; const presented = header.startsWith('Bearer ') ? header.slice(7).trim() : ''; const token = await workerTokens.verify(presented, { ip: req.ip }); - if (!token) { - reply.code(401).send({ error: 'invalid or missing worker token' }); - return; - } + if (!token) throw new InvalidWorkerToken(); req.worker = token; } // Confirms the task exists and is held by the calling worker. - async function heldTask(req, reply) { + async function heldTask(req) { const task = await db.get( `SELECT t.id, t.job_id, t.name, t.state, t.worker_token_id, t.log_size, t.spec, j.project_id, j.head_sha @@ -52,14 +61,8 @@ export default async function workerRoutes(fastify, { cfg, db, git, logs, storag WHERE t.id = {id}`, { id: req.params.task } ); - if (!task) { - reply.code(404).send({ error: 'unknown task' }); - return null; - } - if (task.worker_token_id !== req.worker.id) { - reply.code(403).send({ error: 'task is not held by this worker' }); - return null; - } + if (!task) throw new UnknownTask(); + if (task.worker_token_id !== req.worker.id) throw new TaskNotHeld(); return task; } @@ -86,7 +89,7 @@ export default async function workerRoutes(fastify, { cfg, db, git, logs, storag const base = cfg.server.public_url.replace(/\/+$/, ''); const at = (suffix) => `${base}/api/v1/tasks/${task.id}/${suffix}`; - return reply.send({ + return ok(reply, { task: { id: task.id, name: task.name, @@ -128,11 +131,10 @@ export default async function workerRoutes(fastify, { cfg, db, git, logs, storag // The tree at the task's commit, as a gzipped tar. fastify.get('/tasks/:task/source.tar.gz', async (req, reply) => { - const task = await heldTask(req, reply); - if (!task) return reply; + const task = await heldTask(req); const project = await projects.get(task.project_id); - if (!project) return reply.code(404).send({ error: 'unknown project' }); + if (!project) throw new UnknownProject(); // The tree rooted at the archive, with no wrapping directory: a // worker extracts it at whatever path the task is to run in, and @@ -145,22 +147,19 @@ export default async function workerRoutes(fastify, { cfg, db, git, logs, storag // Append to the live log. X-Log-Offset makes a retry after a dropped // connection safe. fastify.post('/tasks/:task/log', { bodyLimit: LOG_CHUNK_LIMIT }, async (req, reply) => { - const task = await heldTask(req, reply); - if (!task) return reply; + const task = await heldTask(req); const chunks = []; let total = 0; for await (const chunk of req.body) { total += chunk.length; - if (total > LOG_CHUNK_LIMIT) return reply.code(413).send({ error: 'log chunk too large' }); + if (total > LOG_CHUNK_LIMIT) throw new LogChunkTooLarge(); chunks.push(chunk); } const rawOffset = req.headers['x-log-offset']; const offset = rawOffset === undefined ? undefined : Number(rawOffset); - if (offset !== undefined && !Number.isInteger(offset)) { - return reply.code(400).send({ error: 'x-log-offset must be an integer' }); - } + if (offset !== undefined && !Number.isInteger(offset)) throw new InvalidLogOffset(); // The worker masks before sending; this repeats it on ingest so a // worker that does not still cannot write a secret to disk. It works @@ -178,11 +177,11 @@ export default async function workerRoutes(fastify, { cfg, db, git, logs, storag const result = await logs.append(task.job_id, task.id, payload, offset); await db.run('UPDATE tasks SET log_size = {size}, heartbeat_at = {now} WHERE id = {id}', { id: task.id, size: result.size, now: Date.now() }); - return reply.send(result); + return ok(reply, result); } catch (e) { + // Tell the worker where to resume from. if (e instanceof LogOffsetError) { - // Tell the worker where to resume from. - return reply.code(409).send({ error: e.message, expected_offset: e.expected }); + throw new LogOffsetConflict(e.message, { details: { expected_offset: e.expected } }); } throw e; } @@ -207,16 +206,13 @@ export default async function workerRoutes(fastify, { cfg, db, git, logs, storag // Upload one artifact. The path is worker supplied, so it is sanitized // before it can influence a storage key. fastify.post('/tasks/:task/artifacts', async (req, reply) => { - const task = await heldTask(req, reply); - if (!task) return reply; + const task = await heldTask(req); const relPath = sanitizeRelativePath(req.headers['x-artifact-path']); - if (!relPath) return reply.code(400).send({ error: 'x-artifact-path is missing or unusable' }); + if (!relPath) throw new InvalidArtifactPath(); const declared = Number(req.headers['content-length']); - if (!Number.isInteger(declared) || declared < 0) { - return reply.code(411).send({ error: 'content-length is required for artifact uploads' }); - } + if (!Number.isInteger(declared) || declared < 0) throw new ContentLengthRequired(); // Hash on the way past, so the digest costs nothing extra even when the // backend cannot report one. @@ -232,9 +228,7 @@ export default async function workerRoutes(fastify, { cfg, db, git, logs, storag } catch (e) { // A body shorter or longer than content-length is the worker's // mistake, not a server fault. - if (/size mismatch/.test(e.message)) { - return reply.code(400).send({ error: e.message }); - } + if (/size mismatch/.test(e.message)) throw new ArtifactSizeMismatch(e.message); throw e; } @@ -259,21 +253,20 @@ export default async function workerRoutes(fastify, { cfg, db, git, logs, storag } ); - return reply.send({ path: relPath, size: stored.size ?? declared, sha256: digest }); + return ok(reply, { path: relPath, size: stored.size ?? declared, sha256: digest }); }); // Keeps the task alive, and is how a worker learns it should stop. fastify.post('/tasks/:task/heartbeat', async (req, reply) => { const result = await scheduler.heartbeat(req.params.task, req.worker.id); - if (!result.known) return reply.code(404).send({ error: 'unknown task for this worker' }); - return reply.send({ cancelled: result.cancelled, state: result.state }); + if (!result.known) throw new UnknownTask('No such task for this worker'); + return ok(reply, { cancelled: result.cancelled, state: result.state }); }); // Report the outcome. The log is copied to storage here, whatever the // result, so a failed task keeps its output. fastify.post('/tasks/:task/complete', async (req, reply) => { - const task = await heldTask(req, reply); - if (!task) return reply; + const task = await heldTask(req); const body = req.body ?? {}; const success = body.success === true; @@ -286,7 +279,12 @@ export default async function workerRoutes(fastify, { cfg, db, git, logs, storag error, tokenId: req.worker.id, }); - if (!outcome.ok) return reply.code(409).send({ error: outcome.reason }); + // All 409: by this point heldTask has already answered for a task that + // does not exist or is not ours, so these are races rather than mistakes. + if (!outcome.ok) { + const Failure = COMPLETE_FAILURES[outcome.cause] ?? TaskNotRunning; + throw new Failure(outcome.reason, { code: 409 }); + } // A retry keeps writing to the same log, so only archive it once the // task has actually stopped. @@ -298,7 +296,7 @@ export default async function workerRoutes(fastify, { cfg, db, git, logs, storag } } - return reply.send({ + return ok(reply, { state: outcome.state, retry: outcome.retry === true, skipped: outcome.skipped ?? [], diff --git a/src/conductor/scheduler.js b/src/conductor/scheduler.js @@ -410,12 +410,14 @@ export function createScheduler({ cfg, db, git, logs, storage, projects, variabl FROM tasks WHERE id = {id}`, { id: taskId } ); - if (!task) return { ok: false, reason: 'unknown task' }; + // cause is the stable half of this: reason is prose and may be + // reworded, so nothing downstream should be reading it to branch on. + if (!task) return { ok: false, cause: 'unknown-task', reason: 'unknown task' }; if (tokenId !== null && task.worker_token_id !== tokenId) { - return { ok: false, reason: 'task belongs to another worker' }; + return { ok: false, cause: 'not-held', reason: 'task belongs to another worker' }; } if (task.state !== 'running') { - return { ok: false, reason: `task is ${task.state}, not running` }; + return { ok: false, cause: 'not-running', reason: `task is ${task.state}, not running` }; } const now = Date.now(); diff --git a/src/lib/errors.js b/src/lib/errors.js @@ -0,0 +1,144 @@ +// src/lib/errors.js - errors that describe themselves over HTTP +// +// The class name is the identifier on the wire, and code is the HTTP status: +// serialize-error-cjs carries both, where a property of our own would be lost. +// Statics rather than fields, since a subclass field would clobber an override. + +export class ApiError extends Error { + static code = 500; + static defaultMessage = 'The request could not be completed'; + + constructor(message, { code, details } = {}) { + super(message ?? new.target.defaultMessage); + this.name = new.target.name; + this.code = code ?? new.target.code; + if (details !== undefined) this.details = details; + } +} + +// Carries details.request_id, tying a report of "it broke" to the log line. +export class InternalError extends ApiError { + static code = 500; +} + +// For a 4xx nobody threw deliberately, such as a body Fastify itself refused. +export class BadRequest extends ApiError { + static code = 400; + static defaultMessage = 'The request could not be understood'; +} + +// --- not there, or not yours: both answer 404, so neither can be probed --- + +export class UnknownProject extends ApiError { + static code = 404; + static defaultMessage = 'No such project'; +} + +export class UnknownJob extends ApiError { + static code = 404; + static defaultMessage = 'No such job'; +} + +export class UnknownTask extends ApiError { + static code = 404; + static defaultMessage = 'No such task'; +} + +export class UnknownArtifact extends ApiError { + static code = 404; + static defaultMessage = 'No such artifact'; +} + +export class ArtifactNotStored extends ApiError { + static code = 404; + static defaultMessage = 'That artifact is no longer stored'; +} + +export class NotFound extends ApiError { + static code = 404; + static defaultMessage = 'No such endpoint'; +} + +// --- refused --- + +export class InvalidSignature extends ApiError { + static code = 401; + static defaultMessage = 'The request signature is missing or wrong'; +} + +export class InvalidWorkerToken extends ApiError { + static code = 401; + static defaultMessage = 'The worker token is missing or wrong'; +} + +// 403 reaching for a task it never held, 409 losing one mid-report. +export class TaskNotHeld extends ApiError { + static code = 403; + static defaultMessage = 'That task is not held by this worker'; +} + +// --- not actionable as it stands --- + +export class ProjectDisabled extends ApiError { + static code = 409; + static defaultMessage = 'That project is disabled'; +} + +export class TaskNotRunning extends ApiError { + static code = 409; + static defaultMessage = 'That task is not running'; +} + +// Carries details.expected_offset, which is what makes a resume safe. +export class LogOffsetConflict extends ApiError { + static code = 409; + static defaultMessage = 'The log chunk does not continue from the end of the log'; +} + +// --- malformed --- + +export class MalformedJson extends ApiError { + static code = 400; + static defaultMessage = 'The request body is not valid JSON'; +} + +export class NoCommitInPayload extends ApiError { + static code = 400; + static defaultMessage = + 'Could not read a commit from the payload; expected sha, or a GitHub, Gitea or GitLab push event'; +} + +export class InvalidSha extends ApiError { + static code = 400; + static defaultMessage = 'That is not a full commit id'; +} + +export class ShaRequired extends ApiError { + static code = 400; + static defaultMessage = 'sha is required'; +} + +export class InvalidLogOffset extends ApiError { + static code = 400; + static defaultMessage = 'x-log-offset must be an integer'; +} + +export class InvalidArtifactPath extends ApiError { + static code = 400; + static defaultMessage = 'x-artifact-path is missing or unusable'; +} + +export class ArtifactSizeMismatch extends ApiError { + static code = 400; + static defaultMessage = 'The artifact body does not match its declared length'; +} + +export class ContentLengthRequired extends ApiError { + static code = 411; + static defaultMessage = 'content-length is required for artifact uploads'; +} + +export class LogChunkTooLarge extends ApiError { + static code = 413; + static defaultMessage = 'That log chunk is too large'; +} diff --git a/src/lib/page.js b/src/lib/page.js @@ -62,3 +62,61 @@ export function windowOfAll(all, page, perPage = PER_PAGE) { const from = offsetOf(page, perPage); return windowOf({ rows: all.slice(from, from + perPage), total: all.length, page, perPage }); } + +// --- offset windows, for the API --- +// +// The interface pages by number because a person reads "page 3 of 7". A +// program asks from an offset and follows the url it is handed. + +export const API_LIMIT = 50; +export const API_MAX_LIMIT = 200; + +// Anything unusable reads as the default, and the cap applies silently: a +// client asking for everything gets a page, not an error to learn about. +export function limitOf(query, fallback = API_LIMIT, max = API_MAX_LIMIT) { + const n = Number.parseInt(typeof query?.limit === 'string' ? query.limit : '', 10); + if (!Number.isFinite(n) || n < 1) return fallback; + return Math.min(n, max); +} + +export function offsetOfQuery(query, max = API_MAX_LIMIT) { + const n = Number.parseInt(typeof query?.offset === 'string' ? query.offset : '', 10); + if (!Number.isFinite(n) || n < 0) return 0; + // Far enough out that offset + limit stays a safe integer. + return Math.min(n, Number.MAX_SAFE_INTEGER - max); +} + +// next is absent on the last page, so following pages needs no special case. +export function pageBlock({ url, offset, limit, total }) { + const block = {}; + + const after = offset + limit; + if (after < total) { + const next = new URL(url); + next.searchParams.set('offset', String(after)); + next.searchParams.set('limit', String(limit)); + block.next = next.toString(); + } + + block.offset = offset; + block.total = total; + return block; +} + +// The offset twin of paginate, sharing from and where for the same reason. +export async function paginateOffset(db, { + select, from, where = '1 = 1', order, params = {}, offset, limit, +}) { + const rows = await db.all( + `SELECT ${select} FROM ${from} WHERE ${where} ORDER BY ${order} + LIMIT {pageSize} OFFSET {pageFrom}`, + { ...params, pageSize: limit, pageFrom: offset } + ); + + const counted = await db.get( + `SELECT COUNT(*) AS total FROM ${from} WHERE ${where}`, + params + ); + + return { rows, total: Number(counted?.total ?? 0), offset, limit }; +} diff --git a/src/worker/client.js b/src/worker/client.js @@ -6,11 +6,13 @@ // append want very different handling. export class ConductorError extends Error { - constructor(message, { status = 0, body = null } = {}) { + constructor(message, { status = 0, body = null, cause = null } = {}) { super(message); this.name = 'ConductorError'; this.status = status; this.body = body; + // The conductor's own name for what went wrong, where it sent one. + this.cause = cause; } } @@ -18,6 +20,13 @@ export function createClient(cfg) { const base = cfg.conductor_url; const auth = { authorization: `Bearer ${cfg.token}` }; + // The envelope is unwrapped here and nowhere else, so the rest of the + // worker keeps dealing in plain task, log and outcome objects. + async function data(res) { + const body = await res.json(); + return body?.data; + } + async function readError(res, action) { let body = null; let detail = ''; @@ -28,9 +37,11 @@ export function createClient(cfg) { } catch { // A non JSON body is still worth reporting as text. } + const error = body?.error; return new ConductorError(`${action} failed: ${res.status} ${res.statusText} ${detail}`.trim(), { status: res.status, body, + cause: error?.name ?? null, }); } @@ -45,7 +56,7 @@ export function createClient(cfg) { }); if (res.status === 204) return null; if (!res.ok) throw await readError(res, 'claim'); - return (await res.json()).task; + return (await data(res)).task; }, // The tree at the task's commit. Returned as a web stream so it can be @@ -67,10 +78,10 @@ export function createClient(cfg) { if (res.status === 409) { const body = await res.json().catch(() => ({})); - return { conflict: true, expected: body.expected_offset ?? 0 }; + return { conflict: true, expected: body.error?.details?.expected_offset ?? 0 }; } if (!res.ok) throw await readError(res, 'log append'); - return await res.json(); + return await data(res); }, async uploadArtifact(url, relPath, body, size) { @@ -86,7 +97,7 @@ export function createClient(cfg) { duplex: 'half', }); if (!res.ok) throw await readError(res, `artifact upload (${relPath})`); - return await res.json(); + return await data(res); }, // Reports that the task is alive, and learns whether it should stop. @@ -98,7 +109,7 @@ export function createClient(cfg) { }); if (res.status === 404) return { cancelled: true, gone: true }; if (!res.ok) throw await readError(res, 'heartbeat'); - return await res.json(); + return await data(res); }, async done(url, { success, exitCode, error }) { @@ -108,7 +119,7 @@ export function createClient(cfg) { body: JSON.stringify({ success, exit_code: exitCode ?? null, error: error ?? null }), }); if (!res.ok) throw await readError(res, 'done'); - return await res.json(); + return await data(res); }, }; } diff --git a/test/api.test.js b/test/api.test.js @@ -0,0 +1,233 @@ +// test/api.test.js - the response envelope and its pagination +// +// The error names asserted here are the wire contract. They come from the +// class names in lib/errors.js, so a rename has to fail something rather +// than quietly change what clients read. + +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { startHarness, data, apiError } from './helpers/harness.js'; +import { pageBlock, limitOf, offsetOfQuery, API_LIMIT, API_MAX_LIMIT } from '../src/lib/page.js'; + +async function withApi(options, fn) { + const h = await startHarness(options); + try { + await fn(h); + } finally { + await h.stop(); + } +} + +// --- the envelope --- + +test('a success carries ok and data, and nothing else', async () => { + await withApi({ visibility: 'public' }, async (h) => { + const res = await h.app.inject({ method: 'GET', url: '/health' }); + + assert.equal(res.statusCode, 200); + assert.deepEqual(Object.keys(res.json()).sort(), ['data', 'ok']); + assert.equal(res.json().ok, true); + }); +}); + +test('a failure carries a name, a message and the matching status', async () => { + await withApi({ visibility: 'public' }, async (h) => { + const res = await h.app.inject({ method: 'GET', url: '/api/v1/tasks/m2y0000000000000nope' }); + + assert.equal(res.statusCode, 404); + const error = apiError(res); + assert.equal(error.name, 'UnknownTask'); + assert.equal(typeof error.message, 'string'); + assert.ok(error.message.length > 0, 'a message is for a person to read'); + }); +}); + +test('details are absent rather than null when there are none', async () => { + await withApi({ visibility: 'public' }, async (h) => { + const res = await h.app.inject({ method: 'GET', url: '/api/v1/tasks/m2y0000000000000nope' }); + + assert.deepEqual(Object.keys(apiError(res)).sort(), ['message', 'name']); + }); +}); + +test('a url under the api that matches no route is enveloped too', async () => { + await withApi({ visibility: 'public' }, async (h) => { + const res = await h.app.inject({ method: 'GET', url: '/api/v1/no/such/thing' }); + + assert.equal(res.statusCode, 404); + assert.equal(apiError(res).name, 'NotFound'); + }); +}); + +// The handlers are installed on the API plugin alone. Were they installed at +// the root they would answer for the interface as well, and a mistyped page +// url would return JSON to somebody holding a browser. +test('a url outside the api is left to the interface', async () => { + await withApi({ visibility: 'public' }, async (h) => { + const res = await h.app.inject({ method: 'GET', url: '/no/such/page' }); + + assert.ok(!res.body.includes('"ok":false'), 'the interface must not answer in the api envelope'); + }); +}); + +test('a malformed body is refused by name', async () => { + await withApi({ visibility: 'public' }, async (h) => { + const res = await h.app.inject({ + method: 'POST', + url: '/api/v1/projects/demo/jobs', + headers: { 'content-type': 'application/json' }, + payload: '{"sha": not json}', + }); + + assert.equal(res.statusCode, 400); + assert.equal(apiError(res).name, 'MalformedJson'); + }); +}); + +test('the worker protocol reports its refusals by name', async () => { + await withApi({ visibility: 'public' }, async (h) => { + const res = await h.app.inject({ + method: 'POST', + url: '/api/v1/tasks/claim', + headers: { authorization: 'Bearer nonsense', 'content-type': 'application/json' }, + payload: '{}', + }); + + assert.equal(res.statusCode, 401); + assert.equal(apiError(res).name, 'InvalidWorkerToken'); + }); +}); + +// A stack names files, versions and the shape of the tree, and CORS here is +// open to any origin, so it belongs in the log and nowhere else. +test('no error body carries a stack', async () => { + await withApi({ visibility: 'public' }, async (h) => { + for (const url of ['/api/v1/tasks/m2y0000000000000nope', '/api/v1/no/such/thing']) { + const res = await h.app.inject({ method: 'GET', url }); + assert.ok(!/\bstack\b/i.test(res.body), `${url} leaked a stack`); + assert.ok(!res.body.includes('.js:'), `${url} leaked a source location`); + } + }); +}); + +// --- pagination --- + +test('a page reports where it sits and how many there are', async () => { + await withApi({ visibility: 'public' }, async (h) => { + const job = data(await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).job_id; + await h.waitForJob(job); + + const res = await h.app.inject({ method: 'GET', url: `/api/v1/projects/demo/jobs/${job}?limit=2` }); + assert.equal(res.statusCode, 200, res.body); + + const body = res.json(); + assert.equal(body.data.tasks.length, 2, 'the limit is honoured'); + assert.equal(body.pagination.offset, 0); + assert.ok(body.pagination.total > 2, 'the total counts past the page'); + assert.match(body.pagination.next, /offset=2/); + assert.match(body.pagination.next, /limit=2/); + }); +}); + +test('following next walks the whole collection exactly once', async () => { + await withApi({ visibility: 'public' }, async (h) => { + const job = data(await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).job_id; + await h.waitForJob(job); + + const seen = []; + let url = `/api/v1/projects/demo/jobs/${job}?limit=2`; + for (let i = 0; i < 20 && url; i += 1) { + const res = await h.app.inject({ method: 'GET', url }); + const body = res.json(); + seen.push(...body.data.tasks.map((t) => t.id)); + // next is absolute, and inject wants a path. + url = body.pagination.next ? new URL(body.pagination.next).pathname + + new URL(body.pagination.next).search : null; + } + + const total = seen.length; + assert.ok(total > 2, 'the walk should have needed more than one page'); + assert.equal(new Set(seen).size, total, 'no task appeared on two pages'); + }); +}); + +test('next is absent on the last page', async () => { + await withApi({ visibility: 'public' }, async (h) => { + const job = data(await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).job_id; + await h.waitForJob(job); + + const res = await h.app.inject({ method: 'GET', url: `/api/v1/projects/demo/jobs/${job}` }); + const { pagination } = res.json(); + + assert.equal(pagination.offset, 0); + assert.ok(!('next' in pagination), 'a single page has nothing after it'); + }); +}); + +test('an unusable offset or limit falls back rather than failing', async () => { + await withApi({ visibility: 'public' }, async (h) => { + const job = data(await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).job_id; + await h.waitForJob(job); + + const res = await h.app.inject({ + method: 'GET', + url: `/api/v1/projects/demo/jobs/${job}?offset=-5&limit=banana`, + }); + + assert.equal(res.statusCode, 200, res.body); + assert.equal(res.json().pagination.offset, 0); + }); +}); + +test('a task reports its artifacts as a page', async () => { + await withApi({ visibility: 'public' }, async (h) => { + const job = data(await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).job_id; + await h.waitForJob(job); + const task = data(await h.claim({})).task; + + const res = await h.app.inject({ method: 'GET', url: `/api/v1/tasks/${task.id}` }); + assert.equal(res.statusCode, 200, res.body); + + const body = res.json(); + assert.ok(Array.isArray(body.data.artifacts)); + assert.equal(body.pagination.offset, 0); + assert.equal(body.pagination.total, body.data.artifacts.length); + }); +}); + +// --- the helpers underneath --- + +test('a limit is clamped to the cap, and a missing one defaults', () => { + assert.equal(limitOf({}), API_LIMIT); + assert.equal(limitOf({ limit: '10' }), 10); + assert.equal(limitOf({ limit: '100000' }), API_MAX_LIMIT); + assert.equal(limitOf({ limit: '0' }), API_LIMIT); + assert.equal(limitOf({ limit: 'banana' }), API_LIMIT); +}); + +test('an offset reads as zero unless it is a usable number', () => { + assert.equal(offsetOfQuery({}), 0); + assert.equal(offsetOfQuery({ offset: '25' }), 25); + assert.equal(offsetOfQuery({ offset: '-1' }), 0); + assert.equal(offsetOfQuery({ offset: 'banana' }), 0); + assert.ok(Number.isSafeInteger(offsetOfQuery({ offset: String(Number.MAX_VALUE) }))); +}); + +test('next preserves the other query parameters', () => { + const block = pageBlock({ + url: 'https://ci.example.com/api/v1/tasks/t1?project=demo&offset=0&limit=2', + offset: 0, + limit: 2, + total: 9, + }); + + const next = new URL(block.next); + assert.equal(next.searchParams.get('project'), 'demo', 'a filter must survive the page turn'); + assert.equal(next.searchParams.get('offset'), '2'); + assert.equal(next.searchParams.get('limit'), '2'); +}); + +test('the key order is next, offset, total', () => { + const block = pageBlock({ url: 'https://ci.example.com/x', offset: 0, limit: 2, total: 9 }); + assert.deepEqual(Object.keys(block), ['next', 'offset', 'total']); +}); diff --git a/test/conductor.test.js b/test/conductor.test.js @@ -5,7 +5,7 @@ import test from 'node:test'; import assert from 'node:assert/strict'; -import { startHarness, DEFAULT_PIPELINE } from './helpers/harness.js'; +import { startHarness, DEFAULT_PIPELINE, data, apiError } from './helpers/harness.js'; // Tests that exercised the JSON read API, which has been removed. Kept // until the suite is rebuilt against the interface. @@ -57,7 +57,7 @@ async function drain(h, query = {}) { for (let i = 0; i < 50; i += 1) { const res = await h.claim(query); if (res.statusCode === 204) break; - const task = res.json().task; + const task = res.json().data.task; claimed[task.name] = task; } return claimed; @@ -67,22 +67,25 @@ async function claimNamed(h, name, query) { return (await drain(h, query))[name] ?? null; } -async function finish(h, taskId, { success = true, exitCode = 0, error = null } = {}) { - const res = await h.app.inject({ +async function finishRaw(h, taskId, { success = true, exitCode = 0, error = null } = {}) { + return h.app.inject({ method: 'POST', url: `/api/v1/tasks/${taskId}/complete`, headers: { ...h.auth, 'content-type': 'application/json' }, payload: JSON.stringify({ success, exit_code: exitCode, error }), }); - return res.json(); +} + +async function finish(h, taskId, opts) { + return data(await finishRaw(h, taskId, opts)); } test('health reports the selected drivers', async () => { await withHarness({}, async (h) => { const res = await h.app.inject({ method: 'GET', url: '/health' }); assert.equal(res.statusCode, 200); - assert.deepEqual(res.json(), { - ok: true, service: 'conductor', database: 'sqlite', storage: 'local', auth: 'local', + assert.deepEqual(data(res), { + service: 'conductor', database: 'sqlite', storage: 'local', auth: 'local', }); }); }); @@ -92,7 +95,7 @@ test('a signed trigger creates a job and expands the pipeline', async () => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main', actor: 'alice' }); assert.equal(res.statusCode, 200); - const body = res.json(); + const body = data(res); assert.equal(body.status, 'created'); const job = await h.waitForJob(body.job_id); @@ -140,7 +143,7 @@ test('github and gitlab push payloads are understood', async () => { pusher: { name: 'octocat' }, }); assert.equal(github.statusCode, 200); - const githubJob = await h.waitForJob(github.json().job_id); + const githubJob = await h.waitForJob(github.json().data.job_id); assert.equal(githubJob.state, 'running'); const gitlab = await h.trigger({ @@ -149,7 +152,7 @@ test('github and gitlab push payloads are understood', async () => { user_username: 'gl-user', }); assert.equal(gitlab.statusCode, 200); - const gitlabJob = await h.waitForJob(gitlab.json().job_id); + const gitlabJob = await h.waitForJob(gitlab.json().data.job_id); assert.equal(gitlabJob.state, 'running'); }); }); @@ -158,7 +161,7 @@ test('a branch deletion is ignored rather than failing', async () => { await withHarness({}, async (h) => { const res = await h.trigger({ sha: '0'.repeat(40), ref: 'refs/heads/gone' }); assert.equal(res.statusCode, 200); - assert.equal(res.json().status, 'ignored'); + assert.equal(res.json().data.status, 'ignored'); }); }); @@ -166,7 +169,7 @@ test('a broken pipeline is reported as a client error with detail', async () => await withHarness({ pipeline: 'version: 1\ntasks:\n a:\n script: [x]\n' }, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); assert.equal(res.statusCode, 200); - const job = await h.waitForJob(res.json().job_id); + const job = await h.waitForJob(res.json().data.job_id); assert.equal(job.state, 'failed'); assert.ok(job.error?.includes('tasks.a.image')); }); @@ -176,7 +179,7 @@ test('a missing pipeline file is reported clearly', async () => { await withHarness({ pipeline: null }, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); assert.equal(res.statusCode, 200); - const job = await h.waitForJob(res.json().job_id); + const job = await h.waitForJob(res.json().data.job_id); assert.equal(job.state, 'failed'); assert.match(job.error ?? '', /\.conductor\.yml not found/); }); @@ -186,7 +189,7 @@ test('a pipeline may be spelled .conductor.yaml instead of .conductor.yml', asyn await withHarness({ repoConfigPath: '.conductor.yaml' }, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); assert.equal(res.statusCode, 200); - const job = await h.waitForJob(res.json().job_id); + const job = await h.waitForJob(res.json().data.job_id); assert.equal(job.state, 'running'); }); }); @@ -195,7 +198,7 @@ test('a project pointed at a specific file gets no fallback', async () => { await withHarness({ configPath: 'ci/build.yml', repoConfigPath: '.conductor.yml' }, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); assert.equal(res.statusCode, 200); - const job = await h.waitForJob(res.json().job_id); + const job = await h.waitForJob(res.json().data.job_id); assert.equal(job.state, 'failed'); assert.match(job.error ?? '', /ci\/build\.yml not found/); }); @@ -206,7 +209,7 @@ test('a repository carrying both spellings is refused rather than guessed at', a const sha = await h.commit({ '.conductor.yaml': DEFAULT_PIPELINE }, 'add the other spelling'); const res = await h.trigger({ sha, ref: 'refs/heads/main' }); assert.equal(res.statusCode, 200); - const job = await h.waitForJob(res.json().job_id); + const job = await h.waitForJob(res.json().data.job_id); assert.equal(job.state, 'failed'); assert.match(job.error ?? '', /both exist/); }); @@ -218,7 +221,7 @@ test('a pipeline still using the jobs key is told what it was renamed to', async const sha = await h.commit({ '.conductor.yml': old }, 'pre-rename pipeline'); const res = await h.trigger({ sha, ref: 'refs/heads/main' }); assert.equal(res.statusCode, 200); - const job = await h.waitForJob(res.json().job_id); + const job = await h.waitForJob(res.json().data.job_id); assert.equal(job.state, 'failed'); assert.match(job.error ?? '', /was renamed to tasks/); }); @@ -229,7 +232,7 @@ test('a job can be created directly, without a push payload to imitate', async ( const res = await h.createJob({ sha: h.sha, ref: 'refs/heads/main', actor: 'alice' }); assert.equal(res.statusCode, 200); - const body = res.json(); + const body = data(res); assert.equal(body.status, 'created'); const job = await h.waitForJob(body.job_id); @@ -263,7 +266,7 @@ test('creating a job insists on a real commit rather than inferring one', async test('a job can be read back, with the state of every task', async () => { await withHarness({}, async (h) => { - const jobId = (await h.createJob({ sha: h.sha, ref: 'refs/heads/main' })).json().job_id; + const jobId = (await h.createJob({ sha: h.sha, ref: 'refs/heads/main' })).json().data.job_id; await h.waitForJob(jobId); const signature = h.sign(''); @@ -274,7 +277,7 @@ test('a job can be read back, with the state of every task', async () => { }); assert.equal(signed.statusCode, 200, signed.body); - const body = signed.json(); + const body = data(signed); assert.equal(body.job.id, jobId); assert.equal(body.job.state, 'running'); assert.equal(body.tasks.length, 6); @@ -285,19 +288,19 @@ test('a job can be read back, with the state of every task', async () => { test('a public job is readable without any credential at all', async () => { await withHarness({}, async (h) => { - const jobId = (await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).json().job_id; + const jobId = (await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).json().data.job_id; await h.waitForJob(jobId); const res = await h.app.inject({ method: 'GET', url: `/api/v1/projects/demo/jobs/${jobId}` }); assert.equal(res.statusCode, 200, res.body); - assert.equal(res.json().job.visibility, 'public'); + assert.equal(res.json().data.job.visibility, 'public'); }); }); test('a private job needs the trigger secret, and is otherwise absent', async () => { const pipeline = DEFAULT_PIPELINE.replace('version: 1\n', 'version: 1\nvisibility: private\n'); await withHarness({ pipeline }, async (h) => { - const jobId = (await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).json().job_id; + const jobId = (await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).json().data.job_id; await h.waitForJob(jobId); const anon = await h.app.inject({ method: 'GET', url: `/api/v1/projects/demo/jobs/${jobId}` }); @@ -309,13 +312,13 @@ test('a private job needs the trigger secret, and is otherwise absent', async () headers: { 'content-type': 'application/json', 'x-hub-signature-256': h.sign('') }, }); assert.equal(signed.statusCode, 200, signed.body); - assert.equal(signed.json().job.visibility, 'private'); + assert.equal(signed.json().data.job.visibility, 'private'); }); }); test('a job of another project cannot be read through this one', async () => { await withHarness({}, async (h) => { - const jobId = (await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).json().job_id; + const jobId = (await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).json().data.job_id; await h.waitForJob(jobId); const signature = h.sign(''); const res = await h.app.inject({ @@ -347,7 +350,7 @@ async function claimDownstream(h, name) { test('a task can be read back on its own, and by its full name', async () => { await withHarness({}, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - await h.waitForJob(res.json().job_id); + await h.waitForJob(res.json().data.job_id); const task = await claimNamed(h, 'build:arch=x86_64', { arches: 'x86_64' }); const [bare, scoped] = await Promise.all( @@ -355,9 +358,9 @@ test('a task can be read back on its own, and by its full name', async () => { ); assert.equal(bare.statusCode, 200, bare.body); assert.equal(scoped.statusCode, 200, scoped.body); - assert.deepEqual(bare.json(), scoped.json(), 'both paths describe the same task'); + assert.deepEqual(data(bare), data(scoped), 'both paths describe the same task'); - const { task: body } = bare.json(); + const { task: body } = data(bare); assert.equal(body.id, task.id); assert.equal(body.name, 'build:arch=x86_64'); assert.equal(body.base_name, 'build'); @@ -378,13 +381,13 @@ test('a task can be read back on its own, and by its full name', async () => { test('reading a task back carries its dependencies and matrix', async () => { await withHarness({}, async (h) => { const tr = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - await h.waitForJob(tr.json().job_id); + await h.waitForJob(tr.json().data.job_id); const task = await claimDownstream(h, 'package:arch=x86_64,pkg=musl'); const res = await h.app.inject({ method: 'GET', url: `/api/v1/tasks/${task.id}` }); assert.equal(res.statusCode, 200, res.body); - const { task: body } = res.json(); + const { task: body } = data(res); assert.deepEqual(body.matrix, { pkg: 'musl' }); assert.deepEqual(body.needs, ['build:arch=x86_64']); assert.equal(body.workdir, '/work', 'settled when the job was created'); @@ -396,7 +399,7 @@ test('reading a task back carries its dependencies and matrix', async () => { test('reading a task back never exposes its environment, services or script', async () => { await withHarness({}, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - await h.waitForJob(res.json().job_id); + await h.waitForJob(res.json().data.job_id); const task = await claimDownstream(h, 'package:arch=x86_64,pkg=musl'); // The claim payload has them, which is what makes their absence below @@ -408,7 +411,10 @@ test('reading a task back never exposes its environment, services or script', as const res = await h.app.inject({ method: 'GET', url }); assert.equal(res.statusCode, 200, res.body); - const payload = res.json(); + const payload = data(res); + // Asserted before the loop below, which would otherwise pass happily + // against a payload that has no task in it at all. + assert.ok(payload.task, `${url} must describe a task`); for (const key of ['env', 'services', 'script', 'spec', 'worker_token_id', 'log_key', 'storage_key']) { assert.equal(payload.task[key], undefined, `${url} must not carry ${key}`); } @@ -422,7 +428,7 @@ test('reading a task back never exposes its environment, services or script', as test('a task lists the artifacts it stored, with a usable download url', async () => { await withHarness({}, async (h) => { const tr = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - await h.waitForJob(tr.json().job_id); + await h.waitForJob(tr.json().data.job_id); const task = await claimNamed(h, 'build:arch=x86_64', { arches: 'x86_64' }); await h.app.inject({ @@ -435,7 +441,7 @@ test('a task lists the artifacts it stored, with a usable download url', async ( const res = await h.app.inject({ method: 'GET', url: `/api/v1/tasks/${task.id}` }); assert.equal(res.statusCode, 200, res.body); - const { artifacts } = res.json(); + const { artifacts } = data(res); assert.equal(artifacts.length, 1); assert.equal(artifacts[0].path, 'dist/app'); assert.equal(artifacts[0].size, 14); @@ -452,7 +458,7 @@ test('a task lists the artifacts it stored, with a usable download url', async ( test('reading a task back needs no worker token', async () => { await withHarness({}, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - await h.waitForJob(res.json().job_id); + await h.waitForJob(res.json().data.job_id); const task = await claimNamed(h, 'lint', {}); // The worker protocol shares this prefix and puts a token hook over its @@ -468,7 +474,7 @@ test('a private task is absent without a credential, and needs its project named const pipeline = DEFAULT_PIPELINE.replace('version: 1\n', 'version: 1\nvisibility: private\n'); await withHarness({ pipeline }, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - await h.waitForJob(res.json().job_id); + await h.waitForJob(res.json().data.job_id); const task = await claimNamed(h, 'lint', {}); for (const url of taskUrls(task.id)) { @@ -484,7 +490,7 @@ test('a private task is absent without a credential, and needs its project named headers: signed, }); assert.equal(scoped.statusCode, 200, scoped.body); - assert.equal(scoped.json().task.visibility, 'private'); + assert.equal(scoped.json().data.task.visibility, 'private'); // Without a project in the path there is no secret to check against, so // a signature proves nothing here. @@ -498,14 +504,14 @@ test('a private task is readable by an administrator, by either path', async () // scrypt is slow, so the harness only creates the admin when asked. await withHarness({ pipeline, bootstrap: true }, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - await h.waitForJob(res.json().job_id); + await h.waitForJob(res.json().data.job_id); const task = await claimNamed(h, 'lint', {}); const { cookie } = await h.login(); for (const url of taskUrls(task.id)) { const res = await h.app.inject({ method: 'GET', url, headers: { cookie } }); assert.equal(res.statusCode, 200, `${url}: ${res.body}`); - assert.equal(res.json().task.id, task.id); + assert.equal(res.json().data.task.id, task.id); } }); }); @@ -513,7 +519,7 @@ test('a private task is readable by an administrator, by either path', async () test('a task of another project cannot be read through this one', async () => { await withHarness({}, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - await h.waitForJob(res.json().job_id); + await h.waitForJob(res.json().data.job_id); const task = await claimNamed(h, 'lint', {}); const res2 = await h.app.inject({ @@ -530,7 +536,7 @@ test('an unknown task is 404 by either path', async () => { for (const url of taskUrls('m2y0000000000000nope')) { const res = await h.app.inject({ method: 'GET', url }); assert.equal(res.statusCode, 404, `${url} should be 404`); - assert.equal(res.json().error, 'unknown task'); + assert.equal(apiError(res).name, 'UnknownTask'); } }); }); @@ -550,7 +556,7 @@ test('the worker api rejects missing and invalid tokens', async () => { test('independent tasks dispatch concurrently', async () => { await withHarness({}, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - await h.waitForJob(res.json().job_id); + await h.waitForJob(res.json().data.job_id); // The prototype could only ever hand out one task at a time. const [a, b, c] = await Promise.all([ @@ -559,7 +565,7 @@ test('independent tasks dispatch concurrently', async () => { h.claim({ arches: 'x86_64,aarch64' }), ]); - const names = [a, b, c].map((r) => r.json().task.name); + const names = [a, b, c].map((r) => r.json().data.task.name); assert.equal(new Set(names).size, 3, `expected three distinct tasks, got ${names.join(', ')}`); assert.deepEqual(names.slice().sort(), ['build:arch=aarch64', 'build:arch=x86_64', 'lint']); }); @@ -568,7 +574,7 @@ test('independent tasks dispatch concurrently', async () => { test('a task is only offered to a worker that satisfies its requires', async () => { await withHarness({}, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - await h.waitForJob(res.json().job_id); + await h.waitForJob(res.json().data.job_id); // Clear the three root tasks so only package and publish remain. const roots = await drain(h, { arches: 'x86_64,aarch64' }); @@ -581,20 +587,20 @@ test('a task is only offered to a worker that satisfies its requires', async () const with_ = await h.claim({ arches: 'x86_64,aarch64', features: 'sign-key' }); assert.equal(with_.statusCode, 200); - assert.match(with_.json().task.name, /^package:/); + assert.match(with_.json().data.task.name, /^package:/); }); }); test('a worker is not offered work for an architecture it cannot build', async () => { await withHarness({}, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - await h.waitForJob(res.json().job_id); + await h.waitForJob(res.json().data.job_id); const claimed = []; for (let i = 0; i < 3; i += 1) { const res = await h.claim({ arches: 'aarch64' }); if (res.statusCode === 204) break; - claimed.push(res.json().task); + claimed.push(res.json().data.task); } // lint has no arch so it is eligible; the x86_64 build is not. @@ -606,7 +612,7 @@ test('a worker is not offered work for an architecture it cannot build', async ( test('a claimed task carries everything the worker needs', async () => { await withHarness({}, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - await h.waitForJob(res.json().job_id); + await h.waitForJob(res.json().data.job_id); const task = await claimNamed(h, 'build:arch=x86_64', { arches: 'x86_64' }); assert.equal(task.image, 'debian:bookworm-slim'); @@ -635,7 +641,7 @@ test('a claimed task carries everything the worker needs', async () => { test('a task that requires a feature reports it to the worker', async () => { await withHarness({}, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - await h.waitForJob(res.json().job_id); + await h.waitForJob(res.json().data.job_id); const roots = await drain(h, { arches: 'x86_64,aarch64' }); for (const task of Object.values(roots)) await finish(h, task.id, { success: true }); @@ -647,7 +653,7 @@ test('a task that requires a feature reports it to the worker', async () => { test('the standard conductor variables are present in the task environment', async () => { await withHarness({}, async (h) => { - const job = (await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).json().job_id; + const job = (await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).json().data.job_id; await h.waitForJob(job); const task = await claimNamed(h, 'lint', {}); @@ -668,7 +674,7 @@ test('the standard conductor variables are present in the task environment', asy test('the source tarball is served from the mirror', async () => { await withHarness({}, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - await h.waitForJob(res.json().job_id); + await h.waitForJob(res.json().data.job_id); const task = await claimNamed(h, 'lint', {}); const res2 = await h.app.inject({ @@ -687,7 +693,7 @@ test('the source tarball is served from the mirror', async () => { test('a worker cannot touch a task it does not hold', async () => { await withHarness({}, async (h) => { const tr = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - await h.waitForJob(tr.json().job_id); + await h.waitForJob(tr.json().data.job_id); const task = await claimNamed(h, 'lint', {}); const other = await h.services.workerTokens.create('intruder'); @@ -704,7 +710,7 @@ test('a worker cannot touch a task it does not hold', async () => { test('log appends are resumable and deduplicated by offset', async () => { await withHarness({}, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - await h.waitForJob(res.json().job_id); + await h.waitForJob(res.json().data.job_id); const task = await claimNamed(h, 'lint', {}); const url = `/api/v1/tasks/${task.id}/log`; const headers = { ...h.auth, 'content-type': 'application/octet-stream' }; @@ -712,20 +718,20 @@ test('log appends are resumable and deduplicated by offset', async () => { const first = await h.app.inject({ method: 'POST', url, headers: { ...headers, 'x-log-offset': '0' }, payload: Buffer.from('hello\n'), }); - assert.deepEqual(first.json(), { size: 6, written: 6, truncated: false }); + assert.deepEqual(data(first), { size: 6, written: 6, truncated: false }); // A retry resends what was already stored plus new data. const retry = await h.app.inject({ method: 'POST', url, headers: { ...headers, 'x-log-offset': '0' }, payload: Buffer.from('hello\nworld\n'), }); - assert.deepEqual(retry.json(), { size: 12, written: 6, truncated: false }); + assert.deepEqual(data(retry), { size: 12, written: 6, truncated: false }); // A gap is refused, and says where to resume. const gap = await h.app.inject({ method: 'POST', url, headers: { ...headers, 'x-log-offset': '9999' }, payload: Buffer.from('x'), }); assert.equal(gap.statusCode, 409); - assert.equal(gap.json().expected_offset, 12); + assert.equal(apiError(gap).details.expected_offset, 12); const tail = await h.services.logs.read(await jobIdOf(h, task.id), task.id, { offset: 0, limit: 1024 }); assert.equal(tail.data.toString('utf8'), 'hello\nworld\n'); @@ -739,7 +745,7 @@ test('log appends are resumable and deduplicated by offset', async () => { test('an oversized log chunk is refused', async () => { await withHarness({}, async (h) => { const tr = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - await h.waitForJob(tr.json().job_id); + await h.waitForJob(tr.json().data.job_id); const task = await claimNamed(h, 'lint', {}); // Artifacts are streamed and may be large, but a single log append is @@ -759,7 +765,7 @@ test('an oversized log chunk is refused', async () => { test('a finished log moves to storage and is still readable', async () => { await withHarness({}, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - await h.waitForJob(res.json().job_id); + await h.waitForJob(res.json().data.job_id); const task = await claimNamed(h, 'lint', {}); await h.app.inject({ @@ -782,7 +788,7 @@ test('a finished log moves to storage and is still readable', async () => { test('artifacts are stored, hashed, and stripped of traversal', async () => { await withHarness({}, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - await h.waitForJob(res.json().job_id); + await h.waitForJob(res.json().data.job_id); const task = await claimNamed(h, 'build:arch=x86_64', { arches: 'x86_64' }); const upload = await h.app.inject({ @@ -797,7 +803,7 @@ test('artifacts are stored, hashed, and stripped of traversal', async () => { }); assert.equal(upload.statusCode, 200); - const body = upload.json(); + const body = data(upload); assert.equal(body.path, 'etc/passwd', 'traversal should be stripped, not honoured'); assert.equal(body.size, 14); assert.match(body.sha256, /^[0-9a-f]{64}$/); @@ -819,7 +825,7 @@ test('artifacts are stored, hashed, and stripped of traversal', async () => { test('an artifact shorter than its content-length is rejected', async () => { await withHarness({}, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - await h.waitForJob(res.json().job_id); + await h.waitForJob(res.json().data.job_id); const task = await claimNamed(h, 'lint', {}); const res2 = await h.app.inject({ @@ -839,7 +845,7 @@ test('an artifact shorter than its content-length is rejected', async () => { test('success releases dependents, matched on shared dimensions', async () => { await withHarness({}, async (h) => { - const job = (await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).json().job_id; + const job = (await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).json().data.job_id; await h.waitForJob(job); const roots = await drain(h, { arches: 'x86_64,aarch64', features: 'sign-key' }); @@ -858,7 +864,7 @@ test('success releases dependents, matched on shared dimensions', async () => { test('failure skips transitive dependents, not just direct ones', async () => { await withHarness({}, async (h) => { - const job = (await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).json().job_id; + const job = (await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).json().data.job_id; await h.waitForJob(job); const build = await claimNamed(h, 'build:arch=x86_64', { arches: 'x86_64' }); @@ -878,7 +884,7 @@ test('failure skips transitive dependents, not just direct ones', async () => { test('a job settles as failed once every task is terminal', async () => { await withHarness({}, async (h) => { - const job = (await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).json().job_id; + const job = (await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).json().data.job_id; await h.waitForJob(job); const roots = await drain(h, { arches: 'x86_64,aarch64', features: 'sign-key' }); @@ -910,7 +916,7 @@ tasks: needs: [a] `; await withHarness({ pipeline }, async (h) => { - const job = (await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).json().job_id; + const job = (await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).json().data.job_id; await h.waitForJob(job); const a = await claimNamed(h, 'a', {}); @@ -937,7 +943,7 @@ tasks: needs: [flaky] `; await withHarness({ pipeline }, async (h) => { - const job = (await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).json().job_id; + const job = (await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).json().data.job_id; await h.waitForJob(job); const flaky = await claimNamed(h, 'flaky', {}); @@ -962,7 +968,7 @@ tasks: max_attempts: 2 `; await withHarness({ pipeline }, async (h) => { - const job = (await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).json().job_id; + const job = (await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).json().data.job_id; await h.waitForJob(job); const first = await claimNamed(h, 'retried', {}); @@ -980,7 +986,7 @@ tasks: test('heartbeats keep a task alive and report cancellation', async () => { await withHarness({}, async (h) => { - const job = (await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).json().job_id; + const job = (await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).json().data.job_id; await h.waitForJob(job); const task = await claimNamed(h, 'lint', {}); @@ -990,7 +996,7 @@ test('heartbeats keep a task alive and report cancellation', async () => { headers: { ...h.auth, 'content-type': 'application/json' }, payload: '{}', }); - assert.deepEqual(beat.json(), { cancelled: false }); + assert.deepEqual(data(beat), { cancelled: false }); await h.services.scheduler.cancelJob(job); @@ -1000,14 +1006,14 @@ test('heartbeats keep a task alive and report cancellation', async () => { headers: { ...h.auth, 'content-type': 'application/json' }, payload: '{}', }); - assert.equal(after.json().cancelled, true); + assert.equal(after.json().data.cancelled, true); assert.equal((await jobState(h, job)).job.state, 'cancelled'); }); }); test('the reaper fails a task whose worker stopped reporting', async () => { await withHarness({}, async (h) => { - const job = (await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).json().job_id; + const job = (await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).json().data.job_id; await h.waitForJob(job); const task = await claimNamed(h, 'lint', {}); assert.equal(tasksByName(await jobState(h, job)).lint.state, 'running'); @@ -1026,17 +1032,18 @@ test('the reaper fails a task whose worker stopped reporting', async () => { assert.match(after.error, /stopped reporting/); // The worker that lost the task can no longer complete it. - const late = await finish(h, task.id, { success: true }); - assert.match(String(late.error ?? ''), /not running|another worker/); + const late = await finishRaw(h, task.id, { success: true }); + assert.equal(late.statusCode, 409); + assert.equal(apiError(late).name, 'TaskNotRunning'); }); }); test('a second push produces an independent, numbered job', async () => { await withHarness({}, async (h) => { - const first = (await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })).json(); + const first = data(await h.trigger({ sha: h.sha, ref: 'refs/heads/main' })); await h.waitForJob(first.job_id); const sha2 = await h.commit({ 'README.md': 'changed\n' }, 'second commit'); - const second = (await h.trigger({ sha: sha2, ref: 'refs/heads/main' })).json(); + const second = data(await h.trigger({ sha: sha2, ref: 'refs/heads/main' })); await h.waitForJob(second.job_id); assert.notEqual(first.job_id, second.job_id); @@ -1052,13 +1059,13 @@ test('jobs can be listed and filtered by project', apiGone, async () => { await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); const all = await h.app.inject({ method: 'GET', url: '/api/jobs' }); - assert.equal(all.json().jobs.length, 1); + assert.equal(all.json().data.jobs.length, 1); const mine = await h.app.inject({ method: 'GET', url: '/api/jobs?project=demo' }); - assert.equal(mine.json().jobs.length, 1); + assert.equal(mine.json().data.jobs.length, 1); const other = await h.app.inject({ method: 'GET', url: '/api/jobs?project=absent' }); - assert.equal(other.json().jobs.length, 0); + assert.equal(other.json().data.jobs.length, 0); }); }); @@ -1095,7 +1102,7 @@ test('a commit that is not in the repository is refused', async () => { await withHarness({}, async (h) => { const res = await h.trigger({ sha: 'b'.repeat(40), ref: 'refs/heads/main' }); assert.equal(res.statusCode, 200); - const job = await h.waitForJob(res.json().job_id); + const job = await h.waitForJob(res.json().data.job_id); assert.equal(job.state, 'failed'); assert.match(job.error ?? '', /not present in the mirror/); }); @@ -1104,7 +1111,7 @@ test('a commit that is not in the repository is refused', async () => { test('a task is told to fetch an archive, and never the repository itself', async () => { await withHarness({}, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - await h.waitForJob(res.json().job_id); + await h.waitForJob(res.json().data.job_id); const task = await claimNamed(h, 'lint', {}); assert.match(task.endpoints.source, /\/source\.tar\.gz$/); @@ -1123,7 +1130,7 @@ test('the source archive is the bare tree, with no wrapping directory', async () // wrapping directory in the archive would land one level too deep. await withHarness({}, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - await h.waitForJob(res.json().job_id); + await h.waitForJob(res.json().data.job_id); const task = await claimNamed(h, 'lint', {}); const archive = await h.app.inject({ @@ -1165,7 +1172,7 @@ test('the working directory is resolved from the pipeline, then the project', as await withHarness({ pipeline }, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - await h.waitForJob(res.json().job_id); + await h.waitForJob(res.json().data.job_id); const task = await claimNamed(h, 'build', {}); assert.equal(task.workdir, '/usr/src/app', 'the repository has the final say'); }); @@ -1177,7 +1184,7 @@ test('the working directory is resolved from the pipeline, then the project', as { dir: '/srv/build', id: h.project.id } ); const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - await h.waitForJob(res.json().job_id); + await h.waitForJob(res.json().data.job_id); const task = await claimNamed(h, 'lint', {}); assert.equal(task.workdir, '/srv/build'); }); @@ -1185,7 +1192,7 @@ test('the working directory is resolved from the pipeline, then the project', as // With neither, there is a default. await withHarness({}, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - await h.waitForJob(res.json().job_id); + await h.waitForJob(res.json().data.job_id); const task = await claimNamed(h, 'lint', {}); assert.equal(task.workdir, '/work'); }); @@ -1207,7 +1214,7 @@ test('an unusable working directory is refused rather than sanitised', async () await withHarness({ pipeline }, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); assert.equal(res.statusCode, 200); - const job = await h.waitForJob(res.json().job_id); + const job = await h.waitForJob(res.json().data.job_id); assert.equal(job.state, 'failed', `expected ${JSON.stringify(workdir)} to be refused`); assert.ok((job.error ?? '').includes('workdir'), `error should mention workdir: ${job.error}`); }); diff --git a/test/dialects.test.js b/test/dialects.test.js @@ -18,7 +18,7 @@ import path from 'node:path'; import { startDatabase, dialectAvailable } from './helpers/database.js'; import { openDatabase, runMigrations, dialectFromUrl } from '../src/lib/db/index.js'; -import { paginate } from '../src/lib/page.js'; +import { paginate, paginateOffset, pageBlock } from '../src/lib/page.js'; import { createStorage } from '../src/lib/storage/index.js'; import { createLogStore } from '../src/lib/log.js'; import { createRetention } from '../src/conductor/retention.js'; @@ -117,6 +117,22 @@ for (const dialect of ['postgres', 'mysql']) { }); assert.deepEqual(lastPage.rows.map((r) => r.id), ['r5'], 'the offset landed wrong'); + // The API pages by offset over the same binding, and its total feeds + // the decision to send a next url at all, so it has to be a number + // here for the same reason. + const window = await paginateOffset(db, { + select: 'id', from: 'jobs', order: 'number', offset: 2, limit: 2, + }); + assert.deepEqual(window.rows.map((r) => r.id), ['r3', 'r4']); + assert.equal(typeof window.total, 'number', 'COUNT came back as a string'); + assert.equal(window.total, 5); + + const tail = await paginateOffset(db, { + select: 'id', from: 'jobs', order: 'number', offset: 4, limit: 2, + }); + assert.deepEqual(tail.rows.map((r) => r.id), ['r5']); + assert.ok(!('next' in pageBlock({ url: 'https://x.test/j', ...tail })), 'that was the last page'); + const retention = createRetention({ cfg, db, storage, logs, logger: {} }); const first = await retention.sweep({ now: NOW }); diff --git a/test/helpers/harness.js b/test/helpers/harness.js @@ -9,6 +9,7 @@ import os from 'node:os'; import net from 'node:net'; import path from 'node:path'; import crypto from 'node:crypto'; +import assert from 'node:assert/strict'; import { execFile } from 'node:child_process'; import { promisify } from 'node:util'; import { loadConfig } from '../../src/lib/config.js'; @@ -250,6 +251,21 @@ export async function startHarness(options = {}) { }; } +// Unwraps an API response, asserting the envelope on the way through, so +// every call site that reads a body also checks the shape it arrived in. +export function data(res) { + const body = res.json(); + assert.equal(body.ok, true, `expected ok:true, got ${res.statusCode} ${res.body.slice(0, 200)}`); + return body.data; +} + +// The failure half. Returns { name, message, details } for asserting on. +export function apiError(res) { + const body = res.json(); + assert.equal(body.ok, false, `expected ok:false, got ${res.statusCode} ${res.body.slice(0, 200)}`); + return body.error; +} + function silentLogger() { return { info: () => {}, warn: () => {}, error: () => {}, debug: () => {} }; } diff --git a/test/retention.test.js b/test/retention.test.js @@ -425,13 +425,13 @@ test('artifacts.expire in a pipeline reaches the stored artifact', async () => { await withHarness({ pipeline }, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - await h.waitForJob(res.json().job_id); + await h.waitForJob(res.json().data.job_id); const before = Date.now(); for (const name of ['build', 'keep']) { const res = await h.claim({}); assert.equal(res.statusCode, 200, `expected to claim ${name}`); - const task = res.json().task; + const task = res.json().data.task; const upload = await h.app.inject({ method: 'POST', diff --git a/test/ui.test.js b/test/ui.test.js @@ -66,7 +66,7 @@ test('attrs omits absent values and renders bare booleans', () => { test('an anonymous visitor sees public jobs and an invitation to sign in', async () => { await withUi({ visibility: 'public' }, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - await h.waitForJob(res.json().job_id); + await h.waitForJob(res.json().data.job_id); const page = await get(h, '/'); assert.equal(page.statusCode, 200); @@ -82,7 +82,7 @@ test('an anonymous visitor sees public jobs and an invitation to sign in', async test('an anonymous visitor cannot see a private job', async () => { await withUi({ visibility: 'private' }, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - const job = res.json().job_id; + const job = res.json().data.job_id; await h.waitForJob(job); const list = await get(h, '/'); @@ -277,7 +277,7 @@ test('the users page is administrator only and warns about what deletion destroy test('a running job polls, and a finished one stops', async () => { await withUi({ visibility: 'public' }, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - const job = res.json().job_id; + const job = res.json().data.job_id; await h.waitForJob(job); const running = await get(h, `/jobs/${job}`); @@ -294,8 +294,8 @@ test('a running job polls, and a finished one stops', async () => { test('a task page streams the log and stops polling once it completes', async () => { await withUi({ visibility: 'public' }, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - await h.waitForJob(res.json().job_id); - const task = (await h.claim({})).json().task; + await h.waitForJob(res.json().data.job_id); + const task = (await h.claim({})).json().data.task; await h.app.inject({ method: 'POST', @@ -324,8 +324,8 @@ test('a task page streams the log and stops polling once it completes', async () test('log output is escaped, so a task cannot inject markup into the page', async () => { await withUi({ visibility: 'public' }, async (h) => { const res = await h.trigger({ sha: h.sha, ref: 'refs/heads/main' }); - await h.waitForJob(res.json().job_id); - const task = (await h.claim({})).json().task; + await h.waitForJob(res.json().data.job_id); + const task = (await h.claim({})).json().data.task; await h.app.inject({ method: 'POST',