fix: include every commit in build information - #228
Conversation
3245d0a to
aae812f
Compare
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) <noreply@anthropic.com>
aae812f to
59a813a
Compare
sathvikkumar-octo
left a comment
sathvikkumar-octo
left a comment
There was a problem hiding this comment.
Put on some reasonable sounding Claude feedback but approving anyway as I think this is better than what we had before
| if (page.size() < PAGE_SIZE) { | ||
| return new CommitHistory(commits, null); | ||
| } | ||
| if (commits.size() >= MAX_COMMITS) { |
There was a problem hiding this comment.
Spurious truncation warning when the commit count is exactly MAX_COMMITS.
The cap check runs after a full page, so a build with exactly 10000 commits fetches 10 full pages, commits.size() >= MAX_COMMITS is true, and the build information gets stamped "only the first 10000 commits were included" even though every commit was included — Octopus then shows an incomplete-data warning on a complete data set.
Firing needs exactly 10000 commits so it's near-unhittable in practice, but the fix is > instead of >= (or probing one more page before declaring truncation), which is free.
There was a problem hiding this comment.
Fixed in dad606f — the cap check is now commits.size() > MAX_COMMITS, so a full page past the cap is what proves commits were left out. Exactly 10000 commits fetches one more (empty) page and comes back with no warning. Covered by doesNotReportTruncationWhenTheCommitCountIsExactlyTheCap, and stopsAtTheCapAndReportsTheTruncation now expects the extra probe request.
| public CommitHistory fetch(final Build build, final long buildId) { | ||
| try { | ||
| return fetchAllPages(buildId); | ||
| } catch (Exception ex) { |
There was a problem hiding this comment.
Commits already fetched are discarded when a later page fails.
If pages 1–2 succeed (2000 commits) and page 3 throws — transient 500 or read timeout, and there's no retry — this catch block throws away all 2000 and re-fetches via build.fetchChanges(), yielding 100. Needs >1000 commits plus network flakiness so it's uncommon, but flakiness is a matter of time, and the degradation is far larger than it needs to be.
Returning the partial list with a truncation warning would be strictly better. It also matters for the warning text: "only the first 100 commits" is accurate here only by coincidence.
There was a problem hiding this comment.
Fixed in dad606f — the accumulator now lives in fetch(), so a failing page keeps whatever pages already succeeded and returns them with a truncation warning; the build.fetchChanges() fallback only runs when nothing was read at all. Your point about the wording held too: the warning now names the number of commits actually included instead of hardcoding TeamCity's default page size. New test keepsThePagesAlreadyReadWhenALaterPageFails fails page 3 and asserts the first 2000 survive. Still no retry — happy to add one if you think it's worth it, but keeping the partial result seemed like the bigger win.
There was a problem hiding this comment.
will keep expanding this with Claude
There was a problem hiding this comment.
Added the retry in db65ddc, so this thread is fully covered now. Each page gets ATTEMPTS_PER_PAGE = 3 attempts on IOException (the requester turns a non-200 into one, so 500s and read timeouts both retry) with a linear backoff of 1s then 2s. Only once the attempts are exhausted does it settle for the pages already read. The delay is injected as a RetryDelay, so the tests record the pauses rather than sleep — retriesAPageThatFailsAndCarriesOnWhenTheRetrySucceeds and givesUpOnAPageAfterTheAttemptLimit. A malformed-JSON response is deliberately not retried; it falls straight through to the existing handling.
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
2 participants
Closes #168
The
octopus.buildinfofile only ever contained the 100 most recent commits, so work items on older commits never made it to Octopus.build.fetchChanges()in the TeamCity client requests/app/rest/changeswithout acount, and TeamCity then applies its default page size of 100. The client offers no paging option.This PR:
count/start(1000 per page, capped at 10000 commits) using the credentials the agent already holdsIncompleteDataWarningto the build information so Octopus shows when the commit list was capped, instead of silently truncatingfetchChanges()call if the request fails, flagging the truncation rather than failing the stepVerified against a live TeamCity 2025.03.3 with a build containing 150 commits: the old call returns 100, the new one returns all 150.
This code was generated with the assistance of Claude.
🤖 Generated with Claude Code