Fix S3 carve cleanup never running and panic on empty carves (#43045)

[juan-fdz-hawa]

Related issue: Resolves #43045

Fixed a bug where the carve cleanup cron job called the MySQL implementation instead of the S3-aware implementation on S3-configured deployments, meaning expired carves were never marked as expired in S3. Also fixed a panic in S3 carve cleanup that occurred when there were no non-expired carves.

Checklist for submitter

If some of the following don't apply, delete the relevant line.

• Changes file added for user-visible changes in changes/, orbit/changes/ or ee/fleetd-chrome/changes.
  See Changes files for more information.

Testing

• Added/updated automated tests
• QA'd all new/changed functionality manually

Summary by CodeRabbit

• Bug Fixes

  • Cleanup now uses the S3-aware path on S3-configured deployments (no longer the MySQL path)
  • Prevented a panic when no non-expired carves are present during cleanup
  • Improved error aggregation so all cleanup failures are reported
  • Cleanup counters now increment only for successfully updated carves

• Tests

  • Added tests covering S3 carve cleanup behavior, including empty and S3-absent cases

[codecov]

Codecov Report

❌ Patch coverage is 44.44444% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.85%. Comparing base (4b04063) to head (fb5e4ba).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
server/datastore/s3/carves.go	57.14%	2 Missing and 1 partial ⚠️
cmd/fleet/cron.go	0.00%	1 Missing ⚠️
cmd/fleet/serve.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #46462      +/-   ##
==========================================
+ Coverage   66.82%   66.85%   +0.02%     
==========================================
  Files        2808     2808              
  Lines      223582   223603      +21     
  Branches    11347    11347              
==========================================
+ Hits       149413   149487      +74     
+ Misses      60615    60544      -71     
- Partials    13554    13572      +18     

Flag	Coverage Δ
backend	68.58% <44.44%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 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.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[juan-fdz-hawa]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 1428cbc6-9d08-4201-a249-2e70fe859867

📥 Commits

Reviewing files that changed from the base of the PR and between b4309d3 and fb5e4ba.

📒 Files selected for processing (1)

• server/datastore/s3/carves.go

🚧 Files skipped from review as they are similar to previous changes (1)

• server/datastore/s3/carves.go

---

Walkthrough

This PR fixes two bugs in S3 carve cleanup. It routes the cleanup cron job to invoke the S3-aware CarveStore.CleanupCarves instead of the MySQL datastore method, and fixes a panic that occurs when there are no non-expired carves in the database. The S3 implementation now short-circuits early on empty carve lists, uses errors.Join to aggregate update failures while continuing processing, and increments the cleanup counter only after successful updates. New test cases cover both the empty carves scenario and the full S3 cleanup path.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and concisely summarizes the main changes: fixing S3 carve cleanup not running and a panic on empty carves, directly matching the primary objectives.
Description check	✅ Passed	The description includes a related issue reference, marks the changes file added, and indicates automated tests were added and manual QA performed. Required testing and changes file checkboxes are completed.
Linked Issues check	✅ Passed	The PR implements both bug fixes from #43045: wiring carveStore to newCleanupsAndAggregationSchedule for S3-aware cleanup and adding an empty-slice guard in S3CarveStore.CleanupCarves to prevent panic.
Out of Scope Changes check	✅ Passed	All changes directly address the two bugs specified in #43045: changes file for the fix, wiring changes to use S3-aware cleanup, guard against empty slice panic, and tests validating both fixes.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 43045-s3-carve-cleanup-never-runs

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

_{Tip: disable this comment in your organization's Code Review settings.}

[nulmete]

nit: is it worth adding extra information to the error string? maybe the carve's ID?