Merge branch 'claude/amazing-khayyam-38cad4': interrupt builds on server shutdown

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-08-10 17:31:11 +02:00
parent 355d8ae90f 40f54a1298
commit 8b87fbeb7d
7 changed files with 236 additions and 20 deletions
@@ -56,6 +56,8 @@ Three places must stay in sync when config keys change: the `GitTallyConfig` dat

 `BuildExecutor` runs builds asynchronously: up to `builds.maxConcurrent` branches concurrently (default 1), but never more than one build per branch at a time. Each branch builds in its own reusable git worktree at `.git/gittally/worktrees/<branchKey>` (`BranchWorkspaces`), checked out detached at the requested commit — the primary checkout is never used for builds. Status transitions are persisted via `BuildResultRepository` (JSON file under `.git/gittally/`), published to Gitea non-fatally, and emitted as `BuildStatusChangedEvent`s. Cancellation addresses a build by artifact key and terminates the whole process tree. Future code (watcher, server, UI) must not assume a single running build.

+On context close (e.g. systemd SIGTERM), a `ContextClosedEvent` listener in `BuildExecutor` terminates the process trees of all executing builds and waits (bounded) until their results are persisted as INTERRUPTED — a shutdown is never recorded as FAILED. Builds still queued stay PENDING and start no process. Both are re-enqueued by the watcher's startup recovery; INTERRUPTED therefore publishes as Gitea state `pending`, not `failure` (`GiteaStateMapping`).
+
 The runtime is selected per branch behind the `BuildRunner` interface: `DispatchingBuildRunner` (`@Primary`) routes to native `ProcessBuildRunner` (the default) or to `DockerBuildRunner` when `branches.<name>.docker.enabled`. The Docker runner shells out to the `docker` CLI (no SDK): it (re)builds the configured image when the Dockerfile inputs changed (tracked via the `org.gittally.build-inputs-sha256` image label), maintains a per-repo Gradle cache volume, mounts the worktree and the Docker socket into a labelled (`org.hoennig.gittally`) `--rm --init` container, and repairs workspace ownership in-container after each command (under a rootless daemon the container runs as root, which is the host user, and the repair degenerates to `0:0`). Git works inside the container: the primary `.git` is mounted read-only with `.git/gittally/` masked by an empty tmpfs (credential isolation) and the worktree's admin dir mounted read-write (`gitMetadataMounts`). The returned `Process` is the attached `docker run` client, so log streaming and termination work exactly like native builds.

 ## Watcher
