fix: handle plain-text throttle responses in Invoke-AzSpotPlacementScore#29388
fix: handle plain-text throttle responses in Invoke-AzSpotPlacementScore#29388khang-11 wants to merge 1 commit intoAzure:mainfrom
Conversation
When the Compute API returns HTTP 429 with a plain-text body (e.g.
'You are being throttled') instead of JSON, the autorest-generated
client would crash with 'Expected { or [. Was String: You.' because
JsonNode.Parse() was called unconditionally on the response body.
Wrap all FromJson(JsonNode.Parse(...)) call sites in Compute.cs with
try/catch so that unparseable bodies return null. The existing onDefault
handler already falls back to a RestException with the raw response body
as the message when code/message are null, so the user now sees the
actual plain-text error instead of a parse exception stack trace.
Also add playback test cases covering 429 with and without Retry-After
headers to verify no JSON parse exception is thrown.
| Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status. |
|
Fixes: #28765 |
There was a problem hiding this comment.
Pull request overview
This PR addresses a failure mode in the Compute AutoRest client where plain-text HTTP 429 throttling responses cause JsonNode.Parse() to throw, preventing Invoke-AzSpotPlacementScore from surfacing the actual throttling message to users.
Changes:
- Wrap AutoRest model deserialization (
FromJson(JsonNode.Parse(...))) ingenerated/api/Compute.csto avoid throwing on non-JSON response bodies. - Add Pester playback tests for
Invoke-AzSpotPlacementScorecovering throttled 429 responses with and withoutRetry-After. - Extend the SpotPlacementScore recording file with 429 plain-text response entries.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Compute/Compute.Autorest/test/Invoke-AzSpotPlacementScore.Tests.ps1 | Adds playback Pester tests for 429 throttling scenarios to ensure no JSON parse exception leaks. |
| src/Compute/Compute.Autorest/test/Invoke-AzSpotPlacementScore.Recording.json | Adds recorded 429 responses with text/plain bodies (with/without Retry-After). |
| generated/Compute/Compute.Autorest/generated/api/Compute.cs | Makes deserialization tolerant of non-JSON bodies by catching parse failures and returning null. |
| { | ||
| Invoke-AzSpotPlacementScore -Location eastus -DesiredCount $desiredCount -DesiredLocation $desiredLocations -DesiredSize $desiredSizes | ||
| } | Should -Throw | ||
| $Error[0].Exception.Message | Should -Not -Match "Expected '\{' or '\['" | ||
| $Error[0].Exception.Message | Should -Not -BeNullOrEmpty | ||
| } | ||
|
|
||
| It 'ThrottledWithoutRetryAfter' { | ||
| # When the API returns 429 with a plain-text body and no Retry-After header, | ||
| # the cmdlet should surface a clean error message (not a JSON parse exception). | ||
| { | ||
| Invoke-AzSpotPlacementScore -Location eastus -DesiredCount $desiredCount -DesiredLocation $desiredLocations -DesiredSize $desiredSizes | ||
| } | Should -Throw | ||
| $Error[0].Exception.Message | Should -Not -Match "Expected '\{' or '\['" | ||
| $Error[0].Exception.Message | Should -Not -BeNullOrEmpty |
There was a problem hiding this comment.
The throttling tests assert on $Error[0] after Should -Throw. $Error is global state and can contain errors from previous tests, and Pester may not populate it reliably for terminating exceptions. Capture the thrown error explicitly (e.g., Should -Throw -PassThru or a try/catch that stores $_) and assert on that object instead; also consider adding -ErrorAction Stop to the cmdlet invocation so the failure is always terminating for the test.
| { | |
| Invoke-AzSpotPlacementScore -Location eastus -DesiredCount $desiredCount -DesiredLocation $desiredLocations -DesiredSize $desiredSizes | |
| } | Should -Throw | |
| $Error[0].Exception.Message | Should -Not -Match "Expected '\{' or '\['" | |
| $Error[0].Exception.Message | Should -Not -BeNullOrEmpty | |
| } | |
| It 'ThrottledWithoutRetryAfter' { | |
| # When the API returns 429 with a plain-text body and no Retry-After header, | |
| # the cmdlet should surface a clean error message (not a JSON parse exception). | |
| { | |
| Invoke-AzSpotPlacementScore -Location eastus -DesiredCount $desiredCount -DesiredLocation $desiredLocations -DesiredSize $desiredSizes | |
| } | Should -Throw | |
| $Error[0].Exception.Message | Should -Not -Match "Expected '\{' or '\['" | |
| $Error[0].Exception.Message | Should -Not -BeNullOrEmpty | |
| $thrownError = { | |
| Invoke-AzSpotPlacementScore -Location eastus -DesiredCount $desiredCount -DesiredLocation $desiredLocations -DesiredSize $desiredSizes -ErrorAction Stop | |
| } | Should -Throw -PassThru | |
| $thrownError.Exception.Message | Should -Not -Match "Expected '\{' or '\['" | |
| $thrownError.Exception.Message | Should -Not -BeNullOrEmpty | |
| } | |
| It 'ThrottledWithoutRetryAfter' { | |
| # When the API returns 429 with a plain-text body and no Retry-After header, | |
| # the cmdlet should surface a clean error message (not a JSON parse exception). | |
| $thrownError = { | |
| Invoke-AzSpotPlacementScore -Location eastus -DesiredCount $desiredCount -DesiredLocation $desiredLocations -DesiredSize $desiredSizes -ErrorAction Stop | |
| } | Should -Throw -PassThru | |
| $thrownError.Exception.Message | Should -Not -Match "Expected '\{' or '\['" | |
| $thrownError.Exception.Message | Should -Not -BeNullOrEmpty |
| "Response": { | ||
| "StatusCode": 429, | ||
| "Headers": { | ||
| "Retry-After": [ "0" ], | ||
| "x-ms-request-id": [ "throttled-request-id-with-retry-after" ], | ||
| "x-ms-correlation-request-id": [ "throttled-correlation-id-with-retry-after" ], | ||
| "Date": [ "Thu, 24 Jul 2025 08:10:33 GMT" ] | ||
| }, |
There was a problem hiding this comment.
ThrottledWithRetryAfter uses a recording that includes a Retry-After header. The generated Retry429 middleware retries whenever Retry-After is present (even when it is 0), so playback will likely attempt multiple POSTs and expect additional +2, +3, … entries in the recording (or can loop until the mock runs out). Either disable 429 retries for this test run via AZURE_PS_HTTP_MAX_RETRIES_FOR_429 (and potentially AZURE_PS_HTTP_MAX_RETRIES) or extend the recording with the expected retry sequence (e.g., first response with Retry-After, then a terminal response without it).
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
@khang-11 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
|
Summary
Invoke-AzSpotPlacementScorereceives a throttled HTTP 429 response with a plain-text body (e.g.You are being throttled), the autorest-generated client crashes withExpected '{' or '['. Was String: You.becauseJsonNode.Parse()is called unconditionally on the response body regardless of content type.FromJson(JsonNode.Parse(...))call sites inCompute.cswithtry/catchso that unparseable bodies returnnullinstead of throwing. The existingonDefaulthandler already falls back to aRestExceptionusing the raw response body as the message whencode/messageare null, so the user now sees the actual throttle message rather than a parse exception.Invoke-AzSpotPlacementScorecovering 429 with aRetry-Afterheader and 429 without, asserting that no JSON parse exception is surfaced.Test coverage
ThrottledWithRetryAfterRetry-Afterheader → no parse crashThrottledWithoutRetryAfterRetry-Afterheader → no parse crashNotes
_Callmethods inCompute.cs(not just SpotPlacement) since the same parse pattern is repeated for every API in the generated file and any of them could receive a plain-text throttle body.ISendAsync.csretry middleware (Retry429,RetryError) is unchanged.