Repository navigation
refactor(app): extract the process wiring from main.go into internal/app - #585
Conversation
github-actions
Bot
added
documentation
📝 SummarySummary by CodeRabbit
WalkthroughThe pull request moves process construction and lifecycle management into ChangesApplication wiring extraction
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Refactor Sequence Diagram(s)sequenceDiagram
participant Signal as signal context
participant Main as cmd/wavehouse
participant App as internal/app
participant Runtime as HTTP and worker components
Signal->>Main: cancel on SIGINT or SIGTERM
Main->>App: New(config, build info, log level, listener)
App->>Runtime: construct and start wired components
Main->>App: Run(ctx)
Runtime->>App: return on cancellation or component error
Main->>App: Close()
App->>Runtime: release resources in reverse order
Suggested reviewers: Merge Risk: 🟡 Moderate · up to A shutdown-time SIGHUP can delay exit, and a stalled liveness response can hang integration setup indefinitely. These lifecycle and test reliability issues should be fixed before merge. 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
Full details: Linked Issues checkExplanation Issue [ Full details: Out of Scope Changes checkExplanation Issue [
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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://686decc1-wavehouse-docs.wave-rf.workers.dev |
Code Coverage OverviewLanguages: Go GoThe overall line coverage in commit 0832429 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: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 6a153dd1-a751-4b5d-9ee0-4f219958510d
📒 Files selected for processing (26)
.github/labeler.yml.testcoverage.ymlAGENTS.mdCHANGELOG.mdcmd/wavehouse/main.gocmd/wavehouse/main_test.godocs/src/content/docs/architecture.mddocs/src/content/docs/configuration.mdxdocs/src/content/docs/development.mddocs/src/content/docs/ingest-pipeline.mdinternal/api/boot_chain_test.gointernal/api/router.gointernal/app/app.gointernal/app/app_test.gointernal/app/wire.gointernal/mq/embedded.gointernal/observability/provider.gointernal/settings/watch_test.gointernal/stream/subscriber.goscripts/cov/main.goscripts/orchestrator/main.gotests/integration/dlq_test.gotests/integration/ingest_test.gotests/integration/otel_test.gotests/integration/query_test.gotests/integration/setup_test.go
💤 Files with no reviewable changes (1)
- .testcoverage.yml
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. (4)
- GitHub Check: Docs preview
- GitHub Check: E2E tests
- GitHub Check: Coverage
- GitHub Check: Lint
🧰 Additional context used
📓 Path-based instructions (2)
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
WH001 applies to every tracked Markdown file, with no carve-out
📄 CodeRabbit inference engine (AGENTS.md)
Files:
CHANGELOG.mddocs/src/content/docs/development.mddocs/src/content/docs/ingest-pipeline.mdAGENTS.mddocs/src/content/docs/architecture.md
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-09-15T19:03:45.435Z
Learning: Run `make lint` and `make test` before considering work complete.
📚 Learning: 2026-08-13T12:17:52.620Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 470
File: docs/src/content/docs/reverse-proxy.mdx:137-144
Timestamp: 2026-08-13T12:17:52.620Z
Learning: For Wave-RF/WaveHouse documentation, verify claims about implementation control flow against the authoritative implementation source (for example, internal/auth/auth.go) rather than relying solely on docs/** content. Documentation may lag behind or paraphrase behavior, so control-flow claims should be confirmed in source code.
Applied to files:
docs/src/content/docs/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:
cmd/wavehouse/main_test.gointernal/app/app_test.go
📚 Learning: 2026-06-10T15:01:09.027Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 312
File: docs/src/content/docs/development.md:0-0
Timestamp: 2026-06-10T15:01:09.027Z
Learning: In this repo’s Markdown review (all .md files), do not flag capitalization/style issues for literal paths starting with ".github/" (or any substring that is a path beginning with ".github/"). Treat ".github" as the correct lowercase dotfile directory name, even when it appears inside prose or code spans; automated checks such as LanguageTool’s "(GITHUB)" rule commonly produce false positives for this literal filesystem path.
Applied to files:
CHANGELOG.mdAGENTS.md
🪛 ast-grep (0.45.3)
internal/app/wire.go
[warning] 488-492: This http.Server is constructed without a ReadTimeout. Without a read timeout, a slow or malicious client can hold connections open indefinitely (e.g. a Slowloris attack), exhausting server resources and causing a denial of service. Set ReadTimeout (and ideally ReadHeaderTimeout, WriteTimeout, and IdleTimeout) on the http.Server to bound how long the server waits while reading a request.
Context: http.Server{
Addr: fmt.Sprintf(":%d", a.cfg.Server.Port),
Handler: a.handler,
ReadHeaderTimeout: readHeaderTimeout,
}
Note: [CWE-400] Uncontrolled Resource Consumption.
(http-server-missing-read-timeout-go)
[warning] 536-536: A log/format call (log.Print/Printf/Println, the Fatal/Panic variants, fmt.Sprintf, or a structured logger's Info/Warn/Error/Debug method) is given a message built by concatenating a string literal with a non-literal value such as request data. Unsanitized, attacker-controlled input written to logs enables log forging / CRLF injection: an attacker can inject newlines to spoof log entries or break log parsers. Do not concatenate raw input into the log message; pass it as a separate structured field/argument (e.g. 'log.Printf("user: %s", user)' or 'logger.Info("login", "user", user)') and strip or escape newline characters first.
Context: slog.Info("starting "+name, "addr", ln.Addr().String())
Note: [CWE-117] Improper Output Neutralization for Logs.
(log-injection-request-data-concat-go)
[warning] 547-547: A log/format call (log.Print/Printf/Println, the Fatal/Panic variants, fmt.Sprintf, or a structured logger's Info/Warn/Error/Debug method) is given a message built by concatenating a string literal with a non-literal value such as request data. Unsanitized, attacker-controlled input written to logs enables log forging / CRLF injection: an attacker can inject newlines to spoof log entries or break log parsers. Do not concatenate raw input into the log message; pass it as a separate structured field/argument (e.g. 'log.Printf("user: %s", user)' or 'logger.Info("login", "user", user)') and strip or escape newline characters first.
Context: slog.Info("shutting down " + name)
Note: [CWE-117] Improper Output Neutralization for Logs.
(log-injection-request-data-concat-go)
[warning] 551-551: A log/format call (log.Print/Printf/Println, the Fatal/Panic variants, fmt.Sprintf, or a structured logger's Info/Warn/Error/Debug method) is given a message built by concatenating a string literal with a non-literal value such as request data. Unsanitized, attacker-controlled input written to logs enables log forging / CRLF injection: an attacker can inject newlines to spoof log entries or break log parsers. Do not concatenate raw input into the log message; pass it as a separate structured field/argument (e.g. 'log.Printf("user: %s", user)' or 'logger.Info("login", "user", user)') and strip or escape newline characters first.
Context: slog.Error(name+" shutdown error; closing open connections", "error", err)
Note: [CWE-117] Improper Output Neutralization for Logs.
(log-injection-request-data-concat-go)
🪛 LanguageTool
docs/src/content/docs/configuration.mdx
[typographical] ~143-~143: The word ‘When’ starts a question. Add a question mark (“?”) at the end of the sentence.
Context: ... you can run traces-only, logs-only, etc. Prometheus exposition is configured in ...
(WRB_QUESTION_MARK)
[style] ~143-~143: Since ownership is already implied, this phrasing may be redundant.
Context: ... Prometheus exposition is configured in its own top-level prometheus b...
(PRP_OWN)
CHANGELOG.md
[uncategorized] ~23-~23: The official name of this software platform is spelled with a capital “H”.
Context: ...on/setup_test.go, .testcoverage.yml, .github/labeler.yml): app.New` builds every c...
(GITHUB)
AGENTS.md
[grammar] ~27-~27: Please add a punctuation mark at the end of paragraph.
Context: ...and the signal context; everything else is internal/app Seventeen internal pack...
(PUNCTUATION_PARAGRAPH_END)
docs/src/content/docs/architecture.md
[style] ~90-~90: Since ownership is already implied, this phrasing may be redundant.
Context: ...ons.Listenerlets one serve the API on its own listener instead ofserver.port`. - **...
(PRP_OWN)
[typographical] ~129-~129: Consider using an em dash in dialogues and enumerations.
Context: - discovery.go — SchemaRegistry que...
(DASH_RULE)
[style] ~129-~129: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...e SSE announcement are both built from. Each refresh also records the server version (SELECT version()), joins system.tables for each table's create_table_query (kept in-process as TableSchema.DDL and marked json:"-" — an external-engine table renders its wiring in that statement — endpoint, bucket/host, database, username, access key id — so it must never reach /v1/ops/schema; ClickHouse masks the password as [HIDDEN] from ~23.9, so what is withheld here is the topology), reads each column's default_expression and 1-based position alongside its type, discovers the server's default time zone (SELECT timezone()) and bakes every DateTime/DateTime64 column's canonicalization spec (precision + resolved zone) into the cached schema, so the per-record ingest path parses no type strings and loads no zones (#372). Supports periodic auto-refresh, on-d...
(TOO_LONG_SENTENCE)
🔇 Additional comments (26)
.github/labeler.yml (1)
6-10: LGTM!AGENTS.md (1)
29-29: LGTM!Also applies to: 32-32, 69-69, 393-407, 424-426
CHANGELOG.md (1)
23-24: LGTM!docs/src/content/docs/architecture.md (1)
48-55: LGTM!Also applies to: 86-91, 129-129, 148-150
docs/src/content/docs/configuration.mdx (1)
143-143: LGTM!docs/src/content/docs/development.md (1)
348-348: LGTM!Also applies to: 454-454
docs/src/content/docs/ingest-pipeline.md (1)
158-185: LGTM!Also applies to: 235-235
internal/api/boot_chain_test.go (1)
94-94: LGTM!internal/api/router.go (1)
46-46: LGTM!Also applies to: 120-120
internal/mq/embedded.go (1)
66-67: LGTM!internal/observability/provider.go (1)
70-70: LGTM!internal/settings/watch_test.go (1)
81-81: LGTM!internal/stream/subscriber.go (1)
59-59: LGTM!scripts/cov/main.go (1)
76-78: LGTM!Also applies to: 303-304, 708-710
scripts/orchestrator/main.go (1)
311-317: LGTM!Also applies to: 320-320
tests/integration/otel_test.go (1)
218-218: LGTM!Also applies to: 313-314
internal/app/app.go (1)
1-191: LGTM!internal/app/app_test.go (1)
1-348: LGTM!tests/integration/ingest_test.go (1)
33-33: LGTM!Also applies to: 57-57, 94-94, 135-135
cmd/wavehouse/main.go (2)
112-115: LGTM!Also applies to: 131-138, 168-185, 187-197
163-167: 📐 Maintainability & Code QualityThe auth-posture handling remains in
internal/app/wire.go: lines 379–382 emit both JWT/JWKS warnings, lines 385–389 handle the trimmedOperatorKeyand recovery warning, and line 398 passes the trimmed value to the authenticator. The extraction did not remove this behavior.cmd/wavehouse/main_test.go (1)
30-41: LGTM!Also applies to: 45-53, 57-75, 77-110, 112-150
tests/integration/setup_test.go (2)
139-154: LGTM!Also applies to: 162-196, 198-213, 226-239, 241-273, 380-380
214-225: 🩺 Stability & Availability
internal/settings/seed/config.jsondefines exactly the six patchedclickhousefields and the singlemq.max_bytes_gbfield.settings.Store.ClickHouse()dereferences those six fields, andMQConfigcontains onlyMaxBytesGB. The patch therefore does not drop any required settings, so merging the seed objects is unnecessary.tests/integration/dlq_test.go (1)
27-27: LGTM!Also applies to: 72-72, 117-117
tests/integration/query_test.go (1)
110-110: LGTM!Also applies to: 137-137
github-project-automation
Bot
moved this from In progress
to In review
in WaveHouse Task Board
…e test scratch dirs
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/app/wire.go (1)
278-278: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse a fresh timeout context for the rollback.
If
api.EnsureDLQStreamfails because the sharedctxreachesmqResizeTimeout,embedded.Resize(ctx, applied)receives an expired context. The rollback then cannot run, and the ingest and DLQ limits remain inconsistent until another settings adoption. Create a new bounded context for the rollback.Proposed fix
- if err := embedded.Resize(ctx, applied); err != nil { + rollbackCtx, rollbackCancel := context.WithTimeout(context.Background(), mqResizeTimeout) + defer rollbackCancel() + if err := embedded.Resize(rollbackCtx, applied); err != nil {
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: c380b2f3-6d86-4cf8-bf66-09d6cd39ab09
📒 Files selected for processing (2)
internal/app/wire.gotests/integration/setup_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
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-09-15T20:05:19.381Z
Learning: Never force-push or rebase a PR branch
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-09-15T20:05:19.381Z
Learning: Never hand-write markers or `--no-verify`
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-09-15T20:05:19.381Z
Learning: Run `make lint` and `make test` before considering work complete.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-09-15T20:05:19.381Z
Learning: Validate locally before every push
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-09-15T20:05:19.381Z
Learning: Agents must create PRs with `gh pr create --draft`.
🪛 ast-grep (0.45.3)
internal/app/wire.go
[warning] 499-503: This http.Server is constructed without a ReadTimeout. Without a read timeout, a slow or malicious client can hold connections open indefinitely (e.g. a Slowloris attack), exhausting server resources and causing a denial of service. Set ReadTimeout (and ideally ReadHeaderTimeout, WriteTimeout, and IdleTimeout) on the http.Server to bound how long the server waits while reading a request.
Context: http.Server{
Addr: fmt.Sprintf(":%d", a.cfg.Server.Port),
Handler: a.handler,
ReadHeaderTimeout: readHeaderTimeout,
}
Note: [CWE-400] Uncontrolled Resource Consumption.
(http-server-missing-read-timeout-go)
🔇 Additional comments (1)
tests/integration/setup_test.go (1)
155-160: LGTM!
There was a problem hiding this comment.
Overall really good stuff. My findings:
- Ports should be handed out etc by the test orchestrator to find an open port and use that etc, and/or use test containers, so ideally we shouldn't have a port dynamically picked by a test at all. If a port IS taken and is being handed to our process, ie we are testing what happens when wavehouse is trying to be deployed on a server where an old process is running and holds that port, my take is it should fail right away. I think that is what your test in
app_test.gois trying to test for, but on my mac I guess this behavior is slightly different and so that turns into a hang instead of failing. TBH I don't know enough about the networking specifics between Darwin/BSD and GNU+Linux to know what/why the behaviors there are different, so I had Claude help me debug that one a bit and it gave some suggestions in-line on that file. - On stopping the server (ie SIGTERM) we should stop accepting any new in-flight messages and end all SSE connections effectively right away, end all the workers and subscribers to NATS events, watchers/fsnotify, etc, and then hopefully by then any remaining in-flight messages trying to insert or querying a complex query etc have finished and have returned, otherwise I guess probably non-gracefully end them? Then the whole first step should be done in ~10 seconds, with more cleanup needed after that too like closing connections to clickhouse, pebble/remote dedupe store if an interface exists for that (ie future happening soonish), closing MQ (right now in process later remote we disconnect from), caches, OTEL cleanup, etc – and ideally this final cleanup/next steps should also be bounded by like a 10 second context or something – potentially we may want to move the OTeL cleanup to actually happen independently at the very end with its own context too so we get debug info etc from the cleanup process too. Not really sure what the best path is, but I think a 2/3 tiered shutdown process like this is probably/hopefully sufficient?
- As I understand it, we setup during normal operation SIGTERM/SIGINT to start the kill/cleanup process described above, and SIGHUP is set to trigger it to refresh, so to read in new config/settings for instance, as (as I understand it) that is the norm when deploying on something like nomad for instance, where it can auto SIGHUP when a variable/config changes etc. But I think that that isn't fully happening properly or being handled/ignored correctly once a SIGTERM/SIGINT is triggered and the shutdown process has started – like a SIGTERM and then another SIGTERM I think should kill the process right away basically, or maybe kill the current tier its in shutting down right away? Idk for sure, tbd on that – but then SIGHUP should just be ignored altogether at that stage I think.
Then one other thing I want to mention now and we can discuss/decide if we want this to happen in this PR or not, but I am wondering if this app package is the right spot for us to start to decide which implementation of an abstraction gets used and whether or not it even is booted or not, etc. Will discuss more in-person.
…nore SIGHUP and exit on a second signal during the stop
There was a problem hiding this comment.
Actionable comments posted: 3
🟠 Major · Apply the liveness deadline to each HTTP request.
tests/integration/setup_test.go:257
🩺 Stability & Availability | 🟠 Major | ⚡ Quick winApply the liveness deadline to each HTTP request.
setuppassescontext.Background()towaitForLive. The boundnet/http.DefaultClienthas no response timeout. If/livezaccepts a connection but does not respond,Docan block beyond the loop's 30-second deadline, so the integration suite can hang.Create a context with the computed deadline and use it for each request.
Proposed fix
func waitForLive(ctx context.Context, baseURL string, timeout time.Duration) error { deadline := time.Now().Add(timeout) + pollCtx, cancel := context.WithDeadline(ctx, deadline) + defer cancel() last := "no response" for time.Now().Before(deadline) { - req, err := http.NewRequestWithContext(ctx, http.MethodGet, baseURL+"/livez", nil) + req, err := http.NewRequestWithContext(pollCtx, http.MethodGet, baseURL+"/livez", nil)
🟡 Minor · Do not reload after cancellation.
internal/app/wire.go:451-452
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDo not reload after cancellation.
When
ctx.Done()andhupare both ready,selectcan choosehup. Line 452 then callsa.store.Reloadafter shutdown starts. This can delayRunwhile reload hooks execute.Check
ctx.Err()beforeReloadand return when shutdown has started.Proposed fix
case <-hup: + if ctx.Err() != nil { + return nil + } a.store.Reload("sighup")Based on PR objectives, SIGHUP must be ignored during stopping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 975ae879-2fdb-4966-910a-f7c54d83bede
📒 Files selected for processing (14)
CHANGELOG.mdcmd/wavehouse/main.gocmd/wavehouse/main_test.godeployments/compose/standalone.yamldocs/src/content/docs/api.mddocs/src/content/docs/architecture.mddocs/src/content/docs/configuration.mdxdocs/src/content/docs/deployment.mdinternal/api/stream.gointernal/app/app.gointernal/app/app_test.gointernal/app/wire.goscripts/orchestrator/main.gotests/integration/setup_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)
WH001 applies to every tracked Markdown file, with no carve-out
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/content/docs/configuration.mdxCHANGELOG.mddocs/src/content/docs/api.mddocs/src/content/docs/deployment.mddocs/src/content/docs/architecture.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
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-09-16T15:19:39.771Z
Learning: Run `make lint` and `make test` before considering work complete.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-09-16T15:19:39.771Z
Learning: Every new function should have corresponding test cases.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-09-16T15:19:39.771Z
Learning: Validate locally before every push
📚 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-10T15:01:09.027Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 312
File: docs/src/content/docs/development.md:0-0
Timestamp: 2026-06-10T15:01:09.027Z
Learning: In this repo’s Markdown review (all .md files), do not flag capitalization/style issues for literal paths starting with ".github/" (or any substring that is a path beginning with ".github/"). Treat ".github" as the correct lowercase dotfile directory name, even when it appears inside prose or code spans; automated checks such as LanguageTool’s "(GITHUB)" rule commonly produce false positives for this literal filesystem path.
Applied to files:
CHANGELOG.md
🪛 LanguageTool
CHANGELOG.md
[uncategorized] ~23-~23: The official name of this software platform is spelled with a capital “H”.
Context: ...on/setup_test.go, .testcoverage.yml, .github/labeler.yml): app.New` builds every c...
(GITHUB)
docs/src/content/docs/architecture.md
[style] ~90-~90: 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)
🔇 Additional comments (13)
CHANGELOG.md (1)
23-23: LGTM!docs/src/content/docs/architecture.md (4)
55-55: LGTM!
90-91: LGTM!
129-129: LGTM!
148-150: LGTM!docs/src/content/docs/configuration.mdx (2)
45-45: LGTM!
143-143: LGTM!scripts/orchestrator/main.go (1)
311-322: LGTM!docs/src/content/docs/deployment.md (1)
313-323: LGTM!docs/src/content/docs/api.md (1)
598-598: LGTM!deployments/compose/standalone.yaml (1)
53-56: LGTM!internal/app/app.go (1)
4-11: LGTM!Also applies to: 68-73, 112-114, 174-180, 187-187
internal/app/app_test.go (1)
19-19: LGTM!Also applies to: 117-117, 142-143, 214-235, 262-262, 361-366, 373-382, 383-410
…on a second signal, and bound Close with a context
|
@coderabbitai review |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 1ebd3e0a-81bb-4516-8f2d-3b5ab3056569
📒 Files selected for processing (4)
docs/src/content/docs/architecture.mddocs/src/content/docs/deployment.mdinternal/app/app.gointernal/app/app_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 (1)
WH001 applies to every tracked Markdown file, with no carve-out
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/content/docs/architecture.mddocs/src/content/docs/deployment.md
🧠 Learnings (1)
📚 Learning: 2026-06-26T12:23:22.696Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 346
File: internal/stream/subscriber_test.go:9-28
Timestamp: 2026-06-26T12:23:22.696Z
Learning: In this Go repository, prefer table-driven tests (e.g., `[]struct{...}` with `t.Run(...)`) only for tests that cover multiple scenarios/inputs and can be cleanly enumerated. Do not artificially rewrite a clear single-scenario sequential behavioral-flow test into a table-driven form just to fit the pattern; if there’s only one meaningful scenario, keep the test as a straightforward linear flow (as in `TestSubscriber_SendDeliversThenDropsWhenFull`).
Applied to files:
internal/app/app_test.go
🪛 LanguageTool
docs/src/content/docs/architecture.md
[style] ~90-~90: 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] ~90-~90: 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)
🔇 Additional comments (2)
docs/src/content/docs/architecture.md (1)
55-55: LGTM!Also applies to: 90-91, 129-129, 148-150
docs/src/content/docs/deployment.md (1)
318-318: LGTM!
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
…op context after a component failure
0832429
|
@coderabbitai review |
✅ Action performedReview finished.
|
github-project-automation
Bot
moved this from In review
to In progress
in WaveHouse Task Board
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Restore the global OTel logger provider. · app_test.go:100-107
internal/app/app_test.go:100-107
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRestore the global OTel logger provider.
observability.InitProviderinstalls a process-global logger provider when OTel logs are enabled. Successful shutdown closes that provider but does not restore the previous provider. Add thego.opentelemetry.io/otel/log/globalimport and restore it inguardGlobals.Suggested change
savedLP := global.GetLoggerProvider() t.Cleanup(func() { slog.SetDefault(savedLogger) otel.SetTextMapPropagator(savedProp) otel.SetTracerProvider(savedTP) otel.SetMeterProvider(savedMP) global.SetLoggerProvider(savedLP) })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 0bf3469d-ddf4-468d-b730-eb50f8d2f95c
📒 Files selected for processing (2)
internal/app/app_test.gotests/integration/otel_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
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-09-17T13:37:35.707Z
Learning: Run `make lint` and `make test` before considering work complete.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-09-17T13:37:35.707Z
Learning: DRY — one source of truth.
🔇 Additional comments (2)
internal/app/app_test.go (1)
391-394: LGTM!tests/integration/otel_test.go (1)
218-218: LGTM!Also applies to: 313-315
github-project-automation
Bot
moved this from In progress
to Done
in WaveHouse Task Board
Summary
Pulls the ~550 lines of hand wiring and settings-reload hooks out of
cmd/wavehouse/main.gointo a newinternal/apppackage.app.Newbuilds every component from the boot config and the settings directory,Rundrives the long-lived ones (ingest worker, sweeper, hub bridge, keepalive wheel, schema refresh, SIGHUP + directory watcher, API server, Prometheus sidecar) under one erxt is cancelled or a component fails, andClosereleases whatNewopened in reverse order. Each component is wired in one place, with the settings store handed to its wiring function whole, so the per-tenant store registry (#583, story 2) lands there rather than inmain.main.goshrinks to argv dispatch, logger,config.Load,CheckDataDir,app.New,app.Run;run(ctx)takes the signal context and is unit-tested, sothe
.testcoverage.ymlexclude for it is removed.tests/integrationboots through the sameapp.Newagainst its testcontainer instead of a hand-built handler subset that had drifted from the binary.server.shutdown_timeout(open connectionsforce-closed at the deadline), and themq.max_bytes_gbreload hook bounds its JetStream calls to 10s. The e2e orchestrator's kill budget is raised to 20s tomatch.
area/app), and stalemain.gocode comments synced.Test plan
run(ctx)boot/refusal pathsRelated Issues
Closes #140. Part of #583 (story 0).