Skip to content

distribution: default-CI parse coverage + archive_reader leak fix (rstudio/package-manager#18746) - #61

Merged
jonyoder merged 2 commits into
mainfrom
test/18746-distribution-fixtures
Sep 24, 2026
Merged

jonyoder merged 2 commits into
mainfrom
test/18746-distribution-fixtures

Conversation

@jonyoder

Copy link
Copy Markdown
Collaborator

Adds default-CI parse coverage for distribution/ with synthetic, in-test fixtures, and fixes a real file descriptor leak in NewArchiveReader.


Why

distribution.Parse only had test coverage behind the distribution_integration build tag, which needs python/twine/cargo/gpg and clones 12 GitHub repos — plain go test ./... (what CI runs) never exercised it. Along the way we confirmed a real leak: NewArchiveReader opened a file for .tar.gz/.tgz input and didn't close it on the tarReadCheck error path.

Fixture decision

Synthesize wheels and sdists in the test with archive/zip, archive/tar and compress/gzip, written to t.TempDir(), instead of committing sample archives. Every byte is Posit-authored fixture data (gppfixture-* names), so there's no third-party content, no NOTICE change, and nothing here for #60's pending licensing review to cover. The linked issue's original wording ("commit a few small sample wheels and sdists") is not what this does — this keeps the issue's intent (toolchain-free, network-free Parse coverage in default CI) without the third-party-content risk of committed binaries.

Test cases (distribution/parse_fixture_test.go)

case what it pins
wheel, Metadata-Version 2.4, multi-value fields (Classifier, Requires-Dist w/ marker, Provides-Extra, Project-URL, License-Expression, License-File, Dynamic), body as Description METADATA header parsing and multi-value collection
wheel, Metadata-Version 2.1, License: header with continuation lines pkginfo leading-whitespace/continuation handling
sdist .tar.gz, top-level PKG-INFO not the first tar member, deeper egg-info PKG-INFO with differing values shortest-path PKG-INFO selection
wheel + matching .asc file with dummy bytes signature pairing (AddGPGSignature only reads bytes, no gpg needed)
directory with one wheel + one sdist one-level directory expansion and wheel-first grouping

One deviation from the brief: the brief asked for a Description: header with continuation lines. Empirically, BaseDistribution.Parse always overwrites description from the message body afterward — even to an empty string, since io.ReadAll never returns a nil slice — so a Description header's value can never survive into MetadataMap(). License hits the same collapseLeadingWS code path and does survive, so it's used instead to pin the same continuation-handling logic. Worth noting separately: this means Description: header parsing is dead code in practice — real long descriptions live in the body, not the header.

Assertions are literal assert.Equal calls against complete map[string][]string values (no cupaloy — a missing snapshot auto-blesses on first run, which is banned per rstudio/package-manager#19392), plus FileType, PythonVersion, SafeName, BaseFilename, and digests computed independently in the test over the written file bytes. md5_digest is never present (NewPackageFile discards HexDigest().md5), confirmed by the full-map equality rather than a separate check.

Order among multiple wheels is intentionally never asserted by index (groupWheelFilesFirst's sort.Slice isn't stable) — only that every wheel precedes every sdist.

The leak fix

NewArchiveReader's .tar.gz/.tgz branch opened f, then returned early on a tarReadCheck error without closing it (the gzip.NewReader error branch right above already does close it). Fixed with the same close-and-log pattern. Regression test in archive_reader_test.go counts open descriptors via /dev/fd before/after 5 failing calls.

Not fixed, flagging only: tarReader.FileNames has a real double-close (the gzip.NewReader error branch closes f explicitly, and the deferred close runs again right after). It's out of scope for this issue; noting it here rather than silently leaving it.

Mutation proof

  1. Reversed SDist.read's sort.Slice comparator (> instead of <) — TestParse_SDistShortestPathSelection failed on version (3.0.0-stale instead of 3.0.0) and summary. Reverted.
  2. Removed the new f.Close() in archive_reader.go — TestNewArchiveReader_TarReadCheckFailure_DoesNotLeakFile failed (expected: 6, actual: 11, exactly +5 for 5 leaking calls). Reverted.

Verification

  • go test ./... -count=1 and go test -race ./... -count=1: all packages pass.
  • go vet ./...: clean.
  • golangci-lint@v2.11.2 run ./... (CI's pinned version, run from module root): 0 issues. Confirmed errcheck is actually active by planting a temporary unchecked os.Setenv, seeing it flagged, then removing it.
  • gofmt -l .: clean.
  • go list -deps ./... / -test ./...: pgregory.net/rapid absent from the non-test graph, present in the test graph (module's MPL invariant intact).
  • No collision with Add Apache-2.0 §4(b)/(c) provenance blocks and normalize upstream citations (#19394, #19395) #60: NOTICE and the first lines of every file it touches are untouched by this PR; no files are shared between the two diffs.

Refs rstudio/package-manager#18746.

🤖 Generated with Claude Code

jonyoder and others added 2 commits September 24, 2026 10:24
Parse only had test coverage behind the distribution_integration build
tag, which needs python/twine/cargo/gpg and clones GitHub repos, so
default CI (plain `go test ./...`) never exercised it.

Builds wheels and sdists in-test with archive/zip, archive/tar and
compress/gzip instead of committing sample archives, so every byte is
Posit-authored and there is nothing for the pending NOTICE/licensing
review on #60 to cover.

Refs rstudio/package-manager#18746.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
NewArchiveReader opened f for a .tar.gz, then returned early on a
tarReadCheck error without closing it. The gzip.NewReader error branch
right above already closes f; this makes the tarReadCheck branch match.

Refs rstudio/package-manager#18746.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@jonyoder
jonyoder merged commit c899b98 into main Sep 24, 2026
4 checks passed
@jonyoder
jonyoder deleted the test/18746-distribution-fixtures branch September 24, 2026 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant