Added a freeze helper that makes config read-only once loaded

[acburdine]

Follows #30721, now merged — that PR moved the global asset hash into the asset hash service, removing the last two runtime config.set calls, which this one depends on. Rebased onto main, so the diff here is just the freeze itself.

Why are you making it?

Config is fully loaded by the time loadNconf() returns, and nothing in Ghost writes to it afterwards — but nconf doesn't know that, so it re-derives every answer from scratch.

Provider._execute walks all nine stores on every get(), and for an object-valued key it collects a hit from each store and deep-merges them (common.merge builds a fresh Memory store and recursively re-merges the subtree) — every single call. There are 353 config.get call sites in core/, some on per-request paths.

Measured against the real config:

	before	after
get('database')	3526 ns	15 ns
get('url')	1254 ns	37 ns
get('paths:contentPath')	334 ns	29 ns
getSiteUrl() (several gets)	1276 ns	63 ns

In absolute terms this is small — a few hundred gets per request is well under a millisecond — so the immutability guarantee is at least as much of the point as the speed.

What does it do?

freeze() makes the instance read-only and memoises get() by key.

The key property: every cached value is whatever nconf itself returned for that key. So a frozen lookup can't disagree with an unfrozen one — there's no second implementation of nconf's resolution order to keep correct — and with writes rejected, a cache entry can't go stale, so there's no invalidation logic at all.

Details that matter:

• Mutators throw, they don't no-op. nconf's own readOnly flag on each store looks like the obvious lever, but _execute skips read-only stores for a destructive action and returns undefined — so flipping it would turn a config write into a silent failure rather than a loud one.
• required() is not a mutator. Despite sitting alongside them on the provider, nconf implements it as a read — get() per key, throw on missing — so it stays usable while frozen. There's a comment on MUTATORS recording why it's deliberately absent.
• Cached objects are deep-frozen. Object reads are cached by reference, so a caller mutating what it got would otherwise rewrite the cache for every later reader. Freezing makes that a TypeError naming the site. Chosen over cloning deliberately: cloning on read undoes the entire performance win, and cloning once on write still hands the same object to everyone.
• A keyless get() is left uncached. nconf's env store holds the entire environment, so the whole-tree merge materialises it: on a boot here, 139 of 194 top-level keys came from env rather than config, AWS_SECRET_ACCESS_KEY included. Caching that would mean a long-lived object holding every env var. Nothing in core/ calls get() keyless anyway.

One caller had to change. configure() in data/db/connection.js assembles the knex config by mutating the object it's handed, and it's called as configure(config.get('database')). Under caching that made the two ways of reading a key disagree — get('database').pool → {} while get('database:pool') → undefined. It now clones first, which is worth having regardless: mutating the object config handed back was already writing through into nconf's stores for nested keys, since nconf's merge shares subtrees by reference.

Where it freezes: loadNconf freezes as its last step, rather than leaving it to boot. A write during boot then fails loudly instead of quietly working. Nothing in the tree does that today, so this holds the line rather than changing behaviour. Skipped under test, where the suites rewrite config between cases on purpose via configUtils.

No opt-out flag: nothing writes to config after load, so a switch would only exist to re-enable a behaviour we don't want back. The diff touches no config defaults.

Why is this something Ghost users or developers need?

No user-facing change. For developers, config's lifecycle becomes honest and enforced — loaded once, read-only thereafter — so "is this value still the one I read at startup?" stops being a question you have to answer by reading the whole codebase. Any future attempt to use config as mutable runtime state fails immediately, at the write, with a message naming the key.

Testing

• 16 unit tests in test/unit/shared/config/freeze.test.ts: cache hits, cached misses, keyless get() staying uncached, nested keys resolving through the store chain, unfreeze dropping the cache, all 9 mutators throwing, required() still validating while frozen, a rejected write leaving config unchanged, and two covering cached-object immutability.
• Both immutability tests were checked against the bug — they fail without deepFreeze, and one reproduces the exact get('database').pool vs get('database:pool') divergence.
• Booted Ghost for real (GHOST_CI_SHUTDOWN_AFTER_BOOT=1) with freeze-at-load and deep-frozen config: exit 0, zero frozen-write errors and zero TypeErrors. This matters because freeze is skipped under test, so boot is the only thing that exercises the frozen path end to end.
• Validated against the real config in both envs: development freezes (526 key paths resolve, all bound helpers agree, set() throws, the rejected write left config unchanged); NODE_ENV=testing does not freeze and writes still work.
• Three adapter-manager test fixtures build a ConfigInstance by hand and now call bindFreeze too, so they still satisfy the widened type.
• Full unit suite on this head: 8711 passing.
• CI green on f8de8e6, Typecheck included.

Known gap

Freeze is skipped under test, so CI doesn't exercise the frozen path — a mutation on a rarely-hit request path would only surface in production. That's how the configure() case above got through review in the first place. Worth deciding separately whether the freeze path deserves coverage; I'd keep that out of this PR.

---

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

🤖 Generated with Claude Code

https://claude.ai/code/session_011s9ZdgxjVXMr7YTh4UtmtH

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

---

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

[nx-cloud]

🤖 Nx Cloud AI Fix

Ensure the fix-ci command is configured to always run in your CI pipeline to get automatic fixes in future runs. For more information, please see https://nx.dev/ci/features/self-healing-ci

---

View your CI Pipeline Execution ↗ for commit 90d6acc

Command	Status	Duration	Result
nx run-many -t test:types -p ghost	❌ Failed	8s	View ↗
nx run ghost:test:ci:integration	✅ Succeeded	4m 3s	View ↗
nx run ghost:test:integration	✅ Succeeded	3m 49s	View ↗
nx run ghost:test:ci:e2e	✅ Succeeded	3m 43s	View ↗
nx run ghost:test:e2e	✅ Succeeded	2m 54s	View ↗
nx run ghost:test:legacy	✅ Succeeded	3m 9s	View ↗
nx run ghost-monorepo:lint:boundaries	✅ Succeeded	29s	View ↗
nx run-many -t test:unit -p ghost	✅ Succeeded	39s	View ↗
Additional runs (4)	✅ Succeeded	...	View ↗

💡 Dealing with memory or CPU issues? See memory and CPU details with the resource usage add-on ↗.

---

☁️ Nx Cloud last updated this comment at 2026-09-18 09:07:09 UTC

[codecov]

Codecov Report

❌ Patch coverage is 82.97872% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.76%. Comparing base (899ead6) to head (90d6acc).
⚠️ Report is 126 commits behind head on main.

Files with missing lines	Patch %	Lines
ghost/core/core/shared/config/freeze.ts	84.21%	5 Missing and 1 partial ⚠️
...ore/server/adapters/lib/redis/AdapterCacheRedis.js	0.00%	1 Missing ⚠️
ghost/core/loggingrc.js	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #30722      +/-   ##
==========================================
+ Coverage   67.65%   67.76%   +0.10%     
==========================================
  Files        1677     1683       +6     
  Lines       60555    60743     +188     
  Branches    10475    10481       +6     
==========================================
+ Hits        40968    41160     +192     
+ Misses      17268    17264       -4     
  Partials     2319     2319              

