From 18b133a195b3c78de727082593b1acc00fe6ad93 Mon Sep 17 00:00:00 2001 From: GGOBP Date: Wed, 12 Aug 2026 11:32:35 +0900 Subject: [PATCH] fix: bootstrap PostgreSQL without a service manager (closes #823) Fall back to a user-owned initdb/pg_ctl cluster under ~/.mxcli/postgres when no service-managed PostgreSQL becomes ready. Provision through its private Unix socket without requiring a postgres OS account or sudo, while retaining a guarded system-cluster path. Harden the fallback across all review rounds: bound service probes, remove the invalid pg_ctlcluster placeholder, avoid competing with an occupied port, use SCRAM on TCP, enforce 0700 socket access, persist the endpoint atomically, reject legacy host-trust clusters, and validate running-cluster endpoints. Keep sudo/psql non-interactive on the peer-authenticated system socket at the requested port, propagate the canonical endpoint to the runtime, accept bracketed IPv6, force SCRAM password storage on older defaults, and keep password-bearing SQL out of process arguments. Add focused regressions and update the run-local documentation, changelog, and fix-issue record. --- .claude/skills/fix-issue.md | 1 + .claude/skills/mendix/run-local/SKILL.md | 12 +- CHANGELOG.md | 1 + cmd/mxcli/cmd_run.go | 2 +- cmd/mxcli/docker/ensuredb.go | 537 ++++++++++++++-- cmd/mxcli/docker/ensuredb_test.go | 753 ++++++++++++++++++++++- cmd/mxcli/docker/localapp.go | 2 +- cmd/mxcli/docker/runlocal.go | 2 +- docs-site/src/tools/run-local.md | 14 +- 9 files changed, 1261 insertions(+), 63 deletions(-) diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index ab9329b33..3ec306866 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -636,3 +636,4 @@ extracting `OffsetExpression`/`LimitExpression`. | Someone translates the whole app into a language with `create translations`, every run reports success, `mx check` is clean — and the app shows not one translated string | The project had not ENABLED that language. Enabled languages (`Settings$LanguageSettings.Languages`) decide what the build emits; a translation for any other language is stored in the model, survives every check, and is discarded at build time. Measured on mxbuild 11.13.0: a caption translated into de_DE on a blank app (which enables en_US only) builds at 0 errors and the German string appears NOWHERE under `deployment/` — no `translations_de_DE.properties`, and "de_DE" does not occur in the built model at all; the English control appears in three files. A stock app invites the mistake: it enables ONE language while its marketplace modules ship translations in NINE, so "other languages already have translations here" is true and misleading | `mdl/executor/language_enablement.go` (`languageIsEnabled`, `unenabledLanguageWarning`), wired into `mdl/executor/cmd_translations.go` and surfaced by `mdl/executor/cmd_settings.go` (both `show` and `describe`) | Warn, do not refuse: Mendix tolerates the state at **both** layers — measured live over PED, Studio Pro loads the unenabled language's translation and keeps it beside the enabled one (`{"languageCode":"sv_SE","text":"Etikett"}` next to the en_US entry on a caption in an en_US-only project), reporting no error; only the BUILD drops it — and refusing would break the legitimate order of "translate now, enable when the review lands". **Unknown must not read as disabled** — with no settings, or a settings part carrying no `Languages`, a warning would be a guess, and a false warning about a language that IS enabled trains people to ignore the true one. DESCRIBE lists the enabled languages as a **comment**, because MDL cannot author the list (`alter settings LANGUAGE` carries `DefaultLanguageCode` only) and a statement that does not re-execute is worse than a comment that does not. The measurement that settles any doubt is `mxbuild --target=deploy` plus a same-page control string in the default language — `mx check` says nothing either way. Repro `mdl-examples/bug-tests/translations-unenabled-language.mdl` | | **CE0463 on every page carrying a File Uploader**, whatever is on it — an action-free one with nothing but its `datasource:` fails identically. The widget the #956 action-slot work was *about*, and the one it never exercised | **Two independent causes, and each alone still fails.** (A) The widget XML's `defaultType` — the KIND of a default, `defaultType="CallNanoflow"` on the create* actions, `defaultType="Association"` on the datasources — was never parsed, so all six landed as `DefaultType: "None"`. It is part of the widget DEFINITION, exactly like the stale `onChange` of #716. (B) `uploadMode`'s declared default never reached the visibility evaluator, because a primitive mapping carries the XML default in `PropertyMapping.Value` while `widgetValueMap` read only `.Default` (populated for selections alone). With the condition unknown, `hiddenUnnamedProperties` correctly refuses to guess — so `associatedImages` was not pruned, the one `datasource:` clause fanned out into BOTH datasource properties, and the pruned one carried a value. File Uploader is the only widget in a stock 11.13.0 app that declares any `defaultType` (6 properties; every other bundled widget declares 0), which is why one widget was unwritable while the rest were fine | (A) `modelsdk/widgets/mpk/mpk.go` + `sdk/widgets/mpk/mpk.go` (`PropertyDef.DefaultType`, `xml:"defaultType,attr"`), `modelsdk/widgets/augment.go` + `sdk/widgets/augment.go` (`defaultTypeOf`); (B) `mdl/executor/validate_widgets.go` (`widgetValueMap`) | **`mx update-widgets` on a COPY is the oracle when there is no Studio Pro** — and when one IS reachable, `ped_check_errors` over the written documents is the real verdict, with the same page written by the pre-fix binary as the control (it reports the CE0463 text and a `/layoutCall/arguments/0/widgets/0/widgets/0` location; the fixed 17 documents report "No errors found."). `pg_read_page` then shows the editor's own view — on the pre-fix page it lists BOTH `associatedFiles` and `associatedImages` under `uploadMode: "files"`, which is the fanout, and on the fixed one only the active half. It is Studio Pro's own normalizer: run it, confirm it clears the error, then diff its output against yours — a 2,650-path document diff whose *only* differences are the cause. Diff by property NAME, never by index: the two dumps order top-level keys differently and a positional flatten reports ~60 spurious differences that hide the 6 real ones. Widget properties are keyed by `TypePointer` into `Type.ObjectType.PropertyTypes`, so resolve the pointer to its `PropertyKey` first or the diff is unreadable. Two more findings from the same measurement, both still open: mxcli writes widget-action `ParameterMappings` with list marker **3** where Studio Pro writes **2** (and omits the sibling `Variable`/`OutputMappings` keys) — accepted by both mxbuild 11.13 **and Studio Pro**, which resolves every mapping to the right parameter (checked live over PED), so it is a diff-churn item rather than a correctness one; and the visibility extractor misses a `hidePropertiesIn` call inside a **ternary branch** (`cond ? hide([…]) : hide([…])`), as opposed to the ternary *array element* #233 fixed — so `createImageAction` carries no uploadMode rule, MDL-WIDGET10 cannot warn, and wiring it in files mode is a CE0463 the script author gets no help with. Repro `mdl-examples/bug-tests/956-fileuploader-six-action-slots.mdl`. Issue #956 | | Nothing visibly wrong: `create translations` writes a translation that builds at 0 errors and describes back correctly, but the `Texts$Translation` elements it minted carry `$ID`s of a shape Studio Pro never produces | `mdl/translations` minted its own id — 16 raw bytes from `crypto/rand` — instead of going through `types.GenerateID`/`types.UUIDToBlob` like every other element mxcli writes. Mendix stores an element id as a **.NET Guid**: mixed-endian, so the RFC-4122 version nibble lands at byte **7** and the variant bits at byte **8**. Raw bytes satisfy that 1 time in 64. Measured on a stock 11.13.0 app: **44,002 of 44,002** element ids are well-formed on that reading (version 4 or 5, variant `10b`), including all **1,650** translation ids the marketplace modules ship; after `mxcli exec translations.mdl`, **1 of 27** written ids was. `mx check` is silent — a Guid parses any 16 bytes — and so is DESCRIBE, because the id is never shown | `mdl/translations/text.go` (`newElementID`), against `mdl/types/id.go` (`GenerateIDErr`, `UUIDToBlob` — which is where the byte swap lives) | Mint through `types.GenerateIDErr` + `types.UUIDToBlob`. Assert the **form**, not the presence: draw ~200 ids and require version nibble 4 at byte 7 and variant `10b` at byte 8, so a raw-random implementation cannot pass by luck (`TestNewElementID_IsAUUIDInMendixByteOrder`). Note the two measurement traps: decoding with `bson.M` (or any Go map) **loses key order**, so "is `$ID` first" cannot be asked that way — use `bson.D`/`bson.Raw`, or Python, whose dicts preserve insertion order; and read the version nibble at byte **7**, not 6 — at byte 6 Mendix's own ids look like uniform noise, which reads as "Mendix does not care" and inverts the conclusion. The paired control is the whole project: 100% conforming before the run and after the fix, 3.7% for the new ids in between. Repro `mdl-examples/bug-tests/translation-id-form.mdl` | +| `mxcli run --ensure-db` fails to start PostgreSQL when no service manager becomes ready (e.g. Arch): `exec: "pg_ctlcluster": executable file not found in $PATH`, though `initdb`/`pg_ctl`/`psql` are present | `startLocalPostgres` only knew the `service`/`pg_ctlcluster` helpers (the latter a placeholder that could never run), and role/database provisioning assumed `sudo -u postgres`, which a user-owned cluster does not need | `cmd/mxcli/docker/ensuredb.go` (`startLocalPostgres`, `startUserCluster`, `resolveSuperuser`) | Fall back to a user-owned `initdb`/`pg_ctl` cluster under `~/.mxcli/postgres`, but never initialize a competitor when the requested TCP port is already owned by a service-started server that is still recovering. Enforce the passwordless-superuser boundary instead of assuming it: explicitly tighten an existing socket directory to `0700`, set the socket itself to `0700`, and persist `listen_addresses`/`port`/`unix_socket_directories` in `postgresql.conf` so a plain later `pg_ctl start` stays safe. Refuse, with a cleanup path, a cluster made by the earlier development revision if its host authentication is still `trust`. Reuse a running cluster only when both the port and socket directory in `postmaster.pid` match. Keep `sudo -u postgres` for system clusters, but make both sudo and psql non-interactive, retain peer authentication over the system Unix socket at the exact requested port, and force SCRAM password storage for pre-14 defaults. Normalize the endpoint once and return it to the runtime; parse the bracketed form before any last-colon IPv6 compatibility fallback. Tests stub every external tool and exercise slow service startup, legacy-cluster refusal, permissions, persisted settings, endpoint propagation, sudo arguments, and repeated/running cases. Issue #823 | diff --git a/.claude/skills/mendix/run-local/SKILL.md b/.claude/skills/mendix/run-local/SKILL.md index f2b91b3e0..53212da11 100644 --- a/.claude/skills/mendix/run-local/SKILL.md +++ b/.claude/skills/mendix/run-local/SKILL.md @@ -55,8 +55,14 @@ association catalog only at startup; behavioural changes are hot-reloaded. - A **PostgreSQL** database (defaults: `127.0.0.1:5432`, user `mendix`, db derived from the project name; override with `--db-host/--db-name/--db-user/--db-password`). - **`--ensure-db`** provisions it for a fresh session: starts local Postgres if the - port is down and creates the role + database if missing (local superuser via - `sudo -u postgres`). Remote hosts are only checked, not provisioned. + port is down and creates the role + database if missing. It uses a service + manager, or a user-owned `initdb`/`pg_ctl` cluster under `~/.mxcli/postgres` + when no service becomes ready (e.g. Arch) — needing no `postgres` OS account or `sudo`. + Remote hosts are only checked, not provisioned. + The user-owned cluster persists across sessions; its server log is + `~/.mxcli/postgres/server.log`. Stop it with + `pg_ctl -D "$HOME/.mxcli/postgres/data" stop`. To remove it, stop it first and + then delete `~/.mxcli/postgres` (this permanently deletes its databases). - Without `--ensure-db`, create it once and the command errors if it's unreachable: ```bash @@ -273,7 +279,7 @@ export OTEL_TRACES_EXPORTER=otlp OTEL_EXPORTER_OTLP_ENDPOINT=http://localhost:43 mxcli run --local -p app.mpr --trace ``` | `--app-port` / `--admin-port` / `--serve-port` | 8080 / 8090 / 6543 | Ports | -| `--db-host` / `--db-name` / `--db-user` / `--db-password` | 127.0.0.1:5432 / derived / mendix / mendix | Database | +| `--db-host` / `--db-name` / `--db-user` / `--db-password` | 127.0.0.1:5432 / derived / mendix / mendix | Database; bracket IPv6 endpoints (`[::1]:5432`) | ## Pages render in the browser diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d423359f..75b5a71c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). The cause was the fix for the opposite report. #884 was a describe→exec round-trip *moving* a hand-placed start (`145;200` came back as `100;200`), fixed by carrying the stored position over on every rewrite — which then pinned the start of every rewritten flow. Both reports are real, and neither is answerable without asking where the stored value came from. A start sitting at the derived spot is mxcli's own arithmetic handed back, carries no intent, and is now re-derived so it follows the activities; a start anywhere else was placed by a person and still survives. `@start(x, y)` states the position outright and beats both, which is the other half of what #951 reported — before it there was no way to move a start once one had been preserved. Neither `mx check` nor a successful build detects this, in either direction: the Mendix model carries no geometry rules, so a stranded start is a valid document that builds and runs and is merely drawn wrong — 0 errors on mxbuild 11.13.0 before and after. +- **`mxcli run --ensure-db` can start PostgreSQL without a working service manager (#823)** — on hosts that ship neither `service` nor Debian's `pg_ctlcluster` (e.g. Arch Linux), `--ensure-db` failed with `exec: "pg_ctlcluster": executable file not found in $PATH` even though the portable `initdb`/`pg_ctl`/`psql` tools were present. `startLocalPostgres` now falls back to a user-owned cluster under `~/.mxcli/postgres` when no service becomes ready, but never starts a competitor while another process owns the requested port. The cluster is idempotent and needs neither a `postgres` OS account nor passwordless `sudo`: its listen address, port, private socket directory, and `0700` socket permissions persist in PostgreSQL's own configuration; loopback TCP uses SCRAM while role/database provisioning uses local trust only through that private socket. Anyone who ran an earlier development revision of this fix must stop PostgreSQL, remove `~/.mxcli/postgres`, and rerun `--ensure-db`; reuse detects and refuses its insecure host-trust records. The retained system-cluster path is non-interactive, keeps `sudo -u postgres` on the peer-authenticated system Unix socket at the requested port, and creates a SCRAM password even on PostgreSQL versions whose default is MD5. Password-bearing SQL is sent over standard input instead of exposed in process arguments. The canonical endpoint—including bracketed IPv6—is also the one handed to the Mendix runtime. - **A pluggable widget's click/change action survives a describe→exec round-trip** (#956) — the action landed in the `.mxunit` and built at 0 errors, then `describe page` omitted it, so re-executing mxcli's own output deleted the wiring. A widget's storage key is not MDL's name for the slot: Mendix's own widgets suffix theirs (BadgeButton `onClickEvent`, HeatMap `onClickAction`, Combobox `onChangeEvent`) and the writer has stripped that suffix since ledger #14, while the reader looked up the literal string `"onClick"`. It now resolves the stored key through the writer's own `actionSourceForKey`, so the two cannot drift apart again, and reads the change slot as well as the click slot — the generic pluggable path had no `OnChange` read at all, which silently dropped a Slider/RangeSlider/StarRating's only action. diff --git a/cmd/mxcli/cmd_run.go b/cmd/mxcli/cmd_run.go index 7c4ee80aa..fc48e4798 100644 --- a/cmd/mxcli/cmd_run.go +++ b/cmd/mxcli/cmd_run.go @@ -309,7 +309,7 @@ func init() { runCmd.Flags().Int("app-port", 0, "HTTP port for the app (default 8080)") runCmd.Flags().Int("admin-port", 0, "M2EE admin API port (default 8090)") runCmd.Flags().Int("serve-port", 0, "mxbuild --serve port (default 6543)") - runCmd.Flags().String("db-host", "", "Database host:port (default 127.0.0.1:5432)") + runCmd.Flags().String("db-host", "", "Database host:port (IPv6: [::1]:5432; default 127.0.0.1:5432)") runCmd.Flags().String("db-name", "", "Database name (default derived from the project name)") runCmd.Flags().String("db-user", "", "Database user (default mendix)") runCmd.Flags().String("db-password", "", "Database password (default mendix)") diff --git a/cmd/mxcli/docker/ensuredb.go b/cmd/mxcli/docker/ensuredb.go index 6869bfa34..e4230f40f 100644 --- a/cmd/mxcli/docker/ensuredb.go +++ b/cmd/mxcli/docker/ensuredb.go @@ -5,34 +5,92 @@ package docker import ( "fmt" "io" + "net" "os" "os/exec" + "path/filepath" "regexp" + "strconv" "strings" "time" ) // ensuredb.go provisions the local PostgreSQL a standalone runtime needs, so a // fresh session comes up testable without a manual createdb (slice 2 of the -// warm-loop proposal). It is best-effort and devcontainer-shaped: it starts the -// local Postgres service if the port is down, then ensures the app role and -// database exist via a superuser (`sudo -u postgres psql`). For a non-local DB -// host it does nothing but verify reachability — provisioning a remote database -// is not mxcli's business. +// warm-loop proposal). It is best-effort: it starts the local Postgres service +// if the port is down — via a service manager, or a user-owned initdb/pg_ctl +// cluster when no service becomes ready (e.g. Arch, #823) — then ensures the app +// role and database exist via a superuser (a trust connection over the +// user-owned cluster's private socket, or `sudo -u postgres psql` for a system +// cluster). For a non-local DB host it does nothing but verify reachability — +// provisioning a remote database is not mxcli's business. // pgIdent is a conservative PostgreSQL identifier (unquoted): a safe database or // role name. We refuse anything else rather than quote/escape it into DDL. var pgIdent = regexp.MustCompile(`^[a-z_][a-z0-9_]*$`) -// splitHostPort splits "host:port" into host and port, defaulting the port to -// 5432 when absent. +// serviceReadyTimeout bounds how long we probe for a service-manager-started +// server before falling back to a user-owned cluster. It is deliberately short: +// the single authoritative readiness wait lives in EnsureDatabase, so a dead or +// slow attempt must not multiply the overall deadline (#823 review). A variable +// so tests can shrink it further. +var serviceReadyTimeout = 3 * time.Second + +// splitHostPort splits a PostgreSQL endpoint into host and port, defaulting the +// port to 5432 when absent. net.SplitHostPort handles the canonical bracketed +// IPv6 form; the compatibility branch keeps accepting the historical +// unbracketed "::1:5432" spelling used by this CLI. func splitHostPort(hostPort string) (host, port string) { + if host, port, err := net.SplitHostPort(hostPort); err == nil { + return host, port + } + if strings.HasPrefix(hostPort, "[") && strings.HasSuffix(hostPort, "]") { + return strings.TrimSuffix(strings.TrimPrefix(hostPort, "["), "]"), "5432" + } + if strings.Count(hostPort, ":") > 1 { + if i := strings.LastIndexByte(hostPort, ':'); i >= 0 { + candidateHost, candidatePort := hostPort[:i], hostPort[i+1:] + if net.ParseIP(candidateHost) != nil { + if _, err := normalizePostgresPort(candidatePort); err == nil { + return candidateHost, candidatePort + } + } + } + if net.ParseIP(hostPort) != nil { + return hostPort, "5432" + } + } if i := strings.LastIndexByte(hostPort, ':'); i >= 0 { return hostPort[:i], hostPort[i+1:] } return hostPort, "5432" } +// normalizePostgresPort validates a user-supplied PostgreSQL port and returns +// its canonical decimal form, so an invalid value cannot reach PostgreSQL's +// command arguments or managed configuration. +func normalizePostgresPort(port string) (string, error) { + n, err := strconv.ParseUint(port, 10, 16) + if err != nil || n == 0 { + return "", fmt.Errorf("invalid PostgreSQL port %q (expected 1-65535)", port) + } + return strconv.FormatUint(n, 10), nil +} + +// normalizePostgresEndpoint returns the endpoint in the single canonical form +// shared by psql, readiness checks, and the Mendix runtime configuration. +func normalizePostgresEndpoint(hostPort string) (host, port, endpoint string, err error) { + host, port = splitHostPort(hostPort) + port, err = normalizePostgresPort(port) + if err != nil { + return "", "", "", err + } + if host == "" { + host = "127.0.0.1" + } + return host, port, net.JoinHostPort(host, port), nil +} + // isLocalHost reports whether host refers to the local machine (so it is safe to // start a service / use a local superuser). func isLocalHost(host string) bool { @@ -52,8 +110,13 @@ func quoteSQLString(s string) string { // EnsureDatabase makes db reachable and the app role + database present. It is a // no-op when the app can already connect. For a local host whose port is down it -// starts Postgres, then creates the role and database if missing. -func EnsureDatabase(db DBConfig, w io.Writer) error { +// starts Postgres, then creates the role and database if missing. It +// canonicalizes db.Host in place so the runtime receives the exact endpoint +// that was checked and provisioned. +func EnsureDatabase(db *DBConfig, w io.Writer) error { + if db == nil { + return fmt.Errorf("--ensure-db requires a database configuration") + } if strings.ToLower(db.Type) != "postgresql" { return fmt.Errorf("--ensure-db only supports PostgreSQL (DatabaseType=%q)", db.Type) } @@ -63,21 +126,26 @@ func EnsureDatabase(db DBConfig, w io.Writer) error { if !pgIdent.MatchString(db.User) { return fmt.Errorf("unsafe database user %q (expected %s)", db.User, pgIdent) } - host, port := splitHostPort(db.Host) + originalHost := db.Host + host, port, endpoint, err := normalizePostgresEndpoint(originalHost) + if err != nil { + return fmt.Errorf("invalid --db-host %q: %w", originalHost, err) + } + db.Host = endpoint // Already usable? Then we're done. - if canConnectDB(db) { + if canConnectDB(*db) { return nil } + if !isLocalHost(host) { + return fmt.Errorf("database is not usable at %s and host is not local; "+ + "start it and create the %q database (user %q)", db.Host, db.Name, db.User) + } - // Port down: start the local Postgres service (best-effort) or bail for remote. + // Port down: start the local Postgres service (best-effort). if err := pingTCP(db.Host, 2*time.Second); err != nil { - if !isLocalHost(host) { - return fmt.Errorf("database not reachable at %s and host is not local; "+ - "start it and create the %q database (user %q)", db.Host, db.Name, db.User) - } fmt.Fprintln(w, " Starting local PostgreSQL...") - if err := startLocalPostgres(); err != nil { + if err := startLocalPostgres(host, port, w); err != nil { return fmt.Errorf("starting local PostgreSQL: %w", err) } if err := waitPGReady(host, port, 20*time.Second); err != nil { @@ -86,14 +154,18 @@ func EnsureDatabase(db DBConfig, w io.Writer) error { } // Ensure the role and database exist (needs a local superuser). - if err := ensureRole(db, w); err != nil { + su, err := resolveSuperuser(host, port) + if err != nil { + return err + } + if err := ensureRole(su, *db, w); err != nil { return err } - if err := ensureDatabase(db, w); err != nil { + if err := ensureDatabase(su, *db, w); err != nil { return err } - if !canConnectDB(db) { + if !canConnectDB(*db) { return fmt.Errorf("provisioned PostgreSQL but still cannot connect to %q as %q at %s", db.Name, db.User, db.Host) } @@ -104,47 +176,318 @@ func EnsureDatabase(db DBConfig, w io.Writer) error { // canConnectDB reports whether the app can connect to its database as its user. func canConnectDB(db DBConfig) bool { host, port := splitHostPort(db.Host) - cmd := exec.Command("psql", "-h", host, "-p", port, "-U", db.User, "-d", db.Name, + cmd := exec.Command("psql", "-X", "-w", "-h", host, "-p", port, "-U", db.User, "-d", db.Name, "-tAc", "select 1") cmd.Env = append(os.Environ(), "PGPASSWORD="+db.Password, "PGCONNECT_TIMEOUT=3") return cmd.Run() == nil } // startLocalPostgres starts the local PostgreSQL service, trying the common -// service managers in turn. Success is confirmed later by waitPGReady. -func startLocalPostgres() error { +// service managers in turn. When none is present — e.g. on Arch — or they do +// not produce a ready server, it falls back to a user-owned cluster started with +// the portable initdb/pg_ctl tools (#823). +func startLocalPostgres(host, port string, w io.Writer) error { + var err error + port, err = normalizePostgresPort(port) + if err != nil { + return err + } + // Only real, portable service managers belong here. The old + // {"pg_ctlcluster", "--", "start"} entry was a placeholder whose args could + // never start a cluster, so it only ever burned a readiness timeout before + // the fallback — dropped (#823 review). attempts := [][]string{ {"service", "postgresql", "start"}, - {"pg_ctlcluster", "--", "start"}, // placeholder; real cluster args vary } - var lastErr error + var serviceDiag []string for _, a := range attempts { if _, err := exec.LookPath(a[0]); err != nil { - lastErr = err continue } - cmd := exec.Command(a[0], a[1:]...) - if err := cmd.Run(); err == nil { + // The command may exit non-zero yet still bring Postgres up, so a short + // readiness probe decides — not the exit code. The probe is intentionally + // short: the single authoritative 20s wait is in EnsureDatabase, so a + // slow-but-working manager is honoured there rather than paid for here. + out, _ := exec.Command(a[0], a[1:]...).CombinedOutput() + if waitPGReady(host, port, serviceReadyTimeout) == nil { return nil - } else { - lastErr = err } - // `service postgresql start` is the reliable path in the devcontainer; if - // it ran (even non-zero) Postgres may still be coming up — let waitPGReady - // decide rather than failing here. + if d := strings.TrimSpace(string(out)); d != "" { + serviceDiag = append(serviceDiag, a[0]+": "+d) + } + } + + // A service-started server may own the port while crash recovery still makes + // pg_isready report "not ready". Never initialize a competitor in that case: + // EnsureDatabase's authoritative 20-second wait decides whether it recovers. + // This guard also covers a process that won the port between the caller's + // initial reachability check and this fallback. + if pingTCP(net.JoinHostPort(host, port), time.Second) == nil { + return nil + } + + // No service manager made PostgreSQL ready: start a user-owned cluster with + // the portable tools. This needs neither a `postgres` OS account nor sudo. + if err := startUserCluster(host, port, w); err != nil { + if len(serviceDiag) > 0 { + // Surface what the service manager said; otherwise the user sees only + // an initdb/pg_ctl error from two steps later. + return fmt.Errorf("%w\n(a service manager ran first but Postgres did not "+ + "become ready:\n%s)", err, strings.Join(serviceDiag, "\n")) + } + return err + } + return nil +} + +// userClusterDirs returns the state, data, and socket directories for the +// user-owned cluster under ~/.mxcli/postgres. The socket dir is separate and +// short because some systems cap the Unix-socket path length. +func userClusterDirs() (stateDir, dataDir, sockDir string, err error) { + home, err := os.UserHomeDir() + if err != nil { + return "", "", "", fmt.Errorf("determining home directory: %w", err) + } + stateDir = filepath.Join(home, ".mxcli", "postgres") + return stateDir, filepath.Join(stateDir, "data"), filepath.Join(stateDir, "sock"), nil +} + +// writeFileAtomic replaces a cluster configuration file without exposing a +// partially-written file to a concurrent or subsequent postgres start. +func writeFileAtomic(path string, data []byte, mode os.FileMode) error { + tmp, err := os.CreateTemp(filepath.Dir(path), "."+filepath.Base(path)+"-*") + if err != nil { + return err + } + tmpPath := tmp.Name() + keep := false + defer func() { + _ = tmp.Close() + if !keep { + _ = os.Remove(tmpPath) + } + }() + if err := tmp.Chmod(mode); err != nil { + return err + } + if _, err := tmp.Write(data); err != nil { + return err + } + if err := tmp.Sync(); err != nil { + return err + } + if err := tmp.Close(); err != nil { + return err + } + if err := os.Rename(tmpPath, path); err != nil { + return err + } + keep = true + return nil +} + +func postgresConfigString(value string) (string, error) { + if strings.ContainsAny(value, "\x00\r\n") { + return "", fmt.Errorf("value contains a line break or NUL byte") + } + return "'" + strings.ReplaceAll(value, "'", "''") + "'", nil +} + +const ( + mxcliPostgresConfigBegin = "# BEGIN mxcli managed connection settings" + mxcliPostgresConfigEnd = "# END mxcli managed connection settings" +) + +// persistUserClusterConfig makes the private endpoint a property of the +// cluster, so it also holds for a later plain `pg_ctl -D ... start`. +func persistUserClusterConfig(dataDir, host, port, sockDir string) error { + hostValue, err := postgresConfigString(host) + if err != nil { + return fmt.Errorf("quoting listen address: %w", err) + } + sockValue, err := postgresConfigString(sockDir) + if err != nil { + return fmt.Errorf("quoting socket directory: %w", err) + } + managed := fmt.Sprintf("%s\n"+ + "listen_addresses = %s\n"+ + "port = %s\n"+ + "unix_socket_directories = %s\n"+ + "unix_socket_permissions = 0700\n"+ + "%s\n", mxcliPostgresConfigBegin, hostValue, port, sockValue, mxcliPostgresConfigEnd) + + confPath := filepath.Join(dataDir, "postgresql.conf") + conf, err := os.ReadFile(confPath) + if err != nil { + return fmt.Errorf("reading postgresql.conf: %w", err) + } + for { + start := strings.Index(string(conf), mxcliPostgresConfigBegin) + if start < 0 { + break + } + relEnd := strings.Index(string(conf[start:]), mxcliPostgresConfigEnd) + if relEnd < 0 { + return fmt.Errorf("managed block in postgresql.conf is missing its end marker") + } + end := start + relEnd + len(mxcliPostgresConfigEnd) + if end < len(conf) && conf[end] == '\n' { + end++ + } + conf = append(conf[:start], conf[end:]...) + } + conf = []byte(strings.TrimRight(string(conf), "\r\n")) + if len(conf) > 0 { + conf = append(conf, '\n', '\n') + } + conf = append(conf, managed...) + if err := writeFileAtomic(confPath, conf, 0o600); err != nil { + return fmt.Errorf("updating postgresql.conf: %w", err) + } + return nil +} + +// rejectLegacyHostTrust prevents silent reuse of a cluster initialized by an +// earlier development revision, which used trust for loopback TCP. Local trust +// is expected and remains confined to the private Unix socket. +func rejectLegacyHostTrust(dataDir string) error { + hbaPath := filepath.Join(dataDir, "pg_hba.conf") + hba, err := os.ReadFile(hbaPath) + if err != nil { + return fmt.Errorf("reading pg_hba.conf: %w", err) + } + for lineNo, line := range strings.Split(string(hba), "\n") { + fields := strings.Fields(strings.SplitN(line, "#", 2)[0]) + if len(fields) < 5 || !strings.HasPrefix(fields[0], "host") { + continue + } + for _, field := range fields[4:] { + if field == "trust" { + return fmt.Errorf("unsafe host trust authentication in %s line %d; stop PostgreSQL, "+ + "remove ~/.mxcli/postgres, and rerun --ensure-db", hbaPath, lineNo+1) + } + } + } + return nil +} + +// clusterStatus reports whether a server is running from dataDir and, when +// readable, its TCP port and Unix-socket directory from postmaster.pid. +// `pg_ctl status` alone only proves that some server runs from this data directory. +func clusterStatus(dataDir string) (running bool, port, sockDir string) { + if exec.Command("pg_ctl", "-D", dataDir, "status").Run() != nil { + return false, "", "" + } + data, err := os.ReadFile(filepath.Join(dataDir, "postmaster.pid")) + if err != nil { + return true, "", "" // running, but its endpoint is unknown + } + // postmaster.pid: line 1 PID, 2 data dir, 3 start time, 4 port, 5 socket dir… + if lines := strings.Split(string(data), "\n"); len(lines) >= 5 { + return true, strings.TrimSpace(lines[3]), strings.TrimSpace(lines[4]) + } + return true, "", "" +} + +// startUserCluster initializes (once) and starts a PostgreSQL cluster owned by +// the current user under ~/.mxcli/postgres, listening on host:port. It is safe +// to run repeatedly: an initialized data directory is reused and an already +// running server is left alone. +func startUserCluster(host, port string, w io.Writer) error { + stateDir, dataDir, sockDir, err := userClusterDirs() + if err != nil { + return err + } + // Normalise an empty host so the persisted listen address is explicit even if + // this helper is called directly with a bare ":port". + if host == "" { + host = "127.0.0.1" + } + if !isLocalHost(host) { + return fmt.Errorf("refusing to start a user-owned PostgreSQL cluster on non-local host %q", host) + } + port, err = normalizePostgresPort(port) + if err != nil { + return err + } + if err := os.MkdirAll(sockDir, 0o700); err != nil { + return fmt.Errorf("creating PostgreSQL socket directory %s: %w", sockDir, err) + } + // MkdirAll ignores its mode for an existing directory. Tighten it explicitly: + // local trust authentication relies on this directory as its access boundary. + if err := os.Chmod(sockDir, 0o700); err != nil { + return fmt.Errorf("securing PostgreSQL socket directory %s: %w", sockDir, err) + } + + // Initialize once — PG_VERSION marks a data directory initdb has populated. + if _, err := os.Stat(filepath.Join(dataDir, "PG_VERSION")); os.IsNotExist(err) { + fmt.Fprintln(w, " Initializing user-owned PostgreSQL cluster...") + // Bootstrap superuser "postgres". trust over the private 0700 socket keeps + // our own provisioning password-free, but loopback TCP is scram-sha-256: + // binding 127.0.0.1 is not an access control on a multi-user host, so trust + // there would let any local account act as the postgres superuser. + init := exec.Command("initdb", "-D", dataDir, "-U", "postgres", + "--auth-local=trust", "--auth-host=scram-sha-256", "--encoding=UTF8") + if out, err := init.CombinedOutput(); err != nil { + return fmt.Errorf("initializing PostgreSQL cluster in %s: %w\n%s", + dataDir, err, strings.TrimSpace(string(out))) + } + } else if err != nil { + return fmt.Errorf("checking PostgreSQL cluster in %s: %w", dataDir, err) + } + + if err := rejectLegacyHostTrust(dataDir); err != nil { + return err + } + + // A cluster left from an earlier --db-host satisfies `pg_ctl status` even when + // its endpoint differs. Validate the live endpoint before changing its future + // restart configuration, so a rejected request has no hidden side effect. + running, livePort, liveSockDir := clusterStatus(dataDir) + if running { + if livePort == "" { + return fmt.Errorf("a user-owned PostgreSQL cluster is running from %s, but its port "+ + "could not be read from %s", dataDir, filepath.Join(dataDir, "postmaster.pid")) + } + if livePort != port { + return fmt.Errorf("a user-owned PostgreSQL cluster is already running from %s on "+ + "port %s, but port %s was requested — stop it with `pg_ctl -D %s stop` or rerun "+ + "with --db-host %s", dataDir, livePort, port, dataDir, net.JoinHostPort(host, livePort)) + } + if liveSockDir == "" { + return fmt.Errorf("a user-owned PostgreSQL cluster is running from %s, but its socket "+ + "directory could not be read from %s", dataDir, filepath.Join(dataDir, "postmaster.pid")) + } + if filepath.Clean(liveSockDir) != filepath.Clean(sockDir) { + return fmt.Errorf("a user-owned PostgreSQL cluster is running with socket directory %s, "+ + "but the private directory %s is required — stop it with `pg_ctl -D %s stop` and "+ + "rerun --ensure-db", liveSockDir, sockDir, dataDir) + } + } + + if err := persistUserClusterConfig(dataDir, host, port, sockDir); err != nil { + return fmt.Errorf("persisting PostgreSQL connection settings in %s: %w", dataDir, err) + } + if running { return nil } - if lastErr != nil { - return lastErr + + fmt.Fprintln(w, " Starting user-owned PostgreSQL cluster...") + logPath := filepath.Join(stateDir, "server.log") + start := exec.Command("pg_ctl", "-D", dataDir, "-w", + "-t", "30", "-l", logPath, "start") + if out, err := start.CombinedOutput(); err != nil { + return fmt.Errorf("starting PostgreSQL cluster in %s: %w\n%s\n (see the server "+ + "log at %s)", dataDir, err, strings.TrimSpace(string(out)), logPath) } - return fmt.Errorf("no known service manager found to start PostgreSQL") + return nil } // waitPGReady polls pg_isready until the server accepts connections or timeout. func waitPGReady(host, port string, timeout time.Duration) error { if _, err := exec.LookPath("pg_isready"); err != nil { // Fall back to a raw TCP check. - return waitTCP(host+":"+port, timeout) + return waitTCP(net.JoinHostPort(host, port), timeout) } deadline := time.Now().Add(timeout) for time.Now().Before(deadline) { @@ -154,7 +497,7 @@ func waitPGReady(host, port string, timeout time.Duration) error { } time.Sleep(500 * time.Millisecond) } - return fmt.Errorf("PostgreSQL did not become ready at %s:%s within %s", host, port, timeout) + return fmt.Errorf("PostgreSQL did not become ready at %s within %s", net.JoinHostPort(host, port), timeout) } // waitTCP polls a host:port until it accepts a connection or timeout. @@ -169,40 +512,134 @@ func waitTCP(hostPort string, timeout time.Duration) error { return fmt.Errorf("%s did not accept connections within %s", hostPort, timeout) } -// superuserPSQL runs a psql command as the postgres superuser (sudo -u postgres). -func superuserPSQL(args ...string) *exec.Cmd { - full := append([]string{"-u", "postgres", "psql", "-v", "ON_ERROR_STOP=1"}, args...) - return exec.Command("sudo", full...) +// superuser is how we reach a PostgreSQL superuser to provision the role and +// database: a direct `psql -U postgres` against the user-owned cluster (over its +// private socket), or `sudo -u postgres psql` over the system cluster's default +// Unix socket. The latter preserves the peer-auth path used by Debian/Ubuntu. +type superuser struct { + host, port string + sock string // Unix-socket dir for the user-owned cluster; preferred over TCP + sudo bool +} + +// withoutPostgresTargetEnv prevents inherited libpq settings from overriding a +// deliberately host-less sudo connection. In that path, omitting -h selects the +// distribution's default Unix socket; an inherited PGHOST/PGHOSTADDR/PGSERVICE +// could otherwise silently turn it back into TCP or target another cluster. +func withoutPostgresTargetEnv(env []string) []string { + clean := make([]string, 0, len(env)) + for _, entry := range env { + key, _, _ := strings.Cut(entry, "=") + switch key { + case "PGHOST", "PGHOSTADDR", "PGSERVICE": + continue + } + clean = append(clean, entry) + } + return clean +} + +// psql builds a psql command for the superuser. ON_ERROR_STOP makes a failed +// statement a non-zero exit rather than a silent success; -X keeps a user's +// psqlrc from changing this non-interactive provisioning session. +func (s superuser) psql(args ...string) *exec.Cmd { + // Prefer the cluster's private socket: initdb set --auth-local=trust there, + // while loopback TCP is scram-sha-256, so the passwordless superuser can only + // reach itself over the socket. A system-cluster sudo connection also needs a + // Unix socket: changing the OS user to postgres enables peer auth, but gives no + // TCP password. Its requested port still selects the exact default-socket + // cluster. -w never prompts, so every other path fails fast. + host := "" + if s.sock != "" { + host = s.sock + } else if !s.sudo { + host = s.host + } + base := []string{"-X", "-v", "ON_ERROR_STOP=1", "-w"} + if host != "" { + base = append(base, "-h", host) + } + base = append(base, "-p", s.port, "-U", "postgres", "-d", "postgres") + if s.sudo { + // -n prevents sudo from opening /dev/tty for a password; psql's -w above + // independently prevents a database-password prompt. Omit -h to retain the + // system cluster's peer-authenticated Unix socket, but pass the requested + // port so a non-default cluster cannot fall through to port 5432. + sudoBase := []string{"-n", "-u", "postgres", "--", "psql"} + cmd := exec.Command("sudo", append(sudoBase, append(base, args...)...)...) + cmd.Env = withoutPostgresTargetEnv(os.Environ()) + return cmd + } + return exec.Command("psql", append(base, args...)...) +} + +// resolveSuperuser picks a working superuser path: a direct connection (the +// user-owned initdb cluster over its trust socket, or a plain loopback cluster — +// the only paths that work without elevation on Arch) or `sudo -u postgres` for +// a system cluster. +func resolveSuperuser(host, port string) (superuser, error) { + if !isLocalHost(host) { + return superuser{}, fmt.Errorf("refusing to use a local PostgreSQL superuser for non-local host %q", host) + } + // Prefer our cluster's private socket (trust); fall back to a plain + // loopback connection for a pre-existing cluster a user pointed us at. + candidates := []superuser{{host: host, port: port}} + if _, _, sockDir, err := userClusterDirs(); err == nil { + candidates = append([]superuser{{host: host, port: port, sock: sockDir}}, candidates...) + } + for _, direct := range candidates { + if direct.psql("-tAc", "select 1").Run() == nil { + return direct, nil + } + } + if _, err := exec.LookPath("sudo"); err == nil { + sudo := superuser{host: host, port: port, sudo: true} + if sudo.psql("-tAc", "select 1").Run() == nil { + return sudo, nil + } + } + return superuser{}, fmt.Errorf("no local PostgreSQL superuser available to create the " + + "role/database (tried a direct 'psql -U postgres' connection over the cluster socket " + + "and TCP, and non-interactive 'sudo -u postgres')") } // ensureRole creates the app login role if it does not already exist. -func ensureRole(db DBConfig, w io.Writer) error { - check := superuserPSQL("-tAc", fmt.Sprintf("select 1 from pg_roles where rolname='%s'", db.User)) +func ensureRole(su superuser, db DBConfig, w io.Writer) error { + check := su.psql("-tAc", fmt.Sprintf("select 1 from pg_roles where rolname='%s'", db.User)) out, _ := check.Output() if strings.TrimSpace(string(out)) == "1" { return nil } fmt.Fprintf(w, " Creating role %q...\n", db.User) - ddl := fmt.Sprintf("CREATE ROLE %s WITH LOGIN PASSWORD %s CREATEDB", db.User, quoteSQLString(db.Password)) - cmd := superuserPSQL("-c", ddl) + // initdb writes this setting when --auth-host=scram-sha-256 is used, but an + // existing system cluster can still retain PostgreSQL <=13's md5 default. + // Set it for this session so every role created for a SCRAM HBA record gets a + // compatible verifier without changing the administrator's global config. + ddl := fmt.Sprintf("SET standard_conforming_strings = on; SET password_encryption = 'scram-sha-256'; "+ + "CREATE ROLE %s WITH LOGIN PASSWORD %s CREATEDB;\n", + db.User, quoteSQLString(db.Password)) + // Feed the only SQL containing a secret over stdin. Passing it via `-c` would + // expose the database password in the process list to other local users. + cmd := su.psql("-f", "-") + cmd.Stdin = strings.NewReader(ddl) if out, err := cmd.CombinedOutput(); err != nil { return fmt.Errorf("creating role %q: %w\n%s\n"+ - " (need a local postgres superuser via 'sudo -u postgres'; create the role manually if unavailable)", + " (need a local postgres superuser; create the role manually if unavailable)", db.User, err, strings.TrimSpace(string(out))) } return nil } // ensureDatabase creates the app database owned by the app role if it is absent. -func ensureDatabase(db DBConfig, w io.Writer) error { - check := superuserPSQL("-tAc", fmt.Sprintf("select 1 from pg_database where datname='%s'", db.Name)) +func ensureDatabase(su superuser, db DBConfig, w io.Writer) error { + check := su.psql("-tAc", fmt.Sprintf("select 1 from pg_database where datname='%s'", db.Name)) out, _ := check.Output() if strings.TrimSpace(string(out)) == "1" { return nil } fmt.Fprintf(w, " Creating database %q owned by %q...\n", db.Name, db.User) ddl := fmt.Sprintf("CREATE DATABASE %s OWNER %s", db.Name, db.User) - cmd := superuserPSQL("-c", ddl) + cmd := su.psql("-c", ddl) if out, err := cmd.CombinedOutput(); err != nil { return fmt.Errorf("creating database %q: %w\n%s", db.Name, err, strings.TrimSpace(string(out))) } diff --git a/cmd/mxcli/docker/ensuredb_test.go b/cmd/mxcli/docker/ensuredb_test.go index 5cd0ab470..6a5a9efe4 100644 --- a/cmd/mxcli/docker/ensuredb_test.go +++ b/cmd/mxcli/docker/ensuredb_test.go @@ -4,7 +4,15 @@ package docker import ( "io" + "net" + "os" + "path/filepath" + "reflect" + "runtime" + "strconv" + "strings" "testing" + "time" ) func TestSplitHostPort(t *testing.T) { @@ -12,6 +20,10 @@ func TestSplitHostPort(t *testing.T) { {"127.0.0.1:5432", "127.0.0.1", "5432"}, {"db.example.com:6000", "db.example.com", "6000"}, {"localhost", "localhost", "5432"}, + {"[::1]:5544", "::1", "5544"}, + {"[::1]", "::1", "5432"}, + {"::1", "::1", "5432"}, + {"::1:5544", "::1", "5544"}, } for _, c := range cases { h, p := splitHostPort(c.in) @@ -21,6 +33,36 @@ func TestSplitHostPort(t *testing.T) { } } +func TestNormalizePostgresEndpoint(t *testing.T) { + cases := []struct{ in, host, port, endpoint string }{ + {":5432", "127.0.0.1", "5432", "127.0.0.1:5432"}, + {"[::1]:5544", "::1", "5544", "[::1]:5544"}, + {"[::1]", "::1", "5432", "[::1]:5432"}, + {"::1:5544", "::1", "5544", "[::1]:5544"}, + } + for _, c := range cases { + host, port, endpoint, err := normalizePostgresEndpoint(c.in) + if err != nil { + t.Errorf("normalizePostgresEndpoint(%q): %v", c.in, err) + continue + } + if host != c.host || port != c.port || endpoint != c.endpoint { + t.Errorf("normalizePostgresEndpoint(%q) = (%q,%q,%q), want (%q,%q,%q)", + c.in, host, port, endpoint, c.host, c.port, c.endpoint) + } + } +} + +func TestPostgresConfigString(t *testing.T) { + got, err := postgresConfigString("/home/user's db") + if err != nil || got != "'/home/user''s db'" { + t.Fatalf("postgresConfigString escaped value = (%q, %v)", got, err) + } + if _, err := postgresConfigString("safe'\nport = 1"); err == nil { + t.Fatal("configuration line injection should be rejected") + } +} + func TestIsLocalHost(t *testing.T) { for _, h := range []string{"127.0.0.1", "localhost", "::1", ""} { if !isLocalHost(h) { @@ -44,15 +86,720 @@ func TestQuoteSQLString(t *testing.T) { } func TestEnsureDatabase_Validation(t *testing.T) { + if err := EnsureDatabase(nil, io.Discard); err == nil { + t.Error("expected error for a nil database configuration") + } // Non-Postgres type is rejected. - if err := EnsureDatabase(DBConfig{Type: "HSQLDB", Name: "a", User: "u"}, io.Discard); err == nil { + if err := EnsureDatabase(&DBConfig{Type: "HSQLDB", Name: "a", User: "u"}, io.Discard); err == nil { t.Error("expected error for non-PostgreSQL type") } // Unsafe identifiers are rejected (before any exec). - if err := EnsureDatabase(DBConfig{Type: "PostgreSQL", Name: "bad-name;DROP", User: "u", Host: "127.0.0.1:5432"}, io.Discard); err == nil { + if err := EnsureDatabase(&DBConfig{Type: "PostgreSQL", Name: "bad-name;DROP", User: "u", Host: "127.0.0.1:5432"}, io.Discard); err == nil { t.Error("expected error for unsafe database name") } - if err := EnsureDatabase(DBConfig{Type: "PostgreSQL", Name: "ok", User: "bad user", Host: "127.0.0.1:5432"}, io.Discard); err == nil { + if err := EnsureDatabase(&DBConfig{Type: "PostgreSQL", Name: "ok", User: "bad user", Host: "127.0.0.1:5432"}, io.Discard); err == nil { t.Error("expected error for unsafe database user") } + for _, host := range []string{"127.0.0.1:0", "127.0.0.1:5432 -c fsync=off"} { + if err := EnsureDatabase(&DBConfig{Type: "PostgreSQL", Name: "ok", User: "ok", Host: host}, io.Discard); err == nil || !strings.Contains(err.Error(), "invalid --db-host") { + t.Errorf("expected an actionable error for invalid host %q, got %v", host, err) + } + } +} + +func TestEnsureDatabase_NormalizesHostForCaller(t *testing.T) { + dir, logPath := newStubPATH(t) + writeStub(t, dir, "psql", `echo "$@" >> "`+logPath+`"; exit 0`) + db := DBConfig{ + Type: "PostgreSQL", Host: ":5432", Name: "app", User: "mendix", Password: "very-secret", + } + if err := EnsureDatabase(&db, io.Discard); err != nil { + t.Fatal(err) + } + if db.Host != "127.0.0.1:5432" { + t.Fatalf("caller DB host = %q, want canonical loopback endpoint", db.Host) + } + params := runtimeConfigParams(LocalRuntimeOptions{DB: db}, nil) + if got := params["DatabaseHost"]; got != db.Host { + t.Fatalf("runtime DatabaseHost = %q, want provisioned endpoint %q", got, db.Host) + } + if calls := readCalls(t, logPath); !strings.Contains(calls, "-h 127.0.0.1 -p 5432") { + t.Fatalf("psql did not receive the canonical endpoint:\n%s", calls) + } else if !strings.Contains(calls, "-X -w") { + t.Fatalf("connection probe must ignore psqlrc and never prompt:\n%s", calls) + } else if strings.Contains(calls, db.Password) { + t.Fatalf("connection password must not be exposed in psql process arguments:\n%s", calls) + } +} + +func TestEnsureDatabase_RefusesToProvisionRemoteHost(t *testing.T) { + dir, logPath := newStubPATH(t) + writeStub(t, dir, "psql", `echo psql >> "`+logPath+`"; exit 1`) + writeStub(t, dir, "sudo", `echo sudo >> "`+logPath+`"; exit 0`) + writeStub(t, dir, "service", `echo service >> "`+logPath+`"; exit 0`) + writeStub(t, dir, "initdb", `echo initdb >> "`+logPath+`"; exit 0`) + db := DBConfig{ + Type: "PostgreSQL", Host: "db.example.com:5432", Name: "app", User: "mendix", Password: "secret", + } + err := EnsureDatabase(&db, io.Discard) + if err == nil || !strings.Contains(err.Error(), "host is not local") { + t.Fatalf("expected remote provisioning to be refused, got %v", err) + } + if calls := readCalls(t, logPath); calls != "psql\n" { + t.Fatalf("remote failure must only run the connection probe, calls:\n%s", calls) + } +} + +// --- #823: initdb/pg_ctl fallback --- + +func newStubPATH(t *testing.T) (dir, logPath string) { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("shell-stub test not supported on Windows") + } + dir = t.TempDir() + t.Setenv("PATH", dir) + t.Setenv("HOME", t.TempDir()) + return dir, filepath.Join(dir, "calls") +} + +func writeStub(t *testing.T, dir, name, body string) { + t.Helper() + if err := os.WriteFile(filepath.Join(dir, name), []byte("#!/bin/sh\n"+body+"\n"), 0o755); err != nil { + t.Fatalf("writing stub %s: %v", name, err) + } +} + +func initdbStub(logPath string) string { + return `data=""; next=0 +for arg in "$@"; do + [ "$next" = 1 ] && { data="$arg"; next=0; } + [ "$arg" = "-D" ] && next=1 +done +/bin/mkdir -p "$data" +echo 16 > "$data/PG_VERSION" +echo "# PostgreSQL configuration" > "$data/postgresql.conf" +echo "local all all trust" > "$data/pg_hba.conf" +echo "host all all 127.0.0.1/32 scram-sha-256" >> "$data/pg_hba.conf" +echo "host all all ::1/128 scram-sha-256" >> "$data/pg_hba.conf" +echo initdb >> "` + logPath + `"` +} + +func pgctlStub(logPath string, statusCode, startCode int) string { + return `last=""; for arg in "$@"; do last="$arg"; done +[ "$last" = status ] && exit ` + strconv.Itoa(statusCode) + ` +echo pg_ctl_start >> "` + logPath + `" +exit ` + strconv.Itoa(startCode) +} + +func readCalls(t *testing.T, logPath string) string { + t.Helper() + b, err := os.ReadFile(logPath) + if os.IsNotExist(err) { + return "" + } + if err != nil { + t.Fatal(err) + } + return string(b) +} + +// unusedTCPPort returns a currently unbound loopback port. Fallback tests must +// not depend on the developer or CI host having nothing listening on 5432: an +// occupied port intentionally suppresses the user-cluster fallback. +func unusedTCPPort(t *testing.T) string { + t.Helper() + listener, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + port := strconv.Itoa(listener.Addr().(*net.TCPAddr).Port) + if err := listener.Close(); err != nil { + t.Fatal(err) + } + return port +} + +func TestStartLocalPostgres_ServicePaths(t *testing.T) { + t.Run("ready service skips fallback", func(t *testing.T) { + dir, logPath := newStubPATH(t) + writeStub(t, dir, "service", `echo service >> "`+logPath+`"; exit 1`) + writeStub(t, dir, "pg_isready", "exit 0") + writeStub(t, dir, "initdb", initdbStub(logPath)) + writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 3, 0)) + + if err := startLocalPostgres("127.0.0.1", "5432", io.Discard); err != nil { + t.Fatal(err) + } + calls := readCalls(t, logPath) + if !strings.Contains(calls, "service") || strings.Contains(calls, "initdb") { + t.Fatalf("unexpected calls:\n%s", calls) + } + }) + + t.Run("non-ready service uses fallback", func(t *testing.T) { + dir, logPath := newStubPATH(t) + port := unusedTCPPort(t) + oldTimeout := serviceReadyTimeout + serviceReadyTimeout = 50 * time.Millisecond + t.Cleanup(func() { serviceReadyTimeout = oldTimeout }) + writeStub(t, dir, "service", `echo service >> "`+logPath+`"`) + writeStub(t, dir, "pg_isready", "exit 1") + writeStub(t, dir, "initdb", initdbStub(logPath)) + writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 3, 0)) + + if err := startLocalPostgres("127.0.0.1", port, io.Discard); err != nil { + t.Fatal(err) + } + calls := readCalls(t, logPath) + for _, want := range []string{"service", "initdb", "pg_ctl_start"} { + if !strings.Contains(calls, want) { + t.Fatalf("%s was not called:\n%s", want, calls) + } + } + }) +} + +// A service-started PostgreSQL can own the TCP port while pg_isready still +// reports "the database system is starting up" (for example during crash +// recovery). The short service probe must not initialize a competing cluster; +// EnsureDatabase's longer readiness wait is authoritative in this case. +func TestStartLocalPostgres_OccupiedPortSkipsFallback(t *testing.T) { + listener, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = listener.Close() }) + port := strconv.Itoa(listener.Addr().(*net.TCPAddr).Port) + + dir, logPath := newStubPATH(t) + oldTimeout := serviceReadyTimeout + serviceReadyTimeout = 20 * time.Millisecond + t.Cleanup(func() { serviceReadyTimeout = oldTimeout }) + writeStub(t, dir, "service", `echo service >> "`+logPath+`"`) + writeStub(t, dir, "pg_isready", "exit 1") + writeStub(t, dir, "initdb", initdbStub(logPath)) + writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 3, 0)) + + if err := startLocalPostgres("127.0.0.1", port, io.Discard); err != nil { + t.Fatal(err) + } + calls := readCalls(t, logPath) + if !strings.Contains(calls, "service") { + t.Fatalf("service manager was not attempted:\n%s", calls) + } + if strings.Contains(calls, "initdb") || strings.Contains(calls, "pg_ctl_start") { + t.Fatalf("an occupied port must suppress the user-cluster fallback:\n%s", calls) + } +} + +func TestStartLocalPostgres_Fallback(t *testing.T) { + tests := []struct { + name string + tools, initialized bool + statusCode, startCode int + wantErr bool + wantInit, wantStart bool + }{ + {name: "missing tools", wantErr: true}, + {name: "first init", tools: true, statusCode: 3, wantInit: true, wantStart: true}, + {name: "repeated stopped", tools: true, initialized: true, statusCode: 3, wantStart: true}, + {name: "already running", tools: true, initialized: true}, + {name: "start failure", tools: true, statusCode: 3, startCode: 1, wantErr: true, wantInit: true, wantStart: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir, logPath := newStubPATH(t) + port := unusedTCPPort(t) + if tt.initialized { + dataDir := initClusterDir(t) + if tt.statusCode == 0 { + writePostmasterPID(t, dataDir, port) + } + } + if tt.tools { + writeStub(t, dir, "initdb", initdbStub(logPath)) + writeStub(t, dir, "pg_ctl", pgctlStub(logPath, tt.statusCode, tt.startCode)) + } + + err := startLocalPostgres("127.0.0.1", port, io.Discard) + if (err != nil) != tt.wantErr { + t.Fatalf("error = %v, wantErr %v", err, tt.wantErr) + } + calls := readCalls(t, logPath) + if strings.Contains(calls, "initdb") != tt.wantInit { + t.Errorf("initdb calls = %q, want %v", calls, tt.wantInit) + } + if strings.Contains(calls, "pg_ctl_start") != tt.wantStart { + t.Errorf("pg_ctl start calls = %q, want %v", calls, tt.wantStart) + } + }) + } +} + +func TestResolveSuperuser(t *testing.T) { + t.Run("direct", func(t *testing.T) { + dir, logPath := newStubPATH(t) + writeStub(t, dir, "psql", `echo "$@" >> "`+logPath+`"`) + writeStub(t, dir, "sudo", `echo sudo >> "`+logPath+`"`) + su, err := resolveSuperuser("127.0.0.1", "5432") + if err != nil || su.sudo { + t.Fatalf("su=%+v err=%v", su, err) + } + calls := readCalls(t, logPath) + if !strings.Contains(calls, "-U postgres") || strings.Contains(calls, "sudo") { + t.Fatalf("unexpected calls:\n%s", calls) + } + }) + + t.Run("sudo fallback", func(t *testing.T) { + dir, logPath := newStubPATH(t) + writeStub(t, dir, "psql", "exit 1") + // Model a Debian-style cluster: sudo succeeds only through the default + // Unix socket, and the requested port must still be explicit. A numeric + // -h would force TCP, where peer authentication cannot work. + writeStub(t, dir, "sudo", `echo "$@" >> "`+logPath+`" +case " $* " in *" -h "*) exit 1;; esac +case " $* " in *" -p 5544 "*) exit 0;; esac +exit 1`) + su, err := resolveSuperuser("127.0.0.1", "5544") + if err != nil || !su.sudo { + t.Fatalf("su=%+v err=%v", su, err) + } + if calls := readCalls(t, logPath); strings.Contains(calls, " -h ") || + !strings.Contains(calls, " -p 5544 ") { + t.Fatalf("sudo fallback must use the default socket at the requested port:\n%s", calls) + } + }) + + t.Run("unavailable", func(t *testing.T) { + dir, _ := newStubPATH(t) + writeStub(t, dir, "psql", "exit 1") + if _, err := resolveSuperuser("127.0.0.1", "5432"); err == nil { + t.Fatal("expected an error") + } + }) +} + +func TestResolveSuperuser_RefusesRemoteHost(t *testing.T) { + dir, logPath := newStubPATH(t) + writeStub(t, dir, "psql", `echo psql >> "`+logPath+`"; exit 0`) + writeStub(t, dir, "sudo", `echo sudo >> "`+logPath+`"; exit 0`) + if _, err := resolveSuperuser("db.example.com", "5432"); err == nil || + !strings.Contains(err.Error(), "non-local") { + t.Fatalf("expected remote provisioning to be refused, got %v", err) + } + if calls := readCalls(t, logPath); calls != "" { + t.Fatalf("remote host must not invoke a local or sudo superuser:\n%s", calls) + } +} + +func TestSuperuserPSQL_SudoUsesPeerSocketPortAndNeverPrompts(t *testing.T) { + t.Setenv("PGHOST", "127.0.0.1") + t.Setenv("PGHOSTADDR", "127.0.0.1") + t.Setenv("PGSERVICE", "wrong-cluster") + cmd := (superuser{host: "127.0.0.1", port: "5544", sudo: true}).psql("-tAc", "select 1") + want := []string{ + "sudo", "-n", "-u", "postgres", "--", "psql", "-X", "-v", "ON_ERROR_STOP=1", "-w", + "-p", "5544", "-U", "postgres", "-d", "postgres", + "-tAc", "select 1", + } + if !reflect.DeepEqual(cmd.Args, want) { + t.Fatalf("sudo psql args = %#v, want %#v", cmd.Args, want) + } + for _, entry := range cmd.Env { + for _, key := range []string{"PGHOST=", "PGHOSTADDR=", "PGSERVICE="} { + if strings.HasPrefix(entry, key) { + t.Fatalf("sudo psql inherited target override %q", entry) + } + } + } +} + +func TestEnsureRole_ForcesScramPassword(t *testing.T) { + dir, _ := newStubPATH(t) + argsPath := filepath.Join(dir, "psql.args") + sqlPath := filepath.Join(dir, "psql.stdin") + writeStub(t, dir, "psql", `echo "$@" >> "`+argsPath+`"; /bin/cat >> "`+sqlPath+`"`) + su := superuser{host: "127.0.0.1", port: "5432"} + db := DBConfig{Name: "app", User: "mendix", Password: "s'ecret\\path"} + if err := ensureRole(su, db, io.Discard); err != nil { + t.Fatal(err) + } + args := readCalls(t, argsPath) + if strings.Contains(args, db.Password) { + t.Fatalf("role password must not be exposed in psql process arguments:\n%s", args) + } + if !strings.Contains(args, "-f -") { + t.Fatalf("role DDL should be read from standard input, args:\n%s", args) + } + sql := readCalls(t, sqlPath) + if !strings.Contains(sql, "PASSWORD 's''ecret\\path'") { + t.Fatalf("role password was not safely quoted in SQL read from stdin:\n%s", sql) + } + standardStringsAt := strings.Index(sql, "SET standard_conforming_strings = on") + setAt := strings.Index(sql, "SET password_encryption = 'scram-sha-256'") + createAt := strings.Index(sql, "CREATE ROLE mendix") + if standardStringsAt < 0 || standardStringsAt > setAt { + t.Fatalf("role creation must make password quoting independent of server settings:\n%s", sql) + } + if setAt < 0 || createAt < 0 || setAt > createAt { + t.Fatalf("role creation must force SCRAM before assigning the password:\n%s", sql) + } +} + +// writePostmasterPID writes a minimal postmaster.pid whose 4th line is the port, +// matching what a running server publishes (PID, data dir, start time, port, …). +func writePostmasterPID(t *testing.T, dataDir, port string) { + t.Helper() + home, _ := os.UserHomeDir() + writePostmasterPIDWithSocket(t, dataDir, port, filepath.Join(home, ".mxcli", "postgres", "sock")) +} + +func writePostmasterPIDWithSocket(t *testing.T, dataDir, port, sockDir string) { + t.Helper() + body := "12345\n" + dataDir + "\n1700000000\n" + port + "\n" + sockDir + "\n" + if err := os.WriteFile(filepath.Join(dataDir, "postmaster.pid"), []byte(body), 0o600); err != nil { + t.Fatal(err) + } +} + +// initClusterDir pre-creates an initialized data directory under $HOME so the +// initdb branch is skipped. +func initClusterDir(t *testing.T) string { + t.Helper() + home, _ := os.UserHomeDir() + dataDir := filepath.Join(home, ".mxcli", "postgres", "data") + if err := os.MkdirAll(dataDir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dataDir, "PG_VERSION"), []byte("16\n"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dataDir, "postgresql.conf"), []byte("# PostgreSQL configuration\n"), 0o600); err != nil { + t.Fatal(err) + } + hba := "local all all trust\n" + + "host all all 127.0.0.1/32 scram-sha-256\n" + + "host all all ::1/128 scram-sha-256\n" + if err := os.WriteFile(filepath.Join(dataDir, "pg_hba.conf"), []byte(hba), 0o600); err != nil { + t.Fatal(err) + } + return dataDir +} + +// --- #823 review: the dead pg_ctlcluster placeholder is gone --- + +// The old attempts list carried {"pg_ctlcluster", "--", "start"} — a placeholder +// whose args can never start a cluster, so it only burned a readiness timeout. +// Even when present on PATH it must never be executed now. +func TestStartLocalPostgres_NeverRunsPgCtlCluster(t *testing.T) { + dir, logPath := newStubPATH(t) + port := unusedTCPPort(t) + old := serviceReadyTimeout + serviceReadyTimeout = 20 * time.Millisecond + t.Cleanup(func() { serviceReadyTimeout = old }) + // No `service` on PATH; a pg_ctlcluster that would log if ever called. + writeStub(t, dir, "pg_ctlcluster", `echo pg_ctlcluster >> "`+logPath+`"; exit 0`) + writeStub(t, dir, "pg_isready", "exit 1") + writeStub(t, dir, "initdb", initdbStub(logPath)) + writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 3, 0)) + + if err := startLocalPostgres("127.0.0.1", port, io.Discard); err != nil { + t.Fatal(err) + } + calls := readCalls(t, logPath) + if strings.Contains(calls, "pg_ctlcluster") { + t.Fatalf("pg_ctlcluster must never run, got:\n%s", calls) + } + if !strings.Contains(calls, "pg_ctl_start") { + t.Fatalf("expected fallback to start a user cluster, got:\n%s", calls) + } +} + +// A failing service manager's own output must reach the final error rather than +// being replaced by an unrelated initdb/pg_ctl error two steps later. +func TestStartLocalPostgres_SurfacesServiceOutput(t *testing.T) { + dir, _ := newStubPATH(t) + port := unusedTCPPort(t) + old := serviceReadyTimeout + serviceReadyTimeout = 20 * time.Millisecond + t.Cleanup(func() { serviceReadyTimeout = old }) + writeStub(t, dir, "service", `echo "postgresql.service is masked"; exit 1`) + writeStub(t, dir, "pg_isready", "exit 1") + // No initdb/pg_ctl on PATH, so the fallback fails — the error should still + // carry the service manager's message. + err := startLocalPostgres("127.0.0.1", port, io.Discard) + if err == nil { + t.Fatal("expected an error when neither a service nor the portable tools work") + } + if !strings.Contains(err.Error(), "postgresql.service is masked") { + t.Fatalf("service output not surfaced: %v", err) + } +} + +// --- #823 review: initdb hardens loopback TCP (scram), trust only on the socket --- + +func TestStartUserCluster_InitdbAuthArgs(t *testing.T) { + dir, logPath := newStubPATH(t) + port := unusedTCPPort(t) + argsPath := filepath.Join(dir, "initdb.args") + writeStub(t, dir, "initdb", `echo "$@" >> "`+argsPath+`"; `+initdbStub(logPath)) + writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 3, 0)) + + if err := startLocalPostgres("127.0.0.1", port, io.Discard); err != nil { + t.Fatal(err) + } + args := readCalls(t, argsPath) + if !strings.Contains(args, "--auth-host=scram-sha-256") { + t.Errorf("loopback TCP must not be trust; initdb args = %q", args) + } + if strings.Contains(args, "--auth-host=trust") { + t.Errorf("initdb must not set --auth-host=trust; args = %q", args) + } + if !strings.Contains(args, "--auth-local=trust") { + t.Errorf("socket connections should stay trust; args = %q", args) + } +} + +// The socket, listen address, and port must live in the cluster configuration, +// not only in pg_ctl -o flags, so a later plain `pg_ctl -D ... start` remains +// private and starts on the expected endpoint. +func TestStartUserCluster_PersistsSecureConnectionSettings(t *testing.T) { + dir, logPath := newStubPATH(t) + home, _ := os.UserHomeDir() + sockDir := filepath.Join(home, ".mxcli", "postgres", "sock") + if err := os.MkdirAll(sockDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.Chmod(sockDir, 0o755); err != nil { + t.Fatal(err) + } + writeStub(t, dir, "initdb", initdbStub(logPath)) + writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 3, 0)) + + if err := startUserCluster("127.0.0.1", "5544", io.Discard); err != nil { + t.Fatal(err) + } + dataDir := filepath.Join(home, ".mxcli", "postgres", "data") + info, err := os.Stat(sockDir) + if err != nil || info.Mode().Perm() != 0o700 { + t.Fatalf("socket directory = %v, %v; want mode 0700", info, err) + } + postgresConf, err := os.ReadFile(filepath.Join(dataDir, "postgresql.conf")) + if err != nil { + t.Fatal(err) + } + if got := strings.Count(string(postgresConf), mxcliPostgresConfigBegin); got != 1 { + t.Fatalf("postgresql.conf should contain one mxcli block, got %d:\n%s", got, postgresConf) + } + firstConfig := string(postgresConf) + for _, want := range []string{ + "listen_addresses = '127.0.0.1'", + "port = 5544", + "unix_socket_directories = '" + filepath.Join(home, ".mxcli", "postgres", "sock") + "'", + "unix_socket_permissions = 0700", + } { + if !strings.Contains(string(postgresConf), want) { + t.Errorf("postgresql.conf missing %q:\n%s", want, postgresConf) + } + } + info, err = os.Stat(filepath.Join(dataDir, "postgresql.conf")) + if err != nil { + t.Fatal(err) + } + if got := info.Mode().Perm(); got != 0o600 { + t.Fatalf("postgresql.conf mode = %04o, want 0600", got) + } + + // Reapplying the configuration must replace, not duplicate, the managed block. + if err := startUserCluster("127.0.0.1", "5544", io.Discard); err != nil { + t.Fatal(err) + } + postgresConf, err = os.ReadFile(filepath.Join(dataDir, "postgresql.conf")) + if err != nil { + t.Fatal(err) + } + if got := strings.Count(string(postgresConf), mxcliPostgresConfigBegin); got != 1 { + t.Fatalf("repeated configuration added duplicate managed blocks: %d\n%s", got, postgresConf) + } + if string(postgresConf) != firstConfig { + t.Fatalf("repeated configuration was not byte-stable:\n%s", postgresConf) + } +} + +// A cluster initialized by the pre-review branch used host trust. Reuse must +// fail safely with a cleanup path instead of silently retaining that bypass. +func TestStartUserCluster_RejectsLegacyHostTrust(t *testing.T) { + for _, record := range []string{ + "host all all 127.0.0.1/32 trust", + "host all all 127.0.0.1 255.255.255.255 trust", + } { + t.Run(record, func(t *testing.T) { + _, _ = newStubPATH(t) + dataDir := initClusterDir(t) + hba := "local all all trust\n" + record + "\n" + if err := os.WriteFile(filepath.Join(dataDir, "pg_hba.conf"), []byte(hba), 0o600); err != nil { + t.Fatal(err) + } + err := startUserCluster("127.0.0.1", "5432", io.Discard) + if err == nil || !strings.Contains(err.Error(), "unsafe host trust") || + !strings.Contains(err.Error(), "remove ~/.mxcli/postgres") { + t.Fatalf("expected an actionable host-trust error, got %v", err) + } + }) + } +} + +func TestStartUserCluster_RefusesNonLocalHost(t *testing.T) { + if err := startUserCluster("0.0.0.0", "5432", io.Discard); err == nil || + !strings.Contains(err.Error(), "non-local host") { + t.Fatalf("expected a non-local bind refusal, got %v", err) + } +} + +// The direct superuser must reach the cluster over its private trust socket, not +// over loopback TCP (which is now scram-sha-256 and would reject a passwordless +// connection). +func TestResolveSuperuser_PrefersClusterSocket(t *testing.T) { + dir, logPath := newStubPATH(t) + writeStub(t, dir, "psql", `echo "$@" >> "`+logPath+`"`) // exits 0 + writeStub(t, dir, "sudo", `echo sudo >> "`+logPath+`"`) + + su, err := resolveSuperuser("127.0.0.1", "5432") + if err != nil { + t.Fatal(err) + } + home, _ := os.UserHomeDir() + sockDir := filepath.Join(home, ".mxcli", "postgres", "sock") + if su.sock != sockDir { + t.Fatalf("superuser should use the cluster socket %q, got %+v", sockDir, su) + } + if calls := readCalls(t, logPath); !strings.Contains(calls, "-h "+sockDir) { + t.Fatalf("psql should connect over the socket dir, calls:\n%s", calls) + } +} + +// --- #823 review: an empty host normalises to loopback in the server config --- + +func TestStartUserCluster_EmptyHostNormalised(t *testing.T) { + dir, logPath := newStubPATH(t) + initClusterDir(t) // skip initdb; status=3 => not running => start + writeStub(t, dir, "pg_ctl", `last=""; for a in "$@"; do last="$a"; done +[ "$last" = status ] && exit 3 +echo pg_ctl_start >> "`+logPath+`" +exit 0`) + + if err := startUserCluster("", "5432", io.Discard); err != nil { + t.Fatal(err) + } + home, _ := os.UserHomeDir() + conf, err := os.ReadFile(filepath.Join(home, ".mxcli", "postgres", "data", "postgresql.conf")) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(conf), "listen_addresses = '127.0.0.1'") { + t.Fatalf("empty host should become 127.0.0.1 in persisted config:\n%s", conf) + } +} + +// --- #823 review: a running cluster is only reused when its port matches --- + +func TestStartUserCluster_RunningPortGuard(t *testing.T) { + t.Run("matching port is left alone", func(t *testing.T) { + dir, logPath := newStubPATH(t) + port := unusedTCPPort(t) + dataDir := initClusterDir(t) + writePostmasterPID(t, dataDir, port) + writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 0, 0)) // status=0 => running + + if err := startLocalPostgres("127.0.0.1", port, io.Discard); err != nil { + t.Fatal(err) + } + if calls := readCalls(t, logPath); strings.Contains(calls, "pg_ctl_start") { + t.Fatalf("a matching running cluster must not be restarted:\n%s", calls) + } + }) + + t.Run("mismatched port errors instead of waiting", func(t *testing.T) { + dir, logPath := newStubPATH(t) + port := unusedTCPPort(t) + portNumber, err := strconv.Atoi(port) + if err != nil { + t.Fatal(err) + } + livePort := strconv.Itoa(portNumber%65535 + 1) + dataDir := initClusterDir(t) + writePostmasterPID(t, dataDir, livePort) + configPath := filepath.Join(dataDir, "postgresql.conf") + beforeConfig, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 0, 0)) // status=0 => running + + err = startLocalPostgres("127.0.0.1", port, io.Discard) + if err == nil { + t.Fatal("expected an error when a cluster runs on a different port") + } + if !strings.Contains(err.Error(), livePort) || !strings.Contains(err.Error(), port) { + t.Fatalf("error should name both the live and requested ports: %v", err) + } + if strings.Contains(err.Error(), "--db-port") || !strings.Contains(err.Error(), "--db-host") { + t.Fatalf("error should recommend the real --db-host flag: %v", err) + } + if calls := readCalls(t, logPath); strings.Contains(calls, "pg_ctl_start") { + t.Fatalf("a wrong-port cluster must not be restarted:\n%s", calls) + } + afterConfig, readErr := os.ReadFile(configPath) + if readErr != nil { + t.Fatal(readErr) + } + if string(afterConfig) != string(beforeConfig) { + t.Fatalf("a rejected endpoint must not rewrite postgresql.conf:\n%s", afterConfig) + } + }) + + t.Run("unknown running port errors", func(t *testing.T) { + dir, logPath := newStubPATH(t) + initClusterDir(t) + writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 0, 0)) // running, no readable pid file + + err := startUserCluster("127.0.0.1", "5432", io.Discard) + if err == nil || !strings.Contains(err.Error(), "port could not be read") { + t.Fatalf("expected an unknown-port error, got %v", err) + } + }) + + t.Run("unexpected socket directory errors", func(t *testing.T) { + dir, logPath := newStubPATH(t) + dataDir := initClusterDir(t) + writePostmasterPIDWithSocket(t, dataDir, "5432", "/tmp") + writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 0, 0)) // running on an unsafe socket + + err := startUserCluster("127.0.0.1", "5432", io.Discard) + if err == nil { + t.Fatal("expected an error for a cluster using an unexpected socket directory") + } + if !strings.Contains(err.Error(), "/tmp") || !strings.Contains(err.Error(), "stop") { + t.Fatalf("error should name the unsafe socket and how to restart safely: %v", err) + } + }) +} + +// --- #823 review: the start error names the server log --- + +func TestStartUserCluster_StartErrorNamesLog(t *testing.T) { + dir, logPath := newStubPATH(t) + port := unusedTCPPort(t) + initClusterDir(t) + writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 3, 1)) // not running, start fails + + err := startLocalPostgres("127.0.0.1", port, io.Discard) + if err == nil { + t.Fatal("expected a start failure") + } + if !strings.Contains(err.Error(), "server.log") { + t.Fatalf("start error should name the server log: %v", err) + } } diff --git a/cmd/mxcli/docker/localapp.go b/cmd/mxcli/docker/localapp.go index 056c10510..2ff1a70a9 100644 --- a/cmd/mxcli/docker/localapp.go +++ b/cmd/mxcli/docker/localapp.go @@ -171,7 +171,7 @@ func StartLocalApp(opts LocalAppOptions) (*LocalApp, error) { // 3. Database. if opts.EnsureDB { - if err := EnsureDatabase(opts.DB, w); err != nil { + if err := EnsureDatabase(&opts.DB, w); err != nil { return nil, fmt.Errorf("ensuring database: %w", err) } } else if err := pingTCP(opts.DB.Host, 3*time.Second); err != nil { diff --git a/cmd/mxcli/docker/runlocal.go b/cmd/mxcli/docker/runlocal.go index cff1e44bc..47aaa61a9 100644 --- a/cmd/mxcli/docker/runlocal.go +++ b/cmd/mxcli/docker/runlocal.go @@ -607,7 +607,7 @@ func RunLocal(opts LocalRunOptions) error { // reachability and point the user at --ensure-db. if opts.EnsureDB { fmt.Fprintln(w, "Ensuring database...") - if err := EnsureDatabase(opts.DB, w); err != nil { + if err := EnsureDatabase(&opts.DB, w); err != nil { return fmt.Errorf("ensuring database: %w", err) } } else if err := pingTCP(opts.DB.Host, 3*time.Second); err != nil { diff --git a/docs-site/src/tools/run-local.md b/docs-site/src/tools/run-local.md index 6b13f2b77..e64ba5824 100644 --- a/docs-site/src/tools/run-local.md +++ b/docs-site/src/tools/run-local.md @@ -47,9 +47,15 @@ so structural changes need a restart; behavioural changes do not. - A **PostgreSQL** database. Defaults: `127.0.0.1:5432`, user `mendix`, database derived from the project file name (`App1112.mpr` → `app1112`). Two ways to have it: - **`--ensure-db`** (recommended for a fresh session) provisions it: starts the - local Postgres service if the port is down, and creates the app role + database - if missing (via a local `sudo -u postgres` superuser). For a non-local `--db-host` - it only verifies reachability — mxcli won't provision a remote database. + local Postgres server if the port is down, and creates the app role + database + if missing. It uses a service manager, or a user-owned `initdb`/`pg_ctl` cluster + under `~/.mxcli/postgres` when no service becomes ready (e.g. Arch) — no + `postgres` OS account or `sudo` required. For a non-local `--db-host` it only verifies + reachability — mxcli won't provision a remote database. + The user-owned cluster persists across sessions; its server log is + `~/.mxcli/postgres/server.log`. Stop it with + `pg_ctl -D "$HOME/.mxcli/postgres/data" stop`. To remove it, stop it first and + then delete `~/.mxcli/postgres` (this permanently deletes its databases). - Otherwise create it once yourself; without `--ensure-db`, `run --local` stops with an actionable message if the DB is unreachable: @@ -71,7 +77,7 @@ so structural changes need a restart; behavioural changes do not. | `--app-port` | 8080 | App HTTP port | | `--admin-port` | 8090 | M2EE admin API port | | `--serve-port` | 6543 | `mxbuild --serve` port | -| `--db-host` | 127.0.0.1:5432 | Database `host:port` | +| `--db-host` | 127.0.0.1:5432 | Database `host:port`; bracket IPv6 endpoints (`[::1]:5432`) | | `--db-name` | derived from project | Database name | | `--db-user` / `--db-password` | mendix / mendix | Database credentials | | `--screenshot` | off | Capture a Playwright PNG after boot and each applied change |