Skip to content

feat(docker): add container healthcheck - #429

Merged
Ashborne267 merged 1 commit into
StreamGive:mainfrom
TekenujUi90:fix/issue-352
Oct 27, 2026
Merged

Ashborne267 merged 1 commit into
StreamGive:mainfrom
TekenujUi90:fix/issue-352

Conversation

@TekenujUi90

Copy link
Copy Markdown
Contributor

Summary

  • Add a Docker health check against the lightweight /health liveness endpoint.
  • Use a 30-second interval, 3-second timeout, 20-second startup grace period, and three retries.

This is a dedicated assignee-authored PR for #352. The broader open PR #391 also references this issue.

Validation: git diff --check passed. Docker image build was not run locally.

Closes #352.

@TekenujUi90

Copy link
Copy Markdown
Contributor Author

Both failing checks on this PR are unrelated to its change. This PR's diff vs main is exactly:

Dockerfile | 2 ++

(adding a HEALTHCHECK instruction). I checked out the branch (fix/issue-352, head 857797e) in an isolated worktree and confirmed the diff is nothing more than that.

1. "PR safety check" — expected, by design

.github/workflows/pr-safety-check.yml flags any PR touching a Dockerfile or docker-compose*.yml as a blocking "risky change" note (see the RISKY_PATHS table, line 59: changes container build config). This is intentional, unconditional policy for any Dockerfile edit — the workflow isn't evaluating the content, just the fact that the file changed, and it says so directly in its own summary output: "Review these carefully before merging. If they are legitimate, an admin can merge anyway."

There's nothing to fix here. An admin just needs to review the 2-line HEALTHCHECK change and merge past the block.

For what it's worth, the HEALTHCHECK itself checks out:

  • node:22-slim has global fetch (Node 18+), so node -e "fetch(...)" works without extra deps.
  • GET /health exists (src/server.ts), returns 200 {status:"ok"} normally and 503 on DB failure — matches the response.ok ? 0 : 1 exit-code logic in the CMD.
  • --interval=30s --timeout=3s --start-period=20s --retries=3 are all reasonable, standard values.

2. "Lint, typecheck, test" — real, pre-existing bug on main, not a flake

The 10 failures (ReferenceError: sep53Hash is not defined in test/routes/ngoApplications.test.ts) are not caused by this PR and not a parallelism/rate-limit flake. Root cause, found by reading the source directly:

  • test/routes/ngoApplications.test.ts calls sep53Hash(...) at line 41 (inside signedApplicationPayload()), but the file's import list (lines 1–11) never imports sep53Hash — it only imports signAdminRequest from ../helpers/adminAuth.js.
  • test/helpers/adminAuth.ts imports sep53Hash from src/middleware/adminAuth.js for its own internal use (line 3) but does not re-export it.
  • So any test that goes through signedApplicationPayload() throws ReferenceError: sep53Hash is not defined the moment that line executes — deterministically, every time, regardless of test order or concurrency.
  • This isn't caught by npm run typecheck because tsconfig.json explicitly excludes test files ("exclude": ["**/*.test.ts", ...]), so tsc --noEmit never type-checks ngoApplications.test.ts at all. The missing-name error only ever surfaces at runtime, inside vitest.
  • Confirmed via git log -L 41,41:test/routes/ngoApplications.test.ts that this line (and the missing import) was introduced in 1eae5f0 ("feat(applications): verify owner address", 2026-10-20) — well before this PR's branch point, and present unchanged on current origin/main. This PR's branch only adds the Dockerfile lines on top of that same main, so it inherits the bug but didn't cause it.

I could not fully execute the integration suite myself in this review (no local Postgres/Docker available, and I deliberately avoided running prisma generate/npx prisma db push or any other install/lifecycle script beyond npm ci --ignore-scripts per this review's scope), but running just this one file locally fails immediately at module load with Cannot find module './generated/prisma/client.js' once the Prisma client isn't generated — consistent with the file executing exactly as written. The static evidence above (grep of imports + git blame) is conclusive on its own and doesn't depend on reproducing the DB-backed run.

The "Rate limit exceeded, retry in 1 minute" line that appears in the CI log right before the failures is very likely unrelated log noise from @fastify/rate-limit (possibly from an adjacent test or route in the same run) rather than the cause — the sep53Hash failure is a plain missing-import bug, not a shared-state/order issue.

Suggested fix for main (out of scope for this PR, which only touches the Dockerfile): add sep53Hash to the import from ../../src/middleware/adminAuth.js directly in test/routes/ngoApplications.test.ts (it's already imported that way in test/middleware/adminAuth.test.ts), e.g.:

import { sep53Hash } from '../../src/middleware/adminAuth.js';

Recommendation: merge this PR on its own merits (admin override on the safety check), and open/track a separate fix for the sep53Hash import bug on main — re-running CI on this PR after that lands should go green.

@TekenujUi90

Copy link
Copy Markdown
Contributor Author

@Ashborne267 this is ready for a look whenever you have a moment.

@TekenujUi90

Copy link
Copy Markdown
Contributor Author

@Ashborne267 The branch is rebased on current main and GitHub reports no merge conflict. The CI test job fails on the existing sep53Hash is not defined error in test/routes/ngoApplications.test.ts. The PR safety check flags the Dockerfile edit for admin review. Please review the health check change and advise on the required CI/admin handling.

@Ashborne267
Ashborne267 merged commit 00bc421 into StreamGive:main Oct 27, 2026
0 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a Docker HEALTHCHECK

2 participants