Skip to content

Navigation Menu

Sign in
Sign up

Fix infinite retry loop in SetResource.ps1 (missing $iterator increment) - #198

Open
madanmishra1223 wants to merge 2 commits into
microsoft:master from
madanmishra1223:fix/setresource-retry-loop
Open

Fix infinite retry loop in SetResource.ps1 (missing $iterator increment) #198
madanmishra1223 wants to merge 2 commits into
microsoft:master from
madanmishra1223:fix/setresource-retry-loop

Conversation

@madanmishra1223

@madanmishra1223 madanmishra1223 commented Aug 17, 2026

Copy link
Copy Markdown

Problem

UpdateLoop in .ci/scripts/SetResource.ps1 is meant to retry a failing Set-AzResource call a bounded number of times:

$success = $false
$iterator = 1
while( ($success -eq $false) -and ($iterator -le $maxIterations))
{
 try
 {
 ...
 $success = $true
 break
 }
 catch
 {
 Write-Host("Failed to write resource update - ")
 Write-Host($_.Exception.Message)
 Start-Sleep -Seconds 5
 }
}
if($success -eq $false)
{
 throw "Failed to update resources"
}

$iterator is initialized to 1 but is never incremented. When Set-AzResource keeps failing, $success stays $false and $iterator stays 1, so the guard ($iterator -le $maxIterations) is always true.

Consequences:

  • The retry budget the caller passes (UpdateLoop -maxIterations 3 -resource $_) is ignored.
  • A resource that can never be tagged (deleted mid-run, insufficient permissions, a provider that rejects the tag) makes the loop spin forever, sleeping 5 seconds and printing the same error each time, until the pipeline job hits its own timeout.
  • The throw "Failed to update resources" below the loop is unreachable on persistent failure, so a genuine failure is reported as a hung/timed-out job instead of a clear error.

Fix

Increment $iterator in the catch block, so the loop honors -maxIterations and a persistent failure exits via the existing throw.

 Write-Host($_.Exception.Message)
+ $iterator++
 Start-Sleep -Seconds 5

One line changed; no behavior change on the success path (that path still breaks on the first successful call).

 

UpdateLoop initializes $iterator to 1 but never increments it, so the
loop guard ($iterator -le $maxIterations) stays true forever whenever
Set-AzResource keeps failing. Instead of giving up after maxIterations
attempts, the function retries indefinitely with a 5 second sleep
between tries, hanging the pipeline task, and the
"Failed to update resources" throw below the loop is unreachable.
Increment $iterator in the catch block so the retry budget passed by
the caller (-maxIterations 3) is honored and a persistent failure
surfaces as an error instead of a hang.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

@ArshVermaGit Arsh Verma (ArshVermaGit) left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix looks correct and the retry counter now advances on failures as expected. Could you add a small test covering persistent Set-AzResource failure and verifying that -maxIterations is respected? That would prevent this infinite-loop regression from coming back. Otherwise the change looks good.

 

Review feedback: cover persistent Set-AzResource failure and assert that
-maxIterations is respected, so the infinite loop cannot come back.
The failing-forever case needs care: a test that simply keeps failing would
hang rather than fail if the counter regressed. So the mock fails well past
the retry budget and then starts succeeding. A loop that never counts its
attempts still terminates, and the test fails on the assertions instead of
hanging the run. Against the pre-fix script the first test fails in ~180ms
with "Expected an exception ... but no exception was thrown".
Az cmdlets are stubbed and Get-AzResource returns nothing, so dot-sourcing
the script only imports its functions and no Azure call is ever made.
Verified with Pester 6.1.0 on PowerShell 7.6.5: 3 passed with the fix,
1 failed without it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copy link
Copy Markdown
Author

Thanks Arsh Verma (@ArshVermaGit) — test added in .ci/scripts/SetResource.Tests.ps1 (Pester).

The tricky part

A test that just fails forever is the wrong shape here: if the counter ever regressed, that test would hang the run rather than fail it — the same failure mode as the bug. So the mock fails well past the retry budget and then starts succeeding. A loop that never counts its attempts still terminates, and the test fails on the assertions instead of hanging:

$script:attempts = 0
Mock Set-AzResource {
 $script:attempts++
 if ($script:attempts -le 20) { throw 'persistent failure' }
}
{ UpdateLoop -maxIterations 3 -resource $script:resource } |
 Should -Throw 'Failed to update resources'
$script:attempts | Should -Be 3

Three cases covered: gives up after maxIterations on persistent failure, stops retrying as soon as a retry succeeds, and no retry when the first attempt succeeds.

No Azure is touched — the Az cmdlets are stubbed so Pester has something to mock, and Get-AzResource returns nothing, so dot-sourcing the script just imports its functions and the top-level tagging pass is a no-op.

Verified both ways

Pester 6.1.0 on PowerShell 7.6.5.

With the fix:

 [+] gives up after maxIterations when Set-AzResource keeps failing 140ms
 [+] stops retrying as soon as Set-AzResource succeeds 16ms
 [+] does not retry when the first attempt succeeds 35ms
Tests Passed: 3, Failed: 0

With the $iterator++ reverted (bug reintroduced):

 [-] gives up after maxIterations when Set-AzResource keeps failing 181ms
 Expected an exception with message like 'Failed to update resources' to be thrown, but no exception was thrown.
Tests Passed: 2, Failed: 1

181ms, not a hang — which is the point of the escape hatch above.

One note

This repo has no PowerShell test runner wired up (no GitHub Actions workflows at all, and the .ci/ YAML files are Azure DevOps templates for the linked tutorial repos rather than checks on this repo). So nothing runs this file automatically yet — it is run with:

Install-Module Pester -Scope CurrentUser -Force
Invoke-Pester .ci/scripts/SetResource.Tests.ps1

Happy to add a small workflow to run it on PRs if you'd like that, though I left it out to keep this PR scoped to the fix you reviewed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ArshVermaGit Arsh Verma (ArshVermaGit) Awaiting requested review from ArshVermaGit

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /