diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl b/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl index 3e132fcc23..d914eb9edb 100644 --- a/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl +++ b/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl @@ -152,3 +152,4 @@ {"date": "2026-10-03", "area": "cmd/mxcli/check", "symptom": "`mxcli check -p` prints MDL067 (bare commit now WITH events) for a create-or-modify flow already stored with events, which `exec -p` no longer prints", "cause": "cmd_check ran ValidateProgram without the DropSettledCommitNotes filter exec_preflight applies; the project was only connected later, for the reference tier", "fix": "check connects to the project before the semantic report when -p is given, applies DropSettledCommitNotes and StoredTaskClaimViolations, and reuses that connection for the reference tier", "insight": "Two gates over one rule set drift whenever a post-filter lives in only one of them; grep for every caller of ValidateProgram when adding a filter. Control: a flow stored without events still notes", "issue": "ako/mxcli#943", "file": "cmd/mxcli/cmd_check.go", "test": "cmd/mxcli/check_stored_semantics_test.go"} {"date": "2026-10-03", "area": "cmd/mxcli/test", "symptom": "every `mxcli test` run prints 2x MDL-DEPR001 and 2x MDL-V1-SLASH about a script the user never wrote", "cause": "GenerateEndpointMDL emitted a headerless mdl 0 script with `create or replace` and `/` terminators; the test-flow generators had already moved to the version-aware writeScriptHeader/createFlow/writeFlowEnd", "fix": "GenerateEndpointMDL writes mdl 1 through the same helpers (header, create or modify, `;` only); endpoint script is independent of the suite's version", "insight": "A generated script is checked like a user's one; pin it with a test that parses it and asserts ValidateProgram returns nothing. Verified end to end with `mxcli test --local` on a fresh 11.13 app", "issue": "ako/mxcli#943", "file": "cmd/mxcli/testrunner/endpoint.go", "test": "cmd/mxcli/testrunner/endpoint_clean_test.go"} {"date": "2026-10-03", "area": "cmd/mxcli/theme", "symptom": "`theme create acme --from design.css` with `--mxt-font: \"Inter\", system-ui, sans-serif` prints nothing about Inter; the theme ships no woff2 and no @font-face for it and renders in the fallback font wherever Inter is not installed", "cause": "planFonts only decided which VENDORED families to drop; a seeded family outside the vendored set was never looked at, so the silent outcome was the default", "fix": "unvendoredSeededFamilies takes the primary (first) family of each seeded font stack, skips generic families and var() and the families the base partial loads, and CreateResult.UnvendoredFonts carries them to cmd_theme.go, which prints a note per family naming mxcli-fonts/ and the partial", "insight": "Only the first family of a stack is the design's choice; flagging the fallbacks (Helvetica, Arial) would make the note noise. The controls are a vendored family (IBM Plex Mono) and a generic stack, which must stay silent", "issue": "ako/mxcli#944", "file": "cmd/mxcli/theme/create_seeded.go (unvendoredSeededFamilies, planFonts); cmd/mxcli/cmd_theme.go", "test": "cmd/mxcli/theme/create_seeded_test.go (TestCreate_NamesSeededFontsItDoesNotVendor)"} +{"area": "cmd/mxcli/docker", "date": "2026-10-03", "symptom": "`mxcli docker check` (a check) rewrote an MPRv1 project's .mpr permanently, and on v1 and v2 alike rewrote theme-cache/web/theme.compiled.css(.map) and created deployment/sass/main.scss \u2014 with or without --no-update-widgets", "cause": "update-widgets ran on the user's project, protected only by a snapshot/restore of the v2 storage (.mpr + mprcontents/), so v1 had no protection; and `mx check` itself compiles the theme into theme-cache/ and writes deployment/sass/, which nothing guarded", "file": "`cmd/mxcli/docker/check.go` (`Check`), `cmd/mxcli/docker/check_copy.go` (`copyProjectForCheck`)", "insight": "A snapshot of the files you expect a tool to touch protects only those files; measure with a whole-tree hash+mtime diff before/after, which is what showed that plain `mx check` writes too. The fix is to run both mx steps on a temporary copy (skipping deployment/, releases/, theme-cache/, .git, node_modules) and rewrite the copy's path back in mx output. Control: a CE0117 microflow is still reported on both formats with the tree unchanged. `docker build` keeps runUpdateWidgets because it is expected to write deployment/ \u2014 but it still rewrites a v1 .mpr", "refs": ["ako/mxcli#951", "ako/mxcli#568", "ako/mxcli#646"]} diff --git a/.claude/skills/fix-issue/findings/mdl-other.jsonl b/.claude/skills/fix-issue/findings/mdl-other.jsonl index 261f84b4f3..bc8bfa9023 100644 --- a/.claude/skills/fix-issue/findings/mdl-other.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-other.jsonl @@ -90,3 +90,4 @@ {"area": "mdl/catalog", "date": "2026-10-03", "symptom": "`delete $Order` / `change $Order (…)` inside `loop $Order in $Orders` produce no refs row (no delete/change edge to the entity), while `delete $Orders` on the list does; same for the output of `retrieve $Cust from $Order/Mod.Assoc`. show references / impact under-report batch flows.", "cause": "buildVarEntityMap seeded only object/list parameters and create / database-retrieve outputs, from a flattened action list that had already lost the loop's IterableList; loop iterators and association-retrieve outputs never got an entity, so microflowVarActionRef could not resolve them.", "file": "`mdl/catalog/builder_references.go` (buildVarEntityMap, associationTarget, associationEnds)", "insight": "Walk the object collection, not the flattened actions: the iterator's type lives on the LoopedActivity. Objects is not flow order, so map to a fixpoint (first assignment wins, which also bounds it). An association retrieve's output is the OTHER end from the start variable's entity; when the start is neither end (a specialization) leave it unmapped rather than guess. Control in the test: the list delete that always resolved.", "refs": ["mendixlabs/mxcli#1266"]} {"area": "mdl/linter", "date": "2026-10-03", "symptom": "lint CONV013 reports \"Java action call ... uses '' error handling instead of Custom\" on calls that have `on error { \u2026 }`, and CONV014 never fires on `on error continue` on an action", "cause": "Both rules read BaseActivity.ErrorHandlingType, which the model reader never fills: Mendix stores an action activity's error handling on the ACTION (Microflows$JavaActionCallAction.ErrorHandlingType). The '' in the message was the empty field", "file": "`mdl/linter/rules/conv_error_handling.go`; shared reader `sdk/microflows/error_handling.go` (`ObjectErrorHandlingType`)", "insight": "The unit tests had always set the activity field by hand, so they passed against a shape the reader never produces. Build test objects the way flowObjectFromGen does. Two private reflection helpers (executor DESCRIBE, MCP backend) already read the action correctly; the rules had a third, wrong copy. A '' interpolated into a diagnostic is the cheapest tell of a never-populated field", "refs": ["mendixlabs/mxcli#1202"], "rules": ["CONV013", "CONV014"]} {"area": "mdl/catalog", "date": "2026-10-03", "symptom": "activities table / activities_for() has no rows for anything inside a loop (nested loops included) in a microflow, nanoflow or rule; a Starlark rule cannot find a retrieve, commit or delete in a loop", "cause": "buildMicroflows had three near-copy loops (microflow, nanoflow, rule) over ObjectCollection.Objects that never recursed into LoopedActivity.ObjectCollection, although the reader fills it; countDecisionPoints beside them did recurse", "file": "`mdl/catalog/builder_microflows.go` (`insertFlowActivities`, `countFlowActivities`)", "insight": "Three copies of one walk is how the gap stayed in all three flavours. One shared walker writes ParentLoopId/LoopDepth; activities_for() keeps its top-level default (filtering ParentLoopId = '') so bundled rules such as CONV010 keep their counts, and ActivityCount keeps its top-level meaning beside a new TotalActivityCount. Raw SQL over activities now sees loop-body rows", "refs": ["mendixlabs/mxcli#1266"]} +{"area": "mdl/catalog", "date": "2026-10-03", "symptom": "Parallel mxcli processes on one project (8x `lint` on a fresh copy) print `Warning: failed to save catalog cache: failed to create table catalog_meta: table catalog_meta already exists` or `database is locked (SQLITE_BUSY)`; a reader can open a half-written .mxcli/catalog.db", "cause": "buildCatalog removed the cache and SaveToFile wrote into the path in place: VACUUM INTO refuses a non-empty target another process had just created, and the manual-copy fallback then CREATE TABLEd into that same file. Opening a cache (NewFromFile) also always wrote (createTables + schema_version row), so concurrent openers contended for the write lock", "file": "`mdl/catalog/catalog.go` (`SaveToFile`, `NewFromFile`), `mdl/catalog/catalogdb_sqlite.go`, `mdl/executor/cmd_catalog.go` (buildCatalog save)", "insight": "Write to a temp file in the same directory and os.Rename it over the cache \u2014 but a rename alone moves the failure to readers: SQLite refuses a write on a file renamed out from under an open connection (SQLITE_READONLY_DBMOVED, 'attempt to write a readonly database', 1032). So opening a cache at the current schema version must not write at all; the busy_timeout in the DSN covers the remaining writes of an old-version cache. An in-process test with 8 goroutine writers + 4 reader loops reproduces all three errors deterministically, no subprocesses needed", "refs": ["ako/mxcli#951"]} diff --git a/.claude/skills/mendix/custom-widgets/SKILL.md b/.claude/skills/mendix/custom-widgets/SKILL.md index 7826dc847a..5073ff1b55 100644 --- a/.claude/skills/mendix/custom-widgets/SKILL.md +++ b/.claude/skills/mendix/custom-widgets/SKILL.md @@ -303,15 +303,15 @@ MDL032). **CE0463 "update this widget" is EXPECTED after generating charts.** mxcli writes the WidgetType from an embedded 11.6 baseline; the installed Charts.mpk is a -different version, so Studio Pro/mxbuild flags drift. Clear it with **`mxcli docker -check`/`build`** (they normalize the widgets and preserve your storage format). The -whole `mdl-examples/doctype-tests/34-chart-widget-examples.mdl` builds **0 errors** -after normalization. +different version, so Studio Pro/mxbuild flags drift. Clear it with **`mxcli fix +widgets`** (keeps your storage format); `docker check` only normalizes a temp copy, +so check the stored project with `--no-update-widgets`. The whole +`mdl-examples/doctype-tests/34-chart-widget-examples.mdl` builds **0 errors** after. **Do NOT run bare `mx update-widgets` on an MPRv2 project** (an `mprcontents/`-folder project — what `mxcli new` creates): it converts the project to single-file v1 and **deletes `mprcontents/`**, corrupting git, breaking a running `mxcli run --local` -loop, and sometimes making the project unopenable in Studio Pro. `mxcli docker -check`/`build` snapshot/restore the v2 files around the normalization; raw +loop, and sometimes making the project unopenable in Studio Pro. `mxcli fix widgets` +writes the result back as v2, `mxcli docker check` runs on a temporary copy; raw `mx update-widgets` is only safe on a v1 project or a throwaway diagnostic copy. **DESCRIBE round-trips** series/line/scalecolor object-lists (item names are diff --git a/.claude/skills/mendix/migrate-design-prototype/SKILL.md b/.claude/skills/mendix/migrate-design-prototype/SKILL.md index b45d5e165c..f9fc503679 100644 --- a/.claude/skills/mendix/migrate-design-prototype/SKILL.md +++ b/.claude/skills/mendix/migrate-design-prototype/SKILL.md @@ -347,8 +347,8 @@ too — each `series`/`line` binds its own OQL-view datasource + X/Y attributes; Pie/HeatMap bind at the widget level (`ValueAttribute:`, Pie needs `SeriesName:`). See **[Custom & Pluggable Widgets → Charts](../custom-widgets/SKILL.md)** for the chart-type → id table, per-chart required-property gotchas (TimeSeries needs a datetime X, -Bubble needs a size attribute), and the **CE0463 → `mxcli docker check`/`build`** step -(these normalize widgets *and* preserve MPRv2 storage — never run bare +Bubble needs a size attribute), and the **CE0463 → `mxcli fix widgets`** step +(it normalizes the stored widgets *and* preserves MPRv2 storage — never run bare `mx update-widgets` on a `mxcli new` project; it deletes `mprcontents/`). `mdl-examples/doctype-tests/34-chart-widget-examples.mdl` is the full showcase. @@ -659,8 +659,8 @@ the fast index so a design migration doesn't rediscover them. (`Charts.mpk`: column/bar/line/area/pie)** now author via MDL — each `series` (an object-list item inside the chart) binds a datasource plus X/Y attributes: `series s1 (staticDataSource: database from Module.View, staticXAttribute: "X", staticYAttribute: "Y")` - (a per-series OQL view works too). `mxcli docker check`/`build` clear the - widget-version-drift CE0463 (they normalize the widgets and preserve MPRv2 storage — + (a per-series OQL view works too). `mxcli fix widgets` clears the + widget-version-drift CE0463 (it normalizes the stored widgets and preserves MPRv2 storage — do not run bare `mx update-widgets`, which deletes `mprcontents/`). Still lighter when the design allows: a **CSS-background SVG** container (or `HTMLElement`) for sparklines/trends — no datasource — and `ProgressCircle` (`type: expression`, `expressionCurrentValue: '$currentObject/Rate'`, min `'0'` / max `'100'`, diff --git a/CHANGELOG.md b/CHANGELOG.md index f78ba22a38..06feea8c5e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **`docker check` no longer modifies the project** (ako/mxcli#951) — `mx update-widgets` and `mx check` now run on a temporary copy, for MPR v1 and v2 alike, and what mx prints names the project's own paths. Before, a check rewrote an MPR v1 project's `.mpr` permanently (only v2 was restored from a snapshot), and `mx check` itself rewrote `theme-cache/` and created `deployment/sass/` even with `--no-update-widgets`. The output now says that widget definitions were normalised on a copy, and that a CE0463 the stored project still has is therefore not reported: `--no-update-widgets` checks the project as stored, `mxcli fix widgets` applies the normalisation (ako/mxcli#568, #646). Build output, caches and VCS folders are not copied; the copy goes to `$TMPDIR` and is removed afterwards. +- **Parallel mxcli runs on one project no longer fail to save the catalog cache** (ako/mxcli#951) — eight parallel `lint` runs on a fresh copy printed `failed to create table catalog_meta: table catalog_meta already exists` or `database is locked`. The cache is now written to a temporary file next to it and renamed into place, so every run saves and a reader sees the old cache or the new one, never a half-written file; opening a current cache no longer writes to it. - **`create or modify` of a flow matches `commit … with events`, a legacy `split type` spelling and an empty `else` against what is stored** (ako/mxcli#942). Describe prints a stored commit as a bare `commit`, the `when … then` split form, and no empty `else`. The statement diff compared the spelling, so these never matched their own activity. An unchanged re-run reported "Unchanged … (spliced: 1 replaced)". A change inside a loop body next to such a statement was not refused under `mdl 1`, and the loop was rebuilt with new element IDs. `without events` is still a change. **The loop-body refusal also holds when another statement changes next to the loop:** before, only a loop-body change on its own was refused. **`describe` no longer warns that the merge closing an `if` at the end of a loop body "joins no decision"** and would be deleted. The check counted a loop body's flows from the loop's own collection, which holds none. - **Less noise from `check` and `test`** (ako/mxcli#943) — **MDL-WORKFLOW10** no longer warns when the task is claimed in a called microflow: a callee the script creates is read (nested calls too), and with `-p` a stored one; a call that passes the task to a microflow neither can find counts as a possible claim. A callee that does not claim the task it is passed still warns. **`mxcli test`** no longer prints MDL-DEPR001 / MDL-V1-SLASH warnings about the endpoint-registration script it generates itself: that script is `mdl 1`. **`check -p`** drops **MDL067** for a commit already stored the way the script writes it, as `exec` already did. **MDL-WIDGET15** skips a dynamictext with its own `class:` or `style:` (a laid-out label/value pair is not fused text), names the two widgets, and every widget-rule diagnostic now carries its page or snippet as its location instead of "(no module)". - **A required caption with no text in the default language is foreseen** (ako/mxcli#944) — making de_DE the default left a stock app's `Administration.Account_Overview` `tabPage2` (en_US only) empty in de_DE, and mxbuild refused it with CE4899 "Empty caption. [German, Germany]" while `check -p`, `lint` and `exec` said nothing. Measured on 11.14, the tab page caption is the one caption kind the build requires (page titles, buttons, labels, group boxes, column headers, menu items, enumeration captions and messages build without it; page templates and building blocks are not checked). Now: **lint rule QUAL006** (error) lists every tab page caption without the default language; **`alter settings language (DefaultLanguageCode: …)`** prints how many there are and where, with the `alter page … { set (Caption: …) on … }` that fixes one; **`check -p` reports MDL-I18N01** for a script that changes the default — stored captions, and captions the script wrote before the change. And a page created **after** the change in the same script is now written in the new default: the authoring language was resolved once per session, so it was still written in the old one and failed the build too. diff --git a/cmd/mxcli/docker.go b/cmd/mxcli/docker.go index 1c41ee9187..ea2ac40cc9 100644 --- a/cmd/mxcli/docker.go +++ b/cmd/mxcli/docker.go @@ -169,9 +169,18 @@ This catches project errors (broken references, missing attributes, etc.) early, before the slower MxBuild step. The 'docker build' command runs this automatically unless --skip-check is used. -By default, 'mx update-widgets' runs before 'mx check' to normalize -pluggable widget definitions and prevent false CE0463 errors. Use ---no-update-widgets to skip this step. +The check never modifies the project: both mx steps run on a temporary +copy (in $TMPDIR), because mx writes into the project it checks — it +compiles the theme into theme-cache/ and deployment/, and update-widgets +rewrites the model. Build output, caches and VCS folders (deployment/, +releases/, theme-cache/, .git/, node_modules/, ...) are not copied. + +By default, 'mx update-widgets' runs before 'mx check', on that copy, to +normalize pluggable widget definitions and prevent false CE0463 errors. +A CE0463 that normalisation clears is then not reported, although it still +fails MxBuild and 'mxcli run --local' on the stored project. Use +--no-update-widgets to check the project as stored, and 'mxcli fix widgets' +to apply the normalisation to the project. The mx binary is located from the same directory as mxbuild. diff --git a/cmd/mxcli/docker/check.go b/cmd/mxcli/docker/check.go index 91815a0afb..0167520123 100644 --- a/cmd/mxcli/docker/check.go +++ b/cmd/mxcli/docker/check.go @@ -21,8 +21,9 @@ type CheckOptions struct { MxBuildPath string // SkipUpdateWidgets skips the 'mx update-widgets' step before checking. - // By default, update-widgets runs first to normalize pluggable widget - // definitions and prevent false CE0463 errors. + // By default, update-widgets runs first, on the temporary copy the check + // uses, to normalize pluggable widget definitions and prevent false CE0463 + // errors. SkipUpdateWidgets bool // Stdout for output messages. @@ -56,7 +57,24 @@ func copyFile(src, dst string) error { return out.Close() } +// resolveMxForCheck and mxCheckCmd are seams for tests, which substitute stubs +// that behave like the real tools without needing mx. +var resolveMxForCheck = ResolveMxForVersion + +var mxCheckCmd = func(mxPath, mprPath string, w, stderr io.Writer) error { + cmd := exec.Command(mxPath, "check", mprPath) + cmd.Stdout = w + cmd.Stderr = stderr + PrepareMxCommand(cmd) + return cmd.Run() +} + // Check runs 'mx check' on the project to validate it before building. +// +// It never modifies the project. `mx update-widgets` rewrites the model (and +// turns an MPRv2 project into MPRv1), and `mx check` itself writes theme-cache/ +// and deployment/sass/ — so both run on a temporary copy, and what mx prints is +// reported against the project's own paths (ako/mxcli#951). func Check(opts CheckOptions) error { w := opts.Stdout if w == nil { @@ -76,30 +94,56 @@ func Check(opts CheckOptions) error { } } - mxPath, err := ResolveMxForVersion(opts.MxBuildPath, projectVersion) + mxPath, err := resolveMxForCheck(opts.MxBuildPath, projectVersion) if err != nil { return err } fmt.Fprintf(w, "Using mx: %s\n", mxPath) - // Normalize pluggable widget definitions so `mx check` does not report false - // CE0463 ("widget definition changed") errors. runUpdateWidgets preserves the - // project's on-disk storage format; restore is deferred so the check below still - // runs against the widget-normalized model. + projectPath := opts.ProjectPath + if abs, err := filepath.Abs(projectPath); err == nil { + projectPath = abs + } + workMpr, cleanup, err := copyProjectForCheck(projectPath) + if err != nil { + return fmt.Errorf("copy the project to a temporary directory for checking: %w\n"+ + " mx writes into the project it checks, so docker check never runs it on the original;\n"+ + " set TMPDIR to a disk with room for the project", err) + } + defer cleanup() + out := newPathRewriter(w, filepath.Dir(workMpr), filepath.Dir(projectPath)) + errOut := newPathRewriter(stderr, filepath.Dir(workMpr), filepath.Dir(projectPath)) + defer out.Flush() + defer errOut.Flush() + fmt.Fprintln(w, "Checking a temporary copy (mx writes into the project it checks; the project on disk is not changed).") + + // Normalize pluggable widget definitions so `mx check` does not report + // CE0463 ("widget definition changed") for definitions that only need a + // resync. On the copy, so neither the model nor its storage format changes. if !opts.SkipUpdateWidgets { - restore := runUpdateWidgets(mxPath, opts.ProjectPath, w, stderr) - defer restore() + fmt.Fprintln(w, "Normalising widget definitions on the temporary copy...") + if err := updateWidgetsCmd(mxPath, workMpr, out, errOut); err != nil { + out.Flush() + fmt.Fprintf(w, "Warning: update-widgets failed (continuing): %v\n", err) + } } // Run mx check fmt.Fprintf(w, "Checking project %s...\n", opts.ProjectPath) - cmd := exec.Command(mxPath, "check", opts.ProjectPath) - cmd.Stdout = w - cmd.Stderr = stderr - PrepareMxCommand(cmd) + checkErr := mxCheckCmd(mxPath, workMpr, out, errOut) + out.Flush() + errOut.Flush() - if err := cmd.Run(); err != nil { - return fmt.Errorf("project check failed: %w", err) + if !opts.SkipUpdateWidgets { + // #568 / #646: the normalisation can hide a CE0463 the stored project + // still has, which then fails `run --local` and MxBuild. + fmt.Fprintln(w, "Note: widget definitions were normalised on a temporary copy before checking, so a") + fmt.Fprintln(w, " CE0463 the stored project still has is not reported here; it fails MxBuild and") + fmt.Fprintln(w, " `mxcli run --local`. Use --no-update-widgets to check the project as stored, and") + fmt.Fprintln(w, " `mxcli fix widgets` to apply the normalisation to the project.") + } + if checkErr != nil { + return fmt.Errorf("project check failed: %w", checkErr) } fmt.Fprintln(w, "Project check passed.") diff --git a/cmd/mxcli/docker/check_copy.go b/cmd/mxcli/docker/check_copy.go new file mode 100644 index 0000000000..4c9106749a --- /dev/null +++ b/cmd/mxcli/docker/check_copy.go @@ -0,0 +1,167 @@ +// SPDX-License-Identifier: Apache-2.0 + +package docker + +import ( + "bytes" + "fmt" + "io" + "io/fs" + "os" + "path/filepath" + "strings" + "sync" +) + +// copyProjectForCheck copies the project that holds mprPath into a fresh +// temporary directory, so `docker check` can run `mx update-widgets` and +// `mx check` there instead of on the user's project (ako/mxcli#951). +// +// Both tools write into the project they are given: update-widgets rewrites the +// model (and converts MPRv2 to MPRv1), and mx check compiles the theme into +// theme-cache/ and writes deployment/sass/. A snapshot/restore of the v2 storage +// covered only the first half of that, and only on v2 — an MPRv1 project's .mpr +// was rewritten permanently by a "check". +// +// Only what mx reads is copied: build output, caches and VCS metadata are skipped +// (copyProjectTree), so the cost is the model plus the widget, theme and source +// folders. cleanup removes the copy; it is never nil and safe to defer. +func copyProjectForCheck(mprPath string) (workMpr string, cleanup func(), err error) { + cleanup = func() {} + abs, err := filepath.Abs(mprPath) + if err != nil { + return "", cleanup, err + } + if info, err := os.Stat(abs); err != nil { + return "", cleanup, err + } else if info.IsDir() { + return "", cleanup, fmt.Errorf("%s is a directory, not a project file", abs) + } + tmp, err := os.MkdirTemp("", "mxcli-check-*") + if err != nil { + return "", cleanup, err + } + cleanup = func() { os.RemoveAll(tmp) } + + projectDir := filepath.Dir(abs) + dst := filepath.Join(tmp, filepath.Base(projectDir)) + if err := copyProjectTree(projectDir, dst); err != nil { + cleanup() + return "", func() {}, err + } + return filepath.Join(dst, filepath.Base(abs)), cleanup, nil +} + +// checkCopySkipRoot are top-level project folders mx check neither needs nor +// should see: build output and caches it regenerates. +var checkCopySkipRoot = map[string]bool{ + "deployment": true, // MxBuild / mx check output + "releases": true, // exported .mda packages + "theme-cache": true, // compiled theme, regenerated by mx check + ".mendix-cache": true, + ".mxcli": true, // catalog cache, widget docs + ".docker": true, // docker init output +} + +// checkCopySkipAnywhere are folder names skipped at any depth. +var checkCopySkipAnywhere = map[string]bool{ + ".git": true, + ".svn": true, + ".hg": true, + "node_modules": true, +} + +// copyProjectTree copies src to dst, skipping the folders above. A symlinked +// file is copied as a file, so a tool writing to the copy cannot write through +// it into the original; a symlinked folder is recreated as a link. +func copyProjectTree(src, dst string) error { + return filepath.WalkDir(src, func(p string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + rel, err := filepath.Rel(src, p) + if err != nil { + return err + } + target := filepath.Join(dst, rel) + if d.IsDir() { + if p == dst || p == filepath.Dir(dst) { + // $TMPDIR inside the project: do not copy the copy into itself. + return filepath.SkipDir + } + if rel != "." && (checkCopySkipAnywhere[d.Name()] || (!strings.ContainsRune(rel, filepath.Separator) && checkCopySkipRoot[rel])) { + return filepath.SkipDir + } + return os.MkdirAll(target, 0o755) + } + if d.Type()&fs.ModeSymlink != 0 { + info, err := os.Stat(p) + if err != nil { + return nil // dangling link: nothing mx could read either + } + if info.IsDir() { + link, err := os.Readlink(p) + if err != nil { + return err + } + return os.Symlink(link, target) + } + } else if !d.Type().IsRegular() { + return nil // sockets, pipes, devices + } + return copyFile(p, target) + }) +} + +// pathRewriter forwards output line by line with the temporary copy's path +// replaced by the project's own, so what mx reports names the files the user +// has, not a directory that is deleted when the check ends. +type pathRewriter struct { + mu sync.Mutex + w io.Writer + from []string + to string + buf bytes.Buffer +} + +func newPathRewriter(w io.Writer, copyDir, projectDir string) *pathRewriter { + from := []string{copyDir} + if resolved, err := filepath.EvalSymlinks(copyDir); err == nil && resolved != copyDir { + from = append(from, resolved) + } + return &pathRewriter{w: w, from: from, to: projectDir} +} + +func (r *pathRewriter) Write(p []byte) (int, error) { + r.mu.Lock() + defer r.mu.Unlock() + r.buf.Write(p) + for { + i := bytes.IndexByte(r.buf.Bytes(), '\n') + if i < 0 { + break + } + line := string(r.buf.Next(i + 1)) + if _, err := io.WriteString(r.w, r.rewrite(line)); err != nil { + return len(p), err + } + } + return len(p), nil +} + +// Flush writes a trailing partial line. +func (r *pathRewriter) Flush() { + r.mu.Lock() + defer r.mu.Unlock() + if r.buf.Len() > 0 { + io.WriteString(r.w, r.rewrite(r.buf.String())) + r.buf.Reset() + } +} + +func (r *pathRewriter) rewrite(s string) string { + for _, f := range r.from { + s = strings.ReplaceAll(s, f, r.to) + } + return s +} diff --git a/cmd/mxcli/docker/check_integration_test.go b/cmd/mxcli/docker/check_integration_test.go index 981d1244c1..fcfe2bf82e 100644 --- a/cmd/mxcli/docker/check_integration_test.go +++ b/cmd/mxcli/docker/check_integration_test.go @@ -6,6 +6,8 @@ package docker import ( "bytes" + "fmt" + "io" "os" "os/exec" "path/filepath" @@ -79,3 +81,57 @@ func mprStorageVersion(t *testing.T, mprPath string) types.MPRVersion { defer reader.Disconnect() return reader.Version() } + +// TestCheck_LeavesProjectUntouched is the end-to-end guard for ako/mxcli#951: +// with real mx, a default `docker check` and a --no-update-widgets one leave +// every file of an MPRv1 and an MPRv2 project byte-identical with the same +// mtime. Before the fix, update-widgets rewrote a v1 .mpr permanently, and mx +// check wrote theme-cache/ and deployment/sass/ in both formats. +func TestCheck_LeavesProjectUntouched(t *testing.T) { + mxPath, err := ResolveMx("") + if err != nil { + t.Skipf("mx not resolvable: %v", err) + } + scaffold := func(t *testing.T) string { + // Not t.TempDir(): the subtest names make it long enough for + // create-project's template extraction to hit PathTooLongException. + dir, err := os.MkdirTemp("", "chk") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { os.RemoveAll(dir) }) + cmd := exec.Command(mxPath, "create-project") + cmd.Dir = dir + PrepareMxCommand(cmd) + if out, err := cmd.CombinedOutput(); err != nil { + t.Skipf("mx create-project failed: %v\n%s", err, out) + } + return filepath.Join(dir, "App.mpr") + } + + for _, format := range []string{"v2", "v1"} { + for _, skip := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/no-update-widgets=%v", format, skip), func(t *testing.T) { + mprPath := scaffold(t) + if format == "v1" { + // update-widgets converts a v2 project to v1 — the fixture we want. + if err := updateWidgetsCmd(mxPath, mprPath, io.Discard, io.Discard); err != nil { + t.Skipf("could not produce a v1 fixture: %v", err) + } + if v := mprStorageVersion(t, mprPath); v != types.MPRVersionV1 { + t.Skipf("fixture is %v, not v1", v) + } + } + dir := filepath.Dir(mprPath) + before := treeState(t, dir) + var stdout bytes.Buffer + if err := Check(CheckOptions{ProjectPath: mprPath, SkipUpdateWidgets: skip, Stdout: &stdout, Stderr: &stdout}); err != nil { + t.Fatalf("Check: %v\n%s", err, stdout.String()) + } + if d := diffStates(before, treeState(t, dir)); len(d) > 0 { + t.Errorf("docker check modified the project:\n %v", d) + } + }) + } + } +} diff --git a/cmd/mxcli/docker/check_readonly_test.go b/cmd/mxcli/docker/check_readonly_test.go new file mode 100644 index 0000000000..edf4a717df --- /dev/null +++ b/cmd/mxcli/docker/check_readonly_test.go @@ -0,0 +1,275 @@ +// SPDX-License-Identifier: Apache-2.0 + +// ako/mxcli#951 item 1: `docker check` must never modify the user's project. +// +// `mx update-widgets` rewrites the model (and turns an MPRv2 project into +// MPRv1), and `mx check` itself compiles the theme into theme-cache/ and writes +// deployment/sass/. Measured on an 11.13 project: an MPRv1 project's .mpr was +// rewritten permanently by a plain `docker check`, and theme-cache/ and +// deployment/ were touched on v1 and v2 alike, with or without +// --no-update-widgets. Check now runs both tools on a temporary copy. +// +// The stubs below do to the directory they are given what the real tools do, +// so these tests fail the moment a tool is pointed at the project itself. +package docker + +import ( + "bytes" + "crypto/sha256" + "encoding/hex" + "fmt" + "io" + "io/fs" + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +// treeState records every file's content hash and mtime, and every directory, +// under root. +func treeState(t *testing.T, root string) map[string]string { + t.Helper() + state := map[string]string{} + err := filepath.WalkDir(root, func(p string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + rel, _ := filepath.Rel(root, p) + info, err := d.Info() + if err != nil { + return err + } + if d.IsDir() { + state[rel+"/"] = "dir" + return nil + } + b, err := os.ReadFile(p) + if err != nil { + return err + } + sum := sha256.Sum256(b) + state[rel] = hex.EncodeToString(sum[:]) + " " + info.ModTime().UTC().Format(time.RFC3339Nano) + return nil + }) + if err != nil { + t.Fatalf("walk %s: %v", root, err) + } + return state +} + +func diffStates(before, after map[string]string) []string { + var out []string + for k, v := range before { + if a, ok := after[k]; !ok { + out = append(out, "removed "+k) + } else if a != v { + out = append(out, "changed "+k) + } + } + for k := range after { + if _, ok := before[k]; !ok { + out = append(out, "added "+k) + } + } + return out +} + +// addProjectDirs gives a fixture the directories mx touches or reads. +func addProjectDirs(t *testing.T, dir string) { + t.Helper() + for _, f := range []string{ + "widgets/Some.Widget.mpk", + "theme-cache/web/theme.compiled.css", + "theme/web/main.scss", + "javasource/app/Action.java", + } { + p := filepath.Join(dir, f) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, []byte("original "+f), 0o644); err != nil { + t.Fatal(err) + } + } +} + +// stubTools replaces mx resolution and both mx invocations with stubs that +// mutate the project they are pointed at the way the real tools do, and record +// the paths they were given. +func stubTools(t *testing.T) (seen *[]string) { + t.Helper() + var paths []string + origResolve, origUW, origCheck := resolveMxForCheck, updateWidgetsCmd, mxCheckCmd + t.Cleanup(func() { resolveMxForCheck, updateWidgetsCmd, mxCheckCmd = origResolve, origUW, origCheck }) + + resolveMxForCheck = func(string, string) (string, error) { return "mx", nil } + updateWidgetsCmd = func(_, mprPath string, w, _ io.Writer) error { + paths = append(paths, "update-widgets "+mprPath) + dir := filepath.Dir(mprPath) + // Rewrites the model (inlining units on v2) and drops mprcontents/. + if err := os.WriteFile(mprPath, []byte("rewritten by update-widgets"), 0o644); err != nil { + return err + } + return os.RemoveAll(filepath.Join(dir, "mprcontents")) + } + mxCheckCmd = func(_, mprPath string, w, _ io.Writer) error { + paths = append(paths, "check "+mprPath) + dir := filepath.Dir(mprPath) + // Compiles the theme and writes the sass entry point. + if err := os.MkdirAll(filepath.Join(dir, "theme-cache/web"), 0o755); err != nil { + return err + } + if err := os.WriteFile(filepath.Join(dir, "theme-cache/web/theme.compiled.css"), []byte("recompiled"), 0o644); err != nil { + return err + } + if err := os.MkdirAll(filepath.Join(dir, "deployment/sass"), 0o755); err != nil { + return err + } + if err := os.WriteFile(filepath.Join(dir, "deployment/sass/main.scss"), []byte("x"), 0o644); err != nil { + return err + } + // mx reports the project it loaded by path in some messages. + fmt.Fprintf(w, "Loading %s\nThe app contains: 0 errors.\n", mprPath) + return nil + } + return &paths +} + +func TestCheck_DoesNotModifyProject(t *testing.T) { + for _, tc := range []struct { + name string + fixture func(*testing.T) string + skipUpdate bool + wantUWOnACopy bool + }{ + {"v1", v1Fixture, false, true}, + {"v2", v2Fixture, false, true}, + {"v1 --no-update-widgets", v1Fixture, true, false}, + {"v2 --no-update-widgets", v2Fixture, true, false}, + } { + t.Run(tc.name, func(t *testing.T) { + mprPath := tc.fixture(t) + projectDir := filepath.Dir(mprPath) + addProjectDirs(t, projectDir) + seen := stubTools(t) + + tmpRoot := t.TempDir() + t.Setenv("TMPDIR", tmpRoot) + + before := treeState(t, projectDir) + var out bytes.Buffer + if err := Check(CheckOptions{ + ProjectPath: mprPath, + SkipUpdateWidgets: tc.skipUpdate, + Stdout: &out, + Stderr: io.Discard, + }); err != nil { + t.Fatalf("Check: %v\n%s", err, out.String()) + } + after := treeState(t, projectDir) + if d := diffStates(before, after); len(d) > 0 { + t.Errorf("docker check modified the project:\n %s", strings.Join(d, "\n ")) + } + + // Every tool ran, and none of them on the project itself. + if len(*seen) == 0 { + t.Fatal("no mx tool was invoked") + } + for _, s := range *seen { + if strings.HasPrefix(strings.Fields(s)[1], projectDir+string(filepath.Separator)) { + t.Errorf("mx was pointed at the project itself: %s", s) + } + } + ranUW := strings.HasPrefix((*seen)[0], "update-widgets ") + if ranUW != tc.wantUWOnACopy { + t.Errorf("update-widgets ran = %v, want %v (%v)", ranUW, tc.wantUWOnACopy, *seen) + } + + // The output names the project, not the copy, and says what happened. + got := out.String() + if strings.Contains(got, tmpRoot) { + t.Errorf("output leaks the temporary copy's path:\n%s", got) + } + if !strings.Contains(got, "Loading "+mprPath) { + t.Errorf("mx output not reported against the original path:\n%s", got) + } + if tc.wantUWOnACopy && !strings.Contains(got, "normalised on a temporary copy") { + t.Errorf("output does not say widgets were normalised on a copy:\n%s", got) + } + + // The copy is cleaned up. + if entries, _ := os.ReadDir(tmpRoot); len(entries) != 0 { + t.Errorf("temporary copy left behind in %s: %v", tmpRoot, entries) + } + }) + } +} + +// TestCopyProjectForCheck_SkipsOutputs pins that build output, VCS metadata and +// caches are not copied: a large project's deployment/ or .git can dwarf the +// model, and mx check needs neither. +func TestCopyProjectForCheck_SkipsOutputs(t *testing.T) { + src := t.TempDir() + for _, f := range []string{ + "App.mpr", "mprcontents/ab/cd/x.mxunit", "widgets/W.mpk", + "themesource/m/web/main.scss", "theme/web/main.scss", "javasource/a/B.java", + "deployment/run/x.jar", ".git/HEAD", "releases/App.mda", "theme-cache/web/t.css", + ".mxcli/catalog.db", "theme/node_modules/pkg/index.js", ".mendix-cache/x", + } { + p := filepath.Join(src, f) + os.MkdirAll(filepath.Dir(p), 0o755) + os.WriteFile(p, []byte(f), 0o644) + } + dst := t.TempDir() + if err := copyProjectTree(src, dst); err != nil { + t.Fatal(err) + } + for _, f := range []string{"App.mpr", "mprcontents/ab/cd/x.mxunit", "widgets/W.mpk", "themesource/m/web/main.scss", "theme/web/main.scss", "javasource/a/B.java"} { + if _, err := os.Stat(filepath.Join(dst, f)); err != nil { + t.Errorf("%s not copied: %v", f, err) + } + } + for _, f := range []string{"deployment", ".git", "releases", "theme-cache", ".mxcli", "theme/node_modules", ".mendix-cache"} { + if _, err := os.Stat(filepath.Join(dst, f)); err == nil { + t.Errorf("%s was copied; it is output, VCS metadata or a cache", f) + } + } +} + +// TestCopyProjectForCheck_TempDirInsideProject: with $TMPDIR inside the project +// the walk meets its own copy; it must not copy the copy into itself. +func TestCopyProjectForCheck_TempDirInsideProject(t *testing.T) { + proj := t.TempDir() + mpr := filepath.Join(proj, "App.mpr") + if err := os.WriteFile(mpr, []byte("model"), 0o644); err != nil { + t.Fatal(err) + } + tmp := filepath.Join(proj, "tmp") + if err := os.MkdirAll(tmp, 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("TMPDIR", tmp) + work, cleanup, err := copyProjectForCheck(mpr) + if err != nil { + t.Fatalf("copyProjectForCheck: %v", err) + } + defer cleanup() + if b, err := os.ReadFile(work); err != nil || string(b) != "model" { + t.Fatalf("copy of the model = %q, %v", b, err) + } + filepath.WalkDir(filepath.Dir(work), func(p string, d fs.DirEntry, err error) error { + if err == nil && strings.HasPrefix(d.Name(), "mxcli-check-") { + t.Errorf("the copy contains a copy of itself: %s", p) + return filepath.SkipDir + } + return nil + }) +} + +func TestCopyProjectForCheck_MissingProject(t *testing.T) { + if _, _, err := copyProjectForCheck(filepath.Join(t.TempDir(), "nope.mpr")); err == nil { + t.Error("want an error for a project file that does not exist") + } +} diff --git a/cmd/mxcli/docker/check_test.go b/cmd/mxcli/docker/check_test.go index 9ce1d2a4aa..b1ee6d7163 100644 --- a/cmd/mxcli/docker/check_test.go +++ b/cmd/mxcli/docker/check_test.go @@ -157,7 +157,7 @@ func TestCheck_UpdateWidgetsBeforeCheck(t *testing.T) { var stdout, stderr bytes.Buffer opts := CheckOptions{ - ProjectPath: "/tmp/fake.mpr", + ProjectPath: fakeProject(t), MxBuildPath: mxDir, Stdout: &stdout, Stderr: &stderr, @@ -195,7 +195,7 @@ func TestCheck_SkipUpdateWidgetsFlag(t *testing.T) { var stdout, stderr bytes.Buffer opts := CheckOptions{ - ProjectPath: "/tmp/fake.mpr", + ProjectPath: fakeProject(t), MxBuildPath: mxDir, SkipUpdateWidgets: true, Stdout: &stdout, @@ -251,6 +251,7 @@ func TestCheck_UpdateWidgetsReceivesAbsolutePath(t *testing.T) { var stdout, stderr bytes.Buffer // Bare filename — the crash trigger. + t.Chdir(filepath.Dir(fakeProject(t))) Check(CheckOptions{ProjectPath: "fake.mpr", MxBuildPath: dir, Stdout: &stdout, Stderr: &stderr}) logBytes, err := os.ReadFile(logFile) @@ -304,3 +305,15 @@ func TestResolveMxForVersion_PrefersExactCachedVersion(t *testing.T) { t.Errorf("expected exact cached mx %s, got %s", expected, result) } } + +// fakeProject creates an (empty) project file in its own directory. Check copies +// the project's directory before running mx (ako/mxcli#951), so the file has to +// exist, and a directory such as /tmp would be copied whole. +func fakeProject(t *testing.T) string { + t.Helper() + p := filepath.Join(t.TempDir(), "fake.mpr") + if err := os.WriteFile(p, nil, 0o644); err != nil { + t.Fatal(err) + } + return p +} diff --git a/cmd/mxcli/docker/update_widgets.go b/cmd/mxcli/docker/update_widgets.go index 189e3f9275..91c6bfc555 100644 --- a/cmd/mxcli/docker/update_widgets.go +++ b/cmd/mxcli/docker/update_widgets.go @@ -62,7 +62,11 @@ var updateWidgetsCmd = func(mxPath, pathArg string, w, stderr io.Writer) error { // // This lives on the operation, not on a call site, because it was previously // implemented in `Check` only — `Build` carried its own bare invocation and kept -// converting projects (mendixlabs/mxcli#763, then #808). +// converting projects (mendixlabs/mxcli#763, then #808). `Check` no longer uses +// it: it runs update-widgets on a temporary copy (copyProjectForCheck), because a +// check must not modify the project at all — this restores only the v2 storage, +// and an MPRv1 project was rewritten permanently (ako/mxcli#951). `Build` is +// expected to write the project's deployment/, and still uses it. func runUpdateWidgets(mxPath, projectPath string, w, stderr io.Writer) (restore func()) { restore = func() {} if projectPath == "" { diff --git a/docs-site/src/guides/marketplace.md b/docs-site/src/guides/marketplace.md index ef1f957d2b..97242be174 100644 --- a/docs-site/src/guides/marketplace.md +++ b/docs-site/src/guides/marketplace.md @@ -313,7 +313,7 @@ Re-running is free: a second run reports 0 units changed, because [idempotent wr |---|---|---| | `mxcli fix widgets` | **yes** | the fix — after any headless install | | `mxcli fix design-properties` | **yes** | the fix — after any headless install | -| `mxcli docker check` | no | runs the widget resync under a snapshot so the *check* is not tripped by CE0463; the stored model stays stale | +| `mxcli docker check` | no | runs the widget resync on a temporary copy so the *check* is not tripped by CE0463; the stored model stays stale (`--no-update-widgets` checks it as stored) | | `mxcli widget sync` | yes, partial | reconciles widget schemas in mxcli's own code; clears 7 of 40 on the reference fixture | CE6087 is distinct from `CE6083`, which is a *missing* design-property declaration and is fixed by installing everything the package ships — something `install` and `update` already do. diff --git a/mdl/catalog/catalog.go b/mdl/catalog/catalog.go index 516de09513..b3bf74c30c 100644 --- a/mdl/catalog/catalog.go +++ b/mdl/catalog/catalog.go @@ -6,6 +6,8 @@ package catalog import ( "database/sql" "fmt" + "os" + "path/filepath" "strconv" "strings" "time" @@ -348,6 +350,17 @@ func NewFromFile(path string) (*Catalog, error) { return nil, fmt.Errorf("failed to migrate cached catalog: %w", err) } + // A cache already at the current schema version was saved from a catalog + // that ran createTables, so it is complete — open it without writing. Opening + // must stay read-only: a parallel process may rename a fresh cache over this + // path at any moment (SaveToFile is atomic, ako/mxcli#951), and SQLite refuses + // a write to a file that has been moved ("attempt to write a readonly + // database"), while concurrent openers writing the version row contend for + // the lock. + if stored, err := c.GetMeta(MetaSchemaVersion); err == nil && stored == CatalogSchemaVersion { + return c, nil + } + // Idempotent schema upgrade — adds any tables/indexes the cached file // doesn't have yet. Existing data is untouched (unless dropped above). // createTables also records the current schema version in catalog_meta. @@ -443,6 +456,15 @@ func (c *Catalog) migrateIfSchemaMismatch() error { // SaveToFile saves the catalog to a SQLite file. // This copies the in-memory database to a file for persistence. // Requires the underlying CatalogDB to be a *SqliteCatalogDB. +// +// The save is atomic: the database is written to a temporary file in the same +// directory and renamed over path. Parallel mxcli processes on one project all +// save the same cache (ako/mxcli#951: eight parallel `lint` runs), and writing in +// place let them collide — VACUUM INTO refused the file another process had just +// created, the manual fallback then failed with "table catalog_meta already +// exists" or "database is locked", and a reader could open a half-written file. +// With a rename every writer succeeds (the last one wins) and a reader sees the +// old cache or a complete new one, never a partial file. func (c *Catalog) SaveToFile(path string) error { sdb, ok := c.db.(*SqliteCatalogDB) if !ok { @@ -450,15 +472,38 @@ func (c *Catalog) SaveToFile(path string) error { } rawDB := sdb.RawDB() + // Same directory, so the rename cannot cross a filesystem boundary. + tmpFile, err := os.CreateTemp(filepath.Dir(path), filepath.Base(path)+".tmp-*") + if err != nil { + return err + } + tmp := tmpFile.Name() + tmpFile.Close() + // VACUUM INTO requires the target to be absent or empty; it is empty. + committed := false + defer func() { + if !committed { + os.Remove(tmp) + } + }() + // Use SQLite backup API via VACUUM INTO (SQLite 3.27+) // Fall back to manual copy if not available - safePath := strings.ReplaceAll(path, "'", "''") - _, err := rawDB.Exec(fmt.Sprintf("VACUUM INTO '%s'", safePath)) - if err != nil { - // Fall back: export and import - return c.saveToFileManual(path, rawDB) + safePath := strings.ReplaceAll(tmp, "'", "''") + if _, err := rawDB.Exec(fmt.Sprintf("VACUUM INTO '%s'", safePath)); err != nil { + // Fall back: export and import, into a fresh empty temp file. + if err := os.Truncate(tmp, 0); err != nil { + return err + } + if err := c.saveToFileManual(tmp, rawDB); err != nil { + return err + } } + if err := os.Rename(tmp, path); err != nil { + return fmt.Errorf("replace catalog cache %s: %w", path, err) + } + committed = true return nil } diff --git a/mdl/catalog/catalog_save_race_test.go b/mdl/catalog/catalog_save_race_test.go new file mode 100644 index 0000000000..32262e43de --- /dev/null +++ b/mdl/catalog/catalog_save_race_test.go @@ -0,0 +1,139 @@ +// SPDX-License-Identifier: Apache-2.0 + +package catalog + +import ( + "fmt" + "os" + "path/filepath" + "sync" + "testing" + "time" +) + +// TestSaveToFile_ConcurrentWritersAndReaders is the guard for ako/mxcli#951 +// item 2. Eight parallel `mxcli lint` runs on a fresh project copy all built the +// catalog and saved it to the same .mxcli/catalog.db. The save removed the file +// and wrote into the path in place: VACUUM INTO refused a file another process +// had just created, the manual-copy fallback then hit "table catalog_meta already +// exists" or "database is locked", and a reader could open a half-written file. +// +// The save must be atomic: every writer succeeds, and a reader sees either the +// old cache or a complete new one — never a partial file. +func TestSaveToFile_ConcurrentWritersAndReaders(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "catalog.db") + + newCat := func(t *testing.T, mode string) *Catalog { + t.Helper() + cat, err := New() + if err != nil { + t.Fatalf("New: %v", err) + } + if err := cat.SetCacheInfo("/p/app.mpr", time.Unix(1700000000, 0), "11.8.0", mode, time.Second); err != nil { + t.Fatalf("SetCacheInfo: %v", err) + } + return cat + } + + // An existing cache is the common case (the project changed, so every + // process rebuilds and overwrites it). + seed := newCat(t, "fast") + if err := seed.SaveToFile(path); err != nil { + t.Fatalf("seed SaveToFile: %v", err) + } + seed.Close() + + const writers = 8 + cats := make([]*Catalog, writers) + for i := range cats { + cats[i] = newCat(t, "full") + } + defer func() { + for _, c := range cats { + c.Close() + } + }() + + stop := make(chan struct{}) + var readerErrs []error + var mu sync.Mutex + var readers sync.WaitGroup + for r := 0; r < 4; r++ { + readers.Add(1) + go func() { + defer readers.Done() + for { + select { + case <-stop: + return + default: + } + c, err := NewFromFile(path) + if err != nil { + mu.Lock() + readerErrs = append(readerErrs, err) + mu.Unlock() + continue + } + info, err := c.GetCacheInfo() + c.Close() + if err == nil && info.BuildMode != "fast" && info.BuildMode != "full" { + err = fmt.Errorf("reader saw build mode %q (partial cache)", info.BuildMode) + } + if err != nil { + mu.Lock() + readerErrs = append(readerErrs, err) + mu.Unlock() + } + } + }() + } + + start := make(chan struct{}) + errs := make(chan error, writers) + var wg sync.WaitGroup + for i := 0; i < writers; i++ { + wg.Add(1) + go func(c *Catalog) { + defer wg.Done() + <-start + errs <- c.SaveToFile(path) + }(cats[i]) + } + close(start) + wg.Wait() + close(stop) + readers.Wait() + close(errs) + + for err := range errs { + if err != nil { + t.Errorf("concurrent SaveToFile: %v", err) + } + } + for _, err := range readerErrs { + t.Errorf("concurrent reader: %v", err) + } + + final, err := NewFromFile(path) + if err != nil { + t.Fatalf("final cache does not open: %v", err) + } + defer final.Close() + info, err := final.GetCacheInfo() + if err != nil { + t.Fatalf("final cache info: %v", err) + } + if info.BuildMode != "full" { + t.Errorf("final cache build mode = %q, want full", info.BuildMode) + } + + // No temporary files may be left behind next to the cache. + entries, _ := os.ReadDir(dir) + for _, e := range entries { + if n := e.Name(); n != "catalog.db" && n != "catalog.db-journal" && n != "catalog.db-wal" && n != "catalog.db-shm" { + t.Errorf("leftover file next to the cache: %s", n) + } + } +} diff --git a/mdl/catalog/catalogdb_sqlite.go b/mdl/catalog/catalogdb_sqlite.go index 0da6e43bf2..cb7ec87d6e 100644 --- a/mdl/catalog/catalogdb_sqlite.go +++ b/mdl/catalog/catalogdb_sqlite.go @@ -6,6 +6,8 @@ package catalog import ( "database/sql" + "fmt" + "strings" _ "modernc.org/sqlite" ) @@ -28,9 +30,22 @@ func NewSqliteCatalogDB() (*SqliteCatalogDB, error) { return &SqliteCatalogDB{db: db}, nil } +// fileBusyTimeoutMs is how long a connection to an on-disk catalog waits for a +// lock held by another process before failing with SQLITE_BUSY. Opening a cache +// writes to it (schema upgrade, schema version), so parallel mxcli processes on +// one project contend for the write lock for a moment (ako/mxcli#951). +const fileBusyTimeoutMs = 10000 + // NewSqliteCatalogDBFromFile opens a file-based SQLite database. func NewSqliteCatalogDBFromFile(path string) (*SqliteCatalogDB, error) { - db, err := sql.Open("sqlite", path) + dsn := path + // The busy timeout goes in the DSN so it applies to every pooled connection, + // not only the one a PRAGMA statement happens to run on. A '?' in the path + // would be read as the start of the query string; keep the bare path then. + if !strings.Contains(path, "?") { + dsn = fmt.Sprintf("%s?_pragma=busy_timeout(%d)", path, fileBusyTimeoutMs) + } + db, err := sql.Open("sqlite", dsn) if err != nil { return nil, err } diff --git a/mdl/executor/cmd_catalog.go b/mdl/executor/cmd_catalog.go index 0415297b3f..9141297481 100644 --- a/mdl/executor/cmd_catalog.go +++ b/mdl/executor/cmd_catalog.go @@ -578,8 +578,9 @@ func buildCatalog(ctx *ExecContext, full, isSource, communities bool, resolution } cacheDir := filepath.Dir(cachePath) if err := os.MkdirAll(cacheDir, 0755); err == nil { - // Remove existing cache file first - os.Remove(cachePath) + // SaveToFile replaces the cache atomically. Do not remove it first: + // that opened a window in which a parallel process found no cache, + // and the in-place write that followed raced it (ako/mxcli#951). if err := cat.SaveToFile(cachePath); err != nil { fmt.Fprintf(ctx.progress(), "Warning: failed to save catalog cache: %v\n", err) } else { @@ -655,7 +656,6 @@ func execRefreshCatalogStmt(ctx *ExecContext, stmt *ast.RefreshCatalogStmt) erro return mdlerrors.NewBackend("graph analysis", err) } if !loadedFromCache && cachePath != "" { - os.Remove(cachePath) if err := ctx.Catalog.SaveToFile(cachePath); err != nil { fmt.Fprintf(ctx.Output, "Warning: failed to save catalog cache: %v\n", err) }