Renamed activity tables and moved host activities cleanup to activity bounded context. - #41194
Merged
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #41194 +/- ##
==========================================
+ Coverage 66.33% 66.35% +0.02%
==========================================
Files 2474 2477 +3
Lines 198187 198416 +229
Branches 8775 8775
==========================================
+ Hits 131458 131651 +193
- Misses 54848 54865 +17
- Partials 11881 11900 +19
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:
|
getvictor
changed the title
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR continues the activity bounded-context migration by renaming the legacy activities/host_activities tables to activity_past/activity_host_past, and moving host-activity link cleanup into the activity bounded context so host deletions don’t leave stale activity/host associations.
Changes:
- Renames DB tables via a new migration and updates schema.sql accordingly (
activities→activity_past,host_activities→activity_host_past). - Adds
CleanupHostActivitiesto the activity bounded context API/service and wires it into host deletion flows. - Updates service mocks and multiple unit/integration tests to use the new table names and the new activity write service interface.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/testing_utils.go | Updates test service setup to use the renamed activity mock type. |
| server/service/service.go | Switches injected activity dependency to a broader write-service interface. |
| server/service/hosts.go | Calls activity bounded context cleanup when hosts are deleted. |
| server/service/hosts_test.go | Adds assertions that host deletion triggers activity host-link cleanup. |
| server/service/integration_mdm_test.go | Updates raw SQL to reference activity_past. |
| server/service/integration_live_queries_test.go | Updates raw SQL to reference activity_past. |
| server/service/integration_enterprise_test.go | Updates activity-count queries to activity_past. |
| server/service/integration_android_software_test.go | Updates activity-count queries to activity_past. |
| server/mock/service/service_mock.go | Updates mock service signature to accept fleet.ActivityWriteService. |
| server/mock/activity_mock.go | Renames the mock and adds CleanupHostActivities support for tests. |
| server/fleet/service.go | Updates SetActivityService signature to ActivityWriteService. |
| server/fleet/activities.go | Introduces ActivityWriteService interface (new activity + host cleanup). |
| server/datastore/mysql/schema.sql | Renames the activity tables in the canonical schema. |
| server/datastore/mysql/migrations/tables/20260306120000_RenameActivitiesToActivityPast.go | Adds migration to rename activity tables. |
| server/datastore/mysql/hosts.go | Removes host_activities from host deletion reference tables. |
| server/datastore/mysql/activities.go | Updates lock/wipe cleanup queries to renamed tables. |
| server/activity/internal/types/activity.go | Extends activity datastore interface with host-activity cleanup method. |
| server/activity/internal/service/cleanup_host_activities.go | Implements CleanupHostActivities in activity service layer. |
| server/activity/internal/service/service_test.go | Updates mocks to satisfy the extended datastore interface. |
| server/activity/internal/service/handler_test.go | Updates mock service to include CleanupHostActivities. |
| server/activity/internal/testutils/testutils.go | Updates truncate/insert helpers to use renamed tables. |
| server/activity/internal/mysql/new_activity.go | Writes new activities and host links to renamed tables. |
| server/activity/internal/mysql/activity.go | Updates list/cleanup/streaming queries to renamed tables and adds host-link cleanup. |
| server/activity/internal/mysql/activity_test.go | Updates streamed-activity test to reference activity_past. |
| server/activity/internal/mysql/activity_cleanup_test.go | Adds/updates tests around host-link cleanup using renamed join table. |
| server/activity/api/service.go | Extends bounded-context service interface with host-activity cleanup service. |
| server/activity/api/cleanup_host_activities.go | Defines the new public bounded-context cleanup interface. |
| ee/server/service/mdm_external_test.go | Updates to use the renamed activity mock type. |
Comments suppressed due to low confidence (1)
server/datastore/mysql/hosts.go:604
hostRefsno longer includes the activity/host join table. As a result, datastore-level host deletions (ds.DeleteHost/ds.DeleteHosts) will stop removing rows from the join table, leaving stale host links that can prevent activity cleanup (expired activities will look host-linked forever) and grow unbounded. Consider deleting fromactivity_host_pastas part ofdeleteHosts(e.g. add it tohostRefs) or ensure all datastore-level deletions are routed through the activity bounded context cleanup consistently.
// hostRefs are the tables referenced by hosts.
// These tables are cleared when the host is deleted.
var hostRefs = []string{
"host_seen_times",
"host_software",
"host_users",
"host_emails",
"host_additional",
"scheduled_query_stats",
"label_membership",
"policy_membership",
"host_mdm",
"host_munki_info",
"host_device_auth",
"host_batteries",
"host_operating_system",
"host_orbit_info",
"host_munki_issues",
"host_display_names",
"windows_updates",
"host_disks",
"host_updates",
"host_disk_encryption_keys",
"host_software_installed_paths",
"query_results",
"host_mdm_actions",
"host_calendar_events",
"upcoming_activities",
"host_certificates",
"android_devices",
"host_scim_user",
"batch_activity_host_results",
"host_mdm_commands",
"microsoft_compliance_partner_host_statuses",
"host_identity_scep_certificates",
"conditional_access_scep_certificates",
"host_conditional_access",
// unlike for host_software_installs, where we use soft-delete so that
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
WalkthroughThis pull request completes the migration of activity write operations to a bounded context by introducing new database tables, refactoring service interfaces, and implementing host activity cleanup. The changes rename the Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 3
🧹 Nitpick comments (3)
server/activity/internal/service/cleanup_host_activities.go (1)
10-14: Short-circuit empty cleanup requests.Returning early when
hostIDsis empty keeps this method idempotent and avoids pushing empty slices into datastore delete code, whereIN (...)handling is easy to get wrong.🛡️ Suggested change
func (s *Service) CleanupHostActivities(ctx context.Context, hostIDs []uint) error { + if len(hostIDs) == 0 { + return nil + } if err := s.store.CleanupHostActivities(ctx, hostIDs); err != nil { return ctxerr.Wrap(ctx, err, "cleanup host activities") } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/activity/internal/service/cleanup_host_activities.go` around lines 10 - 14, The CleanupHostActivities handler should short-circuit when given an empty hostIDs slice to avoid sending empty IN (...) queries to the datastore; inside Service.CleanupHostActivities check if len(hostIDs) == 0 and immediately return nil, otherwise call s.store.CleanupHostActivities as before (keep existing ctxerr.Wrap behavior and function name CleanupHostActivities to locate change).server/activity/internal/mysql/activity.go (1)
229-244: Refresh the cleanup comment and wrapped error text.The SQL is fine, but the doc comment and
ctxerr.Wrapmessages still sayhost_activities. After the rename, that will point anyone debugging a failure at the wrong table name.♻️ Suggested cleanup
-// CleanupHostActivities removes host_activities rows for the given host IDs. +// CleanupHostActivities removes activity_host_past rows for the given host IDs. func (ds *Datastore) CleanupHostActivities(ctx context.Context, hostIDs []uint) error { ctx, span := tracer.Start(ctx, "activity.mysql.CleanupHostActivities") defer span.End() @@ stmt, args, err := sqlx.In(`DELETE FROM activity_host_past WHERE host_id IN (?)`, hostIDs) if err != nil { - return ctxerr.Wrap(ctx, err, "build host_activities IN query") + return ctxerr.Wrap(ctx, err, "build activity_host_past IN query") } if _, err := ds.primary.ExecContext(ctx, stmt, args...); err != nil { - return ctxerr.Wrap(ctx, err, "delete host_activities for deleted hosts") + return ctxerr.Wrap(ctx, err, "delete activity_host_past rows for deleted hosts") } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/activity/internal/mysql/activity.go` around lines 229 - 244, Update the doc comment and error wrap strings in Datastore.CleanupHostActivities to reference the correct table name activity_host_past (not host_activities); specifically change the top comment for CleanupHostActivities and the two ctxerr.Wrap messages that currently say "build host_activities IN query" and "delete host_activities for deleted hosts" to mention activity_host_past so logs and docs point to the right table.server/mock/activity_mock.go (1)
27-30: Mirror delegate behavior for cleanup calls.
NewActivityforwards toDelegate, butCleanupHostActivitiesignores it. That makes delegated tests exercise the real create path while silently skipping the real cleanup path. Consider wideningDelegatetofleet.ActivityWriteServiceor adding a separate cleanup delegate so both write operations can opt into real behavior consistently.♻️ One way to keep delegation consistent
type MockActivityService struct { NewActivityFunc NewActivityFunc // defaults to NoopNewActivityFunc if nil NewActivityFuncInvoked bool - Delegate activity_api.NewActivityService + Delegate fleet.ActivityWriteService CleanupHostActivitiesFunc func(ctx context.Context, hostIDs []uint) error CleanupHostActivitiesFuncInvoked bool @@ func (m *MockActivityService) CleanupHostActivities(ctx context.Context, hostIDs []uint) error { m.mu.Lock() m.CleanupHostActivitiesFuncInvoked = true m.mu.Unlock() + if m.Delegate != nil { + if err := m.Delegate.CleanupHostActivities(ctx, hostIDs); err != nil { + return err + } + } if m.CleanupHostActivitiesFunc != nil { return m.CleanupHostActivitiesFunc(ctx, hostIDs) } return nil }Also applies to: 54-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/mock/activity_mock.go` around lines 27 - 30, The mock currently forwards NewActivity to Delegate (type activity_api.NewActivityService) but CleanupHostActivities ignores Delegate, causing inconsistent test delegation; change Delegate to a wider fleet.ActivityWriteService (or add a separate CleanupDelegate field) and then implement CleanupHostActivities to call through to Delegate.CleanupHostActivities (or the new CleanupDelegate) when present, while still honoring CleanupHostActivitiesFunc and CleanupHostActivitiesFuncInvoked semantics so both create and cleanup paths can opt into real behavior consistently.
🤖 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/api/cleanup_host_activities.go`:
- Around line 5-8: Update the exported doc comments on the
CleanupHostActivitiesService interface and its CleanupHostActivities method to
remove the concrete table name "host_activities" and use storage-agnostic
wording (e.g., "host activity records" or "activity records") so the public API
does not reference a renamed/removed table; update the comment above the type
and the method signature comment to something like "cleans up host activity
records when hosts are deleted" and "removes activity records for the given host
IDs" while keeping the identifiers CleanupHostActivitiesService and
CleanupHostActivities unchanged.
In `@server/service/hosts.go`:
- Around line 587-589: The current flow calls
svc.activitySvc.CleanupHostActivities after svc.ds.DeleteHosts /
svc.ds.DeleteHost commit, which makes deletion non-atomic and aborts subsequent
steps on cleanup failure; either move the CleanupHostActivities invocation into
the same datastore transaction that performs svc.ds.DeleteHosts /
svc.ds.DeleteHost (so the DB delete and activity cleanup are committed/rolled
back together), or change the error handling to a compensating retry/background
job (enqueue a retry task instead of returning an error) and ensure the MDM
lifecycle work and deletion activity creation still run regardless of immediate
cleanup result; update code paths referencing
svc.activitySvc.CleanupHostActivities, svc.ds.DeleteHosts, svc.ds.DeleteHost,
and the deletion-activity / MDM lifecycle continuation to implement the
transactional unit or reliable retry mechanism.
In `@server/service/integration_live_queries_test.go`:
- Line 279: The test currently selects the latest global live_query activity
(`SELECT details FROM activity_past WHERE activity_type = 'live_query' ORDER BY
id DESC LIMIT 1`), which is flaky; change the SQL in the test to scope the
lookup to the specific entity created in this test by adding a WHERE predicate
that matches this test's query/host/campaign identifiers (e.g. add conditions
like campaign_id = <this test's campaign id> and/or query_id = <q1.ID> or match
details->>'query' = <q1.SQL>), and then remove the reliance on ORDER BY id DESC
so the SELECT returns the activity for q1 deterministically.
---
Nitpick comments:
In `@server/activity/internal/mysql/activity.go`:
- Around line 229-244: Update the doc comment and error wrap strings in
Datastore.CleanupHostActivities to reference the correct table name
activity_host_past (not host_activities); specifically change the top comment
for CleanupHostActivities and the two ctxerr.Wrap messages that currently say
"build host_activities IN query" and "delete host_activities for deleted hosts"
to mention activity_host_past so logs and docs point to the right table.
In `@server/activity/internal/service/cleanup_host_activities.go`:
- Around line 10-14: The CleanupHostActivities handler should short-circuit when
given an empty hostIDs slice to avoid sending empty IN (...) queries to the
datastore; inside Service.CleanupHostActivities check if len(hostIDs) == 0 and
immediately return nil, otherwise call s.store.CleanupHostActivities as before
(keep existing ctxerr.Wrap behavior and function name CleanupHostActivities to
locate change).
In `@server/mock/activity_mock.go`:
- Around line 27-30: The mock currently forwards NewActivity to Delegate (type
activity_api.NewActivityService) but CleanupHostActivities ignores Delegate,
causing inconsistent test delegation; change Delegate to a wider
fleet.ActivityWriteService (or add a separate CleanupDelegate field) and then
implement CleanupHostActivities to call through to
Delegate.CleanupHostActivities (or the new CleanupDelegate) when present, while
still honoring CleanupHostActivitiesFunc and CleanupHostActivitiesFuncInvoked
semantics so both create and cleanup paths can opt into real behavior
consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac07eeb7-5919-42fc-984f-7650e3d282cb
📒 Files selected for processing (28)
ee/server/service/mdm_external_test.goserver/activity/api/cleanup_host_activities.goserver/activity/api/service.goserver/activity/internal/mysql/activity.goserver/activity/internal/mysql/activity_cleanup_test.goserver/activity/internal/mysql/activity_test.goserver/activity/internal/mysql/new_activity.goserver/activity/internal/service/cleanup_host_activities.goserver/activity/internal/service/handler_test.goserver/activity/internal/service/service_test.goserver/activity/internal/testutils/testutils.goserver/activity/internal/types/activity.goserver/datastore/mysql/activities.goserver/datastore/mysql/hosts.goserver/datastore/mysql/migrations/tables/20260306120000_RenameActivitiesToActivityPast.goserver/datastore/mysql/schema.sqlserver/fleet/activities.goserver/fleet/service.goserver/mock/activity_mock.goserver/mock/service/service_mock.goserver/service/hosts.goserver/service/hosts_test.goserver/service/integration_android_software_test.goserver/service/integration_enterprise_test.goserver/service/integration_live_queries_test.goserver/service/integration_mdm_test.goserver/service/service.goserver/service/testing_utils.go
💤 Files with no reviewable changes (1)
- server/datastore/mysql/hosts.go
cdcme
approved these changes
Mar 8, 2026
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
FYI. There is an existing related bug: #41190 (Audit log entries permanently deleted)
Hopefully, the bug fix will delete some of the code touched by this PR.
Checklist for submitter
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements)Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
Summary by CodeRabbit
Bug Fixes
Chores