feat(config): cache.backend=redis selects the shared cache - #630
Merged
Merged
Conversation
mq.backend, cache.backend, dedupe.backend and coord.backend select each layer's implementation; only today's in-process one exists per layer and it is the default. Validate refuses an unknown value, internal/app picks the implementation in one switch per layer, data_dir is probed only when a selected backend keeps state there, and boot logs Config.Warnings. Part of #613. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EJr5tY4WQUy2sc4MbW67vL
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EJr5tY4WQUy2sc4MbW67vL
…ENTS.md Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EJr5tY4WQUy2sc4MbW67vL
The local version index nested each table under its tenant's version and each scope under its table's, and was never pruned: every InvalidateTenant left the tenant's whole index behind. It now holds one version per tenant, (tenant, table) and (tenant, table, scope), bumped in place. A tenant bump drops the tenant's index and its next key gets a process-unique generation; a table bump drops the table's scopes. After each reload the wiring prunes the index to the tenants served. Fixes #262 for the local backend. Part of #613. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EJr5tY4WQUy2sc4MbW67vL
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EJr5tY4WQUy2sc4MbW67vL
…che-redis-wiring # Conflicts: # CHANGELOG.md # docs/src/content/docs/architecture.md
…eat/cache-redis-wiring # Conflicts: # AGENTS.md # docs/src/content/docs/architecture.md # internal/app/wire.go
The boot config's cache.backend now takes redis, configured by a new cache.redis block (WH_CACHE_REDIS_*), and wireCache builds a RedisCache from it, reading the TLS files. A malformed block refuses boot; an unreachable server or a rejected password boots bypassed and keeps reconnecting. An integration test boots two instances over one Redis and one ClickHouse: an ingest through one is served fresh by the other inside the stale entry's TTL, and a paused Redis leaves queries succeeding. The e2e suite now runs against Redis, so its coverage exclude is gone. Part of #613 (E4). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EJr5tY4WQUy2sc4MbW67vL
Exclude LocalCache from the e2e gate now that e2e runs on Redis; document the shared server as a trust boundary, the noeviction and mutation-pipe (#386) staleness cases, and the connection commands an ACL user needs; refuse addresses with surrounding spaces. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EJr5tY4WQUy2sc4MbW67vL
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
This was referenced Sep 25, 2026
Sync #621 with its parent, which now includes main's #612 squash and subsequent main history. Resolved conflicts in AGENTS.md and docs/architecture.md (app/wiring sections): kept cache-snapshot's rewritten prose and combined it with cache-flat-versions' own additions (the wireCache/LocalCache.Prune clauses naming cache in the per-tenant prune set). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
EricAndrechek
added a commit
that referenced
this pull request
Sep 25, 2026
Fixes #631. ## What was wrong Each boot-config default lived in a cleanenv `env-default` tag. cleanenv applies those after the YAML decode, to any field that is still at its zero value. So it could not tell a key the file set to `false`/`0`/`""` apart from a key the file left out. `otel.traces.enabled: false` loaded as `true`, and `sample_rate: 0` loaded as `1.0`. Nothing reported it. ## The fix (direction 1 from the issue) - `internal/config/config.go`: a new unexported `defaults()` is now the only place defaults are defined. `Load` starts from it, cleanenv decodes the YAML over it, and then applies the `WH_*` variables. Precedence is still env > YAML > default. All `env-default` tags are gone. The env tags are unchanged, so `unboundEnv`, `rejectUnknownKeys` and `TestEnvSettingsDir_MatchesStructTag` behave exactly as before. - I did not choose direction 2 (record which keys the file set, then restore them after cleanenv). It would keep the defaults in tags and add a second pass that has to stay in step with cleanenv. Direction 1 removes the cause and is smaller. ## Behaviour change A zero value you write in `config.yaml` now takes effect. If your file relied on the bug: - `sample_rate: 0` now exports no traces, or no DEBUG/INFO logs. Before, it silently exported everything. - A signal set to `enabled: false` is now off. - `shutdown_timeout: 0` now skips the drain. - `cache.l1_max_cost: 0` now refuses boot with `cache init: MaxCost can't be zero`. `server.port: 0` and `data_dir: ""` also refuse boot. An empty `prometheus.path` refuses boot when Prometheus is enabled. To get the default back, delete the key. Env vars already honoured an explicit zero, so they are unchanged. This is described under Fixed in the CHANGELOG. ## Tests (`internal/config/defaults_test.go`, all through `config.Load`) - `TestLoad_YAMLZeroIsKept`: for each key in the issue's table, a YAML false/0/"" comes back unchanged. This is the case that fails if defaults are applied again after the decode. I confirmed it: with the old loader it fails, and so do `TestLoad_IssueReproFile`, `TestLoad_YAMLZeroPortIsRefused` and `TestConfig_NoEnvDefaultTags`. - `TestLoad_IssueReproFile`: loads the issue's repro file as written. - `TestLoad_AbsentKeyGetsDefault`: when the file exists but leaves a key out, that key gets its default. - `TestLoad_EnvWinsOverYAMLZeroAndDefault`: env wins over a YAML zero, an env zero wins over a YAML value, and an env zero wins over the default when there is no file. - `TestZeroCases_CoverEveryNonZeroDefault`: a new non-zero default without regression coverage fails this test. - `TestConfig_NoEnvDefaultTags`: refuses any `env-default` tag. - `TestDocs_DefaultsMatchCode`: each Config field has exactly one row in `configuration.mdx`. The row must name the field's env var, and its Default column must parse to the value in `defaults()`. Doc rows with no matching field also fail. ## Deliberately left out - PR #630's `-1` sentinel for `cache.redis.compress_min_bytes`. Once this lands, `0` could mean "never compress". That change belongs to #630's owner. - Adding a `Validate` check for `cache.l1_max_cost <= 0`. Ristretto already refuses 0 at boot, and a new check would break every test that builds a Config literal without a cache block. ## Reviewers - `pre-push-reviewer` (opus), at 252ef7b: **ship_it**, with 0 MUST, 0 SHOULD and 0 MAY findings. It read cleanenv v1.5.0's source to confirm the precedence. It also checked that no shipped config file (`config.yaml`, `tests/e2e/fixtures/config.yaml`, `deployments/compose/standalone.yaml`) relied on the bug. - `docs-reviewer` (opus), at 252ef7b: **ship_it**, with 0 findings. Gate gap #454: reviewer markers land in the main checkout, not this worktree. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01EJr5tY4WQUy2sc4MbW67vL Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This was referenced Sep 26, 2026
TestReload_PrunesCacheIndexToServedTenants replaced a.cache with a
recorder by hand, so the run-time a.cache.(pruner) check against the
cache wireCache actually builds was never exercised: a change that
stopped wiring a pruning cache left the test green while pruning
silently went dead. Assert the wired cache satisfies pruner before the
swap.
Also reword a stale assertion message ("a rejection releases; it
orphans nothing yet") that no longer describes real wiring now that a
rejection drops the tenant's cache index via Prune, and correct the
CHANGELOG's claim that the Redis-compatible backend (#613) already
bounds its versions with a TTL — that backend does not exist yet.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017aS7rLrH1RKkUMem7X4ckd
…ache-flat-versions # Conflicts: # AGENTS.md # CHANGELOG.md # docs/src/content/docs/architecture.md # internal/app/wire.go # internal/cache/version_manager.go # internal/cache/version_manager_test.go
The version_manager.go bullet read "a bump of a tenant with no index is a no-op" right after introducing BumpTenant, so it looked scoped to that one call. It actually covers any bump — table, scope or tenant — against a tenant with no index, which is what lets Prune and a departed tenant's index stay gone (neither the sharedTables fan-out nor an insert still in flight for a just-pruned tenant can revive it). Found by pre-push review of #621 (origin/feat/cache-snapshot...HEAD). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017aS7rLrH1RKkUMem7X4ckd
The #262 entry said "fixes #262 for the in-process cache", which would close the issue on merge (delete_branch_on_merge + squash_merge_commit_message: PR_BODY carry the PR body's own "Fixes #262" into main's history the same way). #262 has a residual left open on purpose: per-table scope cardinality isn't capped, since scope stays empty until #235 populates it. Reworded to "part of #262" and named the residual, matching the PR body and the issue comment that records the trigger. Found by pre-push review of #621 (origin/feat/cache-snapshot...HEAD). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
feat/cache-redis at 1f24da5 and feat/cache-flat-versions at 95575f2, merged together first (e0d7018); both already carry #614 and main at 5004cd2. Every conflict keeps the new parents' text and re-applies only this PR's edits: the cache.backend: redis wording in AGENTS.md, architecture.md and the CHANGELOG, and the redis case in wireCache. configuration.mdx drops PING from the data commands, which the write probe replaced. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Since #626 bounds a cluster client's topology read by the larger of the two, a dial held by boot or Close is a connect and a handshake, each up to dial_timeout, plus that read. At a 2s dial cap it could reach 6s, past the 5s release budget. Both caps are now 1s, so at most 3s. The shared-cache docs now describe #626's behavior: a refused write opens the breaker, the owing instance bypasses the lookups it would orphan, and connections are replaced every minute after a failover behind a stable address. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…e2e gate - Standalone mode dials only the first address, so a second one was silently ignored: it now refuses boot. A port must be 1-65535. - The manual e2e boot command needs a Redis now that the fixture sets cache.backend: redis; the e2e stack is described as ClickHouse and Redis wherever it was ClickHouse only. - The e2e gate (59.9%) excludes cache_redis.go's rejection paths and pending.go's outage-only retries, as it excludes internal/settings; unit and integration cover both, and the merged total still counts them. - The integration tests read the TTL floor from cache.QueryTimeToTTL. - A failed InvalidateTenant on reconcile logs at WARN, like the worker. - The overview pages mention the shared cache. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
TestVersionManager_BumpWithoutIndex asserted only on vm.size() after its final BumpTenant call, which unconditionally deletes the tenant's index regardless of what the earlier no-index bumps did — so it could not fail against a tableLocked that wrongly creates an index instead of returning nil. Assert after each bump instead, and add the pruned-tenant case a write racing Prune must not revive. Also reword two docs passages that overloaded or misstated a term: architecture.md used "query key" for both the caller's input and the rendered entry key in the same paragraph; AGENTS.md's cache bullet read as if the index maps were keyed by escaped names; CHANGELOG.md's #382 bullet said the version index builds its keys with internal/keyenc, contradicting its own closing sentence that the index's entries are unaffected by the escaping. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017aS7rLrH1RKkUMem7X4ckd
- configuration.mdx: isAuthError also matches NOPERM (an ACL user missing a connection command); the boot-error-logging line only named WRONGPASS/NOAUTH. - CHANGELOG.md: the E4 entry claimed the e2e coverage exclude for the cache files was gone; .testcoverage.yml still excludes internal/cache/pending.go, internal/config/cache_redis.go and internal/cache/(local|version_manager).go from e2e, for reasons the entry now states. Also added the docs/.github files this PR touched that the entry's file list was missing. - pipes.mdx, api.md: "L1" claims that are wrong once cache.backend is redis (there is no L1) — reworded to "the query cache"/"cache". - deployment.md: "Persistence is not needed" was incomplete — a restart *with* persistence reloads a stale snapshot and can serve invalidated results as hits until their TTL. Now says to run without persistence, and that restoring a snapshot is a rollback. - development.md: note that shared_cache_test.go starts its own Redis per test and boots extra cache.backend=redis instances over the integration suite's ClickHouse. - app_test.go: TestRedisConfig_FromLoadedDefaults left Username, DB and TLS at their zero value on both sides of its assert.Equal, so deleting any of their three mapping lines in redisConfig wouldn't fail it. Added TestRedisConfig_UsernameDBTLSMapped, which drives all three to a non-zero value through config.Load. Mutation-checked: deleting each of the three lines in redisConfig fails this test (the TLS line fails to compile instead, since dropping it leaves the local `t` unused — still a build failure). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017aS7rLrH1RKkUMem7X4ckd
…-key mixup too Same overload as architecture.md's version_manager.go bullet: the type doc's "A query key folds all three versions of each dependency" means the rendered entry key QueryKey returns, not the caller's input. Reword to match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017aS7rLrH1RKkUMem7X4ckd
…ache-flat-versions #614's docs-only rewording (query key vs entry key terminology, a new cachetest bullet, a deployment.md tenant-move clarification, a CHANGELOG "pool"->"cache holds" fix) conflicted with this branch's own flat-index rewrite of the same passages. Kept this branch's flat-model content throughout, folding in #614's terminology fixes and its purely additive changes (the cachetest bullet, the deployment.md and development.md wording). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017aS7rLrH1RKkUMem7X4ckd
…s missed version_manager.go's tenants field comment still said "the first query key built for it" — the same overload the type doc three lines above and the other doc fixes in this round eliminated (query key = the caller's input, entry key = what QueryKey renders). Both confirmation reviewers caught this independently. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017aS7rLrH1RKkUMem7X4ckd
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017aS7rLrH1RKkUMem7X4ckd # Conflicts: # AGENTS.md # docs/src/content/docs/architecture.md
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017aS7rLrH1RKkUMem7X4ckd # Conflicts: # AGENTS.md # CHANGELOG.md # docs/src/content/docs/architecture.md
Verified against the merged internal/cache/{redis,breaker}.go:
- rejectsCredentials (WRONGPASS, NOAUTH) now trips the breaker at once
at runtime too, logged at ERROR once per opening and once per
refused probe (breaker.trip()); NOPERM deliberately does not (it
names one key/command, not every operation, so an ACL denial on one
tenant's traffic shouldn't bypass the cache for all of them).
configuration.mdx's circuit-breaker paragraph didn't mention this at
all; added it, distinct from the dial-time isAuthError logging (a
separate function, used only in dial()/dialLoop(), where WRONGPASS,
NOAUTH and NOPERM are all ERROR since any of them blocks the
connection outright before a client exists to send a scoped command
on).
- deployment.md's failover bullet gets the probe's DialTimeout+Timeout
budget and the caveat that every other connection still redials
under Timeout alone, matching architecture.md's redis.go bullet.
- deployment.md's metrics list presented invalidations_total's `ok`/
`deferred` as if they partition the count; they overlap (a retried
landing counts `ok` again), per metrics.go's own doc comment.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017aS7rLrH1RKkUMem7X4ckd
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017aS7rLrH1RKkUMem7X4ckd # Conflicts: # CHANGELOG.md # docs/src/content/docs/architecture.md
Verified against the merged internal/cache/redis.go: - The failover bullet said the probe gets DialTimeout+Timeout; it's now 2×DialTimeout+Timeout for the first write (rueidis bounds the dial and the handshake by DialTimeout in turn), and a write slower than Timeout is repeated under Timeout alone — only the repeat decides whether the breaker closes, so a server that merely answers slowly stays bypassed instead of flapping open and shut every cycle. - Confirmed the probe is unaffected by, and doesn't affect, the boot/ Close 1s-cap arithmetic in configuration.mdx's dial_timeout row and cache_redis.go's maxRedisTimeout comment: probe() runs in an untracked goroutine (no r.wg.Add before `go r.probe(...)`), so Close's r.wg.Wait() never waits on it. No doc change needed there. - "Run it without persistence" didn't say why: stock Redis and Valkey persist by default (periodic RDB save points), so an ordinary crash- restart reloads the last save on its own — the same rollback as restoring a snapshot by hand. Reworded to match #626's CHANGELOG/ architecture.md wording (grepped for other copies; found none). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017aS7rLrH1RKkUMem7X4ckd
Close can wait up to 4s, not 3s: a dial in flight, then the 1s final drain. deferred counts each deferral once, not failed retries. The probe write closes the breaker only within timeout. The breaker's WARN on opening is documented alongside the credentials ERROR. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FyrXjhR7iDg33paioLQHFq
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FyrXjhR7iDg33paioLQHFq
EricAndrechek
added a commit
that referenced
this pull request
Sep 26, 2026
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FyrXjhR7iDg33paioLQHFq
EricAndrechek
added a commit
that referenced
this pull request
Sep 26, 2026
…614) ## Summary This PR makes the query cache correct under concurrent writes and shareable across instances. It folds in #621, #634, #626 and #630, which were reviewed separately against this branch. - **Version snapshot at lookup (fixes #382).** The `cache.Cache` interface is now `Lookup(ctx, tenant, sha, deps) (Entry, Snapshot, error)` and `Set(ctx, Snapshot, value, ttl)`. A result's versions are read once, before its query runs and before the handler takes the tenant's ClickHouse pool, and the fill is filed under what was read. Previously `POST /v1/query` and pipe execution rebuilt the version-folded key after the query, so an insert that landed mid-query filed pre-insert rows under post-insert versions and they were served as fresh until their TTL. A reload that moves a tenant to another address or database now orphans a fill taken from the old pool the same way. The singleflight key and coalescing are unchanged. - **A tenant token on every key.** Every entry key folds the tenant's version, a pipe's dependency-free key included, so `InvalidateTenant` now drops a returning or moved tenant's cached pipe results too. A `Lookup` whose deps name another tenant is refused (`ErrForeignDependency`). `cache.Namespace` carries raw table and scope names and the cache escapes them itself with `internal/keyenc`, so `query.SafeEncodeToken` is gone and names that would run together under an unescaped join can no longer share a key. - **Flat local version index (part of #262).** `VersionManager` holds one version per tenant, per (tenant, table) and per (tenant, table, scope), bumped in place, so the index no longer grows with every bump. A tenant's version is a process-unique generation: `InvalidateTenant` drops the tenant's index and the next key gets a fresh generation. After every settings reload, `LocalCache.Prune` drops the index of each tenant no longer served. - **Write pipes run uncached (fixes #386).** A pipe whose bound SQL `IsMutation` classifies as a write skips the cache lookup, the fill and singleflight, and runs on every call. Before, a repeat within the TTL answered `200` without writing, and N concurrent identical calls became one write. The classifier now reads the leading keyword the way ClickHouse's lexer does (comments, quoted text, heredocs, the whitespace ClickHouse accepts), classifies a `WITH`-led statement by `INSERT INTO` alone, and looks through `EXECUTE AS`. An integration test checks every case, and every keyword in `system.keywords` in 12 `WITH` shapes, against ClickHouse's own parser. - **A Redis-compatible shared cache backend.** `cache.RedisCache` runs against Redis, Valkey, Dragonfly, ElastiCache and MemoryDB, standalone or cluster, using only `GET`, `SET` and `MGET`. Versions are random 8-byte tokens under the tenant's hash tag, and a lookup is one round trip. A lost token can only cause a miss. Values of 1 KiB or more are zstd-compressed, and stored values are capped at 1 MiB. Every operation has a 100 ms timeout, and a failure is a miss, a skipped fill or a deferred invalidation, never a failed query. A circuit breaker opens after 5 consecutive failures, or at once on a reply that refuses writes (`READONLY`, `OOM`, …) or the credentials, and only a successful probe write closes it. Deferred invalidations are retried until they land, and while a process owes one it bypasses the lookups that invalidation would orphan. Eight `wavehouse_cache_*` metrics, all labeled `backend="redis"`, report hits, round-trip time, breaker state, owed invalidations, value size and failed fills. - **`cache.backend: redis`.** A new `cache.redis` boot-config block (`WH_CACHE_REDIS_*`): `addrs`, `mode` (`standalone` or `cluster`), credentials, `db`, TLS files, `key_prefix`, `timeout` and `dial_timeout` (each capped at `1s`), `max_value_bytes`, `compress_min_bytes` and `version_ttl`. `wireCache` builds the backend from it. It is the shared cache that splitting the `api` and `ingest` roles into separate processes needs, and the boot error for such a split now names it; every split is still refused while the queue is embedded. The e2e suite now runs on Redis. ## Behaviour and compatibility notes - **Write pipes answer `X-Cache: BYPASS` with `Cache-Control: no-store` and are never coalesced.** Each call executes, so identical concurrent calls are that many writes. A failed write keeps the read path's status and `code` but always answers `retryable: false` with no `Retry-After`, since the statement may have run. Read pipes are unchanged. - **A write pipe does not invalidate cached reads of the table it writes** (#394, and #343 for read pipes), and its rows do not reach `/v1/stream` subscribers (#362). - **`InvalidateTenant` drops more than before**: a tenant's cached pipe results as well as its query results. Inserts still do not reach pipe results (#343). - **In-process cache keys changed** (the caller's query key is now escaped inside the entry key). They are in-process only, so a restart is the whole migration. - **Shared-cache token keys are a protocol between builds.** Every process sharing a server reads and bumps them for itself, so a later change to that layout needs a rolling-upgrade plan. A change to value keys only orphans entries and is safe to roll. - **Boot with Redis down or refusing the password succeeds, degraded.** The cache starts bypassed and keeps reconnecting. When a closed breaker opens, it logs one `WARN`, or one `ERROR` for rejected credentials. A failed probe reopening it logs at `DEBUG`, unless it failed for another cause than the one last logged (rejected credentials after a restart, say), which is logged at its own level. A long outage is one line. - **`mode: sentinel` refuses boot** until #656. A URL-style address is refused without echoing it, and a standalone server takes exactly one address. - **The ingest worker logs an invalidation that did not land at `WARN`**, not `ERROR`: the shared backend defers and retries it. `wavehouse_cache_invalidations_pending` is the signal to alert on. - **Run the server with an evicting `maxmemory-policy` and without persistence.** Under `noeviction` a full server refuses the token writes. Restoring a snapshot, or a crash-restart that reloads the last save, is a rollback that serves previously invalidated entries until their TTL. The deployment guide covers both. - **Known follow-ups:** - #662: a quoted placeholder lets a bound value break out of its literal. - #663: a write pipe answers `GET`, which proxies and clients may replay. - #666: `BACKUP`, `RESTORE`, `UNDROP` and `MOVE` pipes are not classified as writes. - #671: `SET`, `USE` and `EXECUTE AS` in a pipe leak into the pooled session. - Also still open: per-table scope cardinality (#262, until #235 populates `scope`), the rest of #664 (the probe reconnects one connection of several), Sentinel (#656), and the near-cache. ## Tests - **Conformance suite** `internal/testutil/cachetest.Run`: miss, hit and TTL, dependency order, tenant isolation, foreign deps, the scope lattice, raw names that would run together, `Invalidate`/`InvalidateTenant`, a bump during the query (#382), oversize values, zero snapshots, and concurrent use under `-race`. Shared backends also get two-instances-over-one-store cases. `LocalCache` runs it, and so does `RedisCache` against pinned Redis, Valkey, Dragonfly and a Redis Cluster node. - **#382**: on both cached routes, a bump from inside the ClickHouse call, and one from inside the pool lookup, each give MISS, MISS, HIT. The read's namespace and the ingest worker's bump are pinned to meet for raw table names. - **Flat index**: 10,000 rounds of interleaved bumps and key reads leave the index at its settled size. Generations never repeat, a table bump drops its scopes, and a bump against a tenant with no index records nothing. A reload prunes the index to the tenants still served. - **Write pipes**: `INSERT`, `WITH … INSERT` and `ALTER … DELETE` pipes each run on every call. Three identical concurrent calls are three writes in flight. A read pipe over a table named like a write verb stays cached. Every row of the ClickHouse error table on a write pipe answers `retryable: false`. Over the Redis-backed e2e stack, a write pipe called twice leaves both rows, and a failed one answers `400 clickhouse.rejected` with `retryable: false`. There are 152 table-driven classifier cases, each also checked against `EXPLAIN AST` on the pinned ClickHouse, and 17,928 keyword-named `WITH` statements where the classifier must agree with the parser. - **Redis backend (integration)**: lost tokens miss, and so does a flushed server. On a paused server, lookups fail within the bound, the breaker opens, invalidations are deferred, and all of it recovers. An owed bump holds its lookups. Refused writes (`READONLY`, `OOM`) open the breaker at once. A failover behind a stable address delivers the owed bump. A cluster topology read is bounded. A slow reconnect still closes the breaker, and a slow server stays bypassed. Rotated credentials open the breaker. A restored snapshot behaves as a rollback. Unit tests cover the key schema, the codec and its zip-bomb refusal, the breaker state machine, pending coalescing and collapse, and the breaker logging each opening once and a changed cause at its own level. - **Config and wiring**: defaults, env, YAML, the validation table, URL-style addresses (neither the address nor the secret is echoed), boot against a closed port (bypassed, not failed), and an unreadable TLS file refusing boot. Two `app.New` instances over one Redis: an ingest on one invalidates the other, and with Redis paused, queries bypass and still succeed. - Most behavioural tests are mutation-checked: each fails with the fix removed. - `make ci` passes: static checks, unit, integration, e2e on Redis, and coverage. Fixes #382. Fixes #386. Part of #262. Part of #613. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01FyrXjhR7iDg33paioLQHFq --------- Co-authored-by: taitelee <taitelee@umich.edu> Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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.
Part of #613. Stacked on #626 (
feat/cache-redis), and it also merges #621 (feat/cache-flat-versions), so until #621 merges its commits show in this diff too.What
cache.backend: redisnow selects the shared cache from #626. It is configured by a newcache.redisboot-config block (WH_CACHE_REDIS_*).cache.redis.addrsWH_CACHE_REDIS_ADDRS(comma-separated)backend: redis;host:portonly (a URL is refused), exactly one instandalonemodecache.redis.modeWH_CACHE_REDIS_MODEstandalone(orcluster;sentinelis refused until #656)cache.redis.username/password/dbWH_CACHE_REDIS_{USERNAME,PASSWORD,DB}clickhouse.passwordcache.redis.tls.{enabled,ca_file,cert_file,key_file,server_name,insecure_skip_verify}WH_CACHE_REDIS_TLS_*cache.redis.key_prefixWH_CACHE_REDIS_KEY_PREFIXwhcache.redis.timeout/dial_timeoutWH_CACHE_REDIS_{TIMEOUT,DIAL_TIMEOUT}100ms/1s, each at most1scache.redis.max_value_bytesWH_CACHE_REDIS_MAX_VALUE_BYTES1048576cache.redis.compress_min_bytesWH_CACHE_REDIS_COMPRESS_MIN_BYTES1024;0= nevercache.redis.version_ttlWH_CACHE_REDIS_VERSION_TTL168hinternal/config/cache_redis.go:CacheRedisConfig+CacheRedisTLS, with the block's checks inCache.validate(). Its defaults live indefaults()with the rest of the boot config (fix(config): keep an explicit false/0/"" from config.yaml #632's convention).CacheRedisTLS.Config()builds the*tls.Configfrom the file paths.Validatecalls it, so an unreadable or unparsable file refuses boot.internal/config/backends.go:CacheRedisis added tocacheBackends.Warnings()gains two lines, emitted by a process with theapirole like the others:tls.insecure_skip_verify, and acache.redis.addrsset while the backend islocal(the block is not read).internal/config/config.go: the refusal ofapiwithoutingest(or the reverse) overcache.backend: local, from feat(app): process roles #622, now namescache.backend=redisas the shared cache to set. Every split is still refused whilemq.backendis embedded.internal/app/wire.go:wireCachehas arediscase.redisConfigmaps the block ontocache.RedisConfig(Loadhas already applied the defaults). Both backends share onecomponent(close →Close) and perf(cache): flat version index, pruned per tenant #621's prune hook, which acts only on aprunerand so skips Redis.NOPERMdeliberately does not.Decisions
timeoutanddial_timeoutare each capped at1s. Boot waits out one dial, and so does the cache'sClosewhen a reconnect is in flight (rueidis.NewClienttakes no context). A dial is a connect plus a handshake, each bounded bydial_timeout. In cluster mode it also includes a topology read, which feat(cache): shared cache backend on Redis, Valkey and Dragonfly #626 bounds bymax(dial_timeout, timeout). At the caps a dial takes at most 3s, inside the 5sReleaseTimeoutthat the cache's close shares with the queue, dedupe and ClickHouse released after it (the breaker's own background probe runs in an untracked goroutine and does not affect this arithmetic). A 2s dial cap would have allowed 6s. The defaults are unchanged.sentinel_masteris removed rather than kept and refused. A key that no config can use would clutter the reference, and the strict loader now reports it (andWH_CACHE_REDIS_SENTINEL_MASTER) as unknown, so nobody sets it believing it works. bug(cache): a demoted Redis primary reads as healthy; Sentinel mode is unconfigured #656 can bring it back together with the sentinel credentials and topology refresh it needs.standalonetakes exactly one address. rueidis's single client dials only the first, so a second one (a replica, say) used to be silently ignored. Several addresses are a cluster's seeds, and a port must be 1–65535.WH_CACHE_REDIS_ADDRS=redis://default:s3cret@redis:6379used to fail with the password quoted twice in the boot error. An address containing://or@is now refused by its position, with a pointer tousername,passwordandtls.enabled.compress_min_bytes: 0means never compress, which is the backend's own meaning. fix(config): keep an explicit false/0/"" from config.yaml #632 fixed bug(config): an explicit false/0 in config.yaml is replaced by the field's env-default #631, so a0inconfig.yamlis kept, and the-1sentinel this PR used to need is gone.cache.RedisConfig's checks (address syntax, mode, clusterdb, prefix braces,version_ttl≥ 2s). This way the errors name theWH_*variable, andinternal/configstays a leaf package with no rueidis import.CacheRedisConfig, since the backend constant is alreadyCacheRedis.tls.enabledis off refuses boot, rather than connecting in plaintext.redis:8.10.2-alpine(tmpfs/data, no persistence).tests/e2e/fixtures/config.yamlsetscache.backend: rediswithtimeout: 1s, so a loaded runner doesn't bypass a lookup that the HIT assertions depend on. The e2e gate's cache-related excludes areinternal/cache/pending.go(the retry of an invalidation the server did not take, which needs an outage),internal/config/cache_redis.go(the block's own rejection paths) andinternal/cache/(local|version_manager).go(thelocalbackend, which e2e no longer runs) — the unit suite covers them, and so does the integration suite, whose own app runscache.backend: localand now asserts its hit, insert and fresh-miss lifecycle (TestLocalCache_IngestInvalidates).InvalidateTenantfailure. The shared backend defers and retries it, so a Redis outage used to log an ERROR for every batch; this only lowers the level, and volume stays at one WARN per batch during an outage.wavehouse_cache_invalidations_pendingis the signal to alert on.Tests
internal/config/cache_redis_test.go,defaults_test.go,roles_test.go):compress_min_bytes: 0kept as never;cache.redis.*zero cases in fix(config): keep an explicit false/0/"" from config.yaml #632's tables, andconfiguration.mdxrows checked againstdefaults();sentinel_masterincluded) and unbound env vars;WH_CACHE_REDIS_ADDRS=;internal/app/app_test.go):backend: redisagainst a closed port. The cache is a*cache.RedisCache, bypassed: a lookup misses and a fill is a no-op. A reload's prune hook skips it.TestRedisConfig_FromLoadedDefaultsdrivesredisConfigfromconfig.Loadand compares the result withcache.Default*. AWH_CACHE_REDIS_COMPRESS_MIN_BYTES=0reaches the backend as 0.TestRedisConfig_UsernameDBTLSMapped(new): the defaults test leavesUsername,DBandTLSat their zero value on both sides, so deleting any of their three mapping lines inredisConfigwouldn't fail it. This one drives all three to a non-zero value throughconfig.Loadand asserts on them directly. Mutation-checked: deleting each of the three lines inredisConfigfails it (theTLSline fails to compile instead, since dropping it leaves a local variable unused — still a build failure).tests/integration/shared_cache_test.go, pinnedredis:8.10.2-alpine):TestSharedCache_IngestOnOneInstanceInvalidatesAnother: twoapp.Newinstances over one Redis and the suite's ClickHouse. Mutation-checked: withRedisCache.Invalidateas a no-op, it fails.TestSharedCache_RedisDownQueriesBypass: Redis paused. Queries succeed as MISS, and a row ingested meanwhile is served. After the pause, queries turn back to HITs.TestLocalCache_IngestInvalidates(new): the same lifecycle on the suite's ownlocalapp. Mutation-checked: withLocalCache.Invalidateas a no-op, it fails (the row landed 10.0s after the fill, every round).make ciis green: unit 91.8%, integration 53.5%, e2e 60.3% (on Redis), Go total 94.9%, ts-total 82.28%.Metrics
These are unchanged from #626. They are emitted from the first process that sets
cache.backend: redis. Meterwavehouse-cache, every series labeledbackend="redis", no tenant label:wavehouse_cache_lookups_total{result},wavehouse_cache_op_duration_seconds{op},wavehouse_cache_breaker_open,wavehouse_cache_invalidations_total{result}(okanddeferredoverlap — a retried landing countsokagain),wavehouse_cache_invalidations_pending,wavehouse_cache_value_bytes,wavehouse_cache_oversize_total,wavehouse_cache_set_failures_total{reason}. No monitored deployment sets the backend yet.Deliberately left to later PRs
cache.redis.near_cache.*). The key is refused as unknown until then.pool_size: not added. feat(cache): shared cache backend on Redis, Valkey and Dragonfly #626 did not implement it, and a knob that does nothing would mislead.backend: redis.🤖 Generated with Claude Code
https://claude.ai/code/session_017aS7rLrH1RKkUMem7X4ckd