fix(server): stop server crash from log-rotation rename race (#2837)#782
Merged
Conversation
* fix(server): serialize log rotation per file and contain sink write failures Multiple loggers share one on-disk log file; each getLogger allocates its own PinoFileSink and RotatingAppender. Rotation was serialized only per appender instance, so when the file crossed its size cap two writers raced the statSync then rename check-then-act: the first rename won and the second got ENOENT. That rejection reached the PinoFileSink Writable, which has no error listener under pino.multistream, and escalated to an uncaughtException that killed the server (clients saw the connection drop). Serialize rotation per target path via a shared write chain keyed by currentPath, so every appender to a file runs its rotation atomically. Contain sink write failures in PinoFileSink._write so a dropped log record is reported out of band and can never take the process down. Add a benign ENOENT guard on the rotation rename for external file removal. Regression coverage: two tests pinning concurrent multi-appender rotation and sink-failure containment. * docs(server): correct log-sink drain invariant after per-path chain Address cold-review findings on the per-path rotation change: the flushAllFileSinks JSDoc and its regression test described the old per-instance-chain contract (draining one logger does not drain another), which the shared per-path chain inverts. Correct both, and document the same-path-implies-same-previousPath-and-maxBytes invariant on the shared state map. * docs(server): address review nits on log-sink rotation fix - Reword the Invariant A test comment to past tense (it described the pre-fix bug in present tense referencing the renamed writeChain field). - Flush the suppressed dropped-record count from PinoFileSink.drain() so a graceful shutdown mid-throttle-window keeps the diagnostic, with a test. - Clarify the throttle JSDoc: the 5s window is per sink instance, so N sinks on one path can each emit under a shared failure. - Correct the pathWriteState JSDoc: the tolerance path is conditional on tolerance telemetry being enabled. GitOrigin-RevId: 5ef95c57c751c79db2225e5653064760a1583d6a
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
1 participant
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.
Copybara-translated 1 Inkeep OSS change. Rebase-merge this PR so the prepared commit lands directly on public main.