Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions .testcoverage.yml
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ suites:
sdk: 50 # statements (vitest gates via --coverage.thresholds.statements)

exclude:
# Global excludes apply to every suite render AND to the merged total.
# Use for paths that aren't meaningful coverage anywhere.
paths:
# internal/testutil/ contains shared test helpers (mocks, JWT builders,
# HTTP assertions). It is imported only from *_test.go files, never from
Expand All @@ -41,3 +43,21 @@ exclude:
# aren't part of the shipped binary; they show up in `-coverpkg=./...`
# but never get exercised by tests.
- ^scripts/

# Per-suite excludes layer on top of `paths` but only for that suite's
# render. Crucially, they do NOT apply to the merged total — so a file
# excluded from `unit` here still counts toward the project-wide gate
# via whichever suite(s) actually do cover it. See scripts/cov/main.go
# config.excludesFor for the wiring.
per-suite:
unit:
# cmd/wavehouse/main.go is the binary entry point — 400+ lines of
# service wiring that's only meaningfully exercised by the e2e suite
# (which runs bin/wavehouse-cov against a real ClickHouse + NATS).
# Excluding from `unit` keeps that gate from being dragged down by
# 0% coverage on an untestable wiring file. The merged total still
# counts e2e's coverage of this file. health.go in the same package
# IS unit-testable (httptest server + WH_SERVER_PORT) and stays in
# coverage everywhere — see issue #140 for the deferred refactor
# that would shrink main.go enough to drop this exclude entirely.
- ^cmd/wavehouse/main\.go$
1 change: 1 addition & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ Twelve internal packages under `internal/`:
14. **TypeScript SDK**: `@wavehouse/sdk` — zero-dependency client with typed query builder, real-time SSE, live queries with smart aggregation classification (incrementable/decomposable/poll), and codegen CLI.
15. **Observability invariants**: Stdout is *always* 100% — the slog logger fans out to stdout AND OTLP, and stdout sampling would silently hide records that scraping pipelines (Promtail/Alloy/Vector → Loki) are paying to store. Sampling knobs apply only to OTLP push. WARN+ERROR records always export at 100% regardless of `logs.sample_rate` — silently dropping errors during incidents would be a worse failure mode than the cost of forwarding them all (this is a *non-configurable* floor; do not expose it). gRPC OTel exporters dial lazily, so an unreachable collector never blocks startup; transient failures surface via the OTel SDK's error handler. The OTel Prometheus exporter (when enabled) uses a *private* `prometheus.Registry` to avoid leaking process/Go collectors that `prometheus.DefaultRegisterer` auto-registers into our `/metrics` output. When changing the logger, the sampler, or the provider wiring, preserve these invariants.
16. **Bearer-token-only CORS posture**: WaveHouse is a Bearer-token API — `Authorization: Bearer <jwt>` on every authenticated request, no cookies, no session middleware. The CORS middleware (`internal/api/router.go` `corsMiddleware`) deliberately **never** emits `Access-Control-Allow-Credentials`, because (a) we don't need it (Bearer tokens are explicit request headers, not browser-managed credentials) and (b) the historical pairing of `Allow-Credentials: true` with `Allow-Origin: *` is a CORS spec violation that browsers reject. The `cors_allowed_origins` allowlist controls *which origins can read responses*, not cookie scope. CSRF protection is structural: cross-site requests can't smuggle a Bearer token because the browser won't auto-attach `Authorization` headers cross-origin. Do not reintroduce cookie-based auth or `Access-Control-Allow-Credentials` without a separate design discussion — the current posture is the answer to GitHub issues #29 and #30.
17. **Non-fatal boot**: Schema-discovery failure on boot (ClickHouse unreachable, missing database, transient network blip) is non-fatal — `cmd/wavehouse` records an `api.BootState` diagnostic, binds `:8080`, and retries via `SchemaRegistry.RetryRefresh` (exp backoff 2s → 60s) in the background. `/health` and `/ready` return 503 with the latest diagnostic until a Refresh succeeds, after which `/health` stays 200 for the rest of the process. Keeps supervisor restart loops bounded and gives operators a queryable failure surface (`curl /health`) instead of a restart-log grep.

## Code Conventions

Expand Down
6 changes: 4 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## Unreleased

