ci: add cleanup step for Buck2 build

[jjl9807]

No description provided.

[github-actions]

Claude finished @jjl9807's task in 43s —— View job

---

Code Review Complete ✅

Tasks

• Read the modified workflow file
• Analyze the git diff to understand changes
• Review for code quality and best practices
• Check for security implications
• Provide comprehensive feedback

---

Summary

This PR adds a cleanup step to the Buck2 build job in the CI workflow. The change is well-implemented and follows best practices for GitHub Actions workflows.

Detailed Review

✅ What's Good

1. Proper Use of if: always() (.github/workflows/base.yml:98)

   • The cleanup step correctly uses if: always() to ensure Buck2 artifacts are cleaned up even if the build step fails
   • This prevents disk space issues on self-hosted runners over time

2. Placement and Structure

   • The cleanup step is appropriately placed immediately after the build step
   • Follows GitHub Actions best practices for resource cleanup

3. Self-Hosted Runner Consideration

   • Since this workflow uses runs-on: [self-hosted], cleanup is particularly important to prevent artifact accumulation
   • Buck2 can generate significant build artifacts that would otherwise persist between workflow runs

📝 Minor Observations

1. Whitespace Change (.github/workflows/base.yml:113)

   • An empty line was added before the "Run tests" step
   • This is cosmetic but improves readability

2. No Newline at EOF (.github/workflows/base.yml:116)

   • The file ends without a newline character (indicated by \ No newline at end of file in the diff)
   • This is a very minor style issue - while not technically wrong, most style guides prefer a trailing newline
   • Consider adding a newline at the end of the file: Fix this →

Security ✅

• No security concerns identified
• The buck2 clean command is safe and operates only on Buck2's managed artifacts

Performance ✅

• This change improves performance by preventing disk space issues on self-hosted runners
• The cleanup operation itself is relatively fast compared to the build

Testing ⚠️

• The change cannot be easily tested in isolation
• Consider monitoring self-hosted runner disk usage after this change is merged to verify effectiveness

Documentation ℹ️

• A PR description explaining why this cleanup is needed would be helpful
• Consider adding a comment in the workflow file if this addresses a specific issue (e.g., "Clean up artifacts to prevent disk space issues on self-hosted runners")

Recommendation

APPROVE - This is a solid improvement to the CI workflow that will help maintain self-hosted runner health. The only suggestion is the very minor trailing newline issue, which is optional to fix.

---

[Copilot]

Pull request overview

Adds a post-build cleanup step to the CI workflow to remove Buck2 artifacts after running the Buck2 build job on self-hosted runners.

Changes:

• Add a buck2 clean step after buck2 build in the build job, configured to run even if previous steps fail.
• Minor whitespace-only adjustment in the test job steps.

[github-actions]

Claude finished @jjl9807's task in 1m 5s —— View job

---

Comprehensive Code Review ✅

Tasks

• Read the modified workflow file
• Analyze git diff to understand changes
• Review for code quality and best practices
• Check for security implications
• Assess performance impact
• Verify testing considerations
• Review documentation needs
• Provide comprehensive feedback

---

Summary

This PR adds a cleanup step to the Buck2 build job in the CI workflow. The implementation has been updated to address previous feedback and now includes continue-on-error: true, making it robust and production-ready.

Detailed Analysis

1. Code Quality ✅

Excellent implementation of cleanup step (.github/workflows/base.yml:96-99)

• Properly uses if: always() to ensure cleanup runs regardless of build outcome
• Now includes continue-on-error: true to prevent cleanup failures from breaking the CI pipeline
• Well-structured and follows GitHub Actions best practices
• Clear step naming: "Clean Buck2 artifacts"

Minor whitespace improvement (.github/workflows/base.yml:115)

• Added blank line after checkout step in the test job
• Improves readability and visual separation between steps

2. Security ✅

No security concerns identified:

• buck2 clean is a safe command that only operates on Buck2's managed build artifacts
• No external input is processed
• No credentials or secrets are involved
• Command runs in the repository's workspace context with appropriate permissions

