WriteTransaction.Commit warn-and-continues on unit-file rename failures after the DB is committed — contradicting SaveUnit/FlushUnits' contracts

[MohamedGamalEzzatAli]

WriteTransaction.Commit warn-and-continues on unit-file rename failures after the DB is committed — contradicting SaveUnit/FlushUnits' contracts

Summary

Both copies of WriteTransaction.Commit() commit the SQLite transaction FIRST and then rename
the .mxunit temp files into place with warn-and-continue semantics: a failed rename
prints a fmt.Printf warning (stdout, not stderr) and Commit still returns nil. The
comment in the code says it itself:

// sdk/mpr/writer_core.go:180-199 @ v0.19.0 (same pattern at modelsdk/mpr/writer_core.go:~247)
// Commit database transaction first
if err := wt.tx.Commit(); err != nil { ... }

// Finalize file writes by renaming temp files to final paths
for _, pf := range wt.pendingFiles {
    if err := os.Rename(pf.tempPath, pf.finalPath); err != nil {
        // Log error but continue - DB is already committed
        // This could leave some files in inconsistent state
        fmt.Printf("Warning: failed to finalize file %s: %v\n", pf.finalPath, err)
    }
}
wt.committed = true
return nil

When any rename fails, the Unit table (including ContentsHash) now describes contents
that are NOT on disk — a silently inconsistent MPRv2 model — and every caller up the stack
reports success:

• modelsdk/codec/store.go SaveUnit ("writes a unit's BSON bytes back") returns nil;
• FlushUnits — documented "saves multiple units atomically in a single transaction" —
  returns nil having applied only some of the units on disk;
• modelsdk/model.go's dirty-element flush propagates that nil.

The failure condition is real on Windows (demonstrated)

A .mxunit held open with FILE_SHARE_READ|FILE_SHARE_WRITE but without
FILE_SHARE_DELETE — the default sharing mode of an ordinary open, i.e. what an editor,
indexer, JVM, or sync client typically holds — makes os.Rename onto it fail with
Access is denied. Verified live on v0.19.0 by holding a domain-model unit that way and
running ALTER ENTITY, GRANT, and ALTER PAGE ... SET against it.

Credit where due — the paths we could reach handle it CORRECTLY

All three commands above route through the eager per-unit persist / the page mutator's save,
which propagate the rename error properly: hard error naming the file, exit 1, DB and disk
still consistent, no orphan .tmp:

Error: failed to save modified page: failed to rename unit file: rename
mprcontents\XX\YY\<uuid>.mxunit.tmp mprcontents\XX\YY\<uuid>.mxunit: Access is denied.

We did not manage to drive the WriteTransaction.Commit warn-and-continue path from the CLI
within a reasonable effort, so this report is a code-audit finding about that remaining path,
not a demonstrated end-to-end corruption. But the path is live code with callers, its
semantics contradict both the sibling paths (which fail hard) and its own callers' documented
contracts, and the trigger condition is trivially reachable on Windows.

Suggested direction (any of these would close the gap)

1. Collect rename failures in Commit and return an error (non-nil) listing the units left
   stale — callers already handle Commit errors, and the process exit code then tells the
   truth. This matches the v0.19 theme ("a statement mxcli accepts is a statement mxcli
   honours") and the new exec check-gating philosophy.
2. Optionally pre-flight: attempt an exclusive open of each finalPath before committing the
   DB transaction, so the whole write refuses up front when a unit is held.
3. Either way: route the warning to stderr rather than stdout, so scripted callers that parse
   stdout do not swallow it.

Environment: mxcli v0.19.0 (b313ff0), Windows amd64, Mendix 11.12.1 MPRv2 project.