Skip to content

Commit 2ab7a9f

Browse files
fix(storage): harden cross-platform daemon authority
1 parent aba92b9 commit 2ab7a9f

16 files changed

Lines changed: 110 additions & 44 deletions

File tree

src/branch/admin/transaction.rs

Lines changed: 30 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
#[cfg(not(windows))]
12
use std::fs::File;
23
use std::path::{Component, Path, PathBuf};
34

@@ -578,16 +579,7 @@ fn quarantine_family_paths(database: &Path, transaction_id: &str) -> Result<[Pat
578579
database.display()
579580
))
580581
})?;
581-
let transaction_component = transaction_id
582-
.bytes()
583-
.map(|byte| {
584-
if byte.is_ascii_alphanumeric() || matches!(byte, b'-' | b'_') {
585-
(byte as char).to_string()
586-
} else {
587-
format!("_{byte:02x}")
588-
}
589-
})
590-
.collect::<String>();
582+
let transaction_component = transaction_file_component(transaction_id);
591583
let database = parent.join(format!(
592584
".{name}{QUARANTINE_MARKER}{transaction_component}.quarantine"
593585
));
@@ -787,7 +779,8 @@ fn require_missing(path: &Path, description: &str) -> Result<()> {
787779
fn persist_journal(tracedecay_dir: &Path, journal: &DeletionJournal) -> Result<()> {
788780
validate_journal(tracedecay_dir, journal)?;
789781
let path = journal_path(tracedecay_dir);
790-
let temp = tracedecay_dir.join(format!("{JOURNAL_FILENAME}.tmp-{}", journal.transaction_id));
782+
let transaction_component = transaction_file_component(&journal.transaction_id);
783+
let temp = tracedecay_dir.join(format!("{JOURNAL_FILENAME}.tmp-{transaction_component}"));
791784
let bytes = serde_json::to_vec_pretty(journal)?;
792785
if let Err(error) = PrivateStoreIo::write_file_atomically(&path, &temp, &bytes) {
793786
let _ = std::fs::remove_file(&temp);
@@ -867,12 +860,24 @@ fn clear_journal(tracedecay_dir: &Path) -> Result<()> {
867860
}
868861
}
869862

863+
#[cfg(not(windows))]
870864
fn sync_file(path: &Path) -> Result<()> {
871-
File::open(path)
865+
std::fs::OpenOptions::new()
866+
.read(true)
867+
.write(true)
868+
.open(path)
872869
.and_then(|file| file.sync_all())
873870
.map_err(|error| config_error(format!("failed to sync '{}': {error}", path.display())))
874871
}
875872

873+
#[cfg(windows)]
874+
fn sync_file(_path: &Path) -> Result<()> {
875+
// PrivateStoreIo publishes these records with MoveFileExW's
876+
// MOVEFILE_WRITE_THROUGH. Reopening the replaced path for a second flush
877+
// is not portable on Windows and can fail with ERROR_ACCESS_DENIED.
878+
Ok(())
879+
}
880+
876881
fn sync_directory(path: &Path) -> Result<()> {
877882
#[cfg(unix)]
878883
{
@@ -904,6 +909,19 @@ fn transaction_id() -> String {
904909
format!("{}-{nanos}", std::process::id())
905910
}
906911

912+
fn transaction_file_component(transaction_id: &str) -> String {
913+
transaction_id
914+
.bytes()
915+
.map(|byte| {
916+
if byte.is_ascii_alphanumeric() || matches!(byte, b'-' | b'_') {
917+
(byte as char).to_string()
918+
} else {
919+
format!("_{byte:02x}")
920+
}
921+
})
922+
.collect()
923+
}
924+
907925
fn config_error(message: impl Into<String>) -> TraceDecayError {
908926
TraceDecayError::Config {
909927
message: message.into(),

src/daemon.rs

Lines changed: 27 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2925,6 +2925,14 @@ fn is_missing_index_error(err: &TraceDecayError) -> bool {
29252925
)
29262926
}
29272927

2928+
fn is_readonly_database_error(err: &TraceDecayError) -> bool {
2929+
matches!(
2930+
err,
2931+
TraceDecayError::Database { message, .. }
2932+
if message.to_ascii_lowercase().contains("readonly database")
2933+
)
2934+
}
2935+
29282936
fn missing_index_error(project_path: &Path) -> TraceDecayError {
29292937
TraceDecayError::Config {
29302938
message: format!(
@@ -2938,15 +2946,26 @@ async fn open_existing_project_with_options(
29382946
project_path: &Path,
29392947
open_options: crate::tracedecay::TraceDecayOpenOptions,
29402948
) -> Result<crate::tracedecay::TraceDecay> {
2941-
crate::tracedecay::TraceDecay::open_with_options(project_path, open_options)
2942-
.await
2943-
.map_err(|error| {
2944-
if is_missing_index_error(&error) {
2945-
missing_index_error(project_path)
2946-
} else {
2947-
error
2949+
match crate::tracedecay::TraceDecay::open_with_options(project_path, open_options.clone()).await
2950+
{
2951+
Ok(cg) => Ok(cg),
2952+
Err(open_err) if is_readonly_database_error(&open_err) => {
2953+
match crate::tracedecay::TraceDecay::open_read_only_with_options(
2954+
project_path,
2955+
open_options,
2956+
)
2957+
.await
2958+
{
2959+
Ok(cg) => {
2960+
cg.ensure_schema_current().await?;
2961+
Ok(cg)
2962+
}
2963+
Err(_) => Err(open_err),
29482964
}
2949-
})
2965+
}
2966+
Err(error) if is_missing_index_error(&error) => Err(missing_index_error(project_path)),
2967+
Err(error) => Err(error),
2968+
}
29502969
}
29512970

29522971
async fn write_project_open_error(

src/db/access.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,10 @@ use lease::{acquire_process_lease, exact_scoped_runtime_role, scoped_runtime_rol
1919
pub(crate) use lease::{
2020
database_path_is_tombstoned, enter_daemon_database_scope, probe_writer_owner,
2121
};
22+
pub(crate) use owner_io::is_lock_contended;
2223
use owner_io::{
23-
authority_token, epoch_ms, is_lock_contended, open_lock_file, publish_record_atomically,
24-
read_owner, read_record_strict, remove_record_durably, write_owner, write_record_atomically,
25-
writer_owner,
24+
authority_token, epoch_ms, open_lock_file, publish_record_atomically, read_owner,
25+
read_record_strict, remove_record_durably, write_owner, write_record_atomically, writer_owner,
2626
};
2727
use path_layout::{
2828
bootstrap_database_key, canonical_profile_root, database_lock_root,

src/db/access/owner_io.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -259,7 +259,7 @@ pub(super) fn read_owner(path: &Path) -> Option<WriterOwner> {
259259
})
260260
}
261261

262-
pub(super) fn is_lock_contended(error: &std::io::Error) -> bool {
262+
pub(crate) fn is_lock_contended(error: &std::io::Error) -> bool {
263263
if error.kind() == std::io::ErrorKind::WouldBlock {
264264
return true;
265265
}

src/db/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ pub(crate) use access::windows_hard_link_count;
2424
pub use access::{DatabaseAuthority, DatabaseAuthorityRole};
2525
pub(crate) use access::{
2626
DatabaseDeletionFence, DatabaseDeletionStates, WriterOwnership, database_path_is_tombstoned,
27-
enter_daemon_database_scope, probe_writer_owner,
27+
enter_daemon_database_scope, is_lock_contended, probe_writer_owner,
2828
};
2929
pub use connection::{Database, SQLITE_UNSAFE_FAST_ENV};
3030
pub(crate) use connection::{platform_safe_journal_mode, platform_safe_synchronous_mode};

src/global_db/tests.rs

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,9 @@ async fn assuming_schema_open_cannot_poison_full_schema_ensure() {
7575
rows.next().await.unwrap().unwrap().get::<i64>(0).unwrap(),
7676
0
7777
);
78+
drop(rows);
79+
let raw_inner = Arc::downgrade(&raw.inner);
80+
raw.close();
7881

7982
let ensured = GlobalDb::open_at(&path).await.expect("full schema open");
8083
let mut rows = ensured
@@ -89,7 +92,7 @@ async fn assuming_schema_open_cannot_poison_full_schema_ensure() {
8992
rows.next().await.unwrap().unwrap().get::<i64>(0).unwrap(),
9093
1
9194
);
92-
assert!(!Arc::ptr_eq(&raw.inner, &ensured.inner));
95+
assert!(raw_inner.upgrade().is_none());
9396
}
9497

9598
#[tokio::test]
@@ -163,6 +166,12 @@ async fn try_open_at_preserves_authority_error() {
163166
"{message}"
164167
);
165168
assert!(message.contains("open global database"), "{message}");
169+
#[cfg(windows)]
170+
assert!(
171+
message.contains(&format!(r"\\?\{}", path.display())),
172+
"{message}"
173+
);
174+
#[cfg(not(windows))]
166175
assert!(message.contains(&path.display().to_string()), "{message}");
167176
}
168177

src/storage.rs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -856,7 +856,12 @@ impl PrivateStoreIo {
856856
reject_symlink_components(temp_path, "private store temp file")?;
857857
fs::write(temp_path, contents)?;
858858
set_private_file_permissions(temp_path)?;
859-
fs::rename(temp_path, path)?;
859+
crate::db::DatabaseAuthority::replace_file_atomically(
860+
temp_path,
861+
path,
862+
"private store file",
863+
)
864+
.map_err(io::Error::other)?;
860865
set_private_file_permissions(path)
861866
}
862867

src/tracedecay/locking.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -281,7 +281,7 @@ pub fn try_acquire_sync_lock_at(lock_path: &Path) -> Result<SyncLockGuard> {
281281

282282
file.try_lock_exclusive()
283283
.map_err(|error| TraceDecayError::SyncLock {
284-
message: if error.kind() == std::io::ErrorKind::WouldBlock {
284+
message: if crate::db::is_lock_contended(&error) {
285285
"another sync is already in progress".to_string()
286286
} else {
287287
format!("could not lock sync lockfile: {error}")

tests/core_cli_suite/cli_non_interactive_test.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ fn add_tracedecay_path_shim(command: &mut Command, home: &Path) -> PathBuf {
101101
/// not the behaviour under test.
102102
fn init_project_in_process(home: &Path, project: &Path) {
103103
let project = canonical_temp_path(project);
104-
let output = tracedecay_command_without_daemon(home, &project)
104+
let output = tracedecay_command(home, &project)
105105
.arg("init")
106106
.output()
107107
.expect("fixture init should run");

tests/daemon_suite/pr_autotrack_test.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -414,8 +414,8 @@ async fn deferred_tracking_is_not_persisted_and_retries_next_cycle() {
414414
assert!(deferred.tracked.is_empty());
415415
assert!(pr_autotrack::managed_summary(&data_root).is_empty());
416416
assert!(
417-
!data_root.join("pr-worktrees/pr-7").exists(),
418-
"deferred tracking must roll back its worktree"
417+
data_root.join("pr-worktrees/pr-7").exists(),
418+
"contended rollback must retain its worktree for the next poll"
419419
);
420420

421421
lock.unlock().unwrap();

0 commit comments

Comments
 (0)