fix(chconn): release a tuple's HTTP clients with its pool

[taitelee]

Summary

The native ClickHouse pools follow the tenants: a tuple no served tenant names is released after a grace. The two HTTP-interface consumers, the ingest worker's inserts and the /v1/ops/query proxy, each keep their own http.Client cache, and nothing ever dropped a client from it. A tuple with a tls block gets a TLS config of its own each time its pool opens, so once a tenant was removed, moved to another ClickHouse, or rotated its password, the client made for its old tuple stayed for the life of the process with its idle connections open until the transport's own idle timeout. A test that opened and released one tuple five times counted five clients, each still holding its connection after the native pool had closed.

• Pools.Reconcile also returns the tuples it released, with the grace each pool closes after, whether the tenants on it were removed or moved to another address, database, user or tls block.
• The pools hook has both caches drop the released tuple's client and close its idle connections after that grace, the longest query_timeout among the tenants the tuple had, so a request in flight finishes. A tuple another tenant still names keeps its client; one that opens again gets a fresh one.
• The worker's cache is built by the wiring rather than the worker, so the hook can release from it before Run starts the worker.
• Tuples with no tls block share one client per cache, which stays: nothing in it belongs to one tuple, and a removed tenant's idle plain-HTTP connections ride out the transport's idle timeout on their own.

Test plan

• A released tuple's client is dropped and its idle connections closed after the grace; a tuple another tenant still names keeps its client; the zero-tls client stays; only idle connections are closed (internal/chconn)
• Through the real wiring: a removed tenant's HTTP clients go with its pool (internal/app)
• The query proxy exposes the cache it queries through, and the worker takes the cache it is handed (internal/api, internal/ingest)
• SDK e2e: a tenant keeps being served across a reload that moves it to another connection tuple
• make ci

Related Issues

Closes #713
Part of #583 (story 6)

[coderabbitai]

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: a4c1a024-aea6-4680-97b2-44fb9fdeadb0

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: a5d7662d-9fdb-468b-94b2-5545b8ef43d2

📥 Commits

Reviewing files that changed from the base of the PR and between afd7669 and 9522528.

📒 Files selected for processing (16)

• AGENTS.md
• CHANGELOG.md
• docs/src/content/docs/architecture.md
• internal/api/query.go
• internal/api/query_test.go
• internal/app/app.go
• internal/app/app_test.go
• internal/app/wire.go
• internal/chconn/chconn.go
• internal/chconn/chconn_test.go
• internal/ingest/worker.go
• internal/ingest/worker_test.go
• tests/e2e/sdk/admin.test.ts
• tests/e2e/sdk/settings.ts
• tests/integration/ingest_outage_test.go
• tests/integration/shard_order_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

📚 Code guidelines (4)

CONTRIBUTING.md — auto-discovered
docs/src/content/docs/development.md — auto-discovered
AGENTS.md — auto-discovered
.github/copilot-instructions.md — auto-discovered

📓 Path-based instructions (11)

Source excerpt: Write tests for new functionality.

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Files:

• internal/api/query.go
• internal/app/app.go
• tests/integration/shard_order_test.go
• internal/api/query_test.go
• tests/integration/ingest_outage_test.go
• internal/app/app_test.go
• internal/ingest/worker_test.go
• internal/app/wire.go
• internal/ingest/worker.go
• internal/chconn/chconn.go
• internal/chconn/chconn_test.go

