Skip to content

fix(logging): default rotation path to <log dir>/rotated when unset - #1090

Merged
kriszyp merged 2 commits into
mainfrom
fix/log-rotation-default-path
Jun 2, 2026
Merged

fix(logging): default rotation path to <log dir>/rotated when unset#1090
kriszyp merged 2 commits into
mainfrom
fix/log-rotation-default-path

Conversation

@ldt1996

@ldt1996 ldt1996 commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

closes #1088

Summary

  • Default rotation.path to <dirname(logger.path)>/rotated inside logRotator when not explicitly set, so rotation works out of the box when only LOGGING_ROTATION_MAXSIZE (or LOGGING_ROTATION_INTERVAL) is configured.
  • mkdirSync(rotatedLogDir, { recursive: true }) at init so the first rotation's rename doesn't ENOENT on the freshly defaulted dir.
  • Drop the now-unused PATH_UNDEFINED_MSG constant.

Context

Today the default config ships with logging.rotation.enabled: true, maxSize: 64M, path: null. The rotator at utility/logging/logRotator.ts:48-50 throws 'logging.rotation.path' is undefined, ... when path is null, and harper_logger.ts:638-640 catches the throw silently and re-emits it through the file logger we're trying to rotate. End result: an operator setting only LOGGING_ROTATION_MAXSIZE=512M gets no rotation and no actionable signal until the disk or quota fills.

Three fix shapes were considered in #1088: surface the catch to stderr, default the path, or fail config validation when enabled: true and path: null. This PR takes "default the path" because the other two either don't actually fix the broken default (just make it noisier) or break every existing deploy on upgrade.

The default lives in logRotator rather than in defaultConfig.yaml so it stays adjacent to whatever the actual log file path resolves to at runtime. An explicit LOGGING_ROTATION_PATH continues to override the default, and LOGGING_ROTATION_ENABLED=false continues to disable rotation entirely.

Test

unitTests/utility/logging/logRotator.test.js: the existing "Test error logged if rotation path is undefined" is replaced with "Defaults rotation path to /rotated when path is not set", which calls the rotator with path: null and asserts the rotated file lands under <LOG_DIR_TEST>/rotated/.

@github-actions

github-actions Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Patch cherry-pick: conflict

Cherry-pick onto v5.0 produced conflicts on commit(s): c1b9669a8d19cccceaeb62a3ab4e4d9e670dad3d

The conflict markers are committed on branch cherry-pick/v5.0/pr-1090.
A pull request has been opened to land this patch: #1094

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the log rotator to default the rotation path to <log dir>/rotated when it is not explicitly configured, rather than throwing an error. It also ensures the directory is created, and updates the unit tests to reflect this new behavior. The review feedback suggests replacing the synchronous mkdirSync call with an asynchronous fsProm.mkdir to avoid blocking the Node.js event loop, and consequently removing mkdirSync from the imports.

'use strict';

import { promises as fsProm, createReadStream, createWriteStream } from 'fs';
import { promises as fsProm, createReadStream, createWriteStream, mkdirSync } from 'fs';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Since mkdirSync is being replaced with an asynchronous alternative to prevent blocking the event loop, we should remove it from the fs imports.

Suggested change
import { promises as fsProm, createReadStream, createWriteStream, mkdirSync } from 'fs';
import { promises as fsProm, createReadStream, createWriteStream } from 'fs';

Comment thread utility/logging/logRotator.ts Outdated
Comment on lines +52 to +53
// Ensure the directory exists; moveLogFile's rename would otherwise ENOENT on first rotation.
mkdirSync(rotatedLogDir, { recursive: true });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using mkdirSync blocks the Node.js event loop, which can degrade performance in a high-throughput database environment like HarperDB.

We should perform this directory creation asynchronously using fsProm.mkdir and handle any potential errors gracefully.

Suggested change
// Ensure the directory exists; moveLogFile's rename would otherwise ENOENT on first rotation.
mkdirSync(rotatedLogDir, { recursive: true });
// Ensure the directory exists asynchronously to avoid blocking the event loop.
fsProm.mkdir(rotatedLogDir, { recursive: true }).catch((err) => {
hdbLogger.error('Failed to create rotated log directory', err);
});

@claude

claude Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

@kriszyp
kriszyp merged commit 0f7e641 into main Jun 2, 2026
41 checks passed
@kriszyp
kriszyp deleted the fix/log-rotation-default-path branch June 2, 2026 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Log rotation silently fails when LOGGING_ROTATION_PATH is unset

2 participants