Flag	Coverage Δ
e2e-tests	70.51% <82.97%> (+0.12%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[acburdine]

Benchmarked on the Pro image — and a boot-breaking bug, now fixed on this branch

Two things for this PR. The bug first, because it blocks merge.

AdapterCacheRedis crashed on boot under the freeze

Pushed as c5768c8 on this branch.

AdapterCacheRedis folds ttl into clusterConfig.options by assigning into the object it was handed — and that object is config's own subtree. resolveAdapterOptions returns it directly on one branch and only shallow-copies on the other, so clusterConfig is config's object either way. Frozen, the write throws and Ghost dies during init:

TypeError: Cannot assign to read only property 'ttl' of object '#<Object>'
    at new AdapterCacheRedis (AdapterCacheRedis.js:44)
    at AdapterManager.getAdapter (adapter-manager.ts)
    at core/server/lib/image/index.js:11

This is exactly the failure mode this PR predicts ("Freezing makes that a TypeError naming the site") and exactly the gap it flags. Nothing would have caught it before production:

	Redis adapter configured?	Freeze active?
Ghost CI	n/a	no (NODE_ENV=testing)	passes
Development boot	no adapters block at all	yes	passes
Pro image	adapters.cache.Redis.clusterConfig	yes	throws

The ttl now goes into a derived clusterConfig instead of being assigned into config. Worth having regardless of the freeze: the old write went through into the subtree every later reader of adapters:… sees, and mutated the very object the adapter manager stringifies for its instance key.

Three tests added; they freeze their own input, because freeze is skipped under test and without that they pass against the broken constructor. Two fail against the old code. Also verified against the real path — getAdapter('cache:imageSizes') under a frozen development config with a Redis cluster block reproduces the exact production error without the fix and constructs cleanly with it.

I grepped for sibling sites. The two public-config hits write to their own fresh objects, not config's; no other adapter mutates its config argument. Treat that as "none found" rather than "none exist", since only Pro config exercises most adapters.

The freeze is the single biggest CPU win in the current perf stack

Measured on the Moya Pro benchmark (ghost-perf, 5,000-request weighted frontend mix, fixed-work CPU time), against an integration branch of this PR plus #30704, #30753, #30874, #30886 and #30887 versus main at the merge base. Two pairs.

nconf self time and allocation, reproduced independently in both baselines:

	baseline run 1	baseline run 2	with the freeze
nconf CPU self time	4,327 ms (3.1% of busy)	4,659 ms (3.5%)	absent from the top 25 (<578 ms)
nconf allocated bytes	569.8 MB (2.1%)	571.1 MB (2.1%)	absent from the top 25 (<73 MB)

The combined branch came in at −11.5% CPU for the same fixed workload (pairs: −13.3%, −9.6%), with the two arms not overlapping — the worst integration run still beat the best baseline by 9.6%. nconf is the largest single contributor to that.

Worth noting against this PR's own framing: it describes the absolute effect as "small — a few hundred gets per request is well under a millisecond". Aggregated over a real workload it was 3.1–3.5% of total busy CPU and ~570 MB of allocation per 5,000 requests. The immutability argument stands on its own, but the performance case is stronger than the PR claims.

No boot-memory cost. I flagged boot.rss as a possible small regression after one pair (+2.73 MiB, just outside the noise floor); the repeat put it at +0.08 MiB. It was noise. boot.wall_mean_ms and boot.heap_used_bytes also showed no reproducible change — both flipped sign between pairs.

For context on reading any of these numbers: load.cpu_s on this benchmark carries a 4.2% noise floor (pooled within-version sd across 20 scheduled runs), about half of it runner speed drift that runner.calibration_ms in each result.json can correct for.

---

Generated by Claude Code

[acburdine]

Full audit of config mutation under the freeze — 3 production bugs, and a premise that needs revisiting

Following the AdapterCacheRedis crash, I traced this properly rather than by grep.

Method: instrumented config.get to return a deep clone wrapped in a recording Proxy, so every attempted write to a returned object is logged with a stack trace without throwing and without touching nconf's stores. Then ran the unit, e2e and integration suites. This matters because grepping and booting both miss the silent cases — see the last section.

Coverage: 823 recorded mutation attempts across 40 distinct sites.

Production bugs found (all three now fixed on this branch)

#	Site	Under the freeze	Commit
1	AdapterCacheRedis — folds ttl into clusterConfig.options	Throws, Ghost dies during init	c5768c8
2	loggingrc.js — builds logging config by mutation	Silent. Logger starts with {}	1e5a1a9
3	MigratorConfig.js → knex-migrator	Silent. Migrator connection loses its settings	90d6acc

#2 — @tryghost/logging does try { require(loggingrc) } catch { loggingConfig = {} }, so nothing surfaces. Measured on a development boot:

	with freeze	without
path	ghost/core/	ghost/core/content/logs/
domain	localhost	configured site url
metadata	{}	{"version": …}

Log files to the wrong directory, version and domain metadata gone, no error anywhere.

#3 — the highest-volume site in the whole audit. MigratorConfig.js hands knex-migrator config.get('database') directly, and knex-migrator/lib/database.js mutates it for a mysql2 client:

options.connection.timezone = options.connection.timezone || 'Z';
options.connection.charset  = options.connection.charset  || 'utf8mb4';
options.connection.decimalNumbers = true;
delete options.connection.filename;

Verified against the real frozen config: the sequence returns without error and leaves timezone undefined instead of 'Z'. The migrator then runs without the UTC timezone it means to force — for migrations writing timestamps that's a correctness problem, not a cosmetic one. Reached both from boot (DatabaseStateManager.getState → isDatabaseOK) and from the knex-migrator CLI, which loads the same file. Fixed by handing over a deep clone.

Not a bug

sanitizeDatabaseProperties in config/utils.ts deletes from config.get('database').connection, but runs at loader.ts:77 and freeze() is line 106 — pre-freeze. My probe observes regardless of frozen state, so it over-reports here.

Test-only sites

The remaining ~30 sites are all in test/utils/* and migration tests (fixture-utils.js, db-utils.js, db-template.js), which mutate config.get('database').connection to point at per-worker databases. Harmless today because freeze is skipped under test — but they are the reason it can't simply be switched on there, which is worth knowing given the PR's own "Known gap".

The premise that needs revisiting

This PR's design rests on mutations failing loudly:

Freezing makes that a TypeError naming the site.

That only holds in strict-mode code.

sloppy module body   : no throw, value unchanged
sloppy function      : no throw, value unchanged
class constructor    : THREW TypeError

AdapterCacheRedis threw because the write is inside a class body, which is always strict. loggingrc.js and knex-migrator are plain CommonJS module bodies — sloppy — so they silently no-op. Most of core/**/*.js is sloppy CommonJS.

So for a large share of the codebase, the freeze converts "config write works" into "config write silently does nothing". That's arguably worse than either the old behaviour or a loud failure, and it's exactly why 2 of the 3 bugs above were invisible to CI, to a development boot, and to a Pro boot.

Worth deciding explicitly before this lands — not necessarily blocking, but the justification for deep-freezing over cloning-on-read is weaker than the PR currently states. Options as I see them: keep the freeze and accept silent no-ops outside strict code; add a dev/CI-only recording mode like the probe I used, so these surface in testing; or clone on read for object values, trading the performance win for a guarantee that holds everywhere.

Verification after the two new fixes: e2e 394 passing, and the logging and migrator values confirmed correct under a frozen config.

---

Generated by Claude Code

[acburdine]

Superseded. Closing in favour of #31326 (merged) and #30936.

The two bugs this branch found shipped in #31326, reimplemented rather than cherry-picked: configure() in data/db/connection.js mutating config.get('database'), and AdapterCacheRedis folding ttl into clusterConfig. Both derive a new object now instead of cloning defensively, and configure() moved into its own module so it can be tested with frozen inputs on both client shapes.

The freeze mechanism is replaced in #30936. Rather than memoising get() over nconf and rejecting mutators, nconf's job ends at layering the sources: the merged tree is validated against a zod schema, deep-frozen, and that frozen tree is the only representation anything reads. That removes the two-representations problem — and with it the required()-is-not-a-mutator trap this branch hit, since there's no mutator blocklist.

Two findings from here that were worth keeping:

• The keyless-get() memory concern doesn't hold up. This branch left get() uncached because caching the merged tree would retain every environment variable. Measured, that tree is 34 KiB (117 top-level keys, 59 of them env vars), so Added a frozen, validated config via a zod schema #30936 retains it.
• The proxy prototype had the right instinct. Deep-freezing is unenforceable in sloppy-mode CommonJS, so Added a read-only guard that makes config writes throw in dev and CI #31330 adds a proxy guard gated to development and test*, with the freeze kept for production. Running it found a third write-through site nothing else could have: MigratorConfig.js handing the database subtree to knex-migrator, which mutates what it's given.

No performance left behind: getSiteUrl() is 39ns on the new design against the 63ns memoising achieved here, and get('url') is 30ns.

Also correcting a claim I made on the way, in case it's read later: node --use-strict does not make Ghost's .js files strict. It makes only the entry point strict — a require()d CommonJS module keeps its own strictness — so a suite run under it proves nothing about the modules under test.