Sign in

Search issue titles and bodies, change requests and comments. Private repositories you cannot read are not included.

vole
Clone

Read only, no credentials:

https://rodent.software/git/vole.git

Write over SSH with a registered key:

git clone ssh://git@0.0.0.0:2222/vole.git

A free and open-source forge that lives in your Git repo.

#216

P2: Let one runner run several jobs at once

closed · opened by keenan 2d ago

keenan opened this 2d ago

Why

A runner executes one job at a time and nothing else. ci/agent/agent.go leases a job, blocks in sess.run until it finishes, tidies the caches, and only then asks for the next one. There is no --jobs, no worker pool, and --once exists because that is the only way to get a second job onto a machine: start a second process.

That leaves most of a runner’s hardware idle. A build that is one step waiting on a network fetch, or a repository whose tests are single-threaded, occupies a whole machine for its duration. The common answer to “we need more CI capacity” becomes “another runner”, which is a second host, a second key, a second allowlist entry, and a second --dir that learns the repositories from cold. On a 16-core machine running mostly single-core builds that is a poor trade.

The server side already supports concurrency and needs no change for this:

  • a lease is taken per SSH session and claimed by a conditional update (ci/runs/runs.go, Store.Lease), so two sessions racing the same run means one loses and retries elsewhere;
  • runner_sessions is keyed by session, so one account can hold several;
  • max_jobs_per_repo already bounds how many jobs one repository may run at once, and the lease serves the least busy repository first.

The single-job assumption is the runner process, not the protocol or the queue.

Scope

Add vole runner --jobs N (default 1): N slots in one process, each with its own SSH session and at most one in-flight job. Slots are the existing loop; nothing in the protocol changes.

Rejected alternative: multiplexing N leases on one channel. Replies are unkeyed and matched by order (runnerSession.call), and a lease with wait blocks the server’s decode loop for up to 60s — an idle slot’s long-poll would starve a busy slot’s heartbeat and expire its lease. It would need request ids, per-request goroutines, an outbound write mutex, and a presence schema change, to save connections that SSH can already multiplex at the transport layer.

Shared state: what must land with the flag

These are the parts that are only safe while one job exists per runner. The first two are latent bugs today for two separate processes sharing one --dir; --jobs just makes them reachable without doing anything unusual.

  1. Namespace lifetime. ci/cache Trim measures namespaces and forceRemoveAlls whole ones, and it runs after every job (ci/agent/agent.go). forceRemoveAll chmods the 0555 directories the Go module cache extracts and deletes them. A job holding that namespace has no defence: the docs’ own reason for whole-namespace eviction — “a Go module cache missing one file is a checksum error deep in a build rather than a download” — is exactly what a concurrent eviction produces.

    Fix: a job takes LOCK_SH on its namespace for the duration; Trim and Clear attempt LOCK_EX|LOCK_NB and skip a namespace they cannot have. The tool caches are multi-process safe against themselves, so sharing is the right default; what they are not safe against is a third party unlinking the tree. Same primitive as ci/agent/gitcache.go lockCache/tryLockCache, including the rule that the lock file is never removed.

  2. Eviction belongs on a ticker, not after every job. Once Trim can skip, running it from N slots at once is mostly noise on the cache root. Move it to one background goroutine.

  3. --no-cache needs the lock, not a scope change. It already removes only the job’s own namespace after each job, so the only broad operation is Clear(root) at startup. What it lacks is the lock: the removal after a job can land on a namespace another job has mounted, and Clear at startup can empty a namespace a second process sharing --dir is building in.

  4. Image builds. Two slots may build the same image-file digest at once (ci/agent/build.go), so the build happens twice and the tag is raced. Same bytes either way, so this is waste rather than corruption: in-process singleflight on container.ImageTag, ideally with a lock file so it also holds across processes sharing --dir.

  5. One thing per process, done before the slots. s.stream is per-session state today (ci/agent/agent.go) and becomes local. cleanupJobTrees, maintainTrustedCaches, cleanupStaleContainers, cleanupStaleNetworks, ping and checkRepoFilter run once, before the slots start, rather than N times.

  6. Failure and shutdown. A refused job (malformed payload, --repos that matches nothing) means the runner is misconfigured: cancel all slots rather than stopping the one that happened to see it. SIGTERM should let in-flight jobs finish reporting instead of killing N jobs mid-step.

