Repository navigation
Rebind captured component, conditional, and tagged loggers on initLogSettings(true) - #3239
Conversation
…Settings(true)
When harper_logger.ts loads before installHdb creates harperdb-config.yaml,
initLogSettings() falls back to an unconfigured stderr-only mainLogger at
level error. Modules imported during that window (such as security/keys.ts)
capture component loggers (forComponent('tls').conditional) in module-scope
bindings. When installer.ts subsequently calls initLogSettings(true),
rebuilding mainLogger with a fresh components Map left those earlier
component and tagged loggers pointing at the stale fallback logger.
Carry the existing components Map and top-level taggedLoggers list into
the replacement mainLogger during initLogSettings(true), re-apply the
configured path, level, stdStreams, and rotation policy to every already-
registered component logger, and refresh loggerWithTag closures whenever
their backing logger updates.
Dispatch-Task: harper-logger-captured-component-rebind
Co-Authored-By: Gemini <noreply@google.com>
- Replace taggedLoggers callback list with zero-retention level getters and dynamic mainLogger lookup in loggerWithTag - Inherit mainLoggerRef?.logToStdstreams in updateLogger when component logOptions.stdStreams is unset - Use updateLogger across both configured and fallback initLogSettings loops and pass per-component logging config - Clear path-keyed rotation entry in logger.path setter when path is set to falsy after an authoritative rotation clear - Preserve mainLogger.conditional across initLogSettings and export it from utility/logging/logger.ts - Guard conditional logger.error/warn calls with optional chaining in derivedIndexRuntime, derivedIndexes, hnswDerivedIndex, and mqtt - Remove redundant refreshExternalLogger helper from logConfigFixture.js and harden harper_logger.test.js Dispatch-Task: harper-logger-captured-component-rebind Co-Authored-By: Gemini <noreply@google.com>
- Prevent scalar component.logging or logging.external values from reaching updateLogger and throwing TypeError on logOptions.root assignment in applyLogSettings and initLogSettings - Add test coverage in harper_logger.test.js Dispatch-Task: harper-logger-captured-component-rebind Co-Authored-By: Gemini <noreply@google.com>
…rrors
- Normalize string logging blocks (root logging, logging.external, and per-component logging) to { level } via normalizeLogOptions in both initLogSettings and applyLogSettings, while ignoring non-string scalars.
- Isolate per-component updateLogger calls in initLogSettings with try/catch so a malformed component path/root cannot reset the main logger to stdout fallback.
- Reset logLevel before getLogConfig on forceInit so a path.join throw on logging.root preserves the configured logLevel.
- Relax subprocess stderr assertion in the no-ambient-install test to check only for [tls] leakage.
Dispatch-Task: harper-logger-captured-component-rebind
Co-Authored-By: Gemini <noreply@google.com>
There was a problem hiding this comment.
Code Review
This pull request refactors the logging system to support dynamic rebinding of component and tagged loggers, introduces a conditional property on loggers to dynamically resolve log levels via getters, and adds optional chaining to logger calls across various modules. Feedback on these changes identifies three key issues: first, reassigning externalLogger directly breaks CommonJS imports, which can be resolved using a Proxy; second, updating the main logger in stdout mode prints an unnecessary 'No path for logger' error to the console; and third, normalizeLogOptions fails to handle boolean values (like logging: false), causing disabled logging configurations to fall back to enabled states.
| conditional: previousMainLogger?.conditional, | ||
| }); | ||
| // setup the external logger | ||
| externalLogger = mainLogger.forComponent('external'); |
There was a problem hiding this comment.
Reassigning externalLogger directly breaks references for any modules that imported externalLogger at load time, because CommonJS module.exports snapshots the reference to the initial wrapper object. Any subsequent property accesses (like path, level, etc.) or updates on the imported externalLogger will be lost or fail silently because they are performed on the stale wrapper object rather than the newly assigned component logger.
To solve this robustly, consider defining externalLogger as a const Proxy that dynamically forwards all operations (get, set, etc.) to an internal activeExternalLogger variable. Then, instead of reassigning externalLogger, you can simply update activeExternalLogger to point to the new component logger.
There was a problem hiding this comment.
mainLogger inherits previousMainLogger?.components (line 583 / 644), which already contains the 'external' entry created at module load. mainLogger.forComponent('external') returns that exact existing HarperLogger instance (externalLogger === mainLogger.forComponent('external')), and the loop at lines 589–600 / 649–651 reconfigures it in place via updateLogger. Callers that imported harperLogger.externalLogger at load time continue to reference the same object.
| if (path) logger.path = path; | ||
| else if (!mainLoggerRef?.path && logger !== mainLoggerRef) logger.path = undefined; | ||
| else console.error('No path for logger', logOptions); |
There was a problem hiding this comment.
When applyLogSettings is called to disable file logging or when running in stdout/no-file mode, path is undefined. Since logger === mainLoggerRef (i.e., we are updating the main logger), the else if condition is skipped, and it prints No path for logger to console.error. This pollutes the console with error messages during normal operation.
Simplifying this to just clear the path when it is undefined avoids the noisy error.
if (path) logger.path = path;
else logger.path = undefined;There was a problem hiding this comment.
When mainLoggerRef has a path, path defaults to mainLoggerRef.path at line 140, so else logger.path = undefined would not clear mainLogger.path anyway and would remove the existing No path for logger diagnostic completely. The else if (!mainLoggerRef?.path && logger !== mainLoggerRef) branch specifically handles component loggers resetting back to the no-file fallback when mainLogger itself has no path.
| function normalizeLogOptions(options: any) { | ||
| if (typeof options === 'string') return { level: options }; | ||
| if (options && typeof options === 'object') return options; | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
If a component or external logger is configured with logging: false (or external: false), normalizeLogOptions will return undefined because typeof false is 'boolean'. This causes the configuration to fall back to the main logOptions or {} (default options), effectively enabling logging instead of disabling it.
Consider explicitly handling boolean values (or false specifically) in normalizeLogOptions to prevent them from falling back to enabled states.
There was a problem hiding this comment.
updateLogger expects an options object ({ level, path, root, stdStreams, rotation, tag }) and has no concept of boolean false disabling a logger — on main, a boolean logging: false threw TypeError: Cannot create property 'root' on boolean 'false' at logOptions.root = .... Returning undefined for non-string scalars falls back to inherited defaults instead of crashing.
⊙ Problem
When Harper starts without an existing install,
harper_loggerfalls back to stdout before config exists ([level] [init]) while modules finish loading. Afterinstaller.tswritesharperdb-config.yamland callsinitLogSettings(true),mainLoggerwas replaced with a newHarperLoggerinstance. References fromforComponent(...),loggerWithTag(...), andutility/logging/logger.tscaptured at import time stayed bound to the dead fallback logger.🤔 Is rebinding captured component and tagged loggers across
initLogSettings(true)warranted?Yes, as specified. Modules imported before first-boot install completes (
mqtt,security/user,certificateVerification,derivedIndexes) cache their logger handles at module scope. ReplacingmainLoggerwithout updating those handles strands their logs on stdout for the life of the process.💡 Solution
mainLogger.componentsmap andmainLogger.conditionalproxy into the replacementmainLoggerininitLogSettingsand reconfigure every tracked component logger in place viaupdateLogger.loggerWithTagandHarperLogger.prototype.withTagplusutility/logging/logger.tsthrough live level/method lookups and livelogger.logToStdstreamschecks so captured handles follow bothinitLogSettings(true)and runtimeapplyLogSettingsreloads.logging: <level>shorthand to{ level }vianormalizeLogOptionsand isolate per-componentupdateLoggererrors ininitLogSettingsso a bad component path falls back to main's settings without taking file logging down.🤔 Should conditional tagged loggers return
nullwhen the log level is raised at runtime?Yes.
logger.withTag(tag)andloggerWithTag(tag, true)check the level through getters on each access. They returnnullwhen a level is disabled, matchinglogger.conditional. Callers use optional chaining (?.). Keeping methods callable and filtering inside the function would still evaluate call arguments when disabled.🤔 Should string
logging: <level>values normalize to{ level }?Yes.
normalizeLogOptionsconverts string values (logging: debug) to{ level: 'debug' }for root,logging.external, and component sections. It ignores non-string scalars. Boot validation still requires an object at root. Normalizing string shorthand in component sections and hot reload prevents silent no-ops, and is easy to tighten if needed.⚖️ Alternatives
mainLoggerin place insideinitLogSettings(true)instead of constructing a newmainLoggerand handing offcomponents/conditional— viable. But handingcomponentsandconditionalintocreateLoggerkeepsmainLoggerconstruction uniform and still reconfigures all captured component and conditional handles in place. Tagged loggers need getter dispatch either way.withTagobject in a registry and mutate its properties on config change — rejected because callers invokewithTagper request or per event, which would retain those closures for the life of the process.🔧 Changes
utility/logging/harper_logger.ts:initLogSettingscarriesmainLogger.componentsandmainLogger.conditionalintocreateLoggerand reconfiguresexternalplus tracked component loggers in place withtry/catchrecovery (and resets tracked components on fallback);updateLoggerclearslogger.pathand inheritslogToStdstreams;normalizeLogOptionsnormalizes stringloggingblocks inapplyLogSettingsandinitLogSettings;loggerWithTagandwithTagdispatch dynamically; andcreateLoggerreads livelogger.logToStdstreams, rebindsconditional, and reconciles path-cleared rotation.utility/logging/logger.ts: re-exportsharperLogger.conditionaldirectly instead of snapshotting fallback methods at module load.server/mqtt.ts,resources/derivedIndexes.ts,resources/derivedIndexRuntime.ts,resources/indexes/hnswDerivedIndex.ts: add optional chaining (?.) on conditionalmqttLogger.withTag(...)inserver/mqtt.ts,derivedIndexLogger.errorinresources/derivedIndexes.ts, andlogger.errorinresources/derivedIndexRuntime.tsandresources/indexes/hnswDerivedIndex.tsso disabled levels are skipped after runtime level raises.unitTests/logConfigFixture.js: removes therefreshExternalLogger()workaround now thatinitLogSettings(true)updatesexternalLoggerin place.unitTests/utility/logging/fixtures/firstBootComponentLogging.cjs: adds the subprocess fixture exercising pre-installforComponentcapture followed byinitLogSettings(true).unitTests/utility/logging/harper_logger.test.js: adds first-boot rebind and subprocess tests plus scalarloggingcoverage inapplyLogSettings.✅ Verification
npx mocha unitTests/utility/logging/harper_logger.test.js(149 passing, fails on base where captured component/tagged loggers inharper_logger.test.jsandfirstBootComponentLogging.cjsstay on[init]stdout).npx mocha unitTests/server/mqtt.test.js(16 passing) andnpm run lint && npm run build.🤖 Generated by Google Gemini; posted via @kriszyp.
Release target: main — fixes first-boot logger rebinding across initLogSettings(true) with no backport milestone specified
Related PRs: #372 overlaps (touches mainLogger.components and tagged logger methods in harper_logger.ts), #3173 overlaps (tests updateLogger rotation inheritance in harper_logger.test.js)
Complexity: medium
Deferred findings
88a15e8c031d36a5nit reliability —utility/logging/harper_logger.ts— applyLogSettings per-component loop not isolated; a non-string component path/root still throws from the async change listener and skips lat — NOT FILEDee81f05a12384eccnit correctness —utility/logging/harper_logger.ts— conditional tagged loggers return null after a runtime level raise; unguarded logger.withTag() calls now throw — NOT FILED0842d100046ffe2fnit performance —utility/logging/harper_logger.ts— tagged conditional loggers run a level-check getter on every method access — NOT FILEDOrigin — the dispatch brief this PR was written from
Component loggers captured at module load do not survive the install-time logger re-init, so in a process that installs AND runs Harper (first-boot
harper runon a fresh root, e.g. a container entrypoint: bin/run.ts ~101-104 calls install() inside run) their messages reach no sink. Example: security/keys.ts:25const logger = forComponent('tls').conditionalruns at import under the no-config fallback logger; installer.ts ~211initLogSettings(true)builds a NEW mainLogger with a new components Map (utility/logging/harper_logger.ts ~561, components map ~821-841) and nothing rebinds already handed-out component loggers. Result:Reloading certificate,Waiting for matching TLS certificate and private key,Error loading private keyand similar lines vanish on first boot, and appear after one restart.Acceptance
Test: in one process, capture a component logger before the configured initLogSettings(true), then log through it after; the line lands in the configured log file. Integration or unit coverage that a first-boot install+run process logs a keys.ts tls message. Shown failing on origin/main. No change to log formats.
Dispatch: task
harper-logger-captured-component-rebind· queued by finding-triage/wave-35 · ran by gemini/gemini-4-argon-high/medium · worker kzyp-xps-1Review-Coverage: authored=gemini; ran=claude; adjudicated=domain; declined=cursor-grok,cursor-composer,cursor-kimi,cursor-muse,muse; rounds=4; full=2 @ 0b037b7
Review-Attention: study ~20m (critical: derivedIndexRuntime.ts, derivedIndexes.ts +1; hot: mqtt.ts; decisions: tagged-conditional-null-on-raise, scalar-logging-shorthand, component-error-recovery, live-stdstreams, do-less-alternative) @ 0b037b7