### Fixed
- **Ingest worker no longer infinite-retries permanent delete errors** (`internal/ingest/bento.go`, `internal/ingest/bento_test.go`, `docs/src/content/docs/architecture.md`, `AGENTS.md`): `jsInput.Read`'s `action: "delete"` block called `m.Nak()` on every `chConn.Exec` failure, which JetStream interprets as "redeliver immediately." A delete whose error was *deterministic* (syntax error, unknown table, malformed identifier) would loop forever — clogging the buffer consumer, burning CPU, and spamming logs with the same message. Phase 1 of issue #91: every delete-Exec error is now treated as permanent. The original NATS envelope is published to `dlq.<table>` (reusing the existing `bentoDLQDropped` counter when the DLQ publish itself fails) and the message is `DoubleAck`'d so it leaves the main queue. Issue #91 stays open after this lands as the Phase 2 tracker for transient-vs-permanent error classification (timeouts and network errors should still `Nak()` for retry); Phase 1 alone is the stopgap, Phase 2 makes the trade-off acceptable in production.
### Changed
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved

- **Boot-time schema discovery no longer crashes the binary** (`cmd/wavehouse/main.go`, `internal/api/health.go`, `internal/discovery/discovery.go`, `docs/src/content/docs/api.md`, `docs/src/content/docs/deployment.md`). Previously, any error from the initial `SchemaRegistry.Refresh` — connection-refused, missing database, transient network blip — caused `cmd/wavehouse` to call `os.Exit(1)`. The supervisor would restart the process every ~10s in an unbounded loop, port 8080 never bound, and operators got `connection refused` on probes even when ClickHouse was otherwise healthy. Now the first Refresh failure is non-fatal: a new `api.BootState` is set with the diagnostic, the server still binds `:8080`, and a background goroutine calls the new `SchemaRegistry.RetryRefresh(ctx, 2s, 60s, onAttempt)` with exponential backoff until success or shutdown. While `BootState.Err()` is non-nil, `/health` returns 503 with `{"status":"degraded","error":"…"}` so an operator can `curl /health` to learn why the gateway isn't ready to serve traffic instead of grepping a restart-loop log; `/ready` returns 503 with `{"status":"not ready",…}` for the same reason. Once `BootState.Set(nil)` fires (either from the initial sync `Refresh` or from `SchemaRegistry.RetryRefresh` success), `/health` stays at `200 OK` for the rest of the process lifetime — that flip is sticky and reflects "boot completed once." `/ready`, by contrast, remains conditional on current ClickHouse reachability: it can flip back to `503` on transient runtime blips after boot, since readiness is a probe of "can I serve traffic right now," not "did I ever boot." Resolves #95.

