feat(metaschema): refresh the schema cache on a timer and make it concurrency-safe - #1878
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 21 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesMetaschema cache lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to The change safely refreshes metaschema data across pods, but shutdown can hang if the service is closed with a non-cancelable context. The PR is otherwise mergeable with explicit owner awareness to cancel the refresh context during Close. Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
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 |
Coverage Report for CI Build 31782828016Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.2%) to 48.45%Details
Uncovered Changes
Coverage Regressions4 previously-covered lines in 2 files lost coverage.
Coverage Stats
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 59e3f98a-7150-490f-a857-75e9217738c5
📒 Files selected for processing (8)
cmd/migrate.gocmd/serve.goconfig/sample.config.yamlcore/metaschema/config.gocore/metaschema/service.gocore/metaschema/service_test.godocs/content/docs/reference/configurations.mdxpkg/server/config.go
rohilsurana
left a comment
rohilsurana
left a comment
There was a problem hiding this comment.
Automated review of the metaschema cache refresh. Findings 1 to 4 all trace back to one design choice: reload() does a blind full-cache swap. Left inline below, most serious first.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/metaschema/service.go (1)
182-220: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCancel the refresh context in
Closebefore waiting.Closewaits for the cron callback to finish, butClosedoes not cancel the context captured byInit. Production shutdown cancels its signal context first, but callers that passcontext.Background()can blockCloseindefinitely. Store a cancel function for the refresh context, invoke it inClose, and test cancellation throughRepository.List.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ac267f94-49ec-42ad-adb4-eb07fed5244d
📒 Files selected for processing (5)
core/metaschema/errors.gocore/metaschema/service.gocore/metaschema/service_test.gointernal/api/v1beta1connect/metaschema.gointernal/api/v1beta1connect/metaschema_test.go
| if schemas, err := deps.MetaSchemaService.List(context.Background()); err != nil { | ||
| // prime the metaschema cache and start its periodic refresh | ||
| if err := deps.MetaSchemaService.Init(ctx); err != nil { | ||
| logger.Warn("metaschemas initialization failed", "err", err) |
There was a problem hiding this comment.
Init returns an error now when the first cache load fails, but the caller only logs a warning and moves on. If the DB is unreachable for a moment at boot, the server comes up with an empty cache and Validate passes everything without checking. With refresh_interval: 0 there is no later reload, so it stays that way until the process restarts. Nothing in the logs says validation is off — just one warn line at startup.
Two ways to close it:
- Treat it as a startup failure:
if err := deps.MetaSchemaService.Init(ctx); err != nil {
return fmt.Errorf("metaschemas initialization: %w", err)
}If the schemas can't be loaded, the server fails to start, same as a failed migration a few lines below. This also matches the comment inside Init that says "startup stops here" — right now the caller doesn't actually stop.
- Fetch on demand — when a read finds the cache empty, fall back to
repository.Listand fill it. This is what the code did before this PR, so a failed load at boot heals on the first request that needs a schema. The cost is keeping the fallback path this PR deliberately removed, and until that first successful fetch, writes go through without validation.
I'd go with 1 — it's one line and there's no window where validation is silently off.
There was a problem hiding this comment.
Fixed in 9b062b0 with option 1. cmd/serve.go now returns fmt.Errorf("metaschemas initialization: %w", err) when Init fails, so the server stops at startup instead of coming up with an empty cache and validation silently off, same as the failed-migration path just below. Thanks for catching that the caller was not actually stopping.
3 participants
Summary
Metaschemas hold the JSON schemas that validate entity metadata for users, organizations, groups, and roles. The metaschema service keeps every schema in an in-memory map on the service. Today that map is primed once at boot and after that it only changes on the pod that handled a write. In a multi-pod deployment a schema change made through the API reaches one pod but not the others, so those pods keep validating against the old schema until they restart. The map also has no lock, so concurrent request goroutines on one pod can race on it, which Go turns into a "concurrent map read and map write" panic.
This PR makes the cache safe for concurrent access and reloads it on a timer, so every pod picks up a schema change within a small bounded window.
Changes
sync.RWMutexand switch every service method to a pointer receiver.robfig/cronpattern as the billing sync jobs.Initto prime the cache and start the job, andCloseto stop it.app.metaschema.refresh_intervalconfig. It defaults to1m, and0disables the job for single-pod, local, and test runs.InitandCloseinto server start and shutdown, and document the new setting.Technical Details
Get,List,Validate) take a read lock. Writes (Create,Update,Delete, and the reload) take a write lock.@everyschedules from pod start, so pods that start together reload in lockstep. For this tiny table that query cost is small. Jitter or PostgresLISTEN/NOTIFYare noted as future options if it ever matters.MetaSchemaServiceinterface and its generated mock stay valid.Test Plan
go build ./...,go vet)core/metaschemapass undergo test -race, covering concurrent access alongside a reload, reload picking up a new schema, cache kept on a list error, and the refresh-disabled path