Repository navigation
BUG: make log rotation safe across Windows processes - #5303
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces cross-platform, cross-process log rotation safety by implementing a shared _SafeFileRotationMixin that uses platform-native file locking (msvcrt on Windows and fcntl on Unix) and tracks rotation state via a shared lock file. It updates the rotating file handlers to inherit from this mixin and adds comprehensive unit tests to verify the Windows fallback and rotation coordination. The feedback highlights a potential file descriptor leak in _acquire_rotation_lock if the locking mechanism raises an exception, suggesting wrapping the lock acquisition in a try...except block to ensure the file descriptor is closed.
|
Friendly ping - this PR makes log rotation safe across Windows processes. Would appreciate a review. Thanks! |
qinxuye
left a comment
There was a problem hiding this comment.
Please also address the unresolved Gemini review comment: _acquire_rotation_lock() must close lock_fd if msvcrt.locking() or fcntl.flock() raises.
a04459a to
ec4dd40
Compare
qinxuye
left a comment
There was a problem hiding this comment.
LGTM. Verified both previous findings are fixed: failed lock acquisition closes the descriptor, and Windows writes share the rotation lock across copy/truncate. Both threads are resolved. All 37 focused log-rotation tests passed locally, including the simulated Windows concurrent-write regression; CI is green.
Summary
msvcrt.lockingon Windows while retainingfcntl.flockon POSIXFixes #5284.
Validation
xinference/deploy/test/test_log_rotation.py: 37 passedupstream/mainThe regression coverage includes a deterministic concurrent-write test that pauses after the Windows fallback copy, verifies a sibling writer blocks, and confirms the record is preserved after truncation. Windows locking behavior is simulated on macOS; the repository Windows CI job provides the native-platform check.