### Removed
- **`project-orchestrator.yml` workflow + its three composite-action artifacts** (`.github/workflows/project-orchestrator.yml`, `.github/actions/board-upsert-status/`, `.github/actions/set-linked-issues-status/`, `.github/scripts/board-fetch-item.sh`, `AGENTS.md`, `CHANGELOG.md`): −887 lines net. The orchestrator was the largest single source of cross-trigger complexity on this repo (3-4 workflow_run-chained runs per PR push, `statusCheckRollup` GraphQL perms quirks, integration-token `NONE` for private-org members) for behaviour that is mostly either provided natively by GitHub or a one-click manual operation on a 4-person team. Replaced by: reviewer-assign step in `housekeeping.yml` that fires once on `pull_request_target: opened` / `ready_for_review` (not per-synchronize, so it doesn't re-spam after `dismiss_stale_reviews_on_push`), plus GitHub's native Projects v2 workflows (`Auto-add to project`, `Item added`, `Pull request merged`) configured in the project UI. Trade-offs explicit in the PR body: drafts no longer auto-flip on bot-clean, `CHANGES_REQUESTED` doesn't auto-move the board card, linked-issue card mirroring is dropped. AGENTS.md §"Governance Files" + §"Task Board state machine" + §"Review tooling reference" all rewritten to match. `dependabot-automerge.yml` trimmed in parallel: no more board-upsert step (native handles placement), `PROJECT_BOARD_TOKEN` guard removed (no longer used in this workflow), reviewer list sourced from `board-config.env`'s `ADMINS` via `replace()`, major-bump comment uses the marker-comment upsert pattern from `housekeeping.yml`.
- **`STATUS_*` and old `ADMINS` consumers in `board-config.env`** — STATUS option IDs had only orchestrator-side consumers and are now unreferenced. `ADMINS` was restored to `board-config.env` after the initial orchestrator-removal commit dropped it (Gemini and Claude both flagged the resulting drift across three inlined copies); both `housekeeping.yml` and `dependabot-automerge.yml` now load `ADMINS` from `board-config.env`. `admin-approval.yml` keeps its own inline copy with the documented latency-avoidance reasoning.

### Fixed
- **Ingest worker no longer infinite-retries permanent delete errors** (`internal/ingest/bento.go`, `internal/ingest/bento_test.go`, `docs/src/content/docs/architecture.md`, `AGENTS.md`): `jsInput.Read`'s `action: "delete"` block called `m.Nak()` on every `chConn.Exec` failure, which JetStream interprets as "redeliver immediately." A delete whose error was *deterministic* (syntax error, unknown table, malformed identifier) would loop forever — clogging the buffer consumer, burning CPU, and spamming logs with the same message. Phase 1 of issue #91: every delete-Exec error is now treated as permanent. The original NATS envelope is published to `dlq.<table>` (reusing the existing `bentoDLQDropped` counter when the DLQ publish itself fails) and the message is `DoubleAck`'d so it leaves the main queue. Issue #91 stays open after this lands as the Phase 2 tracker for transient-vs-permanent error classification (timeouts and network errors should still `Nak()` for retry); Phase 1 alone is the stopgap, Phase 2 makes the trade-off acceptable in production.
- **CORS middleware: spec-compliant wildcard, no credentials, no header decoration on same-origin** (`internal/api/router.go`, `internal/api/router_test.go`, `AGENTS.md`, `config.yaml`, `docs/src/content/docs/configuration.md`, `docs/src/content/docs/deployment.md`): closes [#29](https://github.com/Wave-RF/WaveHouse/issues/29) and bookends [#30](https://github.com/Wave-RF/WaveHouse/issues/30). Three behavior changes, one rationale. (1) Dropped `Access-Control-Allow-Credentials: true` entirely — WaveHouse is a Bearer-token API (`Authorization: Bearer <jwt>`), cookies are never used (verified: no `http.Cookie` / `SetCookie` anywhere in the Go tree, TS SDK sends no `credentials: 'include'`), so credentials mode is unnecessary AND the previous combination of `Allow-Credentials: true` with `Allow-Origin: *` violated the CORS spec — browsers reject that pairing, which silently broke any client that ever set `credentials: 'include'`. (2) Requests with no `Origin` header (same-origin browser navigation, server-to-server, curl) now skip the CORS decoration entirely instead of unconditionally stamping `Allow-Methods`/`Allow-Headers`/`Allow-Credentials` on every response. (3) A preflight from a disallowed origin still returns 204 but with no CORS headers, which the browser treats as preflight failure — same outcome as before but without leaking the methods/headers list to origins that aren't allowed. Allowlist mode sets `Vary: Origin` on both hits *and* rejects so shared caches can't memoize a headerless reject under the URL alone and replay it to a later allowed-origin request. Test coverage in `router_test.go` pins each branch: wildcard echoes `*`, allowlist hit echoes origin + `Vary: Origin`, allowlist miss gets no `Allow-Origin` but still gets `Vary: Origin` (both for regular and OPTIONS), no-Origin requests pass through clean, and a table-driven test asserts `Allow-Credentials` is never emitted across wildcard / empty-allowlist / allowlist-hit. Posture is documented as `AGENTS.md` §"Key Design Decisions" item 16 so future contributors don't reintroduce credentials or cookie auth without a design discussion. Config sample updated with the dev recipe (point at `http://localhost:3000` etc. instead of `*` once a frontend is built).
- **OTel shutdown no longer hangs process exit when the collector is unreachable** (`cmd/wavehouse/main.go`): the `defer otelShutdown(context.Background())` was unbounded, and the OTel SDK's batch processors don't fully honor the shutdown context against an unreachable gRPC endpoint (the dial retries with backoff continue past the deadline). Bounded the shutdown to 5s. Discovered while writing `tests/integration/otel_test.go` `TestOTel_UnreachableEndpoint_DoesNotBlockStartupOrEmits`.
- **Coverage badge step no longer fails on every main push** (`.testcoverage.yml`, `AGENTS.md`): the `profile:` field pointed at `tmp/coverage/unit/coverage.txt` (~73% — `./internal/...` only) but `threshold.total` was 80, so `vladopajic/go-test-coverage`'s own gate failed unconditionally even after `make ci` had passed its per-suite + merged gates. Switched the action's profile to `tmp/coverage/total/coverage.txt` — the merged unit+integration+e2e profile `scripts/cov merge` produces at the end of `make ci` (~81% across the same files) — so the 80% gate applies to the project-wide number the badge advertises, not to a single suite. Per-suite gates in the YAML's `suites:` block continue to run inside `make ci` itself against the per-suite profiles before the badge step ever fires. AGENTS.md updated to state 80% project-wide minimum + per-suite minima.
Expand Down
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,7 @@ test-unit: go-mod-download ## Run Go unit tests + render coverage + gate thresho
@printf "$(CYAN)==> Running Unit Tests...$(RESET)\n"
@rm -rf $(COV_UNIT)/data && mkdir -p $(COV_UNIT)/data
@GOCOVERDIR="$(CURDIR)/$(COV_UNIT)/data" go tool gotestsum --format $(GOTESTSUM_FMT) -- \
-tags="$(TAGS)" -cover -race ./internal/... $(ARGS) \
-tags="$(TAGS)" -cover -race ./internal/... ./cmd/... $(ARGS) \
-args -test.gocoverdir="$(CURDIR)/$(COV_UNIT)/data"
@go run ./scripts/cov render unit

Expand Down
88 changes: 88 additions & 0 deletions cmd/wavehouse/health_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
package main

import (
"context"
"net"
"net/http"
"net/http/httptest"
"strconv"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

// runHealthCheck is the Dockerfile HEALTHCHECK probe — the bridge between
// the in-process /health endpoint and the docker daemon's container-health
// state. These tests pin the contract that holds the boot-non-fatal promise
// together: when wavehouse is boot-degraded, /health returns 503 and the
// probe must return 1 so Docker marks the container unhealthy. Without this
// coverage, a regression that swallowed non-200 responses would silently
// re-create the "stuck container" failure mode the PR is solving.
//
// Tests don't run in parallel because they all mutate WH_SERVER_PORT.
// t.Setenv handles save/restore.

// portFromListener pulls the integer port off a net.Listener. We use the
// listener directly (rather than parsing httptest.Server.URL) so the same
// helper works for both started-server cases and the connection-refused
// case where we listen-then-close to claim a known-unused port.
func portFromListener(t *testing.T, ln net.Listener) string {
t.Helper()
addr, ok := ln.Addr().(*net.TCPAddr)
require.True(t, ok, "listener should be a TCP listener")
return strconv.Itoa(addr.Port)
}

func TestRunHealthCheck_Success(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
// The probe targets /health specifically — pin that contract here
// so a refactor that points the probe at /ready (which requires
// ClickHouse) doesn't slip through silently.
assert.Equal(t, "/health", r.URL.Path)
w.WriteHeader(http.StatusOK)
}))
defer srv.Close()

t.Setenv("WH_SERVER_PORT", portFromListener(t, srv.Listener))
assert.Equal(t, 0, runHealthCheck(), "200 OK should map to exit 0")
}

