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/vole.git

Write over SSH with a registered key:

git clone ssh://git@rodent.software:2222/vole.git

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

#220

Stop a runner by draining instead of killing its jobs

merged · by keenan 2d ago · merged into main at 786d7ce1 by keenan 2d ago

keenan opened this 2d ago

What

The last must-fix item on #216: a runner stops by draining instead of killing the jobs it has.

Before this, SIGTERM ended the process mid-step. A build that was nearly done was thrown away, and its run sat unclaimed until the lease expired, so somebody else started it again from nothing. With --jobs, that is N jobs lost instead of one.

  • cmd/vole/runner.go turns SIGINT/SIGTERM into a context cancellation, prints one line saying it is draining, and restores the default disposition (signal.Stop) so a second signal ends the process at once. A stuck drain should not need SIGKILL.
  • Slots stop leasing immediately. A job in flight keeps running and reports as usual; serve checks the context at the top of its loop, so nothing new is picked up.
  • The parked lease is the hard part. A resident slot waits up to 25s for the server’s reply, and that read belongs to the session’s decoder — nothing inside the runner can interrupt it. Cancelling the wait means ending the connection (hangup, a SIGTERM to the ssh process, deliberately taken outside the session’s mutex, which the parked goroutine holds). That costs nothing: a slot parked on a lease is holding no job. The slot then exits cleanly instead of trying to reconnect.

What it does not do

The packaged unit already allows 60 seconds between SIGTERM and SIGKILL (TimeoutStopSec=60s), so a build longer than that is killed anyway. Rather than change the unit’s judgement for everybody, docs/runner.md has a new “Stopping a runner” section and docs/deploy/runner.md shows the drop-in:

sudo systemctl edit vole-runner    # [Service] / TimeoutStopSec=30min

Tests

ci/agent/slot_test.go:

  • TestAParkedLeaseEndsWhenTheRunnerStops: a lease on a session that never answers returns errStopped when the context is cancelled, rather than sitting out its window (guarded by a 5s timeout so a regression fails instead of hanging).
  • TestAStoppedSlotTakesNoWork: a slot told to stop before it leases anything does not lease.

The signal plumbing itself is a CLI concern with no seam worth faking; it is three lines of select, and the cancellation path it triggers is what the above covers.

Verification

gofmt -l ., go vet ./..., go test -tags sqlite_fts5 ./... clean, and vole workflow run verify ok (format, vet, build, licenses, govulncheck, packages, deployment files, links, go test -race -count=1 ./...).

Where #216 stands

Every item on the must-fix list is now in: namespace locks and the trim timer (#217), --jobs (#218), one build per digest (#219), and this.

Two things are still unproven, and I would not close the issue on them:

  • End to end. Nobody has watched --jobs 4 run four real jobs against an instance with a container runtime, or watched four sessions on the runners page. The session half and the slot half are tested separately; the join is a machine.
  • --jobs 2 --once leases at most two jobs and exits. Implemented (each slot returns after its one job, and Run waits for all of them), exercised only indirectly.

Refs #216 (does not close it).

Discussion (1)

keenan 2d ago

CI caught a real race, now fixed in 786d7ce.

--- FAIL: TestSeveralSessionsLeaseDifferentJobs (0.20s)
    testing.go:1619: TempDir RemoveAll cleanup: unlinkat
    /tmp/TestSeveralSessionsLeaseDifferentJobs.../001: directory not empty
FAIL	github.com/rodent-software/vole/server/ssh	2.088s

The test was the trigger, not the cause. The harness removes its temporary directory as the last cleanup step, but Server.Close() only closed the listener: every connection handler, and every session goroutine under it, kept running past that point — free to write into a directory the test was already deleting. Opening four runner sessions in one test made the window easy to hit; the CI machine’s timing did the rest. It reproduces in about one run in a handful on a loaded machine, and never on an idle one, which is why nothing had caught it before.

Fix: “closed” now means nothing this server started is still running.

  • Serve registers each connection in live and adds it to a handlers WaitGroup under the same lock Close takes, so every handler is either registered before the wait starts or never started.
  • handleConn waits for its session goroutines before returning, so the connection handler is not done until the work it started is.
  • Close ends the listener, closes the live connections, and waits. The wait is bounded at 10 s and logged (closeGrace): a git process stuck on a dead filesystem must not hold up a shutdown, and everything a healthy handler does on disconnect takes microseconds.
  • A lease parked on the queue answers on Close too (a done channel in the same select), instead of making the wait sit out its 25-second window. Serve’s existing Runs.Drain() already did this for vole serve; this covers a Close that arrives without one.
  • Closing a closed server is now a no-op rather than an ErrClosed from the listener: every caller is a shutdown path, and none of them can act on it.

Test: TestClosingTheServerEndsItsConnections pins the contract — after Close, the client’s session ends promptly instead of hanging.

Verification: gofmt, go vet, go test -race -tags sqlite_fts5 ./... clean, vole workflow run verify ok, and the CI command (go test -race -count=1 ./...) run twice in a 2-CPU container: server/ssh ok in 69 s and 61 s.

That is on top of the drain change in f642e0a; the CR is ready for review again.