diff --git a/AGENTS.md b/AGENTS.md index 4fda906f3..c14d027d7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -17,7 +17,9 @@ Agent instructions for every coding agent (Claude Code, Codex, Cursor, Copilot, - The only competitor-facing pages in `web/docs/` are the `migrate-from-*.md` import guides. ## Development workflow -Laptop only: if a server already runs on port 4000, apply code changes directly (`make dev` / `make dev-test` hot-reload). `rtk` prefixes, the k8xp VPN and `gopass` are user-global setup, not part of the repo. Sandboxes and Claude cloud sessions: see [Without Docker](#without-docker-cloud-sessions-ci-like-sandboxes). +Laptop only: if a server already runs on port 4000, apply code changes directly (`make dev` / `make dev-test` hot-reload). Never stop or `pkill` that devloop: run anything else on another port and kill it by PID. `rtk` prefixes, the k8xp VPN and `gopass` are user-global setup, not part of the repo. The maintainer's dev instance (`solidping.k8xp.com`) and the prod fleet are operated from the private `k8xp` repo (`k8s/solidping/OPERATIONS.md`). When a git answer gates a decision (current branch, did a merge land), check it with plain `git`: `rtk git` has reported stale and even invented branch names. Sandboxes and Claude cloud sessions: see [Without Docker](#without-docker-cloud-sessions-ci-like-sandboxes). + +Before calling the API of any server, `GET /api/mgmt/version`: it gives the run mode (which credentials apply) and the commit actually serving. 1. Start infrastructure: `docker-compose up -d` 2. Run everything: `make dev` (backend + dash0 + status0 with hot reload) @@ -118,6 +120,11 @@ curl -s -H "Authorization: Bearer $TOKEN" 'http://localhost:4000/api/v1/orgs/def - **Backend**: table-driven tests + testcontainers for integration (see `server/AGENTS.md`) - **Dash0**: Playwright E2E in `web/dash0/e2e/` (see `web/dash0/AGENTS.md`) - Comprehensive coverage expected for new features +- To reproduce or verify UI behaviour, write or run a Playwright spec rather than driving a browser through an MCP tool (much slower). Use the user's browser only when the flow needs their logged-in session. +- Local E2E runs use a side-car server, never the `:4000` devloop. Recipe and traps: [wiki/testing/local-e2e.md](wiki/testing/local-e2e.md). A full suite run must stay under 1 hour; a slower run is a harness bug to fix, not to wait out. +- Flaky tests are bugs: root-cause them, never re-run to green ([wiki/testing/flaky-tests.md](wiki/testing/flaky-tests.md)). A test asserting an absence needs a positive control. +- Fix lint findings in code. Never relax `.golangci.yml` or the eslint config to get green. Local linter vs CI parity, red-CI traps and the release flow: [wiki/runbooks/ci-and-release-triage.md](wiki/runbooks/ci-and-release-triage.md). +- Server behaviours that mislead when operating a deployment (migrations on rollout, inert env vars, provider callbacks, check diagnosis): [wiki/runbooks/operations-gotchas.md](wiki/runbooks/operations-gotchas.md). ## Never name a real company — use `acme` @@ -142,6 +149,7 @@ Exceptions: ## Specs - Filename format: `YYYY-MM-DD-NN-title.md` (`NN` unique per day across `specs/todos/` and `specs/done/YYYY/MM/`) - Active: `specs/todos/`, Done: `specs/done/YYYY/MM/`, Backlog: `specs/backlog/`, Cancelled: `specs/cancelled/` +- Never delete the `specs/todos/` directory itself, even when it is empty. ## Batch branches `/implement-todos` and similar multi-spec runs integrate onto a dated batch branch (e.g. `batch/2026-06-23`). diff --git a/server/AGENTS.md b/server/AGENTS.md index 9ef2a5494..4192a66e4 100644 --- a/server/AGENTS.md +++ b/server/AGENTS.md @@ -104,8 +104,8 @@ Mechanics worth knowing before you touch an existing file: (`make dev` / `dev-test` / `dev-saas`) always runs warn. `solidping migrate repair` re-records checksums for applied migrations (no migration runs) to clear a cosmetic-edit mismatch. See `wiki/conventions/database.md` for the full guard/repair writeup. -- **`internal/middleware/`**: Authentication, CORS, logging, and organization context -- **`internal/config/`**: Configuration management using koanf (YAML + environment variables) +- **`internal/middleware/`**: Authentication, CORS, logging, and organization context. Adding a middleware to a shared chain (`orgGroup`)? Update the hand-built test fixtures that copy that chain in the same change (`wiki/testing/flaky-tests.md`). +- **`internal/config/`**: Configuration management using koanf (YAML + environment variables). The env loader maps every `_` after `SP_` to `.`, so a new `SP_*` variable for a multi-word key (`rate_limiting`, `max_open_conns`...) needs a manual reader in `Load()` next to `applyRateLimitingEnv`, or it is silently ignored. ## Database Schema diff --git a/web/dash0/AGENTS.md b/web/dash0/AGENTS.md index 0276bccef..91e67550c 100644 --- a/web/dash0/AGENTS.md +++ b/web/dash0/AGENTS.md @@ -185,6 +185,13 @@ mirror `incidents.index.tsx` and `jobs.index.tsx`. The primary navigation `replace: true` only for incidental refinements (filters, scope toggles) so they don't spam history. +Caveat: under a layout route, a cold deep link can run a second +search-validation pass with an empty search and drop the param. If a deep +link resets, seed local state from the URL once on mount and write the +param back on every change (`organization.discovery.new.tsx`). Make +`validateSearch` omit absent keys (return `{}`), or every `` to the +route will require a `search` prop. + ### Row actions: icons, not menus In list/table rows, prefer two ghost icon buttons (`Pencil` for edit, diff --git a/wiki/README.md b/wiki/README.md index fb9c5e74f..5a915bba0 100644 --- a/wiki/README.md +++ b/wiki/README.md @@ -102,12 +102,16 @@ Operational procedures for diagnosing the running system. - [runbooks/api-testing-with-curl.md](runbooks/api-testing-with-curl.md) — Log in, save a token, and exercise the REST API by hand with curl; default credentials, forced password rotation, troubleshooting. - [runbooks/claude-cloud.md](runbooks/claude-cloud.md) — Working in a Claude cloud (remote) sandbox with no Docker, secrets or built frontend: `scripts/cloud-setup.sh`, what a clean clone needs (embed placeholders), SQLite smoke server, CI env parity for E2E, what to run before pushing. - [runbooks/observability-toggles.md](runbooks/observability-toggles.md) — `SP_PROMETHEUS_ENABLED`, `SP_METRICS_SCRAPE_TOKEN`, `SP_PROFILER_ENABLED`, `SP_OTEL_ENABLED`: what each switches and how they interact. +- [runbooks/ci-and-release-triage.md](runbooks/ci-and-release-triage.md) — CI and release states that lie: skipped jobs, conflicting PRs with zero runs, flapping run states, local golangci-lint vs CI parity, the tag-triggered release flow, release-please silent failures and their recovery, and why squash-merged branches look unmerged. +- [runbooks/operations-gotchas.md](runbooks/operations-gotchas.md) — Server behaviours that misled whoever ran a real deployment: unlocked migrations on rollout, env vars that silently do nothing, one callback URL per provider app, signed calls to a renamed org, check diagnosis, silent realtime, connection-pool budgets. ## Testing - [testing/coverage.md](testing/coverage.md) — Backend coverage: how `make test-cover` and the `backend-postgres` gate measure it, what is excluded (generated/vendored), the baseline, the `COVERAGE_MIN` ratchet, and report-only frontend coverage - [testing/test-layers.md](testing/test-layers.md) — Which Go test layer runs where (PR CI / nightly / local only): `make test` is `-short` and covers **no** Postgres, `make test-postgres` and the `backend-postgres` job do, `SP_TEST_REQUIRE_POSTGRES` turns a skip into a failure, and the `slowtests` build tag holds the live-network/Docker suites - [testing/e2e-ci.md](testing/e2e-ci.md) — E2E test infrastructure: CI environment, Playwright config, local execution +- [testing/local-e2e.md](testing/local-e2e.md) — Running dash0 E2E on a laptop: the Postgres side-car recipe, six traps that each fake a red run (stale embed, base URL, env parity...), the `make dev-test` fast path, and how to write specs that survive the full suite. +- [testing/flaky-tests.md](testing/flaky-tests.md) — Flaky tests are bugs: rules, proof techniques (widen the window, wall-clock landmines), and the known shapes in this repo (testcontainer contention, hand-built middleware fixtures). - [testing/http-test-checks.md](testing/http-test-checks.md) — Fake API test checks: 5 predefined scenarios (stable, flaky, unstable, slow, 503) - [testing/dash0-e2e-typecheck-and-lint.md](testing/dash0-e2e-typecheck-and-lint.md) — Why `e2e/` has its own tsconfig and scoped lint, and the `page.evaluate` closure guard. diff --git a/wiki/conventions/database.md b/wiki/conventions/database.md index 2bef7ca49..4d298e20a 100644 --- a/wiki/conventions/database.md +++ b/wiki/conventions/database.md @@ -193,6 +193,36 @@ shipping a permanent migration. `014_v0_17_0` is the consolidated v0.17.0 migration, unrelated to that draft; see the consolidation rule above for why reusing the number is safe here and what it costs dev databases.) +### Diagnosing a "no such column" 500 on a dev database + +The usual cause: a spec appended a SECTION to the unreleased migration after your +dev database had already applied it, so the new statements never ran. + +1. **Read the guard first.** `grep -i checksum logs/backend.log*` (glob the rotated + `.1`/`.2` files too: the boot-time warning may only be there). The guard names the + migration, both checksums and the repair options. It once logged the answer 67 + times while someone read migration files instead. +2. **Measure the blast radius.** Grep `logs/backend.log*` for the missing column, not + just the endpoint you noticed. The same missing column once also broke the check + worker's active-incident query, so incident detection was dead on every probe. +3. **Pin the applied version.** `select checksum from migration_checksums where name='NNN'` + gives the sha256 of the file as applied. `git log --format=%h -- ` then + `git show : | shasum -a 256` finds the commit whose version ran. The + missing delta is `git diff HEAD -- `; no `-` lines means it is purely + additive and can be applied by hand. +4. **Prefer a hand fix over a reset when the data matters.** Apply the missing + statements, then sweep the current file object by object (every `create table`, + `alter table ... add column`, `create index` against `sqlite_master` / + `pragma table_info`, and each backfill `UPDATE`'s WHERE as a `count(*)`). A database + can be partially stale after earlier hand fixes, so do not assume. Only then run + `solidping migrate repair` (option 3 above). SQLite picks up an `ADD COLUMN` without a + restart. + +When a consolidation is under way, verify it by replay rather than by the test suite: +apply every `*.up.sql` in order to an empty SQLite file, assert each folded section's +objects exist, apply the `.down.sql` and assert they are gone. Before deleting a +migration file, `grep 'migrations/NNN_' --include=*.go` for tests that read it by name. + **The unreleased migration right now is `025_v0_34_0`** (opened 2026-09-27 by spec 2026-09-27-01, the "Capture now" failure columns on `check_jobs`). `024_v0_33_0` is the last RELEASED migration (it shipped in v0.33.0), so any diff --git a/wiki/conventions/specs-workflow.md b/wiki/conventions/specs-workflow.md index cbf9982cf..2706e1238 100644 --- a/wiki/conventions/specs-workflow.md +++ b/wiki/conventions/specs-workflow.md @@ -48,3 +48,13 @@ How should we solve it? - When searching by slug, match partially (e.g., `credentials` matches `2025-12-06-credentials.md`) - Specs in `todos/` are processed oldest-first by `/implement-todos` - Archived specs go to `specs/done/YYYY/MM/` based on their date prefix +- Never delete `specs/todos/` itself, even when emptying it +- A spec's Proposal and Decisions define its scope. An implementation plan that narrows + them does not, and an audit reads the Proposal, not the plan +- An **investigation** spec (diagnose, not build) names both legitimate outcomes: a + product-side cause gets a fix plus a regression test at the layer that was wrong; an + infra-side or unreachable cause gets a written finding in `wiki/` ("I could not reach + the data, here is what I tried and what to run next" is an honest finding). Restate + its non-goals as hard limits, and say whether small writes to a live system are + allowed to reach a diagnosis. With no code to commit, the closing + `chore: all checks passing for ` commit is made with `--allow-empty` diff --git a/wiki/features/notifications-and-escalation.md b/wiki/features/notifications-and-escalation.md index c54c75211..17d7b0252 100644 --- a/wiki/features/notifications-and-escalation.md +++ b/wiki/features/notifications-and-escalation.md @@ -557,6 +557,15 @@ recorded; they just don't trigger paging. When the window ends, the next failing result will open a fresh incident if the failure persists. There is no "deferred" incident that fires at window end. +Membership is only what the window lists: its checks, plus the checks of +its groups (`ListMaintenanceWindowsForCheck`). **A window with nothing +attached suppresses nothing** and shows no "under maintenance" badge on +status pages. There is no "empty means org-wide" fallback. An operator +who forgets to attach checks believes they are covered and gets paged +during planned work, so dash0 renders a 0 count as an amber warning +(`ChecksCountCell` in `maintenance-windows.index.tsx`), never as a +neutral number. Keep it that way on any new surface showing the count. + ### Per-incident silencing (ack / snooze / manual resolve) Operators can ack, snooze, or manually resolve an incident at any time. diff --git a/wiki/runbooks/ci-and-release-triage.md b/wiki/runbooks/ci-and-release-triage.md new file mode 100644 index 000000000..bd2391bfd --- /dev/null +++ b/wiki/runbooks/ci-and-release-triage.md @@ -0,0 +1,127 @@ +# Reading CI and releases without fooling yourself + +Every item here is a way GitHub Actions, the linters or release-please reported +something that was not true. Read this before concluding "flake", "green" or +"released". + +## CI states that lie + +- **`skipping` is not healthy.** E2E and Build Binary are skipped when an upstream job + (Dash0 Build, Backend Lint & Test) fails. On a long-lived branch with one red job, E2E + regressions pile up unseen, then all land at once when the upstream job goes green, + and look like the fix broke them. Compare with `main`'s runs to know whether E2E is + met or only deferred. +- **A conflicting PR creates no runs at all.** With `mergeStateStatus: DIRTY`, GitHub + cannot build the merge ref, so a new push gets zero workflow runs. `gh pr checks` + says `no checks reported` and exits non-zero, which a naive wait loop reads as + "pending". Confirm with + `gh api "repos/fclairamb/solidping/actions/runs?head_sha=$SHA" --jq .total_count` + (0) and `gh pr view <n> --json mergeStateStatus`. `BLOCKED` only means checks are + pending. Fix by merging `origin/main`; for `go.sum` conflicts take the merged + `go.mod` and run `go mod tidy` rather than hand-resolving hashes. `main` moves during + a long batch: re-check before the final push, not only at the start. +- **Run states can flap.** A job re-queued after a runner loss goes + `completed/failure` back to `in_progress`. Require two consecutive stable reads, and + prefer the REST jobs API over `gh pr checks` buckets. +- **The E2E "server was DOWN" banner is script text.** If the job died at setup + (for example `failed to bind host port ... 55432 ... address already in use`), the + unconditional banner follows. Look for the setup error before believing it. +- **`gh run view --log-failed` refuses while any job still runs.** Use + `gh api repos/fclairamb/solidping/check-runs/<id>/annotations` for lint findings + right away. +- **Playwright browser install used to hang** in the `--with-deps` apt phase until the + 6 h job timeout. Each attempt is now wrapped in `timeout 90` inside a retry loop, and + the job has `timeout-minutes`. A re-run alone was not a reliable fix. + +## Local lint must match CI's linter + +`make lint-back` runs whatever `golangci-lint` is first on `PATH`. CI installs the +version pinned in `.github/workflows/ci.yml` with `install-mode: goinstall`, so its +linter is compiled with CI's Go, and its embedded `go/format` is that Go's. A green +local lint is only a predictor when both match. + +- Check `golangci-lint --version` (it prints the Go it was built with) against + `ci.yml`. After a Go upgrade, re-`go install` the linter: the version number alone + is not enough. A second, older install (for example Homebrew's) can shadow it if + `PATH` order changes. +- Exact replica, without touching the global binary: + ```bash + GOTOOLCHAIN=go<ci-go> GOBIN=<scratch>/bin go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@<ci-version> + cd server && GOTOOLCHAIN=go<ci-go> <scratch>/bin/golangci-lint run ./... + ``` +- Linter versions disagree in ways no directive satisfies. Seen: one `unparam` version + fires on an always-nil `error` return and the next does not, so the `//nolint:unparam` + waiver becomes an unused directive that `nolintlint` fails. A local `unparam` + complaint CI does not raise means the local linter is stale. +- gofmt and gofumpt can deadlock: two composite literals returned inline + (`return []string{...}, []string{...}`) get de-indented by one and re-indented by the + other. Fix the code (assign to locals, `return a, b`), never the config. +- `golangci-lint fmt --diff` skips `_test.go` files while `run` includes them. Reproduce + formatter findings with `run`. The formatter name `run` reports can be unrelated to + the real diff: get the diff from the tool (`gofumpt -d`, + `$(GOTOOLCHAIN=go<ci-go> go env GOROOT)/bin/gofmt -d`; `GOTOOLCHAIN` does not redirect + the `gofmt` on `PATH`). +- Fix lint findings in the code. Never relax `.golangci.yml` to get green. A test-only + exclusion is acceptable only with a `path: _test\.go` filter, after showing the code + fix was tried. + +## Release flow + +1. PRs are squash-merged with the PR title as the commit subject. +2. release-please (`.github/workflows/release-please.yml`, `release-type: simple`) + opens or updates `chore(release): vX.Y.Z` on `release-please--branches--main`. + `feat` bumps the minor, `fix` the patch, hidden types (`chore`, `docs`, `ci`, + `refactor`, `test`) ship without a changelog line or a bump. +3. Expand the changelog in the release PR (see + [changelog conventions](../conventions/changelog.md)). Edit from a separate + worktree; the local `release-please--branches--main` ref is usually stale, so + `git checkout -B <branch> origin/<branch>` there. +4. Merging the release PR makes release-please create the tag and GitHub release. +5. **Build and deploy are tag-triggered.** On the release commit's `main` run the + Docker and deploy jobs show "skipped". The real build is the `vX.Y.Z` tag run. + That deploy only updates the main dev API Deployment; every other deployment is + bumped by hand. + +## release-please fails silently + +The workflow goes green in every case below. The tell is the release PR's +`headRefOid` not moving after a merge, or the release PR still labelled +`autorelease: pending` instead of `autorelease: tagged` after its own merge. + +- **Unparseable PR title.** Double quotes in the title, or no conventional type at all, + drop the commit from both the changelog and the version computation. One such title + once made 147 commits vanish. `.github/workflows/pr-title.yml` validates titles now. + The run log says `commit could not be parsed`. +- **Hand-edit then a fast auto-merge.** Pushing a changelog commit to the release + branch and merging within ~90 s, before release-please re-runs, twice produced no tag. + The `Detect release drift` step catches it. Wait for one release-please run after + editing, or plan to tag by hand. + +Recovery when the tag is missing: + +```bash +FULL=$(gh pr view <releasePR> --json mergeCommit --jq .mergeCommit.oid) # full 40-char sha, a short one is rejected +git show origin/main:CHANGELOG.md | awk '/^## \[X.Y.Z\]/{f=1;next} /^## \[/{f=0} f' > notes.md +gh release create vX.Y.Z --target "$FULL" --title vX.Y.Z --notes-file notes.md +gh pr edit <releasePR> --remove-label "autorelease: pending" --add-label "autorelease: tagged" +gh pr list --state open # close any chore(release) PR with a LOWER version, never merge it +``` + +That failing run often opens a bogus release PR for a lower version; merging it +downgrades the manifest and rewrites `CHANGELOG.md`. A tag created with a user token +does trigger the tag-gated build and deploy. + +When the version itself is wrong (features shipped under a patch), merge a PR whose +squash body carries a `Release-As: X.Y.Z` footer (pass `--subject`/`--body` to +`gh pr merge --squash` so the footer survives), then write the changelog section by +hand on the release branch. + +## Squash merges make every branch look unmerged + +Local branches never have merge ancestry with `main`, so `git branch --no-merged` +lists hundreds of shipped branches, and a three-dot `git diff origin/main...<branch>` +still shows their full diff. To find really unshipped work, compare tips: +`git diff --stat origin/main <branch>` (two dots), or +`git merge-tree --write-tree origin/main <branch>` and compare with +`origin/main^{tree}`. Never bulk-merge branches from `--no-merged`, and never hand-merge +`release-please--branches--main`. diff --git a/wiki/runbooks/operations-gotchas.md b/wiki/runbooks/operations-gotchas.md new file mode 100644 index 000000000..ec2921464 --- /dev/null +++ b/wiki/runbooks/operations-gotchas.md @@ -0,0 +1,127 @@ +# Operating a deployed instance: gotchas + +Behaviours of the server that surprised whoever ran a real deployment. Each one +looked like a different problem at first. Deployment-specific runbooks (hosts, +clusters, secrets) live in the operator's private infrastructure repo, not here. + +## Know what you are talking to + +- `GET /api/mgmt/version` first. It gives the version and commit (is this the build you + think?) and the run mode (which credentials apply). When a host can be routed either + to a cluster or to a developer laptop, the version string tells you which one + answered. +- Check the deployed version against the migration that ships a column before planning + a diagnosis around that column. A diagnostic field added in release N does not exist + on an instance running N-1. + +## Migrations and rolling a release + +- Every pod that opens the main database runs migrations at boot, whatever its + `SP_NODE_ROLE`, and the migrator takes no lock. Roll the API first and let it + finish, then the checks workers. On one fleet bump the API beat a worker by 818 ms: + luck, not design. Deported agents never migrate. +- Old workers break on a new schema or a new job type (`column ... does not exist`, + `unknown job type: ...`) and crash-loop. When `git diff vA..vB -- server/internal/db/*/migrations` + is non-empty, bump every worker right after the API. A worker must never be ahead of + its API: it would run the migration under the old API. +- Bump every deployment of one environment to the same image tag. Never deploy + `:latest` (it drifted once with nothing forcing a re-pull). + +## Configuration that silently does nothing + +- The koanf env loader turns every `_` after `SP_` into a `.`, so a multi-word key + (`rate_limiting`, `requests_per_minute`, `trusted_proxies`, `shutdown_timeout`) is + only read through a manual reader in `server/internal/config/config.go` + (`applyRateLimitingEnv` and friends). Those readers want the exact full name: the + deployment that set `SP_RATE_LIMITING_TRUSTED_PROXIES` (no `SERVER_`) had no trusted + proxies and bucketed every client under the ingress IP. Verify live: + `GET /api/mgmt/limits` returns `callerIp`; an internal cluster address there means + trusted proxies are not applied. +- `SP_DEPLOYMENT_MODE` must be identical on API and checks pods. Checks pods enforce + `maxChecksPerMinute` at dispatch, its default comes from the deployment mode, and the + `self-hosted` default is unlimited. A checks pod missing `saas` runs uncapped, which + also uncaps SMS and voice paging spend. The reservation itself fails open + (`ReserveCheckExecution failed; running check anyway`). +- Under `hostNetwork`, `os.Hostname()` returns the node FQDN, which fails the worker + slug rule. Set `SP_NODE_NAME`. Without a stable name, every restart registers a new + `workers` row, and nothing garbage-collects in-process worker rows (the agent GC only + retires deported agents). Under `hostNetwork`, Kubernetes also defaults `hostPort` to + `containerPort`, so a checks pod that inherits `containerPort: 4000` reserves the + node's `:4000`; set `ports: null`. +- A region whose workers report unknown (NULL) capabilities is running an image older + than the egress self-probe. NULL means "nothing probed", deliberately distinct from + "none". Check the image tag before the network. +- An integration is only `Active()` when its token and id are non-empty. An expired + token still flips the dashboard to "enabled" while every send fails. Leave the + setting empty rather than filling it with a dead value. +- Set `SP_TELEGRAM_BOT_USERNAME` explicitly. The connect surface needs it, and a + boot-time `getMe` call can lose to a cold DNS cache. + +## One callback URL per provider app + +Telegram bots, Discord applications and Meta apps each hold one webhook or +interactions URL. Two environments sharing one app steal each other's callbacks. +Use one bot or app per environment (WhatsApp can instead use one app with a +per-phone-number webhook override). For Discord, set `SP_DISCORD_PUBLIC_KEY` and the +bot token and restart before saving the Interactions Endpoint URL: Discord probes it +with a valid and an invalid signed ping and deactivates it if verification fails. An +unsigned `POST` there answering 401 is correct. Mention commands and thread-reply +ingestion also need `SP_DISCORD_GATEWAY_ENABLED` (off by default). + +## Signed service calls and renamed orgs + +`X-SP-Signature` covers the request path. A call addressed to an org's previous slug +gets redirected (301/308) by `orgSlugRedirect`, the client follows it with the original +signature, and verification fails as a generic 401 (`signature mismatch` in the server +log). When a signed push 401s, check the path first: +`curl -o /dev/null -w '%{http_code} %{redirect_url}' <url>`. A redirect means a stale +slug on the caller's side, not a wrong key. + +## Checks that look healthy or broken for the wrong reason + +- `status: validating` means failures were observed and the threshold is not crossed + yet. It is a failing state, not a warm-up. +- `POST /orgs/:org/checks/validate` validates the config shape only; it never connects. + The real driver error is in a result's `output.error` + (`GET /orgs/:org/results?checkUid=<uid>`). A failure in ~15-30 ms is a protocol-level + rejection (auth); a network problem sits at the timeout. +- A port closed from a public region may be open from inside the target network. Probe + from a private location in that network before calling it down. A name that resolves + can be a DNS wildcard hit; check the answer. +- A passive email check only reads the ingest mailbox's Inbox. Mail the receiving + server classifies as spam lands in Junk and the check sees "no email in period", with + no error. List the Junk, Inbox and Processed folders first; their dates say whether + mail stopped, got junked, or the poller died. A manual test mail from a large + provider (aligned DKIM) can land in Inbox and make the check flap up while the real + sender still gets junked. Behind a Kubernetes LoadBalancer that SNATs (the default + `externalTrafficPolicy: Cluster`), the mail server sees every client as a cluster IP + and SPF always fails; `externalTrafficPolicy: Local` keeps the client IP. +- A maintenance window with no attached checks or groups suppresses nothing (see + [notifications](../features/notifications-and-escalation.md#maintenance-windows)). + +## Realtime updates stop with no error + +Signature: the WebSocket handshake and `subscribed` acks work, but no `update` frame +ever arrives. Cause seen once: a listener leak in the Postgres event notifier backed up +the single `pq.Listener` queue for hours (fixed since with `Unlisten`). Fast tells: + +- `SELECT pg_notification_queue_usage();` visibly above 0 means a listener is behind. +- `/metrics`: `solidping_realtime_hints_published_total` far above + `..._delivered_total`. +- A goroutine dump with hundreds of goroutines blocked on `pq.(*Listener).Ping`. + +The image is distroless (no shell, no pprof by default). Get a goroutine dump with +`kubectl debug <pod> --image=busybox:1.36 --target=backend -q -- sh -c 'kill -QUIT 1'`, +then `kubectl logs <pod> --previous`. The pod restarts, which also clears the backlog. + +## Database connections + +- Pools of every pod sharing one Postgres role must sum below the role's connection + limit (`SP_DB_MAX_OPEN_CONNS` / `SP_DB_MAX_IDLE_CONNS` per deployment). The overrun + only fires on a cold start, when all check runners reserve a connection at once: + `too many connections (SQLSTATE=53300)` confined to the minute after boot. Look at + the per-minute distribution before calling it a steady stream. +- Transaction-mode PgBouncer is not a drop-in: the jobs channel uses `LISTEN`/`NOTIFY`. +- A busy server logs about a dozen lines per second, so the container log rotates + within a minute. A clean `kubectl logs` window is not proof of recovery; read the + log store. diff --git a/wiki/testing/e2e-ci.md b/wiki/testing/e2e-ci.md index 09d84b997..135f931e6 100644 --- a/wiki/testing/e2e-ci.md +++ b/wiki/testing/e2e-ci.md @@ -52,7 +52,7 @@ merged `results.json` when the two shards drift apart. Re-check the split in the - **Browser**: Chromium only - **Database**: Postgres from `docker-compose.yml` (`SP_DB_TYPE=postgres`), reset at start - **Server env**: `SP_RUNMODE=test`, rate limiting off, `SP_AUTH_REGISTRATION_EMAIL_PATTERN=.*` - (a local side-car must match these, see the `/implement-todos` side-car recipe) + (a local side-car must match these, see [local-e2e.md](local-e2e.md)) - **Test credentials**: `test@test.com` / `test` / org `test` ## Playwright Configuration (`web/dash0/playwright.config.ts`) diff --git a/wiki/testing/flaky-tests.md b/wiki/testing/flaky-tests.md new file mode 100644 index 000000000..7a4243c79 --- /dev/null +++ b/wiki/testing/flaky-tests.md @@ -0,0 +1,70 @@ +# Flaky tests are bugs + +A test that fails sometimes is reporting a race, a leak, an unrecovered panic or an +order dependency. Re-running until green ships that bug. Root-cause every +intermittent failure, including CI-only ones. + +## Rules + +- Do not stop at "CI-only flake". Add the observability that captures evidence next + time (keep the server log, do not `& disown` it). +- Prefer guards that turn a silent flake into a hard signal (panic recovery so one bad + request 500s instead of killing the server and cascading connection-refused errors). +- Test-order pollution is a bug in the polluting test. Fix the leak, never loosen the + victim. +- A conditional `test.skip(...)` that skips when a fixture is missing can hide a broken + seed. +- A test asserting an absence ("the secret is not in the logs") needs a positive + control first ("the log line is captured"), or it passes vacuously. +- A fake server must be at least as strict as the real one. The Slack fakes once + accepted JSON bodies on `conversations.list`, which real Slack silently ignores + (it wants form encoding), so a green test proved nothing. + +## Techniques + +**Widen the window.** A test that times out on its full deadline, rather than running +slow, usually lost a notification (subscribe after announce). Prove it with controls, +not re-runs: + +1. Add a `time.Sleep` that widens the suspected window in the unmodified code. It must + reproduce CI's failure exactly (same `file:line`, same message). +2. Apply the fix, keep the sleep. It must pass. + +Passing N times in isolation proves nothing (one such test passed 5 times locally +before its fix). + +**Wall-clock landmines.** A "failed once under load" test may only fail during one +second of the minute, for example when a hash of a hard-coded UID sets a schedule +phase. Tells: a failure margin of nanoseconds over an exact bound, every concurrent +instance failing in the same second, a function's internal clock compared to a +`time.Now()` sampled earlier. Reproduce by starting the test aligned just before the +suspect second, not by stressing the machine. Fix by bounding against a clock sampled +after the call. + +**Pre-existing `-race` findings.** Restore the committed file and re-run before +blaming your change. CI runs `go test -short` without `-race`, so a race can be real +and still not be what CI is failing on. + +## Known shapes in this repo + +- **Postgres testcontainers under parallel load.** A full local `go test ./...` can fail + a scattered set of Postgres, LDAP and PG-notify tests from container contention. + Re-run the failing packages with `go test -p 1 <pkgs>`. If they pass serially it was + contention, which does not make it fine: CI's Postgres layer runs `-p 4` + (`make test-postgres`), so the same contention can reach it. A per-package timeout + there is often intra-package parallelism on a slow runner: `internal/db/postgres` + takes ~3 min locally against Go's default 10 min per-package budget. +- **`internal/config` is hostname-independent.** Hostnames like `Host-002.lan` are + slugified now, so a red `internal/config` is a real regression, not the machine. + Check with `env -u SP_NODE_NAME go test ./internal/config/`. New tests there must + inject the hostname (see `TestSanitizeHostnameSlug_FixedPointProperty`). +- **Hand-built middleware fixtures.** Several handler tests compose their own + middleware chain and claim to mirror `server.go`. Adding a middleware to the real + chain leaves them green while they assert the old behaviour. When you add one, grep + them and update them in the same change: + `grep -rn "Use(authMw\.\|Use(authMW\.\|RequireAuth(authMw\|Shared:.*RequireAuth" --include=*_test.go server/`. + Turn an assertion that a role got through into a refusal test plus a positive control + at the lowest role that still passes. + +E2E-specific traps (side-car env, paging, shared query keys) are in +[local-e2e.md](local-e2e.md). diff --git a/wiki/testing/local-e2e.md b/wiki/testing/local-e2e.md new file mode 100644 index 000000000..d8863f8c7 --- /dev/null +++ b/wiki/testing/local-e2e.md @@ -0,0 +1,109 @@ +# Running dash0 E2E locally + +How to run the Playwright suite on a laptop without disturbing the developer's +`make dev` on `:4000`, and the traps that each produce a convincing false red run. +CI's own setup is in [e2e-ci.md](e2e-ci.md). + +## Rules + +- Never stop or `pkill` the `:4000` devloop (`pkill -f 'solidping serve'` kills it, and + `devloop` does not bring it back). Run a side-car server on another port and kill it + by the PID you recorded. +- Before trusting any result from a server, `curl <base>/api/mgmt/version` and check the + commit. Ports get reused between experiments: an old build has none of the new + frontend, so a test written for it passes there (false green) or fails (false red). +- A full suite run must finish in under 1 hour. ~850 tests pass well under that. A run + trending past an hour is mass failure multiplied by `retries: 2`, usually a harness + problem (base URL, stale embed, wrong env). Stop it and check the harness first. + +## Side-car recipe (full suite, Postgres) + +Copy it literally. `build-dash0` + `build-backend` reads complete and is not. + +```bash +make build-dash0 copy-dash0 && make build-backend # order matters, see trap 1 +nc -z localhost 55432 || docker-compose up -d postgres # the repo's solidping-postgres +docker exec solidping-postgres psql -U postgres -c "DROP DATABASE IF EXISTS solidping_e2e WITH (FORCE)" +docker exec solidping-postgres psql -U postgres -c "CREATE DATABASE solidping_e2e OWNER solidping" +mkdir -p /tmp/e2e-sidecar +SP_RUNMODE=test SP_DB_RESET=true SP_DB_TYPE=postgres \ + SP_DB_URL="postgres://solidping:solidping@localhost:55432/solidping_e2e?sslmode=disable" \ + PORT=4300 SP_DB_MIGRATION_GUARD_MODE=warn \ + SP_SERVER_RATE_LIMITING_REQUESTS_PER_MINUTE=0 SP_SERVER_RATE_LIMITING_MAX_CONCURRENT=0 \ + SP_AUTH_REGISTRATION_EMAIL_PATTERN='.*' \ + ./solidping serve > /tmp/e2e-sidecar/server.log 2>&1 & +echo $! > /tmp/e2e-sidecar/pid +curl -s http://localhost:4300/d/ | grep -o '<title>[^<]*' # must say SolidPing +cd web/dash0 && CI=true E2E_BASE_URL=http://localhost:4300/d/ bun run test:e2e +kill "$(cat /tmp/e2e-sidecar/pid)" +``` + +`CI=true` makes `e2e/global-setup.ts` skip building and starting its own server, and +gives CI's retries. `playwright.config.ts` and `global-setup.ts` both honour +`E2E_BASE_URL`. To mirror CI's sharding, see the `E2E_BASE_URLS` example in +[e2e-ci.md](e2e-ci.md). + +## Traps + +1. **`make build-backend` does not rebuild or re-embed dash0.** It compiles Go against + whatever sits in `server/internal/app/dash0res/`. A spec that adds a `data-testid` + then looks like a missing feature. Run `build-dash0 copy-dash0` first, and before + believing a red run, check the embed: `grep -rl '' server/internal/app/dash0res/`. + Same for a side-car built for an earlier experiment. +2. **`E2E_BASE_URL` must end in `/d/`.** With a bare origin every relative navigation + lands on the server's 404 page. The signature: every browser test red, the few + pure-API tests green. +3. **Match CI's server env.** Without the two rate-limiting zeros, + `magic-wand.spec.ts` gets 429s (it fires 100+ concurrent creates). Without + `SP_AUTH_REGISTRATION_EMAIL_PATTERN='.*'`, `confirm-registration-no-org.spec.ts` + cannot register, and setting it at runtime does nothing (read once at boot). When a + suite is red locally and green in CI, diff your server env against + `.github/workflows/ci.yml` before reading any application code. +4. **Force Postgres.** `SP_RUNMODE=test` with no `SP_DB_TYPE` can end up on SQLite, + which serializes every request on one connection. The suite was moved off it. +5. **Use `solidping_e2e`, never `solidping`.** `SP_DB_RESET=true` against the shared dev + database wipes local data. Drop and recreate the dedicated database per run. +6. **A foreign container may answer to `postgres`.** Other projects on the same laptop + run their own Postgres. `role "solidping" does not exist` is the giveaway. The + repo's service is container `solidping-postgres` on port `55432`. + +## Fast iteration against `make dev-test` + +For one spec file while developing: + +- `make dev-test` (test mode on `:4000`, dash0 Vite on `:5174`), then + `cd web/dash0 && bun run test:e2e:dev --workers=1`. Parallel logins trip the login + rate limiter. +- `SP_DB_RESET=true make dev-test` resets the test database. It persists across + restarts otherwise, and leftovers cause quota, `409` and stale-seed failures. +- Do not stack `make dev-test`. `make kill` frees the ports but leaves the parent + `go run ./cmd/devloop` processes, which then fight over `:4000` and hang requests for + ~30s. Stop the old devloop before starting a new one. +- Restore the developer's normal `make dev` afterwards. + +Local-only tests (excluded from CI) are gated with +`test.skip(!!process.env.CI && !process.env.E2E_LOCAL, "local-only")`, so +`CI=true E2E_LOCAL=1` still runs them against a side-car (see +`discovery-scan-method.spec.ts`). + +## Writing tests that survive the full suite + +Per-spec runs execute one file. CI runs everything in one order. These broke only +in the full suite: + +- **A seeded row on an unfiltered, paged list.** The suite creates users, orgs and + checks as it runs, so the oldest seeded row falls off page 1. Reach the row through + the page's search or filter, with a non-matching query as the negative control. +- **Weak text assertions.** `toContainText("test")` passes on the wrong badge when the + user belongs to `test`, `test2` and `test3`. Assert the exact text. +- **Ambiguous locators.** A card title (`

`) can duplicate the page `

` once data + accumulates. Pin `getByRole("heading", { name, level: 1 })`. +- **Shared React Query keys.** Two screens reading the same key share its 60 s + `staleTime`, so a list seeded by one screen hides a just-created entity from another. + Suspect this whenever two features touch the same entity list. +- **Recharts dots.** `.recharts-wrapper circle` also matches the hover `activeDot`. + Count data dots with `circle:has(title)`. A fractional expected count is the tell + that a selector catches a transient element. +- **Hardcoded `http://localhost:4000`.** A spec that does not derive its API base from + `E2E_BASE_URL` (via `e2e/base-url.ts`) silently hits the developer's devloop when run + against a side-car.