feat(clickhouse): one pool per tuple and one schema registry per tenant by taitelee · Pull Request #610 · Wave-RF/WaveHouse

taitelee · 2026-09-24T05:16:03Z

Summary

The second slice of story 6 of the multi-tenant epic: each tenant reads its own ClickHouse. No behavior change for a settings directory that holds the four files beyond the three noted at the end.

One native pool per distinct clickhouse.addr / database / username / password / tls tuple among the served tenants (chconn.Identity, a plain comparable value; chconn.Pools), shared by the tenants naming it and sized to their largest max_open_conns and max_idle_conns. http_port, http_scheme, headers and query_timeout stay each tenant's own: the HTTP target is composed per tenant over the pool's TLS config, and the deadline is per call. Every reload reconciles the pools: a new tuple opens (never dials), a tenant whose tuple changed is repointed, a tuple no tenant names closes after the longest query_timeout among the tenants it had, and a changed largest ask resizes with the same grace.
The ceiling across pools: clickhouse.max_total_conns bounds the open pools' max_open_conns together. Boot refuses naming the sum and the ceiling. At a reload the walk keeps the ceiling at every step, tenants no longer served leaving first and what was refused placed once more at the end: a resize above it is refused and the pool keeps its size; a tuple that cannot be opened — the ceiling, a certificate file that cannot be read, or a pool the driver refuses to open (opened as the walk places it, so every one of these is undone in place) — leaves its tenants on the pool they had, Params and all (the keep-previous-wiring rule of feat(clickhouse): tls, headers, pool sizes and a connection ceiling #603), or on none when they had none. Both are logged and the next reload retries. A tenant on no pool fails closed: 503 with Retry-After: 30 on POST /v1/query, GET/POST /v1/pipes/{name}, POST /v1/ops/query and POST /v1/ops/schema/refresh, ahead of the cache.
One discovery.SchemaRegistry per served tenant over its pool (a discovery.Source: the pool's connection and the database that pool was opened for, read together once per refresh, so a move the ceiling refused keeps discovering the database the tenant's queries and inserts still use), each with a loop of its own: RetryRefresh until the first success, then StartAutoRefresh at the tenant's schema.refresh_interval, first tick at a random point within the interval so tenants adopted together do not refresh together. Created and stopped from AfterAdopt, all stopped under App.Close within the release budget. wavehouse_schema_refresh_failures_total{tenant} counts a loop's failed attempts. The hub and the ingest worker resolve the registry and the HTTP target through tenant-keyed getters called with the tenant each message's topic names (feat(mq): lead every subject with the tenant #609), so the worker inserts each (tenant, table) batch into its own tenant's ClickHouse.
Probes. Flat mode unchanged: tenant 0's synchronous boot refresh drives BootState, /readyz pings the one pool. Nested: /livez (and /v1/health) is 503 with the latest discovery failure, naming its tenant, while no tenant has completed a first discovery, then 200 for the rest of the process lifetime; nested boot never waits on a tenant's ClickHouse. /readyz pings every open pool at once and is ready at the first answer — concurrently, since the driver waits up to its 30 s dial timeout on a host that does not answer, past a kubelet probe's 1 s — and names every pool that did not answer when none does. SchemaRegistry.Lookup answers ErrNotLoaded before the first success, mapped at the three former 404 sites (and the schema list, where [] would read as no tables) to 503 with Retry-After: 5.
GET /v1/ops/schema, POST /v1/ops/schema/refresh and POST /v1/ops/query take the strict ?tenant= the pipe reads take (feat(settings): nested settings directory with a per-tenant registry #598): absent is tenant 0, 400 malformed, 404 unknown, 503 rejected. The SDK sends it as the tenant option of wh.schema.list(), wh.schema.refresh(), wh.from(t).schema() and wh.sql().
sharedTables narrowed: the worker's invalidation fans out to the tenants on the same ClickHouse address and database (Pools.SharingTables), whatever their user or tls block, since they read the same tables. A tenant adopted after an absence — rejected or removed, so out of that fan-out — or moved to another address or database has its cached structured-query results orphaned in one step: Cache.InvalidateTenant, a tenant generation leading every version key (an enumeration of the index would miss a table no bump ever keyed); a pipe result names no table and keeps its TTL, as after any insert (feat(pipes): resolve table dependencies for cache invalidation #343).
HTTPClients keeps one client per TLS config: the proxy now serves tenants on different configs in alternation.
Passwords: Identity carries the field, but until auth(config): where boot secrets live and how they rotate (jwt_secret, operator_key, clickhouse.password) #529 every tenant's user authenticates with WH_CH_PASSWORD, so the tuple is in effect the address, database, username and tls block.
Three changes reach the single-tenant directory: a table lookup before the first discovery is 503 with Retry-After: 5 rather than 404, on POST /v1/ingest, POST /v1/query and GET /v1/ops/schema; the first periodic refresh fires at a random point within the interval rather than a full interval after boot; and a reload that moves clickhouse.addr or clickhouse.database orphans the structured-query results cached before it, where they were served until their TTL.

Test plan

Follow-ups

A served tenant on no pool (refused by the ceiling) has its inserts fail into the existing failure path: parked on the DLQ, or left for redelivery when its dlq.enabled is off, until a reload gives it a pool.
Story 3 owns what a removed tenant's async paths do; a rejected tenant's registry is dropped and rebuilt on repair, so its first lookups after a repair are a sub-second 503.
Per-tenant ClickHouse passwords wait for auth(config): where boot secrets live and how they rotate (jwt_secret, operator_key, clickhouse.password) #529.
A tenant moved to another database keeps its previous schema until its next refresh (schema.refresh_interval, or POST /v1/ops/schema/refresh?tenant=); the stale list the pools reconcile returns is the natural trigger for an immediate refresh.
Over a nested directory, /livez keeps the last boot-time discovery error of a tenant removed before any tenant loaded, until another tenant loads.
The codegen CLI has neither an operator-key option nor a --tenant flag, so it cannot read a nested directory's schemas (documented as unsupported on the SDK reference page).

Related Issues

Part of #583 (story 6).

…tabase

…e lines

…discovery

taitelee · 2026-09-24T05:16:09Z

@coderabbitai review

coderabbitai · 2026-09-24T05:16:12Z

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 229d151b-ad8c-4c91-8c81-ed1049484aca

📥 Commits

Reviewing files that changed from the base of the PR and between 66ee513 and d85621b.

📒 Files selected for processing (1)

CHANGELOG.md

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

📜 Recent review details

🧰 Additional context used

🪛 LanguageTool

CHANGELOG.md

[typographical] ~13-~13: Consider using an em dash in dialogues and enumerations.
Context: - **One ClickHouse pool per tuple and one...

(DASH_RULE)

[style] ~13-~13: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: - One ClickHouse pool per tuple and one schema registry per tenant (internal/chconn/chconn.go (+ tests), internal/discovery/discovery.go (+ tests), internal/app/discoveries.go (new), internal/app/{app,wire}.go (+ tests), internal/api/{schema,ingest,structured_query,pipes,query,health,errors,clickhouse_exec}.go (+ tests), internal/stream/hub.go, internal/ingest/worker.go, internal/cache/{cache,local,version_manager}.go (+ tests), internal/testutil/{testutil,mocks}.go, tests/integration/{setup,tenants,boot_resilience,query_limits}_test.go, clients/ts/src/{schema,table,sql,client,types}.ts (+ tests), tests/e2e/sdk/admin.test.ts, docs/src/content/docs/{api,deployment,architecture,ingest-pipeline}.md, docs/src/content/docs/{settings-directory,configuration,access-control,reverse-proxy}.mdx, docs/src/content/docs/sdk/{admin,reference,queries}.md, AGENTS.md): the second slice of story 6 of the multi-tenant epic (#583), with no behavior change for a settings directory that holds the four files beyond the three noted at the end. The process opens one native pool per d...

(TOO_LONG_SENTENCE)

[style] ~13-~13: Since ownership is already implied, this phrasing may be redundant.
Context: ...of its own over its pool, kept fresh by its own loop — the boot retry until the first s...

(PRP_OWN)

[style] ~13-~13: Since ownership is already implied, this phrasing may be redundant.
Context: ...e ingest worker inserts each batch into its own tenant's ClickHouse — the tenant the me...

(PRP_OWN)

📝 Summary

Summary by CodeRabbit

New Features
- API and TypeScript SDK requests can target a tenant’s ClickHouse and schema; requests without a tenant use the default.
- Tenants with matching connection settings can share a ClickHouse pool. Pools are reconciled on settings reload and subject to a combined connection limit.
- Schema discovery and query-cache invalidation are tenant-scoped. Inserts also invalidate caches for tenants sharing the same ClickHouse address and database.
- Readiness checks probe all open ClickHouse pools and succeed when any pool responds.
Bug Fixes
- Schema-aware requests return 503 with a retry hint while a tenant’s schema is being discovered or when it has no available connection.
Documentation
- Updated API, SDK, configuration, and deployment guides to explain tenant routing, connection pools, and health-check behavior.

Walkthrough

The change replaces a process-wide ClickHouse connection and schema registry with tenant-aware pools and schema registries. API handlers and ingest resolve resources by tenant. The TypeScript SDK accepts tenant options for schema and SQL operations.

Changes

Per-tenant resources and request routing

Layer / File(s)	Summary
Pool identity and reconciliation `internal/chconn/chconn.go`, `internal/app/app.go`, `internal/app/wire.go`, `internal/app/app_test.go`, `internal/chconn/chconn_test.go`	Pools are shared by tenants with matching connection identity tuples. Reconciliation handles pool resizing, tenant changes, and the aggregate connection ceiling.
Tenant schema discovery `internal/discovery/discovery.go`, `internal/app/discoveries.go`, `internal/app/wire.go`, `internal/discovery/_test.go`, `tests/integration/`	Each served tenant gets a schema registry and discovery loop. The registry tracks whether an initial refresh succeeded, and periodic refresh starts at a randomized point within its interval.
Tenant-aware API handling `internal/api/.go`, `internal/api/_test.go`	Handlers resolve tenant settings, schema registries, and ClickHouse resources. Schema-not-loaded and unavailable-pool cases return `503` responses with `Retry-After` headers.
Cache, ingest, and stream paths `internal/cache/*`, `internal/ingest/worker.go`, `internal/stream/hub.go`, `internal/app/wire.go`	Cache versions support tenant-wide invalidation. Ingest chooses a target by tenant, and stream schema lookups use the event tenant.
Integration and lifecycle validation `internal/app/app_test.go`, `tests/integration/tenants_test.go`, `tests/integration/setup_test.go`, `tests/integration/boot_resilience_test.go`	Tests cover pool reconciliation and shutdown, discovery readiness, tenant-specific schemas, and SDK tenant selection.

SDK and documentation

Layer / File(s)	Summary
Tenant options and architecture documentation `clients/ts/src/`, `docs/src/content/docs/`, `AGENTS.md`, `CHANGELOG.md`	SDK schema and SQL operations accept tenant options. Documentation covers tenant routing, pool and discovery behavior, API errors, and health probes.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~60 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant OpsHandler
  participant TenantRegistry
  participant SchemaRegistry
  participant ClickHousePool
  Client->>OpsHandler: Send request with optional tenant
  OpsHandler->>TenantRegistry: Resolve tenant settings
  TenantRegistry-->>OpsHandler: Return selected settings store
  OpsHandler->>SchemaRegistry: Resolve tenant schema
  SchemaRegistry-->>OpsHandler: Return schema or discovery status
  OpsHandler->>ClickHousePool: Execute against tenant pool
  ClickHousePool-->>OpsHandler: Return query result or pool error
  OpsHandler-->>Client: Return response

Suggested reviewers: ericandrechek

Merge Risk: ⚪ Minimal · up to d8562

This update changes changelog text only; no concrete impact to tenant routing or service behavior is established.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 65.13% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 261 functions across 50 files. (1 skipped…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely identifies the two main changes: one ClickHouse pool per connection tuple and one schema registry per tenant.
Description check	✅ Passed	The description directly explains the per-tenant pools, schema registries, reload behavior, API changes, health probes, cache invalidation, tests, and follow-ups.

Full details: Docstring Coverage

Explanation

Docstring coverage is 65.13% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 261 functions across 50 files. (1 skipped: 1 unsupported.)

Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

✨ Simplify code

Commit to this branch
Create a new PR

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai · 2026-09-24T05:16:16Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

github-actions · 2026-09-24T05:18:41Z

📚 Docs preview is live → https://ac36cdd1-wavehouse-docs.wave-rf.workers.dev

Commit — d85621b: docs(changelog): a flat directory's address or database move orphans its cache
Author — @taitelee
Committed — 2026-09-24 05:24 (UTC-04:00)
Deployed — 2026-09-24 08:53 EDT

github-code-quality · 2026-09-24T05:19:30Z

Code Coverage Overview

Languages: Go

Go

The overall line coverage in commit d85621b in the ch-per-tenant branch remains at 93%, unchanged from commit 82b4199 in the main branch.

Show a line coverage summary of the most impacted files.

File	`main` 82b4199	`ch-per-tenant` d85621b	+/-
`internal/stream/hub.go`	97%	97%	0%
`internal/discov...ry/discovery.go`	96%	96%	0%
`internal/api/query.go`	90%	90%	0%
`internal/api/pipes.go`	91%	91%	0%
`internal/cache/local.go`	91%	91%	0%
`internal/api/cl...ckhouse_exec.go`	84%	84%	0%
`internal/app/wire.go`	91%	92%	+1%
`internal/api/schema.go`	89%	93%	+4%
`internal/chconn/chconn.go`	83%	92%	+9%
`internal/app/discoveries.go`	0%	97%	+97%

_{Updated September 24, 2026 12:53 UTC}

coderabbitai

Actionable comments posted: 5

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: e0b93ca8-248e-4b52-bbe3-406f5400383a

📥 Commits

Reviewing files that changed from the base of the PR and between 82b4199 and 821d365.

📒 Files selected for processing (69)

AGENTS.md
CHANGELOG.md
clients/ts/src/client.ts
clients/ts/src/namespaces.test.ts
clients/ts/src/schema.ts
clients/ts/src/sql.ts
clients/ts/src/table.test.ts
clients/ts/src/table.ts
clients/ts/src/types.ts
docs/src/content/docs/access-control.mdx
docs/src/content/docs/api.md
docs/src/content/docs/architecture.md
docs/src/content/docs/configuration.mdx
docs/src/content/docs/deployment.md
docs/src/content/docs/development.md
docs/src/content/docs/ingest-pipeline.md
docs/src/content/docs/reverse-proxy.mdx
docs/src/content/docs/sdk/admin.md
docs/src/content/docs/sdk/queries.md
docs/src/content/docs/sdk/reference.md
docs/src/content/docs/settings-directory.mdx
internal/api/boot_chain_test.go
internal/api/cache_key.go
internal/api/cache_tenant_test.go
internal/api/clickhouse_exec.go
internal/api/errors.go
internal/api/errors_test.go
internal/api/health.go
internal/api/health_test.go
internal/api/ingest.go
internal/api/ingest_seams_test.go
internal/api/ingest_test.go
internal/api/pipes.go
internal/api/pipes_test.go
internal/api/query.go
internal/api/query_test.go
internal/api/router_test.go
internal/api/schema.go
internal/api/schema_test.go
internal/api/structured_query.go
internal/api/structured_query_test.go
internal/api/tenant_clickhouse_test.go
internal/api/tenant_helpers_test.go
internal/api/tenant_test.go
internal/app/app.go
internal/app/app_test.go
internal/app/discoveries.go
internal/app/wire.go
internal/cache/cache.go
internal/cache/local.go
internal/cache/local_test.go
internal/cache/version_manager.go
internal/cache/version_manager_test.go
internal/chconn/chconn.go
internal/chconn/chconn_test.go
internal/discovery/discovery.go
internal/discovery/discovery_test.go
internal/discovery/timestamp_test.go
internal/ingest/worker.go
internal/ingest/worker_test.go
internal/stream/hub.go
internal/stream/hub_test.go
internal/testutil/mocks.go
internal/testutil/testutil.go
tests/e2e/sdk/admin.test.ts
tests/integration/boot_resilience_test.go
tests/integration/query_limits_test.go
tests/integration/setup_test.go
tests/integration/tenants_test.go

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

📜 Review details

⏰ Context from checks skipped due to timeout. (11)

GitHub Check: Integration tests
GitHub Check: Coverage
GitHub Check: Unit tests
GitHub Check: Docs build
GitHub Check: E2E tests
GitHub Check: Lint
GitHub Check: Analyze (actions)
GitHub Check: Analyze (go)
GitHub Check: Analyze (javascript-typescript)
GitHub Check: Analyze (javascript-typescript)
GitHub Check: Analyze (go)

🧰 Additional context used

📓 Path-based instructions (5)

See [AGENTS.md](AGENTS.md) for project conventions, architecture notes, and AI agent instructions.

📄 CodeRabbit inference engine (CLAUDE.md)

Files:

AGENTS.md

Create `*_test.go` files in the same package as the code under test.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

internal/api/pipes_test.go
internal/api/health_test.go
tests/integration/setup_test.go
internal/api/errors_test.go
internal/api/boot_chain_test.go
internal/api/cache_tenant_test.go
internal/discovery/timestamp_test.go
internal/cache/version_manager_test.go
internal/cache/local_test.go
internal/api/tenant_helpers_test.go
internal/api/tenant_test.go
internal/api/ingest_seams_test.go
internal/api/structured_query_test.go
internal/api/query_test.go
internal/api/schema_test.go
tests/integration/tenants_test.go
tests/integration/boot_resilience_test.go
tests/integration/query_limits_test.go
internal/chconn/chconn_test.go
internal/api/router_test.go
internal/ingest/worker_test.go
internal/stream/hub_test.go
internal/api/ingest_test.go
internal/api/tenant_clickhouse_test.go
internal/discovery/discovery_test.go
internal/app/app_test.go

See [AGENTS.md](../AGENTS.md) for project conventions, architecture notes, and AI agent instructions.

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Files:

AGENTS.md

Create the package under `internal/`.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

internal/api/pipes_test.go
internal/api/health_test.go
internal/api/cache_key.go
internal/api/ingest.go
internal/api/errors_test.go
internal/api/boot_chain_test.go
internal/api/cache_tenant_test.go
internal/discovery/timestamp_test.go
internal/cache/version_manager_test.go
internal/testutil/mocks.go
internal/cache/local_test.go
internal/api/tenant_helpers_test.go
internal/api/tenant_test.go
internal/api/clickhouse_exec.go
internal/api/ingest_seams_test.go
internal/api/errors.go
internal/cache/cache.go
internal/cache/local.go
internal/api/structured_query_test.go
internal/api/health.go
internal/api/query_test.go
internal/ingest/worker.go
internal/app/wire.go
internal/cache/version_manager.go
internal/stream/hub.go
internal/api/schema_test.go
internal/app/discoveries.go
internal/testutil/testutil.go
internal/api/schema.go
internal/discovery/discovery.go
internal/chconn/chconn_test.go
internal/api/router_test.go
internal/app/app.go
internal/api/query.go
internal/ingest/worker_test.go
internal/stream/hub_test.go
internal/api/ingest_test.go
internal/api/structured_query.go
internal/chconn/chconn.go
internal/api/pipes.go
internal/api/tenant_clickhouse_test.go
internal/discovery/discovery_test.go
internal/app/app_test.go

In MDX, leave a blank line between a JSX tag and a code fence.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

docs/src/content/docs/reverse-proxy.mdx
docs/src/content/docs/configuration.mdx
docs/src/content/docs/access-control.mdx
docs/src/content/docs/settings-directory.mdx

🧠 Learnings (2)

📚 Learning: 2026-06-26T12:23:22.696Z

Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 346
File: internal/stream/subscriber_test.go:9-28
Timestamp: 2026-06-26T12:23:22.696Z
Learning: In this Go repository, prefer table-driven tests (e.g., `[]struct{...}` with `t.Run(...)`) only for tests that cover multiple scenarios/inputs and can be cleanly enumerated. Do not artificially rewrite a clear single-scenario sequential behavioral-flow test into a table-driven form just to fit the pattern; if there’s only one meaningful scenario, keep the test as a straightforward linear flow (as in `TestSubscriber_SendDeliversThenDropsWhenFull`).

Applied to files:

internal/ingest/worker_test.go

📚 Learning: 2026-05-23T01:23:59.268Z

Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 174
File: internal/api/ingest_test.go:111-111
Timestamp: 2026-05-23T01:23:59.268Z
Learning: In WaveHouse Go tests in internal/api/**/*_test.go, use internal/testutil.AssertJSONErrorResponse(t, w) for HTTP error-path JSON assertions. Do not use (or reintroduce) package-local assertJSONErrorResponse helpers. AssertJSONErrorResponse verifies the response Content-Type is application/json, includes the X-Content-Type-Options: nosniff header, and that the JSON body contains an "error" field.

Applied to files:

internal/api/tenant_clickhouse_test.go

🪛 ast-grep (0.45.3)

tests/integration/tenants_test.go

[error] 43-43: SQL query is built by concatenating a string literal with a variable and passed to a database/sql call (Query, Exec, QueryRow, Prepare, or their Context variants). String concatenation lets attacker-controlled input alter the query structure, enabling SQL injection. Use parameterized queries with placeholders ('?' or '') and pass the values as separate arguments instead of concatenating them into the query string.
Context: e.chConn.Exec(ctx, "CREATE DATABASE IF NOT EXISTS "+db)
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').

(sql-injection-query-string-concat-go)

[error] 44-44: SQL query is built by concatenating a string literal with a variable and passed to a database/sql call (Query, Exec, QueryRow, Prepare, or their Context variants). String concatenation lets attacker-controlled input alter the query structure, enabling SQL injection. Use parameterized queries with placeholders ('?' or '') and pass the values as separate arguments instead of concatenating them into the query string.
Context: e.chConn.Exec(context.Background(), "DROP DATABASE IF EXISTS "+db)
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').

(sql-injection-query-string-concat-go)

[warning] 45-45: Detected a SQL statement built with 'fmt.Sprintf' and passed directly to 'db.Exec'/'db.ExecContext'. Interpolating values into a query string lets an attacker inject arbitrary SQL. Use parameterized queries instead: pass the SQL with placeholders ('?' or '') as the query argument and supply the values as separate arguments, e.g. 'db.Exec("UPDATE t SET x = ? WHERE id = ?", x, id)'.
Context: e.chConn.Exec(ctx, fmt.Sprintf("CREATE TABLE %s.events (%s) ENGINE = MergeTree() ORDER BY id", db, columns[id]))
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').

(sql-injection-exec-sprintf-go)

[warning] 46-46: Detected a SQL statement built with 'fmt.Sprintf' and passed directly to 'db.Exec'/'db.ExecContext'. Interpolating values into a query string lets an attacker inject arbitrary SQL. Use parameterized queries instead: pass the SQL with placeholders ('?' or '') as the query argument and supply the values as separate arguments, e.g. 'db.Exec("UPDATE t SET x = ? WHERE id = ?", x, id)'.
Context: e.chConn.Exec(ctx, fmt.Sprintf("INSERT INTO %s.events (id) VALUES ('1')", db))
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').

(sql-injection-exec-sprintf-go)

🪛 Betterleaks (1.8.1)

internal/api/query_test.go

[high] 546-546: Detected a potential hardcoded password literal, which may expose account credentials.

(generic-password)

[high] 571-571: Detected a potential hardcoded password literal, which may expose account credentials.

(generic-password)

🪛 golangci-lint (2.13.2)

internal/app/discoveries.go

[error] 98-98: directive //nolint:gosec // G118: held on the tenantDiscovery, called by reconcile or close is unused for linter "gosec"

(nolintlint)

🪛 LanguageTool

docs/src/content/docs/access-control.mdx

[style] ~216-~216: Since ownership is already implied, this phrasing may be redundant.
Context: ...ad the same tables. Scoping a caller to its own rows stays the policy's job, from a val...

(PRP_OWN)

docs/src/content/docs/settings-directory.mdx

[style] ~197-~197: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...a ClickHouse behind a private authority needs ca_file once for both. The certificat...

(EN_REPEATEDWORDS_NEED)

[style] ~203-~203: Since ownership is already implied, this phrasing may be redundant.
Context: ...discovered from its own database over its own pool, on its own `schema.refresh_interv...

(PRP_OWN)

CHANGELOG.md

[typographical] ~13-~13: Consider using an em dash in dialogues and enumerations.
Context: - **One ClickHouse pool per tuple and one...

(DASH_RULE)

[style] ~13-~13: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: - One ClickHouse pool per tuple and one schema registry per tenant (internal/chconn/chconn.go (+ tests), internal/discovery/discovery.go (+ tests), internal/app/discoveries.go (new), internal/app/{app,wire}.go (+ tests), internal/api/{schema,ingest,structured_query,pipes,query,health,errors,clickhouse_exec}.go (+ tests), internal/stream/hub.go, internal/ingest/worker.go, internal/cache/{cache,local,version_manager}.go (+ tests), internal/testutil/{testutil,mocks}.go, tests/integration/{setup,tenants,boot_resilience,query_limits}_test.go, clients/ts/src/{schema,table,sql,client,types}.ts (+ tests), tests/e2e/sdk/admin.test.ts, docs/src/content/docs/{api,deployment,architecture,ingest-pipeline}.md, docs/src/content/docs/{settings-directory,configuration,access-control,reverse-proxy}.mdx, docs/src/content/docs/sdk/{admin,reference,queries}.md, AGENTS.md): the second slice of story 6 of the multi-tenant epic (#583), with no behavior change for a settings directory that holds the four files beyond the two noted at the end. The process opens one native pool per d...

(TOO_LONG_SENTENCE)

[style] ~13-~13: Since ownership is already implied, this phrasing may be redundant.
Context: ...of its own over its pool, kept fresh by its own loop — the boot retry until the first s...

(PRP_OWN)

[style] ~13-~13: Since ownership is already implied, this phrasing may be redundant.
Context: ...e ingest worker inserts each batch into its own tenant's ClickHouse — the tenant the me...

(PRP_OWN)

AGENTS.md

[style] ~43-~43: Consider using the typographical ellipsis character here instead.
Context: ...equest; settings.Store in production, Static(q...) in tests) - policy/ — Hasura-st...

(ELLIPSIS)

[style] ~47-~47: Since ownership is already implied, this phrasing may be redundant.
Context: ... table — and evaluates each event under its own tenant's policy and schema registry; `B...

(PRP_OWN)

docs/src/content/docs/deployment.md

[style] ~388-~388: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...t the parameter — and on SIGHUP — the whole directory is reloaded and mirrors its f...

(EN_REPEATEDWORDS_WHOLE)

[style] ~392-~392: Since ownership is already implied, this phrasing may be redundant.
Context: ... does.** A request is evaluated against its own tenant's policies.json and `pipes.jso...

(PRP_OWN)

[style] ~392-~392: Since ownership is already implied, this phrasing may be redundant.
Context: ...s-directory#clickhouse) — and discovers its own tables from its own database on its own...

(PRP_OWN)

[style] ~392-~392: Since ownership is already implied, this phrasing may be redundant.
Context: ...se) — and discovers its own tables from its own database on its own `schema.refresh_int...

(PRP_OWN)

[style] ~392-~392: Since ownership is already implied, this phrasing may be redundant.
Context: .../v1/streamconnection is authorized by its own tenant'spolicies.json` and receives i...

(PRP_OWN)

[style] ~392-~392: Since ownership is already implied, this phrasing may be redundant.
Context: ...n tenant's policies.json and receives its own tenant's rows alone, the ingest worker ...

(PRP_OWN)

[style] ~392-~392: Since ownership is already implied, this phrasing may be redundant.
Context: ...e, the ingest worker inserts a row into its own tenant's ClickHouse, a failed row is pa...

(PRP_OWN)

[style] ~392-~392: Since ownership is already implied, this phrasing may be redundant.
Context: ...lickHouse, a failed row is parked under its own tenant's dlq.enabled and subject (`dl...

(PRP_OWN)

[style] ~394-~394: Since ownership is already implied, this phrasing may be redundant.
Context: ... while every other tenant's routes keep their own list. Tenant 0's own dedupe store clo...

(PRP_OWN)

docs/src/content/docs/architecture.md

[grammar] ~85-~85: Ensure spelling is correct
Context: ...ave-RF/WaveHouse/issues/319)). Gap-fill replay (mq.Replayer.ReplaySince on the conne...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

[style] ~92-~92: This phrase is redundant. Consider writing “last”.
Context: ...nt. The SIGHUP registration is released last of all. Handler, Registry, and MQ expose...

(LAST_OF_ALL)

[style] ~92-~92: Since ownership is already implied, this phrasing may be redundant.
Context: ...ons.Listenerlets one serve the API on its own listener instead ofserver.port`. - **...

(PRP_OWN)

[style] ~114-~114: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...e tenant simply carries the 0 prefix. BumpTenant (behind InvalidateTenant) advances the tenant version that leads every namespace key of one tenant, orphaning its every namespace, and every cached query keyed by one, in one step (a pipe result names no table and keeps its TTL) — a table no bump ever keyed included, which an enumeration of the index would miss — for a tenant back on a pool after an absence from the fan-out, or moved to another address or database (story 6). The index is per tenant; the cross-...

(TOO_LONG_SENTENCE)

[typographical] ~132-~132: Consider using an em dash in dialogues and enumerations.
Context: - discovery.go — SchemaRegistry, on...

(DASH_RULE)

[style] ~132-~132: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...e SSE announcement are both built from. Each refresh also records the server version (SELECT version()), joins system.tables for each table's create_table_query (kept in-process as TableSchema.DDL and marked json:"-" — an external-engine table renders its wiring in that statement — endpoint, bucket/host, database, username, access key id — so it must never reach /v1/ops/schema; ClickHouse masks the password as [HIDDEN] from ~23.9, so what is withheld here is the topology), reads each column's default_expression and 1-based position alongside its type, discovers the server's default time zone (SELECT timezone()) and bakes every DateTime/DateTime64 column's canonicalization spec (precision + resolved zone) into the cached schema, so the per-record ingest path parses no type strings and loads no zones (#372). Lookup tells the two misses apart ...

(TOO_LONG_SENTENCE)

docs/src/content/docs/api.md

[style] ~271-~271: Consider using the typographical ellipsis character here instead.
Context: ...er sees the ClickHouse schema | | 404 | {"error":"unknown table: ..."} | Table not found in the tenant's di...

(ELLIPSIS)

🔇 Additional comments (32)

internal/chconn/chconn_test.go (1)

26-99: LGTM!

Also applies to: 134-244, 294-304, 307-668, 679-697

internal/app/app.go (1)

49-49: LGTM!

Also applies to: 93-107, 300-303

internal/app/wire.go (1)

190-214: LGTM!

Also applies to: 281-377, 393-450, 632-632, 672-672, 845-884

internal/app/app_test.go (1)

629-690: LGTM!

Also applies to: 1233-1476

tests/integration/setup_test.go (1)

48-48: LGTM!

Also applies to: 196-196, 214-268

internal/discovery/discovery.go (1)

5-38: LGTM!

Also applies to: 198-242, 252-350, 360-361, 399-426, 467-479, 498-544

internal/discovery/discovery_test.go (1)

21-24: LGTM!

Also applies to: 57-60, 106-180, 677-709

internal/discovery/timestamp_test.go (1)

304-304: LGTM!

internal/api/boot_chain_test.go (1)

90-90: LGTM!

internal/testutil/testutil.go (1)

31-32: LGTM!

tests/integration/boot_resilience_test.go (1)

69-69: LGTM!

Also applies to: 76-76, 91-92, 126-126

internal/api/structured_query.go (1)

23-33: LGTM!

Also applies to: 54-59, 84-85, 162-170, 197-197, 231-231

internal/api/clickhouse_exec.go (1)

11-11: LGTM!

Also applies to: 15-32

internal/api/errors.go (1)

27-45: LGTM!

internal/api/health.go (1)

4-4: LGTM!

Also applies to: 14-15, 47-50, 59-60, 88-89

internal/api/health_test.go (1)

68-68: LGTM!

internal/api/ingest.go (1)

40-41: LGTM!

Also applies to: 68-68, 171-175

internal/api/pipes.go (1)

31-39: LGTM!

Also applies to: 49-49, 155-163, 183-183, 188-188

internal/api/pipes_test.go (1)

44-44: LGTM!

internal/api/query.go (1)

16-16: LGTM!

Also applies to: 47-61, 121-121, 164-168, 211-215

internal/api/schema.go (1)

5-5: LGTM!

Also applies to: 9-9, 12-58, 60-99, 101-124

internal/api/structured_query_test.go (1)

45-45: LGTM!

Also applies to: 294-294

internal/api/tenant_clickhouse_test.go (1)

1-299: LGTM!

internal/api/tenant_helpers_test.go (1)

10-10: LGTM!

Also applies to: 14-14, 55-72

internal/api/tenant_test.go (1)

135-135: LGTM!

Also applies to: 140-140, 146-146, 194-194, 216-216, 221-221

internal/api/cache_tenant_test.go (1)

71-81: LGTM!

Also applies to: 106-108

internal/api/errors_test.go (1)

157-157: LGTM!

Also applies to: 188-188, 192-192

internal/api/ingest_test.go (1)

63-63: LGTM!

Also applies to: 84-84, 137-137, 160-160

internal/api/query_test.go (1)

20-20: LGTM!

Also applies to: 33-47, 53-53, 130-130, 151-151, 374-374, 454-454, 494-494, 544-550, 570-572

internal/api/router_test.go (1)

341-346: LGTM!

Also applies to: 523-527, 589-593, 666-670, 734-738, 778-782, 856-860, 881-885, 981-985

internal/api/schema_test.go (1)

22-23: LGTM!

Also applies to: 44-45, 63-64

internal/api/ingest_seams_test.go (1)

51-51: LGTM!

Also applies to: 69-69, 88-88, 132-132, 171-171, 201-201, 227-227

coderabbitai

Actionable comments posted: 2

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 65d875ab-a75f-4e22-8886-6db260818118

📥 Commits

Reviewing files that changed from the base of the PR and between 9b58aea and 0867ec6.

📒 Files selected for processing (15)

AGENTS.md
docs/src/content/docs/api.md
docs/src/content/docs/architecture.md
docs/src/content/docs/deployment.md
docs/src/content/docs/sdk/admin.md
docs/src/content/docs/settings-directory.mdx
internal/api/errors.go
internal/api/pipes.go
internal/api/schema.go
internal/api/structured_query.go
internal/api/tenant_clickhouse_test.go
internal/app/wire.go
internal/cache/version_manager.go
internal/chconn/chconn.go
internal/ingest/worker_test.go

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

📜 Review details

🧰 Additional context used

📓 Path-based instructions (7)

See [AGENTS.md](AGENTS.md) for project conventions, architecture notes, and AI agent instructions.

📄 CodeRabbit inference engine (CLAUDE.md)

Files:

AGENTS.md

In MDX, leave a blank line between a JSX tag and a code fence.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

docs/src/content/docs/settings-directory.mdx

Create `*_test.go` files in the same package as the code under test.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

internal/api/tenant_clickhouse_test.go
internal/ingest/worker_test.go

See [AGENTS.md](../AGENTS.md) for project conventions, architecture notes, and AI agent instructions.

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Files:

AGENTS.md

Create the package under `internal/`.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

internal/api/errors.go
internal/api/tenant_clickhouse_test.go
internal/api/schema.go
internal/api/pipes.go
internal/ingest/worker_test.go
internal/app/wire.go
internal/cache/version_manager.go
internal/api/structured_query.go
internal/chconn/chconn.go

WH001 applies to every tracked Markdown file, with no carve-out

📄 CodeRabbit inference engine (AGENTS.md)

Files:

docs/src/content/docs/sdk/admin.md
docs/src/content/docs/settings-directory.mdx
AGENTS.md
docs/src/content/docs/deployment.md
docs/src/content/docs/architecture.md
docs/src/content/docs/api.md

Editors see WH001 in `.md` only.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

docs/src/content/docs/sdk/admin.md
AGENTS.md
docs/src/content/docs/deployment.md
docs/src/content/docs/architecture.md
docs/src/content/docs/api.md

🪛 LanguageTool

docs/src/content/docs/settings-directory.mdx

[style] ~197-~197: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...a ClickHouse behind a private authority needs ca_file once for both. The certificat...

(EN_REPEATEDWORDS_NEED)

[style] ~203-~203: Since ownership is already implied, this phrasing may be redundant.
Context: ...discovered from its own database over its own pool, on its own `schema.refresh_interv...

(PRP_OWN)

docs/src/content/docs/deployment.md

[style] ~392-~392: Since ownership is already implied, this phrasing may be redundant.
Context: ... does.** A request is evaluated against its own tenant's policies.json and `pipes.jso...

(PRP_OWN)

[style] ~392-~392: Since ownership is already implied, this phrasing may be redundant.
Context: ...s-directory#clickhouse) — and discovers its own tables from its own database on its own...

(PRP_OWN)

[style] ~392-~392: Since ownership is already implied, this phrasing may be redundant.
Context: ...se) — and discovers its own tables from its own database on its own `schema.refresh_int...

(PRP_OWN)

[style] ~392-~392: Since ownership is already implied, this phrasing may be redundant.
Context: .../v1/streamconnection is authorized by its own tenant'spolicies.json` and receives i...

