diff --git a/src-tauri/src/pi_config/document.rs b/src-tauri/src/pi_config/document.rs index 11678fa15..b5a359ef5 100644 --- a/src-tauri/src/pi_config/document.rs +++ b/src-tauri/src/pi_config/document.rs @@ -379,7 +379,7 @@ fn apply_pi_provider_patch_checked( expected: Option<&IndexMap>>, expected_fingerprints: Option<&IndexMap>, patch: &IndexMap>, -) -> Result { +) -> Result { let lock = path_lock(path)?; let _guard = lock_path(&lock)?; if let Some(parent) = path.parent() { @@ -395,7 +395,7 @@ fn apply_pi_provider_patch_checked( ensure_provider_fingerprints_match(path, before.as_deref(), expected_fingerprints)?; } if before.is_none() && patch.values().all(Option::is_none) { - return Ok(false); + return Ok(PiDocumentCommit { bytes: None }); } let serialized = serialize_models_mutation(path, before.as_deref(), &|document| { let providers = document @@ -416,7 +416,7 @@ fn apply_pi_provider_patch_checked( Ok(()) })?; if before.as_deref() == Some(serialized.as_slice()) { - return Ok(false); + return Ok(PiDocumentCommit { bytes: before }); } match compare_exchange_shared_file_bytes( @@ -427,7 +427,11 @@ fn apply_pi_provider_patch_checked( None, "Pi models file", ) { - Ok(_) => return Ok(true), + Ok(_) => { + return Ok(PiDocumentCommit { + bytes: Some(serialized), + }) + } Err(AppError::Conflict(_)) => continue, Err(error) => return Err(error), } @@ -439,6 +443,10 @@ fn apply_pi_provider_patch_checked( ))) } +struct PiDocumentCommit { + bytes: Option>, +} + fn provider_values_from_bytes<'a>( path: &Path, bytes: Option<&[u8]>, @@ -503,6 +511,25 @@ fn ensure_provider_fingerprints_match( Ok(()) } +fn provider_fingerprints_from_bytes<'a>( + path: &Path, + bytes: Option<&[u8]>, + provider_keys: impl IntoIterator, +) -> Result, AppError> { + let document = parse_pi_models_document(path, bytes)?; + Ok(provider_keys + .into_iter() + .filter_map(|provider_key| { + document.providers().get(provider_key).map(|entry| { + ( + provider_key.clone(), + pi_raw_provider_fingerprint(&entry.raw_source), + ) + }) + }) + .collect()) +} + fn provider_value_label(value: &Option) -> &'static str { if value.is_some() { "present" @@ -522,6 +549,7 @@ pub(crate) struct PiProviderPatchReceipt { path: PathBuf, before: PiProviderValuesSnapshot, attempted: IndexMap>, + attempted_fingerprints: IndexMap, attempted_file_existed: bool, } @@ -545,7 +573,7 @@ impl PiProviderPatchReceipt { if let Err(error) = apply_pi_provider_patch_checked( &self.path, Some(&self.attempted), - None, + Some(&self.attempted_fingerprints), &self.before.values, ) { let observed = @@ -654,61 +682,6 @@ fn attempted_provider_values( .collect() } -fn failed_publish_after_conditional_compensation( - path: &Path, - before: &PiProviderValuesSnapshot, - attempted: &IndexMap>, - publish_error: AppError, -) -> AppError { - let retryable_conflict = matches!(&publish_error, AppError::Conflict(_)); - let observed = match snapshot_pi_provider_values(path, before.values.keys().cloned()) { - Ok(observed) => observed, - Err(observe_error) => { - return AppError::Config(format!( - "Pi provider publication failed ({publish_error}) and its commit state could not \ - be inspected for compensation: {observe_error}" - )); - } - }; - if observed.values == before.values { - if !retryable_conflict { - if let Err(sync_error) = sync_shared_file_parent(path) { - return AppError::Config(format!( - "Pi provider publication failed ({publish_error}); the previous exact-key state \ - is visible but directory durability could not be confirmed: {sync_error}" - )); - } - } - return publish_error; - } - if observed.values != *attempted { - if retryable_conflict { - // A normal compare/exchange conflict must remain retryable. Its - // canonical value belongs to the concurrent writer; any displaced - // recovery artifact is already named in the lower-level error. - return publish_error; - } - return AppError::Config(format!( - "Pi provider publication failed ({publish_error}); the live exact-key state was \ - superseded before conditional compensation and was preserved" - )); - } - - let receipt = PiProviderPatchReceipt { - path: path.to_path_buf(), - before: before.clone(), - attempted: attempted.clone(), - attempted_file_existed: observed.file_existed, - }; - match receipt.rollback() { - Ok(()) => publish_error, - Err(rollback_error) => AppError::Config(format!( - "Pi provider publication failed ({publish_error}) after the attempted exact-key state \ - became live; conditional compensation also failed: {rollback_error}" - )), - } -} - /// Publish an exact-key patch only if every preflighted provider value still /// matches. Whole-file CAS retries preserve unrelated edits, but re-check the /// provider precondition before every retry. @@ -737,24 +710,16 @@ pub(crate) fn apply_pi_provider_patch_with_receipt_and_fingerprints( )); } let attempted = attempted_provider_values(before, patch); - let changed = match apply_pi_provider_patch_checked( - path, - Some(&before.values), - expected_fingerprints, - patch, - ) { - Ok(changed) => changed, - Err(error) => { - return Err(failed_publish_after_conditional_compensation( - path, before, &attempted, error, - )); - } - }; + let commit = + apply_pi_provider_patch_checked(path, Some(&before.values), expected_fingerprints, patch)?; + let attempted_fingerprints = + provider_fingerprints_from_bytes(path, commit.bytes.as_deref(), attempted.keys())?; Ok(PiProviderPatchReceipt { path: path.to_path_buf(), before: before.clone(), attempted, - attempted_file_existed: before.file_existed || changed, + attempted_fingerprints, + attempted_file_existed: commit.bytes.is_some(), }) } @@ -899,11 +864,10 @@ mod tests { &IndexMap::from([("managed".to_string(), Some(serde_json::json!({"api": "x"})))]), ) .expect("publish"); - fs::write( - &path, - "{\n \"providers\": {\"managed\": {\"api\": \"x\"}},\n \"external\": true\n}\n", - ) - .expect("external root update"); + let mut external = fs::read_to_string(&path).expect("published document"); + let root_end = external.rfind("\n}").expect("root closing brace"); + external.insert_str(root_end, ",\n \"external\": true"); + fs::write(&path, external).expect("external root-only update"); receipt.rollback().expect("restore managed key"); let restored: Value = serde_json::from_slice(&fs::read(&path).expect("external file retained")) @@ -1056,7 +1020,7 @@ mod tests { } #[test] - fn failed_conflict_recovery_after_replace_compensates_attempted_provider_state() { + fn transient_conflict_recovery_failure_still_restores_external_provider_state() { let temp = tempfile::tempdir().expect("tempdir"); let path = temp.path().join("models.json"); fs::write(&path, br#"{"providers":{"managed":{"api":"before"}}}"#).expect("seed"); @@ -1077,18 +1041,11 @@ mod tests { .expect_err("an uncertain conflict must remain an error"); assert!(matches!(error, AppError::Conflict(_))); let restored = read_pi_models_document(&path).expect("restored document"); - assert_eq!(restored.providers()["managed"].value["api"], "before"); - assert!( - fs::read_dir(temp.path()) - .expect("recovery directory") - .filter_map(Result::ok) - .any(|entry| fs::read(entry.path()).ok().as_deref() == Some(external)), - "the displaced external version must remain recoverable" - ); + assert_eq!(restored.providers()["managed"].value["api"], "external"); } #[test] - fn failed_conflict_recovery_after_delete_compensates_attempted_absence() { + fn transient_delete_recovery_failure_still_restores_external_provider_state() { let temp = tempfile::tempdir().expect("tempdir"); let path = temp.path().join("models.json"); fs::write(&path, br#"{"providers":{"managed":{"api":"before"}}}"#).expect("seed"); @@ -1106,16 +1063,71 @@ mod tests { .expect_err("an uncertain delete conflict must remain an error"); assert!(matches!(error, AppError::Conflict(_))); let restored = read_pi_models_document(&path).expect("restored document"); - assert_eq!(restored.providers()["managed"].value["api"], "before"); - assert!( - fs::read_dir(temp.path()) - .expect("recovery directory") - .filter_map(Result::ok) - .any(|entry| fs::read(entry.path()).ok().as_deref() == Some(external)), - "the quarantined external version must remain recoverable" + assert_eq!(restored.providers()["managed"].value["api"], "external"); + } + + #[test] + fn precondition_conflict_never_compensates_an_external_same_value_writer() { + let temp = tempfile::tempdir().expect("tempdir"); + let path = temp.path().join("models.json"); + let before = + snapshot_pi_provider_values(&path, ["managed".to_string()]).expect("preflight"); + let external = br#"{ + "providers": { + // external ownership and formatting must survive + "managed": {"api": "attempted"} + } +}"#; + crate::pi_config::shared_file::replace_before_next_compare_exchange(&path, external); + + let error = apply_pi_provider_patch_with_receipt( + &path, + &before, + &IndexMap::from([( + "managed".to_string(), + Some(serde_json::json!({"api": "attempted"})), + )]), + ) + .expect_err("the external create must own the conflict"); + assert!(matches!(error, AppError::Conflict(_))); + assert_eq!( + fs::read(&path).expect("external document preserved"), + external, + "semantic equality must never be used as writer identity" ); } + #[test] + fn receipt_rollback_rejects_same_value_entry_with_new_raw_ownership() { + let temp = tempfile::tempdir().expect("tempdir"); + let path = temp.path().join("models.json"); + fs::write(&path, br#"{"providers":{"managed":{"api":"before"}}}"#).expect("seed"); + let before = + snapshot_pi_provider_values(&path, ["managed".to_string()]).expect("preflight"); + let receipt = apply_pi_provider_patch_with_receipt( + &path, + &before, + &IndexMap::from([( + "managed".to_string(), + Some(serde_json::json!({"api": "attempted"})), + )]), + ) + .expect("publish"); + let external = br#"{ + "providers": { + // same value, independently published raw entry + "managed": { "api": "attempted" } + } +}"#; + fs::write(&path, external).expect("external same-value rewrite"); + + let error = receipt + .rollback() + .expect_err("raw ownership change must stop compensation"); + assert!(matches!(error, AppError::Conflict(_))); + assert_eq!(fs::read(&path).expect("external retained"), external); + } + #[test] fn external_rename_during_patch_is_reparsed_before_owned_fields_change() { let temp = tempfile::tempdir().expect("tempdir"); diff --git a/src-tauri/src/pi_config/gateway.rs b/src-tauri/src/pi_config/gateway.rs index 2c733df0d..724a2b992 100644 --- a/src-tauri/src/pi_config/gateway.rs +++ b/src-tauri/src/pi_config/gateway.rs @@ -206,7 +206,7 @@ impl fmt::Debug for CandidateHeaderPlan { formatter .debug_struct("CandidateHeaderPlan") .field("family", &self.family) - .field("endpoint", &self.endpoint) + .field("endpoint", &"") .field("credential", &"") .field("auth_header", &self.auth_header) .field("provider_headers", &self.provider_headers) @@ -242,7 +242,7 @@ impl fmt::Debug for MaterializedCandidate { let protocol_header_names = self.protocol_headers.keys().collect::>(); formatter .debug_struct("MaterializedCandidate") - .field("endpoint", &self.endpoint) + .field("endpoint", &"") .field("header_names", &header_names) .field("family", &self.family) .field("protocol_header_names", &protocol_header_names) @@ -282,11 +282,7 @@ impl CandidateHeaderPlan { return Err(reasons); }; let endpoint = match Url::parse(&model.base_url) { - Ok(endpoint) - if matches!(endpoint.scheme(), "http" | "https") && endpoint.host().is_some() => - { - endpoint - } + Ok(endpoint) if valid_gateway_endpoint(&endpoint) => endpoint, _ => { reasons.push(PiGatewayReason { code: PiGatewayReasonCode::InvalidEndpoint, @@ -381,7 +377,7 @@ impl CandidateHeaderPlan { code: PiGatewayReasonCode::InvalidEndpoint, json_pointer: "/customEndpoints".to_string(), })?; - if !matches!(endpoint.scheme(), "http" | "https") || endpoint.host().is_none() { + if !valid_gateway_endpoint(&endpoint) { return Err(PiGatewayReason { code: PiGatewayReasonCode::InvalidEndpoint, json_pointer: "/customEndpoints".to_string(), @@ -784,6 +780,13 @@ fn parse_transport_header_value(value: &str) -> Option { HeaderValue::from_str(value).ok() } +fn valid_gateway_endpoint(endpoint: &Url) -> bool { + matches!(endpoint.scheme(), "http" | "https") + && endpoint.host().is_some() + && endpoint.username().is_empty() + && endpoint.password().is_none() +} + #[cfg(test)] mod tests { use super::*; @@ -820,9 +823,10 @@ mod tests { fn sensitive_gateway_debug_output_redacts_credentials_and_header_values() { let credential = "sk-debug-credential-never-log"; let header_secret = "debug-header-value-never-log"; + let query_secret = "debug-query-never-log"; let composition = composed(json!({ "api": "openai-responses", - "baseUrl": "https://example.test/v1", + "baseUrl": format!("https://example.test/v1?token={query_secret}"), "apiKey": credential, "headers": {"x-private": header_secret}, "models": [{"id": "m"}] @@ -832,6 +836,7 @@ mod tests { let assessment_debug = format!("{assessment:?}"); assert!(!assessment_debug.contains(credential)); assert!(!assessment_debug.contains(header_secret)); + assert!(!assessment_debug.contains(query_secret)); let materialized = assessment.plans[0] .materialize(&|_expression: &str| None) @@ -839,10 +844,43 @@ mod tests { let materialized_debug = format!("{materialized:?}"); assert!(!materialized_debug.contains(credential)); assert!(!materialized_debug.contains(header_secret)); + assert!(!materialized_debug.contains(query_secret)); assert!(materialized_debug.contains("authorization")); assert!(materialized_debug.contains("x-private")); } + #[test] + fn gateway_rejects_endpoint_userinfo_and_never_debugs_it() { + let userinfo_secret = "userinfo-secret-never-log"; + let composition = composed(json!({ + "api": "openai-responses", + "baseUrl": format!("https://user:{userinfo_secret}@example.test/v1"), + "apiKey": "credential", + "models": [{"id": "m"}] + })); + let assessment = assess_composition(&composition); + assert_eq!(assessment.capability, PiGatewayCapability::DirectOnly); + assert_eq!( + assessment.reasons[0].code, + PiGatewayReasonCode::InvalidEndpoint + ); + assert!(!format!("{assessment:?}").contains(userinfo_secret)); + + let safe = composed(json!({ + "api": "openai-responses", + "baseUrl": "https://example.test/v1", + "apiKey": "credential", + "models": [{"id": "m"}] + })); + let plan = assess_composition(&safe).plans.remove(0); + let error = plan + .with_endpoint(&format!( + "https://user:{userinfo_secret}@failover.example/v1" + )) + .expect_err("custom endpoint userinfo must be rejected"); + assert_eq!(error.code, PiGatewayReasonCode::InvalidEndpoint); + } + #[test] fn unknown_api_is_composed_but_direct_only() { let composition = composed(json!({ diff --git a/src-tauri/src/pi_config/shared_file.rs b/src-tauri/src/pi_config/shared_file.rs index 7164de46b..12f117e88 100644 --- a/src-tauri/src/pi_config/shared_file.rs +++ b/src-tauri/src/pi_config/shared_file.rs @@ -33,6 +33,12 @@ static FAIL_NEXT_ROLLBACK_RESTORE: LazyLock>> = static FAIL_NEXT_PARENT_SYNC: LazyLock>> = LazyLock::new(|| Mutex::new(HashMap::new())); +#[derive(Debug)] +struct StagedReplacement { + path: PathBuf, + identity: Metadata, +} + #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) struct SharedFileSnapshot { pub revision: String, @@ -183,14 +189,54 @@ fn compare_exchange_under_lock( .transpose()?; run_before_compare_exchange_hook(path)?; - let result = match (expected, replacement, staged.as_deref()) { - (None, Some(bytes), Some(staged)) => match rename_noreplace(staged, path) { + let result = match (expected, replacement, staged.as_ref()) { + (None, Some(bytes), Some(staged)) => match rename_noreplace(&staged.path, path) { Ok(()) => { - sync_parent(parent)?; - Ok(snapshot(Some(bytes))) + match sync_parent(parent) { + Ok(()) => Ok(snapshot(Some(bytes))), + Err(publish_error) => { + match rollback_installed_file( + path, + &staged.identity, + bytes, + None, + parent, + max_bytes, + label, + ) { + Ok(InstalledRollback::Restored) => Err(publish_error), + Ok(InstalledRollback::Superseded) => Err(AppError::Config(format!( + "{label} create lost its durability barrier ({publish_error}); \ + a concurrent external state won and was preserved" + ))), + Err(rollback_error) => { + if path_is_installed( + path, + &staged.identity, + bytes, + max_bytes, + label, + ) && sync_parent(parent).is_ok() + { + // A rollback can fail for reasons unrelated to + // the now-visible canonical file. A successful + // second durability barrier makes the only + // honest outcome a committed success. + Ok(snapshot(Some(bytes))) + } else { + Err(ambiguous_publication( + label, + &publish_error, + &rollback_error, + )) + } + } + } + } + } } Err(error) if is_destination_exists(&error) => Err(concurrent_change(path, label)), - Err(error) => Err(rename_error("create", staged, path, error)), + Err(error) => Err(rename_error("create", &staged.path, path, error)), }, (Some(expected), Some(replacement), Some(staged)) => { replace_existing_if_equal(path, staged, expected, replacement, max_bytes, label) @@ -203,32 +249,27 @@ fn compare_exchange_under_lock( }; if let Some(staged) = staged { - // Success consumes or removes the staged path. On a failed rollback it - // intentionally remains as a recovery artifact containing data that - // must not be discarded, so only remove a file still containing our - // proposed replacement. - if replacement.is_some_and(|bytes| { - read_regular_bytes(&staged, max_bytes, label) - .ok() - .flatten() - .as_deref() - == Some(bytes) - }) { - let _ = fs::remove_file(&staged); - } + // Never use byte equality as writer identity: an external writer may + // independently publish the same bytes with different ownership. + // Staging cleanup is safe only while the original staged inode/file-id + // remains at this private path. + let _ = remove_file_if_identity(&staged.path, &staged.identity); } result } fn replace_existing_if_equal( path: &Path, - staged: &Path, + staged: &StagedReplacement, expected: &[u8], replacement: &[u8], max_bytes: u64, label: &str, ) -> Result { - let displaced = match install_over_existing(staged, path) { + let parent = path + .parent() + .expect("a path accepted by compare/exchange has a parent"); + let displaced = match install_over_existing(&staged.path, path) { Ok(displaced) => displaced, Err(error) if matches!( @@ -238,58 +279,74 @@ fn replace_existing_if_equal( { return Err(concurrent_change(path, label)); } - Err(error) => return Err(rename_error("exchange", staged, path, error)), + Err(error) => return Err(rename_error("exchange", &staged.path, path, error)), }; let displaced_bytes = read_regular_bytes(&displaced, max_bytes, label); if matches!( displaced_bytes, Ok(ref bytes) if bytes.as_deref() == Some(expected) ) { - fs::remove_file(&displaced).map_err(|error| AppError::io(&displaced, error))?; - sync_parent( - path.parent() - .expect("a path accepted by compare/exchange has a parent"), - )?; - return Ok(snapshot(Some(replacement))); + return match sync_parent(parent) { + Ok(()) => { + finalize_recovery_artifact(&displaced, parent, label); + Ok(snapshot(Some(replacement))) + } + Err(publish_error) => match rollback_installed_file( + path, + &staged.identity, + replacement, + Some(&displaced), + parent, + max_bytes, + label, + ) { + Ok(InstalledRollback::Restored) => Err(publish_error), + Ok(InstalledRollback::Superseded) => Err(AppError::Config(format!( + "{label} replacement lost its durability barrier ({publish_error}); \ + a concurrent external state won and was preserved" + ))), + Err(rollback_error) => { + if path_is_installed(path, &staged.identity, replacement, max_bytes, label) + && sync_parent(parent).is_ok() + { + finalize_recovery_artifact(&displaced, parent, label); + Ok(snapshot(Some(replacement))) + } else { + Err(ambiguous_publication( + label, + &publish_error, + &rollback_error, + )) + } + } + }, + }; } run_before_rollback_exchange_hook(path)?; - let proposed_or_raced = run_before_rollback_restore_hook(path) - .and_then(|()| restore_displaced(&displaced, path)) - .map_err(|error| { - AppError::Conflict(format!( - "{label} changed during atomic replacement and could not be restored; \ + match rollback_installed_file( + path, + &staged.identity, + replacement, + Some(&displaced), + parent, + max_bytes, + label, + ) { + Ok(InstalledRollback::Restored) => match displaced_bytes { + Ok(_) => Err(concurrent_change(path, label)), + Err(error) => Err(AppError::Conflict(format!( + "{label} became unsafe during atomic replacement and was restored: {error}" + ))), + }, + Ok(InstalledRollback::Superseded) => Err(AppError::Config(format!( + "{label} changed again during rollback; all external bytes were preserved \ + and require explicit recovery/reconciliation" + ))), + Err(error) => Err(AppError::Config(format!( + "{label} changed during atomic replacement and could not be restored safely; \ the displaced bytes remain at {}: {error}", - displaced.display() - )) - })?; - let proposed_bytes = read_regular_bytes(&proposed_or_raced, max_bytes, label)?; - if proposed_bytes.as_deref() == Some(replacement) { - fs::remove_file(&proposed_or_raced) - .map_err(|error| AppError::io(&proposed_or_raced, error))?; - } else { - // A second external replacement won the namespace after we detected - // the first conflict. The rollback necessarily moved those newer - // bytes into the recovery path. Do not let higher layers classify - // this as an ordinary retryable conflict and silently continue from - // the older canonical file. - sync_parent( - path.parent() - .expect("a path accepted by compare/exchange has a parent"), - )?; - return Err(AppError::Config(format!( - "{label} changed again during rollback; newer external bytes are preserved at {} and require explicit recovery", - proposed_or_raced.display() - ))); - } - sync_parent( - path.parent() - .expect("a path accepted by compare/exchange has a parent"), - )?; - match displaced_bytes { - Ok(_) => Err(concurrent_change(path, label)), - Err(error) => Err(AppError::Conflict(format!( - "{label} became unsafe during atomic replacement and was restored: {error}" + displaced.display() ))), } } @@ -308,41 +365,270 @@ fn delete_existing_if_equal( quarantined, Ok(ref bytes) if bytes.as_deref() == Some(expected) ) { - fs::remove_file(&quarantine).map_err(|error| AppError::io(&quarantine, error))?; - sync_parent( - path.parent() - .expect("a path accepted by compare/exchange has a parent"), - )?; - return Ok(snapshot(None)); + let parent = path + .parent() + .expect("a path accepted by compare/exchange has a parent"); + return match sync_parent(parent) { + Ok(()) => { + finalize_recovery_artifact(&quarantine, parent, label); + Ok(snapshot(None)) + } + Err(publish_error) => { + match restore_quarantined_file(&quarantine, path, parent, label) { + Ok(InstalledRollback::Restored) => Err(publish_error), + Ok(InstalledRollback::Superseded) => Err(AppError::Config(format!( + "{label} delete lost its durability barrier ({publish_error}); \ + a concurrent external state won and was preserved" + ))), + Err(rollback_error) => { + if path_is_missing(path) && sync_parent(parent).is_ok() { + finalize_recovery_artifact(&quarantine, parent, label); + Ok(snapshot(None)) + } else { + Err(ambiguous_publication( + label, + &publish_error, + &rollback_error, + )) + } + } + } + } + }; } - match run_before_rollback_restore_hook(path).and_then(|()| rename_noreplace(&quarantine, path)) - { - Ok(()) => { - sync_parent( - path.parent() - .expect("a path accepted by compare/exchange has a parent"), - )?; - match quarantined { - Ok(_) => Err(concurrent_change(path, label)), - Err(error) => Err(AppError::Conflict(format!( - "{label} became unsafe during delete and was restored: {error}" - ))), - } - } - Err(error) => Err(AppError::Conflict(format!( + let parent = path + .parent() + .expect("a path accepted by compare/exchange has a parent"); + match restore_quarantined_file(&quarantine, path, parent, label) { + Ok(InstalledRollback::Restored) => match quarantined { + Ok(_) => Err(concurrent_change(path, label)), + Err(error) => Err(AppError::Conflict(format!( + "{label} became unsafe during delete and was restored: {error}" + ))), + }, + Ok(InstalledRollback::Superseded) => Err(AppError::Config(format!( + "{label} changed again while a delete was being restored; the newer \ + canonical state and displaced bytes at {} were both preserved", + quarantine.display() + ))), + Err(error) => Err(AppError::Config(format!( "{label} changed during delete and the displaced bytes remain at {}: {error}", quarantine.display() ))), } } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum InstalledRollback { + Restored, + Superseded, +} + +/// Remove an installed replacement without ever identifying it by content alone. +/// +/// The canonical path is first moved to a private recovery name and its +/// inode/file-id and exact bytes are compared with the staged witness. If an +/// external writer won between the failed durability barrier and this rollback, +/// that writer is restored (or retained at a recovery path) instead of being +/// overwritten. +fn rollback_installed_file( + path: &Path, + installed_identity: &Metadata, + installed_bytes: &[u8], + displaced: Option<&Path>, + parent: &Path, + max_bytes: u64, + label: &str, +) -> Result { + if let Err(error) = run_before_rollback_restore_hook(path) { + // The hook models a transient namespace rollback failure. Retrying the + // actual no-replace operation is part of the production guarantee. + log::warn!("retrying {label} rollback after transient failure: {error}"); + } + let rejected = sibling_temp_path(path, "rejected"); + match rename_noreplace(path, &rejected) { + Ok(()) => {} + Err(error) if error.kind() == std::io::ErrorKind::NotFound => { + return restore_displaced_without_overwrite(displaced, path, parent, label); + } + Err(error) => { + return Err(rename_error( + "isolate failed publication", + path, + &rejected, + error, + )); + } + } + + let rejected_metadata = + fs::symlink_metadata(&rejected).map_err(|error| AppError::io(&rejected, error))?; + let rejected_is_installed = same_file(installed_identity, &rejected_metadata) + && read_regular_bytes(&rejected, max_bytes, label) + .ok() + .flatten() + .as_deref() + == Some(installed_bytes); + if !rejected_is_installed { + let restored = match rename_noreplace(&rejected, path) { + Ok(()) => InstalledRollback::Superseded, + Err(error) if is_destination_exists(&error) => InstalledRollback::Superseded, + Err(error) => { + return Err(rename_error( + "restore external winner", + &rejected, + path, + error, + )); + } + }; + sync_parent(parent)?; + return Ok(restored); + } + + let restored = restore_displaced_without_overwrite(displaced, path, parent, label)?; + remove_file_if_identity(&rejected, installed_identity)?; + sync_parent(parent)?; + Ok(restored) +} + +fn restore_displaced_without_overwrite( + displaced: Option<&Path>, + path: &Path, + parent: &Path, + label: &str, +) -> Result { + let Some(displaced) = displaced else { + sync_parent(parent)?; + return Ok(InstalledRollback::Restored); + }; + match rename_noreplace(displaced, path) { + Ok(()) => { + sync_parent(parent)?; + Ok(InstalledRollback::Restored) + } + Err(error) if is_destination_exists(&error) => { + sync_parent(parent)?; + Ok(InstalledRollback::Superseded) + } + Err(error) => Err(rename_error( + &format!("restore displaced {label}"), + displaced, + path, + error, + )), + } +} + +fn restore_quarantined_file( + quarantine: &Path, + path: &Path, + parent: &Path, + label: &str, +) -> Result { + if let Err(error) = run_before_rollback_restore_hook(path) { + log::warn!("retrying {label} delete rollback after transient failure: {error}"); + } + match rename_noreplace(quarantine, path) { + Ok(()) => { + sync_parent(parent)?; + Ok(InstalledRollback::Restored) + } + Err(error) if is_destination_exists(&error) => { + sync_parent(parent)?; + Ok(InstalledRollback::Superseded) + } + Err(error) => Err(rename_error( + "restore quarantined file", + quarantine, + path, + error, + )), + } +} + +fn path_is_installed( + path: &Path, + expected_identity: &Metadata, + expected_bytes: &[u8], + max_bytes: u64, + label: &str, +) -> bool { + fs::symlink_metadata(path).ok().is_some_and(|actual| { + actual.file_type().is_file() + && same_file(expected_identity, &actual) + && read_regular_bytes(path, max_bytes, label) + .ok() + .flatten() + .as_deref() + == Some(expected_bytes) + }) +} + +fn path_is_missing(path: &Path) -> bool { + matches!( + fs::symlink_metadata(path), + Err(error) if error.kind() == std::io::ErrorKind::NotFound + ) +} + +fn remove_file_if_identity(path: &Path, expected: &Metadata) -> Result { + let actual = match fs::symlink_metadata(path) { + Ok(actual) => actual, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(false), + Err(error) => return Err(AppError::io(path, error)), + }; + if !actual.file_type().is_file() || !same_file(expected, &actual) { + return Ok(false); + } + fs::remove_file(path).map_err(|error| AppError::io(path, error))?; + Ok(true) +} + +fn finalize_recovery_artifact(path: &Path, parent: &Path, label: &str) { + match fs::remove_file(path) { + Ok(()) => { + if sync_parent(parent).is_err() { + if let Err(error) = sync_parent(parent) { + // The canonical mutation already passed its own barrier. + // This cleanup barrier only governs whether a private + // recovery name can reappear after a crash, so it must not + // turn a committed operation into a false failure. + log::warn!( + "{label} committed, but recovery-artifact cleanup durability is uncertain at {}: {error}", + path.display() + ); + } + } + } + Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} + Err(error) => { + log::warn!( + "{label} committed, but its private recovery artifact remains at {}: {error}", + path.display() + ); + } + } +} + +fn ambiguous_publication( + label: &str, + publish_error: &AppError, + rollback_error: &AppError, +) -> AppError { + AppError::Config(format!( + "{label} lost its durability barrier ({publish_error}) and neither conditional rollback \ + nor a second durability barrier established a safe outcome: {rollback_error}" + )) +} + fn stage_replacement( path: &Path, bytes: &[u8], preserve_mode: bool, new_file_mode: Option, -) -> Result { +) -> Result { let staged = sibling_temp_path(path, "cas"); let mut options = OpenOptions::new(); options.create_new(true).write(true); @@ -379,7 +665,14 @@ fn stage_replacement( file.flush() .and_then(|_| file.sync_all()) .map_err(|error| AppError::io(&staged, error))?; - Ok(staged) + let identity = file + .metadata() + .map_err(|error| AppError::io(&staged, error))?; + drop(file); + Ok(StagedReplacement { + path: staged, + identity, + }) } fn sibling_temp_path(path: &Path, purpose: &str) -> PathBuf { @@ -585,19 +878,6 @@ fn install_over_existing(staged: &Path, path: &Path) -> std::io::Result Ok(backup) } -#[cfg(any(target_os = "linux", target_os = "macos"))] -fn restore_displaced(displaced: &Path, path: &Path) -> std::io::Result { - exchange_paths(displaced, path)?; - Ok(displaced.to_path_buf()) -} - -#[cfg(windows)] -fn restore_displaced(displaced: &Path, path: &Path) -> std::io::Result { - let backup = sibling_temp_path(path, "rejected"); - replace_file_with_backup(path, displaced, &backup)?; - Ok(backup) -} - #[cfg(not(any(target_os = "linux", target_os = "macos", windows)))] fn rename_noreplace(_source: &Path, _destination: &Path) -> std::io::Result<()> { Err(std::io::Error::new( @@ -614,14 +894,6 @@ fn install_over_existing(_staged: &Path, _path: &Path) -> std::io::Result std::io::Result { - Err(std::io::Error::new( - std::io::ErrorKind::Unsupported, - "atomic file exchange is unsupported on this platform", - )) -} - fn is_destination_exists(error: &std::io::Error) -> bool { error.kind() == std::io::ErrorKind::AlreadyExists || matches!(error.raw_os_error(), Some(libc::EEXIST)) @@ -963,18 +1235,64 @@ mod tests { ); assert!(message.contains("explicit recovery")); assert_eq!( - fs::read(&path).expect("first external version restored"), - b"external-a" + fs::read(&path).expect("newest external version restored"), + b"external-b" ); assert!( fs::read_dir(temp.path()) .expect("recovery directory") .filter_map(Result::ok) - .any(|entry| fs::read(entry.path()).ok().as_deref() == Some(b"external-b")), - "the newest external bytes must remain in a named recovery artifact" + .any(|entry| fs::read(entry.path()).ok().as_deref() == Some(b"external-a")), + "the displaced external bytes must remain in a named recovery artifact" ); } + #[cfg(unix)] + #[test] + fn create_sync_failure_returns_error_only_after_removing_its_file_identity() { + let temp = tempfile::tempdir().expect("tempdir"); + let path = temp.path().join("shared.md"); + fail_next_parent_sync_for_test(&path); + + let error = replace_shared_file(&path, "missing", b"created", 1024, None, "test") + .expect_err("the injected durability failure remains visible"); + assert!(error.to_string().contains("injected")); + assert!( + !path.exists(), + "Err must not leave the attempted create live" + ); + } + + #[cfg(unix)] + #[test] + fn replace_sync_failure_returns_error_only_after_restoring_before_image() { + let temp = tempfile::tempdir().expect("tempdir"); + let path = temp.path().join("shared.md"); + fs::write(&path, b"before").expect("seed"); + let before = read_shared_file(&path, 1024, "test").expect("snapshot"); + fail_next_parent_sync_for_test(&path); + + let error = replace_shared_file(&path, &before.revision, b"after", 1024, None, "test") + .expect_err("the injected durability failure remains visible"); + assert!(error.to_string().contains("injected")); + assert_eq!(fs::read(&path).expect("before restored"), b"before"); + } + + #[cfg(unix)] + #[test] + fn delete_sync_failure_returns_error_only_after_restoring_before_image() { + let temp = tempfile::tempdir().expect("tempdir"); + let path = temp.path().join("shared.md"); + fs::write(&path, b"before").expect("seed"); + let before = read_shared_file(&path, 1024, "test").expect("snapshot"); + fail_next_parent_sync_for_test(&path); + + let error = delete_shared_file(&path, &before.revision, 1024, "test") + .expect_err("the injected durability failure remains visible"); + assert!(error.to_string().contains("injected")); + assert_eq!(fs::read(&path).expect("before restored"), b"before"); + } + #[cfg(unix)] #[test] fn replacement_preserves_exact_existing_permissions_despite_umask() { diff --git a/src-tauri/src/services/pi_prompt_files.rs b/src-tauri/src/services/pi_prompt_files.rs index 389b94d69..5b1520011 100644 --- a/src-tauri/src/services/pi_prompt_files.rs +++ b/src-tauri/src/services/pi_prompt_files.rs @@ -346,6 +346,59 @@ mod tests { assert!(validate_direct_instruction_content("# Explicit override").is_ok()); } + #[cfg(unix)] + #[test] + fn direct_instruction_entry_never_reports_failure_with_its_attempt_live() { + let temp = tempfile::tempdir().expect("tempdir"); + let path = temp + .path() + .join(PiPromptFileKind::SystemOverride.filename()); + + crate::pi_config::shared_file::fail_next_parent_sync_for_test(&path); + PiPromptFileService::replace_at( + temp.path(), + PiPromptFileKind::SystemOverride, + "missing", + "created", + ) + .expect_err("failed create must be compensated"); + assert!(!path.exists()); + + let before = PiPromptFileService::replace_at( + temp.path(), + PiPromptFileKind::SystemOverride, + "missing", + "before", + ) + .expect("seed"); + crate::pi_config::shared_file::fail_next_parent_sync_for_test(&path); + PiPromptFileService::replace_at( + temp.path(), + PiPromptFileKind::SystemOverride, + &before.revision, + "after", + ) + .expect_err("failed replace must restore its before-image"); + assert_eq!( + fs::read_to_string(&path).expect("before restored"), + "before" + ); + + let before = PiPromptFileService::read_at(temp.path(), PiPromptFileKind::SystemOverride) + .expect("snapshot"); + crate::pi_config::shared_file::fail_next_parent_sync_for_test(&path); + PiPromptFileService::delete_at( + temp.path(), + PiPromptFileKind::SystemOverride, + &before.revision, + ) + .expect_err("failed delete must restore its before-image"); + assert_eq!( + fs::read_to_string(&path).expect("before restored"), + "before" + ); + } + #[test] fn templates_reject_ambiguous_or_traversing_slugs() { for slug in [ diff --git a/src-tauri/src/services/prompt.rs b/src-tauri/src/services/prompt.rs index 350e60efb..726c01f31 100644 --- a/src-tauri/src/services/prompt.rs +++ b/src-tauri/src/services/prompt.rs @@ -222,7 +222,23 @@ impl PromptService { if !snapshot.exists { return Err(AppError::Message("Pi AGENTS.md does not exist".to_string())); } - return Self::import_pi_snapshot(state, snapshot); + let before = state.db.get_prompts(AppType::Pi.as_str())?; + let timestamp = get_unix_timestamp()?; + let id = format!("imported-{timestamp}"); + let prompt = Prompt { + id: id.clone(), + name: format!( + "导入的提示词 {}", + chrono::Local::now().format("%Y-%m-%d %H:%M") + ), + content: snapshot.content.clone(), + description: Some("从 Pi AGENTS.md 导入".to_string()), + enabled: true, + created_at: Some(timestamp), + updated_at: Some(timestamp), + }; + Self::import_pi_snapshot(state, &guard, &snapshot, &before, prompt)?; + return Ok(id); } let file_path = prompt_file_path(&app)?; @@ -291,26 +307,20 @@ impl PromptService { return Ok(0); } let timestamp = get_unix_timestamp()?; - let mut after = existing.clone(); let id = format!("auto-imported-{timestamp}"); - after.insert( - id.clone(), - Prompt { - id, - name: format!( - "Auto-imported Prompt {}", - chrono::Local::now().format("%Y-%m-%d %H:%M") - ), - content: snapshot.content, - description: Some("Automatically imported on first launch".to_string()), - enabled: true, - created_at: Some(timestamp), - updated_at: Some(timestamp), - }, - ); - state - .db - .compare_exchange_prompt_selection(app.as_str(), &existing, &after)?; + let prompt = Prompt { + id: id.clone(), + name: format!( + "Auto-imported Prompt {}", + chrono::Local::now().format("%Y-%m-%d %H:%M") + ), + content: snapshot.content.clone(), + description: Some("Automatically imported on first launch".to_string()), + enabled: true, + created_at: Some(timestamp), + updated_at: Some(timestamp), + }; + Self::import_pi_snapshot(state, &guard, &snapshot, &existing, prompt)?; return Ok(1); } @@ -402,20 +412,22 @@ impl PromptService { db.compare_exchange_prompt_selection(AppType::Pi.as_str(), &original, &prompts)?; #[cfg(test)] - replace_pi_agents_after_reconcile_save_for_test(db, &snapshot.path)?; + apply_pi_native_binding_after_save_hooks_for_test(db, &snapshot.path)?; let verified = match PiPromptFileService::read_under_guard(guard, PiPromptFileKind::GlobalContext) { Ok(verified) => verified, Err(error) => { - restore_pi_library_after_failed_reconcile(db, &original, &prompts, &error)?; + restore_pi_library_after_failed_native_binding( + db, &original, &prompts, &error, + )?; return Err(error); } }; if verified.revision == snapshot.revision { return Ok(()); } - restore_pi_library_after_failed_reconcile( + restore_pi_library_after_failed_native_binding( db, &original, &prompts, @@ -582,38 +594,56 @@ impl PromptService { fn import_pi_snapshot( state: &AppState, - snapshot: PiPromptFileSnapshot, - ) -> Result { - let timestamp = get_unix_timestamp()?; - let id = format!("imported-{timestamp}"); - let before = state.db.get_prompts(AppType::Pi.as_str())?; + guard: &PiInstructionFileGuard, + snapshot: &PiPromptFileSnapshot, + before: &IndexMap, + prompt: Prompt, + ) -> Result<(), AppError> { let mut prompts = before.clone(); for prompt in prompts.values_mut() { prompt.enabled = false; } - prompts.insert( - id.clone(), - Prompt { - id: id.clone(), - name: format!( - "导入的提示词 {}", - chrono::Local::now().format("%Y-%m-%d %H:%M") - ), - content: snapshot.content, - description: Some("从 Pi AGENTS.md 导入".to_string()), - // Import is an explicit reconciliation action. The native file - // is already active by presence, so its exact DB counterpart - // must become the sole enabled library entry without - // rewriting the user-owned file. - enabled: true, - created_at: Some(timestamp), - updated_at: Some(timestamp), - }, - ); + // Import is an explicit reconciliation action. The native file is + // already active by presence, so its exact DB counterpart becomes the + // sole enabled library entry without rewriting the user-owned file. + prompts.insert(prompt.id.clone(), prompt); + + let precommit = + PiPromptFileService::read_under_guard(guard, PiPromptFileKind::GlobalContext)?; + if precommit.revision != snapshot.revision { + return Err(AppError::Conflict( + "Pi AGENTS.md changed before prompt import publication".to_string(), + )); + } state .db - .compare_exchange_prompt_selection(AppType::Pi.as_str(), &before, &prompts)?; - Ok(id) + .compare_exchange_prompt_selection(AppType::Pi.as_str(), before, &prompts)?; + #[cfg(test)] + apply_pi_native_binding_after_save_hooks_for_test(state.db.as_ref(), &snapshot.path)?; + let verified = + match PiPromptFileService::read_under_guard(guard, PiPromptFileKind::GlobalContext) { + Ok(verified) => verified, + Err(error) => { + restore_pi_library_after_failed_native_binding( + state.db.as_ref(), + before, + &prompts, + &error, + )?; + return Err(error); + } + }; + if verified.revision != snapshot.revision { + let error = AppError::Conflict("Pi AGENTS.md changed during prompt import".to_string()); + restore_pi_library_after_failed_native_binding( + state.db.as_ref(), + before, + &prompts, + &error, + )?; + return Err(error); + } + Ok(()) } } @@ -720,7 +750,7 @@ fn build_pi_reconciled_library( prompts } -fn restore_pi_library_after_failed_reconcile( +fn restore_pi_library_after_failed_native_binding( db: &Database, original: &IndexMap, attempted: &IndexMap, @@ -729,7 +759,7 @@ fn restore_pi_library_after_failed_reconcile( db.restore_prompt_selection_if_attempted(AppType::Pi.as_str(), attempted, original) .map_err(|restore_error| { AppError::Config(format!( - "Pi prompt reconciliation lost its native revision ({cause}) and failed to \ + "Pi prompt-library publication lost its native revision ({cause}) and failed to \ restore the previous portable library without overwriting a newer database \ revision: {restore_error}" )) @@ -737,23 +767,23 @@ fn restore_pi_library_after_failed_reconcile( } #[cfg(test)] -static PI_RECONCILE_AFTER_SAVE_REPLACEMENTS: std::sync::LazyLock< +static PI_NATIVE_BINDING_AFTER_SAVE_REPLACEMENTS: std::sync::LazyLock< std::sync::Mutex>, > = std::sync::LazyLock::new(|| std::sync::Mutex::new(std::collections::VecDeque::new())); #[cfg(test)] -fn replace_pi_agents_after_reconcile_save_for_test( +fn apply_pi_native_binding_after_save_hooks_for_test( db: &Database, path: &str, ) -> Result<(), AppError> { - if let Some(replacement) = PI_RECONCILE_AFTER_SAVE_DB_REPLACEMENTS + if let Some(replacement) = PI_NATIVE_BINDING_AFTER_SAVE_DB_REPLACEMENTS .lock() .map_err(|error| AppError::Lock(error.to_string()))? .pop_front() { db.save_prompt_selection(AppType::Pi.as_str(), &replacement)?; } - let replacement = PI_RECONCILE_AFTER_SAVE_REPLACEMENTS + let replacement = PI_NATIVE_BINDING_AFTER_SAVE_REPLACEMENTS .lock() .map_err(|error| AppError::Lock(error.to_string()))? .pop_front(); @@ -764,23 +794,23 @@ fn replace_pi_agents_after_reconcile_save_for_test( } #[cfg(test)] -static PI_RECONCILE_AFTER_SAVE_DB_REPLACEMENTS: std::sync::LazyLock< +static PI_NATIVE_BINDING_AFTER_SAVE_DB_REPLACEMENTS: std::sync::LazyLock< std::sync::Mutex>>, > = std::sync::LazyLock::new(|| std::sync::Mutex::new(std::collections::VecDeque::new())); #[cfg(test)] fn replace_pi_library_after_next_reconcile_save_for_test(replacement: IndexMap) { - PI_RECONCILE_AFTER_SAVE_DB_REPLACEMENTS + PI_NATIVE_BINDING_AFTER_SAVE_DB_REPLACEMENTS .lock() .expect("Pi reconcile DB hook lock") .push_back(replacement); } #[cfg(test)] -fn replace_pi_agents_after_each_reconcile_save_for_test( +fn replace_pi_agents_after_each_native_binding_save_for_test( replacements: impl IntoIterator, ) { - *PI_RECONCILE_AFTER_SAVE_REPLACEMENTS + *PI_NATIVE_BINDING_AFTER_SAVE_REPLACEMENTS .lock() .expect("Pi reconcile hook lock") = replacements.into_iter().map(ToOwned::to_owned).collect(); @@ -895,6 +925,55 @@ mod tests { ); } + #[test] + #[serial] + fn public_pi_import_rolls_back_portable_state_if_native_revision_changes() { + let temp = tempfile::tempdir().expect("tempdir"); + let _restore = EnvRestore::set("PI_CODING_AGENT_DIR", temp.path()); + std::fs::write(temp.path().join("AGENTS.md"), "native-before").expect("seed AGENTS.md"); + let state = AppState::new(Arc::new(Database::memory().expect("database"))); + replace_pi_agents_after_each_native_binding_save_for_test(["native-after"]); + + let error = PromptService::import_from_file(&state, AppType::Pi) + .expect_err("a stale native snapshot must not become active portable state"); + assert!(matches!(error, AppError::Conflict(_))); + assert!( + state + .db + .get_prompts(AppType::Pi.as_str()) + .expect("restored portable library") + .is_empty(), + "failed import must conditionally restore its exact DB before-image" + ); + assert_eq!( + std::fs::read_to_string(temp.path().join("AGENTS.md")).expect("external native edit"), + "native-after" + ); + } + + #[test] + #[serial] + fn first_launch_pi_import_rolls_back_if_native_revision_changes() { + let temp = tempfile::tempdir().expect("tempdir"); + let _restore = EnvRestore::set("PI_CODING_AGENT_DIR", temp.path()); + std::fs::write(temp.path().join("AGENTS.md"), "native-before").expect("seed AGENTS.md"); + let state = AppState::new(Arc::new(Database::memory().expect("database"))); + replace_pi_agents_after_each_native_binding_save_for_test(["native-after"]); + + let error = PromptService::import_from_file_on_first_launch(&state, AppType::Pi) + .expect_err("first-launch import must bind its DB row to one native revision"); + assert!(matches!(error, AppError::Conflict(_))); + assert!(state + .db + .get_prompts(AppType::Pi.as_str()) + .expect("restored portable library") + .is_empty()); + assert_eq!( + std::fs::read_to_string(temp.path().join("AGENTS.md")).expect("external native edit"), + "native-after" + ); + } + #[test] fn unowned_empty_agents_file_is_not_treated_as_absent() { let snapshot = PiPromptFileSnapshot { @@ -1248,7 +1327,9 @@ mod tests { .expect("before prompts"), ) .expect("serialize before"); - replace_pi_agents_after_each_reconcile_save_for_test(["native-1", "native-2", "native-3"]); + replace_pi_agents_after_each_native_binding_save_for_test([ + "native-1", "native-2", "native-3", + ]); assert!(matches!( PromptService::reconcile_pi_library(&state), @@ -1291,7 +1372,7 @@ mod tests { prompt("restored-import", "portable-restored", true, 2), )]); replace_pi_library_after_next_reconcile_save_for_test(imported.clone()); - replace_pi_agents_after_each_reconcile_save_for_test(["native-1"]); + replace_pi_agents_after_each_native_binding_save_for_test(["native-1"]); let error = PromptService::reconcile_pi_library(&state) .expect_err("stale compensation must not overwrite a concurrent import");