feat(web): add /health endpoint and post-deploy smoke check - #118
feat(web): add /health endpoint and post-deploy smoke check#118mhunter02 wants to merge 2 commits into
Conversation
Expose a cheap D1 readiness probe for uptime monitors and verify it automatically after each web worker deploy. Closes midt-bg#117. Co-authored-by: Cursor <cursoragent@cursor.com>
Преглед на кодаПолезен и добре оформен PR — readiness probe + smoke check след deploy са стандартна практика за Worker + D1. Разделянето на Какво работи добре
Бележки (без блокери)1. Наблюдаемост. Бих логнала грешката в 2. Цена/повърхност (в контекста на #64). 3. Сценарият „недостъпна база“. Тест планът покрива щастливия случай; пътят 4. Козметично. В РезюмеЧиста, добре тествана функционалност с правилен дизайн (resource route + разделена логика). Бележките са дребни — логване при грешка, потвърждение за rate-limit покритието и проверка на пътя при недостъпна база. Иначе изглежда готов за merge. 🙏 |
Log D1 ping failures for wrangler tail, add HEALTH_RATE_LIMITER (60/60s) per midt-bg#64 pattern, and drop unused SELECT alias. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Благодаря за прегледа, @nedda76 — адресирах бележките в последния commit: 1. Наблюдаемост — 2. Rate limit (#64) — 3. Недостъпна база — resource-route инвариантът е запазен (без default export → root loader не се пълни). Пътят 4. Козметично —
|
|
От моя страна (accuracy/security) — чисто. Три неща над вече казаното, за да не повтарям прегледа на @nedda76: Покритието на rate-limit, което Неда повдигна (#64), е затворено коректно. Новият Smoke-стъпката е истинска gate, не само 200. Едно за координация (не блокер за този PR): Иначе и от мен изглежда готов. 🙏 |
Ревю на PR #118 —
|
| Измерение | Оценка | Бележки |
|---|---|---|
| Сигурност | 1.0/1.0 | Минимално излагане; прилагат се security headers; rate-limited; fail-open |
| Тестове | 2.7/3.0 | Прецизни, добре покрити — но непотвърдени в CI (Бележка 1) |
| Качество на кода | 2.0/2.0 | Вярно копие на search/csv/agg limiter-а; последователно |
| Документация | 2.0/2.0 | docs/deploy.md описва probe, rate limit и Access bypass |
| Производителност | 2.0/2.0 | SELECT 1, no-store (без замърсяване на edge cache), с лимит |
| Архитектура | 9.5/10 | Чисто разделение: чист health.ts + тънък loader + middleware limiter |
Проверка на коректността (проверено, не само прочетено)
- ✅ Limiter-ът отговаря на установения шаблон. Извикването с 5 аргумента
rateLimitRequest(req, limiter, isProd, body, name)е идентично по форма на limiter-ите заsearch/csv/aggregation(параметърътnameуправлява degrade логването). Не е разминаване — коректно е. - ✅ Няма кеширане на остарял health.
/healthзадаваCache-Control: no-store; worker-ът кешира само при наличие наs-maxage=\d, затова health винаги еX-Edge-Cache: BYPASSи никога не сеput-ва в edge cache. - ✅ Базата за сигурност все пак се прилага. Loader-ът връща чист
Response, но той минава презhardenResponse→applySecurityHeaders(baseSecurityHeaders()), така че health JSON-ът получава пълната база от headers, въпреки че сам не ги задава. - ✅ Fail-open поведението е тествано. Липсващ binding и хвърлящ limiter и двата резултират в
null(без блокиране) — потвърдено от тестовете.
Бележки
-
⚠️ CI не се е изпълнявало — да се реши преди merge.gh run listпоказва и двете CI изпълнения катоaction_required(продължителност 0s), аmergeStateStatus: UNSTABLE. CI workflow-ът (ci.yml, който се задейства приpull_request) чака одобрение от maintainer (типично за по-нов контрибутор). Тоестpnpm test/typecheckса само заявени локално — не са потвърдени зелени в CI. Maintainer трябва да одобри и пусне workflow-а и да потвърди, че минава, преди merge. Това е единственото нещо между PR-а и чисто одобрение. -
Smoke след deploy е ship-and-alert (без rollback). Smoke стъпката се изпълнява след
pnpm run deploy; провалена/healthprobe проваля job-а, но worker-ът вече е на живо — същият съзнателен компромис като D1 gate-а в feat(etl): pipeline reconciliation gate (#97) #119. Приемливо, но си струва съзнателно отбелязване, тъй като няма автоматичен rollback. -
Production URL твърдо кодиран в
deploy.yml(base="https://sigma.midt.bg"), докато staging се извежда от${SIGMA_WEB_NAME}. Дребна асиметрия — обмислете workflow променлива за production хоста, за да не е домейнът „магически стринг“ в скрипта на job-а. -
Подредба на smoke стъпката спрямо
Initialize LOG_IP_KEY secret. Smoke стъпката предхожда стъпката за инициализация на LOG_IP_KEY; тъй като нямаcontinue-on-error, провал на smoke прекъсва job-а и прескача тази инициализация. Почти сигурно е наред (тя е идемпотентна и се изпълнява при следващия deploy), но потвърдете, че подредбата е умишлена. -
Дребно:
isHealthRequestприемаHEAD, аbuildHealthResponseвинаги включва JSON тяло. Cloudflare премахва телата приHEAD, така че е безвредно — не е нужно действие.
Съответствие с конвенциите: ✅ чисто
Без TODO/заместители, без мъртъв код, без дублиране (преизползва normalizedPathname/rateLimitRequest), тестовете са смислени с проверки на точни стойности (статус кодове, точен JSON, точни аргументи на limit({key}) — отговаря на тестовата конвенция на репото), последователно именуване с peer limiter-ите, коректно разделение на отговорностите.
Препоръка: всичко в diff-а е издържано; единственото отворено е неизпълненият CI pipeline (Бележка 1) — да се одобри и пусне CI и да се потвърди зелено преди merge.
Both this PR and midt-bg#118 (/health) independently claimed namespace_id 1004. A duplicate silently disables one limiter (no deploy error) and git auto-merges the two bindings without conflict, so the dup would ship unnoticed. Take 1005 here (1004 stays with midt-bg#118's reviewed health limiter) to deconflict regardless of merge order.
|
@lyubomir-bozhinov — освен конфликта по Преместих #80 на |
Преглед на кода — PR #118Чиста, добре ограничена промяна за production readiness и точно покритие на #117. Разделянето на Няколко бележки без блокери, като надграждам прегледите по-горе, а не ги повтарям:
Единственото реално условие е, че CI никога не е изпълнявано (в Заключение: Одобрявам — бележки без блокери; моля потвърдете, че CI е зелено преди merge. |
Преглед на кода — PR #118 (
|
ydimitrof
left a comment
There was a problem hiding this comment.
Общ преглед
PR-ът добавя GET /health endpoint (лек D1 SELECT 1 ping) с посветен rate limiter и smoke check след deploy. Кодът е чист, добре структуриран и с добро покритие от тестове. Разделянето на логиката (buildHealthResponse / pingDb) от route loader-а и от worker-level rate limiting е издържано. Не са открити секрети, зловреден код или проблемни зависимости. URL-ите (sigma.midt.bg, *.cf-midt.workers.dev) са собствени домейни на проекта.
Силни страни
- Тестове:
health.test.tsиhealth-rate-limit.test.tsпокриват успешен път, DB грешка (503), нормализиране на пътя с trailing slash, несвързани пътища и fail-open поведение при липсваща/хвърляща binding — смислени тестове, не тривиални. - Сигурност/устойчивост: rate limiter (60/60s на IP) предпазва D1 от abuse;
Cache-Control: no-storeе коректен за readiness probe; fail-open при липсваща binding е разумен избор за health (наличност > строгост) и е тестван. - Консистентност:
HEALTH_RATE_LIMITERе добавен и вREQUIRED_RATE_LIMITERS(wrangler-render.mjs), и вwrangler.jsonc, и е свързан вapp.tsв същия стил като останалите лимитъри. Документацията вdocs/deploy.mdе обновена.
Забележки (незадължителни)
SIGMA_WEB_NAMEв staging клона на smoke check-а не е деклариран вenv:на стъпката — трябва да се потвърди, че идва от job/workflow-level env, иначе staging URL-ът се чупи (https://.cf-midt.workers.dev).- Поради
curl --fail+set -e, при 503/провал стъпката приключва предиecho "$body", така че тялото на неуспешния отговор не се логва за дебъг. /healthразкрива публично статуса на базата (db: ok|error) — минимално разкриване на информация, но приемливо предвид rate limiting-а.
Няма блокиращи проблеми. Препоръчвам изясняване на т.1 преди merge.
| if [ "${{ needs.detect.outputs.env }}" = "production" ]; then | ||
| base="https://sigma.midt.bg" | ||
| else | ||
| base="https://${SIGMA_WEB_NAME}.cf-midt.workers.dev" |
There was a problem hiding this comment.
SIGMA_WEB_NAME се използва тук, но не е деклариран в env: блока на тази стъпка. Моля потвърдете, че е дефиниран на job- или workflow-ниво; в противен случай за не-production средите base става https://.cf-midt.workers.dev и smoke check-ът винаги ще фейлва. Ако вече е глобален env — игнорирайте.
| curl_opts+=(-H "CF-Access-Client-Secret: ${CF_ACCESS_CLIENT_SECRET}") | ||
| fi | ||
| echo "Probing ${url}" | ||
| body="$(curl "${curl_opts[@]}" "$url")" |
There was a problem hiding this comment.
Тъй като curl е с --fail и стъпката ползва set -e, при провал (напр. 503 от DB грешка) командната субституция връща ненулев код и стъпката спира преди echo "$body" на следващия ред — така тялото на неуспешния отговор не се вижда в лога. Помислете за curl -w '%{http_code}' или отделно печатане на тялото при грешка за по-лесен дебъг. Незадължително.
| isProd: boolean, | ||
| ): Promise<Response | null> { | ||
| if (!isHealthRequest(request)) return null; | ||
|
|
There was a problem hiding this comment.
Fail-open поведението (връщане на null при липсваща/хвърляща binding, наследено от rateLimitRequest) е коректен избор за health probe и е покрито от тест. Само отбелязвам, че така при проблем с rate limiter binding-а /health остава напълно неограничен — приемливо за readiness endpoint.
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Health endpoint-ът е коректен като payload ({ ok, ts, db }, без вътрешна инфо, Cache-Control: no-store). Но rate limiter-ът се заобикаля през .data twin-а — детайл на реда по-долу. Отделно PR-ът е dirty (конфликт с main) и иска rebase.
| function isHealthRequest(request: Request): boolean { | ||
| return ( | ||
| (request.method === 'GET' || request.method === 'HEAD') && | ||
| normalizedPathname(request) === '/health' |
There was a problem hiding this comment.
isHealthRequest match-ва точно === '/health', а normalizedPathname маха само trailing slash + lowercase (rate-limit.ts:6-13) — не маха .data. Затова GET /health.data (валиден RRv7 single-fetch URL за route с loader) дава pathname /health.data ≠ /health → rateLimitHealthRoute връща null → loader-ът вика pingDb (SELECT 1) без лимит. Неограничени D1 ping-ове на публичен, unauthenticated endpoint = Denial-of-Wallet.
Fix:
const p = normalizedPathname(request);
return (request.method === 'GET' || request.method === 'HEAD') &&
(p === '/health' || p === '/health.data');Същият .data bypass важи и за /contracts.csv.data през isCsvRequest's endsWith('.csv') (pre-existing, не е от този PR) — струва си да се хване на едно място за всички path-keyed gate-ове.
|
Уточнение към ревюто ми по-горе — препроверих main: shared fix-ът за Този PR внася Действието е просто rebase върху main. Струва си да добавиш и regression тест, който удря |
* ci: забрани трейлър, който кредитира агент, и запази тези с хора Правилото в AGENTS.md се четеше като пълна забрана на `Co-Authored-By:`, а буквалното му спазване би изтрило заслугата на сътрудниците: при squash GitHub съставя трейлърите от авторите на коммитите в PR-а и те са единственото, което държи външния автор в историята - авторът на самия squash коммит винаги е този, който е отворил PR-а. 22 от 376 коммита на main ги носят, включително тези, които пазят заслугата на StanislavBG, Румен и Йоан. Забраненото е друго: трейлър, който кредитира агент - Claude Code, Codex, Cursor, Copilot. Те са инструменти, не сътрудници. Проверката ги лови на ниво PR и казва как се оправя, без да праща никого да пренаписва чужд форк. Тестовете карат проверката срещу истинска история: човешки съавтор и dependabot не се маркират, а трейлърът на Cursor в PR #118 се маркира. Пътьом: post-create.sh предупреждава при самоличност като `t@e.com` или `...MacBook-Pro.local`. И двете вече са влизали в публичната история точно по този път. * style(scripts): форматиране по prettier * ci: премести проверката в задължителната работа, за да блокира наистина Отделната работа се вижда в списъка, но правилото на main изисква само `check`. Значи червен кръст, който нищо не спира - точно класът „проверка, която изглежда, че пази". Стъпката вече е вътре в `check`, тъй че отказът е реален и не зависи от админска промяна по правилото.
|
Този клон е в конфликт с |
Summary
Adds a production readiness probe and automated post-deploy verification — standard deployment-patterns practice for a Cloudflare Worker + D1 app.
GET /health— JSON{ ok, ts, db }with a cheap D1SELECT 1ping; returns503when DB is unreachable;Cache-Control: no-store/healthafter the web worker deploy (retries, optional Cloudflare Access service-token headers)pingDband response shapingdocs/deploy.mdnote on uptime probing and Access bypass for automationCloses #117
Test plan
pnpm --filter @sigma/web test -- app/lib/health.test.tspnpm typecheckCF_ACCESS_CLIENT_ID/CF_ACCESS_CLIENT_SECRETif Access is enabled)curl https://<staging-worker>/healthreturns{"ok":true,"db":"ok",...}Follow-up backlog (from repo audit)
web-vitalsattribution → GA4 inroot.tsx+ CSPconnect-srceslint-plugin-jsx-a11y+ axe Vitest on key routespnpm buildin CI to catch SSR build regressions before deployMade with Cursor