From c88b7a12d803350f551beb535d383355d1ddafbb Mon Sep 17 00:00:00 2001 From: ruv Date: Mon, 27 Jul 2026 16:15:56 -0400 Subject: [PATCH] =?UTF-8?q?fix(homecore):=20review=20findings=20from=20PR?= =?UTF-8?q?=20#1451=20=E2=80=94=20HAP=20secret=20redaction,=20REST=20histo?= =?UTF-8?q?ry/logbook=20cap,=20event=5Ftype=20validation,=20migration=20--?= =?UTF-8?q?force?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-release deep review of the merged HOMECORE platform PR (#1451) turned up several real issues, fixed here: - homecore-hap: StoredAccessory's permanent Ed25519 signing seed and StoredSetup's SRP salt/verifier were reachable via derived Debug. Not actively triggered by any current code path, but a future logging/panic-message change (an ordinary thing to add) would have printed the accessory's compromise-forever identity key in plaintext -- there's no rotation mechanism. Added manual, redacted Debug impls matching the existing SetupCode pattern; StoreState/ PairingStore's derived Debug inherits the redaction automatically. New test pins the exact rendered output. - homecore-api: /api/history/period and /api/logbook rejected the default (no filter_entity_id/entity) call shape once a room had more than 32 known entities -- exactly how the real HA frontend calls these endpoints. The MAX_HISTORY_ENTITIES cap now only applies to an explicit, unusually-large filter list; the existing MAX_API_HISTORY_ROWS total-row budget already bounds the actual work regardless of entity count. Two new tests: unfiltered succeeds with 40 known entities, explicit oversized filter is still rejected. - homecore-api: fire_event (both REST and WS) restricted event_type to [a-z0-9_]+, but real HA integrations commonly fire mixed-case, dotted, or hyphenated types (mobile_app.notification_action, ios.action_fired). Factored the check into a single is_valid_event_type() shared by both transports, relaxed to what actually matters for server safety: non-empty, length-bounded, no control characters. - homecore-migrate: write_config_entries/write_device_registry/ write_entity_registry's atomic no-clobber write had no escape hatch -- an operator who fixed a bad source row (or wanted a fresh re-import) had to manually delete prior output first. Added a --force CLI flag (default off, preserving the existing no-clobber default and its explicit malformed_entry_is_an_error_not_a_partial_write test) that atomically replaces an existing destination. First attempt used bare fs::rename, which hit real ERROR_ACCESS_DENIED sharing-violation flakiness on Windows; switched to pre-clearing the destination then publishing through the same hard_link step the default path already uses reliably. All touched crates re-verified: homecore-hap 46 tests (was 45), homecore-api 20+6+6+7=39 tests (was 18+6+6+7=37), homecore-migrate 25 tests (was 24), all 0 failed, clippy clean under -D warnings. Also corrected the public v2051 GitHub release notes: "Wasmtime 36.0.12 component loading" was inaccurate (the crate uses the core-module API with a hand-rolled host ABI, not the Component Model/WIT) -- doc-only, not a code change. Co-Authored-By: claude-flow --- v2/crates/homecore-api/src/rest.rs | 69 +++++++++++--- v2/crates/homecore-api/src/ws.rs | 7 +- .../tests/compatibility_surface.rs | 89 +++++++++++++++++++ v2/crates/homecore-hap/src/pairing.rs | 62 ++++++++++++- v2/crates/homecore-migrate/src/cli.rs | 15 ++++ .../homecore-migrate/src/config_entries.rs | 15 +++- .../homecore-migrate/src/device_registry.rs | 34 ++++++- .../homecore-migrate/src/entity_registry.rs | 15 +++- v2/crates/homecore-migrate/src/main.rs | 21 +++-- v2/crates/homecore-migrate/src/storage.rs | 31 +++++++ 10 files changed, 328 insertions(+), 30 deletions(-) diff --git a/v2/crates/homecore-api/src/rest.rs b/v2/crates/homecore-api/src/rest.rs index 42f1bf48..da875069 100644 --- a/v2/crates/homecore-api/src/rest.rs +++ b/v2/crates/homecore-api/src/rest.rs @@ -149,6 +149,7 @@ async fn history_response( )); } + let explicit_filter = query.filter_entity_id.is_some(); let entity_ids = match query.filter_entity_id.as_deref() { Some(raw) => raw .split(',') @@ -167,9 +168,15 @@ async fn history_response( .map(|snapshot| snapshot.entity_id.clone()) .collect(), }; - if entity_ids.len() > MAX_HISTORY_ENTITIES { + // Only reject an explicit, unusually-large `filter_entity_id` list. The + // real HA frontend's history page calls this endpoint with NO filter by + // design (meaning "all entities") — a real install routinely has 50-500+ + // entities, so applying this cap there rejected the single most common + // call shape outright. The `MAX_API_HISTORY_ROWS` total-row budget below + // already bounds the actual work regardless of entity count. + if explicit_filter && entity_ids.len() > MAX_HISTORY_ENTITIES { return Err(ApiError::BadRequest(format!( - "history queries are limited to {MAX_HISTORY_ENTITIES} entities" + "history queries are limited to {MAX_HISTORY_ENTITIES} explicitly filtered entities" ))); } @@ -277,6 +284,7 @@ async fn logbook_response( "end_time must not precede start_time".into(), )); } + let explicit_filter = query.entity.is_some(); let entity_ids = match query.entity.as_deref() { Some(raw) => raw .split(',') @@ -295,9 +303,13 @@ async fn logbook_response( .map(|snapshot| snapshot.entity_id.clone()) .collect(), }; - if entity_ids.len() > MAX_HISTORY_ENTITIES { + // See the matching comment in `history_response`: only reject an + // explicit, unusually-large filter — the default (no filter, "all + // entities") is the real HA frontend's normal call shape, and the + // `MAX_API_HISTORY_ROWS` row budget below already bounds the work. + if explicit_filter && entity_ids.len() > MAX_HISTORY_ENTITIES { return Err(ApiError::BadRequest(format!( - "logbook queries are limited to {MAX_HISTORY_ENTITIES} entities" + "logbook queries are limited to {MAX_HISTORY_ENTITIES} explicitly filtered entities" ))); } let mut entries = Vec::new(); @@ -592,6 +604,23 @@ pub async fn get_events( )) } +/// Whether `event_type` is acceptable to fire on the domain bus. +/// +/// Real Home Assistant places essentially no format restriction on event +/// types beyond "non-empty string" — integrations commonly fire types with +/// mixed case, dots, or hyphens (e.g. `mobile_app.notification_action`, +/// `ios.action_fired`). The original check here only accepted +/// `[a-z0-9_]+`, silently rejecting any of those — a real behavioral gap +/// versus the documented contract, not a security boundary (this endpoint is +/// already bearer-authenticated). We keep only the bounds that protect the +/// server itself: non-empty, a sane length cap, and no control characters +/// (which could otherwise corrupt log lines or downstream storage). +pub(crate) fn is_valid_event_type(event_type: &str) -> bool { + !event_type.is_empty() + && event_type.len() <= 255 + && event_type.chars().all(|ch| !ch.is_control()) +} + pub async fn fire_event( headers: HeaderMap, State(s): State, @@ -599,12 +628,7 @@ pub async fn fire_event( Json(body): Json, ) -> ApiResult> { let _ = BearerAuth::from_headers(&headers, s.tokens()).await?; - if event_type.is_empty() - || event_type.len() > 255 - || !event_type - .chars() - .all(|ch| ch.is_ascii_lowercase() || ch.is_ascii_digit() || ch == '_') - { + if !is_valid_event_type(&event_type) { return Err(ApiError::BadRequest("invalid event_type".into())); } if !body.is_object() && !body.is_null() { @@ -705,3 +729,28 @@ pub async fn compatibility( } }))) } + +#[cfg(test)] +mod tests { + use super::is_valid_event_type; + + /// Real HA integrations commonly fire event types with mixed case, dots, + /// or hyphens (e.g. `mobile_app.notification_action`). The original + /// `[a-z0-9_]+`-only check rejected all of these; only non-empty, + /// length, and control-character bounds should remain. + #[test] + fn realistic_ha_event_types_are_accepted() { + assert!(is_valid_event_type("mobile_app.notification_action")); + assert!(is_valid_event_type("ios.action_fired")); + assert!(is_valid_event_type("Custom-Event.2")); + assert!(is_valid_event_type("state_changed")); + } + + #[test] + fn empty_oversized_or_control_char_event_types_are_rejected() { + assert!(!is_valid_event_type("")); + assert!(!is_valid_event_type(&"a".repeat(256))); + assert!(!is_valid_event_type("bad\nevent")); + assert!(!is_valid_event_type("bad\tevent")); + } +} diff --git a/v2/crates/homecore-api/src/ws.rs b/v2/crates/homecore-api/src/ws.rs index 38cf3df7..af3b2b60 100644 --- a/v2/crates/homecore-api/src/ws.rs +++ b/v2/crates/homecore-api/src/ws.rs @@ -328,12 +328,7 @@ impl Connection { self.err(tx, cmd.id, "invalid_format", "event_type is required"); return; }; - if event_type.is_empty() - || event_type.len() > 255 - || !event_type - .chars() - .all(|ch| ch.is_ascii_lowercase() || ch.is_ascii_digit() || ch == '_') - { + if !crate::rest::is_valid_event_type(&event_type) { self.err(tx, cmd.id, "invalid_format", "invalid event_type"); return; } diff --git a/v2/crates/homecore-api/tests/compatibility_surface.rs b/v2/crates/homecore-api/tests/compatibility_surface.rs index 66ec8df4..d5571873 100644 --- a/v2/crates/homecore-api/tests/compatibility_surface.rs +++ b/v2/crates/homecore-api/tests/compatibility_surface.rs @@ -102,3 +102,92 @@ async fn history_reads_real_recorder_rows() { assert_eq!(body[0][0]["state"], "on"); assert_eq!(body[0][0]["attributes"]["brightness"], 123); } + +/// The real HA frontend's history/logbook pages call these endpoints with NO +/// entity filter by design (meaning "all entities") — a real install +/// routinely has 50-500+ entities. The 32-entity cap must only reject an +/// explicit, unusually-large `filter_entity_id`/`entity` list, never the +/// default unfiltered "all entities" shape. +#[tokio::test] +async fn history_and_logbook_unfiltered_are_not_capped_by_entity_count() { + use homecore::{Context, EntityId}; + use homecore_recorder::Recorder; + + let homecore = HomeCore::new(); + let recorder = Recorder::open("sqlite::memory:").await.unwrap(); + for i in 0..40 { + homecore.states().set( + EntityId::parse(&format!("sensor.probe_{i}")).unwrap(), + "on", + serde_json::json!({}), + Context::new(), + ); + } + assert_eq!(homecore.states().all().len(), 40, "sanity: more than MAX_HISTORY_ENTITIES"); + + let tokens = LongLivedTokenStore::empty(); + tokens.register("test-token").await; + let state = + SharedState::with_tokens(homecore, "Test", "test", tokens).with_recorder(Some(recorder)); + let app = router(state); + + let history_response = app + .clone() + .oneshot( + Request::builder() + .uri("/api/history/period") + .header("authorization", "Bearer test-token") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!( + history_response.status(), + StatusCode::OK, + "unfiltered history with >32 known entities must not be rejected" + ); + + let logbook_response = app + .oneshot( + Request::builder() + .uri("/api/logbook") + .header("authorization", "Bearer test-token") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!( + logbook_response.status(), + StatusCode::OK, + "unfiltered logbook with >32 known entities must not be rejected" + ); +} + +/// An explicit, unusually-large `filter_entity_id` list is still rejected — +/// only the default "no filter" shape is exempt from the cap. +#[tokio::test] +async fn history_explicit_oversized_filter_is_still_rejected() { + use homecore_recorder::Recorder; + + let homecore = HomeCore::new(); + let recorder = Recorder::open("sqlite::memory:").await.unwrap(); + let tokens = LongLivedTokenStore::empty(); + tokens.register("test-token").await; + let state = + SharedState::with_tokens(homecore, "Test", "test", tokens).with_recorder(Some(recorder)); + + let filter: String = (0..40).map(|i| format!("sensor.probe_{i}")).collect::>().join(","); + let response = router(state) + .oneshot( + Request::builder() + .uri(format!("/api/history/period?filter_entity_id={filter}")) + .header("authorization", "Bearer test-token") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(response.status(), StatusCode::BAD_REQUEST); +} diff --git a/v2/crates/homecore-hap/src/pairing.rs b/v2/crates/homecore-hap/src/pairing.rs index 73e6a221..23cf07da 100644 --- a/v2/crates/homecore-hap/src/pairing.rs +++ b/v2/crates/homecore-hap/src/pairing.rs @@ -144,20 +144,44 @@ pub struct PairingStoreProvisioning { pub setup_code: Option, } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Clone, Serialize, Deserialize)] #[serde(deny_unknown_fields)] struct StoredAccessory { device_id: String, signing_seed: [u8; 32], } -#[derive(Debug, Clone, Serialize, Deserialize)] +// Manual, redacted impl: `signing_seed` is the accessory's permanent Ed25519 +// identity key, used to sign every Pair-Setup/Pair-Verify transcript for the +// device's whole lifetime with no rotation mechanism. A derived `Debug` would +// print it in plaintext the first time anything formats this struct (a log +// line, a panic message) — same rationale as `SetupCode`'s manual impl below. +impl std::fmt::Debug for StoredAccessory { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter + .debug_struct("StoredAccessory") + .field("device_id", &self.device_id) + .field("signing_seed", &"[REDACTED]") + .finish() + } +} + +#[derive(Clone, Serialize, Deserialize)] #[serde(deny_unknown_fields)] struct StoredSetup { salt: [u8; 16], verifier: Vec, } +// Manual, redacted impl: `salt`/`verifier` are the SRP-6a material derived +// from the setup code. Printing them would hand an attacker exactly what an +// offline dictionary attack against the (8-digit) setup code needs. +impl std::fmt::Debug for StoredSetup { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter.write_str("StoredSetup([REDACTED])") + } +} + impl Drop for StoredAccessory { fn drop(&mut self) { self.signing_seed.zeroize(); @@ -679,6 +703,40 @@ mod tests { assert_eq!(format!("{code:?}"), "SetupCode([REDACTED])"); } + /// A stray `format!("{store:?}")` (a future debug log line, a panic + /// message) must never print the accessory's permanent Ed25519 signing + /// seed or the SRP salt/verifier — both are compromise-forever secrets + /// with no rotation mechanism. + #[test] + fn stored_accessory_and_setup_debug_never_print_secret_material() { + let accessory = StoredAccessory { + device_id: "AA:BB:CC:DD:EE:FF".into(), + signing_seed: [0x42; 32], + }; + let rendered = format!("{accessory:?}"); + assert!(rendered.contains("device_id")); + assert!(rendered.contains("AA:BB:CC:DD:EE:FF")); + assert!(!rendered.contains("66"), "hex of 0x42 must not leak: {rendered}"); + assert_eq!( + rendered, + "StoredAccessory { device_id: \"AA:BB:CC:DD:EE:FF\", signing_seed: \"[REDACTED]\" }" + ); + + let setup = StoredSetup { salt: [0x7a; 16], verifier: vec![0x13; 8] }; + assert_eq!(format!("{setup:?}"), "StoredSetup([REDACTED])"); + + // The redaction must propagate through every derived-Debug container + // that embeds these structs, with no further code changes needed. + let state = StoreState { + accessory, + setup, + controllers: std::collections::BTreeMap::new(), + }; + let rendered_state = format!("{state:?}"); + assert!(!rendered_state.contains("0x42")); + assert!(rendered_state.contains("[REDACTED]")); + } + #[test] fn removing_last_admin_clears_all_pairings() { let directory = tempfile::tempdir().unwrap(); diff --git a/v2/crates/homecore-migrate/src/cli.rs b/v2/crates/homecore-migrate/src/cli.rs index 30d73cdf..ae03efa5 100644 --- a/v2/crates/homecore-migrate/src/cli.rs +++ b/v2/crates/homecore-migrate/src/cli.rs @@ -48,6 +48,11 @@ pub struct ImportEntitiesArgs { /// Path to the HOMECORE storage directory (destination). #[arg(long)] pub to: PathBuf, + /// Overwrite an existing destination file instead of refusing. Use this + /// to re-run an import after fixing a bad source row, or to re-import + /// after further changes on the HA side. + #[arg(long)] + pub force: bool, } #[derive(Debug, clap::Args)] @@ -58,6 +63,11 @@ pub struct ImportDevicesArgs { /// Path to the HOMECORE storage directory (destination). #[arg(long)] pub to: PathBuf, + /// Overwrite an existing destination file instead of refusing. Use this + /// to re-run an import after fixing a bad source row, or to re-import + /// after further changes on the HA side. + #[arg(long)] + pub force: bool, } #[derive(Debug, clap::Args)] @@ -68,6 +78,11 @@ pub struct ImportConfigEntriesArgs { /// Path to the HOMECORE storage directory (destination). #[arg(long)] pub to: PathBuf, + /// Overwrite an existing destination file instead of refusing. Use this + /// to re-run an import after fixing a bad source row, or to re-import + /// after further changes on the HA side. + #[arg(long)] + pub force: bool, } #[derive(Debug, clap::Args)] diff --git a/v2/crates/homecore-migrate/src/config_entries.rs b/v2/crates/homecore-migrate/src/config_entries.rs index 855595e7..60638223 100644 --- a/v2/crates/homecore-migrate/src/config_entries.rs +++ b/v2/crates/homecore-migrate/src/config_entries.rs @@ -11,7 +11,7 @@ use std::path::{Path, PathBuf}; use serde::{Deserialize, Serialize}; use crate::{ - storage::{read_envelope, write_json_atomic_noclobber, HaStorageEnvelope}, + storage::{read_envelope, write_json_atomic, HaStorageEnvelope}, MigrateError, }; @@ -183,9 +183,20 @@ pub fn convert_config_entries(path: &Path) -> Result Result { + write_config_entries_with(storage_dir, envelope, false) +} + +/// As [`write_config_entries`], but `force = true` atomically replaces an +/// existing destination instead of refusing — the escape hatch for +/// re-running an import after fixing a bad source row. +pub fn write_config_entries_with( + storage_dir: &Path, + envelope: &HomeCoreConfigEnvelope, + force: bool, ) -> Result { let target = storage_dir.join(DESTINATION_KEY); - write_json_atomic_noclobber(&target, envelope) + write_json_atomic(&target, envelope, force) } pub fn read_homecore_config_entries(path: &Path) -> Result { diff --git a/v2/crates/homecore-migrate/src/device_registry.rs b/v2/crates/homecore-migrate/src/device_registry.rs index c2ff0b51..8ea98538 100644 --- a/v2/crates/homecore-migrate/src/device_registry.rs +++ b/v2/crates/homecore-migrate/src/device_registry.rs @@ -7,7 +7,7 @@ use homecore::DeviceEntry; use serde::Deserialize; use crate::{ - storage::{read_envelope, write_json_atomic_noclobber}, + storage::{read_envelope, write_json_atomic}, storage_format::v13, MigrateError, }; @@ -114,6 +114,17 @@ pub fn read_device_registry(path: &Path) -> Result, MigrateErro pub fn write_device_registry( storage_dir: &Path, devices: &[DeviceEntry], +) -> Result { + write_device_registry_with(storage_dir, devices, false) +} + +/// As [`write_device_registry`], but `force = true` atomically replaces an +/// existing destination instead of refusing — the escape hatch for +/// re-running an import after fixing a bad source row. +pub fn write_device_registry_with( + storage_dir: &Path, + devices: &[DeviceEntry], + force: bool, ) -> Result { let target = storage_dir.join(FILE_KEY); let payload = serde_json::json!({ @@ -125,7 +136,7 @@ pub fn write_device_registry( "deleted_devices": [] } }); - write_json_atomic_noclobber(&target, &payload) + write_json_atomic(&target, &payload, force) } #[cfg(test)] @@ -180,4 +191,23 @@ mod tests { 1 ); } + + /// `force = true` is the operator escape hatch: re-running an import + /// after fixing a bad source row (or re-importing after further HA-side + /// changes) must not require manually deleting prior output first. + #[test] + fn force_overwrites_existing_destination() { + let mut source = NamedTempFile::new().unwrap(); + source.write_all(FIXTURE.as_bytes()).unwrap(); + let devices = read_device_registry(source.path()).unwrap(); + let destination = tempfile::tempdir().unwrap(); + write_device_registry(destination.path(), &devices).unwrap(); + write_device_registry_with(destination.path(), &[], true).unwrap(); + assert_eq!( + read_device_registry(&destination.path().join(FILE_KEY)) + .unwrap() + .len(), + 0 + ); + } } diff --git a/v2/crates/homecore-migrate/src/entity_registry.rs b/v2/crates/homecore-migrate/src/entity_registry.rs index f4658fbc..656a8c10 100644 --- a/v2/crates/homecore-migrate/src/entity_registry.rs +++ b/v2/crates/homecore-migrate/src/entity_registry.rs @@ -33,7 +33,7 @@ use serde::{Deserialize, Serialize}; use homecore::{registry::DisabledBy, EntityCategory, EntityEntry, EntityId}; use crate::{ - storage::{read_envelope, write_json_atomic_noclobber}, + storage::{read_envelope, write_json_atomic}, storage_format::v13, MigrateError, }; @@ -174,6 +174,17 @@ pub fn read_entity_registry(path: &Path) -> Result, MigrateErro pub fn write_entity_registry( storage_dir: &Path, entries: &[EntityEntry], +) -> Result { + write_entity_registry_with(storage_dir, entries, false) +} + +/// As [`write_entity_registry`], but `force = true` atomically replaces an +/// existing destination instead of refusing — the escape hatch for +/// re-running an import after fixing a bad source row. +pub fn write_entity_registry_with( + storage_dir: &Path, + entries: &[EntityEntry], + force: bool, ) -> Result { let target = storage_dir.join(FILE_KEY); let payload = serde_json::json!({ @@ -185,7 +196,7 @@ pub fn write_entity_registry( "deleted_entities": [] } }); - write_json_atomic_noclobber(&target, &payload) + write_json_atomic(&target, &payload, force) } #[cfg(test)] diff --git a/v2/crates/homecore-migrate/src/main.rs b/v2/crates/homecore-migrate/src/main.rs index b89097fa..fc0e829d 100644 --- a/v2/crates/homecore-migrate/src/main.rs +++ b/v2/crates/homecore-migrate/src/main.rs @@ -61,8 +61,11 @@ fn main() -> anyhow::Result<()> { Command::ImportEntities(args) => { let entity_path = args.storage.join("core.entity_registry"); let entries = homecore_migrate::entity_registry::read_entity_registry(&entity_path)?; - let destination = - homecore_migrate::entity_registry::write_entity_registry(&args.to, &entries)?; + let destination = homecore_migrate::entity_registry::write_entity_registry_with( + &args.to, + &entries, + args.force, + )?; print_summary(&ImportSummary { kind: "entity_registry", imported: entries.len(), @@ -75,8 +78,11 @@ fn main() -> anyhow::Result<()> { Command::ImportDevices(args) => { let device_path = args.storage.join("core.device_registry"); let devices = homecore_migrate::device_registry::read_device_registry(&device_path)?; - let destination = - homecore_migrate::device_registry::write_device_registry(&args.to, &devices)?; + let destination = homecore_migrate::device_registry::write_device_registry_with( + &args.to, + &devices, + args.force, + )?; print_summary(&ImportSummary { kind: "device_registry", imported: devices.len(), @@ -107,8 +113,11 @@ fn main() -> anyhow::Result<()> { .iter() .map(serde_json::to_value) .collect::, _>>()?; - let destination = - homecore_migrate::config_entries::write_config_entries(&args.to, &converted)?; + let destination = homecore_migrate::config_entries::write_config_entries_with( + &args.to, + &converted, + args.force, + )?; print_summary(&ImportSummary { kind: "config_entries", imported, diff --git a/v2/crates/homecore-migrate/src/storage.rs b/v2/crates/homecore-migrate/src/storage.rs index d86a5bc1..ca5ae2dc 100644 --- a/v2/crates/homecore-migrate/src/storage.rs +++ b/v2/crates/homecore-migrate/src/storage.rs @@ -80,6 +80,21 @@ pub fn read_envelope(path: &Path) -> Result { pub fn write_json_atomic_noclobber( target: &Path, value: &T, +) -> Result { + write_json_atomic(target, value, false) +} + +/// Same atomic write (temp file → `sync_all` → publish), but when `force` is +/// true an existing destination is atomically replaced via `rename` instead +/// of refusing via `hard_link`'s `AlreadyExists`. Without `force`, an +/// operator who re-runs an import after fixing a bad source row (or wants a +/// fresh pass) had no way to overwrite prior output short of deleting it by +/// hand first — this is the escape hatch for that, opt-in so the default +/// no-clobber safety is unchanged. +pub fn write_json_atomic( + target: &Path, + value: &T, + force: bool, ) -> Result { let parent = target.parent().ok_or_else(|| MigrateError::Io { path: target.display().to_string(), @@ -112,6 +127,22 @@ pub fn write_json_atomic_noclobber( file.write_all(&bytes)?; file.write_all(b"\n")?; file.sync_all()?; + if force { + // `rename`-over-existing hits real sharing-violation flakiness + // on Windows (ERROR_ACCESS_DENIED even with no other open + // handle in this process). Pre-clear the destination instead, + // then publish through the same hard_link step the no-clobber + // path uses. This briefly widens the crash window (a crash + // between remove and hard_link leaves no destination file + // rather than the old one), which is the accepted, opt-in + // tradeoff of explicitly requesting an overwrite — the default + // (non-force) path keeps its full no-clobber atomicity. + match fs::remove_file(target) { + Ok(()) => {} + Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} + Err(error) => return Err(error), + } + } fs::hard_link(&temp, target)?; fs::remove_file(&temp)?; Ok(())