🐛 Fixed race condition causing 0-byte cards.min.js and cards.min.css

[betschki]

When concurrent requests hit serveMiddleware() before assets are ready, each independently calls load() which triggers fs.writeFile. Because writeFile uses O_TRUNC, concurrent writes truncate the file to 0 bytes before writing − any concurrent readFile between truncate and write sees an empty file. The createPublicFileMiddleware in serve-public-file.js then caches this 0-byte response in memory with a 1-year Cache-Control header, permanently serving empty cards.min.js / cards.min.css until restart.

Impact: Card-related JavaScript and CSS (audio players, galleries, bookmarks, etc.) silently break − the browser receives a 200 OK with an empty body. Audio player timestamps show raw seconds instead of formatted time, gallery layouts break, bookmark cards don't render, etc.

Node.js fs.writeFile docs explicitly warn:

It is unsafe to use fs.writeFile() multiple times on the same file without waiting for the callback.

The proposed fix uses a singleton promise pattern to deduplicate concurrent load() calls, ensuring the asset file is only written once regardless of how many requests arrive before it's ready.

Why is this needed?

On Magic Pages this issue was observed multiple times in containerized deployments with zero-downtime rolling updates (Docker Swarm start-first, Kubernetes rolling updates), where traffic is routed to a fresh container before assets have been built. Health checks or multiple users hitting the site simultaneously can trigger concurrent first-requests. This isn't an issue on smaller sites, but has been observed with bigger sites.

---

Got some code for us? Awesome 🎊!

Please take a minute to explain the change you're making:

• Why are you making it?
• What does it do?
• Why is this something Ghost users or developers need?

Please check your PR against these items:

• I've read and followed the Contributor Guide
• I've explained my change
• I've written an automated test to prove my change works

We appreciate your contribution! 🙏

[coderabbitai]

Walkthrough

This pull request modifies the AssetsMinificationBase class to implement single-flight concurrency control for asset loading operations. A new loading field is introduced and the serveMiddleware flow is updated to ensure only one load operation executes concurrently, with subsequent requests awaiting the same in-flight promise. The invalidate method is modified to reset the loading state. Supporting unit tests validate concurrent request handling, single load invocation, asset write behavior, and proper state clearing after load completion.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the main bug fix: a race condition causing 0-byte minified CSS/JS files. It directly relates to the primary change in the changeset.
Description check	✅ Passed	The description is comprehensive and directly related to the changeset, explaining the root cause, impact, and the proposed fix with context on why it's needed.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

ghost/core/test/unit/frontend/services/assets-minification/assets-minification-base.test.js (1)

196-216: Consider asserting next is not called when load() throws.

The test confirms assets.loading is cleared on error, but doesn't assert that next is never invoked when the middleware rejects. Adding assert.equal(next.callCount, 0) after assert.rejects(...) would make the contract fully explicit and guard against a future refactor accidentally calling next() on error.

✅ Proposed addition

     await assert.rejects(() => middleware({}, {}, next));

+    assert.equal(next.callCount, 0, 'next() must not be called when load() throws');
     assert.equal(assets.loading, null);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@ghost/core/test/unit/frontend/services/assets-minification/assets-minification-base.test.js`
around lines 196 - 216, The test should also assert the middleware does not call
the next handler when load() throws; after the existing await assert.rejects(()
=> middleware({}, {}, next)); add an assertion that next.callCount (or
next.called) is 0 to ensure the serveMiddleware() path for the TestAssets class
does not invoke next on error.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In
`@ghost/core/test/unit/frontend/services/assets-minification/assets-minification-base.test.js`:
- Around line 196-216: The test should also assert the middleware does not call
the next handler when load() throws; after the existing await assert.rejects(()
=> middleware({}, {}, next)); add an assertion that next.callCount (or
next.called) is 0 to ensure the serveMiddleware() path for the TestAssets class
does not invoke next on error.