fix(logging): tolerate rename failure during multi-process log rollover - #46089
Closed
JohnC1009 wants to merge 1 commit into
Closed
fix(logging): tolerate rename failure during multi-process log rollover#46089JohnC1009 wants to merge 1 commit into
JohnC1009 wants to merge 1 commit into
Conversation
On Windows, multiple Hermes processes (gateway, dashboard, cron workers, CLI) write the same agent.log. When the file crosses maxBytes, the loser of the rotation race hits os.rename(base -> base.1) while a peer holds the file open -> WinError 32, which logging.handleError dumps as a traceback on every emit until rotation finally succeeds. Wrap _ManagedRotatingFileHandler.doRollover() so an OSError on rename is caught: re-open the stream on baseFilename and carry on. Whichever process wins the rename rotates; the existing emit() inode-watch reopens the rest onto the fresh inode. Worst case the file grows slightly past maxBytes until a rollover succeeds, instead of spamming '--- Logging error ---'.
Contributor
|
Thanks for the focused Windows rollover investigation. This is already covered on current
Closing as implemented on main. |
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.
Problem
On Windows, multiple Hermes processes (gateway service, dashboard service, cron/profile workers, and the CLI) all write to the same log file (e.g.
agent.log). When the file crossesmaxBytes, whichever process emits next callsdoRollover(), which doesos.rename(base -> base.1). On Windows you cannot rename a file that another process holds open, so the losing process raises:logging.Handler.handleErrorthen dumps the full traceback to stderr (--- Logging error ---) on every emit until the file finally gets rotated. This is non-fatal but produces a wall of tracebacks attached to harmless INFO lines (e.g. model-switch / vision auto-detect logging)._ManagedRotatingFileHandleralready hasWatchedFileHandler-style inode-watching inemit()to recover when a peer wins the rotation race — butdoRollover()itself had no guard, so the losers crash instead of recovering.Fix
Wrap
_ManagedRotatingFileHandler.doRollover()so anOSErroron the rename is caught: the process re-opens its stream onbaseFilenameand carries on. Whichever process wins the rename performs the real rotation; the existingemit()inode-watch then reopens every other process onto the fresh inode. Worst case the file grows slightly pastmaxBytesuntil a rollover succeeds — far better than spamming a traceback per log line.In stdlib
doRollover, the stream is closed and set toNonebefore theos.rename, and the final_open()never runs when the rename raises — so theif self.stream is None: reopenrecovery path is correct (baseFilenamestill exists because the rename failed).Verification
RotatingFileHandler.doRollover()raisesPermissionErrorWinError 32 when a peer holds the file open.doRollover()returns cleanly, the stream stays alive, writes keep working, and a later unblocked rollover correctly createsagent.log.1.hermes_logging.pyimports cleanly; lint passes.Scope: single file,
hermes_logging.py(+19/-1).