3. Performance ✅

Positive performance impact:

• Primary benefit: Prevents disk space accumulation on self-hosted runners over time
• Buck2 build artifacts can be substantial, especially for Rust projects
• Regular cleanup prevents eventual runner disk exhaustion that would cause future builds to fail
• The cleanup operation itself is relatively fast compared to build times
• Using continue-on-error: true ensures cleanup failures don't add latency to the pipeline

Self-hosted runner optimization:

• This is particularly important for runs-on: [self-hosted] configurations
• GitHub-hosted runners are ephemeral (cleanup happens automatically), but self-hosted runners persist between runs
• Without this cleanup, artifacts would accumulate indefinitely

4. Testing ⚠️

Testing considerations:

• Cannot be easily unit-tested in isolation (workflow changes)

• Recommended monitoring after merge:

  • Monitor self-hosted runner disk usage trends
  • Check that builds continue to succeed
  • Verify cleanup step doesn't introduce failures
  • Review workflow run logs to confirm cleanup executes as expected

• The continue-on-error: true addition is crucial because:

  • If Buck2 daemon is not running or has issues, buck2 clean might fail
  • Build success should not be invalidated by cleanup failures
  • This prevents flaky CI due to non-critical post-build operations

5. Documentation ℹ️

Suggestions for improvement:

1. PR Description: The PR has "No description provided"

   • Consider adding context: "Adds cleanup step to prevent Buck2 artifact accumulation on self-hosted runners"
   • Helps future maintainers understand the motivation

2. Inline workflow comment (optional but helpful):

   # Clean up Buck2 artifacts to prevent disk space issues on self-hosted runners
   # Use continue-on-error to prevent cleanup failures from failing the build
   - name: Clean Buck2 artifacts
     run: buck2 clean
     if: always()
     continue-on-error: true

3. Missing trailing newline (.github/workflows/base.yml:117)

   • The file still ends without a newline character (\ No newline at end of file)
   • While not functionally broken, most style guides and tools expect files to end with a newline
   • Fix this →

6. Comparison with Previous Reviews

Improvements made since initial review:

• ✅ Added continue-on-error: true as suggested by copilot-pull-request-reviewer
• This addresses the main concern about cleanup failures causing build job failures
• The implementation is now more robust and production-ready

Outstanding minor issue:

• ⚠️ Trailing newline still missing (cosmetic only, not a blocker)

Technical Deep Dive

Why continue-on-error: true matters:

Buck2's daemon-based architecture means buck2 clean can occasionally fail if:

• The Buck2 daemon is in an unexpected state
• There are file system permission issues (rare but possible)
• Concurrent Buck2 operations are happening

Without continue-on-error: true, any of these edge cases would:

1. Mark the cleanup step as failed (red ❌)
2. Mark the entire build job as failed
3. Block the PR even though the actual build succeeded

With continue-on-error: true:

1. Cleanup failures are logged but don't fail the job
2. Build success/failure is determined solely by the actual build step
3. CI remains stable and predictable

Why if: always() matters:

Without this condition:

• If buck2 build fails, subsequent steps are skipped by default
• Artifacts from the failed build would remain on disk
• Multiple failed builds would accumulate artifacts

With if: always():

• Cleanup runs whether build succeeds or fails
• Ensures consistent disk space management
• Particularly important for debugging failed builds (you want a clean slate for the next run)

Recommendation

✅ APPROVE - This is a well-implemented improvement that:

• Addresses a real operational concern (disk space management)
• Follows GitHub Actions best practices
• Includes proper error handling (continue-on-error: true)
• Maintains CI pipeline stability
• Has minimal risk and positive impact

The only remaining suggestion is the optional trailing newline fix, which is purely cosmetic and not a blocker for merging.

Additional Context

This change is particularly valuable for:

• Self-hosted runners that persist between builds
• Rust/Buck2 projects which can generate large build artifacts
• Long-running projects where artifact accumulation becomes significant over time

---