(PRP_OWN)

[style] ~392-~392: Since ownership is already implied, this phrasing may be redundant.
Context: ...n tenant's policies.json and receives its own tenant's rows alone, the ingest worker ...

(PRP_OWN)

[style] ~392-~392: Since ownership is already implied, this phrasing may be redundant.
Context: ...e, the ingest worker inserts a row into its own tenant's ClickHouse, a failed row is pa...

(PRP_OWN)

[style] ~392-~392: Since ownership is already implied, this phrasing may be redundant.
Context: ...lickHouse, a failed row is parked under its own tenant's dlq.enabled and subject (`dl...

(PRP_OWN)

docs/src/content/docs/architecture.md

[style] ~92-~92: This phrase is redundant. Consider writing “last”.
Context: ...nt. The SIGHUP registration is released last of all. Handler, Registry, and MQ expose...

(LAST_OF_ALL)

[style] ~92-~92: Since ownership is already implied, this phrasing may be redundant.
Context: ...ons.Listenerlets one serve the API on its own listener instead ofserver.port`. - **...

(PRP_OWN)

[typographical] ~132-~132: Consider using an em dash in dialogues and enumerations.
Context: - discovery.go — SchemaRegistry, on...

(DASH_RULE)

[style] ~132-~132: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...e SSE announcement are both built from. Each refresh also records the server version (SELECT version()), joins system.tables for each table's create_table_query (kept in-process as TableSchema.DDL and marked json:"-" — an external-engine table renders its wiring in that statement — endpoint, bucket/host, database, username, access key id — so it must never reach /v1/ops/schema; ClickHouse masks the password as [HIDDEN] from ~23.9, so what is withheld here is the topology), reads each column's default_expression and 1-based position alongside its type, discovers the server's default time zone (SELECT timezone()) and bakes every DateTime/DateTime64 column's canonicalization spec (precision + resolved zone) into the cached schema, so the per-record ingest path parses no type strings and loads no zones (#372). Lookup tells the two misses apart ...

(TOO_LONG_SENTENCE)

🔇 Additional comments (9)

internal/chconn/chconn.go (1)

412-414: LGTM!

Also applies to: 585-586

internal/app/wire.go (1)

198-203: LGTM!

Also applies to: 292-295, 352-354

internal/api/pipes.go (1)

156-157: LGTM!

internal/api/structured_query.go (1)

163-164: LGTM!

internal/api/tenant_clickhouse_test.go (1)

110-113: LGTM!

internal/api/errors.go (1)

29-31: LGTM!

internal/api/schema.go (1)

102-104: LGTM!

docs/src/content/docs/sdk/admin.md (2)

31-31: Scope the admin-role guidance to flat directories.

Line 23 gives the nested-directory exception, but this note still says an admin-role token can authorize schema calls. Nested /v1/ops/* requests return 403 for that token. Limit the JWT advice to flat directories and retain the operator-key guidance for nested directories.

23-23: 🗄️ Data Integrity & Integration

The shared request function retries every 503 response with Retry-After while attempts remain. Both schema.list() and schema.refresh() use this function, so the documented retry claim is supported.

taitelee · 2026-09-24T09:10:36Z

@coderabbitai review

coderabbitai · 2026-09-24T09:10:44Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai

Actionable comments posted: 1

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: bacee61f-5076-44cb-9364-42bec7731efa

📥 Commits

Reviewing files that changed from the base of the PR and between 0867ec6 and 66ee513.

📒 Files selected for processing (7)

CHANGELOG.md
docs/src/content/docs/architecture.md
docs/src/content/docs/deployment.md
internal/app/wire.go
internal/cache/cache.go
internal/chconn/chconn.go
internal/chconn/chconn_test.go

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

📜 Review details

🧰 Additional context used

🪛 LanguageTool

docs/src/content/docs/deployment.md

[style] ~392-~392: Since ownership is already implied, this phrasing may be redundant.
Context: ... does.** A request is evaluated against its own tenant's policies.json and `pipes.jso...

