From 477b54ba2a764588f0ec840856e646222a8b6e5c Mon Sep 17 00:00:00 2001 From: Rob Boerman Date: Fri, 25 Sep 2026 20:55:09 +0200 Subject: [PATCH] fix(worker): map a job id onto the runtime's name rule, so a cron job can start a container (#435) BullMQ's job scheduler mints a cron job's id as repeat::, and jobContainerName used it unchanged, on the strength of a comment saying BullMQ ids are already [A-Za-z0-9._-]. They are not for scheduler jobs, and the runtime refuses ':' in a container or network name. Measured on rootless Podman 5.8.1: pi-job-repeat:t:1790361600000 and its -net network are both refused at create (exit 125, "names must match [a-zA-Z0-9][a-zA-Z0-9_.-]*"), and in a real worker every cron attempt ended container-never-started before any spend. Docker's rule is the same; not run here. jobContainerName now replaces every character outside [A-Za-z0-9._-] with '_', as sanitizeJobId does for file names. Every path that must find the container again (the timeout's stop, the cancel, the per-job network, the log sink) asks this function, so they all agree. Every id other than a scheduler's is already of that shape and is unchanged, which the test pins beside the cron case; the result is checked against the runtime's own rule for the container and its network. Not injective (repeat:a:1 and a literal repeat_a_1 share a name), stated in the comment as a residual no producer here reaches. INT-CONTAINER-RUNTIME-CONTRACT amended, with a revision row. INT-EGRESS-POLICY-CONTRACT UNCHANGED, checked. Signed-off-by: Rob Boerman --- specs/interfaces.md | 5 +++++ worker/src/backend-local.mjs | 23 ++++++++++++++--------- worker/test/backend-local.test.mjs | 15 ++++++++++++--- 3 files changed, 31 insertions(+), 12 deletions(-) diff --git a/specs/interfaces.md b/specs/interfaces.md index aeeb1798..2db6ee73 100644 --- a/specs/interfaces.md +++ b/specs/interfaces.md @@ -942,6 +942,10 @@ refactor apart. statement that nothing in this repo enforces it. - Flags: `--pull=never --rm --init --cap-drop=ALL --security-opt no-new-privileges --memory=4g --cpus=2 --pids-limit=512 --shm-size=1g`, and -- **unless `PI_EGRESS=0`** -- `--network=pi-job--net`. + `` here, and in `--name=pi-job-`, is the queue's job id with every character outside + `[A-Za-z0-9._-]` replaced by `_` (`jobContainerName`, issue #435): a cron job's id is the job scheduler's + `repeat::`, and the runtime refuses `:` in a container or network name (exit 125), so + before this no cron job ever started. Every other id the worker sees is already of that shape. - **Three of those are NOT members of `ISOLATION_FLAGS`, and this is a list of FLAGS rather than a rendering of that constant.** `ISOLATION_FLAGS` (`worker/src/docker-run.mjs`) is the **literal, value-free, unconditional** set, and two places assert every member of it reaches the sandbox argv @@ -4694,3 +4698,4 @@ onFailureTimeoutMs; worker/test/on-failure.test.mjs; worker/test/start-wiring.te | 2026-09-25 | Issue #354, part 2 (the `podman` venue). **`INT-LIVE-PROBE-CONTRACT` AMENDED**: one bullet and one Acceptance sentence. The native venue reads back through the same module, with the `podman` CLI as runner, `buildPodmanRunArgs` as the builder of all four container kinds, `serviceIsRemote === false` as the local gate (asked again before the first command), and the venue's own job user, so every probe carries `--user=` then `--userns=keep-id` and nothing runs where that decision refuses a job; the verdicts are unchanged, their docker-named details name the runtime they ran on, and their wording on `local` is byte-identical. `egress` is not read back on this venue by doctor, whose canary runs on docker only, and it says so; `.github/scripts/podman-conformance.mjs` hands the same verdicts, with a canary of its own, to the harness as its `readBack`. **`INT-CONTAINER-RUNTIME-CONTRACT` AMENDED**, one sentence: part 1's "no venue builds with it yet" now names the venue that does and what it passes (the worker's own `--user`, always, then `--userns=keep-id`, `HOME=/home/pi`, `relabel` from `podman info`). **UNCHANGED, checked**: `INT-SANDBOX-CONTRACT` (still local-only; a run on the podman venue is refused by name), `INT-RUNNER-EXIT-CODE-PROTOCOL` (the integers and their meanings; the attached exit 1 of a never-started container is a residual `DES-PODMAN-NATIVE-ROOTLESS-BACKEND` names, retried and not refunded), `INT-EGRESS-POLICY-CONTRACT` (the same flags; the proxy runs under the same rootless Podman) and `INT-SESSION-STORE-CONTRACT` (the venue stamp records `podman`; an unstamped transcript still reads as `local`). | | 2026-09-25 | Issue #354, part 2, the review round. **`INT-CONTAINER-RUNTIME-CONTRACT` AMENDED**: a new bullet, the pinned namespaces. The Podman builder follows `--userns=keep-id` with `--pid`, `--ipc`, `--uts` and `--cgroupns` private, `--env-host=false` and `--http-proxy=false`, and gives a job with no network `--network=private`, none of which `dockerExtra` can re-set; the network's options are not pinned and cannot be (issue #428). `INT-LIVE-PROBE-CONTRACT` UNCHANGED, checked. | | 2026-09-25 | Issue #427. **`INT-CONTAINER-RUNTIME-CONTRACT` AMENDED**, one sentence in its egress-network bullet: `NODE_USE_ENV_PROXY=1` is not enough inside the runner, because loading the pinned pi replaces the dispatcher it installs, so the runner re-installs an env-proxy dispatcher from pi's own `undici` right after pi is loaded. The four variables and the closed map are UNCHANGED, checked, and so is `INT-EGRESS-POLICY-CONTRACT`. | +| 2026-09-25 | Issue #435. **`INT-CONTAINER-RUNTIME-CONTRACT` AMENDED**, one sentence under the flags: the `` in `--name=pi-job-` and `--network=pi-job--net` is the job id mapped onto `[A-Za-z0-9._-]`, every other character becoming `_`. A cron job's id is the job scheduler's `repeat::`, which Podman refuses as a container or network name (exit 125, measured on 5.8.1; docker's rule is the same, not run), so no cron job had ever started a container. `INT-EGRESS-POLICY-CONTRACT`'s `pi-job--net` row is UNCHANGED, checked: the id it names is the same mapped one, and the reaper's prefix rule does not move. | diff --git a/worker/src/backend-local.mjs b/worker/src/backend-local.mjs index b0c22166..5d87b2bd 100644 --- a/worker/src/backend-local.mjs +++ b/worker/src/backend-local.mjs @@ -66,11 +66,18 @@ export const ENDPOINT_LISTED_STATES = new Set(["running", "paused"]); * `pi-job-`. The name a running job answers to, for `docker stop` on the 30-minute timeout, for the * per-job egress network derived from it, and for the reaper's filter. * - * Not sanitised here: BullMQ ids are already `[A-Za-z0-9._-]`, and the one place a job id comes from - * anywhere else (a sandbox) goes through `sanitizeJobId` under its own prefix. + * Sanitised to the runtimes' own name rule, `[a-zA-Z0-9][a-zA-Z0-9_.-]*` (docker's `RestrictedNameChars`, and + * Podman's, measured), every other character becoming `_`, as `sanitizeJobId` does for file names. It said here that + * BullMQ ids are already that shape, and a cron job's is not: the job scheduler mints `repeat::`, + * so `pi-job-repeat:...` was refused at create (exit 125, measured on Podman 5.8.1) and so was its `-net` network, and + * every cron job ended `container-never-started` before it ever ran (issue #435). The prefix supplies the first + * character. Deterministic, because every path that must find the container again (the timeout's stop, the cancel, + * the network, the log sink) asks this function for the name rather than deriving it. Not injective: `repeat:a:1` + * and a literal `repeat_a_1` share a name. BullMQ ids are unique per queue and no producer here mints an id with `_` + * where a scheduler id has `:`, so it is a residual rather than a collision anything reaches. */ export function jobContainerName(jobId) { - return `${JOB_NAME_PREFIX}${jobId}`; + return `${JOB_NAME_PREFIX}${String(jobId).replace(/[^A-Za-z0-9._-]/g, "_")}`; } /** @@ -334,12 +341,10 @@ function escapedPoint(point) { * `^pi-job-.*-net$`, so an operator's `pi-job-runner_default` had its CONTAINER reaped and its NETWORK left * standing. Two answers to "what is ours" is the defect; which answer to keep is the decision, and the * container half cannot be the one that moves. After a crash nothing distinguishes our `pi-job-` from - * any other name under the prefix, and a charset rule does not separate them either. The ids that reach - * `jobContainerName` are BullMQ's, which that function's own comment records as already `[A-Za-z0-9._-]` and - * deliberately does NOT re-sanitise, so `runner_default` and `runner-db-1` are both shapes a real job id can - * take. (An earlier version of this paragraph credited `sanitizeJobId`, which governs the SANDBOX namespace - * and is not in this path; the conclusion survives because both charsets carry `_` and `-`.) There is no - * stricter rule available that is also TRUE, so the halves agree by widening the network one. + * any other name under the prefix, and a charset rule does not separate them either. `jobContainerName` maps a + * job id onto `[A-Za-z0-9._-]` (since issue #435, when a cron id's `:` was found to be refused by the runtime), so + * `runner_default` and `runner-db-1` are both shapes a real job's name can take. There is no stricter rule + * available that is also TRUE, so the halves agree by widening the network one. * * WHAT THAT COSTS, stated rather than buried in a test diff: a network called `pi-job-mine-net-backup`, or * `pi-job-runner_default`, is now removed by the boot reaper. Their CONTAINERS always were. `SECURITY.md` diff --git a/worker/test/backend-local.test.mjs b/worker/test/backend-local.test.mjs index ab980ad6..a38cffd7 100644 --- a/worker/test/backend-local.test.mjs +++ b/worker/test/backend-local.test.mjs @@ -23,6 +23,16 @@ test("the namespace is one fact, and the producer builds names from it", () => { assert.equal(JOB_NAME_PREFIX, "pi-job-"); assert.equal(jobContainerName("abc123"), "pi-job-abc123"); assert.ok(jobContainerName("x").startsWith(JOB_NAME_PREFIX)); + // Issue #435: a cron job's id is the job scheduler's `repeat::`, which the runtime refuses as + // a name (exit 125, measured on Podman 5.8.1; docker's rule is the same), and so every cron job never started. + // Every other id this worker sees is already of that shape and comes through unchanged. + const RUNTIME_NAME = /^[a-zA-Z0-9][a-zA-Z0-9_.-]*$/; + assert.equal(jobContainerName("repeat:nightly:1790361600000"), "pi-job-repeat_nightly_1790361600000"); + for (const id of ["repeat:nightly:1790361600000", "repeat:a b/c:1", "local-9f3a", "gh-1", "4f1c2d3e-aaaa-bbbb-cccc-0123456789ab", "x.y_z-1"]) { + assert.match(jobContainerName(id), RUNTIME_NAME, id); + assert.match(networkNameFor(jobContainerName(id)), RUNTIME_NAME, `${id} network`); + } + for (const id of ["local-9f3a", "gh-1", "4f1c2d3e-aaaa-bbbb-cccc-0123456789ab"]) assert.equal(jobContainerName(id), `pi-job-${id}`, id); // The sandbox names itself OUTSIDE this namespace on purpose, so a worker restart cannot tear down a // shell an operator is sitting in. A prefix that became a prefix of the sandbox's would silently break // that, and it is the one relationship between the two strings that matters. @@ -730,9 +740,8 @@ test("both halves of the reaper give ONE answer to `what is ours` (#360)", () => const ours = [ jobContainerName("gh-1"), networkNameFor(jobContainerName("gh-1")), - // The shapes a REAL job id can take, which is why no charset rule separates an operator's name from a - // job's: `jobContainerName` does not sanitise, and BullMQ's ids are already `[A-Za-z0-9._-]`, so `_` - // and `-` are both legal. (`sanitizeJobId` governs the SANDBOX namespace and is not in this path.) + // The shapes a REAL job's name can take, which is why no charset rule separates an operator's name from a + // job's: `jobContainerName` maps an id onto `[A-Za-z0-9._-]` (#435), where `_` and `-` are both legal. "pi-job-runner_default", "pi-job-runner-db-1", // WIDENED BY #360, and this is the row that can destroy an operator's object: our exact shape with