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 every file-writing isolate, so a busy HTTP worker no longer grows the active log until the main thread's 60-second audit tick. The sink counts the bytes it actually appends and invokes the write-path rotation guard at a fixed maxSize / 16 checkpoint; an already-oversized file rotates on the next append. The final hot path encodes each batch once, uses that buffer for the append, and records its exact byte length.

Rotation is split into a synchronous rename/descriptor release and an asynchronous publish step. Before compression or retention can unlink an archive, the generation coordinator asks every enumerable in-process peer to close a descriptor still pointing at that inode. An unproven generation remains plain and is retried. Workers that have not received the thread transport cannot claim an empty peer set as proof, pending release requests are bounded, and only the main thread may destructively publish an archive because its worker roster is authoritative.

The follow-up fixes in this pass close the review findings on the existing PR:

• Runtime removal of logging.rotation now retracts the guard owned by that configuration source, while explicit component policies keep precedence over inherited main policies. Re-adding either source reclaims the cached sink safely through the policy-source arbitration.
• Compression catch-up and retention use a deadline rather than a four-file ceiling. One compression failure no longer starves later archives, a busy compression leaves work for the next tick, and the retention cursor resumes fairly when a pass exhausts its budget.
• A failed archive rename no longer diverts all future file logs to daemon stdio. Appends continue to the active file, the cap may temporarily be exceeded, and a parseable in-file diagnostic is prepended on the next successful append.
• Interval rotation notices when another isolate replaced the active generation and restarts its clock, preventing a freshly-created log from being rotated immediately by a stale timer.
• The end-to-end test now asserts every request marker exactly once across active and compressed generations and separately proves that removing the rotation policy reaches both HTTP workers.

For the human reviewer

Please focus on the lifecycle and failure-policy choices below.

• The release proof covers peers enumerable through this process's worker mesh; it does not cover another OS process writing the same path. A worker can still briefly miss a newly admitted peer, but its local proof now only prompts descriptor release and leaves the archive plain; the main-thread audit obtains a fresh proof from its authoritative roster before compression or retention. A separate OS process remains outside that roster, so default compress: false with no retention is non-destructive while configured compression/retention still carries that cross-process limitation.
• A blocked worker deliberately pauses both compression and retention because destroying an inode it may still hold can lose buffered records. The audit emits a warning every five minutes while stalled and a recovery notice afterward. This preserves logs but can let the archive directory grow despite configured retention.
• Compression publishes via temp-file rename but does not fsync the gzip before replacing the authoritative name. A process crash leaves the plain archive intact; an abrupt host/storage failure can theoretically leave a present but non-durable gzip that a later sweep trusts. Crash-left .tmp files can also persist indefinitely when retention is unset.
• Invalid values such as 0K, -1K, and 1xK are now rejected by config validation instead of booting with broken historical behavior. That is intentional strictness, but it can make an upgrade fail fast on an existing typo.
• An explicit component rotation policy for the same file retains the historical last-explicit-owner precedence over inherited main policy. Removing the explicit policy restores main inheritance; removing both clears the guard.
• Retention and its release gate have extensive unit coverage but no full-server deletion assertion. The HTTP integration test covers real two-worker rotation, gzip publication, exactly-once markers, and runtime policy removal. Its fallback archive directory is under the OS temp root; the focused run was same-device, while a runner mounting /tmp separately from Harper's data root would exercise the EXDEV refusal instead.
• Two consecutive Gemini passes disagreed about append encoding: the first called the prior string write plus Buffer.byteLength a major double-scan and requested one buffer; the final pass requested reverting because of buffer allocation. The buffer is retained because appendFileSync must encode the string anyway, while this form reuses that encoding for exact accounting. Claude independently traced this path and found it sound.