(PRP_OWN)

[style] ~392-~392: Since ownership is already implied, this phrasing may be redundant.
Context: ...s-directory#clickhouse) — and discovers its own tables from its own database on its own...

(PRP_OWN)

[style] ~392-~392: Since ownership is already implied, this phrasing may be redundant.
Context: ...se) — and discovers its own tables from its own database on its own `schema.refresh_int...

(PRP_OWN)

[style] ~392-~392: Since ownership is already implied, this phrasing may be redundant.
Context: .../v1/streamconnection is authorized by its own tenant'spolicies.json` and receives i...

(PRP_OWN)

[style] ~392-~392: Since ownership is already implied, this phrasing may be redundant.
Context: ...n tenant's policies.json and receives its own tenant's rows alone, the ingest worker ...

(PRP_OWN)

[style] ~392-~392: Since ownership is already implied, this phrasing may be redundant.
Context: ...e, the ingest worker inserts a row into its own tenant's ClickHouse, a failed row is pa...

(PRP_OWN)

[style] ~392-~392: Since ownership is already implied, this phrasing may be redundant.
Context: ...lickHouse, a failed row is parked under its own tenant's dlq.enabled and subject (`dl...

(PRP_OWN)

CHANGELOG.md

[typographical] ~13-~13: Consider using an em dash in dialogues and enumerations.
Context: - **One ClickHouse pool per tuple and one...

