fix: make the CD smoke check wait for a working app - #138
Merged
Conversation
The CD run for #135 failed with `curl: (7)` and exit code 7, naming no endpoint. Production was healthy minutes later. Two separate defects in local-smoke.sh made that outcome both possible and undiagnosable. wait_until_available tested only curl's exit code. curl exits 0 for any HTTP response it manages to receive, 5xx included, so an App Service still finishing its startup and answering 500 read as "ready" and the checks began immediately. The --timeout 180 that cd.yml passes was never reached. Readiness now requires a 200. perform_request retried every 5xx, including the ones a check asserts on purpose. The /Error/500 probe therefore burned three retries and 30 seconds on every single run while logging "App Service might be restarting" for what was the expected result. Callers now declare the status they expect, and only an unexpected 5xx is retried. Which of the two produced the #135 failure cannot be established from the logs. The one thing they record is that the fourth attempt could not connect; the request that preceded it is unidentifiable because the retry message carried no endpoint. That is the third change: perform_request takes a label and prints it. It has to print the final transport failure itself, because returning non-zero aborts the caller's assignment under set -e before the caller's own message can run. Labels are passed explicitly rather than derived from the argument list. The login POST carries the smoke account password in --data-urlencode, and everything this function prints ends up in the CD log. The regression tests replace curl with a stub whose responses are queued per endpoint, and force the retry delay to zero, so the suite finishes in about a second. Each one was checked against the unfixed script first: readiness-waits-through-500, no-retry-on-an-expected-500, and the failure label all fail without this change. The remaining cases guard behavior that was already correct, and one asserts the production default of 10 seconds still stands, since the tests themselves override it. They run from scripts/test.sh, which is the entry point CI already calls, so no workflow change is needed. test.ps1 runs them too, resolving Git Bash by path first: `bash` on PATH is normally System32's WSL launcher, which fails outright when no distro is installed. local-smoke.ps1 needs no equivalent change: Invoke-WebRequest throws on 5xx, so its readiness loop already keeps waiting, and it has no retry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Git's bin\bash.exe, started as a non-login shell, does not add usr\bin to
PATH in every installation. Where that happens the shell tests die on
dirname, mktemp, mkdir and tr before reaching anything they assert, and
Linux CI cannot see it because CI runs test.sh directly.
Both remedies are applied. usr\bin and bin are prepended to the child's
PATH, and bash is invoked with -l so the MSYS profile runs. That is
belt-and-braces on purpose: the broken setup does not reproduce on this
machine, where bin\bash.exe resolves /usr/bin even from a PATH stripped
down to System32, so there was no way to establish which remedy alone is
enough. -l was confirmed to work on an affected machine. Sourcing a
profile is safe here because the test script derives its paths from
BASH_SOURCE and does not care about the working directory.
PATH is restored in a finally block so a filtered or failing dotnet test
run cannot leave the caller's environment modified.
Verified through four entry points: -File, -Command with
-DotnetArgs @('--no-restore'), -Command with a --filter, and the login
shell invoked directly.
The summary line now counts assertion failures rather than tests, since
a single failing test can report more than one and "10 of 7 checks
failed" read as nonsense.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous commit applied two guards at once because the environment where Git Bash loses usr\bin could not be reproduced here, leaving no way to tell which one carried the fix. Measurements on an affected machine have since settled it: -l alone passes PowerShell PATH additions alone fails both passes The additions do not survive into Git Bash's own PATH, so they never helped where help was needed. Keeping code that is known not to work for the case it was written for is worse than not having it, so only -l remains, and the comment now records the measurement rather than the earlier uncertainty. Removing the PATH mutation also removes the need to restore it, so the try/finally goes with it. Re-verified through -File, -Command with --no-restore, -Command with a --filter, and scripts/test.sh; the caller's PATH is confirmed unchanged after a run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
The CD run for #135 failed with
curl: (7)and exit code 7, naming no endpoint. Production was healthy minutes later. Two separate defects inlocal-smoke.shmade that outcome both possible and undiagnosable.Not in AGENTS.md §3's explicit list, but this decides whether a production deploy and a rollback are considered healthy, so the diff is kept small and this is a separate PR for human review.
Defect 1: readiness accepted a 500 as "ready"
wait_until_available:51 tested only curl's exit code:
curl exits 0 for any HTTP response it receives, 5xx included. An App Service still finishing startup and answering 500 therefore read as "ready" and the checks began immediately — the
--timeout 180thatcd.ymlpasses was never reached. Readiness now requires a 200.Defect 2: expected server errors were retried
perform_request:93 retried every 5xx. But line 245 asserts
/Error/500returns 500:So every single run burned three retries and 30 seconds on a check that had already passed, while logging
App Service might be restartingfor what was the expected result. That is what the three 500-retries in the successful run for #133 were — not a struggling app.Callers now declare the status they expect, and only an unexpected 5xx is retried.
Defect 3: failures were anonymous
Which of the two defects produced the #135 failure cannot be established from the logs. All they record is that the fourth attempt could not connect; the request before it is unidentifiable because the retry message carried no endpoint.
perform_requestnow takes a label and prints it. It has to print the final transport failure itself: returning non-zero aborts the caller's assignment underset -ebefore the caller's own message can run — which is exactly why run #135 produced a bareexit code 7.Labels are passed explicitly rather than derived from the argument list. Line 280 puts the smoke account password in
--data-urlencode, and everything this function prints lands in the CD log.Verification that the tests actually catch these
Every case was run against the unfixed script first. Four fail without this change:
/Error/500succeeds without retrying"Did not retry" is asserted by counting requests in the stub's call log, not by matching log wording, so it cannot pass by accident when a message changes.
How the tests run
curl is replaced by tests/scripts/fake-curl.sh, whose responses are queued per endpoint, and
SMOKE_RETRY_DELAY_SECONDSis forced to 0 so no retry ever waits. Because the tests override that delay, one case asserts the production default of 10 seconds is still in place.Measured runtime of the suite:
The remaining time is the readiness cases, which still spend the real
sleep 0.5between polls, plus process-spawn overhead that is much heavier under MSYS. An earlier revision of this description claimed "about a second", which was wrong.They run from
scripts/test.sh, the entry point CI already calls, so no workflow change is needed. Confirmed in the CI log for this PR:Windows entry point
test.ps1runs the same suite. Two Windows-specific problems had to be handled:bashon PATH is normally System32's WSL launcher. Verified here: it fails withexecvpe(/bin/bash) failed: No such file or directory. Git Bash is resolved by path instead.bin\bash.exedoes not add Git'susr\binto PATH in every installation, and the tests then die ondirname/mktemp/tr.For (2) bash is invoked with
-l. Prepending Git'susr\binandbinfrom PowerShell was tried first and measured on an affected machine as not working — the additions do not survive into Git Bash's own PATH — so that approach was removed rather than kept as a redundant second guard.-laloneVerified through four entry points, all reaching the shell tests:
-File scripts/test.ps1-Command "& './scripts/test.ps1' -DotnetArgs @('--no-restore')"-Command "& './scripts/test.ps1' -DotnetArgs @('--filter','FullyQualifiedName~HomeController')"& 'C:\Program Files\Git\bin\bash.exe' -l tests/scripts/local-smoke.tests.shNot changed
local-smoke.ps1has neither defect:Invoke-WebRequestthrows on 5xx, so its readiness loop already keeps waiting, and it has no retry logic at all.Checks run
scripts/test.sh— 240 unit tests, 7 shell testsscripts/test.ps1— same, via Git Bashscripts/format.ps1— OKbash -non all four shell files (shellcheck is not installed here)Side effect
Dropping the pointless
/Error/500retries takes about 30 seconds off every smoke run and removes the misleading "App Service might be restarting" lines from CD logs.🤖 Generated with Claude Code