From 5453c26e4caf9847aa0fd79826dd46120c089693 Mon Sep 17 00:00:00 2001 From: Suriyakumarvijayanayagam Date: Fri, 4 Sep 2026 12:06:52 +0530 Subject: [PATCH] The schema applies itself, and the setup script stops hiding failures Migrations were run by hand and nothing recorded which had run, so re-running the setup script against an existing database failed on the first CREATE TABLE, and shipping a new migration gave an operator no way to know whether an estate had it. A missed migration is not a startup error - it is a query referencing a column that is not there, surfacing later on whichever endpoint touches it first. server/internal/migrate applies pending migrations at boot and refuses to start against a schema it does not match. One transaction per file holding both the DDL and the row that records it; an advisory lock so two servers starting at once cannot both apply 008; checksums so an edited migration is refused by name rather than silently skipped; numeric ordering so 010 does not run before 009. `migrate -baseline N` adopts a database built before any of this existed, because "the clients table exists" does not say whether 007's index does. Verified on the live database: adopted 001-007, applied 008. 008 adds two indexes on `purchases`, found by asking the database which foreign keys had nothing behind them and then checking what queries the table. The conversion report filters client_id + occurred_at, which is exactly the estate-wide case with no site to narrow it. run-local.sh had two bugs, both found by running it rather than reading it: it reused a broker container whose bind mount pointed at a directory that no longer existed, and it discarded stderr on the mosquitto_passwd call, so under `set -e` it exited at step 5 with no output at all. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HViLj9gYNRtSr7YVZmW5sn --- CLAUDE.md | 187 ++++++++++++++ RUN.md | 61 +++++ desktop/standalone_test.go | 95 +++++++ run-local.sh | 59 ++++- server/cmd/behavision-server/main.go | 32 +++ server/cmd/behavision-server/migrate.go | 107 ++++++++ server/internal/migrate/migrate.go | 285 +++++++++++++++++++++ server/internal/migrate/migrate_test.go | 73 ++++++ server/internal/store/store.go | 8 + server/migrations/008_purchase_indexes.sql | 27 ++ server/migrations/embed.go | 15 ++ 11 files changed, 941 insertions(+), 8 deletions(-) create mode 100644 desktop/standalone_test.go create mode 100644 server/cmd/behavision-server/migrate.go create mode 100644 server/internal/migrate/migrate.go create mode 100644 server/internal/migrate/migrate_test.go create mode 100644 server/migrations/008_purchase_indexes.sql create mode 100644 server/migrations/embed.go diff --git a/CLAUDE.md b/CLAUDE.md index e88fde5..ea446ef 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1984,3 +1984,190 @@ JPEG downloaded → erase → presigned GET **404**, 0 templates, 0 profiles, la setInput/forward; it runs once per track, so the lock costs nothing) and `Gallery`/`IdentityStore`. Locks around shared JPEG state; SQLite in WAL. - Tests are dependency-light (no camera or models needed) — keep it so. + +## A shop PC that runs on its own + +Until now every install was blocked on an enrolment code: the app opened on the +setup screen, and there was no way past it except a credential issued by a +server. That is wrong for the product it claims to be. Recognition, the +cameras, the tracker and the local gallery all run on the shop PC and need no +network at all — so a single-till shop with no head office was being refused +the thing the software is *for* until a component it does not need had blessed +it. + +`RunStandalone()` is the second answer on that screen. It is persisted +(`Config.Standalone`), because a choice that lives only in memory puts the +enrolment-code screen back in front of a shop that has already answered, which +reads as the app forgetting it was ever set up. + +- **"Not linked yet" and "not going to be linked" are different states**, and + the difference is load-bearing rather than cosmetic. An unclaimed PC keeps + its bridge running and queues every visit, deliberately: *"footfall from the + day it was installed is on disk waiting for credentials rather than lost."* + A standalone PC must not. Nothing is ever going to drain that queue, so it + would write up to `SpoolMax` visits — **each carrying a face template, which + is biometric personal data** — to disk for no purpose. `startPipeline` + returns early; the camera reconciler still runs, and reconciles against + nothing. +- **Cloud-only screens are hidden, not shown broken.** The customer record + lives on the server, so Customers disappears; Live and Cameras stay, because + they read the engine on loopback and always could. +- **Live says "Running on this PC only", not "Not linked to head office"** with + an idle dot beside a count of zero. The second is what a fault looks like. +- **Claiming later clears the flag and restarts the pipeline**, so a shop that + grows into a second branch loses nothing it recorded on its own. + +`Session()` and `PipelineStatus()` both report `standalone` as +`cfg.Standalone && !cfg.Configured()` — one expression, in two places that must +never disagree, for the same reason the tray is a client of `EngineStatus()` +rather than a second copy of it. + +## The camera make picker is now on both forms, from one file + +`shared/cameraMakes.js`, imported by the head-office web app **and** the shop +PC's app rather than copied into each. A make that is right in one and stale in +the other is worse than not offering the list at all, because an installer +trusts a filled-in field. + +It exists because the RTSP **path** is the one field nobody can look up: the +address is on a label and the password is in the installer's notes, but the +path is model-specific and undiscoverable, and getting it wrong produces +*"could not open stream"*, which reads like a password problem and is not. + +The desktop form also picked up the autofill defence the web form already had: +a text input next to a password input is a sign-in form as far as a webview is +concerned, so without `autoComplete="new-password"` on the secret and a +non-login `name` on the account, the browser offers the operator's own +Behavision email as the camera's username — which then fails with a message +about credentials that points at the camera. + +## The schema applies itself (`server/internal/migrate`) + +The migrations were run by hand — `psql < 001.sql`, in order, by whoever +remembered — and **nothing anywhere recorded which had run**. Three +consequences, all of which had already happened: + +- Re-running the setup script against an existing database failed on the first + `CREATE TABLE`, so it only ever worked once. Discovered by running it. +- Shipping a migration 008 gave an operator no way to know whether an estate + had it. A missed migration is not a startup error; it is a query referencing + a column that is not there, surfacing later on whichever endpoint touches it + first. +- An interrupted file left a schema nothing could describe. + +Now: `schema_migrations`, and the server applies pending migrations at boot. +Applying at boot rather than as a deploy step is deliberate — an upgrade of +this product is *"copy the new binary and restart it"*, and a migration +somebody has to remember to run is one that does not get run. Failure **stops +the server**: one running against a schema it does not match writes wrong data, +and wrong data outlives the outage that stopping causes. + +Decisions worth keeping: + +- **One transaction per file, holding both the DDL and the row that records + it.** A migration that ran but was not recorded runs again next start; one + recorded but not run leaves a missing column nothing will ever add. +- **An advisory lock held on one connection for the whole run.** Two servers + starting at once is the normal shape of a rolling restart, and both deciding + 008 is pending is not a hypothetical. +- **The content is checksummed.** An already-applied file that has since been + edited means the database does not contain what the repository says it does, + and running the new text now would apply half of it twice. It refuses and + names the file: the fix is a new migration, never an edited one. +- **Ordering is numeric, not alphabetical.** At ten migrations `010` sorts + before `009` as text, and the failure arrives on the day the project reaches + double figures. Two files claiming one version — the ordinary result of two + branches both adding `008` — is refused outright, because whichever ran first + would then be decided by the filesystem, which is not an order. +- **`-baseline N` adopts a database built before any of this existed**, marking + 1..N applied without running them. Guessing was not an option: *"the clients + table exists"* does not say whether 007's index does. An operator states it + once, and the row is marked `baselined` so an adopted database never looks + like one this code built. +- **The migrations are `go:embed`ed**, so the schema travels inside the binary + it belongs to. The consequence to know: a stale binary reports "schema up to + date" about migrations it has never heard of. Rebuild, then migrate. + +`behavision-server migrate [-status|-baseline N]` is the operator's view. +Verified on the live database: adopted 001–007, then applied 008 (two indexes +on `purchases`, found by asking the database which foreign keys had nothing +behind them and then checking what actually queries the table — the conversion +report filters `client_id` + `occurred_at`, which is precisely the estate-wide +case with no site to narrow it). + +## The Windows package (`installer/`) + +`installer/build.ps1` builds it and `installer/behavision.iss` lays it out. + +**The build script must run on Windows, and that is not a preference.** Every +other artefact here cross-compiles from a Mac — the Go binaries with +`GOOS=windows`, the front ends with npm, verified — but PyInstaller freezes the +interpreter and the native wheels (onnxruntime, OpenCV) of the machine it runs +on. There is no cross-target flag and there never has been. So the engine .exe +is built on Windows or it is not built. + +Installed layout, and why it is not flat: + +``` +C:\Program Files\Behavision\ + Behavision.exe the app: window, tray, engine supervisor + behavision-agent.exe the headless agent, for an install with no UI + engine\behavision.exe the engine, plus ~150 native DLLs beside it +C:\ProgramData\Behavision\ everything written: database, logs, models, cameras +``` + +The engine keeps its own folder because it is a one-**folder** PyInstaller +build that brings its DLLs with it — and because Windows filenames are +case-insensitive, so `Behavision.exe` and `behavision.exe` could not share a +directory even if it were tidy to. `config.Defaults().EngineExe` names +`engine\behavision.exe` and `tests/test_installer.py` asserts the two agree: +if they ever disagree the app starts, shows a healthy window, and recognises +nobody. + +- **Admin at install time, never at run time.** Program Files needs elevation; + spawning a child process does not. This is the same reason the product is not + a Windows service: a service runs in session 0 and cannot draw a tray icon. +- **Nothing writable under Program Files.** That split is `behavision/paths.py` + and `agent/pkg/paths`, and the installer must not contradict it — a seeded + writable file under `{app}` works for the administrator who installed it and + fails for the shop assistant who uses it. +- **Models are not bundled.** ~200 MB, downloaded resumably on first run; + bundling them quadruples the installer and forces a re-sign for a model + change. The wizard offers the download and a Start-menu shortcut repeats it, + because a shop PC being set up often has no working internet yet. +- **The WebView2 bootstrapper IS bundled.** Without the runtime the app opens + as an empty white rectangle — not an error, just nothing — which is the worst + failure to hand a shop. Present on Windows 11 and recent Windows 10, absent + on plenty of older machines, and a shop counter is exactly where an older + machine lives. +- **`CloseApplications=yes`.** Replacing the engine's DLLs while it holds the + SQLite WAL and the camera produces a half-upgraded install that fails on the + *next* start, long after anyone would connect the two events. + +`tests/test_installer.py` is the same guard `tests/test_paths.py` is for the +PyInstaller spec: it asserts the installer and the build script name the same +files, that nothing writable is placed under the install root, and that no +model is bundled. It cannot prove the package works on Windows — only a Windows +box can — but it catches the class of mistake that would otherwise get that +far. + +**Not yet done, and it needs a Windows machine:** no `wails build` has ever +run, no installer has been compiled, nothing is code-signed, and the frozen +engine has never been started. Unsigned, SmartScreen will warn on first launch. + +## `run-local.sh` was hiding its own failures + +Two bugs, both found by running it after a reboot rather than by reading it. + +- **A reused container keeps the bind mount it was created with.** `bv-mqtt` + had been created while the working directory was somewhere else, so it came + back up with an empty `/mosquitto/config`, died with *"Unable to open config + file"*, and every `docker exec` after that failed for a reason having nothing + to do with what it was asked. The script now compares the mount source and + recreates the container when it has moved. +- **`>/dev/null 2>&1 || true` on the `mosquitto_passwd` call.** Under `set -e` + the script then exited at step 5 with **no output at all** — the single + hardest failure to diagnose, and it took three runs to find. stderr is no + longer discarded, and the broker is waited for and reported on if it will not + stay up. A failure there means the server cannot authenticate to its own + broker, which is exactly what this script exists to surface early. diff --git a/RUN.md b/RUN.md index 89711b5..a4ae43c 100644 --- a/RUN.md +++ b/RUN.md @@ -215,3 +215,64 @@ cd ../agent && GOOS=windows CGO_ENABLED=0 go build -o behavision-agent.exe . These compile. They have never been RUN on Windows — no `wails build`, no installer, no code signing. + +--- + +## Building the Windows package + +`installer/build.ps1` produces `dist\Behavision-Setup-.exe`. + +**It has to run on Windows.** The Go binaries and the front ends cross-compile +from a Mac (verified), but PyInstaller freezes the interpreter and native +wheels of the machine it runs on — there is no cross-target flag. The engine +.exe is built on Windows or not at all. + +On the Windows box, with Python 3.11+, Go 1.21+, Node LTS and +[Inno Setup 6](https://jrsoftware.org/isdl.php) installed and on PATH: + +```powershell +git clone C:\src\Behavision +cd C:\src\Behavision +powershell -ExecutionPolicy Bypass -File installer\build.ps1 -Version 0.1.0 +``` + +It creates the venv, runs the engine test suite (a package is not worth +building if the engine is broken), freezes the engine, builds the app and the +agent, downloads the WebView2 bootstrapper, stages `dist\Behavision\`, starts +the frozen engine once to prove it runs, and compiles the installer. + +`-SkipInstaller` stops after staging, for testing without Inno Setup. + +### What to check on the Windows box + +1. Install as an administrator. Accept the model download. +2. `C:\Program Files\Behavision\engine\behavision.exe paths` — the state root + must be `C:\ProgramData\Behavision`, not anywhere under Program Files. +3. Launch from the Start menu. **Set this PC up on its own** — no code needed. +4. Cameras → Add camera → pick the make → Test connection → Save. The feed must + appear with no restart. +5. Check `/api/health` reports `recognition_model`. On a 16 GB machine the + 166 MB r50 can lose the fallback chain to the 13 MB mbf, and embeddings are + model-tagged, so which one wins decides whether a gallery carries over. +6. Sign out of the tray (Quit) — recognition must stop with it. Reboot; the app + must come back on its own. +7. Only then link it to head office, from the sidebar. + +Nothing is code-signed yet, so SmartScreen will warn on first launch. + +## Schema + +The server applies pending migrations when it starts and refuses to run against +a schema it does not match. Three ways to look at it by hand: + +```bash +export DATABASE_URL=... +./bv-server migrate -status # what is applied, adopted, pending or CHANGED +./bv-server migrate # apply everything pending +./bv-server migrate -baseline 7 # adopt a database built before tracking existed +``` + +The migrations are compiled into the binary, so **rebuild before migrating** — +a stale binary honestly reports "schema up to date" about files it has never +seen. Never edit an applied migration: the checksum check will refuse it by +name, and the fix is a new file. diff --git a/desktop/standalone_test.go b/desktop/standalone_test.go new file mode 100644 index 0000000..7d285f8 --- /dev/null +++ b/desktop/standalone_test.go @@ -0,0 +1,95 @@ +package main + +import ( + "testing" + + agentcfg "github.com/loyaly/behavision-agent/pkg/config" + agentpaths "github.com/loyaly/behavision-agent/pkg/paths" + + "github.com/loyaly/behavision-desktop/internal/cloud" +) + +// newTestApp builds an App around a config on disk, without startup(): the +// supervisor, the bridge and the broker all belong to a running PC and none of +// them is what these assertions are about. +func newTestApp(t *testing.T, cfg agentcfg.Config) *App { + t.Helper() + t.Setenv("BEHAVISION_DATA_DIR", t.TempDir()) + if err := cfg.Save(agentpaths.AgentConfig()); err != nil { + t.Fatalf("save config: %v", err) + } + return &App{cfg: cfg, cloud: cloud.New("https://example.invalid")} +} + +// A fresh install shows the setup screen. That is the state the whole +// standalone option exists to offer a way out of. +func TestAFreshPCIsNeitherClaimedNorStandalone(t *testing.T) { + a := newTestApp(t, agentcfg.Defaults()) + s := a.Session() + if s.Claimed || s.Standalone || s.LoggedIn { + t.Fatalf("a blank install reported %+v", s) + } +} + +// The choice has to reach disk. One that only lives in memory puts the +// enrolment-code screen back in front of a shop that already answered "we have +// no head office", which reads as the app forgetting it was ever set up. +func TestRunStandaloneIsRememberedAcrossARestart(t *testing.T) { + a := newTestApp(t, agentcfg.Defaults()) + s, err := a.RunStandalone() + if err != nil { + t.Fatalf("RunStandalone: %v", err) + } + if !s.Standalone { + t.Fatal("the session did not report standalone") + } + + // What the next launch sees. + back, err := agentcfg.Load(agentpaths.AgentConfig()) + if err != nil { + t.Fatalf("reload: %v", err) + } + if !back.Standalone { + t.Fatal("standalone was not persisted") + } + if (&App{cfg: back, cloud: cloud.New("https://example.invalid")}). + Session().Standalone != true { + t.Fatal("a reloaded standalone config did not report standalone") + } +} + +// Claimed wins. A PC that has been linked is not standalone whatever the flag +// says, or the head-office screens stay hidden on the one machine that just +// earned them. +func TestBeingClaimedOverridesTheStandaloneFlag(t *testing.T) { + cfg := agentcfg.Defaults() + cfg.Standalone = true + cfg.ClientID, cfg.SiteID, cfg.BrokerURL = "acme", "chennai", "tls://broker:8883" + a := newTestApp(t, cfg) + s := a.Session() + if !s.Claimed { + t.Fatal("a configured PC did not report itself claimed") + } + if s.Standalone { + t.Fatal("a claimed PC still reported standalone") + } + if a.PipelineStatus().Standalone { + t.Fatal("the pipeline status still reported standalone") + } +} + +// "Nothing is being sent because this PC is on its own" and "nothing is being +// sent and something is wrong" look identical from the counters alone, and only +// one of them is a fault. +func TestPipelineStatusSeparatesStandaloneFromUnclaimed(t *testing.T) { + unclaimed := newTestApp(t, agentcfg.Defaults()).PipelineStatus() + if unclaimed.Claimed || unclaimed.Standalone { + t.Fatalf("unclaimed PC reported %+v", unclaimed) + } + cfg := agentcfg.Defaults() + cfg.Standalone = true + alone := newTestApp(t, cfg).PipelineStatus() + if alone.Claimed || !alone.Standalone { + t.Fatalf("standalone PC reported %+v", alone) + } +} diff --git a/run-local.sh b/run-local.sh index 378b8d3..7e04e1b 100755 --- a/run-local.sh +++ b/run-local.sh @@ -21,16 +21,23 @@ mkdir -p "$STATE/mosquitto" step() { printf '\n\033[1m%s\033[0m\n' "$*"; } +# Checked up front rather than discovered in the middle of step 2, where the +# failure is a bare "go: command not found" after two minutes of npm install. +for tool in docker go npm; do + command -v "$tool" >/dev/null 2>&1 || { + printf 'need %s on PATH.\n' "$tool" >&2 + [ "$tool" = go ] && printf 'go is often installed outside the default PATH; try: export PATH="$HOME/go/bin:$PATH"\n' >&2 + exit 1 + } +done + step "1. Postgres (pgvector - migration 001 needs the extension)" docker inspect bv-pg >/dev/null 2>&1 || docker run -d --name bv-pg \ -p "${PG_PORT}:5432" -e POSTGRES_PASSWORD=test -e POSTGRES_DB=behavision \ pgvector/pgvector:pg16 >/dev/null docker start bv-pg >/dev/null 2>&1 || true until docker exec bv-pg pg_isready -U postgres >/dev/null 2>&1; do sleep 1; done -for f in server/migrations/*.sql; do - docker exec -i bv-pg psql -U postgres -d behavision -v ON_ERROR_STOP=1 -q < "$f" -done -echo " migrations applied" +echo " ready (the server applies the schema itself on start)" step "2. Build (the web app builds INTO the Go module, so it goes first)" (cd web && npm install --silent && npm run build >/dev/null) @@ -58,6 +65,12 @@ fi # shellcheck disable=SC1090 . "$STATE/env.sh" +step "3b. Schema" +# The server would do this itself on start, but provisioning below runs BEFORE +# it does and needs the tables to exist. One command either way, and it is the +# same code path the server uses. +"./$STATE/bv-server" migrate + step "4. Mosquitto" if [ ! -f "$STATE/mosquitto/mosquitto.conf" ]; then cat > "$STATE/mosquitto/mosquitto.conf" < "$STATE/mosquitto/acl" : > "$STATE/mosquitto/passwd" fi +# A container is reused only if its config mount still points HERE. The bind +# source is baked in when the container is created, so one made while the +# checkout lived somewhere else - or by a run from another directory - comes +# back up with an empty /mosquitto/config and dies with "Unable to open config +# file", which the old `|| true` below then hid completely. +MQTT_CONF="$PWD/$STATE/mosquitto" +if docker inspect bv-mqtt >/dev/null 2>&1; then + MOUNTED=$(docker inspect bv-mqtt \ + --format '{{range .Mounts}}{{if eq .Destination "/mosquitto/config"}}{{.Source}}{{end}}{{end}}') + if [ "$MOUNTED" != "$MQTT_CONF" ]; then + echo " recreating bv-mqtt (its config was mounted from ${MOUNTED:-nowhere})" + docker rm -f bv-mqtt >/dev/null + fi +fi docker inspect bv-mqtt >/dev/null 2>&1 || docker run -d --name bv-mqtt \ - -p "${MQTT_PORT}:1883" -v "$PWD/$STATE/mosquitto:/mosquitto/config" \ + -p "${MQTT_PORT}:1883" -v "$MQTT_CONF:/mosquitto/config" \ eclipse-mosquitto:2 >/dev/null docker start bv-mqtt >/dev/null 2>&1 || true -sleep 1 + +# Wait for it, and say so if it never arrives. `docker start` returning 0 only +# means the container was launched; mosquitto exits a moment later if it cannot +# read its config, and every `docker exec` after that fails for a reason that +# has nothing to do with what it was asked to do. +for _ in $(seq 1 20); do + docker exec bv-mqtt sh -c 'exit 0' >/dev/null 2>&1 && break + sleep 1 +done +if ! docker exec bv-mqtt sh -c 'exit 0' >/dev/null 2>&1; then + echo " broker will not stay up:" >&2 + docker logs --tail 5 bv-mqtt >&2 + exit 1 +fi +# stderr is NOT discarded here. A failure means the server cannot authenticate +# to its own broker, and the whole point of this script is that you find that +# out now rather than from an empty arrivals feed. docker exec bv-mqtt mosquitto_passwd -b /mosquitto/config/passwd \ - behavision-server "$MQTT_PASSWORD" >/dev/null 2>&1 || true + behavision-server "$MQTT_PASSWORD" >/dev/null docker restart bv-mqtt >/dev/null echo " broker on ${MQTT_PORT}" @@ -101,7 +144,7 @@ SITE_OUT=$("./$STATE/bv-server" provision site -client tenext-retail -slug chenn -name "TeNext Chennai" -tz Asia/Kolkata) BUSER=$(printf '%s' "$SITE_OUT" | sed -n "s/.*passwd \([^ ]*\) .*/\1/p") BPASS=$(printf '%s' "$SITE_OUT" | sed -n "s/.*passwd [^ ]* '\(.*\)'.*/\1/p") -docker exec bv-mqtt mosquitto_passwd -b /mosquitto/config/passwd "$BUSER" "$BPASS" >/dev/null 2>&1 +docker exec bv-mqtt mosquitto_passwd -b /mosquitto/config/passwd "$BUSER" "$BPASS" >/dev/null grep -q "^user $BUSER$" "$STATE/mosquitto/acl" || \ printf '\nuser %s\ntopic write bv/%s/#\n' "$BUSER" "$BUSER" >> "$STATE/mosquitto/acl" docker restart bv-mqtt >/dev/null diff --git a/server/cmd/behavision-server/main.go b/server/cmd/behavision-server/main.go index b7ccdd1..ee02f20 100644 --- a/server/cmd/behavision-server/main.go +++ b/server/cmd/behavision-server/main.go @@ -26,9 +26,11 @@ import ( "github.com/loyaly/behavision-server/internal/assistant" "github.com/loyaly/behavision-server/internal/blob" "github.com/loyaly/behavision-server/internal/ingest" + "github.com/loyaly/behavision-server/internal/migrate" "github.com/loyaly/behavision-server/internal/web" "github.com/loyaly/behavision-server/internal/secret" "github.com/loyaly/behavision-server/internal/store" + "github.com/loyaly/behavision-server/migrations" ) var version = "dev" @@ -44,6 +46,13 @@ func main() { } return } + if len(os.Args) > 1 && os.Args[1] == "migrate" { + if err := runMigrate(os.Args[2:]); err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } + return + } if err := run(); err != nil { log.Fatalf("behavision-server: %v", err) } @@ -79,6 +88,29 @@ func run() error { st.UseLogger(logger) logger.Print("database connected") + // Applied here, not by hand, because an upgrade of this product is "copy + // the new binary and restart it". A migration an operator has to remember + // to run is a migration that does not get run, and its failure is not a + // startup error - it is a query referencing a column that is not there, + // surfacing later on whichever endpoint touches it first. + // + // Refusing to start on failure is deliberate: a server running against a + // schema it does not match writes wrong data, and wrong data outlives the + // outage that stopping causes. + if os.Getenv("BEHAVISION_SKIP_MIGRATE") == "1" { + logger.Print("WARN BEHAVISION_SKIP_MIGRATE=1 - schema not checked") + } else { + applied, err := migrate.Apply(ctx, st.Pool(), migrations.FS) + if err != nil { + return fmt.Errorf("schema: %w", err) + } + if len(applied) == 0 { + logger.Print("schema up to date") + } else { + logger.Printf("schema: applied %s", strings.Join(applied, ", ")) + } + } + // Without the key the server still ingests events and serves reports; only // enrolment fails, and it fails with a message naming the missing variable. // Refusing to start would take a working estate down over a feature that diff --git a/server/cmd/behavision-server/migrate.go b/server/cmd/behavision-server/migrate.go new file mode 100644 index 0000000..714b569 --- /dev/null +++ b/server/cmd/behavision-server/migrate.go @@ -0,0 +1,107 @@ +package main + +import ( + "context" + "flag" + "fmt" + "os" + "strings" + + "github.com/loyaly/behavision-server/internal/migrate" + "github.com/loyaly/behavision-server/internal/store" + "github.com/loyaly/behavision-server/migrations" +) + +// runMigrate is the operator's view of the schema. +// +// The server applies pending migrations itself at boot, so this exists for the +// two things it cannot do: SAY what a database is on without changing it, and +// adopt a database whose schema predates the tracking table. +func runMigrate(args []string) error { + fs := flag.NewFlagSet("migrate", flag.ExitOnError) + status := fs.Bool("status", false, "print what this database has recorded and exit") + baseline := fs.Int("baseline", 0, + "record migrations up to this version as applied WITHOUT running them") + fs.Usage = func() { + fmt.Fprint(os.Stderr, `usage: behavision-server migrate [flags] + + no flags apply everything this database has not recorded + -status print the recorded versions and what is pending + -baseline N adopt a database built before migration tracking existed: + record 1..N as applied without running them. Use it once, on a + database whose schema you know already matches those files. + +DATABASE_URL is required. +`) + fs.PrintDefaults() + } + if err := fs.Parse(args); err != nil { + return err + } + + dsn := os.Getenv("DATABASE_URL") + if dsn == "" { + return fmt.Errorf("DATABASE_URL is required") + } + ctx := context.Background() + st, err := store.Open(ctx, dsn) + if err != nil { + return err + } + defer st.Close() + + files, err := migrate.Load(migrations.FS) + if err != nil { + return err + } + + if *baseline > 0 { + marked, err := migrate.Baseline(ctx, st.Pool(), migrations.FS, *baseline) + if err != nil { + return err + } + if len(marked) == 0 { + fmt.Println("nothing to adopt - every migration up to that version was already recorded") + return nil + } + fmt.Printf("adopted without running: %s\n", strings.Join(marked, ", ")) + fmt.Println("run `migrate` (no flags) to apply anything after that.") + return nil + } + + if *status { + done, err := migrate.Status(ctx, st.Pool()) + if err != nil { + return err + } + seen := map[int]migrate.Record{} + for _, r := range done { + seen[r.Version] = r + } + for _, f := range files { + r, ok := seen[f.Version] + switch { + case !ok: + fmt.Printf(" pending %s\n", f.Name) + case r.Baselined: + fmt.Printf(" adopted %s (recorded, never run here)\n", f.Name) + case r.Checksum != f.Checksum: + fmt.Printf(" CHANGED %s (the file differs from what was applied)\n", f.Name) + default: + fmt.Printf(" applied %s\n", f.Name) + } + } + return nil + } + + applied, err := migrate.Apply(ctx, st.Pool(), migrations.FS) + if err != nil { + return err + } + if len(applied) == 0 { + fmt.Println("schema up to date") + return nil + } + fmt.Printf("applied: %s\n", strings.Join(applied, ", ")) + return nil +} diff --git a/server/internal/migrate/migrate.go b/server/internal/migrate/migrate.go new file mode 100644 index 0000000..600bc18 --- /dev/null +++ b/server/internal/migrate/migrate.go @@ -0,0 +1,285 @@ +// Package migrate applies the schema and records what it applied. +// +// Until this existed the migrations were run by hand - `psql < 001.sql` for +// each file, in order, by whoever remembered - and nothing anywhere recorded +// which had run. Three consequences, all of which had already happened: +// +// - Re-running them against an existing database fails on the first CREATE +// TABLE, so the setup script only worked once. +// - Adding migration 008 to a release gave the operator no way to know +// whether a given estate had it. The failure of a missed migration is not +// a startup error; it is a query referencing a column that is not there, +// surfacing on whichever endpoint touches it first. +// - A half-applied migration - the file interrupted midway - left a schema +// nothing could describe. +// +// So: every file runs inside ONE transaction together with the row that +// records it. Either both happen or neither does, and a database can always +// say which version it is on. +package migrate + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "fmt" + "io/fs" + "sort" + "strconv" + "strings" + + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgxpool" +) + +// lockID namespaces the advisory lock this package takes. Two servers starting +// at the same moment - which is the normal shape of a rolling restart - must +// not both decide migration 008 is pending and both run it. +const lockID int64 = 7623094512340001 + +// File is one migration on disk. +type File struct { + Version int + Name string + SQL string + Checksum string +} + +// Record is one migration as the database remembers it. +type Record struct { + Version int + Name string + Checksum string + Baselined bool +} + +// Load reads and orders the migrations, rejecting anything it cannot place. +func Load(src fs.FS) ([]File, error) { + entries, err := fs.Glob(src, "*.sql") + if err != nil { + return nil, err + } + seen := map[int]string{} + out := make([]File, 0, len(entries)) + for _, name := range entries { + version, err := versionOf(name) + if err != nil { + return nil, err + } + if other, dup := seen[version]; dup { + // Two files claiming one version means the order they run in is + // whatever the filesystem felt like, which is not an order. + return nil, fmt.Errorf("migrations %s and %s share version %d", + other, name, version) + } + seen[version] = name + body, err := fs.ReadFile(src, name) + if err != nil { + return nil, err + } + sum := sha256.Sum256(body) + out = append(out, File{ + Version: version, + Name: name, + SQL: string(body), + Checksum: hex.EncodeToString(sum[:]), + }) + } + sort.Slice(out, func(i, j int) bool { return out[i].Version < out[j].Version }) + return out, nil +} + +func versionOf(name string) (int, error) { + prefix, _, ok := strings.Cut(name, "_") + if !ok { + return 0, fmt.Errorf("migration %q is not named NNN_description.sql", name) + } + n, err := strconv.Atoi(prefix) + if err != nil || n <= 0 { + return 0, fmt.Errorf("migration %q does not start with a version number", name) + } + return n, nil +} + +// Apply runs every migration the database has not recorded, in order. +// +// Returns the names it applied, so a caller can log "nothing to do" rather +// than staying silent about work it did not need to do. +func Apply(ctx context.Context, pool *pgxpool.Pool, src fs.FS) ([]string, error) { + files, err := Load(src) + if err != nil { + return nil, err + } + + conn, err := pool.Acquire(ctx) + if err != nil { + return nil, err + } + defer conn.Release() + + // Taken on this one connection and held for the whole run: a second server + // booting waits here rather than racing us, and the lock is released with + // the connection even if this process is killed. + if _, err := conn.Exec(ctx, `SELECT pg_advisory_lock($1)`, lockID); err != nil { + return nil, fmt.Errorf("migration lock: %w", err) + } + defer func() { + _, _ = conn.Exec(context.WithoutCancel(ctx), + `SELECT pg_advisory_unlock($1)`, lockID) + }() + + if err := ensureTable(ctx, conn.Conn()); err != nil { + return nil, err + } + done, err := recorded(ctx, conn.Conn()) + if err != nil { + return nil, err + } + + var applied []string + for _, f := range files { + prev, ok := done[f.Version] + if ok { + // An already-applied file that has since been edited. The database + // does NOT contain what the repository says it does, and running + // the new text now would apply half of it twice. Refuse and say so: + // the fix is a new migration, never an edited one. + if prev.Checksum != f.Checksum && !prev.Baselined { + return applied, fmt.Errorf( + "migration %s has changed since it was applied "+ + "(recorded %s, file %s) - add a new migration instead of editing one", + f.Name, short(prev.Checksum), short(f.Checksum)) + } + continue + } + if err := applyOne(ctx, conn.Conn(), f); err != nil { + return applied, err + } + applied = append(applied, f.Name) + } + return applied, nil +} + +// applyOne runs one file and records it in the SAME transaction. A migration +// that ran but was not recorded would run again on the next start; one that was +// recorded but did not run leaves a schema missing a column nothing will ever +// add. +func applyOne(ctx context.Context, conn *pgx.Conn, f File) error { + tx, err := conn.Begin(ctx) + if err != nil { + return err + } + defer func() { _ = tx.Rollback(context.WithoutCancel(ctx)) }() + + if _, err := tx.Exec(ctx, f.SQL); err != nil { + return fmt.Errorf("migration %s: %w", f.Name, err) + } + if _, err := tx.Exec(ctx, + `INSERT INTO schema_migrations (version, name, checksum) VALUES ($1, $2, $3)`, + f.Version, f.Name, f.Checksum); err != nil { + return fmt.Errorf("recording migration %s: %w", f.Name, err) + } + return tx.Commit(ctx) +} + +// Baseline records migrations up to and including `through` as applied WITHOUT +// running them. +// +// For the one case that cannot be handled automatically: a database built +// before this package existed, whose schema is already there and whose history +// is not. Guessing is not an option - "the clients table exists" does not say +// whether migration 007's index does - so an operator states it, once, and it +// is recorded as a baseline rather than as a real application. +func Baseline(ctx context.Context, pool *pgxpool.Pool, src fs.FS, through int) ([]string, error) { + files, err := Load(src) + if err != nil { + return nil, err + } + conn, err := pool.Acquire(ctx) + if err != nil { + return nil, err + } + defer conn.Release() + if err := ensureTable(ctx, conn.Conn()); err != nil { + return nil, err + } + var marked []string + for _, f := range files { + if f.Version > through { + continue + } + tag, err := conn.Exec(ctx, + `INSERT INTO schema_migrations (version, name, checksum, baselined) + VALUES ($1, $2, $3, true) ON CONFLICT (version) DO NOTHING`, + f.Version, f.Name, f.Checksum) + if err != nil { + return marked, err + } + if tag.RowsAffected() == 1 { + marked = append(marked, f.Name) + } + } + return marked, nil +} + +// Status reports what the database has recorded, oldest first. +func Status(ctx context.Context, pool *pgxpool.Pool) ([]Record, error) { + conn, err := pool.Acquire(ctx) + if err != nil { + return nil, err + } + defer conn.Release() + if err := ensureTable(ctx, conn.Conn()); err != nil { + return nil, err + } + done, err := recorded(ctx, conn.Conn()) + if err != nil { + return nil, err + } + out := make([]Record, 0, len(done)) + for _, r := range done { + out = append(out, r) + } + sort.Slice(out, func(i, j int) bool { return out[i].Version < out[j].Version }) + return out, nil +} + +func ensureTable(ctx context.Context, conn *pgx.Conn) error { + _, err := conn.Exec(ctx, ` + CREATE TABLE IF NOT EXISTS schema_migrations ( + version integer PRIMARY KEY, + name text NOT NULL, + checksum text NOT NULL, + applied_at timestamptz NOT NULL DEFAULT now(), + -- true when the row records a migration that was NOT run here, + -- because the schema predates this table. Kept distinct so an + -- adopted database never looks like one this code built. + baselined boolean NOT NULL DEFAULT false + )`) + return err +} + +func recorded(ctx context.Context, conn *pgx.Conn) (map[int]Record, error) { + rows, err := conn.Query(ctx, + `SELECT version, name, checksum, baselined FROM schema_migrations`) + if err != nil { + return nil, err + } + defer rows.Close() + out := map[int]Record{} + for rows.Next() { + var r Record + if err := rows.Scan(&r.Version, &r.Name, &r.Checksum, &r.Baselined); err != nil { + return nil, err + } + out[r.Version] = r + } + return out, rows.Err() +} + +func short(sum string) string { + if len(sum) > 12 { + return sum[:12] + } + return sum +} diff --git a/server/internal/migrate/migrate_test.go b/server/internal/migrate/migrate_test.go new file mode 100644 index 0000000..5af75bc --- /dev/null +++ b/server/internal/migrate/migrate_test.go @@ -0,0 +1,73 @@ +package migrate + +import ( + "strings" + "testing" + "testing/fstest" +) + +func TestMigrationsRunInNumericOrderNotAlphabetical(t *testing.T) { + // The bug this prevents: at ten migrations, "10_x.sql" sorts before + // "9_x.sql" as text, so the tenth would run before the ninth and the + // failure would be a column that does not exist yet - on the day the + // project happens to reach double figures. + src := fstest.MapFS{ + "002_b.sql": {Data: []byte("select 2")}, + "010_j.sql": {Data: []byte("select 10")}, + "009_i.sql": {Data: []byte("select 9")}, + "001_a.sql": {Data: []byte("select 1")}, + } + files, err := Load(src) + if err != nil { + t.Fatalf("load: %v", err) + } + got := make([]int, len(files)) + for i, f := range files { + got[i] = f.Version + } + want := []int{1, 2, 9, 10} + for i := range want { + if got[i] != want[i] { + t.Fatalf("order was %v, want %v", got, want) + } + } +} + +func TestTwoFilesCannotShareAVersion(t *testing.T) { + // Two people adding "008" on separate branches is the ordinary way this + // happens. Whichever runs first is then decided by the filesystem, which is + // not an order, and one of them silently never runs at all. + _, err := Load(fstest.MapFS{ + "008_one.sql": {Data: []byte("select 1")}, + "008_two.sql": {Data: []byte("select 2")}, + }) + if err == nil { + t.Fatal("duplicate versions were accepted") + } + if !strings.Contains(err.Error(), "share version 8") { + t.Fatalf("error does not name the collision: %v", err) + } +} + +func TestAFileThatIsNotNumberedIsRejected(t *testing.T) { + for _, name := range []string{"schema.sql", "abc_x.sql", "0_zero.sql"} { + if _, err := Load(fstest.MapFS{name: {Data: []byte("select 1")}}); err == nil { + t.Fatalf("%s was accepted as a migration", name) + } + } +} + +func TestTheChecksumChangesWithTheContent(t *testing.T) { + // What makes an edited-after-applying migration detectable at all. + a, err := Load(fstest.MapFS{"001_a.sql": {Data: []byte("select 1")}}) + if err != nil { + t.Fatal(err) + } + b, err := Load(fstest.MapFS{"001_a.sql": {Data: []byte("select 2")}}) + if err != nil { + t.Fatal(err) + } + if a[0].Checksum == b[0].Checksum { + t.Fatal("two different migrations hashed the same") + } +} diff --git a/server/internal/store/store.go b/server/internal/store/store.go index a8b6dc0..83afdfc 100644 --- a/server/internal/store/store.go +++ b/server/internal/store/store.go @@ -63,6 +63,14 @@ func Open(ctx context.Context, dsn string) (*Store, error) { func (s *Store) Close() { s.pool.Close() } +// Pool exposes the connection pool for the schema migrator. +// +// Deliberately narrow in intent: the migrator has to run arbitrary DDL and +// take an advisory lock, neither of which belongs behind a typed store method. +// Nothing else should reach for this - a query that lives out here is a query +// nothing tenant-scopes. +func (s *Store) Pool() *pgxpool.Pool { return s.pool } + func (s *Store) Ping(ctx context.Context) error { return s.pool.Ping(ctx) } // ResolveSite maps an authenticated MQTT username to a provisioned tenant. diff --git a/server/migrations/008_purchase_indexes.sql b/server/migrations/008_purchase_indexes.sql new file mode 100644 index 0000000..9d3ba83 --- /dev/null +++ b/server/migrations/008_purchase_indexes.sql @@ -0,0 +1,27 @@ +-- Two indexes on `purchases`, both for queries that already exist. +-- +-- Found by asking the database which foreign keys had no index behind them and +-- then checking what actually queries the table, rather than by adding indexes +-- on principle: every one of them costs a write on the path that records a +-- sale. +-- +-- 1. The conversion report filters `client_id` + `occurred_at`, with the site +-- optional - an owner comparing shops is the whole reason that report +-- exists, and that is precisely the case with no site to narrow it. The +-- existing purchases_site_time_idx cannot serve it. Today the table has a +-- handful of rows and a sequential scan is free; purchases is the table +-- that grows with a shop's trade, so this is the one that stops being free. +-- +-- 2. purchases.visit_id is a foreign key with nothing behind it. Every delete +-- of a visit has to prove no purchase references it, which without an index +-- is a full scan per row - and erasing a customer deletes their visits. + +BEGIN; + +CREATE INDEX IF NOT EXISTS purchases_client_time_idx + ON purchases (client_id, occurred_at DESC); + +CREATE INDEX IF NOT EXISTS purchases_visit_idx + ON purchases (visit_id); + +COMMIT; diff --git a/server/migrations/embed.go b/server/migrations/embed.go new file mode 100644 index 0000000..0f74a08 --- /dev/null +++ b/server/migrations/embed.go @@ -0,0 +1,15 @@ +// Package migrations carries the schema files themselves. +// +// It exists only so `go:embed` can reach them: embed cannot see outside its own +// package directory, and moving the .sql files into some internal/ folder would +// break every path that documents them - RUN.md, run-local.sh, the test that +// tells you how to build a database by hand. +package migrations + +import "embed" + +// FS holds every migration, named NNN_description.sql. The number is the +// version and must be unique; the rest is for humans. +// +//go:embed *.sql +var FS embed.FS