The framing gate cleared before implementation: Framing-Verdict: chosen-approach-sound (22fe112820ec). The final exact-head review covered the complete diff with Gemini at 4c0ef25b3bf0; the Claude grader failed at startup and the domain adjudicator timed out, so neither is claimed as final-head coverage. Gemini's repeated buffer-allocation concern was adjudicated against the write path: retaining the buffer avoids separately scanning with Buffer.byteLength and then encoding the same string again inside appendFileSync. Its additional memory-stress-test request and aggregated comment-cleanup nit identified no failing behavior in this CI repair.

Changes

• The analytics task-queue probe in resources/analytics/write.ts now treats a log-path ENOENT race as a completed latency round trip instead of an unhandled rejection when another isolate rotates the file.
• server/threads/logRotationTransport.ts adapts the existing thread mesh to the generation coordinator without reversing the logging-to-thread dependency, and server/threads/manageThreads.js installs that adapter only after its thread-exit hooks exist.
• The audit rotator in utility/logging/logRotator.ts now shares the synchronous generation primitives, gates destructive compression/retention on peer release, bounds sweep work by time, resumes retention fairly, and contains tick failures without overlapping passes.
• validation/configValidator.ts uses the runtime's parseMaxSize implementation, so accepted configuration cannot become a zero, negative, or NaN write-path limit.

Verification

• npm run build at 4c0ef25b3bf0 — passed.
• npx mocha "unitTests/utility/logging/**/*.test.js" at 4c0ef25b3bf0 — 190 passing, 14 pending. The added main-thread publication, zero-inode release, buffered-diagnostic wait, and worker fixture transport cases all passed.
• npm run test:integration -- integrationTests/server/log-rotation-write-path.test.ts at 4c0ef25b3bf0 — 2 passing. The test uses the platform-observable worker count while retaining two-worker Node/Linux coverage.
• The integration fixture's component mapping, private package metadata, and request-driven worker logger drive the real HTTP-worker write path rather than a direct logging helper.
• The earlier same-commit focused Bun run recorded 2 passing. A fresh rerun on this worker could not start because bun is unavailable (spawn bun ENOENT); pushed-head Bun CI is the authoritative rerun.
• unitTests/utility/logging/harper_logger.test.js covers pathless setup and every inherited/explicit rotation-policy ownership transition on isolated paths; unitTests/utility/logging/logRotator.test.js covers compression catch-up, duplicate cleanup, live-log protection, interval resets, failure continuation, and shutdown.
• unitTests/validation/configValidator.test.js covers rejected unusable size values and preserves accepted decimal/exponent forms.
• npm run test:unit:resources — 1,943 passing, 28 pending.
• npm run test:unit:main — 5,319 passing, 196 pending, 12 unrelated/environment failures: the existing Git credentials assertion, ten component-loader cases blocked by this deep worktree resolving dependencies outside its containment rule, and the domain-socket path-length assertion.
• Prettier and oxlint passed on all six files changed by 4c0ef25b3bf0; git diff --check origin/main...HEAD also passed. Full lint still reports 13 pre-existing warnings outside this diff.
• npm run test:unit:windows could not start locally because the gate hard-codes a worktree-local node_modules/mocha/bin/mocha.js; this checkout uses the parent install. The focused cross-platform worker coverage passed locally.
• The full integration sweep encountered unrelated harness/service failures (including Ollama import/service setup and pre-existing fixtures); the focused logging integration passed independently.

Refs #1877

Complexity: complicated

Comment generated by Codex (GPT-5)

_{Review-Coverage: authored=codex; ran=gemini; blocked=claude(exit-1),domain(timeout); declined=cursor-grok,cursor-composer; rounds=8 @ 4c0ef25}

_{Human-Review-Need: 3 @ 4c0ef25}

