feat(settings): hot-reload the settings directory at boot and runtime - #508
feat(settings): hot-reload the settings directory at boot and runtime#508taitelee wants to merge 14 commits into
Conversation
… required keys, and init-settings
github-actions
Bot
added
documentation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughWaveHouse moves tenant tunables into a required JSON settings directory. The application validates and hot-reloads settings from file changes, ChangesSettings directory runtime
Boot, commands, and deployment
Documentation and fixtures
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟠 High · up to This PR makes settings reloadable at boot and runtime, but the current head can still silently discard valid cross-table records, leave runtime settings stale after filesystem event overflow, and cause container boot failures when the documented settings path is followed; these correctness, availability, and deployment-readiness risks should be fixed or explicitly accepted before merge. Sequence Diagram(s)sequenceDiagram
participant Operator
participant WaveHouse
participant SettingsStore
participant DedupeManaged
participant APIConsumers
Operator->>WaveHouse: Edit settings, send SIGHUP, or call reload endpoint
WaveHouse->>SettingsStore: TriggerReload(trigger)
SettingsStore->>SettingsStore: Validate and adopt valid snapshot
SettingsStore->>DedupeManaged: Apply dedupe.enabled
SettingsStore-->>APIConsumers: Updated query, schema, and CORS accessors
WaveHouse-->>Operator: Adoption status and findings
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 44.44% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 90 functions across 28 files. (4 skipped: 4 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
📚 Docs preview is live → https://115384d2-wavehouse-docs.wave-rf.workers.dev |
Code Coverage OverviewLanguages: Go GoThe overall line coverage in commit 82b3457 in the Show a line coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e472820d-ea24-4cec-91dd-ebe59cdf26b7
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (50)
.goreleaser.yaml.testcoverage.ymlAGENTS.mdCHANGELOG.mdcmd/wavehouse/init_settings.gocmd/wavehouse/init_settings_test.gocmd/wavehouse/main.gocmd/wavehouse/validate_test.goconfig.yamldeployments/Dockerfiledeployments/Dockerfile.goreleaserdeployments/compose/standalone.yamldocs/src/content/docs/api.mddocs/src/content/docs/configuration.mdxdocs/src/content/docs/deployment.mddocs/src/content/docs/development.mddocs/src/content/docs/getting-started.mddocs/src/content/docs/reverse-proxy.mdxdocs/src/content/docs/sdk/index.mdxgo.modinternal/api/ingest.gointernal/api/ingest_test.gointernal/api/router.gointernal/api/router_test.gointernal/api/settings.gointernal/api/settings_test.gointernal/api/structured_query.gointernal/api/structured_query_test.gointernal/config/config.gointernal/config/config_test.gointernal/discovery/discovery.gointernal/settings/finding.gointernal/settings/seed.gointernal/settings/seed/config.jsoninternal/settings/seed/pipes.jsoninternal/settings/seed/policies.jsoninternal/settings/seed/roles.jsoninternal/settings/settings.gointernal/settings/store.gointernal/settings/store_test.gointernal/settings/validate.gointernal/settings/validate_test.gointernal/settings/watch.gointernal/settings/watch_test.gotests/e2e/fixtures/config.yamltests/e2e/fixtures/settings/config.jsontests/e2e/fixtures/settings/pipes.jsontests/e2e/fixtures/settings/policies.jsontests/e2e/fixtures/settings/roles.jsontests/integration/query_limits_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. (2)
- GitHub Check: E2E tests
- GitHub Check: Coverage
🧰 Additional context used
📓 Path-based instructions (9)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: - No global state: Dependencies are passed explicitly (constructor injection).
- Go 1.26, strict formatting (
gofumpt, enforced by CI)
Files:
tests/integration/query_limits_test.gocmd/wavehouse/validate_test.gointernal/settings/finding.gocmd/wavehouse/init_settings.gointernal/api/settings_test.gointernal/settings/watch_test.gointernal/api/structured_query_test.gointernal/settings/seed.gointernal/api/structured_query.gocmd/wavehouse/init_settings_test.gointernal/api/ingest.gointernal/settings/watch.gointernal/api/settings.gointernal/settings/settings.gointernal/settings/validate.gointernal/discovery/discovery.gointernal/settings/validate_test.gointernal/api/router.gointernal/api/ingest_test.gointernal/settings/store_test.gointernal/config/config.gointernal/settings/store.gointernal/api/router_test.gointernal/config/config_test.gocmd/wavehouse/main.go
tests/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
- Table-driven tests: Use
tests := []struct{ name string; ... }witht.Run(tt.name, ...)for test cases.
Files:
tests/integration/query_limits_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
- Every new function should have corresponding test cases. Run
make lintandmake testbefore considering work complete.
Files:
tests/integration/query_limits_test.gocmd/wavehouse/validate_test.gointernal/api/settings_test.gointernal/settings/watch_test.gointernal/api/structured_query_test.gocmd/wavehouse/init_settings_test.gointernal/settings/validate_test.gointernal/api/ingest_test.gointernal/settings/store_test.gointernal/api/router_test.gointernal/config/config_test.go
internal/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
- Package naming: Lowercase, single word (or abbreviated).
internal/enforces module privacy.
Files:
internal/settings/finding.gointernal/api/settings_test.gointernal/settings/watch_test.gointernal/api/structured_query_test.gointernal/settings/seed.gointernal/api/structured_query.gointernal/api/ingest.gointernal/settings/watch.gointernal/api/settings.gointernal/settings/settings.gointernal/settings/validate.gointernal/discovery/discovery.gointernal/settings/validate_test.gointernal/api/router.gointernal/api/ingest_test.gointernal/settings/store_test.gointernal/config/config.gointernal/settings/store.gointernal/api/router_test.gointernal/config/config_test.go
internal/api/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
- Bearer-token-only CORS posture (security) — Bearer JWT on every request, no cookies/sessions;
corsMiddlewaredeliberately never emitsAccess-Control-Allow-Credentials(not needed, and*+ credentials is a spec violation browsers reject).
Files:
internal/api/settings_test.gointernal/api/structured_query_test.gointernal/api/structured_query.gointernal/api/ingest.gointernal/api/settings.gointernal/api/router.gointernal/api/ingest_test.gointernal/api/router_test.go
**/*.mdx
📄 CodeRabbit inference engine (AGENTS.md)
- In MDX, leave a blank line between a JSX tag and a code fence.
Files:
docs/src/content/docs/sdk/index.mdxdocs/src/content/docs/reverse-proxy.mdxdocs/src/content/docs/configuration.mdx
docs/src/content/**/*.mdx
📄 CodeRabbit inference engine (AGENTS.md)
docs/src/content/**/*.mdx: - Opt a page into the Cloud CTA withcloudCtafrontmatter, not by importing the component.
- Never hand-write
®or™in prose.- Never hand-write
utm_*params orrelon a link towavehouse.cloudorwave-rf.com. UsecloudLink()/relFor()fromdocs/src/config/outbound.ts.
Files:
docs/src/content/docs/sdk/index.mdxdocs/src/content/docs/reverse-proxy.mdxdocs/src/content/docs/configuration.mdx
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
- Never hard-wrap prose. One paragraph is one line. No wrapping at 72/80 columns, no "semantic linefeeds" splitting a paragraph at sentence boundaries.
Files:
docs/src/content/docs/getting-started.mddocs/src/content/docs/development.mdAGENTS.mddocs/src/content/docs/api.mdCHANGELOG.mddocs/src/content/docs/deployment.md
**/*.{yml,yaml}
📄 CodeRabbit inference engine (AGENTS.md)
New workflows must follow the same pattern — never
@mainor floating tags on third-party actions.
Files:
deployments/compose/standalone.yamltests/e2e/fixtures/config.yamlconfig.yaml
🧠 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/settings/validate_test.gointernal/settings/store_test.go
📚 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/configuration.mdx
🪛 Checkov (3.3.10)
deployments/Dockerfile
[low] 42-42: Ensure the base image uses a non latest version tag
(CKV_DOCKER_7)
🪛 Hadolint (2.15.1)
deployments/Dockerfile
[warning] 42-42: Always tag the version of an image explicitly
(DL3006)
[info] 45-45: Non-numeric user-id may not be resolvable by host system
(DL3066)
🪛 LanguageTool
CHANGELOG.md
[style] ~13-~13: Consider an alternative for the overused word “exactly”.
Context: ...issing one), so the adopted snapshot is exactly what the files say, and once adopted it...
(EXACTLY_PRECISELY)
[style] ~13-~13: Since ownership is already implied, this phrasing may be redundant.
Context: ...et (bare docker run boots; bind-mount your own directory over it), and the dev `config...
(PRP_OWN)
docs/src/content/docs/configuration.mdx
[style] ~188-~188: Consider an alternative for the overused word “exactly”.
Context: ...hey live, and what the server adopts is exactly what the files say. The container image...
(EXACTLY_PRECISELY)
[style] ~188-~188: Since ownership is already implied, this phrasing may be redundant.
Context: ...o a bare docker run boots; bind-mount your own directory over that path to edit. Chec...
(PRP_OWN)
[style] ~204-~204: Consider using the typographical ellipsis character here instead.
Context: ...ttings/reload** (admin-only) — returns {"adopted": bool, "findings": [...]}; 200when adopted,422` when reje...
(ELLIPSIS)
🪛 Trivy (0.73.0)
deployments/Dockerfile
[warning] 42-42: ':latest' tag used
Specify a tag in the 'FROM' statement for image 'gcr.io/distroless/static-debian12'
Rule: DS-0001
(IaC/Dockerfile)
🔇 Additional comments (41)
AGENTS.md (2)
61-62: LGTM!Also applies to: 65-65
57-57: 🗄️ Data Integrity & Integration | 🏗️ Heavy liftMake the Issue
#222table-isolation contract explicit and executable.The changes mention per-table
id_fieldselection but do not represent the required table-qualified deduplication keyspace in the invariant or the e2e settings fixture.
AGENTS.md#L57-L57: state that deduplication keys include table identity and that writes and lookups use the same namespace.tests/e2e/fixtures/settings/config.json#L1-L16: add table overrides and test equal IDs in different tables, or link an existing test that proves both behaviors..testcoverage.yml (1)
75-86: LGTM!config.yaml (1)
53-55: LGTM!Also applies to: 85-99
tests/e2e/fixtures/config.yaml (1)
8-14: LGTM!Also applies to: 31-36
tests/e2e/fixtures/settings/pipes.json (1)
1-1: LGTM!tests/e2e/fixtures/settings/policies.json (1)
1-3: LGTM!tests/e2e/fixtures/settings/roles.json (1)
1-3: LGTM!docs/src/content/docs/api.md (1)
597-597: LGTM!Also applies to: 799-813
docs/src/content/docs/configuration.mdx (1)
26-27: LGTM!Also applies to: 42-43, 70-70, 116-116, 131-137, 188-236, 303-304, 328-331, 392-392
docs/src/content/docs/deployment.md (1)
62-62: LGTM!Also applies to: 157-171, 369-369
docs/src/content/docs/development.md (1)
147-147: LGTM!Also applies to: 231-238
docs/src/content/docs/getting-started.md (1)
98-98: LGTM!Also applies to: 116-116
docs/src/content/docs/reverse-proxy.mdx (1)
25-25: LGTM!Also applies to: 199-199
docs/src/content/docs/sdk/index.mdx (1)
404-404: LGTM!go.mod (1)
23-23: LGTM!internal/settings/finding.go (1)
18-24: LGTM!internal/settings/store_test.go (1)
13-157: LGTM!cmd/wavehouse/validate_test.go (1)
22-22: LGTM!.goreleaser.yaml (1)
85-88: LGTM!internal/api/settings.go (1)
11-51: LGTM!internal/api/structured_query.go (1)
30-35: LGTM!Also applies to: 52-52, 122-126
internal/settings/seed.go (1)
1-61: LGTM!internal/settings/seed/config.json (1)
1-16: LGTM!internal/settings/seed/pipes.json (1)
1-4: LGTM!internal/settings/seed/policies.json (1)
1-2: LGTM!internal/settings/seed/roles.json (1)
1-4: LGTM!internal/settings/validate_test.go (1)
4-4: LGTM!Also applies to: 30-64, 96-96, 264-271, 335-335, 358-368, 378-378
tests/integration/query_limits_test.go (1)
102-102: LGTM!internal/settings/settings.go (1)
63-84: LGTM!Also applies to: 98-118
internal/settings/validate.go (1)
286-309: LGTM!Also applies to: 338-362
internal/settings/store.go (1)
10-131: LGTM!internal/config/config.go (1)
13-32: LGTM!Also applies to: 45-52, 108-112, 142-147, 222-224
internal/config/config_test.go (1)
14-34: LGTM!Also applies to: 101-107, 189-190, 203-216, 227-228, 239-241, 263-264, 282-283, 302-303, 319-320, 344-345, 372-373, 391-392, 427-428, 445-446, 458-459, 472-473, 487-488
cmd/wavehouse/init_settings.go (1)
14-50: LGTM!cmd/wavehouse/main.go (1)
31-31: LGTM!Also applies to: 102-116, 136-137, 177-188, 295-331, 473-473, 531-537
cmd/wavehouse/init_settings_test.go (1)
12-42: LGTM!internal/api/ingest.go (1)
39-45: LGTM!internal/api/router.go (1)
20-43: LGTM!Also applies to: 196-198, 299-341
internal/api/structured_query_test.go (1)
32-43: LGTM!Also applies to: 285-298
internal/discovery/discovery.go (1)
49-59: LGTM!Also applies to: 217-255
github-project-automation
Bot
moved this from In progress
to In review
in WaveHouse Task Board
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/settings/watch.go (1)
80-86: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReconcile settings after a watcher queue overflow.
fsnotify.ErrEventOverflowindicates that filesystem notifications were lost. When this error occurs, reset the debounce timer so its normal path callss.TriggerReload("watch"). That reload reparses the complete settings directory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b2f6f5c1-0bde-4b64-8caa-cd85c395c4cc
📒 Files selected for processing (6)
CHANGELOG.mddeployments/compose/standalone.yamldocs/src/content/docs/configuration.mdxinternal/api/router_test.gointernal/settings/watch.gointernal/settings/watch_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. (10)
- GitHub Check: E2E tests
- GitHub Check: Integration tests
- GitHub Check: Docs build
- GitHub Check: Unit tests
- GitHub Check: Coverage
- GitHub Check: Validate snapshot build
- GitHub Check: Lint
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: - Go 1.26, strict formatting (gofumpt, enforced by CI)
- No global state: Dependencies are passed explicitly (constructor injection).
- Structured logging with
log/slog(JSON handler)
Files:
internal/settings/watch.gointernal/api/router_test.gointernal/settings/watch_test.go
internal/*/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
- Package naming: Lowercase, single word (or abbreviated).
internal/enforces module privacy.
Files:
internal/settings/watch.gointernal/api/router_test.gointernal/settings/watch_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: - Table-driven tests: Usetests := []struct{ name string; ... }witht.Run(tt.name, ...)for test cases.
- Every new function should have corresponding test cases. Run
make lintandmake testbefore considering work complete.
Files:
internal/api/router_test.gointernal/settings/watch_test.go
docs/src/content/docs/**/*.mdx
📄 CodeRabbit inference engine (AGENTS.md)
docs/src/content/docs/**/*.mdx: - Opt a page into the Cloud CTA withcloudCtafrontmatter, not by importing the component.
- Never hand-write
®or™in prose.- Never hand-write
utm_*params orrelon a link towavehouse.cloudorwave-rf.com. UsecloudLink()/relFor()fromdocs/src/config/outbound.ts.
Files:
docs/src/content/docs/configuration.mdx
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Every code change should update the corresponding docs in the same PR. A code change without its doc update is incomplete.
Files:
CHANGELOG.md
🧠 Learnings (2)
📚 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/configuration.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/settings/watch_test.go
🪛 LanguageTool
CHANGELOG.md
[style] ~13-~13: Since ownership is already implied, this phrasing may be redundant.
Context: ...et (bare docker run boots; bind-mount your own directory over it), and the dev `config...
(PRP_OWN)
🔇 Additional comments (6)
internal/api/router_test.go (1)
8-8: LGTM!Also applies to: 160-160, 180-180, 198-198, 217-217, 256-256, 273-273, 291-291, 406-406
CHANGELOG.md (1)
13-13: LGTM!docs/src/content/docs/configuration.mdx (1)
26-27: LGTM!Also applies to: 42-43, 70-70, 116-116, 131-137, 188-236, 283-283, 298-304, 315-315, 328-331, 360-360, 373-376, 386-392
deployments/compose/standalone.yaml (1)
42-58: LGTM!internal/settings/watch.go (1)
6-6: LGTM!Also applies to: 17-17, 26-79
internal/settings/watch_test.go (1)
56-67: LGTM!Also applies to: 70-92
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 690b4cec-07ea-43f0-86b2-faf02d9b7696
📒 Files selected for processing (1)
internal/settings/watch_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 (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Return errors, don't panic. Wrap with
fmt.Errorf("context: %w", err).
Files:
internal/settings/watch_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Every new function should have corresponding test cases.
Files:
internal/settings/watch_test.go
🔇 Additional comments (3)
internal/settings/watch_test.go (3)
5-5: LGTM!Also applies to: 18-28, 32-34
36-43: LGTM!Also applies to: 54-64
76-100: LGTM!
|
@coderabbitai review |
✅ Action performedReview finished.
|
| # them un-deduped (default false → logged + counted, not rejected). | ||
| WH_DEDUPE_REQUIRE_ID: "false" | ||
| WH_SCHEMA_REFRESH_INTERVAL: "60" | ||
| WH_DLQ_ENABLED: "true" |
There was a problem hiding this comment.
Why are these other config things not moved over too?
There was a problem hiding this comment.
I made a split on this based on ownership and lifecycle rather than what simply looks like configuration. Everything left in the environment variables controls resources that run for the entire process. The dedupe variable manages the Pebble store, DLQ variable creates the NATS stream, and the data directory and CH variables define where state lives. These need a full restart to change, so I kept them here The policy file path and pipes directory are the only exceptions. They are legacy seed paths that I'll remove in the next PR.
There was a problem hiding this comment.
Can the DLQ setting (enabled or not) not move into the hot-reload piece too? Like it would presumably be on per-tenant/table, no?
| | `dedupe.enabled` | `WH_DEDUPE_ENABLED` | `false` | Enable event deduplication. When enabled, the ingest handler checks for duplicates using the configured ID field. | | ||
| | `dedupe.id_field` | `WH_DEDUPE_ID_FIELD` | `event_id` | JSON field name in the ingest body used as the dedup key. | | ||
| | `dedupe.require_id` | `WH_DEDUPE_REQUIRE_ID` | `false` | With dedupe enabled, controls what happens to a row missing `id_field` (which can't be deduped, so idempotency wouldn't apply to it). Such a row is always logged at `WARN` and counted by `wavehouse_ingest_dedupe_missing_id_total`, in both modes. Default (`false`): it is then published un-deduped. Set `true` to reject it instead (`400` for a single insert; a per-record failure in a batch) — a server-side tripwire for producers that must guarantee the id. | | ||
| | `dedupe.enabled` | `WH_DEDUPE_ENABLED` | `false` | Enable event deduplication. Boot config because it owns the embedded Pebble store's lifecycle, which only a restart can change. | |
There was a problem hiding this comment.
I'd like pebble and dedupe settings to be able to become hot-reloadable too
| // seedFS holds the starter settings directory: every file present, every | ||
| // key set to its default. It is the ONE place defaults live — the binary | ||
| // has no compiled fallbacks — and it ships three ways: `wavehouse | ||
| // init-settings` writes it out, the container images bake it at | ||
| // /app/settings, and the compose quickstart serves from that copy. | ||
| // | ||
| //go:embed seed/*.json | ||
| var seedFS embed.FS |
There was a problem hiding this comment.
I don't understand this, why do it so many different ways? Like if we are aiming to embed it in the go binary, then why have the docker image builder do it a different way?
There was a problem hiding this comment.
There is only one source for the seed. It uses the files under internal/settings/seed. Two things use this folder. The binary embeds the files directly. This allows the init-settings command to write them anywhere without needing the source tree. The container images just copy the exact same directory. I could not run init-settings during the image build. The build platform creates the goreleaser image but the target platform compiles the binary. Running it during the build would require QEMU. I wrote the Dockerfile specifically to avoid that. The only real inconsistency was how the two Dockerfiles copied the files. I fixed that (comment somewhere above). They both use the exact same copy line now. I also updated the comment to explain this setup instead of saying it ships three ways (on next push).
…st, split settings-directory docs
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config.yaml (1)
52-53: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRemove the stale boot-level
dedupe.enabledkey.The settings migration moves
dedupe.enabledintoconfig.jsonand removesWH_DEDUPE_ENABLED. This file still declaresdedupe.enabled: false, so the development configuration does not follow the new contract. Remove this block and let the settings directory control deduplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 842876f6-f897-43f8-bac6-c712d5521fb9
📒 Files selected for processing (36)
AGENTS.mdCHANGELOG.mdMakefileREADME.mdcmd/wavehouse/main.gocmd/wavehouse/validate_test.goconfig.yamldeployments/Dockerfiledeployments/Dockerfile.goreleaserdeployments/compose/standalone.yamldocs/src/config/sidebar.tsdocs/src/content/docs/api.mddocs/src/content/docs/configuration.mdxdocs/src/content/docs/deployment.mddocs/src/content/docs/development.mddocs/src/content/docs/getting-started.mddocs/src/content/docs/index.mdxdocs/src/content/docs/reverse-proxy.mdxdocs/src/content/docs/settings-directory.mdxinternal/api/ingest.gointernal/api/ingest_test.gointernal/api/router_test.gointernal/api/settings_test.gointernal/config/config.gointernal/config/config_test.gointernal/dedupe/managed.gointernal/dedupe/managed_test.gointernal/settings/seed.gointernal/settings/seed/config.jsoninternal/settings/settings.gointernal/settings/store.gointernal/settings/store_test.gointernal/settings/validate.gointernal/settings/validate_test.gotests/e2e/fixtures/config.yamltests/e2e/fixtures/settings/config.json
💤 Files with no reviewable changes (2)
- internal/config/config_test.go
- internal/config/config.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 (6)
**/*.mdx
📄 CodeRabbit inference engine (AGENTS.md)
- In MDX, leave a blank line between a JSX tag and a code fence.
Files:
docs/src/content/docs/index.mdxdocs/src/content/docs/settings-directory.mdxdocs/src/content/docs/configuration.mdxdocs/src/content/docs/reverse-proxy.mdx
docs/src/content/**/*.mdx
📄 CodeRabbit inference engine (AGENTS.md)
docs/src/content/**/*.mdx: - Opt a page into the Cloud CTA withcloudCtafrontmatter, not by importing the component.
- Never hand-write
®or™in prose.- Never hand-write
utm_*params orrelon a link towavehouse.cloudorwave-rf.com. UsecloudLink()/relFor()fromdocs/src/config/outbound.ts.- Default to top-down:
flowchart TB/TD, anddirection TBinside subgraphs — notLR/RL.
Files:
docs/src/content/docs/index.mdxdocs/src/content/docs/settings-directory.mdxdocs/src/content/docs/configuration.mdxdocs/src/content/docs/reverse-proxy.mdx
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
- Never hard-wrap prose. One paragraph is one line. No wrapping at 72/80 columns, no "semantic linefeeds" splitting a paragraph at sentence boundaries.
Files:
README.mdCHANGELOG.mdAGENTS.mddocs/src/content/docs/development.mddocs/src/content/docs/deployment.mddocs/src/content/docs/getting-started.mddocs/src/content/docs/api.md
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
- No global state: Dependencies are passed explicitly (constructor injection).
Files:
internal/settings/seed.gointernal/dedupe/managed_test.gointernal/api/ingest.gocmd/wavehouse/main.gointernal/settings/settings.gointernal/settings/validate.gocmd/wavehouse/validate_test.gointernal/api/settings_test.gointernal/settings/store_test.gointernal/dedupe/managed.gointernal/api/router_test.gointernal/settings/validate_test.gointernal/api/ingest_test.gointernal/settings/store.go
CHANGELOG.md
📄 CodeRabbit inference engine (AGENTS.md)
Every code change should update the corresponding docs in the same PR. A code change without its doc update is incomplete.
Files:
CHANGELOG.md
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: - Table-driven tests: Usetests := []struct{ name string; ... }witht.Run(tt.name, ...)for test cases.
- Every new function should have corresponding test cases. Run
make lintandmake testbefore considering work complete.
Files:
internal/dedupe/managed_test.gocmd/wavehouse/validate_test.gointernal/api/settings_test.gointernal/settings/store_test.gointernal/api/router_test.gointernal/settings/validate_test.gointernal/api/ingest_test.go
🧠 Learnings (2)
📚 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.mdxdocs/src/content/docs/configuration.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/api/router_test.gointernal/settings/validate_test.go
🪛 LanguageTool
docs/src/content/docs/settings-directory.mdx
[style] ~14-~14: Consider an alternative for the overused word “exactly”.
Context: ...hey live, and what the server adopts is exactly what the files say. The container image...
(EXACTLY_PRECISELY)
[style] ~14-~14: Since ownership is already implied, this phrasing may be redundant.
Context: ...o a bare docker run boots; bind-mount your own directory over that path to edit. Chec...
(PRP_OWN)
[style] ~28-~28: Consider using the typographical ellipsis character here instead.
Context: ...ttings/reload** (admin-only) — returns {"adopted": bool, "findings": [...]}; 200when adopted,422` when reje...
(ELLIPSIS)
CHANGELOG.md
[style] ~13-~13: Since ownership is already implied, this phrasing may be redundant.
Context: ...t-reloadable half of configuration gets its own page; configuration.mdx is boot confi...
(PRP_OWN)
[style] ~13-~13: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...doption, matching wavehouse validate. The tenant tunables migrate out of boot config into the directory's config.json: dedupe.id_field / dedupe.require_id (now with the per-table overrides under dedupe.tables that #222 asked for, resolved per record through the table → global cascade in one atomic snapshot read, so a reload lands at a record boundary and never mixes documents within one record), query.default_max_rows (read per query), schema.refresh_interval (re-read after each tick, so a change applies from the next cycle), and the CORS allowlist (cors.allowed_origins, resolved per request; an empty list [] means allow-all, same as ["*"] — and now that the key is hand-written and required, validation warns on [] so the natural-looking spelling for "no origins" can't hot-reload into allow-all unannounced). The corresponding YAML/env keys are *...
(TOO_LONG_SENTENCE)
[style] ~13-~13: Since ownership is already implied, this phrasing may be redundant.
Context: ...et (bare docker run boots; bind-mount your own directory over it), and the dev `config...
(PRP_OWN)
docs/src/content/docs/development.md
[style] ~141-~141: Consider using the typographical ellipsis character here instead.
Context: ... air -c .air.toml ``` deps-up runs `docker compose ... up -d --wait clickhouse`, which blocks ...
(ELLIPSIS)
[style] ~231-~231: Since ownership is already implied, this phrasing may be redundant.
Context: ...ips with dedupe.enabled: false. Write your own settings directory and flip it there (t...
(PRP_OWN)
docs/src/content/docs/configuration.mdx
[style] ~178-~178: Since ownership is already implied, this phrasing may be redundant.
Context: ... validate / init-settings CLI — has its own page: [Settings Directory](/settings-di...
(PRP_OWN)
🔇 Additional comments (17)
AGENTS.md (1)
36-36: LGTM!Also applies to: 57-65
CHANGELOG.md (1)
13-13: LGTM!config.yaml (1)
8-9: LGTM!Also applies to: 39-40, 67-68, 79-93
docs/src/content/docs/api.md (1)
597-597: LGTM!Also applies to: 799-813
docs/src/config/sidebar.ts (1)
48-48: LGTM!tests/e2e/fixtures/settings/config.json (1)
1-17: LGTM!tests/e2e/fixtures/config.yaml (1)
8-19: LGTM!Also applies to: 28-33
internal/settings/seed.go (1)
12-18: LGTM!internal/settings/seed/config.json (1)
3-3: LGTM!internal/settings/settings.go (1)
75-90: LGTM!internal/settings/store.go (1)
32-36: LGTM!Also applies to: 66-80, 117-138
internal/settings/validate.go (1)
321-323: LGTM!Also applies to: 361-372
internal/settings/store_test.go (1)
77-77: LGTM!Also applies to: 113-113, 133-133, 159-177
internal/settings/validate_test.go (1)
38-62: LGTM!Also applies to: 264-275, 336-337
cmd/wavehouse/main.go (1)
177-188: LGTM!Also applies to: 295-382, 487-488, 545-551
cmd/wavehouse/validate_test.go (1)
22-22: LGTM!deployments/Dockerfile (1)
34-41: LGTM!
…plus review doc/test nits
|
@coderabbitai review |
✅ Action performedReview finished.
|
…ttings, mount or seed a volume
| } | ||
| defer func() { _ = w.Close() }() | ||
| dir := filepath.Clean(s.dir) | ||
| if err := w.Add(dir); err != nil { |
There was a problem hiding this comment.
We only start listening here, but we read the files way earlier in settings.Open. Anything edited in between fires no event, so it just never gets picked up — no log, nothing. That gap has the whole ClickHouse dial in it.
The case that worries me is a ConfigMap update landing during a rolling restart. One burst, nothing touches the dir again, so those pods sit on stale settings until someone bounces them, and they all look healthy the entire time.
Can we just TriggerReload("watch") right after the Add? watch_test.go already retries the write in a loop to dodge this same race, so we kind of already know about it.
There was a problem hiding this comment.
Yes. This actually fixes the race condition that would occur in main.go since the watcher used to start before the dedupe hook was registered. Moving the reload trigger to start after every AfterAdopt hook is in place.
| } | ||
| defer func() { _ = dedup.Close() }() | ||
| } | ||
| settingsStore.AfterAdopt(func() { |
There was a problem hiding this comment.
Can this move above the boot apply? The read at 365 isn't under s.mu, so if a reload lands between there and here there's no hook registered yet and we end up with the settings saying dedupe is on while Managed is still off.
Different problem from the watch.go one I commented on — there the settings never make it into memory, here they do and we just don't act on them.
Window is tiny so I'm not holding the PR on it, but worth flagging that ed5c9ce made it quieter: that state used to be a wall of 500s, now it takes the new ErrDisabled path and silently publishes un-deduped instead. Apply is idempotent so hoisting this and having boot call the same closure should be fine, no?
There was a problem hiding this comment.
Yes. There is only one reconcileDedupe closure now. I registered it with AfterAdopt before the boot apply calls it. Either the hook sees a reload in the middle or the boot apply reads it. There is no race window either way. I moved the SIGHUP and watcher goroutines below this block for the same reason as the previous comment.
| // read above and this call (the two transition at | ||
| // different instants). Publish un-deduped, as a record | ||
| // under the other setting would have been. | ||
| h.logger.WarnContext(ctx, "dedupe switched off mid-reload; publishing without idempotency", "event_id", eventID, "table", table) |
There was a problem hiding this comment.
Can we get a counter next to this like the missing-id case does at 437? WARN exports at 100%, so a busy ingest crossing a reload is a pile of lines with nothing aggregating them.
The bigger reason though — if that main.go ordering race ever latches, this stops being transient and goes unbounded, every record forever, and a counter is the only way you'd ever notice. Probably drop the log line to Debug once the counter is carrying it.
There was a problem hiding this comment.
Adding wavehouse_ingest_dedupe_disabled_total{table} next to the missing-id counter and dropping the line to Debug
…t apply, count reload-window skips; boot-config docs omit tunables
|
The configuration file is only for boot settings now. I removed the intro line about tunables and the CORS sentence. I also deleted the Query, Schema Discovery, and Deduplication sections. The settings directory row is the only mention of settings left in the document. I moved all the details from those old sections into the settings directory file, includes the notes on result shaping, resource limits, refresh cycles, and the on demand endpoint. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2b26c918-4bd0-41e2-bbfe-52ee86eee647
📒 Files selected for processing (15)
CHANGELOG.mdREADME.mdcmd/wavehouse/main.godeployments/Dockerfiledeployments/Dockerfile.goreleaserdeployments/compose/standalone.yamldocs/src/content/docs/configuration.mdxdocs/src/content/docs/deployment.mddocs/src/content/docs/getting-started.mddocs/src/content/docs/settings-directory.mdxinternal/api/ingest.gointernal/config/config.gointernal/settings/seed.gointernal/settings/watch.gointernal/settings/watch_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. (1)
- GitHub Check: Analyze (actions)
🧰 Additional context used
📓 Path-based instructions (8)
- **Column-level access control is a hard cap on every read path.**
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal/api/ingest.go
- **Opt a page into the Cloud CTA with `cloudCta` frontmatter**, not by importing the component.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/content/docs/settings-directory.mdxdocs/src/content/docs/configuration.mdx
- **Table-driven tests**: Use `tests := []struct{ name string; ... }` with `t.Run(tt.name, ...)` for test cases.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal/settings/watch_test.go
- **Every new function should have corresponding test cases.** Run `make lint` and `make test` before considering work complete.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal/settings/watch.gointernal/settings/seed.gocmd/wavehouse/main.gointernal/settings/watch_test.gointernal/config/config.gointernal/api/ingest.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/settings-directory.mdxdocs/src/content/docs/configuration.mdx
- **No global state**: Dependencies are passed explicitly (constructor injection).
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal/settings/watch.gointernal/settings/seed.gocmd/wavehouse/main.gointernal/settings/watch_test.gointernal/config/config.gointernal/api/ingest.go
Every code change should update the corresponding docs in the same PR. A code change without its doc update is incomplete.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
deployments/DockerfileREADME.mdinternal/settings/watch.goCHANGELOG.mddocs/src/content/docs/getting-started.mdinternal/settings/seed.godocs/src/content/docs/settings-directory.mdxdeployments/compose/standalone.yamlcmd/wavehouse/main.godocs/src/content/docs/configuration.mdxinternal/settings/watch_test.godocs/src/content/docs/deployment.mdinternal/config/config.godeployments/Dockerfile.goreleaserinternal/api/ingest.go
- **Never hard-wrap prose. One paragraph is one line.** No wrapping at 72/80 columns, no "semantic linefeeds" splitting a paragraph at sentence boundaries.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
README.mdCHANGELOG.mddocs/src/content/docs/getting-started.mddocs/src/content/docs/deployment.md
🧠 Learnings (2)
📚 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/settings/watch_test.go
🪛 LanguageTool
CHANGELOG.md
[style] ~13-~13: Since ownership is already implied, this phrasing may be redundant.
Context: ...t-reloadable half of configuration gets its own page; configuration.mdx is boot confi...
(PRP_OWN)
[style] ~13-~13: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...doption, matching wavehouse validate. The tenant tunables migrate out of boot config into the directory's config.json: dedupe.id_field / dedupe.require_id (now with the per-table overrides under dedupe.tables that #222 asked for, resolved per record through the table → global cascade in one atomic snapshot read, so a reload lands at a record boundary and never mixes documents within one record), query.default_max_rows (read per query), schema.refresh_interval (re-read after each tick, so a change applies from the next cycle), and the CORS allowlist (cors.allowed_origins, resolved per request; an empty list [] means allow-all, same as ["*"] — and now that the key is hand-written and required, validation warns on [] so the natural-looking spelling for "no origins" can't hot-reload into allow-all unannounced). The corresponding YAML/env keys are *...
(TOO_LONG_SENTENCE)
docs/src/content/docs/settings-directory.mdx
[style] ~14-~14: Consider an alternative for the overused word “exactly”.
Context: ...hey live, and what the server adopts is exactly what the files say. The container image...
(EXACTLY_PRECISELY)
🔇 Additional comments (7)
CHANGELOG.md (1)
13-13: LGTM!README.md (1)
135-135: LGTM!docs/src/content/docs/configuration.mdx (1)
26-26: LGTM!Also applies to: 42-42, 66-66, 106-106, 115-115, 162-166, 214-214, 228-231, 254-258
docs/src/content/docs/deployment.md (1)
19-20: LGTM!Also applies to: 185-185, 367-367
docs/src/content/docs/getting-started.md (1)
23-24: LGTM!docs/src/content/docs/settings-directory.mdx (1)
14-18: LGTM!Also applies to: 44-45, 70-70
cmd/wavehouse/main.go (1)
356-360: 🗄️ Data Integrity & IntegrationNo change needed for this concern. When
Managed.Apply(true)fails,CheckAndMarkreturnsdedupe.ErrUnavailable, andinternal/api/ingest.goaborts with HTTP 500 instead of publishing without idempotency.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 29710ce4-d737-4167-9724-0b3557074aa1
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
CHANGELOG.mddocs/src/content/docs/configuration.mdxdocs/src/content/docs/deployment.mdgo.mod
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 (5)
- **Opt a page into the Cloud CTA with `cloudCta` frontmatter**, not by importing the component.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/content/docs/configuration.mdx
- **Never hand-write `®` or `™` in prose.**
📄 CodeRabbit inference engine (AGENTS.md)
Files:
CHANGELOG.mddocs/src/content/docs/configuration.mdxdocs/src/content/docs/deployment.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/configuration.mdx
Every code change should update the corresponding docs in the same PR. A code change without its doc update is incomplete.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
go.modCHANGELOG.mddocs/src/content/docs/configuration.mdxdocs/src/content/docs/deployment.md
- **Never hard-wrap prose. One paragraph is one line.** No wrapping at 72/80 columns, no "semantic linefeeds" splitting a paragraph at sentence boundaries.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
CHANGELOG.mddocs/src/content/docs/deployment.md
🔇 Additional comments (4)
CHANGELOG.md (1)
13-13: LGTM!docs/src/content/docs/configuration.mdx (1)
18-27: LGTM!Also applies to: 42-42, 66-66, 106-106, 115-115, 162-166, 214-214, 228-231, 242-242, 288-288, 300-302, 311-317
docs/src/content/docs/deployment.md (1)
19-20: LGTM!Also applies to: 63-63, 139-139, 183-183, 365-365
go.mod (1)
23-23: LGTM!Also applies to: 112-112
| # config.json — the hot-reloadable tenant tunables (see the Settings | ||
| # Directory page). Create it with | ||
| # `wavehouse init-settings /etc/wavehouse/settings`. In a container, leave | ||
| # this unset — the image presets WH_SETTINGS_DIR=/app/settings — and mount | ||
| # (or seed) your directory at /app/settings instead; the image ships none. | ||
| WH_SETTINGS_DIR=/etc/wavehouse/settings |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
Use one settings path in the container example.
This repeats the previously reported mismatch. The init-settings command uses /etc/wavehouse/settings, while the comments say to mount /app/settings; Line 164 still sets WH_SETTINGS_DIR=/etc/wavehouse/settings. If an operator follows the documented /app/settings mount, boot rejects the missing directory. Use /app/settings consistently, or remove the variable and keep the image default.
EricAndrechek
left a comment
EricAndrechek
left a comment
There was a problem hiding this comment.
My required changes from you that aren't in per-file reviews:
- Need to update/add the new settings package you've added for
architecture.mdand toAGENTS.mdprobably too, etc - I definitely should have mentioned this sooner, and for that I'm very sorry, but I don't know if I like the command being
init-settings– having a dash in there means it splits as two words when you try and terminal option-backspace and stuff, I'd rather it be one word cleanly. What do other popular projects that ship with an option like this use? I am immediately thinking of options likeinit,bootstrap,defaults, etc but open to pushback or other ideas etc here. - I keep asking but not really hearing about it, so I finally added them all as individual comments… but basically why were the things you had in config all you had? I know pipes and policy are going to come in a follow-up PR, but there are so many other settings things that I think should be there and not the config stuff too that I'd like to at least discuss. Happy to take offline and discuss them all in-person if easier.
Some other/minor nits claude caught for me, verbatim:
-
architecture.md untouched: AGENTS.md §Documentation Sync requires it for "add a package". AGENTS.md:36 was updated for dedupe.Managed; the page it points at wasn't. Stale: :54-67 package tree omits settings/; :111-114 lists only dedupe.go/embedded.go, missing managed.go; :74 omits /v1/ops/settings/reload; :109 still says "All settings use WH_ prefixed env vars". -
PR body is stale — and it becomes the squash commit message: Still says the seed is baked into "both container images at /app/settings". 1593fe5 reversed exactly that (Dockerfile:33-41 now creates a deliberately empty mount point), and the e2e fixture uses its own copy. Merging as-is writes a commit message contradicting its own code. -
The dev settings-dir story dirties the tree and voids the make ci marker: config.yaml:93 points the live, watched dir at the checked-in go:embed seed (internal/settings/seed) — make dev inherits it, so any local experiment edits a tracked file and silently changes what init-settings writes for everyone. The documented escape (development.md:231-236, init-settings ./settings) isn't in .gitignore either, so it leaves four untracked files that change the tree SHA scripts/ci-marker.sh:19-27 computes via git add -A. → Point the dev default at ./tmp/settings (already ignored) or add /settings/ to .gitignore, and have make dev seed via init-settings. -
deployment.md:185, :212 — /app/settings missing from the mount contract: docs "Pre-create /app/data and /app/pipes" is now incomplete (both Dockerfiles also pre-create /app/settings, and the copy-up mechanic at :212 is what makes the named volume land with UID 65532). The whole "Persistent Storage (REQUIRED for containers)" section never mentions it — despite a missing mount there now being the #1 startup failure. -
settings-directory.mdx:70 — "or the reverse" over-claims the counter. Managed.CheckAndMark returns ErrDisabled only when the store's switch is off (managed.go:60-65), the only path that increments (ingest.go:455-462). In the reverse case the handler skips the dedupe block entirely (ingest.go:441) — un-deduped but not counted.
There was a problem hiding this comment.
So something I realized while trying to deploy stuff on WaveHouse-Stats is that this new PR is going to break our documented quickstart stuff… We have it here in the README at :39-:39 and :101-:102, but also in lots of places in the docs I think… Some I notice are fixed I think, like in getting-started and deployment, but not sure how many other files or places may need updating, like maybe index which is the landing page and I think shows some getting started stuff too?
You already have the fix documented at those two doc places, with:
docker compose -f deployments/compose/standalone.yaml run --rm wavehouse init-settings /app/settingsBut I'd love for you to make sure it's documented everywhere, and, crucially, that that actually WORKS – like run it yourself, manually, not telling claude to do it, so that you can see what you get hung up on if anything to make sure its well documented and easy to run as a quickstart should be.
There was a problem hiding this comment.
I did run this. There's no more seed step. standalone.yaml mounts deployments/compose/settings/ (the seed with clickhouse.addr already pointed at the clickhouse service), so the quickstart is just up -d again. This should be documented everywhere you can check on your next review.
Now run:
docker compose -f deployments/compose/standalone.yaml run --rm -v wavehouse-settings:/app/settings wavehouse bootstrap /app/settingsMaking note here that the command is not init-settings and is now bootstrap.
Running the above should output something like: roles.json, policies.json, pipes.json, config.json to /app/settings. The -v points /app/settings at an empty named volume for that one run. Without it, /app/settings is the checked in bind mount, which already has the four files, and bootstrap refuses with /app/settings is not empty and will refuse to overwrite an existing settings directory.
| # them un-deduped (default false → logged + counted, not rejected). | ||
| WH_DEDUPE_REQUIRE_ID: "false" | ||
| WH_SCHEMA_REFRESH_INTERVAL: "60" | ||
| WH_DLQ_ENABLED: "true" |
There was a problem hiding this comment.
Can the DLQ setting (enabled or not) not move into the hot-reload piece too? Like it would presumably be on per-tenant/table, no?
| WH_PIPES_DIR= | ||
|
|
||
| WH_SETTINGS_DIR= | ||
| WH_SETTINGS_DIR=./settings # required, no default (`wavehouse init-settings ./settings`); the container images preset /app/settings; you mount a directory there |
There was a problem hiding this comment.
I could be mistaken, but I don't think that docker likes comments inline on an env file, so if someone tried to copy-paste this and run it exactly with this example it would fail. I think.
There was a problem hiding this comment.
Do we need to add anything about the settings directory/volume stuff in here?
| # and settings.dir: ./settings in .config.local.yaml | ||
| ``` | ||
|
|
||
| The key hot-reloads, so once the server is running you can toggle it by editing `config.json` — no restart. Records dedupe on their `event_id` field by default; the same file overrides the field globally or per table (see [Configuration — Deduplication](/settings-directory#deduplication)). |
There was a problem hiding this comment.
You have the new URL here for settings directory stuff, but the link still says [Configuration – Deduplication] – is tat intended?
There was a problem hiding this comment.
That was outdated. Fixed now.
| type MQ struct { | ||
| GapWindowMinutes int `yaml:"gap_window_minutes" env:"WH_MQ_GAP_WINDOW_MINUTES" env-default:"15"` | ||
| MaxBytesGB int `yaml:"max_bytes_gb" env:"WH_MQ_MAX_BYTES_GB" env-default:"50"` | ||
| } |
There was a problem hiding this comment.
Can this not be hot-reloadable too? How would that work?
| type Cache struct { | ||
| L1MaxCost int64 `yaml:"l1_max_cost" env:"WH_CACHE_L1_MAX_COST" env-default:"67108864"` | ||
| TimestampBucketSeconds int `yaml:"timestamp_bucket_seconds" env:"WH_CACHE_TIMESTAMP_BUCKET_SECONDS" env-default:"60"` | ||
| } |
There was a problem hiding this comment.
Why can't this be hot-reloadable and in settings config?
There was a problem hiding this comment.
I think this may be one of if not the most frequently changed settings in here, especially for new deployments setting up and trialing auth, and should absolutely be hot-reloadable and able to be validated, etc, no? I will note that we may want a new issue to track how to store the secrets here, namely jwt_secret and operator_key, which presumably you wouldn't want in plaintext in a tracked file...
There was a problem hiding this comment.
Can this not be hot-reloadable too?
| // Load reads config from a YAML file (if it exists) with env var overrides. | ||
| func Load(path string) (*Config, error) { | ||
| var cfg Config | ||
| if _, err := os.Stat(path); err == nil { | ||
| if err := cleanenv.ReadConfig(path, &cfg); err != nil { | ||
| return nil, fmt.Errorf("read config: %w", err) | ||
| } | ||
| } else { | ||
| if err := cleanenv.ReadEnv(&cfg); err != nil { | ||
| return nil, fmt.Errorf("read env: %w", err) | ||
| } | ||
| } | ||
|
|
||
| if err := cfg.Validate(); err != nil { | ||
| return nil, fmt.Errorf("validate config: %w", err) | ||
| } | ||
|
|
||
| return &cfg, nil | ||
| } |
There was a problem hiding this comment.
Not sure how or best way to do it, but when we read in the config (probably can't/shouldn't do it for env variables read in, since there will always be ones unrelated to us set), it would be nice if there was a way we could loudly assert/fail when unused values were set (so like if someone still was setting dedupe settings in a yaml/json this read in), or potentially if required ones were unset, etc, but may be more work given we combine with env too, etc – likely a later PR but may worth an issue to track as well, so we can have validate working loudly on the non-hot-reloadable startup ops config too.
…gs unconditionally, drop setters and the legacy auth.Middleware
…eded by make dev, dedupe counter wording
2 participants
Summary
The server now consumes the settings directory that #500 only validated.
settings.dir/WH_SETTINGS_DIRis required: boot validates and adopts it (missing or invalid refuses to start, same contract aspolicy.file_path), and a running instance re-validates and re-adopts on a directory watch (fsnotify with debounce; survives a delete-and-recreate),SIGHUP, or admin-gatedPOST /v1/ops/settings/reload, all through one serialized reload path. A rejected reload, a deleted file, or a vanished directory keeps the previous good snapshot.config.json: the ClickHouse wiring (clickhouse.addr/http_port/http_scheme/database/username/query_timeout—chconn.Managerswaps the connection behind onedriver.Connon reload), the auth verifier wiring (auth.jwks_url/auth.role_claim—auth.Authenticatorswaps a whole verifier atomically;auth.Middlewareis gone),dedupe.enabled(the newdedupe.Managedopens or closes the Pebble store from anAfterAdopthook) withdedupe.id_field/dedupe.require_idand per-table overrides (Per-table dedupe id_field (+ fix cross-table dedupe keyspace collision) #222),dlq.enabledwith per-table overrides,query.default_max_rows/query.timestamp_bucket_seconds,schema.refresh_interval,stream.keepalive_interval/keepalive_buckets(Heartbeater.Reconfigurerebuilds the wheel in place) /gap_window_minutes, andcors.allowed_origins. The YAML/env keys for all of these are removed, and boot config is now strict: an undeclared key refuses to boot, naming it. What stays in boot config is only what can't change under a running process — resource sizing, listeners, observability exporters — and the secrets (clickhouse.password,auth.jwt_secret,auth.operator_key), never in a tracked JSON file.config.jsonkey is required and the binary carries no compiled defaults, so the adopted snapshot is what the files say. Defaults live in one checked-in seed (internal/settings/seed/,go:embedded): the newwavehouse bootstrap [dir]writes it (refusing a non-empty directory; resolvesdirexactly asvalidatedoes — argument, elseWH_SETTINGS_DIR), the compose stack bind-mounts a checked-in copy atdeployments/compose/settings/so the quickstart staysup -d,make devseeds a gitignored./settings, and the e2e fixture ships its own. The container images ship no settings directory:WH_SETTINGS_DIR=/app/settingsis preset and the operator mounts one there — a missing mount refuses to boot rather than running on defaults nobody chose. The seed ships no policy (fail-closed, warned).IngestHandler.DedupeSettings, the structured-querydefaultMaxRows/bucketSecsgetters, the ingest worker'sdlqEnabled, the sweeper'sgapWindow,corsMiddleware's origins getter,SchemaRegistry's database and refresh-interval sources) so a reload applies without a restart andinternal/apistays testable without a directory.Known gap: only
config.jsonchanges affect a running server.roles.json,policies.json, andpipes.jsonare validated and loaded into the snapshot, but editing them does nothing yet — access control still comes frompolicy.file_path+ NATS KV, and pipes from the/v1/ops/pipesAPI + NATS KV. The next PR makes the snapshot the runtime authority for all three and deletes thePUT /v1/ops/policyandPUT/DELETE /v1/ops/pipes/{name}endpoints and the NATS KV stores behind them: without KV there is no bundled node sync, so a node-local write path would let distributed nodes drift. Files (edited directly on standalone, written by the control plane in cloud) become the only write path.Related Issues
Advances #48 (reload wiring; reverses the #48-era runtime-settings direction) and #222 (per-table
id_field; the keyspace fix stays open there). Follow-up surfaced in review: #510 (distroless digest pin).