feat(cache): namespace-versioned cache keys for queries and pipes - #314
Conversation
|
Warning Review limit reached
More reviews will be available in 45 minutes and 21 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds per-table and per-namespace versioning and deterministic query-key construction in VersionManager (Namespace type, NamespaceKey, QueryKey, BumpTable, BumpNamespace). LocalCache.Get/Set now read/write query results using folded keys from (sha, deps) and Invalidate(namespaces) bumps versions. Callers, tests, and mocks updated to the new namespace-shaped API. ChangesQuery-based caching with namespace versioning
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5f93eb70-dbd9-4283-af83-6f8084fcc906
📒 Files selected for processing (11)
internal/api/pipes.gointernal/api/structured_query.gointernal/cache/cache.gointernal/cache/cache_test.gointernal/cache/local.gointernal/cache/local_test.gointernal/cache/version_manager.gointernal/cache/version_manager_test.gointernal/ingest/worker.gointernal/ingest/worker_test.gointernal/testutil/mocks.go
💤 Files with no reviewable changes (1)
- internal/cache/cache_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: CI
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (go)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Use Go 1.26, strict formatting withgofumpt
Use structured logging withlog/slog(JSON handler) in Go code
Use Chi v5 for HTTP routing in Go code
Go error handling: Return errors, don't panic. Wrap withfmt.Errorf("context: %w", err)
No global state in Go code — pass dependencies explicitly via constructor injection
Go package naming: lowercase, single word (or abbreviated), withinternal/enforcing module privacy
Go files must passgofumptformatting (enforced by CI)
Go code must passgolangci-lintchecks (v2.11.4, auto-installed to.bin/on firstmake lint)
Files:
internal/api/pipes.gointernal/api/structured_query.gointernal/testutil/mocks.gointernal/ingest/worker.gointernal/cache/local.gointernal/cache/version_manager_test.gointernal/ingest/worker_test.gointernal/cache/local_test.gointernal/cache/cache.gointernal/cache/version_manager.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Use table-driven tests withtests := []struct{ name string; ... }andt.Run(tt.name, ...)
Use shared mocks frominternal/testutil/(MockPublisher, MockCache, MockDeduplicator, MockSubscriber) in Go tests
Usetestutil.MakeJWT(t, claims)andtestutil.MakeExpiredJWT(t, claims)for auth tests in Go
Usetestutil.NewTestSchemaRegistry(tables)ordiscovery.NewSchemaRegistryFromMap(tables)for schema-aware Go tests
Usepolicy.NewMemoryStore(p)for in-memory policy testing without NATS in Go tests
Usepipes.NewMemoryStore(queries...)for in-memory pipes testing without NATS in Go tests
Usetestutil.AssertJSONResponse(t, rec, status, expected)andtestutil.AssertJSONContains(t, rec, status, substring)for response assertions
Files:
internal/cache/version_manager_test.gointernal/ingest/worker_test.gointernal/cache/local_test.go
🧠 Learnings (2)
📚 Learning: 2026-05-20T20:30:15.808Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 172
File: internal/api/pipes_test.go:106-118
Timestamp: 2026-05-20T20:30:15.808Z
Learning: For WaveHouse pipes authorization allowlist checks, fix the empty-role fail-open behavior by (1) removing any outer guard that prevents allowlist evaluation when the incoming `role` is `""` (e.g., don’t short-circuit with `if role != "" { ... }`), and (2) during allowlist scanning, ensure only non-empty allowlist entries can match—e.g., require `ar != "" && ar == role` (so a malformed allowlist like `["" ]` cannot grant access to an empty incoming role via `"" == ""`).
Applied to files:
internal/api/pipes.go
📚 Learning: 2026-05-25T11:24:21.130Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 180
File: internal/cache/local.go:0-0
Timestamp: 2026-05-25T11:24:21.130Z
Learning: In WaveHouse’s cache packages (e.g., internal/cache/local.go), it’s acceptable to define package-level `var` constants that hold immutable OpenTelemetry metric attribute sets / `metric.MeasurementOption` values (for example: `cacheL1Attrs = metric.WithAttributes(attribute.String("tier","L1"))`). Treat these as stateless, pre-allocated option values (analogous to `regexp.MustCompile(...)`), not mutable global state. When applying the AGENTS.md “no global state / constructor injection” guideline, apply it to application dependencies (e.g., Cache, Publisher, Deduplicator) rather than to these immutable OTel attribute/measurement option variables—do not flag them as constructor-injection violations.
Applied to files:
internal/cache/local.gointernal/cache/version_manager_test.gointernal/cache/local_test.gointernal/cache/cache.gointernal/cache/version_manager.go
🔇 Additional comments (22)
internal/cache/version_manager.go (5)
1-10: LGTM!
12-32: LGTM!
34-51: LGTM!
53-74: LGTM!
76-94: LGTM!internal/cache/cache.go (2)
8-32: LGTM!
34-53: LGTM!internal/cache/local.go (5)
11-29: LGTM!
31-43: LGTM!
45-54: LGTM!
56-87: LGTM!
89-99: LGTM!internal/api/pipes.go (1)
139-177: LGTM!internal/api/structured_query.go (1)
134-134: LGTM!Also applies to: 170-170
internal/ingest/worker.go (2)
481-487: LGTM!
466-479: Confirm empty-scope encoding preserves whole-table cache invalidation
query.SafeEncodeNATS("")returns""(seeinternal/query/ident_test.go“empty string” case andinternal/query/ident.goimplementation), soScope: query.SafeEncodeNATS(pm.scope)keeps empty scopes empty and matches theinternal/cache/cache.gocontract for whole-table bumps.internal/cache/version_manager_test.go (1)
9-72: LGTM!internal/cache/local_test.go (1)
12-165: LGTM!internal/ingest/worker_test.go (3)
140-232: LGTM!
449-535: LGTM!
696-756: LGTM!internal/testutil/mocks.go (1)
117-136: LGTM!
… not Cache.Invalidate
Summary
Adds a versioned, namespace-based cache-key scheme to internal/cache that underpins cache invalidation for both structured queries and (eventually) named pipes. A cached result is keyed by every (table, scope) it depends on — one for a structured query, several for a pipe — so a write to any dependency invalidates it. Groundwork for per-pipe cache invalidation (#178).
Additive and dormant: it sits alongside the existing flat versions map (which still backs the live Cache.Get/Set path), and nothing in the read/ingest paths is wired to it yet, so no behavior change.
QueryKey folds the table version and namespace version of every (table, scope) a result depends on into its key, so a bump of any dependency misses it. A structured query passes one Namespace; a multi-table query (pipe) passes several — same QueryKey, same BumpTable/BumpNamespace.
Intended invalidation policy (once the ingest worker is wired): a write to a named scope → BumpNamespace(table, scope) (that scope + the whole-table view; other scopes survive); a no-scope / whole-table write → BumpTable(table) (whole table in O(1)).
Need todos:
From Claude: Heads-up on scope: these changes are groundwork and not active on the live path yet. The new tableVersions/namespaceVersions registries and the BumpTable/BumpNamespace primitives exist, but nothing calls them on a real ingest — the worker still invalidates through the legacy flat versions map, and the query/pipe handlers still read through the legacy Cache.Get/Set. So a real write today does not update the new tables or namespace maps, and GetQuery isn't reachable from any endpoint. Making it effective is a follow-up that wires the handlers onto GetQuery and the worker onto BumpTable/BumpNamespace (choosing which by the ingest's scope), reaching into internal/api and internal/ingest. This PR is intentionally additive with no behavior change.