From ed9562d1a756e2fb3ed8092cf0aca99035800d68 Mon Sep 17 00:00:00 2001 From: mhoennig Date: Mon, 10 Aug 2026 16:14:38 +0200 Subject: [PATCH] Let cancel terminate auxiliary build phases instead of blocking the slot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cancelling a build only killed the running build process; the synchronous preparation phases — most notably a multi-minute Docker image build, but also the Gradle-volume preparation — ran to completion and kept the concurrency slot occupied, so the next queued build stayed PENDING for a long time. Build runners now report every auxiliary process through an onAuxProcess sink (GitCommandRunner gained an onProcess hook), and the executor registers them like the build process, so cancellation terminates whatever is currently running. Measured on vm4006: cancel to next-build-running is ~3s in the normal case; the unit test covers the aux-phase case. Co-Authored-By: Claude Fable 5 --- .../hoennig/gittally/build/BuildExecutor.kt | 8 ++++ .../de/hoennig/gittally/build/BuildRunner.kt | 8 +++- .../gittally/build/DockerBuildRunner.kt | 9 ++++- .../hoennig/gittally/git/GitCommandRunner.kt | 20 ++++++++-- .../gittally/build/BuildExecutorTest.kt | 39 ++++++++++++++++++- .../gittally/build/DockerBuildRunnerTest.kt | 28 +++++++------ .../gittally/git/GitCommandRunnerTest.kt | 10 +++++ .../gittally/server/NginxProxyManagerTest.kt | 8 ++-- 8 files changed, 107 insertions(+), 23 deletions(-) diff --git a/src/main/kotlin/de/hoennig/gittally/build/BuildExecutor.kt b/src/main/kotlin/de/hoennig/gittally/build/BuildExecutor.kt index b53db36..69f83e4 100644 --- a/src/main/kotlin/de/hoennig/gittally/build/BuildExecutor.kt +++ b/src/main/kotlin/de/hoennig/gittally/build/BuildExecutor.kt @@ -218,6 +218,14 @@ class BuildExecutor( environment = mapOf("branch" to build.runningBuild.branch), repoDir = build.workingDir, branchConfig = branchConfig, + onAuxProcess = { aux -> + // preparation phases (e.g. a Docker image build) must die on cancellation + // too, otherwise a cancelled build blocks its slot until they finish + build.process = aux + if (build.cancelled.get()) { + destroyProcessTree(aux) + } + }, ) build.process = process if (build.cancelled.get()) { diff --git a/src/main/kotlin/de/hoennig/gittally/build/BuildRunner.kt b/src/main/kotlin/de/hoennig/gittally/build/BuildRunner.kt index 8039ef5..8a90d8e 100644 --- a/src/main/kotlin/de/hoennig/gittally/build/BuildRunner.kt +++ b/src/main/kotlin/de/hoennig/gittally/build/BuildRunner.kt @@ -10,6 +10,9 @@ import java.nio.file.Path * which owns log streaming and process-tree termination. * [repoDir] and [branchConfig] let implementations derive per-repository resources * and per-branch settings (step 11: Docker runtime). + * Implementations report every auxiliary process they run before the build command + * (e.g. a Docker image build) via [onAuxProcess], so a cancellation can terminate + * long preparation phases instead of blocking the build slot until they finish. */ interface BuildRunner { fun start( @@ -18,6 +21,7 @@ interface BuildRunner { environment: Map, repoDir: Path = workingDir, branchConfig: BranchConfig = BranchConfig(), + onAuxProcess: (Process) -> Unit = {}, ): Process } @@ -29,6 +33,7 @@ class ProcessBuildRunner : BuildRunner { environment: Map, repoDir: Path, branchConfig: BranchConfig, + onAuxProcess: (Process) -> Unit, ): Process { val processBuilder = ProcessBuilder("bash", "-c", command) processBuilder.directory(workingDir.toFile()) @@ -53,8 +58,9 @@ class DispatchingBuildRunner( environment: Map, repoDir: Path, branchConfig: BranchConfig, + onAuxProcess: (Process) -> Unit, ): Process { val runner = if (branchConfig.docker.enabled) dockerBuildRunner else processBuildRunner - return runner.start(command, workingDir, environment, repoDir, branchConfig) + return runner.start(command, workingDir, environment, repoDir, branchConfig, onAuxProcess) } } diff --git a/src/main/kotlin/de/hoennig/gittally/build/DockerBuildRunner.kt b/src/main/kotlin/de/hoennig/gittally/build/DockerBuildRunner.kt index fd8c4e9..b1c0b35 100644 --- a/src/main/kotlin/de/hoennig/gittally/build/DockerBuildRunner.kt +++ b/src/main/kotlin/de/hoennig/gittally/build/DockerBuildRunner.kt @@ -48,12 +48,13 @@ class DockerBuildRunner( environment: Map, repoDir: Path, branchConfig: BranchConfig, + onAuxProcess: (Process) -> Unit, ): Process { val docker = branchConfig.docker require(docker.image.isNotBlank()) { "branches..docker.image must be set when docker.enabled is true" } val repoKey = ArtifactKeys.repoKey(repoDir) cleanupStaleContainersOnce(repoKey, repoDir) - ensureImage(docker, repoDir) + ensureImage(docker, repoDir, onAuxProcess) val uid = id("-u", repoDir) val gid = id("-g", repoDir) val socket = socketLocator.locate(uid) @@ -64,7 +65,7 @@ class DockerBuildRunner( val ownershipUid = if (rootless) "0" else uid val ownershipGid = if (rootless) "0" else gid val volume = gradleVolumeName(repoKey) - prepareGradleVolume(volume, docker.image, ownershipUid, ownershipGid, repoDir) + prepareGradleVolume(volume, docker.image, ownershipUid, ownershipGid, repoDir, onAuxProcess) val containerName = containerName(repoKey, environment["branch"]) removeContainer(containerName, repoDir) val runCommand = @@ -92,6 +93,7 @@ class DockerBuildRunner( private fun ensureImage( docker: DockerConfig, repoDir: Path, + onAuxProcess: (Process) -> Unit, ) { if (docker.dockerfile.isBlank()) { return @@ -126,6 +128,7 @@ class DockerBuildRunner( docker.context, ), repoDir, + onProcess = onAuxProcess, ) } @@ -140,6 +143,7 @@ class DockerBuildRunner( uid: String, gid: String, repoDir: Path, + onAuxProcess: (Process) -> Unit, ) { val key = "$volume@$image" if (key in preparedVolumes) { @@ -164,6 +168,7 @@ class DockerBuildRunner( gid, ), repoDir, + onProcess = onAuxProcess, ) preparedVolumes += key } diff --git a/src/main/kotlin/de/hoennig/gittally/git/GitCommandRunner.kt b/src/main/kotlin/de/hoennig/gittally/git/GitCommandRunner.kt index f80bab2..8e73119 100644 --- a/src/main/kotlin/de/hoennig/gittally/git/GitCommandRunner.kt +++ b/src/main/kotlin/de/hoennig/gittally/git/GitCommandRunner.kt @@ -23,27 +23,39 @@ class GitCommandException( @Component class GitCommandRunner { + /** [onProcess] receives the started process, so callers can terminate it early (build cancellation). */ fun run( command: List, workingDir: Path, environment: Map = emptyMap(), + onProcess: (Process) -> Unit = {}, ): GitCommandResult { val builder = ProcessBuilder(command).directory(workingDir.toFile()) builder.environment().putAll(environment) val process = builder.start() - // stderr is drained concurrently so neither pipe buffer can block the process - val stderr = CompletableFuture.supplyAsync { process.errorStream.bufferedReader().readText() } - val stdout = process.inputStream.bufferedReader().readText() + onProcess(process) + // stderr is drained concurrently so neither pipe buffer can block the process; + // reads tolerate closed streams, e.g. when onProcess terminated the process early + val stderr = CompletableFuture.supplyAsync { process.errorStream.readSafely() } + val stdout = process.inputStream.readSafely() val exitCode = process.waitFor() return GitCommandResult(exitCode, stdout, stderr.get()) } + private fun java.io.InputStream.readSafely(): String = + try { + bufferedReader().readText() + } catch (_: java.io.IOException) { + "" + } + fun runOrThrow( command: List, workingDir: Path, environment: Map = emptyMap(), + onProcess: (Process) -> Unit = {}, ): GitCommandResult { - val result = run(command, workingDir, environment) + val result = run(command, workingDir, environment, onProcess) if (!result.isSuccess) { throw GitCommandException(command, result) } diff --git a/src/test/kotlin/de/hoennig/gittally/build/BuildExecutorTest.kt b/src/test/kotlin/de/hoennig/gittally/build/BuildExecutorTest.kt index e0782fd..2158aa2 100644 --- a/src/test/kotlin/de/hoennig/gittally/build/BuildExecutorTest.kt +++ b/src/test/kotlin/de/hoennig/gittally/build/BuildExecutorTest.kt @@ -31,6 +31,7 @@ class BuildExecutorTest : FunSpec() { private class Harness( configYaml: String, workspaceSubdir: String? = null, + val buildRunner: BuildRunner = ProcessBuildRunner(), ) { val workingDir: Path = Files.createTempDirectory("gittally-executor-test") val repository = FileBuildResultRepository(workingDir.resolve("build-results.json")) @@ -52,7 +53,7 @@ class BuildExecutorTest : FunSpec() { repository = repository, configLoader = ConfigLoader(), giteaClient = giteaClient, - buildRunner = ProcessBuildRunner(), + buildRunner = buildRunner, workspaces = workspaces, artifactStore = artifactStore, eventPublisher = @@ -73,6 +74,7 @@ class BuildExecutorTest : FunSpec() { cleanCommand: String = "", maxConcurrent: Int = 1, workspaceSubdir: String? = null, + buildRunner: BuildRunner? = null, ) = Harness( """ builds: @@ -83,6 +85,7 @@ class BuildExecutorTest : FunSpec() { cleanCommand: "$cleanCommand" """.trimIndent(), workspaceSubdir = workspaceSubdir, + buildRunner = buildRunner ?: ProcessBuildRunner(), ) private suspend fun awaitStatus( @@ -182,6 +185,40 @@ class BuildExecutorTest : FunSpec() { result.duration.shouldNotBeNull().toMillis() shouldBeLessThan waitMillis } + test("cancel kills a long auxiliary preparation phase instead of waiting for it") { + // like DockerBuildRunner's synchronous image build: a slow aux process runs + // before the build command and fails the build when it was terminated + val auxRunner = + object : BuildRunner { + override fun start( + command: String, + workingDir: java.nio.file.Path, + environment: Map, + repoDir: java.nio.file.Path, + branchConfig: de.hoennig.gittally.config.BranchConfig, + onAuxProcess: (Process) -> Unit, + ): Process { + val aux = ProcessBuilder("bash", "-c", "sleep 60").directory(workingDir.toFile()).start() + onAuxProcess(aux) + aux.waitFor() + check(aux.exitValue() == 0) { "aux phase failed" } + return ProcessBuilder("bash", "-c", "echo built").directory(workingDir.toFile()).start() + } + } + val h = harness("unused", buildRunner = auxRunner) + + val build = h.executor.startBuild("main", "abc123", h.workingDir) + eventually(10.seconds) { + h.repository.latestFor("main")?.status shouldBe BuildStatus.RUNNING + } + h.executor.cancel(build.artifactKey).shouldBeTrue() + + // must beat the 60s aux sleep by a wide margin — the aux process gets killed + eventually(15.seconds) { + h.repository.latestFor("main")?.status shouldBe BuildStatus.CANCELLED + } + } + test("a build cancelled while still queued records neither runningSince nor a duration") { val h = harness("sleep 30") diff --git a/src/test/kotlin/de/hoennig/gittally/build/DockerBuildRunnerTest.kt b/src/test/kotlin/de/hoennig/gittally/build/DockerBuildRunnerTest.kt index d222819..e9edc1a 100644 --- a/src/test/kotlin/de/hoennig/gittally/build/DockerBuildRunnerTest.kt +++ b/src/test/kotlin/de/hoennig/gittally/build/DockerBuildRunnerTest.kt @@ -49,10 +49,10 @@ class DockerBuildRunnerTest : FunSpec() { captured.clear() repoDir = Files.createTempDirectory("gittally-docker-runner") workspace = repoDir.resolve("workspace") - every { commandRunner.run(any(), any(), any()) } returns GitCommandResult(0, "", "") - every { commandRunner.runOrThrow(any(), any(), any()) } returns GitCommandResult(0, "", "") - every { commandRunner.runOrThrow(listOf("id", "-u"), any(), any()) } returns GitCommandResult(0, "1000\n", "") - every { commandRunner.runOrThrow(listOf("id", "-g"), any(), any()) } returns GitCommandResult(0, "1001\n", "") + every { commandRunner.run(any(), any(), any(), any()) } returns GitCommandResult(0, "", "") + every { commandRunner.runOrThrow(any(), any(), any(), any()) } returns GitCommandResult(0, "", "") + every { commandRunner.runOrThrow(listOf("id", "-u"), any(), any(), any()) } returns GitCommandResult(0, "1000\n", "") + every { commandRunner.runOrThrow(listOf("id", "-g"), any(), any(), any()) } returns GitCommandResult(0, "1001\n", "") every { socketLocator.locate("1000") } returns DockerSocket(Paths.get("/var/run/docker.sock"), rootless = false, gid = 999L) runner = DockerBuildRunner(commandRunner, socketLocator) @@ -144,6 +144,8 @@ class DockerBuildRunnerTest : FunSpec() { commandRunner.runOrThrow( match { it.take(2) == listOf("docker", "run") && it.takeLast(2) == listOf("0", "0") }, repoDir, + any(), + any(), ) } } @@ -194,7 +196,7 @@ class DockerBuildRunnerTest : FunSpec() { test("builds a missing image from the Dockerfile with the input labels") { Files.writeString(repoDir.resolve("Dockerfile"), "FROM eclipse-temurin:21\n") - every { commandRunner.run(match { it.take(3) == listOf("docker", "image", "inspect") }, any(), any()) } returns + every { commandRunner.run(match { it.take(3) == listOf("docker", "image", "inspect") }, any(), any(), any()) } returns GitCommandResult(1, "", "no such image") runner.start("./gradlew test", workspace, mapOf("branch" to "main"), repoDir, dockerBranchConfig(dockerfile = "Dockerfile")) @@ -221,6 +223,8 @@ class DockerBuildRunnerTest : FunSpec() { ".", ), repoDir, + any(), + any(), ) } } @@ -229,13 +233,13 @@ class DockerBuildRunnerTest : FunSpec() { Files.writeString(repoDir.resolve("Dockerfile"), "FROM eclipse-temurin:21\n") val dockerfileHash = DockerImageInputs.dockerfileSha256(repoDir.resolve("Dockerfile")) val inputsHash = DockerImageInputs.inputsSha256(dockerfileHash, "Dockerfile", ".") - every { commandRunner.run(match { it.take(3) == listOf("docker", "image", "inspect") }, any(), any()) } returns + every { commandRunner.run(match { it.take(3) == listOf("docker", "image", "inspect") }, any(), any(), any()) } returns GitCommandResult(0, "$inputsHash\n", "") runner.start("./gradlew test", workspace, mapOf("branch" to "main"), repoDir, dockerBranchConfig(dockerfile = "Dockerfile")) verify(exactly = 0) { - commandRunner.runOrThrow(match { it.take(2) == listOf("docker", "build") }, any(), any()) + commandRunner.runOrThrow(match { it.take(2) == listOf("docker", "build") }, any(), any(), any()) } } @@ -247,25 +251,27 @@ class DockerBuildRunnerTest : FunSpec() { val repoKey = ArtifactKeys.repoKey(repoDir) verify(exactly = 1) { - commandRunner.runOrThrow(listOf("docker", "volume", "create", "gittally-gradle-$repoKey"), repoDir) + commandRunner.runOrThrow(listOf("docker", "volume", "create", "gittally-gradle-$repoKey"), repoDir, any(), any()) } verify(exactly = 2) { commandRunner.run( listOf("docker", "rm", "-f", "gittally-build-$repoKey-${ArtifactKeys.branchKey("main")}"), repoDir, + any(), + any(), ) } } test("removes stale labelled build containers once, before the first docker build") { - every { commandRunner.run(match { it.take(3) == listOf("docker", "ps", "-aq") }, any(), any()) } returns + every { commandRunner.run(match { it.take(3) == listOf("docker", "ps", "-aq") }, any(), any(), any()) } returns GitCommandResult(0, "abc\ndef\n", "") runner.start("./gradlew clean", workspace, mapOf("branch" to "main"), repoDir, dockerBranchConfig()) runner.start("./gradlew test", workspace, mapOf("branch" to "main"), repoDir, dockerBranchConfig()) - verify(exactly = 1) { commandRunner.run(match { it.take(3) == listOf("docker", "ps", "-aq") }, any(), any()) } - verify { commandRunner.run(listOf("docker", "rm", "-f", "abc", "def"), repoDir) } + verify(exactly = 1) { commandRunner.run(match { it.take(3) == listOf("docker", "ps", "-aq") }, any(), any(), any()) } + verify { commandRunner.run(listOf("docker", "rm", "-f", "abc", "def"), repoDir, any(), any()) } } test("fails without a configured image") { diff --git a/src/test/kotlin/de/hoennig/gittally/git/GitCommandRunnerTest.kt b/src/test/kotlin/de/hoennig/gittally/git/GitCommandRunnerTest.kt index fe1d47f..396fdc3 100644 --- a/src/test/kotlin/de/hoennig/gittally/git/GitCommandRunnerTest.kt +++ b/src/test/kotlin/de/hoennig/gittally/git/GitCommandRunnerTest.kt @@ -5,6 +5,7 @@ import io.kotest.core.spec.style.FunSpec import io.kotest.matchers.booleans.shouldBeFalse import io.kotest.matchers.booleans.shouldBeTrue import io.kotest.matchers.collections.shouldContainExactly +import io.kotest.matchers.nulls.shouldNotBeNull import io.kotest.matchers.shouldBe import io.kotest.matchers.string.shouldContain import java.nio.file.Files @@ -30,6 +31,15 @@ class GitCommandRunnerTest : FunSpec() { result.stdout.trim() shouldBe tempDir.toRealPath().toString() } + test("hands the started process to onProcess, so callers can terminate it early") { + var seen: Process? = null + + val result = runner.run(listOf("sh", "-c", "sleep 30"), tempDir, onProcess = { seen = it.also(Process::destroy) }) + + result.isSuccess.shouldBeFalse() + seen.shouldNotBeNull().isAlive.shouldBeFalse() + } + test("passes extra environment variables") { val result = runner.run(listOf("sh", "-c", "echo \"\$GITTALLY_TEST_VAR\""), tempDir, mapOf("GITTALLY_TEST_VAR" to "hello")) diff --git a/src/test/kotlin/de/hoennig/gittally/server/NginxProxyManagerTest.kt b/src/test/kotlin/de/hoennig/gittally/server/NginxProxyManagerTest.kt index 5820325..2a98593 100644 --- a/src/test/kotlin/de/hoennig/gittally/server/NginxProxyManagerTest.kt +++ b/src/test/kotlin/de/hoennig/gittally/server/NginxProxyManagerTest.kt @@ -118,7 +118,7 @@ class NginxProxyManagerTest : FunSpec() { sleepCount = 0 repoDir = Files.createTempDirectory("gittally-nginx-repo") stateDir = Files.createTempDirectory("gittally-nginx-state") - every { commandRunner.run(capture(captured), any(), any()) } answers { + every { commandRunner.run(capture(captured), any(), any(), any()) } answers { if (captured.last().take(3) == listOf("docker", "run", "-d")) { configsAtContainerRun += Files.readString(stateDir.resolve("nginx/nginx.conf")) } @@ -246,7 +246,7 @@ class NginxProxyManagerTest : FunSpec() { test("does not start while a foreign container occupies an nginx port") { every { configLoader.load(repoDir) } returns nginxConfig() every { - commandRunner.run(match { it.take(2) == listOf("docker", "ps") && it.contains("--format") }, any(), any()) + commandRunner.run(match { it.take(2) == listOf("docker", "ps") && it.contains("--format") }, any(), any(), any()) } returns GitCommandResult(0, "abc123\tother-app\t0.0.0.0:8080->80/tcp\tvendor=other", "") manager.start() @@ -258,7 +258,7 @@ class NginxProxyManagerTest : FunSpec() { test("removes a stale gittally-named container occupying an nginx port") { every { configLoader.load(repoDir) } returns nginxConfig() every { - commandRunner.run(match { it.take(2) == listOf("docker", "ps") && it.contains("--format") }, any(), any()) + commandRunner.run(match { it.take(2) == listOf("docker", "ps") && it.contains("--format") }, any(), any(), any()) } returnsMany listOf( GitCommandResult(0, "abc123\tgittally-nginx-old\t0.0.0.0:8080->80/tcp\t", ""), @@ -304,7 +304,7 @@ class NginxProxyManagerTest : FunSpec() { test("a docker failure never throws — the HTTP server keeps running") { every { configLoader.load(repoDir) } returns nginxConfig() - every { commandRunner.run(any(), any(), any()) } throws RuntimeException("docker: command not found") + every { commandRunner.run(any(), any(), any(), any()) } throws RuntimeException("docker: command not found") manager.start() }