fix: tolerate locked log rollover on Windows#2234
Open
BFlameSwift wants to merge 2 commits into
Open
Conversation
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough新增 SafeTimedRotatingFileHandler（继承 TimedRotatingFileHandler），在 doRollover 中捕获 PermissionError 并重算/推进 rolloverAt；get_logger() 改用该处理器，轮换参数保持不变。 日志轮换错误处理
代码审查工作量估计🎯 2 (Simple) | ⏱️ ~10 分钟 可能相关的问题
诗句
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/one_dragon/utils/log_utils.py`:
- Line 10: The override method doRollover in class defined in
src/one_dragon/utils/log_utils.py is missing a return type annotation; update
its signature from def doRollover(self) to def doRollover(self) -> None so it
complies with the repository rule that all function signatures must have type
annotations (keep the body unchanged and ensure any subclass/base-class
compatibility remains intact).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4349d4a0-5ce4-4301-bd36-08ecd67613f0
📒 Files selected for processing (1)
src/one_dragon/utils/log_utils.py
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.
Summary
PermissionErrorduring log rollover.log/log.txt--- Logging error ---tracebacks that can flood unattended runners and obscure the real automation stateProblem solved
On Windows,
TimedRotatingFileHandlerrotates the log by renaming the active file, for example:If a previous OneDragon process is still alive, or two OneDragon processes write the same log file around midnight, Windows can reject that rename because the source file is still open by another process:
After that failure, the handler still considers the file due for rollover. That means every later log write can try the same failing rename again, producing repeated
Logging errorstack traces instead of normal progress logs.In unattended launchers such as ScriptChainer, this can make the task look stuck or fail early because the child process keeps emitting logging exceptions, the useful state transitions are buried, and normal startup/enter-game diagnostics become unreliable. In the observed failure, the automation started the game but then failed around the enter-world step with
未识别到大世界, while the runner log was dominated by repeated rolloverPermissionErrortraces.Root cause
The default
TimedRotatingFileHandleris not safe for this Windows multi-process/log-file-lock scenario. A stale process holding yesterday'slog.txtcan make a new run repeatedly fail rollover before normal logging can continue cleanly.Fix
This PR wraps
TimedRotatingFileHandler.doRollover()and handlesPermissionErrorby:.log/log.txtsetup, without changing log format or call sitesThis does not try to kill stale processes or solve unrelated game-window/OCR failures. It prevents a locked log file from turning into a repeated logging failure loop that destabilizes or hides the actual automation state.
Verification
py_compileforsrc/one_dragon/utils/log_utils.pygit diff --checkSummary by CodeRabbit