Already safe: do not rewrite these

  • The saved checkout uses flock per repository (ci/agent/gitcache.go), and prepareWorktree holds it for the whole clone+fetch; the release and site pushes hold it again for the write. flock is per open file description, so it excludes goroutines in one process as well as other processes.
  • Container and network names carry the run id and attempt, and the leftover sweep keeps anything whose vole.runner.pid is alive, so slots cannot reap each other and a crashed --jobs N runner leaves N labelled sets for the next start.
  • Lease ownership is the account, not the session (lease_owner), so two slots of one account are indistinguishable to Store.Held/Heartbeat. That is benign: the attempt number, incremented on every lease, is what actually refuses a stale record (Store.RecordAttempt requires attempts = ?). Worth a comment so nobody later “fixes” ownership to be session-scoped without knowing why it does not have to be.

Residual risk that locks do not fix

  • A workflow can clean /cache for its siblings. The mount is a plain read-write directory and the docs leave “which tool uses /cache” to the repository, so go clean -modcache, npm cache clean --force, or rm -rf /cache/* in one job can break a sibling. Only same repository + same trust + same image share a namespace, and least-busy-repository leasing usually puts slots on different repositories, so the victim is the same repository’s own concurrent builds. See the decision below.
  • Disk overshoot. --cache-size is a total, and a skippable trim means N busy namespaces may exceed it until they finish. Bounded and self-correcting; the operator should be told.
  • Per-container limits are per container. --container-memory, --container-cpus and --container-pids multiply by N, and max_jobs_per_repo becomes the knob that stops one repository claiming all N slots. Belongs with #211.
  • Presence and connection count. The runners page becomes N rows per machine, and N idle slots park N connections against ssh_max_connections (default 64, shared with git traffic). Decide whether a row carries a machine name so the page can say “3 of 8 busy”, or whether fan-out is acceptable.

Decision to make before the code

Whether /cache is the runner’s cache (shared between concurrent jobs of the same repository, evicted by us, siblings may collide) or the job’s cache (per-slot isolation, no eviction races, no reuse). This issue assumes the first: it is the smallest change and it keeps the reuse the cache exists for. If that is the answer, docs/runner.md needs “concurrent builds of the same repository may share /cache” in the same paragraph that promises isolation by trust today.

Acceptance

  • vole runner --jobs 4 leases and runs up to four jobs at once, one per session, and the runners page shows four sessions and what each is building.
  • --jobs 1 behaves exactly as today, including --once.
  • --jobs 2 --once leases at most two jobs and exits when both slots are idle.
  • No namespace is removed, cleared or evicted while a job has it mounted, at any --jobs value. This also holds for two separate vole runner processes sharing one --dir.
  • Two slots building the same image-file digest build it once.
  • A refused job stops the runner and reports why, rather than leaving the other slots running and the process half-alive.
  • docs/runner.md, docs/cli.md, docs/deploy/runner.md and packaging/runner.env explain the flag, the resource arithmetic, and the /cache sharing rule.

Tests

  • Two goroutines on one --dir writing distinct namespaces while the trimmer runs: no namespace disappears under a holder.
  • --no-cache with two jobs at once: neither removes the other’s namespace.
  • N sessions on one account lease N distinct runs (server/ssh/lease_wait_test.go already has the two-client shape), and a stale sixth record is refused by attempt number.
  • A burst on one repository: max_jobs_per_repo caps it while other repositories keep flowing.

Order of work

  1. Namespace locks, trim ticker, --no-cache scoping. Worth landing alone: it fixes the shared---dir case today.
  2. --jobs, the s.stream refactor, the startup/fatal-error restructure.
  3. Image-build singleflight, then docs and packaging/runner.env.

Related: #211 covers bounding what one job may consume; this issue multiplies that budget by the slot count.

Comments (5)

keenan 2d ago

Cache can remain the runner’s cache.

keenan 2d ago

Decision recorded: /cache stays the runner’s cache. Concurrent builds of the same repository share a namespace, and the tool caches’ own locking is what makes that safe; the runner’s job is the lifetime, not the contents.

I also corrected item 3 of the body. --no-cache was already scoped to the job’s own namespace — my earlier claim that it deletes a sibling’s cache was wrong. What it is missing is the lock: the removal after a job, and Clear at startup, can land on a namespace another job or another process has mounted.

That is slice 1, up as #217 (runner-cache-locks):

  • ci/cache: Lock / LockExclusive / TryLockExclusive via flock on unix and a process-local RWMutex elsewhere, following core/git.
  • Trim and Clear take the exclusive lock without waiting and skip a namespace that is held, so a mounted tree is never deleted under a build.
  • A job holds its namespace from before the container starts until after it stops; --no-cache holds it exclusively and empties it under that hold.
  • The runner trims at startup and then on a five-minute timer instead of after each job, so eviction is no longer tied to one job’s lifetime.
  • vole workflow run gets the same lock.
  • Lock files live under <root>/.locks, mirroring the namespace, and are never deleted — the gitcache rule.

vole workflow run verify passes on the branch (format, vet, build, licenses, govulncheck, packages, deployment files, links, go test -race ./...).

Remaining for slice 2: --jobs, the s.stream refactor, per-process startup work and one-fatal-error-cancels-all, then the image-build singleflight and the docs.

keenan 2d ago

Slice 2 is up as #218 (runner-jobs): vole runner --jobs N.

  • N slots in one process, one session each; the server side needed no change.
  • Process work (directories, cache bounds, leftover sweep, connections, --repos check) happens once, before any slot starts.
  • A refused job stops every slot; a lost connection reconnects that slot with a doubling delay and the job is requeued when its lease expires. --once keeps the one-shot shape.
  • The log stream moved from the session to the job, which is what made two jobs on one session impossible to confuse.
  • RUNNER_JOBS in the first-boot renderer, plus docs/runner.md, docs/cli.md, docs/deploy/runner.md and packaging/runner.env.

vole workflow run verify passes, including go test -race ./.... New tests: four sessions lease four distinct runs, max_jobs_per_repo caps a burst across sessions, and the refusal/reconnect split.

Left for slice 3: the image-build singleflight. I also left SIGTERM draining out of this change — it needs signal.NotifyContext plus a way to cut a parked lease short, and a second signal still has to kill the process. With --jobs that is N jobs lost instead of one, so I would take it before calling this feature done; say if you want it in slice 3 or on its own.

keenan 2d ago

Slice 3 is up as #219 (runner-image-builds): one image-file digest is built once per runner process.

Two slots naming the same digest now share the build. The waiter heartbeats on the beat its own build would have — a half-hour build with a 60-second lease would otherwise lose the run it is holding — and stops on a lost lease or a cancel. A failed build fails the waiters with the builder’s reason, and a finished entry is forgotten so a deleted image is rebuilt.

Two processes sharing a --dir can still both build the digest; same bytes, same digest tag, so it is waste rather than damage. Serialising that needs a lock file per digest and I left it out deliberately.

Tests are in ci/agent/build_test.go against the fake runtime, which now sleeps and can fail: two sessions, one digest, one build and one “being built by another job” line in the other job’s log.

So #216 has slices 1–3: #217 (cache namespace locks), #218 (--jobs), #219 (image builds). What remains from the must-fix list is item 6’s second half — SIGTERM draining instead of killing in-flight jobs, which with --jobs costs N jobs instead of one. It needs signal.NotifyContext, a way to cut a parked lease short, and a second signal that still kills the process. I would take that before closing this.

Also: nobody has yet watched --jobs 4 run four real jobs against an instance with a container runtime. The session and slot halves are tested separately; the end-to-end check is a machine, not a unit test.

keenan 2d ago

The last must-fix item is up as #220 (runner-stop-drains): a signal now drains.

  • SIGTERM/SIGINT stops leasing, prints one line, and lets the jobs in flight finish and report; a second signal ends the process at once.
  • The parked lease was the awkward part: that read belongs to the session’s decoder, so the only way to cut it short is to end the connection. A slot parked on a lease holds no job, so hanging up costs nothing.

One thing to know: the packaged unit kills the process 60 seconds after SIGTERM (TimeoutStopSec), so longer builds are still lost. docs/runner.md and docs/deploy/runner.md show the drop-in to raise it; I did not change the unit’s default, because “wait forever on stop” is a machine-by-machine decision.

That is every item on the must-fix list: locks + trim timer (#217), --jobs (#218), one build per digest (#219), and drain (#220).

What is left before closing this is observation, not code:

  1. --jobs 4 running four real jobs on a machine with a container runtime, with the runners page showing four sessions and what each is building.
  2. --jobs 2 --once leasing at most two jobs and exiting.

Both are implemented and the halves are tested separately; the end-to-end run is the one thing a unit test cannot do. If you want, I can do the second by driving the verify workflow against a scratch instance.