Worker liveness: boot-failure alerting, boot watchdog, and an external monitor - #19
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds ChangesOperations health and supervision
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/uptime.yml:
- Around line 54-59: Update the readiness curl invocation to include
--retry-connrefused and --retry-max-time 60, while retaining the existing retry
count and delay. Revise the adjacent comment to describe --retry-max-time as the
overall retry window rather than implying --max-time bounds the entire request.
In `@docs/ops.md`:
- Around line 211-214: Update the retry-ceiling description near the [[restart]]
documentation to state that exhausting 10 retries leaves the Machine stopped
rather than permanently stopped. Note that later starts remain possible, with
Fly Proxy able to wake existing web Machines while worker Machines require
manual or API recovery.
In `@fly.toml`:
- Around line 22-40: The web process currently stays running when lazy getConfig
validation fails because the Fly health check only removes it from routing.
Update the web server startup path around node web/server.js to eagerly invoke
getConfig before serving requests and exit on invalid configuration;
alternatively, revise the Fly check configuration and docs/ops.md to explicitly
document routing removal without restart.
In `@src/app/readyz/route.ts`:
- Around line 61-71: Separate the database connectivity and worker liveness
failure domains in the readyz handler: keep getDb and db.execute(sql`select 1`)
under the database catch that sets body.database, then wrap
getWorkerLiveness(db) in its own catch that marks the worker check as failed
without changing body.database from "ok". Preserve the existing overall error
status behavior and worker status handling.
In `@src/worker/index.ts`:
- Around line 139-166: Update alertBootFailure so ZodError failures retain the
existing invalid-or-missing configuration summary, but all non-ZodError failures
send only a fixed, non-sensitive boot-failure summary to Discord instead of
err.message or String(err). Preserve logging the complete original error through
console.error for non-ZodError failures, while keeping the webhook delivery and
existing alert handling unchanged.
In `@tests/ops-webhook.test.ts`:
- Around line 6-44: Add a test in the postOpsWebhookUrl suite where fetchImpl
rejects with a network error, then assert that postOpsWebhookUrl rejects with an
OpsWebhookError. Keep the existing HTTP 500 coverage unchanged and verify the
catch-path wrapping behavior for fetchImpl failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 817111dc-2a5b-4fa2-98d7-a0e8449ecdf6
📒 Files selected for processing (13)
.github/workflows/uptime.ymldocs/ops.mdfly.tomlpackage.jsonsrc/app/healthz/route.tssrc/app/readyz/route.tssrc/lib/ops-webhook.tssrc/services/worker-health.tssrc/worker/index.tstests/health-routes-invalid-config.test.tstests/health-routes.test.tstests/ops-webhook.test.tstests/worker-health.test.ts
💤 Files with no reviewable changes (1)
- package.json
Comment on lines
+22
to
+40
| # Liveness only — /healthz asserts valid config and nothing else. A failure | ||
| # here pulls the machine from the load balancer and, with the restart policy | ||
| # below, restarts it. /readyz is NOT checked here on purpose: it reports | ||
| # database and worker health, neither of which a web restart can fix, and | ||
| # wiring it would turn a Postgres blip into a restart storm across every web | ||
| # machine. /readyz is polled externally instead (.github/workflows/uptime.yml). | ||
| # | ||
| # force_https is on but Fly's checker speaks plain HTTP to internal_port and | ||
| # does NOT follow redirects, so the header below stops Next from 301-ing the | ||
| # check into a failure. | ||
| [[http_service.checks]] | ||
| interval = "30s" | ||
| timeout = "5s" | ||
| grace_period = "10s" | ||
| method = "GET" | ||
| path = "/healthz" | ||
| [http_service.checks.headers] | ||
| X-Forwarded-Proto = "https" | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
: "${FLY_APP:?Set FLY_APP to a staging Fly app}"
: "${WEB_MACHINE_ID:?Set WEB_MACHINE_ID to the web Machine}"
# After making /healthz return 503:
fly checks list --app "$FLY_APP"
fly m status -d "$WEB_MACHINE_ID"Repository: guarzo/authGD
Length of output: 207
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- fly.toml ---'
cat -n fly.toml | sed -n '1,90p'
printf '%s\n' '--- relevant documentation and workflow references ---'
rg -n -C 4 'healthz|readyz|restart|http_service|checks|retries|force_https|uptime' \
fly.toml docs .github package.json 2>/dev/null || true
printf '%s\n' '--- health endpoint and startup validation references ---'
rg -n -C 3 'healthz|readyz|invalid config|configuration validation|process\.exit|exitCode' \
. --glob '!node_modules' --glob '!dist' --glob '!build' 2>/dev/null | head -n 300Repository: guarzo/authGD
Length of output: 50371
🌐 Web query:
site:fly.io/docs/reference/health-checks Fly.io service checks restart machine failed health check http_service.checks
💡 Result:
Fly.io service-level health checks, including those configured under http_service.checks, do not automatically trigger a restart of the machine when they fail [1]. Instead, the behavior of these checks is strictly related to traffic routing [1]. When a machine fails a service-level health check, the Fly.io proxy marks the machine as unhealthy and stops routing traffic to it [1]. The machine continues to run, and the proxy will resume routing traffic to the instance only after its checks begin passing again [1]. If you require a machine to restart upon failure, this functionality is not provided by the built-in service-level health check configuration [1]. As these checks only influence routing availability, they do not manage the lifecycle or state of the machine itself [1].
Citations:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- health route implementation ---'
for f in $(rg -l 'export.*GET|function GET|healthz' src/app --glob '*.ts' --glob '*.tsx' | head -n 20); do
echo "### $f"
cat -n "$f" | sed -n '1,180p'
done
printf '%s\n' '--- web startup implementation ---'
cat -n web/server.js | sed -n '1,220p'
printf '%s\n' '--- configuration implementation and direct startup calls ---'
cat -n src/config.ts | sed -n '1,180p'
rg -n -C 5 'getConfig\(|createServer|listen\(|process\.on|process\.exit|exitCode' web src --glob '*.js' --glob '*.ts' --glob '*.tsx' | head -n 260Repository: guarzo/authGD
Length of output: 12959
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- web-related tracked files ---'
git ls-files | rg '(^|/)(web|server|next|Dockerfile|package\.json)' | head -n 120
printf '%s\n' '--- startup and configuration files ---'
for f in package.json Dockerfile src/config.ts; do
if [ -f "$f" ]; then
echo "### $f"
cat -n "$f" | sed -n '1,220p'
fi
done
printf '%s\n' '--- references to the configured web command ---'
rg -n -C 5 'node web/server\.js|web/server\.js|next start|next-server|standalone|npm run start' \
. --glob '!node_modules' --glob '!dist' --glob '!build' | head -n 220Repository: guarzo/authGD
Length of output: 23224
Make the web process exit on invalid configuration.
[[http_service.checks]] only removes a Machine from Fly proxy routing. It does not stop or restart the Machine. Because getConfig() is lazy, invalid web configuration can leave node web/server.js running while /healthz returns 503, so retries = 10 is never used. Validate configuration before starting the web server, or update fly.toml and docs/ops.md to describe routing removal only.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@fly.toml` around lines 22 - 40, The web process currently stays running when
lazy getConfig validation fails because the Fly health check only removes it
from routing. Update the web server startup path around node web/server.js to
eagerly invoke getConfig before serving requests and exit on invalid
configuration; alternatively, revise the Fly check configuration and docs/ops.md
to explicitly document routing removal without restart.
Comment on lines
+61
to
+71
| try { | ||
| const db = getDb(); | ||
| await db.execute(sql`select 1`); | ||
| body.database = "ok"; | ||
| body.worker = await getWorkerLiveness(db); | ||
| if (body.worker.status !== "ok") body.status = "error"; | ||
| } catch (err) { | ||
| console.error("readyz: database unreachable", err); | ||
| body.status = "error"; | ||
| body.database = "error"; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Separate the worker-liveness check from the database-connectivity check.
If db.execute(sql\select 1`)succeeds butgetWorkerLiveness(db)throws, the single catch block setsbody.database = "error"`, even though the database was reachable and the failure originated in the worker-liveness query. This contradicts the endpoint's own stated goal: "the body says which one" (Line 33). An on-call responder reading this endpoint's output would misdiagnose a worker-liveness failure as a database outage.
Wrap getWorkerLiveness in its own try/catch so failures are attributed to the correct check.
🐛 Proposed fix to separate the failure domains
try {
const db = getDb();
await db.execute(sql`select 1`);
body.database = "ok";
- body.worker = await getWorkerLiveness(db);
- if (body.worker.status !== "ok") body.status = "error";
} catch (err) {
console.error("readyz: database unreachable", err);
body.status = "error";
body.database = "error";
+ return NextResponse.json(body, { status: 503 });
+ }
+
+ try {
+ body.worker = await getWorkerLiveness(getDb());
+ if (body.worker.status !== "ok") body.status = "error";
+ } catch (err) {
+ console.error("readyz: worker liveness check failed", err);
+ body.status = "error";
}
return NextResponse.json(body, { status: body.status === "ok" ? 200 : 503 });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/readyz/route.ts` around lines 61 - 71, Separate the database
connectivity and worker liveness failure domains in the readyz handler: keep
getDb and db.execute(sql`select 1`) under the database catch that sets
body.database, then wrap getWorkerLiveness(db) in its own catch that marks the
worker check as failed without changing body.database from "ok". Preserve the
existing overall error status behavior and worker status handling.
The worker crashlooped on first deploy, exhausted its restarts, and stayed down silently. Nothing surfaced it. This closes the paths that made that silence possible. - worker: alert DISCORD_OPS_WEBHOOK_URL before exiting on boot failure. The dead-letter handler is registered inside main() after boss.start(), so a worker dying at boot could never reach it. Reads process.env directly because invalid config — the thing getConfig() throws on — is the common cause; honours SYNC_MODE=dry-run so laptops never page ops. - worker: 60s boot watchdog. pg-boss start() retries a dead database forever without resolving or rejecting, so the process hung past its banner indefinitely — alive to Fly, never restarted, never alerted. - /healthz: config validity only, wired to fly.toml http_service checks. - /readyz: config + database + worker staleness, deliberately NOT wired to restarts — a web restart cannot fix either, and would cause a restart storm. - worker liveness derived from the newest sync_run row (no migration; runJob already writes one per execution). 90min threshold, tied to the 30min membership schedule. - fly.toml: explicit [[restart]] policy so the 10-retry ceiling is reviewable. - .github/workflows/uptime.yml polls /readyz, labelled a stopgap. - package.json: drop the duplicate "engines" key that landed on main. Both endpoints are unauthenticated and return failing env var NAMES only, never values; full detail goes to stderr.
#14 landed /api/health and /api/health/sync while this branch was in review. They cover the same ground as the /healthz and /readyz added here, with the same 90-minute threshold, and newestSyncRun orders by the serial primary key so it uses the existing (job_type, id desc) index — the ordering here would have seq-scanned. Keeping both would mean two health surfaces to maintain. Removed as duplicates: /healthz, /readyz, src/services/worker-health.ts and their tests; the fly.toml check block (#14 already checks /api/health). Kept, because #14 did not touch the worker or add a monitor: * boot-failure webhook and the 60s pg-boss watchdog (src/worker/index.ts) * postOpsWebhookUrl, the Config-free webhook path it needs * .github/workflows/uptime.yml, repointed at /api/health/sync — #14's own docstring says that endpoint exists for an external monitor, and none existed * explicit [[restart]], so the 10-retry ceiling from the incident is written down rather than inherited docs/ops.md keeps only the worker half; #14's ## Monitoring section already documents the endpoints.
guarzo
changed the title
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
1 participant
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.
Stage 3. Rebased onto
mainafter #14 landed a parallel health implementation — the endpoints originally in this PR have been removed in favour of/api/healthand/api/health/sync. What remains is the half #14 did not cover: the worker.The incident this addresses
The worker crashlooped on first deploy, exhausted its 10 restarts, and stayed down silently. Nothing surfaced it.
What changed
1. Boot-failure alerting (
src/worker/index.ts)The only webhook caller in the worker process was the dead-letter handler, registered after
boss.start()— so a worker dying at boot could never reach it. Now it posts toDISCORD_OPS_WEBHOOK_URLbefore exiting, readingprocess.envdirectly rather thangetConfig(), because invalid config is the common cause. Suppressed underSYNC_MODE=dry-run.Only ZodError detail is forwarded — it names variables, never values. Everything else sends a fixed summary; the full error goes to stderr.
2. A 60-second boot watchdog — a second silent-failure mode found while testing this
Verified by pointing
DATABASE_URLat a dead port: the process ran until killed, silently. The watchdog exits 1 after 60s, which both engages the restart policy and fires the alert above.3.
.github/workflows/uptime.yml— #14's/api/health/syncdocstring says it exists for an external monitor; none existed. This is that monitor, documented as the stopgap it is. Needsgh variable set APP_BASE_URL --body https://authgd.fly.dev.4. Explicit
[[restart]]infly.toml—on-failure/retries=10was already the default, but leaving it implicit is what made the ceiling invisible. Exhausting it leaves the Machine stopped — recoverable, butworkersits behind no proxy, so nothing wakes it;webis woken by Fly Proxy.5.
postOpsWebhookUrl— a Config-free webhook path with no dry-run guard (the caller owns that). Documented as a sharp edge, with tests.Known gap
Neither endpoint calls
getConfig(), so awebmachine with a missing secret answers 200 on both while every page 500s. Documented under The external poll is a stopgap; not closed here.Verification
typecheckclean ·lint0 errors, 4 pre-existingno-img-elementwarnings ·format:checkclean ·actionlintclean · 313 unit tests, 44 files, passed · 8 e2e passed ·next buildclean.Boot behaviour tested against the real entrypoint (it runs
main()on import, so it cannot be unit-tested): invalid config → alert; dry-run → suppressed; no webhook → exit 1, no hang; unreachable webhook → logged, not masked; dead DB → exit 1 at 64s with the watchdog message; healthy worker + real DB → alive at 75s, no spurious alert.