Enforce logging.rotation.maxSize on the log write path instead of only on the 60-second audit tick

[kriszyp]

logging.rotation.maxSize is now enforced by the writers themselves, so the active log is bounded by the configured size rather than by how much can be written between two audit ticks. Previously the size check ran only inside the 60-second audit setInterval, and only on the main thread, which made the real ceiling write-rate x 60s — QA measured a 1.36 GB active log against a 64K cap, with exactly one rotation across 515,951 requests. Request logging is written by the HTTP workers, which had no rotator at all.

Each writing thread's file sink now owns the cap. After a successful append recordWrite subtracts the payload's byte length from a fixed quantum (maxSize/16); when that quantum expires checkAndRotate stats the pathname once and, at or over the cap, renames and closes the descriptor with nothing awaited in between before it can append again. The bound is therefore a function of maxSize and thread count, never of write rate or event-loop delay. The audit tick keeps its interval, retention and reclamation duties as the backstop. The move itself is unchanged — moveLogFile() is now rotateLogFileSync() followed by publishArchivedGeneration(), and both rotation paths go through those, so there is one implementation of "rename, release, then compress or keep" rather than two.

Because every isolate holds its own descriptor on the same file, an archived generation can still be appended to after the rename, and compressing then unlinking it would destroy those records — no before/after size comparison can rule out a write that has not happened yet. A rotation-only generation coordinator makes the release provable instead: the rotating thread announces the archived inode, every peer closes a descriptor matching it and answers, worker exit counts as an answer, and the plain archive is only ever destroyed once every peer has answered. An unproven generation is retained, retried on the next audit tick, and skipped by retention — and because retention deletes archives whichever thread rotated them, each pass first asks every peer, in one round trip, to release any descriptor that is not on the live generation, and deletes nothing if any peer does not answer. The coordinator's transport is injected by the thread layer, so utility/logging/ still imports nothing from server/threads/.

Three smaller defects in the same fault family are fixed alongside, because each of them independently loses the rotation the operator configured:

• One strict maxSize parser, shared with the config validator. parseInt accepted 0K, -1K and 1xK, which become a byte limit of 0, a negative number, and NaN; sampled once a minute those merely misbehave, but on the write path they are evaluated per flush. Every form that produces a usable cap today, exponent notation included, is still accepted.
• A getFileLogger call carrying no rotation block no longer tears down the rotation an earlier, configured caller installed for that path. Several loggers are created for one log file during startup and the later ones carry no rotation of their own.
• A logger inheriting main's rotation for main's own file keeps the configured rotation.path. fix(logging): external/component loggers inherit main rotation config #1880's path strip exists to avoid a cross-device rename when a component logs somewhere else; when the two loggers write the same file there is no such risk, and stripping discarded the operator's configured archive directory.

The pre-push review ran seven rounds and converged at Adjudicated-Severity: nit; every finding above that is fixed here: the file sink now registers with the coordinator instead of the size guard (a thread rotating only on interval, or with an unusable maxSize, builds no guard but still holds a descriptor, and was answering "released" having closed nothing); every archived generation is tracked until proven, not only the compressed ones, because retention unlinks archives too; one compress decision serves both rotation paths, which previously read it from two different places; and the audit tick's size check now stats and renames in the same turn, so a writing thread can no longer rotate the generation the tick measured and leave the tick to archive the near-empty replacement. Retention proves the archive set quiescent before deleting anything, and the ordering matters: the directory is listed first and only that listing is destroyed, because an archive created after the proof was never covered by it. Peers answer with the log paths they are writing, since components load in the workers and a component's own log is registered only there — the thread that runs retention would otherwise see a live file it had never heard of and delete it by age. Each peer judges its own paths, releasing any descriptor that is not on the live generation of the file it is writing, so nothing has to name the live inode of a log it does not own. The tick also finds and compresses plain archives on disk rather than only the ones this isolate remembers. Losing the rename race is no longer treated as a rotation failure — another thread renaming the generation between this one's stat and its rename was closing the descriptor and diverting five seconds of that thread's log lines to raw stdout — and the tick no longer overlaps itself or runs its retry queue ahead of retention.

