From 09ed193ac7468088042c0a81431bf5874af41165 Mon Sep 17 00:00:00 2001 From: mhoennig Date: Wed, 26 Aug 2026 13:30:11 +0200 Subject: [PATCH] Never prune queued/running builds; dedup manual triggers Two defects seen live on vm4006 when a merged branch was deleted from origin while its last build still ran: - The result prune removed the PENDING/RUNNING entries of branches gone from origin, so the executing build vanished from UI and history and the queue looked stuck. Prune now never touches a PENDING or RUNNING entry (worktrees were already protected). The startup recovery closes out an orphaned PENDING of a gone branch as INTERRUPTED, so the new immunity cannot leak entries. - The apparent hang invited restart clicks, and each click stacked another build of the same commit. startBuild now returns the already queued or executing build of the same branch and commit instead of a duplicate; cancel-requested builds do not block re-queueing, and re-running a finished build stays possible. Co-Authored-By: Claude --- .../hoennig/gittally/build/BuildExecutor.kt | 12 +++++ .../gittally/build/BuildResultRepository.kt | 6 ++- .../build/FileBuildResultRepository.kt | 44 +++++++++++-------- .../de/hoennig/gittally/watcher/Watcher.kt | 5 +++ .../gittally/build/BuildExecutorTest.kt | 25 +++++++++++ .../build/FileBuildResultRepositoryTest.kt | 25 +++++++++++ .../hoennig/gittally/watcher/WatcherTest.kt | 14 ++++++ 7 files changed, 111 insertions(+), 20 deletions(-) diff --git a/src/main/kotlin/de/hoennig/gittally/build/BuildExecutor.kt b/src/main/kotlin/de/hoennig/gittally/build/BuildExecutor.kt index 798c357..9329fdd 100644 --- a/src/main/kotlin/de/hoennig/gittally/build/BuildExecutor.kt +++ b/src/main/kotlin/de/hoennig/gittally/build/BuildExecutor.kt @@ -63,12 +63,24 @@ class BuildExecutor( * Persists a PENDING result and queues the build; returns immediately. * A build of the same branch waits until the branch's previous build finished; * builds of other branches run concurrently while slots are free. + * While a build of the same branch and commit is already queued or executing (and + * not cancel-requested), that build is returned instead of stacking a duplicate — + * a double-triggered UI restart must not queue the same commit twice. Re-running + * a *finished* build stays possible; this only guards the active queue. */ fun startBuild( branch: String, commit: String, workingDir: Path = Paths.get("."), ): RunningBuild { + val duplicate = + builds.values.firstOrNull { + !it.cancelled.get() && it.runningBuild.branch == branch && it.runningBuild.commit == commit + } + if (duplicate != null) { + log.info("build of branch {} at commit {} is already queued or running; not queueing a duplicate", branch, commit) + return duplicate.runningBuild + } val startedAt = Instant.now() val stagingDir = Files.createTempDirectory("gittally-build-") val runningBuild = diff --git a/src/main/kotlin/de/hoennig/gittally/build/BuildResultRepository.kt b/src/main/kotlin/de/hoennig/gittally/build/BuildResultRepository.kt index f184a1b..5e52538 100644 --- a/src/main/kotlin/de/hoennig/gittally/build/BuildResultRepository.kt +++ b/src/main/kotlin/de/hoennig/gittally/build/BuildResultRepository.kt @@ -43,7 +43,11 @@ interface BuildResultRepository { * cutoff are dropped even within the retention count — except each branch's newest entry, * so dormant branches keep their last status. With [keepLatestGreen], the newest SUCCESS * entry of each surviving branch is kept even beyond both limits, so the permanent - * `/branches/…` artifact links stay valid while newer builds fail. Returns the removed entries. + * `/branches/…` artifact links stay valid while newer builds fail. + * PENDING and RUNNING entries are never removed, regardless of all limits and even + * when their branch is gone from [originBranches] — a queued or executing build + * belongs to the executor, and pruning its result would make it invisible in UI + * and history. Returns the removed entries. */ fun prune( originBranches: Collection, diff --git a/src/main/kotlin/de/hoennig/gittally/build/FileBuildResultRepository.kt b/src/main/kotlin/de/hoennig/gittally/build/FileBuildResultRepository.kt index 0520f70..9baa398 100644 --- a/src/main/kotlin/de/hoennig/gittally/build/FileBuildResultRepository.kt +++ b/src/main/kotlin/de/hoennig/gittally/build/FileBuildResultRepository.kt @@ -129,26 +129,32 @@ class FileBuildResultRepository( synchronized(lock) { val results = load() val originBranchSet = originBranches.toSet() + // a queued or executing build belongs to the executor, never to retention: + // pruning its result would make the build invisible in UI and history — seen + // live when a merged branch was deleted from origin while its last build ran + val active = + results.filter { it.status == BuildStatus.PENDING || it.status == BuildStatus.RUNNING } val kept = - results - .filter { it.branch in originBranchSet } - .groupBy { it.branch } - .values - .flatMap { entries -> - val newest = - entries - .sortedByDescending { it.startedAt } - .take(retentionPerBranch.coerceAtLeast(0)) - .filterIndexed { index, entry -> - // the branch's newest entry is never age-pruned - index == 0 || retentionCutoff == null || !entry.startedAt.isBefore(retentionCutoff) - } - val latestGreen = - entries - .filter { keepLatestGreen && it.status == BuildStatus.SUCCESS } - .maxByOrNull { it.startedAt } - newest + listOfNotNull(latestGreen) - }.toSet() + active.toSet() + + results + .filter { it.branch in originBranchSet } + .groupBy { it.branch } + .values + .flatMap { entries -> + val newest = + entries + .sortedByDescending { it.startedAt } + .take(retentionPerBranch.coerceAtLeast(0)) + .filterIndexed { index, entry -> + // the branch's newest entry is never age-pruned + index == 0 || retentionCutoff == null || !entry.startedAt.isBefore(retentionCutoff) + } + val latestGreen = + entries + .filter { keepLatestGreen && it.status == BuildStatus.SUCCESS } + .maxByOrNull { it.startedAt } + newest + listOfNotNull(latestGreen) + }.toSet() val removed = results.filterNot { it in kept } if (removed.isNotEmpty()) { save(results.filter { it in kept }) diff --git a/src/main/kotlin/de/hoennig/gittally/watcher/Watcher.kt b/src/main/kotlin/de/hoennig/gittally/watcher/Watcher.kt index 8584986..b0a74ca 100644 --- a/src/main/kotlin/de/hoennig/gittally/watcher/Watcher.kt +++ b/src/main/kotlin/de/hoennig/gittally/watcher/Watcher.kt @@ -97,6 +97,11 @@ class Watcher( for (result in restartable) { val commit = gitService.originHeadCommit(result.branch, workingDir) if (commit == null) { + if (result.status == BuildStatus.PENDING) { + // a PENDING entry is prune-immune (it normally belongs to the executor); + // close this orphan out so the gone branch can be pruned + repository.updateByArtifactKey(result.artifactKey) { it.copy(status = BuildStatus.INTERRUPTED) } + } log.info("not restarting build of branch {}: branch is gone from origin", result.branch) continue } diff --git a/src/test/kotlin/de/hoennig/gittally/build/BuildExecutorTest.kt b/src/test/kotlin/de/hoennig/gittally/build/BuildExecutorTest.kt index be8f17c..ad0beb5 100644 --- a/src/test/kotlin/de/hoennig/gittally/build/BuildExecutorTest.kt +++ b/src/test/kotlin/de/hoennig/gittally/build/BuildExecutorTest.kt @@ -219,6 +219,31 @@ class BuildExecutorTest : FunSpec() { } } + test("startBuild returns the active build of the same branch and commit instead of stacking a duplicate") { + val h = harness("sleep 30") + + val first = h.executor.startBuild("main", "abc123", h.workingDir) + // a double-triggered UI restart: same branch, same commit, while queued or running + val duplicate = h.executor.startBuild("main", "abc123", h.workingDir) + duplicate.artifactKey shouldBe first.artifactKey + h.repository.history().map { it.artifactKey } shouldContainExactly listOf(first.artifactKey) + + // another commit of the branch is a distinct build, queued behind the first + val newerCommit = h.executor.startBuild("main", "abc124", h.workingDir) + newerCommit.artifactKey shouldNotBe first.artifactKey + + // a cancel-requested build no longer blocks re-queueing its commit + h.executor.cancel(first.artifactKey).shouldBeTrue() + val again = h.executor.startBuild("main", "abc123", h.workingDir) + again.artifactKey shouldNotBe first.artifactKey + + h.executor.cancel(newerCommit.artifactKey).shouldBeTrue() + h.executor.cancel(again.artifactKey).shouldBeTrue() + eventually(30.seconds) { + h.executor.currentBuilds().shouldBeEmpty() + } + } + 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/FileBuildResultRepositoryTest.kt b/src/test/kotlin/de/hoennig/gittally/build/FileBuildResultRepositoryTest.kt index 2a0aad4..2c7c596 100644 --- a/src/test/kotlin/de/hoennig/gittally/build/FileBuildResultRepositoryTest.kt +++ b/src/test/kotlin/de/hoennig/gittally/build/FileBuildResultRepositoryTest.kt @@ -220,6 +220,31 @@ class FileBuildResultRepositoryTest : FunSpec() { ) } + test("prune never removes queued or running results, even of branches gone from origin") { + val repository = FileBuildResultRepository(newFile()) + // a merged branch, deleted from origin while its last build still runs + repository.append(result(branch = "merged", status = BuildStatus.FAILED, startedOffsetSeconds = 0)) + repository.append(result(branch = "merged", status = BuildStatus.RUNNING, startedOffsetSeconds = 60)) + // a queued build beyond the retention count of its branch + repository.append(result(branch = "main", status = BuildStatus.PENDING, startedOffsetSeconds = 0)) + repository.append(result(branch = "main", startedOffsetSeconds = 60)) + + val removed = + repository.prune( + originBranches = listOf("main"), + retentionPerBranch = 1, + retentionCutoff = baseTime.plusSeconds(30), + ) + + removed shouldContainExactly listOf(result(branch = "merged", status = BuildStatus.FAILED, startedOffsetSeconds = 0)) + repository.history() shouldContainExactlyInAnyOrder + listOf( + result(branch = "merged", status = BuildStatus.RUNNING, startedOffsetSeconds = 60), + result(branch = "main", status = BuildStatus.PENDING, startedOffsetSeconds = 0), + result(branch = "main", startedOffsetSeconds = 60), + ) + } + test("prune drops entries older than the retention cutoff even within the retention count") { val repository = FileBuildResultRepository(newFile()) repository.append(result(branch = "main", status = BuildStatus.FAILED, startedOffsetSeconds = 0)) diff --git a/src/test/kotlin/de/hoennig/gittally/watcher/WatcherTest.kt b/src/test/kotlin/de/hoennig/gittally/watcher/WatcherTest.kt index b56b5e1..4613c84 100644 --- a/src/test/kotlin/de/hoennig/gittally/watcher/WatcherTest.kt +++ b/src/test/kotlin/de/hoennig/gittally/watcher/WatcherTest.kt @@ -420,6 +420,20 @@ class WatcherTest : FunSpec() { harness.startedBuilds shouldContainExactly listOf("main" to "commit-2") } + test("startup recovery closes out an orphaned PENDING build of a branch gone from origin") { + val harness = Harness() + val orphan = harness.seed("gone", BuildStatus.PENDING, commit = "commit-1") + + harness.watcher.recoverOnStartup(harness.workingDir) + + // PENDING is prune-immune; left as-is, the gone branch could never be pruned + harness.startedBuilds.shouldBeEmpty() + harness.repository + .history() + .first { it.artifactKey == orphan.artifactKey } + .status shouldBe BuildStatus.INTERRUPTED + } + test("poll prunes results, artifacts, and worktrees of branches gone from origin") { val harness = Harness() harness.seed("main", BuildStatus.SUCCESS, commit = "commit-1")