Repository navigation
ci(docker): verify production image starts and serves health checks (WCN-3794) - #296
mohammadalfaiyazbitgo wants to merge 1 commit into
Conversation
PR CI built the production image but never loaded or ran it, so the image runtime was never exercised. The build now targets linux/amd64 explicitly and loads the image into the runner Docker daemon. A smoke step starts both application modes from that image with TLS disabled, runs AWM and Master Express on a temporary bridge network with a DNS alias, and polls three health checks for 200: POST /ping, POST /advancedwallet/ping, and POST /ping/advancedWalletManager (MBE to AWM over the network). Non-200 or timeout fails the job; the EXIT trap prints container logs on failure and removes containers/network. Ticket: WCN-3794
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
@claude review |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
| local status="none" | ||
| for _ in $(seq 1 30); do | ||
| if status="$(curl -s -o /dev/null -w '%{http_code}' -X POST "${url}")" && [ "${status}" = "200" ]; then | ||
| echo "${url} -> 200" |
There was a problem hiding this comment.
🟡 nit (optional): The smoke test's 30-second limit per health check does not hold. A container that accepts a connection but never answers can stall the PR job for hours. The curl call in wait_for has no --max-time, so one hung request blocks the loop with no limit. Nothing else bounds the step: there is no timeout-minutes, so GitHub's 360-minute job default applies. Fix: pass --max-time (for example --connect-timeout 2 --max-time 5) to the curl in wait_for, and add a timeout-minutes on the step so every check stays bounded.
Why this was flagged
The trigger is a container whose port is open while the app never responds. Two examples are an event loop blocked during startup, and master-express hanging while it waits on an unresponsive AWM (its configured timeout defaults to 305 * 1000 ms in src/initConfig.ts). In wait_for, curl -s -o /dev/null -w '%{http_code}' -X POST "${url}" (pull_request.yaml:177) runs with no --max-time or --connect-timeout, and curl sets no total timeout by default. So the for _ in $(seq 1 30) loop never moves to its next iteration, and the "Timed out waiting" branch and the log dump are never reached. The PR description says each check is "polled up to 30s" and that a timeout fails the job. In fact the job runs until the GitHub default limit (no timeout-minutes is set on the step or job), using up runner minutes and leaving the check unresolved without the logs that cleanup would print. The base branch has no run step, so this hang is new.
Verification: Trigger: a PR's image accepts TCP connections but does not answer a health request. In wait_for, the call is curl -s -o /dev/null -w '%{http_code}' -X POST "${url}" with no --max-time and no --connect-timeout. Nothing else bounds the step: the build-docker-image job and the new step set no timeout-minutes, so GitHub's 360-minute default applies.
|
Closing without merging — this CI-only smoke change is superseded. The actual WCN-3794 scope per the Linear ticket discussion is bumping both Dockerfiles to Node 22.23.4+ with a re-pinned digest (and aligning .nvmrc); a replacement PR will carry that change. |
What
build-docker-imagein.github/workflows/pull_request.yaml) now builds the production image forlinux/amd64explicitly (platforms: linux/amd64) and loads it into the runner Docker daemon (load: true).Smoke test imagestep starts both application modes from that image: AWM (APP_MODE=advanced-wallet-manager, port 13080) and Master Express (APP_MODE=master-express, port 13081) withTLS_MODE=disabled, on a temporary bridge network with a DNS alias so Master Express resolves AWM by name.POST /ping(AWM),POST /advancedwallet/ping(MBE), andPOST /ping/advancedWalletManager(MBE → AWM over the network). Timeout or non-200 fails the job; an EXIT trap prints container logs on failure and always removes both containers and the network.Why
WCN-3794: confirm the Docker image actually runs. Previously the PR pipeline only built the image (no
load, no run), and the release pipeline had no runtime check either. The Dockerfile already pins an AMD64-only Node base digest, so the build target is now explicit instead of implicit.Verification
npm ci/npm run build/ lint /npm test(CI environment) — 439 passingdocker-compose.integ.yml, same AMD64 base digest) — 29 passingadvanced-wallets:wcn-3794with--platform linux/amd64and smoke-ran both modes locally under emulation: all three health endpoints returned 200, including the cross-container MBE → AWM pingTicket: WCN-3794