Moved cleanup activities logic to activity bounded context. - #40663
Merged
Conversation
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
WalkthroughThis PR refactors activity cleanup operations by introducing a new Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/datastore/mysql/activities.go (1)
311-317:⚠️ Potential issue | 🟠 MajorBound batch size in SQL selection to avoid unbounded memory growth.
Line 311, Line 340, and Line 369 materialize full ID lists before chunking. On large tables this can spike memory and make cleanup runs too heavy. Prefer iterative
SELECT ... LIMIT ?+ delete loops so memory and lock windows stay bounded.Suggested fix pattern
- var allUnsavedQueryIDs []uint - if err := sqlx.SelectContext(ctx, ds.reader(ctx), &allUnsavedQueryIDs, selectUnsavedQueryIDs, expiredWindowDays); err != nil { - return ctxerr.Wrap(ctx, err, "selecting expired unsaved query IDs") - } - unsavedQueryIter := slices.Chunk(allUnsavedQueryIDs, deleteIDsBatchSize) - for unsavedQueryIDs := range unsavedQueryIter { + for { + var unsavedQueryIDs []uint + const selectUnsavedQueryIDsBatch = ` + SELECT id + FROM queries + WHERE NOT saved + AND created_at < DATE_SUB(NOW(), INTERVAL ? DAY) + ORDER BY id + LIMIT ?` + if err := sqlx.SelectContext(ctx, ds.reader(ctx), &unsavedQueryIDs, selectUnsavedQueryIDsBatch, expiredWindowDays, deleteIDsBatchSize); err != nil { + return ctxerr.Wrap(ctx, err, "selecting expired unsaved query IDs") + } + if len(unsavedQueryIDs) == 0 { + break + } const deleteStmt = `DELETE FROM queries WHERE id IN (?)` ... }Apply the same bounded-loop approach to campaign and campaign-target cleanup sections.
Also applies to: 340-347, 369-376
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/activities.go` around lines 311 - 317, The code currently materializes full ID arrays (using sqlx.SelectContext into allUnsavedQueryIDs and then slices.Chunk) which can OOM on large tables; change to an iterative bounded SELECT + delete loop that fetches up to deleteIDsBatchSize IDs per iteration and deletes them until no rows are returned. Specifically, replace the sqlx.SelectContext(..., selectUnsavedQueryIDs, expiredWindowDays) + slices.Chunk usage with a loop that calls sqlx.SelectContext(ctx, ds.reader(ctx), &batchIDs, selectUnsavedQueryIDsWithLimit, expiredWindowDays, deleteIDsBatchSize) (or otherwise binds LIMIT), deletes that batch, and repeats until batchIDs is empty; apply the same pattern to the campaign and campaign-target cleanup sections (the other blocks that materialize full ID lists) and keep using deleteIDsBatchSize to bound memory and lock windows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/activity/internal/mysql/activity.go`:
- Around line 205-225: The current read-then-delete in CleanupExpiredActivities
can delete a row that becomes host-linked between statements; fix it by
performing the selection and deletion inside a transaction with row locks: begin
a transaction (use sqlx.BeginTxx on ds.primary), run the select query within the
transaction using FOR UPDATE (modify the existing selectQuery to append "FOR
UPDATE") via tx.SelectContext to populate activityIDs, re-check that selected
IDs have no host links (or rely on the SELECT ... LEFT JOIN ha WHERE
ha.activity_id IS NULL FOR UPDATE), then run the DELETE within the same tx using
tx.ExecContext and commit; update uses of sqlx.SelectContext/sqlx.In to their tx
equivalents so all operations occur in the same transaction context and prevent
the race.
---
Outside diff comments:
In `@server/datastore/mysql/activities.go`:
- Around line 311-317: The code currently materializes full ID arrays (using
sqlx.SelectContext into allUnsavedQueryIDs and then slices.Chunk) which can OOM
on large tables; change to an iterative bounded SELECT + delete loop that
fetches up to deleteIDsBatchSize IDs per iteration and deletes them until no
rows are returned. Specifically, replace the sqlx.SelectContext(...,
selectUnsavedQueryIDs, expiredWindowDays) + slices.Chunk usage with a loop that
calls sqlx.SelectContext(ctx, ds.reader(ctx), &batchIDs,
selectUnsavedQueryIDsWithLimit, expiredWindowDays, deleteIDsBatchSize) (or
otherwise binds LIMIT), deletes that batch, and repeats until batchIDs is empty;
apply the same pattern to the campaign and campaign-target cleanup sections (the
other blocks that materialize full ID lists) and keep using deleteIDsBatchSize
to bound memory and lock windows.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
cmd/fleet/cron.gocmd/fleet/serve.goserver/activity/api/cleanup_expired_activities.goserver/activity/api/service.goserver/activity/bootstrap/testing.goserver/activity/internal/mysql/activity.goserver/activity/internal/mysql/activity_cleanup_test.goserver/activity/internal/service/cleanup_expired_activities.goserver/activity/internal/service/handler_test.goserver/activity/internal/service/service_test.goserver/activity/internal/types/activity.goserver/datastore/mysql/activities.goserver/datastore/mysql/activities_test.goserver/fleet/datastore.goserver/mock/datastore_mock.go
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors cleanup responsibilities to align with the activity bounded-context migration by moving activity expiry deletion behind the activity service, while separating live-query cleanup into its own datastore method and cron job.
Changes:
- Add
CleanupExpiredActivitiesto the activity bounded context (API/service + MySQL implementation + tests). - Replace legacy
CleanupActivitiesAndAssociatedDatawithCleanupExpiredLiveQueriesonfleet.Datastore(and update mocks/tests). - Update cron wiring to run activity cleanup via
activitySvcand live-query cleanup viads.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/mock/datastore_mock.go | Updates datastore mock to match renamed cleanup method (CleanupExpiredLiveQueries). |
| server/fleet/datastore.go | Replaces legacy activity cleanup API with CleanupExpiredLiveQueries in the datastore interface. |
| server/datastore/mysql/activities_test.go | Updates MySQL datastore tests to cover CleanupExpiredLiveQueries behavior. |
| server/datastore/mysql/activities.go | Removes legacy activity cleanup implementation; keeps live-query cleanup as CleanupExpiredLiveQueries. |
| server/activity/internal/types/activity.go | Extends activity bounded-context datastore interface with CleanupExpiredActivities. |
| server/activity/internal/service/service_test.go | Updates service test mocks to satisfy new datastore interface. |
| server/activity/internal/service/handler_test.go | Updates validation-test service mock to satisfy new service interface. |
| server/activity/internal/service/cleanup_expired_activities.go | Adds service method that delegates to the activity datastore cleanup. |
| server/activity/internal/mysql/activity_cleanup_test.go | Adds MySQL integration tests for CleanupExpiredActivities. |
| server/activity/internal/mysql/activity.go | Implements MySQL CleanupExpiredActivities. |
| server/activity/bootstrap/testing.go | Updates noop store to satisfy new datastore interface for tests. |
| server/activity/api/service.go | Adds CleanupExpiredActivitiesService to the activity service interface. |
| server/activity/api/cleanup_expired_activities.go | Introduces API interface for activity cleanup. |
| cmd/fleet/serve.go | Passes activitySvc into the cleanups schedule factory. |
| cmd/fleet/cron.go | Runs activity cleanup via activitySvc and adds separate live-query cleanup job via ds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #40663 +/- ##
==========================================
- Coverage 66.28% 66.27% -0.01%
==========================================
Files 2467 2468 +1
Lines 197503 197520 +17
Branches 8656 8763 +107
==========================================
+ Hits 130905 130911 +6
- Misses 54742 54754 +12
+ Partials 11856 11855 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…activities # Conflicts: # server/activity/bootstrap/testing.go
getvictor
had a problem deploying
to
Docker Hub
February 27, 2026 14:51 — with 
GitHub Actions
Failure
GitHub Actions
Failure
cdcme
approved these changes
Feb 27, 2026
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue: Resolves #38536
Split the activities cleanup job from the queries cleanup job.
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit
Release Notes
New Features
Improvements