[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.

[dawsontoth]

also, windows seems ticked off in the integration tests. Dunno if that's caused by this PR, or something broken on main.

[claude]

@/home/runner/work/_temp/review-comment-body.md

[kriszyp]

also, windows seems ticked off in the integration tests. Dunno if that's caused by this PR, or something broken on main.

It was this PR. main was green on Windows at the time (run 34280999766, all six shards), and here all six failed while Node, Bun and uWS passed — the tell that it was platform-specific rather than flaky.

The mechanism, from the shard logs:

ReferenceError: Cannot access 'ROTATION_REPORT_INTERVAL' before initialization
    at reportRotationProblem (utility/logging/harper_logger.ts:863)
    at logToFile.installRotationGuard (utility/logging/harper_logger.ts:856)
    at getFileLogger (utility/logging/harper_logger.ts:812)
    at createLogger (utility/logging/harper_logger.ts:706)
    at initLogSettings (utility/logging/harper_logger.ts:511)
    at Object.<anonymous> (utility/logging/harper_logger.ts:382)   <- module scope

initLogSettings() is called from harper_logger's own module scope, so it reaches getFileLogger while the module is still evaluating — and ROTATION_REPORT_INTERVAL was declared 380 lines further down, still in its temporal dead zone. Any configuration in which createRotationGuard throws turned its one-line failure report into a ReferenceError thrown out of require, which is why suites with nothing to do with logging (static plugin with urlPath, MCP application profile row-level RBAC, REST WebSocket upgrade) went down with it: no Harper could boot.

Windows is where the guard throws, and that part is working as designed. logging.rotation.path resolves against rootPath while the test harness points logging.root at runner.temp — C: and D: on the runners — and rotation is a rename, so the guard refuses to build across devices rather than failing every write forever. On Linux both land on one filesystem, the guard builds, and the report never fires.

Fixed in ac2e26b: the three file-sink constants moved above the module-scope call, with a unit test that loads harper_logger in a child process against a rotation target that cannot be created (ENOTDIR, portable) and asserts the module finishes loading and reports to stderr. It reproduces the ReferenceError on the previous commit. The rotation integration test's archive directory also moved off os.tmpdir() to beside the runner's log directory, so it is no longer cross-volume there either.

Two other review findings landed in the same commit — the missing mkdir of the log directory before the device check, and the quadratic files.includes in the compression sweep. Details and the four claims I declined are in the PR description.

[claude]

Reviewed; no blockers found.

[DavidCockerill]

The steady-state mechanism reads well and the reasoning is written down where it needs to be — the synchronous rotate-then-close with nothing awaited between, the fail-closed acknowledgement timeout, and the exactly-once handling of buffered writes all hold up under tracing. Moving the cap onto the write path is clearly the right call; a minute of overshoot at request-log rates is a lot of bytes.

Five notes inline. The lead one is a cross-process gap in the release proof, and it's the only one I'd want an answer on before merge — not because I think it blocks, but because it turns on a design question only you can settle.

Nothing here is a regression. The pre-change moveLogFile destroyed archives with no proof at all, so every one of these is a residual gap in a new safety mechanism rather than something this PR breaks.

One process note: no linked issue that I can see — closingIssuesReferences is empty and there's no Closes/Refs in the body.

Coverage, so you can weight this: the first engine pass lost three of four legs (both graded attempts and the domain lens timed out), leaving one model unadjudicated — and its headline finding was wrong. I re-ran to recover the other two, and that's what produced the lead finding below. Worth knowing that one of my own findings from the first pass didn't survive the re-run: I'd flagged the worker-side unprovenArchives map as never drained, and compressPendingArchives's docblock already says exactly why that's handled. My mistake, and the docblock is why it was findable.

— DAIvid (Claude Opus 5)

[cb1kenobi]

This PR moves the log size cap onto every writing thread's file sink so a busy HTTP worker can no longer grow the active log for a full minute, and adds a peer-release proof so an archived log is only compressed or deleted once every thread has closed its descriptor on it. I traced the guard closure wiring, the rotation-notice re-entrancy, the write-gate ordering, and the prove-then-destroy set, and found no blocking defect on the changed lines. The remaining gaps the author lists — cross-process release, gzip durability, and stricter maxSize validation — are disclosed design decisions rather than bugs.

—
Reviewed 4c0ef25