Skip to content

api-diff-closure: goListTimeout is marginal on CI runners, causing intermittent failures #2389

Description

@yuanchen8911

Problem

TestRunReportsCurrentAliasMappings (tools/api-diff-closure/main_test.go:280)
intermittently fails the tests / Test job with:

--- FAIL: TestRunReportsCurrentAliasMappings (120.01s)
    main_test.go:292: run() error = load packages: context deadline exceeded
FAIL	github.com/NVIDIA/aicr/tools/api-diff-closure	120.113s

The bound is goListTimeout = 2 * time.Minute (tools/api-diff-closure/main.go:38),
applied to the package load in run() (main.go:139). The failure is a timeout,
not an assertion — the test never gets a result to compare.

Evidence that the bound is marginal, not wrong

Observed on PR #2380, whose diff is Markdown and YAML comments only — zero .go
files, so nothing in it is reachable from a Go package loader. Three runs of the
same commit (3f0ee2e09):

Attempt Result Duration
1 FAIL 120.258s
2 FAIL 120.113s
3 pass

Locally on the same commit the test passes in 33.7s — roughly 3.5x under the
budget, on a warm module/build cache. Two commits either side also passed CI
(d48a78fc3 on the same branch, 90354855f on PR #2379).

So the runner sometimes completes under 120s and sometimes does not. That is
worse than a deterministic failure: it costs contributors re-run cycles on
unrelated PRs, and it trains reviewers to read a red tests / Test as noise.

Likely mechanism

The test type-checks packages across the whole module from the repository root.
The local-vs-CI gap is consistent with a cold Go module/build cache on the
runner, where the same work takes several times longer than on a developer
machine with a warm cache. The bound has not changed since it was introduced in
#2124; the repository has grown into it rather than something regressing.

Possible directions

Deliberately not choosing one — this belongs to whoever owns
tools/api-diff-closure:

  1. Raise goListTimeout. One line, immediate relief. Mitigates the symptom
    without answering why CI needs more than 3.5x the local time, and the next
    growth in the module tree re-opens it.
  2. Warm the Go module/build cache in CI before the job. Addresses the likely
    cause rather than the bound, but is a workflow change.
  3. Narrow what the test loads. The assertion only needs the alias mappings
    for pkg/client/v1; loading the full module may be more than the test
    requires.
  4. Make the bound configurable so CI can set a higher value than the local
    default, keeping fast local feedback.

Acceptance criteria

  • TestRunReportsCurrentAliasMappings no longer fails intermittently on CI runners
  • Whichever direction is chosen, the reason is recorded next to the value or in the workflow, so the next person hitting it does not have to re-derive the local-vs-CI gap
  • If the bound is kept as a fixed constant, it has enough headroom over observed CI timings to survive normal module growth

Notes

Not caused by, and not fixable in, PR #2380 — that PR's CI is green after a
re-run. Filed separately so a docs change does not carry an unrelated CI fix.

Metadata

Metadata

Assignees

No one assigned

    Labels

    area/citheme/ci-dxCI pipelines, developer experience, and build tooling

    Type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions