Skip to content

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

Merged
taitelee merged 14 commits into
mainfrom
tenant-removal
Sep 24, 2026
Merged

taitelee merged 14 commits into
mainfrom
tenant-removal

Conversation

@taitelee

Copy link
Copy Markdown
Member

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 feat(settings): nested settings directory with a per-tenant registry #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 feat(clickhouse): one pool per tuple and one schema registry per tenant #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 feat(clickhouse): one pool per tuple and one schema registry per tenant #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 feat(dedupe): one store per tenant under data_dir/<tenant>/dedupe #602, and the architecture, configuration, deployment and settings-directory pages.

Test plan

  • make ci passes locally
  • 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
  • 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
  • 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
  • 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
  • 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
  • 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
  • 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
  • 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, feat(cors): decide the allowlist from the request's tenant #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).

@taitelee

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

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

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: 77293bf9-079c-46f7-8360-965fbb9873f1

📥 Commits

Reviewing files that changed from the base of the PR and between 79e7fba and 3c00e16.

📒 Files selected for processing (3)
  • docs/src/content/docs/ingest-pipeline.md
  • internal/ingest/worker.go
  • internal/ingest/worker_test.go

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

📜 Recent review details
🧰 Additional context used
📓 Path-based instructions (2)
Create `*_test.go` files in the same package as the code under test.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

  • internal/ingest/worker_test.go
Editors see WH001 in `.md` only.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

  • docs/src/content/docs/ingest-pipeline.md
🔇 Additional comments (3)
internal/ingest/worker.go (1)

540-543: LGTM!

Also applies to: 548-551, 584-585, 639-639, 691-696, 704-705, 707-707, 710-711

internal/ingest/worker_test.go (1)

1939-1944: LGTM!

Also applies to: 1963-1964, 1966-1969

docs/src/content/docs/ingest-pipeline.md (1)

16-16: LGTM!


📝 Summary

Summary by CodeRabbit

  • New Features
    • Open event streams now end when their tenant is no longer served, including during settings reloads.
    • Deduplication history for all tenants is stored in one shared location and retained when tenants are removed and restored.
    • Tenant IDs can now use the names nats and pebble.
    • Batches for tenants without a ClickHouse connection are sent to the dead-letter queue as a whole when it is enabled; otherwise, they remain available for redelivery.
  • Bug Fixes
    • Removing the last tenant folder from a nested settings directory now removes that tenant from the active configuration.
  • Documentation
    • Updated storage, stream, health-check, and dead-letter queue guidance to reflect these behaviors.

Walkthrough

This pull request consolidates tenant dedupe storage into one shared Pebble instance. It also changes tenant reload and stream handling, supports removal of the final nested tenant, and routes batches without a ClickHouse connection through whole-batch DLQ handling.

Changes

Shared dedupe storage

