fix: resolve critical pagination bugs and add comprehensive testing
- Fix off-by-one error in pagination loop (< to <=) that prevented fetching last page - Add Math.ceil() to maxNumberOfPages calculation for proper limit handling - Replace hardcoded 2000 limit with configurable getMaxArtifactListCount() - Add pagination test for multi-page artifact listing - Add environment variable test for ACTIONS_ARTIFACT_MAX_ARTIFACT_COUNT - Add comprehensive test coverage for getMaxArtifactListCount() function Fixes compound bug where pagination and limit logic capped results at 900 artifacts instead of intended 1000.
This commit is contained in:
@@ -9,12 +9,12 @@ import {retry} from '@octokit/plugin-retry'
|
||||
import {OctokitOptions} from '@octokit/core/dist-types/types'
|
||||
import {internalArtifactTwirpClient} from '../shared/artifact-twirp-client'
|
||||
import {getBackendIdsFromToken} from '../shared/util'
|
||||
import {getMaxArtifactListCount} from '../shared/config'
|
||||
import {ListArtifactsRequest, Timestamp} from '../../generated'
|
||||
|
||||
// Limiting to 2000 for perf reasons
|
||||
const maximumArtifactCount = 2000
|
||||
const maximumArtifactCount = getMaxArtifactListCount()
|
||||
const paginationCount = 100
|
||||
const maxNumberOfPages = maximumArtifactCount / paginationCount
|
||||
const maxNumberOfPages = Math.ceil(maximumArtifactCount / paginationCount)
|
||||
|
||||
export async function listArtifactsPublic(
|
||||
workflowRunId: number,
|
||||
@@ -81,7 +81,7 @@ export async function listArtifactsPublic(
|
||||
// Iterate over any remaining pages
|
||||
for (
|
||||
currentPageNumber;
|
||||
currentPageNumber < numberOfPages;
|
||||
currentPageNumber <= numberOfPages;
|
||||
currentPageNumber++
|
||||
) {
|
||||
debug(`Fetching page ${currentPageNumber} of artifact list`)
|
||||
|
||||
@@ -97,3 +97,22 @@ export function getUploadChunkTimeout(): number {
|
||||
|
||||
return timeout
|
||||
}
|
||||
|
||||
// This value can be changed with ACTIONS_ARTIFACT_MAX_ARTIFACT_COUNT variable.
|
||||
// Defaults to 1000 as a safeguard for rate limiting.
|
||||
export function getMaxArtifactListCount(): number {
|
||||
const maxCountVar = process.env['ACTIONS_ARTIFACT_MAX_ARTIFACT_COUNT']
|
||||
if (!maxCountVar) {
|
||||
return 1000
|
||||
}
|
||||
|
||||
const maxCount = parseInt(maxCountVar)
|
||||
if (isNaN(maxCount) || maxCount < 1) {
|
||||
throw new Error(
|
||||
'Invalid value set for ACTIONS_ARTIFACT_MAX_ARTIFACT_COUNT env variable'
|
||||
)
|
||||
}
|
||||
|
||||
return maxCount
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user