From 40f54a12988530aa54a0e9e730ee16ca38a7d5ea Mon Sep 17 00:00:00 2001 From: mhoennig Date: Mon, 10 Aug 2026 17:26:10 +0200 Subject: [PATCH] Record builds interrupted by a server shutdown as INTERRUPTED, not FAILED A systemd stop killed the running build process and BuildExecutor classified the death as FAILED, posting a red Gitea status; FAILED is not restartable, so the startup recovery never re-enqueued the build. A ContextClosedEvent listener now sets a shuttingDown flag, terminates the process trees of executing builds, and drains until their INTERRUPTED results are persisted. Queued builds stay PENDING without starting a process; recovery re-enqueues both after the restart. INTERRUPTED publishes as Gitea state "pending" instead of "failure", since the build is going to be re-run. Co-Authored-By: Claude Fable 5 --- .claude/skills/architecture/SKILL.md | 2 + ...000-interrupt-builds-on-server-shutdown.md | 87 +++++++++++++++++++ .../hoennig/gittally/build/BuildExecutor.kt | 82 +++++++++++++---- .../gittally/gitea/GiteaStateMapping.kt | 11 ++- .../gittally/build/BuildExecutorTest.kt | 68 +++++++++++++++ .../hoennig/gittally/gitea/GiteaClientTest.kt | 3 +- .../gittally/gitea/GiteaStateMappingTest.kt | 3 +- 7 files changed, 236 insertions(+), 20 deletions(-) create mode 100644 docs/prs/2026-08-10-PR#000-interrupt-builds-on-server-shutdown.md diff --git a/.claude/skills/architecture/SKILL.md b/.claude/skills/architecture/SKILL.md index 943ef9d..67f80b7 100644 --- a/.claude/skills/architecture/SKILL.md +++ b/.claude/skills/architecture/SKILL.md @@ -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/` (`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..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 diff --git a/docs/prs/2026-08-10-PR#000-interrupt-builds-on-server-shutdown.md b/docs/prs/2026-08-10-PR#000-interrupt-builds-on-server-shutdown.md new file mode 100644 index 0000000..5d2a972 --- /dev/null +++ b/docs/prs/2026-08-10-PR#000-interrupt-builds-on-server-shutdown.md @@ -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. diff --git a/src/main/kotlin/de/hoennig/gittally/build/BuildExecutor.kt b/src/main/kotlin/de/hoennig/gittally/build/BuildExecutor.kt index 69f83e4..798c357 100644 --- a/src/main/kotlin/de/hoennig/gittally/build/BuildExecutor.kt +++ b/src/main/kotlin/de/hoennig/gittally/build/BuildExecutor.kt @@ -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 = 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 } } diff --git a/src/main/kotlin/de/hoennig/gittally/gitea/GiteaStateMapping.kt b/src/main/kotlin/de/hoennig/gittally/gitea/GiteaStateMapping.kt index 985eea2..2567b21 100644 --- a/src/main/kotlin/de/hoennig/gittally/gitea/GiteaStateMapping.kt +++ b/src/main/kotlin/de/hoennig/gittally/gitea/GiteaStateMapping.kt @@ -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" } /** diff --git a/src/test/kotlin/de/hoennig/gittally/build/BuildExecutorTest.kt b/src/test/kotlin/de/hoennig/gittally/build/BuildExecutorTest.kt index 2158aa2..be8f17c 100644 --- a/src/test/kotlin/de/hoennig/gittally/build/BuildExecutorTest.kt +++ b/src/test/kotlin/de/hoennig/gittally/build/BuildExecutorTest.kt @@ -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") diff --git a/src/test/kotlin/de/hoennig/gittally/gitea/GiteaClientTest.kt b/src/test/kotlin/de/hoennig/gittally/gitea/GiteaClientTest.kt index dc9cfb2..a63c03d 100644 --- a/src/test/kotlin/de/hoennig/gittally/gitea/GiteaClientTest.kt +++ b/src/test/kotlin/de/hoennig/gittally/gitea/GiteaClientTest.kt @@ -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) -> diff --git a/src/test/kotlin/de/hoennig/gittally/gitea/GiteaStateMappingTest.kt b/src/test/kotlin/de/hoennig/gittally/gitea/GiteaStateMappingTest.kt index 4770441..a1987b6 100644 --- a/src/test/kotlin/de/hoennig/gittally/gitea/GiteaStateMappingTest.kt +++ b/src/test/kotlin/de/hoennig/gittally/gitea/GiteaStateMappingTest.kt @@ -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") {