Revert "Fix SCEP autorenew failing for offline hosts (#44250)"#44535
Open
mostlikelee wants to merge 1 commit intomainfrom
Open
Revert "Fix SCEP autorenew failing for offline hosts (#44250)"#44535mostlikelee wants to merge 1 commit intomainfrom
mostlikelee wants to merge 1 commit intomainfrom
Conversation
This reverts commit 96569a9.
![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
WalkthroughThis pull request removes previously implemented SCEP renewal improvements and managed certificate handling optimizations. The changes include deletion of changelog entries documenting SCEP renewal behavior, simplification of iOS/iPadOS managed certificate profile status transitions to skip the Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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: 2
🤖 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/datastore/mysql/mdm.go`:
- Around line 2933-2936: The UPDATE building logic (updateQuery +
hostProfileClause with values) is incorrectly resetting any non-NULL status
(including in-flight statuses like "pending" and "verifying") to NULL; change
the WHERE clause construction so only failing/terminal statuses eligible for
retry are cleared (e.g., explicit list such as "failed", "error", or other
non-in-flight codes) and ensure in-flight statuses ("pending", "verifying") are
excluded; adjust the values slice used for the parameterized query (refer to
updateQuery, hostProfileClause, and values) accordingly and restore any prior
cooldown/filtering logic that avoided immediate re-enqueue on CA outages (e.g.,
keep failing-row cooldown checks or last_attempt timestamp conditions instead of
blanket NULLing).
In `@server/service/integration_mdm_test.go`:
- Around line 18168-18171: The test currently hardcodes an expiry offset using
time.Now().Add(-2*time.Hour) when updating the challenges table; replace that
literal with a value derived from the system TTL so expiration remains correct
if fleet.OneTimeChallengeTTL changes: compute the timestamp as
time.Now().Add(-fleet.OneTimeChallengeTTL - buffer) (use a small buffer like
time.Minute) and pass that into q.ExecContext in the mysql.ExecAdhocSQL block
that updates stmt for gotChallenge2 so the challenge is reliably expired
relative to fleet.OneTimeChallengeTTL.
🪄 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: 317b3b6e-2c45-4748-a929-cc48fb5aae87
📒 Files selected for processing (8)
changes/44111-scep-autorenew-failserver/datastore/mysql/apple_mdm.goserver/datastore/mysql/apple_mdm_test.goserver/datastore/mysql/challenges_test.goserver/datastore/mysql/host_certificates.goserver/datastore/mysql/mdm.goserver/fleet/mdm.goserver/service/integration_mdm_test.go
💤 Files with no reviewable changes (4)
- server/datastore/mysql/challenges_test.go
- server/datastore/mysql/host_certificates.go
- changes/44111-scep-autorenew-fail
- server/datastore/mysql/apple_mdm_test.go
Contributor
There was a problem hiding this comment.
Pull request overview
Reverts PR #44250’s SCEP autorenew changes to roll back the managed-cert renewal behavior (and associated tests/changelog), pending a forward-fix that avoids the reported regressions.
Changes:
- Revert
fleet.OneTimeChallengeTTLfrom 7 days back to 1 hour. - Revert MySQL MDM renewal/upsert logic changes (including removing
COALESCEpreservation and renewal retry/backoff/status filtering changes). - Remove the regression/integration tests and changes entry that were added for #44111/#44250.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/integration_mdm_test.go | Reverts test logic to hard-coded 2h challenge backdate. |
| server/fleet/mdm.go | Reverts OneTimeChallengeTTL constant back to 1 hour. |
| server/datastore/mysql/mdm.go | Reverts managed-cert upsert and renewal selection/update behavior. |
| server/datastore/mysql/host_certificates.go | Removes iOS managed-cert “flip verifying→verified” side-effect during cert detail updates. |
| server/datastore/mysql/challenges_test.go | Deletes challenge lifecycle/TTL unit tests introduced in #44250. |
| server/datastore/mysql/apple_mdm_test.go | Removes iOS managed-cert and renewal regression subtests introduced in #44250. |
| server/datastore/mysql/apple_mdm.go | Reverts iOS/iPadOS ack behavior to always short-circuit install verifying→verified. |
| changes/44111-scep-autorenew-fail | Removes the changelog entry added in #44250. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mostlikelee
added a commit
that referenced
this pull request
Apr 30, 2026
Reapplies the three independent improvements from #44250 (reverted via #44535) and adds an ingest-side backfill that catches the actual silent-fail mechanism (missed toInsert matcher) without breaking the natural in-flight synchronization between reconcile and the renewal cron. - Bump OneTimeChallengeTTL 1h → 7d so renewals don't fail with "challenge not found" for offline devices that pick up the InstallProfile push days later. - Restrict the renewal cron to settled delivery states ('verified', 'failed') to avoid re-firing renewal while a previous delivery is still in flight. - Gate the new 'failed' branch on a 24h backoff so permanent render-time failures (CA deleted, missing IDP variables) don't loop hourly. - Add backfillHostMDMManagedCertsFromHostCertsDB: when the toInsert matcher in UpdateHostCertificates misses a renewed cert (replica lag, transaction race, verified-without-actual-renewal), look up a matching cert in host_certificates by the 'fleet-<profile_uuid>' substring and populate hmmc. Gated by a 4h grace on hmmc.updated_at so it doesn't clobber the in-flight blank-out, and a monotonic-forward predicate so it's idempotent. Does NOT reintroduce the COALESCE-preserve in BulkUpsertMDMManagedCertificates or the iOS-only park-at-'verifying' carve-out from #44250 — those broke the natural cron synchronization gate (reconcile NULLs hmmc → cron's HAVING IS NOT NULL excludes the row until ingest repopulates). Resolves #44111
Merged
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #44535 +/- ##
==========================================
- Coverage 66.79% 66.77% -0.03%
==========================================
Files 2637 2637
Lines 212132 212110 -22
Branches 9437 9437
==========================================
- Hits 141688 141629 -59
- Misses 57578 57609 +31
- Partials 12866 12872 +6
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:
|
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.
Summary
Reverts #44250. A forward-fix PR that addresses the original issue (#44111) without the regressions described below will follow.
Related issue: #44111 (still open after this revert)
Summary by CodeRabbit