From 8ba9b1780fe44edf6de716a44e2e0e88ef8f144b Mon Sep 17 00:00:00 2001 From: AlanRockefeller Date: Thu, 16 Apr 2026 22:21:46 -0500 Subject: [PATCH 1/2] Optimize bulk metadata refresh and isolate per-image sidecar failures --- faststack/app.py | 26 +++++--- faststack/io/sidecar.py | 45 ++++++++++--- faststack/tests/test_bulk_metadata_map.py | 77 +++++++++++++++++++++++ faststack/thumbnail_view/model.py | 16 +++-- next.prompt | 16 +++++ 5 files changed, 160 insertions(+), 20 deletions(-) create mode 100644 faststack/tests/test_bulk_metadata_map.py create mode 100644 next.prompt diff --git a/faststack/app.py b/faststack/app.py index 127d7a3..e57bbd0 100644 --- a/faststack/app.py +++ b/faststack/app.py @@ -2608,18 +2608,24 @@ def _get_metadata_dict(self, image_path: Path | str) -> dict: } def _get_bulk_metadata_map(self, images=None) -> Dict[str, dict]: - """Get flattened metadata map for the given images (defaults to self.image_files). + """Get flattened metadata map for the given images, keyed by str(img.path). - Used to avoid per-image sidecar lookups on the UI thread during grid refresh. + Used to avoid per-image sidecar lookups on the UI thread during grid + refresh. Keying by the raw path string lets downstream consumers + look up entries directly without re-resolving the stable sidecar key. + + The map is treated as authoritative by the grid (see + ``ThumbnailModel.refresh_from_controller``), so per-image errors must + be isolated — a single bad path must not collapse flags for the + whole folder. """ bulk_map = {} - try: - for img in (images if images is not None else self.image_files): - key = self.sidecar.metadata_key_for_path(img.path) + for img in (images if images is not None else self.image_files): + try: meta = self.sidecar.get_metadata(img.path, create=False) if meta is None: continue - bulk_map[key] = { + bulk_map[str(img.path)] = { "stacked": getattr(meta, "stacked", False), "uploaded": getattr(meta, "uploaded", False), "edited": getattr(meta, "edited", False), @@ -2627,8 +2633,12 @@ def _get_bulk_metadata_map(self, images=None) -> Dict[str, dict]: "favorite": getattr(meta, "favorite", False), "todo": getattr(meta, "todo", False), } - except Exception as e: - log.warning("Failed to build bulk metadata map: %s", e) + except Exception as e: + log.warning( + "Failed to read metadata for %s during bulk build: %s", + img.path, + e, + ) return bulk_map def _invalidate_batch_cache(self): diff --git a/faststack/io/sidecar.py b/faststack/io/sidecar.py index cb2fb92..eae64cd 100644 --- a/faststack/io/sidecar.py +++ b/faststack/io/sidecar.py @@ -46,6 +46,17 @@ def __init__(self, directory: Path, watcher, debug: bool = False): self.path = directory / "faststack.json" self.watcher = watcher self.debug = debug + # Precomputed once: the case-normalized absolute base dir used by + # metadata_key_for_path / _metadata_filename_key on every call. + self._base_dir_normcased = Path( + os.path.normcase(os.path.abspath(str(directory))) + ) + # Bounded per-instance caches: input str → resolved key. Folder + # refresh resolves the same paths repeatedly across bulk-map build, + # flag filter, and grid entry construction. + self._stable_key_cache: dict[str, str] = {} + self._filename_key_cache: dict[str, str] = {} + self._key_cache_max = 8192 self.data = self.load() def stop_watcher(self): @@ -187,21 +198,30 @@ def get_metadata( def metadata_key_for_path(self, image_path: Union[str, Path]) -> str: """Return the stable sidecar key for a concrete image path.""" + cache_key = str(image_path) + cached = self._stable_key_cache.get(cache_key) + if cached is not None: + return cached + path = Path(image_path) if not path.name: return "" if not path.is_absolute(): path = self.directory / path - base_dir = Path(os.path.normcase(os.path.abspath(str(self.directory)))) abs_path = Path(os.path.normcase(os.path.abspath(str(path)))) try: - relative = abs_path.relative_to(base_dir) + relative = abs_path.relative_to(self._base_dir_normcased) stable_path = relative.parent / relative.stem - return str(stable_path).replace("\\", "/") + result = str(stable_path).replace("\\", "/") except ValueError: stable_path = abs_path.parent / abs_path.stem - return str(stable_path).replace("\\", "/") + result = str(stable_path).replace("\\", "/") + + if len(self._stable_key_cache) >= self._key_cache_max: + del self._stable_key_cache[next(iter(self._stable_key_cache))] + self._stable_key_cache[cache_key] = result + return result def _lookup_keys(self, image_ref: Union[str, Path]) -> tuple[str, list[str]]: """Return (stable_key, migration_candidate_keys) for a metadata lookup.""" @@ -230,19 +250,28 @@ def _lookup_keys(self, image_ref: Union[str, Path]) -> tuple[str, list[str]]: def _metadata_filename_key(self, image_path: Union[str, Path]) -> str: """Return the extension-preserving key used by the regressed patch.""" + cache_key = str(image_path) + cached = self._filename_key_cache.get(cache_key) + if cached is not None: + return cached + path = Path(image_path) if not path.name: return "" if not path.is_absolute(): path = self.directory / path - base_dir = Path(os.path.normcase(os.path.abspath(str(self.directory)))) abs_path = Path(os.path.normcase(os.path.abspath(str(path)))) try: - relative = abs_path.relative_to(base_dir) - return str(relative).replace("\\", "/") + relative = abs_path.relative_to(self._base_dir_normcased) + result = str(relative).replace("\\", "/") except ValueError: - return str(abs_path).replace("\\", "/") + result = str(abs_path).replace("\\", "/") + + if len(self._filename_key_cache) >= self._key_cache_max: + del self._filename_key_cache[next(iter(self._filename_key_cache))] + self._filename_key_cache[cache_key] = result + return result def _stable_key_from_key(self, key: str, check_fs: bool = False) -> str: """Convert any historical sidecar key form into today's stable key. diff --git a/faststack/tests/test_bulk_metadata_map.py b/faststack/tests/test_bulk_metadata_map.py new file mode 100644 index 0000000..0f1bbb0 --- /dev/null +++ b/faststack/tests/test_bulk_metadata_map.py @@ -0,0 +1,77 @@ +"""Tests for AppController._get_bulk_metadata_map error isolation. + +The grid (ThumbnailModel.refresh_from_controller) treats a non-None metadata +map as authoritative — it does not fall back to per-image sidecar lookups on +miss. So per-image errors during bulk build must be isolated; one bad path +must not silently force every grid flag to false for the whole folder. +""" + +from faststack.models import EntryMetadata, ImageFile + + +def _make_image(tmp_path, name): + p = tmp_path / name + p.write_bytes(b"\xff\xd8") + return ImageFile(path=p, timestamp=1000.0) + + +def test_per_image_error_does_not_invalidate_other_entries(app_controller, tmp_path): + """One sidecar.get_metadata raise must not drop other images from the map.""" + a = _make_image(tmp_path, "a.jpg") + bad = _make_image(tmp_path, "b.jpg") + c = _make_image(tmp_path, "c.jpg") + app_controller.image_files = [a, bad, c] + + def fake_get_metadata(path, *, create): + if path == bad.path: + raise OSError("simulated sidecar failure for one image") + return EntryMetadata(favorite=True, uploaded=True) + + app_controller.sidecar.get_metadata.side_effect = fake_get_metadata + + bulk_map = app_controller._get_bulk_metadata_map() + + assert str(a.path) in bulk_map + assert str(c.path) in bulk_map + assert str(bad.path) not in bulk_map + assert bulk_map[str(a.path)]["favorite"] is True + assert bulk_map[str(a.path)]["uploaded"] is True + assert bulk_map[str(c.path)]["favorite"] is True + + +def test_all_images_raise_yields_empty_map(app_controller, tmp_path): + """Documents the contract: when every lookup fails, the map is empty. + + The grid treats this empty map as authoritative — the resulting all-false + flags are intentional, not a silent regression. Any future change to the + error-path contract should update this test. + """ + a = _make_image(tmp_path, "a.jpg") + b = _make_image(tmp_path, "b.jpg") + app_controller.image_files = [a, b] + + app_controller.sidecar.get_metadata.side_effect = OSError("everything is broken") + + bulk_map = app_controller._get_bulk_metadata_map() + + assert bulk_map == {} + + +def test_first_image_raises_subsequent_succeed(app_controller, tmp_path): + """Earlier exceptions must not skip later images (regression for outer try).""" + bad = _make_image(tmp_path, "a.jpg") + good = _make_image(tmp_path, "b.jpg") + app_controller.image_files = [bad, good] + + def fake_get_metadata(path, *, create): + if path == bad.path: + raise RuntimeError("first one fails") + return EntryMetadata(stacked=True) + + app_controller.sidecar.get_metadata.side_effect = fake_get_metadata + + bulk_map = app_controller._get_bulk_metadata_map() + + assert str(bad.path) not in bulk_map + assert str(good.path) in bulk_map + assert bulk_map[str(good.path)]["stacked"] is True diff --git a/faststack/thumbnail_view/model.py b/faststack/thumbnail_view/model.py index 6096d71..787619b 100644 --- a/faststack/thumbnail_view/model.py +++ b/faststack/thumbnail_view/model.py @@ -572,10 +572,15 @@ def refresh_from_controller( if self._active_filter_flags: flags = self._active_filter_flags filtered = [] + # When metadata_map is provided (even empty) treat it as + # authoritative — don't fall through to per-image sidecar + # lookups, and key lookups by str(img.path) so we don't + # re-resolve a stable key the bulk-map builder already paid for. + map_authoritative = metadata_map is not None for img in images: try: - if metadata_map and self._metadata_key_fn: - meta = metadata_map.get(self._metadata_key_fn(img.path), {}) + if map_authoritative: + meta = metadata_map.get(str(img.path), {}) elif self._get_metadata: meta = self._get_metadata(img.path) else: @@ -618,6 +623,9 @@ def _add_images_to_entries( self, images: List, metadata_map: Optional[Dict[str, dict]] = None ): """Convert list of objects (ImageFile or similar) to ThumbnailEntry.""" + # See refresh_from_controller: a non-None metadata_map is authoritative + # and keyed by str(img.path). + map_authoritative = metadata_map is not None for img in images: try: # Use mtime from object if available to avoid stat() @@ -636,8 +644,8 @@ def _add_images_to_entries( is_favorite = False is_todo = False - if metadata_map and self._metadata_key_fn: - meta = metadata_map.get(self._metadata_key_fn(img.path), {}) + if map_authoritative: + meta = metadata_map.get(str(img.path), {}) is_stacked = meta.get("stacked", False) is_uploaded = meta.get("uploaded", False) is_edited = meta.get("edited", False) diff --git a/next.prompt b/next.prompt new file mode 100644 index 0000000..05d5f15 --- /dev/null +++ b/next.prompt @@ -0,0 +1,16 @@ +In sidecar.py: cache concrete path resolution and add a stable-key alias index so legacy RAW/filename sidecar keys resolve in O(1) instead of falling back to the O(n) scan. +In app.py (line 2610) and model.py (line 530): build the bulk metadata map keyed by img.path, treat an empty map as authoritative, and only fall back to metadata_key_fn on miss so grid refresh doesn’t re-resolve the same stable key. + +background: + +The reason is that stable-key resolution is on real whole-folder paths today, not just edge cases. get_metadata() can fall back to an O(n) migration scan on misses in sidecar.py (line 152), and the app calls this in folder-wide loops for filtering, bulk metadata build, and batch helpers in app.py (line 830), app.py (line 2617), app.py (line 3114), and app.py (line 3158). The grid path in model.py (line 566) can also resolve the same key again after the bulk map is built. + +I spot-checked it with a local micro-benchmark in the repo venv. The current bulk-map + flag-filter pattern was about 0.30s for 1k stable images and 1.48s for 5k. A first pass over 5k legacy/raw-keyed entries was about 3.25s. So this is big enough to matter on large folders. + +If we do it, I’d prioritize it in this order: + +Stop resolving the same stable key multiple times per image in the refresh/filter path. +Add a small per-SidecarManager cache for metadata_key_for_path(). +Only after that, consider eager one-time legacy-key normalization on load if older sidecars are still common. + + From 2a2eb22f8ff9e4c31dd6dd471a72e7b4049aa1de Mon Sep 17 00:00:00 2001 From: AlanRockefeller Date: Thu, 16 Apr 2026 22:22:12 -0500 Subject: [PATCH 2/2] rm temp file --- next.prompt | 16 ---------------- 1 file changed, 16 deletions(-) delete mode 100644 next.prompt diff --git a/next.prompt b/next.prompt deleted file mode 100644 index 05d5f15..0000000 --- a/next.prompt +++ /dev/null @@ -1,16 +0,0 @@ -In sidecar.py: cache concrete path resolution and add a stable-key alias index so legacy RAW/filename sidecar keys resolve in O(1) instead of falling back to the O(n) scan. -In app.py (line 2610) and model.py (line 530): build the bulk metadata map keyed by img.path, treat an empty map as authoritative, and only fall back to metadata_key_fn on miss so grid refresh doesn’t re-resolve the same stable key. - -background: - -The reason is that stable-key resolution is on real whole-folder paths today, not just edge cases. get_metadata() can fall back to an O(n) migration scan on misses in sidecar.py (line 152), and the app calls this in folder-wide loops for filtering, bulk metadata build, and batch helpers in app.py (line 830), app.py (line 2617), app.py (line 3114), and app.py (line 3158). The grid path in model.py (line 566) can also resolve the same key again after the bulk map is built. - -I spot-checked it with a local micro-benchmark in the repo venv. The current bulk-map + flag-filter pattern was about 0.30s for 1k stable images and 1.48s for 5k. A first pass over 5k legacy/raw-keyed entries was about 3.25s. So this is big enough to matter on large folders. - -If we do it, I’d prioritize it in this order: - -Stop resolving the same stable key multiple times per image in the refresh/filter path. -Add a small per-SidecarManager cache for metadata_key_for_path(). -Only after that, consider eager one-time legacy-key normalization on load if older sidecars are still common. - -