The one adjudicated major is worth naming: the "log rotated" notice is written back through the sink from inside the append that triggered the rotation, and the flush buffer was still holding the batch that append had just written — it was cleared only on the way out. With logImmediately set, which is any notify() or fatal() and the usual way a batch is flushed under load, the re-entrant flush re-joined the whole buffer and wrote every line of it a second time into the new generation; without it, the notice was swallowed. The buffer is now released before anything can re-enter, and a unit test drives a rotation out of a buffered flush and asserts each line lands exactly once — it reproduces the duplication on the unfixed code.

Two more are worth naming because they are about the directory, not the protocol. logging.rotation.path defaults to log — the same directory logging.root defaults to — so the archive directory normally holds the logs currently being written. The new compression sweep would have gzipped and unlinked the active hdb.log on a default install with compress on, and retention, unchanged from main in this respect, could already delete the active log or a component's once it aged past the window. The sweep now only touches files named the way this module names archives, and retention skips any path reported as a live log. Separately, a descriptor that cannot be proven to be on the live generation is now released rather than kept: on a filesystem reporting ino === 0 the batched release was keeping every descriptor and still answering "released", which is the one answer that lets an archive be destroyed under a peer.

For the human reviewer

Six decisions here are reversible but worth your eye, roughly in the order I'd want them challenged.

Two rotators own one file. The write-path guard rotates on size, and the 60-second audit tick still does too. Every rename race and duplicate-archive concern in this change comes from that. I kept the tick's size check as a backstop because the guard only ever fires on a write — a log already over the cap when an idle instance starts would sit there until something logged — and because a thread whose guard failed to build has nothing else. Giving the guard sole ownership of size and leaving the tick with interval and retention is a one-branch change now and a much harder one once operators depend on tick behavior.

The generation coordinator is the largest new surface, and it only pays off with compress: true, which ships off. It is confined to rotation — ordinary writes exchange no messages — injected rather than imported, and fails safe to "retain the plain archive", which is exactly what the default configuration does anyway. The cheaper alternative is to have each writer close its descriptor on the announcement and accept the existing 10-second descriptor timeout; that was considered and rejected in planning because a blocked writer's timer is not a bound, but it is the thing to unwind first if the protocol proves troublesome. Note it does not use broadcastWithAcknowledgement's own ack — that call consumes acks internally and resolves identically for all-acked and timed-out, so it cannot express "unproven" — and its recipient set deliberately includes job workers, which the generic broadcast-eligibility predicate excludes but which do hold log descriptors.

A failing rotation stops writing to the log file rather than letting it exceed the cap. Appending anyway would make the overshoot rate-dependent again, which is the bug. The worst case for this policy was a rotation directory on another filesystem: the rename fails EXDEV deterministically and forever, so file logging would end permanently while the content went to raw stdout even under stdStreams: false. The guard now refuses to build at all in that configuration, with one startup error and today's unrotated behavior, so what remains is a transient failure diverting to stdio for five seconds at a time.

Tightening maxSize validation can fail a boot that previously succeeded. A config containing 0K, -1K or 1xK is now rejected by configValidator, and config validation throws. Those three are broken today in ways an operator would not have chosen — 0K and -1K make the tick rotate on every pass, 1xK disables size rotation silently — and the write path evaluates them per flush rather than once a minute. The logger itself degrades rather than throwing on any path that bypasses validation. Flagged because it is the one behavior here that can stop an upgrade.

One fix is not unit-covered: a size rotation now resets the interval clock. The interval clock took only tick-driven rotations into account, so an instance whose uptime had passed interval archived a freshly-created log every interval on top of the size rotations already doing the work — a defect that predates this change. The fix is one line; I could not write an assertion for it that was not a rotation-count race against the audit timer, and a flaky test is worse than none.

maxSize/16 is the check quantum. It sets both the overshoot bound — maxBytes + T x (quantum + payload), which the tests encode as under 4x — and the syscall rate at small caps. A constant, trivially changed, but it is the number that will appear in incident reports.

