fix: Script reporting error - #8
Conversation
📝 WalkthroughWalkthroughThe load-shed test now uses persistent HTTP requests, captures correlation headers, samples active revision replicas, conditionally sleeps while polling, and reports PASS, FAIL, or INCONCLUSIVE outcomes. An explanatory Kestrel request-size comment was removed without changing configuration. ChangesLoad-shed test execution
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TestScript as loadshed-test.ps1
participant BlobStorage as Blob storage
participant AnalyzeEndpoint as /analyze endpoint
participant AzureCLI as az containerapp revision list
TestScript->>BlobStorage: PUT blob
BlobStorage-->>TestScript: Upload response
TestScript->>AnalyzeEndpoint: POST analyze request
AnalyzeEndpoint-->>TestScript: Success, correlation ID, or HTTP 429
TestScript->>AzureCLI: Read active revision replicas
AzureCLI-->>TestScript: Replica count or read failure
TestScript->>TestScript: Poll jobs and compute verdict
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Azure what-if (rg-cognilens-dev)Show plan |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@scripts/loadshed-test.ps1`:
- Around line 98-105: Dispose the per-iteration PUT request, its
ByteArrayContent, and response in the upload flow around $put and $putResponse,
using try/finally so cleanup occurs on success and failure; ensure response
content is consumed or disposed before releasing the response. Apply the same
disposal pattern to the $analyze/$analyzeResponse POST flow, while preserving
the existing status validation and error behavior.
- Around line 184-208: Update the verdict handling after the $verdict
initialization so PASS remains 0, definite job-loss failures from $stuck remain
1, and scaling-evidence failures from $replicaReadFailures use a distinct
inconclusive code such as 2. Convert the independent outcome checks into a
priority-ordered elseif chain, with the $stuck failure evaluated before the
replica-read inconclusive result, so only one outcome message is printed before
the existing PASS handling.
- Around line 150-158: Wrap the az revision-list invocation in a try/catch
within the replica polling logic so terminating invocation errors, including
executable-not-found, increment replicaReadFailures and emit the existing
warning instead of aborting the loop. Preserve the current success validation
and non-zero LASTEXITCODE fallback, ensuring both failure paths retain
INCONCLUSIVE scaling reporting.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: acef42a7-8571-4ded-b0ce-758990b57abc
📒 Files selected for processing (2)
scripts/loadshed-test.ps1src/CogniLens.Api/Program.cs
💤 Files with no reviewable changes (1)
- src/CogniLens.Api/Program.cs
| $put = New-Object System.Net.Http.HttpRequestMessage([System.Net.Http.HttpMethod]::Put, $create.uploadUrl) | ||
| $put.Headers.Add('x-ms-blob-type', 'BlockBlob') | ||
| $put.Content = New-Object System.Net.Http.ByteArrayContent(, $audioBytes) | ||
| $put.Content.Headers.ContentType = [System.Net.Http.Headers.MediaTypeHeaderValue]::Parse('application/octet-stream') | ||
| $putResponse = $http.SendAsync($put).GetAwaiter().GetResult() | ||
| if (-not $putResponse.IsSuccessStatusCode) { | ||
| throw "Upload for call $($create.callId) failed: $([int]$putResponse.StatusCode) $($putResponse.Content.ReadAsStringAsync().GetAwaiter().GetResult())" | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Dispose HttpRequestMessage/HttpResponseMessage/content objects created inside the loop.
Every iteration of the for loop (up to $Count, default 50, adjustable via -Count) creates a new HttpRequestMessage, ByteArrayContent, and HttpResponseMessage for the blob PUT (Lines 98-105) and another pair for the analyze POST (Lines 118-135), none of which are disposed. Successful responses never even read/dispose Content, which can keep the underlying connection from being released back to the pool until GC finalizes it — undermining the point of switching to a persistent HttpClient for connection reuse, and adding needless memory/socket pressure at higher $Count.
🔧 Proposed fix (wrap each request/response pair in try/finally)
- $put = New-Object System.Net.Http.HttpRequestMessage([System.Net.Http.HttpMethod]::Put, $create.uploadUrl)
- $put.Headers.Add('x-ms-blob-type', 'BlockBlob')
- $put.Content = New-Object System.Net.Http.ByteArrayContent(, $audioBytes)
- $put.Content.Headers.ContentType = [System.Net.Http.Headers.MediaTypeHeaderValue]::Parse('application/octet-stream')
- $putResponse = $http.SendAsync($put).GetAwaiter().GetResult()
- if (-not $putResponse.IsSuccessStatusCode) {
- throw "Upload for call $($create.callId) failed: $([int]$putResponse.StatusCode) $($putResponse.Content.ReadAsStringAsync().GetAwaiter().GetResult())"
- }
+ $put = New-Object System.Net.Http.HttpRequestMessage([System.Net.Http.HttpMethod]::Put, $create.uploadUrl)
+ try {
+ $put.Headers.Add('x-ms-blob-type', 'BlockBlob')
+ $put.Content = New-Object System.Net.Http.ByteArrayContent(, $audioBytes)
+ $put.Content.Headers.ContentType = [System.Net.Http.Headers.MediaTypeHeaderValue]::Parse('application/octet-stream')
+ $putResponse = $http.SendAsync($put).GetAwaiter().GetResult()
+ try {
+ if (-not $putResponse.IsSuccessStatusCode) {
+ throw "Upload for call $($create.callId) failed: $([int]$putResponse.StatusCode) $($putResponse.Content.ReadAsStringAsync().GetAwaiter().GetResult())"
+ }
+ } finally { $putResponse.Dispose() }
+ } finally { $put.Dispose() }Apply the same pattern to $analyze/$analyzeResponse at Lines 118-135.
Also applies to: 118-135
🤖 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 `@scripts/loadshed-test.ps1` around lines 98 - 105, Dispose the per-iteration
PUT request, its ByteArrayContent, and response in the upload flow around $put
and $putResponse, using try/finally so cleanup occurs on success and failure;
ensure response content is consumed or disposed before releasing the response.
Apply the same disposal pattern to the $analyze/$analyzeResponse POST flow,
while preserving the existing status validation and error behavior.
| $replicas = az containerapp revision list --name $WorkerApp --resource-group $ResourceGroup ` | ||
| --query "[?properties.active].properties.replicas | [0]" -o tsv | ||
| if ($LASTEXITCODE -eq 0 -and "$replicas".Trim() -match '^\d+$') { | ||
| if ([int]$replicas -gt $maxReplicas) { $maxReplicas = [int]$replicas } | ||
| } | ||
| else { | ||
| $replicaReadFailures++ | ||
| Write-Warning "Could not read replica count (az exit $LASTEXITCODE). Peak-replica figure will be unreliable." | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)loadshed-test\.ps1$|scripts/' || true
echo "== target excerpt =="
sed -n '1,220p' scripts/loadshed-test.ps1 2>/dev/null || true
echo "== error action / az call usages in target =="
rg -n "ErrorActionPreference|az [^(]|replicaReadFailures|maxReplicas|Write-Warning|trap|Try|Catch" scripts/loadshed-test.ps1 2>/dev/null || trueRepository: Affan2900/cognilens
Length of output: 10648
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== PowerShell availability =="
command -v pwsh || true
command -v powershell || true
if command -v pwsh >/dev/null 2>&1; then
pwsh -NoProfile -Command '$PSVersionTable.PSVersion; Write-Host \"$ErrorActionPreference=$ErrorActionPreference\"; trap {} az containerapp revision list --name bad --resource-group rg --query \"[?properties.active].properties.replicas | [0]\" -o tsv 2>&1; Write-Host \"LASTEXITCODE=$LASTEXITCODE\"; Write-Host \"after\"'
elif command -v powershell >/dev/null 2>&1; then
powershell -NoProfile -Command '$PSVersionTable.PSVersion; Write-Host \"$ErrorActionPreference=$ErrorActionPreference\"; try { az containerapp revision list --name bad --resource-group rg --query "[?properties.active].properties.replicas | [0]" -o tsv 2>&1 } catch { Write-Host \"caught: $_\" }; Write-Host \"LASTEXITCODE=$LASTEXITCODE\"; Write-Host \"after\"'
else
echo "No PowerShell runtime available"
fiRepository: Affan2900/cognilens
Length of output: 592
🌐 Web query:
PowerShell invoke external command az throwing executable not found $ErrorActionPreference Stop LASTEXITCODE documentation
💡 Result:
In PowerShell, an "executable not found" error when calling an external program (like az) is a critical, terminating exception that prevents the command from running [1][2]. This differs from a command that runs but returns a non-zero exit code [3][4]. Handling Executable-Not-Found Errors If PowerShell cannot find the executable (e.g., az), it throws a CommandNotFoundException [2]. Because this occurs before the external program is even invoked, it is a terminating error that obeys the current scope's $ErrorActionPreference [3]. If $ErrorActionPreference is set to 'Stop', the entire script execution will halt immediately upon this error [3]. You can catch this using a try/catch block [1]: try { az --version } catch [System.Management.Automation.CommandNotFoundException] { Write-Error "The Azure CLI executable was not found. Please ensure it is installed and in your PATH." } Distinguishing from Exit Codes External programs do not inherently participate in PowerShell's error handling system [3]. They communicate success or failure via the $LASTEXITCODE automatic variable [3][4]. A non-zero $LASTEXITCODE simply indicates the command ran and failed; it does not trigger a PowerShell exception by default [3][4]. To treat non-zero exit codes as terminating errors that obey $ErrorActionPreference, you can use the $PSNativeCommandUseErrorActionPreference feature (available in PowerShell 7.4+) [4][5]: $PSNativeCommandUseErrorActionPreference = $true $ErrorActionPreference = 'Stop' az account show # Now throws a NativeCommandExitException if exit code is non-zero If you are using an older version of PowerShell or need manual control, the standard approach is to check $LASTEXITCODE immediately after execution [3][4][1]: az account show if ($LASTEXITCODE -ne 0) { throw "Azure CLI command failed with exit code $LASTEXITCODE" } Troubleshooting "Executable Not Found" If you encounter this error frequently, ensure the following: 1. PATH Environment Variable: Verify the directory containing the az executable is included in your system's $env:PATH [6][7]. 2. Execution Policy/Permissions: On some systems (like Linux agents), verify that the file itself is marked as executable [8][9]. 3. Session Environment: If you recently installed the tool, you may need to start a new PowerShell session for the updated PATH environment variable to take effect [6].
Citations:
- 1: https://stackoverflow.com/questions/54443922/erroraction-stop-not-working-in-try-catch
- 2: Invoke-Command: calling a program via a UNC path in a remote session results in the wrong error being reported PowerShell/PowerShell#13226
- 3: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_error_handling?view=powershell-7.6
- 4: https://stackoverflow.com/questions/57468522/powershell-and-process-exit-codes
- 5: Native Command Error Handling PowerShell/PowerShell-RFC#277
- 6: https://learn.microsoft.com/en-us/answers/questions/2194288/what-might-be-the-issue-if-the-error-message-exec
- 7: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_command_precedence?view=powershell-7.6
- 8: https://stackoverflow.com/questions/68332967/error-unable-to-locate-executable-file-powershell-when-running-azure-cli-tas
- 9: Agent unable to find location of
pwshfor PowerShell task - works in Hosted build microsoft/azure-pipelines-agent#1862
Wrap the az call so replica-read failures don’t abort the polling loop.
With $ErrorActionPreference = 'Stop', az invocation failures such as executable-not-found throw before a non-zero $LASTEXITCODE is set, bypassing this fallback and losing the script’s graceful INCONCLUSIVE scaling reporting.
🔧 Proposed fix
- $replicas = az containerapp revision list --name $WorkerApp --resource-group $ResourceGroup `
- --query "[?properties.active].properties.replicas | [0]" -o tsv
- if ($LASTEXITCODE -eq 0 -and "$replicas".Trim() -match '^\d+$') {
- if ([int]$replicas -gt $maxReplicas) { $maxReplicas = [int]$replicas }
- }
- else {
- $replicaReadFailures++
- Write-Warning "Could not read replica count (az exit $LASTEXITCODE). Peak-replica figure will be unreliable."
- }
+ try {
+ $replicas = az containerapp revision list --name $WorkerApp --resource-group $ResourceGroup `
+ --query "[?properties.active].properties.replicas | [0]" -o tsv
+ if ($LASTEXITCODE -eq 0 -and "$replicas".Trim() -match '^\d+$') {
+ if ([int]$replicas -gt $maxReplicas) { $maxReplicas = [int]$replicas }
+ }
+ else {
+ $replicaReadFailures++
+ Write-Warning "Could not read replica count (az exit $LASTEXITCODE). Peak-replica figure will be unreliable."
+ }
+ }
+ catch {
+ $replicaReadFailures++
+ Write-Warning "Could not read replica count ($_). Peak-replica figure will be unreliable."
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $replicas = az containerapp revision list --name $WorkerApp --resource-group $ResourceGroup ` | |
| --query "[?properties.active].properties.replicas | [0]" -o tsv | |
| if ($LASTEXITCODE -eq 0 -and "$replicas".Trim() -match '^\d+$') { | |
| if ([int]$replicas -gt $maxReplicas) { $maxReplicas = [int]$replicas } | |
| } | |
| else { | |
| $replicaReadFailures++ | |
| Write-Warning "Could not read replica count (az exit $LASTEXITCODE). Peak-replica figure will be unreliable." | |
| } | |
| try { | |
| $replicas = az containerapp revision list --name $WorkerApp --resource-group $ResourceGroup ` | |
| --query "[?properties.active].properties.replicas | [0]" -o tsv | |
| if ($LASTEXITCODE -eq 0 -and "$replicas".Trim() -match '^\d+$') { | |
| if ([int]$replicas -gt $maxReplicas) { $maxReplicas = [int]$replicas } | |
| } | |
| else { | |
| $replicaReadFailures++ | |
| Write-Warning "Could not read replica count (az exit $LASTEXITCODE). Peak-replica figure will be unreliable." | |
| } | |
| } | |
| catch { | |
| $replicaReadFailures++ | |
| Write-Warning "Could not read replica count ($_). Peak-replica figure will be unreliable." | |
| } |
🤖 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 `@scripts/loadshed-test.ps1` around lines 150 - 158, Wrap the az revision-list
invocation in a try/catch within the replica polling logic so terminating
invocation errors, including executable-not-found, increment replicaReadFailures
and emit the existing warning instead of aborting the loop. Preserve the current
success validation and non-zero LASTEXITCODE fallback, ensuring both failure
paths retain INCONCLUSIVE scaling reporting.
| $verdict = 0 | ||
|
|
||
| if ($stuck -gt 0) { | ||
| Write-Host "`nFAIL: $stuck job(s) never reached a terminal state — check the poison queue." -ForegroundColor Red | ||
| $jobs | Where-Object { $null -eq $_.Outcome } | ForEach-Object { Write-Host " $($_.CallId)" } | ||
| exit 1 | ||
| $verdict = 1 | ||
| } | ||
| if ($maxReplicas -le 1) { | ||
|
|
||
| if ($replicaReadFailures -gt 0) { | ||
| # Not a pass and not a fail: the scaling claim has no evidence behind it either way. Saying | ||
| # so beats reporting the initial 0 as though it were an observation. | ||
| Write-Host "`nINCONCLUSIVE on scaling: $replicaReadFailures replica read(s) failed, so the peak-replica figure is not trustworthy." -ForegroundColor Yellow | ||
| Write-Host " Check the platform's own record instead: ContainerAppSystemLogs_CL, Reason_s == 'KEDAScaleTargetActivated'." -ForegroundColor Yellow | ||
| $verdict = 1 | ||
| } | ||
| elseif ($maxReplicas -le 1 -and $jobs.Count -gt 1) { | ||
| Write-Host "`nFAIL: worker never scaled past $maxReplicas replica(s) — KEDA rule did not fire." -ForegroundColor Red | ||
| exit 1 | ||
| $verdict = 1 | ||
| } | ||
|
|
||
| if ($verdict -eq 0) { | ||
| Write-Host "`nPASS: nothing lost ($completed completed, $failed failed), peak $maxReplicas worker replica(s)." -ForegroundColor Green | ||
| } | ||
|
|
||
| Write-Host "`nPASS: nothing lost, worker scaled to $maxReplicas replicas." -ForegroundColor Green | ||
| exit $verdict |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
FAIL and INCONCLUSIVE collapse to the same exit code, and their messages can both print together.
The whole point of this PR is fixing script reporting, but $verdict only ever becomes 0 or 1 — a definite job-loss FAIL (Line 189) and a scaling-evidence-unreliable INCONCLUSIVE (Line 197) are indistinguishable to any caller (CI, automation) that gates on exit code, even though the script explicitly wants three distinct outcomes. Additionally, since both blocks execute independently, a run with $stuck -gt 0 and $replicaReadFailures -gt 0 prints both the red FAIL banner and the yellow INCONCLUSIVE banner, which muddies what actually happened.
Consider distinct exit codes (e.g., 0=PASS, 1=FAIL, 2=INCONCLUSIVE) and an elseif chain so only the highest-priority outcome is reported.
🔧 Proposed fix
-$verdict = 0
+$verdict = 0 # 0=PASS 1=FAIL 2=INCONCLUSIVE
if ($stuck -gt 0) {
Write-Host "`nFAIL: $stuck job(s) never reached a terminal state — check the poison queue." -ForegroundColor Red
$jobs | Where-Object { $null -eq $_.Outcome } | ForEach-Object { Write-Host " $($_.CallId)" }
$verdict = 1
}
-if ($replicaReadFailures -gt 0) {
+elseif ($replicaReadFailures -gt 0) {
# Not a pass and not a fail: the scaling claim has no evidence behind it either way. Saying
# so beats reporting the initial 0 as though it were an observation.
Write-Host "`nINCONCLUSIVE on scaling: $replicaReadFailures replica read(s) failed, so the peak-replica figure is not trustworthy." -ForegroundColor Yellow
Write-Host " Check the platform's own record instead: ContainerAppSystemLogs_CL, Reason_s == 'KEDAScaleTargetActivated'." -ForegroundColor Yellow
- $verdict = 1
+ $verdict = 2
}
elseif ($maxReplicas -le 1 -and $jobs.Count -gt 1) {
Write-Host "`nFAIL: worker never scaled past $maxReplicas replica(s) — KEDA rule did not fire." -ForegroundColor Red
$verdict = 1
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $verdict = 0 | |
| if ($stuck -gt 0) { | |
| Write-Host "`nFAIL: $stuck job(s) never reached a terminal state — check the poison queue." -ForegroundColor Red | |
| $jobs | Where-Object { $null -eq $_.Outcome } | ForEach-Object { Write-Host " $($_.CallId)" } | |
| exit 1 | |
| $verdict = 1 | |
| } | |
| if ($maxReplicas -le 1) { | |
| if ($replicaReadFailures -gt 0) { | |
| # Not a pass and not a fail: the scaling claim has no evidence behind it either way. Saying | |
| # so beats reporting the initial 0 as though it were an observation. | |
| Write-Host "`nINCONCLUSIVE on scaling: $replicaReadFailures replica read(s) failed, so the peak-replica figure is not trustworthy." -ForegroundColor Yellow | |
| Write-Host " Check the platform's own record instead: ContainerAppSystemLogs_CL, Reason_s == 'KEDAScaleTargetActivated'." -ForegroundColor Yellow | |
| $verdict = 1 | |
| } | |
| elseif ($maxReplicas -le 1 -and $jobs.Count -gt 1) { | |
| Write-Host "`nFAIL: worker never scaled past $maxReplicas replica(s) — KEDA rule did not fire." -ForegroundColor Red | |
| exit 1 | |
| $verdict = 1 | |
| } | |
| if ($verdict -eq 0) { | |
| Write-Host "`nPASS: nothing lost ($completed completed, $failed failed), peak $maxReplicas worker replica(s)." -ForegroundColor Green | |
| } | |
| Write-Host "`nPASS: nothing lost, worker scaled to $maxReplicas replicas." -ForegroundColor Green | |
| exit $verdict | |
| $verdict = 0 # 0=PASS 1=FAIL 2=INCONCLUSIVE | |
| if ($stuck -gt 0) { | |
| Write-Host "`nFAIL: $stuck job(s) never reached a terminal state — check the poison queue." -ForegroundColor Red | |
| $jobs | Where-Object { $null -eq $_.Outcome } | ForEach-Object { Write-Host " $($_.CallId)" } | |
| $verdict = 1 | |
| } | |
| elseif ($replicaReadFailures -gt 0) { | |
| # Not a pass and not a fail: the scaling claim has no evidence behind it either way. Saying | |
| # so beats reporting the initial 0 as though it were an observation. | |
| Write-Host "`nINCONCLUSIVE on scaling: $replicaReadFailures replica read(s) failed, so the peak-replica figure is not trustworthy." -ForegroundColor Yellow | |
| Write-Host " Check the platform's own record instead: ContainerAppSystemLogs_CL, Reason_s == 'KEDAScaleTargetActivated'." -ForegroundColor Yellow | |
| $verdict = 2 | |
| } | |
| elseif ($maxReplicas -le 1 -and $jobs.Count -gt 1) { | |
| Write-Host "`nFAIL: worker never scaled past $maxReplicas replica(s) — KEDA rule did not fire." -ForegroundColor Red | |
| $verdict = 1 | |
| } | |
| if ($verdict -eq 0) { | |
| Write-Host "`nPASS: nothing lost ($completed completed, $failed failed), peak $maxReplicas worker replica(s)." -ForegroundColor Green | |
| } | |
| exit $verdict |
🤖 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 `@scripts/loadshed-test.ps1` around lines 184 - 208, Update the verdict
handling after the $verdict initialization so PASS remains 0, definite job-loss
failures from $stuck remain 1, and scaling-evidence failures from
$replicaReadFailures use a distinct inconclusive code such as 2. Convert the
independent outcome checks into a priority-ordered elseif chain, with the $stuck
failure evaluated before the replica-read inconclusive result, so only one
outcome message is printed before the existing PASS handling.
Summary by CodeRabbit
Bug Fixes
Chores