fix(xpkg): prevent corrupt package cache entries - #1112
Conversation
Signed-off-by: Victor Chen <vchen2@atlassian.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesPackage cache consistency
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CachedClient
participant PackageCache
participant PackageParser
participant Registry
CachedClient->>PackageCache: Read cached package
PackageCache->>PackageParser: Parse size-limited content
alt Cache is valid
PackageParser-->>CachedClient: Return package
else Cache is invalid and pull policy allows fetch
CachedClient->>PackageCache: Delete invalid entry
CachedClient->>Registry: Fetch package
Registry-->>CachedClient: Return package content
CachedClient->>PackageCache: Store package and wait for result
else Pull policy is Never
PackageParser-->>CachedClient: Return cache error
end
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/xpkg/cache_test.go (2)
328-344: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider deriving the cache file path from
BuildPathinstead of hardcoding.gz.Nice targeted coverage of the new
RUnlockbranch on the gzip failure path. Thank you for adding it.One durability concern for the test itself. The path
/cache/package.gzhardcodescacheContentExt. If that constant ever changes,Getwill open a different path, fail with a not-exist error instead of a gzip error, and this test will still pass. The lock-release assertion would then no longer exercise the gzip branch it was written for, and nothing would signal the loss.Would you consider building the path the same way the cache does?
♻️ Proposed change to track `cacheContentExt`
func TestGetErrorReleasesLock(t *testing.T) { fs := afero.NewMemMapFs() - f, err := fs.Create("/cache/package.gz") + f, err := fs.Create(BuildPath("/cache", "package", cacheContentExt)) if err != nil { t.Fatalf("Create(...): unexpected error: %v", err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/xpkg/cache_test.go` around lines 328 - 344, Update TestGetErrorReleasesLock to derive the created cache file path using the cache’s BuildPath behavior rather than hardcoding “/cache/package.gz”. Keep the invalid gzip contents and existing Get assertion unchanged so the test continues exercising the gzip failure path even if cacheContentExt changes.
226-247: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding
reasonfields, and a table structure where scenarios group naturally.These tests cover the right behaviors.
TestClientGetIgnoresCacheStoreFailureis particularly good, because itsMockStorereturns without drainingrc, which is exactly the early-exit case the new pipe drain inclient.gofixes. That test would hang without the fix.The repository convention for
*_test.gois a table-driven structure withargs/wantand areasonfield per case.TestStoreatpkg/xpkg/cache_test.golines 186-224 follows it. The new tests do not. I recognize the concurrency and lifecycle tests do not fit a table cleanly, so I am not suggesting you force them. Thereasondocumentation is the part that carries most of the value here.
pkg/xpkg/cache_test.go#L226-L247: add a short comment orreasonstring stating the behavior under test, thatStorethenGetreturns identical bytes.pkg/xpkg/cache_test.go#L249-L280: this case is already table-driven. Add areasonfield to the case struct and populate it forContentReadError,GzipCloseError, andFileCloseError.pkg/xpkg/cache_test.go#L282-L326: add areasondescribing thatStoremust wait for an open reader to close.pkg/xpkg/cache_test.go#L328-L359: add areasondescribing that a failedGetmust release the read lock.pkg/xpkg/client_test.go#L1122-L1161: add areasondescribing thatGetmust not return beforeStorecompletes.pkg/xpkg/client_test.go#L1163-L1190: add areasondescribing that aStorefailure must not failGet, and must delete the entry.pkg/xpkg/client_test.go#L1192-L1220: add areasondescribing that a corrupt entry is deleted and refetched underPullIfNotPresent.pkg/xpkg/client_test.go#L1222-L1259: add areasondescribing thatPullNeverpreserves the entry and returns the cache error. Considercmp.Diffwithcmpopts.EquateErrors()against a sentinel instead ofstrings.Containson the message.pkg/xpkg/client_test.go#L1261-L1285: add areasondescribing that a digest reference underPullNeverperforms no registry call.
TestClientGetRefetchesCorruptCacheandTestClientGetPreservesCorruptCacheWithPullNeverdiffer only by pull policy and expected outcome. Would you consider merging those two into one table keyed onpullPolicy?As per path instructions for
**/*_test.go: "Enforce table-driven test structure: PascalCase test names (no underscores), args/want pattern, use cmp.Diff with cmpopts.EquateErrors() for error testing. Check for proper test case naming and reason fields."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/xpkg/cache_test.go` around lines 226 - 247, Add reason documentation to the affected tests: pkg/xpkg/cache_test.go:226-247 for Store/Get byte identity, 249-280 for each read/close error case, 282-326 for waiting on an open reader, and 328-359 for releasing the read lock after failed Get; pkg/xpkg/client_test.go:1122-1161 for waiting for Store, 1163-1190 for ignoring failed Store and deleting the entry, 1192-1220 for refetching corrupt cache entries, 1222-1259 for preserving entries and returning the cache error under PullNever, and 1261-1285 for avoiding registry calls for digest references under PullNever. Keep or convert applicable cases to the repository’s table-driven args/want structure, use cmp.Diff with cmpopts.EquateErrors() for error comparisons, and merge the two corrupt-cache client tests into one pull-policy-keyed table while preserving their distinct expected outcomes.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/xpkg/cache.go`:
- Around line 90-110: Document the read-lock lifecycle on the exported
PackageCache.Get and FsPackageCache.Get APIs: callers must close the returned
io.ReadCloser to release the cache read lock, and failing to do so can block
subsequent Store and Delete calls. Explicitly mark this as a breaking change if
the project’s API documentation supports such annotations, since the lock now
remains held until the reader is closed.
---
Nitpick comments:
In `@pkg/xpkg/cache_test.go`:
- Around line 328-344: Update TestGetErrorReleasesLock to derive the created
cache file path using the cache’s BuildPath behavior rather than hardcoding
“/cache/package.gz”. Keep the invalid gzip contents and existing Get assertion
unchanged so the test continues exercising the gzip failure path even if
cacheContentExt changes.
- Around line 226-247: Add reason documentation to the affected tests:
pkg/xpkg/cache_test.go:226-247 for Store/Get byte identity, 249-280 for each
read/close error case, 282-326 for waiting on an open reader, and 328-359 for
releasing the read lock after failed Get; pkg/xpkg/client_test.go:1122-1161 for
waiting for Store, 1163-1190 for ignoring failed Store and deleting the entry,
1192-1220 for refetching corrupt cache entries, 1222-1259 for preserving entries
and returning the cache error under PullNever, and 1261-1285 for avoiding
registry calls for digest references under PullNever. Keep or convert applicable
cases to the repository’s table-driven args/want structure, use cmp.Diff with
cmpopts.EquateErrors() for error comparisons, and merge the two corrupt-cache
client tests into one pull-policy-keyed table while preserving their distinct
expected outcomes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b214392b-2091-4d46-a3ee-44e557c52143
📒 Files selected for processing (4)
pkg/xpkg/cache.gopkg/xpkg/cache_test.gopkg/xpkg/client.gopkg/xpkg/client_test.go
| func (c *FsPackageCache) Get(id string) (io.ReadCloser, error) { | ||
| c.mu.RLock() | ||
| defer c.mu.RUnlock() | ||
|
|
||
| f, err := c.fs.Open(BuildPath(c.dir, id, cacheContentExt)) | ||
| if err != nil { | ||
| c.mu.RUnlock() | ||
| return nil, err | ||
| } | ||
|
|
||
| return GzipReadCloser(f) | ||
| r, err := GzipReadCloser(f) | ||
| if err != nil { | ||
| _ = f.Close() | ||
| c.mu.RUnlock() | ||
| return nil, err | ||
| } | ||
|
|
||
| return &unlockingReadCloser{ | ||
| ReadCloser: r, | ||
| unlock: c.mu.RUnlock, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the Get doc comment and locate all callers of PackageCache.Get.
set -euo pipefail
echo "=== cache.go Get doc comment ==="
sed -n '85,112p' pkg/xpkg/cache.go
echo "=== PackageCache interface definition ==="
ast-grep run --pattern 'type PackageCache interface { $$$ }' --lang go pkg/xpkg/cache.go
echo "=== callers of .cache.Get / cache.Get across repo ==="
rg -nP --type=go -C4 '\bcache\.Get\s*\(' -g '!**/vendor/**'
echo "=== FsPackageCache construction sites ==="
rg -nP --type=go -C3 '\bNewFsPackageCache\s*\('Repository: crossplane/crossplane-runtime
Length of output: 1087
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== File locations ==="
git ls-files | rg '(^|/)(cache\.go|reader\.go|client\.go)$|_test\.go$' | sort | sed -n '1,120p'
echo "=== cache.go relevant outline ==="
ast-grep outline pkg/xpkg/cache.go --view expanded || true
echo "=== cache.go full relevant section ==="
sed -n '1,150p' pkg/xpkg/cache.go
echo "=== reader.go relevant GzipReadCloser ==="
sed -n '1,180p' pkg/xpkg/reader.go
echo "=== calls to Get methods in Go files ==="
rg -nP --type=go -C3 '\.Get\s*\(|\.get\s*\(' | head -200 || true
echo "=== FsPackageCache construction ==="
rg -nP --type=go -C3 '\bNewFsPackageCache\s*\(' | head -200 || true
echo "=== repo package metadata / breaking label references ==="
git ls-files | rg '(\.github|breaking|CHANGELOG|README|.*\.md)$' | xargs -r rg -n "breaking-change|breaking" | head -200 || trueRepository: crossplane/crossplane-runtime
Length of output: 9250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== xpkg client cache usage ==="
sed -n '1,260p' pkg/xpkg/client.go
echo "=== cache tests around Get ==="
sed -n '1,260p' pkg/xpkg/cache_test.go
echo "=== all PackageCache references (read-only) ==="
rg -nP --type=go -C3 '\bPackageCache\b|\bNewFsPackageCache\s*\(' | head -200
echo "=== exported xpkg symbols in cache/client modules ==="
python3 - <<'PY'
from pathlib import Path
for p in ['pkg/xpkg/cache.go','pkg/xpkg/client.go']:
print(f'--- {p} ---')
data=Path(p).read_text()
for i,line in enumerate(data.splitlines(),1):
if line.startswith(('type ','Func','func ')) and i>=60 and i<=130:
print(f'{i}: {line}')
PYRepository: crossplane/crossplane-runtime
Length of output: 14137
Document the cache read-lock lifecycle on PackageCache.Get / FsPackageCache.Get.
Get now returns an io.ReadCloser that releases the cache RUnlock only when closed. Add a doc requirement such as: the caller must close the returned reader to release the cache read lock; leaking it can block later Store and Delete calls. Also note whether this counts as a breaking-change because the lock lifetime changed from the previous exported API behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/xpkg/cache.go` around lines 90 - 110, Document the read-lock lifecycle on
the exported PackageCache.Get and FsPackageCache.Get APIs: callers must close
the returned io.ReadCloser to release the cache read lock, and failing to do so
can block subsequent Store and Delete calls. Explicitly mark this as a breaking
change if the project’s API documentation supports such annotations, since the
lock now remains held until the reader is closed.
Source: Coding guidelines
Signed-off-by: Victor Chen <vchen2@atlassian.com>
1 participant
Problem
CachedClient.Getstarts writing fetched package content to the cache in a goroutine, but it returns as soon as parsing completes and ignores theStoreresult. If the cache write fails (for example, when the volume is full),FsPackageCache.Storeleaves the truncated gzip file at the final cache path. A later reconcile can then parse that incomplete entry instead of fetching the package again.FsPackageCache.Getalso releases its read lock immediately after opening the file, so a concurrentStorecan truncate the file while it is still being read.Fix
CachedClient.Get.Storereturns early, so a cache failure does not interrupt parsing valid registry content.PullNever.This keeps the existing direct-write cache format and does not require a second package-sized temporary file.
Behavior change
A successful registry fetch can complete even when caching fails, and the failed write no longer remains as a cache hit.
PullNevercontinues to treat operator-provided cache content as authoritative, so Crossplane reports the cache error without deleting or fetching the package.Validation
./nix.sh flake check- all checks passedgo test -race ./pkg/xpkggo test -race ./pkg/xpkg -run 'Test(StoreWaitsForReader|GetErrorReleasesLock|ClientGetWaitsForCacheStore|ClientGetIgnoresCacheStoreFailure)$' -count=100go test ./apis/... ./pkg/...golangci-lint run- 0 issuesRelated to crossplane/crossplane#7712.
I have:
./nix.sh flake checkto ensure this PR is ready for review.Linked a PR or a docs tracking issue to document this change.Addedbackport release-x.ylabels to auto-backport this PR.