@@ -0,0 +1,87 @@
+> **WARNING:** This document describes only the change applied in this PR.
+> It may already be outdated once the next PR is merged.
+> Historic PR-documentation is not maintained along with new PRs — treat it as a snapshot, not as current documentation.
+
+## The Problem
+
+When the server is stopped (systemd SIGTERM) while a build is executing, the build process dies with the service.
+`BuildExecutor.execute` then classified the shutdown-induced exit as FAILED, because only the `cancelled` flag was checked.
+FAILED is terminal and not restartable, so the watcher's startup recovery did not re-enqueue the build.
+Additionally, a red `failure` status was posted to Gitea for a commit that was never actually built to failure.
+Observed on vm4006: a running Docker build died during a service restart and the commit silently stayed red.
+
+## Non-Goals
+
+- No change to explicit cancellation: a user-cancelled build stays CANCELLED and red.
+- No handling of `kill -9`: an unclean kill still leaves a stale RUNNING result, which the existing `markStaleRunningAsInterrupted` startup recovery already covers.
+- No draining of the Spring web layer or watcher — those already have their own shutdown hooks.
+
+## The Scenarios
+
+### Feature: builds interrupted by a server shutdown are recovered, not failed
+
+#### Background
+
+- INTERRUPTED and PENDING are restartable statuses; `Watcher.recoverOnStartup` re-enqueues the latest build of a branch in either status.
+- FAILED is terminal and never re-enqueued.
+
+#### Scenario#000.01: An executing build is recorded as INTERRUPTED on shutdown
+
+So that a service restart never loses a build or marks its commit as failed.
+
+- **Given** a build is executing
+- **When** the application context closes (e.g. systemd SIGTERM)
+- **Then** the build's process tree is terminated
+  - **and** the result is persisted as INTERRUPTED, not FAILED, before the context finishes closing
+  - **and** the startup recovery re-enqueues the branch on the next start
+
+##### Verified by
+
+- [BuildExecutorTest — "shutdown kills an executing build and records INTERRUPTED, not FAILED"](../../src/test/kotlin/de/hoennig/gittally/build/BuildExecutorTest.kt)
+
+#### Scenario#000.02: A queued build stays PENDING over a shutdown
+
+So that queued builds survive a restart the same way executing builds do.
+
+- **Given** a build is queued behind an executing build
+- **When** the application context closes
+- **Then** the queued build starts no process and gets no status transition
+  - **and** it stays PENDING for the startup recovery, which re-enqueues it after the restart
+
+##### Verified by
+
+- [BuildExecutorTest — "a build still queued at shutdown stays PENDING for the startup recovery"](../../src/test/kotlin/de/hoennig/gittally/build/BuildExecutorTest.kt)
+
+#### Scenario#000.03: No failure status is posted to Gitea for an interrupted build
+
+So that a commit does not turn red because of a service restart.
+
+- **Given** a build transitions to INTERRUPTED
+- **When** the status is published to Gitea
+- **Then** the commit-status state is `pending` (description "build interrupted"), not `failure`
+  - **and** the re-enqueued build posts `pending` again after the restart
+
+##### Verified by
+
+- [GiteaStateMappingTest](../../src/test/kotlin/de/hoennig/gittally/gitea/GiteaStateMappingTest.kt)
+- [GiteaClientTest — "maps every build status to the documented Gitea state"](../../src/test/kotlin/de/hoennig/gittally/gitea/GiteaClientTest.kt)
+
+## The Solution
+
+`BuildExecutor` gets a `shuttingDown` flag set by a `ContextClosedEvent` listener (`shutdown()`).
+The listener terminates the process trees of all executing builds and waits (bounded, 20s) until their workers have persisted the INTERRUPTED results — the event fires before bean destruction, so the repository and the Gitea client are still usable.
+The final-status classification checks the flag: a non-zero exit or an exception during shutdown becomes INTERRUPTED instead of FAILED; explicit cancellation and a clean SUCCESS still win.
+Workers that pick up a queued build during shutdown return without any transition, leaving it PENDING.
+The command-start and between-commands gates also check the flag, so no new process is spawned (and orphaned) once shutdown began.
+`GiteaStateMapping` now publishes INTERRUPTED as `pending`: an interrupted build is re-enqueued by the startup recovery, so red would be wrong.
+
+The hard invariants hold: nothing is scheduled (the listener is a lifecycle callback), and the listener is a no-op in CLI runs because `build`/`retry` block until completion before the context closes.
+On a Ctrl-C during a CLI build the same logic applies and correctly records INTERRUPTED.
+
+## Additional Changes
+
+- Documented the shutdown behavior in the [architecture skill](../../.claude/skills/architecture/SKILL.md).
+
+## Follow-up PRs
+
+- None planned.
@@ -5,6 +5,8 @@ import de.hoennig.gittally.config.ConfigLoader
 import de.hoennig.gittally.gitea.GiteaClient
 import org.slf4j.LoggerFactory
 import org.springframework.context.ApplicationEventPublisher
+import org.springframework.context.event.ContextClosedEvent
+import org.springframework.context.event.EventListener
 import org.springframework.stereotype.Service
 import java.io.IOException
 import java.io.InputStream
@@ -51,6 +53,9 @@ class BuildExecutor(
    @Volatile
    private var slots: Semaphore? = null

+    /** Set on context close; in-flight builds then finish as INTERRUPTED instead of FAILED. */
+    private val shuttingDown = AtomicBoolean(false)
+
    /** The builds currently executing, newest last (queued builds are PENDING in the repository). */
    fun currentBuilds(): List<RunningBuild> = builds.values.filter { it.running }.map { it.runningBuild }

@@ -108,9 +113,37 @@ class BuildExecutor(
        return true
    }

+    /**
+     * Runs when the application context closes (e.g. systemd SIGTERM): terminates the
+     * process trees of all executing builds and waits (bounded) until their INTERRUPTED
+     * results are persisted, so a shutdown is never recorded as a build failure.
+     * Builds still queued stay PENDING; the watcher's startup recovery re-enqueues
+     * both PENDING and INTERRUPTED builds after the restart.
+     */
+    @EventListener(ContextClosedEvent::class)
+    fun shutdown() {
+        if (!shuttingDown.compareAndSet(false, true)) {
+            return
+        }
+        val executing = builds.values.filter { it.running }
+        if (executing.isEmpty()) {
+            return
+        }
+        log.info("shutdown requested; interrupting {} executing build(s)", executing.size)
+        executing.forEach { build -> build.process?.let { destroyProcessTree(it) } }
+        val deadline = System.nanoTime() + Duration.ofMillis(SHUTDOWN_DRAIN_TIMEOUT_MILLIS).toNanos()
+        while (executing.any { builds.containsKey(it.runningBuild.artifactKey) } && System.nanoTime() < deadline) {
+            Thread.sleep(50)
+        }
+        executing
+            .filter { builds.containsKey(it.runningBuild.artifactKey) }
+            .forEach { log.warn("build of branch {} was not recorded as interrupted in time", it.runningBuild.branch) }
+    }
+
    private fun execute(build: ActiveBuild) {
        var slot: Semaphore? = null
-        var finalStatus = BuildStatus.FAILED
+        // null keeps the persisted status untouched (a queued build stays PENDING over a shutdown)
+        var finalStatus: BuildStatus? = BuildStatus.FAILED
        var workspace: Path? = null
        try {
            slot = slotsFor(build.workingDir)
@@ -119,6 +152,10 @@ class BuildExecutor(
                finalStatus = BuildStatus.CANCELLED
                return
            }
+            if (shuttingDown.get()) {
+                finalStatus = null
+                return
+            }
            build.running = true
            build.runningBuild.runningSince = Instant.now()
            transition(build, BuildStatus.RUNNING, duration = null)
@@ -134,20 +171,34 @@ class BuildExecutor(
                when {
                    build.cancelled.get() -> BuildStatus.CANCELLED
                    exitCode == 0 -> BuildStatus.SUCCESS
+                    shuttingDown.get() -> BuildStatus.INTERRUPTED
                    else -> BuildStatus.FAILED
                }
        } catch (e: Exception) {
-            finalStatus = if (build.cancelled.get()) BuildStatus.CANCELLED else BuildStatus.FAILED
-            log.error("build of branch {} crashed", build.runningBuild.branch, e)
-            appendToLiveLog(build, "\nbuild crashed: ${e.message}\n")
+            finalStatus =
+                when {
+                    build.cancelled.get() -> BuildStatus.CANCELLED
+                    shuttingDown.get() -> BuildStatus.INTERRUPTED
+                    else -> BuildStatus.FAILED
+                }
+            if (finalStatus == BuildStatus.FAILED) {
+                log.error("build of branch {} crashed", build.runningBuild.branch, e)
+                appendToLiveLog(build, "\nbuild crashed: ${e.message}\n")
+            }
        } finally {
-            // pure build time, without the queue wait; null when the build never started executing
-            val duration = build.runningBuild.runningSince?.let { Duration.between(it, Instant.now()) }
-            val result = transition(build, finalStatus, duration)
-            try {
-                artifactStore.persist(result, build.runningBuild.stagingDir, workspace)
-            } catch (e: Exception) {
-                log.warn("could not persist artifacts of {}: {}", result.artifactKey, e.message)
+            if (finalStatus == BuildStatus.INTERRUPTED) {
+                log.info("build of branch {} interrupted by shutdown", build.runningBuild.branch)
+                appendToLiveLog(build, "\nbuild interrupted by shutdown\n")
+            }
+            if (finalStatus != null) {
+                // pure build time, without the queue wait; null when the build never started executing
+                val duration = build.runningBuild.runningSince?.let { Duration.between(it, Instant.now()) }
+                val result = transition(build, finalStatus, duration)
+                try {
+                    artifactStore.persist(result, build.runningBuild.stagingDir, workspace)
+                } catch (e: Exception) {
+                    log.warn("could not persist artifacts of {}: {}", result.artifactKey, e.message)
+                }
            }
            builds.remove(build.runningBuild.artifactKey)
            slot?.release()
@@ -193,7 +244,7 @@ class BuildExecutor(
                            return cleanExitCode
                        }
                    }
-                    if (build.cancelled.get()) {
+                    if (build.cancelled.get() || shuttingDown.get()) {
                        return CANCELLED_EXIT_CODE
                    }
                    return runCommand(build, branchConfig, branchConfig.buildCommand, workspace, stdoutLog, stderrLog, liveLog)
@@ -220,15 +271,15 @@ class BuildExecutor(
                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
+                    // and shutdown too, otherwise they block their slot until they finish
                    build.process = aux
-                    if (build.cancelled.get()) {
+                    if (build.cancelled.get() || shuttingDown.get()) {
                        destroyProcessTree(aux)
                    }
                },
            )
        build.process = process
-        if (build.cancelled.get()) {
+        if (build.cancelled.get() || shuttingDown.get()) {
            destroyProcessTree(process)
        }
        val stdoutPump = pump(process.inputStream, stdoutLog, liveLog)
@@ -409,5 +460,6 @@ class BuildExecutor(
        const val LIVE_LOG_FILE = "build.log"
        private const val CANCELLED_EXIT_CODE = 130
        private const val PUMP_DRAIN_TIMEOUT_MILLIS = 10_000L
+        private const val SHUTDOWN_DRAIN_TIMEOUT_MILLIS = 20_000L
    }
 }
@@ -2,12 +2,17 @@ package de.hoennig.gittally.gitea

 import de.hoennig.gittally.build.BuildStatus

-/** Gitea commit-status state published for this build status. */
+/**
+ * Gitea commit-status state published for this build status.
+ * INTERRUPTED publishes as `pending`, not `failure`: an interrupted build (e.g. a
+ * server shutdown) is re-enqueued by the startup recovery, so the commit must not
+ * turn red in between.
+ */
 fun BuildStatus.toGiteaState(): String =
    when (this) {
        BuildStatus.SUCCESS -> "success"
-        BuildStatus.FAILED, BuildStatus.INTERRUPTED, BuildStatus.CANCELLED -> "failure"
-        BuildStatus.PENDING, BuildStatus.RUNNING -> "pending"
+        BuildStatus.FAILED, BuildStatus.CANCELLED -> "failure"
+        BuildStatus.PENDING, BuildStatus.RUNNING, BuildStatus.INTERRUPTED -> "pending"
    }

 /**
@@ -292,6 +292,74 @@ class BuildExecutorTest : FunSpec() {
            }
        }

+        test("shutdown kills an executing build and records INTERRUPTED, not FAILED") {
+            val h = harness("echo \$\$ > pid-file; sleep 30")
+
+            val build = h.executor.startBuild("main", "abc123", h.workingDir)
+            eventually(10.seconds) {
+                Files.exists(h.workingDir.resolve("pid-file")).shouldBeTrue()
+            }
+            val pid =
+                Files
+                    .readString(h.workingDir.resolve("pid-file"))
+                    .trim()
+                    .toLong()
+
+            h.executor.shutdown()
+
+            // shutdown() drains synchronously, so the result is already persisted
+            h.repository.latestFor("main")?.status shouldBe BuildStatus.INTERRUPTED
+            h.executor.currentBuilds().shouldBeEmpty()
+            ProcessHandle.of(pid).map { it.isAlive }.orElse(false) shouldBe false
+            h.events.map { it.result.status } shouldContainExactly
+                listOf(BuildStatus.PENDING, BuildStatus.RUNNING, BuildStatus.INTERRUPTED)
+            Files.readString(build.liveLogFile) shouldContain "build interrupted by shutdown"
+            verify { h.giteaClient.publishStatus("abc123", BuildStatus.INTERRUPTED, any(), null, h.workingDir) }
+            verify { h.artifactStore.persist(match { it.status == BuildStatus.INTERRUPTED }, build.stagingDir, any()) }
+        }
+
+        test("a build still queued at shutdown stays PENDING for the startup recovery") {
+            val h = harness("sleep 30")
+
+            val first = h.executor.startBuild("main", "sha-1", h.workingDir)
+            val second = h.executor.startBuild("main", "sha-2", h.workingDir)
+            eventually(10.seconds) {
+                h.repository
+                    .history()
+                    .first { it.artifactKey == first.artifactKey }
+                    .status shouldBe BuildStatus.RUNNING
+            }
+
+            h.executor.shutdown()
+
+            h.repository
+                .history()
+                .first { it.artifactKey == first.artifactKey }
+                .status shouldBe BuildStatus.INTERRUPTED
+            // the branch worker drops the queued build without a transition or a process
+            awaitIdle(h)
+            h.repository
+                .history()
+                .first { it.artifactKey == second.artifactKey }
+                .status shouldBe BuildStatus.PENDING
+            h.events
+                .filter { it.result.artifactKey == second.artifactKey }
+                .map { it.result.status } shouldContainExactly listOf(BuildStatus.PENDING)
+            verify(exactly = 0) { h.giteaClient.publishStatus("sha-2", BuildStatus.RUNNING, any(), any(), any()) }
+        }
+
+        test("shutdown without any build in flight is a no-op") {
+            val h = harness("echo ok")
+
+            h.executor.startBuild("main", "abc123", h.workingDir)
+            awaitStatus(h, "main", BuildStatus.SUCCESS)
+            awaitIdle(h)
+
+            h.executor.shutdown()
+
+            h.repository.latestFor("main")?.status shouldBe BuildStatus.SUCCESS
+        }
+
        test("cancel with an unknown artifact key returns false") {
            val h = harness("echo ok")

@@ -121,10 +121,11 @@ class GiteaClientTest :
                    mapOf(
                        BuildStatus.SUCCESS to "success",
                        BuildStatus.FAILED to "failure",
-                        BuildStatus.INTERRUPTED to "failure",
                        BuildStatus.CANCELLED to "failure",
                        BuildStatus.PENDING to "pending",
                        BuildStatus.RUNNING to "pending",
+                        // interrupted builds are re-enqueued on startup, so the commit must not turn red
+                        BuildStatus.INTERRUPTED to "pending",
                    )

                expectedStates.forEach { (status, state) ->
@@ -11,10 +11,11 @@ class GiteaStateMappingTest :
        test("maps each build status to its Gitea state") {
            BuildStatus.SUCCESS.toGiteaState() shouldBe "success"
            BuildStatus.FAILED.toGiteaState() shouldBe "failure"
-            BuildStatus.INTERRUPTED.toGiteaState() shouldBe "failure"
            BuildStatus.CANCELLED.toGiteaState() shouldBe "failure"
            BuildStatus.PENDING.toGiteaState() shouldBe "pending"
            BuildStatus.RUNNING.toGiteaState() shouldBe "pending"
+            // interrupted builds are re-enqueued on startup, so the commit must not turn red
+            BuildStatus.INTERRUPTED.toGiteaState() shouldBe "pending"
        }

        test("maps Gitea states back to build statuses") {