Fix various DDM stuck remove/pending issues#43382
Fix various DDM stuck remove/pending issues#43382MagnusHJensen wants to merge 10 commits intomainfrom
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.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes several Apple DDM “stuck remove/pending” scenarios by preventing incorrect status updates, making batch processing deterministic for cleanup, and adding a safety-net cleanup for already-stuck rows.
Changes:
- Add tests covering token-collision status reports, cross-batch duplicate remove/install cleanup, and orphaned pending removes.
- Ensure
MDMAppleBatchSetHostDeclarationStatealways runs an orphaned remove/pending cleanup. - Reorder “changed declarations” query to process removals first; adjust status report logic to skip updating existing remove rows.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| server/datastore/mysql/apple_mdm_ddm_test.go | Adds regression tests for stuck remove/pending rows and batch-order edge cases. |
| server/datastore/mysql/apple_mdm.go | Adds an orphaned-remove cleanup safety net; processes removals first; fixes status report update condition. |
| changes/40322-fix-ddm-pending-issues | Adds user-facing release notes for the DDM stuck remove/pending fixes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/datastore/mysql/apple_mdm_ddm_test.go (1)
393-399: Clarify misleading comment about identifier.The comment states "same content/identifier" but the
Identifierfield differs betweend1("com.example.cleanup") andd2("com.example.cleanup.new"). The token matches becauseraw_jsonis identical (which contains the embedded identifier"com.example.cleanup"), not because the struct'sIdentifierfield is the same.📝 Suggested clarification
- // D2 has different name but same content/identifier — same token + // D2 has different name and different Identifier field, but same raw_json content — same token + // (token is derived from raw_json, not from the struct's Identifier field) d2, err := ds.NewMDMAppleDeclaration(ctx, &fleet.MDMAppleDeclaration{ DeclarationUUID: "decl-new", Name: "New Declaration", Identifier: "com.example.cleanup.new", RawJSON: declJSON, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/apple_mdm_ddm_test.go` around lines 393 - 399, The comment above the NewMDMAppleDeclaration call for variable d2 is misleading: update it to say the token matches because RawJSON (declJSON) is identical and contains the embedded identifier, not because the struct's Identifier field equals d1.Identifier; reference the variables d1 and d2 and mention the Identifier and RawJSON fields so the comment clarifies that identical raw_json (declJSON) drives token equality.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/datastore/mysql/apple_mdm_ddm_test.go`:
- Around line 393-399: The comment above the NewMDMAppleDeclaration call for
variable d2 is misleading: update it to say the token matches because RawJSON
(declJSON) is identical and contains the embedded identifier, not because the
struct's Identifier field equals d1.Identifier; reference the variables d1 and
d2 and mention the Identifier and RawJSON fields so the comment clarifies that
identical raw_json (declJSON) drives token equality.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bcdd423e-9839-4767-909e-fe81a2a4680b
📒 Files selected for processing (3)
changes/40332-fix-ddm-pending-issuesserver/datastore/mysql/apple_mdm.goserver/datastore/mysql/apple_mdm_ddm_test.go
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #43382 +/- ##
==========================================
- Coverage 66.89% 66.89% -0.01%
==========================================
Files 2588 2589 +1
Lines 207563 207680 +117
Branches 9284 9284
==========================================
+ Hits 138840 138918 +78
- Misses 56097 56123 +26
- Partials 12626 12639 +13
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 #40322 (Second part)
Checklist for submitter
If some of the following don't apply, delete the relevant line.
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
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
Summary by CodeRabbit
Bug Fixes
Tests
Documentation