Skip to content

feat: monitor service readiness and alert Discord - #283

Open
pusheen5000000 wants to merge 3 commits into
stagingfrom
pusheen5000000-poll-readiness-discord-alerts
Open

pusheen5000000 wants to merge 3 commits into
stagingfrom
pusheen5000000-poll-readiness-discord-alerts

Conversation

@pusheen5000000

Copy link
Copy Markdown
Contributor

What this changes

Adds an external scheduled readiness checker that polls configured HTTPS readiness endpoints, debounces consecutive failures, and sends one down notification plus one recovery notification to a dedicated Discord operations webhook. Includes offline tests and the operational runbook requested by #55.

Why

Closes #55. Readiness failures from #54 need an external observer and human notification instead of relying only on Railway's internal health checks.

Zone

.github · scripts · docs

This is deliberately cross-zone because the implementation spans the scheduled GitHub Actions workflow, checker code, and its deployment/runbook documentation.

How to verify

node --test scripts/readiness-checker.test.js
node --check scripts/readiness-checker.js
node --check scripts/readiness-checker.test.js
git diff --check

Results: all 5 readiness tests passed; both JavaScript syntax checks passed; git diff --check passed. The checker entrypoint was also smoke-tested with an empty endpoint configuration and correctly wrote an empty state file without network access.

Checklist

  • Branched off staging and targeting staging (or this is a deliberate staging → main promotion).
  • Ran the service's test suite locally — the full one, including Postgres if the service has a database.
  • uv run ruff check . and uv run ruff format --check . clean — CI gates both on every Python job.
  • Read and followed the service's docs/CONTRIBUTING.md pre-push checklist.
  • Docs updated in this PR where the change makes them wrong — API.md for a new endpoint, the README config table and .env.example for a new setting, ARCHITECTURE.md for a new trade-off, DEPLOYMENT.md for a new variable or scope.

Deployment notes

  • New env var — set repository Actions variable READINESS_ENDPOINTS to a JSON object of externally reachable HTTPS readiness URLs; optionally set READINESS_CHECKER_RUNNER, READINESS_POLL_INTERVAL_SECONDS, and READINESS_FAILURE_THRESHOLD. Set the dedicated DISCORD_READINESS_WEBHOOK Actions secret before enabling the workflow.
  • Deploy order — configure the external endpoints and webhook after [task]: Add API readiness probes and bot health checks #54 readiness probes are deployed and reachable.

Anything you're unsure about

The monitored readiness URLs must be externally reachable or the workflow must use an approved self-hosted runner via READINESS_CHECKER_RUNNER, because Railway services remain private by default.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@github-actions github-actions Bot added size/l < 500 lines changed zone: docs Owned by the docs zone (docs/CODE-OWNERSHIP.md) zone: scripts Owned by the scripts zone (docs/CODE-OWNERSHIP.md) zone: .github Owned by the .github zone (docs/CODE-OWNERSHIP.md) and removed size/l < 500 lines changed labels Oct 17, 2026

@qiuethan qiuethan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two reliability issues reproduced with offline fakes. Please address these before merge.

Comment thread scripts/readiness-checker.js Outdated
Comment thread scripts/readiness-checker.js Outdated
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@github-actions github-actions Bot added the size/xl >= 500 lines changed label Oct 18, 2026
@qiuethan

Copy link
Copy Markdown
Collaborator

Independently rechecked with a Claude reviewer against head cee9ab3be38361155bdb4e40133f0819e2a7d086 (staging merge base 17c7258f7f2896f174649de605409169c07de007). The following findings remain:

  1. [P2] Reconcile pending recovery notifications before retrying on a failed probe. scripts/readiness-checker.js:166–170

    With the default threshold, 503 → 503 → 200 (recovery webhook returns 429) → 503 → 503 sends the pending “Service recovered” message during the fourth, failing probe. The retry path then sets alertedDown=true, so subsequent failures suppress down notifications even though the last successfully delivered message says recovered. Independent offline reproduction confirmed the stale recovery, lastStatus=503, and no subsequent down alert. Discard a superseded recovery or otherwise reconcile both transitions before delivery and update state according to the notification type.

  2. [P3] Bound webhook delivery so a stalled response cannot stop polling and state saving. scripts/readiness-checker.js:118–122

    The webhook POST has no timeout/abort signal. A fake POST that never settles leaves the sequential loop waiting on the first service, never probes the second, and never reaches saveState(). The five-minute job deadline can terminate the workflow before Node's default five-minute response-header timeout. Add a bounded notification timeout and retain the pending notification on timeout. This is distinct from the resolved rejected/429-send finding: those paths now correctly continue polling.

  3. [P3] Make cache keys unique per attempt to avoid replaying stale state on reruns. readiness-monitor.yml:31–32 and line 45

    github.run_id is unchanged on rerun. An exact cache hit takes precedence over the latest-prefix fallback, and existing cache entries cannot be overwritten. This affects re-running a successful run that already saved its cache. Reproduction: run 101 saves the first failure; 102 sends the outage; 103 correctly suppresses duplicates; rerunning 101 restores its old unalerted state, emits a duplicate outage alert, and cannot persist the updated state. An ordinary failed run with no saved cache does not have this exact-key problem. Include github.run_attempt in both restore and save primary keys so each attempt falls back to the latest state and saves a fresh entry. See GitHub's run contexts and cache semantics.

  4. [P3] Document the interval waits in the manual outage test. docs/READINESS-MONITORING.md:48–51

    Dispatching twice and then restoring the URL does not guarantee the promised alerts: shouldPoll() skips dispatches within 300 seconds of the last actual check. Independent CLI reproduction at minutes 0/1/5/6/10 showed the second manual outage check and immediate recovery check were skipped. State explicitly that operators must wait at least the configured interval since the last actual probe before each step, accounting for scheduled runs too.

Verification: the eight committed tests pass; offline checks covered separate-process JSON persistence, ordinary webhook rejection/429 handling, enabling a previously unset webhook, debounce, recovery retry, silencing, and the scenarios above. The two previously resolved findings remain fixed. All findings concern code introduced in this PR. No live webhook or deployment was used; GitHub cache behavior was modeled offline against the documented semantics.

@qiuethan qiuethan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes on cee9ab3be38361155bdb4e40133f0819e2a7d086 based on the independently verified findings.

Please fix the P2 recovery-state bug before merging: after 503 → 503 → 200 (recovery delivery fails) → 503, the checker delivers a stale “Service recovered” message while the endpoint is down, sets alertedDown=true, and suppresses subsequent outage alerts. Reconcile pending recovery notifications with the latest probe before retrying.

The same review also confirms three P3 issues: unbounded webhook delivery can block later probes and state saving; re-running a successful workflow can restore stale state through an immutable cache key; and the manual outage-test instructions omit the required polling-interval waits.

Claude independently reproduced the findings against this unchanged head. The committed tests pass, and the two previously resolved webhook findings remain fixed. Verification used offline fakes; live Actions cache behavior was modeled against GitHub's documented semantics. Detailed locations, reproductions, and suggested corrections are in the linked comment.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@pusheen5000000

Copy link
Copy Markdown
Contributor Author

Fixed and pushed in 882e5c3.

  • Pending recovery notifications are discarded when the latest probe is down, so stale recovery messages cannot change outage state; regression coverage uses the exact 503 → 503 → 200 (failed recovery delivery) → 503 → 503 sequence.
  • Webhook delivery now has a configurable 10-second timeout (READINESS_NOTIFICATION_TIMEOUT_MS); timed-out deliveries remain pending and later services continue to be checked.
  • Readiness cache keys now include github.run_attempt so workflow reruns restore the latest prior state and save a fresh attempt entry.
  • The runbook now requires waiting at least the configured polling interval between manual probes and accounts for scheduled runs.

Validation: node --test scripts/readiness-checker.test.js — 10/10 passed; JavaScript syntax and git diff --check passed.

@qiuethan qiuethan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes on 882e5c3: two alert reliability issues remain in the revised recovery and timeout handling. The inline comments include reproductions and suggested corrections.

Verification: all 10 committed tests and both JavaScript syntax checks pass on Node 20. Both findings were reproduced against this head using offline fakes and a local HTTP server; no live webhook or deployment was used. git diff --check passes.

Comment on lines +180 to +182
if (previous.pendingNotification?.type === 'recovery') {
delete current.pendingNotification;
current.alertedDown = false;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Preserve the delivered outage state when discarding a stale recovery

With the default threshold, 503 → 503 → 200 (recovery webhook fails) → 503 → 200 delivers the initial down notification but never delivers recovery. On the fourth probe this branch deletes the pending recovery and sets alertedDown=false; the fifth and all subsequent healthy probes therefore have no reason to send a recovery. I reproduced this on Node 20, including another healthy check: the only delivered message remained the original outage, with no pending notification left.

Discard the superseded recovery message, but preserve the fact that a down alert was successfully delivered until a recovery is successfully delivered. Add regression coverage for a single failing probe after the failed recovery delivery, followed by sustained success.

Comment on lines +112 to +116
const delivery = sendNotification(alert.content);
const timedOut = new Promise((resolve) => {
timeout = setTimeout(() => resolve(false), timeoutMs);
});
return (await Promise.race([delivery, timedOut])) !== false;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Abort the webhook POST when the delivery timeout expires

Promise.race() stops waiting without cancelling the underlying POST, which has no abort signal. I reproduced this with the actual CLI on Node 20 and a local server that accepts requests but never responds: with a 30 ms notification timeout, both services were checked and the state file was written, yet the process was still alive after one second with both webhook sockets open. The committed test uses an unresolved promise without an active socket, so it misses this behavior.

A prolonged stall can exhaust the workflow's five-minute job deadline before the separate cache-save step runs, losing updated debounce and pending-notification state. Pass an abort signal to the webhook fetch and abort the request on timeout; include a process-level test that verifies the CLI exits after a stalled delivery.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/xl >= 500 lines changed zone: docs Owned by the docs zone (docs/CODE-OWNERSHIP.md) zone: .github Owned by the .github zone (docs/CODE-OWNERSHIP.md) zone: scripts Owned by the scripts zone (docs/CODE-OWNERSHIP.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[task]: Poll service readiness and send Discord outage alerts

2 participants