From d4f5e2fc015542ebe153b06bcadc7caae54fd194 Mon Sep 17 00:00:00 2001 From: ldm0 Date: Sat, 19 Sep 2026 02:07:57 +0800 Subject: [PATCH] fix(profile): simplify file writes for Windows --- .github/workflows/ci.yml | 19 +++++ moli-browser-profile/src/lib.rs | 4 +- .../src/{atomic_write.rs => profile_file.rs} | 75 ++++++++----------- moli-browser-profile/src/profile_manifest.rs | 4 +- moli-cookie-import/src/lib.rs | 2 +- .../context_bootstrap/web_storage/store.rs | 2 +- .../service_worker_runtime/resource_store.rs | 2 +- moli-storage-service/src/buckets.rs | 4 +- 8 files changed, 61 insertions(+), 51 deletions(-) rename moli-browser-profile/src/{atomic_write.rs => profile_file.rs} (62%) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 199b92756..e3b4da514 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -143,6 +143,25 @@ jobs: - name: Run full workspace tests run: cargo nextest run --workspace --profile ci + windows-browser-profile: + name: Windows browser profile + runs-on: windows-2025 + timeout-minutes: 20 + steps: + - name: Checkout repository + uses: actions/checkout@v4 + + - name: Install pinned Rust toolchain + shell: pwsh + run: | + $ErrorActionPreference = "Stop" + $PSNativeCommandUseErrorActionPreference = $true + $toolchain = (Get-Content rust-toolchain -Raw).Trim() + rustup toolchain install $toolchain --profile minimal --no-self-update + + - name: Test browser profile persistence on Windows + run: cargo test --locked -p moli-browser-profile + python-harness-tests: name: Python harness tests runs-on: ubuntu-latest diff --git a/moli-browser-profile/src/lib.rs b/moli-browser-profile/src/lib.rs index 3bccc80e4..22136b3bd 100644 --- a/moli-browser-profile/src/lib.rs +++ b/moli-browser-profile/src/lib.rs @@ -1,6 +1,6 @@ -mod atomic_write; mod identity; mod profile; +mod profile_file; mod profile_lock; mod profile_manifest; mod profile_partition_id; @@ -8,13 +8,13 @@ mod profile_paths; mod user_agent_override; mod window_surface; -pub use atomic_write::write_file_atomically; pub use identity::{ BrowserBrandVersion, BrowserIdentityProfile, BrowserUserAgentMetadataOverride, parse_accept_language, }; pub use moli_cookie_cache::{load_cookie_cache, save_cookie_cache}; pub use profile::{BrowserProfile, BrowserProfilePartition}; +pub use profile_file::write_profile_file; pub use profile_lock::{BrowserProfileLock, acquire_profile_lock}; pub use profile_manifest::{ BrowserProfileManifest, BrowserProfilePartitionManifest, PROFILE_MANIFEST_VERSION, diff --git a/moli-browser-profile/src/atomic_write.rs b/moli-browser-profile/src/profile_file.rs similarity index 62% rename from moli-browser-profile/src/atomic_write.rs rename to moli-browser-profile/src/profile_file.rs index dfe94478b..c37c4cf68 100644 --- a/moli-browser-profile/src/atomic_write.rs +++ b/moli-browser-profile/src/profile_file.rs @@ -1,7 +1,6 @@ use std::{ ffi::OsString, - fs::{self, OpenOptions}, - io::Write, + fs, path::{Path, PathBuf}, sync::atomic::{AtomicU64, Ordering}, time::{SystemTime, UNIX_EPOCH}, @@ -11,9 +10,9 @@ use anyhow::{Context, Result}; static TEMP_COUNTER: AtomicU64 = AtomicU64::new(0); -/// Atomically replaces a profile file and makes both its bytes and directory -/// entry durable before reporting success. -pub fn write_file_atomically(path: &Path, bytes: &[u8], label: &str) -> Result<()> { +/// Writes a profile file via a temporary file without forcing a disk sync. +/// The completed write replaces the target; crash durability is not guaranteed. +pub fn write_profile_file(path: &Path, bytes: &[u8], label: &str) -> Result<()> { if let Some(parent) = path.parent() && !parent.as_os_str().is_empty() { @@ -23,24 +22,15 @@ pub fn write_file_atomically(path: &Path, bytes: &[u8], label: &str) -> Result<( let tmp_path = unique_temp_path(path); let result = (|| { - let mut tmp = OpenOptions::new() - .create_new(true) - .write(true) - .open(&tmp_path) - .with_context(|| format!("failed to create {label} `{}`", tmp_path.display()))?; - tmp.write_all(bytes) + fs::write(&tmp_path, bytes) .with_context(|| format!("failed to write {label} `{}`", tmp_path.display()))?; - tmp.sync_all() - .with_context(|| format!("failed to sync {label} `{}`", tmp_path.display()))?; - drop(tmp); fs::rename(&tmp_path, path).with_context(|| { format!( "failed to replace {label} `{}` from `{}`", path.display(), tmp_path.display() ) - })?; - sync_parent_directory(path, label) + }) })(); if result.is_err() { let _ = fs::remove_file(&tmp_path); @@ -48,21 +38,6 @@ pub fn write_file_atomically(path: &Path, bytes: &[u8], label: &str) -> Result<( result } -fn sync_parent_directory(path: &Path, label: &str) -> Result<()> { - let parent = path - .parent() - .filter(|parent| !parent.as_os_str().is_empty()) - .unwrap_or_else(|| Path::new(".")); - fs::File::open(parent) - .and_then(|directory| directory.sync_all()) - .with_context(|| { - format!( - "failed to sync {label} parent directory `{}`", - parent.display() - ) - }) -} - fn unique_temp_path(path: &Path) -> PathBuf { let nonce = SystemTime::now() .duration_since(UNIX_EPOCH) @@ -89,7 +64,7 @@ mod tests { use anyhow::Result; - use super::write_file_atomically; + use super::write_profile_file; struct TempDir { path: PathBuf, @@ -102,7 +77,7 @@ mod tests { .expect("system clock should be after epoch") .as_nanos(); let path = std::env::temp_dir().join(format!( - "moli-atomic-write-{name}-{}-{nonce}", + "moli-profile-file-{name}-{}-{nonce}", std::process::id() )); Self { path } @@ -116,7 +91,22 @@ mod tests { } #[test] - fn atomic_write_uses_unique_temp_name_instead_of_fixed_tmp_path() -> Result<()> { + fn profile_file_creates_and_overwrites_file_in_new_directory() -> Result<()> { + let temp = TempDir::new("create-replace"); + let parent = temp.path.join("profile with spaces \u{914d}\u{7f6e}"); + let target = parent.join("profile.json"); + + write_profile_file(&target, b"initial profile", "profile test")?; + assert_eq!(fs::read(&target)?, b"initial profile"); + + write_profile_file(&target, b"updated", "profile test")?; + assert_eq!(fs::read(&target)?, b"updated"); + assert_eq!(fs::read_dir(&parent)?.count(), 1); + Ok(()) + } + + #[test] + fn profile_file_uses_unique_temp_name_instead_of_fixed_tmp_path() -> Result<()> { let temp = TempDir::new("unique"); let target = temp.path.join("profile.json"); let mut fixed_tmp = target.as_os_str().to_owned(); @@ -126,7 +116,7 @@ mod tests { fs::write(&target, b"old profile")?; fs::write(&fixed_tmp, b"stale fixed tmp")?; - write_file_atomically(&target, b"new profile", "profile test")?; + write_profile_file(&target, b"new profile", "profile test")?; assert_eq!(fs::read(&target)?, b"new profile"); assert_eq!(fs::read(&fixed_tmp)?, b"stale fixed tmp"); @@ -134,18 +124,19 @@ mod tests { } #[test] - fn atomic_write_removes_unique_temp_after_replace_error() -> Result<()> { + fn profile_file_removes_unique_temp_after_replace_error() -> Result<()> { let temp = TempDir::new("replace-error"); let target = temp.path.join("profile.json"); fs::create_dir_all(&target)?; - assert!(write_file_atomically(&target, b"new profile", "profile test").is_err()); + let error = write_profile_file(&target, b"new profile", "profile test") + .expect_err("replacing a directory should fail"); - let generated_temps = fs::read_dir(&temp.path)? - .filter_map(|entry| entry.ok()) - .filter(|entry| entry.file_type().is_ok_and(|file_type| file_type.is_file())) - .count(); - assert_eq!(generated_temps, 0); + assert!(error.to_string().contains("failed to replace profile test")); + assert!(error.to_string().contains(&target.display().to_string())); + assert!(error.downcast_ref::().is_some()); + assert_eq!(fs::read_dir(&temp.path)?.count(), 1); + assert!(target.is_dir()); Ok(()) } } diff --git a/moli-browser-profile/src/profile_manifest.rs b/moli-browser-profile/src/profile_manifest.rs index 31b7485ad..fdea7fdbf 100644 --- a/moli-browser-profile/src/profile_manifest.rs +++ b/moli-browser-profile/src/profile_manifest.rs @@ -4,7 +4,7 @@ use anyhow::{Context, Result, bail}; use serde::{Deserialize, Serialize}; use serde_json::Value; -use crate::{BrowserProfilePaths, DEFAULT_PROFILE_PARTITION_ID, write_file_atomically}; +use crate::{BrowserProfilePaths, DEFAULT_PROFILE_PARTITION_ID, write_profile_file}; pub const PROFILE_MANIFEST_VERSION: u32 = 5; @@ -166,7 +166,7 @@ fn save_profile_manifest( ) -> Result<()> { let bytes = serde_json::to_vec_pretty(manifest).context("failed to serialize profile manifest")?; - write_file_atomically(&paths.manifest_path, &bytes, "profile manifest") + write_profile_file(&paths.manifest_path, &bytes, "profile manifest") } fn validate_profile_manifest( diff --git a/moli-cookie-import/src/lib.rs b/moli-cookie-import/src/lib.rs index 5f6b769c5..ff1c72b3a 100644 --- a/moli-cookie-import/src/lib.rs +++ b/moli-cookie-import/src/lib.rs @@ -161,7 +161,7 @@ pub fn import_session_state(request: &ImportRequest<'_>) -> Result R let bytes = serde_json::to_vec_pretty(&file).context("failed to serialize localStorage store")?; - moli_browser_profile::write_file_atomically(path, &bytes, "localStorage store") + moli_browser_profile::write_profile_file(path, &bytes, "localStorage store") } #[cfg(test)] diff --git a/moli-renderer-v8/src/service_worker_runtime/resource_store.rs b/moli-renderer-v8/src/service_worker_runtime/resource_store.rs index a4a080d51..84cc0f365 100644 --- a/moli-renderer-v8/src/service_worker_runtime/resource_store.rs +++ b/moli-renderer-v8/src/service_worker_runtime/resource_store.rs @@ -318,7 +318,7 @@ fn persist_json_service_worker_resource_store( }; let bytes = serde_json::to_vec_pretty(&file) .context("failed to serialize Service Worker resource store")?; - moli_browser_profile::write_file_atomically(path, &bytes, "Service Worker resource store") + moli_browser_profile::write_profile_file(path, &bytes, "Service Worker resource store") } #[derive(Debug, Serialize, Deserialize)] diff --git a/moli-storage-service/src/buckets.rs b/moli-storage-service/src/buckets.rs index 6b1019c91..bf1fcc7bc 100644 --- a/moli-storage-service/src/buckets.rs +++ b/moli-storage-service/src/buckets.rs @@ -2145,7 +2145,7 @@ fn save_storage_bucket_cache_file( }; let bytes = serde_json::to_vec_pretty(&json) .context("failed to serialize StorageBucket CacheStorage")?; - moli_browser_profile::write_file_atomically(path, &bytes, "StorageBucket CacheStorage file") + moli_browser_profile::write_profile_file(path, &bytes, "StorageBucket CacheStorage file") } impl JsonStorageBucketBackend { @@ -2285,7 +2285,7 @@ impl JsonStorageBucketBackend { }; let bytes = serde_json::to_vec_pretty(&json).context("failed to serialize storage bucket store")?; - moli_browser_profile::write_file_atomically(&self.path, &bytes, "storage bucket store") + moli_browser_profile::write_profile_file(&self.path, &bytes, "storage bucket store") } fn migrate_legacy_implicit_default_cache_storage(&mut self) -> bool {