Fix time.Ticker leak in conditional access polling#48538
Open
sharon-fdm wants to merge 2 commits into
Open
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #48538 +/- ##
==========================================
+ Coverage 67.90% 68.01% +0.10%
==========================================
Files 3678 3678
Lines 233673 233763 +90
Branches 12452 12452
==========================================
+ Hits 158686 158990 +304
+ Misses 60724 60472 -252
- Partials 14263 14301 +38
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:
|
…42901) Adds a test that instruments the ticker factory to verify every ticker created during macOS conditional access polling is properly stopped, preventing resource leaks. The test detects the old time.Tick bug by confirming the injectable factory is used and Stop() is called for each ticker. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![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.
Contributor
|
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 (3)
WalkthroughThis change fixes a resource leak in Microsoft Conditional Access polling for macOS hosts. A package-level Changes
Sequence Diagram(s)sequenceDiagram
participant setHostConditionalAccess
participant newConditionalAccessTicker
participant tickCh
setHostConditionalAccess->>newConditionalAccessTicker: create ticker
newConditionalAccessTicker-->>setHostConditionalAccess: tickCh, tickStop
setHostConditionalAccess->>setHostConditionalAccess: defer tickStop()
loop poll until completion or timeout
tickCh->>setHostConditionalAccess: tick
setHostConditionalAccess->>setHostConditionalAccess: check message status
end
setHostConditionalAccess->>tickStop: Stop() on return
Related issues: Suggested labels: bug, go, server Suggested reviewers: fleetdm maintainers familiar with server/service/osquery.go Poem: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Contributor
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Fixes a goroutine/resource leak in the Microsoft Conditional Access polling loop by replacing time.Tick (unstoppable) with a stoppable ticker, and adds a unit test to verify ticker cleanup behavior.
Changes:
- Replace
time.Tickwith atime.NewTicker-backed, stoppable ticker in the macOS conditional access polling loop. - Add an injectable ticker factory (
newConditionalAccessTicker) to enable deterministic testing of ticker cleanup. - Add a unit test (
TestSetHostConditionalAccess_TickerCleanup) that verifies each polling invocation stops its ticker.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| server/service/osquery.go | Replaces time.Tick usage with a stoppable ticker via an injectable factory to prevent leaks in the conditional access polling loop. |
| server/service/osquery_conditional_access_test.go | Adds a focused unit test to assert tickers created for polling are properly stopped. |
| changes/42901-fix-ticker-leak | User-visible changes entry (excluded from review by content exclusion policy). |
Files excluded by content exclusion policy (1)
- changes/42901-fix-ticker-leak
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+86
to
+101
| // Use a very short poll interval so the test runs fast. | ||
| origWait := conditionalAccessSetWaitTime | ||
| conditionalAccessSetWaitTime = 1 * time.Millisecond | ||
| t.Cleanup(func() { | ||
| conditionalAccessSetWaitTime = origWait | ||
| }) | ||
|
|
||
| // Instrument the ticker factory to track Stop() calls. | ||
| var tickersCreated atomic.Int32 | ||
| var tickersStopped atomic.Int32 | ||
|
|
||
| origTickerFactory := newConditionalAccessTicker | ||
| newConditionalAccessTicker = func(d time.Duration) (<-chan time.Time, func()) { | ||
| tickersCreated.Add(1) | ||
| ticker := time.NewTicker(d) | ||
| return ticker.C, func() { |
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
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: Closes #42901
Checklist for submitter
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Timeouts are implemented and retries are limited to avoid infinite loops
Summary
time.Tickwithtime.NewTicker+defer ticker.Stop()in the conditional access polling loop inserver/service/osquery.gonewConditionalAccessTicker) to enable deterministic testing of cleanup behaviorReproduction
The leak was reproduced locally by writing a unit test (
TestSetHostConditionalAccess_TickerCleanup) that instruments the ticker factory to track creation andStop()calls.With the old
time.Tickcode (reverted locally to verify):The test detected 0 tickers created through the factory (because
time.Tickbypasses it entirely), proving the fix is absent andStop()is never called:With the fix applied:
All 10 tickers are created and all 10 are properly stopped:
Testing
TestSetHostConditionalAccess_TickerCleanup-- unit test that verifies every ticker created in the polling loop is properly stopped after the function returns. CallssetHostConditionalAccess10 times with"darwin"platform (which triggers the polling path) and asserts that all 10 tickers were created and stopped.go vet ./server/service/...-- passed with no issuesgo test -run "ConditionalAccess" ./server/service/ -count=1) -- all passedTestConditionalAccess*inintegration_enterprise_test.go) requireMYSQL_TEST=1 REDIS_TEST=1and will be validated in CI🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests