Repository navigation
ci(backend): deflake Keycloak and Temporal test fixtures - #151
Conversation
Keycloak: testcontainers 4.14's readiness probe falls back to the HTTP port's /health/ready when the management port refuses a connection. That path is a 404 on Keycloak >= 25, and HTTPError is not among the errors its retry decorator catches, so a container that was still booting failed the whole session (run 38454571680: 13 setup errors). Poll only the management port with a bounded retry, and pin the image to 26.7.5 instead of :latest. Temporal: WorkflowEnvironment.start_local downloaded the dev server from temporal.download on every run and intermittently failed with "error decoding response body" (run 39444224018). CI now installs a pinned, checksum-verified CLI from the GitHub release (cached per version) and the fixture uses it via TEMPORAL_CLI_PATH; locally the SDK still downloads as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
NovrusShehaj
left a comment
There was a problem hiding this comment.
No bugs found; this looks safe to merge. Notes below are non-blocking.
Verified
- Keycloak root cause matches the locked testcontainers 4.14.2 source:
_readiness_probefalls back to the HTTP port's/health/readyonConnectionError(a 404 on Keycloak ≥ 25), and the resultingHTTPErrorisn't in@wait_container_is_ready(ConnectionError, ReadTimeout). The same code is in 4.13.3 and 4.14.0. quay.io/keycloak/keycloak:26.7.5exists on quay.TEMPORAL_CLI_SHA256matchestemporal_cli_1.9.1_linux_amd64.tar.gzin the v1.9.1 release'schecksums.txt. temporalio 1.30.0'sstart_localacceptsdev_server_existing_path.- CI: run 39642239080 went through 5 attempts and all passed (1478 passed, 17 skipped). Attempt 1 had no cache: it downloaded the CLI, verified the checksum (
temporal_cli.tar.gz: OK) and saved the cache. Attempts 2–5 restored the CLI from the cache. That covers the last unticked test-plan item.
Suggestions
-
The override drops upstream's log waits (
container.py#L67). Instart-devmode, upstream also waits for the"started in …"and"Created temporary admin user|Added user '"log lines. Skipping them is fine today: the tests only call the realm's token endpoint (tokens.py), never the admin client (get_client()), and 8 green runs show the realm import has finished by the time/health/readypasses. Nothing records the deviation, though, so a docstring sentence would warn whoever first reaches for the admin client. -
The 180 s comment overstates its scope (
container.py#L24). It says the deadline covers a cold runner pulling the image, but the pull happens insuper().start()(docker_client.run) before_readiness_proberuns. The deadline only covers boot plus the realm import. For reference, upstream's default is 120 s (TC_MAX_TRIES=120× 1 s). -
A timeout leaves little to debug (
container.py#L75-L78). TheTimeoutErrorcarries only the last request error, usually "connection refused", when the Keycloak log is what you'd want from a failed CI run. Appending the tail ofself.get_logs()would fix that. Checking whether the container has exited would also make a badrealm.jsonfail at once instead of after 180 s. -
A cached CLI is never re-verified (
ci.yaml#L39). The checksum runs only on a cache miss; a cache hit trusts the binary as is. Cache scoping (PR runs can't write caches thatmainreads) keeps the risk low. Caching the tarball and verifying it on every run would give the full guarantee for about a second. -
CI's CLI version can drift from local (
ci.yaml#L19). CI pins CLI 1.9.1, while local runs get the SDK's"default"dev-server version, which follows the temporalio version. A temporalio upgrade won't move CI unless someone remembers. A comment next toTEMPORAL_CLI_VERSIONto bump it with temporalio would help. Optionally,backend/CLAUDE.mdcould mention thatTEMPORAL_CLI_PATHalso skips the download in local dev.
I'll push follow-ups for 1–3 to this branch.
Follow-up to review on #151: - The readiness probe now stops at once with a RuntimeError when the container has exited, instead of polling until the 180 s deadline. That error and the timeout both carry the last 50 lines of Keycloak's output, so a failed CI run shows why. - Document that the probe skips upstream's "started in" / admin-user log waits, why that is safe for these tests, and what to restore before using the admin client. - Correct the timeout comment: the image pull happens in DockerContainer.start(), before the probe, and is not counted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
NovrusShehaj
left a comment
There was a problem hiding this comment.
Re-review at 2f710f1: items 1–3 from the earlier review are addressed, with no new issues.
- 1: the docstring records the skipped log waits and says to restore the admin-user wait before using
get_client(). - 2: the timeout comment is corrected.
- 3: an exited container now fails at once (1.3 s instead of 180 s against a real container), and both errors include the last 50 lines of Keycloak's output.
backend-ci is green on 2f710f1 (1478 passed, 17 skipped). One trade-off: the loop now calls Docker (reload(), get_logs()) inside the error handler, so a one-off Docker API error ends startup instead of being retried. testcontainers' own log waits check the container on every poll the same way, so this is fine as is.
Still open, both optional: 4 (a cached CLI isn't re-verified) and 5 (CI's CLI version can drift from local). I've updated the description with the new behaviour and the CI results.
jcschaff
left a comment
There was a problem hiding this comment.
looks very good and save - approved.
Summary
backend-cihas been failing intermittently for reasons unrelated to the PRs it runs on. Two separate flakes, each fixed at its root cause. Test-only and CI-only changes: nobiosim_server/code changes, nothing to release or deploy.1. Keycloak testcontainer readiness (e.g. PR #126, run 38454571680)
All 13 errors were
404 Client Error: Not Found for url: http://localhost:<port>/health/ready. In testcontainers 4.14.2,KeycloakContainer._readiness_probe:<management port 9000>/health/ready;ConnectionError), falls back to<http port 8080>/health/ready, which is a 404 on Keycloak ≥ 25;raise_for_status()raisesHTTPError, which is not one of the exceptions@wait_container_is_ready(ConnectionError, ReadTimeout)retries, so startup fails immediately.The outcome depends on whether the management port is up on the first poll, which is why it was intermittent. On top of that, the image was the unpinned
quay.io/keycloak/keycloak:latest.Fix (
backend/tests/fixtures/keycloak/container.py): a smallKeycloakContainersubclass whose probe polls only the management port and retries anyRequestExceptionuntil a 180 s deadline. If the container exits first, the probe fails immediately instead of waiting out the deadline, and both failures include the last 50 lines of Keycloak's output. The image is pinned to26.7.5.2. Temporal dev-server download (
main, run 39444224018)11 errors:
RuntimeError: Failed starting Temporal dev server: error decoding response body.WorkflowEnvironment.start_localdownloads the dev server from temporal.download on every CI run.Fix:
ci.yamlinstalls a pinned Temporal CLI (1.9.1) from the GitHub release. The download usescurl --retry, is verified against the release's SHA-256, and is cached per version withactions/cache. The step then exportsTEMPORAL_CLI_PATH.temporal_fixtures.pypasses it asdev_server_existing_path. When it's unset (local dev), the SDK downloads exactly as before.Test plan
uv run ruff check .: cleanuv run mypy biosim_server tests: clean (193 files)test_temporal.py,test_simulations_run.py,test_verification_completion.py), using a pre-installed CLI (macOS build of the same 1.9.1): 84 passedTEMPORAL_CLI_PATHset: 1478 passed, 17 skipped (same credential-gated skips as CI)backend-cigreen on this PR and stable across re-runs: 5/5 attempts of run 39642239080 on0609708, plus run 39657354708 on2f710f1(1478 passed, 17 skipped each)