From 2b812ebe21f3d736128c76227cee9cba8a5e55dc Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Sat, 29 Aug 2026 22:19:29 +0200 Subject: [PATCH 1/2] Give memory back in the order the user will miss it least FR-PLAT-AND-5. Android asks for memory back through onTrimMemory and kills the process if it is not given; until now nothing listened, so the answer was always "no". A tiered registry answers instead: GPU caches first, then proxies, then thumbnails, driven from android_main on MainEvent::LowMemory and MainEvent::Stop. The order is the argument. A backgrounded app has no window to draw and therefore no use for a render pipeline, while its thumbnails are exactly what the user will be looking at half a second after they come back -- so going into the background frees only the GPU tier, and only being measured against death frees everything. Sinks register beside the cache they free and hold weak handles, so the registry cannot keep a controller -- and every decoded portrait in it -- alive past the interface it belonged to. `try_borrow_mut` and skip: a warning can land mid-render, freeing textures under the code drawing with them is worse than missing one, and a warning not acted on is always followed by another. The GPU test is the one that matters: an eviction must change no pixel. A freed intermediate pool whose `colour_key` promise still stands renders an empty texture, and nothing else would have caught it. Co-Authored-By: Claude Opus 5 (1M context) --- apps/darkroom-android/src/lib.rs | 42 ++++- core/dr-gpu/src/adjust.rs | 38 ++++ core/dr-gpu/src/detail.rs | 31 ++++ core/dr-gpu/tests/detail_stage.rs | 53 ++++++ ui/dr-ui/src/develop.rs | 27 +++ ui/dr-ui/src/identity_ui.rs | 15 ++ ui/dr-ui/src/lib.rs | 43 +++++ ui/dr-ui/src/memory.rs | 276 ++++++++++++++++++++++++++++++ 8 files changed, 524 insertions(+), 1 deletion(-) create mode 100644 ui/dr-ui/src/memory.rs diff --git a/apps/darkroom-android/src/lib.rs b/apps/darkroom-android/src/lib.rs index 79b20b3..6eda761 100644 --- a/apps/darkroom-android/src/lib.rs +++ b/apps/darkroom-android/src/lib.rs @@ -51,7 +51,47 @@ fn android_main(app: slint::android::AndroidApp) { // After the data dir and before anything asks whether a model is present. install_bundled_face_models(&app); - if let Err(e) = slint::android::init(app) { + // TRACES: FR-PLAT-AND-5 + // The listener is the whole reason this is not the one-line + // `slint::android::init(app)`. Slint owns the event loop on Android, so + // the platform's lifecycle and memory events reach the application only if + // it asks for them here — and it must ask *before* the loop starts, which + // is why this sits between the data directory and `dr_ui::run`. + // + // The listener runs inside `poll_events`, on the same thread the event + // loop and every interface cache live on, which is what lets + // `dr_ui::memory` be a thread-local registry of plain `Fn()` rather than a + // cross-thread channel (see its module documentation). + // + // # Why two events and not eight + // + // FR-PLAT-AND-5 names `onTrimMemory`, whose `TRIM_MEMORY_*` levels grade + // how badly the system wants the memory back. Those levels do not exist + // here: `ComponentCallbacks2` is a Java interface implemented by an + // `Activity` or `Application`, and this app has neither — it is a bare + // `NativeActivity`, whose native callback table offers only the ungraded + // `onLowMemory`. android-activity surfaces exactly that as `LowMemory`. + // Reading the grades would mean shipping a Java subclass to forward them, + // which is a distribution-manifest change and not this one. + // + // `Stop` recovers the one grade that matters most anyway, and for free. + // It is the moment the activity stops being visible — `TRIM_MEMORY_UI_HIDDEN` + // in all but name — and it is the cheapest possible time to give memory + // back, because nothing that is freed has to be drawn again before anyone + // sees it. `Pause` deliberately does not qualify: a permission dialog or + // the share sheet pauses an activity that is still on screen behind it, + // and throwing away its render pipeline would make every such interruption + // cost a full re-render. + use slint::android::android_activity::{MainEvent, PollEvent}; + if let Err(e) = slint::android::init_with_event_listener(app, |event| match event { + PollEvent::Main(MainEvent::LowMemory) => { + dr_ui::memory::relieve(dr_ui::memory::Level::Critical); + } + PollEvent::Main(MainEvent::Stop) => { + dr_ui::memory::relieve(dr_ui::memory::Level::UiHidden); + } + _ => {} + }) { log::error!("Slint Android backend failed to initialise: {e}"); return; } diff --git a/core/dr-gpu/src/adjust.rs b/core/dr-gpu/src/adjust.rs index 90109bc..0a2d2e7 100644 --- a/core/dr-gpu/src/adjust.rs +++ b/core/dr-gpu/src/adjust.rs @@ -1026,6 +1026,44 @@ impl AdjustPass { h } + /// TRACES: FR-PLAT-AND-5 | NFR-RES-1 + /// Give back every allocation this pass is holding only to be fast again. + /// + /// What goes, and why each is safe to lose: + /// + /// - **The compiled pipelines**, here and in the detail stage. A pure + /// lookup keyed by structure hash with a compile-on-miss behind it, and + /// unbounded until now — nothing ever removed an entry, so a session + /// that visited enough distinct edit structures accumulated shader + /// objects for the life of the process. + /// - **The detail intermediates**, which are viewport-sized `Rgba16Float` + /// and, as `detail.rs` says of them, grow but never shrink. + /// - **The two output textures.** Dropping these does not take the picture + /// off the screen: whatever was handed to the compositor holds its own + /// reference to the `wgpu::Texture`, so releasing ours only means the + /// *next* render allocates rather than reuses. `ensure_target` already + /// treats an empty slot as "allocate", because that is the state it + /// starts in. + /// + /// **`colour_key` must be cleared with them, and this is the part that + /// would bite.** The key is the promise that slot 0 of the detail pool + /// still holds the fused colour result, and it is what lets a sharpening + /// slider skip the colour chain (FR-DEV-3d). Freeing the pool while the + /// promise stood would make the next detail-only render sample a + /// just-allocated texture with nothing in it — a silently wrong frame, not + /// a failure, and one that would only appear on a device under memory + /// pressure. + /// + /// What deliberately stays: the demosaiced source is not this pass's to + /// drop, the film tables are set once by a caller that will not be asked + /// again, and the bind group layouts are bytes rather than megabytes. + pub fn release_caches(&mut self) { + self.cache.clear(); + self.detail.release_caches(); + self.targets = [None, None]; + self.colour_key = None; + } + /// How many distinct pipelines are compiled. Exposed for tests asserting /// that slider movement does not recompile. pub fn cached_pipelines(&self) -> usize { diff --git a/core/dr-gpu/src/detail.rs b/core/dr-gpu/src/detail.rs index 5c70c49..14553b3 100644 --- a/core/dr-gpu/src/detail.rs +++ b/core/dr-gpu/src/detail.rs @@ -157,6 +157,24 @@ impl Intermediates { self.allocations += 1; } } + + /// TRACES: FR-PLAT-AND-5 + /// Drop the pool, leaving it as [`Intermediates::new`] left it. + /// + /// The size is reset along with the slots, not merely because it is tidy: + /// [`Self::ensure`] only refills when the count is short *or* the size + /// differs, so a pool cleared while still claiming its old dimensions is + /// indistinguishable from one that never held anything — which is fine + /// here, and would stop being fine the moment `ensure` grew a fast path + /// that trusted the stored size. `allocations` deliberately keeps + /// counting: it exists so a test can see textures being made, and a + /// counter reset on eviction would hide a reallocation storm rather than + /// report one. + fn release(&mut self) { + self.slots.clear(); + self.width = 0; + self.height = 0; + } } /// Runs the detail stage. @@ -526,6 +544,19 @@ impl DetailRunner { self.cache.len() } + /// TRACES: FR-PLAT-AND-5 + /// Give back everything this stage is only holding to be fast. + /// + /// Both pools and the pipeline cache. Nothing here is state: a pool slot + /// is re-created by the next [`Intermediates::ensure`] and a pipeline by + /// the next compile-on-miss, so the only cost of this call is the work of + /// doing both again. + pub(crate) fn release_caches(&mut self) { + self.cache.clear(); + self.pool.release(); + self.reduced.release(); + } + /// How many intermediate textures have been allocated since this pass was /// created. For tests — see [`crate::MaskPass::allocations`] for the /// regression this shape of counter exists to catch. diff --git a/core/dr-gpu/tests/detail_stage.rs b/core/dr-gpu/tests/detail_stage.rs index ab18ef5..dd14be0 100644 --- a/core/dr-gpu/tests/detail_stage.rs +++ b/core/dr-gpu/tests/detail_stage.rs @@ -413,3 +413,56 @@ fn an_empty_chain_falls_through_to_the_ordinary_render() { assert_eq!(pass.detail_dispatches(), 0); assert_eq!(pass.detail_allocations(), 0); } + +/// TRACES: FR-PLAT-AND-5 +#[test] +fn eviction_gives_the_pools_back_without_changing_a_pixel() { + // The half of memory-pressure eviction that cannot be checked by looking + // at a counter. `release_caches` frees the detail pool, and slot 0 of that + // pool is where the fused colour result lives between frames — so the + // render after an eviction has to notice that the promise recorded in + // `colour_key` no longer holds and run the colour chain again. + // + // Leave the key standing and this test does not error: it draws. It draws + // whatever a freshly-allocated texture happens to contain, which is the + // failure worth building a test around, because on a device it would + // appear only under memory pressure and only as a wrong-looking photograph. + let Some(ctx) = ctx() else { return }; + const SIZE: u32 = 48; + let source = step_edge(&ctx, SIZE); + let mut pass = AdjustPass::new(&ctx); + let mut graph = EditGraph::with_detail_probe(); + graph.set_param(PROBE, RADIUS, 0.05); + + let before = render(&ctx, &mut pass, &graph, &source, SIZE); + assert!( + pass.cached_pipelines() > 0, + "the colour pass compiled something" + ); + assert!( + pass.cached_detail_pipelines() > 0, + "so did the detail stage" + ); + let allocations = pass.detail_allocations(); + assert!(allocations > 0, "and the pool holds textures"); + + pass.release_caches(); + assert_eq!(pass.cached_pipelines(), 0); + assert_eq!(pass.cached_detail_pipelines(), 0); + + // The same edit at the same size. Nothing about the picture changed, so + // nothing about the pixels may change either — only what it cost. + let after = render(&ctx, &mut pass, &graph, &source, SIZE); + assert_eq!(before.len(), after.len()); + for (i, (a, b)) in before.iter().zip(&after).enumerate() { + assert!( + a.abs_diff(*b) <= 1, + "byte {i}: {a} before eviction, {b} after — the colour chain did \ + not re-run, so this frame is reading an empty intermediate" + ); + } + assert!( + pass.detail_allocations() > allocations, + "the pool was rebuilt, which is the evidence it was really given back" + ); +} diff --git a/ui/dr-ui/src/develop.rs b/ui/dr-ui/src/develop.rs index 199dac9..be23599 100644 --- a/ui/dr-ui/src/develop.rs +++ b/ui/dr-ui/src/develop.rs @@ -2943,6 +2943,33 @@ impl DevelopSession { Ok((image, rw, rh)) } + /// TRACES: FR-PLAT-AND-5 | NFR-RES-1 + /// Give back the GPU memory this session is holding only to be fast. + /// + /// The edit is untouched: the graph and its history are CPU-side by + /// design (ARCH §6.1), so the photograph, the undo stack and the viewport + /// all survive and the next frame simply costs what the first one did. + /// + /// # What is not released, and what it is waiting on + /// + /// The demosaiced source is the largest single allocation a session holds + /// — a 24 MP frame is about 190 MB of `Rgba16Float` — and it is + /// deliberately kept. Dropping it would need the session to be able to + /// rebuild itself from the file, and rebuilding a session from a durable + /// record is FR-PLAT-AND-3, which is not built. Freeing it now would not + /// be an eviction; it would be closing the photograph without telling + /// anyone. Likewise the subject distance fields and the segmentation map: + /// each is guarded by a key recording what it was built from, and freeing + /// one without invalidating its key is the failure `AdjustPass` documents + /// under `colour_key`. + /// + /// So this is the part of the GPU tier that can be given back and asked + /// for again with no other machinery, which is exactly as far as an + /// eviction should go. + pub fn release_gpu_caches(&mut self) { + self.adjust.release_caches(); + } + /// The displayed size, for sizing the viewport. /// /// The *framed* size, not the sensor's: cropping and quarter turns change diff --git a/ui/dr-ui/src/identity_ui.rs b/ui/dr-ui/src/identity_ui.rs index 48f7cb6..32ae950 100644 --- a/ui/dr-ui/src/identity_ui.rs +++ b/ui/dr-ui/src/identity_ui.rs @@ -105,6 +105,21 @@ impl IdentityController { fn clear_picks(&self) { self.picked.borrow_mut().clear(); } + + /// TRACES: FR-PLAT-AND-5 | NFR-RES-1 + /// Drop the decoded rail portraits. + /// + /// The one in-memory image cache in this crate that is unbounded by + /// anything but the library: one decoded portrait per person, kept for as + /// long as the person exists. On a library with a few hundred named people + /// that is worth tens of megabytes of nothing but a saved decode. + /// + /// Costless to lose. `refresh` rebuilds any portrait it does not find, so + /// the only consequence is the JPEG decode this cache exists to skip, and + /// only for the people the rail is actually showing at the time. + pub fn clear_covers(&self) { + self.covers.borrow_mut().clear(); + } } /// Push the people rail and the face grid into the window. diff --git a/ui/dr-ui/src/lib.rs b/ui/dr-ui/src/lib.rs index 70fd502..d8f796e 100644 --- a/ui/dr-ui/src/lib.rs +++ b/ui/dr-ui/src/lib.rs @@ -38,6 +38,7 @@ mod library_ui; #[cfg(live_style)] mod live_style; mod masks_ui; +pub mod memory; mod net_runtime; mod presets; mod remote; @@ -1003,6 +1004,22 @@ pub fn run(paths: Vec) -> Result<()> { // faces are ticked for a split. let identity = std::rc::Rc::new(identity_ui::IdentityController::new()); + // TRACES: FR-PLAT-AND-5 + // The thumbnail tier. Registered here, beside the thing it frees, so that + // a controller which grows another cache is one line from offering it up. + // + // Weak, not strong: `run` returns when the window closes, and a registry + // holding the last reference to a controller would keep it — and every + // decoded portrait in it — alive past the interface it belonged to. + { + let identity = std::rc::Rc::downgrade(&identity); + memory::evict_at(memory::Tier::Thumbnails, move || { + if let Some(ctl) = identity.upgrade() { + ctl.clear_covers(); + } + }); + } + // Launch screen: shown when there is nothing to display — no local paths // and no configured library. A user who has already signed in and chosen // a folder goes straight to their images (FR-NC-1). @@ -1425,6 +1442,32 @@ pub fn run(paths: Vec) -> Result<()> { // The current develop session, if the file yielded sensor data. let session: Rc>> = Rc::new(RefCell::new(None)); + // TRACES: FR-PLAT-AND-5 + // The GPU tier — the first thing given back under memory pressure, and on + // Android the only thing given back merely for going into the background. + // + // `try_borrow_mut` rather than `borrow_mut`, and the miss is not an error + // worth reporting. A memory warning can land in the middle of a render, at + // which point the slot is already borrowed and freeing its textures under + // the code drawing with them is not something to do politely — skipping is + // correct, because the pass that is running will have finished by the time + // the platform asks again, and a warning that has not been acted on is + // always followed by another one. + { + let session = Rc::downgrade(&session); + memory::evict_at(memory::Tier::Gpu, move || { + let Some(session) = session.upgrade() else { + return; + }; + let Ok(mut slot) = session.try_borrow_mut() else { + return; + }; + if let Some(open) = slot.as_mut() { + open.release_gpu_caches(); + } + }); + } + // TRACES: FR-DEV-6 | FR-CAT-8 // The settings clipboard, and where the open image's edit is stored. // diff --git a/ui/dr-ui/src/memory.rs b/ui/dr-ui/src/memory.rs new file mode 100644 index 0000000..37e2a9a --- /dev/null +++ b/ui/dr-ui/src/memory.rs @@ -0,0 +1,276 @@ +//! TRACES: FR-PLAT-AND-5 | NFR-RES-1 | FR-NC-6b +//! Giving memory back when the platform asks for it. +//! +//! Android kills the process that will not shrink. It does not negotiate and +//! it does not warn twice, and the app it kills is the one holding the most — +//! which, on a photo editor, is always this one. So the question this module +//! answers is not "how much can be freed" but "in what order", because the +//! caches differ enormously in what losing them costs. +//! +//! # The order, and why it is that order +//! +//! FR-PLAT-AND-5 states it: GPU tiles first, then proxies, then thumbnails. +//! Read as a rule rather than a list, it is *cheapest to rebuild goes first* — +//! a GPU allocation is remade from data already in memory, a proxy is remade +//! from a file already on disk, and a thumbnail may cost a network fetch. +//! [`Tier`] is that order written down where the code can be held to it, so +//! adding a cache means choosing its tier rather than choosing its position in +//! a hand-maintained sequence. +//! +//! What each tier actually reaches in this build is documented on the variant, +//! including where it reaches nothing yet. An empty tier is worth keeping +//! visible: it says the order is complete and the coverage is not. +//! +//! # Why this is a registry rather than a function that frees things +//! +//! Every cache worth evicting lives behind an `Rc>` owned by a +//! local in [`crate::run`], which is a two-thousand-line function whose +//! callbacks each hold their own handle. There is no central object to reach +//! them through, and inventing one to serve eviction alone would be a large +//! change to how the interface is wired for a small change in what it does. +//! +//! So `run` hands this module a closure per cache as it builds each one, and +//! this module owns only the ordering. The registration is next to the thing +//! being registered, which is also the property that keeps it honest: a cache +//! added later is one line away from being evictable, and a cache removed +//! takes its sink with it. +//! +//! # Everything here is single-threaded, and that is not a limitation +//! +//! The registry is a `thread_local`, holding `Fn()` rather than `Fn() + Send`, +//! because the pressure signal already arrives on the thread that owns the +//! caches. Slint's Android backend calls the event listener from inside +//! `poll_events`, which runs on the same thread as the event loop, which is +//! the thread `run` built everything on. Marshalling through +//! `invoke_from_event_loop` would add a hop and a lifetime question to solve a +//! problem that does not exist — and would arrive *after* the moment the +//! system asked, which for a memory warning is the one thing that matters. +//! +//! Anything reached from a worker thread — the thumbnail store, the original +//! cache — is on disk and bounded by its own budget (NFR-RES-4), and is not +//! what a memory warning is about. + +use std::cell::RefCell; + +/// How hard the platform is asking. +/// +/// Two levels rather than Android's eight, because two is what the platform +/// actually delivers to this app. `ComponentCallbacks2.onTrimMemory` and its +/// `TRIM_MEMORY_*` grades are a Java callback on an `Activity` or +/// `Application`; a `NativeActivity` receives only `ANativeActivityCallbacks`, +/// whose memory callback is the ungraded `onLowMemory` — which is what +/// android-activity surfaces as `MainEvent::LowMemory`. Modelling grades the +/// entry point cannot observe would be modelling a wish. +/// +/// [`Self::UiHidden`] recovers the one distinction that *is* observable and is +/// worth acting on, because it is the cheapest moment to give memory back: +/// nothing is on screen, so nothing that is freed has to be drawn again before +/// the user notices. It corresponds to `TRIM_MEMORY_UI_HIDDEN` in intent and +/// is derived from the activity being stopped rather than from a memory +/// warning at all. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Level { + /// The app is no longer on screen. Free what only a visible window needs. + UiHidden, + /// The system says it is short of memory. Free everything that can be + /// rebuilt. + Critical, +} + +/// What a cache costs to lose, as an order. +/// +/// Declared in eviction order and iterated in declaration order by +/// [`Tier::ORDER`], so the sequence FR-PLAT-AND-5 specifies is a property of +/// this type rather than of each call site. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Tier { + /// GPU allocations that are rebuilt from data the process still holds. + /// + /// The develop session's compiled pipelines, its detail intermediates and + /// its output textures. Rebuilt by the next render from the demosaiced + /// source, which is still resident — see + /// [`DevelopSession::release_gpu_caches`](crate::DevelopSession::release_gpu_caches) + /// for what is deliberately kept and what that is waiting on. + /// + /// First because it is both the largest evictable pool on a mobile GPU and + /// the cheapest to refill: no I/O, no network, one frame's work. + Gpu, + /// Decoded image data rebuilt by reading a file again. + /// + /// **Nothing registers here in this build, and the tier is kept anyway.** + /// There is no in-memory proxy cache: the only decoded full-size frame in + /// the process is the open develop session's, which belongs to + /// [`Tier::Gpu`] and cannot be dropped until a session can be rebuilt from + /// a durable record (FR-PLAT-AND-3). The on-disk original cache is a + /// different thing wearing the same word — freeing disk relieves no memory + /// pressure, and it already has a budget and an LRU of its own + /// (`dr_catalog::Cache`, NFR-RES-4). + Proxies, + /// Decoded thumbnails, rebuilt by decoding a stored JPEG again — or, at + /// worst, by fetching one. + /// + /// Last because this is the tier a user sees losing: an evicted portrait + /// is a rail that redraws, and an evicted grid cell is a photograph that + /// greys out and comes back. + Thumbnails, +} + +impl Tier { + /// The eviction order, in one place. + pub const ORDER: [Tier; 3] = [Tier::Gpu, Tier::Proxies, Tier::Thumbnails]; + + /// Whether this tier is given up at this level of pressure. + /// + /// Hiding the window frees the GPU tier and nothing else. That is not + /// caution about the rest — it is that a backgrounded app has no window to + /// draw and therefore no use at all for a render pipeline, while its + /// thumbnails are exactly what the user will be looking at half a second + /// after they come back. Under [`Level::Critical`] the process is being + /// measured against being killed, and a slow return beats no return. + fn evicted_at(self, level: Level) -> bool { + match level { + Level::UiHidden => matches!(self, Tier::Gpu), + Level::Critical => true, + } + } +} + +/// A cache that has offered itself up, and the tier it goes in. +/// +/// Named because the registry is a `Vec` of these and the nested type is hard +/// to read at the use site rather than because either half means anything on +/// its own. +type Sink = (Tier, Box); + +thread_local! { + /// Registered sinks, in the order they were registered within a tier. + /// + /// Within a tier the order is registration order and nothing depends on + /// it; between tiers it is [`Tier::ORDER`], which everything depends on. + static SINKS: RefCell> = const { RefCell::new(Vec::new()) }; +} + +/// Offer a cache up for eviction at `tier`. +/// +/// Called as each cache is built, so that the registration reads next to the +/// thing it is about. The closure is kept for the life of the thread; it must +/// therefore hold weak or shared handles rather than borrow anything, which is +/// the natural shape here because everything it can reach is already an `Rc`. +pub(crate) fn evict_at(tier: Tier, sink: impl Fn() + 'static) { + SINKS.with_borrow_mut(|sinks| sinks.push((tier, Box::new(sink)))); +} + +/// TRACES: FR-PLAT-AND-5 +/// Give memory back, in [`Tier::ORDER`], as far down as `level` calls for. +/// +/// Safe to call when nothing is registered — before the window is built, or on +/// a platform that never asks — in which case it does nothing at all. +/// +/// The registry is taken out of the cell for the duration rather than borrowed +/// across the calls. A sink runs arbitrary interface code, and interface code +/// that registered another cache, or called this again, would otherwise meet a +/// `RefCell` it had already borrowed and abort the process. Freeing memory is +/// the wrong moment to be brittle about re-entry. +pub fn relieve(level: Level) { + let taken: Vec<(Tier, Box)> = SINKS.with_borrow_mut(std::mem::take); + let mut run = 0usize; + for tier in Tier::ORDER { + if !tier.evicted_at(level) { + continue; + } + for (t, sink) in &taken { + if *t == tier { + sink(); + run += 1; + } + } + } + // Put them back, keeping anything a sink registered while it ran — after, + // so the order within a tier stays registration order. + SINKS.with_borrow_mut(|sinks| { + let added = std::mem::replace(sinks, taken); + sinks.extend(added); + }); + log::info!("memory pressure ({level:?}): ran {run} eviction(s)"); +} + +#[cfg(test)] +mod tests { + use super::*; + use std::rc::Rc; + + /// Registers one sink per tier, backwards, and hands back what they saw. + fn recorder() -> Rc>> { + let seen = Rc::new(RefCell::new(Vec::new())); + for tier in [Tier::Thumbnails, Tier::Proxies, Tier::Gpu] { + let seen = seen.clone(); + evict_at(tier, move || seen.borrow_mut().push(tier)); + } + seen + } + + fn reset() { + SINKS.with_borrow_mut(|s| s.clear()); + } + + #[test] + fn eviction_runs_cheapest_to_rebuild_first() { + // Registered deliberately backwards, because the guarantee is about + // the tier and not about who registered first. A handler that simply + // ran its list would pass every other assertion here and fail this + // one — and on a device it would throw away thumbnails to keep a + // render pipeline that nothing was going to draw. + reset(); + let seen = recorder(); + relieve(Level::Critical); + assert_eq!( + *seen.borrow(), + vec![Tier::Gpu, Tier::Proxies, Tier::Thumbnails] + ); + reset(); + } + + #[test] + fn hiding_the_window_costs_only_the_gpu() { + // The cheap moment: give back what a window that is not on screen + // cannot use, and keep what the user will be looking at when they come + // back. Widening this to everything would make every task switch a + // reload of the grid. + reset(); + let seen = recorder(); + relieve(Level::UiHidden); + assert_eq!(*seen.borrow(), vec![Tier::Gpu]); + reset(); + } + + #[test] + fn pressure_before_anything_is_registered_is_not_a_failure() { + // The launch window: `android_main` installs the listener before + // `run` builds a single cache, so the first minutes of a cold start + // can deliver a warning to an empty registry. + reset(); + relieve(Level::Critical); + } + + #[test] + fn a_sink_may_register_another_without_deadlocking() { + // Guards the re-entry the take-and-restore exists for: a sink is + // interface code, and interface code that reached this module again + // would otherwise meet a borrow it already held. + reset(); + let seen = Rc::new(RefCell::new(0usize)); + { + let seen = seen.clone(); + evict_at(Tier::Gpu, move || { + *seen.borrow_mut() += 1; + evict_at(Tier::Thumbnails, || {}); + }); + } + relieve(Level::Critical); + assert_eq!(*seen.borrow(), 1); + // And the one it added survived, rather than being dropped with the + // temporary list. + assert_eq!(SINKS.with_borrow(|sinks| sinks.len()), 2); + reset(); + } +} From 75fd5619ca12c3fc9d0904d1fd4410e28a921dbf Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Sat, 29 Aug 2026 22:19:38 +0200 Subject: [PATCH 2/2] Refuse a scan whose root has gone, instead of reporting it empty FR-PLAT-AND-2, and a silent failure on both platforms. `dr_sync::scan` stepped over a NotFound or PermissionDenied the way it does for a child that vanished mid-walk -- correct for a child, wrong for the root, where it ended the walk, returned Ok with nothing in it, and reported a successful scan of a library that was no longer there. A lost root is now its own error. The images under it are marked Availability::Offline per FR-CAT-9 and no catalog row is deleted; `library::persist` clears the mark per file as each one is listed again, so a root that comes back needs no repair step. Partly satisfied rather than closed, and the gap is worth stating. The recovery half is real and reachable on Android today, because `map_status` turns Nextcloud's 403 and 404 into it and Nextcloud is how a phone actually gets a library in this build. The causes the requirement names -- revocation, reinstall, a removed card -- are properties of a persisted tree permission, and there is none: SAF does not exist here, `SourceRef::Document` is constructed only in test modules, and `LocalStorage` rejects the variant outright. When SAF lands it becomes a third producer of this error and nothing above it changes, which is why the discovery belongs in the connector. Co-Authored-By: Claude Opus 5 (1M context) --- core/dr-catalog/src/lib.rs | 2 +- core/dr-catalog/src/walk.rs | 64 ++++++++++++++- core/dr-sync-folder/src/lib.rs | 25 ++++-- core/dr-sync-folder/src/tests.rs | 17 +++- core/dr-sync/src/error.rs | 70 ++++++++++++++-- core/dr-sync/src/scan.rs | 134 +++++++++++++++++++++++++++++++ ui/dr-ui/src/library.rs | 106 ++++++++++++++++++++++-- ui/dr-ui/src/library_ui.rs | 98 +++++++++++++++++++--- 8 files changed, 478 insertions(+), 38 deletions(-) diff --git a/core/dr-catalog/src/lib.rs b/core/dr-catalog/src/lib.rs index c99c94c..41c124c 100644 --- a/core/dr-catalog/src/lib.rs +++ b/core/dr-catalog/src/lib.rs @@ -63,7 +63,7 @@ pub use query::{Query, Sort}; pub use rating::{Judgement, MAX_RATING}; pub use scan::{DirAction, DirState, EntryAction, ScanOutcome}; pub use trash::{TrashedImage, TRASH_DIR}; -pub use walk::{ensure_root, scan_root, RootKind, ScanProgress, ScanReport}; +pub use walk::{ensure_root, mark_root_offline, scan_root, RootKind, ScanProgress, ScanReport}; /// One row of the library grid. /// diff --git a/core/dr-catalog/src/walk.rs b/core/dr-catalog/src/walk.rs index 11246d3..7d935a1 100644 --- a/core/dr-catalog/src/walk.rs +++ b/core/dr-catalog/src/walk.rs @@ -432,7 +432,7 @@ fn bump_generation(conn: &Connection, root: RootId, now: i64) -> Result Result Result<(), CatalogError> { +/// +/// # Why the ETag goes with the mtime +/// +/// The three columns are the same fact told by three kinds of storage: a local +/// directory proves it is unchanged with its mtime and entry count, and a +/// remote one proves it with a propagating ETag (ARCH §6.6). Clearing two of +/// them and leaving the third would disarm the re-listing on exactly the +/// libraries this is most likely to be called for — a remote scan prunes on +/// the ETag alone, so a root that came back would be walked, found unchanged +/// at every level, pruned whole, and left with every row still marked offline +/// and nothing that would ever clear the mark. +/// +/// # Public, because losing a root is not only the local walk's business +/// +/// This began as the private end of [`scan_root`]'s root-failure branches, +/// which is the only route a library reached through [`Storage`] can take. +/// The application does not currently take that route at all: it opens +/// libraries through `dr-sync`'s connectors, so the discovery happens in a +/// crate that cannot see this one's internals, and the correct response is +/// identical (FR-PLAT-AND-2). Exported rather than reimplemented beside the +/// caller that found out — a second copy would be a second thing to remember +/// when the ETag rule below changes. +/// +/// [`Storage`]: dr_plat::Storage +pub fn mark_root_offline(conn: &Connection, root: RootId) -> Result<(), CatalogError> { let root_id = root.0 as i64; conn.execute( "UPDATE images SET availability = ?1 WHERE root_id = ?2 AND availability != ?1", rusqlite::params![availability_code(Availability::Offline), root_id], )?; conn.execute( - "UPDATE folders SET mtime = NULL, entry_count = NULL WHERE root_id = ?1", + "UPDATE folders SET mtime = NULL, entry_count = NULL, etag = NULL WHERE root_id = ?1", [root_id], )?; Ok(()) @@ -995,6 +1019,40 @@ mod tests { ); } + /// TRACES: FR-PLAT-AND-2 | FR-CAT-9 + #[test] + fn marking_a_root_offline_forgets_the_remote_validator_too() { + // The half of the marking that only a remote library can notice, and + // the reason it has to be here rather than beside the connector: a + // remote scan prunes on the propagating ETag alone (ARCH §6.6). Clear + // the local mtime and leave the ETag standing and a library that came + // back would be walked, found unchanged at every level, pruned whole, + // and left with every row still marked offline — with nothing that + // would ever clear the mark, because clearing it is something only a + // listing can do. + // + // Written directly because this module never writes an ETag; it is + // `ui/dr-ui/src/library.rs`'s scan that does, against the same table. + let lib = Library::new("etag-forgotten"); + lib.file("2026/IMG.CR3", b"raw"); + lib.scan(); + lib.conn() + .execute( + "UPDATE folders SET etag = 'e1' WHERE root_id = ?1", + [lib.root.0 as i64], + ) + .expect("etag"); + assert!(lib.count("SELECT COUNT(*) FROM folders WHERE etag IS NOT NULL") > 0); + + mark_root_offline(lib.conn(), lib.root).expect("mark"); + + assert_eq!( + lib.count("SELECT COUNT(*) FROM folders WHERE etag IS NOT NULL"), + 0, + "an unreachable library must be re-listed, not pruned as unchanged" + ); + } + #[test] fn a_root_that_comes_back_is_available_again() { // The other half: a drive plugged back in must return the library to diff --git a/core/dr-sync-folder/src/lib.rs b/core/dr-sync-folder/src/lib.rs index 69f0d22..a4df0ba 100644 --- a/core/dr-sync-folder/src/lib.rs +++ b/core/dr-sync-folder/src/lib.rs @@ -216,10 +216,17 @@ impl std::fmt::Debug for FolderBackend { impl FolderBackend { /// Open the folder at `root`. /// - /// The directory must exist now. It may stop existing later — a drive - /// unplugged, a mount dropped — and that surfaces per-operation as - /// [`RemoteError::Network`], which is what puts the app into offline mode - /// and leaves the catalog readable, exactly as a dead server does. + /// The directory must exist now, and not existing is + /// [`RemoteError::RootUnavailable`] — the library folder could not be + /// opened, which is the whole of what this knows. A drive unplugged + /// between sessions and a path typed wrongly at setup are the same + /// observation from here, and both are answered the same way: keep the + /// catalog, say which folder, and offer it again (FR-PLAT-AND-2). + /// + /// A mount dropped *during* a session surfaces per-operation as + /// [`RemoteError::Network`] instead, which is what puts the app into + /// offline mode and leaves the catalog readable, exactly as a dead server + /// does. pub fn new(root: impl Into) -> Result { Self::with_vfs(root, Arc::new(NoVfs)) } @@ -232,7 +239,15 @@ impl FolderBackend { pub fn with_vfs(root: impl Into, vfs: Arc) -> Result { let root = root.into(); if !root.is_dir() { - return Err(RemoteError::Configuration(format!( + // TRACES: FR-PLAT-AND-2 + // Not `Configuration`, which is where this lived while there was + // nothing better. The distinction that matters is not "was the + // account written wrongly" — which nothing here can know — but + // "can this library be opened", and a caller that knows the + // library was working yesterday can act on the second answer: + // mark what it holds as offline rather than deleting it, and ask + // for the folder again (FR-CAT-9). + return Err(RemoteError::RootUnavailable(format!( "{} is not a folder", root.display() ))); diff --git a/core/dr-sync-folder/src/tests.rs b/core/dr-sync-folder/src/tests.rs index 606cdf1..3ecea0b 100644 --- a/core/dr-sync-folder/src/tests.rs +++ b/core/dr-sync-folder/src/tests.rs @@ -48,13 +48,22 @@ fn names(entries: &[RemoteEntry]) -> Vec { // --- opening -------------------------------------------------------------- +/// TRACES: FR-PLAT-AND-2 #[test] -fn a_missing_folder_is_a_configuration_error_not_a_network_one() { - // It must not put the app into offline mode: nothing was unreachable, the - // account names somewhere that is not a folder. +fn a_missing_folder_is_an_unavailable_root_not_a_network_failure() { + // Still not offline mode — nothing was unreachable over a wire, and + // reporting it as a network failure would tell the user to wait for a + // connection that is working. + // + // `RootUnavailable` rather than `Configuration`, because the caller that + // has to act on this is the one whose library worked yesterday: an + // ejected card is indistinguishable from a mistyped path here, and only + // the first of those has a catalog full of ratings to protect. let err = FolderBackend::new("/definitely/not/here").unwrap_err(); - assert!(matches!(err, RemoteError::Configuration(_)), "{err:?}"); + assert!(matches!(err, RemoteError::RootUnavailable(_)), "{err:?}"); + assert!(err.indicates_lost_root()); assert!(!err.indicates_offline()); + assert!(err.to_string().contains("/definitely/not/here"), "{err}"); } // --- listing -------------------------------------------------------------- diff --git a/core/dr-sync/src/error.rs b/core/dr-sync/src/error.rs index 0f966bf..de57c94 100644 --- a/core/dr-sync/src/error.rs +++ b/core/dr-sync/src/error.rs @@ -61,14 +61,18 @@ pub enum RemoteError { /// connector for. /// /// **Not a network failure and not an auth failure**, which is why it is - /// its own variant. A folder library whose directory has been unmounted, - /// or an account naming a backend a cut-down build was not compiled with, - /// produces a request that never leaves the process — reporting either as - /// `Network` would put the app into offline mode and tell the user their - /// connection is down, and reporting them as `AuthFailed` would send them - /// to re-enter a credential that is fine. The message names what is wrong - /// with the configuration, because that is the only thing that will fix - /// it. + /// its own variant. An account naming a backend a cut-down build was not + /// compiled with, or a path that would leave the library folder, produces + /// a request that never leaves the process — reporting either as `Network` + /// would put the app into offline mode and tell the user their connection + /// is down, and reporting them as `AuthFailed` would send them to re-enter + /// a credential that is fine. The message names what is wrong with the + /// configuration, because that is the only thing that will fix it. + /// + /// A folder library whose directory is not there was once reported here + /// too, and is now [`RootUnavailable`](Self::RootUnavailable): it is not + /// something wrong with the configuration, it is the library being gone, + /// and only the second of those has a catalog to protect. #[error("account misconfigured: {0}")] Configuration(String), @@ -105,6 +109,43 @@ pub enum RemoteError { #[error("operation cancelled")] Cancelled, + + /// TRACES: FR-PLAT-AND-2 | FR-CAT-9 + /// The library root itself could not be opened. + /// + /// **The one failure that is about the library rather than about a file in + /// it**, and it is a separate variant because every other classification + /// of it is wrong in a way that costs the user something: + /// + /// - As [`NotFound`](Self::NotFound) it is indistinguishable from a folder + /// deleted between listing its parent and reaching it, which the walk + /// correctly steps over — so a whole library going away is reported as a + /// successful scan that found nothing. + /// - As [`PermissionDenied`](Self::PermissionDenied) it inherits a message + /// about Nextcloud share permissions and sidecar writes, which is + /// accurate for the case it was written for and nonsense for a tree + /// grant the user revoked in system settings. + /// - As [`Network`](Self::Network) it would claim the connection is down, + /// which is a promise that waiting will fix it. + /// + /// Today this is a Nextcloud root that answers 404 or 403 — deleted, or a + /// share withdrawn — or a folder library whose directory is not there. It + /// is also, exactly, the shape a revoked Android tree permission will have + /// when the Storage Access Framework connector FR-PLAT-AND-1 asks for + /// exists: the tree URI still stored, the permission behind it gone, every + /// read failing at the root and nowhere else. **That connector is not + /// built**, so no SAF grant can be lost yet; what this variant does is put + /// the recovery FR-PLAT-AND-2 requires in the one place all three causes + /// pass through, so the third needs no new handling above it. + /// + /// The response is the same for all of them and is the point of the + /// variant: mark what the catalog holds as offline, keep every row, and + /// say which library and why (FR-CAT-9). + /// + /// The string is the underlying failure, not a rewrite of it. What the + /// user is told is composed where the library's name is known. + #[error("the library folder could not be opened: {0}")] + RootUnavailable(String), } impl RemoteError { @@ -150,6 +191,19 @@ impl RemoteError { pub fn indicates_offline(&self) -> bool { matches!(self, RemoteError::Network(_)) } + + /// TRACES: FR-PLAT-AND-2 + /// Whether the *library* is gone, as opposed to the server or one file. + /// + /// Kept beside [`Self::indicates_offline`] because the two answer the same + /// shape of question and must not be confused. Both put the app into a + /// degraded mode that keeps working from the catalog, but they differ in + /// what the user is told and in what would end it: an offline library + /// comes back when the network does, and an unavailable root comes back + /// only when someone grants access again. + pub fn indicates_lost_root(&self) -> bool { + matches!(self, RemoteError::RootUnavailable(_)) + } } #[cfg(test)] diff --git a/core/dr-sync/src/scan.rs b/core/dr-sync/src/scan.rs index 6f723cd..6e4bad5 100644 --- a/core/dr-sync/src/scan.rs +++ b/core/dr-sync/src/scan.rs @@ -171,6 +171,37 @@ where let entries = match backend.list(&dir, None).await { Ok(e) => e, + + // TRACES: FR-PLAT-AND-2 | FR-CAT-9 + // The root is the one directory the walk may not step over, and + // `depth == 0` is the only place it can be — nothing is ever + // pushed at that depth but the root itself. + // + // Below, a directory that has gone is a directory that went away + // between its parent being listed and it being reached, and + // continuing is right. At the root the identical error means the + // *library* is gone, and continuing is catastrophic in a way that + // is completely silent: the walk ends, the scan succeeds having + // found nothing, and the app reports a healthy library with no new + // images while every path in the catalog now points nowhere. + // + // Refused rather than reclassified. Only these two causes are — + // a `Network` failure at the root is still a network failure, and + // must stay one or an unplugged network cable would present itself + // as a revoked permission and offline mode would never engage. + Err(RemoteError::NotFound(_)) if depth == 0 => { + return Err(RemoteError::RootUnavailable(format!( + "{root} is no longer there" + ))); + } + Err(RemoteError::PermissionDenied) if depth == 0 => { + // Deliberately not the variant's own message, which describes + // a Nextcloud share that refuses to *update* a sidecar. At the + // root nothing has been read at all. + return Err(RemoteError::RootUnavailable(format!( + "{root} can no longer be read" + ))); + } Err(RemoteError::NotFound(_)) => { // Deleted between listing its parent and reaching it. log::debug!("scan: {dir} vanished during the walk"); @@ -239,6 +270,20 @@ mod tests { caps: Capabilities, lists: RefCell, probes: RefCell, + /// Directories whose listing fails, and how. + /// + /// An absent directory is not enough to model this: the fake answers + /// an unknown path with an empty listing, which is exactly the shape + /// the walk must *not* confuse with a library that has gone away. + deny: HashMap, + } + + /// The two ways a real backend refuses a directory that is still named in + /// the catalog: it is not there, or it may not be read. + #[derive(Clone, Copy)] + enum Deny { + Missing, + Forbidden, } // The fake is single-threaded; tests never share it across threads. @@ -307,8 +352,15 @@ mod tests { }, lists: RefCell::new(0), probes: RefCell::new(0), + deny: HashMap::new(), } } + + /// Make one directory refuse to be listed. + fn denying(mut self, path: &str, how: Deny) -> Self { + self.deny.insert(path.to_string(), how); + self + } } #[async_trait] @@ -325,6 +377,11 @@ mod tests { _since: Option<&Validator>, ) -> Result, RemoteError> { *self.lists.borrow_mut() += 1; + match self.deny.get(dir.as_str()) { + Some(Deny::Missing) => return Err(RemoteError::NotFound(dir.to_string())), + Some(Deny::Forbidden) => return Err(RemoteError::PermissionDenied), + None => {} + } Ok(self.tree.get(dir.as_str()).cloned().unwrap_or_default()) } async fn dir_validator(&self, dir: &RemotePath) -> Result { @@ -404,6 +461,83 @@ mod tests { assert_eq!(r.progress.directories_listed, 3); } + /// TRACES: FR-PLAT-AND-2 | FR-CAT-9 + #[tokio::test] + async fn a_root_that_is_gone_is_a_failure_and_not_an_empty_library() { + // The silent one. A vanished directory below the root is stepped over, + // and before this the root was stepped over on the same terms — which + // ended the walk immediately, returned `Ok` with nothing in it, and + // let the app report a successful scan of a library that no longer + // exists. Nothing in that path is ever told the library went away, so + // nothing marks it offline and nothing tells the user. + let b = + FakeBackend::sample(ChangeDetection::PropagatingEtags).denying("Photos", Deny::Missing); + let e = scan( + &b, + &RemotePath::new("Photos"), + &FormatFilter::all(), + &HashMap::new(), + |_| {}, + ) + .await + .expect_err("a library that is not there is not a library with no photographs"); + + assert!(e.indicates_lost_root(), "got {e:?}"); + assert!(!e.indicates_offline(), "waiting will not bring this back"); + assert!(e.to_string().contains("Photos"), "names the library: {e}"); + } + + /// TRACES: FR-PLAT-AND-2 + #[tokio::test] + async fn a_root_that_may_not_be_read_reports_the_root_and_not_the_share_advice() { + // A Nextcloud share withdrawn, a directory the process may no longer + // read — and the shape a revoked Android tree grant will have when one + // can be held at all. Reported as plain `PermissionDenied` it would + // have carried that variant's message, which is several lines about a + // Nextcloud share refusing to *update* an existing sidecar: advice for + // a case where reads work, offered to a user whose reads have stopped + // entirely. + let b = FakeBackend::sample(ChangeDetection::PropagatingEtags) + .denying("Photos", Deny::Forbidden); + let e = scan( + &b, + &RemotePath::new("Photos"), + &FormatFilter::all(), + &HashMap::new(), + |_| {}, + ) + .await + .expect_err("a root that cannot be read is a failed scan"); + + assert!(e.indicates_lost_root(), "got {e:?}"); + assert!( + !e.to_string().contains("sidecar"), + "the share-permission advice does not belong here: {e}" + ); + } + + /// TRACES: FR-PLAT-AND-2 + #[tokio::test] + async fn a_folder_that_goes_away_below_the_root_is_still_stepped_over() { + // The other side of the split, and the reason the root is keyed on + // depth rather than on the error. A subfolder deleted between its + // parent being listed and it being reached is ordinary, and failing + // the scan over it would abandon every photograph beside it. + let b = FakeBackend::sample(ChangeDetection::PropagatingEtags) + .denying("Photos/2025", Deny::Missing); + let r = scan( + &b, + &RemotePath::new("Photos"), + &FormatFilter::all(), + &HashMap::new(), + |_| {}, + ) + .await + .expect("one folder going away is not the library going away"); + + assert_eq!(r.images.len(), 1, "2026 was still walked"); + } + /// A library with a trash folder holding a soft-deleted image. fn with_trash() -> FakeBackend { let mut b = FakeBackend::sample(ChangeDetection::PropagatingEtags); diff --git a/ui/dr-ui/src/library.rs b/ui/dr-ui/src/library.rs index 8bc230c..ffd7d94 100644 --- a/ui/dr-ui/src/library.rs +++ b/ui/dr-ui/src/library.rs @@ -80,7 +80,20 @@ pub enum ScanMessage { /// amount of string matching on the far side can reliably recover it. /// Without the flag a dead connection and a bad password produce the same /// banner, which sends the user to re-enter a credential that was fine. - Failed { message: String, offline: bool }, + /// + /// `lost_root` is the same idea one step further out, and it is carried + /// separately from `offline` rather than folded into it because the two + /// end differently. An offline library comes back when the network does, + /// with nothing asked of anyone; a library whose root cannot be opened + /// comes back only when someone restores access to it — a share put back + /// on the server, a drive plugged in, and in time a document tree granted + /// again once one can be (FR-PLAT-AND-2). Both show the same grid of what + /// is stored locally, and they must not offer the same explanation. + Failed { + message: String, + offline: bool, + lost_root: bool, + }, } /// One decoded thumbnail, ready for the grid. @@ -1146,6 +1159,7 @@ pub fn spawn_scan( let _ = tx.send(ScanMessage::Failed { message: e.message, offline: e.offline, + lost_root: e.lost_root, }); } }); @@ -1160,6 +1174,7 @@ pub fn spawn_scan( struct ScanFailure { message: String, offline: bool, + lost_root: bool, } impl ScanFailure { @@ -1169,6 +1184,7 @@ impl ScanFailure { Self { message: message.to_string(), offline: false, + lost_root: false, } } } @@ -1177,11 +1193,48 @@ impl From for ScanFailure { fn from(e: dr_sync::RemoteError) -> Self { Self { offline: e.indicates_offline(), + lost_root: e.indicates_lost_root(), message: e.to_string(), } } } +/// TRACES: FR-PLAT-AND-2 | FR-CAT-9 +/// Record that a library can no longer be opened, without losing it. +/// +/// Called on the worker, before the failure crosses the channel, because this +/// is where the catalog handle is — and because the marking must be durable +/// whether or not anyone is left to draw a banner. A process killed between +/// the failure and the next launch must still come back knowing what it could +/// not reach. +/// +/// Nothing is deleted. Every rating, every edit and every row stays exactly +/// where it was; what changes is that the images now say they are offline, so +/// the grid can show them as held-not-here rather than as ordinary +/// photographs whose thumbnails happen to be failing one at a time. +/// +/// A root with no row yet is the first scan of a library that has never +/// succeeded, and there is nothing to mark — the failure alone is the whole +/// story, and the launch screen is where it is told. +fn mark_library_offline(catalog: &Catalog, root: &str) { + let conn = catalog.connection(); + let root_id: Option = conn + .query_row( + "SELECT id FROM roots WHERE label = ?1 AND kind = 'remote'", + [root], + |r| r.get(0), + ) + .ok(); + let Some(root_id) = root_id else { + log::info!("library {root} has no catalog root yet; nothing to mark offline"); + return; + }; + match dr_catalog::mark_root_offline(conn, dr_types::RootId(root_id as u64)) { + Ok(()) => log::warn!("library {root} is unreachable; its images are marked offline"), + Err(e) => log::error!("could not mark {root} offline: {e}"), + } +} + fn run_scan( tx: &Sender, conn: Connection, @@ -1199,21 +1252,50 @@ fn run_scan( let rt = crate::net_runtime::build().map_err(ScanFailure::local)?; rt.block_on(async { - let backend = crate::remote::connect(&conn).map_err(ScanFailure::local)?; + // TRACES: FR-PLAT-AND-2 | FR-CAT-9 + // Classified rather than flattened to a local failure, because the + // removed-card case never gets as far as a request: the folder + // connector checks its root when it is constructed, so a library on an + // ejected card fails here and not in the walk. Reported as an ordinary + // error it left the grid showing a healthy library of images that + // could no longer be opened, one silent thumbnail failure at a time. + let backend = match crate::remote::connect(&conn) { + Ok(b) => b, + Err(e) => { + if e.indicates_lost_root() { + mark_library_offline(&catalog, &root); + } + return Err(e.into()); + } + }; // Stored folder ETags, so an unchanged subtree is skipped whole. On a // first run this is empty and the walk is complete; on every run after // it is what keeps cost proportional to what changed (ARCH §8.4). let known = load_folder_etags(&catalog, &root); - let result = dr_sync::scan(&*backend, &RemotePath::new(&root), &filter, &known, |p| { + let scanned = dr_sync::scan(&*backend, &RemotePath::new(&root), &filter, &known, |p| { let _ = tx.send(ScanMessage::Progress { directories: p.directories_listed, pruned: p.directories_pruned, images: p.images_found, }); }) - .await?; + .await; + + // TRACES: FR-PLAT-AND-2 | FR-CAT-9 + // Written before the failure is reported, not after: the banner is a + // consequence of the catalog state and not the other way round, and a + // process that dies between the two must come back knowing. + let result = match scanned { + Ok(r) => r, + Err(e) => { + if e.indicates_lost_root() { + mark_library_offline(&catalog, &root); + } + return Err(e.into()); + } + }; persist(&catalog, &root, &result).map_err(ScanFailure::local)?; @@ -1306,13 +1388,27 @@ fn persist( .ok() }); + // TRACES: FR-PLAT-AND-2 | FR-CAT-9 + // The `availability` arm is what ends an offline library, and it does + // it one photograph at a time. 3 is `Availability::Offline` and 0 is + // `MetadataOnly`, the same code this statement inserts new rows with — + // so a row that was marked offline when the root became unreachable is + // returned to exactly the state a fresh scan would have given it, and + // a row that was never marked is not touched at all. + // + // Conditional rather than a blanket reset for the same reason + // `dr_catalog::walk` restores per file rather than per root: the only + // thing that may clear "I could not reach this" is having reached it, + // and this statement runs precisely once per file the scan listed. tx.execute( "INSERT INTO images(root_id, folder_id, source_ref, format, file_size, availability, metadata_state, added_at) VALUES (?1, ?2, ?3, ?4, ?5, 0, 1, ?6) ON CONFLICT(root_id, source_ref) DO UPDATE SET file_size = excluded.file_size, - folder_id = excluded.folder_id", + folder_id = excluded.folder_id, + availability = CASE WHEN images.availability = 3 + THEN 0 ELSE images.availability END", rusqlite::params![ root_id, folder_id, diff --git a/ui/dr-ui/src/library_ui.rs b/ui/dr-ui/src/library_ui.rs index ae6844d..dfef1f9 100644 --- a/ui/dr-ui/src/library_ui.rs +++ b/ui/dr-ui/src/library_ui.rs @@ -270,6 +270,22 @@ pub struct LibraryController { /// judgement, and carrying the old one over would report a server down /// that was never contacted. reachability: RefCell, + /// TRACES: FR-PLAT-AND-2 | FR-CAT-9 + /// Why the library folder itself could not be opened, if it could not. + /// + /// Beside [`Self::reachability`] rather than inside it, because + /// `dr_sync::Reachability` models *the server*, and it is deliberately + /// unmoved by a refusal — a forbidden file must not report the network as + /// down (see its own tests). A revoked tree grant is a refusal, so folding + /// it in would either break that rule or need an exception carved through + /// it. + /// + /// Set only by a scan that failed at the root, and cleared only by one + /// that succeeded. Both states drive the same banner as being offline + /// does, because what the user can do is the same — carry on with what is + /// stored on the device — but the sentence under it is different, and so + /// is what will end it. + root_lost: RefCell>, /// TRACES: FR-NC-6a /// Drains the pin downloader. Held so a second pin replaces the timer /// rather than leaving two draining the same finished channel. @@ -372,6 +388,7 @@ impl LibraryController { sidecar_timer: RefCell::new(None), generation: std::cell::Cell::new(0), reachability: RefCell::new(dr_sync::Reachability::new()), + root_lost: RefCell::new(None), outbox_timer: RefCell::new(None), outbox_maybe_dirty: std::cell::Cell::new(true), geometry_timer: RefCell::new(None), @@ -443,10 +460,15 @@ impl LibraryController { } } - /// TRACES: FR-CAT-9 - /// Whether the app currently believes the server is unreachable. + /// TRACES: FR-CAT-9 | FR-PLAT-AND-2 + /// Whether the library cannot be reached, for either of the two reasons. + /// + /// One answer rather than two because every caller asks it for the same + /// purpose: to decide whether starting a transfer is worth attempting. + /// A revoked grant fails that question exactly as a dead network does, and + /// a sync started against it would spend its retries proving it. pub fn is_offline(&self) -> bool { - self.reachability.borrow().is_offline() + self.reachability.borrow().is_offline() || self.root_lost.borrow().is_some() } /// Whether the grid is narrowed to locally-stored originals. @@ -941,6 +963,17 @@ fn drain_scan( { log::info!("back online"); } + // TRACES: FR-PLAT-AND-2 + // And it is the only evidence that clears a lost root, + // for the same reason: the walk began by listing the + // root, so a scan that finished is a root that opened. + // The rows it marked offline are restored one at a + // time by `library::persist`, as each file is listed + // again — this only stops the banner claiming what is + // no longer true. + if ctl.root_lost.borrow_mut().take().is_some() { + log::info!("library folder is readable again"); + } refresh_offline(&w, ctl); // An incremental rescan lists almost nothing, so @@ -995,7 +1028,11 @@ fn drain_scan( stop(&ctl.scan_timer); return; } - ScanMessage::Failed { message, offline } => { + ScanMessage::Failed { + message, + offline, + lost_root, + } => { log::warn!("scan failed: {message}"); w.set_library_scanning(false); // Recorded as a failure even where it is only the @@ -1004,7 +1041,26 @@ fn drain_scan( // stopped because of it. job.fail(message.clone()); - if offline { + if lost_root { + // TRACES: FR-PLAT-AND-2 | FR-CAT-9 + // The library folder itself could not be opened — + // a share withdrawn, an unplugged drive, and in + // time a revoked document-tree grant. The worker + // has already marked every row under this root + // offline and deleted none of them; this is the + // half the user sees. + // + // Tested first because it is also true that the + // library is unreachable, and the generic answer + // would be reached first and be less useful. + *ctl.root_lost.borrow_mut() = Some(message); + refresh_offline(&w, ctl); + // Same reason as the offline arm below: without + // this a launch that began with a revoked grant + // shows an empty grid, which is the one impression + // this whole path exists to avoid. + open_catalog_for_offline(&w, ctl, &catalog_path, &coll_ctl); + } else if offline { // Not an error state. The catalog from the last // successful scan is still on disk and still // accurate for everything already indexed, so the @@ -1585,17 +1641,35 @@ fn scope_is_pinned(catalog: &Catalog, images: &[dr_types::ImageId]) -> bool { /// which is what keeps it testable without a display server. fn refresh_offline(window: &AppWindow, ctl: &Rc) { let reach = ctl.reachability.borrow(); - let offline = reach.is_offline(); + // TRACES: FR-PLAT-AND-2 + // A lost root wins over a dead network, and does so even when both are + // true — which is the ordinary case, since the scan that discovered the + // grant was gone was also the last request the app made. Reported the + // other way round the user is told to wait for a connection that is + // working, and the thing that would actually fix it is never mentioned. + let lost = ctl.root_lost.borrow(); + let offline = reach.is_offline() || lost.is_some(); window.set_library_offline(offline); - window.set_library_offline_reason(reach.reason().unwrap_or_default().into()); + window.set_library_offline_reason(match lost.as_deref() { + Some(why) => why.into(), + None => reach.reason().unwrap_or_default().into(), + }); window.set_library_offline_since( - reach - .offline_for(std::time::Instant::now()) - .map(describe_duration) - .unwrap_or_default() - .into(), + // A duration is what a network outage has and a revoked permission + // does not: "for 4 minutes" invites waiting, and waiting is precisely + // what will not help here. + if lost.is_some() { + slint::SharedString::default() + } else { + reach + .offline_for(std::time::Instant::now()) + .map(describe_duration) + .unwrap_or_default() + .into() + }, ); + drop(lost); // A stale scan error under an offline banner reports one problem twice. if offline {