P1: Add readiness probe distinct from liveness (#169) - #187
Conversation
dkijania
added
production-readiness
|
Verdict: NOT MERGEABLE (blocking) 🚫 One blocker. Everything else in this PR checks out — the shape is right (distinct endpoints, DB-free liveness, clean body, real test), the problem is that the readiness ping shares the main query pool and has no upper bound, which under load turns "slow" into "every replica NotReady at once". What I checked
Blocker — the readiness ping runs on the shared pool with no timeout, so DB slowness reads as DB down on every replica simultaneouslyWhat breaks. All replicas go Why. Two facts compose:
What is already running is the worst case in the brief: the explorer's analytics query pulls Secondary, same root cause: during a genuine network blackhole to Postgres (dropped packets rather than refused connections), each probe leaves a pending query alive for the full 30s Fix. Two small changes: give pings their own single connection so they can never queue behind analytics traffic, and bound the wait in the plugin so the server answers 503 itself instead of hanging. Splitting it this way also makes the timeout unit-testable without a database.
+/**
+ * Connect deadline for the readiness pinger, in seconds. Kept short so a probe
+ * fails fast rather than hanging for the driver's 30s default.
+ */
+const PING_CONNECT_TIMEOUT_S = 2;
+
export class ArchiveNodeAdapter implements DatabaseAdapter {
private client: postgres.Sql;
+ /**
+ * Dedicated single-connection client for readiness pings. Deliberately kept
+ * off the main pool: postgres.js pipelines onto busy connections, so a ping
+ * issued on `client` can sit behind a 2000-block analytics query and make a
+ * merely-busy database look unreachable.
+ */
+ private pingClient: postgres.Sql;
private eventsService: IEventsService;
@@
this.client = postgres(connectionString);
+ this.pingClient = postgres(connectionString, {
+ max: 1,
+ idle_timeout: 60,
+ connect_timeout: PING_CONNECT_TIMEOUT_S,
+ });
this.eventsService = new EventsService(this.client);
@@
async ping(): Promise<boolean> {
try {
- await this.client`SELECT 1`;
+ await this.pingClient`SELECT 1`;
return true;
- } catch {
+ } catch (error) {
+ // Server-side log only — the probe response body never carries this.
+ console.warn(
+ `[readiness] database ping failed: ${
+ error instanceof Error ? error.message : String(error)
+ }`
+ );
return false;
}
}
@@
async close() {
- return this.client.end();
+ await Promise.all([this.client.end(), this.pingClient.end()]);
}
const READINESS_PATH = '/readiness';
+/**
+ * Upper bound on a single readiness ping. Must stay comfortably under the
+ * kubelet's `timeoutSeconds` so the server answers 503 itself rather than
+ * leaving the probe to time out with no response at all.
+ */
+const DEFAULT_PING_TIMEOUT_MS = 2000;
+
+function pingTimeoutFromEnv(value: string | undefined): number {
+ const parsed = Number(value);
+ return Number.isFinite(parsed) && parsed > 0 ? parsed : DEFAULT_PING_TIMEOUT_MS;
+}
+
+/** Resolves, never rejects: a probe endpoint must not be able to return 500. */
+async function pingWithin(
+ db: Pick<DatabaseAdapter, 'ping'>,
+ timeoutMs: number
+): Promise<boolean> {
+ let timer: NodeJS.Timeout | undefined;
+ const timedOut = new Promise<boolean>((resolve) => {
+ timer = setTimeout(() => {
+ console.warn(`[readiness] database ping exceeded ${timeoutMs}ms`);
+ resolve(false);
+ }, timeoutMs);
+ });
+ const answered = Promise.resolve()
+ .then(() => db.ping())
+ .catch(() => false);
+ try {
+ return await Promise.race([answered, timedOut]);
+ } finally {
+ clearTimeout(timer);
+ }
+}
+
-function useReadiness(db: Pick<DatabaseAdapter, 'ping'>): Plugin {
+function useReadiness(
+ db: Pick<DatabaseAdapter, 'ping'>,
+ timeoutMs: number = pingTimeoutFromEnv(process.env.READINESS_PING_TIMEOUT_MS)
+): Plugin {
return {
async onRequest({ url, endResponse, fetchAPI }) {
if (url.pathname !== READINESS_PATH) return;
- const ready = await db.ping();
+ const ready = await pingWithin(db, timeoutMs);
endResponse(
new fetchAPI.Response(
JSON.stringify({ status: ready ? 'ready' : 'not ready' }),
{
status: ready ? 200 : 503,
- headers: { 'content-type': 'application/json' },
+ headers: {
+ 'content-type': 'application/json',
+ 'cache-control': 'no-store',
+ },
}
)
);
},
};
}
CORS_ORIGIN?: string;
+ READINESS_PING_TIMEOUT_MS?: string;
| `CORS_ORIGIN` | `*` | CORS allowed origin |
+| `READINESS_PING_TIMEOUT_MS` | `2000` | Upper bound on the `/readiness` database ping. Exceeding it returns 503 rather than leaving the probe to hang. Keep it below the orchestrator's probe `timeoutSeconds`. | Use `/healthcheck` as the Kubernetes **liveness** probe and `/readiness` as the **readiness** probe: a node whose database is unreachable reports not-ready (so it stops receiving traffic) while staying live (so it isn't needlessly restarted).
+
+Suggested probe settings — readiness must tolerate a brief blip, because every replica shares one Postgres and would otherwise leave rotation simultaneously:
+
+```yaml
+livenessProbe: # process only; never let this depend on the DB
+ httpGet: { path: /healthcheck, port: 8080 }
+ initialDelaySeconds: 10
+ periodSeconds: 15
+ timeoutSeconds: 5
+ failureThreshold: 3
+readinessProbe:
+ httpGet: { path: /readiness, port: 8080 }
+ initialDelaySeconds: 5
+ periodSeconds: 10
+ timeoutSeconds: 3 # > READINESS_PING_TIMEOUT_MS
+ failureThreshold: 3 # ~30s of continuous failure before leaving rotation
+ successThreshold: 1
+```Regression test — append to test('reports not-ready when the ping exceeds its timeout', async () => {
const yoga = createYoga({
schema,
graphqlEndpoint: '/',
// A ping that never settles — the shape of a SELECT 1 stuck behind a
// long-running analytics query on the same connection.
plugins: [useReadiness({ ping: () => new Promise<boolean>(() => {}) }, 20)],
});
const response = await yoga.fetch(`http://localhost${READINESS_PATH}`);
assert.strictEqual(response.status, 503);
assert.deepStrictEqual(await response.json(), { status: 'not ready' });
});
test('reports not-ready, not 500, when the ping rejects, and leaks nothing', async () => {
const yoga = serverWith(async () => {
throw new Error('connect ECONNREFUSED 10.0.0.5:5432');
});
const response = await yoga.fetch(`http://localhost${READINESS_PATH}`);
assert.strictEqual(response.status, 503);
const body = await response.text();
assert.deepStrictEqual(JSON.parse(body), { status: 'not ready' });
assert.ok(!body.includes('ECONNREFUSED'));
assert.ok(!body.includes('5432'));
});Cost of the extra client is one additional Postgres connection per replica, and Non-blocking nits
Automated second-pass review — focus: downstream compatibility with mina-explorer / mina-explorer-api. |
`/healthcheck` is Yoga's built-in liveness check — it only confirms the process is serving HTTP, not that the database is reachable. An orchestrator therefore keeps routing traffic to an instance whose Postgres is down. Add a `/readiness` endpoint that pings the database (`SELECT 1`) and returns 200 when it answers, 503 otherwise. A lightweight `ping()` is added to the database adapter (resolves false instead of throwing). The readiness plugin is prepended to the plugin list so probes short-circuit before any other request hook (e.g. rate limiting) can interfere. Use `/healthcheck` for the Kubernetes liveness probe and `/readiness` for the readiness probe: a node with an unreachable DB reports not-ready (stops getting traffic) while staying live (not needlessly restarted). Docs updated; unit tests cover 200/503 and confirm normal GraphQL requests pass through untouched. Closes #169. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QSuak9smCHbp4N17xjjLF6
670e185 to
2496d84
Compare
SanabriaRusso
left a comment
SanabriaRusso
left a comment
There was a problem hiding this comment.
Approving — the round-1 blocker is fixed, and fixed structurally rather than papered over.
The thing I most wanted to check was whether this was a real isolation fix or just a Promise.race wrapped around the shared pool. It's the real fix: src/db/archive-node-adapter/archive-node-adapter.ts:56-60 constructs a separate postgres.js instance — postgres(connectionString, { max: 1, idle_timeout: 60, connect_timeout: 2 }) — and ping() at :115 queries this.pingClient, not this.client. That means a 2000-block analytics query saturating this.client cannot delay the ping, and a hung ping cannot burn a shared-pool connection. That self-inflicted-pressure loop was the whole of the round-1 blocker.
Everything else on the unblock list landed:
pingWithin()bound withclearTimeoutinfinally, resolves-never-rejects (src/server/readiness.ts:29-49).READINESS_PING_TIMEOUT_MSinsrc/envionment.d.ts:8, both.env.example.*, parsed atreadiness.ts:22-27with a 2000 ms default and a> 0guard.- Docs:
docs/getting-started.md:181env table, plus a liveness-vs-readiness explanation and a probe-YAML snippet. - Both regression tests:
tests/unit/readiness.test.ts:32(timeout → 503, 20 ms budget against a never-resolving ping) and:50(rejection → 503, asserting the body leaks neitherECONNREFUSEDnor5432). - Liveness untouched:
src/server/server.ts:21stillhealthCheckEndpoint: '/healthcheck', and the plugin early-returns for any path but/readiness. close()now ends both clients (archive-node-adapter.ts:128), so #188 won't leak the ping connection.
Timings check out: with #196's readiness timeoutSeconds: 3 / periodSeconds: 10 / failureThreshold: 6, a 2000 ms ping budget fits with margin and gives 60 s tolerance. Round 1's "30 s tolerance vs 30 s statement_timeout" concern is now moot in both directions — the ping is bounded at 2 s on a dedicated connection, so it is structurally independent of #182's (now 15 s) statement_timeout.
Downstream: no impact on either consumer. /readiness is a new path neither calls. mina-explorer-api's { __typename } probe is explicitly protected by the test at tests/unit/readiness.test.ts:69, which asserts HTTP 200 while the DB reports not-ready. Nice.
Non-blocking notes:
- Single-flight would be cheap hardening. With the DB wedged,
pingWithinreturnsfalseat 2 s but the underlyingSELECT 1stays queued on themax:1client, so each subsequent probe queues another. Behaviour stays correct (every probe times out → NotReady, which is the truth) and it's ~6 queued no-ops/minute, so this is not a production risk — but a small guard makes it strictly bounded:let inFlight: Promise<boolean> | undefined; const answered = inFlight ?? (inFlight = Promise.resolve() .then(() => db.ping()).catch(() => false) .finally(() => { inFlight = undefined; }));
connect_timeout: 2(seconds) exactly equals the defaultREADINESS_PING_TIMEOUT_MSof 2000, so the driver's own connect error never wins the race. Both paths produce 503, so it's cosmetic — butconnect_timeout: 1would let the more informative driver error reach the log.ping()logs viaconsole.warnrather than the structured logger #190 introduces, and fires once per failed probe per replica (6/min during an outage). Worth reconciling when #190 lands.- Doc/manifest drift:
docs/getting-started.mdsuggests readinessfailureThreshold: 3while #196's manifest uses6. Both are fine against a 2 s ping; just pick one so operators aren't handed two numbers. - Merge ordering with #182 (both edit
archive-node-adapter.ts:44): resolve tothis.client = postgres(connectionString, buildPostgresOptions());and leavepingClienton its own literal options — passingbuildPostgresOptions()to the ping client would give itmax: 10and undo the isolation this PR exists to provide. Optionally give the ping clientconnection: { statement_timeout: '2000' }so a wedgedSELECT 1is cancelled server-side too. After both land each replica holdsPG_MAX_CONNECTIONS + 1connections, so thePG_MAX_CONNECTIONSline indocs/getting-started.md:177deserves a one-word tweak.
Worth calling out as a bonus: registering useReadiness ahead of the user plugin list (server.ts:26) structurally exempts /readiness from #185's rate limiter, closing one of #185's round-1 to-dos for free.
2 participants
What & why
Part of the production-readiness epic (#163). Closes #169.
/healthcheckis Yoga's built-in liveness check — it only confirms the process is serving HTTP, not that the database is reachable. An orchestrator therefore keeps routing traffic to an instance whose Postgres is down.Changes
/readinessendpoint (src/server/readiness.ts) that pings the DB and returns 200 when it answers, 503 otherwise.ping()added to the database adapter — runsSELECT 1, resolvesfalseinstead of throwing.Probe usage
/healthcheck/readinessSELECT 1)A node with an unreachable DB reports not-ready (stops getting traffic) while staying live (not needlessly restarted).
Testing
npm run build— cleannpm run test:unit— all pass (200/503 end-to-end through Yoga; confirms normal GraphQL requests are not intercepted)npm run lint— cleannpx prettier --debug-check .— exit 0🤖 Generated with Claude Code