(DASH_RULE)

[style] ~13-~13: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: - One ClickHouse pool per tuple and one schema registry per tenant (internal/chconn/chconn.go (+ tests), internal/discovery/discovery.go (+ tests), internal/app/discoveries.go (new), internal/app/{app,wire}.go (+ tests), internal/api/{schema,ingest,structured_query,pipes,query,health,errors,clickhouse_exec}.go (+ tests), internal/stream/hub.go, internal/ingest/worker.go, internal/cache/{cache,local,version_manager}.go (+ tests), internal/testutil/{testutil,mocks}.go, tests/integration/{setup,tenants,boot_resilience,query_limits}_test.go, clients/ts/src/{schema,table,sql,client,types}.ts (+ tests), tests/e2e/sdk/admin.test.ts, docs/src/content/docs/{api,deployment,architecture,ingest-pipeline}.md, docs/src/content/docs/{settings-directory,configuration,access-control,reverse-proxy}.mdx, docs/src/content/docs/sdk/{admin,reference,queries}.md, AGENTS.md): the second slice of story 6 of the multi-tenant epic (#583), with no behavior change for a settings directory that holds the four files beyond the two noted at the end. The process opens one native pool per d...

(TOO_LONG_SENTENCE)

[style] ~13-~13: Since ownership is already implied, this phrasing may be redundant.
Context: ...of its own over its pool, kept fresh by its own loop — the boot retry until the first s...

(PRP_OWN)

[style] ~13-~13: Since ownership is already implied, this phrasing may be redundant.
Context: ...e ingest worker inserts each batch into its own tenant's ClickHouse — the tenant the me...

(PRP_OWN)

docs/src/content/docs/architecture.md

[typographical] ~197-~197: Consider using an em dash in dialogues and enumerations.
Context: - chconn.go — Pools holds one `Mana...

(DASH_RULE)

🔇 Additional comments (6)

internal/chconn/chconn.go (1)

530-531: LGTM!

Also applies to: 631-638

internal/chconn/chconn_test.go (1)

372-386: LGTM!

internal/app/wire.go (1)

202-203: LGTM!

internal/cache/cache.go (1)

34-39: LGTM!

docs/src/content/docs/architecture.md (1)

197-197: LGTM!

docs/src/content/docs/deployment.md (1)

392-392: LGTM!

…its cache

taitelee · 2026-09-24T10:25:17Z

@coderabbitai review

coderabbitai · 2026-09-24T10:25:24Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

#611) ## Summary Story 3 of the multi-tenant epic: what removing or rejecting a tenant at runtime does. A settings directory that holds the four files sees no change beyond the dedupe store moving back to `<data_dir>/pebble`. - **A tenant's open streams end with it.** A `GET /v1/stream` used to outlive a tenant a reload removed or rejected: the hub read no policy for it and withheld every row while the keepalive wheel held the connection open, so the client could not tell it from a quiet table. After every reload the hub now evicts the subscribers of each tenant no longer served (`Hub.Prune`, through a close-once `Subscriber.Evict`), and the handler ends the stream. That covers a gap-fill in progress (`replayContext` now also watches the eviction) and a stream `TenantMW` admitted just before the reload but registered with the hub just after the prune: the handler checks right after registering (`StreamHandler.Served`), and since the registry swaps its map before its hooks run, one of the two always sees it. The reconnect gets `404` (removed) or `503` (rejected). The SDK needs no change: it reconnects after a server-ended stream, stops on a `404`, and retries a `503`, resuming from `Last-Event-ID` once the folder is back — in a browser going cross-origin, only where the refusal passes CORS (see Follow-ups). The stale comment in `sse.ts` that read a `404` as a missed route is fixed. - **The last tenant can be removed too.** An emptied nested directory used to read as a change of shape (`Validate`'s flat directory missing its files), which rejected the reload whole and left that tenant served. A registry started with tenant folders now reads it as no folder left, so the reload removes the tenant like any other. Boot still reads an empty directory as the four files, missing. Reloading a deleted folder by name still leaves its tenant rejected (`503`), as `registry_test.go` has pinned since #598. The flip side: a whole-directory reload that finds the directory emptied by accident drops every tenant until the folders are back and reloaded, as it already drops any one tenant whose folder is deleted. `wavehouse validate` still reads an empty directory as boot does, as the four files missing, so a server restarted before a folder is written back refuses to boot. - **`/v1/health` keeps resolving a tenant, deliberately.** Its `404` tells a caller with no token no more than every tenant route's does, since they all answer before authenticating, which needs the tenant's verifier; exempting it would take its CORS answer from tenant `0`'s list, which a nested directory need not have. Recorded at the route and in `api.md`. - **A removed or rejected tenant's queued rows go to the dead-letter queue.** With no pool the insert fails and `dlqFor` reads the registry miss as on, so the rows are parked under `dlq.<tenant>.<table>`: never dropped, never inserted into another tenant's ClickHouse, and not left unacked, where they would hold the ack floor, stop the sweeper, and fill the one shared stream toward `mq.max_bytes_gb` until every tenant's ingest got `503`. A batch with no ClickHouse connection (its tenant no longer served, or no pool could be opened for it, such as by the connection ceiling) now also skips the row-by-row retry it could never pass and meets its dead-letter switch once, whole, logged once per batch rather than twice per row; a served tenant on no pool still honors its `dlq.enabled`. - **`/livez` drops a gone tenant's diagnostic.** Over a nested directory, a discovery error naming a tenant that stops being served before any tenant has loaded goes back to `no tenant has completed a first discovery yet` (a #610 follow-up). The schema side needed nothing new: story 6 already stops a gone tenant's refresh loop, now pinned by the removal test. - **Dedupe: one Pebble instance for every tenant** (its own commit). The wiring hands the embedded implementation `data_dir` once (`dedupe.NewEmbedded`) and asks it for each tenant's store (`Embedded.Tenant`, the `Factory`); the implementation keeps every tenant's seen ids in one instance at `<data_dir>/pebble`, each key `<tenant>\x00<id>`. Each tenant behaves as before: the instance is open only while some served tenant has dedupe on (a server with dedupe off opens nothing), a tenant switched off, rejected or removed keeps its seen ids, and the Pebble gauges are one set (`Embedded.Stats`; `Stats` leaves the per-tenant `Deduplicator`, `Managed` and `Stores`). Measured with Pebble v1.1.5, built like the release: an instance per tenant cost 13 goroutines, 7 open files and about 0.5 MB of heap each, so 1,000 tenants meant 13,000 goroutines, 7,000 files, about 500 MB of heap and 7.6 s of opens at boot, against 13, 7, about 4 MB and 14 ms for one instance; synced writes were 1.7x faster shared. There is nothing to migrate: `moveLegacyDedupeStore` and the old `<data_dir>/pebble` handling are deleted, and `tenant.Parse` no longer reserves `nats` and `pebble`, which it did only because a tenant's state lived at `<data_dir>/<tenant>`. One consequence of sharing: an instance that cannot open fails closed every tenant with dedupe on, not one. - **Sweep.** Every sentence that deferred to story 3 or that this change makes false is updated: the `perTenant` comment, the CHANGELOG entry on a removed-and-restored tenant's cache (settled by #610's `Cache.InvalidateTenant`), `deployment.md`'s "What a lost tenant `0` costs", and the per-tenant dedupe layout across the wiring, config, compose, Dockerfiles, `AGENTS.md`, the CHANGELOG entry for #602, and the architecture, configuration, deployment and settings-directory pages. ## Test plan - [x] `make ci` passes locally - [x] Pre-push reviewers: five rounds; the code review ended at `ship_it`, and the docs review's last round (three wording fixes) is applied without a further round - [x] `stream`: `Evict` is close-once; `Prune` evicts every subscriber of a tenant no longer served, on each topic and under each role, and no one else's - [x] `api`: a stream ends on its own when its tenant stops being served, whether idle, mid-gap-fill, or admitted before the reload and registered after the prune - [x] `app`, over a real listener: the stop ends every stream; a reload that rejects one tenant and then removes another ends each one's stream at once and cleanly while the other's stays open, and the reconnects get `503` and `404`; a reload a flat directory rejects leaves the stream open - [x] `app`, remove then restore: a rejected and then a removed tenant release pool, registry, refresh loop, verifier and dedupe store; the removed one's routes answer `404`, the worker gets no target and the switch on, and `/livez` goes back to the no-tenant line; restored, it is served over a fresh pool, registry and verifier (the JWKS is fetched again), and an id sent before the removal is still a duplicate - [x] `settings`: removing every folder removes every tenant and runs the hooks, and a folder written back is a tenant again; an emptied flat directory is still a rejected reload; an empty directory still refuses boot - [x] `ingest`: a batch with no ClickHouse connection meets its switch once, parked under its own topic and acked, or left unacked, with no request made, while the served tenant beside it inserts its own rows alone - [x] `dedupe`: tenants' ids never meet in the shared instance, however their keys would join; the instance opens with the first store and closes with the last, its ids kept; `Stats` is the instance's one set; an unopenable instance fails every tenant's store and the next apply retries - [ ] Manual: a nested directory with two tenants and dedupe on, a `wh.from(table).stream()` open on each; remove one folder and reload, watch its stream end and the SDK stop on the `404`; restore it and send an id it sent before ## Follow-ups - Over a nested directory serving no tenant `0`, a removed tenant's `404` carries no CORS headers (refusals read tenant `0`'s list, #600), so the browser SDK sees a network error and keeps re-dialing instead of stopping. Whatever turns away an unknown subdomain in front of WaveHouse should answer with CORS headers, the preflight included. If browser clients should stop on their own, one option is a final event on the evicted stream before it closes, which that connection's own CORS lets the browser read; that is a design question for Eric. - `sdk/streaming.md` and `sdk/reference.md` say the tenant-resolution `404` comes "when you send `X-Tenant-ID`"; over a nested directory with no `0` folder a client that sends no header gets `404 unknown tenant: 0` too, and removing a `0` folder now ends such a client's stream into it. The wording predates this branch. ## Related Issues Part of #583 (story 3).

Closes #141. Part of #613. ## What `SchemaRegistry.RetryRefresh` used to sleep exactly `2s * 2^n`, capped at 60s. That means N instances retrying against one recovering ClickHouse fire in lockstep once they all reach the cap. With this change, each sleep is a uniform draw from `[0, backoff)` (full jitter). The bound still doubles from 2s to 60s. **Why full jitter instead of the ±10% the issue proposed:** at the 60s cap, ±10% spreads the retries over only 12s. Full jitter spreads them over the whole 60s window. That makes it the strongest way to break lockstep without state or coordination (AWS's "Exponential Backoff and Jitter" analysis). One side effect: the mean wait halves. So during an outage, a failing tenant's retries, its log lines and `wavehouse_schema_refresh_failures_total` come about twice as often. Nothing in this repo consumes that counter yet (it was new in #610). The CHANGELOG entry mentions this. The random source is an injectable `retryDelay func(time.Duration) time.Duration` field (`rand.N` by default), set up the same way as the existing `firstTick`. It adds no dependency. ## Tests - `TestRetryRefresh_BackoffIsBounded` now records the backoff each sleep is drawn within (1, 2, 4, 4, 4 ms) instead of timing the wall clock, so it can't flake. - `TestRetryRefresh_SleepsTheJitteredDelay`: the loop sleeps the drawn delay, not the backoff. - `TestRetryRefresh_DelayIsSpreadOverTheBackoff`: 200 draws from the production `retryDelay` all fall in `[0, backoff)`, reach both the bottom and top quarters, and are nearly all distinct. ## Left for later - The #141 criterion "confirm the jitter range once clustered mode has a topology config" is conditional and stays with that work. - There is no shared backoff helper yet. `internal/auth` (the JWKS retry) has its own unjittered loop, and `feat/ch-error-classes` is adding a ClickHouse backoff in `internal/ingest`. Consolidating them is a follow-up. 🤖 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>

The #610 changelog entry still said InvalidateTenant leaves pipe results alone, which this PR makes false; its "structured-query results" now read "cached results". QueryKey's comment said a pipe passes several namespaces (none yet, #343), and AGENTS.md asked every cache.Cache implementation, not every backend, to run cachetest.Run. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B1tJWUp6oaDoH1usLwGtLF

## Summary A reload that changed a tenant's `clickhouse.addr` or `clickhouse.database` orphaned the tenant's cache but left its schema registry as it was. Until the tenant's loop fired at `schema.refresh_interval`, its queries and inserts were validated against the previous database's schema and run against the new one. A moved tenant now starts over the way a tenant back after a rejection or removal already does: - The pools hook drops the registry of every tenant `Pools.Reconcile` reports stale, beside the cache invalidation it already ran. The hook runs no network I/O. - The discovery hook, which runs after it, builds each a fresh registry over the pool the tenant is on now. The first discovery runs at once in the tenant's own loop, so a reload never waits on ClickHouse. - Until that discovery succeeds, the tenant's table lookups answer `503` with `Retry-After: 5`, as before any first discovery. They are never answered from the previous database's schema. - A discovery that fails is logged with its tenant (`schema discovery retry failed`), counted in `wavehouse_schema_refresh_failures_total`, and retried with backoff from two seconds to sixty. - A tenant whose address and database did not change keeps its registry and its loop. A flat directory's tenant `0` moves the same way. A process without the api role, which discovers no schema, only repoints its pool. One behavior change to note: when the new database cannot be reached, the moved tenant's ingest and queries answer `503` until it can. Before, they kept validating against the old schema. A second commit removes a wall-clock assertion from `TestDynamo_ThrottledCallEndsOnItsLastAttempt`, which failed under load (312ms against a 250ms bound) while the call itself ended correctly. The test still proves the call ended on its last attempt and not on the deadline, from the error. ## Test plan - [x] `TestReload_MovedTenantDiscoversTheNewDatabase`: over a nested directory with two tenants, the moved tenant reads the new database's table and not the old one, the other tenant keeps its registry and loop, and a reload that moves nobody touches nobody - [x] `TestReload_MovedTenantDiscoversTheNewDatabase_Flat`: the same move for tenant `0` - [x] `TestReload_MovedTenantFailedDiscoveryIsRetried`: the failure is logged with the tenant, ingest answers `503` for a table the old schema had, and the loop loads once the database answers - [x] `TestReload_OpsOnlyMovedTenant`: an ingest-only process repoints its pool and has no registry - [x] The three move tests fail with the drop removed - [x] The dedupe test passed 90 of 90 runs on one CPU under load - [ ] Not covered: a move between two real ClickHouse databases (the app tests use a fake connection over the production source), and a move that changes only `clickhouse.addr` ## Related Issues Closes #638 Part of #583 (story 6); follow-up named in #610.

taitelee added 12 commits September 23, 2026 12:49

feat(discovery): per-tenant registry lookups over a connection getter

e8d6a66

feat(clickhouse): one pool per tuple and one schema registry per tenant

8f6b885

feat(cache): fan an insert out to the tenants sharing its tables

2f586ea

feat(sdk): tenant option on the schema reads, the refresh and sql

0c8b285

docs: per-tenant clickhouse pools, discovery, probes and the ceiling

3b74734

docs: sweep the last shared-clickhouse wording

4de967a

Merge origin/main into ch-per-tenant

464dda2

fix(app): flip the nested boot state under one lock; review doc fixes

c11413f

fix(discovery): read the database from the pool the connection came from

9a51e50

fix(cache): orphan a tenant moved to another clickhouse address or da…

b39e7f7

…tabase

docs: certificate reads per pool, the cache interface, the chconn tre…

13aadd3

…e lines

docs(api): what a tenant answers when its clickhouse goes down after …

821d365

…discovery

github-project-automation Bot added this to WaveHouse Task Board Sep 24, 2026

github-project-automation Bot moved this to Backlog in WaveHouse Task Board Sep 24, 2026

github-advanced-security Bot found potential problems Sep 24, 2026

View reviewed changes

Comment thread internal/app/wire.go Fixed

coderabbitai Bot requested changes Sep 24, 2026

View reviewed changes

Comment thread docs/src/content/docs/deployment.md Outdated

Comment thread docs/src/content/docs/sdk/admin.md Outdated

Comment thread docs/src/content/docs/sdk/reference.md Outdated

Comment thread docs/src/content/docs/settings-directory.mdx Outdated

Comment thread internal/chconn/chconn.go

coderabbitai Bot requested changes Sep 24, 2026

View reviewed changes

Comment thread docs/src/content/docs/deployment.md Outdated

Comment thread internal/chconn/chconn.go

taitelee added 2 commits September 24, 2026 04:11

fix(chconn): never open a new pool above the ceiling

7412465

docs: a new pool's opening size, and a rewrapped cache comment

66ee513

coderabbitai Bot requested changes Sep 24, 2026

View reviewed changes

Comment thread CHANGELOG.md Outdated

docs(changelog): a flat directory's address or database move orphans …

d85621b

…its cache

coderabbitai Bot approved these changes Sep 24, 2026

View reviewed changes

taitelee marked this pull request as ready for review September 24, 2026 12:50

taitelee requested review from a team and EricAndrechek September 24, 2026 12:50

taitelee merged commit e8acfd8 into main Sep 24, 2026
32 of 33 checks passed

taitelee deleted the ch-per-tenant branch September 24, 2026 12:54

github-project-automation Bot moved this from In review to Done in WaveHouse Task Board Sep 24, 2026

This was referenced Sep 24, 2026

epic(settings): multi-tenant settings directory and tenant-scoped runtime #583

Open

feat(app): end a removed tenant's streams, one Pebble for every tenant #611

Merged

taitelee mentioned this pull request Sep 29, 2026

fix(app): rediscover a tenant's schema when a reload moves it #682

Merged

6 of 7 tasks

This was referenced Oct 1, 2026

test(app): a tenant move that changes only clickhouse.addr, and a move between real databases, are untested #708

Open

bug(api): the HTTP reader never prunes per-pool clients when a tenant is removed, moved or rotates credentials #713

Open

Conversation

taitelee commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Follow-ups

Related Issues

Uh oh!

taitelee commented Sep 24, 2026

Uh oh!

coderabbitai Bot commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Summary by CodeRabbit

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-code-quality Bot commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage Overview

Go

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

taitelee commented Sep 24, 2026

Uh oh!

coderabbitai Bot commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

taitelee commented Sep 24, 2026

Uh oh!

coderabbitai Bot commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

taitelee commented Sep 24, 2026 •

edited

Loading

coderabbitai Bot commented Sep 24, 2026 •

edited

Loading

coderabbitai Bot commented Sep 24, 2026 •

edited

Loading

github-actions Bot commented Sep 24, 2026 •

edited

Loading

github-code-quality Bot commented Sep 24, 2026 •

edited

Loading

coderabbitai Bot commented Sep 24, 2026 •

edited

Loading

coderabbitai Bot commented Sep 24, 2026 •

edited

Loading