Fix tight install loop on continous automations feature - #46823
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR implements cooldown throttling for continuous policy automations to prevent tight install-refetch-rerun loops. It adds a datastore method to query recently verified VPP installs, exposes UpdatedAt on HostLastInstallData, integrates cooldown checks in software and VPP policy automation loops (newly failing policies bypass cooldown), and adds unit and integration tests plus mock wiring to validate the behavior. Possibly related issues
🚥 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 docstrings
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/service/osquery_test.go (1)
4802-4809: ⚡ Quick winMake cooldown interval explicit in test setup.
Both tests currently rely on
config.TestConfig()defaulting the policy update interval to 1 hour. Setting the interval explicitly in these test setups will make the assertions resilient to future config-default changes.Also applies to: 4822-4823
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/service/osquery_test.go` around lines 4802 - 4809, The test depends on config.TestConfig() defaulting the policy update interval to 1 hour; make that explicit by creating a local config, set its PolicyUpdateInterval (e.g. cfg.PolicyUpdateInterval = 1*time.Hour), and pass that cfg into newTestServiceWithConfig (used with TestServerOpts/mockClock) before calling svcImpl.continuousAutomationOnCooldown; apply the same change for the second test block as well so the assertions no longer rely on TestConfig() defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/datastore/mysql/vpp.go`:
- Around line 1289-1292: The SQL predicate is using created_at for the cooldown
window causing recently verified but long-running installs to be excluded;
update the query in the host_vpp_software_installs lookup (the SELECT
DISTINCT(adam_id) ... WHERE host_id = ? AND canceled = 0 AND verification_at IS
NOT NULL ...) to compare verification_at against NOW() - INTERVAL ? SECOND
(using the existing hostID and seconds parameters) instead of created_at,
keeping the DISTINCT(adam_id) and existing error handling (err != nil && err !=
sql.ErrNoRows) intact.
---
Nitpick comments:
In `@server/service/osquery_test.go`:
- Around line 4802-4809: The test depends on config.TestConfig() defaulting the
policy update interval to 1 hour; make that explicit by creating a local config,
set its PolicyUpdateInterval (e.g. cfg.PolicyUpdateInterval = 1*time.Hour), and
pass that cfg into newTestServiceWithConfig (used with TestServerOpts/mockClock)
before calling svcImpl.continuousAutomationOnCooldown; apply the same change for
the second test block as well so the assertions no longer rely on TestConfig()
defaults.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae3a8052-ff6a-43b8-92e4-0ce86b8247ec
📒 Files selected for processing (8)
server/datastore/mysql/software_installers.goserver/datastore/mysql/vpp.goserver/datastore/mysql/vpp_test.goserver/fleet/datastore.goserver/fleet/software_installer.goserver/mock/datastore_mock.goserver/service/osquery.goserver/service/osquery_test.go
There was a problem hiding this comment.
Pull request overview
This PR addresses a tight install→refetch→policy re-run→install loop that can occur when continuous policy automations are enabled and a policy keeps failing even after successful installs, by adding a cooldown aligned to the policy update interval.
Changes:
- Add a service-level cooldown helper and apply it to continuous software installer policy automations.
- Add VPP continuous automation throttling based on recent verified installs within the policy update interval.
- Extend datastore plumbing (interfaces, mock, MySQL) and add tests for the cooldown + new MySQL query.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/osquery.go | Adds cooldown helper and throttling logic for continuous software installer and VPP policy automations. |
| server/service/osquery_test.go | Adds unit tests for cooldown helper and software installer throttling behavior. |
| server/mock/datastore_mock.go | Adds mock support for the new VPP “recently verified installs” datastore method. |
| server/fleet/software_installer.go | Extends HostLastInstallData with CreatedAt for throttling decisions. |
| server/fleet/datastore.go | Adds MapAdamIDsRecentlyVerifiedInstalls to the datastore interface with docs. |
| server/datastore/mysql/vpp.go | Implements MapAdamIDsRecentlyVerifiedInstalls in MySQL. |
| server/datastore/mysql/vpp_test.go | Adds MySQL test coverage for MapAdamIDsRecentlyVerifiedInstalls. |
| server/datastore/mysql/software_installers.go | Updates last-install queries to return created_at for throttling. |
💡 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 #46823 +/- ##
==========================================
- Coverage 66.94% 66.94% -0.01%
==========================================
Files 2840 2840
Lines 225348 225382 +34
Branches 11537 11537
==========================================
+ Hits 150865 150886 +21
- Misses 60789 60797 +8
- Partials 13694 13699 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| `SELECT DISTINCT(adam_id) FROM host_vpp_software_installs | ||
| WHERE host_id = ? AND canceled = 0 AND removed = 0 | ||
| AND verification_at >= NOW() - INTERVAL ? SECOND`, |
There was a problem hiding this comment.
nit: wondering if this will require a covering index in the short term
There was a problem hiding this comment.
Definitely. Though given it's only checked on some policies/automations (and on distributed/write not distributed/read) it's probably a-ok for now. We'll know more during load tests.
Related issue: Resolves #45149 (adds to)
Testing
Summary by CodeRabbit
Bug Fixes
New Features
Tests