fix(hlog): logger_set_file switches an already-open log file - #892
Merged
Merged
Conversation
logger_set_file only copied the new path. Once the logger had written a line, fp_ stayed open on the old file and logfile_shift reopened only on a day change, so a later hlog_set_file had no effect until midnight. It now closes the current file under the logger mutex; the next write opens the new one. logger_init initializes the mutex before its own logger_set_file call. unittest/hlog_test.c checks the switch after the first write. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
logger_set_file still uses strncpy without guaranteed NUL-termination, which can lead to out-of-bounds reads when parsing the copied path.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
This PR fixes hlog file switching so that logger_set_file() takes effect immediately after logging has begun by closing any already-open log file (under the logger mutex) and forcing the next write to reopen using the updated path. It also adjusts initialization order so the mutex is initialized before logger_set_file() is invoked, and adds a unit test to validate switching after the first log write.
Changes:
- Initialize
logger->mutex_before callinglogger_set_file()duringlogger_init(). - Update
logger_set_file()to close the currentFILE*and reset rotation state under the mutex. - Extend
unittest/hlog_test.cto verify thathlog_set_file()switches the active log file after logging has started.
| File | Description |
|---|---|
| base/hlog.c | Ensures log file switching closes the active file under mutex and reopens on next write; fixes init ordering. |
| unittest/hlog_test.c | Adds a runtime check that the current log file name changes after calling hlog_set_file() post-write. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The logger is malloc'd, so a path of sizeof(filepath) - 1 characters or more left filepath unterminated for the strrchr/strcmp that follow. Reword the switch test's comment: it checks the switch once the log file is open. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Adding a mutex lock inside logger_set_file introduces a clear deadlock hazard with custom handlers invoked under the same mutex, and the API can still expose a stale cur_logfile value immediately after switching.
Review effort: Lite
Findings: 2
Open (2)
Resolved since last review (2)
logger_get_cur_file returned the closed file's name until the next write reopened one; it now reads empty until then. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The changes correctly synchronize logfile switching, reset the relevant state safely, and add a focused unit test that validates the regression scenario.
Review effort: Lite
Findings: None
Resolved since last review (2)
aleksisch
pushed a commit
to GaijinEntertainment/daScript
that referenced
this pull request
Sep 28, 2026
…uire, Windows bash in the manifest test, seqbox window dasHV carries a libhv patch (modules/dasHV/patch_libhv.cmake, the ExternalProject PATCH_COMMAND): logger_set_file closes the open log file under the logger mutex, so hv_set_log_file switches files after libhv has logged - test_log_file failed in every lane that runs the dasHV folder in one process. The same fix is upstream as ithewei/libhv#892; the patch drops when the pin moves past it. The script patches from a saved pristine copy and fails the build, naming the anchor, when one is gone; tests-cpp/big/dashv_patch_libhv (label small) drives it over a fixture. dasllama_fat_start.das: dasllama_env's only use sits in the llvm_tune static_if half, so the LLVM-off lint lane flags the require; nolint:STYLE030 like strings_boost beside it. ci/test_render_manifests.py runs checksum_assets.sh through Git's bash - a bare "bash" from Python on Windows is System32's WSL launcher. test_seqbox_concurrent_writers keeps running past its 250 ms window until a snapshot was published and read (10 s cap) - a starved runner stopped the workers before they ran. tests-cpp/REVIEW.das skips the lane-wiring check for a file whose every test runs ${CMAKE_COMMAND} or ${CMAKE_CTEST_COMMAND} and that declares no target or custom command - the patch test builds nothing; writing_cpp_tests.md names that shape, and tests-cpp/REVIEW.md binds any later change to that check to keep reporting a file that builds something and leaves its lane unwired. make-pr reads at info unless DAS_LOG_LEVEL is set - under the default warning floor its gate ledger printed nothing. The guard is utils/common/log_floor.das, which preflight now calls too. CLAUDE.md's to_log line names the floor. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
pull Bot
pushed a commit
to forksnd/daScript
that referenced
this pull request
Sep 28, 2026
…uire, Windows bash in the manifest test, seqbox window dasHV carries a libhv patch (modules/dasHV/patch_libhv.cmake, the ExternalProject PATCH_COMMAND): logger_set_file closes the open log file under the logger mutex, so hv_set_log_file switches files after libhv has logged - test_log_file failed in every lane that runs the dasHV folder in one process. The same fix is upstream as ithewei/libhv#892; the patch drops when the pin moves past it. The script patches from a saved pristine copy and fails the build, naming the anchor, when one is gone; tests-cpp/big/dashv_patch_libhv (label small) drives it over a fixture. dasllama_fat_start.das: dasllama_env's only use sits in the llvm_tune static_if half, so the LLVM-off lint lane flags the require; nolint:STYLE030 like strings_boost beside it. ci/test_render_manifests.py runs checksum_assets.sh through Git's bash - a bare "bash" from Python on Windows is System32's WSL launcher. test_seqbox_concurrent_writers keeps running past its 250 ms window until a snapshot was published and read (10 s cap) - a starved runner stopped the workers before they ran. tests-cpp/REVIEW.das skips the lane-wiring check for a file whose every test runs ${CMAKE_COMMAND} or ${CMAKE_CTEST_COMMAND} and that declares no target or custom command - the patch test builds nothing; writing_cpp_tests.md names that shape, and tests-cpp/REVIEW.md binds any later change to that check to keep reporting a file that builds something and leaves its lane unwired. make-pr reads at info unless DAS_LOG_LEVEL is set - under the default warning floor its gate ledger printed nothing. The guard is utils/common/log_floor.das, which preflight now calls too. CLAUDE.md's to_log line names the floor. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
logger_set_fileonly copied the new path. Once the logger had written a line,fp_stayed open on the old file andlogfile_shiftreopened only on a day change, so a laterhlog_set_filehad no effect until midnight. It now closes the current file under the logger mutex; the next write opens the new one.logger_initinitializes the mutex before its ownlogger_set_filecall.unittest/hlog_test.cchecks the switch after the first write.