fix(threads): worker threads must inherit the parent's in-process config overrides - #2024
fix(threads): worker threads must inherit the parent's in-process config overrides#2024kriszyp wants to merge 9 commits into
Conversation
Each worker thread reads harper-config.yaml from disk independently (environmentManager.ts's flatConfigObj is module-scoped, i.e. per-thread), so in-process overrides applied via env.setProperty() on the main thread (installer bootstrap, the unitTests/apiTests harness's per-run isolation) never reached HTTP/job worker threads. A worker fell back to whatever was actually installed on disk, silently binding the ambient config's ports on the wildcard address instead of the caller's overrides. - environmentManager.ts: setProperty() now records every override into an in-order map; getConfigOverrides() exposes it; initSync() replays inherited overrides (from workerData.configOverrides) once per thread, and again on a forced re-init. - manageThreads.js: register a 'configOverrides' workerData provider via the existing registerWorkerDataProvider extension point, so every worker spawned from any thread inherits its parent's effective overrides rather than the on-disk config. - setupTestApp.mjs: also null out operationsApi.network.securePort alongside the existing port override, mirroring what it already does for http.securePort — an ambient install with a non-null securePort left that stale TLS port in the effective config next to the plain-port override. - mqtt-test.mjs: both mTLS tests selected the client certificate by the literal string 'localhost'; switched to getThisNodeName(), the same function the server uses to name its own self-signed node certificate, so the lookup works regardless of the ambient install's hostname. Refs the apitests worker-config-isolation task. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rrides componentLoader.ts's root-component load reads getConfigObj()'s nested tree directly (config = getConfigObj() for isRoot), and derives each built-in component's network port/protocol from it (e.g. operationsApi's network.securePort decides whether operationsServer builds a TLS-only Fastify instance). updateConfigObject() — the function env.setProperty() calls — only ever wrote to the flattened flatConfigObj map, so any override applied in-process never reached that nested tree: componentLoader kept reading whatever was on disk regardless of the override. This was invisible against a CI-shaped install (on-disk operationsApi port/securePort already matched what the test harness wants — plain, port 9925), but on an install whose ambient operationsApi.network.securePort is non-null, componentLoader still saw the on-disk securePort and built a TLS-only listener, so the test harness's plain-HTTP operations-API POSTs got "socket hang up" against it even though the harness had correctly overridden the port to null via setProperty. updateConfigObject() now also writes the same value into the nested configObj tree, at the path implied by the flat key's underscore segments (mirroring how updateConfigValue's file-write path already resolves the on-disk YAML path). Verified against an adversarial local install shaped like the acceptance criteria (operationsApi/http port+securePort swapped, non-'localhost' node hostname). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ritance fix
Independent review (codex + gemini + grok + harper-domain) on the prior two
commits found several real defects in the override-propagation mechanism:
- configUtils.ts's updateConfigObject() unconditionally created configObj = {}
before mirroring into it. getConfigObj() treats a falsy configObj as "not yet
initialized" and lazily calls initConfig() — an empty-but-truthy configObj
permanently short-circuited that lazy init, so a setProperty() call before
the config file exists (e.g. during install) could leave componentLoader's
root-component load seeing zero components. Now only mirrors when configObj
is already a real, initialized tree.
- The same mirroring logic clobbered an existing scalar value (e.g. a legacy
`threads: 4` shorthand) into `{}` when descending through it for an
unrelated nested override. Now only auto-vivifies a missing segment; an
existing non-object segment is left untouched (the flat map still gets the
override either way).
- environmentManager.ts's initSync(force) only replayed workerData-inherited
overrides, not overrides applied locally on the current thread (including
the main thread, which has no workerData at all). A forced re-init — e.g.
security/keys.ts's RESTART handling — re-reads the whole config from disk
and was silently dropping every local override in the process. Added
reapplyAllOverrides(), which replays the full appliedOverrides set (locally
applied + previously inherited) after a forced reload.
- The mTLS-cert fix in the prior commit only touched the TCP mTLS test;
mqtt-test.mjs's WSS mTLS sibling (identical code, one extra indent level)
still selected the client cert by the literal string 'localhost' — an
indentation mismatch caused the earlier replace_all to silently miss it.
Now fixed identically to the TCP test.
- applyInheritedConfigOverrides() used a bare `for...in` over the
workerData-derived object; switched to Object.keys() so an unrelated
Object.prototype addition can't get replayed into config.
- Corrected the module comment claiming setProperty() is "never touched on a
normal production boot" — dataLayer/harperBridge/lmdbBridge's
initializePaths.js and utility/lmdb/environmentUtility.ts call it at
runtime with the resolved databases/storage.path config on every
LMDB-engine install.
Also added unit test coverage the review flagged as missing: getConfigOverrides()
tracking, reapplyAllOverrides() replay semantics, the forced-vs-non-forced
initSync() distinction, and the nested-mirror/lazy-init/scalar-clobber
behavior in configUtils.ts. Fixed a pre-existing test-hygiene bug found along
the way: environmentManager.test.js's 'Test initTestEnvironment function'
block rewired setProperty to a no-op stub without ever reverting it, silently
breaking any later describe block in the file that relied on the real
implementation.
Re-verified against the same adversarial and CI-shaped installs: 194
passing / 0 failing on both.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ng, derived-value leakage)
Second cross-model review round (codex + gemini + grok + harper-domain) on
the fix-round commit found two more real defects, both reproducing the same
parent/worker (or reload/disk) skew this branch exists to eliminate:
- appliedOverrides was keyed by the raw propName passed to setProperty(),
but CONFIG_PARAM_MAP is many-to-one (e.g. SERVER_PORT_KEY and
OPERATIONSAPI_NETWORK_PORT both canonicalize to
'operationsApi_network_port'; five different keys canonicalize to
'threads_count'). Two aliases for the same param got two separate Map
entries, and replay order (Map insertion order, not canonical-value
recency) could apply a stale alias after a fresher one — a worker could
end up disagreeing with its own parent on the very value this mechanism
exists to synchronize. Now keys by the canonical CONFIG_PARAM_MAP name
(falling back to the raw name for anything unmapped), so every alias for
the same param collapses onto one entry that always reflects the latest
write.
- dataLayer/harperBridge/lmdbBridge/lmdbUtility/initializePaths.js and
utility/lmdb/environmentUtility.ts called env.setProperty() to cache a
value they had just *derived* from reading the config (a schema path
resolved from CLI args, a database's on-disk path) — not an operator/
harness override. Going through setProperty() made that derived value
outlive a forced config reload (reapplyAllOverrides() would stamp the
stale derivation back over a freshly re-read disk config, e.g. after an
admin edits storage.path and triggers a RESTART) and get shipped to
every worker as if it were an intentional override. Both call sites now
call configUtils.updateConfigObject() directly, which updates the same
in-memory state without joining the override-tracking/propagation
mechanism.
Findings not acted on this round (per the domain adjudication — dropped as
either already covered, non-reproducible, or pre-existing/out of scope):
gemini's prototype-pollution "blocker" (split('_') can't produce a
__proto__/constructor segment reachable through this path — traced and
confirmed unreachable), gemini's "initSync(false) drops pre-existing
overrides" (inverted — initConfig(false) skips the disk read entirely once
flatConfigObj is set, so there's nothing to drop), and the ambient
`network.https` switches / non-cloneable-value / auto-vivification-of-
absent-sections points, which are pre-existing or narrow enough to track
separately (see dispatch Findings).
Re-verified: adversarial + CI-shaped test:unit:apitests (194/0 both),
targeted unit tests, and a short-path throwaway worktree run of
test:unit:main + test:unit:resources (both clean, matching baseline) — see
dispatch log for full detail.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…al keying
Third cross-model review round (codex + gemini + grok + harper-domain) on the
round-2 fix found one more real, confirmed defect — a regression the
round-2 canonical-keying change itself introduced:
- setProperty('HDB_ROOT', v) has two effects: installProps.HDB_ROOT = v
(installPropsToSave keys on the raw HDB_ROOT_KEY name), and the flat/nested
config gets updated. Round 2 made appliedOverrides key by the canonical
CONFIG_PARAM_MAP name ('rootPath'), so a replay calls setProperty('rootPath',
v) — updating config correctly, but installPropsToSave['rootPath'] doesn't
exist, so the installProps side effect silently no-ops on replay. A worker
that inherited an isolated rootPath override (e.g. the apiTests harness's
per-PID root path) would have env.get(ROOTPATH) report the isolated dir
while getHdbBasePath() stayed pinned to the ambient install — the exact
divergence this whole branch exists to eliminate, just moved to a
different accessor. Fixed by reordering initSync(): the installProps.
HDB_ROOT sync now runs after inherited/forced-reload override replay,
re-deriving from the (by then correctly-replayed) config value instead of
the pre-replay disk read — this works regardless of which alias produced
the override, since it goes through the config value rather than replaying
setProperty's own installPropsToSave side effect a second time.
Also fixed a smaller, related correctness gap the same round surfaced:
updateConfigObject()'s nested-tree mirror wrote an `undefined` value through
as an enumerable object key (squashObj, two screens away in the same file,
already treats `undefined` as "absent" and skips it). Now deletes the key
instead, matching that convention — several root-config readers iterate
configObj's own keys and assume every one holds a real value.
Findings not acted on this round (per the domain adjudication — all three
outside-model "blocker"/"major" claims traced and confirmed non-issues):
gemini's `envMgr is not defined` claim (manageThreads.js already uses envMgr
consistently — false positive), gemini's prototype-pollution claims via
`__proto__`/`constructor` segments (traced: CONFIG_PARAM_MAP lookup on an
attacker string returns undefined before the loop is ever reached), and
gemini's repeat of the already-dropped "initSync(false) drops pre-existing
overrides" claim (still inverted — nothing to drop). A snapshot-by-reference
concern (mutating a live object after it's been recorded as an override) and
a legacy-scalar/nested-mirror disagreement were both rated minor/nit with
"no live production trigger today" and are tracked as follow-ups rather than
fixed here (see dispatch Findings) — see git log for full detail.
Re-verified once more: adversarial + CI-shaped test:unit:apitests (194/0
both), targeted unit tests (94 passing, 3 new).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…to-vivify on delete
Fourth pre-push review round (codex + gemini + grok; the harper-domain leg
timed out, so this round's triage is my own trace-verification, documented
below) found two more real, concrete bugs and one real test-coverage gap in
the nested-mirror mechanism from the last three commits:
- SETTINGS_PATH_KEY ('settings_path') lives in hdbTerms.BOOT_PROP_PARAMS,
not the harper-config.yaml schema (its own comment: "used in the boot prop
file not the settings file"). CONFIG_PARAM_MAP still maps it (so the flat
map has always worked correctly for it), but updateConfigObject()'s
nested-mirror addition would split it on '_' and write
configObj.settings.path — a bogus top-level 'settings' section that
componentLoader.ts would try to load as a real component, since it treats
every truthy top-level root-config key as one. This isn't hypothetical:
utility/install/installer.ts calls setProperty() with this exact key on
every install, and unitTests/testUtils.js's initTestEnvironment() does
too. Fixed: updateConfigObject() now skips the nested mirror entirely for
any canonical key that resolves through BOOT_PROP_PARAMS (currently just
this one key, but the exclusion is by namespace, not a one-off name
check).
- The prior "delete rather than write undefined" fix still auto-vivified
missing ancestor segments before deleting the leaf, e.g. replaying
`mcp_operations_rateLimit_identityHeader = undefined` onto a config with
no `mcp` block would leave `configObj.mcp.operations = {}` behind —
again a bogus, now-empty section a presence-gated component check could
read as "configured". Fixed: when the value being set is undefined and an
ancestor segment doesn't exist, there's nothing to delete, so return
immediately rather than creating anything.
- Added a test that was genuinely missing: the round-2 canonical-keying fix
had no test using two actually-different aliases for the same canonical
param (the existing test reused the same literal key twice, which doesn't
exercise CONFIG_PARAM_MAP resolution at all). Added one using
SERVER_PORT_KEY / OPERATIONSAPI_NETWORK_PORT, which both canonicalize to
'operationsApi_network_port'.
Findings not acted on this round, based on my own trace since the domain
leg was unavailable: a repeat of the already-adjudicated (round 3) "initSync
drops pre-existing overrides" claim — re-traced here too, still inverted,
initConfig(force=false) skips its disk read entirely once flatConfigObj is
already set by an earlier setProperty, so there's nothing to drop; and a
repeat of the already-adjudicated (round 3) scalar-parent flat/nested
disagreement claim, where the concrete failure described doesn't reproduce
because squashObj never emits the divergent flat key for a scalar-shorthand
parent in the first place. Both are documented in the round-3 commit
message and the dispatch Findings, not re-litigated in full here.
Re-verified: adversarial + CI-shaped test:unit:apitests (194/0 both),
targeted units (97 passing, 3 new).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ode-name lookup misses Fifth/sixth pre-push review round (codex + gemini + grok; domain leg timed out twice) found a real gap in the getThisNodeName()-based cert lookup from two commits ago: getThisNodeName() is derived from the CURRENT effective config (node.hostname, or a fallback chain through the operations-API listener address) and caches its result per-process. On an install with no node.hostname set at all, the install-time process resolves that fallback chain against the on-disk, un-overridden config (typically landing on '127.0.0.1', since a bare port number with no host prefix doesn't parse as a hostname) and names the cert accordingly — but the TEST process resolves the same fallback chain AFTER the harness has already overridden operationsApi.network.port to the per-run loopback address, landing on a different value. The two processes' node identities diverge, so the name-match lookup silently returns no certificate and the mTLS connection fails with "certificate required" — reproduced locally against a fresh hostname-null install (194/194 before the fix's absence would have caught it only by accident on this repo's other test hosts, which all use an explicit hostname). This isn't really an mTLS-test bug so much as getThisNodeName() being the wrong tool for "identify this install's own cert" across two different processes with two different effective configs — the test only needs "the node's own cert, not the CA's", and a fresh/non-replicated install has exactly one non-authority cert. Both mTLS tests (TCP and WSS) now fall back to any non-authority certificate when the name lookup doesn't find one (getThisNodeName() is tried first since it's still the more precise match when install and test agree, e.g. an explicit non-'localhost' hostname). (Also fixes a second instance of the same indentation-mismatch mistake from the very first commit: a replace_all edit again silently touched only one of the two near-identical it() blocks in this file, because the WSS block sits one tab deeper inside its wrapping try — verified this time by reading both blocks back afterward.) Findings not acted on this round, traced myself (domain unavailable both attempts): gemini's "NON_NESTED_CONFIG_PARAMS.has() is case-sensitive" claim is a false positive — configObjKey is always the canonical, constant-cased value from CONFIG_PARAM_MAP (param.toLowerCase() is looked up, but the stored *value* has fixed casing baked into hdbTerms.ts, independent of the caller's casing), so no bypass exists; the dependent "case-sensitivity is untested" finding goes with it. Gemini's repeated prototype-pollution "blocker" claim was already traced and dropped in rounds 2 and 3 with the same reasoning (CONFIG_PARAM_MAP lookup on an attacker string returns undefined before the split/traverse loop is ever reached) and is dropped again here for the same reason. The "no deterministic worker-spawn integration test" point has now recurred across every round; still deferred (workerData isn't a rewire-able local binding once compiled, making the natural unit-test approach unworkable, and a real Worker-spawn test needs more care than remaining time allows) — tracked as a real-work follow-up in the dispatch Findings rather than re-litigated per-round from here on. Re-verified: adversarial + CI-shaped + a fresh hostname-null install, all test:unit:apitests 194/0. Targeted units unaffected (no test changes this commit; mocha still 97 passing). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces robust propagation and synchronization of in-process configuration overrides across main and worker threads, ensuring consistency and preventing configuration skew. It updates updateConfigObject to mirror overrides into the nested configuration tree, tracks overrides using canonical parameter names, and replays them upon worker thread initialization or forced configuration reloads. Additionally, direct configuration updates are used instead of setProperty for derived values to avoid unintended persistence. Feedback on the changes suggests using loose inequality (!= null) instead of strict inequality (!== undefined) when checking configObj to safely guard against both null and undefined values, adhering to the repository's style guide.
|
Reviewed; no blockers found. |
Multi-line Map array-literal wrapping in a test file I added; no behavior change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…i-code-assist) configObj is only ever assigned undefined (initial) or a parsed config object, so this was never reachable with the current callers, but != null is both the repo's preferred idiom for null-or-undefined checks and a real guard against a future/edge-case null (e.g. a config doc that parses to null) reaching the property access below it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
What
The
unitTests/apiTestssuite's isolation (per-PID rootPath, per-process loopback address, port overrides) is applied with in-processenv.setProperty()calls on the main thread. Worker threads spawned during the run booted with a freshenvironmentManager/configUtilsmodule state and re-readharper-config.yamlfrom disk, so a worker silently fell back to whatever was actually installed and bound its ports on the wildcard address — the suite only passed when the developer's ambient install happened to be shaped like CI's.Two closely-related root causes, both closing the same invariant ("a worker's effective config equals its parent's effective config"):
Worker threads never received the parent's overrides at all.
environmentManager.ts'sflatConfigObjis module-scoped (each worker thread is its own V8 realm), populated once from disk.setProperty()only ever mutated the calling thread's own copy.setProperty()now records every override into a canonical-name-keyed map (getConfigOverrides()), so different legacy aliases for the same param (e.g.SERVER_PORT_KEY/OPERATIONSAPI_NETWORK_PORT) collapse onto one last-write-wins entry instead of two independently-ordered ones.manageThreads.jsregisters aconfigOverridesworkerData provider via the existingregisterWorkerDataProviderextension point, so every worker spawned from any thread — HTTP, job, or nested — inherits its parent's overrides.initSync()replays inherited overrides once per thread, and replays the full accumulated override set again after any forced re-init (e.g. a component RESTART), since that re-reads the whole config from disk and would otherwise silently drop everything layered on top of it — on any thread, including main.Even on the main thread, overrides never reached the nested config tree
componentLoader.tsreads for built-in components.configUtils.tskeeps config in two representations: a flat map and a nested tree (configObj, viagetConfigObj()).updateConfigObject()— whatsetProperty()calls — only ever wrote the flat map.componentLoader.ts's root-component load reads the nested tree directly and derives each built-in's network port/protocol from it (e.g.operationsApi.network.securePortdecides whetheroperationsServerbuilds a TLS-only Fastify instance). So even with (1) fixed, a worker — or the main thread itself — whose ambient install had a non-nulloperationsApi.network.securePortstill built a TLS-only listener, because componentLoader kept reading the stale on-disk value regardless of any override. Invisible on a CI-shaped install only because the on-disk value already happened to match what the harness wants.updateConfigObject()now also mirrors into the nested tree, at the path implied by the flat key's underscore segments — with guards added over several review rounds: it never auto-vivifiesconfigObjbefore a real config has ever loaded (would permanently short-circuitgetConfigObj()'s lazy init), never clobbers an existing scalar shorthand (e.g.threads: 4) by descending through it, deletes rather than writes anundefinedvalue (and doesn't auto-vivify ancestors just to immediately delete the leaf), and excludes params that live inBOOT_PROP_PARAMSrather than the actual YAML schema (settings_pathis boot-props-file bookkeeping, not a realsettings:config section — mirroring it naively would register a bogussettingscomponent).Three smaller, related fixes:
setupTestApp.mjsnulledhttp.securePortwhen overridinghttp.port, but never did the same foroperationsApi.network.securePort— an asymmetry that (combined with fix 2) left a stale TLS port in the effective config next to the plain-port override.mqtt-test.mjs's two mTLS tests selected the client certificate by the literal string'localhost'. Switched to falling back throughgetThisNodeName()(the function the server itself uses to name its self-signed node cert) and then to any non-authority certificate, sincegetThisNodeName()'s own fallback chain can itself resolve differently between the install process and the test process on a hostname-null install (a fresh/non-replicated install has exactly one non-authority cert, so "any" is unambiguous here).dataLayer/harperBridge/lmdbBridge/lmdbUtility/initializePaths.jsandutility/lmdb/environmentUtility.tswere callingenv.setProperty()to cache values derived from config they'd just read (a schema path resolved from CLI args, a database's on-disk path) rather than genuine operator/harness overrides. Going throughsetProperty()made those derived values outlive a forced config reload and get shipped to every worker as if they were intentional overrides. Both now callconfigUtils.updateConfigObject()directly, which updates the same in-memory state without joining the override-tracking/propagation mechanism.Production-path impact
config/configUtils.ts'supdateConfigObject()change affects the same code path used at install time (utility/install/installer.ts) and any otherenv.setProperty()caller. This is a correctness fix within that path, not new behavior: it makesconfigObj/flatConfigObjconsistent with each other, which is whatsetProperty()'s contract already implied. A worker that receives no overrides sees the newworkerDataprovider returnundefinedand get skipped (per its existing "throws or returns non-cloneable → logged and skipped" contract) — production instances without in-process overrides see no behavior change.Known limitation (out of scope for this PR)
Root components that use the newer Plugin API (
handleApplication-based — MQTT, REST, fastifyRoutes, graphqlQuerying) read their config viaOptionsWatcher, which re-parsesharper-config.yamloff disk directly and never consultsgetConfigObj()/flatConfigObjat all.env.setProperty()overrides are invisible to that path. This PR's apitests verification (three different install shapes, including concurrent runs) didn't surface it because MQTT is still loaded through the older,componentLoader-driven path this PR does fix — but it's a real, pre-existing gap for any Plugin-API component, worth its own follow-up issue.Testing
Verified against three different local installs (
node ./dist/bin/harper.js install+ isolatedHOME, never the shared ambient install):operationsApi.network.port: null/securePort: 9925,http.port: null/securePort: 9926,node.hostnameset to a non-'localhost'value — matches this issue's exact regression scenario):npm run test:unit:apitests194 passing / 0 failing (was 181/10 on unpatched origin/main; 185/9 with only the worker-inheritance fix, before the nested-tree fix).DEFAULTS_MODE=dev,NODE_HOSTNAME=localhost): 194/0, unaffected.node.hostnameunset entirely): 194/0 — this is what caught the mTLS cert-fallback issue above.npm run test:unit:main(4043 passing) andnpm run test:unit:resources(1336 passing): unaffected, matches origin/main baseline (verified via a short-path throwaway worktree, since this repo's own worktree convention nests deep enough to independently trip two unrelated, pre-existing path-length issues — a domain-socket path limit and a Node module-sandbox check on a bare verification copy without its ownnode_modules— neither caused by this change).[http/N] ... EADDRINUSE ... :::1883in apitests output; no server banner reports a Root Path outside the per-PIDunitTests/envDir/<pid>directory.npm run test:unit:apitestsruns get distinct loopback addresses with noEADDRINUSEand correct per-run Root Path banners — the wildcard-bind collision this PR targets is gone. A separate mqtt-test.mjs timing sensitivity was observed under 2x concurrent full-suite load on this shared dev machine; it isn't port/address-collision-shaped (addresses were confirmed distinct) and wasn't root-caused — flagged as a follow-up, not blocking.environmentManager.test.js) and the nested-mirror guards (configUtils.test.js).This branch went through seven rounds of cross-model pre-push review (codex + gemini + grok + an internal Harper-domain pass) as fixes were made — see the commit messages for each round's findings and disposition.
🤖 Generated with Claude Code