Add caching for /_skill-validation - #5504
Merged
Merged
Conversation
The endpoint prerendered every distinct tool module on every request, so a frequent monitor spent most polls near the prerender latency and periodically crossed its degraded/timeout thresholds even when nothing was wrong. Cache the per-realm result and refresh it off the request path: a poll serves the last computed result immediately and, once that result is older than REFRESH_AFTER_MS, kicks a deduped background refresh for the next poll. Only the first poll after a process start computes synchronously, so the response stays a definite pass/fail. The response now reports `ageSeconds` so callers can see result freshness. Add a `refresh=true` query param that forces a synchronous recompute — used by the endpoint tests (which share a realm URL) for determinism, and useful operationally to force a fresh check on demand. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
backspace
marked this pull request as ready for review
July 15, 2026 18:54
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a6d25b954
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
pending.finally(...) returned a second promise that rejected unobserved when a refresh threw (search/DB error, prerenderer throw), surfacing as an unhandled rejection that can terminate the process — the caller only observes `pending` itself. Move the refreshInFlight cleanup into a try/finally inside the promise body so `pending` stays the only promise. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The identity check referenced `pending` inside its own initializer closure, which TS flags as used-before-assigned. It's also unnecessary: the dedup guard hands a concurrent caller the existing in-flight promise, so no second refresh for a realm can be queued while this one runs — the map entry is always this one when the finally executes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces flakiness/latency for the realm-server monitoring endpoint /_skill-validation by caching per-realm validation results and refreshing them off the request path, while also surfacing ageSeconds so monitors can tell how fresh the served result is.
Changes:
- Add an in-process per-realm cache for
/_skill-validationresults, with a background refresh once results are older than a refresh threshold. - Add
refresh=truesupport to force an on-request recompute. - Update the existing endpoint tests to use
refresh=trueso assertions aren’t affected by cached results.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/realm-server/handlers/handle-skill-validation.ts | Introduces cached validation results + background refresh + ageSeconds, and factors validation computation into a helper. |
| packages/realm-server/tests/server-endpoints/skill-validation-test.ts | Adjusts existing tests to request a synchronous recompute via refresh=true. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The result cache and in-flight refresh map were module-level singletons keyed by realm URL, so any two realm servers in one process shared cached results — cross-contaminating otherwise-isolated test fixtures. Hold both maps in the handler factory closure instead: production is one server per process (unchanged), and same-process servers no longer share a cache. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cover the caching contract with a counting prerenderer proxy: a cold poll runs the sweep, a warm poll serves the cache without touching the prerenderer, and refresh=true forces a recompute. Now that the result cache is per handler instance, the two existing endpoint tests no longer need refresh=true to escape a cross-fixture cached result, so drop it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
lukemelia
approved these changes
Jul 16, 2026
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
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.
The purpose of the
/_skill-validationendpoint is to give us an early warning if skills are likely to be broken in the assistant. Since it’s been deployed it’s been flaky (which is why I kept it in the drafts channel):This is because it needs to prerender every tool module, which sometimes reaches Checkly “degraded” status, and occasionally even exceeds the Checkly 30s maximum. We shouldn’t have alarms going off for reasons unrelated to their actual goals.
This PR adds caching of the validation results. A request for
/_skill-validationschedules a validation refresh so the next request gets its results. The response includesageSecondsto indicate when the results are from.