Source excerpt: **Integration test needing Docker** → add a subtest under `tests/integration/` (e.g.

📄 CodeRabbit inference engine (docs/src/content/docs/development.md)

Files:

• tests/integration/shard_order_test.go
• tests/integration/ingest_outage_test.go

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

📄 CodeRabbit inference engine (AGENTS.md)

Files:

• tests/integration/shard_order_test.go
• internal/api/query_test.go
• tests/integration/ingest_outage_test.go
• internal/app/app_test.go
• internal/ingest/worker_test.go
• internal/chconn/chconn_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

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

📄 CodeRabbit inference engine (AGENTS.md)

Files:

• internal/api/query.go
• internal/app/app.go
• internal/api/query_test.go
• internal/app/app_test.go
• internal/ingest/worker_test.go
• internal/app/wire.go
• internal/ingest/worker.go
• internal/chconn/chconn.go
• internal/chconn/chconn_test.go

Source excerpt: **Never hard-wrap prose.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

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

Source excerpt: **Docs prose**: never hard-wrap Markdown — one paragraph is one line.

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Files:

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

Source excerpt: **Go 1.26**, strict formatting (`gofumpt`, enforced by CI)

📄 CodeRabbit inference engine (AGENTS.md)

Files:

• internal/api/query.go
• internal/app/app.go
• tests/integration/shard_order_test.go
• internal/api/query_test.go
• tests/integration/ingest_outage_test.go
• internal/app/app_test.go
• internal/ingest/worker_test.go
• internal/app/wire.go
• internal/ingest/worker.go
• internal/chconn/chconn.go
• internal/chconn/chconn_test.go

Source excerpt: **Formatting**: Code must be formatted with `gofumpt` (a strict superset of `gofmt`).

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Files:

• internal/api/query.go
• internal/app/app.go
• tests/integration/shard_order_test.go
• internal/api/query_test.go
• tests/integration/ingest_outage_test.go
• internal/app/app_test.go
• internal/ingest/worker_test.go
• internal/app/wire.go
• internal/ingest/worker.go
• internal/chconn/chconn.go
• internal/chconn/chconn_test.go

Source excerpt: **Strict Go formatting**: Use `gofumpt` (a stricter superset of `gofmt`, enforced by CI).

📄 CodeRabbit inference engine (docs/src/content/docs/development.md)

Files:

• internal/api/query.go
• internal/app/app.go
• tests/integration/shard_order_test.go
• internal/api/query_test.go
• tests/integration/ingest_outage_test.go
• internal/app/app_test.go
• internal/ingest/worker_test.go
• internal/app/wire.go
• internal/ingest/worker.go
• internal/chconn/chconn.go
• internal/chconn/chconn_test.go

Source excerpt: **E2E test via SDK** → add a `tests/e2e/sdk/*.test.ts` file.

📄 CodeRabbit inference engine (docs/src/content/docs/development.md)

Files:

• tests/e2e/sdk/admin.test.ts

🧠 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/api/query_test.go

📚 Learning: 2026-08-11T21:55:53.726Z

Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: clients/go/stream_test.go:159-176
Timestamp: 2026-08-11T21:55:53.726Z
Learning: For Go files in this repository, do not report direct type assertions solely because the `forcetypeassert` rule is commented out in `.golangci.yml`. Only flag a type assertion when there is an independent correctness, safety, or maintainability issue.

Applied to files:

• internal/chconn/chconn_test.go

🪛 LanguageTool

CHANGELOG.md

[typographical] ~111-~111: Consider using an em dash in dialogues and enumerations.
Context: - **A ClickHouse tuple's HTTP clients are...

(DASH_RULE)

---

[style] ~111-~111: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...thub.com//issues/583). The ingest worker and the /v1/ops/query proxy each keep one HTTP client per TLS config (chconn.HTTPClients), and a connection tuple with a tls block gets a config of its own each time its pool opens, so the client each had made for such a tuple stayed for the life of the process once the tuple was gone, its idle connections open until the transport's own 90-second idle timeout rather than closing with the pool: a test that opens and releases one tuple five times counted five clients, each still holding its connection after the native pool had closed. Pools.Reconcile now also returns the ...

(TOO_LONG_SENTENCE)

---

📝 Summary

Summary by CodeRabbit

• Bug Fixes
  • HTTP connections associated with removed ClickHouse pools now close after the pool’s grace period, including idle connections. Active requests remain unaffected.
  • Pools still in use retain their connections, and reopened pools receive fresh connections.
  • Shared connections for ClickHouse configurations without TLS remain available.

Walkthrough

The change ties cached ClickHouse HTTP clients to pool tuple release. Pool reconciliation returns released tuples, and application reload wiring schedules client removal after the pool grace period. Ingest workers now use caller-provided client caches. Tests cover cache retention, idle connection closure, and reload behavior.

Changes

ClickHouse HTTP client lifecycle

Layer / File(s)	Summary
Report pool releases and retire cached clients internal/chconn/chconn.go, internal/chconn/chconn_test.go, CHANGELOG.md, AGENTS.md, docs/src/content/docs/architecture.md	Pools.Reconcile returns released tuples with their TLS configuration and grace period. HTTPClients.Release removes matching clients after that period and closes idle connections. The nil-config client remains shared.
Register client caches and release them on reload internal/app/*, internal/api/query.go, internal/api/query_test.go, internal/ingest/worker.go, internal/ingest/worker_test.go, tests/integration/*, tests/e2e/sdk/*	The app registers the ingest worker and query handler client caches and releases clients for reconciled tuples. StartIngestWorker accepts a caller-provided cache. Tests cover reload behavior, connection closure, and a ClickHouse TLS configuration change.

Priority: ➖ Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant App
  participant Pools
  participant HTTPClients
  App->>Pools: Reconcile wanted members
  Pools-->>App: Return released tuples
  App->>HTTPClients: Release each tuple
  HTTPClients->>HTTPClients: After grace, remove client and close idle connections

Loading

Suggested reviewers: ericandrechek

Merge Risk: ⚪ Minimal · up to 95225

The client cleanup appears to preserve clients for active tenants, including when a reload reports another pool error. No issue identified here needs resolution before merge.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 79.07% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 43 functions across 13 files. (3 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	Issue #713 requires HTTP clients to follow native ClickHouse pool retirement. The PR returns released tuples from Pools.Reconcile, including TLS identity and grace. wireClickHouse releases each tu…
Out of Scope Changes check	✅ Passed	The changed code supports issue #713. The worker cache ownership, query-handler accessor, application wiring, lifecycle tests, documentation, and SDK reload test verify or document client release duri…
Title check	✅ Passed	The title clearly and concisely describes the main change: releasing a tuple's HTTP clients with its ClickHouse pool.
Description check	✅ Passed	The description directly explains the client lifecycle change, implementation approach, and test coverage described by the changeset.

Full details: Docstring Coverage

Explanation

Docstring coverage is 79.07% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 43 functions across 13 files. (3 skipped: 3 unsupported.)

✨ 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

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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://a21088a1-wavehouse-docs.wave-rf.workers.dev

• Commit — 29fbb0b: Merge remote-tracking branch 'origin/main' into fix/http-clients-follow-pools
• Author — @taitelee
• Committed — 2026-10-08 15:26 (UTC-04:00)
• Deployed — 2026-10-08 15:34 EDT

[github-code-quality]

Code Coverage Overview

Languages: Go

Go

The overall line coverage in commit 29fbb0b in the fix/http-clients-fol... branch remains at 93%, unchanged from commit 1227ed0 in the main branch.

Show a line coverage summary of the most impacted files.

File	main 1227ed0	fix/http-clients-fol... 29fbb0b	+/-
internal/ingest/worker.go	97%	97%	0%
internal/chconn/chconn.go	93%	93%	0%
internal/app/wire.go	93%	93%	0%
internal/api/query.go	89%	90%	+1%

---

_{Updated October 08, 2026 19:36 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.