The transport is wired by a require at the bottom of manageThreads.js. That is load-order-sensitive coupling, chosen to keep the logging → threads dependency direction that harper_logger.ts:3 documents. An explicit init call from the server bootstrap would be testable without importing the whole thread manager; I would take that change.

Archive rate is now write-rate / maxSize by definition, and rotation retention is unset by default (static/defaultConfig.yaml). At the deliberately tiny caps QA used that is many archives per second; at the shipped 64M default and QA's measured rate it is about one every three seconds. Disk footprint is no worse than today's single unbounded file, but inode growth is new and is bounded only by an operator's retention setting. No rate limiter is proposed — one would reintroduce exactly the rate-dependent bound this change removes — and a default retention felt like a separate decision.

Archive filenames gain a thread id. #1880 made them <source>-<hash>-<iso>-<pid>-<seq>.log; that is unique per process, and worker threads share the pid while the sequence counter is per-isolate module state, so once any thread can rotate the suffix needs threadId too. Nothing in the tree parses these names, but they are operator-visible.

Two nits I consciously declined, both in the review's decision ledger. There is no floor on the rotation rate: at a deliberately tiny cap each rotation costs a mesh broadcast plus a descriptor close on every peer, and a coalescing window is a reasonable ask — but a rate limiter reintroduces exactly the rate-dependent bound this change removes, so I would rather that be a separate, deliberate decision. And path.resolve normalises relative aliases but not case-insensitive spellings or symlinks; realpathSync per file per tick is not worth it when every path in play comes from one config resolution.

On review coverage. Seven pre-push rounds ran. The Gemini leg is unauthenticated on this worker and never ran; the Codex leg timed out on the full 1,400-line diff three times and completed every delta round after that. Rounds 1 and 2 (Cursor/Grok plus the Harper domain adjudicator) and rounds 4 through 7 (Codex plus the adjudicator) all produced independent findings, and the head carries a current receipt. Round 3 produced nothing.

Declined: a benchmark threshold asserted in CI. The numbers are below; asserting a lines/second floor in a unit test is a flake source on shared runners, so the cost is measured and reported rather than gated.

The framing gate cleared on the third planning round: Framing-Verdict: chosen-approach-sound. The first two returned better-alternative-exists and both sets of corrections were adopted — a fixed quantum instead of a remaining budget (a per-thread remaining budget lets T writers each add ~maxBytes before any of them looks again), a synchronous handoff instead of a promise (an async rotation reintroduces write-rate x event-loop delay), and finally the coordinator itself in place of a before/after size comparison, which cannot rule out a write that has not happened yet.

Verification

Fails on base, passes with the fix. integrationTests/server/log-rotation-write-path.test.ts starts a real Harper with two HTTP workers and a component that emits a unique marker per request, drives 120 requests past a 64K cap, and finishes well inside one audit interval so nothing but the write path can rotate. On origin/main it fails at the first assertion with zero archives; on this branch it passes, every generation is under the bound, and every request marker appears exactly once across the active log and all archives.

The integration test runs with compress: true — the only setting under which an archive is destroyed, and therefore the only one that drives the coordinator's release-then-unlink path through the real thread mesh rather than a fake transport — and asserts that a .gz is actually published.

Unit coverage in unitTests/utility/logging/logRotationGuard.test.js and logGenerationCoordinator.test.js: peak-bounded rotation sampled during the writes; a pre-existing oversized log rotating on the first write; exactly-once across generations; a real worker_threads writer sharing the path; multi-byte payloads counted in bytes; rotation disabled; a broken rotation target failing closed to stdio and then recovering when repaired; the coordinator publishing only after every peer answers, retaining the plain archive when one never does, treating worker exit as an answer, holding an unproven archive back from retention until a later pass proves it, and leaving the plain archive authoritative when compression fails; and the parser's accept/reject set, shared with a new configValidator case.

