Emit failed VPP/in-house install activity, release reserved license - #46332
Merged
Conversation
…able managed-config variable
cdcme
force-pushed
the
fix-45854-failed-install-activity
branch
from
May 28, 2026 13:55
6766c37 to
0fabaa8
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #46332 +/- ##
==========================================
- Coverage 66.89% 66.88% -0.02%
==========================================
Files 2782 2784 +2
Lines 221734 222108 +374
Branches 11356 11243 -113
==========================================
+ Hits 148334 148554 +220
- Misses 59999 60123 +124
- Partials 13401 13431 +30
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:
|
Member
Author
|
@claude review once |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Member
Author
|
@claude review once |
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
There was a problem hiding this comment.
Pull request overview
This PR improves iOS/iPadOS VPP and in-house app install failure handling by recording Fleet-side pre-flight failures as failed install activities (instead of rejecting/silently failing), and by preventing/reclaiming reserved VPP license seats to avoid leaks. It also updates the UI install-details modals to display actor-specific failure copy and surface the failure reason for these Fleet-side failures.
Changes:
- Record pre-flight managed-config substitution failures as failed install activities (VPP and in-house), without enqueueing an MDM command or creating zombie upcoming activities.
- Add VPP license seat release via
DisassociateAssetson post-association failures and on cancel of a pending VPP install. - Update activity-feed driven install-details modals to render actor-driven failure messaging and show Fleet-side failure reasons.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/integration_mdm_test.go | Extends VPP mock server to capture /assets/disassociate calls for license-release assertions. |
| server/service/integration_apple_vpp_config_test.go | Adds integration coverage for pre-flight VPP failures being recorded (not 400), and for license release on cancel. |
| server/service/activities.go | Releases reserved VPP seats when canceling a VPP install activity. |
| server/mock/datastore_mock.go | Adds mock datastore hooks for recording failed installs and fetching cancel-release info. |
| server/mdm/apple/vpp/api.go | Adds DisassociateAssets support for releasing VPP seat reservations. |
| server/mdm/apple/install_application.go | Introduces a typed unresolvable-variable error to preserve per-variable failure details. |
| server/fleet/vpp.go | Adds VPPInstallReleaseInfo data model for cancel-time seat-release decisions. |
| server/fleet/datastore.go | Extends datastore interface for recording Fleet-side failed installs and cancel-release lookups. |
| server/fleet/activities.go | Adds failure_reason to install activity detail payloads. |
| server/datastore/mysql/vpp.go | Implements Fleet-side failed VPP install recording and cancel-release info query; adjusts summary SQL to include pre-flight failures. |
| server/datastore/mysql/software_installers.go | Adjusts status queries/joins to include Fleet-side pre-flight failures for VPP and in-house installs. |
| server/datastore/mysql/in_house_apps.go | Adjusts past-activity query joins and adds failed in-house install recording helper. |
| frontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsx | Passes actor/envelope fields + failure reason into install-details modals from host activity feed. |
| frontend/pages/DashboardPage/cards/ActivityFeed/ActivityFeed.tsx | Passes actor/envelope fields + failure reason into install-details modals from global activity feed. |
| frontend/interfaces/activity.ts | Adds from_auto_update and failure_reason fields to activity details typing. |
| frontend/components/ActivityItem/ActivityItem.tsx | Includes actor_full_name / fleet_initiated in the details-click payload. |
| frontend/components/ActivityDetails/InstallDetails/VppInstallDetailsModal/VppInstallDetailsModal.tsx | Renders actor-driven failure copy + Details section for Fleet-side pre-flight failures; skips command-result fetch when applicable. |
| frontend/components/ActivityDetails/InstallDetails/VppInstallDetailsModal/VppInstallDetailsModal.tests.tsx | Adds unit tests for actor-driven failure message logic in VPP modal. |
| frontend/components/ActivityDetails/InstallDetails/SoftwareIpaInstallDetailsModal/SoftwareIpaInstallDetailsModal.tsx | Mirrors VPP modal behavior for Fleet-side pre-flight failures for in-house (.ipa) installs. |
| ee/server/service/software_installers.go | Adds pre-flight substitution checks + failed-install recording for VPP and in-house install paths; disassociates on post-association failures. |
| ee/server/service/install_vpp_associate_test.go | Updates unit test setup to account for new pre-flight configuration lookup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR adds pre-flight checks for Apple managed-app-configuration Fleet variables and a typed UnresolvableAppConfigVarError; records failed VPP and in-house installs (sets verification_failed_at and emits activities with failureReason); updates datastore interfaces and SQL joins so failed-install rows are visible without nano_command_results; introduces VPP DisassociateAssets and best-effort seat-release on insertion failure and cancel; extends mocks and integration tests; and threads failureReason and actor metadata through the frontend activity feed and install-details modals. Possibly related issues
Possibly related PRs
🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsx (1)
799-877:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThread IPA pre-flight fields all the way into the modal props.
You correctly store
failureReason,actorFullName,fleetInitiated, andselfServicehere, but they’re dropped before render on the IPA modal path (Line 1799+), so this flow won’t show actor-specific failed-install copy or error details.Proposed fix
{ipaPackageInstallDetails && ( <SoftwareIpaInstallDetailsModal details={{ appName: ipaPackageInstallDetails.appName || "", fleetInstallStatus: (ipaPackageInstallDetails.fleetInstallStatus || "pending_install") as SoftwareInstallUninstallStatus, hostDisplayName: ipaPackageInstallDetails.hostDisplayName || "", commandUuid: ipaPackageInstallDetails.commandUuid || "", + failureReason: ipaPackageInstallDetails.failureReason, + actorFullName: ipaPackageInstallDetails.actorFullName, + fleetInitiated: ipaPackageInstallDetails.fleetInitiated, + selfService: ipaPackageInstallDetails.selfService, }} onCancel={onCancelIpaSoftwareInstallDetailsModal} /> )}🤖 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 `@frontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsx` around lines 799 - 877, The onShowActivityDetails handler stores failureReason, actorFullName, fleetInitiated, and selfService into ipa package state via setIpaPackageInstallDetails, but those fields are not forwarded into the IPA install modal when rendering; update the ipa package state shape (used by setIpaPackageInstallDetails) to include these four fields and then pass them through to the IPA modal component props (the component that consumes ipaPackageInstallDetails in the render path), so the modal receives failureReason, actorFullName, fleetInitiated, and selfService for display.server/datastore/mysql/in_house_apps.go (1)
69-81:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMirror managed app configuration updates to both
.iparows.This insert path now establishes that one
.ipaneeds identical config under both the iOS and iPadOS installer IDs, butSaveInHouseAppUpdatesstill sets/clears configuration only forpayload.InstallerID. After an edit, one platform can keep stale config while the other gets the new value, so pre-flight validation and install behavior will diverge by platform.🤖 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/datastore/mysql/in_house_apps.go` around lines 69 - 81, SaveInHouseAppUpdates currently updates/clears configuration only for payload.InstallerID, which leaves the paired .ipa row (iPadOS or iOS) stale; change SaveInHouseAppUpdates to mirror configuration changes to both installer IDs by calling ds.updateInHouseAppConfigurationTx (or the same transaction-based helper used elsewhere) for both installerID and installerIDIpad (or compute the counterpart ID the same way the insert path does) so that when payload.Configuration is non-empty or being cleared the update is applied to both rows.
🤖 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/in_house_apps.go`:
- Around line 783-818: RecordFailedInHouseAppInstall currently skips the
existence checks used by InsertHostInHouseAppInstall: before inserting,
explicitly verify the host exists (same guard used in
InsertHostInHouseAppInstall) and after tx.ExecContext(insStmt, ...) check the
sql.Result.RowsAffected(); if 0 treat it as not found and return a
notFound("install_command") style error instead of proceeding to build the
activity. Make these checks inside the ds.withRetryTxx transaction, and only
call ds.getPastActivityDataForInHouseAppInstallDB (and return user/act) when the
insert reported RowsAffected()>0.
In `@server/datastore/mysql/vpp.go`:
- Around line 1412-1437: RecordFailedVPPAppInstall currently inserts a failed
VPP install without validating host existence; mirror the guard used in
InsertHostVPPSoftwareInstall by checking the host exists (using the same
lookup/validation logic that InsertHostVPPSoftwareInstall uses) before running
the INSERT, and return the same error/behavior when the host is missing or
stale; locate the host existence check in InsertHostVPPSoftwareInstall and apply
the same pattern around hostID in RecordFailedVPPAppInstall so callers get
consistent failures.
In `@server/service/activities.go`:
- Around line 209-214: You’re hard-coding PricingParam: "STDQ" in the
vpp.DisassociateAssetsRequest which can mismatch the original association and
cause disassociation to fail; update the code that builds the request in
activities.go to use the actual pricing tier from the stored association/asset
metadata (the same field that was saved when the seat was reserved) instead of
"STDQ" — e.g. read the stored pricing param linked to info.AdamID (or the asset
record tied to the reservation) and set Assets: []vpp.Asset{{AdamID:
info.AdamID, PricingParam: storedPricingParam}}, and if no metadata exists, omit
or pass an empty PricingParam so VPP can infer the correct tier.
---
Outside diff comments:
In `@frontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsx`:
- Around line 799-877: The onShowActivityDetails handler stores failureReason,
actorFullName, fleetInitiated, and selfService into ipa package state via
setIpaPackageInstallDetails, but those fields are not forwarded into the IPA
install modal when rendering; update the ipa package state shape (used by
setIpaPackageInstallDetails) to include these four fields and then pass them
through to the IPA modal component props (the component that consumes
ipaPackageInstallDetails in the render path), so the modal receives
failureReason, actorFullName, fleetInitiated, and selfService for display.
In `@server/datastore/mysql/in_house_apps.go`:
- Around line 69-81: SaveInHouseAppUpdates currently updates/clears
configuration only for payload.InstallerID, which leaves the paired .ipa row
(iPadOS or iOS) stale; change SaveInHouseAppUpdates to mirror configuration
changes to both installer IDs by calling ds.updateInHouseAppConfigurationTx (or
the same transaction-based helper used elsewhere) for both installerID and
installerIDIpad (or compute the counterpart ID the same way the insert path
does) so that when payload.Configuration is non-empty or being cleared the
update is applied to both rows.
🪄 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: 10611de3-95ef-42a1-8304-81eb289ec445
📒 Files selected for processing (21)
ee/server/service/install_vpp_associate_test.goee/server/service/software_installers.gofrontend/components/ActivityDetails/InstallDetails/SoftwareIpaInstallDetailsModal/SoftwareIpaInstallDetailsModal.tsxfrontend/components/ActivityDetails/InstallDetails/VppInstallDetailsModal/VppInstallDetailsModal.tests.tsxfrontend/components/ActivityDetails/InstallDetails/VppInstallDetailsModal/VppInstallDetailsModal.tsxfrontend/components/ActivityItem/ActivityItem.tsxfrontend/interfaces/activity.tsfrontend/pages/DashboardPage/cards/ActivityFeed/ActivityFeed.tsxfrontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsxserver/datastore/mysql/in_house_apps.goserver/datastore/mysql/software_installers.goserver/datastore/mysql/vpp.goserver/fleet/activities.goserver/fleet/datastore.goserver/fleet/vpp.goserver/mdm/apple/install_application.goserver/mdm/apple/vpp/api.goserver/mock/datastore_mock.goserver/service/activities.goserver/service/integration_apple_vpp_config_test.goserver/service/integration_mdm_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
server/datastore/mysql/vpp.go (1)
1412-1437:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMirror the host existence guard here.
This failure path can insert an orphaned
host_vpp_software_installsrow whenhostIDis stale or already deleted.InsertHostVPPSoftwareInstallexplicitly checkshosts.idfirst because this table can't enforce the FK, so this path should do the same to keep success and failure installs consistent.🤖 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/datastore/mysql/vpp.go` around lines 1412 - 1437, RecordFailedVPPAppInstall can insert orphaned host_vpp_software_installs rows when hostID is stale; mirror the existence guard used in InsertHostVPPSoftwareInstall by validating the host exists before doing the INSERT. Inside the same transaction passed to ds.withRetryTxx, query the hosts table for the given hostID (using the same SELECT used by InsertHostVPPSoftwareInstall) and return a not-found error if no row exists, then proceed with the INSERT only after the host check succeeds; keep the check within the tx so it remains atomic with the insert.server/service/activities.go (1)
218-223:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t hard-code
STDQfor seat release.If the original association used a different pricing tier, this
DisassociateAssetsrequest won't match the reserved asset and the cancel path can still leak the seat. Please reuse the asset/pricing metadata from the association path, or look up the asset before disassociating.🤖 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/activities.go` around lines 218 - 223, The DisassociateAssetsRequest is hard-coding PricingParam="STDQ" which can fail to match the originally reserved asset and leak seats; update the cancel flow to reuse the exact pricing metadata from the original association (or fetch the asset before disassociating) instead of hard-coding STDQ—locate where DisassociateAssetsRequest is built (vpp.DisassociateAssetsRequest and vpp.Asset with AdamID/PricingParam) and replace the literal with the stored association's PricingParam (or call the lookup used in the association path) so the AdamID+PricingParam pair matches the reservation being released.
🤖 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 1507-1542: The VPP release-info lookup currently uses
ds.reader(ctx) for both the activated and upcoming queries (activatedStmt and
upcomingStmt) which can miss just-created rows on lagging replicas; update the
lookup in the releaseVPPSeat flow to use the writer connection (ds.writer(ctx))
or execute both queries inside the cancel transaction so they read from the
primary, replacing ds.reader(ctx) with ds.writer(ctx) (or use the transaction's
connection) when calling sqlx.GetContext for activatedStmt and upcomingStmt to
avoid replica-lag NotFound behavior.
---
Duplicate comments:
In `@server/datastore/mysql/vpp.go`:
- Around line 1412-1437: RecordFailedVPPAppInstall can insert orphaned
host_vpp_software_installs rows when hostID is stale; mirror the existence guard
used in InsertHostVPPSoftwareInstall by validating the host exists before doing
the INSERT. Inside the same transaction passed to ds.withRetryTxx, query the
hosts table for the given hostID (using the same SELECT used by
InsertHostVPPSoftwareInstall) and return a not-found error if no row exists,
then proceed with the INSERT only after the host check succeeds; keep the check
within the tx so it remains atomic with the insert.
In `@server/service/activities.go`:
- Around line 218-223: The DisassociateAssetsRequest is hard-coding
PricingParam="STDQ" which can fail to match the originally reserved asset and
leak seats; update the cancel flow to reuse the exact pricing metadata from the
original association (or fetch the asset before disassociating) instead of
hard-coding STDQ—locate where DisassociateAssetsRequest is built
(vpp.DisassociateAssetsRequest and vpp.Asset with AdamID/PricingParam) and
replace the literal with the stored association's PricingParam (or call the
lookup used in the association path) so the AdamID+PricingParam pair matches the
reservation being released.
🪄 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: 6c951c54-d1ef-4c41-9f39-99f47c978fff
📒 Files selected for processing (5)
ee/server/service/software_installers.goserver/datastore/mysql/vpp.goserver/fleet/software_installer.goserver/mdm/apple/install_application.goserver/service/activities.go
🚧 Files skipped from review as they are similar to previous changes (2)
- ee/server/service/software_installers.go
- server/mdm/apple/install_application.go
Contributor
|
Actionable comments posted: 0 |
1 similar comment
Contributor
|
Actionable comments posted: 0 |
jkatz01
reviewed
May 28, 2026
jkatz01
approved these changes
May 28, 2026
jkatz01
left a comment
jkatz01
left a comment
Member
There was a problem hiding this comment.
Awesome, thanks for fixing the license leak!
cdcme
added a commit
that referenced
this pull request
May 28, 2026
6 tasks
cdcme
added a commit
that referenced
this pull request
May 29, 2026
This was referenced May 29, 2026
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:
Resolves #45851, #45854
Fixes three things:
Failed installs now show up. An iOS/iPadOS VPP or in-house install whose managed-config references an IdP variable the host can't supply (e.g.
$FLEET_VAR_HOST_END_USER_IDP_DEPARTMENT) used to return HTTP 400 (admin) or fail silently (self-service / auto). It now records a failed activity with the specific unresolvable variable as the reason.No license leak on that path. Fleet checks the variables before calling
AssociateAssets, so no seat is reserved for installs that won't go through.General license-leak fix. Added
vpp.DisassociateAssetsand call it on post-association failures and on cancel of a pending VPP install — so any reserved-but-not-used seat gets released.The check goes in
InstallVPPAppPostValidation, the one place every VPP install path passes through (admin, self-service, policy, auto-update, setup experience, worker). The reason goes in the activity's existing JSON details so we didn't have to add a migration. Known follow-up: the Host > Software tab modal still says "Install details are no longer available" — it opens from inventory, not the activity, so it doesn't see the reason. The activity feed click works.Checklist for submitter
Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
For unreleased bug fixes in a release candidate, one of:
Summary by CodeRabbit
New Features
Bug Fixes
Tests