From e195c3b3b2cb16a63992a76334ee4abe84bda95d Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 17 Nov 2025 21:17:11 +0000 Subject: [PATCH] Fix critical data loss bug in Windows WAL rotation CRITICAL BUG FIX: The previous Windows implementation had a fatal flaw where it deleted the old WAL file before renaming, creating a window where all data could be lost if the rename failed. Previous (BROKEN) Windows code: remove_file_or_dir(wal->file_path); // Delete old file rename(...); // <-- If this fails, we've lost all data! New implementation uses MoveFileExW with MOVEFILE_REPLACE_EXISTING: - Windows: MoveFileExW(..., MOVEFILE_REPLACE_EXISTING) atomically replaces the destination file, matching Unix semantics - Unix/Linux: rename() continues to atomically replace as before - Both platforms now have atomic file replacement with no data loss window This ensures durability on both platforms - if the operation fails at any point, we still have either the old file or the new file, never losing all data. --- src/wal.c | 46 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 35 insertions(+), 11 deletions(-) diff --git a/src/wal.c b/src/wal.c index 8331a70..dfc98e0 100644 --- a/src/wal.c +++ b/src/wal.c @@ -159,11 +159,42 @@ static int swap_file(WAL *wal) file_unlock(wal->handle); file_close(wal->handle); - // On Unix, rename() atomically replaces the destination file. - // On Windows, rename() doesn't overwrite, so we need to delete first. + // Atomically rename the temporary file to replace the old file + // On Unix: rename() atomically replaces the destination + // On Windows: we use MoveFileEx with MOVEFILE_REPLACE_EXISTING for atomicity #ifdef _WIN32 - // Remove the old file before renaming on Windows - if (remove_file_or_dir(wal->file_path) < 0) { + // On Windows, use MoveFileEx for atomic replace + char old_path_zt[1<<10]; + if (wal->file_path.len >= (int) sizeof(old_path_zt)) { + file_unlock(temp_handle); + file_close(temp_handle); + remove_file_or_dir(temp_path); + return -1; + } + memcpy(old_path_zt, wal->file_path.ptr, wal->file_path.len); + old_path_zt[wal->file_path.len] = '\0'; + + WCHAR old_path_w[MAX_PATH]; + WCHAR temp_path_w[MAX_PATH]; + + if (!MultiByteToWideChar(CP_UTF8, 0, old_path_zt, -1, old_path_w, MAX_PATH) || + !MultiByteToWideChar(CP_UTF8, 0, temp_path_buf, -1, temp_path_w, MAX_PATH)) { + file_unlock(temp_handle); + file_close(temp_handle); + remove_file_or_dir(temp_path); + return -1; + } + + // MOVEFILE_REPLACE_EXISTING allows atomic overwrite + if (!MoveFileExW(temp_path_w, old_path_w, MOVEFILE_REPLACE_EXISTING)) { + file_unlock(temp_handle); + file_close(temp_handle); + remove_file_or_dir(temp_path); + return -1; + } +#else + // On Unix/Linux, rename() atomically replaces the destination + if (rename_file_or_dir(temp_path, wal->file_path) < 0) { file_unlock(temp_handle); file_close(temp_handle); remove_file_or_dir(temp_path); @@ -171,13 +202,6 @@ static int swap_file(WAL *wal) } #endif - // Rename the temporary file to replace the old file - if (rename_file_or_dir(temp_path, wal->file_path) < 0) { - file_unlock(temp_handle); - file_close(temp_handle); - return -1; - } - // Update the WAL to use the new file handle wal->handle = temp_handle; wal->entry_count = 0;