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

[taitelee]

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

• 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
• TestReload_MovedTenantDiscoversTheNewDatabase_Flat: the same move for tenant 0
• 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
• TestReload_OpsOnlyMovedTenant: an ingest-only process repoints its pool and has no registry
• The three move tests fail with the drop removed
• 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.

[coderabbitai]

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

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: f00393f2-038e-44c4-80a9-af098e26de91

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 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: 16c31cf0-62f4-4faf-843c-927363a4b5eb

📥 Commits

Reviewing files that changed from the base of the PR and between 8d47b2b and ddd489f.

📒 Files selected for processing (1)

• internal/app/app_test.go

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

📜 Recent review details

🧰 Additional context used

🧠 Learnings (1)

📓 Common learnings

Learnt from: CR
Repo: Wave-RF/WaveHouse

Timestamp: 2026-09-30T00:18:59.579Z
Learning: Source excerpt:
# AGENTS.md — AI Agent Instructions for WaveHouse

## Operating Rules

1. **Validate locally before every push** — run `make ci` the documented way ([§Running `make ci`](#running-make-ci-for-agents)). Don't use CI as your first feedback loop.

🔇 Additional comments (1)

internal/app/app_test.go (1)

2033-2035: LGTM!

Also applies to: 2050-2050

---

📝 Summary

Summary by CodeRabbit

• Bug Fixes
  • When a tenant moves to a different ClickHouse address or database, schema discovery starts for the new target instead of using the previous database’s schema.
  • Until the new schema is discovered, table lookups return 503 with a Retry-After response. Discovery retries automatically if the database is unavailable.
  • If a pool move is refused, discovery continues against the database still used by the tenant’s queries and inserts.
  • Events already accepted remain queued, and the next worker flush uses the tenant’s current target.
  • Tenants whose address and database do not change retain their existing schema.

Walkthrough

When a reload changes a tenant’s ClickHouse address or database, the app drops its schema registry and starts discovery against the current pool. Tests cover registry replacement, unchanged tenants, failed discovery, loop shutdown, and ingest-only reloads. A DynamoDB throttling test no longer measures elapsed time.

Changes

Tenant schema discovery reload

Layer / File(s)	Summary
Drop and rebuild moved-tenant registries internal/app/discoveries.go, internal/app/wire.go, internal/discovery/discovery.go, AGENTS.md, CHANGELOG.md, docs/src/content/docs/architecture.md, docs/src/content/docs/settings-directory.mdx	Reload wiring drops stale tenants’ registries before cache invalidation. Discovery then reconciles against the tenants’ current pools. Documentation describes registry replacement, lookup behavior before discovery succeeds, and retry behavior.
Validate registry reload and shutdown behavior internal/testutil/testutil.go, internal/app/app_test.go, internal/app/roles_test.go	Tests verify registry replacement, unchanged tenants, failed discovery and recovery, cache invalidation ordering, retired-loop shutdown, and ingest-only reloads.

DynamoDB throttling test

Layer / File(s)	Summary
Assert throttling from the returned error internal/dedupe/dynamodb_test.go	The test removes its elapsed-time measurement and assertion. It retains checks for exhausted SDK attempts, a throttle cause, and no deadline error.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant SettingsReload
  participant PoolReconciliation
  participant Discovery
  participant TenantLookup
  SettingsReload->>PoolReconciliation: Reconcile tenant pools
  PoolReconciliation->>Discovery: Drop registry for moved tenant
  Discovery->>Discovery: Reconcile against current pool
  Discovery->>TenantLookup: Provide schema after discovery succeeds

Loading

Suggested reviewers: ericandrechek

Merge Risk: ⚪ Minimal · up to ddd48

Reloads refresh schemas when a tenant changes ClickHouse address or database, while username- or TLS-only changes retain normal refresh behavior. No concrete merge-blocking risk remains.

Security Architecture Review

Security architecture risk: 🔵 Low · up to ddd48

Tenant moves now discard outdated schema information and reject schema-dependent requests until rediscovery succeeds. No new access-control bypass was identified. Requests already in progress can still span a database change, so the change does not establish fully atomic cutover.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The directly changed state is the schema registry and discovery loop of each stale tenant within an API process. A reload can affect multiple moved tenants, but the registry operations do not change tenant identity or grant callers additional database authority.

Security Findings and Attack Paths

• inferred — A query can obtain a schema, read caller-controlled input, and only later obtain its tenant connection. A reload between those operations can still produce old-schema/new-pool execution. The base/head comparison leaves this split acquisition unchanged; the PR removes stale schemas from subsequent lookups but does not introduce the mixed-generation exposure. Its security consequences and accepted in-flight semantics remain unproven.

Trust Boundaries and Controls

• observed — The structured-query path still resolves table permission from the request's role and claims before building SQL. This PR changes schema availability, not that authorization sequence. The schema-unavailable branch returns before reaching query construction or connection use.

Resilience and Maintainability Implications

• observed — A failed replacement discovery does not restore the previous registry. It retries until success or cancellation, while retirement preserves ownership of an already-running refresh for shutdown. These mechanisms favor schema-consistent rejection over availability against outdated schema state.

Hardening Proposals

• proposed — If atomic request-level cutover is required, define a shared schema/pool generation contract and validate overlapping requests and repeated moves against it. This would address the pre-existing split-acquisition window rather than a verified new vulnerability.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The change in internal/dedupe/dynamodb_test.go removes a wall-clock assertion from an unrelated throttling test. It does not implement or verify issue #638. The remaining discovery code, tests, docu…	Remove the internal/dedupe/dynamodb_test.go change from this pull request, or move it to a separate pull request.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: rediscovering a tenant's schema after a reload moves it.
Description check	✅ Passed	The description directly explains the schema registry, discovery, retry, error response, and test changes in the pull request.
Linked Issues check	✅ Passed	Issue #638 requires stale tenants that change clickhouse.database or clickhouse.addr to use the new schema on the next request. The PR drops registries for stale tenants before cache invalidation,…
Docstring Coverage	✅ Passed	Docstring coverage is 93.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 7 files.

Full details: Out of Scope Changes check

Explanation

The change in internal/dedupe/dynamodb_test.go removes a wall-clock assertion from an unrelated throttling test. It does not implement or verify issue #638. The remaining discovery code, tests, documentation, and test utility support schema rediscovery.

✨ Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 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

• X
• Mastodon
• Reddit
• LinkedIn

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

[github-actions]

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

• Commit — ec80186: Merge remote-tracking branch 'origin/main' into fix/schema-refresh-on-tenant-move
• Author — @taitelee
• Committed — 2026-09-30 13:14 (UTC-04:00)
• Deployed — 2026-09-30 14:30 EDT

[github-code-quality]

Code Coverage Overview

Languages: Go

Go

The overall line coverage in commit ec80186 in the fix/schema-refresh-o... branch remains at 93%, unchanged from commit 6fa9723 in the main branch.

Show a line coverage summary of the most impacted files.

File	main 6fa9723	fix/schema-refresh-o... ec80186	+/-
internal/mq/embedded.go	90%	90%	0%
internal/cache/redis.go	94%	94%	0%
internal/ingest/worker.go	97%	97%	0%
internal/app/wire.go	93%	93%	0%
internal/app/discoveries.go	97%	100%	+3%

---

_{Updated September 30, 2026 19:22 UTC}

[taitelee]

@coderabbitai review

[coderabbitai]

✅ 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: 2

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 958cb146-bebe-44b0-8e96-18386cda6293

📥 Commits

Reviewing files that changed from the base of the PR and between 0de28ad and 8b4eb2a.

📒 Files selected for processing (11)

• AGENTS.md
• CHANGELOG.md
• docs/src/content/docs/architecture.md
• docs/src/content/docs/settings-directory.mdx
• internal/app/app_test.go
• internal/app/discoveries.go
• internal/app/roles_test.go
• internal/app/wire.go
• internal/dedupe/dynamodb_test.go
• internal/discovery/discovery.go
• internal/testutil/testutil.go

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

📜 Review details

🧰 Additional context used

📓 Path-based instructions (4)

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

📄 CodeRabbit inference engine (CLAUDE.md)

Files:

• AGENTS.md

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

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

Files:

• AGENTS.md

Source excerpt: **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

Source excerpt: **WH001 applies to every tracked Markdown file, with no carve-out** — `AGENTS.md`, `CHANGELOG.md`, `.github/` CI docs and `.claude/` agent prompts included.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

• AGENTS.md
• docs/src/content/docs/architecture.md
• CHANGELOG.md

🧠 Learnings (3)

📓 Common learnings

Learnt from: CR
Repo: Wave-RF/WaveHouse

Timestamp: 2026-09-29T18:14:21.880Z
Learning: Source excerpt:
# AGENTS.md — AI Agent Instructions for WaveHouse

## Testing Conventions

- **Every new function should have corresponding test cases.** Run `make lint` and `make test` before considering work complete.

📚 Learning: 2026-08-13T12:17:52.620Z

Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 470
File: docs/src/content/docs/reverse-proxy.mdx:137-144
Timestamp: 2026-08-13T12:17:52.620Z
Learning: For Wave-RF/WaveHouse documentation, verify claims about implementation control flow against the authoritative implementation source (for example, internal/auth/auth.go) rather than relying solely on docs/** content. Documentation may lag behind or paraphrase behavior, so control-flow claims should be confirmed in source code.

Applied to files:

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

📚 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/app/roles_test.go

🪛 LanguageTool

docs/src/content/docs/architecture.md

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

(DASH_RULE)

---

[style] ~156-~156: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...ne snapshot (App.discoverySource over chconn.Pools.For in production, so a reload that repoints the tenant to another user or tls block, which reads the same tables, applies to the next refresh, one that moves it to another address or database has internal/app's discoveries drop its registry and build a fresh one, whose first discovery runs at once and whose lookups answer ErrNotLoaded until it succeeds (#638), and a refused move keeps discovering the database the tenant's queries and inserts still use; no pool is ErrNoConnection), queries system.columns to discover ClickHouse table schemas, keeping each column's default_kind so IsInsertable / InsertableColumns / InsertableColumnNames (memoized per table at refresh) can decide the insertable subset the ingest envelope and the SSE announcement are both built from. Each refresh also records the server ve...

(TOO_LONG_SENTENCE)

---

[style] ~156-~156: 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)

CHANGELOG.md

[typographical] ~93-~93: Consider using an em dash in dialogues and enumerations.
Context: - **A tenant moved to another ClickHouse ...

(DASH_RULE)

🔇 Additional comments (9)

internal/dedupe/dynamodb_test.go (1)

557-559: LGTM!

internal/discovery/discovery.go (1)

203-204: LGTM!

AGENTS.md (1)

40-40: LGTM!

CHANGELOG.md (1)

93-93: LGTM!

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

156-156: LGTM!

docs/src/content/docs/settings-directory.mdx (1)

208-208: LGTM!

internal/testutil/testutil.go (1)

39-43: LGTM!

internal/app/app_test.go (1)

1806-1868: LGTM!

Also applies to: 1870-1914, 1916-1934, 1936-1968

internal/app/roles_test.go (1)

159-174: LGTM!

Outdated

[taitelee]

@coderabbitai review

[coderabbitai]

✅ 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: 94e9f27d-faf9-4213-a14b-a13b13f64278

📥 Commits

Reviewing files that changed from the base of the PR and between 8b4eb2a and 8d47b2b.

📒 Files selected for processing (3)

• internal/app/app_test.go
• internal/app/discoveries.go
• internal/app/wire.go

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

📜 Review details

🧰 Additional context used

📓 Path-based instructions (2)

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

📄 CodeRabbit inference engine (AGENTS.md)

Files:

• internal/app/app_test.go

Source excerpt: Create the package under `internal/`.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

• internal/app/wire.go
• internal/app/discoveries.go
• internal/app/app_test.go

🔇 Additional comments (3)

internal/app/wire.go (1)

320-329: LGTM!

internal/app/discoveries.go (1)

29-30: LGTM!

Also applies to: 41-46, 51-51, 88-88, 95-107, 120-120, 141-141, 158-159, 167-167, 169-169, 171-171, 174-174, 176-178, 181-181

internal/app/app_test.go (1)

1970-2008: LGTM!

[taitelee]

@coderabbitai review

[coderabbitai]

✅ 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.