Fixed Android hosts being auto-deleted by host expiry - #45471
Conversation
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.
There was a problem hiding this comment.
Pull request overview
Fixes an hourly enroll/delete loop for re-enrolled Android hosts when host expiry is enabled. EnrollOrbit (and EnrollOsquery) previously initialized timestamp columns to 1970-01-02 00:00:00, but CleanupExpiredHosts only nullifies the canonical 2000-01-01 00:00:00 sentinel — causing fresh rows to be treated as last-seen in 1970 and immediately deleted. The PR switches both enroll paths to use common_mysql.GetDefaultNonZeroTime() (2000-01-01 UTC), which matches the sentinel CleanupExpiredHosts already recognizes (and is consistent with the Android setTimesToNonZero path).
Changes:
- Replace
time.Unix(0, 0).Add(24 * time.Hour)withcommon_mysql.GetDefaultNonZeroTime()inEnrollOrbitandEnrollOsquery. - Add a user-visible changes note describing the fix.
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| server/datastore/mysql/hosts.go | Use canonical 2000-01-01 sentinel for newly inserted enroll rows so cleanup SQL's NULLIF recognizes them. |
| changes/45258-android-host-expiry-loop | Release note for the bug fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
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 (2)
WalkthroughThis PR fixes a host expiry issue where Android devices re-enrolled after their Fleet host record was deleted would be immediately deleted again by the cleanup cron, creating an indefinite hourly loop. The fix updates both 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
| var host fleet.Host | ||
| err := ds.withRetryTxx(ctx, func(tx sqlx.ExtContext) error { | ||
| zeroTime := time.Unix(0, 0).Add(24 * time.Hour) | ||
| zeroTime := common_mysql.GetDefaultNonZeroTime() |
There was a problem hiding this comment.
Is this for consistency? (Or is there "osquery" API enrollment in Android?)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #45471 +/- ##
==========================================
+ Coverage 66.71% 66.73% +0.02%
==========================================
Files 2734 2736 +2
Lines 218832 218850 +18
Branches 10820 10820
==========================================
+ Hits 145987 146047 +60
+ Misses 59625 59597 -28
+ Partials 13220 13206 -14
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:
|
Related issue: Resolves #45258
The change to zero time is conventionally consistent with the rest of the codebase, and produces zero observable behavior differences anywhere I could find except for the cleanup SQL (which is the bug being fixed).
Decided not to do a migration since we do not expect hosts to actually stick around with the
1970-01-02value, and we have not heard about this issue from customers.Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Summary by CodeRabbit