feat(web): add /api/health/ready endpoint with dependency health checks#1507
Open
Harsh23Kashyap wants to merge 12 commits into
Open
feat(web): add /api/health/ready endpoint with dependency health checks#1507Harsh23Kashyap wants to merge 12 commits into
Harsh23Kashyap wants to merge 12 commits into
Conversation
The vendored Zoekt webserver gRPC client construction needs node:path, @grpc/grpc-js, @grpc/proto-loader, and the ZOEKT_WEBSERVER_URL env. Pulling those at module load time also drags the @opentelemetry CJS chain into anything that imports the client. Move the construction behind a single loadZoektClient() helper that dynamically imports the heavy deps, caches the resulting client, and lives in its own module so readiness / search / stream-search callers can import it without that boot-time cost and can mock it cleanly in tests.
Standard Kubernetes-style readiness probe. The existing /api/health
liveness endpoint is left untouched.
GET /api/health/ready runs three checks in parallel:
- postgres: prisma.$queryRaw SELECT 1
- redis: getRedisClient().ping() (rejects non-PONG responses)
- zoekt: an empty gRPC List with a 1s wall-time cap
Each check is bounded by a 2s timeout, so the worst-case request
time is bounded even when one dependency hangs. Returns 200 with
{status:ok, checks:{...}} when all three pass, 503 with
{status:degraded, checks:{...}} otherwise. Per-check payload includes
status, latencyMs, and an error message when degraded.
The endpoint is unauthenticated (matches the existing /api/health
policy) and PostHog tracking is disabled.
Six cases: - 200 + status:ok when all three dependencies are reachable - 503 + postgres error when Postgres is unreachable - 503 + redis error when Redis ping fails - 503 + zoekt error when the gRPC call errors - 503 + redis error when Redis returns a non-PONG response - all three checks run in parallel (Promise.all), not serially The zoekt client is mocked via the new @/lib/zoektClient module so the test does not pull in @grpc/grpc-js at test-load time.
New docs/docs/api-reference/health.mdx describes both endpoints, the response shape, the per-dependency checks, and example Docker Compose and Kubernetes probes. Wired into the existing System group in docs/docs.json.
References issue sourcebot-dev#1506.
Open
9 tasks
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an unauthenticated ChangesHealth readiness
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Probe
participant ReadinessRoute
participant Postgres
participant Redis
participant Zoekt
Probe->>ReadinessRoute: GET /api/health/ready
ReadinessRoute->>Postgres: SELECT 1
ReadinessRoute->>Redis: PING
ReadinessRoute->>Zoekt: List RPC
Postgres-->>ReadinessRoute: Check result
Redis-->>ReadinessRoute: Check result
Zoekt-->>ReadinessRoute: Check result
ReadinessRoute-->>Probe: 200 ok or 503 degraded
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/web/src/app/api/(server)/health/ready/route.test.ts (1)
56-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for a hung dependency timeout.
The suite covers immediate failures but not the two-second timeout contract. Mock one dependency to never settle, advance fake timers, and assert its check reports
errorand the route returns503.🤖 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 `@packages/web/src/app/api/`(server)/health/ready/route.test.ts around lines 56 - 151, Add a test alongside the existing dependency failure cases that leaves one dependency promise pending, enables or uses fake timers, advances time past the route’s two-second timeout, then awaits GET and asserts a 503 degraded response with the hung dependency’s check marked error. Restore timer behavior and mocks consistently with the surrounding tests.
🤖 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 `@CHANGELOG.md`:
- Around line 10-11: Update the changelog entry for GET /api/health/ready to
replace the issue `#1506` link with this pull request’s #<id> link using the
/pull/<id> URL format, keeping the PR reference at the end of the line.
In `@packages/web/src/app/api/`(server)/health/ready/route.ts:
- Around line 91-108: Move the loadZoektClient() call into the callback passed
to withTimeout('zoekt', ...) so both lazy client initialization and the
subsequent client.List readiness probe are covered by READINESS_TIMEOUT_MS.
Preserve the existing List request and error propagation behavior.
- Around line 29-50: Update the readiness dependency checks that use withTimeout
so the underlying Postgres raw query, ioredis PING, and Zoekt List operations
have socket/driver-level timeouts or native cancellation configured before
invocation. Ensure each operation terminates when its per-check timeout expires,
rather than merely allowing withTimeout to reject while the work continues in
the background.
---
Nitpick comments:
In `@packages/web/src/app/api/`(server)/health/ready/route.test.ts:
- Around line 56-151: Add a test alongside the existing dependency failure cases
that leaves one dependency promise pending, enables or uses fake timers,
advances time past the route’s two-second timeout, then awaits GET and asserts a
503 degraded response with the hung dependency’s check marked error. Restore
timer behavior and mocks consistently with the surrounding tests.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 660d10e5-c810-4023-a74b-ee4dc3591380
📒 Files selected for processing (6)
CHANGELOG.mddocs/docs.jsondocs/docs/api-reference/health.mdxpackages/web/src/app/api/(server)/health/ready/route.test.tspackages/web/src/app/api/(server)/health/ready/route.tspackages/web/src/lib/zoektClient.ts
The previous implementation cached the first init attempt in zoektClientPromise and never cleared it. A transient startup error (proto load failure, network timeout) would then permanently mark the Zoekt readiness check as failed until the process restarted. This was a pre-existing latent bug in the zoektClient helper, but the readiness route made it visible. The fix: store the resolved client (not the in-flight promise) so a failed build never reaches the cache, and a subsequent call can retry the init. Addresses Bugbot finding cd93d60f on the same file.
Two related issues surfaced from bot review on the readiness route: 1. `checkZoekt` called `loadZoektClient()` outside the `withTimeout` wrapper, so a stalled first-call Zoekt init (vendored proto load, network DNS, etc.) could exceed the documented 2s bound. Move the init call inside the timeout so it shares the same bound as the gRPC call. 2. `withTimeout` raced the check promise against the timeout and discarded the loser. If the loser later rejected, no one was awaiting it, surfacing as an unhandled-promise-rejection warning in the Node process during a hung-dependency outage. Attach a no-op `.catch` to the check promise so late rejections are absorbed; the visible result (the timeout error or the actual check error) is unchanged. Addresses Bugbot findings 597dbe01 and 367ae57f, and CodeRabbit finding 'Cancel timed-out readiness dependency operations' on the same file.
New test attaches a process-level 'unhandledRejection' listener and asserts that the readiness probe does not surface the check promise's rejection to it, even when the race has already returned the timeout error. Locks in the no-op `.catch` in `withTimeout`.
…ebot-dev#1506 Coding guidelines: changelog entries must reference the PR id, not the issue. The previous link pointed at the issue (which is what was known at the time the entry was written). This aligns the entry with the convention.
…e is a SearchOptions field) Address Cursor Bugbot finding on the readiness probe (PR sourcebot-dev#1507 review sourcebot-dev#3): the gRPC `List` call was being issued with `{ opts: { max_wall_time: ... } }`, but `max_wall_time` is a `SearchOptions` field, not a `ListOptions` field. The Zoekt server silently dropped it, so the intended 1-second server-side timeout never applied and the call was only bounded by the 2-second client-side timeout in `READINESS_TIMEOUT_MS`. On a hung Zoekt instance the request would still return inside 2s, but the in-flight RPC could keep the Zoekt worker busy for longer than necessary. Drop the bogus `opts`, document why, and add a regression test that locks in `List` being called with an empty options object. The probe now issues the smallest valid request: `client.List({}, cb)`.
7 tasks
Cached the in-flight buildClient promise instead of the resolved client. Two overlapping readiness requests on a cold process now share one gRPC/proto init rather than each starting a full init; the catch clears the cached promise on failure so the next call retries.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/web/src/lib/zoektClient.ts`:
- Line 13: Update the cached initialization flow around clientPromise and
buildClient so a hung dynamic import or proto load cannot remain pending
indefinitely. Add loader-level timeout or cancellation handling aligned with the
2-second readiness deadline, and clear clientPromise when initialization exceeds
that deadline so later probes can retry instead of reusing the stalled promise.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 84e7a669-f4bd-4588-87e5-80f8de3e6b4d
📒 Files selected for processing (1)
packages/web/src/lib/zoektClient.ts
Two follow-ups to the in-flight build fix: 1. protoLoader.loadSync ran on the main thread, blocking the per-check withTimeout timers and the parallel Postgres/Redis work during a cold Zoekt init. Switched to protoLoader.load (async) so the event loop stays responsive while descriptors are compiled. 2. A hung dynamic import or proto load would leave clientPromise pending forever, poisoning every subsequent readiness probe. Added a 1500ms build ceiling (well under the 2s route deadline) via Promise.race; the catch in loadZoektClient clears the cached promise on the timeout so the next call retries.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b0e2403. Configure here.
| ); | ||
| }); | ||
| try { | ||
| return await Promise.race([build(), timeout]); |
There was a problem hiding this comment.
Orphan build rejection unhandled
Medium Severity
In buildClient, when Promise.race times out, the underlying client build continues in the background. If this background work later rejects, its rejection is unhandled, which can lead to an unhandled promise rejection in Node.
Reviewed by Cursor Bugbot for commit b0e2403. Configure here.
| } | ||
| }); | ||
| }); | ||
| }, READINESS_TIMEOUT_MS); |
There was a problem hiding this comment.
Zoekt init RPC share timeout
Medium Severity
The Zoekt readiness check wraps loadZoektClient() and the gRPC List call in one 2000ms withTimeout, while client build alone may take up to 1500ms. A successful but slow cold init can leave too little time for List, so the probe returns degraded even when Postgres, Redis, and Zoekt are healthy.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b0e2403. Configure here.
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.
Summary
Adds
GET /api/health/ready, a Kubernetes-style readiness probe that returns per-dependency health (Postgres, Redis, Zoekt). The existingGET /api/healthliveness endpoint is unchanged.Self-hosted operators can now wire Sourcebot into a Kubernetes
readinessProbeor a load balancer health check and have the probe reflect whether the instance can actually serve traffic.Motivation
GET /api/healthcurrently returns a hardcoded{ "status": "ok" }regardless of whether Postgres, Redis, or Zoekt are reachable. A pod that has lost its database connection still reports "ok" to a Kubernetes readiness probe, so traffic keeps being routed to a broken instance.The previous FR for this (#727) was closed because the endpoint existed, but the original ask was for "structured response with details on its inner components' health status" — which was never delivered.
Closes #1506.
Changes
packages/web/src/app/api/(server)/health/ready/route.ts— new endpoint. Three checks run in parallel viaPromise.all; each is bounded by a 2s timeout, so the worst-case request time is ~2s even when one dependency hangs. Returns 200 with{status:ok, checks:{...}}on success, 503 with{status:degraded, checks:{...}}on any failure. Per-check payload includesstatus,latencyMs, and (on error)error.packages/web/src/lib/zoektClient.ts— extracted the lazy Zoekt gRPC client construction into its own module so the readiness route can mock it in tests without pulling the@grpc/grpc-js+@opentelemetryCJS chain at test-load time. The dynamic-import + cached-promise pattern also avoids the boot-time cost of loading gRPC on the happy path where only some requests actually need it.packages/web/src/app/api/(server)/health/ready/route.test.ts— six test cases covering the healthy path, each of the three degraded paths, the non-PONGresponse path, and the parallel-execution invariant.docs/docs/api-reference/health.mdx— new docs page describing both endpoints, the response shape, and example Docker Composehealthcheck:and KuberneteslivenessProbe/readinessProbeblocks. Wired into the existing System group indocs/docs.json.CHANGELOG.md— entry under[Unreleased] → Added.Design decisions
Promise.allkeeps worst-case latency atmax(timeouts), notsum(timeouts). A test asserts this explicitly.Listwith a 1s wall-time cap for Zoekt. This is the smallest gRPC request that proves the channel is alive end-to-end. A dedicatedWebserverService.Healthmethod would be cleaner but doesn't exist in the vendored Zoekt proto.Listreturns success even with zero repos indexed. This is the right behavior for a liveness/readiness probe (the question is "can we talk to Zoekt?", not "are there repos to search?"); astrict=truequery param for "shards must be non-empty" is noted in the issue's future-work section./api/healthpolicy and the documented contract that orchestrator probes should be free to hit the endpoint at any frequency.Verification
yarn workspace @sourcebot/web test --run "health/ready"— 6/6 tests passyarn workspace @sourcebot/web lint— 0 errors (4 pre-existing warnings in unrelated files)tsc --noEmit -p packages/web/tsconfig.json— 0 errors in the new files (pre-existing errors inauth.tsand a few EE files are documented inHANDOFF.md§21, out of scope)Backward compatibility
Fully backwards compatible. The existing
/api/healthendpoint is unchanged. The new endpoint is purely additive. Operators who do not configure it see no change.Risks
failureThreshold; the per-check 2s timeout absorbs brief blips.Listwith a 1s wall-time cap; under load this could add to Zoekt's request rate. The check runs at most once per probe interval per pod (~0.3 RPS for the default 10s interval on a 3-pod deployment), which is negligible.Out of scope (tracked in #1506 as future work)
sourcebot_readiness_check_duration_seconds{check="postgres"}, etc.) — would build on the same per-check helpers introduced here./api/health/ready?strict=truemode that treats an empty Zoekt shard set as degraded.Note
Low Risk
Additive, unauthenticated health API with no auth or data-model changes; main operational risk is probe misconfiguration causing traffic drain during brief dependency blips.
Overview
Adds
GET /api/health/ready, a public readiness endpoint that reports whether Postgres, Redis, and Zoekt are reachable.GET /api/healthstays the lightweight liveness check and is unchanged.The new route runs the three dependency checks in parallel, each capped at 2s, and returns 200 with per-check
status,latencyMs, and overallok, or 503 withdegradedand error details when any check fails. Zoekt is probed via a minimal gRPCListcall; client construction is moved intozoektClient.tswith lazy init, build timeout, and test-friendly mocking.Docs add a health API reference page (liveness vs readiness, response shape, K8s/Docker examples) and a CHANGELOG entry. Tests cover healthy/degraded paths, parallel execution, timeout/rejection handling, and Zoekt request shape.
Reviewed by Cursor Bugbot for commit b0e2403. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
GET /api/health/ready) that checks PostgreSQL, Redis, and Zoekt and returns per-check status, latency, and overallok/degraded.Documentation
GET /api/health) semantics.Tests