func TestRunHealthCheck_BootDegraded503(t *testing.T) {
// This is the user-facing contract under boot-degraded state: the
// in-process /health returns 503 with a diagnostic body, the probe
// sees the non-200, returns 1, and Docker's HEALTHCHECK marks the
// container unhealthy. The K8s startupProbe path in deployment.md
// relies on exactly this signal.
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
w.WriteHeader(http.StatusServiceUnavailable)
_, _ = w.Write([]byte(`{"status":"degraded","error":"schema discovery: connection refused"}`))
}))
defer srv.Close()

t.Setenv("WH_SERVER_PORT", portFromListener(t, srv.Listener))
assert.Equal(t, 1, runHealthCheck(), "503 should map to exit 1 — Docker marks unhealthy")
}

func TestRunHealthCheck_ConnectionRefused(t *testing.T) {
// Wavehouse hasn't bound :8080 yet (e.g., still in the pre-listen
// phase of `run()`, or crashed entirely). Bind+close a TCP listener
// to claim a port that's now guaranteed unused, so the probe's GET
// gets a real connect-refused rather than a slow timeout.
var lc net.ListenConfig
ln, err := lc.Listen(context.Background(), "tcp", "127.0.0.1:0")
require.NoError(t, err)
port := portFromListener(t, ln)
require.NoError(t, ln.Close())

t.Setenv("WH_SERVER_PORT", port)
assert.Equal(t, 1, runHealthCheck(), "connection refused should map to exit 1")
}

func TestRunHealthCheck_InvalidPortEnv(t *testing.T) {
// Garbage env var should fail fast at the parse step, not propagate
// a confusing dial error. Catches the case where an operator
// fat-fingers WH_SERVER_PORT in a deployment manifest.
t.Setenv("WH_SERVER_PORT", "not-a-number")
assert.Equal(t, 1, runHealthCheck(), "non-integer WH_SERVER_PORT should exit 1")
}
Loading
Loading