Layer / File(s) Summary
Shared Pebble instance and tenant stores
internal/dedupe/*, internal/api/ingest_test.go, docs/src/content/docs/architecture.md, AGENTS.md
Tenant stores share a lazily opened Pebble instance at data_dir/pebble. Keys include the tenant ID and a NUL separator. The instance stays open while any tenant store is open, and statistics come from the instance.
Application wiring and storage configuration
internal/app/*, internal/config/config.go, config.yaml, deployments/*, docs/src/content/docs/configuration.mdx, docs/src/content/docs/deployment.md, docs/src/content/docs/settings-directory.mdx, internal/observability/metrics.go, internal/testutil/mocks.go, CHANGELOG.md
Application wiring uses the shared instance and its metrics. Configuration and deployment descriptions specify the shared Pebble path. Tests cover instance-open failures and tenant dedupe settings.

Tenant reload and stream lifecycle

Layer / File(s) Summary
Nested settings and tenant ID validation
internal/settings/*, internal/tenant/*, docs/src/content/docs/architecture.md, docs/src/content/docs/api.md, docs/src/content/docs/deployment.md, AGENTS.md
A nested reload can remove the final tenant folder. The empty-root boot case remains invalid. Tenant IDs nats and pebble are no longer reserved.
Stream eviction and handler cancellation
internal/stream/*, internal/api/stream.go, internal/api/stream_test.go, internal/app/wire.go, docs/src/content/docs/api.md, docs/src/content/docs/architecture.md, CHANGELOG.md, AGENTS.md
Reload wiring prunes subscribers for tenants no longer served. The stream handler ends those streams, including when eviction occurs during replay or before Hub registration.
Reload resource reconciliation
internal/app/*, internal/app/wire.go, docs/src/content/docs/api.md, docs/src/content/docs/deployment.md, CHANGELOG.md
Reload hooks reconcile discovery and tenant resources. Tests cover removal and rejection, including diagnostic updates and resource release.

Batches without a ClickHouse connection

Layer / File(s) Summary
No-target batch handling and DLQ behavior
internal/ingest/worker.go, internal/ingest/worker_test.go, docs/src/content/docs/api.md, docs/src/content/docs/architecture.md, docs/src/content/docs/ingest-pipeline.md, docs/src/content/docs/settings-directory.mdx, docs/src/content/docs/deployment.md
The worker skips row-by-row retry when a tenant has no ClickHouse connection. It checks the DLQ switch once for the batch, parks rows when enabled, and leaves them unacked when disabled.

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

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant App
  participant Hub
  participant Subscriber
  participant StreamHandler
  App->>Hub: Prune tenants no longer served
  Hub->>Subscriber: Evict subscriber
  Subscriber->>StreamHandler: Close Evicted channel
  StreamHandler->>StreamHandler: Cancel replay and end stream
Loading
sequenceDiagram
  participant Worker
  participant ClickHouseTarget
  participant DLQSwitch
  participant NATS
  Worker->>ClickHouseTarget: Insert batch
  ClickHouseTarget-->>Worker: No connection
  Worker->>DLQSwitch: Check switch once for batch
  alt DLQ enabled
    Worker->>NATS: Publish batch rows to DLQ
  else DLQ disabled
    Worker->>NATS: Leave batch unacked for redelivery
  end
Loading

Suggested reviewers: ericandrechek

Merge Risk: 🟡 Moderate · up to 3c00e

Existing dedupe history may not be recognized after upgrade, allowing previously accepted events to be accepted again. Resolve the migration concern before merging.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 76.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 50 functions across 28 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 identifies both primary changes: ending streams for removed tenants and consolidating dedupe storage into one Pebble instance.
Description check ✅ Passed The description directly explains the stream, reload, ingestion, diagnostics, and shared Pebble changes, and includes the test plan and follow-ups.
Full details: Docstring Coverage

Explanation

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

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 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

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

@coderabbitai

coderabbitai Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
✅ 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 github-actions Bot added documentation Improvements or additions to documentation go Pull requests that update go code area/observability Metrics, logs, traces, health, profiling area/api HTTP handlers, routing, middleware area/ingest Ingest pipeline (Bento, batching, DLQ) area/dedupe Deduplication (Pebble, ScyllaDB) area/sdk TypeScript SDK (clients/ts/) area/docs Documentation, site/, README area/infra CI, build, deploy, Docker, release area/app Process wiring (internal/app): component build, run, release area/tenant Tenant id, header resolution, per-tenant settings (internal/tenant) labels Sep 24, 2026
@github-actions

github-actions Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

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

  • Commit — 3c00e16: fix(ingest): decide a no-connection batch's DLQ disposition once per batch
  • Author — @taitelee
  • Committed — 2026-09-24 15:35 (UTC-04:00)
  • Deployed — 2026-09-24 15:57 EDT

@github-code-quality

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

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: Go

Go

The overall line coverage in commit 3c00e16 in the tenant-removal branch remains at 93%, unchanged from commit e8acfd8 in the main branch.

Show a line coverage summary of the most impacted files.
File main e8acfd8 tenant-removal 3c00e16 +/-
internal/dedupe/stores.go 96% 94% -2%
internal/app/wire.go 92% 92% 0%
internal/ingest/worker.go 97% 97% 0%
internal/stream/hub.go 97% 97% 0%
internal/api/router.go 98% 98% 0%
internal/dedupe/managed.go 100% 100% 0%
internal/settings/registry.go 100% 100% 0%
internal/settings/tree.go 100% 100% 0%
internal/dedupe/embedded.go 86% 92% +6%
internal/api/stream.go 61% 78% +17%

Updated September 24, 2026 19:44 UTC

coderabbitai[bot]
coderabbitai Bot previously requested changes Sep 24, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 1a5f56a5-330f-421b-9784-799539936873

📥 Commits

Reviewing files that changed from the base of the PR and between e8acfd8 and 79e7fba.

📒 Files selected for processing (44)
  • AGENTS.md
  • CHANGELOG.md
  • clients/ts/src/stream/sse.ts
  • config.yaml
  • deployments/Dockerfile
  • deployments/Dockerfile.goreleaser
  • deployments/compose/standalone.yaml
  • 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/ingest-pipeline.md
  • docs/src/content/docs/settings-directory.mdx
  • internal/api/ingest_test.go
  • internal/api/router.go
  • internal/api/stream.go
  • internal/api/stream_test.go
  • internal/app/app.go
  • internal/app/app_test.go
  • internal/app/wire.go
  • internal/config/config.go
  • internal/dedupe/dedupe.go
  • internal/dedupe/embedded.go
  • internal/dedupe/embedded_test.go
  • internal/dedupe/managed.go
  • internal/dedupe/managed_test.go
  • internal/dedupe/stores.go
  • internal/dedupe/stores_test.go
  • internal/ingest/worker.go
  • internal/ingest/worker_test.go
  • internal/observability/metrics.go
  • internal/settings/registry.go
  • internal/settings/registry_test.go
  • internal/settings/settings.go
  • internal/settings/tree.go
  • internal/settings/tree_test.go
  • internal/stream/bucket.go
  • internal/stream/hub.go
  • internal/stream/hub_test.go
  • internal/stream/subscriber.go
  • internal/stream/subscriber_test.go
  • internal/tenant/tenant.go
  • internal/tenant/tenant_test.go
  • internal/testutil/mocks.go
💤 Files with no reviewable changes (4)
  • internal/dedupe/dedupe.go
  • internal/testutil/mocks.go
  • internal/tenant/tenant_test.go
  • internal/settings/tree_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. (12)
  • GitHub Check: Integration tests
  • GitHub Check: Unit tests
  • GitHub Check: Docs build
  • GitHub Check: Coverage
  • GitHub Check: E2E tests
  • GitHub Check: Lint
  • GitHub Check: Validate snapshot build
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Analyze (go)
  • GitHub Check: Analyze (actions)
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Analyze (go)
🧰 Additional context used
📓 Path-based instructions (11)
See [AGENTS.md](AGENTS.md) for project conventions, architecture notes, and AI agent instructions.

📄 CodeRabbit inference engine (CLAUDE.md)

Files:

  • AGENTS.md
Add the field to the appropriate struct in `internal/config/config.go` with `yaml`, `env`, and `env-default` tags.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

  • internal/config/config.go
Register the route in `internal/api/router.go`.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

  • internal/api/router.go
Wire dependencies in `internal/app/wire.go`.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

  • internal/app/wire.go
Create `*_test.go` files in the same package as the code under test.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

  • internal/stream/hub_test.go
  • internal/settings/registry_test.go
  • internal/stream/subscriber_test.go
  • internal/ingest/worker_test.go
  • internal/dedupe/stores_test.go
  • internal/dedupe/managed_test.go
  • internal/dedupe/embedded_test.go
  • internal/api/ingest_test.go
  • internal/app/app_test.go
  • internal/api/stream_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/config/config.go
  • internal/stream/hub_test.go
  • internal/stream/bucket.go
  • internal/settings/registry_test.go
  • internal/observability/metrics.go
  • internal/stream/subscriber_test.go
  • internal/api/router.go
  • internal/dedupe/managed.go
  • internal/settings/settings.go
  • internal/app/app.go
  • internal/stream/subscriber.go
  • internal/ingest/worker_test.go
  • internal/tenant/tenant.go
  • internal/dedupe/stores_test.go
  • internal/dedupe/managed_test.go
  • internal/dedupe/stores.go
  • internal/stream/hub.go
  • internal/dedupe/embedded_test.go
  • internal/api/ingest_test.go
  • internal/ingest/worker.go
  • internal/app/app_test.go
  • internal/settings/tree.go
  • internal/settings/registry.go
  • internal/api/stream_test.go
  • internal/dedupe/embedded.go
  • internal/api/stream.go
  • internal/app/wire.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/configuration.mdx
  • docs/src/content/docs/settings-directory.mdx
Document in `docs/src/content/docs/configuration.mdx`.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

  • docs/src/content/docs/configuration.mdx
Document in `docs/src/content/docs/architecture.md`.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

  • docs/src/content/docs/architecture.md
Document in `docs/src/content/docs/api.md`.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

  • docs/src/content/docs/api.md
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: Wave-RF/WaveHouse

Timestamp: 2026-09-24T19:17:33.496Z
Learning: **Go 1.26**, strict formatting (`gofumpt`, enforced by CI)
📚 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/stream/hub_test.go
🪛 LanguageTool
docs/src/content/docs/api.md

[grammar] ~615-~615: Ensure spelling is correct
Context: ...ployment#multi-tenant-deployments)), so where tenant 0 is not served or its list do...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

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

[style] ~188-~188: Since ownership is already implied, this phrasing may be redundant.
Context: ...itch is on: each tenant's store follows its own folder's dedupe.enabled the same way;...

(PRP_OWN)

CHANGELOG.md

[typographical] ~13-~13: Consider using an em dash in dialogues and enumerations.
Context: - **A tenant removed or rejected at runti...

(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: - A tenant removed or rejected at runtime has its open streams ended (internal/stream/{hub,subscriber,bucket}.go (+ tests), internal/api/stream.go (+ tests), internal/api/router.go, internal/settings/{registry,tree}.go (+ tests), internal/ingest/worker.go (+ tests), internal/app/wire.go (+ tests), clients/ts/src/stream/sse.ts, docs/src/content/docs/{api,deployment,architecture,ingest-pipeline}.md, docs/src/content/docs/settings-directory.mdx, AGENTS.md): story 3 of the multi-tenant epic (#583). A GET /v1/stream used to outlive i...

(TOO_LONG_SENTENCE)


[style] ~13-~13: 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: ...ld pass, and meets its DLQ switch once, whole, logged once per batch rather than twic...

(EN_REPEATEDWORDS_WHOLE)


[style] ~13-~13: Since ownership is already implied, this phrasing may be redundant.
Context: ...on, so its queued rows are parked under its own subject rather than dropped, inserted i...

(PRP_OWN)


[style] ~14-~14: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...s completed a first discovery yet. - **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`](#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] ~14-~14: 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] ~14-~14: 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)


[style] ~18-~18: Since ownership is already implied, this phrasing may be redundant.
Context: ...o restructure. A tenant's store follows its own folder's dedupe.enabled rather than t...

(PRP_OWN)


[typographical] ~79-~79: Consider using an em dash in dialogues and enumerations.
Context: - **An insert invalidates a table's cache...

(DASH_RULE)

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] ~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: ...ved: delete its folder, then reload the whole directory. Its open streams end, its ro...

(EN_REPEATEDWORDS_WHOLE)


[style] ~388-~388: Since ownership is already implied, this phrasing may be redundant.
Context: ...queued rows are parked on the DLQ under its own subject; nothing it stored is deleted, ...

(PRP_OWN)


[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)

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; `P...

(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] ~99-~99: Since ownership is already implied, this phrasing may be redundant.
Context: ...ame — and each event is evaluated under its own tenant's policy (the PolicySource rea...

(PRP_OWN)


[style] ~99-~99: Since ownership is already implied, this phrasing may be redundant.
Context: ...th's: ReplayProjector tracks drift in its own state and the two are not reconciled ([...

(PRP_OWN)


[style] ~187-~187: 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 as it was. The tenant map is replaced whole by a reload, so a lookup is one lock-fr...

(EN_REPEATEDWORDS_WHOLE)

🔇 Additional comments (37)
internal/ingest/worker_test.go (1)

1937-1997: LGTM!

internal/settings/registry.go (1)

34-37: LGTM!

Also applies to: 189-189, 250-259

internal/settings/registry_test.go (1)

96-100: LGTM!

Also applies to: 393-432, 567-567

internal/settings/tree.go (1)

86-87: LGTM!

Also applies to: 107-120

internal/tenant/tenant.go (1)

30-31: LGTM!

internal/api/router.go (1)

147-154: LGTM!

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

114-114: LGTM!

Also applies to: 155-155, 615-615, 876-876

internal/stream/hub.go (1)

174-193: LGTM!

internal/stream/hub_test.go (1)

1576-1602: LGTM!

internal/stream/subscriber.go (1)

41-47: LGTM!

Also applies to: 159-166

internal/stream/subscriber_test.go (1)

30-30: LGTM!

Also applies to: 40-48

internal/api/stream_test.go (1)

181-258: LGTM!

clients/ts/src/stream/sse.ts (1)

347-354: LGTM!

internal/stream/bucket.go (1)

10-10: LGTM!

AGENTS.md (1)

32-32: LGTM!

Also applies to: 38-38, 46-48, 61-61

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

85-85: LGTM!

Also applies to: 93-93, 99-100, 126-128, 139-139, 157-157, 187-187, 193-193, 244-245

internal/dedupe/embedded.go (1)

61-107: LGTM!

Also applies to: 136-147

internal/dedupe/embedded_test.go (1)

14-136: LGTM!

internal/dedupe/managed.go (1)

25-27: LGTM!

internal/dedupe/managed_test.go (1)

14-14: LGTM!

Also applies to: 38-38, 59-59, 90-93

internal/dedupe/stores.go (1)

14-23: LGTM!

Also applies to: 54-59

internal/dedupe/stores_test.go (1)

14-130: LGTM!

internal/api/ingest_test.go (1)

689-697: LGTM!

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

123-127: LGTM!

Also applies to: 188-188, 213-219

internal/app/app.go (1)

108-111: LGTM!

internal/app/wire.go (1)

156-171: LGTM!

Also applies to: 188-191, 398-400, 420-423, 438-458, 472-526, 565-565, 615-621, 766-766, 775-778, 844-844

internal/app/app_test.go (1)

215-216: LGTM!

Also applies to: 469-566, 1114-1228, 1388-1468

internal/observability/metrics.go (1)

22-23: LGTM!

internal/config/config.go (1)

14-15: LGTM!

config.yaml (1)

5-8: LGTM!

deployments/Dockerfile (1)

28-28: LGTM!

deployments/Dockerfile.goreleaser (1)

7-7: LGTM!

deployments/compose/standalone.yaml (1)

20-21: LGTM!

docs/src/content/docs/configuration.mdx (1)

38-38: LGTM!

Also applies to: 183-183

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

172-188: LGTM!

Also applies to: 321-321, 348-348, 384-394, 449-449

internal/settings/settings.go (1)

140-142: LGTM!

CHANGELOG.md (1)

13-13: LGTM!

Also applies to: 18-18, 79-79

Comment thread internal/dedupe/embedded.go
Comment thread internal/ingest/worker.go Outdated
@github-project-automation github-project-automation Bot moved this from Backlog to In review in WaveHouse Task Board Sep 24, 2026
@EricAndrechek

Copy link
Copy Markdown
Member

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
✅ 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.

@taitelee
taitelee marked this pull request as ready for review September 24, 2026 19:54
@taitelee
taitelee requested review from a team and EricAndrechek September 24, 2026 19:54
@taitelee
taitelee merged commit 93d8019 into main Sep 24, 2026
29 checks passed
@taitelee
taitelee deleted the tenant-removal branch September 24, 2026 19:54
@github-project-automation github-project-automation Bot moved this from In review to Done in WaveHouse Task Board Sep 24, 2026
taitelee added a commit that referenced this pull request Oct 1, 2026
#711)

## Summary

A `settings.dir` holding folders but none of the four files is read as
nested. When every folder was rejected, or none was named by a tenant id
(a fresh volume whose only entry is `lost+found`, a parent directory
mounted by mistake), `settings.Open` still returned a registry and the
server came up serving no tenant, answering every tenant route `unknown
tenant`. Before #598 that root refused boot.

Boot now refuses a nested root that would serve no tenant, with the
findings plus one naming the rule, through the same path a flat invalid
root takes, so the error still points at `wavehouse validate` and
`wavehouse bootstrap`. The rule is boot's alone: a whole-directory
reload that finds every folder gone or broken still drops every tenant
and keeps running (#611), and `wavehouse validate` is unchanged. A root
with one valid folder beside a rejected or stray one still boots and
serves that tenant, and an empty or missing root still refuses as the
four files, missing.

## Test plan

- [x] `settings.Open` refuses a root whose every folder is rejected, and
a root whose only folder is `lost+found`, each with the findings
- [x] `settings.Open` on one valid folder beside a broken one and a
stray one opens and serves the valid tenant
- [x] Existing pins unchanged: an emptied directory on reload removes
every tenant and keeps running; empty and missing roots refuse as
"config.json: missing"; `wavehouse validate` exit codes
- [x] `make ci` passes locally, integration and e2e included

## Related Issues

Closes #599
Part of #583
<!--
Checklist for the author (not kept in the squash commit message):

- `make ci` passes locally
- Docs updated per AGENTS.md "Documentation & Consistency Sync" rules
- CHANGELOG.md [Unreleased] entry added
- Tests cover new / changed behavior (70 % minimum, 80 %+ preferred)

The PR title is the squash commit subject — use Conventional Commits
(`feat:`, `fix:`, `docs:`, `refactor:`, `test:`, `chore:`, `ci:`,
`deps:`, `build:`, `perf:`, `revert:`, `style:`). The PR body below is
the squash commit message, so keep it tight.
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/api HTTP handlers, routing, middleware area/app Process wiring (internal/app): component build, run, release area/dedupe Deduplication (Pebble, ScyllaDB) area/docs Documentation, site/, README area/infra CI, build, deploy, Docker, release area/ingest Ingest pipeline (Bento, batching, DLQ) area/observability Metrics, logs, traces, health, profiling area/sdk TypeScript SDK (clients/ts/) area/tenant Tenant id, header resolution, per-tenant settings (internal/tenant) documentation Improvements or additions to documentation go Pull requests that update go code

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants