diff --git a/docs/features/project-setup.md b/docs/features/project-setup.md index c8fc30d31..2977f0431 100644 --- a/docs/features/project-setup.md +++ b/docs/features/project-setup.md @@ -177,5 +177,8 @@ If a step fails, you are prompted to continue or abort: `--list-steps` and `--step` are how a caller with no terminal works through the same list the selector shows: the dashboard's site wizard enumerates the steps for a project, shows them as checkboxes, and runs the ticked ones one at a time. -Naming a step this directory does not offer fails with the list it does offer, -so a stale plan is reported rather than half-run. +The plan is re-derived on every invocation, and its steps are gated on live +state: whether the site is already secured, what a worker scan finds, a file the +previous step wrote. So a step named here that the plan no longer offers is +work that no longer needs doing, and it is reported as such and passed over +rather than failing the run and stopping the rest of a queued list. diff --git a/docs/usage/framework-definitions.md b/docs/usage/framework-definitions.md index 52e9b6ba4..4d5bb8902 100644 --- a/docs/usage/framework-definitions.md +++ b/docs/usage/framework-definitions.md @@ -101,9 +101,26 @@ A service a project picks in its `.lerd.yaml` is expected to appear in the env f ### Offering SQLite -A framework that can run on a file database declares a `sqlite` service alongside its others, and that declaration is what puts SQLite in the database choice at `lerd init`. A framework declaring none is never offered it, since picking it would configure a project for a database its application cannot open. A project lerd recognises no framework for keeps the option: nothing has declared otherwise. +A framework that can run on a file database declares an `env.sqlite` block, and that declaration is what puts SQLite in the database choice at `lerd init`. It takes the same `detect` and `vars` a service mapping takes: the detect rules say a project is already on a file database, the vars are what lerd writes to point it at one. A framework declaring none is never offered it, since picking it would configure a project for a database its application cannot open. A project lerd recognises no framework for keeps the option: nothing has declared otherwise. -Choosing it records nothing in `.lerd.yaml`. SQLite has no preset, no container and nothing to install, so a `services:` entry for it is one every surface then has to explain away, and the project's own configuration already says it is on SQLite, which is what lerd reads to answer that question. An entry left by an older lerd is ignored where it is found. +```yaml +env: + file: .env + sqlite: + detect: + - key: DB_CONNECTION + value_prefix: sqlite + vars: + - DB_CONNECTION=sqlite + - DB_DATABASE=database/database.sqlite + services: + mysql: + # … +``` + +It sits beside `services:` rather than among them because SQLite is not a service. Nothing installs it, starts it or draws a card for it, and a binary that read it as a service entry would announce it and then try to start a container that does not exist. That is also why it is a separate field rather than a `services:` key: a published definition reaches every install within a day, whatever version it runs, and an unknown field is ignored while an unknown service is not. + +Choosing it records nothing in `.lerd.yaml`, for the same reason: the project's own configuration already says it is on SQLite, which is what lerd reads to answer that question. An entry left by an older lerd is ignored where it is found. ### Drop-in services @@ -163,6 +180,14 @@ env: command: key:generate fallback_prefix: "base64:" + sqlite: # wiring for a file database (optional). Not a + detect: # service: nothing installs, starts or draws it. + - key: DB_CONNECTION # Declaring it is what offers SQLite at `lerd init`. + value_prefix: sqlite + vars: + - "DB_CONNECTION=sqlite" + - "DB_DATABASE=database/database.sqlite" + # Per-service env detection and variable injection for `lerd env` # # Template variables available in vars values: diff --git a/internal/cli/env.go b/internal/cli/env.go index 4d6460186..9ddd1e84c 100644 --- a/internal/cli/env.go +++ b/internal/cli/env.go @@ -23,6 +23,7 @@ import ( phpDet "github.com/geodro/lerd/internal/php" "github.com/geodro/lerd/internal/podman" "github.com/geodro/lerd/internal/serviceops" + "github.com/geodro/lerd/internal/sitedoctor" "github.com/geodro/lerd/internal/sitetpl" "github.com/spf13/cobra" ) @@ -461,8 +462,10 @@ func projectUsesSQLite(lerdYAMLServices map[string]bool, envMap map[string]strin if fw == nil || externalDB || userPickedDBFromYAML(lerdYAMLServices) { return false } - def, ok := fw.Env.Services["sqlite"] - return ok && frameworkServiceDetected(def, envMap) + if fw.Env.SQLite == nil { + return false + } + return frameworkServiceDetected(*fw.Env.SQLite, envMap) } func userPickedDBFromYAML(lerdYAMLServices map[string]bool) bool { @@ -908,23 +911,15 @@ func runEnv(_ *cobra.Command, _ []string) error { } // 3a-bis. SQLite is not a containerized service but is a valid choice from - // the init wizard / runtime DB prompt. Apply the framework's sqlite env vars - // and ensure the database file exists so migrations can run immediately. No - // service to start, no SQL DB to create. - if projectUsesSQLite(lerdYAMLServices, envMap, fw, externalDBPicked(extServices)) { + // the init wizard / runtime DB prompt. Apply the framework's sqlite env vars; + // the database file itself is created after step 4e, once every value that + // can name it, the personal override included, has had its say. + sqliteWired := projectUsesSQLite(lerdYAMLServices, envMap, fw, externalDBPicked(extServices)) + if sqliteWired { envApplyLine("sqlite", !lerdYAMLServices["sqlite"]) - for _, kv := range serviceEnvVars("sqlite") { + for _, kv := range sqliteVarsFor(fw) { k, v, _ := strings.Cut(kv, "=") - updates[k] = v - } - sqlitePath := filepath.Join(cwd, "database", "database.sqlite") - if _, statErr := os.Stat(sqlitePath); os.IsNotExist(statErr) { - if err := os.MkdirAll(filepath.Dir(sqlitePath), 0o755); err == nil { - if f, err := os.Create(sqlitePath); err == nil { - _ = f.Close() - envInfo(" Created %s\n", filepath.Join("database", "database.sqlite")) - } - } + updates[k] = applySiteHandle(v, tplCtx) } } @@ -1079,6 +1074,26 @@ func runEnv(_ *cobra.Command, _ []string) error { } } + // The SQLite file is created here, not at 3a-bis, so it is the one the + // final values name: an override pointing the database somewhere else, or a + // DSN like Symfony's naming var/data.db, must not leave an empty stray file + // at a default path while the file the application opens is still missing. + // Where it lands follows the doctor's own resolution rules. + if sqliteWired { + if rel, ok := sitedoctor.SQLiteFileFromValues(updates, fw); ok { + if target, create := sitedoctor.SQLiteCreationTarget(cwd, fw, filepath.FromSlash(rel)); create { + if err := os.MkdirAll(filepath.Dir(target), 0o755); err == nil { + if f, err := os.Create(target); err == nil { + _ = f.Close() + if shown, relErr := filepath.Rel(cwd, target); relErr == nil { + envInfo(" Created %s\n", shown) + } + } + } + } + } + } + // 5. Rewrite the env file preserving order, comments, and blank lines. // Whether the connection actually moved is worked out before writing, by // comparing what is about to be written against what the file already diff --git a/internal/cli/env_test.go b/internal/cli/env_test.go index 0f0caf7eb..5d9969eab 100644 --- a/internal/cli/env_test.go +++ b/internal/cli/env_test.go @@ -839,16 +839,18 @@ func TestWiredVarsFor_ExternalUnmappedKeepsThePresetKeys(t *testing.T) { // the file itself, and the first request 500'd on a database that was never // created. The project's own configuration is the signal instead. func TestProjectUsesSQLite(t *testing.T) { - laravelish := &config.Framework{Env: config.FrameworkEnvConf{Services: map[string]config.FrameworkServiceDef{ - "sqlite": { + laravelish := &config.Framework{Env: config.FrameworkEnvConf{ + SQLite: &config.FrameworkServiceDef{ Detect: []config.FrameworkServiceDetect{{Key: "DB_CONNECTION", ValuePrefix: "sqlite"}}, Vars: []string{"DB_CONNECTION=sqlite", "DB_DATABASE=database/database.sqlite"}, }, - "mysql": { - Detect: []config.FrameworkServiceDetect{{Key: "DB_CONNECTION", ValuePrefix: "mysql"}}, - Vars: []string{"DB_CONNECTION=mysql", "DB_HOST=lerd-mysql"}, + Services: map[string]config.FrameworkServiceDef{ + "mysql": { + Detect: []config.FrameworkServiceDetect{{Key: "DB_CONNECTION", ValuePrefix: "mysql"}}, + Vars: []string{"DB_CONNECTION=mysql", "DB_HOST=lerd-mysql"}, + }, }, - }}} + }} sqliteEnv := map[string]string{"DB_CONNECTION": "sqlite"} for _, tc := range []struct { diff --git a/internal/cli/init.go b/internal/cli/init.go index 194b5ef93..26da50244 100644 --- a/internal/cli/init.go +++ b/internal/cli/init.go @@ -500,6 +500,9 @@ func runCustomContainerWizard(cwd string, defaults *config.ProjectConfig, gcfg * Container: containerCfg, AppURL: defaults.AppURL, Domains: defaults.Domains, + // This wizard never asks about Node, so a pin the project carries is + // not its to drop. + NodeVersion: defaults.NodeVersion, }, nil } @@ -730,15 +733,14 @@ func persistedServices(dbChoice string, nonDB []string) []string { } // frameworkSupportsSQLite reports whether a framework can be wired to a file -// database, which the definition says by declaring a sqlite service alongside -// its others. A project with no framework at all keeps the option: nothing has -// declared otherwise, and lerd should not decide for it. +// database, which the definition says with its own sqlite wiring. A project +// with no framework at all keeps the option: nothing has declared otherwise, +// and lerd should not decide for it. func frameworkSupportsSQLite(fw *config.Framework) bool { - if fw == nil || len(fw.Env.Services) == 0 { + if fw == nil || !fw.HasEnvConfig() { return true } - _, ok := fw.Env.Services["sqlite"] - return ok + return fw.Env.SQLite != nil } func formatDBOptionLabel(name string) string { diff --git a/internal/cli/project_questions.go b/internal/cli/project_questions.go index 4191986bc..7fb677bf3 100644 --- a/internal/cli/project_questions.go +++ b/internal/cli/project_questions.go @@ -454,12 +454,23 @@ func SaveProjectAnswers(cwd string, answers ProjectAnswers) error { // carrying over the parts of an existing config the questions never ask about // (the public dir, the app URL, extra domains, custom workers). func projectConfigFromAnswers(cwd string, defaults *config.ProjectConfig, a ProjectAnswers, httpsAvailable bool) (*config.ProjectConfig, error) { + // An empty answer means two different things. The dashboard only renders + // the Node question for a PHP project on a machine where lerd manages Node, + // and there it offers a "Not pinned" entry whose value is exactly this empty + // string, so empty is the user clearing the pin. Everywhere else, proxy and + // container kinds included, the question was never asked and empty must not + // erase what the project pins. + nodeVersion := a.NodeVersion + if nodeVersion == "" && !(a.Kind == ProjectKindPHP && lerdManagesNode()) { + nodeVersion = defaults.NodeVersion + } + cfg := &config.ProjectConfig{ PublicDir: defaults.PublicDir, Secured: persistedSecured(a.Secured, httpsAvailable, defaults.Secured), AppURL: defaults.AppURL, Domains: defaults.Domains, - NodeVersion: a.NodeVersion, + NodeVersion: nodeVersion, CustomWorkers: defaults.CustomWorkers, } @@ -476,9 +487,6 @@ func projectConfigFromAnswers(cwd string, defaults *config.ProjectConfig, a Proj cfg.Proxy.HostEnvKey = defaults.Proxy.HostEnvKey cfg.Proxy.InjectHost = defaults.Proxy.InjectHost } - if cfg.NodeVersion == "" { - cfg.NodeVersion = defaults.NodeVersion - } return cfg, nil case ProjectKindContainer: @@ -496,9 +504,6 @@ func projectConfigFromAnswers(cwd string, defaults *config.ProjectConfig, a Proj cfg.Container.Target = defaults.Container.Target cfg.Container.SSL = defaults.Container.SSL } - if cfg.NodeVersion == "" { - cfg.NodeVersion = defaults.NodeVersion - } return cfg, nil } diff --git a/internal/cli/project_questions_test.go b/internal/cli/project_questions_test.go index 496b48007..401d8634a 100644 --- a/internal/cli/project_questions_test.go +++ b/internal/cli/project_questions_test.go @@ -211,3 +211,81 @@ func TestSaveProjectAnswersRejectsProxyWithoutPort(t *testing.T) { t.Fatal("a proxy answer with no port should be refused") } } + +// A pinned Node version is the project's, not the wizard's to drop. Where lerd +// does not manage Node the question is never filled in, so the empty answer that +// comes back must not erase what .lerd.yaml already pins. +func TestProjectConfigFromAnswersKeepsAPinnedNodeVersion(t *testing.T) { + setNodeManaged(t, false) + defaults := &config.ProjectConfig{NodeVersion: "20"} + + for _, a := range []ProjectAnswers{ + {Kind: ProjectKindPHP, PHPVersion: "8.3"}, + {Kind: ProjectKindProxy, ProxyCommand: "npm run dev", ProxyPort: 5173}, + {Kind: ProjectKindContainer, ContainerPort: 8080}, + } { + cfg, err := projectConfigFromAnswers(t.TempDir(), defaults, a, true) + if err != nil { + t.Fatalf("%s: %v", a.Kind, err) + } + if cfg.NodeVersion != "20" { + t.Errorf("%s: node_version = %q, want the pin kept", a.Kind, cfg.NodeVersion) + } + } +} + +// setNodeManaged persists the Node-management choice the wizard reads, so a test +// can put itself on either side of what an empty answer means. +func setNodeManaged(t *testing.T, managed bool) { + t.Helper() + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + t.Setenv("XDG_DATA_HOME", t.TempDir()) + cfg, err := config.LoadGlobal() + if err != nil { + t.Fatal(err) + } + cfg.SetNodeManaged(managed) + if err := config.SaveGlobal(cfg); err != nil { + t.Fatal(err) + } +} + +// Where lerd manages Node the wizard offers an unpinned entry whose value is the +// empty string, so an empty answer is the user clearing the pin and has to be +// saved as one. Restoring the old value would leave no way to unpin from the +// dashboard at all. +func TestProjectConfigFromAnswersHonoursClearingTheNodePin(t *testing.T) { + setNodeManaged(t, true) + defaults := &config.ProjectConfig{NodeVersion: "20"} + + cfg, err := projectConfigFromAnswers(t.TempDir(), defaults, ProjectAnswers{ + Kind: ProjectKindPHP, PHPVersion: "8.3", NodeVersion: "", + }, true) + if err != nil { + t.Fatal(err) + } + if cfg.NodeVersion != "" { + t.Errorf("node_version = %q, want the pin cleared", cfg.NodeVersion) + } +} + +// The dashboard renders the Node question only for the PHP kind, so a proxy or +// container answer arrives empty even on a machine where lerd manages Node. +// That empty is nobody having been asked, and the pin stays. +func TestProjectConfigFromAnswersKeepsThePinWhereTheQuestionIsNotAsked(t *testing.T) { + setNodeManaged(t, true) + defaults := &config.ProjectConfig{NodeVersion: "22"} + + for _, a := range []ProjectAnswers{ + {Kind: ProjectKindProxy, ProxyCommand: "npm run dev", ProxyPort: 5173}, + {Kind: ProjectKindContainer, ContainerPort: 8080}, + } { + cfg, err := projectConfigFromAnswers(t.TempDir(), defaults, a, true) + if err != nil { + t.Fatalf("%s: %v", a.Kind, err) + } + if cfg.NodeVersion != "22" { + t.Errorf("%s: node_version = %q, want the pin kept", a.Kind, cfg.NodeVersion) + } + } +} diff --git a/internal/cli/services.go b/internal/cli/services.go index cbcb1c120..24bc92322 100644 --- a/internal/cli/services.go +++ b/internal/cli/services.go @@ -27,15 +27,24 @@ import ( // Backed by the preset YAMLs so adding a default preset surfaces here automatically. func knownServices() []string { return config.DefaultPresetNames() } -// sqliteEnvVars are the Laravel-standard env values for the sqlite "service" -// (which isn't a podman container — just a per-project file). Kept hardcoded -// because there's no preset YAML to host it: sqlite has no image, no port, -// and no install flow. +// sqliteEnvVars are the keys a framework that declares no sqlite wiring of its +// own gets. They are the dotenv names Laravel and the frameworks built on it +// use, which is what every project reaching this fallback has. var sqliteEnvVars = []string{ "DB_CONNECTION=sqlite", "DB_DATABASE=database/database.sqlite", } +// sqliteVarsFor returns the env values that point a project at its file +// database. Which keys those are is the definition's to say, like every other +// wiring: a Datasources driver reaches CakePHP and DB_CONNECTION does not. +func sqliteVarsFor(fw *config.Framework) []string { + if fw != nil && fw.Env.SQLite != nil && len(fw.Env.SQLite.Vars) > 0 { + return fw.Env.SQLite.Vars + } + return sqliteEnvVars +} + // serviceEnvVars returns the recommended Laravel .env KEY=VALUE pairs for a // default-preset service or sqlite. Returns nil for any other name. func serviceEnvVars(name string) []string { diff --git a/internal/cli/setup.go b/internal/cli/setup.go index d94ded899..7be021490 100644 --- a/internal/cli/setup.go +++ b/internal/cli/setup.go @@ -138,33 +138,23 @@ func printSetupStepPlan(cwd string, skipOpen bool) error { // (the asset build needs the install that precedes it). A name the plan does // not offer is an error naming what it does offer, so a caller working from a // stale list is told rather than silently running less than it asked for. -func selectSetupSteps(steps []setupStep, labels []string) ([]setupStep, error) { +func selectSetupSteps(steps []setupStep, labels []string) (selected []setupStep, skipped []string) { wanted := make(map[string]bool, len(labels)) for _, l := range labels { wanted[strings.TrimSpace(l)] = true } - var selected []setupStep for _, s := range steps { if wanted[s.label] { selected = append(selected, s) delete(wanted, s.label) } } - if len(wanted) > 0 { - unknown := make([]string, 0, len(wanted)) - for l := range wanted { - unknown = append(unknown, l) - } - sort.Strings(unknown) - offered := make([]string, 0, len(steps)) - for _, s := range steps { - offered = append(offered, s.label) - } - return nil, fmt.Errorf("no such setup step: %s — this directory offers: %s", - strings.Join(unknown, ", "), strings.Join(offered, ", ")) + for l := range wanted { + skipped = append(skipped, l) } - return selected, nil + sort.Strings(skipped) + return selected, skipped } // runNamedSetupSteps runs exactly the steps it is given. The configure phase is @@ -173,9 +163,13 @@ func selectSetupSteps(steps []setupStep, labels []string) ([]setupStep, error) { // invocation. Output is left alone rather than captured, so a caller streaming // this sees the step's progress as it happens. func runNamedSetupSteps(cwd string, labels []string, skipOpen bool) error { - selected, err := selectSetupSteps(planSetupSteps(cwd, skipOpen), labels) - if err != nil { - return err + // The plan is re-derived per invocation and its steps are gated on live state: + // securing the site, a worker scan, a file the previous step wrote. A label + // the plan no longer offers is work that no longer needs doing, so it is + // reported and passed over rather than failing the caller's whole queue. + selected, skipped := selectSetupSteps(planSetupSteps(cwd, skipOpen), labels) + for _, label := range skipped { + fmt.Printf("→ %s (nothing left to do)\n", label) } for _, s := range selected { fmt.Printf("→ %s\n", s.label) diff --git a/internal/cli/setup_plan_test.go b/internal/cli/setup_plan_test.go index 098699661..c67989f83 100644 --- a/internal/cli/setup_plan_test.go +++ b/internal/cli/setup_plan_test.go @@ -4,7 +4,6 @@ import ( "encoding/json" "os" "path/filepath" - "strings" "testing" ) @@ -130,23 +129,27 @@ func TestSetupStepPlanJSON(t *testing.T) { } } -// Naming a step that this directory does not offer is a mistake worth reporting -// rather than a silent no-op, and the message names what is on offer. -func TestSelectSetupStepsRejectsUnknownLabel(t *testing.T) { +// The plan is re-derived for every `lerd setup --step`, and its steps are gated +// on state an earlier step or the watcher can flip. A queued label the plan no +// longer offers is work already done, so it is reported back and passed over +// rather than failing the caller and stopping the rest of its queue. +func TestSelectSetupStepsSkipsAStepThePlanNoLongerOffers(t *testing.T) { isolateSetupPlan(t) dir := t.TempDir() writePlanFixture(t, dir, map[string]string{"composer.json": `{"require":{}}`}) steps := planSetupSteps(dir, true) - if _, err := selectSetupSteps(steps, []string{"composer install"}); err != nil { - t.Fatalf("selecting an offered step failed: %v", err) + selected, skipped := selectSetupSteps(steps, []string{"composer install"}) + if len(selected) != 1 || len(skipped) != 0 { + t.Fatalf("an offered step was not selected: %d selected, skipped %v", len(selected), skipped) } - _, err := selectSetupSteps(steps, []string{"npm install/ci"}) - if err == nil { - t.Fatal("selecting a step the plan does not offer should fail") + + selected, skipped = selectSetupSteps(steps, []string{"npm install/ci"}) + if len(selected) != 0 { + t.Errorf("selected %d steps for a label the plan does not offer", len(selected)) } - if !strings.Contains(err.Error(), "composer install") { - t.Errorf("error should list the offered steps, got: %v", err) + if len(skipped) != 1 || skipped[0] != "npm install/ci" { + t.Errorf("the vanished label was not reported back: %v", skipped) } } @@ -161,9 +164,9 @@ func TestSelectSetupStepsKeepsPlanOrder(t *testing.T) { }) steps := planSetupSteps(dir, true) - selected, err := selectSetupSteps(steps, []string{"npm run build", "composer install"}) - if err != nil { - t.Fatal(err) + selected, skipped := selectSetupSteps(steps, []string{"npm run build", "composer install"}) + if len(skipped) != 0 { + t.Fatalf("offered steps were skipped: %v", skipped) } if len(selected) != 2 { t.Fatalf("selected %d steps, want 2", len(selected)) diff --git a/internal/cli/sqlite_option_test.go b/internal/cli/sqlite_option_test.go index 941b80c14..595811c7b 100644 --- a/internal/cli/sqlite_option_test.go +++ b/internal/cli/sqlite_option_test.go @@ -1,9 +1,11 @@ package cli import ( + "strings" "testing" "github.com/geodro/lerd/internal/config" + "github.com/geodro/lerd/internal/sitedoctor" ) // Which databases a framework can use is the definition's to declare, like @@ -14,10 +16,12 @@ func TestBuildDatabaseOptions_offersSQLiteOnlyWhereDeclared(t *testing.T) { t.Setenv("XDG_CONFIG_HOME", t.TempDir()) t.Setenv("XDG_DATA_HOME", t.TempDir()) - declares := &config.Framework{Env: config.FrameworkEnvConf{Services: map[string]config.FrameworkServiceDef{ - "sqlite": {Vars: []string{"DB_CONNECTION=sqlite"}}, - "mysql": {Vars: []string{"DB_HOST=lerd-mysql"}}, - }}} + declares := &config.Framework{Env: config.FrameworkEnvConf{ + SQLite: &config.FrameworkServiceDef{Vars: []string{"DB_CONNECTION=sqlite"}}, + Services: map[string]config.FrameworkServiceDef{ + "mysql": {Vars: []string{"DB_HOST=lerd-mysql"}}, + }, + }} _, names := buildDatabaseOptions(declares) if !names["sqlite"] { t.Error("a framework declaring sqlite was not offered it") @@ -27,7 +31,39 @@ func TestBuildDatabaseOptions_offersSQLiteOnlyWhereDeclared(t *testing.T) { "mysql": {Vars: []string{"DB_HOST=lerd-mysql"}}, }}} if _, names := buildDatabaseOptions(declaresNot); names["sqlite"] { - t.Error("a framework that declares no sqlite service was offered it anyway") + t.Error("a framework that declares no sqlite wiring was offered it anyway") + } +} + +// A file database is not a service: nothing installs it, starts it or draws a +// card for it. A definition that names it among the services is not what +// declares it, and an older binary reading that entry would try to start it. +func TestBuildDatabaseOptions_ignoresSQLiteAmongServices(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + t.Setenv("XDG_DATA_HOME", t.TempDir()) + + asService := &config.Framework{Env: config.FrameworkEnvConf{Services: map[string]config.FrameworkServiceDef{ + "sqlite": {Vars: []string{"DB_CONNECTION=sqlite"}}, + "mysql": {Vars: []string{"DB_HOST=lerd-mysql"}}, + }}} + if _, names := buildDatabaseOptions(asService); names["sqlite"] { + t.Error("sqlite listed as a service was taken as a declaration") + } +} + +// Which keys point a project at its file database is the definition's to say. +// Writing DB_CONNECTION into a CakePHP project reaches nothing it reads. +func TestSQLiteVarsFor_comeFromTheDefinition(t *testing.T) { + cakeish := &config.Framework{Env: config.FrameworkEnvConf{SQLite: &config.FrameworkServiceDef{ + Vars: []string{`Datasources.default.driver=Cake\Database\Driver\Sqlite`}, + }}} + got := sqliteVarsFor(cakeish) + if len(got) != 1 || got[0] != `Datasources.default.driver=Cake\Database\Driver\Sqlite` { + t.Errorf("declared sqlite vars were not used, got %v", got) + } + + if got := sqliteVarsFor(&config.Framework{}); len(got) == 0 || got[0] != "DB_CONNECTION=sqlite" { + t.Errorf("a framework declaring none did not fall back to the dotenv keys, got %v", got) } } @@ -45,3 +81,39 @@ func TestBuildDatabaseOptions_keepsSQLiteWithoutAFramework(t *testing.T) { t.Error("a framework declaring no env services at all was not offered sqlite") } } + +// The file lerd creates has to be the one the framework's own values name. A +// Symfony project keeps its path inside the DSN, and creating Laravel's +// database/database.sqlite beside it leaves an empty stray file while the file +// the application opens is still missing. +func TestSQLiteFileFollowsTheDeclaredValues(t *testing.T) { + symfonyish := &config.Framework{Env: config.FrameworkEnvConf{ + SQLite: &config.FrameworkServiceDef{ + Detect: []config.FrameworkServiceDetect{{Key: "DATABASE_URL", ValuePrefix: "sqlite://"}}, + Vars: []string{"DATABASE_URL=sqlite:///%kernel.project_dir%/var/data.db"}, + }, + }} + vals := map[string]string{} + for _, kv := range sqliteVarsFor(symfonyish) { + k, v, _ := strings.Cut(kv, "=") + vals[k] = v + } + got, ok := sitedoctor.SQLiteFileFromValues(vals, symfonyish) + if !ok || got != "var/data.db" { + t.Errorf("resolved %q (ok=%v), want var/data.db", got, ok) + } + + laravelish := &config.Framework{Env: config.FrameworkEnvConf{ + SQLite: &config.FrameworkServiceDef{ + Vars: []string{"DB_CONNECTION=sqlite", "DB_DATABASE=database/database.sqlite"}, + }, + }} + vals = map[string]string{} + for _, kv := range sqliteVarsFor(laravelish) { + k, v, _ := strings.Cut(kv, "=") + vals[k] = v + } + if got, ok := sitedoctor.SQLiteFileFromValues(vals, laravelish); !ok || got != "database/database.sqlite" { + t.Errorf("resolved %q (ok=%v), want database/database.sqlite", got, ok) + } +} diff --git a/internal/config/framework.go b/internal/config/framework.go index 262b689b4..fe595ec7b 100644 --- a/internal/config/framework.go +++ b/internal/config/framework.go @@ -477,6 +477,16 @@ type FrameworkEnvConf struct { // Keys match the built-in service names: mysql, postgres, redis, meilisearch, rustfs, mailpit. Services map[string]FrameworkServiceDef `yaml:"services,omitempty"` + // SQLite declares how a framework is wired to a file database: its detect + // rules say a project is already on one, its vars point it at one. + // + // It sits beside Services rather than among them because it is not a + // service. Nothing installs it, starts it or draws a card for it, and an + // older binary reading it as a service entry would announce it and then try + // to start a container that does not exist. An unknown field is ignored + // instead, for the same reason AppFile above is a new field. + SQLite *FrameworkServiceDef `yaml:"sqlite,omitempty"` + // KeyGeneration describes how to generate an application key if missing. KeyGeneration *EnvKeyGeneration `yaml:"key_generation,omitempty"` } @@ -489,7 +499,8 @@ func (f *Framework) HasEnvConfig() bool { return false } e := f.Env - return e.File != "" || e.FallbackFile != "" || e.ExampleFile != "" || e.KeyGeneration != nil || len(e.Services) > 0 + return e.File != "" || e.FallbackFile != "" || e.ExampleFile != "" || e.KeyGeneration != nil || + len(e.Services) > 0 || e.SQLite != nil } // FrameworkNotifications lets a definition decline a warning that says nothing diff --git a/internal/config/worker_icon.go b/internal/config/worker_icon.go index 1aa9b446e..8baadd734 100644 --- a/internal/config/worker_icon.go +++ b/internal/config/worker_icon.go @@ -77,9 +77,7 @@ func WorkerMarks() WorkerMarkSet { // framework. Workers are declared per version, and the newest file that has // them answers, so a framework whose latest release added one still reports it. func cachedFrameworkWorkers(name string) map[string]FrameworkWorker { - paths, _ := filepath.Glob(filepath.Join(StoreFrameworksDir(), name+"@*.yaml")) - paths = append(paths, filepath.Join(StoreFrameworksDir(), name+".yaml")) - sort.Sort(sort.Reverse(sort.StringSlice(paths))) + paths := append(versionedFrameworkPaths(name), filepath.Join(StoreFrameworksDir(), name+".yaml")) for _, p := range paths { if fw := loadFrameworkYAML(p); fw != nil && len(fw.Workers) > 0 { return fw.Workers diff --git a/internal/config/worker_icon_test.go b/internal/config/worker_icon_test.go index eb5ef51de..cfba77fb0 100644 --- a/internal/config/worker_icon_test.go +++ b/internal/config/worker_icon_test.go @@ -96,3 +96,25 @@ func TestWorkerMarks_VersionsCollapseToOneEntry(t *testing.T) { t.Errorf("want queue and vite once each, got %v", got) } } + +// Versions are numbers, not strings: a machine that once resolved a Laravel 9 +// project still has laravel@9.yaml beside laravel@12.yaml, and sorting the +// names would let 9 outrank 12 and answer with a definition two majors old. +func TestWorkerMarks_TheNewestVersionAnswers(t *testing.T) { + t.Setenv("XDG_DATA_HOME", t.TempDir()) + writeCachedFramework(t, "laravel@9.yaml", `name: laravel +label: Laravel +version: "9" +workers: + queue: + label: Queue Worker + command: php artisan queue:work + icon: queue +`) + writeCachedFramework(t, "laravel@12.yaml", laravelWithWorkers) + + got := WorkerMarks() + if _, ok := got.Workers["laravel/vite"]; !ok { + t.Errorf("the newer definition's worker is missing, an older file answered: %v", got.Workers) + } +} diff --git a/internal/envfile/phparray.go b/internal/envfile/phparray.go index 82a5b4e23..1b48ccd74 100644 --- a/internal/envfile/phparray.go +++ b/internal/envfile/phparray.go @@ -97,6 +97,25 @@ func ApplyPhpArrayUpdates(path string, updates map[string]string) error { } sort.Strings(keys) + // A key naming a node another key descends through cannot hold at the same + // time as that key: the node is either the scalar or the array. The deeper + // key is the one that makes the other's parent exist, so it wins, and the + // shallower one is dropped rather than fought over the same span of the file. + kept := keys[:0] + for _, k := range keys { + ancestor := false + for _, other := range keys { + if other != k && strings.HasPrefix(other, k+".") { + ancestor = true + break + } + } + if !ancestor { + kept = append(kept, k) + } + } + keys = kept + if root != nil && root.kind == phpArray { return writePhpArrayInPlace(path, original, root, keys, updates) } @@ -131,21 +150,34 @@ func writePhpArrayInPlace(path, original string, root *phpValue, keys []string, // under the same new parent produce one entry rather than two of the same name. grafts := map[*phpValue]*phpValue{} var graftOrder []*phpValue + replacements := map[*phpValue]*phpValue{} + var replaceOrder []*phpValue + + type scalarEdit struct { + node *phpValue + text string + } + var scalars []scalarEdit for _, key := range keys { segs := strings.Split(key, ".") node, rest := descendPhpArray(root, segs) if len(rest) == 0 { - edits = append(edits, edit{node.start, node.end, + scalars = append(scalars, scalarEdit{node, renderPhpValue(scalarValue(updates[key], node.kind), indentAt(original, node.start))}) continue } if node.kind != phpArray { // Something that is not an array sits where one has to be. Replacing it - // is the only way through, and it is what the whole-file writer did. - replacement := &phpValue{kind: phpArray} + // is the only way through, and every key reaching it shares the one + // replacement: a second edit over the same span would splice over the first. + replacement := replacements[node] + if replacement == nil { + replacement = &phpValue{kind: phpArray} + replacements[node] = replacement + replaceOrder = append(replaceOrder, node) + } setPath(replacement, rest, updates[key]) - edits = append(edits, edit{node.start, node.end, renderPhpValue(replacement, indentAt(original, node.start))}) continue } graft := grafts[node] @@ -157,18 +189,52 @@ func writePhpArrayInPlace(path, original string, root *phpValue, keys []string, setPath(graft, rest, updates[key]) } + for _, s := range scalars { + // A node other keys descend through is written as the array they need, + // and that replacement covers this very span. Emitting both would put two + // edits on it. + if replacements[s.node] != nil { + continue + } + edits = append(edits, edit{s.node.start, s.node.end, s.text}) + } + + for _, node := range replaceOrder { + edits = append(edits, edit{node.start, node.end, + renderPhpValue(replacements[node], indentAt(original, node.start))}) + } + for _, node := range graftOrder { at, indent, ok := phpArrayInsertion(original, node) if !ok { - // An array written on one line has nowhere to insert a line, so it is - // reprinted whole. It has no comments inside it to lose. - merged := clonePhpValue(node) - for _, e := range grafts[node].entries { - setPathValue(merged, []string{e.key}, e.val) + // An array written on one line has no line to insert into, so the new + // entries go inline before its closing bracket. Reprinting the node + // whole would claim a span other edits may sit inside. + var b strings.Builder + for i, e := range grafts[node].entries { + if i > 0 { + b.WriteString(", ") + } + b.WriteString("'" + escapeSingle(e.key) + "' => ") + printValue(&b, e.val, 0) } - edits = append(edits, edit{node.start, node.end, renderPhpValue(merged, indentAt(original, node.start))}) + insertAt := node.end - 1 + edits = append(edits, edit{insertAt, insertAt, inlineSep(original, node.start, insertAt) + b.String()}) continue } + // The last entry may lack the trailing comma PHP needs between it and + // what is inserted after it. Anything but whitespace between them (a + // comment holding a comma, say) leaves this alone; the verify pass below + // then refuses the write rather than guess. + if n := len(node.entries); n > 0 { + lastEnd := node.entries[n-1].val.end + for lastEnd > 0 && (original[lastEnd-1] == ' ' || original[lastEnd-1] == '\t' || original[lastEnd-1] == '\n' || original[lastEnd-1] == '\r') { + lastEnd-- + } + if lastEnd <= at && !strings.Contains(original[lastEnd:at], ",") { + edits = append(edits, edit{lastEnd, lastEnd, ","}) + } + } var b strings.Builder for _, e := range grafts[node].entries { b.WriteString(indent + "'" + escapeSingle(e.key) + "' => ") @@ -178,15 +244,87 @@ func writePhpArrayInPlace(path, original string, root *phpValue, keys []string, edits = append(edits, edit{at, at, b.String()}) } - // Applied back to front so each splice leaves the earlier offsets valid. + // Applied back to front so each splice leaves the earlier offsets valid, + // which holds only while every edit sits wholly before the one applied + // before it. The edits are built never to overlap; one that does anyway + // would be spliced against an offset that has already moved and cut the + // file mid-expression, so it is an error, not a judgement call. sort.SliceStable(edits, func(i, j int) bool { return edits[i].start > edits[j].start }) out := original + bound := len(original) for _, e := range edits { + if e.end > bound { + return fmt.Errorf("refusing to rewrite %s: overlapping edits at offset %d", path, e.start) + } out = out[:e.start] + e.text + out[e.end:] + bound = e.start + } + if err := verifyPhpArrayRewrite(original, out, keys, updates); err != nil { + return fmt.Errorf("refusing to rewrite %s: %w", path, err) } return writePhpArrayFile(path, original, out) } +// inlineSep returns what separates an inline insertion from the entry before +// it: nothing straight after the opening bracket or an existing comma, a comma +// and space after an entry. +func inlineSep(src string, open, insertAt int) string { + i := insertAt + for i > open && (src[i-1] == ' ' || src[i-1] == '\t') { + i-- + } + if i <= open+1 || src[i-1] == ',' || src[i-1] == '[' || src[i-1] == '(' { + return "" + } + return ", " +} + +// verifyPhpArrayRewrite refuses a rewrite that damaged the file: the output +// must still parse, every update must read back as written, and every key the +// updates did not touch must still hold its old value. The writer has had more +// ways to be wrong than anyone predicted, and each reported success; whatever +// shape the next one takes, it becomes an error here instead of a corrupted +// config the application is left to discover. +func verifyPhpArrayRewrite(original, out string, keys []string, updates map[string]string) error { + root, err := parsePhpReturn(out) + if err != nil || root == nil { + return fmt.Errorf("the rewrite no longer parses: %w", err) + } + got := map[string]string{} + flatten("", root, got) + for _, k := range keys { + if got[k] != updates[k] { + return fmt.Errorf("the rewrite lost %s: %q instead of %q", k, got[k], updates[k]) + } + } + origRoot, err := parsePhpReturn(original) + if err != nil || origRoot == nil { + return nil + } + was := map[string]string{} + flatten("", origRoot, was) + for k, v := range was { + if updateTouches(k, keys) { + continue + } + if got[k] != v { + return fmt.Errorf("the rewrite changed %s unasked: %q instead of %q", k, got[k], v) + } + } + return nil +} + +// updateTouches reports whether an update key claims k: exactly, as one of its +// descendants, or as an ancestor a deeper update rebuilt on the way down. +func updateTouches(k string, keys []string) bool { + for _, u := range keys { + if u == k || strings.HasPrefix(k, u+".") || strings.HasPrefix(u, k+".") { + return true + } + } + return false +} + // descendPhpArray walks as far into the tree as the file already goes, returning // the deepest node reached and the segments still to be created below it. func descendPhpArray(root *phpValue, segs []string) (*phpValue, []string) { @@ -543,15 +681,25 @@ func (p *phpParser) parseValueAt() (*phpValue, error) { p.pos++ } kind := phpInt + digits := 0 for p.pos < len(p.src) { d := p.src[p.pos] if d == '.' || d == 'e' || d == 'E' || d == '+' || d == '-' { kind = phpFloat - } else if d < '0' || d > '9' { + } else if d >= '0' && d <= '9' { + digits++ + } else { break } p.pos++ } + // -PHP_INT_MAX is not the number "-": a sign with no digits behind it, + // or a number running straight into an identifier, is an expression and + // is kept whole. + if digits == 0 || (p.pos < len(p.src) && isIdentByte(p.src[p.pos])) { + p.pos = start + return p.parseExpression() + } return &phpValue{kind: kind, str: p.src[start:p.pos]}, nil } return p.parseExpression() @@ -639,6 +787,11 @@ func (p *phpParser) parseArrayBody(closer byte) (*phpValue, error) { p.skipTrivia() if p.pos < len(p.src) && p.src[p.pos] == ',' { p.pos++ + } else if p.pos >= len(p.src) || p.src[p.pos] != closer { + // PHP requires the comma between entries; only the last may omit it. + // Reading a file without them as if it parsed hides exactly the + // corruption the writer's own guard exists to catch. + return nil, fmt.Errorf("expected ',' or '%c' after array entry at offset %d", closer, p.pos) } } } diff --git a/internal/envfile/phparray_test.go b/internal/envfile/phparray_test.go index 229822476..d73d41e0e 100644 --- a/internal/envfile/phparray_test.go +++ b/internal/envfile/phparray_test.go @@ -2,6 +2,7 @@ package envfile import ( "os" + "os/exec" "path/filepath" "strings" "testing" @@ -463,6 +464,42 @@ func TestApplyPhpArrayUpdates_GroupsKeysUnderOneNewParent(t *testing.T) { } } +// Several keys descending through the same value that is not an array, as an +// app_local.php reading its datasource from env() has. One replacement serves +// them all, or the second splices over the first and the file stops parsing. +func TestApplyPhpArrayUpdates_KeysThroughOneNonArray(t *testing.T) { + path := filepath.Join(t.TempDir(), "app_local.php") + body := " env('DATABASE_URL'),\n 'debug' => true,\n];\n" + if err := os.WriteFile(path, []byte(body), 0644); err != nil { + t.Fatal(err) + } + + if err := ApplyPhpArrayUpdates(path, map[string]string{ + "Datasources.default.database": "site", + "Datasources.default.host": "lerd-mysql", + "Datasources.default.username": "lerd", + }); err != nil { + t.Fatalf("ApplyPhpArrayUpdates: %v", err) + } + vals, err := ReadPhpArray(path) + if err != nil { + t.Fatalf("re-read: %v", err) + } + for key, want := range map[string]string{ + "Datasources.default.database": "site", + "Datasources.default.host": "lerd-mysql", + "Datasources.default.username": "lerd", + } { + if vals[key] != want { + out, _ := os.ReadFile(path) + t.Errorf("%s = %q, want %q:\n%s", key, vals[key], want, out) + } + } + if out, _ := os.ReadFile(path); !strings.Contains(string(out), "'debug' => true") { + t.Errorf("the rest of the file did not survive:\n%s", out) + } +} + // A value the reader cannot evaluate is nobody's to report: the key is absent // from a read rather than carrying the source text as if it were the value. func TestReadPhpArray_OmitsExpressionValues(t *testing.T) { @@ -485,3 +522,182 @@ func TestReadPhpArray_OmitsExpressionValues(t *testing.T) { t.Errorf("host = %q, want the literal alongside the expressions", vals["Datasources.default.host"]) } } + +// phpLint runs php -l over the file where php is on PATH, as an extra layer on +// top of the always-on assertions: a splice landing mid-expression is a file +// only php calls wrong. Absence of php weakens nothing below, it only skips +// this one extra check. +func phpLint(t *testing.T, path string) { + t.Helper() + bin, err := exec.LookPath("php") + if err != nil { + return + } + out, err := exec.Command(bin, "-l", path).CombinedOutput() + if err != nil { + body, _ := os.ReadFile(path) + t.Fatalf("php rejected the rewritten file: %v\n%s\n--- file ---\n%s", err, out, body) + } +} + +// A key naming a node and another key descending through it cannot both hold: +// the node is either the scalar or the array. The deeper key is the one that +// makes the other's parent exist, so it wins, and the file must stay parseable +// with the rest of it intact. +func TestApplyPhpArrayUpdates_KeyNamingANodeAnotherDescendsThrough(t *testing.T) { + path := filepath.Join(t.TempDir(), "app_local.php") + body := " null,\n 'debug' => true,\n];\n" + if err := os.WriteFile(path, []byte(body), 0644); err != nil { + t.Fatal(err) + } + + if err := ApplyPhpArrayUpdates(path, map[string]string{ + "Datasources": "x", + "Datasources.default.database": "site", + }); err != nil { + t.Fatalf("ApplyPhpArrayUpdates: %v", err) + } + phpLint(t, path) + + vals, err := ReadPhpArray(path) + if err != nil { + out, _ := os.ReadFile(path) + t.Fatalf("re-read: %v\n%s", err, out) + } + if vals["Datasources.default.database"] != "site" { + t.Errorf("the deeper key did not survive: %v", vals) + } + if vals["Datasources"] == "x" { + t.Errorf("the shallower key overwrote the array the deeper one needs: %v", vals) + } + if vals["debug"] != "true" { + t.Errorf("an unrelated key was damaged: %v", vals) + } +} + +// The same collision through an array node: the key naming the whole array is +// dropped in favour of the insertion into it, and the insertion lands. +func TestApplyPhpArrayUpdates_WholeArrayAndAnInsertionIntoIt(t *testing.T) { + path := filepath.Join(t.TempDir(), "app_local.php") + if err := os.WriteFile(path, []byte(cakeAppLocalPHP), 0644); err != nil { + t.Fatal(err) + } + + if err := ApplyPhpArrayUpdates(path, map[string]string{ + "Datasources.default": "x", + "Datasources.default.port": "3306", + }); err != nil { + t.Fatalf("ApplyPhpArrayUpdates: %v", err) + } + phpLint(t, path) + + vals, err := ReadPhpArray(path) + if err != nil { + out, _ := os.ReadFile(path) + t.Fatalf("re-read: %v\n%s", err, out) + } + if vals["Datasources.default.port"] != "3306" { + t.Errorf("the insertion was lost: %v", vals) + } + if vals["Datasources.default"] == "x" { + t.Errorf("the array was overwritten by the shallower key: %v", vals) + } + out, _ := os.ReadFile(path) + if !strings.Contains(string(out), "use function Cake\\Core\\env;") { + t.Errorf("the file lost its import:\n%s", out) + } +} + +// A scalar update inside a single-line array and a new key grafted into the +// same array are two edits into one span of the file. Neither may be lost. +func TestApplyPhpArrayUpdates_EditAndGraftInOneSingleLineArray(t *testing.T) { + path := filepath.Join(t.TempDir(), "app_local.php") + body := " ['host' => 'localhost'],\n 'debug' => true,\n];\n" + if err := os.WriteFile(path, []byte(body), 0644); err != nil { + t.Fatal(err) + } + + if err := ApplyPhpArrayUpdates(path, map[string]string{ + "Datasources.host": "db", + "Datasources.port": "3306", + }); err != nil { + t.Fatalf("ApplyPhpArrayUpdates: %v", err) + } + phpLint(t, path) + + vals, err := ReadPhpArray(path) + if err != nil { + out, _ := os.ReadFile(path) + t.Fatalf("re-read: %v\n%s", err, out) + } + if vals["Datasources.host"] != "db" || vals["Datasources.port"] != "3306" { + out, _ := os.ReadFile(path) + t.Errorf("an edit was lost: %v\n%s", vals, out) + } +} + +// A last entry written without a trailing comma is how plenty of hand-edited +// configs read. Grafting after it must supply the comma PHP needs between them. +func TestApplyPhpArrayUpdates_GraftAfterEntryWithoutTrailingComma(t *testing.T) { + path := filepath.Join(t.TempDir(), "env.php") + body := " [\n 'host' => 'localhost'\n ]\n];\n" + if err := os.WriteFile(path, []byte(body), 0644); err != nil { + t.Fatal(err) + } + + if err := ApplyPhpArrayUpdates(path, map[string]string{"db.port": "3306"}); err != nil { + t.Fatalf("ApplyPhpArrayUpdates: %v", err) + } + phpLint(t, path) + + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + want := " [\n 'host' => 'localhost',\n 'port' => '3306',\n ]\n];\n" + if string(got) != want { + t.Errorf("rewritten file:\n%s\nwant:\n%s", got, want) + } +} + +// A negative constant is an expression, not the number '-'. It reads as no +// value and reprints untouched, like every other expression. +func TestReadPhpArray_NegativeConstantIsAnExpression(t *testing.T) { + path := writeTemp(t, " ['limit' => -PHP_INT_MAX],\n];\n") + vals, err := ReadPhpArray(path) + if err != nil { + t.Fatal(err) + } + if v, ok := vals["a.limit"]; ok { + t.Errorf("an expression reported a value: %q", v) + } + if _, ok := vals["a.0"]; ok { + t.Errorf("a phantom positional entry appeared: %v", vals) + } + + if err := ApplyPhpArrayUpdates(path, map[string]string{"a.extra": "1"}); err != nil { + t.Fatalf("ApplyPhpArrayUpdates: %v", err) + } + phpLint(t, path) + out, _ := os.ReadFile(path) + if !strings.Contains(string(out), "-PHP_INT_MAX") { + t.Errorf("the expression did not survive the rewrite:\n%s", out) + } + vals, err = ReadPhpArray(path) + if err != nil { + t.Fatalf("re-read: %v\n%s", err, out) + } + if vals["a.extra"] != "1" { + t.Errorf("the added key was lost: %v", vals) + } +} + +// A file whose entries are not comma-separated is not PHP, however it got that +// way. Reading it as if it were hides exactly the corruption the writer's own +// guard exists to catch. +func TestReadPhpArray_RefusesMissingCommaBetweenEntries(t *testing.T) { + path := writeTemp(t, " 1\n 'b' => 2,\n];\n") + if _, err := ReadPhpArray(path); err == nil { + t.Error("a file php rejects was read as if it parsed") + } +} diff --git a/internal/serviceops/reconcile.go b/internal/serviceops/reconcile.go index c5365275f..1bedd4036 100644 --- a/internal/serviceops/reconcile.go +++ b/internal/serviceops/reconcile.go @@ -12,7 +12,7 @@ import ( // Seams so tests can drive reconcile without real podman/quadlet work. var ( - ensureQuadletFn = EnsureCustomServiceQuadlet + ensureQuadletFn = ensureCustomServiceQuadletDiff listManagedServiceNames = podman.ListManagedServiceNames orphanContainerRunningFn = podman.ContainerRunningQuiet materializeFilesFn = config.MaterializeServiceFiles @@ -71,20 +71,34 @@ func ReconcileServices(emit func(PhaseEvent)) (ReconcileResult, error) { // mount changes the unit too, and restarting the old one brings the // container back without the mount, leaving it a pass behind until // something restarts it again. + unitChanged := false if !unitInstalled || slices.Contains(res.DefinitionsRefreshed, svc.Name) { - if err := ensureQuadletFn(svc); err != nil { + changed, err := ensureQuadletFn(svc) + if err != nil { errs = append(errs, fmt.Errorf("regenerating quadlet for %s: %w", svc.Name, err)) continue } + unitChanged = changed if !unitInstalled { res.QuadletsRegenerated = append(res.QuadletsRegenerated, svc.Name) } } if unitInstalled { - if applied, err := RestartIfConfigDrifted(svc.Name, svc.Preset); err != nil { + applied, err := RestartIfConfigDrifted(svc.Name, svc.Preset) + if err != nil { errs = append(errs, err) } else if applied { res.ConfigsApplied = append(res.ConfigsApplied, svc.Name) + } else if unitChanged { + // The drift check reads the materialised config files and is blind to + // the unit itself, so a definition that moved a port, changed the image + // or added an environment variable would sit rewritten on disk while the + // container kept running on what it started with. + if restarted, err := restartRunningUnit(svc.Name); err != nil { + errs = append(errs, err) + } else if restarted { + res.ConfigsApplied = append(res.ConfigsApplied, svc.Name) + } } } } @@ -235,3 +249,15 @@ func orphanCandidates() []string { } return names } + +// restartRunningUnit restarts a service only if its container is up, so a +// rewritten unit reaches a running service without starting a stopped one. +func restartRunningUnit(name string) (bool, error) { + if _, running := containerStartedAtFn("lerd-" + name); !running { + return false, nil + } + if err := restartUnitFn("lerd-" + name); err != nil { + return false, fmt.Errorf("restarting %s after a unit change: %w", name, err) + } + return true, nil +} diff --git a/internal/serviceops/reconcile_test.go b/internal/serviceops/reconcile_test.go index f482e010b..3ee8edb48 100644 --- a/internal/serviceops/reconcile_test.go +++ b/internal/serviceops/reconcile_test.go @@ -329,9 +329,9 @@ func TestReconcileServices_continuesPastForwardError(t *testing.T) { prev := ensureQuadletFn t.Cleanup(func() { ensureQuadletFn = prev }) - ensureQuadletFn = func(svc *config.CustomService) error { + ensureQuadletFn = func(svc *config.CustomService) (bool, error) { if svc.Name == "bad" { - return errors.New("boom") + return false, errors.New("boom") } return prev(svc) } @@ -456,9 +456,9 @@ func TestReconcileServices_regeneratesBeforeRestartingOnDrift(t *testing.T) { var order []string prevEnsure := ensureQuadletFn t.Cleanup(func() { ensureQuadletFn = prevEnsure }) - ensureQuadletFn = func(*config.CustomService) error { + ensureQuadletFn = func(*config.CustomService) (bool, error) { order = append(order, "regenerate") - return nil + return false, nil } boot := time.Unix(1_000_000, 0) @@ -478,3 +478,77 @@ func TestReconcileServices_regeneratesBeforeRestartingOnDrift(t *testing.T) { t.Fatalf("want the unit regenerated before the drift restart, got %v", order) } } + +// A store change that touches only the unit — a moved port, a new image, an +// added environment variable — leaves every materialised config file untouched, +// so the drift check sees nothing and the running container keeps whatever it +// started with until someone restarts it by hand. +func TestReconcileServices_restartsOnAUnitOnlyChange(t *testing.T) { + reconcileEnv(t) + if err := config.SaveStorePreset("probe-svc", []byte("name: probe-svc\nimage: example/probe:1\ndashboard: http://localhost:9999\n")); err != nil { + t.Fatalf("store preset: %v", err) + } + if err := config.SaveCustomService(&config.CustomService{Name: "probe-svc", Image: "example/probe:1", Preset: "probe-svc"}); err != nil { + t.Fatalf("save: %v", err) + } + writeQuadlet(t, "probe-svc", true) + + var order []string + prevEnsure := ensureQuadletFn + t.Cleanup(func() { ensureQuadletFn = prevEnsure }) + ensureQuadletFn = func(*config.CustomService) (bool, error) { + order = append(order, "regenerate") + return true, nil + } + + boot := time.Unix(1_000_000, 0) + restore := swapDriftSeams(t, + func(*config.CustomService) error { return nil }, + // No config file is newer than the container: nothing drifted on disk. + func(*config.CustomService) (time.Time, bool) { return time.Time{}, false }, + func(string) (time.Time, bool) { return boot, true }, + func(string) error { order = append(order, "restart"); return nil }, + func(string) bool { return true }, + ) + defer restore() + + if _, err := ReconcileServices(nil); err != nil { + t.Fatalf("reconcile: %v", err) + } + if len(order) != 2 || order[0] != "regenerate" || order[1] != "restart" { + t.Fatalf("a rewritten unit never reached the running container, got %v", order) + } +} + +// A rewritten unit for a service that is not running must not start it. +func TestReconcileServices_leavesAStoppedServiceStopped(t *testing.T) { + reconcileEnv(t) + if err := config.SaveStorePreset("probe-svc", []byte("name: probe-svc\nimage: example/probe:1\ndashboard: http://localhost:9999\n")); err != nil { + t.Fatalf("store preset: %v", err) + } + if err := config.SaveCustomService(&config.CustomService{Name: "probe-svc", Image: "example/probe:1", Preset: "probe-svc"}); err != nil { + t.Fatalf("save: %v", err) + } + writeQuadlet(t, "probe-svc", true) + + prevEnsure := ensureQuadletFn + t.Cleanup(func() { ensureQuadletFn = prevEnsure }) + ensureQuadletFn = func(*config.CustomService) (bool, error) { return true, nil } + + restarted := false + restore := swapDriftSeams(t, + func(*config.CustomService) error { return nil }, + func(*config.CustomService) (time.Time, bool) { return time.Time{}, false }, + func(string) (time.Time, bool) { return time.Time{}, false }, + func(string) error { restarted = true; return nil }, + func(string) bool { return true }, + ) + defer restore() + + if _, err := ReconcileServices(nil); err != nil { + t.Fatalf("reconcile: %v", err) + } + if restarted { + t.Error("a stopped service was started by a unit rewrite") + } +} diff --git a/internal/sitedoctor/sqlite_target.go b/internal/sitedoctor/sqlite_target.go index ab7463650..3b9278ca5 100644 --- a/internal/sitedoctor/sqlite_target.go +++ b/internal/sitedoctor/sqlite_target.go @@ -1,6 +1,7 @@ package sitedoctor import ( + "os" "path/filepath" "strings" @@ -21,6 +22,13 @@ import ( // ok is false when the project is not configured for SQLite at all, which is // most of them. func declaredSQLiteFile(envPath, envFormat string, fw *config.Framework) (string, bool) { + return SQLiteFileFromValues(envfile.Values(envPath, envFormat), fw) +} + +// SQLiteFileFromValues is declaredSQLiteFile against values already in hand, +// which is what `lerd env` has: it knows what it is about to write before the +// file says it, and the file it creates has to be the one those values name. +func SQLiteFileFromValues(vals map[string]string, fw *config.Framework) (string, bool) { declared := declaredEnvKeys(fw) if len(declared) == 0 { // A framework that declares no env vocabulary, or no framework at all, @@ -28,7 +36,30 @@ func declaredSQLiteFile(envPath, envFormat string, fw *config.Framework) (string // convention such a project follows. declared = map[string]bool{"DB_CONNECTION": true, "DB_DATABASE": true} } - vals := envfile.Values(envPath, envFormat) + + // A framework with a sqlite wiring of its own says exactly how a project on + // a file database reads: its detect rules. CakePHP spells the connection + // Cake\Database\Driver\Sqlite and CodeIgniter SQLite3, neither of which a + // generic scan for the word "sqlite" is entitled to assume. + if fw != nil && fw.Env.SQLite != nil { + for _, rule := range fw.Env.SQLite.Detect { + if rule.Key == "" { + continue + } + val, exists := vals[rule.Key] + if !exists || (rule.ValuePrefix != "" && !strings.HasPrefix(val, rule.ValuePrefix)) { + continue + } + // A DSN-shaped rule carries the path in the matched value itself; + // a driver-shaped one names it in the database key beside it. + if file := sqliteDSNPath(val); file != "" { + return file, true + } + if file := declaredCompanionValue(vals, declared, rule.Key); file != "" { + return file, true + } + } + } // The flat shape: a declared key names the connection, another names the // file. Laravel spells them DB_CONNECTION and DB_DATABASE; a framework @@ -119,7 +150,16 @@ func declaredEnvKeys(fw *config.Framework) map[string]bool { for _, kv := range fw.Env.Vars { add(kv) } + defs := make([]config.FrameworkServiceDef, 0, len(fw.Env.Services)+1) for _, def := range fw.Env.Services { + defs = append(defs, def) + } + // The file database is wired through keys of its own, and they are as much + // the project's vocabulary as any service's. + if fw.Env.SQLite != nil { + defs = append(defs, *fw.Env.SQLite) + } + for _, def := range defs { for _, rule := range def.Detect { if rule.Key != "" { keys[rule.Key] = true @@ -142,6 +182,32 @@ func declaredEnvKeys(fw *config.Framework) map[string]bool { // root, which is a directory down. Both are checked, and a database found at // either is the site's, so a healthy 20 MB file is not reported missing because // lerd measured from the wrong end. +// SQLiteCreationTarget returns where `lerd env` should create a declared +// SQLite file, by the same rules the checks above read by. Nothing to create +// when the file already exists anywhere the application could open it, or when +// the declared path is absolute: that file is the user's own to manage, not +// lerd's to build a directory tree for. Among the candidates, one whose parent +// directory already exists is where the project keeps such files (Laravel's +// database/, Drupal's files dir under the docroot); failing that, the project +// root resolution stands. +func SQLiteCreationTarget(projectPath string, fw *config.Framework, dbFile string) (string, bool) { + if filepath.IsAbs(dbFile) { + return "", false + } + paths := sqliteFilePaths(projectPath, publicDirOf(fw), dbFile) + for _, p := range paths { + if _, err := os.Stat(p); err == nil { + return "", false + } + } + for _, p := range paths { + if fi, err := os.Stat(filepath.Dir(p)); err == nil && fi.IsDir() { + return p, true + } + } + return paths[0], true +} + func sqliteFilePaths(projectPath, publicDir, dbFile string) []string { if filepath.IsAbs(dbFile) { return []string{dbFile} diff --git a/internal/sitedoctor/sqlite_target_test.go b/internal/sitedoctor/sqlite_target_test.go index 7f71ed258..744acf684 100644 --- a/internal/sitedoctor/sqlite_target_test.go +++ b/internal/sitedoctor/sqlite_target_test.go @@ -147,3 +147,119 @@ func TestDeclaredCompanionValue_noLaravelDefaultForADottedKey(t *testing.T) { t.Errorf("companion = %q, want Laravel's default", got) } } + +// A framework's own detect rules say how a project on a file database reads. +// CakePHP spells the connection as a driver class and CodeIgniter as SQLite3, +// neither of which the generic scan for the word "sqlite" may assume. +func TestSQLiteFileFromValues_ReadsTheDeclaredDetectRules(t *testing.T) { + for _, tc := range []struct { + name string + detect []config.FrameworkServiceDetect + vals map[string]string + want string + }{ + { + "cakephp driver class", + []config.FrameworkServiceDetect{{Key: "Datasources.default.driver", ValuePrefix: `Cake\Database\Driver\Sqlite`}}, + map[string]string{ + "Datasources.default.driver": `Cake\Database\Driver\Sqlite`, + "Datasources.default.database": "database/database.sqlite", + }, + "database/database.sqlite", + }, + { + "codeigniter SQLite3", + []config.FrameworkServiceDetect{{Key: "database.default.DBDriver", ValuePrefix: "SQLite3"}}, + map[string]string{ + "database.default.DBDriver": "SQLite3", + "database.default.database": "writable/db.sqlite3", + }, + "writable/db.sqlite3", + }, + { + "laravel flat pair", + []config.FrameworkServiceDetect{{Key: "DB_CONNECTION", ValuePrefix: "sqlite"}}, + map[string]string{"DB_CONNECTION": "sqlite", "DB_DATABASE": "database/database.sqlite"}, + "database/database.sqlite", + }, + { + "symfony DSN", + []config.FrameworkServiceDetect{{Key: "DATABASE_URL", ValuePrefix: "sqlite://"}}, + map[string]string{"DATABASE_URL": "sqlite:///%kernel.project_dir%/var/data.db"}, + "var/data.db", + }, + } { + t.Run(tc.name, func(t *testing.T) { + // The store schema ships the wiring vars alongside the detect + // rules; they are the vocabulary the companion key is found in. + vars := make([]string, 0, len(tc.vals)) + for k, v := range tc.vals { + vars = append(vars, k+"="+v) + } + fw := &config.Framework{Env: config.FrameworkEnvConf{SQLite: &config.FrameworkServiceDef{ + Detect: tc.detect, + Vars: vars, + }}} + got, ok := SQLiteFileFromValues(tc.vals, fw) + if !ok || got != tc.want { + t.Errorf("resolved %q (ok=%v), want %q", got, ok, tc.want) + } + }) + } +} + +// A detect rule that does not match must not detect: a MySQL driver value is +// not a file database however the framework spells it. +func TestSQLiteFileFromValues_ARuleThatDoesNotMatchSaysNothing(t *testing.T) { + fw := &config.Framework{Env: config.FrameworkEnvConf{SQLite: &config.FrameworkServiceDef{ + Detect: []config.FrameworkServiceDetect{{Key: "Datasources.default.driver", ValuePrefix: `Cake\Database\Driver\Sqlite`}}, + }}} + vals := map[string]string{ + "Datasources.default.driver": `Cake\Database\Driver\Mysql`, + "Datasources.default.database": "app", + } + if got, ok := SQLiteFileFromValues(vals, fw); ok { + t.Errorf("a mysql driver detected as sqlite: %q", got) + } +} + +// Where the file gets created follows the same resolution the checks read by: +// never an absolute path or one that already exists, preferably a candidate +// whose parent directory the project already has. +func TestSQLiteCreationTarget(t *testing.T) { + drupalish := &config.Framework{PublicDir: "web"} + + t.Run("absolute is not lerd's to create", func(t *testing.T) { + if p, ok := SQLiteCreationTarget(t.TempDir(), nil, "/var/db/app.db"); ok { + t.Errorf("offered to create %q", p) + } + }) + + t.Run("an existing file needs nothing", func(t *testing.T) { + dir := t.TempDir() + os.MkdirAll(filepath.Join(dir, "database"), 0o755) + os.WriteFile(filepath.Join(dir, "database", "db.sqlite"), nil, 0o644) + if p, ok := SQLiteCreationTarget(dir, nil, "database/db.sqlite"); ok { + t.Errorf("offered to recreate %q", p) + } + }) + + t.Run("docroot candidate wins when its parent exists", func(t *testing.T) { + dir := t.TempDir() + os.MkdirAll(filepath.Join(dir, "web", "sites", "default", "files"), 0o755) + p, ok := SQLiteCreationTarget(dir, drupalish, filepath.Join("sites", "default", "files", ".ht.sqlite")) + want := filepath.Join(dir, "web", "sites", "default", "files", ".ht.sqlite") + if !ok || p != want { + t.Errorf("target %q (ok=%v), want %q", p, ok, want) + } + }) + + t.Run("project root stands when no parent exists", func(t *testing.T) { + dir := t.TempDir() + p, ok := SQLiteCreationTarget(dir, drupalish, "database/database.sqlite") + want := filepath.Join(dir, "database", "database.sqlite") + if !ok || p != want { + t.Errorf("target %q (ok=%v), want %q", p, ok, want) + } + }) +} diff --git a/internal/ui/databases.go b/internal/ui/databases.go index b295ed1ab..740cf06ee 100644 --- a/internal/ui/databases.go +++ b/internal/ui/databases.go @@ -58,26 +58,40 @@ type dbOwner struct { branch string } -// databaseSiteIndex maps each database name in the given engine to the site that -// owns it, resolved through each site's framework declaration and from the -// isolated databases worktrees have registered. A "_testing" database maps -// to the same owner as "", so both link to the same place. When a group +// databaseSiteIndexes maps each engine to the databases owned in it, keyed by +// database name, resolved through each site's framework declaration and from +// the isolated databases worktrees have registered. A "_testing" database +// maps to the same owner as "", so both link to the same place. When a group // shares one database across a main site and its secondaries, the database // belongs to the group main, so a secondary that merely shares it never wins // over the main. -func databaseSiteIndex(service string) map[string]dbOwner { +// +// Every engine is answered from one pass over the sites: resolving a site's +// targets detects its framework, which is far too much work to repeat per +// engine on every poll of the Databases tab. +func databaseSiteIndexes() map[string]map[string]dbOwner { reg, err := config.LoadSites() if err != nil { return nil } - idx := map[string]dbOwner{} + byService := map[string]map[string]dbOwner{} + idxFor := func(service string) map[string]dbOwner { + if byService[service] == nil { + byService[service] = map[string]dbOwner{} + } + return byService[service] + } // authoritative[db] is true once db is claimed by a site that owns it rather // than a secondary sharing the group's database. - authoritative := map[string]bool{} - claim := func(db string, owner dbOwner, owns bool) { - if _, seen := idx[db]; !seen || (!authoritative[db] && owns) { + authoritative := map[string]map[string]bool{} + claim := func(service, db string, owner dbOwner, owns bool) { + idx := idxFor(service) + if authoritative[service] == nil { + authoritative[service] = map[string]bool{} + } + if _, seen := idx[db]; !seen || (!authoritative[service][db] && owns) { idx[db] = owner - authoritative[db] = owns + authoritative[service][db] = owns } } domains := map[string]string{} @@ -86,35 +100,30 @@ func databaseSiteIndex(service string) map[string]dbOwner { continue } domains[s.Name] = s.PrimaryDomain() - db := "" + owns := !(s.IsGroupSecondary() && s.GroupSharedDB) + owner := dbOwner{domain: s.PrimaryDomain()} for _, t := range config.DBTargetsFor(s.Path) { - if t.Service == service { - db = t.Database - break + if t.Database == "" { + continue } + claim(t.Service, t.Database, owner, owns) + claim(t.Service, t.Database+testingDBSuffix, owner, owns) } - if db == "" { - continue - } - owns := !(s.IsGroupSecondary() && s.GroupSharedDB) - owner := dbOwner{domain: s.PrimaryDomain()} - claim(db, owner, owns) - claim(db+testingDBSuffix, owner, owns) } entries, err := config.LoadWorktreeDBRegistry() if err != nil { - return idx + return byService } for _, e := range entries { domain := domains[e.Site] - if e.Service != service || e.DBName == "" || domain == "" { + if e.DBName == "" || domain == "" { continue } owner := dbOwner{domain: domain, branch: e.Branch} - claim(e.DBName, owner, true) - claim(e.DBName+testingDBSuffix, owner, true) + claim(e.Service, e.DBName, owner, true) + claim(e.Service, e.DBName+testingDBSuffix, owner, true) } - return idx + return byService } // isDatabaseEngine reports whether a service belongs on the Databases surface: a @@ -156,7 +165,7 @@ func installedDBEngines() []string { // databaseEngine builds one engine's response, introspecting its databases and // snapshots only when the container is running. -func databaseEngine(name string) dbEngineResponse { +func databaseEngine(name string, siteIndex map[string]dbOwner) dbEngineResponse { base := buildServiceResponse(name) family := config.FamilyOfName(name) snapOps := serviceops.SnapshotSupported(name, false) @@ -187,7 +196,6 @@ func databaseEngine(name string) dbEngineResponse { eng.Error = err.Error() return eng } - siteIndex := databaseSiteIndex(name) for _, db := range dbs { owner := siteIndex[db.Name] entry := dbEntryResponse{ @@ -212,9 +220,10 @@ func databaseEngine(name string) dbEngineResponse { // handleDatabases lists every installed database engine and its databases. func handleDatabases(w http.ResponseWriter, _ *http.Request) { names := installedDBEngines() + indexes := databaseSiteIndexes() engines := make([]dbEngineResponse, 0, len(names)) for _, name := range names { - engines = append(engines, databaseEngine(name)) + engines = append(engines, databaseEngine(name, indexes[name])) } writeJSON(w, engines) } @@ -236,7 +245,7 @@ func handleDatabaseAction(w http.ResponseWriter, r *http.Request) { // GET /api/databases/ returns just that engine, for its detail tab. if len(parts) == 1 { if r.Method == http.MethodGet { - writeJSON(w, databaseEngine(service)) + writeJSON(w, databaseEngine(service, databaseSiteIndexes()[service])) return } http.Error(w, "not found", http.StatusNotFound) diff --git a/internal/ui/databases_site_index_test.go b/internal/ui/databases_site_index_test.go index 01ea6cfa4..8362eb692 100644 --- a/internal/ui/databases_site_index_test.go +++ b/internal/ui/databases_site_index_test.go @@ -52,7 +52,7 @@ func TestDatabaseSiteIndex_WordPressSiteOwnsItsDatabase(t *testing.T) { t.Fatal(err) } - if got := databaseSiteIndex("mysql")["blog"]; got.domain != "blog.test" { + if got := databaseSiteIndexes()["mysql"]["blog"]; got.domain != "blog.test" { t.Errorf("blog database = %+v, want domain blog.test", got) } } @@ -74,7 +74,7 @@ func TestDatabaseSiteIndex_IsolatedWorktreeDBCarriesItsBranch(t *testing.T) { t.Fatal(err) } - idx := databaseSiteIndex("mysql") + idx := databaseSiteIndexes()["mysql"] if got := idx["astrolov"]; got.domain != "astrolov.test" || got.branch != "" { t.Errorf("parent database = %+v, want domain astrolov.test with no branch", got) @@ -102,7 +102,7 @@ func TestDatabaseSiteIndex_IgnoresWorktreeDBsOnAnotherService(t *testing.T) { t.Fatal(err) } - if got, ok := databaseSiteIndex("mysql")["astrolov_staging"]; ok { + if got, ok := databaseSiteIndexes()["mysql"]["astrolov_staging"]; ok { t.Errorf("postgres worktree database surfaced on mysql: %+v", got) } } diff --git a/internal/ui/entities_test.go b/internal/ui/entities_test.go index 553890b23..2d62eed27 100644 --- a/internal/ui/entities_test.go +++ b/internal/ui/entities_test.go @@ -57,7 +57,7 @@ func TestSortEntityActions(t *testing.T) { // typed as an array on the client and a null breaks iteration over it. func TestDatabaseEntryAlwaysCarriesASnapshotList(t *testing.T) { t.Setenv("XDG_CONFIG_HOME", t.TempDir()) - eng := databaseEngine("nosuchengine") + eng := databaseEngine("nosuchengine", nil) for _, db := range eng.Databases { if db.Snapshots == nil { t.Errorf("database %q carries a nil snapshot list", db.Name) diff --git a/internal/ui/runs.go b/internal/ui/runs.go index 1cd2a27a7..2a3c46f24 100644 --- a/internal/ui/runs.go +++ b/internal/ui/runs.go @@ -18,6 +18,11 @@ import ( // of it, and the buffer is what a reload reads back. const runMaxLines = 2000 +// runMaxLineBytes caps one line of output. A command that writes progress with +// a bare carriage return produces a single line megabytes long; it is emitted +// in pieces this size rather than held whole. +const runMaxLineBytes = 1024 * 1024 + // runRetention is how long a finished run stays readable, so a page that // reloads just as the run ends still finds its result rather than a 404. const runRetention = 30 * time.Minute @@ -224,10 +229,25 @@ var execRun = func(ctx context.Context, r *run, argv []string, dir string) error close(waited) }() - scanner := bufio.NewScanner(pr) - scanner.Buffer(make([]byte, 0, 64*1024), 1024*1024) - for scanner.Scan() { - r.append(scanner.Text()) + // Nothing else reads this pipe, so the drain has to reach EOF whatever the + // command writes: a reader that gave up on a line longer than its buffer + // would leave the command blocked writing to it and cmd.Wait below never + // returning. An over-long line is emitted in pieces instead of abandoned. + reader := bufio.NewReaderSize(pr, 64*1024) + var line []byte + for { + chunk, isPrefix, err := reader.ReadLine() + if err != nil { + break + } + line = append(line, chunk...) + if !isPrefix || len(line) >= runMaxLineBytes { + r.append(string(line)) + line = line[:0] + } + } + if len(line) > 0 { + r.append(string(line)) } <-waited return waitErr @@ -244,6 +264,11 @@ func (reg *runRegistry) Get(id string) (*run, bool) { // ForDir lists the runs started for a directory, newest first, so a reloaded // wizard finds the work it left behind rather than starting it again. func (reg *runRegistry) ForDir(dir string) []runSnapshot { + // Also swept here, not only when a run starts: a machine that scaffolded one + // project and was left alone would otherwise hold that run, its buffered + // output and its place in every listing for as long as lerd-ui is up. + reg.sweep() + reg.mu.Lock() all := make([]*run, 0, len(reg.runs)) for _, r := range reg.runs { diff --git a/internal/ui/runs_http.go b/internal/ui/runs_http.go index 88e6e42ae..c5f61a60a 100644 --- a/internal/ui/runs_http.go +++ b/internal/ui/runs_http.go @@ -178,7 +178,11 @@ func handleRunStream(w http.ResponseWriter, r *http.Request) { lines, next, done := current.read(from) from = next for _, line := range lines { - fmt.Fprintf(w, "data: %s\n\n", strings.ReplaceAll(line, "\\", "\\\\")) + // SSE ends a field at a carriage return as readily as at a newline, so + // the \r a progress bar writes mid-line would split one line of output + // into two frames. Nothing else needs escaping: the client reads the + // payload as it arrives. + fmt.Fprintf(w, "data: %s\n\n", strings.ReplaceAll(line, "\r", "")) } flusher.Flush() if done { diff --git a/internal/ui/runs_test.go b/internal/ui/runs_test.go index 45d7967d7..75b21bb95 100644 --- a/internal/ui/runs_test.go +++ b/internal/ui/runs_test.go @@ -240,6 +240,49 @@ func TestHandleRunStreamReplaysAndCloses(t *testing.T) { } } +// The stream is read by a client that does not unescape anything, so a line +// reaches the log exactly as it is written here. A PHP namespace is the case +// that matters: it is what a failed scaffold prints. +func TestHandleRunStreamWritesLinesVerbatim(t *testing.T) { + stubRunExec(t, func(r *run) error { + r.append(`Illuminate\Foundation\Bootstrap\HandleExceptions`) + return nil + }) + r := runs.Start(runKindSetup, t.TempDir(), "", []string{"lerd", "setup"}) + waitForStatus(t, r, runDone) + + req := httptest.NewRequest(http.MethodGet, "/api/runs/"+r.ID+"/stream", nil) + rr := httptest.NewRecorder() + handleRunStream(rr, req) + + if out := rr.Body.String(); !strings.Contains(out, `data: Illuminate\Foundation\Bootstrap\HandleExceptions`) { + t.Errorf("backslashes did not survive the stream unchanged: %q", out) + } +} + +// A carriage return inside a line would end the SSE data field early and split +// one line of output into a frame the client reads as another event. +func TestHandleRunStreamDropsEmbeddedCarriageReturns(t *testing.T) { + stubRunExec(t, func(r *run) error { + r.append("Downloading: 40%\rDownloading: 100%") + return nil + }) + r := runs.Start(runKindSetup, t.TempDir(), "", []string{"lerd", "setup"}) + waitForStatus(t, r, runDone) + + req := httptest.NewRequest(http.MethodGet, "/api/runs/"+r.ID+"/stream", nil) + rr := httptest.NewRecorder() + handleRunStream(rr, req) + + out := rr.Body.String() + if strings.Contains(out, "\r") { + t.Errorf("a carriage return reached the stream and split the frame: %q", out) + } + if !strings.Contains(out, "data: Downloading: 40%Downloading: 100%") { + t.Errorf("the line did not survive: %q", out) + } +} + func TestHandleRunStreamUnknownRunIs404(t *testing.T) { req := httptest.NewRequest(http.MethodGet, "/api/runs/nope/stream", nil) rr := httptest.NewRecorder() @@ -277,3 +320,48 @@ func TestHandleRunsListsEveryRunWithoutADirectory(t *testing.T) { t.Errorf("the run is missing from the unfiltered listing: %+v", out.Runs) } } + +// Nothing else reads the pipe the command writes to, so a reader that gives up +// on an over-long line leaves the command blocked writing to it and the run +// running for ever. Composer and npm both write progress as one long line. +func TestExecRunSurvivesAnOverlongLine(t *testing.T) { + r := &run{ID: "x", status: runRunning} + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + defer cancel() + + done := make(chan error, 1) + go func() { + done <- execRun(ctx, r, []string{"sh", "-c", `head -c 2000000 /dev/zero | tr '\0' A; echo; echo tail`}, t.TempDir()) + }() + + select { + case err := <-done: + if err != nil { + t.Fatalf("execRun: %v", err) + } + case <-time.After(15 * time.Second): + t.Fatal("execRun never returned: the reader stopped draining the pipe") + } + + lines, _, _ := r.read(0) + if len(lines) == 0 || lines[len(lines)-1] != "tail" { + t.Errorf("output after the long line was lost: %d lines", len(lines)) + } +} + +// Retention has to mean what the constant says on an idle daemon too, not only +// on one that keeps being given new work. +func TestRunRegistryReleasesFinishedRunsWithoutANewRun(t *testing.T) { + stubRunExec(t, func(r *run) error { return nil }) + dir := t.TempDir() + r := runs.Start(runKindSetup, dir, "", []string{"lerd", "setup"}) + waitForStatus(t, r, runDone) + + r.mu.Lock() + r.finished = time.Now().Add(-2 * runRetention) + r.mu.Unlock() + + if got := runs.ForDir(dir); len(got) != 0 { + t.Errorf("a run past its retention was still listed: %v", got) + } +} diff --git a/internal/ui/web/src/modals/SiteWizardModal.svelte b/internal/ui/web/src/modals/SiteWizardModal.svelte index b29d02039..d44930100 100644 --- a/internal/ui/web/src/modals/SiteWizardModal.svelte +++ b/internal/ui/web/src/modals/SiteWizardModal.svelte @@ -218,6 +218,10 @@ async function scaffold() { if (!name.trim()) return; + // The target is known before the run starts, and persisting it now is what + // lets a resume carry on into the questions even after the finished run has + // aged out of the registry. + dir = joinPath(parent, name.trim()); const ok = await runAndWait( { kind: 'scaffold', dir: parent, name: name.trim(), framework, framework_version: frameworkVersion }, m.siteWizard_scaffolding() @@ -381,6 +385,17 @@ running = true; runTitle = known.label || m.siteWizard_scaffolding(); logs = []; + // The queue that continues after a setup run reads each step's optional + // flag off the plan, which a reopened modal has not loaded yet; without + // it every remaining step counts as required and one optional failure + // stops the rest. + if (runKind === 'setup' && dir) { + try { + steps = await setupSteps(dir); + } catch { + steps = []; + } + } // The run panel takes the body from here: following a run that is still // going does not return until it ends, and holding the loader up for // that long is what hid the output the user came back to watch. @@ -396,6 +411,9 @@ // Nothing left to reattach to: rebuild the step from the project on disk. if (!dir) return; resuming = false; + // A scaffold whose run has aged out of the registry left its project on + // disk; the questions are what comes after it, same as afterRun. + if (step === 'create') await loadQuestions(); if (step === 'questions') await loadQuestions(); if (step === 'setup') await loadSetupSteps(); } finally { diff --git a/internal/ui/web/src/modals/SiteWizardModal.test.ts b/internal/ui/web/src/modals/SiteWizardModal.test.ts index 04ea1bdf4..cabd90027 100644 --- a/internal/ui/web/src/modals/SiteWizardModal.test.ts +++ b/internal/ui/web/src/modals/SiteWizardModal.test.ts @@ -246,6 +246,75 @@ describe('SiteWizardModal', () => { }); }); + // The plan is what says which steps are optional. A resumed queue that never + // loaded it would treat every remaining step as required, so one optional + // failure would stop the rest. + it('keeps optional steps optional when the queue resumes', async () => { + localStorage.setItem( + 'lerd.siteWizard', + JSON.stringify({ + step: 'setup', + dir: '/home/u/acme', + runId: 'run-setup-head', + runKind: 'setup', + queue: ['composer install', 'npm audit', 'npm run build'] + }) + ); + runsForDir.mockResolvedValue([ + { id: 'run-setup-head', kind: 'setup', dir: '/home/u/acme', status: 'done', started: 0 } + ]); + setupSteps.mockResolvedValue([ + { label: 'composer install', enabled: true, optional: false }, + { label: 'npm audit', enabled: true, optional: true }, + { label: 'npm run build', enabled: true, optional: false } + ]); + startRun.mockImplementation(async (req: { kind: string; steps?: string[] }) => ({ + id: 'run-' + (req.steps?.[0] ?? req.kind), + kind: req.kind, + dir: '/home/u/acme', + status: 'done' as const, + started: 0 + })); + streamRun.mockImplementation(async (id: string, onEvent: (e: unknown) => void) => { + onEvent({ done: true, ok: id !== 'run-npm audit' }); + }); + + render(SiteWizardModal); + + await waitFor(() => { + const setupCalls = startRun.mock.calls + .map((c) => c[0] as { kind: string; steps?: string[] }) + .filter((c) => c.kind === 'setup') + .map((c) => c.steps?.[0]); + // The optional audit failed and the build after it still ran. + expect(setupCalls).toEqual(['npm audit', 'npm run build']); + }); + }); + + // A finished run is only kept for so long. A scaffold parked past that is + // still a scaffolded project on disk, and reopening the wizard carries it + // into the questions instead of dead-ending on the create form. + it('continues a scaffold whose run has aged out of the registry', async () => { + localStorage.setItem( + 'lerd.siteWizard', + JSON.stringify({ + step: 'create', + dir: '/home/u/acme', + parent: '/home/u', + name: 'acme', + runId: 'run-scaffold', + runKind: 'scaffold' + }) + ); + runsForDir.mockResolvedValue([]); + + render(SiteWizardModal); + + await waitFor(() => { + expect(projectQuestions).toHaveBeenCalledWith('/home/u/acme'); + }); + }); + // Coming back to a run that is still going has to show the run, not a // spinner: watching the output is the whole point of reopening it. it('shows the live run rather than a loader when it reattaches', async () => {