Fix VPP API retry recursion causing server OOM - #46659
Merged
Merged
Conversation
The Apple VPP App and Book Management client retried transient Apple errors (HTTP 500 + Retry-After, and the rate-limit error number) by calling itself recursively, with the rate-limit branch nesting retry.Do inside retry.Do. When Apple's VPP endpoint flapped during a wave of setup-experience installs, a single attempt fanned out into thousands of in-flight retrying requests (each holding an open response body, connection, cancel-watcher goroutine, OTel span and timer), exhausting goroutines/heap and OOM-killing the server. Replace the recursion with a flat, bounded retry loop (1 initial attempt plus 3 retries), close each response before retrying, honor Apple's Retry-After capped at 30s, and thread context through the VPP calls so the backoff is cancellable.
![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 (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThis PR fixes an out-of-memory crash in the VPP API client's retry logic. The problem was recursive retry handling: when Apple VPP returned error 9646 (rate-limited) or HTTP 500 with 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 |
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes runaway retry recursion in the Apple VPP client that could lead to unbounded retries and server OOM when Apple returns transient failures (notably error 9646 and HTTP 500 with Retry-After). It replaces recursive/self-nesting retries with a single bounded retry loop, ensures response bodies are closed before retrying, caps Retry-After, and threads context.Context through the VPP API so retries/backoff are cancellable.
Changes:
- Convert VPP API entrypoints to accept
context.Contextand build requests withhttp.NewRequestWithContext. - Replace recursive retry behavior in
do()with a bounded attempt loop and context-aware backoff sleep; cap AppleRetry-Afterwaits. - Add/extend unit tests to ensure retry behavior is bounded, non-recursive, and context-cancellable.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/activities.go | Passes request context through to VPP disassociation calls. |
| server/mdm/apple/vpp/api.go | Adds ctx-aware VPP request construction and replaces recursive retry logic with bounded loop + capped backoff. |
| server/mdm/apple/vpp/api_test.go | Updates tests for ctx signatures and adds coverage ensuring retries are bounded/non-recursive and cancelable. |
| ee/server/service/vpp_users.go | Passes ctx through to VPP v1 user registration calls. |
| ee/server/service/software_installers.go | Passes ctx through to VPP assignment/associate/disassociate calls used during installs. |
| changes/46656-vpp-api-derecursion-oom | Changelog entry documenting the OOM fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -192,7 +192,7 @@ func (r *AssociateAssetsRequest) Validate() error { | |||
| // according the the request parameters provided. | |||
Comment on lines
+608
to
+612
| // For HTTP 5xx server error responses, a Retry-After header indicates how | ||
| // long the client must wait before making additional requests. | ||
| // | ||
| // https://developer.apple.com/documentation/devicemanagement/app_and_book_management/handling_error_responses#3742679 | ||
| retryAfter := resp.Header.Get("Retry-After") | ||
| if resp.StatusCode == http.StatusInternalServerError && retryAfter != "" { | ||
| seconds, err := strconv.ParseInt(retryAfter, 10, 0) | ||
| if err != nil { | ||
| return fmt.Errorf("parsing retry-after header: %w", err) | ||
| if ra := resp.Header.Get("Retry-After"); resp.StatusCode == http.StatusInternalServerError && ra != "" { |
Comment on lines
+612
to
616
| if ra := resp.Header.Get("Retry-After"); resp.StatusCode == http.StatusInternalServerError && ra != "" { | ||
| seconds, perr := strconv.ParseInt(ra, 10, 0) | ||
| if perr != nil { | ||
| return true, 0, fmt.Errorf("parsing retry-after header: %w", perr) | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
changes/46656-vpp-api-derecursion-oom (1)
1-1: ⚡ Quick winConsider broader wording to reflect full scope of the fix.
The changelog entry says "during app installs," but the fix applies to all VPP API operations (AssociateAssets, DisassociateAssets, RegisterUser, GetAssignments). The OOM crash could occur during user registration, assignment queries, or app/book disassociation—not just app installs.
Consider rephrasing to "during VPP (App and Book Management) operations" or "when interacting with Apple's VPP API" for more accurate coverage.
📝 Suggested wording
-* Fixed a server out-of-memory crash that could occur when Apple's VPP (App and Book Management) API repeatedly returned transient errors (HTTP 500 with Retry-After, or error 9646) during app installs. +* Fixed a server out-of-memory crash that could occur when Apple's VPP (App and Book Management) API repeatedly returned transient errors (HTTP 500 with Retry-After, or error 9646) during VPP operations.🤖 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 `@changes/46656-vpp-api-derecursion-oom` at line 1, Update the changelog entry to broaden scope: replace "during app installs" with wording that covers all VPP API interactions (e.g., "during VPP (App and Book Management) operations" or "when interacting with Apple's VPP API") and mention the affected operations such as AssociateAssets, DisassociateAssets, RegisterUser, and GetAssignments so readers understand the fix prevents OOM crashes across installs, registrations, assignment queries, and disassociations.
🤖 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.
Nitpick comments:
In `@changes/46656-vpp-api-derecursion-oom`:
- Line 1: Update the changelog entry to broaden scope: replace "during app
installs" with wording that covers all VPP API interactions (e.g., "during VPP
(App and Book Management) operations" or "when interacting with Apple's VPP
API") and mention the affected operations such as AssociateAssets,
DisassociateAssets, RegisterUser, and GetAssignments so readers understand the
fix prevents OOM crashes across installs, registrations, assignment queries, and
disassociations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c2d7cc43-b755-467a-8751-65d381ac9713
📒 Files selected for processing (6)
changes/46656-vpp-api-derecursion-oomee/server/service/software_installers.goee/server/service/vpp_users.goserver/mdm/apple/vpp/api.goserver/mdm/apple/vpp/api_test.goserver/service/activities.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@changes/46656-vpp-api-derecursion-oom`:
- Line 1: Update the changelog line to indicate the fix applies to all VPP API
operations (not just app installs); mention that the out-of-memory crash could
occur when Apple's VPP API repeatedly returned transient errors (HTTP 500 with
Retry-After or error 9646) during operations such as app installs, user
registration and seat releases — reference the call sites in vpp_users.go and
activities.go so maintainers can confirm scope. Keep wording concise and
accurate, e.g. replace "during app installs" with "during VPP API operations
(e.g., app installs, user registration, seat releases)".
In `@ee/server/service/software_installers.go`:
- Around line 1712-1715: The rollback call to vpp.DisassociateAssets currently
uses the request ctx which may be canceled, causing the VPP seat-release to
abort and leak; modify the compensation path to call vpp.DisassociateAssets with
a detached context (e.g., context.Background()) or a short background context
with its own timeout instead of ctx so the release proceeds even if the request
context is canceled; keep the existing logging via svc.logger.ErrorContext and
preserve the "err", host.ID and vppApp.AdamID details when handling errors from
DisassociateAssets.
In `@server/mdm/apple/vpp/api_test.go`:
- Around line 741-757: The test currently only checks total duration <2s but
doesn't prove retries respect the capped backoff; modify the fake server handler
(setupFakeServer closure used by AssociateAssets) to record per-request
timestamps (e.g., append time.Now() to a slice) each time it increments calls,
then after AssociateAssets returns assert calls == vppMaxAttempts and that
successive timestamp differences are at least the capped backoff (maxVPPBackoff)
minus a small jitter and less than the large Retry-After value — this will fail
if do() ignores or skips the capped Retry-After delays.
In `@server/mdm/apple/vpp/api.go`:
- Around line 541-593: The loop in do() currently returns a generic fmt.Errorf
when the local VPP retry budget is exhausted, which prevents callers (like
GetConfig) from recognizing it as a non-retryable VPP error; change the final
return so it returns an ErrorResponse (the same concrete type GetConfig treats
as terminal) with a clear message and identifying code so upstream retry logic
will suppress further retries. Specifically, replace the final fmt.Errorf(...)
with constructing and returning an *ErrorResponse (or the existing VPP error
struct used elsewhere) containing the exhausted-attempts message and an
appropriate status/code (e.g., a distinct code or "retry budget exhausted" text)
so callers can detect and stop retrying.
In `@server/service/activities.go`:
- Around line 244-245: After the cancel transaction succeeds, the call to
vpp.DisassociateAssets should not use the request-scoped ctx because a
canceled/timeout request will abort the seat-release; replace the request
context with a detached context (e.g., context.Background() or a server-wide
non-cancelable context) when calling vpp.DisassociateAssets(tokenDB.Token, req)
so the VPP seat release runs to completion even if the HTTP request was
canceled; keep passing the same tokenDB.Token and req but invoke
vpp.DisassociateAssets with the detached context instead of the request ctx.
🪄 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: 0c9c6d34-5486-4241-b7e2-b20c3eff00cc
📒 Files selected for processing (6)
changes/46656-vpp-api-derecursion-oomee/server/service/software_installers.goee/server/service/vpp_users.goserver/mdm/apple/vpp/api.goserver/mdm/apple/vpp/api_test.goserver/service/activities.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
server/mdm/apple/vpp/api_test.go (1)
785-805:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe new clamp test still doesn't prove the retry loop honors the capped delay.
Lines 800-804 confirm
doVPPAttemptreturnsmaxVPPBackoff, but they don't verifydo()actually waits that long before the next attempt. Since Lines 741-757 only assert the whole call finishes in under 2s, the suite would still pass if retries happened immediately. Please add per-attempt timestamp assertions in the bounded retry test so skipped backoff fails deterministically.🤖 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/mdm/apple/vpp/api_test.go` around lines 785 - 805, The test currently only checks doVPPAttempt returns a clamped duration but doesn't prove the higher-level retry loop (do) actually waits that long; modify the bounded-retry test that exercises do (the test around the retry loop) to record per-attempt timestamps from the fake server handler (e.g., push time.Now() into a slice each request) and after the call assert that the interval between attempts meets the expected backoff (or at least is non-trivially >= the clamped maxVPPBackoff or a small tolerance), so any skipped backoff will deterministically fail; reference the do function and the existing TestDoVPPAttemptClampsRetryAfter/doVPPAttempt behavior to locate where to add the timestamp capture and assertions.
🤖 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.
Duplicate comments:
In `@server/mdm/apple/vpp/api_test.go`:
- Around line 785-805: The test currently only checks doVPPAttempt returns a
clamped duration but doesn't prove the higher-level retry loop (do) actually
waits that long; modify the bounded-retry test that exercises do (the test
around the retry loop) to record per-attempt timestamps from the fake server
handler (e.g., push time.Now() into a slice each request) and after the call
assert that the interval between attempts meets the expected backoff (or at
least is non-trivially >= the clamped maxVPPBackoff or a small tolerance), so
any skipped backoff will deterministically fail; reference the do function and
the existing TestDoVPPAttemptClampsRetryAfter/doVPPAttempt behavior to locate
where to add the timestamp capture and assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e99ce6d-e2a8-4a02-813d-40954072ced3
📒 Files selected for processing (2)
server/mdm/apple/vpp/api.goserver/mdm/apple/vpp/api_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/mdm/apple/vpp/api.go
cdcme
previously approved these changes
Jun 2, 2026
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #46659 +/- ##
==========================================
+ Coverage 66.84% 66.89% +0.04%
==========================================
Files 2809 2810 +1
Lines 223540 224190 +650
Branches 11354 11354
==========================================
+ Hits 149427 149963 +536
- Misses 60568 60603 +35
- Partials 13545 13624 +79
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:
|
cdcme
approved these changes
Jun 2, 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 #46656
server/mdm/apple/vpp.doretried transient Apple errors by calling itself recursively, with the rate-limit branch nestingretry.Doinsideretry.Do.This change replaces the recursion with a single retry loop (respecting the prior 1 initial attempt + 3 retries), closes each response before retrying, honors Apple's
Retry-Aftercapped at 30s so that a multi-minute value can't block a synchronous request, and threadscontextthrough the VPP calls so the backoff is cancellable. The retry timings are otherwise unchanged from before.Following @sgress454 suggestion, I considered routing this through the shared
retry.Dohelper (a single attempt wrapped inretry.Do+ an error filter) but figured out that:owns its own wait schedule and its error filter returns an outcome enum rather than a duration, so it can't honor Apple's per-responseRetry-After` value.retrypackage to receive an extractxparam so that the backoff is context-aware (which IMHO is more blast radius than this incident fix should carry).Checklist for submitter
changes/.Testing
What was verified. The new automated test cannot run against
main(the fix changes the VPP function signatures and adds the retry knobs), so to confirm the actual failure mode I checked outmainand ran a small repro that drives the VPP client against an Apple endpoint that always returns the rate-limit error. Onmain, the call never returns —do()recurses without bound — and the repro times out:vpp-retry-repro.md
On this branch the same scenario returns a bounded error promptly. That behavior is covered by the new
TestDoRetryIsBoundedAndNonRecursive(bounded rate-limit retries,Retry-Afterhonored-but-capped, and context cancellation), and the fullserver/mdm/apple/vpppackage passes.I did not perform an end-to-end QA against a live Apple endpoint.
Summary by CodeRabbit