fix(logging): stop the no-config window dropping every log line - #2467
Merged
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request simplifies the formatting of two test assertions in unitTests/resources/query-array-scoping.test.js by condensing multi-line assert.deepStrictEqual calls into single lines. There are no review comments, and I have no feedback to provide.
kriszyp
marked this pull request as ready for review
September 2, 2026 12:07
github-actions
Bot
requested review from
Ethan-Arrowood,
cb1kenobi and
heskew
September 2, 2026 12:07
Contributor
|
Reviewed; no blockers found. |
initLogSettings() falls back to a no-config branch on any host without a harperdb-config.yaml — the install window, and a fresh CI runner. It sets `log_to_file = false; logToStdstreams = true`, which reads as "the streams are the sink now", and then called createLogger() without passing stdStreams. createLogger destructures that option into a local of the same name, which shadows the module-level flag inside logStdOut/logStdErr, so the branch wrote nowhere at all: before a config exists, every log line Harper produced was silently dropped, install errors included. That branch also returns before the stdioLogging() call at the end of initLogSettings(), so the streams it now writes to would have had no EPIPE/EIO listener — `harper install | head -1` closes the reader, and the async error would land on a stream with none and take the install down. Install the guards there too; their write override is inert on this branch, because log_to_file is false. Found from the Windows unit gate, where the guard's warn-cadence case counted 0 of 2 warnings (harper#2364). #2468 has since made that harness bring its own config, which is the right fix for the test; this is the product bug underneath it, which that leaves in place. A child-process case pins both halves: it spawns with ROOTPATH at a directory holding no config, which reaches the fallback on an installed machine and a bare one alike, and asserts the warning on stderr and the guard listener on each stream. It fails on the old createLogger() call, and again without the stdioLogging() call. Verified: `npm run test:unit:windows` (the Windows gate's own groups, on Linux). Refs #2364 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gPFHKnh2qasUuy53fdzbe
kriszyp
force-pushed
the
fix/format-check-query-array-scoping
branch
from
September 2, 2026 15:05
97396cc to
521369e
Compare
cb1kenobi
approved these changes
Sep 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
initLogSettings()falls back to a no-config branch on any host without aharperdb-config.yaml— the install window, and a fresh CI runner. It setslog_to_file = false; logToStdstreams = true, which reads as "the streams are the sink now", and then calledcreateLogger()without passingstdStreams.createLoggerdestructures that option into a local of the same name, which shadows the module-level flag insidelogStdOut/logStdErr— so the branch wrote nowhere at all. Before a config exists, every log line Harper produced was silently dropped, install diagnostics included.That branch also
returns before thestdioLogging()call at the end ofinitLogSettings(), so the streams it now writes to would have had no EPIPE/EIO listener:harper install | head -1closes the reader and the async error lands on a stream with none, taking the install down. The guards are installed on that branch too; their write override is inert there, becauselog_to_fileis false.This is what the Windows gate was actually reporting when
watcherFallback.test.js's warn-cadence case counted 0 of 2 warnings (#2364): the Ubuntu unit job runsharper installfirst and the Windows job does not, so only Windows had no config to read. #2468 gave that harness a config of its own — the right fix for the test — which leaves the product behaviour unchanged.A child-process case pins both halves. It spawns with
ROOTPATHat a directory holding no config, which reaches the fallback on an installed machine and a bare one alike (with boot properties present the config read throws ENOENT; without theminitLogSettings()only swallows that failure whenROOTPATHdoes hold a config), and asserts the warning on stderr plus the guard listener on each stream.For the human reviewer
notify/inforoute to stdout, so a CLI emitting machine-readable stdout before a config exists would interleave log lines with it (none does at those levels today); and the branch hard-setslogToStdstreams = truewhile its cmd/env loop honours onlyLOGGING_LEVEL/LOGGING_CONSOLE, so an operator's explicitLOGGING_STDSTREAMS=falseis overridden in that window, where it was previously inert by accident.createLoggerso the module-levellogToStdstreamsstays visible and no caller can silently drop it. The shadow is the actual trap and it will re-fire on the next new caller — but the rename changeslogStdOut's fallback semantics for every logger, which is a call for a human rather than a side effect of a CI fix.disableStdio(), which replaces the process-globalnativeStdWriteand therefore mutes stderr along with stdout — worth its own change, not this one.logStdOut/logStdErrread the per-instancelogger.logToStdstreamsin the file branch but the closure capture in the no-file branch, whileupdateLoggerwrites only the property — so withlogging.file: false, flippingstdStreamsthrough the config watcher has no effect. Same option-plumbing trap, one level up. Alsoif (process.env.DEV_MODE) logToStdstreams = trueruns aftercreateLogger()in both branches, so it has been dead for the same shadowing reason.Verification
npm run test:unit:windows(the Windows gate's own group runner, on Linux) — all 9 groups pass, 3637 tests.createLogger()option fails it on the missing warning; removing thestdioLogging()call fails it onstdout-guard=true stderr-guard=true.AGENTS.md—node:assertagainst a real child process, nosinon/rewire; the fallback branch is reachable by spawning with a config-lessROOTPATH, so there was nothing to stub.npx prettier --check .passes repo-wide;npx oxlintclean on the changed files.Refs #2364, #2468
🤖 Generated with Claude Code
https://claude.ai/code/session_017gPFHKnh2qasUuy53fdzbe
Co-Authored-By: Claude Opus 5 noreply@anthropic.com
Review-Coverage: authored=claude; ran=codex; adjudicated=domain; blocked=gemini(auth); declined=cursor-grok,cursor-composer; rounds=8 @ 521369e
Human-Review-Need: 3 (decisions: epipe-survive-vs-crash, stdstreams-fix-at-callsite-vs-shadowing, child-process-fixture-vs-rewire) @ 521369e