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() }