From 2afab5c81fdaea579f806c7253b190e722c6317a 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. Target sudo/psql at the requested endpoint without prompts, 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 | 510 ++++++++++++++-- cmd/mxcli/docker/ensuredb_test.go | 733 ++++++++++++++++++++++- cmd/mxcli/docker/localapp.go | 2 +- cmd/mxcli/docker/runlocal.go | 2 +- docs-site/src/tools/run-local.md | 14 +- 9 files changed, 1214 insertions(+), 63 deletions(-) diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index db9e64e5c..37293bfbf 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -612,3 +612,4 @@ extracting `OffsetExpression`/`LimitExpression`. | A microflow rewritten by `CREATE OR MODIFY MICROFLOW` is drawn with its **StartEvent stranded** — every activity where the script asked, the start left behind, joined to its own first activity by a long mostly-empty line across the canvas. Reproduces on every rewrite, and copying a working script's `@position` pattern does not help | The fix for the OPPOSITE report. #884 was a describe→exec round-trip MOVING a hand-placed start (145;200 → 100;200), fixed by carrying the stored position over on every rewrite; that then **pinned** the start of every rewritten flow, so it no longer followed activities the same script had just moved. Both reports are real and neither is answerable without asking where the stored value came from | `mdl/executor/cmd_microflows_start_position.go` (`authoredStartPosition`, `derivedStartPosition`, `startAnnotationLines`), `mdl/executor/cmd_microflows_create.go` (`storedStartPosition`), `mdl/executor/cmd_microflows_builder_graph.go` (StartEvent construction) | Carry the stored position over **only when it is not where the layout would have put it** — a start at `first.X − spacing` on `first.Y` is mxcli's own arithmetic handed back and carries no intent, so re-derive it; anywhere else, a person put it there, so keep it. Add `@start(x, y)` on the first statement (the `@merge` precedent — it positions the other node with no statement of its own) so the position can be *stated* rather than inferred, and have DESCRIBE emit it **only for a non-derived start**, which is what makes the round-trip exact without pinning every described flow. **`mx check` is not the oracle, and neither is a green build**: the Mendix model carries no geometry rules, so a stranded start is a valid document — 0 errors on mxbuild 11.13.0 before and after. Verify by reading the coordinates back off the stored document. **Emit from BOTH describers**: `formatMicroflowActivities` and `formatMicroflowActivitiesWithSourceMap` are near-duplicates, and the first cut of this fix patched only the second, so `describe microflow` silently dropped the line. Controls: reverting the narrowing reproduces the stranded start, reverting the builder arm reproduces "@start does nothing", reverting either describer reproduces the lossy round-trip. Repro `mdl-examples/bug-tests/951-microflow-start-event-position.mdl`. Issue #951 | | `SPLIT TYPE … END SPLIT` followed by any statement draws that statement **on top of the merge** in Studio Pro, joined by a zero-length sequence flow, and the merge itself sits far to the right of the branches it joins (branches at x=720, merge at x=1480). `CASE` and `IF/ELSE` with the same graph shape lay out correctly. `mx check` reports 0 errors either way | Two defects in `addStructuredInheritanceSplit`. (1) It ended with `fb.posX = mergeX` — the merge's own centre — where `addEnumSplit` steps to `mergeX + HorizontalSpacing/2` and `addIfStatement` to `mergeX + MergeSize + HorizontalSpacing/2`; three builders, three conventions, and the type split's was zero. (2) Branch width came from `measureStatements(appendInheritanceBodies(s))`, which concatenated every branch body into ONE list and measured it as a single left-to-right run — so the merge slid right by an activity-plus-spacing per **extra branch**, not per branch *width*. `layout.go`'s own `measureInheritanceSplitStatement` always took the max, so the builder disagreed with its measurer | `mdl/executor/cmd_microflows_builder_actions.go` (`addStructuredInheritanceSplit`; `appendInheritanceBodies` → `inheritanceBranchBodies`); tests `mdl/executor/cmd_microflows_builder_split_geometry_test.go`; repro `mdl-examples/bug-tests/953-split-type-merge-overlap.mdl` | **Geometry is invisible to every automatic check below Studio Pro** — the model is valid, so `mx check` passes, the build passes, and `describe` output looks right until you read the coordinates. `describe` printing `@position`/`@merge` IS the headless oracle: diff those, don't open the modeler. **The split builders had zero positional coverage** before this (grep `Position` in the enum-split and inheritance-split tests: nothing), which is the same gap behind the loop-box sizing bugs #790 and #884. **Pick the IF convention, not the CASE one**: the constants are centre-to-centre for a 40px edge gap, and `CASE`'s `+HorizontalSpacing/2` leaves a following activity's left edge exactly touching the merge (measured: merge 890, activity 970, both edges at 910) — a lesser pre-existing nit deliberately left alone, because changing it re-lays-out every enum split ever written. **`@merge(x,y)` was not a workaround**: `fb.posX = mergeX` read the overridden value, so moving the merge by hand moved the stacked element with it. A fix needs a control that the neighbouring construct did NOT move — `TestEnumSplitGeometryIsUnchanged` — or "the type split matches CASE now" can be achieved by breaking CASE. Issue #953 | | A bundled skill renders with a stray `---` fence and a duplicated `name:` at the top of its body, and `mxcli init` ships it that way. `TestEmbeddedSkillsCarryAgentSkillsFrontmatter` passes — the frontmatter it checks is valid | The #906 migration **prepended** a `name`/`description` block to every skill, including the one file that already had one (`custom-widgets`, under its pre-rename name `mendix-custom-widgets`). Everything after the FIRST `---` block is body, so the old block became visible text. The test's `frontmatter.FindSubmatch` matches the first block and validates it correctly, which is precisely why it is blind to a second one | `.claude/skills/mendix/custom-widgets/SKILL.md` (two blocks merged into one, keeping the old block's trigger words — child slots, `.def.json`, engine internals); guard added to `cmd/mxcli/init_skills_standard_test.go` | **A first-block check cannot see a rename-and-prepend**, and that is the exact shape of a migration that renames things — so a bulk frontmatter edit needs a guard against a SECOND block, not a better check of the first. **Measure the pre-state before a bulk prepend**: 1 of 68 files already had frontmatter, which was known and written down, and the migration script still did not branch on it. **Do not attribute a defect to the change you are looking at** — this was reported against PR #222, whose diff touches only the `description:` inside the first block and shows the stray `---` as unchanged context; `git log -S 'name: mendix-custom-widgets'` settles it in one command. Two adjacent traps from the same report: `make test` did not depend on `sync-all` while `make build` did, so a bare `go build` left the generated `cmd/mxcli/skills` stale and six tests failed — and the failure message named the `go:embed` directive, which was fine, instead of the missing build step. A **differential across refs is only as sound as the procedure both sides share**: the same stale build made main fail 4 and the branch fail 6, which looked like a regression and was a test-count difference. mxcli-formula1 finding 68 | +| `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, pass the exact host and 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 fc5f35428..ab4acf753 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,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, targets the exact requested host and 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. ## [0.19.0] - 2026-08-21 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..74882bf2a 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,107 @@ 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 the original `sudo -u postgres psql` for a system cluster. +type superuser struct { + host, port string + sock string // Unix-socket dir for the user-owned cluster; preferred over TCP + sudo bool +} + +// 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. -w never prompts, so probing an unrelated + // password-protected system cluster fails fast instead of hanging. + host := s.host + if s.sock != "" { + host = s.sock + } + base := []string{"-X", "-v", "ON_ERROR_STOP=1", "-w", + "-h", host, "-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. Pass the requested + // endpoint explicitly so provisioning cannot fall through to another + // cluster's default socket. + sudoBase := []string{"-n", "-u", "postgres", "--", "psql"} + return exec.Command("sudo", append(sudoBase, append(base, args...)...)...) + } + 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..bc9f88d04 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,700 @@ 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, _ := newStubPATH(t) + writeStub(t, dir, "psql", "exit 1") + writeStub(t, dir, "sudo", "exit 0") + su, err := resolveSuperuser("127.0.0.1", "5432") + if err != nil || !su.sudo { + t.Fatalf("su=%+v err=%v", su, err) + } + }) + + 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_SudoUsesEndpointAndNeverPrompts(t *testing.T) { + 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", + "-h", "127.0.0.1", "-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) + } +} + +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 |