Skip to content

Conversation

@leodido
Copy link
Contributor

@leodido leodido commented Nov 10, 2025

Problem

The sign-cache command was re-uploading ALL artifacts to the remote cache, including those that were already present (downloaded from cache). This caused several issues:

  1. Unnecessary network traffic and storage costs - Re-uploading artifacts that already exist
  2. Potential digest mismatches - If artifacts are re-compressed with different timestamps (see fix: make gzip compression deterministic #262)
  3. Overwrites existing artifacts - Downloaded artifacts get re-uploaded, potentially with different content

Current Behavior

Build Job:
  1. Build artifact → Upload to S3 (digest A)

Sign Job:
  2. Download artifact from S3 (digest A)
  3. Create attestation (digest A)
  4. Re-upload artifact to S3 (digest A) ← Unnecessary!
  5. Upload attestation

Expected Behavior (SLSA L3)

Build Job:
  1. Build artifact (no upload)

Sign & Upload Job:
  2. Create attestation
  3. Upload ONLY locally built artifacts
  4. Skip downloaded artifacts (already in S3)
  5. Upload attestation

Solution

Add a HasFile() method to the RemoteCache interface to check if an artifact already exists before uploading. The sign-cache command now:

  • ✅ Checks if artifact exists in remote cache
  • ✅ Skips upload if it already exists (downloaded artifacts)
  • ✅ Uploads only new artifacts (locally built)
  • ✅ Always uploads attestation files (they're new)

This aligns with the build command behavior, which only uploads newly built packages via GetNewPackagesForCache().

Changes

Core Changes

  • Add HasFile(ctx, key) method to RemoteCache interface
  • Implement HasFile() in:
    • S3Cache - uses storage.HasObject()
    • GSUtilCache - uses gsutil stat
    • NoRemoteCache - returns false
  • Update UploadArtifactWithAttestation() to check before uploading artifacts

Supporting Changes

  • Update wrapper types (pushOnlyRemoteCache, pullOnlyRemoteCache) to implement HasFile()
  • Add comprehensive tests for skip/upload behavior
  • Update mock implementations in tests

Testing

New Tests

  • TestArtifactUploader_SkipsExistingArtifacts - Verifies existing artifacts are not re-uploaded
  • TestArtifactUploader_UploadsNewArtifacts - Verifies new artifacts are uploaded

Test Results

$ go test ./pkg/leeway/signing/... -v
✅ All tests pass

Benefits

  • Prevents re-uploading downloaded artifacts - Saves bandwidth and storage
  • Reduces network traffic and costs - Only uploads what's needed
  • Avoids potential digest mismatches - No re-compression of existing artifacts
  • Maintains SLSA attestation integrity - Artifacts match their attestations
  • Aligns with build command behavior - Consistent upload logic

Example Log Output

Before (Re-uploads everything)

INFO Successfully uploaded artifact to remote cache artifact=existing.tar.gz
INFO Successfully uploaded attestation file artifact=existing.tar.gz

After (Skips existing)

INFO Artifact already exists in remote cache, skipping upload artifact=existing.tar.gz
INFO Successfully uploaded attestation file artifact=existing.tar.gz

Related

@leodido leodido self-assigned this Nov 10, 2025
The sign-cache command was re-uploading ALL artifacts, including those
that were already in the remote cache. This caused issues:

1. Unnecessary network traffic and storage costs
2. Potential digest mismatches if artifacts were re-compressed
3. Overwrites existing artifacts that may have been downloaded

This fix adds a HasFile() method to the RemoteCache interface to check
if an artifact already exists before uploading. The sign-cache command
now:

- Checks if artifact exists in remote cache
- Skips upload if it already exists (downloaded artifacts)
- Uploads only new artifacts (locally built)
- Always uploads attestation files (they're new)

This aligns with the build command behavior, which only uploads newly
built packages via GetNewPackagesForCache().

Changes:
- Add HasFile() method to RemoteCache interface
- Implement HasFile() in S3Cache, GSUtilCache, NoRemoteCache
- Update UploadArtifactWithAttestation to check before uploading
- Add tests for skip/upload behavior
- Update wrapper types (pushOnly/pullOnly) to implement HasFile()

Benefits:
- Prevents re-uploading downloaded artifacts
- Reduces network traffic and costs
- Avoids potential digest mismatches
- Maintains SLSA attestation integrity

Co-authored-by: Ona <no-reply@ona.com>
@leodido leodido force-pushed the leo/fix/sign-cache-avoid-reuploading-downloaded-artifacts branch from 87891a4 to 0d18574 Compare November 10, 2025 15:33
Improve test coverage to prevent regression of the feature that avoids
re-uploading downloaded artifacts:

1. Track upload call counts in mock to verify behavior
   - Add uploadCalls map to mockRemoteCacheUpload
   - Increment counter on each UploadFile call

2. Enhance existing tests with call count assertions
   - TestArtifactUploader_SkipsExistingArtifacts: Verify uploadCalls = 0
   - TestArtifactUploader_UploadsNewArtifacts: Verify uploadCalls = 1

3. Add workflow simulation tests
   - TestArtifactUploader_SimulatesDownloadedArtifactWorkflow:
     Simulates build job uploading, sign job downloading, and verifies
     artifact is NOT re-uploaded (uploadCalls stays at 1)

   - TestArtifactUploader_SimulatesLocallyBuiltArtifactWorkflow:
     Simulates locally built artifact and verifies both artifact and
     attestation are uploaded

These tests ensure the feature cannot regress by explicitly verifying
that UploadFile is NOT called for existing artifacts.

Co-authored-by: Ona <no-reply@ona.com>
@leodido leodido changed the title Avoid re-uploading downloaded artifacts in sign-cache fix: avoid re-uploading downloaded artifacts in sign-cache Nov 10, 2025
@leodido leodido merged commit 3ecf8b3 into main Nov 10, 2025
6 checks passed
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.

3 participants