Gates run: npm run build, npm run test:unit:main (5312 passing, 1 pre-existing environment-dependent failure — configValidator.test.js's "does not warn when a relative rootPath resolves within the limit" resolves a relative root against process.cwd(), which is long enough in a worktree to exceed the socket-path limit; it fails identically on origin/main in this checkout), npm run test:unit:resources, npm run test:integration:all, npm run format:write, npm run lint:required.

Hot-path cost, 200,000 lines of ~150 bytes through the real file sink (not asserted in a test):

rotation	throughput
off	995,516 lines/s
maxSize: 64M (the shipped default)	929,444 lines/s
maxSize: 64K (rotating constantly)	933,712 lines/s

About 6.6% with the guard on, and no measurable additional cost at a cap small enough to rotate hundreds of times during the run — roughly 140 MB/s against the ~21 MB/s QA measured in the failing case.

This is the second of #1877's two fix sites; the first (an external/component logger with no rotation block of its own losing rotation entirely) shipped in #1880. Left as Refs rather than Fixes so closing the issue stays a human call once both halves have been seen together.

Refs #1877

Complexity: complicated

_{Review-Coverage: authored=claude; ran=codex; blocked=gemini(auth); declined=cursor-grok,cursor-composer,domain; rounds=6 @ bbc2023}

_{Human-Review-Need: 4 @ bbc2023}

[github-actions]

Release cherry-pick v5.2: cancelled

Cherry-pick branch cherry-pick/v5.2/pr-2475 was deleted — this PR no longer targets v5.2 (milestone is now v5.3).

[gemini-code-assist]

Code Review

This pull request introduces log rotation on the write path to prevent log overshoots, coordinating descriptor releases across threads via a new log generation coordinator and enforcing limits with a write-path rotation guard. The review identified three key issues: a potential ENOENT error at startup if the parent directory of the log path does not exist, an O(N^2) performance bottleneck when checking for compressed archives that can be optimized using a Set, and a potential TypeError in harper_logger.ts if mainLoggerRef is null or undefined when accessing its path.

[gemini-code-assist]

If the parent directory of logPath does not exist yet at startup (which is common on clean installations or when using custom log paths), statSync(dirname(logPath)) will throw an ENOENT error. This will cause the creation of the rotationGuard to fail and silently disable log rotation for that path entirely.

To prevent this, we should ensure that the parent directory of logPath is created recursively before performing the statSync check, just like we do for rotatedLogDir.

	const logDir = dirname(logPath);
	mkdirSync(rotatedLogDir, { recursive: true });
	mkdirSync(logDir, { recursive: true });
	// Rotation is a rename, and a rename across devices can never succeed. Refusing to build the guard
	// here turns a misconfiguration that would otherwise fail closed on every write — ending file
	// logging for the life of the process — into one startup error and today's unrotated behavior.
	if (statSync(rotatedLogDir).dev !== statSync(logDir).dev) {

[gemini-code-assist]

Performing files.includes inside a loop over files results in an O(N^2) complexity, where N is the number of files in rotatedLogDir. If the directory contains a large number of archived files, this can block the event loop during the audit tick.

We can optimize this to O(N) by converting files to a Set first and performing O(1) lookups.

Suggested change

	let compressed = 0;
	for (const file of files) {
	if (!file.endsWith('.log') || !isArchiveName(file) || files.includes(`${file}.gz`)) continue;
	const fileSet = new Set(files);
	let compressed = 0;
	for (const file of files) {
	if (!file.endsWith('.log') || !isArchiveName(file) || fileSet.has(file + '.gz')) continue;

[gemini-code-assist]

Since mainLoggerRef can potentially be null or undefined (as anticipated by the optional chaining mainLoggerRef?.rotation and mainLoggerRef?.path on line 123), accessing mainLoggerRef.path directly on line 120 will throw a TypeError at runtime.

We should use optional chaining here as well and guard the dirname call to ensure robust defensive programming.

Suggested change

	} else {
	path = mainLoggerRef.path;
	if (!logOptions.root) logOptions.root = pathModule.dirname(path);
	}
	} else {
	path = mainLoggerRef?.path;
	if (path && !logOptions.root) logOptions.root = pathModule.dirname(path);
	}