feat(web): add ?strict=true mode to /api/health/ready for empty-shard detection#1512
Open
Harsh23Kashyap wants to merge 20 commits into
Open
feat(web): add ?strict=true mode to /api/health/ready for empty-shard detection#1512Harsh23Kashyap wants to merge 20 commits into
Harsh23Kashyap wants to merge 20 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.
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)`.
Self-hosted operators wiring Sourcebot into a Kubernetes readinessProbe typically want two distinct signals: - "soft ready" — the process is up and the dependencies are reachable. This is the default behavior of /api/health/ready. - "strictly ready" — the process is up, the dependencies are reachable, and the instance can actually serve a search that returns results. A node that has not finished indexing anything is not strictly ready even if all three RPCs succeed. Today there is no way to ask the probe the second question. A node can pass liveness, pass readiness, and still be useless to a search request. Add ?strict=true to /api/health/ready. When strict mode is on and the Zoekt List response is empty (neither repos nor repos_map carries any entries), the Zoekt check is reported as status: "empty" with the error string "no repositories indexed (strict mode)" and the overall response becomes 503 / status: "degraded". The Postgres and Redis checks behave identically in both modes. The response always includes a top-level strict field so operators can confirm which mode was applied. An unparseable strict value (e.g. ?strict=yes) is rejected as 400 by the Zod schema; the schema intentionally does not use z.coerce.boolean() because that is just Boolean(value) under the hood and would treat the string "false" as truthy. Closes sourcebot-dev#1511. Implementation notes: - The gRPC List callback now actually receives and surfaces the response object so the strict check can inspect it. The loose type in zoektClient.ts is updated accordingly: List's callback signature is (err, result) => void, not (err) => void. The full gRPC response type is wider than we need; we narrow it to a ZoektListResponse summary so the test mock can stay small. - The empty check covers both ListOptions.Field values (RepoListFieldRepos -> repos, RepoListFieldReposMap -> repos_map) so the check is correct regardless of which field the server populates. - ZoektListResponse is exported from zoektClient.ts so the test can type its mock callback against the same shape.
Seven new test cases for the ?strict=true mode on /api/health/ready: - Returns 200 with strict:true when Zoekt has at least one indexed repo. - Returns 503 with zoekt.status:"empty" when Zoekt has no indexed repos (default Field = RepoListFieldRepos, empty repos array). - Returns 503 in strict mode when the Zoekt response uses repos_map (Field = RepoListFieldReposMap, empty map). - Returns 200 in strict mode when repos_map is non-empty. - Backward-compat: returns 200 with strict:false and zoekt.status:"ok" when Zoekt has zero repos (the default behavior must NOT consider empty shards as a failure unless strict mode is requested). - Treats an absent strict parameter as strict:false. - Returns 400 with a clear error message when ?strict=yes (anything other than the literal strings "true" or "false" fails the Zod refinement). The existing eight tests are updated to pass a minimal NextRequest stand-in (via a new makeRequest helper) so the route can read the strict query parameter, and the Zoekt List mock now passes a response object to its callback (the route inspects the response in strict mode).
Adds a 'Strict mode' subsection to docs/docs/api-reference/health.mdx that explains the operator motivation, the new response fields (strict top-level, zoekt.status:"empty"), and the distinction between error and empty in the Zoekt check's status field. Updates the response-shape examples to include the new strict field and adds a Kubernetes readinessProbe snippet that points at the strict path for deployments that should not receive traffic until the shard set is non-empty.
Adds an Added entry under [Unreleased] for the new ?strict=true query parameter on /api/health/ready, referencing issue sourcebot-dev#1511.
- route.ts: trim a couple of over-explained comments around the strict Zoekt check; the proto's __Output type already guarantees the shape we read, so the defensive 'stub mismatch' framing is unnecessary. - route.test.ts: pull the verbose ZoektListCallback type out to a single alias so the seven mockImplementation blocks stop carrying the same 80-character inline signature. - health.mdx: drop em dashes per the docs style guide.
Contributor
WalkthroughAdds an unauthenticated ChangesReadiness health endpoint
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ReadinessRoute
participant Postgres
participant Redis
participant Zoekt
Client->>ReadinessRoute: GET /api/health/ready?strict=true
ReadinessRoute->>Postgres: SELECT 1
ReadinessRoute->>Redis: PING
ReadinessRoute->>Zoekt: List({})
Postgres-->>ReadinessRoute: check result
Redis-->>ReadinessRoute: PONG or error
Zoekt-->>ReadinessRoute: repositories or error
ReadinessRoute-->>Client: 200 ok or 503 degraded
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Address Cursor Bugbot finding on PR sourcebot-dev#1512: a malformed gRPC response can fire the List callback with `err === null` and `result === undefined`. Without a guard, the route would throw on `response.repos` and the failure would surface as a generic Node unhandled-rejection path instead of the controlled `error` status that the response shape advertises. `zoektSearch` applies the same `error || !response` pattern (`features/search/zoektSearcher.ts`). Reject with a clear error message when the callback fires with no error but no response either, matching that contract. Added a regression test that fires the List callback with `callback(null, undefined)` and asserts the response carries `zoekt.status:"error"` and `zoekt.error` mentioning the missing response.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3fa3c63. Configure here.
![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 (2)
packages/web/src/lib/zoektClient.ts (2)
21-65: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConcurrent cold-start calls to
loadZoektClient()can build the client redundantly.Since
cachedClientis only checked (not reserved) beforebuildClient()resolves, overlapping first calls each rebuild independently. Not a correctness bug — just wasted work on concurrent cold starts. Consider caching the in-flight promise instead of the resolved client if this matters in practice.🤖 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/lib/zoektClient.ts` around lines 21 - 65, Update loadZoektClient and the client cache to store and reuse an in-flight initialization promise, so concurrent cold-start calls share one buildClient invocation. Cache only successful initialization and clear the pending promise when buildClient fails, preserving retries on subsequent calls.
30-32: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAnchor the vendored proto path to the package location.
This loader duplicates the search searcher’s
process.cwd()-relative proto path; if the Next process is launched outsidepackages/web, both paths resolve incorrectly and Zoekt fails to initialize. Useimport.meta.dirname-relative resolution, e.g.../../vendor/zoekt/grpc/protos, unless the codebase intentionally requires this cwd constraint everywhere.🤖 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/lib/zoektClient.ts` around lines 30 - 32, Update the proto path construction in the Zoekt loader to resolve from the module’s package location via import.meta.dirname, using the existing ../../vendor/zoekt/grpc/protos relative path instead of process.cwd(). Keep protoPath resolving webserver/v1/webserver.proto beneath that anchored base path.
🤖 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`:
- Line 12: Update the changelog entry’s trailing link to reference the pull
request using the required
[#<id>](https://github.com/sourcebot-dev/sourcebot/pull/<id>) format, while
preserving the existing single-sentence description.
In `@docs/docs.json`:
- Around line 219-222: Update the API documentation navigation group in
docs.json to eliminate the duplicate health entry: either remove
docs/api-reference/health when GET /api/health is sourced from
sourcebot-public.openapi.json, or add /api/health/ready to that OpenAPI spec and
retain the page only if it documents a public endpoint.
In `@packages/web/src/app/api/`(server)/health/ready/route.ts:
- Around line 82-89: Replace the raw err.message values returned by
checkPostgres, checkRedis, and checkZoekt with a generic client-safe error
string while preserving the existing error status and latency fields. Continue
relying on the route’s server-side logger.warn checks logging to retain full
diagnostic details without exposing infrastructure information through the
public response.
---
Nitpick comments:
In `@packages/web/src/lib/zoektClient.ts`:
- Around line 21-65: Update loadZoektClient and the client cache to store and
reuse an in-flight initialization promise, so concurrent cold-start calls share
one buildClient invocation. Cache only successful initialization and clear the
pending promise when buildClient fails, preserving retries on subsequent calls.
- Around line 30-32: Update the proto path construction in the Zoekt loader to
resolve from the module’s package location via import.meta.dirname, using the
existing ../../vendor/zoekt/grpc/protos relative path instead of process.cwd().
Keep protoPath resolving webserver/v1/webserver.proto beneath that anchored base
path.
🪄 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: 711f990d-035b-42f5-bfdd-291919ea88c6
📒 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
…oekt client Address Cursor Bugbot finding on PR sourcebot-dev#1512 (zoektClient.ts#L48-L52): the readiness probe's Zoekt client was created without `grpc.max_receive_message_length` / `grpc.max_send_message_length`, while `zoektSearcher` uses 500MB. A healthy Zoekt instance with a large repo catalog could hit the default 4MB gRPC receive cap, fail the probe, and report `zoekt.status:"error"` even though search against the same backend would succeed. Match the channel options `zoektSearcher` uses (500MB receive / send) so a healthy Zoekt does not fail the probe on a large catalog. Also loosens the constructor type in zoektClient.ts to accept the third `options` argument; the loose type was previously hard-coded to two parameters, which would have rejected this change at tsc time.
…eadiness probe Address CodeRabbit security finding on PR sourcebot-dev#1512: the readiness route returns raw `err.message` from Prisma / Redis / gRPC in the public JSON response. Since the route is unauthenticated and may be reachable at a public URL, those messages can leak internal hostnames, ports, or connection details to any caller. - Add a new `errorDetail` field on the per-check `CheckResult` that carries the raw `err.message`. - Replace the public `error` field on degraded checks with a generic "<label> check failed: see server logs" string. - The GET handler logs the FULL per-check result (with `errorDetail`) when the overall probe is degraded, so operators still have the diagnostic detail on the server side. - The response body uses a stripped view that omits `errorDetail`. Tests updated: the four degraded-path tests now assert the generic public error string, that the internal message (containing fake hostnames) does not appear in the response, that `errorDetail` is undefined on the wire, and that the full message is present in the server-side log call.
…json Address CodeRabbit finding on PR sourcebot-dev#1512: `docs/api-reference/health.mdx` already documents both `/api/health` (Liveness section) and `/api/health/ready` (Readiness section), so the mdx page is the canonical doc for the health endpoints. The standalone `GET /api/health` entry under the System group was sourced from the OpenAPI document and duplicated the liveness info as a second nav item. Remove the `GET /api/health` entry. `docs/api-reference/health` becomes the single nav entry for both endpoints.
…ssue sourcebot-dev#1511 Address CodeRabbit finding on PR sourcebot-dev#1512: per the project changelog format guideline, entries should link to the PR, not the issue.
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
?strict=truetoGET /api/health/readyfor empty-shard detection. The default behavior of the readiness probe is unchanged: a ZoektListRPC that returns an empty result still counts as a successful probe, because the question a readiness probe answers is "can this instance talk to its dependencies?". With?strict=true, an empty Zoekt shard set is treated as503 / status: "degraded"andzoekt.status: "empty", distinguishing "Zoekt is up but the instance has not finished indexing" from "Zoekt is unreachable".Fixes #1511.
Motivation
A self-hosted operator wiring Sourcebot into a Kubernetes
readinessProbe(or a load-balancer health check) typically wants two distinct signals:Today there is no way to ask the probe the second question. A node can pass liveness, pass readiness, and still be useless to a search request. The default behavior is the right one for "can we talk to our dependencies?" but operators who want "is this instance useful?" need a way to ask it.
Changes
packages/web/src/app/api/(server)/health/ready/route.ts— adds thestrictquery param (parsed by a Zod schema; rejects anything other than the literal strings"true"/"false"as 400), threads it intocheckZoekt, and adds a third check status"empty"for the strict-empty case. ThecheckZoektfunction now actually captures and inspects theListresponse (it previously only consumed the callback's error argument). The gRPC callback signature inzoektClient.tsis corrected accordingly: it always was(err, result) => void, not(err) => void.packages/web/src/lib/zoektClient.ts— adds a looseZoektListResponsesummary type alongside the existingZoektClientso the route and its test can refer to the same shape without pulling in the full gRPC response type. Fixes theListcallback signature.packages/web/src/app/api/(server)/health/ready/route.test.ts— seven new test cases: strict with one indexed repo → 200; strict with emptyrepos→ 503 /empty; strict with emptyrepos_map(theRepoListFieldReposMapfield) → 503 /empty; strict with non-emptyrepos_map→ 200; non-strict with emptyrepos→ 200 (backward-compat); absentstrictparameter treated asfalse; invalidstrictvalue → 400. Existing tests updated to pass a minimalNextRequeststand-in (the route now needsnextUrl.searchParams).docs/docs/api-reference/health.mdx— new "Strict mode" subsection explaining the operator motivation, the new response fields, and theerrorvsemptydistinction. The K8sreadinessProbeexample gets a comment pointing at the strict path for deployments that should not receive traffic until the shard set is non-empty.CHANGELOG.md— Added entry under[Unreleased]linking to [FR] Add ?strict=true mode to /api/health/ready for empty-shard detection #1511.Design decisions
/api/health/ready/strictwould duplicate the route, the auth policy, the PostHog tracking opt-out, and the per-check response shape. The query param keeps it additive.?strict=truesee exactly the same response as before. A backwards-incompatible behavior change for a recently-merged endpoint is a poor first move."empty"instead of overloading"error"."error"means the check itself failed (Zoekt unreachable, timeout, etc.)."empty"means the check passed but the result is empty in strict mode. They are different operational signals and an operator looking at the response should be able to tell them apart without parsing theerrorstring.reposandrepos_map. The Zoekt gRPC server populates different fields depending onListOptions.Field(RepoListFieldReposvsRepoListFieldReposMap). An empty response in either shape is "strictly empty" and the check covers both.z.string().refine(...)instead ofz.coerce.boolean(). The latter is justBoolean(value)under the hood, which would treat the string"false"as truthy. Accept only the two literal strings we mean to support, refuse anything else as 400.ZoektListResponsesummary type. The full gRPC response type is much wider than we need; exporting a narrow summary keeps the test mock small and avoids coupling the route's error handling to the full proto type.Verification
yarn workspace @sourcebot/web test --run "health/ready"— 15/15 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 filesBackward compatibility
Fully backwards compatible. The default response shape gains a single new field (
strict); all other fields and all status codes are unchanged whenstrictis absent orfalse.Stack
This PR is stacked on #1507 (the readiness probe itself). The first commit in the diff (
f5193aea) is themax_wall_timefollow-up from the PR #1507 review, not new work for this PR. Once #1507 merges, this branch will be rebased ontomainand that commit will fall away; the remaining four commits are the actual?strict=truework.Out of scope (tracked in #1511 as future work)
Risks
Listresponse shape differs between proto versions. The check handles bothreposandrepos_map; an empty result in either is treated as strict-empty. The existingzoektClientreturns the full gRPC__Outputshape, so no information is lost.?strict=truein a dev environment with no indexed repos will see the probe fail with a clearerror: "no repositories indexed (strict mode)"rather than a generic "degraded". The docs call this out.Note
Low Risk
Additive, unauthenticated operational endpoints with backward-compatible defaults; main caveat is probe load on Postgres and misconfigured strict readiness blocking traffic until indexing completes.
Overview
Adds
GET /api/health/ready, a public readiness probe that checks Postgres (SELECT 1), Redis (PING), and Zoekt (emptyListgRPC) in parallel with a 2s per-check timeout. It returns 200 with per-check latency when all pass, or 503 withstatus: "degraded"and generic client-safe errors (details logged server-side).GET /api/healthstays the lightweight liveness probe.Optional
?strict=truetreats an empty Zoekt shard set as not ready:zoekt.status: "empty"and 503, separate fromerrorwhen Zoekt is unreachable. Default /strict=falsebehavior is unchanged (empty index still counts as reachable). Invalidstrictvalues return 400.Introduces
packages/web/src/lib/zoektClient.ts(mockable gRPC client +ZoektListResponse) and extensive Vitest coverage. Docs adddocs/api-reference/health.mdx(K8s/Docker examples, strict mode);CHANGELOG.mdanddocs.jsonare updated.Reviewed by Cursor Bugbot for commit 44e43bf. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
GET /api/health) and readiness (GET /api/health/ready) that probe Postgres, Redis, and Zoekt.?strict=trueto mark health degraded when Zoekt has no indexed repositories.