Skip to content

fix(logging,config): logging and config robustness (review batch 4) #420

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Maxaubert merged 1 commit into from
Oct 10, 2026
Merged

fix(logging,config): logging and config robustness (review batch 4) #420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
250 changes: 139 additions & 111 deletions src/config.cpp
View file
Open in desktop

Large diffs are not rendered by default.

31 changes: 20 additions & 11 deletions src/config_path.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,21 +13,31 @@

namespace wind {

// Writability probe: a create-and-delete-on-close sentinel with a PER-PROCESS name. The old shared
// ".windwritetest" opened with share mode 0 failed for the second of two processes starting at the
// same moment, which then fell back to %LOCALAPPDATA% and split the ini and logs (review item 74).
inline bool DirIsWritable(const std::wstring& dir) {
std::wstring sentinel = dir + L"\\.windwritetest." + std::to_wstring(GetCurrentProcessId());
HANDLE h = CreateFileW(sentinel.c_str(), GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL | FILE_FLAG_DELETE_ON_CLOSE, nullptr);
if (h == INVALID_HANDLE_VALUE) return false;
CloseHandle(h);
return true;
}

// Resolved once per process: the exe location and its writability do not change, and every caller
// (tick reload, settings, tray) would otherwise repeat the probe and the seed check.
inline std::wstring ResolveIniPath() {
static const std::wstring cached = [] {
wchar_t exePathBuf[MAX_PATH];
GetModuleFileNameW(nullptr, exePathBuf, MAX_PATH);
wchar_t* slash = wcsrchr(exePathBuf, L'\\');
if (slash) *slash = L'\0';
std::wstring exeDir(exePathBuf);
std::wstring exeIni = exeDir + L"\\magnifier.ini";

// Write a sentinel file that is auto-deleted on close, to probe writability without leaving a
// trace. If this succeeds the exe dir is fine (dev / portable install).
std::wstring sentinel = exeDir + L"\\.windwritetest";
HANDLE h = CreateFileW(sentinel.c_str(), GENERIC_WRITE, 0, nullptr, CREATE_ALWAYS,
FILE_ATTRIBUTE_NORMAL | FILE_FLAG_DELETE_ON_CLOSE, nullptr);
bool exeDirWritable = (h != INVALID_HANDLE_VALUE);
if (exeDirWritable) { CloseHandle(h); return exeIni; }
// If the probe succeeds the exe dir is fine (dev / portable install).
if (DirIsWritable(exeDir)) return exeIni;

// Read-only install (typically C:\Program Files\Wind). Fall back to %LOCALAPPDATA%\Wind.
wchar_t buf[MAX_PATH];
Expand All @@ -44,6 +54,8 @@ inline std::wstring ResolveIniPath() {
CopyFileW(exeIni.c_str(), lapIni.c_str(), FALSE);
}
return lapIni;
}();
return cached;
}

// Directory for logs + crash dumps. Mirrors ResolveIniPath: exe dir if writable (dev/portable),
Expand All @@ -59,11 +71,8 @@ inline std::wstring ResolveLogDir() {
if (slash) *slash = L'\0';
std::wstring exeDir(exePathBuf);

std::wstring sentinel = exeDir + L"\\.windwritetest";
HANDLE h = CreateFileW(sentinel.c_str(), GENERIC_WRITE, 0, nullptr, CREATE_ALWAYS,
FILE_ATTRIBUTE_NORMAL | FILE_FLAG_DELETE_ON_CLOSE, nullptr);
std::wstring base;
if (h != INVALID_HANDLE_VALUE) { CloseHandle(h); base = exeDir; }
if (DirIsWritable(exeDir)) base = exeDir;
else {
wchar_t buf[MAX_PATH];
DWORD n = GetEnvironmentVariableW(L"LOCALAPPDATA", buf, MAX_PATH);
Expand Down
14 changes: 8 additions & 6 deletions src/config_ui/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,8 @@ static std::string DoSwitchProfile(const std::string& name, bool mirrorOutgoing
std::string profText;
if (!wind::ReadTextFileOk(pp, profText)) return "Could not read the profile file";
{ std::string terr = wind::ProfileTextError(profText); if (!terr.empty()) return terr; }
// Held to the end: the whole read-modify-write (and the rollback) is one unit (review item 71).
wind::IniWriteLock iniLock;
std::string oldLive;
if (!wind::ReadLiveIni(IniPath(), oldLive)) return "Could not read the config file";
// Capture hand edits (openIni) into the outgoing profile before the live ini is replaced.
Expand Down Expand Up @@ -397,8 +399,7 @@ static void HandleWebMessage(ICoreWebView2* wv, const std::wstring& jsonW) {
// showed the new value while the ini kept the old one. Tell the page, which says so.
// setConfig writes the live ini (the session) only; the profile file changes on Save.
std::string live;
if (!wind::ReadLiveIni(IniPath(), live) ||
!WriteFileAtomic(IniPath(), wind::UpdateIniText(live, key, value))) {
if (wind::UpdateIniKeys(IniPath(), { { key, value } }, &live) != wind::IniUpdate::Ok) {
wind::Log(wind::LogLevel::Warn, "config", "setConfig: writing %s=%s failed",
key.c_str(), value.c_str());
wv->PostWebMessageAsJson(
Expand All @@ -410,7 +411,7 @@ static void HandleWebMessage(ICoreWebView2* wv, const std::wstring& jsonW) {
// Keybind captures: also written straight into the active profile so they survive
// a later Discard or restart while other changes stay unsaved. Only this one key
// moves; the rest of the profile file stays as saved.
const std::string active = ActiveProfileName(wind::UpdateIniText(live, key, value));
const std::string active = ActiveProfileName(live);
std::string prof;
if (!active.empty() && SafeName(active) &&
GetFileAttributesW(ProfilePath(active).c_str()) != INVALID_FILE_ATTRIBUTES &&
Expand All @@ -431,6 +432,7 @@ static void HandleWebMessage(ICoreWebView2* wv, const std::wstring& jsonW) {
Widen(std::string("{\"type\":\"sessionSaved\",\"ok\":") + (ok ? "true" : "false") + "}").c_str());
} else if (type == "discardSession") {
// Discard: the live ini goes back to the saved profile (globals kept); reply with fresh state.
wind::IniWriteLock iniLock; // item 71
std::string live;
if (ReadIni(live)) {
const std::string active = ActiveProfileName(live);
Expand Down Expand Up @@ -542,9 +544,8 @@ static void HandleWebMessage(ICoreWebView2* wv, const std::wstring& jsonW) {
PostProfiles(wv, err.empty(), err);
} else if (type == "createProfile") {
// Factory defaults by design (spec): absent keys fall back to built-in defaults. `model` is
// seeded EXPLICITLY because the UI schema default (hybrid/"Auto") and the core's missing-key
// default (render) disagree; writing the documented product default keeps core, host, tray,
// and UI in agreement. Globals (onboarded=1, uiPalette, showAdvanced) carry over in MakeLiveText.
// seeded EXPLICITLY so a fresh profile file always names its engine (the
// core's own missing-key default is hybrid, the same as the UI schema's "Auto"). Globals (onboarded=1, uiPalette, showAdvanced) carry over in MakeLiveText.
// fromCurrent ("Duplicate current"): the new profile is the live session as it stands, unsaved
// changes included, built here from the live text so `model` (and everything else) is exactly
// what is running: no restart, no write-back by the page, and the outgoing profile is NOT
Expand Down Expand Up @@ -593,6 +594,7 @@ static void HandleWebMessage(ICoreWebView2* wv, const std::wstring& jsonW) {
if (err.empty() && wind::SameProfileName(vals["profile"], from)) {
// The pointer update must land or the live ini names a file that no longer exists;
// verify the write and roll the rename back if it failed.
wind::IniWriteLock iniLock; // item 71
std::string live;
if (!wind::ReadLiveIni(IniPath(), live) ||
!wind::WriteTextFileAtomic(IniPath(), wind::UpdateIniText(live, "profile", to))) {
Expand Down
38 changes: 30 additions & 8 deletions src/logging.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,12 +135,17 @@ namespace {
}

// wind-<tag>.log -> wind-<tag>.1.log -> wind-<tag>.2.log; oldest dropped.
void RotateIfNeeded(const std::wstring& dir, const std::wstring& stem) {
// Returns false only when the base log is over the cap and could NOT be moved aside (a tailer
// without FILE_SHARE_DELETE holds it): the caller keeps appending and backs off (review item 68).
// The base moves to a side name FIRST, so a refused move leaves every older generation intact.
bool RotateIfNeeded(const std::wstring& dir, const std::wstring& stem) {
std::wstring base = dir + L"\\" + stem + L".log";
WIN32_FILE_ATTRIBUTE_DATA d{};
if (!GetFileAttributesExW(base.c_str(), GetFileExInfoStandard, &d)) return;
if (!GetFileAttributesExW(base.c_str(), GetFileExInfoStandard, &d)) return true;
ULARGE_INTEGER sz; sz.LowPart = d.nFileSizeLow; sz.HighPart = d.nFileSizeHigh;
if (!ShouldRotate(sz.QuadPart, kLogMaxBytes)) return;
if (!ShouldRotate(sz.QuadPart, kLogMaxBytes)) return true;
std::wstring side = dir + L"\\" + stem + L".rotating.log";
if (!MoveFileExW(base.c_str(), side.c_str(), MOVEFILE_REPLACE_EXISTING)) return false;
// Drop the oldest, shift the rest up by one generation.
std::wstring oldest = dir + L"\\" + stem + L"." + std::to_wstring(kLogGenerations - 1) + L".log";
DeleteFileW(oldest.c_str());
Expand All @@ -150,7 +155,9 @@ namespace {
MoveFileExW(from.c_str(), to.c_str(), MOVEFILE_REPLACE_EXISTING);
}
std::wstring to1 = dir + L"\\" + stem + L".1.log";
MoveFileExW(base.c_str(), to1.c_str(), MOVEFILE_REPLACE_EXISTING);
if (!MoveFileExW(side.c_str(), to1.c_str(), MOVEFILE_REPLACE_EXISTING))
DeleteFileW(side.c_str()); // .1 is held open: drop the old data rather than leave it behind
return true;
}
} // namespace

Expand Down Expand Up @@ -212,20 +219,32 @@ static void PruneStrayPidLogs(const std::wstring& dir) {
// by or a LogFlush asked, rotates at the cap. Below normal priority: nothing waits on it except
// LogFlush callers (export, crash, shutdown).
static void WriterAppend(const std::string& batch) {
if (batch.empty() || g_logFile == INVALID_HANDLE_VALUE) return;
if (batch.empty()) return;
if (g_logFile == INVALID_HANDLE_VALUE) { // a failed reopen after rotation: retry, do not stay silent
g_logFile = CreateFileW(g_logPath.c_str(), FILE_APPEND_DATA, FILE_SHARE_READ,
nullptr, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr);
if (g_logFile == INVALID_HANDLE_VALUE) return;
}
DWORD wrote = 0;
WriteFile(g_logFile, batch.data(), (DWORD)batch.size(), &wrote, nullptr);
g_fileBytes += wrote;
}

static void WriterRotateIfNeeded() {
if (!g_ownsBase || !ShouldRotate(g_fileBytes, kLogMaxBytes)) return;
if (!g_ownsBase || g_logFile == INVALID_HANDLE_VALUE || !ShouldRotate(g_fileBytes, kLogMaxBytes)) return;
FlushFileBuffers(g_logFile);
CloseHandle(g_logFile);
RotateIfNeeded(g_logDir, g_logStem);
const bool moved = RotateIfNeeded(g_logDir, g_logStem);
g_logFile = CreateFileW(g_logPath.c_str(), FILE_APPEND_DATA, FILE_SHARE_READ,
nullptr, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr);
g_fileBytes = 0;
if (!moved && g_logFile != INVALID_HANDLE_VALUE) {
// The base could not be moved aside: it is still the live log. Count its real size less one
// cap's worth, so the next attempt comes after another cap of lines, not on every pass.
LARGE_INTEGER sz{};
if (GetFileSizeEx(g_logFile, &sz) && (unsigned long long)sz.QuadPart > kLogMaxBytes)
g_fileBytes = (unsigned long long)sz.QuadPart - kLogMaxBytes;
}
}

// Move a per-PID fallback log onto the shared base log once this process owns the app (review
Expand Down Expand Up @@ -596,7 +615,10 @@ bool ZipLogDir(const wchar_t* destZipPath) {

auto psQuote = [](const std::wstring& s) {
std::wstring out = L"'";
for (wchar_t c : s) { if (c == L'\'') out += L"''"; else out += c; }
// PowerShell reads the typographic quotes U+2018-201B as single quotes too: double them all.
for (wchar_t c : s) {
if (c == L'\'' || (c >= 0x2018 && c <= 0x201B)) { out += c; out += c; } else out += c;
}
out += L"'";
return out;
};
Expand Down
17 changes: 15 additions & 2 deletions src/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3261,7 +3261,20 @@ int WINAPI wWinMain(HINSTANCE hInst, HINSTANCE, PWSTR, int) {
if (!(GetEnvironmentVariableA("WIND_TESTLOG", tlPath, sizeof(tlPath)) > 0 && tlPath[0])) {
char ctl[512] = {};
if (ExpandEnvironmentStringsA("%LOCALAPPDATA%\\Wind\\testlog.txt", ctl, sizeof(ctl)) > 0) {
if (FILE* cf = fopen(ctl, "rb")) {
// Ignore a control file older than a day: a leftover would record telemetry on every launch.
WIN32_FILE_ATTRIBUTE_DATA cfa{};
bool fresh = true;
if (GetFileAttributesExA(ctl, GetFileExInfoStandard, &cfa)) {
FILETIME nowFt; GetSystemTimeAsFileTime(&nowFt);
ULARGE_INTEGER a, b;
a.LowPart = nowFt.dwLowDateTime; a.HighPart = nowFt.dwHighDateTime;
b.LowPart = cfa.ftLastWriteTime.dwLowDateTime; b.HighPart = cfa.ftLastWriteTime.dwHighDateTime;
const long long ageSec = ((long long)a.QuadPart - (long long)b.QuadPart) / 10000000LL;
fresh = wind::TestlogControlFresh(ageSec);
if (!fresh)
wind::Log(wind::LogLevel::Warn, "test", "ignoring stale testlog.txt (%lld s old)", ageSec);
}
if (fresh) if (FILE* cf = fopen(ctl, "rb")) {
size_t n = fread(tlPath, 1, sizeof(tlPath) - 1, cf);
fclose(cf);
while (n > 0 && (tlPath[n - 1] == '\r' || tlPath[n - 1] == '\n' ||
Expand All @@ -3272,7 +3285,7 @@ int WINAPI wWinMain(HINSTANCE hInst, HINSTANCE, PWSTR, int) {
}
}
if (tlPath[0] && g_testlog.open(tlPath))
wind::Log(wind::LogLevel::Info, "test", "telemetry -> %s", tlPath);
wind::Log(wind::LogLevel::Warn, "test", "telemetry -> %s", tlPath);
}

// Auto-detect the display refresh rate so we never assume a fixed rate (the dev's 144Hz).
Expand Down
26 changes: 18 additions & 8 deletions src/profiles.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,10 @@ bool IsGlobalProfileKey(const std::string& key) {
}
std::string ProfileNameError(const std::string& name) {
if (name.empty()) return "Name cannot be empty";
if (name.size() > 40) return "Name is too long (max 40 characters)";
// 40 characters, not bytes: count UTF-8 lead bytes so a non-ASCII name gets the same room as in the UI.
size_t chars = 0;
for (unsigned char c : name) if ((c & 0xC0) != 0x80) ++chars;
if (chars > 40) return "Name is too long (max 40 characters)";
for (unsigned char c : name) {
if (c < 0x20) return "Name contains a control character";
if (std::string("\\/:*?\"<>|").find((char)c) != std::string::npos)
Expand All @@ -33,8 +36,10 @@ std::string ProfileNameError(const std::string& name) {
static const char* reserved[] = {"con","prn","aux","nul",
"com1","com2","com3","com4","com5","com6","com7","com8","com9",
"lpt1","lpt2","lpt3","lpt4","lpt5","lpt6","lpt7","lpt8","lpt9"};
const std::string l = lower(name);
for (const char* r : reserved) if (l == r) return "That name is reserved by Windows";
// Windows reserves the device name with ANY extension too ("con.x", "NUL.txt", "com1 .ini").
std::string stem = lower(name.substr(0, name.find('.')));
while (!stem.empty() && stem.back() == ' ') stem.pop_back();
for (const char* r : reserved) if (stem == r) return "That name is reserved by Windows";
return "";
}
bool ProfileNameTaken(const std::string& name, const std::vector<std::string>& names) {
Expand Down Expand Up @@ -112,14 +117,19 @@ bool SessionDiffers(const std::string& liveText, const std::string& profileText)
for (auto it = b.begin(); it != b.end();) it = IsGlobalProfileKey(it->first) ? b.erase(it) : std::next(it);
// A key one side lacks reads as the built-in default (the first-run template carries each
// templated key's default), so a tray drag that wrote an explicit default back (Warmth 0 -> 40
// -> 0) is not "unsaved". Keys outside the template stay missing-vs-present. "model" is left
// out: the core's missing-key engine and the template's explicit one are different questions
// (see createProfile in config_ui/main.cpp).
static const std::map<std::string, std::string> defaults = ReadIniValues(DefaultIniText());
// -> 0) is not "unsaved". The UI keys the template omits take theirs from Config itself, so
// dragging "Release glide" back to 45 is not "unsaved" either (review item 70).
static const std::map<std::string, std::string> defaults = [] {
std::map<std::string, std::string> d = ReadIniValues(DefaultIniText());
const Config c;
d.emplace("txSamplingMode", std::to_string(c.txSamplingMode));
d.emplace("zoomEaseOutMs", std::to_string(c.zoomEaseOutMs));
d.emplace("lockApps", c.lockApps);
return d;
}();
auto valueOf = [&](const std::map<std::string, std::string>& m, const std::string& k, std::string& out) {
auto it = m.find(k);
if (it != m.end()) { out = it->second; return true; }
if (k == "model") return false;
auto d = defaults.find(k);
if (d == defaults.end()) return false;
out = d->second;
Expand Down
37 changes: 37 additions & 0 deletions src/profiles_io.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <fstream>
#include <sstream>
#include <algorithm>
#include <map>
#include "profiles.h"
#include "config_ui/ini_edit.h"
#include "config.h" // MigrateWheelMods (pure)
Expand Down Expand Up @@ -134,6 +135,42 @@ inline bool WriteTextFileAtomic(const std::wstring& path, const std::string& tex
return false;
}
}
// Cross-process lock for read-modify-write of the live ini (review item 71). Settings, the tray flyout
// and the engine pick all read the ini, change a key and replace the file; two of them interleaving
// lost one write. A named mutex serialises them. A timeout proceeds unlocked (no worse than before);
// an abandoned mutex (a writer died holding it) is simply taken over.
class IniWriteLock {
public:
explicit IniWriteLock(DWORD waitMs = 3000) {
h_ = CreateMutexW(nullptr, FALSE, L"Local\\Wind_IniWrite");
if (h_) {
const DWORD r = WaitForSingleObject(h_, waitMs);
owned_ = (r == WAIT_OBJECT_0 || r == WAIT_ABANDONED);
}
}
~IniWriteLock() {
if (h_) { if (owned_) ReleaseMutex(h_); CloseHandle(h_); }
}
IniWriteLock(const IniWriteLock&) = delete;
IniWriteLock& operator=(const IniWriteLock&) = delete;
private:
HANDLE h_ = nullptr;
bool owned_ = false;
};
enum class IniUpdate { Ok, Unreadable, WriteFailed };
// Locked read-modify-write of the live ini: sets every key in `kv`. Unreadable = the file exists but
// could not be read (nothing written); WriteFailed = the atomic replace failed. If `outText` is given
// it receives the new text on success.
inline IniUpdate UpdateIniKeys(const std::wstring& path, const std::map<std::string, std::string>& kv,
std::string* outText = nullptr) {
IniWriteLock lock;
std::string text;
if (!ReadLiveIni(path, text)) return IniUpdate::Unreadable;
for (const auto& e : kv) text = UpdateIniText(text, e.first, e.second);
if (!WriteTextFileAtomic(path, text)) return IniUpdate::WriteFailed;
if (outText) *outText = text;
return IniUpdate::Ok;
}
// Sweep the temp files a killed process left behind (WriteTextFileAtomic's "<file>.ini.<pid>.tmp"):
// the installer force-kills WindConfig.exe and Wind can crash between the write and the rename.
// Looks in the ini's folder and in profiles\. A live writer's temp exists for milliseconds, so
Expand Down
Loading
Loading