fix(generated-output): publish artifacts transactionally - #796
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66202143ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for _, file := range files { | ||
| wrote, err := writeFileIfChangedStatus(file.path, file.contents) | ||
| if err != nil { | ||
| return Result{}, reporter.fail("write", err) |
There was a problem hiding this comment.
When any write after the first fails during publication (for example ENOSPC, a permissions change, or an unwritable report/security-manifest path), this loop has already renamed earlier files into place and then returns an error. That still leaves mixed old/new output for write-time failures despite the new planned-publish contract; stage all temporary files first and only commit renames after every file is ready, or otherwise roll back/limit the guarantee to planning failures.
Useful? React with 👍 / 👎.
| - Disk static output and generated app source are planned before publication. | ||
| Final file replacements use same-directory temporary files followed by atomic | ||
| rename, so validation, formatting, manifest/report generation, and security | ||
| manifest generation fail before touching committed files. |
There was a problem hiding this comment.
This new rule says disk static output is planned before publication, but gowdk dev still uses BuildIncrementalFromValidatedProgram (internal/gowdkcmd/dev_loop.go:131), whose incremental path writes CSS/runtime/page files as it goes before later cleanup and manifest/report steps. In incremental rebuilds, a late error can still partially update the served output, so the documented contract is broader than the implementation; either apply the planned-publish path to incremental builds or explicitly scope this guarantee to full static builds.
Useful? React with 👍 / 👎.
| finalizeCSSArtifact(&artifact) | ||
| result.CSSArtifacts = append(result.CSSArtifacts, artifact.CSSArtifact) | ||
| files = append(files, plannedPublishedFile{path: artifact.Path, contents: artifact.contents, countWrite: true}) | ||
| reporter.debug("write", "css_written", "CSS artifact written", BuildEvent{Path: eventPath(outputDir, artifact.Path)}) |
There was a problem hiding this comment.
When a later planning step fails after this point, such as an invalid SEO addon BaseURL, the build now returns before publishing any planned files, but the returned debug BuildError.Report already says css_written/page_written. That makes failed transactional builds report artifacts as written even though the transaction correctly aborted; defer these events until the publish loop succeeds or rename them to planned events.
Useful? React with 👍 / 👎.
| if err := os.Rename(tempName, filePath); err != nil { | ||
| return false, err |
There was a problem hiding this comment.
For Windows and other non-Unix builds, this helper does not satisfy the new atomic-publication contract: I checked go doc os.Rename, which says Rename is not an atomic operation on non-Unix platforms even within one directory. Since GOWDK is portable and the docs now promise atomic final replacements, use a platform-specific atomic replace helper or qualify the guarantee for non-Unix users.
Useful? React with 👍 / 👎.
| if err != nil { | ||
| return err | ||
| } | ||
| planned = append(planned, plannedFile{path: targetPath, contents: payload}) |
There was a problem hiding this comment.
For generated app builds with many or large embeddable assets, this now retains the contents of every copied output file in planned until all planning finishes, whereas the previous path only held one file at a time. Large static outputs can therefore spike memory or fail app generation before publication; keep the plan as paths/temp files or stream each file into a staged temp instead of accumulating all payloads in memory.
Useful? React with 👍 / 👎.
1 participant
What changed
Why
Disk builds and generated app emission could leave mixed old/new output when later manifest, report, security, formatting, or app-generation stages failed. This narrows the publication boundary so those failures happen before committed files are touched.
Refs #669.
Verification
go test ./internal/buildgen ./internal/appgen ./internal/compilergo build ./cmd/gowdkgo test ./internal/gowdkcmdwas also attempted before branching cleanly; it failed on an existing missing fixture,.github/workflows/release-smoke.yml.