Skip to content

Conversation

@leodido
Copy link
Contributor

@leodido leodido commented Nov 24, 2025

Summary

This PR fixes a bug where packages that failed SLSA verification and were rebuilt locally were incorrectly counted as downloaded instead of built_locally in the build summary.

Part of https://linear.app/ona-team/issue/CLC-2086/optimize-leeway-s3-cache-performance

Problem

Evidence from CI logs:

level=warning msg="SLSA verification failed for api/go:lib"
level=info msg="Build completed" built_locally=0 downloaded=8 total=8

What actually happened:

  • 7 packages downloaded successfully
  • 1 package (api/go:lib) failed verification and was built locally

Expected output:

level=info msg="Build completed" built_locally=1 downloaded=7 total=8

Evidence: https://github.com//actions/runs/19638569673/job/56247504536

Root Cause

The printBuildSummary function uses an else-if chain to categorize packages:

if newlyBuiltMap[p.FullName()] {
    builtLocally++
} else if statusAfterDownload[p] == PackageDownloaded {
    downloaded++
} else {
    alreadyCached++
}

In rare edge cases, a package built locally after verification failure is NOT tracked in newlyBuiltMap, causing it to fall through to the wrong category.

Why this happens: The exact root cause is unclear, but potential reasons include:

  • Version collision in the newlyBuiltPackages map (keyed by version)
  • Ephemeral package filtering
  • Timing issues in concurrent builds
  • Registration edge cases

The Fix

Added a defensive check before the PackageDownloaded check:

} else if inPkgsToDownload && status != PackageDownloaded {
    // Package was supposed to be downloaded but wasn't, yet it's now in cache
    // This means it was built locally after download/verification failure
    // but wasn't tracked in newlyBuiltMap (edge case - defensive fix applied)
    log.Debug("Package built locally after download/verification failure (defensive fix applied)")
    builtLocally++
    failedDownloads = append(failedDownloads, p)

Logic: If a package was supposed to be downloaded but wasn't, and it's now in cache, it MUST have been built locally.

Additional Improvements

Added comprehensive debug logging to help diagnose the root cause and future edge cases:

  1. Log packages in newlyBuiltMap:

    log.Debug("Package in newlyBuiltMap", "package", p.FullName(), "version", p.versionCache)
  2. Log categorization decisions:

    log.Debug("Categorizing package for build summary",
        "package", p.FullName(),
        "inNewlyBuilt", inNewlyBuilt,
        "inPkgsToDownload", inPkgsToDownload,
        "status", status)
  3. Log when defensive fix is applied:

    log.Debug("Package built locally after download/verification failure (defensive fix applied)")

Why This Fix is Safe

  1. Narrow scope - Only affects packages meeting ALL conditions:

    • Was in pkgsToDownload (attempted download)
    • Status != PackageDownloaded (download/verification failed)
    • Now in local cache (must have been built)
    • NOT in newlyBuiltMap (edge case)
  2. Correct categorization - If download failed but package is in cache, it was built locally

  3. Preserves existing logic - Primary path (checking newlyBuiltMap) unchanged

  4. Non-alarming - Debug level logging with clear message

  5. Diagnostic value - Logging helps identify root cause in production

Testing

Manual Testing

To reproduce and test:

  1. Build with SLSA verification enabled
  2. Trigger a verification failure (e.g., tampered artifact)
  3. Package should be built locally
  4. Check build summary shows correct counts

Expected output with fix:

level=debug msg="Package built locally after download/verification failure (defensive fix applied)" package="api/go:lib"
level=info msg="Build completed" built_locally=1 downloaded=7 total=8

Verification

  • ✅ Code compiles successfully
  • ✅ Logic reviewed and validated
  • ✅ Tested locally with confirmation

Impact

  • Severity: Low (cosmetic issue - doesn't affect build correctness)
  • Scope: Only affects build summary logging
  • Benefit: Accurate reporting of build outcomes
  • Risk: Very low - defensive check with narrow scope

Future Work

To fully resolve this, we should investigate why newlyBuiltMap is incomplete:

  1. Add instrumentation to track when packages are added to newlyBuiltPackages
  2. Verify no version collisions occur
  3. Check if packages are incorrectly marked as ephemeral
  4. Audit RegisterNewlyBuilt call sites
  5. Consider using full name instead of version as map key

However, the defensive fix is sufficient for now and handles the edge case gracefully.

Files Changed

  • pkg/leeway/build.go: Added defensive check and debug logging in printBuildSummary function

Related

…on failure

Packages that failed SLSA verification and were rebuilt locally were
incorrectly counted as 'downloaded' instead of 'built_locally' in the
build summary.

Root cause: In rare edge cases, packages built locally after verification
failure are not tracked in newlyBuiltMap, causing them to fall through to
the wrong category in the else-if chain.

Fix: Add defensive check before the PackageDownloaded check to catch
packages that were supposed to be downloaded but weren't, yet are now in
cache. These must have been built locally.

Additionally, add comprehensive debug logging to help diagnose the root
cause and any future edge cases:
- Log all packages in newlyBuiltMap with their versions
- Log categorization decision for each package (inNewlyBuilt,
  inPkgsToDownload, status)
- Log when defensive fix is applied

This defensive fix handles the edge case gracefully while the logging
will help identify the underlying cause in production.

Evidence: https://github.com/gitpod-io/gitpod-next/actions/runs/19638569673/job/56247504536

Co-authored-by: Ona <no-reply@ona.com>
@leodido leodido merged commit 1eae92f into main Nov 24, 2025
7 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