diff --git a/Plugin/Kerbcast/KerbcastCore.cs b/Plugin/Kerbcast/KerbcastCore.cs index 7e1c869..b2a726f 100644 --- a/Plugin/Kerbcast/KerbcastCore.cs +++ b/Plugin/Kerbcast/KerbcastCore.cs @@ -268,12 +268,19 @@ private void OnPartDestroyed(Part part) if (affected != null) MarkFxDirtyForVessel(affected); } - // Decoupling, staging, fairing jettison etc. change a vessel's part set - // without firing onPartDestroyed for our tracked part. Rebuild the FX - // draw lists so they match the current parts. + // Decoupling, staging, docking, fairing jettison etc. change a + // vessel's part set without firing onVesselChange (no pilot switch) + // or onPartDestroyed for our tracked parts. Pick up any newly-joined + // parts (e.g. a docking partner's cameras) immediately rather than + // waiting for an incidental later vessel switch — additive only, see + // RebuildCameraList's disposeMissing doc comment for why staged-off + // debris must not be torn down here. Also rebuild the FX draw lists + // so they match the current parts. private void OnVesselWasModified(Vessel v) { - if (v != null) MarkFxDirtyForVessel(v); + if (v == null) return; + RebuildCameraList(v, disposeMissing: false); + MarkFxDirtyForVessel(v); } private void MarkFxDirtyForVessel(Vessel v) @@ -306,10 +313,24 @@ private static uint SyntheticFlightId(uint baseId, int moduleIdx, string cameraN } } - private void RebuildCameraList(Vessel vessel) + // disposeMissing=true (onVesselChange — an actual pilot switch): scope + // the camera list to `vessel`, tearing down anything not on it. This + // is the documented "leaving a vessel drops its cameras" behavior. + // + // disposeMissing=false (onVesselWasModified — dock/stage without a + // pilot switch): additive only. A part leaving `vessel` via staging + // is still physically out there on debris and should only stop + // streaming when it's actually destroyed (physics range / + // onPartDestroyed), not because this vessel's membership changed — + // so nothing already tracked gets touched, and only cameras for + // newly-appeared parts (e.g. a docking partner's) get added. + private void RebuildCameraList(Vessel vessel, bool disposeMissing = true) { - foreach (var cam in _cameras) cam.Dispose(); - _cameras.Clear(); + if (disposeMissing) + { + foreach (var cam in _cameras) cam.Dispose(); + _cameras.Clear(); + } if (vessel == null) return; @@ -365,6 +386,11 @@ private void RebuildCameraList(Vessel vessel) // get a deterministic hash of (flightID, cameraName) // so they're stable across loads. uint flightId = SyntheticFlightId(part.flightID, moduleIdx, hullcam.cameraName); + if (!disposeMissing && _cameras.Any(c => c.FlightId == flightId)) + { + moduleIdx++; + continue; + } var panCap = PartCapabilities.ForPart(hullcam.part.partInfo?.name ?? ""); _cameras.Add(new KerbcastCamera( hullcam, diff --git a/docs/TROUBLESHOOTING.md b/docs/TROUBLESHOOTING.md index dd2781e..5e42475 100644 --- a/docs/TROUBLESHOOTING.md +++ b/docs/TROUBLESHOOTING.md @@ -109,5 +109,13 @@ leaves your own game view untouched. ## Still stuck? -Open an issue at with the -`[Kerbcast]`/`[Kerbcast.sidecar]` slice of KSP.log and your OS + KSP version. +1. Turn on verbose logging: uncomment `DebugCameraLogging = true` in + `GameData/Kerbcast/PluginData/settings.cfg` (see + [INSTALL.md](INSTALL.md#configuration) for where per-camera settings + live). +2. Reproduce the problem, then quit KSP so the log file is flushed. +3. Open an issue at with: + - the `[Kerbcast]`/`[Kerbcast.sidecar]` slice of KSP.log + - your OS + KSP version + - which vessel/camera part was affected, and what you were doing right + before it happened (vessel switch, revert, scene change, …) diff --git a/sidecar/src/cameras.rs b/sidecar/src/cameras.rs index 447dc09..d812e26 100644 --- a/sidecar/src/cameras.rs +++ b/sidecar/src/cameras.rs @@ -27,6 +27,29 @@ use crate::encoder::{EncoderBackend, SessionHealth}; use crate::protocol::{CameraLifecycle, CameraState as ProtocolCameraState, Layer, QualityPreset}; use crate::shared_mem::{ControlBlock, MmapFrameRing, MmapRingConfig}; +/// Filesystem identity for a `.ring` file: distinguishes "same path, same +/// underlying file" from "same path, file was unlinked and a new one +/// created in its place". Inode is exact on unix; `MetadataExt::file_index` +/// would give the same on Windows but is still unstable +/// (`windows_by_handle`, rust-lang/rust#63010), so non-unix falls back to +/// the mtime in nanoseconds — coarser (a same-tick delete+recreate could in +/// theory collide) but still a large improvement over never detecting the +/// swap at all. +#[cfg(unix)] +fn ring_file_identity(meta: &std::fs::Metadata) -> u64 { + use std::os::unix::fs::MetadataExt; + meta.ino() +} + +#[cfg(not(unix))] +fn ring_file_identity(meta: &std::fs::Metadata) -> u64 { + meta.modified() + .ok() + .and_then(|t| t.duration_since(std::time::UNIX_EPOCH).ok()) + .map(|d| d.as_nanos() as u64) + .unwrap_or(0) +} + /// On-disk shape of `global.status.json` — the plugin → sidecar push /// half of the IPC. The plugin rewrites this file at ~1Hz with the /// current effective state for every tracked camera plus the global @@ -255,6 +278,16 @@ struct InfoManifest { pub struct CameraState { pub flight_id: u32, pub ring: MmapFrameRing, + /// Filesystem identity (inode on unix, file index on windows) of the + /// `.ring` file this entry attached to. `RebuildCameraList` on the + /// plugin side can delete and recreate a ring at the same path within + /// one Unity frame for a camera whose part persists across a vessel + /// switch, without writing a `destroyed` tombstone (that's reserved for + /// actual part destruction). A same-second delete+recreate can share an + /// mtime, so identity — not mtime — is what `rescan` compares to notice + /// the file underneath changed and reattach instead of leaving the mmap + /// pointed at the unlinked original. + pub ring_identity: u64, pub max_width: u32, pub max_height: u32, pub part_name: String, @@ -730,14 +763,35 @@ impl CameraRegistry { // no ring file, so they won't appear in `found`). let mut attached: Vec = Vec::new(); for (id, path) in &found { + let found_identity = match tokio::fs::metadata(path).await { + Ok(meta) => ring_file_identity(&meta), + Err(_) => 0, + }; // A ring that reappears for a flight_id we already hold as a // destroyed tombstone means KSP reused that part.flightID for a // freshly spawned camera (it recycles ids across revert/recover), // so fall through and resurrect it: the insert below replaces the // tombstone with a fresh active entry and the attach is announced - // to peers so their tiles rebind. A live entry is left untouched. + // to peers so their tiles rebind. A live entry with an unchanged + // ring file identity is left untouched. + // + // A live (non-destroyed) entry whose ring identity *has* changed + // means the plugin deleted and recreated the ring at this path + // without a destroyed tombstone — e.g. RebuildCameraList tearing + // down and rebuilding a camera on `onVesselChange` whose part + // persisted across the switch. Without this check the old mmap + // stays pointed at the unlinked file and the feed goes stale + // forever, since nothing else in the registry ever changes to + // signal a reattach is needed. let resurrecting = match cameras.get(id) { Some(existing) if existing.destroyed.load(Ordering::Acquire) => true, + Some(existing) if existing.ring_identity != found_identity => { + info!( + flight_id = id, + "ring file identity changed with no destroyed tombstone — reattaching" + ); + true + } Some(_) => continue, None => false, }; @@ -759,6 +813,7 @@ impl CameraRegistry { Arc::new(CameraState { flight_id: *id, ring, + ring_identity: found_identity, max_width: self.ring_cfg.max_width, max_height: self.ring_cfg.max_height, part_name: manifest.part_name, @@ -1547,6 +1602,67 @@ mod tests { ); } + // Reproduces the vessel-switch stale-feed bug (kerbcast#2): a camera + // whose part persists across `onVesselChange` gets torn down and rebuilt + // by RebuildCameraList without a destroyed tombstone (that path is + // reserved for actual part destruction), deleting and recreating the + // ring at the same flight_id within one frame. Before the ring_identity + // check this was silently ignored by rescan (the entry wasn't flagged + // destroyed) and the old mmap stayed pointed at the unlinked file + // forever — no browser refresh could recover it. + // + // unix-only: the identity check is exact there (inode). The non-unix + // fallback (mtime) can't distinguish two files created back-to-back + // within the same tick, which is exactly what this test does — that's + // an inherent limit of the tier-2 fallback, not something to assert on. + #[cfg(unix)] + #[tokio::test] + async fn live_camera_reattaches_when_ring_recreated_without_destroyed_tombstone() { + let dir = tempfile::tempdir().expect("tempdir"); + let cfg = MmapRingConfig { + slot_count: 4, + max_width: 64, + max_height: 64, + }; + let shm = dir.path(); + let id: u32 = 7; + let write_manifest = || { + let content = format!( + r#"{{"lifecycle":"active","flight_id":{id},"part_name":"hc.navcam","part_title":"NavCam","camera_name":"NavCam","vessel_name":"booster","supports_zoom":false,"fov":60.0,"fov_min":10.0,"fov_max":90.0,"supports_pan":false,"pan_yaw_min":0.0,"pan_yaw_max":0.0,"pan_pitch_min":0.0,"pan_pitch_max":0.0}}"#, + ); + std::fs::write(shm.join(format!("{id}.info.json")), content).unwrap(); + }; + let ring_path = shm.join(format!("{id}.ring")); + let registry = CameraRegistry::new(shm.to_path_buf(), cfg); + + // 1. Fresh camera attaches active. + MmapFrameRing::create(&ring_path, cfg).expect("create ring"); + write_manifest(); + registry.rescan().await; + let first = registry.get(id).await.expect("camera attached"); + assert!(!first.destroyed.load(Ordering::Acquire)); + + // 2. Vessel switch: plugin's normal (non-destroyed) teardown deletes + // the ring and info.json with no tombstone, then RebuildCameraList + // immediately recreates the same flight_id — all within one frame, + // well before the sidecar's next ~1Hz rescan tick. + std::fs::remove_file(&ring_path).unwrap(); + std::fs::remove_file(shm.join(format!("{id}.info.json"))).unwrap(); + MmapFrameRing::create(&ring_path, cfg).expect("recreate ring"); + write_manifest(); + + let outcome = registry.rescan().await; + assert!( + outcome.attached.contains(&id), + "reattach must be announced so peers rebind and stop reading the stale mmap" + ); + let second = registry.get(id).await.expect("camera still present"); + assert!( + !std::sync::Arc::ptr_eq(&first, &second), + "the registry entry must be replaced, not left pointing at the unlinked ring" + ); + } + // The force-render profiling override keeps a peerless camera subscribed // (so the plugin renders + reports telemetry), and clearing it releases the // camera again — the cleanup path POST /profile/render?on=false relies on.