From 59a813a86dbad0ffd151f31ce7877fdad847f7a7 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Wed, 12 Aug 2026 18:11:20 +1000 Subject: [PATCH 1/3] fix: include every commit in build information The build information file only ever carried the most recent 100 commits, because the TeamCity client requests changes without a count and TeamCity then applies its default page size. Fetch the changes over the REST API with explicit paging instead, and flag the build information when the commit list is capped. Closes #168 Co-Authored-By: Claude Opus 5 (1M context) --- ...BuildInfoUtils.java => CommitHistory.java} | 29 ++- .../teamcity/agent/CommitHistoryFetcher.java | 194 +++++++++++++++++ .../agent/OctopusBuildInformation.java | 1 + .../OctopusBuildInformationBuildProcess.java | 9 +- .../agent/OctopusBuildInformationBuilder.java | 15 +- .../cli/BuildInformationBuildProcess.java | 11 +- .../agent/CommitHistoryFetcherTest.java | 195 ++++++++++++++++++ 7 files changed, 419 insertions(+), 35 deletions(-) rename octopus-agent/src/main/java/octopus/teamcity/agent/{BuildInfoUtils.java => CommitHistory.java} (51%) create mode 100644 octopus-agent/src/main/java/octopus/teamcity/agent/CommitHistoryFetcher.java create mode 100644 octopus-agent/src/test/java/octopus/teamcity/agent/CommitHistoryFetcherTest.java diff --git a/octopus-agent/src/main/java/octopus/teamcity/agent/BuildInfoUtils.java b/octopus-agent/src/main/java/octopus/teamcity/agent/CommitHistory.java similarity index 51% rename from octopus-agent/src/main/java/octopus/teamcity/agent/BuildInfoUtils.java rename to octopus-agent/src/main/java/octopus/teamcity/agent/CommitHistory.java index 30ddc6b9..6f61332b 100644 --- a/octopus-agent/src/main/java/octopus/teamcity/agent/BuildInfoUtils.java +++ b/octopus-agent/src/main/java/octopus/teamcity/agent/CommitHistory.java @@ -15,30 +15,25 @@ */ package octopus.teamcity.agent; -import java.util.ArrayList; import java.util.List; -import com.google.gson.Gson; -import com.google.gson.GsonBuilder; import octopus.teamcity.common.Commit; -import org.jetbrains.teamcity.rest.Build; -import org.jetbrains.teamcity.rest.Change; -public class BuildInfoUtils { - public static String createJsonCommitHistory(final Build build) { - final List changes = build.fetchChanges(); +public class CommitHistory { - final List commits = new ArrayList<>(); - for (Change change : changes) { + private final List commits; + private final String incompleteDataWarning; - final Commit c = new Commit(); - c.Id = change.getVersion(); - c.Comment = change.getComment(); + public CommitHistory(final List commits, final String incompleteDataWarning) { + this.commits = commits; + this.incompleteDataWarning = incompleteDataWarning; + } - commits.add(c); - } + public List getCommits() { + return commits; + } - final Gson gson = new GsonBuilder().setPrettyPrinting().disableHtmlEscaping().create(); - return gson.toJson(commits); + public String getIncompleteDataWarning() { + return incompleteDataWarning; } } diff --git a/octopus-agent/src/main/java/octopus/teamcity/agent/CommitHistoryFetcher.java b/octopus-agent/src/main/java/octopus/teamcity/agent/CommitHistoryFetcher.java new file mode 100644 index 00000000..83169ffd --- /dev/null +++ b/octopus-agent/src/main/java/octopus/teamcity/agent/CommitHistoryFetcher.java @@ -0,0 +1,194 @@ +/* + * Copyright 2000-2012 Octopus Deploy Pty. Ltd. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package octopus.teamcity.agent; + +import java.io.IOException; +import java.io.InputStream; +import java.net.HttpURLConnection; +import java.net.URL; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.Base64; +import java.util.List; + +import com.google.gson.Gson; +import com.google.gson.GsonBuilder; +import jetbrains.buildServer.agent.BuildProgressLogger; +import octopus.teamcity.common.Commit; +import org.apache.commons.io.IOUtils; +import org.jetbrains.teamcity.rest.Build; +import org.jetbrains.teamcity.rest.Change; + +public class CommitHistoryFetcher { + + static final int PAGE_SIZE = 1000; + static final int MAX_COMMITS = 10000; + static final int TEAMCITY_DEFAULT_PAGE_SIZE = 100; + + private static final Gson GSON = new GsonBuilder().create(); + + interface ChangePageRequester { + String get(String url) throws IOException; + } + + private final String serverUrl; + private final BuildProgressLogger logger; + private final ChangePageRequester requester; + + public CommitHistoryFetcher( + final String serverUrl, + final String accessUser, + final String accessCode, + final BuildProgressLogger logger) { + this(serverUrl, logger, new BasicAuthRequester(accessUser, accessCode)); + } + + CommitHistoryFetcher( + final String serverUrl, + final BuildProgressLogger logger, + final ChangePageRequester requester) { + this.serverUrl = + serverUrl.endsWith("/") ? serverUrl.substring(0, serverUrl.length() - 1) : serverUrl; + this.logger = logger; + this.requester = requester; + } + + public CommitHistory fetch(final Build build, final long buildId) { + try { + return fetchAllPages(buildId); + } catch (Exception ex) { + logger.warning( + "Unable to read the full change list from the TeamCity REST API (" + + ex + + "). Falling back to the first page of changes only."); + final List commits = toCommits(build.fetchChanges()); + return new CommitHistory(commits, fallbackWarning(commits.size())); + } + } + + // The client library requests changes without a count, so TeamCity caps it at its default page. + private static String fallbackWarning(final int commitCount) { + if (commitCount < TEAMCITY_DEFAULT_PAGE_SIZE) { + return null; + } + return "The full change list could not be read from TeamCity, so only the first " + + TEAMCITY_DEFAULT_PAGE_SIZE + + " commits of this build were included in the build information."; + } + + private CommitHistory fetchAllPages(final long buildId) throws IOException { + final List commits = new ArrayList<>(); + int start = 0; + while (true) { + final List page = parsePage(requester.get(changesUrl(buildId, start))); + commits.addAll(page); + + if (page.size() < PAGE_SIZE) { + return new CommitHistory(commits, null); + } + if (commits.size() >= MAX_COMMITS) { + return new CommitHistory( + new ArrayList<>(commits.subList(0, MAX_COMMITS)), + "Only the first " + + MAX_COMMITS + + " commits of this build were included in the build information."); + } + start += PAGE_SIZE; + } + } + + private String changesUrl(final long buildId, final int start) { + // Without an explicit count TeamCity returns only its default page of 100 changes. + return serverUrl + + "/httpAuth/app/rest/changes?locator=build:(id:" + + buildId + + "),count:" + + PAGE_SIZE + + ",start:" + + start + + "&fields=change(version,comment)"; + } + + private static List parsePage(final String body) { + final ChangesPage page = GSON.fromJson(body, ChangesPage.class); + final List commits = new ArrayList<>(); + if (page == null || page.change == null) { + return commits; + } + for (final ChangeJson change : page.change) { + commits.add(toCommit(change.version, change.comment)); + } + return commits; + } + + private static List toCommits(final List changes) { + final List commits = new ArrayList<>(); + for (final Change change : changes) { + commits.add(toCommit(change.getVersion(), change.getComment())); + } + return commits; + } + + private static Commit toCommit(final String version, final String comment) { + final Commit commit = new Commit(); + commit.Id = version; + commit.Comment = comment; + return commit; + } + + private static final class ChangesPage { + List change; + } + + private static final class ChangeJson { + String version; + String comment; + } + + private static final class BasicAuthRequester implements ChangePageRequester { + + private static final int TIMEOUT_MILLIS = 30000; + + private final String authorization; + + BasicAuthRequester(final String accessUser, final String accessCode) { + final byte[] credentials = (accessUser + ":" + accessCode).getBytes(StandardCharsets.UTF_8); + this.authorization = "Basic " + Base64.getEncoder().encodeToString(credentials); + } + + @Override + public String get(final String url) throws IOException { + final HttpURLConnection connection = (HttpURLConnection) new URL(url).openConnection(); + try { + connection.setRequestMethod("GET"); + connection.setConnectTimeout(TIMEOUT_MILLIS); + connection.setReadTimeout(TIMEOUT_MILLIS); + connection.setRequestProperty("Authorization", authorization); + connection.setRequestProperty("Accept", "application/json"); + + final int statusCode = connection.getResponseCode(); + if (statusCode != HttpURLConnection.HTTP_OK) { + throw new IOException("TeamCity responded with HTTP " + statusCode + " for " + url); + } + try (InputStream responseStream = connection.getInputStream()) { + return IOUtils.toString(responseStream, StandardCharsets.UTF_8); + } + } finally { + connection.disconnect(); + } + } + } +} diff --git a/octopus-agent/src/main/java/octopus/teamcity/agent/OctopusBuildInformation.java b/octopus-agent/src/main/java/octopus/teamcity/agent/OctopusBuildInformation.java index 3a621f78..73e044e6 100644 --- a/octopus-agent/src/main/java/octopus/teamcity/agent/OctopusBuildInformation.java +++ b/octopus-agent/src/main/java/octopus/teamcity/agent/OctopusBuildInformation.java @@ -14,6 +14,7 @@ public class OctopusBuildInformation { public String VcsCommitNumber; public List Commits; + public String IncompleteDataWarning; public OctopusBuildInformation() { BuildEnvironment = "TeamCity"; diff --git a/octopus-agent/src/main/java/octopus/teamcity/agent/OctopusBuildInformationBuildProcess.java b/octopus-agent/src/main/java/octopus/teamcity/agent/OctopusBuildInformationBuildProcess.java index 790e98f0..c9c8791a 100644 --- a/octopus-agent/src/main/java/octopus/teamcity/agent/OctopusBuildInformationBuildProcess.java +++ b/octopus-agent/src/main/java/octopus/teamcity/agent/OctopusBuildInformationBuildProcess.java @@ -16,8 +16,6 @@ package octopus.teamcity.agent; -import static octopus.teamcity.agent.BuildInfoUtils.createJsonCommitHistory; - import java.io.File; import java.nio.charset.StandardCharsets; import java.nio.file.Paths; @@ -91,13 +89,18 @@ protected OctopusCommandBuilder createCommand() { final String branch = BranchResolver.resolve(parameters, constants, restfulBuild.getBranch().getName()); + final CommitHistory commitHistory = + new CommitHistoryFetcher( + teamCityServerUrl, build.getAccessUser(), build.getAccessCode(), buildLogger) + .fetch(restfulBuild, build.getBuildId()); + final OctopusBuildInformation buildInformation = builder.build( sharedConfigParameters.get("octopus_vcstype"), sharedConfigParameters.get("vcsroot.url"), sharedConfigParameters.get("build.vcs.number"), branch, - createJsonCommitHistory(restfulBuild), + commitHistory, buildUrlString, buildNumber); diff --git a/octopus-agent/src/main/java/octopus/teamcity/agent/OctopusBuildInformationBuilder.java b/octopus-agent/src/main/java/octopus/teamcity/agent/OctopusBuildInformationBuilder.java index 5131f49e..ea48c878 100644 --- a/octopus-agent/src/main/java/octopus/teamcity/agent/OctopusBuildInformationBuilder.java +++ b/octopus-agent/src/main/java/octopus/teamcity/agent/OctopusBuildInformationBuilder.java @@ -1,12 +1,5 @@ package octopus.teamcity.agent; -import java.util.List; - -import com.google.gson.Gson; -import com.google.gson.GsonBuilder; -import com.google.gson.reflect.TypeToken; -import octopus.teamcity.common.Commit; - public class OctopusBuildInformationBuilder { public OctopusBuildInformation build( @@ -14,16 +7,14 @@ public OctopusBuildInformation build( final String vcsRoot, final String vcsCommitNumber, final String branch, - final String commitsJson, + final CommitHistory commitHistory, final String externalBuildUrl, final String buildNumber) { final OctopusBuildInformation buildInformation = new OctopusBuildInformation(); - final Gson gson = new GsonBuilder().create(); - - buildInformation.Commits = - gson.fromJson(commitsJson, new TypeToken>() {}.getType()); + buildInformation.Commits = commitHistory.getCommits(); + buildInformation.IncompleteDataWarning = commitHistory.getIncompleteDataWarning(); buildInformation.Branch = branch; buildInformation.BuildNumber = buildNumber; buildInformation.BuildUrl = externalBuildUrl; diff --git a/octopus-agent/src/main/java/octopus/teamcity/agent/cli/BuildInformationBuildProcess.java b/octopus-agent/src/main/java/octopus/teamcity/agent/cli/BuildInformationBuildProcess.java index bd0fdced..55598602 100644 --- a/octopus-agent/src/main/java/octopus/teamcity/agent/cli/BuildInformationBuildProcess.java +++ b/octopus-agent/src/main/java/octopus/teamcity/agent/cli/BuildInformationBuildProcess.java @@ -16,8 +16,6 @@ package octopus.teamcity.agent.cli; -import static octopus.teamcity.agent.BuildInfoUtils.createJsonCommitHistory; - import java.io.File; import java.nio.charset.StandardCharsets; import java.nio.file.Paths; @@ -28,6 +26,8 @@ import jetbrains.buildServer.agent.AgentRunningBuild; import jetbrains.buildServer.agent.BuildRunnerContext; import octopus.teamcity.agent.BranchResolver; +import octopus.teamcity.agent.CommitHistory; +import octopus.teamcity.agent.CommitHistoryFetcher; import octopus.teamcity.agent.OctopusBuildInformation; import octopus.teamcity.agent.OctopusBuildInformationBuilder; import octopus.teamcity.agent.OctopusBuildInformationWriter; @@ -95,13 +95,18 @@ protected List createCommand() { final String branch = BranchResolver.resolve(parameters, constants, restfulBuild.getBranch().getName()); + final CommitHistory commitHistory = + new CommitHistoryFetcher( + teamCityServerUrl, build.getAccessUser(), build.getAccessCode(), logger) + .fetch(restfulBuild, build.getBuildId()); + final OctopusBuildInformation buildInformation = builder.build( sharedConfigParameters.get("octopus_vcstype"), sharedConfigParameters.get("vcsroot.url"), sharedConfigParameters.get("build.vcs.number"), branch, - createJsonCommitHistory(restfulBuild), + commitHistory, buildUrlString, buildNumber); diff --git a/octopus-agent/src/test/java/octopus/teamcity/agent/CommitHistoryFetcherTest.java b/octopus-agent/src/test/java/octopus/teamcity/agent/CommitHistoryFetcherTest.java new file mode 100644 index 00000000..31e76a86 --- /dev/null +++ b/octopus-agent/src/test/java/octopus/teamcity/agent/CommitHistoryFetcherTest.java @@ -0,0 +1,195 @@ +package octopus.teamcity.agent; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; + +import jetbrains.buildServer.agent.BuildProgressLogger; +import octopus.teamcity.common.Commit; +import org.jetbrains.teamcity.rest.Build; +import org.jetbrains.teamcity.rest.Change; +import org.junit.jupiter.api.Test; + +class CommitHistoryFetcherTest { + + private static final String SERVER_URL = "https://teamcity.example.com/"; + private static final long BUILD_ID = 42L; + + private final BuildProgressLogger logger = mock(BuildProgressLogger.class); + private final List requestedUrls = new ArrayList<>(); + + @Test + void returnsEveryCommitFromASinglePage() { + final CommitHistory history = fetcherReturningPagesOf(3).fetch(mock(Build.class), BUILD_ID); + + assertThat(history.getCommits()) + .extracting(commit -> commit.Id) + .containsExactly("commit-0", "commit-1", "commit-2"); + assertThat(history.getIncompleteDataWarning()).isNull(); + assertThat(requestedUrls).hasSize(1); + } + + @Test + void pagesUntilAPageIsShorterThanThePageSize() { + final CommitHistory history = + fetcherReturningPagesOf(CommitHistoryFetcher.PAGE_SIZE, CommitHistoryFetcher.PAGE_SIZE, 250) + .fetch(mock(Build.class), BUILD_ID); + + assertThat(history.getCommits()).hasSize(2250); + assertThat(history.getCommits().get(0).Id).isEqualTo("commit-0"); + assertThat(history.getCommits().get(1000).Id).isEqualTo("commit-1000"); + assertThat(history.getCommits().get(2249).Id).isEqualTo("commit-2249"); + assertThat(history.getIncompleteDataWarning()).isNull(); + assertThat(requestedUrls) + .hasSize(3) + .allSatisfy(url -> assertThat(url).contains("count:" + CommitHistoryFetcher.PAGE_SIZE)); + assertThat(requestedUrls.get(0)).contains(",start:0&"); + assertThat(requestedUrls.get(1)).contains(",start:1000&"); + assertThat(requestedUrls.get(2)).contains(",start:2000&"); + } + + @Test + void asksForAnExplicitCountSoTeamCityDoesNotCapThePageAt100() { + fetcherReturningPagesOf(1).fetch(mock(Build.class), BUILD_ID); + + assertThat(requestedUrls) + .containsExactly( + "https://teamcity.example.com/httpAuth/app/rest/changes" + + "?locator=build:(id:42),count:1000,start:0&fields=change(version,comment)"); + } + + @Test + void stopsAtTheCapAndReportsTheTruncation() { + final int fullPages = CommitHistoryFetcher.MAX_COMMITS / CommitHistoryFetcher.PAGE_SIZE; + final int[] pageSizes = new int[fullPages + 1]; + Arrays.fill(pageSizes, CommitHistoryFetcher.PAGE_SIZE); + + final CommitHistory history = + fetcherReturningPagesOf(pageSizes).fetch(mock(Build.class), BUILD_ID); + + assertThat(history.getCommits()).hasSize(CommitHistoryFetcher.MAX_COMMITS); + assertThat(history.getIncompleteDataWarning()) + .contains(String.valueOf(CommitHistoryFetcher.MAX_COMMITS)); + assertThat(requestedUrls).hasSize(fullPages); + } + + @Test + void mapsChangeVersionToIdAndChangeCommentToComment() { + final CommitHistoryFetcher fetcher = + new CommitHistoryFetcher( + SERVER_URL, + logger, + url -> "{\"count\":1,\"change\":[{\"version\":\"deadbeef\",\"comment\":\"Fix it\"}]}"); + + final List commits = fetcher.fetch(mock(Build.class), BUILD_ID).getCommits(); + + assertThat(commits).hasSize(1); + assertThat(commits.get(0).Id).isEqualTo("deadbeef"); + assertThat(commits.get(0).Comment).isEqualTo("Fix it"); + } + + @Test + void fallsBackToTheTeamCityClientWhenTheRequestFails() { + final CommitHistoryFetcher fetcher = + new CommitHistoryFetcher( + SERVER_URL, + logger, + url -> { + throw new IOException("connection refused"); + }); + + final CommitHistory history = fetcher.fetch(buildWithChange("abc123", "a change"), BUILD_ID); + + assertThat(history.getCommits()).hasSize(1); + assertThat(history.getCommits().get(0).Id).isEqualTo("abc123"); + assertThat(history.getCommits().get(0).Comment).isEqualTo("a change"); + assertThat(history.getIncompleteDataWarning()).isNull(); + verify(logger).warning(anyString()); + } + + @Test + void reportsTruncationWhenTheFallbackHitsTheTeamCityDefaultPageSize() { + final CommitHistoryFetcher fetcher = + new CommitHistoryFetcher( + SERVER_URL, + logger, + url -> { + throw new IOException("connection refused"); + }); + + final CommitHistory history = + fetcher.fetch(buildWithChanges(CommitHistoryFetcher.TEAMCITY_DEFAULT_PAGE_SIZE), BUILD_ID); + + assertThat(history.getCommits()).hasSize(CommitHistoryFetcher.TEAMCITY_DEFAULT_PAGE_SIZE); + assertThat(history.getIncompleteDataWarning()) + .contains(String.valueOf(CommitHistoryFetcher.TEAMCITY_DEFAULT_PAGE_SIZE)); + } + + @Test + void fallsBackToTheTeamCityClientWhenTheResponseIsNotValidJson() { + final CommitHistoryFetcher fetcher = + new CommitHistoryFetcher(SERVER_URL, logger, url -> "not json"); + + final CommitHistory history = fetcher.fetch(buildWithChange("abc123", "a change"), BUILD_ID); + + assertThat(history.getCommits()).extracting(commit -> commit.Id).containsExactly("abc123"); + verify(logger).warning(anyString()); + } + + private CommitHistoryFetcher fetcherReturningPagesOf(final int... pageSizes) { + return new CommitHistoryFetcher( + SERVER_URL, + logger, + url -> { + final int pageIndex = requestedUrls.size(); + requestedUrls.add(url); + return changesJson(pageIndex * CommitHistoryFetcher.PAGE_SIZE, pageSizes[pageIndex]); + }); + } + + private static String changesJson(final int firstCommitIndex, final int count) { + final StringBuilder json = + new StringBuilder("{\"count\":").append(count).append(",\"change\":["); + for (int i = 0; i < count; i++) { + if (i > 0) { + json.append(','); + } + final int commitIndex = firstCommitIndex + i; + json.append("{\"version\":\"commit-") + .append(commitIndex) + .append("\",\"comment\":\"comment ") + .append(commitIndex) + .append("\"}"); + } + return json.append("]}").toString(); + } + + private static Build buildWithChange(final String version, final String comment) { + final Change change = mock(Change.class); + when(change.getVersion()).thenReturn(version); + when(change.getComment()).thenReturn(comment); + final Build build = mock(Build.class); + when(build.fetchChanges()).thenReturn(Collections.singletonList(change)); + return build; + } + + private static Build buildWithChanges(final int count) { + final List changes = new ArrayList<>(); + for (int i = 0; i < count; i++) { + final Change change = mock(Change.class); + when(change.getVersion()).thenReturn("commit-" + i); + changes.add(change); + } + final Build build = mock(Build.class); + when(build.fetchChanges()).thenReturn(changes); + return build; + } +} From dad606fb95541d8aebcba4a55ca413245299d0b4 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Thu, 20 Aug 2026 14:34:14 +1000 Subject: [PATCH 2/3] fix: keep the commits already read when a page fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things the review picked up on the paging code: A page that fails part way through no longer discards the pages that already succeeded — the commits read so far are kept and flagged as truncated, instead of falling back to the client call and dropping to 100. The truncation warning now names the number of commits actually included rather than assuming TeamCity's default page size. The cap check also treated a build with exactly 10000 commits as truncated. It now needs a page past the cap to declare truncation, so a complete list is never stamped with an incomplete-data warning. Co-Authored-By: Claude Opus 5 (1M context) --- .../teamcity/agent/CommitHistoryFetcher.java | 31 +++++++++---- .../agent/CommitHistoryFetcherTest.java | 43 ++++++++++++++++++- 2 files changed, 64 insertions(+), 10 deletions(-) diff --git a/octopus-agent/src/main/java/octopus/teamcity/agent/CommitHistoryFetcher.java b/octopus-agent/src/main/java/octopus/teamcity/agent/CommitHistoryFetcher.java index 83169ffd..7ecbabc4 100644 --- a/octopus-agent/src/main/java/octopus/teamcity/agent/CommitHistoryFetcher.java +++ b/octopus-agent/src/main/java/octopus/teamcity/agent/CommitHistoryFetcher.java @@ -67,15 +67,25 @@ public CommitHistoryFetcher( } public CommitHistory fetch(final Build build, final long buildId) { + final List commits = new ArrayList<>(); try { - return fetchAllPages(buildId); + return fetchAllPages(buildId, commits); } catch (Exception ex) { + if (!commits.isEmpty()) { + logger.warning( + "Unable to read the rest of the change list from the TeamCity REST API (" + + ex + + "). Keeping the " + + commits.size() + + " commits already read."); + return new CommitHistory(commits, truncationWarning(commits.size())); + } logger.warning( - "Unable to read the full change list from the TeamCity REST API (" + "Unable to read the change list from the TeamCity REST API (" + ex + "). Falling back to the first page of changes only."); - final List commits = toCommits(build.fetchChanges()); - return new CommitHistory(commits, fallbackWarning(commits.size())); + final List fallback = toCommits(build.fetchChanges()); + return new CommitHistory(fallback, fallbackWarning(fallback.size())); } } @@ -84,13 +94,17 @@ private static String fallbackWarning(final int commitCount) { if (commitCount < TEAMCITY_DEFAULT_PAGE_SIZE) { return null; } + return truncationWarning(commitCount); + } + + private static String truncationWarning(final int commitCount) { return "The full change list could not be read from TeamCity, so only the first " - + TEAMCITY_DEFAULT_PAGE_SIZE + + commitCount + " commits of this build were included in the build information."; } - private CommitHistory fetchAllPages(final long buildId) throws IOException { - final List commits = new ArrayList<>(); + private CommitHistory fetchAllPages(final long buildId, final List commits) + throws IOException { int start = 0; while (true) { final List page = parsePage(requester.get(changesUrl(buildId, start))); @@ -99,7 +113,8 @@ private CommitHistory fetchAllPages(final long buildId) throws IOException { if (page.size() < PAGE_SIZE) { return new CommitHistory(commits, null); } - if (commits.size() >= MAX_COMMITS) { + // Only a page past the cap proves commits were left out; exactly MAX_COMMITS is complete. + if (commits.size() > MAX_COMMITS) { return new CommitHistory( new ArrayList<>(commits.subList(0, MAX_COMMITS)), "Only the first " diff --git a/octopus-agent/src/test/java/octopus/teamcity/agent/CommitHistoryFetcherTest.java b/octopus-agent/src/test/java/octopus/teamcity/agent/CommitHistoryFetcherTest.java index 31e76a86..77f2826a 100644 --- a/octopus-agent/src/test/java/octopus/teamcity/agent/CommitHistoryFetcherTest.java +++ b/octopus-agent/src/test/java/octopus/teamcity/agent/CommitHistoryFetcherTest.java @@ -78,7 +78,46 @@ void stopsAtTheCapAndReportsTheTruncation() { assertThat(history.getCommits()).hasSize(CommitHistoryFetcher.MAX_COMMITS); assertThat(history.getIncompleteDataWarning()) .contains(String.valueOf(CommitHistoryFetcher.MAX_COMMITS)); - assertThat(requestedUrls).hasSize(fullPages); + assertThat(requestedUrls).hasSize(fullPages + 1); + } + + @Test + void doesNotReportTruncationWhenTheCommitCountIsExactlyTheCap() { + final int fullPages = CommitHistoryFetcher.MAX_COMMITS / CommitHistoryFetcher.PAGE_SIZE; + final int[] pageSizes = new int[fullPages + 1]; + Arrays.fill(pageSizes, CommitHistoryFetcher.PAGE_SIZE); + pageSizes[fullPages] = 0; + + final CommitHistory history = + fetcherReturningPagesOf(pageSizes).fetch(mock(Build.class), BUILD_ID); + + assertThat(history.getCommits()).hasSize(CommitHistoryFetcher.MAX_COMMITS); + assertThat(history.getIncompleteDataWarning()).isNull(); + } + + @Test + void keepsThePagesAlreadyReadWhenALaterPageFails() { + final CommitHistoryFetcher fetcher = + new CommitHistoryFetcher( + SERVER_URL, + logger, + url -> { + final int pageIndex = requestedUrls.size(); + requestedUrls.add(url); + if (pageIndex == 2) { + throw new IOException("read timed out"); + } + return changesJson( + pageIndex * CommitHistoryFetcher.PAGE_SIZE, CommitHistoryFetcher.PAGE_SIZE); + }); + + final CommitHistory history = fetcher.fetch(buildWithChange("abc123", "a change"), BUILD_ID); + + assertThat(history.getCommits()).hasSize(2 * CommitHistoryFetcher.PAGE_SIZE); + assertThat(history.getCommits().get(0).Id).isEqualTo("commit-0"); + assertThat(history.getIncompleteDataWarning()) + .contains(String.valueOf(2 * CommitHistoryFetcher.PAGE_SIZE)); + verify(logger).warning(anyString()); } @Test @@ -130,7 +169,7 @@ void reportsTruncationWhenTheFallbackHitsTheTeamCityDefaultPageSize() { assertThat(history.getCommits()).hasSize(CommitHistoryFetcher.TEAMCITY_DEFAULT_PAGE_SIZE); assertThat(history.getIncompleteDataWarning()) - .contains(String.valueOf(CommitHistoryFetcher.TEAMCITY_DEFAULT_PAGE_SIZE)); + .contains("only the first " + CommitHistoryFetcher.TEAMCITY_DEFAULT_PAGE_SIZE + " commits"); } @Test From db65ddc3119d4ebfe0574dc6309853d780eb1e7a Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Thu, 20 Aug 2026 15:04:01 +1000 Subject: [PATCH 3/3] fix: retry a page of changes before settling for a partial list A page can fail on a transient 500 or a read timeout, so each page now gets three attempts with a linear backoff before the fetch settles for whatever it has already read. The delay is injected so the tests record it rather than sleep. Co-Authored-By: Claude Opus 5 (1M context) --- .../teamcity/agent/CommitHistoryFetcher.java | 53 +++++++++++- .../agent/CommitHistoryFetcherTest.java | 80 +++++++++++++------ 2 files changed, 107 insertions(+), 26 deletions(-) diff --git a/octopus-agent/src/main/java/octopus/teamcity/agent/CommitHistoryFetcher.java b/octopus-agent/src/main/java/octopus/teamcity/agent/CommitHistoryFetcher.java index 7ecbabc4..d3e2a89a 100644 --- a/octopus-agent/src/main/java/octopus/teamcity/agent/CommitHistoryFetcher.java +++ b/octopus-agent/src/main/java/octopus/teamcity/agent/CommitHistoryFetcher.java @@ -37,6 +37,8 @@ public class CommitHistoryFetcher { static final int PAGE_SIZE = 1000; static final int MAX_COMMITS = 10000; static final int TEAMCITY_DEFAULT_PAGE_SIZE = 100; + static final int ATTEMPTS_PER_PAGE = 3; + static final long RETRY_DELAY_MILLIS = 1000L; private static final Gson GSON = new GsonBuilder().create(); @@ -44,26 +46,37 @@ interface ChangePageRequester { String get(String url) throws IOException; } + interface RetryDelay { + void pause(int failedAttempts) throws InterruptedException; + } + private final String serverUrl; private final BuildProgressLogger logger; private final ChangePageRequester requester; + private final RetryDelay retryDelay; public CommitHistoryFetcher( final String serverUrl, final String accessUser, final String accessCode, final BuildProgressLogger logger) { - this(serverUrl, logger, new BasicAuthRequester(accessUser, accessCode)); + this( + serverUrl, + logger, + new BasicAuthRequester(accessUser, accessCode), + failedAttempts -> Thread.sleep(failedAttempts * RETRY_DELAY_MILLIS)); } CommitHistoryFetcher( final String serverUrl, final BuildProgressLogger logger, - final ChangePageRequester requester) { + final ChangePageRequester requester, + final RetryDelay retryDelay) { this.serverUrl = serverUrl.endsWith("/") ? serverUrl.substring(0, serverUrl.length() - 1) : serverUrl; this.logger = logger; this.requester = requester; + this.retryDelay = retryDelay; } public CommitHistory fetch(final Build build, final long buildId) { @@ -107,7 +120,7 @@ private CommitHistory fetchAllPages(final long buildId, final List commi throws IOException { int start = 0; while (true) { - final List page = parsePage(requester.get(changesUrl(buildId, start))); + final List page = parsePage(getWithRetries(changesUrl(buildId, start))); commits.addAll(page); if (page.size() < PAGE_SIZE) { @@ -125,6 +138,40 @@ private CommitHistory fetchAllPages(final long buildId, final List commi } } + // A page can fail on a transient 500 or a read timeout, so give each one a few attempts before + // settling for whatever has been read so far. + private String getWithRetries(final String url) throws IOException { + IOException lastFailure = null; + for (int attempt = 1; attempt <= ATTEMPTS_PER_PAGE; attempt++) { + try { + return requester.get(url); + } catch (IOException ex) { + lastFailure = ex; + if (attempt < ATTEMPTS_PER_PAGE) { + logger.warning( + "Reading a page of changes from the TeamCity REST API failed (" + + ex + + "). Retrying, attempt " + + (attempt + 1) + + " of " + + ATTEMPTS_PER_PAGE + + "."); + pauseBeforeRetry(attempt); + } + } + } + throw lastFailure; + } + + private void pauseBeforeRetry(final int failedAttempts) throws IOException { + try { + retryDelay.pause(failedAttempts); + } catch (InterruptedException ex) { + Thread.currentThread().interrupt(); + throw new IOException("Interrupted while waiting to retry the change list request", ex); + } + } + private String changesUrl(final long buildId, final int start) { // Without an explicit count TeamCity returns only its default page of 100 changes. return serverUrl diff --git a/octopus-agent/src/test/java/octopus/teamcity/agent/CommitHistoryFetcherTest.java b/octopus-agent/src/test/java/octopus/teamcity/agent/CommitHistoryFetcherTest.java index 77f2826a..7d2fe7db 100644 --- a/octopus-agent/src/test/java/octopus/teamcity/agent/CommitHistoryFetcherTest.java +++ b/octopus-agent/src/test/java/octopus/teamcity/agent/CommitHistoryFetcherTest.java @@ -2,6 +2,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.atLeastOnce; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -25,6 +26,7 @@ class CommitHistoryFetcherTest { private final BuildProgressLogger logger = mock(BuildProgressLogger.class); private final List requestedUrls = new ArrayList<>(); + private final List pauses = new ArrayList<>(); @Test void returnsEveryCommitFromASinglePage() { @@ -98,17 +100,15 @@ void doesNotReportTruncationWhenTheCommitCountIsExactlyTheCap() { @Test void keepsThePagesAlreadyReadWhenALaterPageFails() { final CommitHistoryFetcher fetcher = - new CommitHistoryFetcher( - SERVER_URL, - logger, + fetcherWith( url -> { - final int pageIndex = requestedUrls.size(); requestedUrls.add(url); - if (pageIndex == 2) { + if (url.contains(",start:2000&")) { throw new IOException("read timed out"); } return changesJson( - pageIndex * CommitHistoryFetcher.PAGE_SIZE, CommitHistoryFetcher.PAGE_SIZE); + (requestedUrls.size() - 1) * CommitHistoryFetcher.PAGE_SIZE, + CommitHistoryFetcher.PAGE_SIZE); }); final CommitHistory history = fetcher.fetch(buildWithChange("abc123", "a change"), BUILD_ID); @@ -117,15 +117,13 @@ void keepsThePagesAlreadyReadWhenALaterPageFails() { assertThat(history.getCommits().get(0).Id).isEqualTo("commit-0"); assertThat(history.getIncompleteDataWarning()) .contains(String.valueOf(2 * CommitHistoryFetcher.PAGE_SIZE)); - verify(logger).warning(anyString()); + verify(logger, atLeastOnce()).warning(anyString()); } @Test void mapsChangeVersionToIdAndChangeCommentToComment() { final CommitHistoryFetcher fetcher = - new CommitHistoryFetcher( - SERVER_URL, - logger, + fetcherWith( url -> "{\"count\":1,\"change\":[{\"version\":\"deadbeef\",\"comment\":\"Fix it\"}]}"); final List commits = fetcher.fetch(mock(Build.class), BUILD_ID).getCommits(); @@ -138,9 +136,7 @@ void mapsChangeVersionToIdAndChangeCommentToComment() { @Test void fallsBackToTheTeamCityClientWhenTheRequestFails() { final CommitHistoryFetcher fetcher = - new CommitHistoryFetcher( - SERVER_URL, - logger, + fetcherWith( url -> { throw new IOException("connection refused"); }); @@ -151,15 +147,13 @@ void fallsBackToTheTeamCityClientWhenTheRequestFails() { assertThat(history.getCommits().get(0).Id).isEqualTo("abc123"); assertThat(history.getCommits().get(0).Comment).isEqualTo("a change"); assertThat(history.getIncompleteDataWarning()).isNull(); - verify(logger).warning(anyString()); + verify(logger, atLeastOnce()).warning(anyString()); } @Test void reportsTruncationWhenTheFallbackHitsTheTeamCityDefaultPageSize() { final CommitHistoryFetcher fetcher = - new CommitHistoryFetcher( - SERVER_URL, - logger, + fetcherWith( url -> { throw new IOException("connection refused"); }); @@ -174,19 +168,53 @@ void reportsTruncationWhenTheFallbackHitsTheTeamCityDefaultPageSize() { @Test void fallsBackToTheTeamCityClientWhenTheResponseIsNotValidJson() { - final CommitHistoryFetcher fetcher = - new CommitHistoryFetcher(SERVER_URL, logger, url -> "not json"); + final CommitHistoryFetcher fetcher = fetcherWith(url -> "not json"); final CommitHistory history = fetcher.fetch(buildWithChange("abc123", "a change"), BUILD_ID); assertThat(history.getCommits()).extracting(commit -> commit.Id).containsExactly("abc123"); - verify(logger).warning(anyString()); + verify(logger, atLeastOnce()).warning(anyString()); + } + + @Test + void retriesAPageThatFailsAndCarriesOnWhenTheRetrySucceeds() { + final CommitHistoryFetcher fetcher = + fetcherWith( + url -> { + requestedUrls.add(url); + if (requestedUrls.size() == 1) { + throw new IOException("TeamCity responded with HTTP 500 for " + url); + } + return changesJson(0, 3); + }); + + final CommitHistory history = fetcher.fetch(mock(Build.class), BUILD_ID); + + assertThat(history.getCommits()) + .extracting(commit -> commit.Id) + .containsExactly("commit-0", "commit-1", "commit-2"); + assertThat(history.getIncompleteDataWarning()).isNull(); + assertThat(requestedUrls).hasSize(2); + assertThat(pauses).containsExactly(1); + } + + @Test + void givesUpOnAPageAfterTheAttemptLimit() { + final CommitHistoryFetcher fetcher = + fetcherWith( + url -> { + requestedUrls.add(url); + throw new IOException("connection refused"); + }); + + fetcher.fetch(buildWithChange("abc123", "a change"), BUILD_ID); + + assertThat(requestedUrls).hasSize(CommitHistoryFetcher.ATTEMPTS_PER_PAGE); + assertThat(pauses).containsExactly(1, 2); } private CommitHistoryFetcher fetcherReturningPagesOf(final int... pageSizes) { - return new CommitHistoryFetcher( - SERVER_URL, - logger, + return fetcherWith( url -> { final int pageIndex = requestedUrls.size(); requestedUrls.add(url); @@ -194,6 +222,12 @@ private CommitHistoryFetcher fetcherReturningPagesOf(final int... pageSizes) { }); } + // Records the backoff instead of sleeping, so the retry tests stay fast. + private CommitHistoryFetcher fetcherWith( + final CommitHistoryFetcher.ChangePageRequester requester) { + return new CommitHistoryFetcher(SERVER_URL, logger, requester, pauses::add); + } + private static String changesJson(final int firstCommitIndex, final int count) { final StringBuilder json = new StringBuilder("{\"count\":").append(count).append(",\"change\":[");