Skip to content

Make FileLogger thread-safe - #6571

Open
Nils Peper (MSFT) (nilspep) wants to merge 1 commit into
microsoft:masterfrom
nilspep:fix/filelogger-thread-safety
Open

Nils Peper (MSFT) (nilspep) wants to merge 1 commit into
microsoft:masterfrom
nilspep:fix/filelogger-thread-safety

Conversation

@nilspep

@nilspep Nils Peper (MSFT) (nilspep) commented Sep 26, 2026 •

Copy link
Copy Markdown

📖 Description

FileLogger writes to a single std::ofstream without synchronization. When a diagnostic logger is shared across threads, concurrent Write / WriteDirect calls can interleave inside one record, and the maximum-size wrap (HandleMaximumFileSize / WrapLogFile) can race with other writers. This can produce merged or torn log lines.

Upstream code already writes to the same context logger from other threads. For example, winget configure routes configuration processor diagnostics (processor.Diagnostics(...) in ConfigurationFlow.cpp) and WinRT progress callbacks into the context's diagnostic logger.

This change adds a per-instance mutex that serializes:

  • WriteDirect, which Write uses; this covers the size check, wrapping and the stream write;
  • SetTag(HeadersComplete);
  • SetMaximumSize.

The mutex is held through a std::unique_ptr so that FileLogger stays movable. Single-threaded behavior and output format are unchanged.

🔗 References

Resolves #6570

🔍 Validation

  • New test FileLogger_ConcurrentWritesPreserveCompleteLines: 8 threads write 1,000 records each, formatted and direct, with and without a maximum file size. Every line in the file must be exactly one complete record.
    • Without the fix, it failed in 3 of 3 runs on an x64 Release build (torn or merged lines).
    • With the fix, it passed in 3 of 3 runs.
  • The complete AppInstallerCLITests suite passes on x64 Release with the fix (1,123 test cases).

✅ Checklist

🤖 AI Assistance

  • AI assistance was used and has been disclosed in this PR

Fix designed by me; GitHub Copilot assisted with implementation, testing and validation under my direction.

📋 Issue Type

  • Bug fix
  • Feature
  • Task
Microsoft Reviewers: Open in CodeFlow

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@nilspep

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree company="Microsoft"

FileLogger writes to a single std::ofstream without synchronization, so
concurrent writers sharing a diagnostic logger can interleave records and
race with the maximum-size wrap. Serialize WriteDirect, SetTag and
SetMaximumSize with a per-instance mutex, held through a unique_ptr to
keep FileLogger movable.

Add a regression test with 8 concurrent writers that checks every line
is one complete record.
@peperizal1133-afk

This comment was marked as off-topic.

@JohnMcPMS

Copy link
Copy Markdown
Member

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FileLogger is not thread-safe; concurrent writes can produce torn log lines

3 participants