Skip to content

s3: match the SymLink metadata key case-insensitively in ReadLink - #1625

Open
barnumbirr wants to merge 2 commits into
aptly-dev:masterfrom
barnumbirr:fix-readlink-metadata-case
Open

s3: match the SymLink metadata key case-insensitively in ReadLink#1625
barnumbirr wants to merge 2 commits into
aptly-dev:masterfrom
barnumbirr:fix-readlink-metadata-case

Conversation

@barnumbirr

Copy link
Copy Markdown

Description of the Change

S3 lowercases user metadata keys. SymLink writes "SymLink", but ReadLink
looks up output.Metadata["SymLink"] and the value comes back under
"symlink" — so the lookup misses and ReadLink returns an empty string with a
nil error. To the caller that reads as "this object is not a link" rather
than as a failure.

deb.packageIndexByHash uses ReadLink to find the physical file its .old
pointer refers to, and removes it before rotating:

linkTarget, err = file.parent.publishedStorage.ReadLink(oldIndexPath)
if err == nil {
    // If we managed to resolve the link target: delete it. This is the
    // oldest physical index file we no longer need.
    _ = file.parent.publishedStorage.Remove(linkTarget)
}

With "" returned, the Remove is a no-op and its error is discarded. The
pointers rotate correctly, so the bug is invisible in the published metadata —
but every superseded by-hash index file is left behind, one per index file per
hash algorithm per publish, indefinitely.

Filesystem backends are unaffected: they have real symlinks and never go
through this path.

Evidence

Measured on a live R2-backed archive publishing three suites a few times a day,
with Acquire-By-Hash enabled on 2026-08-24:

dists/ total 711  ->  by-hash 672, plain 39

Only 39 index files are current. The other 672 are orphaned by-hash copies,
accruing at roughly 48 per publish. Reading the metadata back from R2 directly
shows the casing:

Packages       metadata keys: ['symlink']
Packages.old   metadata keys: ['symlink']

Why the existing test does not catch it

TestSymLink round-trips SymLinkReadLink, but ends with
c.Skip("copy not available in s3test"), so it never runs — the test server has
no CopyObject. This path has therefore never been covered.

The regression test added here writes the metadata with PutObject instead, so
it needs no CopyObject support. It fails before the change with
obtained string = "" and passes after.

Checklist

  • allow Maintainers to edit PR (rebase, run coverage, help with tests, ...)
  • unit-test added (if change is algorithm)
  • functional test added/updated (if change is functional) — the functional
    suite has no S3 backend; the unit test covers the decoding directly
  • man page updated (if applicable) — no user-visible interface change
  • go build ./s3/..., go vet ./s3/..., gofmt -l s3/ and go test ./s3/...
    all clean

S3 lowercases user metadata keys. SymLink writes "SymLink" and ReadLink
looks up output.Metadata["SymLink"], but the value comes back under
"symlink", so the lookup misses and ReadLink returns an empty string with
a nil error. To the caller that reads as "this object is not a link"
rather than as a failure.

deb.packageIndexByHash uses ReadLink to find the physical file its .old
pointer refers to, and removes it before rotating:

    linkTarget, err = publishedStorage.ReadLink(oldIndexPath)
    if err == nil {
        _ = publishedStorage.Remove(linkTarget)
    }

With "" returned the Remove is a no-op and its error is discarded, so the
superseded by-hash index file is never deleted. The pointers rotate
correctly and the physical files accumulate: one per index file per hash
algorithm per publish, indefinitely. Filesystem backends are unaffected
because they have real symlinks.

Measured on a live R2-backed archive publishing three suites a few times
a day: dists/ held 711 objects, of which 672 were by-hash and only 39
were current indexes, growing by about 48 objects per publish since
Acquire-By-Hash was enabled.

The existing TestSymLink cannot catch this, because it ends in
c.Skip("copy not available in s3test") and never runs. The regression
test added here writes the metadata with PutObject instead, so it needs
no CopyObject support, and fails before this change with
obtained string = "".
@codecov

codecov Bot commented Aug 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.08%. Comparing base (f59b0d2) to head (ed180e8).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1625      +/-   ##
==========================================
- Coverage   77.37%   77.08%   -0.30%     
==========================================
  Files         165      165              
  Lines       15747    15750       +3     
==========================================
- Hits        12185    12141      -44     
- Misses       2356     2408      +52     
+ Partials     1206     1201       -5     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov/patch flagged the change at 75% of diff hit. The uncovered lines
were the two the existing test could not reach: the fallthrough when no
metadata key matches, and the HeadObject error return.

Both are worth pinning regardless of the coverage gate.
packageIndexByHash calls ReadLink on a path it has only just confirmed
exists and treats an error as "leave it alone", so "this is not a link"
and "I could not find out" have to stay distinguishable: the first is an
empty string with a nil error, the second is an error.

ReadLink goes to 100%.
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